[libcamera-devel] [PATCH v5 1/6] pipeline: raspberrypi: Refactor stream configuration routine

[Laurent Pinchart]

Hi Naush,

Thank you for the patch.

On Fri, Jan 29, 2021 at 03:11:49PM +0000, Naushir Patuck wrote:
> Refactor the high/low resolution stream format and output selection
> routine. This change is in preparation of adding 1/4 resolution output
> for fast colour denoise.
> 
> Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> Reviewed-by: David Plowman <david.plowman at raspberrypi.com>
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> ---
>  .../pipeline/raspberrypi/raspberrypi.cpp      | 57 ++++++-------------
>  1 file changed, 16 insertions(+), 41 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index 524cc960dd37..d9895c779725 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -624,52 +624,27 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)
>  			continue;
>  		}
>  
> -		if (i == maxIndex) {
> -			/* ISP main output format. */
> -			V4L2VideoDevice *dev = data->isp_[Isp::Output0].dev();
> -			V4L2PixelFormat fourcc = dev->toV4L2PixelFormat(cfg.pixelFormat);
> -			format.size = cfg.size;
> -			format.fourcc = fourcc;
> -
> -			ret = dev->setFormat(&format);
> -			if (ret)
> -				return -EINVAL;
> -
> -			if (format.size != cfg.size || format.fourcc != fourcc) {
> -				LOG(RPI, Error)
> -					<< "Failed to set format on ISP capture0 device: "
> -					<< format.toString();
> -				return -EINVAL;
> -			}
> -
> -			cfg.setStream(&data->isp_[Isp::Output0]);
> -			data->isp_[Isp::Output0].setExternal(true);
> -		}
> +		/* The largest resolution gets routed to the ISP Output 0 node. */
> +		RPi::Stream *stream = (i == maxIndex) ? &data->isp_[Isp::Output0]

No need for the outer parentheses.

> +						      : &data->isp_[Isp::Output1];
>  
> -		/*
> -		 * ISP second output format. This fallthrough means that if a
> -		 * second output stream has not been configured, we simply use
> -		 * the Output0 configuration.
> -		 */
> -		V4L2VideoDevice *dev = data->isp_[Isp::Output1].dev();
> -		format.fourcc = dev->toV4L2PixelFormat(cfg.pixelFormat);
> +		V4L2PixelFormat fourcc = stream->dev()->toV4L2PixelFormat(cfg.pixelFormat);
>  		format.size = cfg.size;
> +		format.fourcc = fourcc;
>  
> -		ret = dev->setFormat(&format);
> -		if (ret) {
> +		ret = stream->dev()->setFormat(&format);
> +		if (ret)
> +			return -EINVAL;
> +
> +		if (format.size != cfg.size || format.fourcc != fourcc) {
>  			LOG(RPI, Error)
> -				<< "Failed to set format on ISP capture1 device: "
> -				<< format.toString();
> -			return ret;
> -		}
> -		/*
> -		 * If we have not yet provided a stream for this config, it
> -		 * means this is to be routed from Output1.
> -		 */
> -		if (!cfg.stream()) {
> -			cfg.setStream(&data->isp_[Isp::Output1]);
> -			data->isp_[Isp::Output1].setExternal(true);
> +				<< "Failed get requested format on " << stream->name()

"get" or "set" ?

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

I can fix these small issues when applying.

> +				<< ", returned " << format.toString();
> +			return -EINVAL;
>  		}
> +
> +		cfg.setStream(stream);
> +		stream->setExternal(true);
>  	}
>  
>  	/* ISP statistics output format. */

-- 
Regards,

Laurent Pinchart