| Message ID | 20210116150257.2970-1-me@fabwu.ch |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Fabian, 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. > > 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 > > 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 ? > + 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 > -- > 2.30.0 >
Hi Jacopo, I've just got the message that my driver needs to be fixed ^^ I like this change as it transparently states the requirements. On 18.01.21 11:06, Jacopo Mondi wrote: > Hi Fabian, > > 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. >> >> 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 >> >> 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 ? > Yes I can do that. Should I post a patch for the raspberry pipeline as well? >> + 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 >> -- >> 2.30.0 >>
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. > > 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 > > > > 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. > > + 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 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@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? >>> + 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, + 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@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 > >
Hi Fabian, Jacopo, Naush, David ;-) On 22/01/2021 08:52, Jacopo Mondi wrote: > Hi Fabian, > + Naush for RPi question Fixing the +CC for the question below - but also adding David as he wrote the original Transforms. -- Kieran > > 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@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 >>> > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel >
Hi all, On Fri, 22 Jan 2021 at 12:20, Kieran Bingham < kieran.bingham@ideasonboard.com> wrote: > Hi Fabian, Jacopo, Naush, David ;-) > > On 22/01/2021 08:52, Jacopo Mondi wrote: > > Hi Fabian, > > + Naush for RPi question > > Fixing the +CC for the question below - but also adding David as he > wrote the original Transforms. > > -- > Kieran > > > > > > 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@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. > I think we should be ok with defaulting to 0 if the rotation property is unavailable. Worst that would happen is that we would produce an inverted picture - and then the user knows to add rotation in the driver :-) I'll let David have the final say though. Regards, Naush > > > >> > >>>>> + 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 > >>> > > _______________________________________________ > > libcamera-devel mailing list > > libcamera-devel@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel > > > > -- > Regards > -- > Kieran >
Hi everyone On Fri, 22 Jan 2021 at 13:07, Naushir Patuck <naush@raspberrypi.com> wrote: > > Hi all, > > > On Fri, 22 Jan 2021 at 12:20, Kieran Bingham <kieran.bingham@ideasonboard.com> wrote: >> >> Hi Fabian, Jacopo, Naush, David ;-) >> >> On 22/01/2021 08:52, Jacopo Mondi wrote: >> > Hi Fabian, >> > + Naush for RPi question >> >> Fixing the +CC for the question below - but also adding David as he >> wrote the original Transforms. >> >> -- >> Kieran >> >> >> > >> > 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@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. > > > I think we should be ok with defaulting to 0 if the rotation property is unavailable. Worst that would happen is that we would produce an inverted picture - and then the user knows to add rotation in the driver :-) > I'll let David have the final say though. Sounds fine to me too! Thanks David > > Regards, > Naush > >> >> > >> >> >> >>>>> + 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 >> >>> >> > _______________________________________________ >> > libcamera-devel mailing list >> > libcamera-devel@lists.libcamera.org >> > https://lists.libcamera.org/listinfo/libcamera-devel >> > >> >> -- >> Regards >> -- >> Kieran
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); + 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 src/libcamera/pipeline/ipu3/ipu3.cpp | 81 +++++++++++++++++++++++++++- 1 file changed, 79 insertions(+), 2 deletions(-)