Re: [PATCH v11 43/66] media: uapi: Add V4L2_CID_BINNING control for binning configuration

[Sakari Ailus <sakari.ailus@linux.intel.com>]

Hi Jacopo,

Thank you for the review.

On Mon, Sep 01, 2025 at 07:27:13PM +0200, Jacopo Mondi wrote:
> Hi Sakari
> 
> On Mon, Aug 25, 2025 at 12:50:44PM +0300, Sakari Ailus wrote:
> > Add V4L2_CID_BINNING control for configuring binning and enumerating a
> > camera sensor's binning capabilities. The control combines horizontal and
> > vertical binning into a single control as the two are generally related.
> >
> > New drivers should use this control to configure binning.
> >
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> > ---
> >  .../media/drivers/camera-sensor.rst           | 12 ++++++++
> >  .../media/v4l/ext-ctrls-camera.rst            | 29 +++++++++++++++++++
> >  drivers/media/v4l2-core/v4l2-ctrls-defs.c     |  2 ++
> >  include/uapi/linux/v4l2-controls.h            |  1 +
> >  4 files changed, 44 insertions(+)
> >
> > diff --git a/Documentation/userspace-api/media/drivers/camera-sensor.rst b/Documentation/userspace-api/media/drivers/camera-sensor.rst
> > index 39f3f91c6733..ef1f51862980 100644
> > --- a/Documentation/userspace-api/media/drivers/camera-sensor.rst
> > +++ b/Documentation/userspace-api/media/drivers/camera-sensor.rst
> > @@ -120,6 +120,18 @@ 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 has traditionally been configured using :ref:`the compose selection
> > +rectangle <v4l2-selection-targets-table>`. The :ref:`V4L2_CID_BINNING
> 
> I would add "... >` on the internal sink pad that represents the image
> data source.

The compose target has this semantics on the common raw sensor model but
this text is more generic than that.

> 
> > +<v4l2-cid-camera-sensor-binning>` is also available for binning configuration and
> 
> missing 'control' after the :ref:`...` so that it reads
> 
> The V4L2_CID_BINNING -control- is also available
> 
> when rendered

Yes.

> 
> 
> > +users should use it when it's available. Drivers supporting the control shall
> 
> I would not make it just about users, but also for drivers.
> 
> "The V4L2_CID_BINNING control has been introduced as the preferred way
> for drivers and userspace applications for configuring binning."

The UAPI documentation is also relevant for drivers but usually not the
other way around.

> 
> > +also support the compose rectangle, albeit the rectangle may be read-only when
> > +the control is present.
> > +
> > +Binning isn't affected by flipping.
> > +
> >  .. _media_using_camera_sensor_drivers_embedded_data:
> >
> >  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 cdc515c60468..18b484ff5d75 100644
> > --- a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> > +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> > @@ -672,3 +672,32 @@ enum v4l2_scene_mode -
> >
> >      As modes differ for each sensor, menu items are not standardized by this
> >      control and are left to the programmer.
> > +
> > +.. _v4l2-cid-camera-sensor-binning:
> > +
> > +``V4L2_CID_BINNING_FACTORS (integer menu)``
> > +
> > +    Horizontal and vertical binning factors. Binning combines several
> > +    horizontal, vertical or both pixel values into a single pixel. It is a way
> > +    to scale an image. Binning typically produces fairly good quality output.
> > +
> > +    Determines both horizontal and vertical binning factors for a camera
> > +    sensor. The values are encoded in the following way:
> > +
> > +.. flat-table::
> > +    :header-rows:  1
> > +    :stub-columns: 0
> > +
> > +    * - Bits
> > +      - Synopsis
> > +    * - 48--63
> > +      - Horizontal binning numerator.
> > +    * - 32--47
> > +      - Horizontal binning denominator.
> > +    * - 16--31
> > +      - Vertical binning numerator.
> > +    * - 0--15
> > +      - Vertical binning denominator.
> > +
> > +For instance, a value of ``0x0001000300020003`` indicates binning by 3
> > +(horizontally) * 3/2 (vertically).
> 
> indicates an horizontal binning factor of 3 and 3/2 vertical one.

I prefer the text in the patch.

I also realised that the patch is adding the control to the old camera
control class; I'll move it to the image source class.

> 
> > diff --git a/drivers/media/v4l2-core/v4l2-ctrls-defs.c b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> > index a7ea380de5ee..5e1c28850e87 100644
> > --- a/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> > +++ b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> > @@ -1087,6 +1087,7 @@ const char *v4l2_ctrl_get_name(u32 id)
> >  	case V4L2_CID_CAMERA_ORIENTATION:	return "Camera Orientation";
> >  	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";
> >
> >  	/* FM Radio Modulator controls */
> >  	/* Keep the order of the 'case's the same as in v4l2-controls.h! */
> > @@ -1427,6 +1428,7 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
> >  	case V4L2_CID_HDR_SENSOR_MODE:
> >  		*type = V4L2_CTRL_TYPE_MENU;
> >  		break;
> > +	case V4L2_CID_BINNING_FACTORS:
> >  	case V4L2_CID_LINK_FREQ:
> >  		*type = V4L2_CTRL_TYPE_INTEGER_MENU;
> >  		break;
> > diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
> > index e25c9d669687..90f47f4780e5 100644
> > --- a/include/uapi/linux/v4l2-controls.h
> > +++ b/include/uapi/linux/v4l2-controls.h
> > @@ -1101,6 +1101,7 @@ enum v4l2_auto_focus_range {
> >  #define V4L2_CID_CAMERA_SENSOR_ROTATION		(V4L2_CID_CAMERA_CLASS_BASE+35)
> >
> >  #define V4L2_CID_HDR_SENSOR_MODE		(V4L2_CID_CAMERA_CLASS_BASE+36)
> > +#define V4L2_CID_BINNING_FACTORS		(V4L2_CID_CAMERA_CLASS_BASE+37)
> 
> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

Thank you!

-- 
Kind regards,

Sakari Ailus