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