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

[Nicolas Dufresne]

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@xxxxxxxxxxxxx>
> > > > > ---
> > > > >   .../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
> > > > > 
> > > >