* [PATCH] media: i2c: ov08d10: add get_selection support
@ 2026-10-11 19:55 Max Bretschneider via B4 Relay
2026-10-12 10:01 ` Sakari Ailus
0 siblings, 1 reply; 3+ messages in thread
From: Max Bretschneider via B4 Relay @ 2026-10-11 19:55 UTC (permalink / raw)
To: Jimmy Su, Matthias Fend, Sakari Ailus, Mauro Carvalho Chehab
Cc: linux-media, linux-kernel, Max Bretschneider
From: Max Bretschneider <max.bretschneider@leica-geosystems.com>
The driver does not implement .get_selection, so it never reports the
analogue crop rectangle of the active mode. Userspace has no way to
learn which region of the pixel array a given mode reads out, and
cannot assume a fixed one, because the modes differ: the full mode
reads 3280x2460 at offset (4,8), while the 3264x2448 and the 2x2
binned 1632x1224 modes read 3264x2448 at offset (12,16).
Consumers that derive the sensor scale from the analogue crop and the
output size therefore compute a wrong result for the cropped and
binned modes, and may reject those configurations.
Track the analogue crop rectangle per mode, store it in the subdev
state when the format is set, and report it through .get_selection.
Also report V4L2_SEL_TGT_CROP_DEFAULT, V4L2_SEL_TGT_CROP_BOUNDS and
V4L2_SEL_TGT_NATIVE_SIZE from the pixel array geometry. Implement
.init_state so a newly allocated state holds a valid format and crop,
and add the v4l2_subdev_init_finalize() and v4l2_subdev_cleanup()
calls required to use the subdev active state.
The pixel array geometry and the per-mode offsets are taken from the
sensor datasheet and the recommended reference register sets.
Signed-off-by: Max Bretschneider <max.bretschneider@leica-geosystems.com>
---
Tested on a Hexagon/Leica CS50 (i.MX8MP, rkisp1) running 6.16.12 with the
equivalent of this patch applied, where it fixes configuration of the 2x2
binned 1632x1224 mode with libcamera's rkisp1 CAC algorithm enabled.
Compile-tested against media/next.
Modelled on imx335.
---
drivers/media/i2c/ov08d10.c | 74 ++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 73 insertions(+), 1 deletion(-)
diff --git a/drivers/media/i2c/ov08d10.c b/drivers/media/i2c/ov08d10.c
index 0710d2243cc0..b32a1246df45 100644
--- a/drivers/media/i2c/ov08d10.c
+++ b/drivers/media/i2c/ov08d10.c
@@ -65,6 +65,14 @@
#define OV08D10_REG_FLIP_OPT 0x32
#define OV08D10_REG_FLIP_MASK 0x3
+/* OV08D10 native and active pixel array size */
+#define OV08D10_NATIVE_WIDTH 3288
+#define OV08D10_NATIVE_HEIGHT 2476
+#define OV08D10_PIXEL_ARRAY_LEFT 4
+#define OV08D10_PIXEL_ARRAY_TOP 8
+#define OV08D10_PIXEL_ARRAY_WIDTH 3280
+#define OV08D10_PIXEL_ARRAY_HEIGHT 2464
+
#define to_ov08d10(_sd) container_of(_sd, struct ov08d10, sd)
struct ov08d10_reg {
@@ -110,6 +118,9 @@ struct ov08d10_mode {
/* Number of data lanes */
u8 data_lanes;
+
+ /* Analogue crop rectangle, in native sensor coordinates */
+ struct v4l2_rect crop;
};
/* 3280x2460, 3264x2448 need 720Mbps/lane, 2 lanes - 19.2 MHz */
@@ -630,6 +641,7 @@ static const struct ov08d10_lane_cfg lane_cfg_2 = {
},
.link_freq_index = 0,
.data_lanes = 2,
+ .crop = { .left = 4, .top = 8, .width = 3280, .height = 2460 },
},
{
.width = 3264,
@@ -643,6 +655,7 @@ static const struct ov08d10_lane_cfg lane_cfg_2 = {
},
.link_freq_index = 0,
.data_lanes = 2,
+ .crop = { .left = 12, .top = 16, .width = 3264, .height = 2448 },
},
{
.width = 1632,
@@ -656,6 +669,7 @@ static const struct ov08d10_lane_cfg lane_cfg_2 = {
},
.link_freq_index = 1,
.data_lanes = 2,
+ .crop = { .left = 12, .top = 16, .width = 3264, .height = 2448 },
}}
};
@@ -955,6 +969,36 @@ static int ov08d10_set_ctrl(struct v4l2_ctrl *ctrl)
return ret;
}
+static int ov08d10_get_selection(struct v4l2_subdev *sd,
+ const struct v4l2_subdev_client_info *ci,
+ struct v4l2_subdev_state *sd_state,
+ struct v4l2_subdev_selection *sel)
+{
+ switch (sel->target) {
+ case V4L2_SEL_TGT_CROP:
+ sel->r = *v4l2_subdev_state_get_crop(sd_state, 0);
+ return 0;
+
+ case V4L2_SEL_TGT_NATIVE_SIZE:
+ sel->r.left = 0;
+ sel->r.top = 0;
+ sel->r.width = OV08D10_NATIVE_WIDTH;
+ sel->r.height = OV08D10_NATIVE_HEIGHT;
+ return 0;
+
+ case V4L2_SEL_TGT_CROP_DEFAULT:
+ case V4L2_SEL_TGT_CROP_BOUNDS:
+ sel->r.left = OV08D10_PIXEL_ARRAY_LEFT;
+ sel->r.top = OV08D10_PIXEL_ARRAY_TOP;
+ sel->r.width = OV08D10_PIXEL_ARRAY_WIDTH;
+ sel->r.height = OV08D10_PIXEL_ARRAY_HEIGHT;
+ return 0;
+
+ default:
+ return -EINVAL;
+ }
+}
+
static const struct v4l2_ctrl_ops ov08d10_ctrl_ops = {
.s_ctrl = ov08d10_set_ctrl,
};
@@ -1194,6 +1238,9 @@ static int ov08d10_set_format(struct v4l2_subdev *sd,
mutex_lock(&ov08d10->mutex);
ov08d10_update_pad_format(ov08d10, mode, &fmt->format);
+
+ *v4l2_subdev_state_get_crop(sd_state, fmt->pad) = mode->crop;
+
if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
*v4l2_subdev_state_get_format(sd_state, fmt->pad) =
fmt->format;
@@ -1297,6 +1344,19 @@ static int ov08d10_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
return 0;
}
+static int ov08d10_init_state(struct v4l2_subdev *sd,
+ struct v4l2_subdev_state *sd_state)
+{
+ struct ov08d10 *ov08d10 = to_ov08d10(sd);
+ const struct ov08d10_mode *mode = &ov08d10->priv_lane->sp_modes[0];
+
+ *v4l2_subdev_state_get_crop(sd_state, 0) = mode->crop;
+ ov08d10_update_pad_format(ov08d10, mode,
+ v4l2_subdev_state_get_format(sd_state, 0));
+
+ return 0;
+}
+
static const struct v4l2_subdev_video_ops ov08d10_video_ops = {
.s_stream = ov08d10_set_stream,
};
@@ -1306,6 +1366,7 @@ static const struct v4l2_subdev_pad_ops ov08d10_pad_ops = {
.get_fmt = ov08d10_get_format,
.enum_mbus_code = ov08d10_enum_mbus_code,
.enum_frame_size = ov08d10_enum_frame_size,
+ .get_selection = ov08d10_get_selection,
};
static const struct v4l2_subdev_ops ov08d10_subdev_ops = {
@@ -1315,6 +1376,7 @@ static const struct v4l2_subdev_ops ov08d10_subdev_ops = {
static const struct v4l2_subdev_internal_ops ov08d10_internal_ops = {
.open = ov08d10_open,
+ .init_state = ov08d10_init_state,
};
static int ov08d10_power_off(struct device *dev)
@@ -1470,6 +1532,7 @@ static void ov08d10_remove(struct i2c_client *client)
struct ov08d10 *ov08d10 = to_ov08d10(sd);
v4l2_async_unregister_subdev(sd);
+ v4l2_subdev_cleanup(sd);
media_entity_cleanup(&sd->entity);
v4l2_ctrl_handler_free(sd->ctrl_handler);
pm_runtime_disable(ov08d10->dev);
@@ -1565,17 +1628,26 @@ static int ov08d10_probe(struct i2c_client *client)
pm_runtime_set_active(ov08d10->dev);
pm_runtime_enable(ov08d10->dev);
+ ret = v4l2_subdev_init_finalize(&ov08d10->sd);
+ if (ret) {
+ dev_err(ov08d10->dev, "failed to init subdev state: %d\n", ret);
+ goto probe_error_media_entity_cleanup;
+ }
+
ret = v4l2_async_register_subdev_sensor(&ov08d10->sd);
if (ret < 0) {
dev_err(ov08d10->dev, "failed to register V4L2 subdev: %d\n",
ret);
- goto probe_error_media_entity_cleanup;
+ goto probe_error_subdev_cleanup;
}
pm_runtime_idle(ov08d10->dev);
return 0;
+probe_error_subdev_cleanup:
+ v4l2_subdev_cleanup(&ov08d10->sd);
+
probe_error_media_entity_cleanup:
pm_runtime_disable(ov08d10->dev);
pm_runtime_set_suspended(ov08d10->dev);
---
base-commit: 2dcdfb625c3b8fe87454e19dfbc54b3e3f0ad70e
change-id: 20260929-ov08d10-get-selection-caa7b6cb8dfe
Best regards,
--
Max Bretschneider <max.bretschneider@leica-geosystems.com>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] media: i2c: ov08d10: add get_selection support
2026-10-11 19:55 [PATCH] media: i2c: ov08d10: add get_selection support Max Bretschneider via B4 Relay
@ 2026-10-12 10:01 ` Sakari Ailus
2026-10-23 22:28 ` BRETSCHNEIDER Max
0 siblings, 1 reply; 3+ messages in thread
From: Sakari Ailus @ 2026-10-12 10:01 UTC (permalink / raw)
To: max.bretschneider
Cc: Jimmy Su, Matthias Fend, Mauro Carvalho Chehab, linux-media,
linux-kernel
Hi Max,
Thank you for the patch.
On Sun, Oct 11, 2026 at 09:55:23PM +0200, Max Bretschneider via B4 Relay wrote:
> From: Max Bretschneider <max.bretschneider@leica-geosystems.com>
>
> The driver does not implement .get_selection, so it never reports the
> analogue crop rectangle of the active mode. Userspace has no way to
> learn which region of the pixel array a given mode reads out, and
> cannot assume a fixed one, because the modes differ: the full mode
> reads 3280x2460 at offset (4,8), while the 3264x2448 and the 2x2
> binned 1632x1224 modes read 3264x2448 at offset (12,16).
>
> Consumers that derive the sensor scale from the analogue crop and the
> output size therefore compute a wrong result for the cropped and
> binned modes, and may reject those configurations.
>
> Track the analogue crop rectangle per mode, store it in the subdev
> state when the format is set, and report it through .get_selection.
> Also report V4L2_SEL_TGT_CROP_DEFAULT, V4L2_SEL_TGT_CROP_BOUNDS and
> V4L2_SEL_TGT_NATIVE_SIZE from the pixel array geometry. Implement
> .init_state so a newly allocated state holds a valid format and crop,
> and add the v4l2_subdev_init_finalize() and v4l2_subdev_cleanup()
> calls required to use the subdev active state.
>
> The pixel array geometry and the per-mode offsets are taken from the
> sensor datasheet and the recommended reference register sets.
>
> Signed-off-by: Max Bretschneider <max.bretschneider@leica-geosystems.com>
> ---
> Tested on a Hexagon/Leica CS50 (i.MX8MP, rkisp1) running 6.16.12 with the
> equivalent of this patch applied, where it fixes configuration of the 2x2
> binned 1632x1224 mode with libcamera's rkisp1 CAC algorithm enabled.
> Compile-tested against media/next.
Can you test on a newer kernel? That being said, there are unlikely to be
issues without that, but it's better to be sure.
>
> Modelled on imx335.
> ---
> drivers/media/i2c/ov08d10.c | 74 ++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 73 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/i2c/ov08d10.c b/drivers/media/i2c/ov08d10.c
> index 0710d2243cc0..b32a1246df45 100644
> --- a/drivers/media/i2c/ov08d10.c
> +++ b/drivers/media/i2c/ov08d10.c
> @@ -65,6 +65,14 @@
> #define OV08D10_REG_FLIP_OPT 0x32
> #define OV08D10_REG_FLIP_MASK 0x3
>
> +/* OV08D10 native and active pixel array size */
> +#define OV08D10_NATIVE_WIDTH 3288
> +#define OV08D10_NATIVE_HEIGHT 2476
> +#define OV08D10_PIXEL_ARRAY_LEFT 4
> +#define OV08D10_PIXEL_ARRAY_TOP 8
> +#define OV08D10_PIXEL_ARRAY_WIDTH 3280
> +#define OV08D10_PIXEL_ARRAY_HEIGHT 2464
> +
> #define to_ov08d10(_sd) container_of(_sd, struct ov08d10, sd)
>
> struct ov08d10_reg {
> @@ -110,6 +118,9 @@ struct ov08d10_mode {
>
> /* Number of data lanes */
> u8 data_lanes;
> +
> + /* Analogue crop rectangle, in native sensor coordinates */
> + struct v4l2_rect crop;
> };
>
> /* 3280x2460, 3264x2448 need 720Mbps/lane, 2 lanes - 19.2 MHz */
> @@ -630,6 +641,7 @@ static const struct ov08d10_lane_cfg lane_cfg_2 = {
> },
> .link_freq_index = 0,
> .data_lanes = 2,
> + .crop = { .left = 4, .top = 8, .width = 3280, .height = 2460 },
> },
> {
> .width = 3264,
> @@ -643,6 +655,7 @@ static const struct ov08d10_lane_cfg lane_cfg_2 = {
> },
> .link_freq_index = 0,
> .data_lanes = 2,
> + .crop = { .left = 12, .top = 16, .width = 3264, .height = 2448 },
> },
> {
> .width = 1632,
> @@ -656,6 +669,7 @@ static const struct ov08d10_lane_cfg lane_cfg_2 = {
> },
> .link_freq_index = 1,
> .data_lanes = 2,
> + .crop = { .left = 12, .top = 16, .width = 3264, .height = 2448 },
> }}
> };
>
> @@ -955,6 +969,36 @@ static int ov08d10_set_ctrl(struct v4l2_ctrl *ctrl)
> return ret;
> }
>
> +static int ov08d10_get_selection(struct v4l2_subdev *sd,
> + const struct v4l2_subdev_client_info *ci,
> + struct v4l2_subdev_state *sd_state,
> + struct v4l2_subdev_selection *sel)
> +{
> + switch (sel->target) {
> + case V4L2_SEL_TGT_CROP:
> + sel->r = *v4l2_subdev_state_get_crop(sd_state, 0);
> + return 0;
> +
> + case V4L2_SEL_TGT_NATIVE_SIZE:
> + sel->r.left = 0;
> + sel->r.top = 0;
> + sel->r.width = OV08D10_NATIVE_WIDTH;
> + sel->r.height = OV08D10_NATIVE_HEIGHT;
> + return 0;
> +
> + case V4L2_SEL_TGT_CROP_DEFAULT:
> + case V4L2_SEL_TGT_CROP_BOUNDS:
> + sel->r.left = OV08D10_PIXEL_ARRAY_LEFT;
> + sel->r.top = OV08D10_PIXEL_ARRAY_TOP;
> + sel->r.width = OV08D10_PIXEL_ARRAY_WIDTH;
> + sel->r.height = OV08D10_PIXEL_ARRAY_HEIGHT;
> + return 0;
> +
> + default:
> + return -EINVAL;
> + }
> +}
> +
> static const struct v4l2_ctrl_ops ov08d10_ctrl_ops = {
> .s_ctrl = ov08d10_set_ctrl,
> };
> @@ -1194,6 +1238,9 @@ static int ov08d10_set_format(struct v4l2_subdev *sd,
>
> mutex_lock(&ov08d10->mutex);
> ov08d10_update_pad_format(ov08d10, mode, &fmt->format);
> +
> + *v4l2_subdev_state_get_crop(sd_state, fmt->pad) = mode->crop;
> +
> if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
> *v4l2_subdev_state_get_format(sd_state, fmt->pad) =
> fmt->format;
> @@ -1297,6 +1344,19 @@ static int ov08d10_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
> return 0;
> }
>
> +static int ov08d10_init_state(struct v4l2_subdev *sd,
> + struct v4l2_subdev_state *sd_state)
> +{
> + struct ov08d10 *ov08d10 = to_ov08d10(sd);
> + const struct ov08d10_mode *mode = &ov08d10->priv_lane->sp_modes[0];
> +
> + *v4l2_subdev_state_get_crop(sd_state, 0) = mode->crop;
> + ov08d10_update_pad_format(ov08d10, mode,
> + v4l2_subdev_state_get_format(sd_state, 0));
> +
> + return 0;
> +}
> +
> static const struct v4l2_subdev_video_ops ov08d10_video_ops = {
> .s_stream = ov08d10_set_stream,
> };
> @@ -1306,6 +1366,7 @@ static const struct v4l2_subdev_pad_ops ov08d10_pad_ops = {
> .get_fmt = ov08d10_get_format,
> .enum_mbus_code = ov08d10_enum_mbus_code,
> .enum_frame_size = ov08d10_enum_frame_size,
> + .get_selection = ov08d10_get_selection,
It'd be nice to have the same ordering as in the driver (and also not place
the get_selection callback in the middle of control handling).
> };
>
> static const struct v4l2_subdev_ops ov08d10_subdev_ops = {
> @@ -1315,6 +1376,7 @@ static const struct v4l2_subdev_ops ov08d10_subdev_ops = {
>
> static const struct v4l2_subdev_internal_ops ov08d10_internal_ops = {
> .open = ov08d10_open,
> + .init_state = ov08d10_init_state,
> };
>
> static int ov08d10_power_off(struct device *dev)
> @@ -1470,6 +1532,7 @@ static void ov08d10_remove(struct i2c_client *client)
> struct ov08d10 *ov08d10 = to_ov08d10(sd);
>
> v4l2_async_unregister_subdev(sd);
> + v4l2_subdev_cleanup(sd);
> media_entity_cleanup(&sd->entity);
> v4l2_ctrl_handler_free(sd->ctrl_handler);
> pm_runtime_disable(ov08d10->dev);
> @@ -1565,17 +1628,26 @@ static int ov08d10_probe(struct i2c_client *client)
> pm_runtime_set_active(ov08d10->dev);
> pm_runtime_enable(ov08d10->dev);
>
> + ret = v4l2_subdev_init_finalize(&ov08d10->sd);
Can you convert this driver to fully utilise sub-device state, as you're
initialising it here?
There's probably not much to do, but in particular the active format should
be stored in sub-device state and the control handler's lock be used as the
state lock. Also storing the active mode in the context struct would be
nice to avoid. These should constitute a separate patch (that also includes
init_finalize()/cleanup() calls) before this one.
> + if (ret) {
> + dev_err(ov08d10->dev, "failed to init subdev state: %d\n", ret);
> + goto probe_error_media_entity_cleanup;
> + }
> +
> ret = v4l2_async_register_subdev_sensor(&ov08d10->sd);
> if (ret < 0) {
> dev_err(ov08d10->dev, "failed to register V4L2 subdev: %d\n",
> ret);
> - goto probe_error_media_entity_cleanup;
> + goto probe_error_subdev_cleanup;
> }
>
> pm_runtime_idle(ov08d10->dev);
>
> return 0;
>
> +probe_error_subdev_cleanup:
> + v4l2_subdev_cleanup(&ov08d10->sd);
> +
> probe_error_media_entity_cleanup:
> pm_runtime_disable(ov08d10->dev);
> pm_runtime_set_suspended(ov08d10->dev);
>
--
Kind regards,
Sakari Ailus
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] media: i2c: ov08d10: add get_selection support
2026-10-12 10:01 ` Sakari Ailus
@ 2026-10-23 22:28 ` BRETSCHNEIDER Max
0 siblings, 0 replies; 3+ messages in thread
From: BRETSCHNEIDER Max @ 2026-10-23 22:28 UTC (permalink / raw)
To: Sakari Ailus
Cc: Jimmy Su, Matthias Fend, Mauro Carvalho Chehab, linux-media,
linux-kernel
Hi Sakari,
thank you for the review! Sorry it took me a while to get through this. v2 follows.
> > Tested on a Hexagon/Leica CS50 (i.MX8MP, rkisp1) running 6.16.12 with the
> > equivalent of this patch applied, where it fixes configuration of the 2x2
> > binned 1632x1224 mode with libcamera's rkisp1 CAC algorithm enabled.
> > Compile-tested against media/next.
>
> Can you test on a newer kernel? That being said, there are unlikely to be
> issues without that, but it's better to be sure.
Yes I will test this on a newer kernel as well but my hardware access is currently
a bit limited, so it will probably take me a week or two.
> > + .get_selection = ov08d10_get_selection,
>
> It'd be nice to have the same ordering as in the driver (and also not place
> the get_selection callback in the middle of control handling).
Moved it out of the control handling section so it now sits with the other
pad op implementations. With that the struct order already matches the
order in the driver, so I have left the struct itself alone (apart from adding
the get_selection entry)
> > + ret = v4l2_subdev_init_finalize(&ov08d10->sd);
>
> Can you convert this driver to fully utilise sub-device state, as you're
> initialising it here?
>
> There's probably not much to do, but in particular the active format should
> be stored in sub-device state and the control handler's lock be used as the
> state lock. Also storing the active mode in the context struct would be
> nice to avoid. These should constitute a separate patch (that also includes
> init_finalize()/cleanup() calls) before this one.
Done, but I stumbled on some things which is why this grew into a small series. v2 is:
1. A fix for a NULL pointer dereference in ov08d10_get_hwcfg(), which logs
ov08d10->cur_mode->data_lanes before cur_mode is assigned in probe(). I found this
while doing patch #3. It is tagged for stable and patch 3 does depend on it, but of
course I am happy to send that seperately if you would prefer this.
2. Conversion to the subdev active state.
3. Dropping the (now-redundant) cached mode pointer and recovering the mode
now from the state (like imx{214,355} do).
4. get_selection, as in v1.
One thing about patch 2 I was unsure about and wanted to mention: it replaces the
driver's get_fmt() with v4l2_subdev_get_fmt(). The old get_fmt() recomputed the media
bus code from the flip controls on every call. The new version in patch 2 now refreshes
the stored code in set_ctrl() instead. There are other ways to deal with this which I found,
for example ov02c10, where the readout window is shifted when flipping to avoid a Bayer
order mismatch entirely. I am not fully certain but maybe this would be an option on the
ov08d10 too, although whether it would be the better option I don't know.
Also I thought it'd now make sense to split patches 2 and 3 mainly so that the media bus
code refresh part in patch 2 can be reviewed on its own. Of course I'd also be happy to
squash them if you would prefer that.
--
Thanks again and best regards,
Max
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2026-10-23 22:28 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2026-10-11 19:55 [PATCH] media: i2c: ov08d10: add get_selection support Max Bretschneider via B4 Relay
2026-10-12 10:01 ` Sakari Ailus
2026-10-23 22:28 ` BRETSCHNEIDER Max
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®