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
>
