Em Wed, 20 Mar 2019 13:20:07 +0100
Hans Verkuil <hverkuil@xxxxxxxxx> 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?
> > 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.
> I've decided to drop the patch adding this capability flag. The vb2
> requires_requests flag remains, as does the EBADR error code + updated
> documentation for that error code, since that is still needed. But signaling
> to userspace that it is required is something we can add later when we have
> a bit more time to think about it.
Ok.
>
> I'll respin and repost the series.
>
> Regards,
>
> Hans
>
> >
> >>
> >> I believe patches 1-3 make sense, but I also agree that the documentation
> >> and commit logs can be improved.
> >>
> >> I can either respin with updated patches 1-3, or, if you still have concerns,
> >> drop 1-3 and repost the remainder of the series. But then I'll need to add
> >> checks against the use of the stateless vicodec decoder without requests in
> >> patch 21/23.
> >
> > Whatever you prefer. If the remaining patches don't require it, you could
> > just tag the pull request as new and ping me on IRC. I'll review the remaining
> > ones, skipping the V4L2_BUF_CAP_REQUIRES_REQUESTS specific patches.
> >
> >>
> >> But this really doesn't belong in a driver. These checks should be done in the
> >> vb2 core.
> >
> > Yeah, I agree.
> >
> >>
> >> Regards,
> >>
> >> Hans
> >>
> >>>
> >>>
> >>>>
> >>>> Return Value
> >>>> ============
> >>>> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> >>>> index 1db220da3bcc..97e6a6a968ba 100644
> >>>> --- a/include/uapi/linux/videodev2.h
> >>>> +++ b/include/uapi/linux/videodev2.h
> >>>> @@ -895,6 +895,7 @@ struct v4l2_requestbuffers {
> >>>> #define V4L2_BUF_CAP_SUPPORTS_DMABUF (1 << 2)
> >>>> #define V4L2_BUF_CAP_SUPPORTS_REQUESTS (1 << 3)
> >>>> #define V4L2_BUF_CAP_SUPPORTS_ORPHANED_BUFS (1 << 4)
> >>>> +#define V4L2_BUF_CAP_REQUIRES_REQUESTS (1 << 5)
> >>>>
> >>>> /**
> >>>> * struct v4l2_plane - plane info for multi-planar buffers
> >>>
> >>>
> >>>
> >>> Thanks,
> >>> Mauro
> >>>
> >>
> >
> >
> >
> > Thanks,
> > Mauro
> >
>
Thanks,
Mauro
