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

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

Hi David, Stefan,

Thank you for the review.

On Mon, Oct 20, 2025 at 03:42:22PM +0100, David Plowman wrote:
> Hi Sakari, Stefan and everyone!
> 
> On Fri, 10 Oct 2025 at 11:21, Stefan Klug <stefan.klug@ideasonboard.com> wrote:
> >
> > 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
> 
> Even later to the party, for which my apologies!

Well, it's not yet in upstream, but hopefully gets there in not so distant
future.

> 
> > 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'd like to second the notion that we should give summing/averaging
> some thought. We (Raspberry Pi in this context!) already have user
> space code that tries to guess this information from only obliquely
> related information from the driver. It would clearly be nice to know
> this unambiguously across all sensors.
> 
> The case in point is the IMX708 (as found in the Raspberry Pi Camera
> Module 3). Here there are two versions of the "2x2 binning mode".
> 
> There is the "summation" mode, in which pixels have 4x the level of
> the full resolution mode, as I think you would expect. This means you
> have to use 1/4 the combined exposure * gain value in the full
> resolution mode to get an equivalently bright image.
> 
> Then there's the "averaging" mode. In this mode we get 2x the level of
> the full resolution mode. You might infer that one of the dimensions
> (either horizontal or vertical) always sums, and on the other you have
> a choice - though the datasheet doesn't explicitly confirm that
> anywhere so far as I can see.
> 
> A different but related observation is that I have a feeling I've come
> across sensors that may do both averaging and summation in a single
> dimension. So you'd get 1/4 the output width (or height), but 2x the
> pixel level because one factor of two sums, and the other averages.
> Perhaps someone else can confirm whether I'm just imagining that...?

CCS considers binning factors and mode independent but indeed in reality
things aren't always that simple. Many sensors do binning in both analogue
and digital domains and obviously analogue binning requires hardware
support whereas digitally it's not quite as much hardware-bound. For
instance, doing e.g. analogue binning vertically and digital binning
horizontally makes sense and vertical binning may be limited to what is
supported in analogue domain. Selecting between summing and averaging may
well not be possible in analogue domain either.

Reflecting this in UAPI makes sense.

You can make guesses on whether binning takes place vertically or
horizontally based on blanking limits but it'd be nice if this was properly
documented in sensor datasheets (it almost never is).

I was thinking of providing binning factors via the BINNING_FACTORS control
as defined above and add another control to select the binning mode
(integer menu again) and have the value depend on BINNING_FACTORS control.
Gathering a structured representation in userspace this way could be more
practical than by just providing a flat list (by enumerating the menu
items) from the kernel.

Either way, standardising binning modes somehow would seem reasonable for
the benefit of the userspace, even though in low level implementations
there are obviously differences. What does matter especially is whether
summing or averaging or a combination of two is used; perhaps we could say
the values are multiplied by the multiple of horizontal and vertical
binning factors divided by a divisor we provide via a control (sometimes
settable!). E.g. 2x2 binning and binning divider 4 would mean averaging and
1 summing. The value would obviously depend on the binning factors.

-- 
Kind regards,

Sakari Ailus