On 3/20/19 1:37 PM, Mauro Carvalho Chehab wrote:
> 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?
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.
>
>>> 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.
Regards,
Hans
>
>> 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
>
