[v2] sensor: camera_sensor_legacy/raw: Support separate H/V flips

27542 diff mbox series

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 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 2 adresses the comments from review.

Best regards,
Stefan


 src/libcamera/sensor/camera_sensor_legacy.cpp | 77 ++++++++++--------
 src/libcamera/sensor/camera_sensor_raw.cpp    | 80 +++++++++++--------
 2 files changed, 90 insertions(+), 67 deletions(-)

On Mon, Aug 03, 2026 at 11:52:47AM +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 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 2 adresses the comments from review.
>
> Best regards,
> Stefan
>
>
>  src/libcamera/sensor/camera_sensor_legacy.cpp | 77 ++++++++++--------
>  src/libcamera/sensor/camera_sensor_raw.cpp    | 80 +++++++++++--------
>  2 files changed, 90 insertions(+), 67 deletions(-)
>
> diff --git a/src/libcamera/sensor/camera_sensor_legacy.cpp b/src/libcamera/sensor/camera_sensor_legacy.cpp
> index 6a683821f219..5dce0a6eacfe 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,32 @@ 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)) {

nit: fits on one line.

The idea is to only set supportHFlips_, supportVFlips_ and
flipsAlterBayerOrder_ if the V4L2_CTRL_FLAG_MODIFY_LAYOUT is
consistent between the two flips, provided both of them are available.

Albeit equivalent, isn't it clear:

        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;
        }

> +		supportHFlips_ = hflip.first;
> +		supportVFlips_ = vflip.first;
> +		flipsAlterBayerOrder_ = supportHFlips_ ? hflip.second : vflip.second;

Regardless of the logic order of the check, this can be just an ||

> +	} else {
> +		LOG(CameraSensor, Error)
> +			<< "Flips with differing alter bayer order properties"
> +			<< " are unsupported. Disabling flipping.";
>  	}
>
> -	if (!supportFlips_)
> -		LOG(CameraSensor, Debug)
> -			<< "Camera sensor does not support horizontal/vertical flip";
> -
>  	/*
>  	 * Make sure the required selection targets are supported.
>  	 *
> @@ -759,14 +766,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 +946,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 +955,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;
> +	}
> +

Minor comment apart, we're introducing the requirement that both flips, if
present, shall expose the same MODIFY_LAYOUT flag layout.

I presume it's reasonable, but should we also add the requirement to
Documentation/sensor_driver_requirements.rst which currently only
reports:

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.

Thanks
  j

>  	/*
>  	 * 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..6efd72727773 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,32 @@ 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)) {
> +		supportHFlips_ = hflip.first;
> +		supportVFlips_ = vflip.first;
> +		flipsAlterBayerOrder_ = supportHFlips_ ? hflip.second : vflip.second;
> +	} else {
> +		LOG(CameraSensor, Error)
> +			<< "Flips with differing alter bayer order properties"
> +			<< " are unsupported. Disabling flipping.";
>  	}
>
> -	if (!supportFlips_)
> -		LOG(CameraSensor, Debug)
> -			<< "Camera sensor does not support horizontal/vertical flip";
> -
>  	/*
>  	 * 5. Discover ancillary devices.
>  	 *
> @@ -819,14 +827,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 +1064,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 +1073,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 Jacopo,

Thank you for the review.

Quoting Jacopo Mondi (2026-08-03 12:28:14)
> On Mon, Aug 03, 2026 at 11:52:47AM +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 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 2 adresses the comments from review.
> >
> > Best regards,
> > Stefan
> >
> >
> >  src/libcamera/sensor/camera_sensor_legacy.cpp | 77 ++++++++++--------
> >  src/libcamera/sensor/camera_sensor_raw.cpp    | 80 +++++++++++--------
> >  2 files changed, 90 insertions(+), 67 deletions(-)
> >
> > diff --git a/src/libcamera/sensor/camera_sensor_legacy.cpp b/src/libcamera/sensor/camera_sensor_legacy.cpp
> > index 6a683821f219..5dce0a6eacfe 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,32 @@ 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)) {
> 
> nit: fits on one line.
> 
> The idea is to only set supportHFlips_, supportVFlips_ and
> flipsAlterBayerOrder_ if the V4L2_CTRL_FLAG_MODIFY_LAYOUT is
> consistent between the two flips, provided both of them are available.
> 
> Albeit equivalent, isn't it clear:
> 
>         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;
>         }

I don't really care too much on the order. I think I just took it from
Laurents proposal. I had the feeling there was a tendency to put the
good case into the first branch. Your order is easier to read for me as
well, so if there are no other arguments, I'll flip it.

> 
> > +             supportHFlips_ = hflip.first;
> > +             supportVFlips_ = vflip.first;
> > +             flipsAlterBayerOrder_ = supportHFlips_ ? hflip.second : vflip.second;
> 
> Regardless of the logic order of the check, this can be just an ||

That's a good point, thanks.

> 
> > +     } else {
> > +             LOG(CameraSensor, Error)
> > +                     << "Flips with differing alter bayer order properties"
> > +                     << " are unsupported. Disabling flipping.";
> >       }
> >
> > -     if (!supportFlips_)
> > -             LOG(CameraSensor, Debug)
> > -                     << "Camera sensor does not support horizontal/vertical flip";
> > -
> >       /*
> >        * Make sure the required selection targets are supported.
> >        *
> > @@ -759,14 +766,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 +946,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 +955,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;
> > +     }
> > +
> 
> Minor comment apart, we're introducing the requirement that both flips, if
> present, shall expose the same MODIFY_LAYOUT flag layout.
> 
> I presume it's reasonable, but should we also add the requirement to
> Documentation/sensor_driver_requirements.rst which currently only
> reports:
> 
> 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.

I'm not sure about that. I think that restriction only applies to
libcamera (I just didn't implement support for that case). On the V4L2
side I don't see a need to make that restriction. And if we ever meet a
sensor that has the MODIFY_LAYOUT flag on one axis only, we need to add
support for that in libcamera. Or did I get you wrong?

Best regards,
Stefan

> 
> Thanks
>   j
> 
> >       /*
> >        * 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..6efd72727773 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,32 @@ 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)) {
> > +             supportHFlips_ = hflip.first;
> > +             supportVFlips_ = vflip.first;
> > +             flipsAlterBayerOrder_ = supportHFlips_ ? hflip.second : vflip.second;
> > +     } else {
> > +             LOG(CameraSensor, Error)
> > +                     << "Flips with differing alter bayer order properties"
> > +                     << " are unsupported. Disabling flipping.";
> >       }
> >
> > -     if (!supportFlips_)
> > -             LOG(CameraSensor, Debug)
> > -                     << "Camera sensor does not support horizontal/vertical flip";
> > -
> >       /*
> >        * 5. Discover ancillary devices.
> >        *
> > @@ -819,14 +827,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 +1064,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 +1073,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 Stefan

On Mon, Aug 03, 2026 at 01:03:35PM +0200, Stefan Klug wrote:
> Hi Jacopo,
>
> Thank you for the review.
>
> Quoting Jacopo Mondi (2026-08-03 12:28:14)
> > On Mon, Aug 03, 2026 at 11:52:47AM +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 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 2 adresses the comments from review.
> > >
> > > Best regards,
> > > Stefan
> > >
> > >
> > >  src/libcamera/sensor/camera_sensor_legacy.cpp | 77 ++++++++++--------
> > >  src/libcamera/sensor/camera_sensor_raw.cpp    | 80 +++++++++++--------
> > >  2 files changed, 90 insertions(+), 67 deletions(-)
> > >
> > > diff --git a/src/libcamera/sensor/camera_sensor_legacy.cpp b/src/libcamera/sensor/camera_sensor_legacy.cpp
> > > index 6a683821f219..5dce0a6eacfe 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,32 @@ 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)) {
> >
> > nit: fits on one line.
> >
> > The idea is to only set supportHFlips_, supportVFlips_ and
> > flipsAlterBayerOrder_ if the V4L2_CTRL_FLAG_MODIFY_LAYOUT is
> > consistent between the two flips, provided both of them are available.
> >
> > Albeit equivalent, isn't it clear:
> >
> >         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;
> >         }
>
> I don't really care too much on the order. I think I just took it from
> Laurents proposal. I had the feeling there was a tendency to put the
> good case into the first branch. Your order is easier to read for me as
> well, so if there are no other arguments, I'll flip it.
>

Up to you, really. It's a minor

> >
> > > +             supportHFlips_ = hflip.first;
> > > +             supportVFlips_ = vflip.first;
> > > +             flipsAlterBayerOrder_ = supportHFlips_ ? hflip.second : vflip.second;
> >
> > Regardless of the logic order of the check, this can be just an ||
>
> That's a good point, thanks.
>
> >
> > > +     } else {
> > > +             LOG(CameraSensor, Error)
> > > +                     << "Flips with differing alter bayer order properties"
> > > +                     << " are unsupported. Disabling flipping.";
> > >       }
> > >
> > > -     if (!supportFlips_)
> > > -             LOG(CameraSensor, Debug)
> > > -                     << "Camera sensor does not support horizontal/vertical flip";
> > > -
> > >       /*
> > >        * Make sure the required selection targets are supported.
> > >        *
> > > @@ -759,14 +766,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 +946,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 +955,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;
> > > +     }
> > > +
> >
> > Minor comment apart, we're introducing the requirement that both flips, if
> > present, shall expose the same MODIFY_LAYOUT flag layout.
> >
> > I presume it's reasonable, but should we also add the requirement to
> > Documentation/sensor_driver_requirements.rst which currently only
> > reports:
> >
> > 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.
>
> I'm not sure about that. I think that restriction only applies to
> libcamera (I just didn't implement support for that case). On the V4L2

Maybe I missed your point, but Documentation/sensor_driver_requirements.rst
is libcamera :)

> side I don't see a need to make that restriction. And if we ever meet a
> sensor that has the MODIFY_LAYOUT flag on one axis only, we need to add
> support for that in libcamera. Or did I get you wrong?

Or we require that if a sensor supports flipping along both axes, both
(or none) should have the MODIFY_LAYOUT flag.

>
> Best regards,
> Stefan
>
> >
> > Thanks
> >   j
> >
> > >       /*
> > >        * 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..6efd72727773 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,32 @@ 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)) {
> > > +             supportHFlips_ = hflip.first;
> > > +             supportVFlips_ = vflip.first;
> > > +             flipsAlterBayerOrder_ = supportHFlips_ ? hflip.second : vflip.second;
> > > +     } else {
> > > +             LOG(CameraSensor, Error)
> > > +                     << "Flips with differing alter bayer order properties"
> > > +                     << " are unsupported. Disabling flipping.";
> > >       }
> > >
> > > -     if (!supportFlips_)
> > > -             LOG(CameraSensor, Debug)
> > > -                     << "Camera sensor does not support horizontal/vertical flip";
> > > -
> > >       /*
> > >        * 5. Discover ancillary devices.
> > >        *
> > > @@ -819,14 +827,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 +1064,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 +1073,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
> > >

Quoting Jacopo Mondi (2026-08-03 14:21:27)
> Hi Stefan
> 
> On Mon, Aug 03, 2026 at 01:03:35PM +0200, Stefan Klug wrote:
> > Hi Jacopo,
> >
> > Thank you for the review.
> >
> > Quoting Jacopo Mondi (2026-08-03 12:28:14)
> > > On Mon, Aug 03, 2026 at 11:52:47AM +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 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 2 adresses the comments from review.
> > > >
> > > > Best regards,
> > > > Stefan
> > > >
> > > >
> > > >  src/libcamera/sensor/camera_sensor_legacy.cpp | 77 ++++++++++--------
> > > >  src/libcamera/sensor/camera_sensor_raw.cpp    | 80 +++++++++++--------
> > > >  2 files changed, 90 insertions(+), 67 deletions(-)
> > > >
> > > > diff --git a/src/libcamera/sensor/camera_sensor_legacy.cpp b/src/libcamera/sensor/camera_sensor_legacy.cpp
> > > > index 6a683821f219..5dce0a6eacfe 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,32 @@ 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)) {
> > >
> > > nit: fits on one line.
> > >
> > > The idea is to only set supportHFlips_, supportVFlips_ and
> > > flipsAlterBayerOrder_ if the V4L2_CTRL_FLAG_MODIFY_LAYOUT is
> > > consistent between the two flips, provided both of them are available.
> > >
> > > Albeit equivalent, isn't it clear:
> > >
> > >         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;
> > >         }
> >
> > I don't really care too much on the order. I think I just took it from
> > Laurents proposal. I had the feeling there was a tendency to put the
> > good case into the first branch. Your order is easier to read for me as
> > well, so if there are no other arguments, I'll flip it.
> >
> 
> Up to you, really. It's a minor
> 
> > >
> > > > +             supportHFlips_ = hflip.first;
> > > > +             supportVFlips_ = vflip.first;
> > > > +             flipsAlterBayerOrder_ = supportHFlips_ ? hflip.second : vflip.second;
> > >
> > > Regardless of the logic order of the check, this can be just an ||
> >
> > That's a good point, thanks.
> >
> > >
> > > > +     } else {
> > > > +             LOG(CameraSensor, Error)
> > > > +                     << "Flips with differing alter bayer order properties"
> > > > +                     << " are unsupported. Disabling flipping.";
> > > >       }
> > > >
> > > > -     if (!supportFlips_)
> > > > -             LOG(CameraSensor, Debug)
> > > > -                     << "Camera sensor does not support horizontal/vertical flip";
> > > > -
> > > >       /*
> > > >        * Make sure the required selection targets are supported.
> > > >        *
> > > > @@ -759,14 +766,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 +946,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 +955,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;
> > > > +     }
> > > > +
> > >
> > > Minor comment apart, we're introducing the requirement that both flips, if
> > > present, shall expose the same MODIFY_LAYOUT flag layout.
> > >
> > > I presume it's reasonable, but should we also add the requirement to
> > > Documentation/sensor_driver_requirements.rst which currently only
> > > reports:
> > >
> > > 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.
> >
> > I'm not sure about that. I think that restriction only applies to
> > libcamera (I just didn't implement support for that case). On the V4L2
> 
> Maybe I missed your point, but Documentation/sensor_driver_requirements.rst
> is libcamera :)

I know, but it is also a bit like the inofficial linux sensor driver
requirements.

> 
> > side I don't see a need to make that restriction. And if we ever meet a
> > sensor that has the MODIFY_LAYOUT flag on one axis only, we need to add
> > support for that in libcamera. Or did I get you wrong?
> 
> Or we require that if a sensor supports flipping along both axes, both
> (or none) should have the MODIFY_LAYOUT flag.

Isn't that a sensor property? So we can't really enforce that?

Would you be fine with that paragraph in sensor_driver_requirements:

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 both controls are provided
libcamera currently requires them to have the same value for
V4L2_CTRL_FLAG_MODIFY_LAYOUT or ignores them otherwise.


To make it clear that it is a limitation of libcamera and not a general
requirement?

Best regards,
Stefan

> 
> >
> > Best regards,
> > Stefan
> >
> > >
> > > Thanks
> > >   j
> > >
> > > >       /*
> > > >        * 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..6efd72727773 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,32 @@ 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)) {
> > > > +             supportHFlips_ = hflip.first;
> > > > +             supportVFlips_ = vflip.first;
> > > > +             flipsAlterBayerOrder_ = supportHFlips_ ? hflip.second : vflip.second;
> > > > +     } else {
> > > > +             LOG(CameraSensor, Error)
> > > > +                     << "Flips with differing alter bayer order properties"
> > > > +                     << " are unsupported. Disabling flipping.";
> > > >       }
> > > >
> > > > -     if (!supportFlips_)
> > > > -             LOG(CameraSensor, Debug)
> > > > -                     << "Camera sensor does not support horizontal/vertical flip";
> > > > -
> > > >       /*
> > > >        * 5. Discover ancillary devices.
> > > >        *
> > > > @@ -819,14 +827,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 +1064,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 +1073,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 Stefan

On Tue, Aug 04, 2026 at 12:58:12PM +0200, Stefan Klug wrote:
> Quoting Jacopo Mondi (2026-08-03 14:21:27)
> > Hi Stefan
> >
> > On Mon, Aug 03, 2026 at 01:03:35PM +0200, Stefan Klug wrote:
> > > Hi Jacopo,
> > >
> > > Thank you for the review.
> > >
> > > Quoting Jacopo Mondi (2026-08-03 12:28:14)
> > > > On Mon, Aug 03, 2026 at 11:52:47AM +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 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 2 adresses the comments from review.
> > > > >
> > > > > Best regards,
> > > > > Stefan
> > > > >
> > > > >
> > > > >  src/libcamera/sensor/camera_sensor_legacy.cpp | 77 ++++++++++--------
> > > > >  src/libcamera/sensor/camera_sensor_raw.cpp    | 80 +++++++++++--------
> > > > >  2 files changed, 90 insertions(+), 67 deletions(-)
> > > > >
> > > > > diff --git a/src/libcamera/sensor/camera_sensor_legacy.cpp b/src/libcamera/sensor/camera_sensor_legacy.cpp
> > > > > index 6a683821f219..5dce0a6eacfe 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,32 @@ 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)) {
> > > >
> > > > nit: fits on one line.
> > > >
> > > > The idea is to only set supportHFlips_, supportVFlips_ and
> > > > flipsAlterBayerOrder_ if the V4L2_CTRL_FLAG_MODIFY_LAYOUT is
> > > > consistent between the two flips, provided both of them are available.
> > > >
> > > > Albeit equivalent, isn't it clear:
> > > >
> > > >         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;
> > > >         }
> > >
> > > I don't really care too much on the order. I think I just took it from
> > > Laurents proposal. I had the feeling there was a tendency to put the
> > > good case into the first branch. Your order is easier to read for me as
> > > well, so if there are no other arguments, I'll flip it.
> > >
> >
> > Up to you, really. It's a minor
> >
> > > >
> > > > > +             supportHFlips_ = hflip.first;
> > > > > +             supportVFlips_ = vflip.first;
> > > > > +             flipsAlterBayerOrder_ = supportHFlips_ ? hflip.second : vflip.second;
> > > >
> > > > Regardless of the logic order of the check, this can be just an ||
> > >
> > > That's a good point, thanks.
> > >
> > > >
> > > > > +     } else {
> > > > > +             LOG(CameraSensor, Error)
> > > > > +                     << "Flips with differing alter bayer order properties"
> > > > > +                     << " are unsupported. Disabling flipping.";
> > > > >       }
> > > > >
> > > > > -     if (!supportFlips_)
> > > > > -             LOG(CameraSensor, Debug)
> > > > > -                     << "Camera sensor does not support horizontal/vertical flip";
> > > > > -
> > > > >       /*
> > > > >        * Make sure the required selection targets are supported.
> > > > >        *
> > > > > @@ -759,14 +766,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 +946,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 +955,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;
> > > > > +     }
> > > > > +
> > > >
> > > > Minor comment apart, we're introducing the requirement that both flips, if
> > > > present, shall expose the same MODIFY_LAYOUT flag layout.
> > > >
> > > > I presume it's reasonable, but should we also add the requirement to
> > > > Documentation/sensor_driver_requirements.rst which currently only
> > > > reports:
> > > >
> > > > 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.
> > >
> > > I'm not sure about that. I think that restriction only applies to
> > > libcamera (I just didn't implement support for that case). On the V4L2
> >
> > Maybe I missed your point, but Documentation/sensor_driver_requirements.rst
> > is libcamera :)
>
> I know, but it is also a bit like the inofficial linux sensor driver
> requirements.
>

Well, my interpreation is that those are the libcamera requirements

> >
> > > side I don't see a need to make that restriction. And if we ever meet a
> > > sensor that has the MODIFY_LAYOUT flag on one axis only, we need to add
> > > support for that in libcamera. Or did I get you wrong?
> >
> > Or we require that if a sensor supports flipping along both axes, both
> > (or none) should have the MODIFY_LAYOUT flag.
>
> Isn't that a sensor property? So we can't really enforce that?

well, it depends...

Some drivers change the crop rectangle position by 1 pixel to maintain
the Bayer pattern constant. Some sensors instead automatically adjust
the sizes to maintain the Bayer patter without driver's intervention.

We can dictate what driver behaviour we expect, of course we can't
control the hardware. Do you think there are sensors which
automatically adjust the sizes only in 1 direction ?

>
> Would you be fine with that paragraph in sensor_driver_requirements:
>
> 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 both controls are provided
> libcamera currently requires them to have the same value for
> V4L2_CTRL_FLAG_MODIFY_LAYOUT or ignores them otherwise.
>

I would simply say:

* `V4L2_CID_HFLIP`_
* `V4L2_CID_VFLIP`_

.. _V4L2_CID_HFLIP: https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/control.html
.. _V4L2_CID_VFLIP: https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/control.html

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.

>
> To make it clear that it is a limitation of libcamera and not a general
> requirement?

I wouldn't be concerned about that. sensor_driver_requirements.rst has
always been about libcamera only, it lives in our sources and does
only apply to libcamera.

>
> Best regards,
> Stefan
>
> >
> > >
> > > Best regards,
> > > Stefan
> > >
> > > >
> > > > Thanks
> > > >   j
> > > >
> > > > >       /*
> > > > >        * 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..6efd72727773 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,32 @@ 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)) {
> > > > > +             supportHFlips_ = hflip.first;
> > > > > +             supportVFlips_ = vflip.first;
> > > > > +             flipsAlterBayerOrder_ = supportHFlips_ ? hflip.second : vflip.second;
> > > > > +     } else {
> > > > > +             LOG(CameraSensor, Error)
> > > > > +                     << "Flips with differing alter bayer order properties"
> > > > > +                     << " are unsupported. Disabling flipping.";
> > > > >       }
> > > > >
> > > > > -     if (!supportFlips_)
> > > > > -             LOG(CameraSensor, Debug)
> > > > > -                     << "Camera sensor does not support horizontal/vertical flip";
> > > > > -
> > > > >       /*
> > > > >        * 5. Discover ancillary devices.
> > > > >        *
> > > > > @@ -819,14 +827,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 +1064,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 +1073,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 Jacopo,

Quoting Jacopo Mondi (2026-08-04 13:31:52)
> Hi Stefan
> 
> On Tue, Aug 04, 2026 at 12:58:12PM +0200, Stefan Klug wrote:
> > Quoting Jacopo Mondi (2026-08-03 14:21:27)
> > > Hi Stefan
> > >
> > > On Mon, Aug 03, 2026 at 01:03:35PM +0200, Stefan Klug wrote:
> > > > Hi Jacopo,
> > > >
> > > > Thank you for the review.
> > > >
> > > > Quoting Jacopo Mondi (2026-08-03 12:28:14)
> > > > > On Mon, Aug 03, 2026 at 11:52:47AM +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 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 2 adresses the comments from review.
> > > > > >
> > > > > > Best regards,
> > > > > > Stefan
> > > > > >
> > > > > >
> > > > > >  src/libcamera/sensor/camera_sensor_legacy.cpp | 77 ++++++++++--------
> > > > > >  src/libcamera/sensor/camera_sensor_raw.cpp    | 80 +++++++++++--------
> > > > > >  2 files changed, 90 insertions(+), 67 deletions(-)
> > > > > >
> > > > > > diff --git a/src/libcamera/sensor/camera_sensor_legacy.cpp b/src/libcamera/sensor/camera_sensor_legacy.cpp
> > > > > > index 6a683821f219..5dce0a6eacfe 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,32 @@ 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)) {
> > > > >
> > > > > nit: fits on one line.
> > > > >
> > > > > The idea is to only set supportHFlips_, supportVFlips_ and
> > > > > flipsAlterBayerOrder_ if the V4L2_CTRL_FLAG_MODIFY_LAYOUT is
> > > > > consistent between the two flips, provided both of them are available.
> > > > >
> > > > > Albeit equivalent, isn't it clear:
> > > > >
> > > > >         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;
> > > > >         }
> > > >
> > > > I don't really care too much on the order. I think I just took it from
> > > > Laurents proposal. I had the feeling there was a tendency to put the
> > > > good case into the first branch. Your order is easier to read for me as
> > > > well, so if there are no other arguments, I'll flip it.
> > > >
> > >
> > > Up to you, really. It's a minor
> > >
> > > > >
> > > > > > +             supportHFlips_ = hflip.first;
> > > > > > +             supportVFlips_ = vflip.first;
> > > > > > +             flipsAlterBayerOrder_ = supportHFlips_ ? hflip.second : vflip.second;
> > > > >
> > > > > Regardless of the logic order of the check, this can be just an ||
> > > >
> > > > That's a good point, thanks.
> > > >
> > > > >
> > > > > > +     } else {
> > > > > > +             LOG(CameraSensor, Error)
> > > > > > +                     << "Flips with differing alter bayer order properties"
> > > > > > +                     << " are unsupported. Disabling flipping.";
> > > > > >       }
> > > > > >
> > > > > > -     if (!supportFlips_)
> > > > > > -             LOG(CameraSensor, Debug)
> > > > > > -                     << "Camera sensor does not support horizontal/vertical flip";
> > > > > > -
> > > > > >       /*
> > > > > >        * Make sure the required selection targets are supported.
> > > > > >        *
> > > > > > @@ -759,14 +766,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 +946,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 +955,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;
> > > > > > +     }
> > > > > > +
> > > > >
> > > > > Minor comment apart, we're introducing the requirement that both flips, if
> > > > > present, shall expose the same MODIFY_LAYOUT flag layout.
> > > > >
> > > > > I presume it's reasonable, but should we also add the requirement to
> > > > > Documentation/sensor_driver_requirements.rst which currently only
> > > > > reports:
> > > > >
> > > > > 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.
> > > >
> > > > I'm not sure about that. I think that restriction only applies to
> > > > libcamera (I just didn't implement support for that case). On the V4L2
> > >
> > > Maybe I missed your point, but Documentation/sensor_driver_requirements.rst
> > > is libcamera :)
> >
> > I know, but it is also a bit like the inofficial linux sensor driver
> > requirements.
> >
> 
> Well, my interpreation is that those are the libcamera requirements
> 
> > >
> > > > side I don't see a need to make that restriction. And if we ever meet a
> > > > sensor that has the MODIFY_LAYOUT flag on one axis only, we need to add
> > > > support for that in libcamera. Or did I get you wrong?
> > >
> > > Or we require that if a sensor supports flipping along both axes, both
> > > (or none) should have the MODIFY_LAYOUT flag.
> >
> > Isn't that a sensor property? So we can't really enforce that?
> 
> well, it depends...
> 
> Some drivers change the crop rectangle position by 1 pixel to maintain
> the Bayer pattern constant. Some sensors instead automatically adjust
> the sizes to maintain the Bayer patter without driver's intervention.
> 
> We can dictate what driver behaviour we expect, of course we can't
> control the hardware. Do you think there are sensors which
> automatically adjust the sizes only in 1 direction ?

I never saw such a thing. So the discussion is quite academic :-) We can
revise as soon as we see such a sensor.

> 
> >
> > Would you be fine with that paragraph in sensor_driver_requirements:
> >
> > 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 both controls are provided
> > libcamera currently requires them to have the same value for
> > V4L2_CTRL_FLAG_MODIFY_LAYOUT or ignores them otherwise.
> >
> 
> I would simply say:
> 
> * `V4L2_CID_HFLIP`_
> * `V4L2_CID_VFLIP`_
> 
> .. _V4L2_CID_HFLIP: https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/control.html
> .. _V4L2_CID_VFLIP: https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/control.html
> 
> 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.

As written above, I'm not aware of any sensor doing it differently on
one axis. So we will most likely never hit that question. I'll go for
this version.

> 
> >
> > To make it clear that it is a limitation of libcamera and not a general
> > requirement?
> 
> I wouldn't be concerned about that. sensor_driver_requirements.rst has
> always been about libcamera only, it lives in our sources and does
> only apply to libcamera.

For me the question was more the distinction between a requirement
because we purposely require it or the current status quo, because
something is not yet implement. But as we'll most likely never hit that
case it doesn't matter.

Best regards,
Stefan

> 
> >
> > Best regards,
> > Stefan
> >
> > >
> > > >
> > > > Best regards,
> > > > Stefan
> > > >
> > > > >
> > > > > Thanks
> > > > >   j
> > > > >
> > > > > >       /*
> > > > > >        * 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..6efd72727773 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,32 @@ 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)) {
> > > > > > +             supportHFlips_ = hflip.first;
> > > > > > +             supportVFlips_ = vflip.first;
> > > > > > +             flipsAlterBayerOrder_ = supportHFlips_ ? hflip.second : vflip.second;
> > > > > > +     } else {
> > > > > > +             LOG(CameraSensor, Error)
> > > > > > +                     << "Flips with differing alter bayer order properties"
> > > > > > +                     << " are unsupported. Disabling flipping.";
> > > > > >       }
> > > > > >
> > > > > > -     if (!supportFlips_)
> > > > > > -             LOG(CameraSensor, Debug)
> > > > > > -                     << "Camera sensor does not support horizontal/vertical flip";
> > > > > > -
> > > > > >       /*
> > > > > >        * 5. Discover ancillary devices.
> > > > > >        *
> > > > > > @@ -819,14 +827,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 +1064,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 +1073,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
> > > > > >