[libcamera-devel] [PATCH v2] libcamera: ipu3: Add rotation to ipu3 pipeline
Jacopo Mondi
jacopo at jmondi.org
Fri Jan 22 09:52:10 CET 2021
Hi Fabian,
+ Naush for RPi question
On Fri, Jan 22, 2021 at 09:10:42AM +0100, Fabian Wüthrich wrote:
> 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?
I would not mind, but feel free to stick to IPU3 if that's quicker.
Cc Naush for RPi. I'm thinking that, being RPi a bit more 'strict' as
platform and with a little more control over sensor drivers/fw, maybe
they want to refuse setups where no rotation is specified.
>
> >>> + 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