Message-ID: <38e022d0-cc8f-4df2-8a81-69513c854035@nxp.com>
Date: Sun, 20 Jul 2025 21:56:04 +0300
From: Mirela Rabulea <mirela.rabulea@....com>
To: 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, julien.vuillaumier@....com,
 celine.laurencin@....com
Subject: Re: Re: [RFC 0/2] Add standard exposure and gain controls for
 multiple captures

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

Powered by blists - more mailing lists