Re: [PATCH v5 02/23] videodev2.h: add V4L2_BUF_CAP_REQUIRES_REQUESTS

[Mauro Carvalho Chehab <mchehab+samsung@kernel.org>]

Em Wed, 20 Mar 2019 13:41:42 +0100
Hans Verkuil <hverkuil@xs4all.nl> escreveu:

> On 3/20/19 1:37 PM, Mauro Carvalho Chehab wrote:
> > Em Wed, 20 Mar 2019 13:20:07 +0100
> > Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> >   
> >>>> The only affected driver is the staging cedrus driver. And that will
> >>>> actually crash if you try to use it without requests.
> >>>>
> >>>> If you look at patch 3 you'll see that it just sets the flag and doesn't
> >>>> remove any code that was supposed to check for use-without-requests.
> >>>> That's because there never was a check and the driver would just crash.
> >>>>
> >>>> So we're safe here.    
> >>>
> >>> Making it mandatory for the cedrus driver makes sense, but no other
> >>> current driver should ever use it.     
> >>
> >> The only other drivers that implement the request API are vivid and vim2m.
> >>
> >> For both the request API is optional.
> >>
> >> And of course this patch series that adds the stateless decoder support to
> >> vicodec, so vicodec is the only other driver besides the cedrus driver that
> >> sets this flag.  
> > 
> > The current vicodec implementation is only stateless?  
> 
> vicodec before this series is only stateful. After this series a new video node
> is added which is for the stateless decoder. And that device will require
> requests.

I see. see below.

> 
> >   
> >>> The problem I see is that, as we advance on improving the requests API,
> >>> non-stateless-codec drivers may end supporting the request API. 
> >>> That's perfectly fine, but such other drivers should *never* be
> >>> changed to use V4L2_BUF_CAP_REQUIRES_REQUESTS. This also applies to any
> >>> new driver that it is not implementing a stateless codec.
> >>>
> >>> Btw, as this seems to be a requirement only for stateless codec drivers,
> >>> perhaps we should (at least in Kernelspace) to use, instead, a
> >>> V4L2_BUF_CAP_STATELESS_CODEC_ONLY flag, with the V4L2 core would
> >>> translate it to V4L2_BUF_CAP_REQUIRES_REQUESTS before returning it to
> >>> userspace, and have a special #ifdef at the userspace header, in order
> >>> to prevent this flag to be set directly by a random driver.    
> >>
> >> I don't think this makes sense. Requiring requests is not something you
> >> can miss since you have to code for it.
> >>
> >> However, there is something else that we need to think about and that is
> >> that V4L2_BUF_CAP_REQUIRES_REQUESTS can be format specific. E.g. a stateless
> >> codec driver can also support a JPEG codec, and for that format requests
> >> are most likely not required at all. So this capability might actually be
> >> format-specific.  
> > 
> > Yes, on formats that don't have temporal compression, there's no sense
> > to make request API mandatory.
> > 
> > For formats that have temporal compression, the codec driver can either 
> > be stateless or stateful (or even support both modes).
> > 
> > It sounds to me that a flag like that should be returned by S_FMT and
> > TRY_FMT or on a separate ioctl.
> > 
> > It also seems to make sense if userspace could select between stateless
> > and stateful modes, if the driver supports both modes for the same
> > fourcc.  
> 
> That can't happen. The stateless formats have their own fourcc. It really
> is a different format.

Well, if we're already defining different fourcc for the stateless
codecs, then I think that your proposed patch:

	[PATCH v5.1 2/2] cedrus: set requires_requests

Is not the best way to implement it.

	Instead, I would have a helper function like:

static int v4l2_format_requires_request_api(uint32 fourcc)
{
	switch(fourcc) {
	case V4L2_PIX_FMT_MPEG2_SLICE:
		return 1;
	default:
		return 0;
	}
}

called by V4L2 core at S_FMT handler in order to set vq->requires_requests.

Also, in this case, I would add a V4L2_FMT_FLAG_REQUIRE_REQUEST_API or
V4L2_FMT_FLAG_STATELESS_CODEC flag at VIDIOC_ENUM_FMT in order to
indicate it.

Thanks,
Mauro