[libcamera-devel,v3,9/9] pipeline: raspberrypi: Apply sensor flips at the start of configure()
diff mbox series

Message ID 20211027092803.3671096-10-naush@raspberrypi.com
State Superseded
Headers show
Series
  • Raspberry Pi: Conversion to media controller
Related show

Commit Message

Naushir Patuck Oct. 27, 2021, 9:28 a.m. UTC
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 before we set the sensor mode in
configure(), thereby not needing the second pair of format sets in start().

Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
---
 .../pipeline/raspberrypi/raspberrypi.cpp      | 56 ++++++-------------
 1 file changed, 16 insertions(+), 40 deletions(-)

Comments

Kieran Bingham Oct. 27, 2021, 12:33 p.m. UTC | #1
Quoting Naushir Patuck (2021-10-27 10:28:03)
> 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 before we set the sensor mode in
> configure(), thereby not needing the second pair of format sets in start().
> 
> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> Reviewed-by: David Plowman <david.plowman@raspberrypi.com>

Will the user be able to flip at runtime? I suspect they can't currently
anyway so it doesn't matter, but anyway, I can't see anything wrong with
this:


Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> ---
>  .../pipeline/raspberrypi/raspberrypi.cpp      | 56 ++++++-------------
>  1 file changed, 16 insertions(+), 40 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index 52521857b61c..eaa2676b891e 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -657,16 +657,28 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)
>                 }
>         }
>  
> +       /*
> +        * 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);
> +       }
> +
>         /* First calculate the best sensor mode we can use based on the user request. */
>         V4L2SubdeviceFormat sensorFormat = findBestFormat(data->sensorFormats_, rawStream ? sensorSize : maxSize);
>         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.
> -        */
>         V4L2DeviceFormat unicamFormat = toV4L2DeviceFormat(sensorFormat, packing);
>         ret = data->unicam_[Unicam::Image].dev()->setFormat(&unicamFormat);
>         if (ret)
> @@ -676,10 +688,6 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)
>                        << " - Selected sensor format: " << sensorFormat.toString()
>                        << " - Selected unicam format: " << 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;
> @@ -902,23 +910,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 = mbusCodeToBayerFormat(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);
>  
> @@ -1332,10 +1323,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;
> @@ -1386,17 +1373,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);
>  
> -- 
> 2.25.1
>
Naushir Patuck Oct. 27, 2021, 12:41 p.m. UTC | #2
Hi Kieran,

Thank you for your feedback,

On Wed, 27 Oct 2021 at 13:33, Kieran Bingham <
kieran.bingham@ideasonboard.com> wrote:

> Quoting Naushir Patuck (2021-10-27 10:28:03)
> > 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 before we set the sensor mode in
> > configure(), thereby not needing the second pair of format sets in
> start().
> >
> > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> > Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
>
> Will the user be able to flip at runtime? I suspect they can't currently
> anyway so it doesn't matter, but anyway, I can't see anything wrong with
> this:
>

No runtime changes of flips are not possible.  That would be too
complicated to deal with :-)

Regards,
Naush


>
>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>
> > ---
> >  .../pipeline/raspberrypi/raspberrypi.cpp      | 56 ++++++-------------
> >  1 file changed, 16 insertions(+), 40 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > index 52521857b61c..eaa2676b891e 100644
> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > @@ -657,16 +657,28 @@ int PipelineHandlerRPi::configure(Camera *camera,
> CameraConfiguration *config)
> >                 }
> >         }
> >
> > +       /*
> > +        * 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);
> > +       }
> > +
> >         /* First calculate the best sensor mode we can use based on the
> user request. */
> >         V4L2SubdeviceFormat sensorFormat =
> findBestFormat(data->sensorFormats_, rawStream ? sensorSize : maxSize);
> >         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.
> > -        */
> >         V4L2DeviceFormat unicamFormat = toV4L2DeviceFormat(sensorFormat,
> packing);
> >         ret =
> data->unicam_[Unicam::Image].dev()->setFormat(&unicamFormat);
> >         if (ret)
> > @@ -676,10 +688,6 @@ int PipelineHandlerRPi::configure(Camera *camera,
> CameraConfiguration *config)
> >                        << " - Selected sensor format: " <<
> sensorFormat.toString()
> >                        << " - Selected unicam format: " <<
> 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;
> > @@ -902,23 +910,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 =
> mbusCodeToBayerFormat(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);
> >
> > @@ -1332,10 +1323,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;
> > @@ -1386,17 +1373,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);
> >
> > --
> > 2.25.1
> >
>
Laurent Pinchart Oct. 27, 2021, 6:28 p.m. UTC | #3
Hi Naush,

Thank you for the patch.

On Wed, Oct 27, 2021 at 10:28:03AM +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 before we set the sensor mode in
> configure(), thereby not needing the second pair of format sets in start().
> 
> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> Reviewed-by: David Plowman <david.plowman@raspberrypi.com>

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

> ---
>  .../pipeline/raspberrypi/raspberrypi.cpp      | 56 ++++++-------------
>  1 file changed, 16 insertions(+), 40 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index 52521857b61c..eaa2676b891e 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -657,16 +657,28 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)
>  		}
>  	}
>  
> +	/*
> +	 * 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);
> +	}
> +
>  	/* First calculate the best sensor mode we can use based on the user request. */
>  	V4L2SubdeviceFormat sensorFormat = findBestFormat(data->sensorFormats_, rawStream ? sensorSize : maxSize);
>  	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.
> -	 */
>  	V4L2DeviceFormat unicamFormat = toV4L2DeviceFormat(sensorFormat, packing);
>  	ret = data->unicam_[Unicam::Image].dev()->setFormat(&unicamFormat);
>  	if (ret)
> @@ -676,10 +688,6 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)
>  		       << " - Selected sensor format: " << sensorFormat.toString()
>  		       << " - Selected unicam format: " << 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;
> @@ -902,23 +910,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 = mbusCodeToBayerFormat(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);
>  
> @@ -1332,10 +1323,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;
> @@ -1386,17 +1373,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);
>

Patch
diff mbox series

diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
index 52521857b61c..eaa2676b891e 100644
--- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
+++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
@@ -657,16 +657,28 @@  int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)
 		}
 	}
 
+	/*
+	 * 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);
+	}
+
 	/* First calculate the best sensor mode we can use based on the user request. */
 	V4L2SubdeviceFormat sensorFormat = findBestFormat(data->sensorFormats_, rawStream ? sensorSize : maxSize);
 	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.
-	 */
 	V4L2DeviceFormat unicamFormat = toV4L2DeviceFormat(sensorFormat, packing);
 	ret = data->unicam_[Unicam::Image].dev()->setFormat(&unicamFormat);
 	if (ret)
@@ -676,10 +688,6 @@  int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)
 		       << " - Selected sensor format: " << sensorFormat.toString()
 		       << " - Selected unicam format: " << 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;
@@ -902,23 +910,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 = mbusCodeToBayerFormat(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);
 
@@ -1332,10 +1323,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;
@@ -1386,17 +1373,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);