[libcamera-devel] [PATCH v2 09/13] libcamera: ipu3: Do not duplicate data in IPU3Stream

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Sun, Jun 28, 2020 at 02:15:28AM +0200, Niklas Söderlund wrote:
> Do not keep the duplicated ImgUDevice::ImgUOutput information in both
> the stream and camera data classes. Remove it from the stream and only
> access it from the camera data class.
> 
> Which stream is which can instead be checked by comparing it to the
> known streams in camera data. This match how streams are checked in
> other parts of the code making the pipeline more coherent.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
> * Changes since v1
> - s/driver/pipeline/ in commit message.
> - Reorder if ; if else ; else statement in
>   PipelineHandlerIPU3::queueRequestDevice()
> ---
>  src/libcamera/pipeline/ipu3/ipu3.cpp | 32 ++++++++++++++++------------
>  1 file changed, 18 insertions(+), 14 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index e817f842f1216a7f..c1520ec40fe7b57c 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -35,13 +35,11 @@ class IPU3Stream : public Stream
>  {
>  public:
>  	IPU3Stream()
> -		: active_(false), raw_(false), device_(nullptr)
> +		: active_(false)
>  	{
>  	}
>  
>  	bool active_;
> -	bool raw_;
> -	ImgUDevice::ImgUOutput *device_;
>  };
>  
>  class IPU3CameraData : public CameraData
> @@ -276,7 +274,7 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()
>  		const StreamConfiguration oldCfg = cfg;
>  		const IPU3Stream *stream = streams_[i];
>  
> -		if (stream->raw_) {
> +		if (stream == &data_->rawStream_) {
>  			cfg = cio2Configuration_;
>  		} else {
>  			bool scale = stream == &data_->vfStream_;
> @@ -572,13 +570,16 @@ int PipelineHandlerIPU3::exportFrameBuffers(Camera *camera, Stream *stream,
>  					    std::vector<std::unique_ptr<FrameBuffer>> *buffers)
>  {
>  	IPU3CameraData *data = cameraData(camera);
> -	IPU3Stream *ipu3stream = static_cast<IPU3Stream *>(stream);
>  	unsigned int count = stream->configuration().bufferCount;
>  
> -	if (ipu3stream->raw_)
> +	if (stream == &data->outStream_)
> +		return data->imgu_->output_.dev->exportBuffers(count, buffers);
> +	else if (stream == &data->vfStream_)
> +		return data->imgu_->viewfinder_.dev->exportBuffers(count, buffers);
> +	else if (stream == &data->rawStream_)
>  		return data->cio2_.exportBuffers(count, buffers);
>  
> -	return ipu3stream->device_->dev->exportBuffers(count, buffers);
> +	return -EINVAL;
>  }
>  
>  /**
> @@ -685,11 +686,17 @@ int PipelineHandlerIPU3::queueRequestDevice(Camera *camera, Request *request)
>  	/* Queue all buffers from the request aimed for the ImgU. */
>  	for (auto it : request->buffers()) {
>  		IPU3Stream *stream = static_cast<IPU3Stream *>(it.first);
> -		if (stream->raw_)
> -			continue;
> -
>  		FrameBuffer *buffer = it.second;
> -		int ret = stream->device_->dev->queueBuffer(buffer);
> +
> +		if (stream == &data->rawStream_)
> +			continue;
> +
> +		int ret = 0;
> +		if (stream == &data->outStream_)
> +			ret = data->imgu_->output_.dev->queueBuffer(buffer);
> +		else if (stream == &data->vfStream_)
> +			ret = data->imgu_->viewfinder_.dev->queueBuffer(buffer);

		else
			continue;

or

		else
			ret = 0;

and dropping the previous &data->rawStream_ check would allow not
initializing the ret variable to 0.

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

> +
>  		if (ret < 0)
>  			error = ret;
>  	}
> @@ -801,9 +808,6 @@ int PipelineHandlerIPU3::registerCameras()
>  		 * second.
>  		 */
>  		data->imgu_ = numCameras ? &imgu1_ : &imgu0_;
> -		data->outStream_.device_ = &data->imgu_->output_;
> -		data->vfStream_.device_ = &data->imgu_->viewfinder_;
> -		data->rawStream_.raw_ = true;
>  
>  		/*
>  		 * Connect video devices' 'bufferReady' signals to their

-- 
Regards,

Laurent Pinchart