[v4] sensor: camera_sensor_legacy/raw: Support separate H/V flips
diff mbox series

Message ID 20260807075935.2553938-1-stefan.klug@ideasonboard.com
State New
Headers show
Series
  • [v4] sensor: camera_sensor_legacy/raw: Support separate H/V flips
Related show

Commit Message

Stefan Klug Aug. 7, 2026, 7:59 a.m. UTC 
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>
Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

---

Changes in v4:
- Fixed bug that used V4L2_CID_HFLIP instead of V4L2_CID_VFLIP
  introduced in v2
- Collected tag

Changes in v3:
- Swapped if and replaced ternary operator by || in support logic
- Improved description in sensor_driver_requirements

Changes in v2:
- Replaced duplicate query flip code with lambda
- Reworked apply flip code to call setControls only once
- Replaced a & by &&
- Applied the change to CameraSensorRaw as well
- Small comment/logging changes
---

Hi all,

I had this patch somewhere in my tree for a long time. Now I stumbled
over this issue again and had to manually rebase the changes. So maybe
it is time to upstream :-)

Version 4 fixes a copy paste bug introduced in v2.

Best regards,
Stefan

 Documentation/sensor_driver_requirements.rst  |  2 +
 src/libcamera/sensor/camera_sensor_legacy.cpp | 76 ++++++++++--------
 src/libcamera/sensor/camera_sensor_raw.cpp    | 79 +++++++++++--------
 3 files changed, 90 insertions(+), 67 deletions(-)

Patch
diff mbox series 

diff --git a/Documentation/sensor_driver_requirements.rst b/Documentation/sensor_driver_requirements.rst
index 0e516b34a215..1caa2f96d682 100644
--- a/Documentation/sensor_driver_requirements.rst
+++ b/Documentation/sensor_driver_requirements.rst
@@ -73,6 +73,8 @@  In order to support rotating the image the sensor driver should support
 The controls must be writable from userspace. In case of a RAW Bayer sensors,
 drivers should correctly report if vertical/horizontal flips modify the Bayer
 pattern ordering by reporting the `V4L2_CTRL_FLAG_MODIFY_LAYOUT` control flag.
+If the flag is set for one of the two controls, it shall be set for the other
+one as well.
 
 The sensor driver should implement support for the V4L2 Selection API,
 specifically it should implement support for the
diff --git a/src/libcamera/sensor/camera_sensor_legacy.cpp b/src/libcamera/sensor/camera_sensor_legacy.cpp
index 6a683821f219..4338abc523a8 100644
--- a/src/libcamera/sensor/camera_sensor_legacy.cpp
+++ b/src/libcamera/sensor/camera_sensor_legacy.cpp
@@ -133,7 +133,8 @@  private:
 	Size pixelArraySize_;
 	Rectangle activeArea_;
 	const BayerFormat *bayerFormat_;
-	bool supportFlips_;
+	bool supportHFlips_;
+	bool supportVFlips_;
 	bool flipsAlterBayerOrder_;
 	Orientation mountingOrientation_;
 
@@ -153,7 +154,7 @@  private:
 
 CameraSensorLegacy::CameraSensorLegacy(const MediaEntity *entity)
 	: entity_(entity), pad_(UINT_MAX), staticProps_(nullptr),
-	  bayerFormat_(nullptr), supportFlips_(false),
+	  bayerFormat_(nullptr), supportHFlips_(false), supportVFlips_(false),
 	  flipsAlterBayerOrder_(false), properties_(properties::properties)
 {
 }
@@ -359,26 +360,31 @@  int CameraSensorLegacy::validateSensorDriver()
 		}
 	}
 
-	/*
-	 * Verify if sensor supports horizontal/vertical flips
-	 *
-	 * \todo Handle horizontal and vertical flips independently.
-	 */
-	const struct v4l2_query_ext_ctrl *hflipInfo = subdev_->controlInfo(V4L2_CID_HFLIP);
-	const struct v4l2_query_ext_ctrl *vflipInfo = subdev_->controlInfo(V4L2_CID_VFLIP);
-	if (hflipInfo && !(hflipInfo->flags & V4L2_CTRL_FLAG_READ_ONLY) &&
-	    vflipInfo && !(vflipInfo->flags & V4L2_CTRL_FLAG_READ_ONLY)) {
-		supportFlips_ = true;
+	/* Verify if sensor supports horizontal/vertical flips. */
+	auto queryFlip = [&](uint32_t id, const char *name) -> std::pair<bool, bool> {
+		const struct v4l2_query_ext_ctrl *flipInfo = subdev_->controlInfo(id);
+		if (!flipInfo || flipInfo->flags & V4L2_CTRL_FLAG_READ_ONLY) {
+			LOG(CameraSensor, Debug)
+				<< "Camera sensor does not support " << name << " flip";
+			return { false, false };
+		}
 
-		if (hflipInfo->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT ||
-		    vflipInfo->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT)
-			flipsAlterBayerOrder_ = true;
+		return { true, flipInfo->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT };
+	};
+
+	auto hflip = queryFlip(V4L2_CID_HFLIP, "horizontal");
+	auto vflip = queryFlip(V4L2_CID_VFLIP, "vertical");
+
+	if (hflip.first && vflip.first && (hflip.second != vflip.second)) {
+		LOG(CameraSensor, Error)
+			<< "Flips with differing alter bayer order properties"
+			<< " are unsupported. Disabling flipping.";
+	} else {
+		supportHFlips_ = hflip.first;
+		supportVFlips_ = vflip.first;
+		flipsAlterBayerOrder_ = hflip.second || vflip.second;
 	}
 
-	if (!supportFlips_)
-		LOG(CameraSensor, Debug)
-			<< "Camera sensor does not support horizontal/vertical flip";
-
 	/*
 	 * Make sure the required selection targets are supported.
 	 *
@@ -759,14 +765,16 @@  CameraSensorLegacy::getFormat(Span<const unsigned int> mbusCodes,
 int CameraSensorLegacy::setFormat(V4L2SubdeviceFormat *format, Transform transform)
 {
 	/* Configure flips if the sensor supports that. */
-	if (supportFlips_) {
-		ControlList flipCtrls(subdev_->controls());
-
+	ControlList flipCtrls(subdev_->controls());
+	if (supportHFlips_)
 		flipCtrls.set(V4L2_CID_HFLIP,
 			      static_cast<int32_t>(!!(transform & Transform::HFlip)));
+
+	if (supportVFlips_)
 		flipCtrls.set(V4L2_CID_VFLIP,
 			      static_cast<int32_t>(!!(transform & Transform::VFlip)));
 
+	if (!flipCtrls.empty()) {
 		int ret = subdev_->setControls(&flipCtrls);
 		if (ret)
 			return ret;
@@ -937,17 +945,8 @@  int CameraSensorLegacy::sensorInfo(IPACameraSensorInfo *info) const
 Transform CameraSensorLegacy::computeTransform(Orientation *orientation) const
 {
 	/*
-	 * If we cannot do any flips we cannot change the native camera mounting
-	 * orientation.
-	 */
-	if (!supportFlips_) {
-		*orientation = mountingOrientation_;
-		return Transform::Identity;
-	}
-
-	/*
-	 * Now compute the required transform to obtain 'orientation' starting
-	 * from the mounting rotation.
+	 * Compute the required transform to obtain 'orientation' starting from
+	 * the mounting rotation.
 	 *
 	 * As a note:
 	 * 	orientation / mountingOrientation_ = transform
@@ -955,6 +954,17 @@  Transform CameraSensorLegacy::computeTransform(Orientation *orientation) const
 	 */
 	Transform transform = *orientation / mountingOrientation_;
 
+	/* If we cannot do the required flips, fall back to identity. */
+	if (!supportHFlips_ && !!(transform & Transform::HFlip)) {
+		*orientation = mountingOrientation_;
+		return Transform::Identity;
+	}
+
+	if (!supportVFlips_ && !!(transform & Transform::VFlip)) {
+		*orientation = mountingOrientation_;
+		return Transform::Identity;
+	}
+
 	/*
 	 * If transform contains any Transpose we cannot do it, so adjust
 	 * 'orientation' to report the image native orientation and return Identity.
diff --git a/src/libcamera/sensor/camera_sensor_raw.cpp b/src/libcamera/sensor/camera_sensor_raw.cpp
index 10eba0331fe8..849423dd20cf 100644
--- a/src/libcamera/sensor/camera_sensor_raw.cpp
+++ b/src/libcamera/sensor/camera_sensor_raw.cpp
@@ -144,7 +144,8 @@  private:
 	Size pixelArraySize_;
 	Rectangle activeArea_;
 	BayerFormat::Order cfaPattern_;
-	bool supportFlips_;
+	bool supportHFlips_;
+	bool supportVFlips_;
 	bool flipsAlterBayerOrder_;
 	Orientation mountingOrientation_;
 
@@ -162,8 +163,9 @@  private:
  */
 
 CameraSensorRaw::CameraSensorRaw(const MediaEntity *entity)
-	: entity_(entity), staticProps_(nullptr), supportFlips_(false),
-	  flipsAlterBayerOrder_(false), properties_(properties::properties)
+	: entity_(entity), staticProps_(nullptr), supportHFlips_(false),
+	  supportVFlips_(false), flipsAlterBayerOrder_(false),
+	  properties_(properties::properties)
 {
 }
 
@@ -479,26 +481,31 @@  std::optional<int> CameraSensorRaw::init()
 		return { ret };
 	}
 
-	/*
-	 * Verify if sensor supports horizontal/vertical flips
-	 *
-	 * \todo Handle horizontal and vertical flips independently.
-	 */
-	const struct v4l2_query_ext_ctrl *hflipInfo = subdev_->controlInfo(V4L2_CID_HFLIP);
-	const struct v4l2_query_ext_ctrl *vflipInfo = subdev_->controlInfo(V4L2_CID_VFLIP);
-	if (hflipInfo && !(hflipInfo->flags & V4L2_CTRL_FLAG_READ_ONLY) &&
-	    vflipInfo && !(vflipInfo->flags & V4L2_CTRL_FLAG_READ_ONLY)) {
-		supportFlips_ = true;
+	/* Verify if sensor supports horizontal/vertical flips. */
+	auto queryFlip = [&](uint32_t id, const char *name) -> std::pair<bool, bool> {
+		const struct v4l2_query_ext_ctrl *flipInfo = subdev_->controlInfo(id);
+		if (!flipInfo || flipInfo->flags & V4L2_CTRL_FLAG_READ_ONLY) {
+			LOG(CameraSensor, Debug)
+				<< "Camera sensor does not support " << name << " flip";
+			return { false, false };
+		}
 
-		if (hflipInfo->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT ||
-		    vflipInfo->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT)
-			flipsAlterBayerOrder_ = true;
+		return { true, flipInfo->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT };
+	};
+
+	auto hflip = queryFlip(V4L2_CID_HFLIP, "horizontal");
+	auto vflip = queryFlip(V4L2_CID_VFLIP, "vertical");
+
+	if (hflip.first && vflip.first && (hflip.second != vflip.second)) {
+		LOG(CameraSensor, Error)
+			<< "Flips with differing alter bayer order properties"
+			<< " are unsupported. Disabling flipping.";
+	} else {
+		supportHFlips_ = hflip.first;
+		supportVFlips_ = vflip.first;
+		flipsAlterBayerOrder_ = hflip.second || vflip.second;
 	}
 
-	if (!supportFlips_)
-		LOG(CameraSensor, Debug)
-			<< "Camera sensor does not support horizontal/vertical flip";
-
 	/*
 	 * 5. Discover ancillary devices.
 	 *
@@ -819,14 +826,16 @@  CameraSensorRaw::getFormat(Span<const unsigned int> mbusCodes,
 int CameraSensorRaw::setFormat(V4L2SubdeviceFormat *format, Transform transform)
 {
 	/* Configure flips if the sensor supports that. */
-	if (supportFlips_) {
-		ControlList flipCtrls(subdev_->controls());
-
+	ControlList flipCtrls(subdev_->controls());
+	if (supportHFlips_)
 		flipCtrls.set(V4L2_CID_HFLIP,
 			      static_cast<int32_t>(!!(transform & Transform::HFlip)));
+
+	if (supportVFlips_)
 		flipCtrls.set(V4L2_CID_VFLIP,
 			      static_cast<int32_t>(!!(transform & Transform::VFlip)));
 
+	if (!flipCtrls.empty()) {
 		int ret = subdev_->setControls(&flipCtrls);
 		if (ret)
 			return ret;
@@ -1054,17 +1063,8 @@  int CameraSensorRaw::sensorInfo(IPACameraSensorInfo *info) const
 Transform CameraSensorRaw::computeTransform(Orientation *orientation) const
 {
 	/*
-	 * If we cannot do any flips we cannot change the native camera mounting
-	 * orientation.
-	 */
-	if (!supportFlips_) {
-		*orientation = mountingOrientation_;
-		return Transform::Identity;
-	}
-
-	/*
-	 * Now compute the required transform to obtain 'orientation' starting
-	 * from the mounting rotation.
+	 * Compute the required transform to obtain 'orientation' starting from
+	 * the mounting rotation.
 	 *
 	 * As a note:
 	 * 	orientation / mountingOrientation_ = transform
@@ -1072,6 +1072,17 @@  Transform CameraSensorRaw::computeTransform(Orientation *orientation) const
 	 */
 	Transform transform = *orientation / mountingOrientation_;
 
+	/* If we cannot do the required flips, fall back to identity. */
+	if (!supportHFlips_ && !!(transform & Transform::HFlip)) {
+		*orientation = mountingOrientation_;
+		return Transform::Identity;
+	}
+
+	if (!supportVFlips_ && !!(transform & Transform::VFlip)) {
+		*orientation = mountingOrientation_;
+		return Transform::Identity;
+	}
+
 	/*
 	 * If transform contains any Transpose we cannot do it, so adjust
 	 * 'orientation' to report the image native orientation and return Identity.