Re: [PATCH v5 1/9] videodev2.h: add V4L2_CTRL_FLAG_DYNAMIC_ARRAY

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 6/15/22 10:39, Laurent Pinchart wrote:
> Hi Hans,
> 
> On Wed, Jun 15, 2022 at 09:37:17AM +0200, Hans Verkuil wrote:
>> On 6/14/22 22:46, Laurent Pinchart wrote:
>>> On Tue, May 03, 2022 at 11:39:17AM +0200, Xavier Roumegue wrote:
>>>> From: Hans Verkuil <hverkuil-cisco@xxxxxxxxx>
>>>>
>>>> Add a new flag that indicates that this control is a dynamically sized
>>>> array. Also document this flag.
>>>>
>>>> Currently dynamically sized arrays are limited to one dimensional arrays,
>>>> but that might change in the future if there is a need for it.
>>>>
>>>> The initial use-case of dynamic arrays are stateless codecs. A frame
>>>> can be divided in many slices, so you want to provide an array containing
>>>> slice information for each slice. Typically the number of slices is small,
>>>> but the standard allow for hundreds or thousands of slices. Dynamic arrays
>>>> are a good solution since sizing the array for the worst case would waste
>>>> substantial amounts of memory.
>>>>
>>>> Signed-off-by: Hans Verkuil <hverkuil-cisco@xxxxxxxxx>
>>>> ---
>>>>  .../userspace-api/media/v4l/vidioc-queryctrl.rst          | 8 ++++++++
>>>>  include/uapi/linux/videodev2.h                            | 1 +
>>>>  2 files changed, 9 insertions(+)
>>>>
>>>> diff --git a/Documentation/userspace-api/media/v4l/vidioc-queryctrl.rst b/Documentation/userspace-api/media/v4l/vidioc-queryctrl.rst
>>>> index 88f630252d98..a20dfa2a933b 100644
>>>> --- a/Documentation/userspace-api/media/v4l/vidioc-queryctrl.rst
>>>> +++ b/Documentation/userspace-api/media/v4l/vidioc-queryctrl.rst
>>>> @@ -625,6 +625,14 @@ See also the examples in :ref:`control`.
>>>>  	``V4L2_CTRL_FLAG_GRABBED`` flag when buffers are allocated or
>>>>  	streaming is in progress since most drivers do not support changing
>>>>  	the format in that case.
>>>> +    * - ``V4L2_CTRL_FLAG_DYNAMIC_ARRAY``
>>>> +      - 0x0800
>>>> +      - This control is a dynamically sized 1-dimensional array. It
>>>> +        behaves the same as a regular array, except that the number
>>>> +	of elements as reported by the ``elems`` field is between 1 and
>>>> +	``dims[0]``. So setting the control with a differently sized
>>>> +	array will change the ``elems`` field when the control is
>>>> +	queried afterwards.
>>>
>>> Wrong indentation.
>>
>> No, it's just that the tab in front makes it look a bit weird.
> 
> I thought .rst files had to use spaces for indentation, but it seems I
> was wrong.
> 
>>> Can the dimension be changed by the application only, or by the driver
>>> too ? In the latter case, is an event generated ?
>>
>> Interesting question. 'dims[0]' is the maximum number of elements allowed
>> by the driver for this dynamic array. Applications can set the control to
>> any number of elements up to that maximum. For the current use case I expect
>> that this maximum reflects what the hardware supports, and is not really
>> related to the video resolution. Currently the code expects the maximum
>> number of elements (dims[0]) to remain constant.
> 
> The maximum should be fixed, I agree. The video resolution however
> matters, as it dictates (in the DW100 case) the number of actual

Ah, this patch is part of the dw100 series, I thought it was the same patch
that was part of the HEVC series. Sorry, I missed that.

I'll have to review this again in that context. I'll reply later, once I've
done that.

Regards,

	Hans

> elements in the control array. When changing the format, the control
> value thus needs to be updated accordingly. There are multiple
> strategies there. One of them would be for the driver to automatically
> update the control value, most probably to a default (as there's no way
> the driver could derive a new meaningful value from the old one).
> Another one, which is implemented in this series, is to keep the control
> as-is, but ignore its value at stream on time if it hasn't been updated
> by userspace. There could be other options.
> 
> Do we want to mandate a particular strategy here, or leave it to
> individual controls ?
> 
>> Technically a driver can increase dims[0] as needed. Should that be needed,
>> then we probably need an event to signal that.
>>
>>> Considering this in the context of this series, the driver needs to
>>> change the dimension, as the use case is to size the control based on
>>> the image size. Do we want to document here that the driver will reset
>>> the control to a default value when the dimension changes, or is that
>>> something that should be control-specific ?
>>
>> As mentioned above, in the current context the maximum number of allowed
>> elements in a dynamic array is fixed, so this issue does not occur.
>>
>>>>  
>>>>  Return Value
>>>>  ============
>>>> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
>>>> index 3768a0a80830..8df13defde75 100644
>>>> --- a/include/uapi/linux/videodev2.h
>>>> +++ b/include/uapi/linux/videodev2.h
>>>> @@ -1886,6 +1886,7 @@ struct v4l2_querymenu {
>>>>  #define V4L2_CTRL_FLAG_HAS_PAYLOAD	0x0100
>>>>  #define V4L2_CTRL_FLAG_EXECUTE_ON_WRITE	0x0200
>>>>  #define V4L2_CTRL_FLAG_MODIFY_LAYOUT	0x0400
>>>> +#define V4L2_CTRL_FLAG_DYNAMIC_ARRAY	0x0800
>>>>  
>>>>  /*  Query flags, to be ORed with the control ID */
>>>>  #define V4L2_CTRL_FLAG_NEXT_CTRL	0x80000000
> 



[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux