| Message ID | 20210122120132.3717-1-me@fabwu.ch |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
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@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@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 >
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@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@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 >> >
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@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@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 > >> > >
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
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. Signed-off-by: Fabian Wüthrich <me@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(-)