Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   John Cox <jc@kynesim.co.uk>
To:     =?utf-8?Q?Jernej_=C5=A0krabec?= <jernej.skrabec@gmail.com>
Cc:     Benjamin Gaignard <benjamin.gaignard@collabora.com>,
        mchehab@kernel.org, ezequiel@vanguardiasur.com.ar,
        p.zabel@pengutronix.de, gregkh@linuxfoundation.org,
        mripard@kernel.org, paul.kocialkowski@bootlin.com, wens@csie.org,
        samuel@sholland.org, nicolas.dufresne@collabora.com,
        andrzej.p@collabora.com, Hans Verkuil <hverkuil@xs4all.nl>,
        linux-media@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-rockchip@lists.infradead.org, linux-staging@lists.linux.dev,
        linux-arm-kernel@lists.infradead.org, linux-sunxi@lists.linux.dev,
        kernel@collabora.com
Subject: Re: [PATCH v8 09/17] media: uapi: HEVC: Define V4L2_CID_STATELESS_HEVC_SLICE_PARAMS as a dynamic array
Date:   Tue, 14 Jun 2022 18:50:36 +0100
Message-ID: <cgihahl6gu9089b1ph9tj4vobftaoatbjt@4ax.com>
References: <20220614083614.240641-1-benjamin.gaignard@collabora.com> <20220614083614.240641-10-benjamin.gaignard@collabora.com> <e16c48e0-39ab-80db-0d14-2b3f97079823@xs4all.nl> <2830572.e9J7NaK4W3@kista>
In-Reply-To: <2830572.e9J7NaK4W3@kista>
User-Agent: ForteAgent/8.00.32.1272
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

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.
>> >=20
>> > Define the max size of the dynamic that can driver can set in .dims =
=3D {}.
>> >=20
>> > Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
>> > ---
>> > version 7:
>> > - Add Jernej patch to set array dims for Cedrus
>> >=20
>> > 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
>> >=20
>> >  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(+)
>> >=20
>> > 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.
>> > =20
>> >  .. c:type:: v4l2_ctrl_hevc_slice_params
>> > =20
>> > 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=20
>v4l2_ctrl_type *type,
>> >  		break;
>> >  	case V4L2_CID_STATELESS_HEVC_SLICE_PARAMS:
>> >  		*type =3D V4L2_CTRL_TYPE_HEVC_SLICE_PARAMS;
>> > +		*flags |=3D V4L2_CTRL_FLAG_DYNAMIC_ARRAY;
>> >  		break;
>> >  	case V4L2_CID_STATELESS_HEVC_SCALING_MATRIX:
>> >  		*type =3D 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[] =3D=20
>{
>> >  	{
>> >  		.cfg =3D {
>> >  			.id	=3D=20
>V4L2_CID_STATELESS_HEVC_SLICE_PARAMS,
>> > +			.dims   =3D { 1 },
>>=20
>> 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=20
>array length? If so, this is a bit awkward for cases where there is =
really no=20
>limit. Like in this case. Cedrus can process only one slice which means =
that=20
>driver will have to loop over all submitted slices, one by one. Thus, =
there is=20
>no real limitation. It could be set to 10, 1000 or even more. Any =
suggestion=20
>for appropriate upper limit is appreciated.

Likewise for Pi there is no real upper limit.

Regards

John Cox

>Best regards,
>Jernej
>
>>=20
>> >  		},
>> >  		.codec		=3D 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=20
>V4L2_HEVC_SLICE_PARAMS_FLAG_SLICE_LOOP_FILTER_ACROSS_SLICES_ENABLED =
(1ULL <<=20
>8)
>> >  #define V4L2_HEVC_SLICE_PARAMS_FLAG_DEPENDENT_SLICE_SEGMENT	(1ULL =
<<=20
>9)
>> > =20
>> > +#define V4L2_HEVC_SLICE_MAX_COUNT	600
>> > +
>>=20
>> 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).
>>=20
>> If it is needed in the future, it can always be added.
>>=20
>> Regards,
>>=20
>> 	Hans
>>=20
>> >  /**
>> >   * 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=20
>slice data
>> >   * @nal_unit_type: specifies the coding type of the slice (B, P or =
I)
>>=20
>>=20
>