[libcamera-devel] [PATCH v3] libcamera: ipu3: Add rotation to ipu3 pipeline
Fabian Wüthrich
me at fabwu.ch
Tue Feb 2 09:13:15 CET 2021
Hi Kieran,
Thanks for your review.
On 01.02.21 18:26, Kieran Bingham wrote:
> 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 think this line should allow applications to apply a rotation but I haven't
tested it yet.
>> - if (transform != Transform::Identity) {
>> - transform = Transform::Identity;
>> + Transform combined = transform * data_->rotationTransform_;
> 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.
>
Agree. The purpose of this patch is to make rotations work accross the whole
stack (ACPI SSDB buffer -> fwnodes -> V4L2 controls -> libcamera).
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>
Thanks for the tag :)
> I believe Jacopo's tag has carried forwards on this too? If so - I'll
> let Jacopo handle merging.
>
While rebasing my changes, I saw that a new method `initControls()` was added.
Should I move this part into `initControls()`?
>> @@ -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
>>
Best,
Fabian
> --
> 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
>>
>
More information about the libcamera-devel
mailing list