Message-ID: <dddcad1a-1f0a-4ecc-8093-8a75ec24d2ec@nxp.com>
Date: Tue, 22 Jul 2025 11:53:53 +0200
From: Julien Vuillaumier <julien.vuillaumier@....com>
To: Mirela Rabulea <mirela.rabulea@....com>,
 Laurent Pinchart <laurent.pinchart@...asonboard.com>
Cc: mchehab@...nel.org, sakari.ailus@...ux.intel.com,
 hverkuil-cisco@...all.nl, ribalda@...omium.org, jai.luthra@...asonboard.com,
 laurentiu.palcu@....com, linux-media@...r.kernel.org,
 linux-kernel@...r.kernel.org, LnxRevLi@....com, celine.laurencin@....com
Subject: Re: [RFC 0/2] Add standard exposure and gain controls for multiple
 captures

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

Powered by blists - more mailing lists