Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   John Cox <jc@kynesim.co.uk>
To:     Hans Verkuil <hverkuil-cisco@xs4all.nl>
Cc:     Benjamin Gaignard <benjamin.gaignard@collabora.com>,
        ezequiel@collabora.com, p.zabel@pengutronix.de, mchehab@kernel.org,
        robh+dt@kernel.org, shawnguo@kernel.org, s.hauer@pengutronix.de,
        festevam@gmail.com, lee.jones@linaro.org,
        gregkh@linuxfoundation.org, mripard@kernel.org,
        paul.kocialkowski@bootlin.com, wens@csie.org,
        jernej.skrabec@siol.net, emil.l.velikov@gmail.com,
        kernel@pengutronix.de, linux-imx@nxp.com,
        linux-media@vger.kernel.org, linux-rockchip@lists.infradead.org,
        devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
        linux-kernel@vger.kernel.org, devel@driverdev.osuosl.org,
        kernel@collabora.com, cphealy@gmail.com
Subject: Re: [PATCH v10 6/9] media: uapi: Add a control for HANTRO driver
Date:   Thu, 06 May 2021 14:11:28 +0100
Message-ID: <n7q79gl86gvdo00sgsg1r5beittohsu4ta@4ax.com>
References: <20210420121046.181889-1-benjamin.gaignard@collabora.com> <20210420121046.181889-7-benjamin.gaignard@collabora.com> <a7c9fe23-2900-ac90-7131-21380fbfc793@xs4all.nl> <1cf94540-7f4d-0179-dd1e-0b82ee30f6d2@collabora.com> <815a4bd6-599b-cfb8-9ddc-efa4b7092c23@xs4all.nl>
In-Reply-To: <815a4bd6-599b-cfb8-9ddc-efa4b7092c23@xs4all.nl>
User-Agent: ForteAgent/8.00.32.1272
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

>On 05/05/2021 17:20, Benjamin Gaignard wrote:
>>=20
>> Le 05/05/2021 =C3=A0 16:55, Hans Verkuil a =C3=A9crit=C2=A0:
>>> On 20/04/2021 14:10, Benjamin Gaignard wrote:
>>>> The HEVC HANTRO driver needs to know the number of bits to skip at
>>>> the beginning of the slice header.
>>>> That is a hardware specific requirement so create a dedicated =
control
>>>> for this purpose.
>>>>
>>>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
>>>> ---
>>>>   .../userspace-api/media/drivers/hantro.rst    | 19 =
+++++++++++++++++++
>>>>   .../userspace-api/media/drivers/index.rst     |  1 +
>>>>   include/media/hevc-ctrls.h                    | 13 +++++++++++++
>>>>   3 files changed, 33 insertions(+)
>>>>   create mode 100644 =
Documentation/userspace-api/media/drivers/hantro.rst
>>>>
>>>> diff --git a/Documentation/userspace-api/media/drivers/hantro.rst =
b/Documentation/userspace-api/media/drivers/hantro.rst
>>>> new file mode 100644
>>>> index 000000000000..cd9754b4e005
>>>> --- /dev/null
>>>> +++ b/Documentation/userspace-api/media/drivers/hantro.rst
>>>> @@ -0,0 +1,19 @@
>>>> +.. SPDX-License-Identifier: GPL-2.0
>>>> +
>>>> +Hantro video decoder driver
>>>> =
+=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D
>>>> +
>>>> +The Hantro video decoder driver implements the following =
driver-specific controls:
>>>> +
>>>> +``V4L2_CID_HANTRO_HEVC_SLICE_HEADER_SKIP (integer)``
>>>> +    Specifies to Hantro HEVC video decoder driver the number of =
data (in bits) to
>>>> +    skip in the slice segment header.
>>>> +    If non-IDR, the bits to be skipped go from syntax element =
"pic_output_flag"
>>>> +    to before syntax element "slice_temporal_mvp_enabled_flag".
>>>> +    If IDR, the skipped bits are just "pic_output_flag"
>>>> +    (separate_colour_plane_flag is not supported).
>>> I'm not very keen on this. Without this information the video data =
cannot be
>>> decoded, or will it just be suboptimal?
>>=20
>> Without that information the video can't be decoded.
>>=20
>>>
>>> The problem is that a generic decoder would have to know that the HW =
is a hantro,
>>> and then call this control. If they don't (and are testing on =
non-hantro HW), then
>>> it won't work, thus defeating the purpose of the HW independent =
decoder API.
>>>
>>> Since hantro is widely used, and if there is no other way to do this =
beside explitely
>>> setting this control, then perhaps this should be part of the =
standard HEVC API.
>>> Non-hantro drivers that do not need this can just skip it.
>>=20
>> Even if I put this parameter in decode_params structure that would =
means that a generic
>> userland decoder will have to know how the compute this value for =
hantro HW since it
>> isn't something that could be done on kernel side.
>
>But since hantro is very common, any userland decoder will need to =
calculate this anyway.
>So perhaps it is better to have this as part of the decode_params?
>
>I'd like to know what others think about this.

I don't know exactly what I think on this - its all a bit of a mess. I
don't think this is going to be the last HEVC decoder that needs some
non-standard setup that can't be trivially extracted from a standard
slice header parse. So if future decoders are going to have to generate
custom attributes to cope with their quirks then Hantro probably should
too. And if Hantro is common then the userspace progs will at least have
a framework for dealing with this sort of thing so when the next oddity
comes along.

Regards

John Cox

>Regards,
>
>	Hans
>
>>=20
>>=20
>> Regards,
>> Benjamin
>>=20
>>>
>>> Regards,
>>>
>>> 	Hans
>>>
>>>> +
>>>> +.. note::
>>>> +
>>>> +        This control is not yet part of the public kernel API and
>>>> +        it is expected to change.
>>>> diff --git a/Documentation/userspace-api/media/drivers/index.rst =
b/Documentation/userspace-api/media/drivers/index.rst
>>>> index 1a9038f5f9fa..12e3c512d718 100644
>>>> --- a/Documentation/userspace-api/media/drivers/index.rst
>>>> +++ b/Documentation/userspace-api/media/drivers/index.rst
>>>> @@ -33,6 +33,7 @@ For more details see the file COPYING in the =
source distribution of Linux.
>>>>  =20
>>>>   	ccs
>>>>   	cx2341x-uapi
>>>> +        hantro
>>>>   	imx-uapi
>>>>   	max2175
>>>>   	meye-uapi
>>>> diff --git a/include/media/hevc-ctrls.h b/include/media/hevc-ctrls.h
>>>> index 8e0109eea454..b713eeed1915 100644
>>>> --- a/include/media/hevc-ctrls.h
>>>> +++ b/include/media/hevc-ctrls.h
>>>> @@ -224,4 +224,17 @@ struct v4l2_ctrl_hevc_decode_params {
>>>>   	__u64	flags;
>>>>   };
>>>>  =20
>>>> +/*  MPEG-class control IDs specific to the Hantro driver as defined=
 by V4L2 */
>>>> +#define V4L2_CID_CODEC_HANTRO_BASE				(V4L2_CTRL_CLASS_CODEC | =
0x1200)
>>>> +/*
>>>> + * V4L2_CID_HANTRO_HEVC_SLICE_HEADER_SKIP -
>>>> + * the number of data (in bits) to skip in the
>>>> + * slice segment header.
>>>> + * If non-IDR, the bits to be skipped go from syntax element =
"pic_output_flag"
>>>> + * to before syntax element "slice_temporal_mvp_enabled_flag".
>>>> + * If IDR, the skipped bits are just "pic_output_flag"
>>>> + * (separate_colour_plane_flag is not supported).
>>>> + */
>>>> +#define V4L2_CID_HANTRO_HEVC_SLICE_HEADER_SKIP	=
(V4L2_CID_CODEC_HANTRO_BASE + 0)
>>>> +
>>>>   #endif
>>>>
>>>