Re: [PATCH v11 44/66] media: uapi: Add controls for sub-sampling configuration

[Jacopo Mondi <jacopo.mondi@ideasonboard.com>]

Hello

On Mon, Mar 30, 2026 at 07:20:14PM +0300, Sakari Ailus wrote:
> Hi Benjamin,
>
> On Mon, Mar 30, 2026 at 05:00:44PM +0200, Benjamin Mugnier wrote:
> > Hi Sakari
> >
> > Le 26/03/2026 à 17:54, Sakari Ailus a écrit :
> > > Hi Benjamin,
> > >
> > > On Thu, Mar 26, 2026 at 05:05:47PM +0100, Benjamin Mugnier wrote:
> > >> Hi Sakari,
> > >>
> > >> I'm so late to the party I'm afraid the door might already be closed :)
> > >
> > > The patches aren't merged so it's still possible to change things...
> > >
> > >>
> > >> Le 25/08/2025 à 11:50, Sakari Ailus a écrit :
> > >>> Sub-sampling is a way to decrease the data rates after the pixel array by
> > >>> systematically discarding some samples, either vertically or horizontally
> > >>> or both. Add two controls for the purpose and document them. The
> > >>> sub-sampling configuration is taken into account in the compose rectangle.
> > >>>
> > >>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > >>> Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> > >>> ---
> > >>>  .../media/drivers/camera-sensor.rst              | 16 +++++++++++++---
> > >>>  .../userspace-api/media/v4l/ext-ctrls-camera.rst | 11 +++++++++++
> > >>>  drivers/media/v4l2-core/v4l2-ctrls-defs.c        |  2 ++
> > >>>  include/uapi/linux/v4l2-controls.h               |  2 ++
> > >>>  4 files changed, 28 insertions(+), 3 deletions(-)
> > >>>
> > >>> diff --git a/Documentation/userspace-api/media/drivers/camera-sensor.rst b/Documentation/userspace-api/media/drivers/camera-sensor.rst
> > >>> index ef1f51862980..b0ad0d778396 100644
> > >>> --- a/Documentation/userspace-api/media/drivers/camera-sensor.rst
> > >>> +++ b/Documentation/userspace-api/media/drivers/camera-sensor.rst
> > >>> @@ -120,8 +120,8 @@ values programmed by the register sequences. The default values of these
> > >>>  controls shall be 0 (disabled). Especially these controls shall not be inverted,
> > >>>  independently of the sensor's mounting rotation.
> > >>>
> > >>> -Binning
> > >>> --------
> > >>> +Binning and sub-sampling
> > >>> +------------------------
> > >>>
> > >>>  Binning has traditionally been configured using :ref:`the compose selection
> > >>>  rectangle <v4l2-selection-targets-table>`. The :ref:`V4L2_CID_BINNING
> > >>> @@ -130,7 +130,17 @@ users should use it when it's available. Drivers supporting the control shall
> > >>>  also support the compose rectangle, albeit the rectangle may be read-only when
> > >>>  the control is present.
> > >>>
> > >>> -Binning isn't affected by flipping.
> > >>> +Sub-sampling is often supported as part of a camera sensor's binning
> > >>> +functionality and performed after the binning operation. Sub-sampling typically
> > >>> +produces quality-wise worse results than binning. Sub-sampling factors are
> > >>> +independent horizontally and vertically and they are controlled using two
> > >>> +controls, :ref:`V4L2_CID_SUBSAMPLING_HORIZONTAL and
> > >>> +V4L2_CID_SUBSAMPLING_VERTICAL <v4l2-cid-camera-sensor-subsampling>`. In
> > >>> +sub-sampling, the image size before sub-sampling is horizontally and vertically
> > >>> +divided by the respective sub-sampling factors. Drivers supporting the control shall
> > >>> +also reflect the sub-sampling configuration in the compose rectangle.
> > >>> +
> > >>> +Binning and sub-sampling aren't affected by flipping.
> > >>>
> > >>>  .. _media_using_camera_sensor_drivers_embedded_data:
> > >>>
> > >>> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> > >>> index 18b484ff5d75..577b73045bee 100644
> > >>> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> > >>> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> > >>> @@ -701,3 +701,14 @@ enum v4l2_scene_mode -
> > >>>
> > >>>  For instance, a value of ``0x0001000300020003`` indicates binning by 3
> > >>>  (horizontally) * 3/2 (vertically).
> > >>> +
> > >>> +.. _v4l2-cid-camera-sensor-subsampling:
> > >>> +
> > >>> +``V4L2_CID_SUBSAMPLING_HORIZONTAL`` and ``V4L2_CID_SUBSAMPLING_VERTICAL``
> > >>> +(integer)
> > >>> +
> > >>> +    Horizontal and vertical subsampling factors.
> > >>> +
> > >>> +    Sub-sampling is used to downscale an image, horizontally and vertically, by
> > >>> +    discarding a part of the image data. Typically sub-sampling produces lower
> > >>> +    quality images than binning.
> > >>> diff --git a/drivers/media/v4l2-core/v4l2-ctrls-defs.c b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> > >>> index 5e1c28850e87..9f8816bfffbe 100644
> > >>> --- a/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> > >>> +++ b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> > >>> @@ -1088,6 +1088,8 @@ const char *v4l2_ctrl_get_name(u32 id)
> > >>>  	case V4L2_CID_CAMERA_SENSOR_ROTATION:	return "Camera Sensor Rotation";
> > >>>  	case V4L2_CID_HDR_SENSOR_MODE:		return "HDR Sensor Mode";
> > >>>  	case V4L2_CID_BINNING_FACTORS:		return "Binning Factors";
> > >>> +	case V4L2_CID_SUBSAMPLING_HORIZONTAL:	return "Sub-Sampling Factor, Horizontal";
> > >>> +	case V4L2_CID_SUBSAMPLING_VERTICAL:	return "Sub-Sampling Factor, Vertical";
> > >>
> > >> I applied this patch in my tree and I'm currently implementing binning
> > >> and sub-sampling support for the vd55g1. First of all, thank you for
> > >> your work.
> > >>
> > >> The vd55g1 camera sensor only performs sub-sampling in both horizontal
> > >> and vertical axes at the same time, no decoupling. For now I  modified
> > >> the patch to have a single control handling sub-sampling as a whole.
> > >> Maybe there is a more idiomatic way to approach this kind of problem
> > >> that I might have missed ? Kind of 'linking' controls together if that
> > >> make sense.
> > >>
> > >> I'm unusure if having both sub-sampling axes tied together is common
> > >> behavior among camera sensors or not. Do you think this is something
> > >> worth addressing in the serie ?
> > >
> > > The reason a single control was added for the purpose was that it does
> > > allow for implementations that tie horizontal and vertical binning factors.
> > > I'm not sure which one is more common but both can be supported this way.
> > > Untied factors typically means having a large number of options though.
> > >
> > > In CCS horizontal and vertical binning are related, too.
> > >
> > > How many options do you have for each?
> > >
> >
> > I have binning x2 and x4, and subsampling x2, x4, and x8. But I only
> > care for x2 for both at the moment. You can find more at H.2.53
> > READOUT_CTRL in the user manual [1].
>
> I think I misread your explanation earlier, missing two perhaps impotant
> letters "d" and "e". Another option would be to implement a third
> sub-sampling control that would apply to both directions: this is about
> controlling the sensor only so I don't see this being an issue.
>
> Controls are a bit awkward when it comes to direct dependencies.

Can't a driver that has its vertical/horizontal binning factors
coupled together use v4l2_ctrl_cluster() on V4L2_CID_SUBSAMPLING_HORIZONTAL
and V4L2_CID_SUBSAMPLING_VERTICAL ?

Thanks
   j

>
> >
> > I came up with something like this :
> >
> >   case V4L2_CID_BINNING_FACTORS:
> >     if (ctrl->val != ctrl->cur.val) {
> >       sensor->subsampling_h_ctrl->cur.val = 1;
> >       sensor->subsampling_h_ctrl->val = 1;
> >       sensor->subsampling_v_ctrl->cur.val = 1;
> >       sensor->subsampling_v_ctrl->val = 1;
> >     }
> >     break;
> >   case V4L2_CID_SUBSAMPLING_HORIZONTAL:
> >     if (ctrl->val != ctrl->cur.val) {
> >       sensor->binning_ctrl->cur.val = 0;
> >       sensor->binning_ctrl->val = 0;
> >       sensor->subsampling_v_ctrl->cur.val = ctrl->val;
> >       sensor->subsampling_v_ctrl->val = ctrl->val;
> >     }
> >     break;
> >   case V4L2_CID_SUBSAMPLING_VERTICAL:
> >     if (ctrl->val != ctrl->cur.val) {
> >       sensor->binning_ctrl->cur.val = 0;
> >       sensor->binning_ctrl->val = 0;
> >       sensor->subsampling_h_ctrl->cur.val = ctrl->val;
> >       sensor->subsampling_h_ctrl->val = ctrl->val;
> >     }
> >     break;
> >
> > Allowing to reset the binning if the sub subsampling is set or the other
> > way around, and tying both sub samplings together.
> >
> > I don't find it very elegant but it works. Are we fine with this kind of
> > code for sensors having these restrictions ? As you may have guessed
> > they applies for most ST cameras.
> >
> >
> > [1]
> > https://www.st.com/resource/en/user_manual/um3224-how-to-integrate-and-configure-the-vd55g1-sensor-stmicroelectronics.pdf
>
> --
> Kind regards,
>
> Sakari Ailus