diff mbox series

[v10,6/9] media: uapi: Add a control for HANTRO driver

Message ID 20210420121046.181889-7-benjamin.gaignard@collabora.com
State Superseded
Headers show
Series Add HANTRO G2/HEVC decoder support for IMX8MQ | expand

Commit Message

Benjamin Gaignard April 20, 2021, 12:10 p.m. UTC
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

Comments

Ezequiel Garcia April 29, 2021, 1:43 p.m. UTC | #1
On Tue, 2021-04-20 at 14:10 +0200, 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>

Reviewed-by: Ezequiel Garcia <ezequiel@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
> +===========================
> +
> +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).
> +
> +.. 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.
>  
>         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;
>  };
>  
> +/*  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
Hans Verkuil May 5, 2021, 2:55 p.m. UTC | #2
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

> +===========================

> +

> +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?

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.

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.

>  

>  	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;

>  };

>  

> +/*  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

>
John Cox May 5, 2021, 3:18 p.m. UTC | #3
>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
>+===========================
>+
>+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).

What happens if it is a dependant_slice_segement or
output_flag_present_flag?  Those flags are all dependant on
dependant_slice_segement being false.  I'm guessing 0 but it maybe
should be documented.
Likewise if output_flag_present_flag is false pic_output_flag will not
be coded, so maybe express it as "after slice_type" rather than "before
pic_output_flag"?

Regards

John Cox

>+.. 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.
> 
> 	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;
> };
> 
>+/*  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
Hans Verkuil May 6, 2021, 12:50 p.m. UTC | #4
On 05/05/2021 17:20, Benjamin Gaignard wrote:
> 
> Le 05/05/2021 à 16:55, Hans Verkuil a écrit :
>> 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
>>> +===========================
>>> +
>>> +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?
> 
> Without that information the video can't be decoded.
> 
>>
>> 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.
> 
> 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.

Regards,

	Hans

> 
> 
> Regards,
> Benjamin
> 
>>
>> 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.
>>>   
>>>   	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;
>>>   };
>>>   
>>> +/*  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
>>>
>>
John Cox May 6, 2021, 1:11 p.m. UTC | #5
>On 05/05/2021 17:20, Benjamin Gaignard wrote:
>> 
>> Le 05/05/2021 à 16:55, Hans Verkuil a écrit :
>>> 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
>>>> +===========================
>>>> +
>>>> +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?
>> 
>> Without that information the video can't be decoded.
>> 
>>>
>>> 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.
>> 
>> 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
>
>> 
>> 
>> Regards,
>> Benjamin
>> 
>>>
>>> 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.
>>>>   
>>>>   	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;
>>>>   };
>>>>   
>>>> +/*  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
>>>>
>>>
Nicolas Dufresne May 6, 2021, 2:29 p.m. UTC | #6
Le jeudi 06 mai 2021 à 14:11 +0100, John Cox a écrit :
> > On 05/05/2021 17:20, Benjamin Gaignard wrote:

> > > 

> > > Le 05/05/2021 à 16:55, Hans Verkuil a écrit :

> > > > 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

> > > > > +===========================

> > > > > +

> > > > > +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?

> > > 

> > > Without that information the video can't be decoded.

> > > 

> > > > 

> > > > 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.

> > > 

> > > 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


There is no better way to describe the state of my own opinion about this.

> 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.


To add to this, when we moved it out of the decode_params, we were actually
making it an example. We use large structure for the common stuff because is
convenient, but with the current infrastructure, the cost of adding controls is
rather low.

So we need to think if we want to hide or highlight what looks like hardware
design specific needs. There is nothing particularly wrong in the hardware, as
Hantro traditionally parse a lot of the headers, but I suppose they don't really
want to implement skip parsers because at some point the hardware becomes quite
big and complex, skipping bits is just trivial.

One thing I've been discussing with Benjamin yesterday is that while splitting,
we also made the data exactly what the HW wants, which is a skip. A more
reusable representation would have been to provide two offsets in the header.
This way if another HW need a different skip, but with a different stop
position, you can share the start position. Though, it's no longer a 1:1 match
with how the HW is programmed, so not an easy call.

As for having more quirks in more HW, the newer chips are designed with a
constraints these days. As an example, you will notice that inside mpp (rockchip
library) they use Microsoft DXVA parameters and use that as a contraint during
the design. From comment Alex made around Mediatek, they actually used Google
downstream Linux API as a constraint. As we do cover existing API like DXVA,
NVDEC and VA as far as my review went. I don't really expect in fact newer
design to require quirks/extensions so often, but making this one a split
control would serve as an example how to keep things clear.

Now, what I believe is missing in the story is a way for userspace to detect
that extra (non standard) controls are needed. There might be other support
decoder on the platform, or even a software decoder may be more suitable for the
use cas then a corrupted output (which is what happens if you ignore the hantro
control). So perhaps we should think of way to flag the requirement for some
extra controls. Perhaps in the form of a bitmask of quirks, so the userspace can
check early if it has the required implementation and fallback to something else
if not.

This is the type of API missing we have had in many other places in the fast, we
did fix it after that fact, which was not ideal, but still acceptable. So I'm
not like oh no, we screwed up the other stable API. But we have a use case here,
perhaps we can learn from it ?

p.s. I try to avoid extensions as this makes me think of the extra paremeters
associates with the bitstream profile we may not support. We already provide
list of support profiles, and have a good story, tested with stateful decoder on
how to introduce new paremters along with new profiles.

p.s. Notice that if we want to revive the VA driver (VA does not have this
skip), we need to stop modifying the VA API, and just re-parse whatever is
missing. Having a separate control can be used as a clear indication that double
parsing is not needed for the specific implementation. Same would apply if some
Wine folks want to emulate DXVA over V4L2 API (though unlikely as this is rarely
seen on desktop).

> 

> Regards

> 

> John Cox

> 

> > Regards,

> > 

> > 	Hans

> > 

> > > 

> > > 

> > > Regards,

> > > Benjamin

> > > 

> > > > 

> > > > 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.

> > > > >   

> > > > >   	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;

> > > > >   };

> > > > >   

> > > > > +/*  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

> > > > > 

> > > >
Ezequiel Garcia May 16, 2021, 11:04 p.m. UTC | #7
Hi Hans,

On Thu, 2021-05-06 at 14:50 +0200, Hans Verkuil wrote:
> On 05/05/2021 17:20, Benjamin Gaignard wrote:

> > 

> > Le 05/05/2021 à 16:55, Hans Verkuil a écrit :

> > > 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

> > > > +===========================

> > > > +

> > > > +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?

> > 

> > Without that information the video can't be decoded.

> > 

> > > 

> > > 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.

> > 

> > 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.

> 


As you know, I'm not a fan of carrying these "unstable" APIs around.
I know it's better than nothing, but I feel they create the illusion
of the interface being supported in mainline. Since it's unstable,
it's difficult for applications to adopt them.

As Nicolas mentioned, this means neither FFmpeg nor GStreamer will adopt
these APIs, which worries me, as that means we lose two major user bases.

My personal take from this, is that we need to find ways to stabilize
our stateless codec APIs in less time and perhaps with less effort.

IMO, a less stiff interface could help us here, and that's why I think
having hardware-specific controls can be useful. Hardware designers
can be so creative :)

I'm not against introducing this specific parameter in
v4l2_ctrl_hevc_codec_params, arguing that Hantro is widely used,
but I'd like us to be open to hardware-specific controls as a way
to extend the APIs seamlessly.

Applications won't have to _know_ what hardware they are running on,
they can just use VIDIOC_QUERYCTRL to find out which controls are needed.

Thanks,
Ezequiel
Nicolas Dufresne May 18, 2021, 5:17 p.m. UTC | #8
Le dimanche 16 mai 2021 à 20:04 -0300, Ezequiel Garcia a écrit :
> Hi Hans,
> 
> On Thu, 2021-05-06 at 14:50 +0200, Hans Verkuil wrote:
> > On 05/05/2021 17:20, Benjamin Gaignard wrote:
> > > 
> > > Le 05/05/2021 à 16:55, Hans Verkuil a écrit :
> > > > 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
> > > > > +===========================
> > > > > +
> > > > > +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?
> > > 
> > > Without that information the video can't be decoded.
> > > 
> > > > 
> > > > 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.
> > > 
> > > 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.
> > 
> 
> As you know, I'm not a fan of carrying these "unstable" APIs around.
> I know it's better than nothing, but I feel they create the illusion
> of the interface being supported in mainline. Since it's unstable,
> it's difficult for applications to adopt them.
> 
> As Nicolas mentioned, this means neither FFmpeg nor GStreamer will adopt
> these APIs, which worries me, as that means we lose two major user bases.
> 
> My personal take from this, is that we need to find ways to stabilize
> our stateless codec APIs in less time and perhaps with less effort.
> 
> IMO, a less stiff interface could help us here, and that's why I think
> having hardware-specific controls can be useful. Hardware designers
> can be so creative :)
> 
> I'm not against introducing this specific parameter in
> v4l2_ctrl_hevc_codec_params, arguing that Hantro is widely used,
> but I'd like us to be open to hardware-specific controls as a way
> to extend the APIs seamlessly.
> 
> Applications won't have to _know_ what hardware they are running on,
> they can just use VIDIOC_QUERYCTRL to find out which controls are needed.

Can you extend on this, perhaps we need an RFC for this specific mechanism. I
don't immediatly see how I could enumerate controls and figure-out which one are
needed. Perhaps we need to add new control flags for mandatory control ? This
way userspace could detect unsupported HW if it finds a mandatory control that
it does not know about ?

> 
> Thanks,
> Ezequiel
>
diff mbox series

Patch

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
+===========================
+
+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).
+
+.. 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.
 
 	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;
 };
 
+/*  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