[PATCH v8 09/17] media: uapi: HEVC: Define V4L2_CID_STATELESS_HEVC_SLICE_PARAMS as a dynamic array

[John Cox]

Hi

>Dne torek, 14. junij 2022 ob 15:50:23 CEST je Hans Verkuil napisal(a):
>> On 6/14/22 10:36, Benjamin Gaignard wrote:
>> > Make explicit that V4L2_CID_STATELESS_HEVC_SLICE_PARAMS control is
>> > a dynamic array control type.
>> > Some drivers may be able to receive multiple slices in one control
>> > to improve decoding performance.
>> > 
>> > Define the max size of the dynamic that can driver can set in .dims = {}.
>> > 
>> > Signed-off-by: Benjamin Gaignard <benjamin.gaignard at collabora.com>
>> > ---
>> > version 7:
>> > - Add Jernej patch to set array dims for Cedrus
>> > 
>> > version 6:
>> > - Set V4L2_CTRL_FLAG_DYNAMIC_ARRAY flag automatically when using
>> >   V4L2_CID_STATELESS_HEVC_SLICE_PARAMS control.
>> > - Add a define for max slices count
>> > 
>> >  Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst | 2 ++
>> >  drivers/media/v4l2-core/v4l2-ctrls-defs.c                 | 1 +
>> >  drivers/staging/media/sunxi/cedrus/cedrus.c               | 1 +
>> >  include/media/hevc-ctrls.h                                | 5 +++++
>> >  4 files changed, 9 insertions(+)
>> > 
>> > diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst b/
>Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
>> > index 06b967de140c..0796b1563daa 100644
>> > --- a/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
>> > +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-codec.rst
>> > @@ -2986,6 +2986,8 @@ enum v4l2_mpeg_video_hevc_size_of_length_field -
>> >      These bitstream parameters are defined according to :ref:`hevc`.
>> >      They are described in section 7.4.7 "General slice segment header
>> >      semantics" of the specification.
>> > +    This control is a dynamically sized 1-dimensional array,
>> > +    V4L2_CTRL_FLAG_DYNAMIC_ARRAY flag must be set when using it.
>> >  
>> >  .. c:type:: v4l2_ctrl_hevc_slice_params
>> >  
>> > diff --git a/drivers/media/v4l2-core/v4l2-ctrls-defs.c b/drivers/media/
>v4l2-core/v4l2-ctrls-defs.c
>> > index 9f55503cd3d6..d594efbcbb93 100644
>> > --- a/drivers/media/v4l2-core/v4l2-ctrls-defs.c
>> > +++ b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
>> > @@ -1510,6 +1510,7 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum 
>v4l2_ctrl_type *type,
>> >  		break;
>> >  	case V4L2_CID_STATELESS_HEVC_SLICE_PARAMS:
>> >  		*type = V4L2_CTRL_TYPE_HEVC_SLICE_PARAMS;
>> > +		*flags |= V4L2_CTRL_FLAG_DYNAMIC_ARRAY;
>> >  		break;
>> >  	case V4L2_CID_STATELESS_HEVC_SCALING_MATRIX:
>> >  		*type = V4L2_CTRL_TYPE_HEVC_SCALING_MATRIX;
>> > diff --git a/drivers/staging/media/sunxi/cedrus/cedrus.c b/drivers/staging/
>media/sunxi/cedrus/cedrus.c
>> > index 87be975a72b6..f3391c7c811c 100644
>> > --- a/drivers/staging/media/sunxi/cedrus/cedrus.c
>> > +++ b/drivers/staging/media/sunxi/cedrus/cedrus.c
>> > @@ -178,6 +178,7 @@ static const struct cedrus_control cedrus_controls[] = 
>{
>> >  	{
>> >  		.cfg = {
>> >  			.id	= 
>V4L2_CID_STATELESS_HEVC_SLICE_PARAMS,
>> > +			.dims   = { 1 },
>> 
>> So cedrus HW supports only a single SLICE_PARAMS struct? Perhaps add that
>> as a comment.
>
>Yes, for now. I have WIP patch which will remove this limitation.
>
>Please corrrect me if I'm wrong, but this dimension represents maximum allowed 
>array length? If so, this is a bit awkward for cases where there is really no 
>limit. Like in this case. Cedrus can process only one slice which means that 
>driver will have to loop over all submitted slices, one by one. Thus, there is 
>no real limitation. It could be set to 10, 1000 or even more. Any suggestion 
>for appropriate upper limit is appreciated.

Likewise for Pi there is no real upper limit.

Regards

John Cox

>Best regards,
>Jernej
>
>> 
>> >  		},
>> >  		.codec		= CEDRUS_CODEC_H265,
>> >  	},
>> > diff --git a/include/media/hevc-ctrls.h b/include/media/hevc-ctrls.h
>> > index 0dbd5d681c28..140151609c96 100644
>> > --- a/include/media/hevc-ctrls.h
>> > +++ b/include/media/hevc-ctrls.h
>> > @@ -311,9 +311,14 @@ struct v4l2_hevc_pred_weight_table {
>> >  #define 
>V4L2_HEVC_SLICE_PARAMS_FLAG_SLICE_LOOP_FILTER_ACROSS_SLICES_ENABLED (1ULL << 
>8)
>> >  #define V4L2_HEVC_SLICE_PARAMS_FLAG_DEPENDENT_SLICE_SEGMENT	(1ULL << 
>9)
>> >  
>> > +#define V4L2_HEVC_SLICE_MAX_COUNT	600
>> > +
>> 
>> I would drop this define. It is not used at the moment, and I think it
>> is the driver that should set this through the dims field (as cedrus
>> does above). It is likely to be limited by hardware anyway (just my guess).
>> 
>> If it is needed in the future, it can always be added.
>> 
>> Regards,
>> 
>> 	Hans
>> 
>> >  /**
>> >   * v4l2_ctrl_hevc_slice_params - HEVC slice parameters
>> >   *
>> > + * This control is a dynamically sized 1-dimensional array,
>> > + * V4L2_CTRL_FLAG_DYNAMIC_ARRAY flag must be set when using it.
>> > + *
>> >   * @bit_size: size (in bits) of the current slice data
>> >   * @data_bit_offset: offset (in bits) to the video data in the current 
>slice data
>> >   * @nal_unit_type: specifies the coding type of the slice (B, P or I)
>> 
>> 
>