Re: [PATCH] media: i2c: ov08d10: add get_selection support

From: Sakari Ailus

Date: Mon Oct 12 2026 - 06:07:38 EST

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@xxxxxxxxxxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxxxxxxxxxx>
> ---
> 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