Message-ID: <a473ea02de9b54482c1d2c82db2e4c7512920022.camel@ndufresne.ca>
Date:   Tue, 18 May 2021 13:17:56 -0400
From:   Nicolas Dufresne <nicolas@...fresne.ca>
To:     Ezequiel Garcia <ezequiel@...labora.com>,
        Hans Verkuil <hverkuil-cisco@...all.nl>,
        Benjamin Gaignard <benjamin.gaignard@...labora.com>,
        p.zabel@...gutronix.de, mchehab@...nel.org, robh+dt@...nel.org,
        shawnguo@...nel.org, s.hauer@...gutronix.de, festevam@...il.com,
        lee.jones@...aro.org, gregkh@...uxfoundation.org,
        mripard@...nel.org, paul.kocialkowski@...tlin.com, wens@...e.org,
        jernej.skrabec@...l.net, emil.l.velikov@...il.com
Cc:     kernel@...gutronix.de, linux-imx@....com,
        linux-media@...r.kernel.org, linux-rockchip@...ts.infradead.org,
        devicetree@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        linux-kernel@...r.kernel.org, devel@...verdev.osuosl.org,
        kernel@...labora.com, cphealy@...il.com
Subject: Re: [PATCH v10 6/9] media: uapi: Add a control for HANTRO driver

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

Powered by blists - more mailing lists