[libcamera-devel] [PATCH v2 18/25] libcamera: pipelines: Add FrameBuffer handlers
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Jan 8 04:00:10 CET 2020
Hi Niklas,
Thank you for the patch.
In the subject line, s/pipelines/pipeline/
On Mon, Dec 30, 2019 at 01:05:03PM +0100, Niklas Söderlund wrote:
> Extend the pipeline handlers to support the FrameBuffer API with three
> new methods to handle allocation, importing and freeing of buffers. The
> new methods will replace allocateBuffers() and freeBuffers().
>
> The FrameBuffer API will use the methods on a stream level and either
> allocate or import buffers for each active stream controlled from the
> Camera class and an upcoming FrameBufferAllocator helper. With this new
> API the implementation in pipeline handlers can be made simpler as all
> streams don't need to be handled in allocateBuffers().
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> ---
> src/libcamera/include/pipeline_handler.h | 7 +++
> src/libcamera/pipeline/ipu3/ipu3.cpp | 34 +++++++++++++
> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 25 ++++++++++
> src/libcamera/pipeline/uvcvideo.cpp | 31 ++++++++++++
> src/libcamera/pipeline/vimc.cpp | 31 ++++++++++++
> src/libcamera/pipeline_handler.cpp | 62 ++++++++++++++++++++++++
> 6 files changed, 190 insertions(+)
>
> diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h
> index f3622631022d87b9..520d3fccaaa130cc 100644
> --- a/src/libcamera/include/pipeline_handler.h
> +++ b/src/libcamera/include/pipeline_handler.h
> @@ -69,6 +69,13 @@ public:
> const StreamRoles &roles) = 0;
> virtual int configure(Camera *camera, CameraConfiguration *config) = 0;
>
> + virtual int allocateFrameBuffers(Camera *camera, Stream *stream,
> + unsigned int count,
> + std::vector<std::unique_ptr<FrameBuffer>> *buffers) = 0;
> + virtual int importFrameBuffers(Camera *camera, Stream *stream,
> + unsigned int count) = 0;
allocate vs. import sounds a bit strange. Should we s/allocate/export/ ?
> + virtual void freeFrameBuffers(Camera *camera, Stream *stream) = 0;
> +
> virtual int allocateBuffers(Camera *camera,
> const std::set<Stream *> &streams) = 0;
> virtual int freeBuffers(Camera *camera,
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index 030ba2b6a0df2e0b..f9bddcc88523301f 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -210,6 +210,13 @@ public:
> const StreamRoles &roles) override;
> int configure(Camera *camera, CameraConfiguration *config) override;
>
> + int allocateFrameBuffers(Camera *camera, Stream *stream,
> + unsigned int count,
> + std::vector<std::unique_ptr<FrameBuffer>> *buffers) override;
> + int importFrameBuffers(Camera *camera, Stream *stream,
> + unsigned int count) override;
> + void freeFrameBuffers(Camera *camera, Stream *stream) override;
> +
> int allocateBuffers(Camera *camera,
> const std::set<Stream *> &streams) override;
> int freeBuffers(Camera *camera,
> @@ -616,6 +623,33 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)
> return 0;
> }
>
> +int PipelineHandlerIPU3::allocateFrameBuffers(Camera *camera, Stream *stream,
> + unsigned int count,
> + std::vector<std::unique_ptr<FrameBuffer>> *buffers)
> +{
> + IPU3Stream *ipu3stream = static_cast<IPU3Stream *>(stream);
> + V4L2VideoDevice *video = ipu3stream->device_->dev;
> +
> + return video->exportBuffers(count, buffers);
> +}
> +
> +int PipelineHandlerIPU3::importFrameBuffers(Camera *camera, Stream *stream,
> + unsigned int count)
> +{
> + IPU3Stream *ipu3stream = static_cast<IPU3Stream *>(stream);
> + V4L2VideoDevice *video = ipu3stream->device_->dev;
> +
> + return video->importBuffers(count);
> +}
> +
> +void PipelineHandlerIPU3::freeFrameBuffers(Camera *camera, Stream *stream)
> +{
> + IPU3Stream *ipu3stream = static_cast<IPU3Stream *>(stream);
> + V4L2VideoDevice *video = ipu3stream->device_->dev;
> +
> + video->releaseBuffers();
> +}
> +
> /**
> * \todo Clarify if 'viewfinder' and 'stat' nodes have to be set up and
> * started even if not in use. As of now, if not properly configured and
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index ea581aaaa85088cd..e77925f6f9deff08 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -175,6 +175,13 @@ public:
> const StreamRoles &roles) override;
> int configure(Camera *camera, CameraConfiguration *config) override;
>
> + int allocateFrameBuffers(Camera *camera, Stream *stream,
> + unsigned int count,
> + std::vector<std::unique_ptr<FrameBuffer>> *buffers) override;
> + int importFrameBuffers(Camera *camera, Stream *stream,
> + unsigned int count) override;
> + void freeFrameBuffers(Camera *camera, Stream *stream) override;
> +
> int allocateBuffers(Camera *camera,
> const std::set<Stream *> &streams) override;
> int freeBuffers(Camera *camera,
> @@ -666,6 +673,24 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)
> return 0;
> }
>
> +int PipelineHandlerRkISP1::allocateFrameBuffers(Camera *camera, Stream *stream,
> + unsigned int count,
> + std::vector<std::unique_ptr<FrameBuffer>> *buffers)
> +{
> + return video_->exportBuffers(count, buffers);
> +}
> +
> +int PipelineHandlerRkISP1::importFrameBuffers(Camera *camera, Stream *stream,
> + unsigned int count)
> +{
> + return video_->importBuffers(count);
> +}
> +
> +void PipelineHandlerRkISP1::freeFrameBuffers(Camera *camera, Stream *stream)
> +{
> + video_->releaseBuffers();
> +}
> +
> int PipelineHandlerRkISP1::allocateBuffers(Camera *camera,
> const std::set<Stream *> &streams)
> {
> diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp
> index 3043366bee38bcfc..655c5e6d96a696d6 100644
> --- a/src/libcamera/pipeline/uvcvideo.cpp
> +++ b/src/libcamera/pipeline/uvcvideo.cpp
> @@ -64,6 +64,13 @@ public:
> const StreamRoles &roles) override;
> int configure(Camera *camera, CameraConfiguration *config) override;
>
> + int allocateFrameBuffers(Camera *camera, Stream *stream,
> + unsigned int count,
> + std::vector<std::unique_ptr<FrameBuffer>> *buffers) override;
> + int importFrameBuffers(Camera *camera, Stream *stream,
> + unsigned int count) override;
> + void freeFrameBuffers(Camera *camera, Stream *stream) override;
> +
> int allocateBuffers(Camera *camera,
> const std::set<Stream *> &streams) override;
> int freeBuffers(Camera *camera,
> @@ -192,6 +199,30 @@ int PipelineHandlerUVC::configure(Camera *camera, CameraConfiguration *config)
> return 0;
> }
>
> +int PipelineHandlerUVC::allocateFrameBuffers(Camera *camera, Stream *stream,
> + unsigned int count,
> + std::vector<std::unique_ptr<FrameBuffer>> *buffers)
> +{
> + UVCCameraData *data = cameraData(camera);
> +
> + return data->video_->exportBuffers(count, buffers);
> +}
> +
> +int PipelineHandlerUVC::importFrameBuffers(Camera *camera, Stream *stream,
> + unsigned int count)
> +{
> + UVCCameraData *data = cameraData(camera);
> +
> + return data->video_->importBuffers(count);
> +}
> +
> +void PipelineHandlerUVC::freeFrameBuffers(Camera *camera, Stream *stream)
> +{
> + UVCCameraData *data = cameraData(camera);
> +
> + data->video_->releaseBuffers();
> +}
> +
> int PipelineHandlerUVC::allocateBuffers(Camera *camera,
> const std::set<Stream *> &streams)
> {
> diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp
> index 292900bcf8d1b359..02235ffb0515982f 100644
> --- a/src/libcamera/pipeline/vimc.cpp
> +++ b/src/libcamera/pipeline/vimc.cpp
> @@ -82,6 +82,13 @@ public:
> const StreamRoles &roles) override;
> int configure(Camera *camera, CameraConfiguration *config) override;
>
> + int allocateFrameBuffers(Camera *camera, Stream *stream,
> + unsigned int count,
> + std::vector<std::unique_ptr<FrameBuffer>> *buffers) override;
> + int importFrameBuffers(Camera *camera, Stream *stream,
> + unsigned int count) override;
> + void freeFrameBuffers(Camera *camera, Stream *stream) override;
> +
> int allocateBuffers(Camera *camera,
> const std::set<Stream *> &streams) override;
> int freeBuffers(Camera *camera,
> @@ -259,6 +266,30 @@ int PipelineHandlerVimc::configure(Camera *camera, CameraConfiguration *config)
> return 0;
> }
>
> +int PipelineHandlerVimc::allocateFrameBuffers(Camera *camera, Stream *stream,
> + unsigned int count,
> + std::vector<std::unique_ptr<FrameBuffer>> *buffers)
> +{
> + VimcCameraData *data = cameraData(camera);
> +
> + return data->video_->exportBuffers(count, buffers);
> +}
> +
> +int PipelineHandlerVimc::importFrameBuffers(Camera *camera, Stream *stream,
> + unsigned int count)
> +{
> + VimcCameraData *data = cameraData(camera);
> +
> + return data->video_->importBuffers(count);
> +}
> +
> +void PipelineHandlerVimc::freeFrameBuffers(Camera *camera, Stream *stream)
> +{
> + VimcCameraData *data = cameraData(camera);
> +
> + data->video_->releaseBuffers();
> +}
> +
> int PipelineHandlerVimc::allocateBuffers(Camera *camera,
> const std::set<Stream *> &streams)
> {
> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> index 5badf31ce793edf8..fb69ca8e97216e6c 100644
> --- a/src/libcamera/pipeline_handler.cpp
> +++ b/src/libcamera/pipeline_handler.cpp
> @@ -287,6 +287,68 @@ const ControlInfoMap &PipelineHandler::controls(Camera *camera)
> * \return 0 on success or a negative error code otherwise
> */
>
> +/**
> + * \fn PipelineHandler::allocateFrameBuffers()
> + * \brief Allocate buffers for \a stream
> + * \param[in] camera The camera to allocate buffers on
I think "The camera" would be enough. Same for the methods below.
> + * \param[in] stream The stream to allocate buffers for
> + * \param[in] count Number of buffers to allocate
> + * \param[out] buffers Array of buffers successfully allocated
> + *
> + * Allocate buffers for \a stream using the resources associated with the stream
> + * inside the pipelinehandler.
s/pipelinehandler/pipeline handler/
> + *
> + * The method could be called multiple times to operate on different streams.
> + * It is not allowed to call allocateFrameBuffers() and importFrameBuffers()
> + * on the same \a stream as they are mutually exclusive operations. The
> + * pipelinehandler must guarantee that all streams exposed as part of the Camera
> + * can be used simultaneously with all combinations of the two.
* This method allocates buffers for the \a stream from the devices associated
* with the stream in the corresponding pipeline handler. Those buffers shall be
* suitable to be added to a Request for the stream, and shall be mappable to
* the CPU through their associated dmabufs with mmap().
This tells the pipeline handler authors what they need to return.
*
* The method may only be called after the Camera has been configured and before
* it gets started, or after it gets stopped. It shall be called only for
* streams that are part of the active camera configuration, and at most once
* per stream until buffers for the stream are freed with freeFrameBuffers().
This tells the pipeline handler authors when they shall expect the
method to be called.
*
* allocateFrameBuffers() shall also allocate all other resources required by
* the pipeline handler for the stream to prepare for starting the Camera. This
* responsibility is shared with importFrameBuffers(), and one and only one of
* those two methods shall be called for each stream until the buffers are
* freed. The pipeline handler shall support all combinations of
* allocateFrameBuffers() and importFrameBuffers() for the streams contained in
* any camera configuration.
> + *
> + * This is a helper which may be used by libcamera helper classes to allocate
> + * buffers from the stream itself. The allocated buffers may then be treated
> + * in the same way as if they where externally allocated.
I would drop this paragraph.
> + *
> + * The only intended caller is the FrameBufferAllocator helper.
> + *
> + * \return 0 on success or a negative error code otherwise
> + */
> +
> +/**
> + * \fn PipelineHandler::importFrameBuffers()
> + * \brief Prepare \a stream to use external buffers
> + * \param[in] camera The camera to prepare for import on
> + * \param[in] stream The stream to prepare for import
> + * \param[in] count Number of buffers to import
> + *
> + * Prepare the \a stream to use \a count number of buffers allocated outside
> + * the pipelinehandler.
> + *
> + * The method could be called multiple times to operate on different streams.
> + * It is not allowed to call allocateFrameBuffers() and importFrameBuffers()
> + * on the same \a stream as they are mutually exclusive operations. The
> + * pipelinehandler must guarantee that all streams exposed as part of the Camera
> + * can be used simultaneously with all combinations of the two.
> + *
* This method prepares the pipeline handler to use \a count buffers provided by
* the application for the \a stream.
*
* The method may only be called after the Camera has been configured and before
* it gets started, or after it gets stopped. It shall be called only for
* streams that are part of the active camera configuration, and at most once
* per stream until buffers for the stream are freed with freeFrameBuffers().
*
* importFrameBuffers() shall also allocate all other resources required by the
* pipeline handler for the stream to prepare for starting the Camera. This
* responsibility is shared with allocateFrameBuffers(), and one and only one of
* those two methods shall be called for each stream until the buffers are
* freed. The pipeline handler shall support all combinations of
* allocateFrameBuffers() and importFrameBuffers() for the streams contained in
* any camera configuration.
I'm still not 100% sure this API is the best, I wonder if we shouldn't
have an optional allocateFrameBuffers() and a prepare() that is called
unconditionally for all streams, with a parameter telling whether
allocateFrameBuffers() has been called. This could be done on top of
this series, we need to evaluate the impact on pipeline handlers, and
it's likely easier to do so on top.
> + * This is a helper which may be used by libcamera helper classes to import
> + * buffers to the \a stream from external sources.
I would drop this paragraph.
> + * The only intended caller is the FrameBufferAllocator helper.
That's not true, the only intended caller is the Camera::start() method.
> + *
> + * \return 0 on success or a negative error code otherwise
> + */
> +
> +/**
> + * \fn PipelineHandler::freeFrameBuffers()
> + * \brief Free buffers allocated from the stram
s/stram/stream/
> + * \param[in] camera The camera to free buffers on
> + * \param[in] stream The stream to free buffers for
> + *
> + * This is a helper that frees buffers and resources allocated using
> + * allocateFrameBuffers() or importFrameBuffers().
* This method shall free all buffers and all other resources allocated for the
* \a stream by allocateFrameBuffers() or importFrameBuffers(). It shall be
* called only after a successful call to either of these two methods, and only
* once per stream.
> + *
> + * The only intended caller is the FrameBufferAllocator helper.
This isn't correct, the method is called by Camera::stop() too.
Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> + */
> +
> /**
> * \fn PipelineHandler::allocateBuffers()
> * \brief Allocate buffers for a stream
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list