[libcamera-devel] [PATCH 03/13] libcamera: ipu3: Always import buffers for ImgU sinks

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Sat, Jun 27, 2020 at 05:00:33AM +0200, Niklas Söderlund wrote:
> When the IPU3 pipeline was first developed buffers of sinks from the
> ImgU that where not active still needed to have allocated buffers
> associated with them or streaming was not allowed to start. This is no
> longer true, it's enough that the sinks have imported buffers for
> streaming to start. As we already need to import buffers for streams
> that are active we can align the two cases and always import buffers.
> 
> With this there is no longer a reason to store the allocated
> FrameBuffers to keep them alive and the vector tracking them can be
> removed.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> ---
>  src/libcamera/pipeline/ipu3/ipu3.cpp | 25 ++++---------------------
>  1 file changed, 4 insertions(+), 21 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index 405550b1302fb370..5a473e18c082cee8 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -49,7 +49,6 @@ public:
>  		V4L2VideoDevice *dev;
>  		unsigned int pad;
>  		std::string name;
> -		std::vector<std::unique_ptr<FrameBuffer>> buffers;
>  	};
>  
>  	ImgUDevice()
> @@ -1124,9 +1123,6 @@ int ImgUDevice::configureOutput(ImgUOutput *output,
>   */
>  int ImgUDevice::allocateBuffers(IPU3CameraData *data, unsigned int bufferCount)
>  {
> -	IPU3Stream *outStream = &data->outStream_;
> -	IPU3Stream *vfStream = &data->vfStream_;
> -
>  	/* Share buffers between CIO2 output and ImgU input. */
>  	int ret = input_->importBuffers(bufferCount);
>  	if (ret) {
> @@ -1147,28 +1143,15 @@ int ImgUDevice::allocateBuffers(IPU3CameraData *data, unsigned int bufferCount)
>  		goto error;
>  	}
>  
> -	/*
> -	 * Allocate buffers for both outputs. If an output is active, prepare
> -	 * for buffer import, otherwise allocate internal buffers. Use the same
> -	 * number of buffers in either case.
> -	 */

I'd keep a small comment here, maybe

	/* Import buffers for all outputs, regardless of whether the
	 * corresponding stream is active or inactive, as the driver needs
	 * buffers to be requested on the V4L2 devices in order to operate.
	 */

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> -	if (outStream->active_)
> -		ret = output_.dev->importBuffers(bufferCount);
> -	else
> -		ret = output_.dev->allocateBuffers(bufferCount,
> -						   &output_.buffers);
> +	ret = output_.dev->importBuffers(bufferCount);
>  	if (ret < 0) {
> -		LOG(IPU3, Error) << "Failed to allocate ImgU output buffers";
> +		LOG(IPU3, Error) << "Failed to import ImgU output buffers";
>  		goto error;
>  	}
>  
> -	if (vfStream->active_)
> -		ret = viewfinder_.dev->importBuffers(bufferCount);
> -	else
> -		ret = viewfinder_.dev->allocateBuffers(bufferCount,
> -						       &viewfinder_.buffers);
> +	ret = viewfinder_.dev->importBuffers(bufferCount);
>  	if (ret < 0) {
> -		LOG(IPU3, Error) << "Failed to allocate ImgU viewfinder buffers";
> +		LOG(IPU3, Error) << "Failed to import ImgU viewfinder buffers";
>  		goto error;
>  	}
>  

-- 
Regards,

Laurent Pinchart