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

[Stefan Klug <stefan.klug@ideasonboard.com>]

Hi Sakari,

Quoting Sakari Ailus (2025-08-25 11:50:44)
> 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
> +<v4l2-cid-camera-sensor-binning>` is also available for binning configuration and
> +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.
> +
>  .. _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.

I know I'm late to the game, but I'd like to mention it anyways. I
expect there will be usecases where we'd like to distinguish between
different binning strategies (e.g. summing to increase SNR and
weighting). But I believe it will be difficult to actually standardize
on specific binning strategies as these are very sensor specific. On the
other hand the available strategies will be tied to the binning modes.
What about sparing some bits to encode the binning strategy in a sensor
specific manner?

I don't think we'll ever need more than 1 byte per
numerator/denominator. We could even spend 12 bits per value, and would
still have 16 bits left for additional information.

What do you think?

Best regards,
Stefan

> +
> +For instance, a value of ``0x0001000300020003`` indicates binning by 3
> +(horizontally) * 3/2 (vertically).
> 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)
>  
>  /* FM Modulator class control IDs */
>  
> -- 
> 2.47.2
> 
>