Date:   Thu, 09 Sep 2021 15:40:36 -0400
From:   Nicolas Dufresne <nicolas@...fresne.ca>
To:     Ming Qian <ming.qian@....com>,
        "mchehab@...nel.org" <mchehab@...nel.org>,
        "shawnguo@...nel.org" <shawnguo@...nel.org>,
        "robh+dt@...nel.org" <robh+dt@...nel.org>,
        "s.hauer@...gutronix.de" <s.hauer@...gutronix.de>
Cc:     "hverkuil-cisco@...all.nl" <hverkuil-cisco@...all.nl>,
        "kernel@...gutronix.de" <kernel@...gutronix.de>,
        "festevam@...il.com" <festevam@...il.com>,
        dl-linux-imx <linux-imx@....com>,
        Aisheng Dong <aisheng.dong@....com>,
        "linux-media@...r.kernel.org" <linux-media@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [EXT] Re: [PATCH v8 03/15] media:Add v4l2 buf flag codec data.

Le jeudi 09 septembre 2021 à 02:20 +0000, Ming Qian a écrit :
> > -----Original Message-----
> > From: Nicolas Dufresne [mailto:nicolas@...fresne.ca]
> > Sent: Wednesday, September 8, 2021 9:15 PM
> > To: Ming Qian <ming.qian@....com>; mchehab@...nel.org;
> > shawnguo@...nel.org; robh+dt@...nel.org; s.hauer@...gutronix.de
> > Cc: hverkuil-cisco@...all.nl; kernel@...gutronix.de; festevam@...il.com;
> > dl-linux-imx <linux-imx@....com>; Aisheng Dong <aisheng.dong@....com>;
> > linux-media@...r.kernel.org; linux-kernel@...r.kernel.org;
> > devicetree@...r.kernel.org; linux-arm-kernel@...ts.infradead.org
> > Subject: [EXT] Re: [PATCH v8 03/15] media:Add v4l2 buf flag codec data.
> > 
> > Caution: EXT Email
> > 
> > Hi Ming,
> > 
> > thanks for the patch. I'm doing a first pass review of the new APIs you are
> > introducing.
> > 
> > Le mardi 07 septembre 2021 à 17:49 +0800, Ming Qian a écrit :
> > > In some decoing scenarios, application may queue a buffer that only
> > > contains codec config data, and the driver needs to know whether is it
> > > a frame or not.
> > > So we add a buf flag to tell this case.
> > > 
> > > Signed-off-by: Ming Qian <ming.qian@....com>
> > > Signed-off-by: Shijie Qin <shijie.qin@....com>
> > > Signed-off-by: Zhou Peng <eagle.zhou@....com>
> > > ---
> > >  Documentation/userspace-api/media/v4l/buffer.rst | 7 +++++++
> > >  include/uapi/linux/videodev2.h                   | 1 +
> > >  2 files changed, 8 insertions(+)
> > > 
> > > diff --git a/Documentation/userspace-api/media/v4l/buffer.rst
> > > b/Documentation/userspace-api/media/v4l/buffer.rst
> > > index e991ba73d873..11013bcf8a41 100644
> > > --- a/Documentation/userspace-api/media/v4l/buffer.rst
> > > +++ b/Documentation/userspace-api/media/v4l/buffer.rst
> > > @@ -607,6 +607,13 @@ Buffer Flags
> > >       the format. Any subsequent call to the
> > >       :ref:`VIDIOC_DQBUF <VIDIOC_QBUF>` ioctl will not block anymore,
> > >       but return an ``EPIPE`` error code.
> > > +    * .. _`V4L2-BUF-FLAG-CODECCONFIG`:
> > > +
> > > +      - ``V4L2_BUF_FLAG_CODECCONFIG``
> > > +      - 0x00200000
> > > +      - The buffer only contains codec config data, eg. sps and pps.
> > > +    Applications can set this bit when ``type`` refers to an output
> > > +    stream, this flag is usually used by v4l2 decoder.
> > 
> > Currently, the bottom line is that all decoders needs a frame or field
> > accompanied with the headers (only Annex B. being defined and supported for
> > now). Decoders can be more flexible (and some are). The documentation here
> > is not clear enough, remember that we must not break compatibility.
> > 
> > I think you have to clarify the intention. This flag exist in OMX and have
> > been
> > source of confusion and inter-operability since the start.
> > 
> > - If this flag is entirely optional, and is just an optimization, then
> > adding it this
> > way is fine, but the documentation should be updated.
> > 
> > - If this flag is required only when the header is split, then this flag
> > need to be
> > accompanied with a V4L2_BUF_CAP_SUPPORTS_SPLIT_CODECCONFIG (or
> > similar name, shorter could be nice). This is so that userspace can detect
> > if that
> > feature is supported or not.
> > 
> > - If having split codecconfig is strictly needed for your driver, then this
> > flag is
> > not the proper solution. The only solution I'd see for that, would be to use
> > something else then V4L2_PIX_FMT_H264 so that existing userspace are not
> > tricked into trying to use your driver the wrong way. I think such header
> > could
> > make sense with H264_NO_SC (though not super clear what this is exactly,
> > it's
> > not really used), or a clearer new format H264_AVCC/AVCC3
> 
> Hi Nicolas,
> 
>     This flag is optional, and in fact, our driver doesn't want to receive a
> splited header,
> It's best that every buffer contains a frame.
>     But in some case, the client may enqueue some buffer that only contains
> the header data without any frame data.
> And our driver need to know this case, for this type of buffer, we have two
> cases to handle.
>     1. ignore the timestamp.
> 	2. the amphion decoder needs a ring buffer for decoding, driver will
> copy the coded data to the ring buffer, and update the write pointer, then
> pass a frame count to firmware, firmware will use the frame count to determine
> whether starting decoding a frame or not, if the frame count is incorrect, it
> may led to vpu hang. So for this type of buffer, we won't increase the frame
> count.
> 
>     The flag is required only when the header is split, I agree with you that
> it's better to add a capability flag. Actually our driver doesn't want meet
> this case, but this situation does exist in some applications, I add this flag
> to help handle it.
>     Currently we meet this case in android platform.

Thanks, that clarify were this comes from. Perhaps you could drop this from your
initial patchset, and we can handle this separatly ?

I remain a bit worried about the possible VPU hang, as the door is still wide
open to DoS on this HW from random streams. Have you considered raising this
issue to Amphion ? Perhaps there is a different way you could deal with partial
frames ?

> 
> > 
> > >      * .. _`V4L2-BUF-FLAG-REQUEST-FD`:
> > > 
> > >        - ``V4L2_BUF_FLAG_REQUEST_FD``
> > > diff --git a/include/uapi/linux/videodev2.h
> > > b/include/uapi/linux/videodev2.h index 167c0e40ec06..5bb0682b4a23
> > > 100644
> > > --- a/include/uapi/linux/videodev2.h
> > > +++ b/include/uapi/linux/videodev2.h
> > > @@ -1119,6 +1119,7 @@ static inline __u64 v4l2_timeval_to_ns(const
> > struct timeval *tv)
> > >  #define V4L2_BUF_FLAG_TSTAMP_SRC_SOE         0x00010000
> > >  /* mem2mem encoder/decoder */
> > >  #define V4L2_BUF_FLAG_LAST                   0x00100000
> > > +#define V4L2_BUF_FLAG_CODECCONFIG            0x00200000
> > >  /* request_fd is valid */
> > >  #define V4L2_BUF_FLAG_REQUEST_FD             0x00800000
> > > 
> > 
> 

Powered by blists - more mailing lists