| Message ID | 20260805101207.1177471-1-stefan.klug@ideasonboard.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series |
|
| Related | show |
Hi Stefan On Wed, Aug 05, 2026 at 12:12:03PM +0200, Stefan Klug wrote: > CameraSensorLegacy and CameraSensorRaw support flipping on the sensor > side only if the driver supports both, V4L2_CID_HFLIP and > V4L2_CID_VFLIP. In cases where a driver only supports a single flip > mode, flipping support gets disabled completely. This is unexpected and > annoying when validating sensor drivers. > > Fix that by handling vertical and horizontal flips separately. > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> > > --- > > Changes in v3: > - Swapped if and replaced ternary operator by || in support logic > - Improved description in sensor_driver_requirements > > Changes in v2: > - Replaced duplicate query flip code with lambda > - Reworked apply flip code to call setControls only once > - Replaced a & by && > - Applied the change to CameraSensorRaw as well > - Small comment/logging changes > --- > > Hi all, > > I had this patch somewhere in my tree for a long time. Now I stumbled > over this issue again and had to manually rebase the changes. So maybe > it is time to upstream :-) > > Version 3 adresses the comments from last review. > > Best regards, > Stefan > > Documentation/sensor_driver_requirements.rst | 2 + > src/libcamera/sensor/camera_sensor_legacy.cpp | 76 ++++++++++-------- > src/libcamera/sensor/camera_sensor_raw.cpp | 79 +++++++++++-------- > 3 files changed, 90 insertions(+), 67 deletions(-) > > diff --git a/Documentation/sensor_driver_requirements.rst b/Documentation/sensor_driver_requirements.rst > index 0e516b34a215..1caa2f96d682 100644 > --- a/Documentation/sensor_driver_requirements.rst > +++ b/Documentation/sensor_driver_requirements.rst > @@ -73,6 +73,8 @@ In order to support rotating the image the sensor driver should support > The controls must be writable from userspace. In case of a RAW Bayer sensors, > drivers should correctly report if vertical/horizontal flips modify the Bayer > pattern ordering by reporting the `V4L2_CTRL_FLAG_MODIFY_LAYOUT` control flag. > +If the flag is set for one of the two controls, it shall be set for the other > +one as well. > > The sensor driver should implement support for the V4L2 Selection API, > specifically it should implement support for the > diff --git a/src/libcamera/sensor/camera_sensor_legacy.cpp b/src/libcamera/sensor/camera_sensor_legacy.cpp > index 6a683821f219..751d6f3e1c04 100644 > --- a/src/libcamera/sensor/camera_sensor_legacy.cpp > +++ b/src/libcamera/sensor/camera_sensor_legacy.cpp > @@ -133,7 +133,8 @@ private: > Size pixelArraySize_; > Rectangle activeArea_; > const BayerFormat *bayerFormat_; > - bool supportFlips_; > + bool supportHFlips_; > + bool supportVFlips_; > bool flipsAlterBayerOrder_; > Orientation mountingOrientation_; > > @@ -153,7 +154,7 @@ private: > > CameraSensorLegacy::CameraSensorLegacy(const MediaEntity *entity) > : entity_(entity), pad_(UINT_MAX), staticProps_(nullptr), > - bayerFormat_(nullptr), supportFlips_(false), > + bayerFormat_(nullptr), supportHFlips_(false), supportVFlips_(false), > flipsAlterBayerOrder_(false), properties_(properties::properties) > { > } > @@ -359,26 +360,31 @@ int CameraSensorLegacy::validateSensorDriver() > } > } > > - /* > - * Verify if sensor supports horizontal/vertical flips > - * > - * \todo Handle horizontal and vertical flips independently. > - */ > - const struct v4l2_query_ext_ctrl *hflipInfo = subdev_->controlInfo(V4L2_CID_HFLIP); > - const struct v4l2_query_ext_ctrl *vflipInfo = subdev_->controlInfo(V4L2_CID_VFLIP); > - if (hflipInfo && !(hflipInfo->flags & V4L2_CTRL_FLAG_READ_ONLY) && > - vflipInfo && !(vflipInfo->flags & V4L2_CTRL_FLAG_READ_ONLY)) { > - supportFlips_ = true; > + /* Verify if sensor supports horizontal/vertical flips. */ > + auto queryFlip = [&](uint32_t id, const char *name) -> std::pair<bool, bool> { > + const struct v4l2_query_ext_ctrl *flipInfo = subdev_->controlInfo(id); > + if (!flipInfo || flipInfo->flags & V4L2_CTRL_FLAG_READ_ONLY) { > + LOG(CameraSensor, Debug) > + << "Camera sensor does not support " << name << " flip"; > + return { false, false }; > + } > > - if (hflipInfo->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT || > - vflipInfo->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT) > - flipsAlterBayerOrder_ = true; > + return { true, flipInfo->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT }; > + }; > + > + auto hflip = queryFlip(V4L2_CID_HFLIP, "horizontal"); > + auto vflip = queryFlip(V4L2_CID_HFLIP, "vertical"); > + > + if (hflip.first && vflip.first && (hflip.second != vflip.second)) { > + LOG(CameraSensor, Error) > + << "Flips with differing alter bayer order properties" > + << " are unsupported. Disabling flipping."; > + } else { > + supportHFlips_ = hflip.first; > + supportVFlips_ = vflip.first; > + flipsAlterBayerOrder_ = hflip.second || vflip.second; > } > > - if (!supportFlips_) > - LOG(CameraSensor, Debug) > - << "Camera sensor does not support horizontal/vertical flip"; > - > /* > * Make sure the required selection targets are supported. > * > @@ -759,14 +765,16 @@ CameraSensorLegacy::getFormat(Span<const unsigned int> mbusCodes, > int CameraSensorLegacy::setFormat(V4L2SubdeviceFormat *format, Transform transform) > { > /* Configure flips if the sensor supports that. */ > - if (supportFlips_) { > - ControlList flipCtrls(subdev_->controls()); > - > + ControlList flipCtrls(subdev_->controls()); > + if (supportHFlips_) > flipCtrls.set(V4L2_CID_HFLIP, > static_cast<int32_t>(!!(transform & Transform::HFlip))); > + > + if (supportVFlips_) These checks should not be needed, as computeTransform() won't set the Transform::HFlip or VFlip flags in 'transform'. Better safe than sorry Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > flipCtrls.set(V4L2_CID_VFLIP, > static_cast<int32_t>(!!(transform & Transform::VFlip))); > > + if (!flipCtrls.empty()) { > int ret = subdev_->setControls(&flipCtrls); > if (ret) > return ret; > @@ -937,17 +945,8 @@ int CameraSensorLegacy::sensorInfo(IPACameraSensorInfo *info) const > Transform CameraSensorLegacy::computeTransform(Orientation *orientation) const > { > /* > - * If we cannot do any flips we cannot change the native camera mounting > - * orientation. > - */ > - if (!supportFlips_) { > - *orientation = mountingOrientation_; > - return Transform::Identity; > - } > - > - /* > - * Now compute the required transform to obtain 'orientation' starting > - * from the mounting rotation. > + * Compute the required transform to obtain 'orientation' starting from > + * the mounting rotation. > * > * As a note: > * orientation / mountingOrientation_ = transform > @@ -955,6 +954,17 @@ Transform CameraSensorLegacy::computeTransform(Orientation *orientation) const > */ > Transform transform = *orientation / mountingOrientation_; > > + /* If we cannot do the required flips, fall back to identity. */ > + if (!supportHFlips_ && !!(transform & Transform::HFlip)) { > + *orientation = mountingOrientation_; > + return Transform::Identity; > + } > + > + if (!supportVFlips_ && !!(transform & Transform::VFlip)) { > + *orientation = mountingOrientation_; > + return Transform::Identity; > + } > + > /* > * If transform contains any Transpose we cannot do it, so adjust > * 'orientation' to report the image native orientation and return Identity. > diff --git a/src/libcamera/sensor/camera_sensor_raw.cpp b/src/libcamera/sensor/camera_sensor_raw.cpp > index 10eba0331fe8..52718bf0c3f5 100644 > --- a/src/libcamera/sensor/camera_sensor_raw.cpp > +++ b/src/libcamera/sensor/camera_sensor_raw.cpp > @@ -144,7 +144,8 @@ private: > Size pixelArraySize_; > Rectangle activeArea_; > BayerFormat::Order cfaPattern_; > - bool supportFlips_; > + bool supportHFlips_; > + bool supportVFlips_; > bool flipsAlterBayerOrder_; > Orientation mountingOrientation_; > > @@ -162,8 +163,9 @@ private: > */ > > CameraSensorRaw::CameraSensorRaw(const MediaEntity *entity) > - : entity_(entity), staticProps_(nullptr), supportFlips_(false), > - flipsAlterBayerOrder_(false), properties_(properties::properties) > + : entity_(entity), staticProps_(nullptr), supportHFlips_(false), > + supportVFlips_(false), flipsAlterBayerOrder_(false), > + properties_(properties::properties) > { > } > > @@ -479,26 +481,31 @@ std::optional<int> CameraSensorRaw::init() > return { ret }; > } > > - /* > - * Verify if sensor supports horizontal/vertical flips > - * > - * \todo Handle horizontal and vertical flips independently. > - */ > - const struct v4l2_query_ext_ctrl *hflipInfo = subdev_->controlInfo(V4L2_CID_HFLIP); > - const struct v4l2_query_ext_ctrl *vflipInfo = subdev_->controlInfo(V4L2_CID_VFLIP); > - if (hflipInfo && !(hflipInfo->flags & V4L2_CTRL_FLAG_READ_ONLY) && > - vflipInfo && !(vflipInfo->flags & V4L2_CTRL_FLAG_READ_ONLY)) { > - supportFlips_ = true; > + /* Verify if sensor supports horizontal/vertical flips. */ > + auto queryFlip = [&](uint32_t id, const char *name) -> std::pair<bool, bool> { > + const struct v4l2_query_ext_ctrl *flipInfo = subdev_->controlInfo(id); > + if (!flipInfo || flipInfo->flags & V4L2_CTRL_FLAG_READ_ONLY) { > + LOG(CameraSensor, Debug) > + << "Camera sensor does not support " << name << " flip"; > + return { false, false }; > + } > > - if (hflipInfo->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT || > - vflipInfo->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT) > - flipsAlterBayerOrder_ = true; > + return { true, flipInfo->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT }; > + }; > + > + auto hflip = queryFlip(V4L2_CID_HFLIP, "horizontal"); > + auto vflip = queryFlip(V4L2_CID_HFLIP, "vertical"); > + > + if (hflip.first && vflip.first && (hflip.second != vflip.second)) { > + LOG(CameraSensor, Error) > + << "Flips with differing alter bayer order properties" > + << " are unsupported. Disabling flipping."; > + } else { > + supportHFlips_ = hflip.first; > + supportVFlips_ = vflip.first; > + flipsAlterBayerOrder_ = hflip.second || vflip.second; > } > > - if (!supportFlips_) > - LOG(CameraSensor, Debug) > - << "Camera sensor does not support horizontal/vertical flip"; > - > /* > * 5. Discover ancillary devices. > * > @@ -819,14 +826,16 @@ CameraSensorRaw::getFormat(Span<const unsigned int> mbusCodes, > int CameraSensorRaw::setFormat(V4L2SubdeviceFormat *format, Transform transform) > { > /* Configure flips if the sensor supports that. */ > - if (supportFlips_) { > - ControlList flipCtrls(subdev_->controls()); > - > + ControlList flipCtrls(subdev_->controls()); > + if (supportHFlips_) > flipCtrls.set(V4L2_CID_HFLIP, > static_cast<int32_t>(!!(transform & Transform::HFlip))); > + > + if (supportVFlips_) > flipCtrls.set(V4L2_CID_VFLIP, > static_cast<int32_t>(!!(transform & Transform::VFlip))); > > + if (!flipCtrls.empty()) { > int ret = subdev_->setControls(&flipCtrls); > if (ret) > return ret; > @@ -1054,17 +1063,8 @@ int CameraSensorRaw::sensorInfo(IPACameraSensorInfo *info) const > Transform CameraSensorRaw::computeTransform(Orientation *orientation) const > { > /* > - * If we cannot do any flips we cannot change the native camera mounting > - * orientation. > - */ > - if (!supportFlips_) { > - *orientation = mountingOrientation_; > - return Transform::Identity; > - } > - > - /* > - * Now compute the required transform to obtain 'orientation' starting > - * from the mounting rotation. > + * Compute the required transform to obtain 'orientation' starting from > + * the mounting rotation. > * > * As a note: > * orientation / mountingOrientation_ = transform > @@ -1072,6 +1072,17 @@ Transform CameraSensorRaw::computeTransform(Orientation *orientation) const > */ > Transform transform = *orientation / mountingOrientation_; > > + /* If we cannot do the required flips, fall back to identity. */ > + if (!supportHFlips_ && !!(transform & Transform::HFlip)) { > + *orientation = mountingOrientation_; > + return Transform::Identity; > + } > + > + if (!supportVFlips_ && !!(transform & Transform::VFlip)) { > + *orientation = mountingOrientation_; > + return Transform::Identity; > + } > + > /* > * If transform contains any Transpose we cannot do it, so adjust > * 'orientation' to report the image native orientation and return Identity. > -- > 2.53.0 >
2026. 08. 05. 14:21 keltezéssel, Jacopo Mondi írta: > Hi Stefan > > On Wed, Aug 05, 2026 at 12:12:03PM +0200, Stefan Klug wrote: >> CameraSensorLegacy and CameraSensorRaw support flipping on the sensor >> side only if the driver supports both, V4L2_CID_HFLIP and >> V4L2_CID_VFLIP. In cases where a driver only supports a single flip >> mode, flipping support gets disabled completely. This is unexpected and >> annoying when validating sensor drivers. >> >> Fix that by handling vertical and horizontal flips separately. >> >> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> >> >> --- >> >> Changes in v3: >> - Swapped if and replaced ternary operator by || in support logic >> - Improved description in sensor_driver_requirements >> >> Changes in v2: >> - Replaced duplicate query flip code with lambda >> - Reworked apply flip code to call setControls only once >> - Replaced a & by && >> - Applied the change to CameraSensorRaw as well >> - Small comment/logging changes >> --- >> >> Hi all, >> >> I had this patch somewhere in my tree for a long time. Now I stumbled >> over this issue again and had to manually rebase the changes. So maybe >> it is time to upstream :-) >> >> Version 3 adresses the comments from last review. >> >> Best regards, >> Stefan >> >> Documentation/sensor_driver_requirements.rst | 2 + >> src/libcamera/sensor/camera_sensor_legacy.cpp | 76 ++++++++++-------- >> src/libcamera/sensor/camera_sensor_raw.cpp | 79 +++++++++++-------- >> 3 files changed, 90 insertions(+), 67 deletions(-) >> >> diff --git a/Documentation/sensor_driver_requirements.rst b/Documentation/sensor_driver_requirements.rst >> index 0e516b34a215..1caa2f96d682 100644 >> --- a/Documentation/sensor_driver_requirements.rst >> +++ b/Documentation/sensor_driver_requirements.rst >> @@ -73,6 +73,8 @@ In order to support rotating the image the sensor driver should support >> The controls must be writable from userspace. In case of a RAW Bayer sensors, >> drivers should correctly report if vertical/horizontal flips modify the Bayer >> pattern ordering by reporting the `V4L2_CTRL_FLAG_MODIFY_LAYOUT` control flag. >> +If the flag is set for one of the two controls, it shall be set for the other >> +one as well. >> >> The sensor driver should implement support for the V4L2 Selection API, >> specifically it should implement support for the >> diff --git a/src/libcamera/sensor/camera_sensor_legacy.cpp b/src/libcamera/sensor/camera_sensor_legacy.cpp >> index 6a683821f219..751d6f3e1c04 100644 >> --- a/src/libcamera/sensor/camera_sensor_legacy.cpp >> +++ b/src/libcamera/sensor/camera_sensor_legacy.cpp >> @@ -133,7 +133,8 @@ private: >> Size pixelArraySize_; >> Rectangle activeArea_; >> const BayerFormat *bayerFormat_; >> - bool supportFlips_; >> + bool supportHFlips_; >> + bool supportVFlips_; >> bool flipsAlterBayerOrder_; >> Orientation mountingOrientation_; >> >> @@ -153,7 +154,7 @@ private: >> >> CameraSensorLegacy::CameraSensorLegacy(const MediaEntity *entity) >> : entity_(entity), pad_(UINT_MAX), staticProps_(nullptr), >> - bayerFormat_(nullptr), supportFlips_(false), >> + bayerFormat_(nullptr), supportHFlips_(false), supportVFlips_(false), >> flipsAlterBayerOrder_(false), properties_(properties::properties) >> { >> } >> @@ -359,26 +360,31 @@ int CameraSensorLegacy::validateSensorDriver() >> } >> } >> >> - /* >> - * Verify if sensor supports horizontal/vertical flips >> - * >> - * \todo Handle horizontal and vertical flips independently. >> - */ >> - const struct v4l2_query_ext_ctrl *hflipInfo = subdev_->controlInfo(V4L2_CID_HFLIP); >> - const struct v4l2_query_ext_ctrl *vflipInfo = subdev_->controlInfo(V4L2_CID_VFLIP); >> - if (hflipInfo && !(hflipInfo->flags & V4L2_CTRL_FLAG_READ_ONLY) && >> - vflipInfo && !(vflipInfo->flags & V4L2_CTRL_FLAG_READ_ONLY)) { >> - supportFlips_ = true; >> + /* Verify if sensor supports horizontal/vertical flips. */ >> + auto queryFlip = [&](uint32_t id, const char *name) -> std::pair<bool, bool> { >> + const struct v4l2_query_ext_ctrl *flipInfo = subdev_->controlInfo(id); >> + if (!flipInfo || flipInfo->flags & V4L2_CTRL_FLAG_READ_ONLY) { >> + LOG(CameraSensor, Debug) >> + << "Camera sensor does not support " << name << " flip"; >> + return { false, false }; >> + } >> >> - if (hflipInfo->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT || >> - vflipInfo->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT) >> - flipsAlterBayerOrder_ = true; >> + return { true, flipInfo->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT }; >> + }; >> + >> + auto hflip = queryFlip(V4L2_CID_HFLIP, "horizontal"); >> + auto vflip = queryFlip(V4L2_CID_HFLIP, "vertical"); V4L2_CID_VFLIP >> + >> + if (hflip.first && vflip.first && (hflip.second != vflip.second)) { >> + LOG(CameraSensor, Error) >> + << "Flips with differing alter bayer order properties" >> + << " are unsupported. Disabling flipping."; >> + } else { >> + supportHFlips_ = hflip.first; >> + supportVFlips_ = vflip.first; >> + flipsAlterBayerOrder_ = hflip.second || vflip.second; >> } >> >> - if (!supportFlips_) >> - LOG(CameraSensor, Debug) >> - << "Camera sensor does not support horizontal/vertical flip"; >> - >> /* >> * Make sure the required selection targets are supported. >> * >> @@ -759,14 +765,16 @@ CameraSensorLegacy::getFormat(Span<const unsigned int> mbusCodes, >> int CameraSensorLegacy::setFormat(V4L2SubdeviceFormat *format, Transform transform) >> { >> /* Configure flips if the sensor supports that. */ >> - if (supportFlips_) { >> - ControlList flipCtrls(subdev_->controls()); >> - >> + ControlList flipCtrls(subdev_->controls()); >> + if (supportHFlips_) >> flipCtrls.set(V4L2_CID_HFLIP, >> static_cast<int32_t>(!!(transform & Transform::HFlip))); >> + >> + if (supportVFlips_) > > These checks should not be needed, as computeTransform() won't set > the Transform::HFlip or VFlip flags in 'transform'. I think it's needed because if flips are supported and e.g. `!(transform & Transform::HFlip)` then `V4L2_CID_HFLIP` must be set to false in case it was enabled earlier, right? > > Better safe than sorry > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > >> flipCtrls.set(V4L2_CID_VFLIP, >> static_cast<int32_t>(!!(transform & Transform::VFlip))); >> >> + if (!flipCtrls.empty()) { >> int ret = subdev_->setControls(&flipCtrls); >> if (ret) >> return ret; >> @@ -937,17 +945,8 @@ int CameraSensorLegacy::sensorInfo(IPACameraSensorInfo *info) const >> Transform CameraSensorLegacy::computeTransform(Orientation *orientation) const >> { >> /* >> - * If we cannot do any flips we cannot change the native camera mounting >> - * orientation. >> - */ >> - if (!supportFlips_) { >> - *orientation = mountingOrientation_; >> - return Transform::Identity; >> - } >> - >> - /* >> - * Now compute the required transform to obtain 'orientation' starting >> - * from the mounting rotation. >> + * Compute the required transform to obtain 'orientation' starting from >> + * the mounting rotation. >> * >> * As a note: >> * orientation / mountingOrientation_ = transform >> @@ -955,6 +954,17 @@ Transform CameraSensorLegacy::computeTransform(Orientation *orientation) const >> */ >> Transform transform = *orientation / mountingOrientation_; >> >> + /* If we cannot do the required flips, fall back to identity. */ >> + if (!supportHFlips_ && !!(transform & Transform::HFlip)) { >> + *orientation = mountingOrientation_; >> + return Transform::Identity; >> + } >> + >> + if (!supportVFlips_ && !!(transform & Transform::VFlip)) { >> + *orientation = mountingOrientation_; >> + return Transform::Identity; >> + } >> + >> /* >> * If transform contains any Transpose we cannot do it, so adjust >> * 'orientation' to report the image native orientation and return Identity. >> diff --git a/src/libcamera/sensor/camera_sensor_raw.cpp b/src/libcamera/sensor/camera_sensor_raw.cpp >> index 10eba0331fe8..52718bf0c3f5 100644 >> --- a/src/libcamera/sensor/camera_sensor_raw.cpp >> +++ b/src/libcamera/sensor/camera_sensor_raw.cpp >> @@ -144,7 +144,8 @@ private: >> Size pixelArraySize_; >> Rectangle activeArea_; >> BayerFormat::Order cfaPattern_; >> - bool supportFlips_; >> + bool supportHFlips_; >> + bool supportVFlips_; >> bool flipsAlterBayerOrder_; >> Orientation mountingOrientation_; >> >> @@ -162,8 +163,9 @@ private: >> */ >> >> CameraSensorRaw::CameraSensorRaw(const MediaEntity *entity) >> - : entity_(entity), staticProps_(nullptr), supportFlips_(false), >> - flipsAlterBayerOrder_(false), properties_(properties::properties) >> + : entity_(entity), staticProps_(nullptr), supportHFlips_(false), >> + supportVFlips_(false), flipsAlterBayerOrder_(false), >> + properties_(properties::properties) >> { >> } >> >> @@ -479,26 +481,31 @@ std::optional<int> CameraSensorRaw::init() >> return { ret }; >> } >> >> - /* >> - * Verify if sensor supports horizontal/vertical flips >> - * >> - * \todo Handle horizontal and vertical flips independently. >> - */ >> - const struct v4l2_query_ext_ctrl *hflipInfo = subdev_->controlInfo(V4L2_CID_HFLIP); >> - const struct v4l2_query_ext_ctrl *vflipInfo = subdev_->controlInfo(V4L2_CID_VFLIP); >> - if (hflipInfo && !(hflipInfo->flags & V4L2_CTRL_FLAG_READ_ONLY) && >> - vflipInfo && !(vflipInfo->flags & V4L2_CTRL_FLAG_READ_ONLY)) { >> - supportFlips_ = true; >> + /* Verify if sensor supports horizontal/vertical flips. */ >> + auto queryFlip = [&](uint32_t id, const char *name) -> std::pair<bool, bool> { >> + const struct v4l2_query_ext_ctrl *flipInfo = subdev_->controlInfo(id); >> + if (!flipInfo || flipInfo->flags & V4L2_CTRL_FLAG_READ_ONLY) { >> + LOG(CameraSensor, Debug) >> + << "Camera sensor does not support " << name << " flip"; >> + return { false, false }; >> + } >> >> - if (hflipInfo->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT || >> - vflipInfo->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT) >> - flipsAlterBayerOrder_ = true; >> + return { true, flipInfo->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT }; >> + }; >> + >> + auto hflip = queryFlip(V4L2_CID_HFLIP, "horizontal"); >> + auto vflip = queryFlip(V4L2_CID_HFLIP, "vertical"); V4L2_CID_VFLIP >> + >> + if (hflip.first && vflip.first && (hflip.second != vflip.second)) { >> + LOG(CameraSensor, Error) >> + << "Flips with differing alter bayer order properties" >> + << " are unsupported. Disabling flipping."; >> + } else { >> + supportHFlips_ = hflip.first; >> + supportVFlips_ = vflip.first; >> + flipsAlterBayerOrder_ = hflip.second || vflip.second; >> } >> >> - if (!supportFlips_) >> - LOG(CameraSensor, Debug) >> - << "Camera sensor does not support horizontal/vertical flip"; >> - >> /* >> * 5. Discover ancillary devices. >> * >> @@ -819,14 +826,16 @@ CameraSensorRaw::getFormat(Span<const unsigned int> mbusCodes, >> int CameraSensorRaw::setFormat(V4L2SubdeviceFormat *format, Transform transform) >> { >> /* Configure flips if the sensor supports that. */ >> - if (supportFlips_) { >> - ControlList flipCtrls(subdev_->controls()); >> - >> + ControlList flipCtrls(subdev_->controls()); >> + if (supportHFlips_) >> flipCtrls.set(V4L2_CID_HFLIP, >> static_cast<int32_t>(!!(transform & Transform::HFlip))); >> + >> + if (supportVFlips_) >> flipCtrls.set(V4L2_CID_VFLIP, >> static_cast<int32_t>(!!(transform & Transform::VFlip))); >> >> + if (!flipCtrls.empty()) { >> int ret = subdev_->setControls(&flipCtrls); >> if (ret) >> return ret; >> @@ -1054,17 +1063,8 @@ int CameraSensorRaw::sensorInfo(IPACameraSensorInfo *info) const >> Transform CameraSensorRaw::computeTransform(Orientation *orientation) const >> { >> /* >> - * If we cannot do any flips we cannot change the native camera mounting >> - * orientation. >> - */ >> - if (!supportFlips_) { >> - *orientation = mountingOrientation_; >> - return Transform::Identity; >> - } >> - >> - /* >> - * Now compute the required transform to obtain 'orientation' starting >> - * from the mounting rotation. >> + * Compute the required transform to obtain 'orientation' starting from >> + * the mounting rotation. >> * >> * As a note: >> * orientation / mountingOrientation_ = transform >> @@ -1072,6 +1072,17 @@ Transform CameraSensorRaw::computeTransform(Orientation *orientation) const >> */ >> Transform transform = *orientation / mountingOrientation_; >> >> + /* If we cannot do the required flips, fall back to identity. */ >> + if (!supportHFlips_ && !!(transform & Transform::HFlip)) { >> + *orientation = mountingOrientation_; >> + return Transform::Identity; >> + } >> + >> + if (!supportVFlips_ && !!(transform & Transform::VFlip)) { >> + *orientation = mountingOrientation_; >> + return Transform::Identity; >> + } >> + >> /* >> * If transform contains any Transpose we cannot do it, so adjust >> * 'orientation' to report the image native orientation and return Identity. >> -- >> 2.53.0 >>
Hi Barnabás, Quoting Barnabás Pőcze (2026-08-06 11:02:58) > 2026. 08. 05. 14:21 keltezéssel, Jacopo Mondi írta: > > Hi Stefan > > > > On Wed, Aug 05, 2026 at 12:12:03PM +0200, Stefan Klug wrote: > >> CameraSensorLegacy and CameraSensorRaw support flipping on the sensor > >> side only if the driver supports both, V4L2_CID_HFLIP and > >> V4L2_CID_VFLIP. In cases where a driver only supports a single flip > >> mode, flipping support gets disabled completely. This is unexpected and > >> annoying when validating sensor drivers. > >> > >> Fix that by handling vertical and horizontal flips separately. > >> > >> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> > >> > >> --- > >> > >> Changes in v3: > >> - Swapped if and replaced ternary operator by || in support logic > >> - Improved description in sensor_driver_requirements > >> > >> Changes in v2: > >> - Replaced duplicate query flip code with lambda > >> - Reworked apply flip code to call setControls only once > >> - Replaced a & by && > >> - Applied the change to CameraSensorRaw as well > >> - Small comment/logging changes > >> --- > >> > >> Hi all, > >> > >> I had this patch somewhere in my tree for a long time. Now I stumbled > >> over this issue again and had to manually rebase the changes. So maybe > >> it is time to upstream :-) > >> > >> Version 3 adresses the comments from last review. > >> > >> Best regards, > >> Stefan > >> > >> Documentation/sensor_driver_requirements.rst | 2 + > >> src/libcamera/sensor/camera_sensor_legacy.cpp | 76 ++++++++++-------- > >> src/libcamera/sensor/camera_sensor_raw.cpp | 79 +++++++++++-------- > >> 3 files changed, 90 insertions(+), 67 deletions(-) > >> > >> diff --git a/Documentation/sensor_driver_requirements.rst b/Documentation/sensor_driver_requirements.rst > >> index 0e516b34a215..1caa2f96d682 100644 > >> --- a/Documentation/sensor_driver_requirements.rst > >> +++ b/Documentation/sensor_driver_requirements.rst > >> @@ -73,6 +73,8 @@ In order to support rotating the image the sensor driver should support > >> The controls must be writable from userspace. In case of a RAW Bayer sensors, > >> drivers should correctly report if vertical/horizontal flips modify the Bayer > >> pattern ordering by reporting the `V4L2_CTRL_FLAG_MODIFY_LAYOUT` control flag. > >> +If the flag is set for one of the two controls, it shall be set for the other > >> +one as well. > >> > >> The sensor driver should implement support for the V4L2 Selection API, > >> specifically it should implement support for the > >> diff --git a/src/libcamera/sensor/camera_sensor_legacy.cpp b/src/libcamera/sensor/camera_sensor_legacy.cpp > >> index 6a683821f219..751d6f3e1c04 100644 > >> --- a/src/libcamera/sensor/camera_sensor_legacy.cpp > >> +++ b/src/libcamera/sensor/camera_sensor_legacy.cpp > >> @@ -133,7 +133,8 @@ private: > >> Size pixelArraySize_; > >> Rectangle activeArea_; > >> const BayerFormat *bayerFormat_; > >> - bool supportFlips_; > >> + bool supportHFlips_; > >> + bool supportVFlips_; > >> bool flipsAlterBayerOrder_; > >> Orientation mountingOrientation_; > >> > >> @@ -153,7 +154,7 @@ private: > >> > >> CameraSensorLegacy::CameraSensorLegacy(const MediaEntity *entity) > >> : entity_(entity), pad_(UINT_MAX), staticProps_(nullptr), > >> - bayerFormat_(nullptr), supportFlips_(false), > >> + bayerFormat_(nullptr), supportHFlips_(false), supportVFlips_(false), > >> flipsAlterBayerOrder_(false), properties_(properties::properties) > >> { > >> } > >> @@ -359,26 +360,31 @@ int CameraSensorLegacy::validateSensorDriver() > >> } > >> } > >> > >> - /* > >> - * Verify if sensor supports horizontal/vertical flips > >> - * > >> - * \todo Handle horizontal and vertical flips independently. > >> - */ > >> - const struct v4l2_query_ext_ctrl *hflipInfo = subdev_->controlInfo(V4L2_CID_HFLIP); > >> - const struct v4l2_query_ext_ctrl *vflipInfo = subdev_->controlInfo(V4L2_CID_VFLIP); > >> - if (hflipInfo && !(hflipInfo->flags & V4L2_CTRL_FLAG_READ_ONLY) && > >> - vflipInfo && !(vflipInfo->flags & V4L2_CTRL_FLAG_READ_ONLY)) { > >> - supportFlips_ = true; > >> + /* Verify if sensor supports horizontal/vertical flips. */ > >> + auto queryFlip = [&](uint32_t id, const char *name) -> std::pair<bool, bool> { > >> + const struct v4l2_query_ext_ctrl *flipInfo = subdev_->controlInfo(id); > >> + if (!flipInfo || flipInfo->flags & V4L2_CTRL_FLAG_READ_ONLY) { > >> + LOG(CameraSensor, Debug) > >> + << "Camera sensor does not support " << name << " flip"; > >> + return { false, false }; > >> + } > >> > >> - if (hflipInfo->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT || > >> - vflipInfo->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT) > >> - flipsAlterBayerOrder_ = true; > >> + return { true, flipInfo->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT }; > >> + }; > >> + > >> + auto hflip = queryFlip(V4L2_CID_HFLIP, "horizontal"); > >> + auto vflip = queryFlip(V4L2_CID_HFLIP, "vertical"); > > V4L2_CID_VFLIP Ouch. Where did that sneak in... > > > >> + > >> + if (hflip.first && vflip.first && (hflip.second != vflip.second)) { > >> + LOG(CameraSensor, Error) > >> + << "Flips with differing alter bayer order properties" > >> + << " are unsupported. Disabling flipping."; > >> + } else { > >> + supportHFlips_ = hflip.first; > >> + supportVFlips_ = vflip.first; > >> + flipsAlterBayerOrder_ = hflip.second || vflip.second; > >> } > >> > >> - if (!supportFlips_) > >> - LOG(CameraSensor, Debug) > >> - << "Camera sensor does not support horizontal/vertical flip"; > >> - > >> /* > >> * Make sure the required selection targets are supported. > >> * > >> @@ -759,14 +765,16 @@ CameraSensorLegacy::getFormat(Span<const unsigned int> mbusCodes, > >> int CameraSensorLegacy::setFormat(V4L2SubdeviceFormat *format, Transform transform) > >> { > >> /* Configure flips if the sensor supports that. */ > >> - if (supportFlips_) { > >> - ControlList flipCtrls(subdev_->controls()); > >> - > >> + ControlList flipCtrls(subdev_->controls()); > >> + if (supportHFlips_) > >> flipCtrls.set(V4L2_CID_HFLIP, > >> static_cast<int32_t>(!!(transform & Transform::HFlip))); > >> + > >> + if (supportVFlips_) > > > > These checks should not be needed, as computeTransform() won't set > > the Transform::HFlip or VFlip flags in 'transform'. > > I think it's needed because if flips are supported and e.g. `!(transform & Transform::HFlip)` > then `V4L2_CID_HFLIP` must be set to false in case it was enabled earlier, right? Yes. > > > > > > Better safe than sorry > > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > > > > >> flipCtrls.set(V4L2_CID_VFLIP, > >> static_cast<int32_t>(!!(transform & Transform::VFlip))); > >> > >> + if (!flipCtrls.empty()) { > >> int ret = subdev_->setControls(&flipCtrls); > >> if (ret) > >> return ret; > >> @@ -937,17 +945,8 @@ int CameraSensorLegacy::sensorInfo(IPACameraSensorInfo *info) const > >> Transform CameraSensorLegacy::computeTransform(Orientation *orientation) const > >> { > >> /* > >> - * If we cannot do any flips we cannot change the native camera mounting > >> - * orientation. > >> - */ > >> - if (!supportFlips_) { > >> - *orientation = mountingOrientation_; > >> - return Transform::Identity; > >> - } > >> - > >> - /* > >> - * Now compute the required transform to obtain 'orientation' starting > >> - * from the mounting rotation. > >> + * Compute the required transform to obtain 'orientation' starting from > >> + * the mounting rotation. > >> * > >> * As a note: > >> * orientation / mountingOrientation_ = transform > >> @@ -955,6 +954,17 @@ Transform CameraSensorLegacy::computeTransform(Orientation *orientation) const > >> */ > >> Transform transform = *orientation / mountingOrientation_; > >> > >> + /* If we cannot do the required flips, fall back to identity. */ > >> + if (!supportHFlips_ && !!(transform & Transform::HFlip)) { > >> + *orientation = mountingOrientation_; > >> + return Transform::Identity; > >> + } > >> + > >> + if (!supportVFlips_ && !!(transform & Transform::VFlip)) { > >> + *orientation = mountingOrientation_; > >> + return Transform::Identity; > >> + } > >> + > >> /* > >> * If transform contains any Transpose we cannot do it, so adjust > >> * 'orientation' to report the image native orientation and return Identity. > >> diff --git a/src/libcamera/sensor/camera_sensor_raw.cpp b/src/libcamera/sensor/camera_sensor_raw.cpp > >> index 10eba0331fe8..52718bf0c3f5 100644 > >> --- a/src/libcamera/sensor/camera_sensor_raw.cpp > >> +++ b/src/libcamera/sensor/camera_sensor_raw.cpp > >> @@ -144,7 +144,8 @@ private: > >> Size pixelArraySize_; > >> Rectangle activeArea_; > >> BayerFormat::Order cfaPattern_; > >> - bool supportFlips_; > >> + bool supportHFlips_; > >> + bool supportVFlips_; > >> bool flipsAlterBayerOrder_; > >> Orientation mountingOrientation_; > >> > >> @@ -162,8 +163,9 @@ private: > >> */ > >> > >> CameraSensorRaw::CameraSensorRaw(const MediaEntity *entity) > >> - : entity_(entity), staticProps_(nullptr), supportFlips_(false), > >> - flipsAlterBayerOrder_(false), properties_(properties::properties) > >> + : entity_(entity), staticProps_(nullptr), supportHFlips_(false), > >> + supportVFlips_(false), flipsAlterBayerOrder_(false), > >> + properties_(properties::properties) > >> { > >> } > >> > >> @@ -479,26 +481,31 @@ std::optional<int> CameraSensorRaw::init() > >> return { ret }; > >> } > >> > >> - /* > >> - * Verify if sensor supports horizontal/vertical flips > >> - * > >> - * \todo Handle horizontal and vertical flips independently. > >> - */ > >> - const struct v4l2_query_ext_ctrl *hflipInfo = subdev_->controlInfo(V4L2_CID_HFLIP); > >> - const struct v4l2_query_ext_ctrl *vflipInfo = subdev_->controlInfo(V4L2_CID_VFLIP); > >> - if (hflipInfo && !(hflipInfo->flags & V4L2_CTRL_FLAG_READ_ONLY) && > >> - vflipInfo && !(vflipInfo->flags & V4L2_CTRL_FLAG_READ_ONLY)) { > >> - supportFlips_ = true; > >> + /* Verify if sensor supports horizontal/vertical flips. */ > >> + auto queryFlip = [&](uint32_t id, const char *name) -> std::pair<bool, bool> { > >> + const struct v4l2_query_ext_ctrl *flipInfo = subdev_->controlInfo(id); > >> + if (!flipInfo || flipInfo->flags & V4L2_CTRL_FLAG_READ_ONLY) { > >> + LOG(CameraSensor, Debug) > >> + << "Camera sensor does not support " << name << " flip"; > >> + return { false, false }; > >> + } > >> > >> - if (hflipInfo->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT || > >> - vflipInfo->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT) > >> - flipsAlterBayerOrder_ = true; > >> + return { true, flipInfo->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT }; > >> + }; > >> + > >> + auto hflip = queryFlip(V4L2_CID_HFLIP, "horizontal"); > >> + auto vflip = queryFlip(V4L2_CID_HFLIP, "vertical"); > > V4L2_CID_VFLIP > > > >> + > >> + if (hflip.first && vflip.first && (hflip.second != vflip.second)) { > >> + LOG(CameraSensor, Error) > >> + << "Flips with differing alter bayer order properties" > >> + << " are unsupported. Disabling flipping."; > >> + } else { > >> + supportHFlips_ = hflip.first; > >> + supportVFlips_ = vflip.first; > >> + flipsAlterBayerOrder_ = hflip.second || vflip.second; > >> } > >> > >> - if (!supportFlips_) > >> - LOG(CameraSensor, Debug) > >> - << "Camera sensor does not support horizontal/vertical flip"; > >> - > >> /* > >> * 5. Discover ancillary devices. > >> * > >> @@ -819,14 +826,16 @@ CameraSensorRaw::getFormat(Span<const unsigned int> mbusCodes, > >> int CameraSensorRaw::setFormat(V4L2SubdeviceFormat *format, Transform transform) > >> { > >> /* Configure flips if the sensor supports that. */ > >> - if (supportFlips_) { > >> - ControlList flipCtrls(subdev_->controls()); > >> - > >> + ControlList flipCtrls(subdev_->controls()); > >> + if (supportHFlips_) > >> flipCtrls.set(V4L2_CID_HFLIP, > >> static_cast<int32_t>(!!(transform & Transform::HFlip))); > >> + > >> + if (supportVFlips_) > >> flipCtrls.set(V4L2_CID_VFLIP, > >> static_cast<int32_t>(!!(transform & Transform::VFlip))); > >> > >> + if (!flipCtrls.empty()) { > >> int ret = subdev_->setControls(&flipCtrls); > >> if (ret) > >> return ret; > >> @@ -1054,17 +1063,8 @@ int CameraSensorRaw::sensorInfo(IPACameraSensorInfo *info) const > >> Transform CameraSensorRaw::computeTransform(Orientation *orientation) const > >> { > >> /* > >> - * If we cannot do any flips we cannot change the native camera mounting > >> - * orientation. > >> - */ > >> - if (!supportFlips_) { > >> - *orientation = mountingOrientation_; > >> - return Transform::Identity; > >> - } > >> - > >> - /* > >> - * Now compute the required transform to obtain 'orientation' starting > >> - * from the mounting rotation. > >> + * Compute the required transform to obtain 'orientation' starting from > >> + * the mounting rotation. > >> * > >> * As a note: > >> * orientation / mountingOrientation_ = transform > >> @@ -1072,6 +1072,17 @@ Transform CameraSensorRaw::computeTransform(Orientation *orientation) const > >> */ > >> Transform transform = *orientation / mountingOrientation_; > >> > >> + /* If we cannot do the required flips, fall back to identity. */ > >> + if (!supportHFlips_ && !!(transform & Transform::HFlip)) { > >> + *orientation = mountingOrientation_; > >> + return Transform::Identity; > >> + } > >> + > >> + if (!supportVFlips_ && !!(transform & Transform::VFlip)) { > >> + *orientation = mountingOrientation_; > >> + return Transform::Identity; > >> + } > >> + > >> /* > >> * If transform contains any Transpose we cannot do it, so adjust > >> * 'orientation' to report the image native orientation and return Identity. > >> -- > >> 2.53.0 > >> >
diff --git a/Documentation/sensor_driver_requirements.rst b/Documentation/sensor_driver_requirements.rst index 0e516b34a215..1caa2f96d682 100644 --- a/Documentation/sensor_driver_requirements.rst +++ b/Documentation/sensor_driver_requirements.rst @@ -73,6 +73,8 @@ In order to support rotating the image the sensor driver should support The controls must be writable from userspace. In case of a RAW Bayer sensors, drivers should correctly report if vertical/horizontal flips modify the Bayer pattern ordering by reporting the `V4L2_CTRL_FLAG_MODIFY_LAYOUT` control flag. +If the flag is set for one of the two controls, it shall be set for the other +one as well. The sensor driver should implement support for the V4L2 Selection API, specifically it should implement support for the diff --git a/src/libcamera/sensor/camera_sensor_legacy.cpp b/src/libcamera/sensor/camera_sensor_legacy.cpp index 6a683821f219..751d6f3e1c04 100644 --- a/src/libcamera/sensor/camera_sensor_legacy.cpp +++ b/src/libcamera/sensor/camera_sensor_legacy.cpp @@ -133,7 +133,8 @@ private: Size pixelArraySize_; Rectangle activeArea_; const BayerFormat *bayerFormat_; - bool supportFlips_; + bool supportHFlips_; + bool supportVFlips_; bool flipsAlterBayerOrder_; Orientation mountingOrientation_; @@ -153,7 +154,7 @@ private: CameraSensorLegacy::CameraSensorLegacy(const MediaEntity *entity) : entity_(entity), pad_(UINT_MAX), staticProps_(nullptr), - bayerFormat_(nullptr), supportFlips_(false), + bayerFormat_(nullptr), supportHFlips_(false), supportVFlips_(false), flipsAlterBayerOrder_(false), properties_(properties::properties) { } @@ -359,26 +360,31 @@ int CameraSensorLegacy::validateSensorDriver() } } - /* - * Verify if sensor supports horizontal/vertical flips - * - * \todo Handle horizontal and vertical flips independently. - */ - const struct v4l2_query_ext_ctrl *hflipInfo = subdev_->controlInfo(V4L2_CID_HFLIP); - const struct v4l2_query_ext_ctrl *vflipInfo = subdev_->controlInfo(V4L2_CID_VFLIP); - if (hflipInfo && !(hflipInfo->flags & V4L2_CTRL_FLAG_READ_ONLY) && - vflipInfo && !(vflipInfo->flags & V4L2_CTRL_FLAG_READ_ONLY)) { - supportFlips_ = true; + /* Verify if sensor supports horizontal/vertical flips. */ + auto queryFlip = [&](uint32_t id, const char *name) -> std::pair<bool, bool> { + const struct v4l2_query_ext_ctrl *flipInfo = subdev_->controlInfo(id); + if (!flipInfo || flipInfo->flags & V4L2_CTRL_FLAG_READ_ONLY) { + LOG(CameraSensor, Debug) + << "Camera sensor does not support " << name << " flip"; + return { false, false }; + } - if (hflipInfo->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT || - vflipInfo->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT) - flipsAlterBayerOrder_ = true; + return { true, flipInfo->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT }; + }; + + auto hflip = queryFlip(V4L2_CID_HFLIP, "horizontal"); + auto vflip = queryFlip(V4L2_CID_HFLIP, "vertical"); + + if (hflip.first && vflip.first && (hflip.second != vflip.second)) { + LOG(CameraSensor, Error) + << "Flips with differing alter bayer order properties" + << " are unsupported. Disabling flipping."; + } else { + supportHFlips_ = hflip.first; + supportVFlips_ = vflip.first; + flipsAlterBayerOrder_ = hflip.second || vflip.second; } - if (!supportFlips_) - LOG(CameraSensor, Debug) - << "Camera sensor does not support horizontal/vertical flip"; - /* * Make sure the required selection targets are supported. * @@ -759,14 +765,16 @@ CameraSensorLegacy::getFormat(Span<const unsigned int> mbusCodes, int CameraSensorLegacy::setFormat(V4L2SubdeviceFormat *format, Transform transform) { /* Configure flips if the sensor supports that. */ - if (supportFlips_) { - ControlList flipCtrls(subdev_->controls()); - + ControlList flipCtrls(subdev_->controls()); + if (supportHFlips_) flipCtrls.set(V4L2_CID_HFLIP, static_cast<int32_t>(!!(transform & Transform::HFlip))); + + if (supportVFlips_) flipCtrls.set(V4L2_CID_VFLIP, static_cast<int32_t>(!!(transform & Transform::VFlip))); + if (!flipCtrls.empty()) { int ret = subdev_->setControls(&flipCtrls); if (ret) return ret; @@ -937,17 +945,8 @@ int CameraSensorLegacy::sensorInfo(IPACameraSensorInfo *info) const Transform CameraSensorLegacy::computeTransform(Orientation *orientation) const { /* - * If we cannot do any flips we cannot change the native camera mounting - * orientation. - */ - if (!supportFlips_) { - *orientation = mountingOrientation_; - return Transform::Identity; - } - - /* - * Now compute the required transform to obtain 'orientation' starting - * from the mounting rotation. + * Compute the required transform to obtain 'orientation' starting from + * the mounting rotation. * * As a note: * orientation / mountingOrientation_ = transform @@ -955,6 +954,17 @@ Transform CameraSensorLegacy::computeTransform(Orientation *orientation) const */ Transform transform = *orientation / mountingOrientation_; + /* If we cannot do the required flips, fall back to identity. */ + if (!supportHFlips_ && !!(transform & Transform::HFlip)) { + *orientation = mountingOrientation_; + return Transform::Identity; + } + + if (!supportVFlips_ && !!(transform & Transform::VFlip)) { + *orientation = mountingOrientation_; + return Transform::Identity; + } + /* * If transform contains any Transpose we cannot do it, so adjust * 'orientation' to report the image native orientation and return Identity. diff --git a/src/libcamera/sensor/camera_sensor_raw.cpp b/src/libcamera/sensor/camera_sensor_raw.cpp index 10eba0331fe8..52718bf0c3f5 100644 --- a/src/libcamera/sensor/camera_sensor_raw.cpp +++ b/src/libcamera/sensor/camera_sensor_raw.cpp @@ -144,7 +144,8 @@ private: Size pixelArraySize_; Rectangle activeArea_; BayerFormat::Order cfaPattern_; - bool supportFlips_; + bool supportHFlips_; + bool supportVFlips_; bool flipsAlterBayerOrder_; Orientation mountingOrientation_; @@ -162,8 +163,9 @@ private: */ CameraSensorRaw::CameraSensorRaw(const MediaEntity *entity) - : entity_(entity), staticProps_(nullptr), supportFlips_(false), - flipsAlterBayerOrder_(false), properties_(properties::properties) + : entity_(entity), staticProps_(nullptr), supportHFlips_(false), + supportVFlips_(false), flipsAlterBayerOrder_(false), + properties_(properties::properties) { } @@ -479,26 +481,31 @@ std::optional<int> CameraSensorRaw::init() return { ret }; } - /* - * Verify if sensor supports horizontal/vertical flips - * - * \todo Handle horizontal and vertical flips independently. - */ - const struct v4l2_query_ext_ctrl *hflipInfo = subdev_->controlInfo(V4L2_CID_HFLIP); - const struct v4l2_query_ext_ctrl *vflipInfo = subdev_->controlInfo(V4L2_CID_VFLIP); - if (hflipInfo && !(hflipInfo->flags & V4L2_CTRL_FLAG_READ_ONLY) && - vflipInfo && !(vflipInfo->flags & V4L2_CTRL_FLAG_READ_ONLY)) { - supportFlips_ = true; + /* Verify if sensor supports horizontal/vertical flips. */ + auto queryFlip = [&](uint32_t id, const char *name) -> std::pair<bool, bool> { + const struct v4l2_query_ext_ctrl *flipInfo = subdev_->controlInfo(id); + if (!flipInfo || flipInfo->flags & V4L2_CTRL_FLAG_READ_ONLY) { + LOG(CameraSensor, Debug) + << "Camera sensor does not support " << name << " flip"; + return { false, false }; + } - if (hflipInfo->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT || - vflipInfo->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT) - flipsAlterBayerOrder_ = true; + return { true, flipInfo->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT }; + }; + + auto hflip = queryFlip(V4L2_CID_HFLIP, "horizontal"); + auto vflip = queryFlip(V4L2_CID_HFLIP, "vertical"); + + if (hflip.first && vflip.first && (hflip.second != vflip.second)) { + LOG(CameraSensor, Error) + << "Flips with differing alter bayer order properties" + << " are unsupported. Disabling flipping."; + } else { + supportHFlips_ = hflip.first; + supportVFlips_ = vflip.first; + flipsAlterBayerOrder_ = hflip.second || vflip.second; } - if (!supportFlips_) - LOG(CameraSensor, Debug) - << "Camera sensor does not support horizontal/vertical flip"; - /* * 5. Discover ancillary devices. * @@ -819,14 +826,16 @@ CameraSensorRaw::getFormat(Span<const unsigned int> mbusCodes, int CameraSensorRaw::setFormat(V4L2SubdeviceFormat *format, Transform transform) { /* Configure flips if the sensor supports that. */ - if (supportFlips_) { - ControlList flipCtrls(subdev_->controls()); - + ControlList flipCtrls(subdev_->controls()); + if (supportHFlips_) flipCtrls.set(V4L2_CID_HFLIP, static_cast<int32_t>(!!(transform & Transform::HFlip))); + + if (supportVFlips_) flipCtrls.set(V4L2_CID_VFLIP, static_cast<int32_t>(!!(transform & Transform::VFlip))); + if (!flipCtrls.empty()) { int ret = subdev_->setControls(&flipCtrls); if (ret) return ret; @@ -1054,17 +1063,8 @@ int CameraSensorRaw::sensorInfo(IPACameraSensorInfo *info) const Transform CameraSensorRaw::computeTransform(Orientation *orientation) const { /* - * If we cannot do any flips we cannot change the native camera mounting - * orientation. - */ - if (!supportFlips_) { - *orientation = mountingOrientation_; - return Transform::Identity; - } - - /* - * Now compute the required transform to obtain 'orientation' starting - * from the mounting rotation. + * Compute the required transform to obtain 'orientation' starting from + * the mounting rotation. * * As a note: * orientation / mountingOrientation_ = transform @@ -1072,6 +1072,17 @@ Transform CameraSensorRaw::computeTransform(Orientation *orientation) const */ Transform transform = *orientation / mountingOrientation_; + /* If we cannot do the required flips, fall back to identity. */ + if (!supportHFlips_ && !!(transform & Transform::HFlip)) { + *orientation = mountingOrientation_; + return Transform::Identity; + } + + if (!supportVFlips_ && !!(transform & Transform::VFlip)) { + *orientation = mountingOrientation_; + return Transform::Identity; + } + /* * If transform contains any Transpose we cannot do it, so adjust * 'orientation' to report the image native orientation and return Identity.
CameraSensorLegacy and CameraSensorRaw support flipping on the sensor side only if the driver supports both, V4L2_CID_HFLIP and V4L2_CID_VFLIP. In cases where a driver only supports a single flip mode, flipping support gets disabled completely. This is unexpected and annoying when validating sensor drivers. Fix that by handling vertical and horizontal flips separately. Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> --- Changes in v3: - Swapped if and replaced ternary operator by || in support logic - Improved description in sensor_driver_requirements Changes in v2: - Replaced duplicate query flip code with lambda - Reworked apply flip code to call setControls only once - Replaced a & by && - Applied the change to CameraSensorRaw as well - Small comment/logging changes --- Hi all, I had this patch somewhere in my tree for a long time. Now I stumbled over this issue again and had to manually rebase the changes. So maybe it is time to upstream :-) Version 3 adresses the comments from last review. Best regards, Stefan Documentation/sensor_driver_requirements.rst | 2 + src/libcamera/sensor/camera_sensor_legacy.cpp | 76 ++++++++++-------- src/libcamera/sensor/camera_sensor_raw.cpp | 79 +++++++++++-------- 3 files changed, 90 insertions(+), 67 deletions(-)