| Message ID | 20260731143741.689779-1-stefan.klug@ideasonboard.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Hi 2026. 07. 31. 16:37 keltezéssel, Stefan Klug írta: > CameraSensorLegacy supports flipping on the sensor side only if the > driver supports 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. What about `CameraSensorRaw`? > > Fix that by handling vertical and horizontal flips separately. > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> > --- > 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 :-) > > Best regards, > Stefan > > src/libcamera/sensor/camera_sensor_legacy.cpp | 72 +++++++++++++------ > 1 file changed, 52 insertions(+), 20 deletions(-) > > diff --git a/src/libcamera/sensor/camera_sensor_legacy.cpp b/src/libcamera/sensor/camera_sensor_legacy.cpp > index 6a683821f219..0f8e3c8cf251 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) > { > } > @@ -365,19 +366,37 @@ int CameraSensorLegacy::validateSensorDriver() > * \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; > + if (hflipInfo && !(hflipInfo->flags & V4L2_CTRL_FLAG_READ_ONLY)) { > + supportHFlips_ = true; > > - if (hflipInfo->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT || > - vflipInfo->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT) > + if (hflipInfo->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT) > flipsAlterBayerOrder_ = true; > + } else { > + LOG(CameraSensor, Debug) > + << "Camera sensor does not support horizontal flip"; > } > > - if (!supportFlips_) > + const struct v4l2_query_ext_ctrl *vflipInfo = subdev_->controlInfo(V4L2_CID_VFLIP); > + if (vflipInfo && !(vflipInfo->flags & V4L2_CTRL_FLAG_READ_ONLY)) { > + supportVFlips_ = true; > + > + if (vflipInfo->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT) > + flipsAlterBayerOrder_ = true; > + } else { > LOG(CameraSensor, Debug) > - << "Camera sensor does not support horizontal/vertical flip"; > + << "Camera sensor does not support vertical flip"; > + } > + > + if (supportHFlips_ && supportVFlips_ && > + ((vflipInfo->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT) != > + (hflipInfo->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT))) { > + LOG(CameraSensor, Error) > + << "Flips have different alter bayer order properties." > + << " Disable flipping altogether"; This is theoretically possible, right? Should it be a warning with an "unsupported" somewhere in the message? > + supportHFlips_ = false; > + supportVFlips_ = false; > + flipsAlterBayerOrder_ = false; > + } > > /* > * Make sure the required selection targets are supported. > @@ -759,11 +778,20 @@ CameraSensorLegacy::getFormat(Span<const unsigned int> mbusCodes, > int CameraSensorLegacy::setFormat(V4L2SubdeviceFormat *format, Transform transform) > { > /* Configure flips if the sensor supports that. */ > - if (supportFlips_) { > + if (supportHFlips_) { > ControlList flipCtrls(subdev_->controls()); > > flipCtrls.set(V4L2_CID_HFLIP, > static_cast<int32_t>(!!(transform & Transform::HFlip))); > + > + int ret = subdev_->setControls(&flipCtrls); > + if (ret) > + return ret; > + } > + > + if (supportVFlips_) { > + ControlList flipCtrls(subdev_->controls()); > + > flipCtrls.set(V4L2_CID_VFLIP, > static_cast<int32_t>(!!(transform & Transform::VFlip))); > > @@ -936,15 +964,6 @@ 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. > @@ -955,6 +974,19 @@ 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)) { I would write if (!supportHFlips_ && (transform & Transform::HFlip)) > + *orientation = mountingOrientation_; > + return Transform::Identity; > + } > + > + if (!supportVFlips_ & !!(transform & Transform::VFlip)) { Similarly here. > + *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.
On Fri, Jul 31, 2026 at 05:07:27PM +0200, Barnabás Pőcze wrote: > 2026. 07. 31. 16:37 keltezéssel, Stefan Klug írta: > > CameraSensorLegacy supports flipping on the sensor side only if the > > driver supports 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. > > What about `CameraSensorRaw`? > > > Fix that by handling vertical and horizontal flips separately. > > > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> > > --- > > 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 :-) > > > > Best regards, > > Stefan > > > > src/libcamera/sensor/camera_sensor_legacy.cpp | 72 +++++++++++++------ > > 1 file changed, 52 insertions(+), 20 deletions(-) > > > > diff --git a/src/libcamera/sensor/camera_sensor_legacy.cpp b/src/libcamera/sensor/camera_sensor_legacy.cpp > > index 6a683821f219..0f8e3c8cf251 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) > > { > > } > > @@ -365,19 +366,37 @@ int CameraSensorLegacy::validateSensorDriver() > > * \todo Handle horizontal and vertical flips independently. This needs an update. > > */ > > 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; > > + if (hflipInfo && !(hflipInfo->flags & V4L2_CTRL_FLAG_READ_ONLY)) { > > + supportHFlips_ = true; > > > > - if (hflipInfo->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT || > > - vflipInfo->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT) > > + if (hflipInfo->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT) > > flipsAlterBayerOrder_ = true; > > + } else { > > + LOG(CameraSensor, Debug) > > + << "Camera sensor does not support horizontal flip"; > > } > > > > - if (!supportFlips_) > > + const struct v4l2_query_ext_ctrl *vflipInfo = subdev_->controlInfo(V4L2_CID_VFLIP); > > + if (vflipInfo && !(vflipInfo->flags & V4L2_CTRL_FLAG_READ_ONLY)) { > > + supportVFlips_ = true; > > + > > + if (vflipInfo->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT) > > + flipsAlterBayerOrder_ = true; > > + } else { > > LOG(CameraSensor, Debug) > > - << "Camera sensor does not support horizontal/vertical flip"; > > + << "Camera sensor does not support vertical flip"; > > + } > > + This essentially repeats the same code twice. A small helper function could help std::tuple<bool, bool> CameraSensorLegacy::queryFlip(uint32_t id, const char *name) { const struct v4l2_query_ext_ctrl *info = subdev_->controlInfo(id); if (!info || info->flags & V4L2_CTRL_FLAG_READ_ONLY)) { LOG(CameraSensor, Debug) << "Camera sensor does not support " << name << " flip"; return { false, false }; } return { true, info->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT }; } which can be used with something like auto hflip = queryFlip(V4L2_CID_HFLIP, "horizontal"); auto vflip = queryFlip(V4L2_CID_VFLIP, "vertical"); if (hflip.get<1> == vflip.get<1>) { supportHFlips_= hflip.get<0>; supportVFlips_= vflip.get<0>; flipsAlterBayerOrder_ = hflip.get<1>; } else { LOG(CameraSensor, Error) << "Flips have different alter bayer order properties." << " Disable flipping altogether"; } A small structure to avoid the get<0> and get<1> may be better. > > + if (supportHFlips_ && supportVFlips_ && > > + ((vflipInfo->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT) != > > + (hflipInfo->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT))) { > > + LOG(CameraSensor, Error) > > + << "Flips have different alter bayer order properties." > > + << " Disable flipping altogether"; > > This is theoretically possible, right? Should it be a warning with > an "unsupported" somewhere in the message? > > > + supportHFlips_ = false; > > + supportVFlips_ = false; > > + flipsAlterBayerOrder_ = false; > > + } > > > > /* > > * Make sure the required selection targets are supported. > > @@ -759,11 +778,20 @@ CameraSensorLegacy::getFormat(Span<const unsigned int> mbusCodes, > > int CameraSensorLegacy::setFormat(V4L2SubdeviceFormat *format, Transform transform) > > { > > /* Configure flips if the sensor supports that. */ > > - if (supportFlips_) { > > + if (supportHFlips_) { > > ControlList flipCtrls(subdev_->controls()); > > > > flipCtrls.set(V4L2_CID_HFLIP, > > static_cast<int32_t>(!!(transform & Transform::HFlip))); > > + > > + int ret = subdev_->setControls(&flipCtrls); > > + if (ret) > > + return ret; > > + } > > + > > + if (supportVFlips_) { > > + ControlList flipCtrls(subdev_->controls()); > > + > > flipCtrls.set(V4L2_CID_VFLIP, > > static_cast<int32_t>(!!(transform & Transform::VFlip))); > > > > @@ -936,15 +964,6 @@ 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 s/Now compute/Compute/ > > * from the mounting rotation. > > @@ -955,6 +974,19 @@ 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)) { > > I would write > > if (!supportHFlips_ && (transform & Transform::HFlip)) > > > + *orientation = mountingOrientation_; > > + return Transform::Identity; > > + } > > + > > + if (!supportVFlips_ & !!(transform & Transform::VFlip)) { > > Similarly here. > > > + *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_legacy.cpp b/src/libcamera/sensor/camera_sensor_legacy.cpp index 6a683821f219..0f8e3c8cf251 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) { } @@ -365,19 +366,37 @@ int CameraSensorLegacy::validateSensorDriver() * \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; + if (hflipInfo && !(hflipInfo->flags & V4L2_CTRL_FLAG_READ_ONLY)) { + supportHFlips_ = true; - if (hflipInfo->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT || - vflipInfo->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT) + if (hflipInfo->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT) flipsAlterBayerOrder_ = true; + } else { + LOG(CameraSensor, Debug) + << "Camera sensor does not support horizontal flip"; } - if (!supportFlips_) + const struct v4l2_query_ext_ctrl *vflipInfo = subdev_->controlInfo(V4L2_CID_VFLIP); + if (vflipInfo && !(vflipInfo->flags & V4L2_CTRL_FLAG_READ_ONLY)) { + supportVFlips_ = true; + + if (vflipInfo->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT) + flipsAlterBayerOrder_ = true; + } else { LOG(CameraSensor, Debug) - << "Camera sensor does not support horizontal/vertical flip"; + << "Camera sensor does not support vertical flip"; + } + + if (supportHFlips_ && supportVFlips_ && + ((vflipInfo->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT) != + (hflipInfo->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT))) { + LOG(CameraSensor, Error) + << "Flips have different alter bayer order properties." + << " Disable flipping altogether"; + supportHFlips_ = false; + supportVFlips_ = false; + flipsAlterBayerOrder_ = false; + } /* * Make sure the required selection targets are supported. @@ -759,11 +778,20 @@ CameraSensorLegacy::getFormat(Span<const unsigned int> mbusCodes, int CameraSensorLegacy::setFormat(V4L2SubdeviceFormat *format, Transform transform) { /* Configure flips if the sensor supports that. */ - if (supportFlips_) { + if (supportHFlips_) { ControlList flipCtrls(subdev_->controls()); flipCtrls.set(V4L2_CID_HFLIP, static_cast<int32_t>(!!(transform & Transform::HFlip))); + + int ret = subdev_->setControls(&flipCtrls); + if (ret) + return ret; + } + + if (supportVFlips_) { + ControlList flipCtrls(subdev_->controls()); + flipCtrls.set(V4L2_CID_VFLIP, static_cast<int32_t>(!!(transform & Transform::VFlip))); @@ -936,15 +964,6 @@ 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. @@ -955,6 +974,19 @@ 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.
CameraSensorLegacy supports flipping on the sensor side only if the driver supports 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> --- 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 :-) Best regards, Stefan src/libcamera/sensor/camera_sensor_legacy.cpp | 72 +++++++++++++------ 1 file changed, 52 insertions(+), 20 deletions(-)