[libcamera-devel] [PATCH v2 6/6] pipeline: raspberrypi: Apply sensor flips at the start of configure()

[Laurent Pinchart]

Hi Naush,

Thank you for the patch.

On Fri, Oct 22, 2021 at 03:39:07PM +0100, Naushir Patuck wrote:
> Sensor flips might change the Bayer order of the requested format. The existing
> code would set a sensor format along with the appropriate Unicam and ISP input
> formats, but reset the latter two on start() once the flips had been requested.
> 
> We can now set the sensor flips just after we set the sensor mode in
> configure(), thereby not needing the second pair of format sets in start().
> 
> Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> Review-by: David Plowman <david.plowman at raspberrypi.com>
> ---
>  .../pipeline/raspberrypi/raspberrypi.cpp      | 60 +++++++------------
>  1 file changed, 21 insertions(+), 39 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index c5e9607c7d95..ad6f1231bbf6 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -627,16 +627,34 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)
>  
>  	/* First calculate the best sensor mode we can use based on the user request. */
>  	V4L2SubdeviceFormat sensorFormat = findBestMode(data->sensorFormats_, rawStream ? sensorSize : maxSize);
> -	V4L2DeviceFormat unicamFormat = toV4L2DeviceFormat(sensorFormat, unpacked);
>  
>  	ret = data->sensor_->setFormat(&sensorFormat);
>  	if (ret)
>  		return ret;
>  
>  	/*
> -	 * Unicam image output format. The ISP input format gets set at start,
> -	 * just in case we have swapped bayer orders due to flips.
> +	 * Configure the H/V flip controls based on the combination of
> +	 * the sensor and user transform.
>  	 */
> +	if (data->supportsFlips_) {
> +		const RPiCameraConfiguration *rpiConfig =
> +			static_cast<const RPiCameraConfiguration *>(config);
> +		ControlList controls;
> +
> +		controls.set(V4L2_CID_HFLIP,
> +			     static_cast<int32_t>(!!(rpiConfig->combinedTransform_ & Transform::HFlip)));
> +		controls.set(V4L2_CID_VFLIP,
> +			     static_cast<int32_t>(!!(rpiConfig->combinedTransform_ & Transform::VFlip)));
> +		data->setSensorControls(controls);
> +
> +		/*
> +		 * IPA configure may have changed the sensor flips - hence the bayer
> +		 * order. So update the sensor format now.
> +		 */
> +		data->sensor_->device()->getFormat(0, &sensorFormat);
> +	}

If you moved this whole block before the sensor_->setFormat() call,
could you drop getFormat() ?

> +
> +	V4L2DeviceFormat unicamFormat = toV4L2DeviceFormat(sensorFormat, unpacked);
>  	ret = data->unicam_[Unicam::Image].dev()->setFormat(&unicamFormat);
>  	if (ret)
>  		return ret;
> @@ -645,10 +663,6 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)
>  		       << " - Selected sensor mode: " << sensorFormat.toString()
>  		       << " - Selected unicam mode: " << unicamFormat.toString();
>  
> -	/*
> -	 * This format may be reset on start() if the bayer order has changed
> -	 * because of flips in the sensor.
> -	 */
>  	ret = data->isp_[Isp::Input].dev()->setFormat(&unicamFormat);
>  	if (ret)
>  		return ret;
> @@ -871,23 +885,6 @@ int PipelineHandlerRPi::start(Camera *camera, const ControlList *controls)
>  		return ret;
>  	}
>  
> -	/*
> -	 * IPA configure may have changed the sensor flips - hence the bayer
> -	 * order. Get the sensor format and set the ISP input now.
> -	 */
> -	V4L2SubdeviceFormat sensorFormat;
> -	data->sensor_->device()->getFormat(0, &sensorFormat);
> -
> -	V4L2DeviceFormat ispFormat;
> -	ispFormat.fourcc = BayerFormat::fromMbusCode(sensorFormat.mbus_code).toV4L2PixelFormat();
> -	ispFormat.size = sensorFormat.size;
> -
> -	ret = data->isp_[Isp::Input].dev()->setFormat(&ispFormat);
> -	if (ret) {
> -		stop(camera);
> -		return ret;
> -	}
> -
>  	/* Enable SOF event generation. */
>  	data->unicam_[Unicam::Image].dev()->setFrameStartEnabled(true);
>  
> @@ -1301,10 +1298,6 @@ int RPiCameraData::loadIPA(ipa::RPi::SensorConfig *sensorConfig)
>  
>  int RPiCameraData::configureIPA(const CameraConfiguration *config)
>  {
> -	/* We know config must be an RPiCameraConfiguration. */
> -	const RPiCameraConfiguration *rpiConfig =
> -		static_cast<const RPiCameraConfiguration *>(config);
> -
>  	std::map<unsigned int, IPAStream> streamConfig;
>  	std::map<unsigned int, ControlInfoMap> entityControls;
>  	ipa::RPi::IPAConfig ipaConfig;
> @@ -1355,17 +1348,6 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config)
>  		return -EPIPE;
>  	}
>  
> -	/*
> -	 * Configure the H/V flip controls based on the combination of
> -	 * the sensor and user transform.
> -	 */
> -	if (supportsFlips_) {
> -		controls.set(V4L2_CID_HFLIP,
> -			     static_cast<int32_t>(!!(rpiConfig->combinedTransform_ & Transform::HFlip)));
> -		controls.set(V4L2_CID_VFLIP,
> -			     static_cast<int32_t>(!!(rpiConfig->combinedTransform_ & Transform::VFlip)));
> -	}
> -
>  	if (!controls.empty())
>  		setSensorControls(controls);
>  

-- 
Regards,

Laurent Pinchart