[libcamera-devel] [PATCH v2 04/14] libcamera: pipeline: Add method to prepare internal buffers

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Fri, Aug 30, 2019 at 01:26:43AM +0200, Niklas Söderlund wrote:
> Buffers internal to a pipeline handler (buffers cycled between a CSI-2
> pipeline to a ISP pipeline, parameters and statistics buffers) needs to
> be prepared before they can be properly used inside a pipeline handler.
> 
> At this point the preparation consists of mapping the Buffer objects
> memory and associating it with a request. Instead of adding this helper
> on the Buffer object itself add it to the pipeline handler base class to
> prevent it from being exposed to applications.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> ---
>  src/libcamera/include/pipeline_handler.h |  2 ++
>  src/libcamera/pipeline_handler.cpp       | 18 ++++++++++++++++++
>  2 files changed, 20 insertions(+)
> 
> diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h
> index ffc7adb802215313..91d40ef40a465c4e 100644
> --- a/src/libcamera/include/pipeline_handler.h
> +++ b/src/libcamera/include/pipeline_handler.h
> @@ -98,6 +98,8 @@ protected:
>  
>  	CameraData *cameraData(const Camera *camera);
>  
> +	void prepareInternalBuffer(Buffer *buffer, Request *request,
> +				   BufferMemory *mem);
>  	CameraManager *manager_;
>  
>  private:
> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> index 558b4b254d111e31..846272485c7d2fc0 100644
> --- a/src/libcamera/pipeline_handler.cpp
> +++ b/src/libcamera/pipeline_handler.cpp
> @@ -485,6 +485,24 @@ void PipelineHandler::hotplugMediaDevice(MediaDevice *media)
>  	media->disconnected.connect(this, &PipelineHandler::mediaDeviceDisconnected);
>  }
>  
> +/**
> + * \brief Prepare buffer for internal usage by a pipeline handler
> + * \param[in,out] buffer The buffer to prepare
> + * \param[in] request The request to associate the \a buffer with
> + * \param[in] mem The memory to associate the \a buffer with
> + *
> + * Pipeline handlers creating internal buffers to facilitate data flow in the
> + * pipeline need to prepare the buffers by setting up the buffer object state.
> + * This function help pipeline handler implementations to perform this
> + * preparation.
> + */

I'm afraid I don't like this much. First of all, reading the
documentation, I have no idea how/when this is supposed to be used, so
we need to improve that. Then, looking at the rest of the series, I see
this method only used for the rkisp1 stats and params buffers. We have
other internal buffers, such as between the CIO2 and ImgU, or ImgU
params or stats buffers. We should use this method consistently, and I
would like to see pipeline handlers updated to use it as part of this
patch, to show how it is supposed to be used (although if the
documentation was clearer maybe I wouldn't feel this need).

Finally, as mentioned in a review of v1, this belongs to the Buffer
class. I understand you don't want to expose this to applications, but
the method really doesn't belong to the PipelineHandler class. At the
very least it should be a global helper method. I think this however
calls for a rework of the buffer API.

> +void PipelineHandler::prepareInternalBuffer(Buffer *buffer, Request *request,
> +					    BufferMemory *mem)
> +{
> +	buffer->request_ = request;
> +	buffer->mem_ = mem;
> +}
> +
>  /**
>   * \brief Slot for the MediaDevice disconnected signal
>   */

-- 
Regards,

Laurent Pinchart