Add new standard controls as U32 arrays, for sensors with multiple
captures: V4L2_CID_EXPOSURE_MULTI, V4L2_CID_AGAIN_MULTI and
V4L2_CID_DGAIN_MULTI. These will be particularly useful for sensors
that have multiple captures, but the HDR merge is done inside the sensor,
in the end exposing a single stream, but still requiring AEC control
for all captures.

All controls are in the same class, so they could all be set
atomically via VIDIOC_S_EXT_CTRLS, this could turn out to be
useful in case of sensors with context switching.

Each element of the array will hold an u32 value (exposure or gain)
for one capture. The size of the array is up to the sensor driver which
will implement the controls and initialize them via v4l2_ctrl_new_custom().
With this approach, the user-space will have to set valid values
for all the captures represented in the array.

The v4l2-core only supports one scalar min/max/step value for the
entire array, and each element is validated and adjusted to be within
these bounds in v4l2_ctrl_type_op_validate(). The significance for the
maximum value for the exposure control could be "the max value for the
long exposure" or "the max value for the sum of all exposures". If none
of these is ok, the sensor driver can adjust the values as supported and
the user space can use the TRY operation to query the sensor for the
minimum or maximum values.

Mirela Rabulea (2):
  LF-15161-6: media: Add exposure and gain controls for multiple
    captures
  LF-15161-7: Documentation: media: Describe exposure and gain controls
    for multiple captures

 .../media/v4l/ext-ctrls-image-source.rst             | 12 ++++++++++++
 drivers/media/v4l2-core/v4l2-ctrls-defs.c            |  8 ++++++++
 include/uapi/linux/v4l2-controls.h                   |  3 +++
 3 files changed, 23 insertions(+)

-- 
2.43.0

Hi Mirela,

On Fri, Jul 11, 2025 at 01:05:42AM +0300, Mirela Rabulea wrote:
> Add new standard controls as U32 arrays, for sensors with multiple
> captures: V4L2_CID_EXPOSURE_MULTI, V4L2_CID_AGAIN_MULTI and
> V4L2_CID_DGAIN_MULTI. These will be particularly useful for sensors
> that have multiple captures, but the HDR merge is done inside the sensor,
> in the end exposing a single stream, but still requiring AEC control
> for all captures.

It's also useful for sensors supporting DOL or DCG with HDR merge being
performed outside of the sensor.

> All controls are in the same class, so they could all be set
> atomically via VIDIOC_S_EXT_CTRLS, this could turn out to be
> useful in case of sensors with context switching.

Agreed, we should be able to set them all. Are we still unable to set
controls from multiple classes atomatically ? I thought that limitation
has been lifted.

> Each element of the array will hold an u32 value (exposure or gain)
> for one capture. The size of the array is up to the sensor driver which
> will implement the controls and initialize them via v4l2_ctrl_new_custom().
> With this approach, the user-space will have to set valid values
> for all the captures represented in the array.

I'll comment on the controls themselves in patch 2/2.

> The v4l2-core only supports one scalar min/max/step value for the
> entire array, and each element is validated and adjusted to be within
> these bounds in v4l2_ctrl_type_op_validate(). The significance for the
> maximum value for the exposure control could be "the max value for the
> long exposure" or "the max value for the sum of all exposures". If none
> of these is ok, the sensor driver can adjust the values as supported and
> the user space can use the TRY operation to query the sensor for the
> minimum or maximum values.

Hmmmm... I wonder if we would need the ability to report different
limits for different array elements. There may be over-engineering
though, my experience with libcamera is that userspace really needs
detailed information about those controls, and attempting to convey the
precise information through the kernel-userspace API is bound to fail.
That's why we implement a sensor database in libcamera, with information
about how to convert control values to real gain and exposure time.
Exposing (close to) raw register values and letting userspace handle the
rest may be better.

> Mirela Rabulea (2):
>   LF-15161-6: media: Add exposure and gain controls for multiple
>     captures
>   LF-15161-7: Documentation: media: Describe exposure and gain controls
>     for multiple captures

Did you forget to remove the LF-* identifiers ? :-)

> 
>  .../media/v4l/ext-ctrls-image-source.rst             | 12 ++++++++++++
>  drivers/media/v4l2-core/v4l2-ctrls-defs.c            |  8 ++++++++
>  include/uapi/linux/v4l2-controls.h                   |  3 +++
>  3 files changed, 23 insertions(+)

-- 
Regards,

Laurent Pinchart

On Wed, Jul 16, 2025 at 02:59:54AM +0300, Laurent Pinchart wrote:
> On Fri, Jul 11, 2025 at 01:05:42AM +0300, Mirela Rabulea wrote:
> > Add new standard controls as U32 arrays, for sensors with multiple
> > captures: V4L2_CID_EXPOSURE_MULTI, V4L2_CID_AGAIN_MULTI and
> > V4L2_CID_DGAIN_MULTI. These will be particularly useful for sensors
> > that have multiple captures, but the HDR merge is done inside the sensor,
> > in the end exposing a single stream, but still requiring AEC control
> > for all captures.
> 
> It's also useful for sensors supporting DOL or DCG with HDR merge being
> performed outside of the sensor.

Regarless of where HDR merge is implemented, we will also need controls
to select the HDR mode. We have V4L2_CID_HDR_SENSOR_MODE, which doesn't
standardize the values, and that's not good enough. At least for DOL and
DCG with HDR merge implemented outside of the sensor, we need to
standardize the modes.

Can you tell which sensor(s) you're working with ?

> > All controls are in the same class, so they could all be set
> > atomically via VIDIOC_S_EXT_CTRLS, this could turn out to be
> > useful in case of sensors with context switching.
> 
> Agreed, we should be able to set them all. Are we still unable to set
> controls from multiple classes atomatically ? I thought that limitation
> has been lifted.
> 
> > Each element of the array will hold an u32 value (exposure or gain)
> > for one capture. The size of the array is up to the sensor driver which
> > will implement the controls and initialize them via v4l2_ctrl_new_custom().
> > With this approach, the user-space will have to set valid values
> > for all the captures represented in the array.
> 
> I'll comment on the controls themselves in patch 2/2.
> 
> > The v4l2-core only supports one scalar min/max/step value for the
> > entire array, and each element is validated and adjusted to be within
> > these bounds in v4l2_ctrl_type_op_validate(). The significance for the
> > maximum value for the exposure control could be "the max value for the
> > long exposure" or "the max value for the sum of all exposures". If none
> > of these is ok, the sensor driver can adjust the values as supported and
> > the user space can use the TRY operation to query the sensor for the
> > minimum or maximum values.
> 
> Hmmmm... I wonder if we would need the ability to report different
> limits for different array elements. There may be over-engineering
> though, my experience with libcamera is that userspace really needs
> detailed information about those controls, and attempting to convey the
> precise information through the kernel-userspace API is bound to fail.
> That's why we implement a sensor database in libcamera, with information
> about how to convert control values to real gain and exposure time.
> Exposing (close to) raw register values and letting userspace handle the
> rest may be better.
> 
> > Mirela Rabulea (2):
> >   LF-15161-6: media: Add exposure and gain controls for multiple
> >     captures
> >   LF-15161-7: Documentation: media: Describe exposure and gain controls
> >     for multiple captures
> 
> Did you forget to remove the LF-* identifiers ? :-)
> 
> > 
> >  .../media/v4l/ext-ctrls-image-source.rst             | 12 ++++++++++++
> >  drivers/media/v4l2-core/v4l2-ctrls-defs.c            |  8 ++++++++
> >  include/uapi/linux/v4l2-controls.h                   |  3 +++
> >  3 files changed, 23 insertions(+)

-- 
Regards,

Laurent Pinchart

Hi Laurent,

On 16/07/2025 02:12, Laurent Pinchart wrote:
> On Wed, Jul 16, 2025 at 02:59:54AM +0300, Laurent Pinchart wrote:
>> On Fri, Jul 11, 2025 at 01:05:42AM +0300, Mirela Rabulea wrote:
>>> Add new standard controls as U32 arrays, for sensors with multiple
>>> captures: V4L2_CID_EXPOSURE_MULTI, V4L2_CID_AGAIN_MULTI and
>>> V4L2_CID_DGAIN_MULTI. These will be particularly useful for sensors
>>> that have multiple captures, but the HDR merge is done inside the sensor,
>>> in the end exposing a single stream, but still requiring AEC control
>>> for all captures.
>>
>> It's also useful for sensors supporting DOL or DCG with HDR merge being
>> performed outside of the sensor.
> 
> Regarless of where HDR merge is implemented, we will also need controls
> to select the HDR mode. We have V4L2_CID_HDR_SENSOR_MODE, which doesn't
> standardize the values, and that's not good enough. At least for DOL and
> DCG with HDR merge implemented outside of the sensor, we need to
> standardize the modes.

For the HDR-capable sensors with the HDR merge implemented outside, the 
short capture(s) are likely implemented as separate streams, in order to 
match the raw camera sensor model.
In that case, the SDR/HDR mode switch, when supported, can be done by 
configuring the sensor device internal route for the short capture stream.

You mentioned the need to be able to select the HDR mode in a standard 
way. Could you elaborate on the foreseen usage: would it be to select 
SDR/HDR operation, to select between different HDR sub-modes, to inform 
user space about HDR capability... ?

> 
> Can you tell which sensor(s) you're working with ?
> 
>>> All controls are in the same class, so they could all be set
>>> atomically via VIDIOC_S_EXT_CTRLS, this could turn out to be
>>> useful in case of sensors with context switching.
>>
>> Agreed, we should be able to set them all. Are we still unable to set
>> controls from multiple classes atomatically ? I thought that limitation
>> has been lifted.
>>
>>> Each element of the array will hold an u32 value (exposure or gain)
>>> for one capture. The size of the array is up to the sensor driver which
>>> will implement the controls and initialize them via v4l2_ctrl_new_custom().
>>> With this approach, the user-space will have to set valid values
>>> for all the captures represented in the array.
>>
>> I'll comment on the controls themselves in patch 2/2.
>>
>>> The v4l2-core only supports one scalar min/max/step value for the
>>> entire array, and each element is validated and adjusted to be within
>>> these bounds in v4l2_ctrl_type_op_validate(). The significance for the
>>> maximum value for the exposure control could be "the max value for the
>>> long exposure" or "the max value for the sum of all exposures". If none
>>> of these is ok, the sensor driver can adjust the values as supported and
>>> the user space can use the TRY operation to query the sensor for the
>>> minimum or maximum values.
>>
>> Hmmmm... I wonder if we would need the ability to report different
>> limits for different array elements. There may be over-engineering
>> though, my experience with libcamera is that userspace really needs
>> detailed information about those controls, and attempting to convey the
>> precise information through the kernel-userspace API is bound to fail.
>> That's why we implement a sensor database in libcamera, with information
>> about how to convert control values to real gain and exposure time.
>> Exposing (close to) raw register values and letting userspace handle the
>> rest may be better.
>>
>>> Mirela Rabulea (2):
>>>    LF-15161-6: media: Add exposure and gain controls for multiple
>>>      captures
>>>    LF-15161-7: Documentation: media: Describe exposure and gain controls
>>>      for multiple captures
>>
>> Did you forget to remove the LF-* identifiers ? :-)
>>
>>>
>>>   .../media/v4l/ext-ctrls-image-source.rst             | 12 ++++++++++++
>>>   drivers/media/v4l2-core/v4l2-ctrls-defs.c            |  8 ++++++++
>>>   include/uapi/linux/v4l2-controls.h                   |  3 +++
>>>   3 files changed, 23 insertions(+)
> 
> --
> Regards,
> 
> Laurent Pinchart

Thanks,
Julien

On Tue, Jul 22, 2025 at 10:46:16AM +0200, Julien Vuillaumier wrote:
> On 16/07/2025 02:12, Laurent Pinchart wrote:
> > On Wed, Jul 16, 2025 at 02:59:54AM +0300, Laurent Pinchart wrote:
> >> On Fri, Jul 11, 2025 at 01:05:42AM +0300, Mirela Rabulea wrote:
> >>> Add new standard controls as U32 arrays, for sensors with multiple
> >>> captures: V4L2_CID_EXPOSURE_MULTI, V4L2_CID_AGAIN_MULTI and
> >>> V4L2_CID_DGAIN_MULTI. These will be particularly useful for sensors
> >>> that have multiple captures, but the HDR merge is done inside the sensor,
> >>> in the end exposing a single stream, but still requiring AEC control
> >>> for all captures.
> >>
> >> It's also useful for sensors supporting DOL or DCG with HDR merge being
> >> performed outside of the sensor.
> > 
> > Regarless of where HDR merge is implemented, we will also need controls
> > to select the HDR mode. We have V4L2_CID_HDR_SENSOR_MODE, which doesn't
> > standardize the values, and that's not good enough. At least for DOL and
> > DCG with HDR merge implemented outside of the sensor, we need to
> > standardize the modes.
> 
> For the HDR-capable sensors with the HDR merge implemented outside, the 
> short capture(s) are likely implemented as separate streams, in order to 
> match the raw camera sensor model.

Yes, that's my expectation. They should use a different data type or a
different virtual channel (I expect most sensors to support both
options).

> In that case, the SDR/HDR mode switch, when supported, can be done by 
> configuring the sensor device internal route for the short capture stream.

That's an option too, but it won't allow us to select between different
HDR modes. For instance, the AR0830 supports both DOL (2 exposures) and
DCG (2 gains). We would need a way to select between those two modes.

> You mentioned the need to be able to select the HDR mode in a standard 
> way. Could you elaborate on the foreseen usage: would it be to select 
> SDR/HDR operation, to select between different HDR sub-modes, to inform 
> user space about HDR capability... ?

Both. From a libcamera perspective, I want standardized controls for
this, to avoid sensor-specific code as much as possible.

> > Can you tell which sensor(s) you're working with ?
> > 
> >>> All controls are in the same class, so they could all be set
> >>> atomically via VIDIOC_S_EXT_CTRLS, this could turn out to be
> >>> useful in case of sensors with context switching.
> >>
> >> Agreed, we should be able to set them all. Are we still unable to set
> >> controls from multiple classes atomatically ? I thought that limitation
> >> has been lifted.
> >>
> >>> Each element of the array will hold an u32 value (exposure or gain)
> >>> for one capture. The size of the array is up to the sensor driver which
> >>> will implement the controls and initialize them via v4l2_ctrl_new_custom().
> >>> With this approach, the user-space will have to set valid values
> >>> for all the captures represented in the array.
> >>
> >> I'll comment on the controls themselves in patch 2/2.
> >>
> >>> The v4l2-core only supports one scalar min/max/step value for the
> >>> entire array, and each element is validated and adjusted to be within
> >>> these bounds in v4l2_ctrl_type_op_validate(). The significance for the
> >>> maximum value for the exposure control could be "the max value for the
> >>> long exposure" or "the max value for the sum of all exposures". If none
> >>> of these is ok, the sensor driver can adjust the values as supported and
> >>> the user space can use the TRY operation to query the sensor for the
> >>> minimum or maximum values.
> >>
> >> Hmmmm... I wonder if we would need the ability to report different
> >> limits for different array elements. There may be over-engineering
> >> though, my experience with libcamera is that userspace really needs
> >> detailed information about those controls, and attempting to convey the
> >> precise information through the kernel-userspace API is bound to fail.
> >> That's why we implement a sensor database in libcamera, with information
> >> about how to convert control values to real gain and exposure time.
> >> Exposing (close to) raw register values and letting userspace handle the
> >> rest may be better.
> >>
> >>> Mirela Rabulea (2):
> >>>    LF-15161-6: media: Add exposure and gain controls for multiple
> >>>      captures
> >>>    LF-15161-7: Documentation: media: Describe exposure and gain controls
> >>>      for multiple captures
> >>
> >> Did you forget to remove the LF-* identifiers ? :-)
> >>
> >>>
> >>>   .../media/v4l/ext-ctrls-image-source.rst             | 12 ++++++++++++
> >>>   drivers/media/v4l2-core/v4l2-ctrls-defs.c            |  8 ++++++++
> >>>   include/uapi/linux/v4l2-controls.h                   |  3 +++
> >>>   3 files changed, 23 insertions(+)

-- 
Regards,

Laurent Pinchart

Hi Laurent and all,

On 7/23/25 17:00, Laurent Pinchart wrote:

> 
> On Tue, Jul 22, 2025 at 10:46:16AM +0200, Julien Vuillaumier wrote:
>> On 16/07/2025 02:12, Laurent Pinchart wrote:
>>> On Wed, Jul 16, 2025 at 02:59:54AM +0300, Laurent Pinchart wrote:
>>>> On Fri, Jul 11, 2025 at 01:05:42AM +0300, Mirela Rabulea wrote:
>>>>> Add new standard controls as U32 arrays, for sensors with multiple
>>>>> captures: V4L2_CID_EXPOSURE_MULTI, V4L2_CID_AGAIN_MULTI and
>>>>> V4L2_CID_DGAIN_MULTI. These will be particularly useful for sensors
>>>>> that have multiple captures, but the HDR merge is done inside the sensor,
>>>>> in the end exposing a single stream, but still requiring AEC control
>>>>> for all captures.
>>>>
>>>> It's also useful for sensors supporting DOL or DCG with HDR merge being
>>>> performed outside of the sensor.
>>>
>>> Regarless of where HDR merge is implemented, we will also need controls
>>> to select the HDR mode. We have V4L2_CID_HDR_SENSOR_MODE, which doesn't
>>> standardize the values, and that's not good enough. At least for DOL and
>>> DCG with HDR merge implemented outside of the sensor, we need to
>>> standardize the modes.
>>
>> For the HDR-capable sensors with the HDR merge implemented outside, the
>> short capture(s) are likely implemented as separate streams, in order to
>> match the raw camera sensor model.
> 
> Yes, that's my expectation. They should use a different data type or a
> different virtual channel (I expect most sensors to support both
> options).
> 
>> In that case, the SDR/HDR mode switch, when supported, can be done by
>> configuring the sensor device internal route for the short capture stream.
> 
> That's an option too, but it won't allow us to select between different
> HDR modes. For instance, the AR0830 supports both DOL (2 exposures) and
> DCG (2 gains). We would need a way to select between those two modes.
> 
>> You mentioned the need to be able to select the HDR mode in a standard
>> way. Could you elaborate on the foreseen usage: would it be to select
>> SDR/HDR operation, to select between different HDR sub-modes, to inform
>> user space about HDR capability... ?
> 
> Both. From a libcamera perspective, I want standardized controls for
> this, to avoid sensor-specific code as much as possible.

This sounds complicated to achieve, at least with the one existing 
V4L2_CID_HDR_SENSOR_MODE control. There are a multitude of modes and 
technologies used around HDR sensors.

A few things we can handle with existing API's:
- number of exposures (the ones that have a separated stream)
- bitdepth (by mbus code, can give a hint on compression type)

There are other aspects we need to offer user-space some controls on, in 
order to be able to select the desired mode:
-number of exposures, even when not exposed as a separate stream
-number of captures (which in some cases may be different than the 
number of exposures)
-type of gain, or number of gains (DCG, LCG, HCG)
-type: sequential/ staggered / interleaved
-type of companding (none, PWL, other types?)
-LFM indication
-LFM replacement enable/disabled
-SPD data present (Split-Pixel Detection, as LFM enhancement)

For ox03c10, the sensor hdr mode can be determined by these factors: 
number of exposures (dual/triple), staggered/unstaggered, companded (pwl 
20/16/14/12)/uncompanded, LFM/LFM+SPD/none, with all combinations 
possible. HDR data is on VC0, LFM/SPD if enabled on VC1.

For os08a20 there is staggered hdr mode or sequential hdr mode (via 
context switching, up to 4 set each group having different exposure/gain 
sets). For staggered HDR mode there are 2 possible output modes: on 
separate 2 virtual channels for long/short exposure, or on single VC 
with dummy lines.

For ov2775, besides no hdr(either HCG or LCG), there is single exposure 
hdr (DCG) or dual exposure hdr (DCG + VS). The DCG data may be combined 
or not (HCG+LCG). Compression may apply.

I'm sure there are a lot of other fancy HDR related technologies around.
Do you think this can be standardized? Up to what level of detail? I 
think most sensor drivers will only support a limited number of hdr 
modes, out of the multitude supported by the hardware. Parameters that 
may be relevant for one sensor may have no relevance for others.
What exactly is disturbing with the current approach, where each driver 
defines the hdr modes it supports, and what do you expect to have for 
libcamera?

Is this standardization talk something you would like to conclude in the 
context of multi-controls, or can it go as a separate topic? I propose 
divide-et-impera, conquer one by one ;)

Thanks,
Mirela

> 
>>> Can you tell which sensor(s) you're working with ?
>>>
>>>>> All controls are in the same class, so they could all be set
>>>>> atomically via VIDIOC_S_EXT_CTRLS, this could turn out to be
>>>>> useful in case of sensors with context switching.
>>>>
>>>> Agreed, we should be able to set them all. Are we still unable to set
>>>> controls from multiple classes atomatically ? I thought that limitation
>>>> has been lifted.
>>>>
>>>>> Each element of the array will hold an u32 value (exposure or gain)
>>>>> for one capture. The size of the array is up to the sensor driver which
>>>>> will implement the controls and initialize them via v4l2_ctrl_new_custom().
>>>>> With this approach, the user-space will have to set valid values
>>>>> for all the captures represented in the array.
>>>>
>>>> I'll comment on the controls themselves in patch 2/2.
>>>>
>>>>> The v4l2-core only supports one scalar min/max/step value for the
>>>>> entire array, and each element is validated and adjusted to be within
>>>>> these bounds in v4l2_ctrl_type_op_validate(). The significance for the
>>>>> maximum value for the exposure control could be "the max value for the
>>>>> long exposure" or "the max value for the sum of all exposures". If none
>>>>> of these is ok, the sensor driver can adjust the values as supported and
>>>>> the user space can use the TRY operation to query the sensor for the
>>>>> minimum or maximum values.
>>>>
>>>> Hmmmm... I wonder if we would need the ability to report different
>>>> limits for different array elements. There may be over-engineering
>>>> though, my experience with libcamera is that userspace really needs
>>>> detailed information about those controls, and attempting to convey the
>>>> precise information through the kernel-userspace API is bound to fail.
>>>> That's why we implement a sensor database in libcamera, with information
>>>> about how to convert control values to real gain and exposure time.
>>>> Exposing (close to) raw register values and letting userspace handle the
>>>> rest may be better.
>>>>
>>>>> Mirela Rabulea (2):
>>>>>     LF-15161-6: media: Add exposure and gain controls for multiple
>>>>>       captures
>>>>>     LF-15161-7: Documentation: media: Describe exposure and gain controls
>>>>>       for multiple captures
>>>>
>>>> Did you forget to remove the LF-* identifiers ? :-)
>>>>
>>>>>
>>>>>    .../media/v4l/ext-ctrls-image-source.rst             | 12 ++++++++++++
>>>>>    drivers/media/v4l2-core/v4l2-ctrls-defs.c            |  8 ++++++++
>>>>>    include/uapi/linux/v4l2-controls.h                   |  3 +++
>>>>>    3 files changed, 23 insertions(+)
> 
> --
> Regards,
> 
> Laurent Pinchart

Hi Laurent,

On 7/16/25 03:12, Laurent Pinchart wrote:
> 
> 
> On Wed, Jul 16, 2025 at 02:59:54AM +0300, Laurent Pinchart wrote:
>> On Fri, Jul 11, 2025 at 01:05:42AM +0300, Mirela Rabulea wrote:
>>> Add new standard controls as U32 arrays, for sensors with multiple
>>> captures: V4L2_CID_EXPOSURE_MULTI, V4L2_CID_AGAIN_MULTI and
>>> V4L2_CID_DGAIN_MULTI. These will be particularly useful for sensors
>>> that have multiple captures, but the HDR merge is done inside the sensor,
>>> in the end exposing a single stream, but still requiring AEC control
>>> for all captures.
>>
>> It's also useful for sensors supporting DOL or DCG with HDR merge being
>> performed outside of the sensor.
> 
> Regarless of where HDR merge is implemented, we will also need controls
> to select the HDR mode. We have V4L2_CID_HDR_SENSOR_MODE, which doesn't
> standardize the values, and that's not good enough. At least for DOL and
> DCG with HDR merge implemented outside of the sensor, we need to
> standardize the modes.
> 
> Can you tell which sensor(s) you're working with ?

We are working mostly with these 3:
Omnivision's os08a20 (2 exposures staggered hdr, each exposure on a 
separate virtual channel, there are also other hdr modes which we do not 
use)
Omnivision ox05b1s (RGB-Ir with context switching based on group holds, 
1 context optimized for RGB, the other context optimized for Ir, each 
context on a different virtual channel)
Omnivision ox03c10 (4 exposures, hdr merge in sensor).

> 
>>> All controls are in the same class, so they could all be set
>>> atomically via VIDIOC_S_EXT_CTRLS, this could turn out to be
>>> useful in case of sensors with context switching.
>>
>> Agreed, we should be able to set them all. Are we still unable to set
>> controls from multiple classes atomatically ? I thought that limitation
>> has been lifted.


Maybe I need some background check on this, but looking at kernel tag 
next-20250718, this comment still lies in the documentation:
"These ioctls allow the caller to get or set multiple controls
atomically. Control IDs are grouped into control classes (see
:ref:`ctrl-class`) and all controls in the control array must belong
to the same control class."

Maybe it needs to be updated, or not...since there is also this check in 
check_ext_ctrls():
	/* Check that all controls are from the same control class. */
	for (i = 0; i < c->count; i++) {
		if (V4L2_CTRL_ID2WHICH(c->controls[i].id) != c->which) {
			c->error_idx = ioctl == VIDIOC_TRY_EXT_CTRLS ? i :
								      c->count;
			return false;
		}
	}

There is also another inconvenient, the VIDIOC_S_EXT_CTRLS does not 
reach the v4l2 subdevice driver, what we get in the sensor driver is a 
set of .s_ctrl calls. I don't know about other sensors, but for the 
Omivision sensors which I am familiar with, the group holds feature 
could be used to get multiple registers to be applied atomically in the 
same frame, but the sensor driver would need to know when to start and 
when to end filling the group hold with the desired registers. If there 
is some similar feature in other sensors, I think the VIDIOC_S_EXT_CTRLS 
should have a corresponding v4l2-subdev operation, so that it can be 
implemented in the sensor subdevice driver. This would probably require 
some changes in the v4l2 core, as currently the subdev_do_ioctl() 
function does not let the VIDIOC_S_EXT_CTRLS go to the subdevice.

Laurent, Hans, any thoughts on this?


>>
>>> Each element of the array will hold an u32 value (exposure or gain)
>>> for one capture. The size of the array is up to the sensor driver which
>>> will implement the controls and initialize them via v4l2_ctrl_new_custom().
>>> With this approach, the user-space will have to set valid values
>>> for all the captures represented in the array.
>>
>> I'll comment on the controls themselves in patch 2/2.
>>
>>> The v4l2-core only supports one scalar min/max/step value for the
>>> entire array, and each element is validated and adjusted to be within
>>> these bounds in v4l2_ctrl_type_op_validate(). The significance for the
>>> maximum value for the exposure control could be "the max value for the
>>> long exposure" or "the max value for the sum of all exposures". If none
>>> of these is ok, the sensor driver can adjust the values as supported and
>>> the user space can use the TRY operation to query the sensor for the
>>> minimum or maximum values.
>>
>> Hmmmm... I wonder if we would need the ability to report different
>> limits for different array elements. There may be over-engineering
>> though, my experience with libcamera is that userspace really needs
>> detailed information about those controls, and attempting to convey the
>> precise information through the kernel-userspace API is bound to fail.
>> That's why we implement a sensor database in libcamera, with information
>> about how to convert control values to real gain and exposure time.
>> Exposing (close to) raw register values and letting userspace handle the
>> rest may be better.

Julien, any thoughts on this?

If we don't need to report different limits for different array 
elements, we are fine, just we need to document better what those limits 
stand for in case of arrays.

>>
>>> Mirela Rabulea (2):
>>>    LF-15161-6: media: Add exposure and gain controls for multiple
>>>      captures
>>>    LF-15161-7: Documentation: media: Describe exposure and gain controls
>>>      for multiple captures
>>
>> Did you forget to remove the LF-* identifiers ? :-)

Yes, at least in the cover-letter, my bad :(

Thanks for feedback.

Regards,
Mirela
>>
>>>
>>>   .../media/v4l/ext-ctrls-image-source.rst             | 12 ++++++++++++
>>>   drivers/media/v4l2-core/v4l2-ctrls-defs.c            |  8 ++++++++
>>>   include/uapi/linux/v4l2-controls.h                   |  3 +++
>>>   3 files changed, 23 insertions(+)
> 
> --
> Regards,
> 
> Laurent Pinchart

Hi Mirela,

On 20/07/2025 20:56, Mirela Rabulea wrote:
> Hi Laurent,
> 
> On 7/16/25 03:12, Laurent Pinchart wrote:
>>
>>
>> On Wed, Jul 16, 2025 at 02:59:54AM +0300, Laurent Pinchart wrote:
>>> On Fri, Jul 11, 2025 at 01:05:42AM +0300, Mirela Rabulea wrote:
>>>> Add new standard controls as U32 arrays, for sensors with multiple
>>>> captures: V4L2_CID_EXPOSURE_MULTI, V4L2_CID_AGAIN_MULTI and
>>>> V4L2_CID_DGAIN_MULTI. These will be particularly useful for sensors
>>>> that have multiple captures, but the HDR merge is done inside the 
>>>> sensor,
>>>> in the end exposing a single stream, but still requiring AEC control
>>>> for all captures.
>>>
>>> It's also useful for sensors supporting DOL or DCG with HDR merge being
>>> performed outside of the sensor.
>>
>> Regarless of where HDR merge is implemented, we will also need controls
>> to select the HDR mode. We have V4L2_CID_HDR_SENSOR_MODE, which doesn't
>> standardize the values, and that's not good enough. At least for DOL and
>> DCG with HDR merge implemented outside of the sensor, we need to
>> standardize the modes.
>>
>> Can you tell which sensor(s) you're working with ?
> 
> We are working mostly with these 3:
> Omnivision's os08a20 (2 exposures staggered hdr, each exposure on a 
> separate virtual channel, there are also other hdr modes which we do not 
> use)
> Omnivision ox05b1s (RGB-Ir with context switching based on group holds, 
> 1 context optimized for RGB, the other context optimized for Ir, each 
> context on a different virtual channel)
> Omnivision ox03c10 (4 exposures, hdr merge in sensor).
> 
>>
>>>> All controls are in the same class, so they could all be set
>>>> atomically via VIDIOC_S_EXT_CTRLS, this could turn out to be
>>>> useful in case of sensors with context switching.
>>>
>>> Agreed, we should be able to set them all. Are we still unable to set
>>> controls from multiple classes atomatically ? I thought that limitation
>>> has been lifted.
> 
> 
> Maybe I need some background check on this, but looking at kernel tag 
> next-20250718, this comment still lies in the documentation:
> "These ioctls allow the caller to get or set multiple controls
> atomically. Control IDs are grouped into control classes (see
> :ref:`ctrl-class`) and all controls in the control array must belong
> to the same control class."
> 
> Maybe it needs to be updated, or not...since there is also this check in 
> check_ext_ctrls():
>      /* Check that all controls are from the same control class. */
>      for (i = 0; i < c->count; i++) {
>          if (V4L2_CTRL_ID2WHICH(c->controls[i].id) != c->which) {
>              c->error_idx = ioctl == VIDIOC_TRY_EXT_CTRLS ? i :
>                                        c->count;
>              return false;
>          }
>      }
> 
> There is also another inconvenient, the VIDIOC_S_EXT_CTRLS does not 
> reach the v4l2 subdevice driver, what we get in the sensor driver is a 
> set of .s_ctrl calls. I don't know about other sensors, but for the 
> Omivision sensors which I am familiar with, the group holds feature 
> could be used to get multiple registers to be applied atomically in the 
> same frame, but the sensor driver would need to know when to start and 
> when to end filling the group hold with the desired registers. If there 
> is some similar feature in other sensors, I think the VIDIOC_S_EXT_CTRLS 
> should have a corresponding v4l2-subdev operation, so that it can be 
> implemented in the sensor subdevice driver. This would probably require 
> some changes in the v4l2 core, as currently the subdev_do_ioctl() 
> function does not let the VIDIOC_S_EXT_CTRLS go to the subdevice.
> 
> Laurent, Hans, any thoughts on this?
> 
> 
>>>
>>>> Each element of the array will hold an u32 value (exposure or gain)
>>>> for one capture. The size of the array is up to the sensor driver which
>>>> will implement the controls and initialize them via 
>>>> v4l2_ctrl_new_custom().
>>>> With this approach, the user-space will have to set valid values
>>>> for all the captures represented in the array.
>>>
>>> I'll comment on the controls themselves in patch 2/2.
>>>
>>>> The v4l2-core only supports one scalar min/max/step value for the
>>>> entire array, and each element is validated and adjusted to be within
>>>> these bounds in v4l2_ctrl_type_op_validate(). The significance for the
>>>> maximum value for the exposure control could be "the max value for the
>>>> long exposure" or "the max value for the sum of all exposures". If none
>>>> of these is ok, the sensor driver can adjust the values as supported 
>>>> and
>>>> the user space can use the TRY operation to query the sensor for the
>>>> minimum or maximum values.
>>>
>>> Hmmmm... I wonder if we would need the ability to report different
>>> limits for different array elements. There may be over-engineering
>>> though, my experience with libcamera is that userspace really needs
>>> detailed information about those controls, and attempting to convey the
>>> precise information through the kernel-userspace API is bound to fail.
>>> That's why we implement a sensor database in libcamera, with information
>>> about how to convert control values to real gain and exposure time.
>>> Exposing (close to) raw register values and letting userspace handle the
>>> rest may be better.
> 
> Julien, any thoughts on this?


Reporting min/max value per array element could have made sense for some 
controls. For instance we have a HDR sensor whose long capture analog 
gain range is different from the shorter captures gain. Conversely, it 
may not work well for the multi-capture exposure control where the 
constraint can be more about the sum of the exposures for each capture 
rather than the individual exposure values. In that case, exposing 
min/max values per array element does not really help the user space.

Thus, having the user space to have the necessary insight into each 
sensor specifics for its AEC control seems to be the versatile option.

> 
> If we don't need to report different limits for different array 
> elements, we are fine, just we need to document better what those limits 
> stand for in case of arrays.
> 
>>>
>>>> Mirela Rabulea (2):
>>>>    LF-15161-6: media: Add exposure and gain controls for multiple
>>>>      captures
>>>>    LF-15161-7: Documentation: media: Describe exposure and gain 
>>>> controls
>>>>      for multiple captures
>>>
>>> Did you forget to remove the LF-* identifiers ? :-)
> 
> Yes, at least in the cover-letter, my bad :(
> 
> Thanks for feedback.
> 
> Regards,
> Mirela
>>>
>>>>
>>>>   .../media/v4l/ext-ctrls-image-source.rst             | 12 
>>>> ++++++++++++
>>>>   drivers/media/v4l2-core/v4l2-ctrls-defs.c            |  8 ++++++++
>>>>   include/uapi/linux/v4l2-controls.h                   |  3 +++
>>>>   3 files changed, 23 insertions(+)
>>
>> -- 
>> Regards,
>>
>> Laurent Pinchart
> 

Thanks,
Julien

On Tue, Jul 22, 2025 at 11:53:53AM +0200, Julien Vuillaumier wrote:
> On 20/07/2025 20:56, Mirela Rabulea wrote:
> > On 7/16/25 03:12, Laurent Pinchart wrote:
> >> On Wed, Jul 16, 2025 at 02:59:54AM +0300, Laurent Pinchart wrote:
> >>> On Fri, Jul 11, 2025 at 01:05:42AM +0300, Mirela Rabulea wrote:
> >>>> Add new standard controls as U32 arrays, for sensors with multiple
> >>>> captures: V4L2_CID_EXPOSURE_MULTI, V4L2_CID_AGAIN_MULTI and
> >>>> V4L2_CID_DGAIN_MULTI. These will be particularly useful for sensors
> >>>> that have multiple captures, but the HDR merge is done inside the 
> >>>> sensor,
> >>>> in the end exposing a single stream, but still requiring AEC control
> >>>> for all captures.
> >>>
> >>> It's also useful for sensors supporting DOL or DCG with HDR merge being
> >>> performed outside of the sensor.
> >>
> >> Regarless of where HDR merge is implemented, we will also need controls
> >> to select the HDR mode. We have V4L2_CID_HDR_SENSOR_MODE, which doesn't
> >> standardize the values, and that's not good enough. At least for DOL and
> >> DCG with HDR merge implemented outside of the sensor, we need to
> >> standardize the modes.
> >>
> >> Can you tell which sensor(s) you're working with ?
> > 
> > We are working mostly with these 3:
> > Omnivision's os08a20 (2 exposures staggered hdr, each exposure on a 
> > separate virtual channel, there are also other hdr modes which we do not 
> > use)
> > Omnivision ox05b1s (RGB-Ir with context switching based on group holds, 
> > 1 context optimized for RGB, the other context optimized for Ir, each 
> > context on a different virtual channel)
> > Omnivision ox03c10 (4 exposures, hdr merge in sensor).
> > 
> >>>> All controls are in the same class, so they could all be set
> >>>> atomically via VIDIOC_S_EXT_CTRLS, this could turn out to be
> >>>> useful in case of sensors with context switching.
> >>>
> >>> Agreed, we should be able to set them all. Are we still unable to set
> >>> controls from multiple classes atomatically ? I thought that limitation
> >>> has been lifted.
> > 
> > Maybe I need some background check on this, but looking at kernel tag 
> > next-20250718, this comment still lies in the documentation:
> > "These ioctls allow the caller to get or set multiple controls
> > atomically. Control IDs are grouped into control classes (see
> > :ref:`ctrl-class`) and all controls in the control array must belong
> > to the same control class."
> > 
> > Maybe it needs to be updated, or not...since there is also this check in 
> > check_ext_ctrls():
> >      /* Check that all controls are from the same control class. */
> >      for (i = 0; i < c->count; i++) {
> >          if (V4L2_CTRL_ID2WHICH(c->controls[i].id) != c->which) {
> >              c->error_idx = ioctl == VIDIOC_TRY_EXT_CTRLS ? i :
> >                                        c->count;
> >              return false;
> >          }
> >      }

This only when c->which is set to a control class. If you set it to
V4L2_CTRL_WHICH_CUR_VAL (equal to 0) then you can set (or get) controls
from multiple classes in one go.

> > There is also another inconvenient, the VIDIOC_S_EXT_CTRLS does not 
> > reach the v4l2 subdevice driver, what we get in the sensor driver is a 
> > set of .s_ctrl calls. I don't know about other sensors, but for the 
> > Omivision sensors which I am familiar with, the group holds feature 
> > could be used to get multiple registers to be applied atomically in the 
> > same frame, but the sensor driver would need to know when to start and 
> > when to end filling the group hold with the desired registers. If there 
> > is some similar feature in other sensors, I think the VIDIOC_S_EXT_CTRLS 
> > should have a corresponding v4l2-subdev operation, so that it can be 
> > implemented in the sensor subdevice driver. This would probably require 
> > some changes in the v4l2 core, as currently the subdev_do_ioctl() 
> > function does not let the VIDIOC_S_EXT_CTRLS go to the subdevice.
> > 
> > Laurent, Hans, any thoughts on this?

I can think of at least 3 ways to handle this.

The first method would be to group all controls in a cluster. That way
you will get a single .s_ctrl() call per VIDIOC_S_EXT_CTRLS. You will
have to iterate over the controls to see which ones have changed, and
configure the sensor accordingly. This short-circuits the logic in the
control framework that dispatches individual controls to separate
.s_ctrl() calls (or rather still goes through that logic, but doesn't
make use of it), and requires reimplementing it manually in the
.s_ctrl() handler. It's not ideal.

The second method would be to add new .begin() and .end() (name to be
bikeshedded) control operations. I experimented with this a while ago to
expose group hold to userspace, but never upstreamed the patches as I
didn't really need them in the end. Alternatively, the VIDIOC_S_EXT_CTRL
could be exposed to drivers, allowing them to implement begin/end
operations before and after calling the control framework. I don't have
a strong preference (maybe Hans would).

I increasingly think that the control framework doesn't provide the best
value for subdevs. It has been developed for video devices, and for
subdevs in video-centric devices where subdevs are hidden behind a video
device, but not for MC-centric use cases where subdevs are exposed to
userspace. The third option would be to implement something better,
dropping the useless features and adding support for the needs of modern
devices, but that would be much more work.

> >>>> Each element of the array will hold an u32 value (exposure or gain)
> >>>> for one capture. The size of the array is up to the sensor driver which
> >>>> will implement the controls and initialize them via 
> >>>> v4l2_ctrl_new_custom().
> >>>> With this approach, the user-space will have to set valid values
> >>>> for all the captures represented in the array.
> >>>
> >>> I'll comment on the controls themselves in patch 2/2.
> >>>
> >>>> The v4l2-core only supports one scalar min/max/step value for the
> >>>> entire array, and each element is validated and adjusted to be within
> >>>> these bounds in v4l2_ctrl_type_op_validate(). The significance for the
> >>>> maximum value for the exposure control could be "the max value for the
> >>>> long exposure" or "the max value for the sum of all exposures". If none
> >>>> of these is ok, the sensor driver can adjust the values as supported and
> >>>> the user space can use the TRY operation to query the sensor for the
> >>>> minimum or maximum values.
> >>>
> >>> Hmmmm... I wonder if we would need the ability to report different
> >>> limits for different array elements. There may be over-engineering
> >>> though, my experience with libcamera is that userspace really needs
> >>> detailed information about those controls, and attempting to convey the
> >>> precise information through the kernel-userspace API is bound to fail.
> >>> That's why we implement a sensor database in libcamera, with information
> >>> about how to convert control values to real gain and exposure time.
> >>> Exposing (close to) raw register values and letting userspace handle the
> >>> rest may be better.
> > 
> > Julien, any thoughts on this?
> 
> Reporting min/max value per array element could have made sense for some 
> controls. For instance we have a HDR sensor whose long capture analog 
> gain range is different from the shorter captures gain. Conversely, it 
> may not work well for the multi-capture exposure control where the 
> constraint can be more about the sum of the exposures for each capture 
> rather than the individual exposure values. In that case, exposing 
> min/max values per array element does not really help the user space.
> 
> Thus, having the user space to have the necessary insight into each 
> sensor specifics for its AEC control seems to be the versatile option.

Then I think we should look at a libcamera implementation alongside with
this patch series, and review them together.

> > If we don't need to report different limits for different array 
> > elements, we are fine, just we need to document better what those limits 
> > stand for in case of arrays.
> > 
> >>>> Mirela Rabulea (2):
> >>>>    LF-15161-6: media: Add exposure and gain controls for multiple
> >>>>      captures
> >>>>    LF-15161-7: Documentation: media: Describe exposure and gain 
> >>>> controls
> >>>>      for multiple captures
> >>>
> >>> Did you forget to remove the LF-* identifiers ? :-)
> > 
> > Yes, at least in the cover-letter, my bad :(
> > 
> > Thanks for feedback.
> > 
> >>>>
> >>>>   .../media/v4l/ext-ctrls-image-source.rst             | 12 ++++++++++++
> >>>>   drivers/media/v4l2-core/v4l2-ctrls-defs.c            |  8 ++++++++
> >>>>   include/uapi/linux/v4l2-controls.h                   |  3 +++
> >>>>   3 files changed, 23 insertions(+)

-- 
Regards,

Laurent Pinchart

On 23/07/2025 17:02, Laurent Pinchart wrote:
> On Tue, Jul 22, 2025 at 11:53:53AM +0200, Julien Vuillaumier wrote:
>> On 20/07/2025 20:56, Mirela Rabulea wrote:
>>> On 7/16/25 03:12, Laurent Pinchart wrote:
>>>> On Wed, Jul 16, 2025 at 02:59:54AM +0300, Laurent Pinchart wrote:
>>>>> On Fri, Jul 11, 2025 at 01:05:42AM +0300, Mirela Rabulea wrote:
>>>>>> Add new standard controls as U32 arrays, for sensors with multiple
>>>>>> captures: V4L2_CID_EXPOSURE_MULTI, V4L2_CID_AGAIN_MULTI and
>>>>>> V4L2_CID_DGAIN_MULTI. These will be particularly useful for sensors
>>>>>> that have multiple captures, but the HDR merge is done inside the 
>>>>>> sensor,
>>>>>> in the end exposing a single stream, but still requiring AEC control
>>>>>> for all captures.
>>>>>
>>>>> It's also useful for sensors supporting DOL or DCG with HDR merge being
>>>>> performed outside of the sensor.
>>>>
>>>> Regarless of where HDR merge is implemented, we will also need controls
>>>> to select the HDR mode. We have V4L2_CID_HDR_SENSOR_MODE, which doesn't
>>>> standardize the values, and that's not good enough. At least for DOL and
>>>> DCG with HDR merge implemented outside of the sensor, we need to
>>>> standardize the modes.
>>>>
>>>> Can you tell which sensor(s) you're working with ?
>>>
>>> We are working mostly with these 3:
>>> Omnivision's os08a20 (2 exposures staggered hdr, each exposure on a 
>>> separate virtual channel, there are also other hdr modes which we do not 
>>> use)
>>> Omnivision ox05b1s (RGB-Ir with context switching based on group holds, 
>>> 1 context optimized for RGB, the other context optimized for Ir, each 
>>> context on a different virtual channel)
>>> Omnivision ox03c10 (4 exposures, hdr merge in sensor).
>>>
>>>>>> All controls are in the same class, so they could all be set
>>>>>> atomically via VIDIOC_S_EXT_CTRLS, this could turn out to be
>>>>>> useful in case of sensors with context switching.
>>>>>
>>>>> Agreed, we should be able to set them all. Are we still unable to set
>>>>> controls from multiple classes atomatically ? I thought that limitation
>>>>> has been lifted.
>>>
>>> Maybe I need some background check on this, but looking at kernel tag 
>>> next-20250718, this comment still lies in the documentation:
>>> "These ioctls allow the caller to get or set multiple controls
>>> atomically. Control IDs are grouped into control classes (see
>>> :ref:`ctrl-class`) and all controls in the control array must belong
>>> to the same control class."
>>>
>>> Maybe it needs to be updated, or not...since there is also this check in 
>>> check_ext_ctrls():
>>>      /* Check that all controls are from the same control class. */
>>>      for (i = 0; i < c->count; i++) {
>>>          if (V4L2_CTRL_ID2WHICH(c->controls[i].id) != c->which) {
>>>              c->error_idx = ioctl == VIDIOC_TRY_EXT_CTRLS ? i :
>>>                                        c->count;
>>>              return false;
>>>          }
>>>      }
> 
> This only when c->which is set to a control class. If you set it to
> V4L2_CTRL_WHICH_CUR_VAL (equal to 0) then you can set (or get) controls
> from multiple classes in one go.

That's correct. Early implementations of the control framework required
that all controls were from the same control class, but later I dropped
that requirement and you can just set 'which' to 0 and it no longer matters.

> 
>>> There is also another inconvenient, the VIDIOC_S_EXT_CTRLS does not 
>>> reach the v4l2 subdevice driver, what we get in the sensor driver is a 
>>> set of .s_ctrl calls. I don't know about other sensors, but for the 
>>> Omivision sensors which I am familiar with, the group holds feature 
>>> could be used to get multiple registers to be applied atomically in the 
>>> same frame, but the sensor driver would need to know when to start and 
>>> when to end filling the group hold with the desired registers. If there 
>>> is some similar feature in other sensors, I think the VIDIOC_S_EXT_CTRLS 
>>> should have a corresponding v4l2-subdev operation, so that it can be 
>>> implemented in the sensor subdevice driver. This would probably require 
>>> some changes in the v4l2 core, as currently the subdev_do_ioctl() 
>>> function does not let the VIDIOC_S_EXT_CTRLS go to the subdevice.
>>>
>>> Laurent, Hans, any thoughts on this?
> 
> I can think of at least 3 ways to handle this.
> 
> The first method would be to group all controls in a cluster. That way
> you will get a single .s_ctrl() call per VIDIOC_S_EXT_CTRLS. You will
> have to iterate over the controls to see which ones have changed, and
> configure the sensor accordingly. This short-circuits the logic in the
> control framework that dispatches individual controls to separate
> .s_ctrl() calls (or rather still goes through that logic, but doesn't
> make use of it), and requires reimplementing it manually in the
> .s_ctrl() handler. It's not ideal.

This should work out-of-the-box.

> 
> The second method would be to add new .begin() and .end() (name to be
> bikeshedded) control operations. I experimented with this a while ago to
> expose group hold to userspace, but never upstreamed the patches as I
> didn't really need them in the end. Alternatively, the VIDIOC_S_EXT_CTRL
> could be exposed to drivers, allowing them to implement begin/end
> operations before and after calling the control framework. I don't have
> a strong preference (maybe Hans would).

I wonder if you could make 'HOLD_BEGIN' and 'HOLD_END' button controls, and
start and end the control array in VIDIOC_S_EXT_CTRLS with it. There are
some issues that need to be figured out, but I think this is doable.

Regards,

	Hans

> 
> I increasingly think that the control framework doesn't provide the best
> value for subdevs. It has been developed for video devices, and for
> subdevs in video-centric devices where subdevs are hidden behind a video
> device, but not for MC-centric use cases where subdevs are exposed to
> userspace. The third option would be to implement something better,
> dropping the useless features and adding support for the needs of modern
> devices, but that would be much more work.
> 
>>>>>> Each element of the array will hold an u32 value (exposure or gain)
>>>>>> for one capture. The size of the array is up to the sensor driver which
>>>>>> will implement the controls and initialize them via 
>>>>>> v4l2_ctrl_new_custom().
>>>>>> With this approach, the user-space will have to set valid values
>>>>>> for all the captures represented in the array.
>>>>>
>>>>> I'll comment on the controls themselves in patch 2/2.
>>>>>
>>>>>> The v4l2-core only supports one scalar min/max/step value for the
>>>>>> entire array, and each element is validated and adjusted to be within
>>>>>> these bounds in v4l2_ctrl_type_op_validate(). The significance for the
>>>>>> maximum value for the exposure control could be "the max value for the
>>>>>> long exposure" or "the max value for the sum of all exposures". If none
>>>>>> of these is ok, the sensor driver can adjust the values as supported and
>>>>>> the user space can use the TRY operation to query the sensor for the
>>>>>> minimum or maximum values.
>>>>>
>>>>> Hmmmm... I wonder if we would need the ability to report different
>>>>> limits for different array elements. There may be over-engineering
>>>>> though, my experience with libcamera is that userspace really needs
>>>>> detailed information about those controls, and attempting to convey the
>>>>> precise information through the kernel-userspace API is bound to fail.
>>>>> That's why we implement a sensor database in libcamera, with information
>>>>> about how to convert control values to real gain and exposure time.
>>>>> Exposing (close to) raw register values and letting userspace handle the
>>>>> rest may be better.
>>>
>>> Julien, any thoughts on this?
>>
>> Reporting min/max value per array element could have made sense for some 
>> controls. For instance we have a HDR sensor whose long capture analog 

Actually, support for this exists. See the VIDIOC_G_EXT_CTRLS documentation
and look for V4L2_CTRL_WHICH_DEF_VAL/V4L2_CTRL_WHICH_MIN_VAL/V4L2_CTRL_WHICH_MAX_VAL.

>> gain range is different from the shorter captures gain. Conversely, it 
>> may not work well for the multi-capture exposure control where the 
>> constraint can be more about the sum of the exposures for each capture 
>> rather than the individual exposure values. In that case, exposing 
>> min/max values per array element does not really help the user space.
>>
>> Thus, having the user space to have the necessary insight into each 
>> sensor specifics for its AEC control seems to be the versatile option.
> 
> Then I think we should look at a libcamera implementation alongside with
> this patch series, and review them together.
> 
>>> If we don't need to report different limits for different array 
>>> elements, we are fine, just we need to document better what those limits 
>>> stand for in case of arrays.
>>>
>>>>>> Mirela Rabulea (2):
>>>>>>    LF-15161-6: media: Add exposure and gain controls for multiple
>>>>>>      captures
>>>>>>    LF-15161-7: Documentation: media: Describe exposure and gain 
>>>>>> controls
>>>>>>      for multiple captures
>>>>>
>>>>> Did you forget to remove the LF-* identifiers ? :-)
>>>
>>> Yes, at least in the cover-letter, my bad :(
>>>
>>> Thanks for feedback.
>>>
>>>>>>
>>>>>>   .../media/v4l/ext-ctrls-image-source.rst             | 12 ++++++++++++
>>>>>>   drivers/media/v4l2-core/v4l2-ctrls-defs.c            |  8 ++++++++
>>>>>>   include/uapi/linux/v4l2-controls.h                   |  3 +++
>>>>>>   3 files changed, 23 insertions(+)
> 

Hi Hans,

On Fri, Jul 25, 2025 at 11:01:11AM +0200, Hans Verkuil wrote:
> On 23/07/2025 17:02, Laurent Pinchart wrote:
> > On Tue, Jul 22, 2025 at 11:53:53AM +0200, Julien Vuillaumier wrote:
> >> On 20/07/2025 20:56, Mirela Rabulea wrote:
> >>> On 7/16/25 03:12, Laurent Pinchart wrote:
> >>>> On Wed, Jul 16, 2025 at 02:59:54AM +0300, Laurent Pinchart wrote:
> >>>>> On Fri, Jul 11, 2025 at 01:05:42AM +0300, Mirela Rabulea wrote:
> >>>>>> Add new standard controls as U32 arrays, for sensors with multiple
> >>>>>> captures: V4L2_CID_EXPOSURE_MULTI, V4L2_CID_AGAIN_MULTI and
> >>>>>> V4L2_CID_DGAIN_MULTI. These will be particularly useful for sensors
> >>>>>> that have multiple captures, but the HDR merge is done inside the 
> >>>>>> sensor,
> >>>>>> in the end exposing a single stream, but still requiring AEC control
> >>>>>> for all captures.
> >>>>>
> >>>>> It's also useful for sensors supporting DOL or DCG with HDR merge being
> >>>>> performed outside of the sensor.
> >>>>
> >>>> Regarless of where HDR merge is implemented, we will also need controls
> >>>> to select the HDR mode. We have V4L2_CID_HDR_SENSOR_MODE, which doesn't
> >>>> standardize the values, and that's not good enough. At least for DOL and
> >>>> DCG with HDR merge implemented outside of the sensor, we need to
> >>>> standardize the modes.
> >>>>
> >>>> Can you tell which sensor(s) you're working with ?
> >>>
> >>> We are working mostly with these 3:
> >>> Omnivision's os08a20 (2 exposures staggered hdr, each exposure on a 
> >>> separate virtual channel, there are also other hdr modes which we do not 
> >>> use)
> >>> Omnivision ox05b1s (RGB-Ir with context switching based on group holds, 
> >>> 1 context optimized for RGB, the other context optimized for Ir, each 
> >>> context on a different virtual channel)
> >>> Omnivision ox03c10 (4 exposures, hdr merge in sensor).
> >>>
> >>>>>> All controls are in the same class, so they could all be set
> >>>>>> atomically via VIDIOC_S_EXT_CTRLS, this could turn out to be
> >>>>>> useful in case of sensors with context switching.
> >>>>>
> >>>>> Agreed, we should be able to set them all. Are we still unable to set
> >>>>> controls from multiple classes atomatically ? I thought that limitation
> >>>>> has been lifted.
> >>>
> >>> Maybe I need some background check on this, but looking at kernel tag 
> >>> next-20250718, this comment still lies in the documentation:
> >>> "These ioctls allow the caller to get or set multiple controls
> >>> atomically. Control IDs are grouped into control classes (see
> >>> :ref:`ctrl-class`) and all controls in the control array must belong
> >>> to the same control class."
> >>>
> >>> Maybe it needs to be updated, or not...since there is also this check in 
> >>> check_ext_ctrls():
> >>>      /* Check that all controls are from the same control class. */
> >>>      for (i = 0; i < c->count; i++) {
> >>>          if (V4L2_CTRL_ID2WHICH(c->controls[i].id) != c->which) {
> >>>              c->error_idx = ioctl == VIDIOC_TRY_EXT_CTRLS ? i :
> >>>                                        c->count;
> >>>              return false;
> >>>          }
> >>>      }
> > 
> > This only when c->which is set to a control class. If you set it to
> > V4L2_CTRL_WHICH_CUR_VAL (equal to 0) then you can set (or get) controls
> > from multiple classes in one go.
> 
> That's correct. Early implementations of the control framework required
> that all controls were from the same control class, but later I dropped
> that requirement and you can just set 'which' to 0 and it no longer matters.
> 
> >>> There is also another inconvenient, the VIDIOC_S_EXT_CTRLS does not 
> >>> reach the v4l2 subdevice driver, what we get in the sensor driver is a 
> >>> set of .s_ctrl calls. I don't know about other sensors, but for the 
> >>> Omivision sensors which I am familiar with, the group holds feature 
> >>> could be used to get multiple registers to be applied atomically in the 
> >>> same frame, but the sensor driver would need to know when to start and 
> >>> when to end filling the group hold with the desired registers. If there 
> >>> is some similar feature in other sensors, I think the VIDIOC_S_EXT_CTRLS 
> >>> should have a corresponding v4l2-subdev operation, so that it can be 
> >>> implemented in the sensor subdevice driver. This would probably require 
> >>> some changes in the v4l2 core, as currently the subdev_do_ioctl() 
> >>> function does not let the VIDIOC_S_EXT_CTRLS go to the subdevice.
> >>>
> >>> Laurent, Hans, any thoughts on this?
> > 
> > I can think of at least 3 ways to handle this.
> > 
> > The first method would be to group all controls in a cluster. That way
> > you will get a single .s_ctrl() call per VIDIOC_S_EXT_CTRLS. You will
> > have to iterate over the controls to see which ones have changed, and
> > configure the sensor accordingly. This short-circuits the logic in the
> > control framework that dispatches individual controls to separate
> > .s_ctrl() calls (or rather still goes through that logic, but doesn't
> > make use of it), and requires reimplementing it manually in the
> > .s_ctrl() handler. It's not ideal.
> 
> This should work out-of-the-box.
> 
> > The second method would be to add new .begin() and .end() (name to be
> > bikeshedded) control operations. I experimented with this a while ago to
> > expose group hold to userspace, but never upstreamed the patches as I
> > didn't really need them in the end. Alternatively, the VIDIOC_S_EXT_CTRL
> > could be exposed to drivers, allowing them to implement begin/end
> > operations before and after calling the control framework. I don't have
> > a strong preference (maybe Hans would).
> 
> I wonder if you could make 'HOLD_BEGIN' and 'HOLD_END' button controls, and
> start and end the control array in VIDIOC_S_EXT_CTRLS with it. There are
> some issues that need to be figured out, but I think this is doable.

That seems needlessly complicated for userspace. Ultimately, what we
want in userspace is to set several controls in an atomic way. What
mechanism the sensor and driver use for this should be internal. As we
already have a way to set multiple controls in the V4L2 API, I don't see
a reason to introduce more userspace complexity.

> > I increasingly think that the control framework doesn't provide the best
> > value for subdevs. It has been developed for video devices, and for
> > subdevs in video-centric devices where subdevs are hidden behind a video
> > device, but not for MC-centric use cases where subdevs are exposed to
> > userspace. The third option would be to implement something better,
> > dropping the useless features and adding support for the needs of modern
> > devices, but that would be much more work.
> > 
> >>>>>> Each element of the array will hold an u32 value (exposure or gain)
> >>>>>> for one capture. The size of the array is up to the sensor driver which
> >>>>>> will implement the controls and initialize them via 
> >>>>>> v4l2_ctrl_new_custom().
> >>>>>> With this approach, the user-space will have to set valid values
> >>>>>> for all the captures represented in the array.
> >>>>>
> >>>>> I'll comment on the controls themselves in patch 2/2.
> >>>>>
> >>>>>> The v4l2-core only supports one scalar min/max/step value for the
> >>>>>> entire array, and each element is validated and adjusted to be within
> >>>>>> these bounds in v4l2_ctrl_type_op_validate(). The significance for the
> >>>>>> maximum value for the exposure control could be "the max value for the
> >>>>>> long exposure" or "the max value for the sum of all exposures". If none
> >>>>>> of these is ok, the sensor driver can adjust the values as supported and
> >>>>>> the user space can use the TRY operation to query the sensor for the
> >>>>>> minimum or maximum values.
> >>>>>
> >>>>> Hmmmm... I wonder if we would need the ability to report different
> >>>>> limits for different array elements. There may be over-engineering
> >>>>> though, my experience with libcamera is that userspace really needs
> >>>>> detailed information about those controls, and attempting to convey the
> >>>>> precise information through the kernel-userspace API is bound to fail.
> >>>>> That's why we implement a sensor database in libcamera, with information
> >>>>> about how to convert control values to real gain and exposure time.
> >>>>> Exposing (close to) raw register values and letting userspace handle the
> >>>>> rest may be better.
> >>>
> >>> Julien, any thoughts on this?
> >>
> >> Reporting min/max value per array element could have made sense for some 
> >> controls. For instance we have a HDR sensor whose long capture analog 
> 
> Actually, support for this exists. See the VIDIOC_G_EXT_CTRLS documentation
> and look for V4L2_CTRL_WHICH_DEF_VAL/V4L2_CTRL_WHICH_MIN_VAL/V4L2_CTRL_WHICH_MAX_VAL.

Ah thanks.

> >> gain range is different from the shorter captures gain. Conversely, it 
> >> may not work well for the multi-capture exposure control where the 
> >> constraint can be more about the sum of the exposures for each capture 
> >> rather than the individual exposure values. In that case, exposing 
> >> min/max values per array element does not really help the user space.
> >>
> >> Thus, having the user space to have the necessary insight into each 
> >> sensor specifics for its AEC control seems to be the versatile option.
> > 
> > Then I think we should look at a libcamera implementation alongside with
> > this patch series, and review them together.
> > 
> >>> If we don't need to report different limits for different array 
> >>> elements, we are fine, just we need to document better what those limits 
> >>> stand for in case of arrays.
> >>>
> >>>>>> Mirela Rabulea (2):
> >>>>>>    LF-15161-6: media: Add exposure and gain controls for multiple
> >>>>>>      captures
> >>>>>>    LF-15161-7: Documentation: media: Describe exposure and gain 
> >>>>>> controls
> >>>>>>      for multiple captures
> >>>>>
> >>>>> Did you forget to remove the LF-* identifiers ? :-)
> >>>
> >>> Yes, at least in the cover-letter, my bad :(
> >>>
> >>> Thanks for feedback.
> >>>
> >>>>>>   .../media/v4l/ext-ctrls-image-source.rst             | 12 ++++++++++++
> >>>>>>   drivers/media/v4l2-core/v4l2-ctrls-defs.c            |  8 ++++++++
> >>>>>>   include/uapi/linux/v4l2-controls.h                   |  3 +++
> >>>>>>   3 files changed, 23 insertions(+)

-- 
Regards,

Laurent Pinchart