[libcamera-devel] [PATCH v2] libcamera: ipu3: Add rotation to ipu3 pipeline

[Fabian Wüthrich]

Hi Laurent,

On 22.01.21 00:37, Laurent Pinchart wrote:
> Hi Jacopo and Fabian,
> 
> On Mon, Jan 18, 2021 at 11:06:31AM +0100, Jacopo Mondi wrote:
>> On Sat, Jan 16, 2021 at 04:02:58PM +0100, Fabian Wüthrich wrote:
>>> Use the same transformation logic as in the raspberry pipeline to
>>> implement rotations in the ipu3 pipeline.
> 
> This should eventually be shared between different pipeline handlers,
> but for now it's fine.
> 

Agreed.

>>> Tested on a Surface Book 2 with an experimental driver for OV5693.
>>>
>>> Signed-off-by: Fabian Wüthrich <me at fabwu.ch>
>>> ---
>>> Changes in v2:
>>>  - Cache rotationTransform in CameraData
>>>  - Use separate controls for sensor
>>>
>>>  src/libcamera/pipeline/ipu3/ipu3.cpp | 81 +++++++++++++++++++++++++++-
>>>  1 file changed, 79 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
>>> index 73304ea7..8db5f2f1 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,15 @@ public:
>>>  	Stream outStream_;
>>>  	Stream vfStream_;
>>>  	Stream rawStream_;
>>> +
>>> +	/* Save transformation given by the sensor rotation */
>>> +	Transform rotationTransform_;
>>> +
>>> +	/*
>>> +	 * Manage horizontal and vertical flips supported (or not) by the
>>> +	 * sensor.
>>> +	 */
>>> +	bool supportsFlips_;
>>>  };
>>>
>>>  class IPU3CameraConfiguration : public CameraConfiguration
>>> @@ -74,6 +84,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 +156,49 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()
>>>  	if (config_.empty())
>>>  		return Invalid;
>>>
>>> -	if (transform != Transform::Identity) {
>>> -		transform = Transform::Identity;
>>> +	Transform combined = transform * data_->rotationTransform_;
>>> +
>>> +	/*
>>> +	 * We combine the platform and user transform, but must "adjust away"
>>> +	 * any combined result that includes a transposition, 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, and 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 is the inverse of the rotation. (Recall that
>>> +		 * combined = transform * rotationTransform.)
>>> +		 */
>>> +		transform = -data_->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 +591,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_) {
>>> +		ControlList sensor_ctrls(cio2->sensor()->controls());
>>> +		sensor_ctrls.set(V4L2_CID_HFLIP,
>>> +				 static_cast<int32_t>(!!(config->combinedTransform_ & Transform::HFlip)));
>>> +		sensor_ctrls.set(V4L2_CID_VFLIP,
>>> +				 static_cast<int32_t>(!!(config->combinedTransform_ & Transform::VFlip)));
>>> +		cio2->sensor()->setControls(&sensor_ctrls);
>>> +	}
>>> +
>>>  	return 0;
>>>  }
>>>
>>> @@ -775,9 +839,22 @@ int PipelineHandlerIPU3::registerCameras()
>>>  		/* Initialize the camera properties. */
>>>  		data->properties_ = cio2->sensor()->properties();
>>>
>>> +		/* Convert the sensor rotation to a transformation */
>>> +		int32_t rotation = data->properties_.get(properties::Rotation);
>>
>> There's no guarantee the CameraSensor class register the Rotation
>> property. It was defaulted to 0 if the information was not available
>> from the firmware interface.
>>
>> It is now not anymore (a very recent change, sorry)
>> https://git.libcamera.org/libcamera/libcamera.git/commit/?id=c35123e6f91b6269cf6f5ae9216aeccebc545c75
>>
>> Not sure what would be best, default to 0 in each pipeline handler
>> that need that information ?
> 
> That sounds good to me for now.
> 

Ok, thanks for checking. Should I provide a patch for the raspberry pipeline as well?

>>> +		bool success;
>>> +		data->rotationTransform_ = transformFromRotation(rotation, &success);
>>> +		if (!success)
>>> +			LOG(IPU3, Warning) << "Invalid rotation of " << rotation << " degrees - ignoring";
>>> +
>>>  		/* 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
>