Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Message-ID: <868c17e24e0789838871167b008baf81b9876ef7.camel@ndufresne.ca>
Subject: Re: [EXT] Re: [PATCH v8 03/15] media:Add v4l2 buf flag codec data.
From:   Nicolas Dufresne <nicolas@ndufresne.ca>
To:     Ming Qian <ming.qian@nxp.com>,
        "mchehab@kernel.org" <mchehab@kernel.org>,
        "shawnguo@kernel.org" <shawnguo@kernel.org>,
        "robh+dt@kernel.org" <robh+dt@kernel.org>,
        "s.hauer@pengutronix.de" <s.hauer@pengutronix.de>
Cc:     "hverkuil-cisco@xs4all.nl" <hverkuil-cisco@xs4all.nl>,
        "kernel@pengutronix.de" <kernel@pengutronix.de>,
        "festevam@gmail.com" <festevam@gmail.com>,
        dl-linux-imx <linux-imx@nxp.com>,
        Aisheng Dong <aisheng.dong@nxp.com>,
        "linux-media@vger.kernel.org" <linux-media@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
        "linux-arm-kernel@lists.infradead.org" 
        <linux-arm-kernel@lists.infradead.org>
Date:   Thu, 09 Sep 2021 15:40:36 -0400
In-Reply-To: <AM6PR04MB6341540B142F744B2E6F3379E7D59@AM6PR04MB6341.eurprd04.prod.outlook.com>
References: <cover.1631002447.git.ming.qian@nxp.com>
         <7ef1840137417c33f5ef7ca611c90fc274926851.1631002447.git.ming.qian@nxp.com>
         <79cdfe0af999f574642314289e1734df5e2032eb.camel@ndufresne.ca>
         <AM6PR04MB6341540B142F744B2E6F3379E7D59@AM6PR04MB6341.eurprd04.prod.outlook.com>
Content-Type: text/plain; charset="UTF-8"
User-Agent: Evolution 3.40.4 (3.40.4-1.fc34) 
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Precedence: bulk

Le jeudi 09 septembre 2021 à 02:20 +0000, Ming Qian a écrit :
> > -----Original Message-----
> > From: Nicolas Dufresne [mailto:nicolas@ndufresne.ca]
> > Sent: Wednesday, September 8, 2021 9:15 PM
> > To: Ming Qian <ming.qian@nxp.com>; mchehab@kernel.org;
> > shawnguo@kernel.org; robh+dt@kernel.org; s.hauer@pengutronix.de
> > Cc: hverkuil-cisco@xs4all.nl; kernel@pengutronix.de; festevam@gmail.com;
> > dl-linux-imx <linux-imx@nxp.com>; Aisheng Dong <aisheng.dong@nxp.com>;
> > linux-media@vger.kernel.org; linux-kernel@vger.kernel.org;
> > devicetree@vger.kernel.org; linux-arm-kernel@lists.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@nxp.com>
> > > Signed-off-by: Shijie Qin <shijie.qin@nxp.com>
> > > Signed-off-by: Zhou Peng <eagle.zhou@nxp.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
> > > 
> > 
>