linux-kernel - Re: [PATCH v2 2/3] media: uapi: Add VP9 stateless decoder controls

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20200504211354.5b8cafd4@collabora.com>
Date:   Mon, 4 May 2020 21:13:54 +0200
From:   Boris Brezillon <boris.brezillon@...labora.com>
To:     Nicolas Dufresne <nicolas.dufresne@...labora.com>
Cc:     Ezequiel Garcia <ezequiel@...labora.com>,
        Hans Verkuil <hverkuil@...all.nl>, linux-media@...r.kernel.org,
        linux-rockchip@...ts.infradead.org, linux-kernel@...r.kernel.org,
        Tomasz Figa <tfiga@...omium.org>, kernel@...labora.com,
        Jonas Karlman <jonas@...boo.se>,
        Heiko Stuebner <heiko@...ech.de>,
        Alexandre Courbot <acourbot@...omium.org>,
        Jeffrey Kardatzke <jkardatzke@...omium.org>
Subject: Re: [PATCH v2 2/3] media: uapi: Add VP9 stateless decoder controls

On Mon, 04 May 2020 14:38:23 -0400
Nicolas Dufresne <nicolas.dufresne@...labora.com> wrote:

> Le lundi 04 mai 2020 à 14:01 -0400, Nicolas Dufresne a écrit :
> > Le samedi 02 mai 2020 à 19:55 -0300, Ezequiel Garcia a écrit :  
> > > +Nicolas
> > > 
> > > On Sat, 2020-05-02 at 20:37 +0200, Boris Brezillon wrote:  
> > > > On Fri, 01 May 2020 13:57:49 -0300
> > > > Ezequiel Garcia <ezequiel@...labora.com> wrote:
> > > >   
> > > > > > > +
> > > > > > > +.. tabularcolumns:: |p{1.5cm}|p{6.3cm}|p{9.4cm}|
> > > > > > > +
> > > > > > > +.. flat-table:: enum v4l2_vp9_reset_frame_context
> > > > > > > +    :header-rows:  0
> > > > > > > +    :stub-columns: 0
> > > > > > > +    :widths:       1 2
> > > > > > > +
> > > > > > > +    * - ``V4L2_VP9_RESET_FRAME_CTX_NONE``
> > > > > > > +      - Do not reset any frame context.
> > > > > > > +    * - ``V4L2_VP9_RESET_FRAME_CTX_NONE_ALT``
> > > > > > > +      - Do not reset any frame context. This is an alternative value for
> > > > > > > +        V4L2_VP9_RESET_FRAME_CTX_NONE.    
> > > > > > 
> > > > > > Add `` around V4L2_VP9_RESET_FRAME_CTX_NONE.
> > > > > >     
> > > > > 
> > > > > Hm, now that I look closer, what's the point
> > > > > of having the NONE_ALT in our uAPI if it
> > > > > has same meaning as NONE?
> > > > > 
> > > > > I think it can be removed.  
> > > > 
> > > > The intent was to match the spec so that one can pass the value
> > > > extracted from the bitstream directly.  
> > 
> > reset_frame_contextspecifies whether the frame context should be reset
> > to default values:
> >   − 0 or 1 means do not reset any frame context
> >   − 2 resets just the context specified in the frame header
> >   − 3 resets all cont
> > 
> > But aren't we going too far by making this an emum ? In Microsfot DXVA,
> > we pass that value without interpreting it in userspace. For the
> > following RKVDEC, it is (suspiciously ?) ignored. Maybe just passing
> > over the value would make more sense, less work ?  
> 
> I have looked deeper. So basically when 2 and 3, that needs to be done
> by userspace is set back the associated probs arrays to their default
> values (see section 10.5 or the spec).
> 
> https://github.com/rockchip-linux/mpp/blob/develop/mpp/codec/dec/vp9/vp9d_parser.c#L1021
> 
> It seems that for both VAAPI And DXVA, the drivers takes care of that
> reset. So I'd like to ask, shall we code these defaults inside the
> driver ? I think we do similar things in JPEG side. But if we keep it
> the way it is, this should be strictly documented, otherwise anyone
> porting from DXVA or VAAPI will be tricked by this.

IIRC, some book keeping had to be done in userspace anyway, so I didn't
feel the need for resetting probe context in kernel space (tt's always
hard to draw a clear line of what should be done in userspace and what
should be automated by kernel drivers for those stateless decoders).
I suspect some engines have hardware probs contexts, and in that case,
you'd have to reset those when this field is set to 2 or 3, but that's
not the case here.