[libcamera-devel] [PATCH v3] libcamera: ipu3: Add rotation to ipu3 pipeline
Jacopo Mondi
jacopo at jmondi.org
Tue Feb 2 18:56:58 CET 2021
Hi Fabian,
On Tue, Feb 02, 2021 at 09:13:15AM +0100, Fabian Wüthrich wrote:
> 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()`?
Yes please, sorry about this, there has been quite some churn in the ipu3.cpp
file recently.
I tried re-basing your patch on the latest master and some older
version to rebase locally, but it doesn't cleanly apply. Could you
please rebase and re-send (and while at there collect the two small
comments I added here) ?
Thanks
j
>
> >> @@ -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