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

Commit Message

Dafna Hirschfeld March 6, 2019, 9:13 p.m. UTC

From: Hans Verkuil <[e-mail]>

Add capability to indicate that requests are required instead of
merely supported.

Signed-off-by: Hans Verkuil <[e-mail]>
---
 Documentation/media/uapi/v4l/vidioc-reqbufs.rst | 4 ++++
 include/uapi/linux/videodev2.h                  | 1 +
 2 files changed, 5 insertions(+)

Comments

Paul Kocialkowski March 12, 2019, 3:29 p.m. UTC | #1

Hi,

On Wed, 2019-03-06 at 13:13 -0800, Dafna Hirschfeld wrote:
> From: Hans Verkuil <[e-mail]>
> 
> Add capability to indicate that requests are required instead of
> merely supported.
> 
> Signed-off-by: Hans Verkuil <[e-mail]>

Reviewed-by: Paul Kocialkowski <[e-mail]>

Cheers,

Paul

> ---
>  Documentation/media/uapi/v4l/vidioc-reqbufs.rst | 4 ++++
>  include/uapi/linux/videodev2.h                  | 1 +
>  2 files changed, 5 insertions(+)
> 
> diff --git a/Documentation/media/uapi/v4l/vidioc-reqbufs.rst b/Documentation/media/uapi/v4l/vidioc-reqbufs.rst
> index d7faef10e39b..d42a3d9a7db3 100644
> --- a/Documentation/media/uapi/v4l/vidioc-reqbufs.rst
> +++ b/Documentation/media/uapi/v4l/vidioc-reqbufs.rst
> @@ -125,6 +125,7 @@ aborting or finishing any DMA in progress, an implicit
>  .. _V4L2-BUF-CAP-SUPPORTS-DMABUF:
>  .. _V4L2-BUF-CAP-SUPPORTS-REQUESTS:
>  .. _V4L2-BUF-CAP-SUPPORTS-ORPHANED-BUFS:
> +.. _V4L2-BUF-CAP-REQUIRES-REQUESTS:
>  
>  .. cssclass:: longtable
>  
> @@ -150,6 +151,9 @@ aborting or finishing any DMA in progress, an implicit
>        - The kernel allows calling :ref:`VIDIOC_REQBUFS` while buffers are still
>          mapped or exported via DMABUF. These orphaned buffers will be freed
>          when they are unmapped or when the exported DMABUF fds are closed.
> +    * - ``V4L2_BUF_CAP_REQUIRES_REQUESTS``
> +      - 0x00000020
> +      - This buffer type requires the use of :ref:`requests <media-request-api>`.
>  
>  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

Mauro Carvalho Chehab March 20, 2019, 10:11 a.m. UTC | #2

Em Wed,  6 Mar 2019 13:13:22 -0800
Dafna Hirschfeld <[e-mail]> escreveu:

> From: Hans Verkuil <[e-mail]>
> 
> Add capability to indicate that requests are required instead of
> merely supported.

Not sure if I liked this patch, and for sure it lacks a lot of documentation:

First of all, the patch description doesn't help. For example, it doesn't
explain or mention any use case example that would require (instead of
merely support) a request.

> 
> Signed-off-by: Hans Verkuil <[e-mail]>
> ---
>  Documentation/media/uapi/v4l/vidioc-reqbufs.rst | 4 ++++
>  include/uapi/linux/videodev2.h                  | 1 +
>  2 files changed, 5 insertions(+)
> 
> diff --git a/Documentation/media/uapi/v4l/vidioc-reqbufs.rst b/Documentation/media/uapi/v4l/vidioc-reqbufs.rst
> index d7faef10e39b..d42a3d9a7db3 100644
> --- a/Documentation/media/uapi/v4l/vidioc-reqbufs.rst
> +++ b/Documentation/media/uapi/v4l/vidioc-reqbufs.rst
> @@ -125,6 +125,7 @@ aborting or finishing any DMA in progress, an implicit
>  .. _V4L2-BUF-CAP-SUPPORTS-DMABUF:
>  .. _V4L2-BUF-CAP-SUPPORTS-REQUESTS:
>  .. _V4L2-BUF-CAP-SUPPORTS-ORPHANED-BUFS:
> +.. _V4L2-BUF-CAP-REQUIRES-REQUESTS:
>  
>  .. cssclass:: longtable
>  
> @@ -150,6 +151,9 @@ aborting or finishing any DMA in progress, an implicit
>        - The kernel allows calling :ref:`VIDIOC_REQBUFS` while buffers are still
>          mapped or exported via DMABUF. These orphaned buffers will be freed
>          when they are unmapped or when the exported DMABUF fds are closed.
> +    * - ``V4L2_BUF_CAP_REQUIRES_REQUESTS``
> +      - 0x00000020
> +      - This buffer type requires the use of :ref:`requests <media-request-api>`.

And the documentation here is really poor, as it doesn't explain what's
the API and drivers expected behavior with regards to this flag.

I mean, if, on a new driver, requests are mandatory, what happens if a
non-request-API aware application tries to use it? 

Another thing that concerns me a lot is that people might want to add it
to existing drivers. Well, if an application was written before the
addition of this driver, and request API become mandatory, such app
will stop working, if it doesn't use request API.

At very least, it should be mentioned somewhere that existing drivers
should never set this flag, as this would break it for existing
userspace apps.

Still, I would prefer to not have to add something like that.


>  
>  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

Hans Verkuil March 20, 2019, 10:39 a.m. UTC | #3

On 3/20/19 11:11 AM, Mauro Carvalho Chehab wrote:
> Em Wed,  6 Mar 2019 13:13:22 -0800
> Dafna Hirschfeld <[e-mail]> escreveu:
> 
>> From: Hans Verkuil <[e-mail]>
>>
>> Add capability to indicate that requests are required instead of
>> merely supported.
> 
> Not sure if I liked this patch, and for sure it lacks a lot of documentation:
> 
> First of all, the patch description doesn't help. For example, it doesn't
> explain or mention any use case example that would require (instead of
> merely support) a request.

Stateless codecs require the use of requests (i.e. they can't function without
this).

However, right now every driver has to check for this and return an error when
an attempt is made to stream without requests.

And userspace has no way of knowing whether requests are required by the driver
as opposed to being optional.

That's what this attempts to do: show to userspace that requests are required,
and add a vb2 flag that will force the core to check this so drivers do not need
to test for it.

Currently the only drivers that would need this are cedrus and vicodec.

> 
>>
>> Signed-off-by: Hans Verkuil <[e-mail]>
>> ---
>>  Documentation/media/uapi/v4l/vidioc-reqbufs.rst | 4 ++++
>>  include/uapi/linux/videodev2.h                  | 1 +
>>  2 files changed, 5 insertions(+)
>>
>> diff --git a/Documentation/media/uapi/v4l/vidioc-reqbufs.rst b/Documentation/media/uapi/v4l/vidioc-reqbufs.rst
>> index d7faef10e39b..d42a3d9a7db3 100644
>> --- a/Documentation/media/uapi/v4l/vidioc-reqbufs.rst
>> +++ b/Documentation/media/uapi/v4l/vidioc-reqbufs.rst
>> @@ -125,6 +125,7 @@ aborting or finishing any DMA in progress, an implicit
>>  .. _V4L2-BUF-CAP-SUPPORTS-DMABUF:
>>  .. _V4L2-BUF-CAP-SUPPORTS-REQUESTS:
>>  .. _V4L2-BUF-CAP-SUPPORTS-ORPHANED-BUFS:
>> +.. _V4L2-BUF-CAP-REQUIRES-REQUESTS:
>>  
>>  .. cssclass:: longtable
>>  
>> @@ -150,6 +151,9 @@ aborting or finishing any DMA in progress, an implicit
>>        - The kernel allows calling :ref:`VIDIOC_REQBUFS` while buffers are still
>>          mapped or exported via DMABUF. These orphaned buffers will be freed
>>          when they are unmapped or when the exported DMABUF fds are closed.
>> +    * - ``V4L2_BUF_CAP_REQUIRES_REQUESTS``
>> +      - 0x00000020
>> +      - This buffer type requires the use of :ref:`requests <media-request-api>`.
> 
> And the documentation here is really poor, as it doesn't explain what's
> the API and drivers expected behavior with regards to this flag.
> 
> I mean, if, on a new driver, requests are mandatory, what happens if a
> non-request-API aware application tries to use it? 

An error will be returned. And that error needs to be documented, I agree.

All this does is shift the check from the driver to the v4l2 core. It doesn't
change anything for userspace, except that with this capability flag userspace
can detect beforehand that requests are required.

> 
> Another thing that concerns me a lot is that people might want to add it
> to existing drivers. Well, if an application was written before the
> addition of this driver, and request API become mandatory, such app
> will stop working, if it doesn't use request API.
> At very least, it should be mentioned somewhere that existing drivers
> should never set this flag, as this would break it for existing
> userspace apps.
> 
> Still, I would prefer to not have to add something like that.

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.

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.

But this really doesn't belong in a driver. These checks should be done in the
vb2 core.

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
>

Mauro Carvalho Chehab March 20, 2019, 11:42 a.m. UTC | #4

Em Wed, 20 Mar 2019 11:39:47 +0100
Hans Verkuil <[e-mail]> escreveu:

> On 3/20/19 11:11 AM, Mauro Carvalho Chehab wrote:
> > Em Wed,  6 Mar 2019 13:13:22 -0800
> > Dafna Hirschfeld <[e-mail]> escreveu:
> >   
> >> From: Hans Verkuil <[e-mail]>
> >>
> >> Add capability to indicate that requests are required instead of
> >> merely supported.  
> > 
> > Not sure if I liked this patch, and for sure it lacks a lot of documentation:
> > 
> > First of all, the patch description doesn't help. For example, it doesn't
> > explain or mention any use case example that would require (instead of
> > merely support) a request.  
> 
> Stateless codecs require the use of requests (i.e. they can't function without
> this).
> 
> However, right now every driver has to check for this and return an error when
> an attempt is made to stream without requests.
> 
> And userspace has no way of knowing whether requests are required by the driver
> as opposed to being optional.
> 
> That's what this attempts to do: show to userspace that requests are required,
> and add a vb2 flag that will force the core to check this so drivers do not need
> to test for it.
> 
> Currently the only drivers that would need this are cedrus and vicodec.

I see. Please add a comment like that at this patch's description.

> 
> >   
> >>
> >> Signed-off-by: Hans Verkuil <[e-mail]>
> >> ---
> >>  Documentation/media/uapi/v4l/vidioc-reqbufs.rst | 4 ++++
> >>  include/uapi/linux/videodev2.h                  | 1 +
> >>  2 files changed, 5 insertions(+)
> >>
> >> diff --git a/Documentation/media/uapi/v4l/vidioc-reqbufs.rst b/Documentation/media/uapi/v4l/vidioc-reqbufs.rst
> >> index d7faef10e39b..d42a3d9a7db3 100644
> >> --- a/Documentation/media/uapi/v4l/vidioc-reqbufs.rst
> >> +++ b/Documentation/media/uapi/v4l/vidioc-reqbufs.rst
> >> @@ -125,6 +125,7 @@ aborting or finishing any DMA in progress, an implicit
> >>  .. _V4L2-BUF-CAP-SUPPORTS-DMABUF:
> >>  .. _V4L2-BUF-CAP-SUPPORTS-REQUESTS:
> >>  .. _V4L2-BUF-CAP-SUPPORTS-ORPHANED-BUFS:
> >> +.. _V4L2-BUF-CAP-REQUIRES-REQUESTS:
> >>  
> >>  .. cssclass:: longtable
> >>  
> >> @@ -150,6 +151,9 @@ aborting or finishing any DMA in progress, an implicit
> >>        - The kernel allows calling :ref:`VIDIOC_REQBUFS` while buffers are still
> >>          mapped or exported via DMABUF. These orphaned buffers will be freed
> >>          when they are unmapped or when the exported DMABUF fds are closed.
> >> +    * - ``V4L2_BUF_CAP_REQUIRES_REQUESTS``
> >> +      - 0x00000020
> >> +      - This buffer type requires the use of :ref:`requests <media-request-api>`.  
> > 
> > And the documentation here is really poor, as it doesn't explain what's
> > the API and drivers expected behavior with regards to this flag.
> > 
> > I mean, if, on a new driver, requests are mandatory, what happens if a
> > non-request-API aware application tries to use it?   
> 
> An error will be returned. And that error needs to be documented, I agree.

As discussed at the #v4l channel, EBADR error code seems to be an
appropriate error code for it. Please document it.

> 
> All this does is shift the check from the driver to the v4l2 core. It doesn't
> change anything for userspace, except that with this capability flag userspace
> can detect beforehand that requests are required.

Yeah, checking at the core makes sense.

> 
> > 
> > Another thing that concerns me a lot is that people might want to add it
> > to existing drivers. Well, if an application was written before the
> > addition of this driver, and request API become mandatory, such app
> > will stop working, if it doesn't use request API.
> > At very least, it should be mentioned somewhere that existing drivers
> > should never set this flag, as this would break it for existing
> > userspace apps.
> > 
> > Still, I would prefer to not have to add something like that.  
> 
> 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 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 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

Hans Verkuil March 20, 2019, 12:20 p.m. UTC | #5

On 3/20/19 12:42 PM, Mauro Carvalho Chehab wrote:
> Em Wed, 20 Mar 2019 11:39:47 +0100
> Hans Verkuil <[e-mail]> escreveu:
> 
>> On 3/20/19 11:11 AM, Mauro Carvalho Chehab wrote:
>>> Em Wed,  6 Mar 2019 13:13:22 -0800
>>> Dafna Hirschfeld <[e-mail]> escreveu:
>>>   
>>>> From: Hans Verkuil <[e-mail]>
>>>>
>>>> Add capability to indicate that requests are required instead of
>>>> merely supported.  
>>>
>>> Not sure if I liked this patch, and for sure it lacks a lot of documentation:
>>>
>>> First of all, the patch description doesn't help. For example, it doesn't
>>> explain or mention any use case example that would require (instead of
>>> merely support) a request.  
>>
>> Stateless codecs require the use of requests (i.e. they can't function without
>> this).
>>
>> However, right now every driver has to check for this and return an error when
>> an attempt is made to stream without requests.
>>
>> And userspace has no way of knowing whether requests are required by the driver
>> as opposed to being optional.
>>
>> That's what this attempts to do: show to userspace that requests are required,
>> and add a vb2 flag that will force the core to check this so drivers do not need
>> to test for it.
>>
>> Currently the only drivers that would need this are cedrus and vicodec.
> 
> I see. Please add a comment like that at this patch's description.
> 
>>
>>>   
>>>>
>>>> Signed-off-by: Hans Verkuil <[e-mail]>
>>>> ---
>>>>  Documentation/media/uapi/v4l/vidioc-reqbufs.rst | 4 ++++
>>>>  include/uapi/linux/videodev2.h                  | 1 +
>>>>  2 files changed, 5 insertions(+)
>>>>
>>>> diff --git a/Documentation/media/uapi/v4l/vidioc-reqbufs.rst b/Documentation/media/uapi/v4l/vidioc-reqbufs.rst
>>>> index d7faef10e39b..d42a3d9a7db3 100644
>>>> --- a/Documentation/media/uapi/v4l/vidioc-reqbufs.rst
>>>> +++ b/Documentation/media/uapi/v4l/vidioc-reqbufs.rst
>>>> @@ -125,6 +125,7 @@ aborting or finishing any DMA in progress, an implicit
>>>>  .. _V4L2-BUF-CAP-SUPPORTS-DMABUF:
>>>>  .. _V4L2-BUF-CAP-SUPPORTS-REQUESTS:
>>>>  .. _V4L2-BUF-CAP-SUPPORTS-ORPHANED-BUFS:
>>>> +.. _V4L2-BUF-CAP-REQUIRES-REQUESTS:
>>>>  
>>>>  .. cssclass:: longtable
>>>>  
>>>> @@ -150,6 +151,9 @@ aborting or finishing any DMA in progress, an implicit
>>>>        - The kernel allows calling :ref:`VIDIOC_REQBUFS` while buffers are still
>>>>          mapped or exported via DMABUF. These orphaned buffers will be freed
>>>>          when they are unmapped or when the exported DMABUF fds are closed.
>>>> +    * - ``V4L2_BUF_CAP_REQUIRES_REQUESTS``
>>>> +      - 0x00000020
>>>> +      - This buffer type requires the use of :ref:`requests <media-request-api>`.  
>>>
>>> And the documentation here is really poor, as it doesn't explain what's
>>> the API and drivers expected behavior with regards to this flag.
>>>
>>> I mean, if, on a new driver, requests are mandatory, what happens if a
>>> non-request-API aware application tries to use it?   
>>
>> An error will be returned. And that error needs to be documented, I agree.
> 
> As discussed at the #v4l channel, EBADR error code seems to be an
> appropriate error code for it. Please document it.
> 
>>
>> All this does is shift the check from the driver to the v4l2 core. It doesn't
>> change anything for userspace, except that with this capability flag userspace
>> can detect beforehand that requests are required.
> 
> Yeah, checking at the core makes sense.
> 
>>
>>>
>>> Another thing that concerns me a lot is that people might want to add it
>>> to existing drivers. Well, if an application was written before the
>>> addition of this driver, and request API become mandatory, such app
>>> will stop working, if it doesn't use request API.
>>> At very least, it should be mentioned somewhere that existing drivers
>>> should never set this flag, as this would break it for existing
>>> userspace apps.
>>>
>>> Still, I would prefer to not have to add something like that.  
>>
>> 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 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.

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.

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
>

Mauro Carvalho Chehab March 20, 2019, 12:37 p.m. UTC | #6

Em Wed, 20 Mar 2019 13:20:07 +0100
Hans Verkuil <[e-mail]> 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

Hans Verkuil March 20, 2019, 12:41 p.m. UTC | #7

On 3/20/19 1:37 PM, Mauro Carvalho Chehab wrote:
> Em Wed, 20 Mar 2019 13:20:07 +0100
> Hans Verkuil <[e-mail]> 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
>

Mauro Carvalho Chehab March 20, 2019, 1:09 p.m. UTC | #8

Em Wed, 20 Mar 2019 13:41:42 +0100
Hans Verkuil <[e-mail]> escreveu:

> On 3/20/19 1:37 PM, Mauro Carvalho Chehab wrote:
> > Em Wed, 20 Mar 2019 13:20:07 +0100
> > Hans Verkuil <[e-mail]> 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

Patch

diff --git a/Documentation/media/uapi/v4l/vidioc-reqbufs.rst b/Documentation/media/uapi/v4l/vidioc-reqbufs.rst
index d7faef10e39b..d42a3d9a7db3 100644
--- a/Documentation/media/uapi/v4l/vidioc-reqbufs.rst
+++ b/Documentation/media/uapi/v4l/vidioc-reqbufs.rst
@@ -125,6 +125,7 @@  aborting or finishing any DMA in progress, an implicit
 .. _V4L2-BUF-CAP-SUPPORTS-DMABUF:
 .. _V4L2-BUF-CAP-SUPPORTS-REQUESTS:
 .. _V4L2-BUF-CAP-SUPPORTS-ORPHANED-BUFS:
+.. _V4L2-BUF-CAP-REQUIRES-REQUESTS:
 
 .. cssclass:: longtable
 
@@ -150,6 +151,9 @@  aborting or finishing any DMA in progress, an implicit
       - The kernel allows calling :ref:`VIDIOC_REQBUFS` while buffers are still
         mapped or exported via DMABUF. These orphaned buffers will be freed
         when they are unmapped or when the exported DMABUF fds are closed.
+    * - ``V4L2_BUF_CAP_REQUIRES_REQUESTS``
+      - 0x00000020
+      - This buffer type requires the use of :ref:`requests <media-request-api>`.
 
 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