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

[Boris Brezillon <boris.brezillon@collabora.com>]

On Mon, 04 May 2020 14:38:23 -0400
Nicolas Dufresne <nicolas.dufresne@collabora.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@collabora.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.