[libcamera-devel] libcamera: ipu3: Add rotation to ipu3 pipeline
diff mbox series

Message ID 20210106133513.34118-1-me@fabwu.ch
State Superseded
Headers show
Series
  • [libcamera-devel] libcamera: ipu3: Add rotation to ipu3 pipeline
Related show

Commit Message

Fabian Wüthrich Jan. 6, 2021, 1:35 p.m. UTC
Use the same transformation logic as in the raspberry pipeline to
implement rotations in the ipu3 pipeline.

Tested on a Surface Book 2 with an experimental driver for OV5693.

Signed-off-by: Fabian Wüthrich <me@fabwu.ch>
---
 src/libcamera/pipeline/ipu3/ipu3.cpp | 76 +++++++++++++++++++++++++++-
 1 file changed, 74 insertions(+), 2 deletions(-)

Comments

Jacopo Mondi Jan. 7, 2021, 5:33 p.m. UTC | #1
Hi Fabian

On Wed, Jan 06, 2021 at 02:35:13PM +0100, Fabian Wüthrich wrote:
> Use the same transformation logic as in the raspberry pipeline to
> implement rotations in the ipu3 pipeline.
>
> Tested on a Surface Book 2 with an experimental driver for OV5693.
>
> Signed-off-by: Fabian Wüthrich <me@fabwu.ch>
> ---
>  src/libcamera/pipeline/ipu3/ipu3.cpp | 76 +++++++++++++++++++++++++++-
>  1 file changed, 74 insertions(+), 2 deletions(-)
>
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index f1151733..76d75a74 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -14,6 +14,7 @@
>  #include <libcamera/camera.h>
>  #include <libcamera/control_ids.h>
>  #include <libcamera/formats.h>
> +#include <libcamera/property_ids.h>
>  #include <libcamera/request.h>
>  #include <libcamera/stream.h>
>
> @@ -62,6 +63,12 @@ public:
>  	Stream outStream_;
>  	Stream vfStream_;
>  	Stream rawStream_;
> +
> +	/*
> +	 * Manage horizontal and vertical flips supported (or not) by the
> +	 * sensor.
> +	 */
> +	bool supportsFlips_;
>  };
>
>  class IPU3CameraConfiguration : public CameraConfiguration
> @@ -74,6 +81,9 @@ public:
>  	const StreamConfiguration &cio2Format() const { return cio2Configuration_; }
>  	const ImgUDevice::PipeConfig imguConfig() const { return pipeConfig_; }
>
> +	/* Cache the combinedTransform_ that will be applied to the sensor */
> +	Transform combinedTransform_;
> +
>  private:
>  	/*
>  	 * The IPU3CameraData instance is guaranteed to be valid as long as the
> @@ -143,11 +153,54 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()
>  	if (config_.empty())
>  		return Invalid;
>
> -	if (transform != Transform::Identity) {
> -		transform = Transform::Identity;
> +	int32_t rotation = data_->cio2_.sensor()->properties().get(properties::Rotation);
> +	bool success;
> +	Transform rotationTransform = transformFromRotation(rotation, &success);

Can you cache this in CameraData ?
At registerCamera() time we initialize the properties so we can cache
the above 'rotationTransform' just after

		/* Initialize the camera properties. */
		data->properties_ = cio2->sensor()->properties();

Instead of re-calculating it every validation.

> +	if (!success)
> +		LOG(IPU3, Warning) << "Invalid rotation of " << rotation << " degrees - ignoring";
> +	Transform combined = transform * rotationTransform;
> +
> +	/*
> +	 * We combine the platform and user transform, but must "adjust away"
> +	 * any combined result that includes a transform, as we can't do those.
> +	 * In this case, flipping only the transpose bit is helpful to
> +	 * applications - they either get the transform they requested, or have
> +	 * to do a simple transpose themselves (they don't have to worry about
> +	 * the other possible cases).
> +	 */

The 'rotationTransform' obtained by inspecting the sensor rotation
does not contain  any Transpose, looking at
Transform::transformFromRotation().

Can you address the transpose directly in the
CameraConfiguration::transpose field before multiplying it by
rotationTransform ?

> +	if (!!(combined & Transform::Transpose)) {
> +		/*
> +		 * Flipping the transpose bit in "transform" flips it in the
> +		 * combined result too (as it's the last thing that happens),
> +		 * which is of course clearing it.
> +		 */
> +		transform ^= Transform::Transpose;
> +		combined &= ~Transform::Transpose;
>  		status = Adjusted;
>  	}
>
> +	/*
> +	 * We also check if the sensor doesn't do h/vflips at all, in which
> +	 * case we clear them, ad the application will have to do everything.

s/ad/and

> +	 */
> +	if (!data_->supportsFlips_ && !!combined) {
> +		/*
> +		 * If the sensor can do no transforms, then combined must be
> +		 * changed to the identity. The only user transform that gives
> +		 * rise to this the inverse of the rotation. (Recall that

s/this the/this is the/

> +		 * combined = transform * rotationTransform.)
> +		 */
> +		transform = -rotationTransform;
> +		combined = Transform::Identity;
> +		status = Adjusted;
> +	}
> +
> +	/*
> +	 * Store the final combined transform that configure() will need to
> +	 * apply to the sensor to save us working it out again.
> +	 */
> +	combinedTransform_ = combined;
> +
>  	/* Cap the number of entries to the available streams. */
>  	if (config_.size() > IPU3_MAX_STREAMS) {
>  		config_.resize(IPU3_MAX_STREAMS);
> @@ -540,6 +593,19 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)
>  		return ret;
>  	}
>
> +	/*
> +	 * Configure the H/V flip controls based on the combination of
> +	 * the sensor and user transform.
> +	 */
> +	if (data->supportsFlips_) {
> +		ctrls = cio2->sensor()->controls();

I would not re-use ctrls, or rather we should probably:

--- a/include/libcamera/controls.h
+++ b/include/libcamera/controls.h
@@ -352,7 +352,7 @@ private:
 public:
        ControlList();
        ControlList(const ControlIdMap &idmap, ControlValidator *validator = nullptr);
-       ControlList(const ControlInfoMap &infoMap, ControlValidator *validator = nullptr);
+       explicit ControlList(const ControlInfoMap &infoMap, ControlValidator *validator = nullptr);

to avoid this assignment. But maybe it's just me

Thanks
   j

> +		ctrls.set(V4L2_CID_HFLIP,
> +			  static_cast<int32_t>(!!(config->combinedTransform_ & Transform::HFlip)));
> +		ctrls.set(V4L2_CID_VFLIP,
> +			  static_cast<int32_t>(!!(config->combinedTransform_ & Transform::VFlip)));
> +		cio2->sensor()->setControls(&ctrls);
> +	}
> +
>  	return 0;
>  }
>
> @@ -779,6 +845,12 @@ int PipelineHandlerIPU3::registerCameras()
>  		/* Initialze the camera controls. */
>  		data->controlInfo_ = IPU3Controls;
>
> +		ControlList ctrls = cio2->sensor()->getControls({ V4L2_CID_HFLIP });
> +		if (!ctrls.empty()) {
> +			/* We assume it will support vflips too... */
> +			data->supportsFlips_ = true;
> +		}
> +
>  		/**
>  		 * \todo Dynamically assign ImgU and output devices to each
>  		 * stream and camera; as of now, limit support to two cameras
> --
> 2.30.0
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Fabian Wüthrich Jan. 11, 2021, 3:21 p.m. UTC | #2
Hi Jacopo

Thanks for your review. 

On 07.01.21 18:33, Jacopo Mondi wrote:
> Hi Fabian
> 
> On Wed, Jan 06, 2021 at 02:35:13PM +0100, Fabian Wüthrich wrote:
>> Use the same transformation logic as in the raspberry pipeline to
>> implement rotations in the ipu3 pipeline.
>>
>> Tested on a Surface Book 2 with an experimental driver for OV5693.
>>
>> Signed-off-by: Fabian Wüthrich <me@fabwu.ch>
>> ---
>>  src/libcamera/pipeline/ipu3/ipu3.cpp | 76 +++++++++++++++++++++++++++-
>>  1 file changed, 74 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
>> index f1151733..76d75a74 100644
>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
>> @@ -14,6 +14,7 @@
>>  #include <libcamera/camera.h>
>>  #include <libcamera/control_ids.h>
>>  #include <libcamera/formats.h>
>> +#include <libcamera/property_ids.h>
>>  #include <libcamera/request.h>
>>  #include <libcamera/stream.h>
>>
>> @@ -62,6 +63,12 @@ public:
>>  	Stream outStream_;
>>  	Stream vfStream_;
>>  	Stream rawStream_;
>> +
>> +	/*
>> +	 * Manage horizontal and vertical flips supported (or not) by the
>> +	 * sensor.
>> +	 */
>> +	bool supportsFlips_;
>>  };
>>
>>  class IPU3CameraConfiguration : public CameraConfiguration
>> @@ -74,6 +81,9 @@ public:
>>  	const StreamConfiguration &cio2Format() const { return cio2Configuration_; }
>>  	const ImgUDevice::PipeConfig imguConfig() const { return pipeConfig_; }
>>
>> +	/* Cache the combinedTransform_ that will be applied to the sensor */
>> +	Transform combinedTransform_;
>> +
>>  private:
>>  	/*
>>  	 * The IPU3CameraData instance is guaranteed to be valid as long as the
>> @@ -143,11 +153,54 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()
>>  	if (config_.empty())
>>  		return Invalid;
>>
>> -	if (transform != Transform::Identity) {
>> -		transform = Transform::Identity;
>> +	int32_t rotation = data_->cio2_.sensor()->properties().get(properties::Rotation);
>> +	bool success;
>> +	Transform rotationTransform = transformFromRotation(rotation, &success);
> 
> Can you cache this in CameraData ?
> At registerCamera() time we initialize the properties so we can cache
> the above 'rotationTransform' just after
> 
> 		/* Initialize the camera properties. */
> 		data->properties_ = cio2->sensor()->properties();
> 
> Instead of re-calculating it every validation.
> 

Done

>> +	if (!success)
>> +		LOG(IPU3, Warning) << "Invalid rotation of " << rotation << " degrees - ignoring";
>> +	Transform combined = transform * rotationTransform;
>> +
>> +	/*
>> +	 * We combine the platform and user transform, but must "adjust away"
>> +	 * any combined result that includes a transform, as we can't do those.
>> +	 * In this case, flipping only the transpose bit is helpful to
>> +	 * applications - they either get the transform they requested, or have
>> +	 * to do a simple transpose themselves (they don't have to worry about
>> +	 * the other possible cases).
>> +	 */
> 
> The 'rotationTransform' obtained by inspecting the sensor rotation
> does not contain  any Transpose, looking at
> Transform::transformFromRotation().
> 
> Can you address the transpose directly in the
> CameraConfiguration::transpose field before multiplying it by
> rotationTransform ?
> 

I copied the code from the raspberry pipeline (see 
src/libcamera/pipeline/raspberrypi/raspberrypi.cpp line 287 ff.) and
assumed that this is the correct way to handle transpositions.

Should I just flip the transpose bit of CameraConfiguration::transpose
and then combine it with the sensor rotation?

And if so should I send a patch for the raspberry pipeline as well?

>> +	if (!!(combined & Transform::Transpose)) {
>> +		/*
>> +		 * Flipping the transpose bit in "transform" flips it in the
>> +		 * combined result too (as it's the last thing that happens),
>> +		 * which is of course clearing it.
>> +		 */
>> +		transform ^= Transform::Transpose;
>> +		combined &= ~Transform::Transpose;
>>  		status = Adjusted;
>>  	}
>>
>> +	/*
>> +	 * We also check if the sensor doesn't do h/vflips at all, in which
>> +	 * case we clear them, ad the application will have to do everything.
> 
> s/ad/and
> 

Done

>> +	 */
>> +	if (!data_->supportsFlips_ && !!combined) {
>> +		/*
>> +		 * If the sensor can do no transforms, then combined must be
>> +		 * changed to the identity. The only user transform that gives
>> +		 * rise to this the inverse of the rotation. (Recall that
> 
> s/this the/this is the/
> 

Done

>> +		 * combined = transform * rotationTransform.)
>> +		 */
>> +		transform = -rotationTransform;
>> +		combined = Transform::Identity;
>> +		status = Adjusted;
>> +	}
>> +
>> +	/*
>> +	 * Store the final combined transform that configure() will need to
>> +	 * apply to the sensor to save us working it out again.
>> +	 */
>> +	combinedTransform_ = combined;
>> +
>>  	/* Cap the number of entries to the available streams. */
>>  	if (config_.size() > IPU3_MAX_STREAMS) {
>>  		config_.resize(IPU3_MAX_STREAMS);
>> @@ -540,6 +593,19 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)
>>  		return ret;
>>  	}
>>
>> +	/*
>> +	 * Configure the H/V flip controls based on the combination of
>> +	 * the sensor and user transform.
>> +	 */
>> +	if (data->supportsFlips_) {
>> +		ctrls = cio2->sensor()->controls();
> 
> I would not re-use ctrls, or rather we should probably:
> 
> --- a/include/libcamera/controls.h
> +++ b/include/libcamera/controls.h
> @@ -352,7 +352,7 @@ private:
>  public:
>         ControlList();
>         ControlList(const ControlIdMap &idmap, ControlValidator *validator = nullptr);
> -       ControlList(const ControlInfoMap &infoMap, ControlValidator *validator = nullptr);
> +       explicit ControlList(const ControlInfoMap &infoMap, ControlValidator *validator = nullptr);
> 
> to avoid this assignment. But maybe it's just me
> 

I created a sensor_ctrls variable to prevent the re-use.

> Thanks
>    j
> 
>> +		ctrls.set(V4L2_CID_HFLIP,
>> +			  static_cast<int32_t>(!!(config->combinedTransform_ & Transform::HFlip)));
>> +		ctrls.set(V4L2_CID_VFLIP,
>> +			  static_cast<int32_t>(!!(config->combinedTransform_ & Transform::VFlip)));
>> +		cio2->sensor()->setControls(&ctrls);
>> +	}
>> +
>>  	return 0;
>>  }
>>
>> @@ -779,6 +845,12 @@ int PipelineHandlerIPU3::registerCameras()
>>  		/* Initialze the camera controls. */
>>  		data->controlInfo_ = IPU3Controls;
>>
>> +		ControlList ctrls = cio2->sensor()->getControls({ V4L2_CID_HFLIP });
>> +		if (!ctrls.empty()) {
>> +			/* We assume it will support vflips too... */
>> +			data->supportsFlips_ = true;
>> +		}
>> +
>>  		/**
>>  		 * \todo Dynamically assign ImgU and output devices to each
>>  		 * stream and camera; as of now, limit support to two cameras
>> --
>> 2.30.0
>>
>> _______________________________________________
>> libcamera-devel mailing list
>> libcamera-devel@lists.libcamera.org
>> https://lists.libcamera.org/listinfo/libcamera-devel
Jacopo Mondi Jan. 11, 2021, 3:45 p.m. UTC | #3
Hi Fabian

On Mon, Jan 11, 2021 at 04:21:49PM +0100, Fabian Wüthrich wrote:
> Hi Jacopo
>
> Thanks for your review.
>
> On 07.01.21 18:33, Jacopo Mondi wrote:
> > Hi Fabian
> >
> > On Wed, Jan 06, 2021 at 02:35:13PM +0100, Fabian Wüthrich wrote:
> >> Use the same transformation logic as in the raspberry pipeline to
> >> implement rotations in the ipu3 pipeline.
> >>
> >> Tested on a Surface Book 2 with an experimental driver for OV5693.
> >>
> >> Signed-off-by: Fabian Wüthrich <me@fabwu.ch>
> >> ---
> >>  src/libcamera/pipeline/ipu3/ipu3.cpp | 76 +++++++++++++++++++++++++++-
> >>  1 file changed, 74 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> >> index f1151733..76d75a74 100644
> >> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> >> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> >> @@ -14,6 +14,7 @@
> >>  #include <libcamera/camera.h>
> >>  #include <libcamera/control_ids.h>
> >>  #include <libcamera/formats.h>
> >> +#include <libcamera/property_ids.h>
> >>  #include <libcamera/request.h>
> >>  #include <libcamera/stream.h>
> >>
> >> @@ -62,6 +63,12 @@ public:
> >>  	Stream outStream_;
> >>  	Stream vfStream_;
> >>  	Stream rawStream_;
> >> +
> >> +	/*
> >> +	 * Manage horizontal and vertical flips supported (or not) by the
> >> +	 * sensor.
> >> +	 */
> >> +	bool supportsFlips_;
> >>  };
> >>
> >>  class IPU3CameraConfiguration : public CameraConfiguration
> >> @@ -74,6 +81,9 @@ public:
> >>  	const StreamConfiguration &cio2Format() const { return cio2Configuration_; }
> >>  	const ImgUDevice::PipeConfig imguConfig() const { return pipeConfig_; }
> >>
> >> +	/* Cache the combinedTransform_ that will be applied to the sensor */
> >> +	Transform combinedTransform_;
> >> +
> >>  private:
> >>  	/*
> >>  	 * The IPU3CameraData instance is guaranteed to be valid as long as the
> >> @@ -143,11 +153,54 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()
> >>  	if (config_.empty())
> >>  		return Invalid;
> >>
> >> -	if (transform != Transform::Identity) {
> >> -		transform = Transform::Identity;
> >> +	int32_t rotation = data_->cio2_.sensor()->properties().get(properties::Rotation);
> >> +	bool success;
> >> +	Transform rotationTransform = transformFromRotation(rotation, &success);
> >
> > Can you cache this in CameraData ?
> > At registerCamera() time we initialize the properties so we can cache
> > the above 'rotationTransform' just after
> >
> > 		/* Initialize the camera properties. */
> > 		data->properties_ = cio2->sensor()->properties();
> >
> > Instead of re-calculating it every validation.
> >
>
> Done
>
> >> +	if (!success)
> >> +		LOG(IPU3, Warning) << "Invalid rotation of " << rotation << " degrees - ignoring";
> >> +	Transform combined = transform * rotationTransform;
> >> +
> >> +	/*
> >> +	 * We combine the platform and user transform, but must "adjust away"
> >> +	 * any combined result that includes a transform, as we can't do those.
> >> +	 * In this case, flipping only the transpose bit is helpful to
> >> +	 * applications - they either get the transform they requested, or have
> >> +	 * to do a simple transpose themselves (they don't have to worry about
> >> +	 * the other possible cases).
> >> +	 */
> >
> > The 'rotationTransform' obtained by inspecting the sensor rotation
> > does not contain  any Transpose, looking at
> > Transform::transformFromRotation().
> >
> > Can you address the transpose directly in the
> > CameraConfiguration::transpose field before multiplying it by
> > rotationTransform ?
> >
>
> I copied the code from the raspberry pipeline (see
> src/libcamera/pipeline/raspberrypi/raspberrypi.cpp line 287 ff.) and
> assumed that this is the correct way to handle transpositions.
>
> Should I just flip the transpose bit of CameraConfiguration::transpose
> and then combine it with the sensor rotation?
>
> And if so should I send a patch for the raspberry pipeline as well?
>

Ok, didn't know it came from there. I think it's a minor
optimization, and changing it in two places might delay the acceptance
of this patch (and require you more work). If the two implementations are
algigned, they can be changed in one go later.

With the other comments addressed
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
  j

> >> +	if (!!(combined & Transform::Transpose)) {
> >> +		/*
> >> +		 * Flipping the transpose bit in "transform" flips it in the
> >> +		 * combined result too (as it's the last thing that happens),
> >> +		 * which is of course clearing it.
> >> +		 */
> >> +		transform ^= Transform::Transpose;
> >> +		combined &= ~Transform::Transpose;
> >>  		status = Adjusted;
> >>  	}
> >>
> >> +	/*
> >> +	 * We also check if the sensor doesn't do h/vflips at all, in which
> >> +	 * case we clear them, ad the application will have to do everything.
> >
> > s/ad/and
> >
>
> Done
>
> >> +	 */
> >> +	if (!data_->supportsFlips_ && !!combined) {
> >> +		/*
> >> +		 * If the sensor can do no transforms, then combined must be
> >> +		 * changed to the identity. The only user transform that gives
> >> +		 * rise to this the inverse of the rotation. (Recall that
> >
> > s/this the/this is the/
> >
>
> Done
>
> >> +		 * combined = transform * rotationTransform.)
> >> +		 */
> >> +		transform = -rotationTransform;
> >> +		combined = Transform::Identity;
> >> +		status = Adjusted;
> >> +	}
> >> +
> >> +	/*
> >> +	 * Store the final combined transform that configure() will need to
> >> +	 * apply to the sensor to save us working it out again.
> >> +	 */
> >> +	combinedTransform_ = combined;
> >> +
> >>  	/* Cap the number of entries to the available streams. */
> >>  	if (config_.size() > IPU3_MAX_STREAMS) {
> >>  		config_.resize(IPU3_MAX_STREAMS);
> >> @@ -540,6 +593,19 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)
> >>  		return ret;
> >>  	}
> >>
> >> +	/*
> >> +	 * Configure the H/V flip controls based on the combination of
> >> +	 * the sensor and user transform.
> >> +	 */
> >> +	if (data->supportsFlips_) {
> >> +		ctrls = cio2->sensor()->controls();
> >
> > I would not re-use ctrls, or rather we should probably:
> >
> > --- a/include/libcamera/controls.h
> > +++ b/include/libcamera/controls.h
> > @@ -352,7 +352,7 @@ private:
> >  public:
> >         ControlList();
> >         ControlList(const ControlIdMap &idmap, ControlValidator *validator = nullptr);
> > -       ControlList(const ControlInfoMap &infoMap, ControlValidator *validator = nullptr);
> > +       explicit ControlList(const ControlInfoMap &infoMap, ControlValidator *validator = nullptr);
> >
> > to avoid this assignment. But maybe it's just me
> >
>
> I created a sensor_ctrls variable to prevent the re-use.
>
> > Thanks
> >    j
> >
> >> +		ctrls.set(V4L2_CID_HFLIP,
> >> +			  static_cast<int32_t>(!!(config->combinedTransform_ & Transform::HFlip)));
> >> +		ctrls.set(V4L2_CID_VFLIP,
> >> +			  static_cast<int32_t>(!!(config->combinedTransform_ & Transform::VFlip)));
> >> +		cio2->sensor()->setControls(&ctrls);
> >> +	}
> >> +
> >>  	return 0;
> >>  }
> >>
> >> @@ -779,6 +845,12 @@ int PipelineHandlerIPU3::registerCameras()
> >>  		/* Initialze the camera controls. */
> >>  		data->controlInfo_ = IPU3Controls;
> >>
> >> +		ControlList ctrls = cio2->sensor()->getControls({ V4L2_CID_HFLIP });
> >> +		if (!ctrls.empty()) {
> >> +			/* We assume it will support vflips too... */
> >> +			data->supportsFlips_ = true;
> >> +		}
> >> +
> >>  		/**
> >>  		 * \todo Dynamically assign ImgU and output devices to each
> >>  		 * stream and camera; as of now, limit support to two cameras
> >> --
> >> 2.30.0
> >>
> >> _______________________________________________
> >> libcamera-devel mailing list
> >> libcamera-devel@lists.libcamera.org
> >> https://lists.libcamera.org/listinfo/libcamera-devel

Patch
diff mbox series

diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
index f1151733..76d75a74 100644
--- a/src/libcamera/pipeline/ipu3/ipu3.cpp
+++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
@@ -14,6 +14,7 @@ 
 #include <libcamera/camera.h>
 #include <libcamera/control_ids.h>
 #include <libcamera/formats.h>
+#include <libcamera/property_ids.h>
 #include <libcamera/request.h>
 #include <libcamera/stream.h>
 
@@ -62,6 +63,12 @@  public:
 	Stream outStream_;
 	Stream vfStream_;
 	Stream rawStream_;
+
+	/*
+	 * Manage horizontal and vertical flips supported (or not) by the
+	 * sensor.
+	 */
+	bool supportsFlips_;
 };
 
 class IPU3CameraConfiguration : public CameraConfiguration
@@ -74,6 +81,9 @@  public:
 	const StreamConfiguration &cio2Format() const { return cio2Configuration_; }
 	const ImgUDevice::PipeConfig imguConfig() const { return pipeConfig_; }
 
+	/* Cache the combinedTransform_ that will be applied to the sensor */
+	Transform combinedTransform_;
+
 private:
 	/*
 	 * The IPU3CameraData instance is guaranteed to be valid as long as the
@@ -143,11 +153,54 @@  CameraConfiguration::Status IPU3CameraConfiguration::validate()
 	if (config_.empty())
 		return Invalid;
 
-	if (transform != Transform::Identity) {
-		transform = Transform::Identity;
+	int32_t rotation = data_->cio2_.sensor()->properties().get(properties::Rotation);
+	bool success;
+	Transform rotationTransform = transformFromRotation(rotation, &success);
+	if (!success)
+		LOG(IPU3, Warning) << "Invalid rotation of " << rotation << " degrees - ignoring";
+	Transform combined = transform * rotationTransform;
+
+	/*
+	 * We combine the platform and user transform, but must "adjust away"
+	 * any combined result that includes a transform, as we can't do those.
+	 * In this case, flipping only the transpose bit is helpful to
+	 * applications - they either get the transform they requested, or have
+	 * to do a simple transpose themselves (they don't have to worry about
+	 * the other possible cases).
+	 */
+	if (!!(combined & Transform::Transpose)) {
+		/*
+		 * Flipping the transpose bit in "transform" flips it in the
+		 * combined result too (as it's the last thing that happens),
+		 * which is of course clearing it.
+		 */
+		transform ^= Transform::Transpose;
+		combined &= ~Transform::Transpose;
 		status = Adjusted;
 	}
 
+	/*
+	 * We also check if the sensor doesn't do h/vflips at all, in which
+	 * case we clear them, ad the application will have to do everything.
+	 */
+	if (!data_->supportsFlips_ && !!combined) {
+		/*
+		 * If the sensor can do no transforms, then combined must be
+		 * changed to the identity. The only user transform that gives
+		 * rise to this the inverse of the rotation. (Recall that
+		 * combined = transform * rotationTransform.)
+		 */
+		transform = -rotationTransform;
+		combined = Transform::Identity;
+		status = Adjusted;
+	}
+
+	/*
+	 * Store the final combined transform that configure() will need to
+	 * apply to the sensor to save us working it out again.
+	 */
+	combinedTransform_ = combined;
+
 	/* Cap the number of entries to the available streams. */
 	if (config_.size() > IPU3_MAX_STREAMS) {
 		config_.resize(IPU3_MAX_STREAMS);
@@ -540,6 +593,19 @@  int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)
 		return ret;
 	}
 
+	/*
+	 * Configure the H/V flip controls based on the combination of
+	 * the sensor and user transform.
+	 */
+	if (data->supportsFlips_) {
+		ctrls = cio2->sensor()->controls();
+		ctrls.set(V4L2_CID_HFLIP,
+			  static_cast<int32_t>(!!(config->combinedTransform_ & Transform::HFlip)));
+		ctrls.set(V4L2_CID_VFLIP,
+			  static_cast<int32_t>(!!(config->combinedTransform_ & Transform::VFlip)));
+		cio2->sensor()->setControls(&ctrls);
+	}
+
 	return 0;
 }
 
@@ -779,6 +845,12 @@  int PipelineHandlerIPU3::registerCameras()
 		/* Initialze the camera controls. */
 		data->controlInfo_ = IPU3Controls;
 
+		ControlList ctrls = cio2->sensor()->getControls({ V4L2_CID_HFLIP });
+		if (!ctrls.empty()) {
+			/* We assume it will support vflips too... */
+			data->supportsFlips_ = true;
+		}
+
 		/**
 		 * \todo Dynamically assign ImgU and output devices to each
 		 * stream and camera; as of now, limit support to two cameras