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

[Kieran Bingham]

Hi Fabian,

On 22/01/2021 12:01, 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.

As far as I can see, this doesn't yet support setting the rotation as a
control from an application? But I don't think we should require that of
this patch.

I believe getting this rotation support in will help progress the topic,
but I'm weary that we might need to consider later how applications
determine if we have or haven't rotated in the pipeline.

But I think we can handle that later when we hit more directly.

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

I believe Jacopo's tag has carried forwards on this too? If so - I'll
let Jacopo handle merging.

--
Kieran


> 
> Signed-off-by: Fabian Wüthrich <me at fabwu.ch>
> ---
> Changes in v2:
>  - Cache rotationTransform in CameraData
>  - Use separate controls for sensor
> 
> Changes in v3:
>  - Default rotation to 0 if sensor doesn't expose it
> 
>  src/libcamera/pipeline/ipu3/ipu3.cpp | 90 +++++++++++++++++++++++++++-
>  1 file changed, 88 insertions(+), 2 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index 73304ea7..d620acf3 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,31 @@ int PipelineHandlerIPU3::registerCameras()
>  		/* Initialize the camera properties. */
>  		data->properties_ = cio2->sensor()->properties();
>  
> +		/* Convert the sensor rotation to a transformation */
> +		int32_t rotation;
> +		if (data->properties_.contains(properties::Rotation)) {
> +			rotation = data->properties_.get(properties::Rotation);
> +		} else {
> +			/* If sensor driver doesn't expose rotation, default rotation to 0 */
> +			rotation = 0;
> +			LOG(IPU3, Warning) << "Rotation control not exposed by " << cio2->sensor()->id()
> +					   << ". Assume rotation 0.";
> +		}
> +
> +		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
> 

-- 
Regards
--
Kieran