[RFC] Request API and V4L2 capabilities

[RFC] Request API and V4L2 capabilities

[Hans Verkuil]

Hi all,

While the Request API patch series addresses all the core API issues, there
are some high-level considerations as well:

1) How can the application tell that the Request API is supported and for
   which buffer types (capture/output) and pixel formats?

2) How can the application tell if the Request API is required as opposed to being
   optional?

3) Some controls may be required in each request, how to let userspace know this?
   Is it even necessary to inform userspace?

4) (For bonus points): How to let the application know which streaming I/O modes
   are available? That's never been possible before, but it would be very nice
   indeed if that's made explicit.

Since the Request API associates data with frame buffers it makes sense to expose
this as a new capability field in struct v4l2_requestbuffers and struct v4l2_create_buffers.

The first struct has 2 reserved fields, the second has 8, so it's not a problem to
take one for a capability field. Both structs also have a buffer type, so we know
if this is requested for a capture or output buffer type. The pixel format is known
in the driver, so HAS/REQUIRES_REQUESTS can be set based on that. I doubt we'll have
drivers where the request caps would actually depend on the pixel format, but it
theoretically possible. For both ioctls you can call them with count=0 at the start
of the application. REQBUFS has of course the side-effect of deleting all buffers,
but at the start of your application you don't have any yet. CREATE_BUFS has no
side-effects.

I propose adding these capabilities:

#define V4L2_BUF_CAP_HAS_REQUESTS	0x00000001
#define V4L2_BUF_CAP_REQUIRES_REQUESTS	0x00000002
#define V4L2_BUF_CAP_HAS_MMAP		0x00000100
#define V4L2_BUF_CAP_HAS_USERPTR	0x00000200
#define V4L2_BUF_CAP_HAS_DMABUF		0x00000400

If REQUIRES_REQUESTS is set, then HAS_REQUESTS is also set.

At this time I think that REQUIRES_REQUESTS would only need to be set for the
output queue of stateless codecs.

If capabilities is 0, then it's from an old kernel and all you know is that
requests are certainly not supported, and that MMAP is supported. Whether USERPTR
or DMABUF are supported isn't known in that case (just try it :-) ).

Strictly speaking we do not need these HAS_MMAP/USERPTR/DMABUF caps, but it is very
easy to add if we create a new capability field anyway, and it has always annoyed
the hell out of me that we didn't have a good way to let userspace know what
streaming I/O modes we support. And with vb2 it's easy to implement.

Regarding point 3: I think this should be documented next to the pixel format. I.e.
the MPEG-2 Slice format used by the stateless cedrus codec requires the request API
and that two MPEG-2 controls (slice params and quantization matrices) must be present
in each request.

I am not sure a control flag (e.g. V4L2_CTRL_FLAG_REQUIRED_IN_REQ) is needed here.
It's really implied by the fact that you use a stateless codec. It doesn't help
generic applications like v4l2-ctl or qv4l2 either since in order to support
stateless codecs they will have to know about the details of these controls anyway.

So I am inclined to say that it is not necessary to expose this information in
the API, but it has to be documented together with the pixel format documentation.

Comments? Ideas?

Regards,

	Hans

Re: [RFC] Request API and V4L2 capabilities

[Paul Kocialkowski]

[-- Attachment #1: Type: text/plain, Size: 6735 bytes --]

Hi Hans and all,

On Sat, 2018-08-04 at 15:50 +0200, Hans Verkuil wrote:
> Hi all,
> 
> While the Request API patch series addresses all the core API issues, there
> are some high-level considerations as well:
> 
> 1) How can the application tell that the Request API is supported and for
>    which buffer types (capture/output) and pixel formats?
> 
> 2) How can the application tell if the Request API is required as opposed to being
>    optional?
> 
> 3) Some controls may be required in each request, how to let userspace know this?
>    Is it even necessary to inform userspace?
> 
> 4) (For bonus points): How to let the application know which streaming I/O modes
>    are available? That's never been possible before, but it would be very nice
>    indeed if that's made explicit.

Thanks for bringing up these considerations and questions, which perhaps
cover the last missing bits for streamlined use of the request API by
userspace. I would suggest another item, related to 3):

5) How can applications tell whether the driver supports a specific
codec profile/level, not only for encoding but also for decoding? It's
common for low-end embedded hardware to not support the most advanced
profiles (e.g. H264 high profile).

> Since the Request API associates data with frame buffers it makes sense to expose
> this as a new capability field in struct v4l2_requestbuffers and struct v4l2_create_buffers.
> 
> The first struct has 2 reserved fields, the second has 8, so it's not a problem to
> take one for a capability field. Both structs also have a buffer type, so we know
> if this is requested for a capture or output buffer type. The pixel format is known
> in the driver, so HAS/REQUIRES_REQUESTS can be set based on that. I doubt we'll have
> drivers where the request caps would actually depend on the pixel format, but it
> theoretically possible. For both ioctls you can call them with count=0 at the start
> of the application. REQBUFS has of course the side-effect of deleting all buffers,
> but at the start of your application you don't have any yet. CREATE_BUFS has no
> side-effects.

My initial thoughts on this point were to have flags exposed in
v4l2_capability, but now that you're saying it, it does make sense for
the flag to be associated with a buffer rather than the global device.

In addition, I've heard of cases (IIRC it was some Rockchip platforms)
where the platform has both stateless and stateful VPUs (I think it was
stateless up to H264 and stateful for H265). This would allow supporting
these two hardware blocks under the same video device (if that makes
sense anyway). And even if there's no immediate need, it's always good
to have this level of granularity (with little drawbacks).

> I propose adding these capabilities:
> 
> #define V4L2_BUF_CAP_HAS_REQUESTS	0x00000001
> #define V4L2_BUF_CAP_REQUIRES_REQUESTS	0x00000002
> #define V4L2_BUF_CAP_HAS_MMAP		0x00000100
> #define V4L2_BUF_CAP_HAS_USERPTR	0x00000200
> #define V4L2_BUF_CAP_HAS_DMABUF		0x00000400
> 
> If REQUIRES_REQUESTS is set, then HAS_REQUESTS is also set.
> 
> At this time I think that REQUIRES_REQUESTS would only need to be set for the
> output queue of stateless codecs.
> 
> If capabilities is 0, then it's from an old kernel and all you know is that
> requests are certainly not supported, and that MMAP is supported. Whether USERPTR
> or DMABUF are supported isn't known in that case (just try it :-) ).

Sounds good to me!

> Strictly speaking we do not need these HAS_MMAP/USERPTR/DMABUF caps, but it is very
> easy to add if we create a new capability field anyway, and it has always annoyed
> the hell out of me that we didn't have a good way to let userspace know what
> streaming I/O modes we support. And with vb2 it's easy to implement.

I totally agree here, it would be very nice to take the occasion to
expose to userspace what I/O modes are available. The current try-and-
see approach works, but this feels much better indeed.

> Regarding point 3: I think this should be documented next to the pixel format. I.e.
> the MPEG-2 Slice format used by the stateless cedrus codec requires the request API
> and that two MPEG-2 controls (slice params and quantization matrices) must be present
> in each request.
> 
> I am not sure a control flag (e.g. V4L2_CTRL_FLAG_REQUIRED_IN_REQ) is needed here.
> It's really implied by the fact that you use a stateless codec. It doesn't help
> generic applications like v4l2-ctl or qv4l2 either since in order to support
> stateless codecs they will have to know about the details of these controls anyway.
> 
> So I am inclined to say that it is not necessary to expose this information in
> the API, but it has to be documented together with the pixel format documentation.

I think this is affected by considerations about codec profile/level
support. More specifically, some controls will only be required for
supporting advanced codec profiles/levels, so they can only be
explicitly marked with appropriate flags by the driver when the target
profile/level is known. And I don't think it would be sane for userspace
to explicitly set what profile/level it's aiming at. As a result, I
don't think we can explicitly mark controls as required or optional.

I also like the idea that it should instead be implicit and that the
documentation should detail which specific stateless metadata controls
are required for a given profile/level.

As for controls validation, the approach followed in the Cedrus driver
is to check that the most basic controls are filled and allow having
missing controls for those that match advanced profiles.

Since this approach feels somewhat generic enough to be applied to all
stateless VPU drivers, maybe this should be made a helper in the
framework?

In addition, I see a need for exposing the maximum profile/level that
the driver supports for decoding. I would suggest reusing the already-
existing dedicated controls used for encoding for this purpose. For
decoders, they would be used to expose the (read-only) maximum
profile/level that is supported by the hardware and keep using them as a
settable value in a range (matching the level of support) for encoders.

This is necessary for userspace to determine whether a given video can
be decoded in hardware or not. Instead of half-way decoding the video
(ending up in funky results), this would easily allow skipping hardware
decoding and e.g. falling back on software decoding.

What do you think?

Cheers,

Paul

-- 
Paul Kocialkowski, Bootlin (formerly Free Electrons)
Embedded Linux and kernel engineering
https://bootlin.com

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC] Request API and V4L2 capabilities

[Hans Verkuil]

On 08/06/2018 10:16 AM, Paul Kocialkowski wrote:
> Hi Hans and all,
> 
> On Sat, 2018-08-04 at 15:50 +0200, Hans Verkuil wrote:
>> Hi all,
>>
>> While the Request API patch series addresses all the core API issues, there
>> are some high-level considerations as well:
>>
>> 1) How can the application tell that the Request API is supported and for
>>    which buffer types (capture/output) and pixel formats?
>>
>> 2) How can the application tell if the Request API is required as opposed to being
>>    optional?
>>
>> 3) Some controls may be required in each request, how to let userspace know this?
>>    Is it even necessary to inform userspace?
>>
>> 4) (For bonus points): How to let the application know which streaming I/O modes
>>    are available? That's never been possible before, but it would be very nice
>>    indeed if that's made explicit.
> 
> Thanks for bringing up these considerations and questions, which perhaps
> cover the last missing bits for streamlined use of the request API by
> userspace. I would suggest another item, related to 3):
> 
> 5) How can applications tell whether the driver supports a specific
> codec profile/level, not only for encoding but also for decoding? It's
> common for low-end embedded hardware to not support the most advanced
> profiles (e.g. H264 high profile).
> 
>> Since the Request API associates data with frame buffers it makes sense to expose
>> this as a new capability field in struct v4l2_requestbuffers and struct v4l2_create_buffers.
>>
>> The first struct has 2 reserved fields, the second has 8, so it's not a problem to
>> take one for a capability field. Both structs also have a buffer type, so we know
>> if this is requested for a capture or output buffer type. The pixel format is known
>> in the driver, so HAS/REQUIRES_REQUESTS can be set based on that. I doubt we'll have
>> drivers where the request caps would actually depend on the pixel format, but it
>> theoretically possible. For both ioctls you can call them with count=0 at the start
>> of the application. REQBUFS has of course the side-effect of deleting all buffers,
>> but at the start of your application you don't have any yet. CREATE_BUFS has no
>> side-effects.
> 
> My initial thoughts on this point were to have flags exposed in
> v4l2_capability, but now that you're saying it, it does make sense for
> the flag to be associated with a buffer rather than the global device.
> 
> In addition, I've heard of cases (IIRC it was some Rockchip platforms)
> where the platform has both stateless and stateful VPUs (I think it was
> stateless up to H264 and stateful for H265). This would allow supporting
> these two hardware blocks under the same video device (if that makes
> sense anyway). And even if there's no immediate need, it's always good
> to have this level of granularity (with little drawbacks).
> 
>> I propose adding these capabilities:
>>
>> #define V4L2_BUF_CAP_HAS_REQUESTS	0x00000001
>> #define V4L2_BUF_CAP_REQUIRES_REQUESTS	0x00000002
>> #define V4L2_BUF_CAP_HAS_MMAP		0x00000100
>> #define V4L2_BUF_CAP_HAS_USERPTR	0x00000200
>> #define V4L2_BUF_CAP_HAS_DMABUF		0x00000400
>>
>> If REQUIRES_REQUESTS is set, then HAS_REQUESTS is also set.
>>
>> At this time I think that REQUIRES_REQUESTS would only need to be set for the
>> output queue of stateless codecs.
>>
>> If capabilities is 0, then it's from an old kernel and all you know is that
>> requests are certainly not supported, and that MMAP is supported. Whether USERPTR
>> or DMABUF are supported isn't known in that case (just try it :-) ).
> 
> Sounds good to me!
> 
>> Strictly speaking we do not need these HAS_MMAP/USERPTR/DMABUF caps, but it is very
>> easy to add if we create a new capability field anyway, and it has always annoyed
>> the hell out of me that we didn't have a good way to let userspace know what
>> streaming I/O modes we support. And with vb2 it's easy to implement.
> 
> I totally agree here, it would be very nice to take the occasion to
> expose to userspace what I/O modes are available. The current try-and-
> see approach works, but this feels much better indeed.
> 
>> Regarding point 3: I think this should be documented next to the pixel format. I.e.
>> the MPEG-2 Slice format used by the stateless cedrus codec requires the request API
>> and that two MPEG-2 controls (slice params and quantization matrices) must be present
>> in each request.
>>
>> I am not sure a control flag (e.g. V4L2_CTRL_FLAG_REQUIRED_IN_REQ) is needed here.
>> It's really implied by the fact that you use a stateless codec. It doesn't help
>> generic applications like v4l2-ctl or qv4l2 either since in order to support
>> stateless codecs they will have to know about the details of these controls anyway.
>>
>> So I am inclined to say that it is not necessary to expose this information in
>> the API, but it has to be documented together with the pixel format documentation.
> 
> I think this is affected by considerations about codec profile/level
> support. More specifically, some controls will only be required for
> supporting advanced codec profiles/levels, so they can only be
> explicitly marked with appropriate flags by the driver when the target
> profile/level is known. And I don't think it would be sane for userspace
> to explicitly set what profile/level it's aiming at. As a result, I
> don't think we can explicitly mark controls as required or optional.
> 
> I also like the idea that it should instead be implicit and that the
> documentation should detail which specific stateless metadata controls
> are required for a given profile/level.
> 
> As for controls validation, the approach followed in the Cedrus driver
> is to check that the most basic controls are filled and allow having
> missing controls for those that match advanced profiles.
> 
> Since this approach feels somewhat generic enough to be applied to all
> stateless VPU drivers, maybe this should be made a helper in the
> framework?

Sounds reasonable. Not sure if it will be in the first version, but it is
easy to add later.

> In addition, I see a need for exposing the maximum profile/level that
> the driver supports for decoding. I would suggest reusing the already-
> existing dedicated controls used for encoding for this purpose. For
> decoders, they would be used to expose the (read-only) maximum
> profile/level that is supported by the hardware and keep using them as a
> settable value in a range (matching the level of support) for encoders.
> 
> This is necessary for userspace to determine whether a given video can
> be decoded in hardware or not. Instead of half-way decoding the video
> (ending up in funky results), this would easily allow skipping hardware
> decoding and e.g. falling back on software decoding.

I think it might be better to expose this through new read-only bitmask
controls: i.e. a bitmask containing the supported profiles and levels.

Reusing the existing controls for a decoder is odd since there is not
really a concept of a 'current' value since you just want to report what
is supported. And I am not sure if all decoders can report the profile
or level that they detect.

Regards,

	Hans

Re: [RFC] Request API and V4L2 capabilities

[Paul Kocialkowski]

[-- Attachment #1: Type: text/plain, Size: 4686 bytes --]

Hi,

On Mon, 2018-08-06 at 10:32 +0200, Hans Verkuil wrote:
> On 08/06/2018 10:16 AM, Paul Kocialkowski wrote:
> > On Sat, 2018-08-04 at 15:50 +0200, Hans Verkuil wrote:
> > > Regarding point 3: I think this should be documented next to the pixel format. I.e.
> > > the MPEG-2 Slice format used by the stateless cedrus codec requires the request API
> > > and that two MPEG-2 controls (slice params and quantization matrices) must be present
> > > in each request.
> > > 
> > > I am not sure a control flag (e.g. V4L2_CTRL_FLAG_REQUIRED_IN_REQ) is needed here.
> > > It's really implied by the fact that you use a stateless codec. It doesn't help
> > > generic applications like v4l2-ctl or qv4l2 either since in order to support
> > > stateless codecs they will have to know about the details of these controls anyway.
> > > 
> > > So I am inclined to say that it is not necessary to expose this information in
> > > the API, but it has to be documented together with the pixel format documentation.
> > 
> > I think this is affected by considerations about codec profile/level
> > support. More specifically, some controls will only be required for
> > supporting advanced codec profiles/levels, so they can only be
> > explicitly marked with appropriate flags by the driver when the target
> > profile/level is known. And I don't think it would be sane for userspace
> > to explicitly set what profile/level it's aiming at. As a result, I
> > don't think we can explicitly mark controls as required or optional.
> > 
> > I also like the idea that it should instead be implicit and that the
> > documentation should detail which specific stateless metadata controls
> > are required for a given profile/level.
> > 
> > As for controls validation, the approach followed in the Cedrus driver
> > is to check that the most basic controls are filled and allow having
> > missing controls for those that match advanced profiles.
> > 
> > Since this approach feels somewhat generic enough to be applied to all
> > stateless VPU drivers, maybe this should be made a helper in the
> > framework?
> 
> Sounds reasonable. Not sure if it will be in the first version, but it is
> easy to add later.

Definitely, I don't think this is such a high priority for now either.

> > In addition, I see a need for exposing the maximum profile/level that
> > the driver supports for decoding. I would suggest reusing the already-
> > existing dedicated controls used for encoding for this purpose. For
> > decoders, they would be used to expose the (read-only) maximum
> > profile/level that is supported by the hardware and keep using them as a
> > settable value in a range (matching the level of support) for encoders.
> > 
> > This is necessary for userspace to determine whether a given video can
> > be decoded in hardware or not. Instead of half-way decoding the video
> > (ending up in funky results), this would easily allow skipping hardware
> > decoding and e.g. falling back on software decoding.
> 
> I think it might be better to expose this through new read-only bitmask
> controls: i.e. a bitmask containing the supported profiles and levels.

It seems that this is more or less what the coda driver is doing for
decoding actually, although it uses a menu control between min/max
supported profile/levels, with a mask to "blacklist" the unsupported
values. Then, the V4L2_CTRL_FLAG_READ_ONLY flag is set to keep the
control read-only.

> Reusing the existing controls for a decoder is odd since there is not
> really a concept of a 'current' value since you just want to report what
> is supported. And I am not sure if all decoders can report the profile
> or level that they detect.

Is that really a problem when the READ_ONLY flag is set? I thought it
was designed to fit this specific case, when the driver reports a value
that userspace cannot affect.

Otherwise, I agree that having a bitmask type would be a better fit, but
I think it would be beneficial to keep the already-defined control and
associated values, which implies using the menu control type for both
encoders and decoders.

If this is not an option, I would be in favour of adding per-codec read-
only bitmask controls (e.g. for H264 something like
V4L2_CID_MPEG_VIDEO_H264_PROFILE_SUPPORT) that expose the already-
existing profile/level definitions as bit identifiers (a bit like coda
is using them to craft a mask for the menu items to blacklist) for
decoding only.

What do you think?

Cheers,

Paul

-- 
Paul Kocialkowski, Bootlin (formerly Free Electrons)
Embedded Linux and kernel engineering
https://bootlin.com

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC] Request API and V4L2 capabilities

[Hans Verkuil]

On 08/06/2018 11:13 AM, Paul Kocialkowski wrote:
> Hi,
> 
> On Mon, 2018-08-06 at 10:32 +0200, Hans Verkuil wrote:
>> On 08/06/2018 10:16 AM, Paul Kocialkowski wrote:
>>> On Sat, 2018-08-04 at 15:50 +0200, Hans Verkuil wrote:
>>>> Regarding point 3: I think this should be documented next to the pixel format. I.e.
>>>> the MPEG-2 Slice format used by the stateless cedrus codec requires the request API
>>>> and that two MPEG-2 controls (slice params and quantization matrices) must be present
>>>> in each request.
>>>>
>>>> I am not sure a control flag (e.g. V4L2_CTRL_FLAG_REQUIRED_IN_REQ) is needed here.
>>>> It's really implied by the fact that you use a stateless codec. It doesn't help
>>>> generic applications like v4l2-ctl or qv4l2 either since in order to support
>>>> stateless codecs they will have to know about the details of these controls anyway.
>>>>
>>>> So I am inclined to say that it is not necessary to expose this information in
>>>> the API, but it has to be documented together with the pixel format documentation.
>>>
>>> I think this is affected by considerations about codec profile/level
>>> support. More specifically, some controls will only be required for
>>> supporting advanced codec profiles/levels, so they can only be
>>> explicitly marked with appropriate flags by the driver when the target
>>> profile/level is known. And I don't think it would be sane for userspace
>>> to explicitly set what profile/level it's aiming at. As a result, I
>>> don't think we can explicitly mark controls as required or optional.
>>>
>>> I also like the idea that it should instead be implicit and that the
>>> documentation should detail which specific stateless metadata controls
>>> are required for a given profile/level.
>>>
>>> As for controls validation, the approach followed in the Cedrus driver
>>> is to check that the most basic controls are filled and allow having
>>> missing controls for those that match advanced profiles.
>>>
>>> Since this approach feels somewhat generic enough to be applied to all
>>> stateless VPU drivers, maybe this should be made a helper in the
>>> framework?
>>
>> Sounds reasonable. Not sure if it will be in the first version, but it is
>> easy to add later.
> 
> Definitely, I don't think this is such a high priority for now either.
> 
>>> In addition, I see a need for exposing the maximum profile/level that
>>> the driver supports for decoding. I would suggest reusing the already-
>>> existing dedicated controls used for encoding for this purpose. For
>>> decoders, they would be used to expose the (read-only) maximum
>>> profile/level that is supported by the hardware and keep using them as a
>>> settable value in a range (matching the level of support) for encoders.
>>>
>>> This is necessary for userspace to determine whether a given video can
>>> be decoded in hardware or not. Instead of half-way decoding the video
>>> (ending up in funky results), this would easily allow skipping hardware
>>> decoding and e.g. falling back on software decoding.
>>
>> I think it might be better to expose this through new read-only bitmask
>> controls: i.e. a bitmask containing the supported profiles and levels.
> 
> It seems that this is more or less what the coda driver is doing for
> decoding actually, although it uses a menu control between min/max
> supported profile/levels, with a mask to "blacklist" the unsupported
> values. Then, the V4L2_CTRL_FLAG_READ_ONLY flag is set to keep the
> control read-only.
> 
>> Reusing the existing controls for a decoder is odd since there is not
>> really a concept of a 'current' value since you just want to report what
>> is supported. And I am not sure if all decoders can report the profile
>> or level that they detect.
> 
> Is that really a problem when the READ_ONLY flag is set? I thought it
> was designed to fit this specific case, when the driver reports a value
> that userspace cannot affect.

Well, for read-only menu controls the current value of the control would
have to indicate what the current profile/level is that is being decoded.

That's not really relevant since what you want is just to query the
supported profiles/levels. A read-only bitmask control is the fastest
method (if only because using a menu control requires the application to
enumerate all possibilities with QUERYMENU).

> 
> Otherwise, I agree that having a bitmask type would be a better fit, but
> I think it would be beneficial to keep the already-defined control and
> associated values, which implies using the menu control type for both
> encoders and decoders.
> 
> If this is not an option, I would be in favour of adding per-codec read-
> only bitmask controls (e.g. for H264 something like
> V4L2_CID_MPEG_VIDEO_H264_PROFILE_SUPPORT) that expose the already-
> existing profile/level definitions as bit identifiers (a bit like coda
> is using them to craft a mask for the menu items to blacklist) for
> decoding only.

That's what I have in mind, yes. I'd like Tomasz' input as well, though.

Regards,

	Hans

> What do you think?
> 
> Cheers,
> 
> Paul
> 

Re: [RFC] Request API and V4L2 capabilities

[Paul Kocialkowski]

[-- Attachment #1: Type: text/plain, Size: 6049 bytes --]

On Mon, 2018-08-06 at 11:23 +0200, Hans Verkuil wrote:
> On 08/06/2018 11:13 AM, Paul Kocialkowski wrote:
> > Hi,
> > 
> > On Mon, 2018-08-06 at 10:32 +0200, Hans Verkuil wrote:
> > > On 08/06/2018 10:16 AM, Paul Kocialkowski wrote:
> > > > On Sat, 2018-08-04 at 15:50 +0200, Hans Verkuil wrote:
> > > > > Regarding point 3: I think this should be documented next to the pixel format. I.e.
> > > > > the MPEG-2 Slice format used by the stateless cedrus codec requires the request API
> > > > > and that two MPEG-2 controls (slice params and quantization matrices) must be present
> > > > > in each request.
> > > > > 
> > > > > I am not sure a control flag (e.g. V4L2_CTRL_FLAG_REQUIRED_IN_REQ) is needed here.
> > > > > It's really implied by the fact that you use a stateless codec. It doesn't help
> > > > > generic applications like v4l2-ctl or qv4l2 either since in order to support
> > > > > stateless codecs they will have to know about the details of these controls anyway.
> > > > > 
> > > > > So I am inclined to say that it is not necessary to expose this information in
> > > > > the API, but it has to be documented together with the pixel format documentation.
> > > > 
> > > > I think this is affected by considerations about codec profile/level
> > > > support. More specifically, some controls will only be required for
> > > > supporting advanced codec profiles/levels, so they can only be
> > > > explicitly marked with appropriate flags by the driver when the target
> > > > profile/level is known. And I don't think it would be sane for userspace
> > > > to explicitly set what profile/level it's aiming at. As a result, I
> > > > don't think we can explicitly mark controls as required or optional.
> > > > 
> > > > I also like the idea that it should instead be implicit and that the
> > > > documentation should detail which specific stateless metadata controls
> > > > are required for a given profile/level.
> > > > 
> > > > As for controls validation, the approach followed in the Cedrus driver
> > > > is to check that the most basic controls are filled and allow having
> > > > missing controls for those that match advanced profiles.
> > > > 
> > > > Since this approach feels somewhat generic enough to be applied to all
> > > > stateless VPU drivers, maybe this should be made a helper in the
> > > > framework?
> > > 
> > > Sounds reasonable. Not sure if it will be in the first version, but it is
> > > easy to add later.
> > 
> > Definitely, I don't think this is such a high priority for now either.
> > 
> > > > In addition, I see a need for exposing the maximum profile/level that
> > > > the driver supports for decoding. I would suggest reusing the already-
> > > > existing dedicated controls used for encoding for this purpose. For
> > > > decoders, they would be used to expose the (read-only) maximum
> > > > profile/level that is supported by the hardware and keep using them as a
> > > > settable value in a range (matching the level of support) for encoders.
> > > > 
> > > > This is necessary for userspace to determine whether a given video can
> > > > be decoded in hardware or not. Instead of half-way decoding the video
> > > > (ending up in funky results), this would easily allow skipping hardware
> > > > decoding and e.g. falling back on software decoding.
> > > 
> > > I think it might be better to expose this through new read-only bitmask
> > > controls: i.e. a bitmask containing the supported profiles and levels.
> > 
> > It seems that this is more or less what the coda driver is doing for
> > decoding actually, although it uses a menu control between min/max
> > supported profile/levels, with a mask to "blacklist" the unsupported
> > values. Then, the V4L2_CTRL_FLAG_READ_ONLY flag is set to keep the
> > control read-only.
> > 
> > > Reusing the existing controls for a decoder is odd since there is not
> > > really a concept of a 'current' value since you just want to report what
> > > is supported. And I am not sure if all decoders can report the profile
> > > or level that they detect.
> > 
> > Is that really a problem when the READ_ONLY flag is set? I thought it
> > was designed to fit this specific case, when the driver reports a value
> > that userspace cannot affect.
> 
> Well, for read-only menu controls the current value of the control would
> have to indicate what the current profile/level is that is being decoded.
>
> That's not really relevant since what you want is just to query the
> supported profiles/levels. A read-only bitmask control is the fastest
> method (if only because using a menu control requires the application to
> enumerate all possibilities with QUERYMENU).

Ah yes, I finally understand the issue with what the current control
value represents here. Since I don't think the driver should have to
bother with figuring out the profile in use (as expressed earlier, I
think it should be implicit, through the codec metadata controls and
features used), I no longer believe it's best to have the same control
for both encoding and decoding.

> > Otherwise, I agree that having a bitmask type would be a better fit, but
> > I think it would be beneficial to keep the already-defined control and
> > associated values, which implies using the menu control type for both
> > encoders and decoders.
> > 
> > If this is not an option, I would be in favour of adding per-codec read-
> > only bitmask controls (e.g. for H264 something like
> > V4L2_CID_MPEG_VIDEO_H264_PROFILE_SUPPORT) that expose the already-
> > existing profile/level definitions as bit identifiers (a bit like coda
> > is using them to craft a mask for the menu items to blacklist) for
> > decoding only.
> 
> That's what I have in mind, yes. I'd like Tomasz' input as well, though.

Okay, so this sounds good to me at this point!

Cheers,

Paul

-- 
Paul Kocialkowski, Bootlin (formerly Free Electrons)
Embedded Linux and kernel engineering
https://bootlin.com

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC] Request API and V4L2 capabilities

[Ezequiel Garcia]

On 4 August 2018 at 10:50, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> Hi all,
>
> While the Request API patch series addresses all the core API issues, there
> are some high-level considerations as well:
>
> 1) How can the application tell that the Request API is supported and for
>    which buffer types (capture/output) and pixel formats?
>
> 2) How can the application tell if the Request API is required as opposed to being
>    optional?
>
> 3) Some controls may be required in each request, how to let userspace know this?
>    Is it even necessary to inform userspace?
>
> 4) (For bonus points): How to let the application know which streaming I/O modes
>    are available? That's never been possible before, but it would be very nice
>    indeed if that's made explicit.
>
> Since the Request API associates data with frame buffers it makes sense to expose
> this as a new capability field in struct v4l2_requestbuffers and struct v4l2_create_buffers.
>
> The first struct has 2 reserved fields, the second has 8, so it's not a problem to
> take one for a capability field. Both structs also have a buffer type, so we know
> if this is requested for a capture or output buffer type. The pixel format is known
> in the driver, so HAS/REQUIRES_REQUESTS can be set based on that. I doubt we'll have
> drivers where the request caps would actually depend on the pixel format, but it
> theoretically possible.

Actually, I think that for stateless JPEG encoders and decoders, an application
could work without the Request API. In that case, the same encoding/decoding
parameters would be used for all the encoded/decoded buffers.

So, it seems that having per-pixelformat capabilities sounds correct.

> For both ioctls you can call them with count=0 at the start
> of the application. REQBUFS has of course the side-effect of deleting all buffers,
> but at the start of your application you don't have any yet. CREATE_BUFS has no
> side-effects.
>
> I propose adding these capabilities:
>
> #define V4L2_BUF_CAP_HAS_REQUESTS       0x00000001
> #define V4L2_BUF_CAP_REQUIRES_REQUESTS  0x00000002
> #define V4L2_BUF_CAP_HAS_MMAP           0x00000100
> #define V4L2_BUF_CAP_HAS_USERPTR        0x00000200
> #define V4L2_BUF_CAP_HAS_DMABUF         0x00000400
>
> If REQUIRES_REQUESTS is set, then HAS_REQUESTS is also set.
>
> At this time I think that REQUIRES_REQUESTS would only need to be set for the
> output queue of stateless codecs.
>
> If capabilities is 0, then it's from an old kernel and all you know is that
> requests are certainly not supported, and that MMAP is supported. Whether USERPTR
> or DMABUF are supported isn't known in that case (just try it :-) ).
>
> Strictly speaking we do not need these HAS_MMAP/USERPTR/DMABUF caps, but it is very
> easy to add if we create a new capability field anyway, and it has always annoyed
> the hell out of me that we didn't have a good way to let userspace know what
> streaming I/O modes we support. And with vb2 it's easy to implement.
>
> Regarding point 3: I think this should be documented next to the pixel format. I.e.
> the MPEG-2 Slice format used by the stateless cedrus codec requires the request API
> and that two MPEG-2 controls (slice params and quantization matrices) must be present
> in each request.
>
> I am not sure a control flag (e.g. V4L2_CTRL_FLAG_REQUIRED_IN_REQ) is needed here.
> It's really implied by the fact that you use a stateless codec. It doesn't help
> generic applications like v4l2-ctl or qv4l2 either since in order to support
> stateless codecs they will have to know about the details of these controls anyway.
>

This makes a lot of sense to me.

> So I am inclined to say that it is not necessary to expose this information in
> the API, but it has to be documented together with the pixel format documentation.
>
> Comments? Ideas?
>

-- 
Ezequiel García, VanguardiaSur
www.vanguardiasur.com.ar

Re: [RFC] Request API and V4L2 capabilities

[Mauro Carvalho Chehab]

Em Sat, 4 Aug 2018 15:50:04 +0200
Hans Verkuil <hverkuil@xs4all.nl> escreveu:

> Hi all,
> 
> While the Request API patch series addresses all the core API issues, there
> are some high-level considerations as well:
> 
> 1) How can the application tell that the Request API is supported and for
>    which buffer types (capture/output) and pixel formats?
> 
> 2) How can the application tell if the Request API is required as opposed to being
>    optional?

Huh? Why would it be mandatory?

> 
> 3) Some controls may be required in each request, how to let userspace know this?
>    Is it even necessary to inform userspace?

Again, why would it need to have a set of mandatory controls for requests
to work? If this is really required,  it should have a way to send such
list to userspace.

> 
> 4) (For bonus points): How to let the application know which streaming I/O modes
>    are available? That's never been possible before, but it would be very nice
>    indeed if that's made explicit.
> 
> Since the Request API associates data with frame buffers it makes sense to expose
> this as a new capability field in struct v4l2_requestbuffers and struct v4l2_create_buffers.
> 
> The first struct has 2 reserved fields, the second has 8, so it's not a problem to
> take one for a capability field. Both structs also have a buffer type, so we know
> if this is requested for a capture or output buffer type. The pixel format is known
> in the driver, so HAS/REQUIRES_REQUESTS can be set based on that. I doubt we'll have
> drivers where the request caps would actually depend on the pixel format, but it
> theoretically possible. For both ioctls you can call them with count=0 at the start
> of the application. REQBUFS has of course the side-effect of deleting all buffers,
> but at the start of your application you don't have any yet. CREATE_BUFS has no
> side-effects.
> 
> I propose adding these capabilities:
> 
> #define V4L2_BUF_CAP_HAS_REQUESTS	0x00000001

I'm OK with that.

> #define V4L2_BUF_CAP_REQUIRES_REQUESTS	0x00000002

But I'm not ok with breaking even more userspace support by forcing 
requests.

> #define V4L2_BUF_CAP_HAS_MMAP		0x00000100
> #define V4L2_BUF_CAP_HAS_USERPTR	0x00000200
> #define V4L2_BUF_CAP_HAS_DMABUF		0x00000400

Those sounds ok to me too.

> 
> If REQUIRES_REQUESTS is set, then HAS_REQUESTS is also set.
> 
> At this time I think that REQUIRES_REQUESTS would only need to be set for the
> output queue of stateless codecs.

Same as before: I don't see the need of support a request-only driver.

> 
> If capabilities is 0, then it's from an old kernel and all you know is that
> requests are certainly not supported, and that MMAP is supported. Whether USERPTR
> or DMABUF are supported isn't known in that case (just try it :-) ).
> 
> Strictly speaking we do not need these HAS_MMAP/USERPTR/DMABUF caps, but it is very
> easy to add if we create a new capability field anyway, and it has always annoyed
> the hell out of me that we didn't have a good way to let userspace know what
> streaming I/O modes we support. And with vb2 it's easy to implement.

Yeah, that sounds a bonus to me too.

> Regarding point 3: I think this should be documented next to the pixel format. I.e.
> the MPEG-2 Slice format used by the stateless cedrus codec requires the request API
> and that two MPEG-2 controls (slice params and quantization matrices) must be present
> in each request.

Makes sense to document with the pixel format...

> I am not sure a control flag (e.g. V4L2_CTRL_FLAG_REQUIRED_IN_REQ) is needed here.

but it sounds worth to also have a flag.

> It's really implied by the fact that you use a stateless codec. It doesn't help
> generic applications like v4l2-ctl or qv4l2 either since in order to support
> stateless codecs they will have to know about the details of these controls anyway.

Yeah, but they could skip enum those ioctls if they see one marked with
V4L2_CTRL_FLAG_REQUIRED_IN_REQ and don't know how to use. Then, default
to not use request API. 

Then, the driver would use a default that would work (even not providing
the best possible compression).

> So I am inclined to say that it is not necessary to expose this information in
> the API, but it has to be documented together with the pixel format documentation.
> 
> Comments? Ideas?
> 
> Regards,
> 
> 	Hans

Thanks,
Mauro

Re: [RFC] Request API and V4L2 capabilities

[Maxime Jourdan]

2018-08-06 11:13 GMT+02:00 Paul Kocialkowski <paul.kocialkowski@bootlin.com>:
> Hi,
>
> On Mon, 2018-08-06 at 10:32 +0200, Hans Verkuil wrote:
>> On 08/06/2018 10:16 AM, Paul Kocialkowski wrote:
>> > On Sat, 2018-08-04 at 15:50 +0200, Hans Verkuil wrote:
>> > > Regarding point 3: I think this should be documented next to the pixel format. I.e.
>> > > the MPEG-2 Slice format used by the stateless cedrus codec requires the request API
>> > > and that two MPEG-2 controls (slice params and quantization matrices) must be present
>> > > in each request.
>> > >
>> > > I am not sure a control flag (e.g. V4L2_CTRL_FLAG_REQUIRED_IN_REQ) is needed here.
>> > > It's really implied by the fact that you use a stateless codec. It doesn't help
>> > > generic applications like v4l2-ctl or qv4l2 either since in order to support
>> > > stateless codecs they will have to know about the details of these controls anyway.
>> > >
>> > > So I am inclined to say that it is not necessary to expose this information in
>> > > the API, but it has to be documented together with the pixel format documentation.
>> >
>> > I think this is affected by considerations about codec profile/level
>> > support. More specifically, some controls will only be required for
>> > supporting advanced codec profiles/levels, so they can only be
>> > explicitly marked with appropriate flags by the driver when the target
>> > profile/level is known. And I don't think it would be sane for userspace
>> > to explicitly set what profile/level it's aiming at. As a result, I
>> > don't think we can explicitly mark controls as required or optional.
>> >
>> > I also like the idea that it should instead be implicit and that the
>> > documentation should detail which specific stateless metadata controls
>> > are required for a given profile/level.
>> >
>> > As for controls validation, the approach followed in the Cedrus driver
>> > is to check that the most basic controls are filled and allow having
>> > missing controls for those that match advanced profiles.
>> >
>> > Since this approach feels somewhat generic enough to be applied to all
>> > stateless VPU drivers, maybe this should be made a helper in the
>> > framework?
>>
>> Sounds reasonable. Not sure if it will be in the first version, but it is
>> easy to add later.
>
> Definitely, I don't think this is such a high priority for now either.
>
>> > In addition, I see a need for exposing the maximum profile/level that
>> > the driver supports for decoding. I would suggest reusing the already-
>> > existing dedicated controls used for encoding for this purpose. For
>> > decoders, they would be used to expose the (read-only) maximum
>> > profile/level that is supported by the hardware and keep using them as a
>> > settable value in a range (matching the level of support) for encoders.
>> >
>> > This is necessary for userspace to determine whether a given video can
>> > be decoded in hardware or not. Instead of half-way decoding the video
>> > (ending up in funky results), this would easily allow skipping hardware
>> > decoding and e.g. falling back on software decoding.
>>
>> I think it might be better to expose this through new read-only bitmask
>> controls: i.e. a bitmask containing the supported profiles and levels.
>
> It seems that this is more or less what the coda driver is doing for
> decoding actually, although it uses a menu control between min/max
> supported profile/levels, with a mask to "blacklist" the unsupported
> values. Then, the V4L2_CTRL_FLAG_READ_ONLY flag is set to keep the
> control read-only.
>
>> Reusing the existing controls for a decoder is odd since there is not
>> really a concept of a 'current' value since you just want to report what
>> is supported. And I am not sure if all decoders can report the profile
>> or level that they detect.
>
> Is that really a problem when the READ_ONLY flag is set? I thought it
> was designed to fit this specific case, when the driver reports a value
> that userspace cannot affect.
>
> Otherwise, I agree that having a bitmask type would be a better fit, but
> I think it would be beneficial to keep the already-defined control and
> associated values, which implies using the menu control type for both
> encoders and decoders.
>
> If this is not an option, I would be in favour of adding per-codec read-
> only bitmask controls (e.g. for H264 something like
> V4L2_CID_MPEG_VIDEO_H264_PROFILE_SUPPORT) that expose the already-
> existing profile/level definitions as bit identifiers (a bit like coda
> is using them to craft a mask for the menu items to blacklist) for
> decoding only.
>
> What do you think?

Hi Paul, I think we need to go deeper than just exposing the supported
profiles/levels and also include a way to query the CAPTURE pixel
formats that are supported for each profile.

Maybe HEVC Main produces yuv420p but HEVC Main10 gives you
yuv420p10le. Maybe H.264 HiP produces NV12 but H.264 Hi422P produces
YUYV while also supporting down-sampling to NV12.

I don't know the specifics of each platform and the only example I can
think of is the Amlogic HEVC decoder that can produce NV12 for Main,
but only outputs a compressed proprietary format for Main10.

I unfortunately don't have an idea about how to implement that, but
I'll think about it.

Maxime

> Cheers,
>
> Paul
>
> --
> Paul Kocialkowski, Bootlin (formerly Free Electrons)
> Embedded Linux and kernel engineering
> https://bootlin.com

Re: [RFC] Request API and V4L2 capabilities

[Nicolas Dufresne]

[-- Attachment #1: Type: text/plain, Size: 7369 bytes --]

Le lundi 06 août 2018 à 10:16 +0200, Paul Kocialkowski a écrit :
> Hi Hans and all,
> 
> On Sat, 2018-08-04 at 15:50 +0200, Hans Verkuil wrote:
> > Hi all,
> > 
> > While the Request API patch series addresses all the core API issues, there
> > are some high-level considerations as well:
> > 
> > 1) How can the application tell that the Request API is supported and for
> >    which buffer types (capture/output) and pixel formats?
> > 
> > 2) How can the application tell if the Request API is required as opposed to being
> >    optional?
> > 
> > 3) Some controls may be required in each request, how to let userspace know this?
> >    Is it even necessary to inform userspace?
> > 
> > 4) (For bonus points): How to let the application know which streaming I/O modes
> >    are available? That's never been possible before, but it would be very nice
> >    indeed if that's made explicit.
> 
> Thanks for bringing up these considerations and questions, which perhaps
> cover the last missing bits for streamlined use of the request API by
> userspace. I would suggest another item, related to 3):
> 
> 5) How can applications tell whether the driver supports a specific
> codec profile/level, not only for encoding but also for decoding? It's
> common for low-end embedded hardware to not support the most advanced
> profiles (e.g. H264 high profile).

Hi Paul, after some discussion with Philip, he sent a proposal patch
that enables profile/level extended CID support to decoders too. The
control is made read-only, the point is not really the CID get/set but
that the controls allow enumerating the supported values. This seems
quite straightforward and easy to use.

This enumeration is already provided this way some of the existing
sate-full encoders. 

> 
> > Since the Request API associates data with frame buffers it makes sense to expose
> > this as a new capability field in struct v4l2_requestbuffers and struct v4l2_create_buffers.
> > 
> > The first struct has 2 reserved fields, the second has 8, so it's not a problem to
> > take one for a capability field. Both structs also have a buffer type, so we know
> > if this is requested for a capture or output buffer type. The pixel format is known
> > in the driver, so HAS/REQUIRES_REQUESTS can be set based on that. I doubt we'll have
> > drivers where the request caps would actually depend on the pixel format, but it
> > theoretically possible. For both ioctls you can call them with count=0 at the start
> > of the application. REQBUFS has of course the side-effect of deleting all buffers,
> > but at the start of your application you don't have any yet. CREATE_BUFS has no
> > side-effects.
> 
> My initial thoughts on this point were to have flags exposed in
> v4l2_capability, but now that you're saying it, it does make sense for
> the flag to be associated with a buffer rather than the global device.
> 
> In addition, I've heard of cases (IIRC it was some Rockchip platforms)
> where the platform has both stateless and stateful VPUs (I think it was
> stateless up to H264 and stateful for H265). This would allow supporting
> these two hardware blocks under the same video device (if that makes
> sense anyway). And even if there's no immediate need, it's always good
> to have this level of granularity (with little drawbacks).
> 
> > I propose adding these capabilities:
> > 
> > #define V4L2_BUF_CAP_HAS_REQUESTS	0x00000001
> > #define V4L2_BUF_CAP_REQUIRES_REQUESTS	0x00000002
> > #define V4L2_BUF_CAP_HAS_MMAP		0x00000100
> > #define V4L2_BUF_CAP_HAS_USERPTR	0x00000200
> > #define V4L2_BUF_CAP_HAS_DMABUF		0x00000400
> > 
> > If REQUIRES_REQUESTS is set, then HAS_REQUESTS is also set.
> > 
> > At this time I think that REQUIRES_REQUESTS would only need to be set for the
> > output queue of stateless codecs.
> > 
> > If capabilities is 0, then it's from an old kernel and all you know is that
> > requests are certainly not supported, and that MMAP is supported. Whether USERPTR
> > or DMABUF are supported isn't known in that case (just try it :-) ).
> 
> Sounds good to me!
> 
> > Strictly speaking we do not need these HAS_MMAP/USERPTR/DMABUF caps, but it is very
> > easy to add if we create a new capability field anyway, and it has always annoyed
> > the hell out of me that we didn't have a good way to let userspace know what
> > streaming I/O modes we support. And with vb2 it's easy to implement.
> 
> I totally agree here, it would be very nice to take the occasion to
> expose to userspace what I/O modes are available. The current try-and-
> see approach works, but this feels much better indeed.
> 
> > Regarding point 3: I think this should be documented next to the pixel format. I.e.
> > the MPEG-2 Slice format used by the stateless cedrus codec requires the request API
> > and that two MPEG-2 controls (slice params and quantization matrices) must be present
> > in each request.
> > 
> > I am not sure a control flag (e.g. V4L2_CTRL_FLAG_REQUIRED_IN_REQ) is needed here.
> > It's really implied by the fact that you use a stateless codec. It doesn't help
> > generic applications like v4l2-ctl or qv4l2 either since in order to support
> > stateless codecs they will have to know about the details of these controls anyway.
> > 
> > So I am inclined to say that it is not necessary to expose this information in
> > the API, but it has to be documented together with the pixel format documentation.
> 
> I think this is affected by considerations about codec profile/level
> support. More specifically, some controls will only be required for
> supporting advanced codec profiles/levels, so they can only be
> explicitly marked with appropriate flags by the driver when the target
> profile/level is known. And I don't think it would be sane for userspace
> to explicitly set what profile/level it's aiming at. As a result, I
> don't think we can explicitly mark controls as required or optional.
> 
> I also like the idea that it should instead be implicit and that the
> documentation should detail which specific stateless metadata controls
> are required for a given profile/level.
> 
> As for controls validation, the approach followed in the Cedrus driver
> is to check that the most basic controls are filled and allow having
> missing controls for those that match advanced profiles.
> 
> Since this approach feels somewhat generic enough to be applied to all
> stateless VPU drivers, maybe this should be made a helper in the
> framework?
> 
> In addition, I see a need for exposing the maximum profile/level that
> the driver supports for decoding. I would suggest reusing the already-
> existing dedicated controls used for encoding for this purpose. For
> decoders, they would be used to expose the (read-only) maximum
> profile/level that is supported by the hardware and keep using them as a
> settable value in a range (matching the level of support) for encoders.
> 
> This is necessary for userspace to determine whether a given video can
> be decoded in hardware or not. Instead of half-way decoding the video
> (ending up in funky results), this would easily allow skipping hardware
> decoding and e.g. falling back on software decoding.
> 
> What do you think?
> 
> Cheers,
> 
> Paul
> 

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [RFC] Request API and V4L2 capabilities

[Hans Verkuil]

On 06/08/18 10:16, Paul Kocialkowski wrote:
> Hi Hans and all,
> 
> On Sat, 2018-08-04 at 15:50 +0200, Hans Verkuil wrote:
>> Hi all,
>>
>> While the Request API patch series addresses all the core API issues, there
>> are some high-level considerations as well:
>>
>> 1) How can the application tell that the Request API is supported and for
>>    which buffer types (capture/output) and pixel formats?
>>
>> 2) How can the application tell if the Request API is required as opposed to being
>>    optional?
>>
>> 3) Some controls may be required in each request, how to let userspace know this?
>>    Is it even necessary to inform userspace?
>>
>> 4) (For bonus points): How to let the application know which streaming I/O modes
>>    are available? That's never been possible before, but it would be very nice
>>    indeed if that's made explicit.
> 
> Thanks for bringing up these considerations and questions, which perhaps
> cover the last missing bits for streamlined use of the request API by
> userspace. I would suggest another item, related to 3):
> 
> 5) How can applications tell whether the driver supports a specific
> codec profile/level, not only for encoding but also for decoding? It's
> common for low-end embedded hardware to not support the most advanced
> profiles (e.g. H264 high profile).

Just to point out that this is unrelated to the Request API. I see this as
a separate topic that is probably worth an RFC by itself.

Regards,

	Hans

Re: [RFC] Request API and V4L2 capabilities

[Hans Verkuil]

On 15/08/18 14:11, Mauro Carvalho Chehab wrote:
> Em Sat, 4 Aug 2018 15:50:04 +0200
> Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> 
>> Hi all,
>>
>> While the Request API patch series addresses all the core API issues, there
>> are some high-level considerations as well:
>>
>> 1) How can the application tell that the Request API is supported and for
>>    which buffer types (capture/output) and pixel formats?
>>
>> 2) How can the application tell if the Request API is required as opposed to being
>>    optional?
> 
> Huh? Why would it be mandatory?

It is mandatory for stateless codecs: you can't use them without the Request API since
each frame needs the state as well. If you could make a driver for a stateless codec
without the Request API we wouldn't have had to spend ages on developing it in the first
place, would we? :-)

> 
>>
>> 3) Some controls may be required in each request, how to let userspace know this?
>>    Is it even necessary to inform userspace?
> 
> Again, why would it need to have a set of mandatory controls for requests
> to work? If this is really required,  it should have a way to send such
> list to userspace.

Also for stateless codecs: each frame needs the state information. This is done
through one or more controls (see the cedrus driver implementation). Which controls
those are is standardized, but it will differ depending on the HW codec (MPEG, H264,
HEVC will almost certainly each require different state information).

Obviously this will have to be documented as part of the pixel format for each
HW codec.

> 
>>
>> 4) (For bonus points): How to let the application know which streaming I/O modes
>>    are available? That's never been possible before, but it would be very nice
>>    indeed if that's made explicit.
>>
>> Since the Request API associates data with frame buffers it makes sense to expose
>> this as a new capability field in struct v4l2_requestbuffers and struct v4l2_create_buffers.
>>
>> The first struct has 2 reserved fields, the second has 8, so it's not a problem to
>> take one for a capability field. Both structs also have a buffer type, so we know
>> if this is requested for a capture or output buffer type. The pixel format is known
>> in the driver, so HAS/REQUIRES_REQUESTS can be set based on that. I doubt we'll have
>> drivers where the request caps would actually depend on the pixel format, but it
>> theoretically possible. For both ioctls you can call them with count=0 at the start
>> of the application. REQBUFS has of course the side-effect of deleting all buffers,
>> but at the start of your application you don't have any yet. CREATE_BUFS has no
>> side-effects.
>>
>> I propose adding these capabilities:
>>
>> #define V4L2_BUF_CAP_HAS_REQUESTS	0x00000001
> 
> I'm OK with that.
> 
>> #define V4L2_BUF_CAP_REQUIRES_REQUESTS	0x00000002
> 
> But I'm not ok with breaking even more userspace support by forcing 
> requests.

You don't break userspace. This is for stateless codecs, which didn't
exist before precisely because they require the Request API.

> 
>> #define V4L2_BUF_CAP_HAS_MMAP		0x00000100
>> #define V4L2_BUF_CAP_HAS_USERPTR	0x00000200
>> #define V4L2_BUF_CAP_HAS_DMABUF		0x00000400
> 
> Those sounds ok to me too.
> 
>>
>> If REQUIRES_REQUESTS is set, then HAS_REQUESTS is also set.
>>
>> At this time I think that REQUIRES_REQUESTS would only need to be set for the
>> output queue of stateless codecs.
> 
> Same as before: I don't see the need of support a request-only driver.
> 
>>
>> If capabilities is 0, then it's from an old kernel and all you know is that
>> requests are certainly not supported, and that MMAP is supported. Whether USERPTR
>> or DMABUF are supported isn't known in that case (just try it :-) ).
>>
>> Strictly speaking we do not need these HAS_MMAP/USERPTR/DMABUF caps, but it is very
>> easy to add if we create a new capability field anyway, and it has always annoyed
>> the hell out of me that we didn't have a good way to let userspace know what
>> streaming I/O modes we support. And with vb2 it's easy to implement.
> 
> Yeah, that sounds a bonus to me too.
> 
>> Regarding point 3: I think this should be documented next to the pixel format. I.e.
>> the MPEG-2 Slice format used by the stateless cedrus codec requires the request API
>> and that two MPEG-2 controls (slice params and quantization matrices) must be present
>> in each request.
> 
> Makes sense to document with the pixel format...
> 
>> I am not sure a control flag (e.g. V4L2_CTRL_FLAG_REQUIRED_IN_REQ) is needed here.
> 
> but it sounds worth to also have a flag.

I'll wait to get some more feedback. I don't have a very strong opinion on
this.

> 
>> It's really implied by the fact that you use a stateless codec. It doesn't help
>> generic applications like v4l2-ctl or qv4l2 either since in order to support
>> stateless codecs they will have to know about the details of these controls anyway.
> 
> Yeah, but they could skip enum those ioctls if they see one marked with
> V4L2_CTRL_FLAG_REQUIRED_IN_REQ and don't know how to use. Then, default
> to not use request API. 

I would expect that V4L2_CTRL_FLAG_REQUIRED_IN_REQ only makes sense if
V4L2_BUF_CAP_REQUIRES_REQUESTS is also set. But then you already know through
the documentation which controls as required.

I can't think of a reason why V4L2_CTRL_FLAG_REQUIRED_IN_REQ would be set
but V4L2_BUF_CAP_REQUIRES_REQUESTS isn't.

Regards,

	Hans

> 
> Then, the driver would use a default that would work (even not providing
> the best possible compression).
> 
>> So I am inclined to say that it is not necessary to expose this information in
>> the API, but it has to be documented together with the pixel format documentation.
>>
>> Comments? Ideas?
>>
>> Regards,
>>
>> 	Hans
> 
> Thanks,
> Mauro
> 

Re: [RFC] Request API and V4L2 capabilities

[Nicolas Dufresne]

[-- Attachment #1: Type: text/plain, Size: 5260 bytes --]

Le samedi 04 août 2018 à 15:50 +0200, Hans Verkuil a écrit :
> Hi all,
> 
> While the Request API patch series addresses all the core API issues, there
> are some high-level considerations as well:
> 
> 1) How can the application tell that the Request API is supported and for
>    which buffer types (capture/output) and pixel formats?
> 
> 2) How can the application tell if the Request API is required as opposed to being
>    optional?
> 
> 3) Some controls may be required in each request, how to let userspace know this?
>    Is it even necessary to inform userspace?

For state-less codec, there is a very strict set of controls that must
be supported / filled. The data format pretty much dictate this.

For complex camera's and video transformation m2m devices, there is a
gap indeed. Duplicating the formats for this case does not seem like
the right approach.

> 
> 4) (For bonus points): How to let the application know which streaming I/O modes
>    are available? That's never been possible before, but it would be very nice
>    indeed if that's made explicit.

In GStreamer, we call REQBUFS(type, count=0) for each types we support.
This call should never fail, unless the type is not supported. We build
a list of supported I/O mode this way. It's also a no-op, because we
didn't allocate any buffers yet.

> 
> Since the Request API associates data with frame buffers it makes sense to expose
> this as a new capability field in struct v4l2_requestbuffers and struct v4l2_create_buffers.
> 
> The first struct has 2 reserved fields, the second has 8, so it's not a problem to
> take one for a capability field. Both structs also have a buffer type, so we know
> if this is requested for a capture or output buffer type. The pixel format is known
> in the driver, so HAS/REQUIRES_REQUESTS can be set based on that. I doubt we'll have
> drivers where the request caps would actually depend on the pixel format, but it
> theoretically possible. For both ioctls you can call them with count=0 at the start
> of the application. REQBUFS has of course the side-effect of deleting all buffers,
> but at the start of your application you don't have any yet. CREATE_BUFS has no
> side-effects.
> 
> I propose adding these capabilities:
> 
> #define V4L2_BUF_CAP_HAS_REQUESTS	0x00000001
> #define V4L2_BUF_CAP_REQUIRES_REQUESTS	0x00000002
> #define V4L2_BUF_CAP_HAS_MMAP		0x00000100
> #define V4L2_BUF_CAP_HAS_USERPTR	0x00000200
> #define V4L2_BUF_CAP_HAS_DMABUF		0x00000400

Looks similar to the bit map we create inside GStreamer using the
described technique. Though we also add HAS_CREATE_BUFS to the lot.

My main concern is in userspace like GStreamer, the difficulty is to
sort drivers that we support, from the ones that we don't. So if we
don't support requests yet, we would like to detect this early. As
CODEC don't really have an initial format, I believe that before S_FMT,
any kind of call to REQBUFS might fail at the moment. So detection
would be very late.

Thoughm be aware this is a totally artificial issue in the short term
since state-less CODEC uses dedicated formats.

> 
> If REQUIRES_REQUESTS is set, then HAS_REQUESTS is also set.
> 
> At this time I think that REQUIRES_REQUESTS would only need to be set for the
> output queue of stateless codecs.
> 
> If capabilities is 0, then it's from an old kernel and all you know is that
> requests are certainly not supported, and that MMAP is supported. Whether USERPTR
> or DMABUF are supported isn't known in that case (just try it :-) ).

Just a clarification, the doc is pretty clear the MMAP is supported if
the device capability have STREAMING in it.

> 
> Strictly speaking we do not need these HAS_MMAP/USERPTR/DMABUF caps, but it is very
> easy to add if we create a new capability field anyway, and it has always annoyed
> the hell out of me that we didn't have a good way to let userspace know what
> streaming I/O modes we support. And with vb2 it's easy to implement.
> 
> Regarding point 3: I think this should be documented next to the pixel format. I.e.
> the MPEG-2 Slice format used by the stateless cedrus codec requires the request API
> and that two MPEG-2 controls (slice params and quantization matrices) must be present
> in each request.
> 
> I am not sure a control flag (e.g. V4L2_CTRL_FLAG_REQUIRED_IN_REQ) is needed here.
> It's really implied by the fact that you use a stateless codec. It doesn't help
> generic applications like v4l2-ctl or qv4l2 either since in order to support
> stateless codecs they will have to know about the details of these controls anyway.

Right, I don't think this is needed in the short term, as we target
only stateless CODEC. But this is important use case for let's say
request to cameras. When we get there, we will need a mechnism to
list all the controls that can be included in a request, and also all
the only the must be present (if any).

> 
> So I am inclined to say that it is not necessary to expose this information in
> the API, but it has to be documented together with the pixel format documentation.

I'd prefer to say it's not urgent.

> 
> Comments? Ideas?
> 
> Regards,
> 
> 	Hans

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [RFC] Request API and V4L2 capabilities

[Nicolas Dufresne]

[-- Attachment #1: Type: text/plain, Size: 5596 bytes --]

Le mercredi 15 août 2018 à 09:11 -0300, Mauro Carvalho Chehab a écrit :
> Em Sat, 4 Aug 2018 15:50:04 +0200
> Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> 
> > Hi all,
> > 
> > While the Request API patch series addresses all the core API
> > issues, there
> > are some high-level considerations as well:
> > 
> > 1) How can the application tell that the Request API is supported
> > and for
> >    which buffer types (capture/output) and pixel formats?
> > 
> > 2) How can the application tell if the Request API is required as
> > opposed to being
> >    optional?
> 
> Huh? Why would it be mandatory?
> 
> > 
> > 3) Some controls may be required in each request, how to let
> > userspace know this?
> >    Is it even necessary to inform userspace?
> 
> Again, why would it need to have a set of mandatory controls for
> requests
> to work? If this is really required,  it should have a way to send
> such
> list to userspace.
> 
> > 
> > 4) (For bonus points): How to let the application know which
> > streaming I/O modes
> >    are available? That's never been possible before, but it would
> > be very nice
> >    indeed if that's made explicit.
> > 
> > Since the Request API associates data with frame buffers it makes
> > sense to expose
> > this as a new capability field in struct v4l2_requestbuffers and
> > struct v4l2_create_buffers.
> > 
> > The first struct has 2 reserved fields, the second has 8, so it's
> > not a problem to
> > take one for a capability field. Both structs also have a buffer
> > type, so we know
> > if this is requested for a capture or output buffer type. The pixel
> > format is known
> > in the driver, so HAS/REQUIRES_REQUESTS can be set based on that. I
> > doubt we'll have
> > drivers where the request caps would actually depend on the pixel
> > format, but it
> > theoretically possible. For both ioctls you can call them with
> > count=0 at the start
> > of the application. REQBUFS has of course the side-effect of
> > deleting all buffers,
> > but at the start of your application you don't have any yet.
> > CREATE_BUFS has no
> > side-effects.
> > 
> > I propose adding these capabilities:
> > 
> > #define V4L2_BUF_CAP_HAS_REQUESTS	0x00000001
> 
> I'm OK with that.
> 
> > #define V4L2_BUF_CAP_REQUIRES_REQUESTS	0x00000002
> 
> But I'm not ok with breaking even more userspace support by forcing 
> requests.

This is not breaking userspace, not in regard to state-less CODEC.
Stateless CODECs uses a set of new pixel formats specifically designed
for driving an accelerator rather then a full CODEC.

The controls are needed to provide a state to the accelerator, so the
accelerator knows what to do. Though, because of the nature of CODECs,
queuing multiple buffers is strictly needed. Without the request, there
is no way to figure-out which CID changes goes with which picture.

There is no way an existing userspace will break as there is no way it
can support these drivers as a) the formats aren't defined yet b) the
CIDs didn't exist. 

> 
> > #define V4L2_BUF_CAP_HAS_MMAP		0x00000100
> > #define V4L2_BUF_CAP_HAS_USERPTR	0x00000200
> > #define V4L2_BUF_CAP_HAS_DMABUF		0x00000400
> 
> Those sounds ok to me too.
> 
> > 
> > If REQUIRES_REQUESTS is set, then HAS_REQUESTS is also set.
> > 
> > At this time I think that REQUIRES_REQUESTS would only need to be
> > set for the
> > output queue of stateless codecs.
> 
> Same as before: I don't see the need of support a request-only
> driver.
> 
> > 
> > If capabilities is 0, then it's from an old kernel and all you know
> > is that
> > requests are certainly not supported, and that MMAP is supported.
> > Whether USERPTR
> > or DMABUF are supported isn't known in that case (just try it :-)
> > ).
> > 
> > Strictly speaking we do not need these HAS_MMAP/USERPTR/DMABUF
> > caps, but it is very
> > easy to add if we create a new capability field anyway, and it has
> > always annoyed
> > the hell out of me that we didn't have a good way to let userspace
> > know what
> > streaming I/O modes we support. And with vb2 it's easy to
> > implement.
> 
> Yeah, that sounds a bonus to me too.
> 
> > Regarding point 3: I think this should be documented next to the
> > pixel format. I.e.
> > the MPEG-2 Slice format used by the stateless cedrus codec requires
> > the request API
> > and that two MPEG-2 controls (slice params and quantization
> > matrices) must be present
> > in each request.
> 
> Makes sense to document with the pixel format...
> 
> > I am not sure a control flag (e.g. V4L2_CTRL_FLAG_REQUIRED_IN_REQ)
> > is needed here.
> 
> but it sounds worth to also have a flag.
> 
> > It's really implied by the fact that you use a stateless codec. It
> > doesn't help
> > generic applications like v4l2-ctl or qv4l2 either since in order
> > to support
> > stateless codecs they will have to know about the details of these
> > controls anyway.
> 
> Yeah, but they could skip enum those ioctls if they see one marked
> with
> V4L2_CTRL_FLAG_REQUIRED_IN_REQ and don't know how to use. Then,
> default
> to not use request API. 
> 
> Then, the driver would use a default that would work (even not
> providing
> the best possible compression).
> 
> > So I am inclined to say that it is not necessary to expose this
> > information in
> > the API, but it has to be documented together with the pixel format
> > documentation.
> > 
> > Comments? Ideas?
> > 
> > Regards,
> > 
> > 	Hans
> 
> Thanks,
> Mauro

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [RFC] Request API and V4L2 capabilities

[Tomasz Figa]

Hi Hans, Paul,

On Mon, Aug 6, 2018 at 6:29 PM Paul Kocialkowski
<paul.kocialkowski@bootlin.com> wrote:
>
> On Mon, 2018-08-06 at 11:23 +0200, Hans Verkuil wrote:
> > On 08/06/2018 11:13 AM, Paul Kocialkowski wrote:
> > > Hi,
> > >
> > > On Mon, 2018-08-06 at 10:32 +0200, Hans Verkuil wrote:
> > > > On 08/06/2018 10:16 AM, Paul Kocialkowski wrote:
> > > > > On Sat, 2018-08-04 at 15:50 +0200, Hans Verkuil wrote:
> > > > > > Regarding point 3: I think this should be documented next to the pixel format. I.e.
> > > > > > the MPEG-2 Slice format used by the stateless cedrus codec requires the request API
> > > > > > and that two MPEG-2 controls (slice params and quantization matrices) must be present
> > > > > > in each request.
> > > > > >
> > > > > > I am not sure a control flag (e.g. V4L2_CTRL_FLAG_REQUIRED_IN_REQ) is needed here.
> > > > > > It's really implied by the fact that you use a stateless codec. It doesn't help
> > > > > > generic applications like v4l2-ctl or qv4l2 either since in order to support
> > > > > > stateless codecs they will have to know about the details of these controls anyway.
> > > > > >
> > > > > > So I am inclined to say that it is not necessary to expose this information in
> > > > > > the API, but it has to be documented together with the pixel format documentation.
> > > > >
> > > > > I think this is affected by considerations about codec profile/level
> > > > > support. More specifically, some controls will only be required for
> > > > > supporting advanced codec profiles/levels, so they can only be
> > > > > explicitly marked with appropriate flags by the driver when the target
> > > > > profile/level is known. And I don't think it would be sane for userspace
> > > > > to explicitly set what profile/level it's aiming at. As a result, I
> > > > > don't think we can explicitly mark controls as required or optional.

I'm not sure this is entirely true. The hardware may need to be
explicitly told what profile the video is. It may even not be the
hardware, but the driver itself too, given that the profile may imply
the CAPTURE pixel format, e.g. for VP9 profiles:

profile 0
color depth: 8 bit/sample, chroma subsampling: 4:2:0
profile 1
color depth: 8 bit, chroma subsampling: 4:2:0, 4:2:2, 4:4:4
profile 2
color depth: 10–12 bit, chroma subsampling: 4:2:0
profile 3
color depth: 10–12 bit, chroma subsampling: 4:2:0, 4:2:2, 4:4:4

(reference: https://en.wikipedia.org/wiki/VP9#Profiles)

> > > > >
> > > > > I also like the idea that it should instead be implicit and that the
> > > > > documentation should detail which specific stateless metadata controls
> > > > > are required for a given profile/level.
> > > > >
> > > > > As for controls validation, the approach followed in the Cedrus driver
> > > > > is to check that the most basic controls are filled and allow having
> > > > > missing controls for those that match advanced profiles.
> > > > >
> > > > > Since this approach feels somewhat generic enough to be applied to all
> > > > > stateless VPU drivers, maybe this should be made a helper in the
> > > > > framework?
> > > >
> > > > Sounds reasonable. Not sure if it will be in the first version, but it is
> > > > easy to add later.
> > >
> > > Definitely, I don't think this is such a high priority for now either.
> > >

We may want to put strict requirements on what controls are provided
for given codec+profile/level. Otherwise we might get some user space
that doesn't provide some of them and works only by luck, e.g. because
some hardware defaults on initial drivers luckily match the needed
values. Even if we don't validate it in the code yet, we should put a
big warning saying that not providing the required controls would
result in undefined behavior.

> > > > > In addition, I see a need for exposing the maximum profile/level that
> > > > > the driver supports for decoding. I would suggest reusing the already-
> > > > > existing dedicated controls used for encoding for this purpose. For
> > > > > decoders, they would be used to expose the (read-only) maximum
> > > > > profile/level that is supported by the hardware and keep using them as a
> > > > > settable value in a range (matching the level of support) for encoders.
> > > > >
> > > > > This is necessary for userspace to determine whether a given video can
> > > > > be decoded in hardware or not. Instead of half-way decoding the video
> > > > > (ending up in funky results), this would easily allow skipping hardware
> > > > > decoding and e.g. falling back on software decoding.
> > > >
> > > > I think it might be better to expose this through new read-only bitmask
> > > > controls: i.e. a bitmask containing the supported profiles and levels.
> > >
> > > It seems that this is more or less what the coda driver is doing for
> > > decoding actually, although it uses a menu control between min/max
> > > supported profile/levels, with a mask to "blacklist" the unsupported
> > > values. Then, the V4L2_CTRL_FLAG_READ_ONLY flag is set to keep the
> > > control read-only.
> > >
> > > > Reusing the existing controls for a decoder is odd since there is not
> > > > really a concept of a 'current' value since you just want to report what
> > > > is supported. And I am not sure if all decoders can report the profile
> > > > or level that they detect.
> > >
> > > Is that really a problem when the READ_ONLY flag is set? I thought it
> > > was designed to fit this specific case, when the driver reports a value
> > > that userspace cannot affect.
> >
> > Well, for read-only menu controls the current value of the control would
> > have to indicate what the current profile/level is that is being decoded.
> >
> > That's not really relevant since what you want is just to query the
> > supported profiles/levels. A read-only bitmask control is the fastest
> > method (if only because using a menu control requires the application to
> > enumerate all possibilities with QUERYMENU).

Besides querying for supported profiles,
 - For stateless codecs we also need to set the profile, since the
codec itself does only the number crunching.
 - For stateful codecs, the decoder would also report the detected
profile after parsing the bitstream (although this is possibly not of
a big importance to the user space).

As for querying itself, there is still more to it than could be
handled with just a read only control. To detect what CAPTURE formats
are supported for given profile, one would have to set the profile
control first and then use ENUM_FMT.

>
> Ah yes, I finally understand the issue with what the current control
> value represents here. Since I don't think the driver should have to
> bother with figuring out the profile in use (as expressed earlier, I
> think it should be implicit, through the codec metadata controls and
> features used), I no longer believe it's best to have the same control
> for both encoding and decoding.
>
> > > Otherwise, I agree that having a bitmask type would be a better fit, but
> > > I think it would be beneficial to keep the already-defined control and
> > > associated values, which implies using the menu control type for both
> > > encoders and decoders.
> > >
> > > If this is not an option, I would be in favour of adding per-codec read-
> > > only bitmask controls (e.g. for H264 something like
> > > V4L2_CID_MPEG_VIDEO_H264_PROFILE_SUPPORT) that expose the already-
> > > existing profile/level definitions as bit identifiers (a bit like coda
> > > is using them to craft a mask for the menu items to blacklist) for
> > > decoding only.
> >
> > That's what I have in mind, yes. I'd like Tomasz' input as well, though.

Thanks for valuing my input! Hopefully the comments don't turn that
into overestimation. ;) Sorry for being terribly late to the party,
last 2 weeks have been extremely busy.

Best regards,
Tomasz

Re: [RFC] Request API and V4L2 capabilities

[Tomasz Figa]

On Wed, Aug 15, 2018 at 11:18 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>
> On 15/08/18 14:11, Mauro Carvalho Chehab wrote:
> > Em Sat, 4 Aug 2018 15:50:04 +0200
> > Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> >
> >> Hi all,
> >>
> >> While the Request API patch series addresses all the core API issues, there
> >> are some high-level considerations as well:
> >>
> >> 1) How can the application tell that the Request API is supported and for
> >>    which buffer types (capture/output) and pixel formats?
> >>
> >> 2) How can the application tell if the Request API is required as opposed to being
> >>    optional?
> >
> > Huh? Why would it be mandatory?
>
> It is mandatory for stateless codecs: you can't use them without the Request API since
> each frame needs the state as well. If you could make a driver for a stateless codec
> without the Request API we wouldn't have had to spend ages on developing it in the first
> place, would we? :-)

Technically, one could still do the synchronous S_EXT_CTRL, QBUF,
DQBUF, S_EXT_CTRL... loop, but I'm not sure if this is something worth
considering.

[snip]
> >> Regarding point 3: I think this should be documented next to the pixel format. I.e.
> >> the MPEG-2 Slice format used by the stateless cedrus codec requires the request API
> >> and that two MPEG-2 controls (slice params and quantization matrices) must be present
> >> in each request.
> >
> > Makes sense to document with the pixel format...
> >
> >> I am not sure a control flag (e.g. V4L2_CTRL_FLAG_REQUIRED_IN_REQ) is needed here.
> >
> > but it sounds worth to also have a flag.
>
> I'll wait to get some more feedback. I don't have a very strong opinion on
> this.
>

I don't see any value in a flag for codecs. Querying the controls for
the flag, if it's already required as a part of the Stateless Codec
Interface for given pixel format, would only mean some redundant code
both in kernel and user space.

For other use cases, I'm not sure if we can say that a control is
really required in a request. I think it should be something for the
user space to decide, depending on the synchronization (and other)
needs of given use case.

Best regards,
Tomasz

Re: [RFC] Request API and V4L2 capabilities

[Tomasz Figa]

On Mon, Aug 6, 2018 at 5:32 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>
> On 08/06/2018 10:16 AM, Paul Kocialkowski wrote:
> > Hi Hans and all,
> >
> > On Sat, 2018-08-04 at 15:50 +0200, Hans Verkuil wrote:
> >> Hi all,
> >>
> >> While the Request API patch series addresses all the core API issues, there
> >> are some high-level considerations as well:
> >>
> >> 1) How can the application tell that the Request API is supported and for
> >>    which buffer types (capture/output) and pixel formats?
> >>
> >> 2) How can the application tell if the Request API is required as opposed to being
> >>    optional?
> >>
> >> 3) Some controls may be required in each request, how to let userspace know this?
> >>    Is it even necessary to inform userspace?
> >>
> >> 4) (For bonus points): How to let the application know which streaming I/O modes
> >>    are available? That's never been possible before, but it would be very nice
> >>    indeed if that's made explicit.
> >
> > Thanks for bringing up these considerations and questions, which perhaps
> > cover the last missing bits for streamlined use of the request API by
> > userspace. I would suggest another item, related to 3):
> >
> > 5) How can applications tell whether the driver supports a specific
> > codec profile/level, not only for encoding but also for decoding? It's
> > common for low-end embedded hardware to not support the most advanced
> > profiles (e.g. H264 high profile).
> >
> >> Since the Request API associates data with frame buffers it makes sense to expose
> >> this as a new capability field in struct v4l2_requestbuffers and struct v4l2_create_buffers.
> >>
> >> The first struct has 2 reserved fields, the second has 8, so it's not a problem to
> >> take one for a capability field. Both structs also have a buffer type, so we know
> >> if this is requested for a capture or output buffer type. The pixel format is known
> >> in the driver, so HAS/REQUIRES_REQUESTS can be set based on that. I doubt we'll have
> >> drivers where the request caps would actually depend on the pixel format, but it
> >> theoretically possible. For both ioctls you can call them with count=0 at the start
> >> of the application. REQBUFS has of course the side-effect of deleting all buffers,
> >> but at the start of your application you don't have any yet. CREATE_BUFS has no
> >> side-effects.
> >
> > My initial thoughts on this point were to have flags exposed in
> > v4l2_capability, but now that you're saying it, it does make sense for
> > the flag to be associated with a buffer rather than the global device.
> >
> > In addition, I've heard of cases (IIRC it was some Rockchip platforms)
> > where the platform has both stateless and stateful VPUs (I think it was
> > stateless up to H264 and stateful for H265).

AFAIR, on Rockchip those are two separate hardware blocks.

> > This would allow supporting
> > these two hardware blocks under the same video device (if that makes
> > sense anyway). And even if there's no immediate need, it's always good
> > to have this level of granularity (with little drawbacks).
> >
> >> I propose adding these capabilities:
> >>
> >> #define V4L2_BUF_CAP_HAS_REQUESTS    0x00000001
> >> #define V4L2_BUF_CAP_REQUIRES_REQUESTS       0x00000002
> >> #define V4L2_BUF_CAP_HAS_MMAP                0x00000100
> >> #define V4L2_BUF_CAP_HAS_USERPTR     0x00000200
> >> #define V4L2_BUF_CAP_HAS_DMABUF              0x00000400
> >>
> >> If REQUIRES_REQUESTS is set, then HAS_REQUESTS is also set.
> >>
> >> At this time I think that REQUIRES_REQUESTS would only need to be set for the
> >> output queue of stateless codecs.
> >>
> >> If capabilities is 0, then it's from an old kernel and all you know is that
> >> requests are certainly not supported, and that MMAP is supported. Whether USERPTR
> >> or DMABUF are supported isn't known in that case (just try it :-) ).
> >
> > Sounds good to me!

SGTM.

+/- the fact that I'm not convinced if something really "requires
requests" at V4L2 interface level. Would that mean that we strictly
prohibit the lame synchronous way for stateless codecs? (Possibly
useful for debugging.)

Best regards,
Tomasz

Re: [RFC] Request API and V4L2 capabilities

[Alexandre Courbot]

On Tue, Aug 21, 2018 at 6:15 PM Tomasz Figa <tfiga@chromium.org> wrote:
>
> On Wed, Aug 15, 2018 at 11:18 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
> >
> > On 15/08/18 14:11, Mauro Carvalho Chehab wrote:
> > > Em Sat, 4 Aug 2018 15:50:04 +0200
> > > Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> > >
> > >> Hi all,
> > >>
> > >> While the Request API patch series addresses all the core API issues, there
> > >> are some high-level considerations as well:
> > >>
> > >> 1) How can the application tell that the Request API is supported and for
> > >>    which buffer types (capture/output) and pixel formats?
> > >>
> > >> 2) How can the application tell if the Request API is required as opposed to being
> > >>    optional?
> > >
> > > Huh? Why would it be mandatory?
> >
> > It is mandatory for stateless codecs: you can't use them without the Request API since
> > each frame needs the state as well. If you could make a driver for a stateless codec
> > without the Request API we wouldn't have had to spend ages on developing it in the first
> > place, would we? :-)
>
> Technically, one could still do the synchronous S_EXT_CTRL, QBUF,
> DQBUF, S_EXT_CTRL... loop, but I'm not sure if this is something worth
> considering.

Having this option is useful for driver bringup at the very least. And
why add artificial constraints if the two APIs are able to work
independently, albeit in a sub-optimal way?

>
> [snip]
> > >> Regarding point 3: I think this should be documented next to the pixel format. I.e.
> > >> the MPEG-2 Slice format used by the stateless cedrus codec requires the request API
> > >> and that two MPEG-2 controls (slice params and quantization matrices) must be present
> > >> in each request.
> > >
> > > Makes sense to document with the pixel format...
> > >
> > >> I am not sure a control flag (e.g. V4L2_CTRL_FLAG_REQUIRED_IN_REQ) is needed here.
> > >
> > > but it sounds worth to also have a flag.
> >
> > I'll wait to get some more feedback. I don't have a very strong opinion on
> > this.
> >
>
> I don't see any value in a flag for codecs. Querying the controls for
> the flag, if it's already required as a part of the Stateless Codec
> Interface for given pixel format, would only mean some redundant code
> both in kernel and user space.
>
> For other use cases, I'm not sure if we can say that a control is
> really required in a request. I think it should be something for the
> user space to decide, depending on the synchronization (and other)
> needs of given use case.
>
> Best regards,
> Tomasz

Re: [RFC] Request API and V4L2 capabilities

[Paul Kocialkowski]

[-- Attachment #1: Type: text/plain, Size: 1580 bytes --]

Hi,

[...]

On Wed, 2018-08-15 at 14:51 +0200, Maxime Jourdan wrote:
> Hi Paul, I think we need to go deeper than just exposing the supported
> profiles/levels and also include a way to query the CAPTURE pixel
> formats that are supported for each profile.
> 
> Maybe HEVC Main produces yuv420p but HEVC Main10 gives you
> yuv420p10le. Maybe H.264 HiP produces NV12 but H.264 Hi422P produces
> YUYV while also supporting down-sampling to NV12.

Well, I think we're looking at this backwards. Userspace certainly known
what destination format is relevant for the video, so it shouldn't have
to query the driver about it except to check that the format is indeed
supported.

> I don't know the specifics of each platform and the only example I can
> think of is the Amlogic HEVC decoder that can produce NV12 for Main,
> but only outputs a compressed proprietary format for Main10.
> 
> I unfortunately don't have an idea about how to implement that, but
> I'll think about it.

On the first generations of Allwinner platforms, we also have a vendor-
specific format as output, that we expose with a dedicated format.
There's no particular interfacing issue with that. Only that userspace
has to be aware of the format and how to deal with it.

Cheers,

Paul

> > Cheers,
> > 
> > Paul
> > 
> > --
> > Paul Kocialkowski, Bootlin (formerly Free Electrons)
> > Embedded Linux and kernel engineering
> > https://bootlin.com
-- 
Paul Kocialkowski, Bootlin (formerly Free Electrons)
Embedded Linux and kernel engineering
https://bootlin.com

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC] Request API and V4L2 capabilities

[Tomasz Figa]

On Wed, Aug 22, 2018 at 10:21 PM Paul Kocialkowski
<paul.kocialkowski@bootlin.com> wrote:
>
> Hi,
>
> [...]
>
> On Wed, 2018-08-15 at 14:51 +0200, Maxime Jourdan wrote:
> > Hi Paul, I think we need to go deeper than just exposing the supported
> > profiles/levels and also include a way to query the CAPTURE pixel
> > formats that are supported for each profile.
> >
> > Maybe HEVC Main produces yuv420p but HEVC Main10 gives you
> > yuv420p10le. Maybe H.264 HiP produces NV12 but H.264 Hi422P produces
> > YUYV while also supporting down-sampling to NV12.
>
> Well, I think we're looking at this backwards. Userspace certainly known
> what destination format is relevant for the video, so it shouldn't have
> to query the driver about it except to check that the format is indeed
> supported.

Typically the profile itself only defines the sub-sampling and sample
size, but not the exact set of formats supported by the hardware. VP9
profile 0 is expected to decode into YUV 4:2:0 8-bit, but whether that
would be NV12, YUV420, NV21 or maybe some hw-specific tiled format
(like NV12MT), is entirely up to the hardware/driver. Userspace will
definitely need to know if the decoder can decode the video into a
format, which it can later use (display).

>
> > I don't know the specifics of each platform and the only example I can
> > think of is the Amlogic HEVC decoder that can produce NV12 for Main,
> > but only outputs a compressed proprietary format for Main10.
> >
> > I unfortunately don't have an idea about how to implement that, but
> > I'll think about it.
>
> On the first generations of Allwinner platforms, we also have a vendor-
> specific format as output, that we expose with a dedicated format.
> There's no particular interfacing issue with that. Only that userspace
> has to be aware of the format and how to deal with it.
>

Typically a decode operates as a part of a pipeline with other
components. If your display doesn't let you display format X on an
overlay plane, you may want to use a post-processor hardware to
convert it to format Y. Or maybe use GPU to blit the video into the
primary plane? Or maybe you need to do both, because format X is a
decoder-specific tiled format and the GPU can't deal with it and all
the overlay planes are occupied with some other surfaces? Or maybe
it's just cheaper to do software decode rather than the conversion+GPU
blit? This is something that normally needs to be queried before the
video playback is initialized.

Best regards,
Tomasz

Re: [RFC] Request API and V4L2 capabilities

[Paul Kocialkowski]

[-- Attachment #1: Type: text/plain, Size: 10964 bytes --]

Hi,

On Tue, 2018-08-21 at 17:52 +0900, Tomasz Figa wrote:
> Hi Hans, Paul,
> 
> On Mon, Aug 6, 2018 at 6:29 PM Paul Kocialkowski
> <paul.kocialkowski@bootlin.com> wrote:
> > 
> > On Mon, 2018-08-06 at 11:23 +0200, Hans Verkuil wrote:
> > > On 08/06/2018 11:13 AM, Paul Kocialkowski wrote:
> > > > Hi,
> > > > 
> > > > On Mon, 2018-08-06 at 10:32 +0200, Hans Verkuil wrote:
> > > > > On 08/06/2018 10:16 AM, Paul Kocialkowski wrote:
> > > > > > On Sat, 2018-08-04 at 15:50 +0200, Hans Verkuil wrote:
> > > > > > > Regarding point 3: I think this should be documented next to the pixel format. I.e.
> > > > > > > the MPEG-2 Slice format used by the stateless cedrus codec requires the request API
> > > > > > > and that two MPEG-2 controls (slice params and quantization matrices) must be present
> > > > > > > in each request.
> > > > > > > 
> > > > > > > I am not sure a control flag (e.g. V4L2_CTRL_FLAG_REQUIRED_IN_REQ) is needed here.
> > > > > > > It's really implied by the fact that you use a stateless codec. It doesn't help
> > > > > > > generic applications like v4l2-ctl or qv4l2 either since in order to support
> > > > > > > stateless codecs they will have to know about the details of these controls anyway.
> > > > > > > 
> > > > > > > So I am inclined to say that it is not necessary to expose this information in
> > > > > > > the API, but it has to be documented together with the pixel format documentation.
> > > > > > 
> > > > > > I think this is affected by considerations about codec profile/level
> > > > > > support. More specifically, some controls will only be required for
> > > > > > supporting advanced codec profiles/levels, so they can only be
> > > > > > explicitly marked with appropriate flags by the driver when the target
> > > > > > profile/level is known. And I don't think it would be sane for userspace
> > > > > > to explicitly set what profile/level it's aiming at. As a result, I
> > > > > > don't think we can explicitly mark controls as required or optional.
> 
> I'm not sure this is entirely true. The hardware may need to be
> explicitly told what profile the video is. It may even not be the
> hardware, but the driver itself too, given that the profile may imply
> the CAPTURE pixel format, e.g. for VP9 profiles:
> 
> profile 0
> color depth: 8 bit/sample, chroma subsampling: 4:2:0
> profile 1
> color depth: 8 bit, chroma subsampling: 4:2:0, 4:2:2, 4:4:4
> profile 2
> color depth: 10–12 bit, chroma subsampling: 4:2:0
> profile 3
> color depth: 10–12 bit, chroma subsampling: 4:2:0, 4:2:2, 4:4:4
> 
> (reference: https://en.wikipedia.org/wiki/VP9#Profiles)

I think it would be fair to expect userspace to select the right
destination format (and maybe have the driver error if there's a
mismatch with the meta-data) instead of having the driver somewhat
expose what format should be used.

But maybe this would be an API violation, since all the enumerated
formats are probably supposed to be selectable?

We could also look at it the other way round and consider that selecting
an exposed format is always legit, but that it implies passing a
bitstream that matches it or the driver will error (because of an
invalid bitstream passed, not because of a "wrong" selected format).

As far as I understood, the profile/level information is there to
indicate a set of supported features by the decoder, not as an
information used for the decoding process. Each corresponding feature is
enabled or not in the bitstream meta-data and that's all the information
the decoder really needs.

This is why I think that setting the profile/level explicitly is not
justified by the nature of the process and adding it only for
convenience or marking whether controls are optional doesn't seem
justified at this point, in my opinion.

> > > > > > I also like the idea that it should instead be implicit and that the
> > > > > > documentation should detail which specific stateless metadata controls
> > > > > > are required for a given profile/level.
> > > > > > 
> > > > > > As for controls validation, the approach followed in the Cedrus driver
> > > > > > is to check that the most basic controls are filled and allow having
> > > > > > missing controls for those that match advanced profiles.
> > > > > > 
> > > > > > Since this approach feels somewhat generic enough to be applied to all
> > > > > > stateless VPU drivers, maybe this should be made a helper in the
> > > > > > framework?
> > > > > 
> > > > > Sounds reasonable. Not sure if it will be in the first version, but it is
> > > > > easy to add later.
> > > > 
> > > > Definitely, I don't think this is such a high priority for now either.
> > > > 
> 
> We may want to put strict requirements on what controls are provided
> for given codec+profile/level. Otherwise we might get some user space
> that doesn't provide some of them and works only by luck, e.g. because
> some hardware defaults on initial drivers luckily match the needed
> values. Even if we don't validate it in the code yet, we should put a
> big warning saying that not providing the required controls would
> result in undefined behavior.

I don't think having such strict requirements are a good thing. Even
with the level/profile made explicit, what if the video under-uses its
features and thus legitimately doesn't need to have all the controls
that could be supported with the level/profile? This can probably also
be frame-specific, so some frames could require more controls than
others.

This also leads me to believe that the profile/level indication should
be used as a support indication to userspace, not as a way to expose the
required features for decoding to the kernel

We could still enforce checks for the most basic controls (that are used
for all types of slices to decode) and error if they are missing. We
could also check the bits that indicate more advanced features in these
basic controls and decide what other controls are required from that.

> > > > > > In addition, I see a need for exposing the maximum profile/level that
> > > > > > the driver supports for decoding. I would suggest reusing the already-
> > > > > > existing dedicated controls used for encoding for this purpose. For
> > > > > > decoders, they would be used to expose the (read-only) maximum
> > > > > > profile/level that is supported by the hardware and keep using them as a
> > > > > > settable value in a range (matching the level of support) for encoders.
> > > > > > 
> > > > > > This is necessary for userspace to determine whether a given video can
> > > > > > be decoded in hardware or not. Instead of half-way decoding the video
> > > > > > (ending up in funky results), this would easily allow skipping hardware
> > > > > > decoding and e.g. falling back on software decoding.
> > > > > 
> > > > > I think it might be better to expose this through new read-only bitmask
> > > > > controls: i.e. a bitmask containing the supported profiles and levels.
> > > > 
> > > > It seems that this is more or less what the coda driver is doing for
> > > > decoding actually, although it uses a menu control between min/max
> > > > supported profile/levels, with a mask to "blacklist" the unsupported
> > > > values. Then, the V4L2_CTRL_FLAG_READ_ONLY flag is set to keep the
> > > > control read-only.
> > > > 
> > > > > Reusing the existing controls for a decoder is odd since there is not
> > > > > really a concept of a 'current' value since you just want to report what
> > > > > is supported. And I am not sure if all decoders can report the profile
> > > > > or level that they detect.
> > > > 
> > > > Is that really a problem when the READ_ONLY flag is set? I thought it
> > > > was designed to fit this specific case, when the driver reports a value
> > > > that userspace cannot affect.
> > > 
> > > Well, for read-only menu controls the current value of the control would
> > > have to indicate what the current profile/level is that is being decoded.
> > > 
> > > That's not really relevant since what you want is just to query the
> > > supported profiles/levels. A read-only bitmask control is the fastest
> > > method (if only because using a menu control requires the application to
> > > enumerate all possibilities with QUERYMENU).
> 
> Besides querying for supported profiles,
>  - For stateless codecs we also need to set the profile, since the
> codec itself does only the number crunching.

I disagree here, see above.

>  - For stateful codecs, the decoder would also report the detected
> profile after parsing the bitstream (although this is possibly not of
> a big importance to the user space).

I don't follow the logic behind this. This means informing userspace of
the capabilities required to decode the video that the VPU is currently
decoding... so that it can decided whether to decode it?

> As for querying itself, there is still more to it than could be
> handled with just a read only control. To detect what CAPTURE formats
> are supported for given profile, one would have to set the profile
> control first and then use ENUM_FMT.

Or consider that the bitstream is invalid if it doesn't match the
selected format and let userspace pick the appropriate format.

> > Ah yes, I finally understand the issue with what the current control
> > value represents here. Since I don't think the driver should have to
> > bother with figuring out the profile in use (as expressed earlier, I
> > think it should be implicit, through the codec metadata controls and
> > features used), I no longer believe it's best to have the same control
> > for both encoding and decoding.
> > 
> > > > Otherwise, I agree that having a bitmask type would be a better fit, but
> > > > I think it would be beneficial to keep the already-defined control and
> > > > associated values, which implies using the menu control type for both
> > > > encoders and decoders.
> > > > 
> > > > If this is not an option, I would be in favour of adding per-codec read-
> > > > only bitmask controls (e.g. for H264 something like
> > > > V4L2_CID_MPEG_VIDEO_H264_PROFILE_SUPPORT) that expose the already-
> > > > existing profile/level definitions as bit identifiers (a bit like coda
> > > > is using them to craft a mask for the menu items to blacklist) for
> > > > decoding only.
> > > 
> > > That's what I have in mind, yes. I'd like Tomasz' input as well, though.
> 
> Thanks for valuing my input! Hopefully the comments don't turn that
> into overestimation. ;) Sorry for being terribly late to the party,
> last 2 weeks have been extremely busy.

Taking the occasion to note that I have also been slow to respond here,
with ongoing work on H265 support.

-- 
Paul Kocialkowski, Bootlin (formerly Free Electrons)
Embedded Linux and kernel engineering
https://bootlin.com

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC] Request API and V4L2 capabilities

[Paul Kocialkowski]

[-- Attachment #1: Type: text/plain, Size: 8055 bytes --]

Hi,

On Wed, 2018-08-15 at 09:57 -0400, Nicolas Dufresne wrote:
> Le lundi 06 août 2018 à 10:16 +0200, Paul Kocialkowski a écrit :
> > Hi Hans and all,
> > 
> > On Sat, 2018-08-04 at 15:50 +0200, Hans Verkuil wrote:
> > > Hi all,
> > > 
> > > While the Request API patch series addresses all the core API issues, there
> > > are some high-level considerations as well:
> > > 
> > > 1) How can the application tell that the Request API is supported and for
> > >    which buffer types (capture/output) and pixel formats?
> > > 
> > > 2) How can the application tell if the Request API is required as opposed to being
> > >    optional?
> > > 
> > > 3) Some controls may be required in each request, how to let userspace know this?
> > >    Is it even necessary to inform userspace?
> > > 
> > > 4) (For bonus points): How to let the application know which streaming I/O modes
> > >    are available? That's never been possible before, but it would be very nice
> > >    indeed if that's made explicit.
> > 
> > Thanks for bringing up these considerations and questions, which perhaps
> > cover the last missing bits for streamlined use of the request API by
> > userspace. I would suggest another item, related to 3):
> > 
> > 5) How can applications tell whether the driver supports a specific
> > codec profile/level, not only for encoding but also for decoding? It's
> > common for low-end embedded hardware to not support the most advanced
> > profiles (e.g. H264 high profile).
> 
> Hi Paul, after some discussion with Philip, he sent a proposal patch
> that enables profile/level extended CID support to decoders too. The
> control is made read-only, the point is not really the CID get/set but
> that the controls allow enumerating the supported values. This seems
> quite straightforward and easy to use.
> 
> This enumeration is already provided this way some of the existing
> sate-full encoders. 

Sounds great, thanks for looking into it! I looked for the patch in the
list, but couldn't find it off-hand. Do you have a link to it? I would
like to bind it to the Cedrus VPU driver eventually.

Cheers,

Paul

> > > Since the Request API associates data with frame buffers it makes sense to expose
> > > this as a new capability field in struct v4l2_requestbuffers and struct v4l2_create_buffers.
> > > 
> > > The first struct has 2 reserved fields, the second has 8, so it's not a problem to
> > > take one for a capability field. Both structs also have a buffer type, so we know
> > > if this is requested for a capture or output buffer type. The pixel format is known
> > > in the driver, so HAS/REQUIRES_REQUESTS can be set based on that. I doubt we'll have
> > > drivers where the request caps would actually depend on the pixel format, but it
> > > theoretically possible. For both ioctls you can call them with count=0 at the start
> > > of the application. REQBUFS has of course the side-effect of deleting all buffers,
> > > but at the start of your application you don't have any yet. CREATE_BUFS has no
> > > side-effects.
> > 
> > My initial thoughts on this point were to have flags exposed in
> > v4l2_capability, but now that you're saying it, it does make sense for
> > the flag to be associated with a buffer rather than the global device.
> > 
> > In addition, I've heard of cases (IIRC it was some Rockchip platforms)
> > where the platform has both stateless and stateful VPUs (I think it was
> > stateless up to H264 and stateful for H265). This would allow supporting
> > these two hardware blocks under the same video device (if that makes
> > sense anyway). And even if there's no immediate need, it's always good
> > to have this level of granularity (with little drawbacks).
> > 
> > > I propose adding these capabilities:
> > > 
> > > #define V4L2_BUF_CAP_HAS_REQUESTS	0x00000001
> > > #define V4L2_BUF_CAP_REQUIRES_REQUESTS	0x00000002
> > > #define V4L2_BUF_CAP_HAS_MMAP		0x00000100
> > > #define V4L2_BUF_CAP_HAS_USERPTR	0x00000200
> > > #define V4L2_BUF_CAP_HAS_DMABUF		0x00000400
> > > 
> > > If REQUIRES_REQUESTS is set, then HAS_REQUESTS is also set.
> > > 
> > > At this time I think that REQUIRES_REQUESTS would only need to be set for the
> > > output queue of stateless codecs.
> > > 
> > > If capabilities is 0, then it's from an old kernel and all you know is that
> > > requests are certainly not supported, and that MMAP is supported. Whether USERPTR
> > > or DMABUF are supported isn't known in that case (just try it :-) ).
> > 
> > Sounds good to me!
> > 
> > > Strictly speaking we do not need these HAS_MMAP/USERPTR/DMABUF caps, but it is very
> > > easy to add if we create a new capability field anyway, and it has always annoyed
> > > the hell out of me that we didn't have a good way to let userspace know what
> > > streaming I/O modes we support. And with vb2 it's easy to implement.
> > 
> > I totally agree here, it would be very nice to take the occasion to
> > expose to userspace what I/O modes are available. The current try-and-
> > see approach works, but this feels much better indeed.
> > 
> > > Regarding point 3: I think this should be documented next to the pixel format. I.e.
> > > the MPEG-2 Slice format used by the stateless cedrus codec requires the request API
> > > and that two MPEG-2 controls (slice params and quantization matrices) must be present
> > > in each request.
> > > 
> > > I am not sure a control flag (e.g. V4L2_CTRL_FLAG_REQUIRED_IN_REQ) is needed here.
> > > It's really implied by the fact that you use a stateless codec. It doesn't help
> > > generic applications like v4l2-ctl or qv4l2 either since in order to support
> > > stateless codecs they will have to know about the details of these controls anyway.
> > > 
> > > So I am inclined to say that it is not necessary to expose this information in
> > > the API, but it has to be documented together with the pixel format documentation.
> > 
> > I think this is affected by considerations about codec profile/level
> > support. More specifically, some controls will only be required for
> > supporting advanced codec profiles/levels, so they can only be
> > explicitly marked with appropriate flags by the driver when the target
> > profile/level is known. And I don't think it would be sane for userspace
> > to explicitly set what profile/level it's aiming at. As a result, I
> > don't think we can explicitly mark controls as required or optional.
> > 
> > I also like the idea that it should instead be implicit and that the
> > documentation should detail which specific stateless metadata controls
> > are required for a given profile/level.
> > 
> > As for controls validation, the approach followed in the Cedrus driver
> > is to check that the most basic controls are filled and allow having
> > missing controls for those that match advanced profiles.
> > 
> > Since this approach feels somewhat generic enough to be applied to all
> > stateless VPU drivers, maybe this should be made a helper in the
> > framework?
> > 
> > In addition, I see a need for exposing the maximum profile/level that
> > the driver supports for decoding. I would suggest reusing the already-
> > existing dedicated controls used for encoding for this purpose. For
> > decoders, they would be used to expose the (read-only) maximum
> > profile/level that is supported by the hardware and keep using them as a
> > settable value in a range (matching the level of support) for encoders.
> > 
> > This is necessary for userspace to determine whether a given video can
> > be decoded in hardware or not. Instead of half-way decoding the video
> > (ending up in funky results), this would easily allow skipping hardware
> > decoding and e.g. falling back on software decoding.
> > 
> > What do you think?
> > 
> > Cheers,
> > 
> > Paul
> > 
-- 
Paul Kocialkowski, Bootlin (formerly Free Electrons)
Embedded Linux and kernel engineering
https://bootlin.com

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC] Request API and V4L2 capabilities

[Ezequiel Garcia]

On Wed, 2018-08-22 at 16:10 +0200, Paul Kocialkowski wrote:
> Hi,
> 
> On Tue, 2018-08-21 at 17:52 +0900, Tomasz Figa wrote:
> > Hi Hans, Paul,
> > 
> > On Mon, Aug 6, 2018 at 6:29 PM Paul Kocialkowski
> > <paul.kocialkowski@bootlin.com> wrote:
> > > 
> > > On Mon, 2018-08-06 at 11:23 +0200, Hans Verkuil wrote:
> > > > On 08/06/2018 11:13 AM, Paul Kocialkowski wrote:
> > > > > Hi,
> > > > > 
> > > > > On Mon, 2018-08-06 at 10:32 +0200, Hans Verkuil wrote:
> > > > > > On 08/06/2018 10:16 AM, Paul Kocialkowski wrote:
> > > > > > > On Sat, 2018-08-04 at 15:50 +0200, Hans Verkuil wrote:
> > > > > > > > Regarding point 3: I think this should be documented next to the pixel format. I.e.
> > > > > > > > the MPEG-2 Slice format used by the stateless cedrus codec requires the request API
> > > > > > > > and that two MPEG-2 controls (slice params and quantization matrices) must be present
> > > > > > > > in each request.
> > > > > > > > 
> > > > > > > > I am not sure a control flag (e.g. V4L2_CTRL_FLAG_REQUIRED_IN_REQ) is needed here.
> > > > > > > > It's really implied by the fact that you use a stateless codec. It doesn't help
> > > > > > > > generic applications like v4l2-ctl or qv4l2 either since in order to support
> > > > > > > > stateless codecs they will have to know about the details of these controls anyway.
> > > > > > > > 
> > > > > > > > So I am inclined to say that it is not necessary to expose this information in
> > > > > > > > the API, but it has to be documented together with the pixel format documentation.
> > > > > > > 
> > > > > > > I think this is affected by considerations about codec profile/level
> > > > > > > support. More specifically, some controls will only be required for
> > > > > > > supporting advanced codec profiles/levels, so they can only be
> > > > > > > explicitly marked with appropriate flags by the driver when the target
> > > > > > > profile/level is known. And I don't think it would be sane for userspace
> > > > > > > to explicitly set what profile/level it's aiming at. As a result, I
> > > > > > > don't think we can explicitly mark controls as required or optional.
> > 
> > I'm not sure this is entirely true. The hardware may need to be
> > explicitly told what profile the video is. It may even not be the
> > hardware, but the driver itself too, given that the profile may imply
> > the CAPTURE pixel format, e.g. for VP9 profiles:
> > 
> > profile 0
> > color depth: 8 bit/sample, chroma subsampling: 4:2:0
> > profile 1
> > color depth: 8 bit, chroma subsampling: 4:2:0, 4:2:2, 4:4:4
> > profile 2
> > color depth: 10–12 bit, chroma subsampling: 4:2:0
> > profile 3
> > color depth: 10–12 bit, chroma subsampling: 4:2:0, 4:2:2, 4:4:4
> > 
> > (reference: https://en.wikipedia.org/wiki/VP9#Profiles)
> 
> I think it would be fair to expect userspace to select the right
> destination format (and maybe have the driver error if there's a
> mismatch with the meta-data) instead of having the driver somewhat
> expose what format should be used.
> 
> But maybe this would be an API violation, since all the enumerated
> formats are probably supposed to be selectable?
> 
> We could also look at it the other way round and consider that selecting
> an exposed format is always legit, but that it implies passing a
> bitstream that matches it or the driver will error (because of an
> invalid bitstream passed, not because of a "wrong" selected format).
> 

The API requires the user to negotiate via TRY_FMT/S_FMT. The driver
usually does not return error on invalid formats, and simply return
a format it can work with. I think the kernel-user contract has to
guarantee if the driver accepted a given format, it won't fail to
encoder or decode.

I think that's why it makes sense for stateless drivers to set the
profile/levels for a given job, in order to properly negotiate
the input and output formats (for both encoders and decoders).  

[1] https://linuxtv.org/downloads/v4l-dvb-apis/uapi/v4l/vidioc-g-fmt.html

Re: [RFC] Request API and V4L2 capabilities

[Nicolas Dufresne]

[-- Attachment #1: Type: text/plain, Size: 8397 bytes --]

Le mercredi 22 août 2018 à 16:52 +0200, Paul Kocialkowski a écrit :
> Hi,
> 
> On Wed, 2018-08-15 at 09:57 -0400, Nicolas Dufresne wrote:
> > Le lundi 06 août 2018 à 10:16 +0200, Paul Kocialkowski a écrit :
> > > Hi Hans and all,
> > > 
> > > On Sat, 2018-08-04 at 15:50 +0200, Hans Verkuil wrote:
> > > > Hi all,
> > > > 
> > > > While the Request API patch series addresses all the core API issues, there
> > > > are some high-level considerations as well:
> > > > 
> > > > 1) How can the application tell that the Request API is supported and for
> > > >    which buffer types (capture/output) and pixel formats?
> > > > 
> > > > 2) How can the application tell if the Request API is required as opposed to being
> > > >    optional?
> > > > 
> > > > 3) Some controls may be required in each request, how to let userspace know this?
> > > >    Is it even necessary to inform userspace?
> > > > 
> > > > 4) (For bonus points): How to let the application know which streaming I/O modes
> > > >    are available? That's never been possible before, but it would be very nice
> > > >    indeed if that's made explicit.
> > > 
> > > Thanks for bringing up these considerations and questions, which perhaps
> > > cover the last missing bits for streamlined use of the request API by
> > > userspace. I would suggest another item, related to 3):
> > > 
> > > 5) How can applications tell whether the driver supports a specific
> > > codec profile/level, not only for encoding but also for decoding? It's
> > > common for low-end embedded hardware to not support the most advanced
> > > profiles (e.g. H264 high profile).
> > 
> > Hi Paul, after some discussion with Philip, he sent a proposal patch
> > that enables profile/level extended CID support to decoders too. The
> > control is made read-only, the point is not really the CID get/set but
> > that the controls allow enumerating the supported values. This seems
> > quite straightforward and easy to use.
> > 
> > This enumeration is already provided this way some of the existing
> > sate-full encoders. 
> 
> Sounds great, thanks for looking into it! I looked for the patch in the
> list, but couldn't find it off-hand. Do you have a link to it? I would
> like to bind it to the Cedrus VPU driver eventually.

I believe it is that one:
https://patchwork.kernel.org/patch/10494379/



> 
> Cheers,
> 
> Paul
> 
> > > > Since the Request API associates data with frame buffers it makes sense to expose
> > > > this as a new capability field in struct v4l2_requestbuffers and struct v4l2_create_buffers.
> > > > 
> > > > The first struct has 2 reserved fields, the second has 8, so it's not a problem to
> > > > take one for a capability field. Both structs also have a buffer type, so we know
> > > > if this is requested for a capture or output buffer type. The pixel format is known
> > > > in the driver, so HAS/REQUIRES_REQUESTS can be set based on that. I doubt we'll have
> > > > drivers where the request caps would actually depend on the pixel format, but it
> > > > theoretically possible. For both ioctls you can call them with count=0 at the start
> > > > of the application. REQBUFS has of course the side-effect of deleting all buffers,
> > > > but at the start of your application you don't have any yet. CREATE_BUFS has no
> > > > side-effects.
> > > 
> > > My initial thoughts on this point were to have flags exposed in
> > > v4l2_capability, but now that you're saying it, it does make sense for
> > > the flag to be associated with a buffer rather than the global device.
> > > 
> > > In addition, I've heard of cases (IIRC it was some Rockchip platforms)
> > > where the platform has both stateless and stateful VPUs (I think it was
> > > stateless up to H264 and stateful for H265). This would allow supporting
> > > these two hardware blocks under the same video device (if that makes
> > > sense anyway). And even if there's no immediate need, it's always good
> > > to have this level of granularity (with little drawbacks).
> > > 
> > > > I propose adding these capabilities:
> > > > 
> > > > #define V4L2_BUF_CAP_HAS_REQUESTS	0x00000001
> > > > #define V4L2_BUF_CAP_REQUIRES_REQUESTS	0x00000002
> > > > #define V4L2_BUF_CAP_HAS_MMAP		0x00000100
> > > > #define V4L2_BUF_CAP_HAS_USERPTR	0x00000200
> > > > #define V4L2_BUF_CAP_HAS_DMABUF		0x00000400
> > > > 
> > > > If REQUIRES_REQUESTS is set, then HAS_REQUESTS is also set.
> > > > 
> > > > At this time I think that REQUIRES_REQUESTS would only need to be set for the
> > > > output queue of stateless codecs.
> > > > 
> > > > If capabilities is 0, then it's from an old kernel and all you know is that
> > > > requests are certainly not supported, and that MMAP is supported. Whether USERPTR
> > > > or DMABUF are supported isn't known in that case (just try it :-) ).
> > > 
> > > Sounds good to me!
> > > 
> > > > Strictly speaking we do not need these HAS_MMAP/USERPTR/DMABUF caps, but it is very
> > > > easy to add if we create a new capability field anyway, and it has always annoyed
> > > > the hell out of me that we didn't have a good way to let userspace know what
> > > > streaming I/O modes we support. And with vb2 it's easy to implement.
> > > 
> > > I totally agree here, it would be very nice to take the occasion to
> > > expose to userspace what I/O modes are available. The current try-and-
> > > see approach works, but this feels much better indeed.
> > > 
> > > > Regarding point 3: I think this should be documented next to the pixel format. I.e.
> > > > the MPEG-2 Slice format used by the stateless cedrus codec requires the request API
> > > > and that two MPEG-2 controls (slice params and quantization matrices) must be present
> > > > in each request.
> > > > 
> > > > I am not sure a control flag (e.g. V4L2_CTRL_FLAG_REQUIRED_IN_REQ) is needed here.
> > > > It's really implied by the fact that you use a stateless codec. It doesn't help
> > > > generic applications like v4l2-ctl or qv4l2 either since in order to support
> > > > stateless codecs they will have to know about the details of these controls anyway.
> > > > 
> > > > So I am inclined to say that it is not necessary to expose this information in
> > > > the API, but it has to be documented together with the pixel format documentation.
> > > 
> > > I think this is affected by considerations about codec profile/level
> > > support. More specifically, some controls will only be required for
> > > supporting advanced codec profiles/levels, so they can only be
> > > explicitly marked with appropriate flags by the driver when the target
> > > profile/level is known. And I don't think it would be sane for userspace
> > > to explicitly set what profile/level it's aiming at. As a result, I
> > > don't think we can explicitly mark controls as required or optional.
> > > 
> > > I also like the idea that it should instead be implicit and that the
> > > documentation should detail which specific stateless metadata controls
> > > are required for a given profile/level.
> > > 
> > > As for controls validation, the approach followed in the Cedrus driver
> > > is to check that the most basic controls are filled and allow having
> > > missing controls for those that match advanced profiles.
> > > 
> > > Since this approach feels somewhat generic enough to be applied to all
> > > stateless VPU drivers, maybe this should be made a helper in the
> > > framework?
> > > 
> > > In addition, I see a need for exposing the maximum profile/level that
> > > the driver supports for decoding. I would suggest reusing the already-
> > > existing dedicated controls used for encoding for this purpose. For
> > > decoders, they would be used to expose the (read-only) maximum
> > > profile/level that is supported by the hardware and keep using them as a
> > > settable value in a range (matching the level of support) for encoders.
> > > 
> > > This is necessary for userspace to determine whether a given video can
> > > be decoded in hardware or not. Instead of half-way decoding the video
> > > (ending up in funky results), this would easily allow skipping hardware
> > > decoding and e.g. falling back on software decoding.
> > > 
> > > What do you think?
> > > 
> > > Cheers,
> > > 
> > > Paul
> > > 

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [RFC] Request API and V4L2 capabilities

[Paul Kocialkowski]

[-- Attachment #1: Type: text/plain, Size: 5810 bytes --]

Hi,

On Wed, 2018-08-22 at 14:33 -0300, Ezequiel Garcia wrote:
> On Wed, 2018-08-22 at 16:10 +0200, Paul Kocialkowski wrote:
> > Hi,
> > 
> > On Tue, 2018-08-21 at 17:52 +0900, Tomasz Figa wrote:
> > > Hi Hans, Paul,
> > > 
> > > On Mon, Aug 6, 2018 at 6:29 PM Paul Kocialkowski
> > > <paul.kocialkowski@bootlin.com> wrote:
> > > > 
> > > > On Mon, 2018-08-06 at 11:23 +0200, Hans Verkuil wrote:
> > > > > On 08/06/2018 11:13 AM, Paul Kocialkowski wrote:
> > > > > > Hi,
> > > > > > 
> > > > > > On Mon, 2018-08-06 at 10:32 +0200, Hans Verkuil wrote:
> > > > > > > On 08/06/2018 10:16 AM, Paul Kocialkowski wrote:
> > > > > > > > On Sat, 2018-08-04 at 15:50 +0200, Hans Verkuil wrote:
> > > > > > > > > Regarding point 3: I think this should be documented next to the pixel format. I.e.
> > > > > > > > > the MPEG-2 Slice format used by the stateless cedrus codec requires the request API
> > > > > > > > > and that two MPEG-2 controls (slice params and quantization matrices) must be present
> > > > > > > > > in each request.
> > > > > > > > > 
> > > > > > > > > I am not sure a control flag (e.g. V4L2_CTRL_FLAG_REQUIRED_IN_REQ) is needed here.
> > > > > > > > > It's really implied by the fact that you use a stateless codec. It doesn't help
> > > > > > > > > generic applications like v4l2-ctl or qv4l2 either since in order to support
> > > > > > > > > stateless codecs they will have to know about the details of these controls anyway.
> > > > > > > > > 
> > > > > > > > > So I am inclined to say that it is not necessary to expose this information in
> > > > > > > > > the API, but it has to be documented together with the pixel format documentation.
> > > > > > > > 
> > > > > > > > I think this is affected by considerations about codec profile/level
> > > > > > > > support. More specifically, some controls will only be required for
> > > > > > > > supporting advanced codec profiles/levels, so they can only be
> > > > > > > > explicitly marked with appropriate flags by the driver when the target
> > > > > > > > profile/level is known. And I don't think it would be sane for userspace
> > > > > > > > to explicitly set what profile/level it's aiming at. As a result, I
> > > > > > > > don't think we can explicitly mark controls as required or optional.
> > > 
> > > I'm not sure this is entirely true. The hardware may need to be
> > > explicitly told what profile the video is. It may even not be the
> > > hardware, but the driver itself too, given that the profile may imply
> > > the CAPTURE pixel format, e.g. for VP9 profiles:
> > > 
> > > profile 0
> > > color depth: 8 bit/sample, chroma subsampling: 4:2:0
> > > profile 1
> > > color depth: 8 bit, chroma subsampling: 4:2:0, 4:2:2, 4:4:4
> > > profile 2
> > > color depth: 10–12 bit, chroma subsampling: 4:2:0
> > > profile 3
> > > color depth: 10–12 bit, chroma subsampling: 4:2:0, 4:2:2, 4:4:4
> > > 
> > > (reference: https://en.wikipedia.org/wiki/VP9#Profiles)
> > 
> > I think it would be fair to expect userspace to select the right
> > destination format (and maybe have the driver error if there's a
> > mismatch with the meta-data) instead of having the driver somewhat
> > expose what format should be used.
> > 
> > But maybe this would be an API violation, since all the enumerated
> > formats are probably supposed to be selectable?
> > 
> > We could also look at it the other way round and consider that selecting
> > an exposed format is always legit, but that it implies passing a
> > bitstream that matches it or the driver will error (because of an
> > invalid bitstream passed, not because of a "wrong" selected format).
> > 
> 
> The API requires the user to negotiate via TRY_FMT/S_FMT. The driver
> usually does not return error on invalid formats, and simply return
> a format it can work with. I think the kernel-user contract has to
> guarantee if the driver accepted a given format, it won't fail to
> encoder or decode.

Well, the issue here is that in order to correctly enumerate the
formats, the driver needs to be aware of:
1. in what destination format the bitstream data is decoded to;
2. what format convesions the VPU can do.

Step 1 is known by userspace but is only passed to the driver with the
bitstream metadata from controls. This is much too late for trimming the
list of supported formats.

I'm not sure that passing an extra information to the driver early to
trim the list would make sense, because it comes to down to telling the
driver what format to target and then asking the driver was format it
supports, which is rather redundant.

I think the information we need to expose to userspace is whether the
driver supports a destination format that does not match the bitstream
format. We could make it part of the spec that, by lack of this
indication, the provided bitstream is expected to match the format that
was selected.

> I think that's why it makes sense for stateless drivers to set the
> profile/levels for a given job, in order to properly negotiate
> the input and output formats (for both encoders and decoders).  

I still fail to follow this logic. As far as I understood, profile/level
are indications of hardware capabilities required to decode the video,
not an indication of the precise features selected for decoding,
especially when it comes the format. As Tomasz mentionned in the
previous email, various formats may be supported by the same profile, so
setting the profile/level does not indicate which format is appropriate
for decoding the bitstream.

> [1] https://linuxtv.org/downloads/v4l-dvb-apis/uapi/v4l/vidioc-g-fmt.html
-- 
Paul Kocialkowski, Bootlin (formerly Free Electrons)
Embedded Linux and kernel engineering
https://bootlin.com

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC] Request API and V4L2 capabilities

[Hans Verkuil]

Hi all,

After reading through the comments I came to the following conclusions:

On 08/04/18 15:50, Hans Verkuil wrote:
> Hi all,
> 
> While the Request API patch series addresses all the core API issues, there
> are some high-level considerations as well:
> 
> 1) How can the application tell that the Request API is supported and for
>    which buffer types (capture/output) and pixel formats?
> 
> 2) How can the application tell if the Request API is required as opposed to being
>    optional?
> 
> 3) Some controls may be required in each request, how to let userspace know this?
>    Is it even necessary to inform userspace?
> 
> 4) (For bonus points): How to let the application know which streaming I/O modes
>    are available? That's never been possible before, but it would be very nice
>    indeed if that's made explicit.
> 
> Since the Request API associates data with frame buffers it makes sense to expose
> this as a new capability field in struct v4l2_requestbuffers and struct v4l2_create_buffers.
> 
> The first struct has 2 reserved fields, the second has 8, so it's not a problem to
> take one for a capability field. Both structs also have a buffer type, so we know
> if this is requested for a capture or output buffer type. The pixel format is known
> in the driver, so HAS/REQUIRES_REQUESTS can be set based on that. I doubt we'll have
> drivers where the request caps would actually depend on the pixel format, but it
> theoretically possible. For both ioctls you can call them with count=0 at the start
> of the application. REQBUFS has of course the side-effect of deleting all buffers,
> but at the start of your application you don't have any yet. CREATE_BUFS has no
> side-effects.
> 
> I propose adding these capabilities:
> 
> #define V4L2_BUF_CAP_HAS_REQUESTS	0x00000001
> #define V4L2_BUF_CAP_REQUIRES_REQUESTS	0x00000002
> #define V4L2_BUF_CAP_HAS_MMAP		0x00000100
> #define V4L2_BUF_CAP_HAS_USERPTR	0x00000200
> #define V4L2_BUF_CAP_HAS_DMABUF		0x00000400

I substituted SUPPORTS for HAS and dropped the REQUIRES_REQUESTS capability.
As Tomasz mentioned, technically (at least for stateless codecs) you could
handle just one frame at a time without using requests. It's very inefficient,
but it would work.

Otherwise I have implemented this as specified above.

> 
> If REQUIRES_REQUESTS is set, then HAS_REQUESTS is also set.
> 
> At this time I think that REQUIRES_REQUESTS would only need to be set for the
> output queue of stateless codecs.
> 
> If capabilities is 0, then it's from an old kernel and all you know is that
> requests are certainly not supported, and that MMAP is supported. Whether USERPTR
> or DMABUF are supported isn't known in that case (just try it :-) ).
> 
> Strictly speaking we do not need these HAS_MMAP/USERPTR/DMABUF caps, but it is very
> easy to add if we create a new capability field anyway, and it has always annoyed
> the hell out of me that we didn't have a good way to let userspace know what
> streaming I/O modes we support. And with vb2 it's easy to implement.
> 
> Regarding point 3: I think this should be documented next to the pixel format. I.e.
> the MPEG-2 Slice format used by the stateless cedrus codec requires the request API
> and that two MPEG-2 controls (slice params and quantization matrices) must be present
> in each request.

Everyone seemed to agree with this: which controls are required to be present in a
request should be documented as part of the corresponding pixel format.

> I am not sure a control flag (e.g. V4L2_CTRL_FLAG_REQUIRED_IN_REQ) is needed here.

Nobody liked this, so this proposed flag is dropped.

Regards,

	Hans

> It's really implied by the fact that you use a stateless codec. It doesn't help
> generic applications like v4l2-ctl or qv4l2 either since in order to support
> stateless codecs they will have to know about the details of these controls anyway.
> 
> So I am inclined to say that it is not necessary to expose this information in
> the API, but it has to be documented together with the pixel format documentation.
> 
> Comments? Ideas?
> 
> Regards,
> 
> 	Hans
> 

Re: [RFC] Request API and V4L2 capabilities

[Nicolas Dufresne]

[-- Attachment #1: Type: text/plain, Size: 8037 bytes --]

Le jeudi 23 août 2018 à 10:05 +0200, Paul Kocialkowski a écrit :
> Hi,
> 
> On Wed, 2018-08-22 at 14:33 -0300, Ezequiel Garcia wrote:
> > On Wed, 2018-08-22 at 16:10 +0200, Paul Kocialkowski wrote:
> > > Hi,
> > > 
> > > On Tue, 2018-08-21 at 17:52 +0900, Tomasz Figa wrote:
> > > > Hi Hans, Paul,
> > > > 
> > > > On Mon, Aug 6, 2018 at 6:29 PM Paul Kocialkowski
> > > > <paul.kocialkowski@bootlin.com> wrote:
> > > > > 
> > > > > On Mon, 2018-08-06 at 11:23 +0200, Hans Verkuil wrote:
> > > > > > On 08/06/2018 11:13 AM, Paul Kocialkowski wrote:
> > > > > > > Hi,
> > > > > > > 
> > > > > > > On Mon, 2018-08-06 at 10:32 +0200, Hans Verkuil wrote:
> > > > > > > > On 08/06/2018 10:16 AM, Paul Kocialkowski wrote:
> > > > > > > > > On Sat, 2018-08-04 at 15:50 +0200, Hans Verkuil wrote:
> > > > > > > > > > Regarding point 3: I think this should be documented next to the pixel format. I.e.
> > > > > > > > > > the MPEG-2 Slice format used by the stateless cedrus codec requires the request API
> > > > > > > > > > and that two MPEG-2 controls (slice params and quantization matrices) must be present
> > > > > > > > > > in each request.
> > > > > > > > > > 
> > > > > > > > > > I am not sure a control flag (e.g. V4L2_CTRL_FLAG_REQUIRED_IN_REQ) is needed here.
> > > > > > > > > > It's really implied by the fact that you use a stateless codec. It doesn't help
> > > > > > > > > > generic applications like v4l2-ctl or qv4l2 either since in order to support
> > > > > > > > > > stateless codecs they will have to know about the details of these controls anyway.
> > > > > > > > > > 
> > > > > > > > > > So I am inclined to say that it is not necessary to expose this information in
> > > > > > > > > > the API, but it has to be documented together with the pixel format documentation.
> > > > > > > > > 
> > > > > > > > > I think this is affected by considerations about codec profile/level
> > > > > > > > > support. More specifically, some controls will only be required for
> > > > > > > > > supporting advanced codec profiles/levels, so they can only be
> > > > > > > > > explicitly marked with appropriate flags by the driver when the target
> > > > > > > > > profile/level is known. And I don't think it would be sane for userspace
> > > > > > > > > to explicitly set what profile/level it's aiming at. As a result, I
> > > > > > > > > don't think we can explicitly mark controls as required or optional.
> > > > 
> > > > I'm not sure this is entirely true. The hardware may need to be
> > > > explicitly told what profile the video is. It may even not be the
> > > > hardware, but the driver itself too, given that the profile may imply
> > > > the CAPTURE pixel format, e.g. for VP9 profiles:
> > > > 
> > > > profile 0
> > > > color depth: 8 bit/sample, chroma subsampling: 4:2:0
> > > > profile 1
> > > > color depth: 8 bit, chroma subsampling: 4:2:0, 4:2:2, 4:4:4
> > > > profile 2
> > > > color depth: 10–12 bit, chroma subsampling: 4:2:0
> > > > profile 3
> > > > color depth: 10–12 bit, chroma subsampling: 4:2:0, 4:2:2, 4:4:4
> > > > 
> > > > (reference: https://en.wikipedia.org/wiki/VP9#Profiles)
> > > 
> > > I think it would be fair to expect userspace to select the right
> > > destination format (and maybe have the driver error if there's a
> > > mismatch with the meta-data) instead of having the driver somewhat
> > > expose what format should be used.
> > > 
> > > But maybe this would be an API violation, since all the enumerated
> > > formats are probably supposed to be selectable?
> > > 
> > > We could also look at it the other way round and consider that selecting
> > > an exposed format is always legit, but that it implies passing a
> > > bitstream that matches it or the driver will error (because of an
> > > invalid bitstream passed, not because of a "wrong" selected format).
> > > 
> > 
> > The API requires the user to negotiate via TRY_FMT/S_FMT. The driver
> > usually does not return error on invalid formats, and simply return
> > a format it can work with. I think the kernel-user contract has to
> > guarantee if the driver accepted a given format, it won't fail to
> > encoder or decode.
> 
> Well, the issue here is that in order to correctly enumerate the
> formats, the driver needs to be aware of:
> 1. in what destination format the bitstream data is decoded to;

This is covered by the state-full specification patch if I remember
correctly. So the driver, if it's a multi-format, will first return all
possible formats, and later on, will return the proper subset. So let's
take an encoder, after setting the capture format, the enumeration of
the raw formats could then be limited to what is supported for this
output. For an H264 encoder, what could also affect this list is the
profile that being set. For decoder, this list is reduced after
sufficient headers information has been given. Though for decoder it's
also limited case, since it only apply to decoder that do a conversion
before releasing the output buffer (like CODA does).

What isn't so nice with this approach, is that it adds an implicit
uninitialized state after open() which isn't usual to the V4L2 API and
might not be that trivial or nice to handle in drivers.

> 2. what format convesions the VPU can do.

whenever possible, exposing the color conversion as a seperate m2m
driver is better approach. Makes the driver simpler and avoids having
to add this double enumeration support.

> 
> Step 1 is known by userspace but is only passed to the driver with the
> bitstream metadata from controls. This is much too late for trimming the
> list of supported formats.
> 
> I'm not sure that passing an extra information to the driver early to
> trim the list would make sense, because it comes to down to telling the
> driver what format to target and then asking the driver was format it
> supports, which is rather redundant.
> 
> I think the information we need to expose to userspace is whether the
> driver supports a destination format that does not match the bitstream
> format. We could make it part of the spec that, by lack of this
> indication, the provided bitstream is expected to match the format that
> was selected.

I'm not sure why you consider this too late. With decoder, the OUTPUT
and CAPTURE stream is asynchronous. So we start streaming on the OUTPUT
until the driver notify (V4L2_EVENT_SOURCE_CHANGE). We then enumerate
the formats again at that moment, and then configure and start the
CAPTURE.

> 
> > I think that's why it makes sense for stateless drivers to set the
> > profile/levels for a given job, in order to properly negotiate
> > the input and output formats (for both encoders and decoders).  
> 
> I still fail to follow this logic. As far as I understood, profile/level
> are indications of hardware capabilities required to decode the video,
> not an indication of the precise features selected for decoding,
> especially when it comes the format. As Tomasz mentionned in the
> previous email, various formats may be supported by the same profile, so
> setting the profile/level does not indicate which format is appropriate
> for decoding the bitstream.

For decoder, the only purpose I have ever seen of setting the
profile/level is for pre-allocation purpose. This information gives an
upper bound to the largest required buffer needed to hold a frame,
though as you said, it's not a fixed indication. For stateless codec,
this information is describe in the PPS table, with much more details
then just the profile/level. So for stateless *decoder*, the
profile/level controls would be a bit redundant. Though the control
enumerate list if profile/level is important to userspace in order to
change some generic error into "Sorry, your hardware does not support
profile X" or to be able to fallback to another decoder if any.

> 
> > [1] https://linuxtv.org/downloads/v4l-dvb-apis/uapi/v4l/vidioc-g-fmt.html

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [RFC] Request API and V4L2 capabilities

[Nicolas Dufresne]

[-- Attachment #1: Type: text/plain, Size: 915 bytes --]

Le jeudi 23 août 2018 à 16:31 +0200, Hans Verkuil a écrit :
> > I propose adding these capabilities:
> > 
> > #define V4L2_BUF_CAP_HAS_REQUESTS     0x00000001
> > #define V4L2_BUF_CAP_REQUIRES_REQUESTS        0x00000002
> > #define V4L2_BUF_CAP_HAS_MMAP         0x00000100
> > #define V4L2_BUF_CAP_HAS_USERPTR      0x00000200
> > #define V4L2_BUF_CAP_HAS_DMABUF               0x00000400
> 
> I substituted SUPPORTS for HAS and dropped the REQUIRES_REQUESTS capability.
> As Tomasz mentioned, technically (at least for stateless codecs) you could
> handle just one frame at a time without using requests. It's very inefficient,
> but it would work.

I thought the request was providing a data structure to refer back to
the frames, so each codec don't have to implement one. Do you mean that
the framework will implicitly request requests in that mode ? or simply
that there is no such helper ?

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [RFC] Request API and V4L2 capabilities

[Hans Verkuil]

On 08/23/2018 07:37 PM, Nicolas Dufresne wrote:
> Le jeudi 23 août 2018 à 16:31 +0200, Hans Verkuil a écrit :
>>> I propose adding these capabilities:
>>>
>>> #define V4L2_BUF_CAP_HAS_REQUESTS     0x00000001
>>> #define V4L2_BUF_CAP_REQUIRES_REQUESTS        0x00000002
>>> #define V4L2_BUF_CAP_HAS_MMAP         0x00000100
>>> #define V4L2_BUF_CAP_HAS_USERPTR      0x00000200
>>> #define V4L2_BUF_CAP_HAS_DMABUF               0x00000400
>>
>> I substituted SUPPORTS for HAS and dropped the REQUIRES_REQUESTS capability.
>> As Tomasz mentioned, technically (at least for stateless codecs) you could
>> handle just one frame at a time without using requests. It's very inefficient,
>> but it would work.
> 
> I thought the request was providing a data structure to refer back to
> the frames, so each codec don't have to implement one. Do you mean that
> the framework will implicitly request requests in that mode ? or simply
> that there is no such helper ?

Yes, that's done through controls as well.

The idea would be that you set the necessary controls, queue a buffer to
the output queue, dequeue a buffer from the output queue, read back any
new state information and repeat the process.

That said, I'm not sure if the cedrus driver for example can handle this
at the moment. It is also inefficient and it won't work if codecs require
more than one buffer in the queue for whatever reason.

Tomasz, Paul, please correct me if I am wrong.

In any case, I think we can do without this proposed capability since it is
simply a requirement when implementing the pixelformat for the stateless
codec that the Request API will be available and it should be documented
as such in the spec.

Regards,

	Hans

Re: [RFC] Request API and V4L2 capabilities

[Paul Kocialkowski]

[-- Attachment #1: Type: text/plain, Size: 2421 bytes --]

Hi,

On Fri, 2018-08-24 at 09:29 +0200, Hans Verkuil wrote:
> On 08/23/2018 07:37 PM, Nicolas Dufresne wrote:
> > Le jeudi 23 août 2018 à 16:31 +0200, Hans Verkuil a écrit :
> > > > I propose adding these capabilities:
> > > > 
> > > > #define V4L2_BUF_CAP_HAS_REQUESTS     0x00000001
> > > > #define V4L2_BUF_CAP_REQUIRES_REQUESTS        0x00000002
> > > > #define V4L2_BUF_CAP_HAS_MMAP         0x00000100
> > > > #define V4L2_BUF_CAP_HAS_USERPTR      0x00000200
> > > > #define V4L2_BUF_CAP_HAS_DMABUF               0x00000400
> > > 
> > > I substituted SUPPORTS for HAS and dropped the REQUIRES_REQUESTS capability.
> > > As Tomasz mentioned, technically (at least for stateless codecs) you could
> > > handle just one frame at a time without using requests. It's very inefficient,
> > > but it would work.
> > 
> > I thought the request was providing a data structure to refer back to
> > the frames, so each codec don't have to implement one. Do you mean that
> > the framework will implicitly request requests in that mode ? or simply
> > that there is no such helper ?
> 
> Yes, that's done through controls as well.
> 
> The idea would be that you set the necessary controls, queue a buffer to
> the output queue, dequeue a buffer from the output queue, read back any
> new state information and repeat the process.
> 
> That said, I'm not sure if the cedrus driver for example can handle this
> at the moment. It is also inefficient and it won't work if codecs require
> more than one buffer in the queue for whatever reason.
> 
> Tomasz, Paul, please correct me if I am wrong.

I haven't tested decoding without requests, but I suppose it should work
when submitting only one source buffer at a time.

By the way, note that our VAAPI backend for the request API gets the
slice data and metadata for each frame sequentially and we are not yet
starting to fill the next request before the current one was completed
(fences would probably help implement that).

> In any case, I think we can do without this proposed capability since it is
> simply a requirement when implementing the pixelformat for the stateless
> codec that the Request API will be available and it should be documented
> as such in the spec.

Agreed.

Cheers,

Paul

-- 
Paul Kocialkowski, Bootlin (formerly Free Electrons)
Embedded Linux and kernel engineering
https://bootlin.com

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC] Request API and V4L2 capabilities

[Tomasz Figa]

On Wed, Aug 22, 2018 at 11:10 PM Paul Kocialkowski
<paul.kocialkowski@bootlin.com> wrote:
>
> Hi,
>
> On Tue, 2018-08-21 at 17:52 +0900, Tomasz Figa wrote:
> > Hi Hans, Paul,
> >
> > On Mon, Aug 6, 2018 at 6:29 PM Paul Kocialkowski
> > <paul.kocialkowski@bootlin.com> wrote:
> > >
> > > On Mon, 2018-08-06 at 11:23 +0200, Hans Verkuil wrote:
> > > > On 08/06/2018 11:13 AM, Paul Kocialkowski wrote:
> > > > > Hi,
> > > > >
> > > > > On Mon, 2018-08-06 at 10:32 +0200, Hans Verkuil wrote:
> > > > > > On 08/06/2018 10:16 AM, Paul Kocialkowski wrote:
> > > > > > > On Sat, 2018-08-04 at 15:50 +0200, Hans Verkuil wrote:
> > > > > > > > Regarding point 3: I think this should be documented next to the pixel format. I.e.
> > > > > > > > the MPEG-2 Slice format used by the stateless cedrus codec requires the request API
> > > > > > > > and that two MPEG-2 controls (slice params and quantization matrices) must be present
> > > > > > > > in each request.
> > > > > > > >
> > > > > > > > I am not sure a control flag (e.g. V4L2_CTRL_FLAG_REQUIRED_IN_REQ) is needed here.
> > > > > > > > It's really implied by the fact that you use a stateless codec. It doesn't help
> > > > > > > > generic applications like v4l2-ctl or qv4l2 either since in order to support
> > > > > > > > stateless codecs they will have to know about the details of these controls anyway.
> > > > > > > >
> > > > > > > > So I am inclined to say that it is not necessary to expose this information in
> > > > > > > > the API, but it has to be documented together with the pixel format documentation.
> > > > > > >
> > > > > > > I think this is affected by considerations about codec profile/level
> > > > > > > support. More specifically, some controls will only be required for
> > > > > > > supporting advanced codec profiles/levels, so they can only be
> > > > > > > explicitly marked with appropriate flags by the driver when the target
> > > > > > > profile/level is known. And I don't think it would be sane for userspace
> > > > > > > to explicitly set what profile/level it's aiming at. As a result, I
> > > > > > > don't think we can explicitly mark controls as required or optional.
> >
> > I'm not sure this is entirely true. The hardware may need to be
> > explicitly told what profile the video is. It may even not be the
> > hardware, but the driver itself too, given that the profile may imply
> > the CAPTURE pixel format, e.g. for VP9 profiles:
> >
> > profile 0
> > color depth: 8 bit/sample, chroma subsampling: 4:2:0
> > profile 1
> > color depth: 8 bit, chroma subsampling: 4:2:0, 4:2:2, 4:4:4
> > profile 2
> > color depth: 10–12 bit, chroma subsampling: 4:2:0
> > profile 3
> > color depth: 10–12 bit, chroma subsampling: 4:2:0, 4:2:2, 4:4:4
> >
> > (reference: https://en.wikipedia.org/wiki/VP9#Profiles)
>
> I think it would be fair to expect userspace to select the right
> destination format (and maybe have the driver error if there's a
> mismatch with the meta-data) instead of having the driver somewhat
> expose what format should be used.

There are many different memory representations of the same physical
YUV format, just for YUV 4:2:0: NV12, NV12M, NV21, NV21M, YUV420,
YUV420M, YVU420, YVU420M. It depends on hardware and driver which one
would be available for given stream to decode. How is the user space
expected to know which one is, without querying the driver first?

>
> But maybe this would be an API violation, since all the enumerated
> formats are probably supposed to be selectable?

Correct.

>
> We could also look at it the other way round and consider that selecting
> an exposed format is always legit, but that it implies passing a
> bitstream that matches it or the driver will error (because of an
> invalid bitstream passed, not because of a "wrong" selected format).

As per above, it's unlikely that a generic user space can set the
right format. It may be able to narrow down the list of exposed
formats to those which make sense for the generic information about
the stream it has, e.g. VP9 profile 0 -> YUV 4:2:0, but there may
still be a constraint on which representation is allowed depending on
stream features.

>
> As far as I understood, the profile/level information is there to
> indicate a set of supported features by the decoder, not as an
> information used for the decoding process. Each corresponding feature is
> enabled or not in the bitstream meta-data and that's all the information
> the decoder really needs.
>
> This is why I think that setting the profile/level explicitly is not
> justified by the nature of the process and adding it only for
> convenience or marking whether controls are optional doesn't seem
> justified at this point, in my opinion.

Okay, it's actually a good point. Whether given format can be
supported is not entirely dictated by profile/level. There may be some
initial negotiation needed, involving the user space setting the
parsed stream parameters through respective controls.

>
> > > > > > > I also like the idea that it should instead be implicit and that the
> > > > > > > documentation should detail which specific stateless metadata controls
> > > > > > > are required for a given profile/level.
> > > > > > >
> > > > > > > As for controls validation, the approach followed in the Cedrus driver
> > > > > > > is to check that the most basic controls are filled and allow having
> > > > > > > missing controls for those that match advanced profiles.
> > > > > > >
> > > > > > > Since this approach feels somewhat generic enough to be applied to all
> > > > > > > stateless VPU drivers, maybe this should be made a helper in the
> > > > > > > framework?
> > > > > >
> > > > > > Sounds reasonable. Not sure if it will be in the first version, but it is
> > > > > > easy to add later.
> > > > >
> > > > > Definitely, I don't think this is such a high priority for now either.
> > > > >
> >
> > We may want to put strict requirements on what controls are provided
> > for given codec+profile/level. Otherwise we might get some user space
> > that doesn't provide some of them and works only by luck, e.g. because
> > some hardware defaults on initial drivers luckily match the needed
> > values. Even if we don't validate it in the code yet, we should put a
> > big warning saying that not providing the required controls would
> > result in undefined behavior.
>
> I don't think having such strict requirements are a good thing. Even
> with the level/profile made explicit, what if the video under-uses its
> features and thus legitimately doesn't need to have all the controls
> that could be supported with the level/profile? This can probably also
> be frame-specific, so some frames could require more controls than
> others.

That's exactly the best first step towards an user space that relies
on unspecified behavior. Even if the video doesn't use all the
features as per given profile/level, user space should be expected to
initialize the state with reasonable defaults, since it's essentially
for the driver/hardware to decide which state it needs to do the
decoding.

Whether profile/level is the right indicator to judge what's required
and what's not is a different question. It indeed may not be. However,
we need to clearly state what controls and when the application needs
to set in the specification. I'm not talking about requirements as for
bailing out with an error code, but exactly as for having that
included in the specification and correct behavior only guaranteed if
followed correctly.

>
> This also leads me to believe that the profile/level indication should
> be used as a support indication to userspace, not as a way to expose the
> required features for decoding to the kernel
>
> We could still enforce checks for the most basic controls (that are used
> for all types of slices to decode) and error if they are missing. We
> could also check the bits that indicate more advanced features in these
> basic controls and decide what other controls are required from that.
>
> > > > > > > In addition, I see a need for exposing the maximum profile/level that
> > > > > > > the driver supports for decoding. I would suggest reusing the already-
> > > > > > > existing dedicated controls used for encoding for this purpose. For
> > > > > > > decoders, they would be used to expose the (read-only) maximum
> > > > > > > profile/level that is supported by the hardware and keep using them as a
> > > > > > > settable value in a range (matching the level of support) for encoders.
> > > > > > >
> > > > > > > This is necessary for userspace to determine whether a given video can
> > > > > > > be decoded in hardware or not. Instead of half-way decoding the video
> > > > > > > (ending up in funky results), this would easily allow skipping hardware
> > > > > > > decoding and e.g. falling back on software decoding.
> > > > > >
> > > > > > I think it might be better to expose this through new read-only bitmask
> > > > > > controls: i.e. a bitmask containing the supported profiles and levels.
> > > > >
> > > > > It seems that this is more or less what the coda driver is doing for
> > > > > decoding actually, although it uses a menu control between min/max
> > > > > supported profile/levels, with a mask to "blacklist" the unsupported
> > > > > values. Then, the V4L2_CTRL_FLAG_READ_ONLY flag is set to keep the
> > > > > control read-only.
> > > > >
> > > > > > Reusing the existing controls for a decoder is odd since there is not
> > > > > > really a concept of a 'current' value since you just want to report what
> > > > > > is supported. And I am not sure if all decoders can report the profile
> > > > > > or level that they detect.
> > > > >
> > > > > Is that really a problem when the READ_ONLY flag is set? I thought it
> > > > > was designed to fit this specific case, when the driver reports a value
> > > > > that userspace cannot affect.
> > > >
> > > > Well, for read-only menu controls the current value of the control would
> > > > have to indicate what the current profile/level is that is being decoded.
> > > >
> > > > That's not really relevant since what you want is just to query the
> > > > supported profiles/levels. A read-only bitmask control is the fastest
> > > > method (if only because using a menu control requires the application to
> > > > enumerate all possibilities with QUERYMENU).
> >
> > Besides querying for supported profiles,
> >  - For stateless codecs we also need to set the profile, since the
> > codec itself does only the number crunching.
>
> I disagree here, see above.
>

Indeed, profile is normally a function of various parameters included
in the more general decoder state (e.g. bitstream headers).

> >  - For stateful codecs, the decoder would also report the detected
> > profile after parsing the bitstream (although this is possibly not of
> > a big importance to the user space).
>
> I don't follow the logic behind this. This means informing userspace of
> the capabilities required to decode the video that the VPU is currently
> decoding... so that it can decided whether to decode it?

That's what some of current drivers do, but I also fail to see it
being useful. We should make sure that there is no user space relying
on this behavior, if we intend to change it.

>
> > As for querying itself, there is still more to it than could be
> > handled with just a read only control. To detect what CAPTURE formats
> > are supported for given profile, one would have to set the profile
> > control first and then use ENUM_FMT.
>
> Or consider that the bitstream is invalid if it doesn't match the
> selected format and let userspace pick the appropriate format.

See above. The appropriate format is only known to the driver.

Best regards,
Tomasz

Re: [RFC] Request API and V4L2 capabilities

[Tomasz Figa]

On Thu, Aug 23, 2018 at 5:05 PM Paul Kocialkowski
<paul.kocialkowski@bootlin.com> wrote:
>
> Hi,
>
> On Wed, 2018-08-22 at 14:33 -0300, Ezequiel Garcia wrote:
> > On Wed, 2018-08-22 at 16:10 +0200, Paul Kocialkowski wrote:
> > > Hi,
> > >
> > > On Tue, 2018-08-21 at 17:52 +0900, Tomasz Figa wrote:
> > > > Hi Hans, Paul,
> > > >
> > > > On Mon, Aug 6, 2018 at 6:29 PM Paul Kocialkowski
> > > > <paul.kocialkowski@bootlin.com> wrote:
> > > > >
> > > > > On Mon, 2018-08-06 at 11:23 +0200, Hans Verkuil wrote:
> > > > > > On 08/06/2018 11:13 AM, Paul Kocialkowski wrote:
> > > > > > > Hi,
> > > > > > >
> > > > > > > On Mon, 2018-08-06 at 10:32 +0200, Hans Verkuil wrote:
> > > > > > > > On 08/06/2018 10:16 AM, Paul Kocialkowski wrote:
> > > > > > > > > On Sat, 2018-08-04 at 15:50 +0200, Hans Verkuil wrote:
> > > > > > > > > > Regarding point 3: I think this should be documented next to the pixel format. I.e.
> > > > > > > > > > the MPEG-2 Slice format used by the stateless cedrus codec requires the request API
> > > > > > > > > > and that two MPEG-2 controls (slice params and quantization matrices) must be present
> > > > > > > > > > in each request.
> > > > > > > > > >
> > > > > > > > > > I am not sure a control flag (e.g. V4L2_CTRL_FLAG_REQUIRED_IN_REQ) is needed here.
> > > > > > > > > > It's really implied by the fact that you use a stateless codec. It doesn't help
> > > > > > > > > > generic applications like v4l2-ctl or qv4l2 either since in order to support
> > > > > > > > > > stateless codecs they will have to know about the details of these controls anyway.
> > > > > > > > > >
> > > > > > > > > > So I am inclined to say that it is not necessary to expose this information in
> > > > > > > > > > the API, but it has to be documented together with the pixel format documentation.
> > > > > > > > >
> > > > > > > > > I think this is affected by considerations about codec profile/level
> > > > > > > > > support. More specifically, some controls will only be required for
> > > > > > > > > supporting advanced codec profiles/levels, so they can only be
> > > > > > > > > explicitly marked with appropriate flags by the driver when the target
> > > > > > > > > profile/level is known. And I don't think it would be sane for userspace
> > > > > > > > > to explicitly set what profile/level it's aiming at. As a result, I
> > > > > > > > > don't think we can explicitly mark controls as required or optional.
> > > >
> > > > I'm not sure this is entirely true. The hardware may need to be
> > > > explicitly told what profile the video is. It may even not be the
> > > > hardware, but the driver itself too, given that the profile may imply
> > > > the CAPTURE pixel format, e.g. for VP9 profiles:
> > > >
> > > > profile 0
> > > > color depth: 8 bit/sample, chroma subsampling: 4:2:0
> > > > profile 1
> > > > color depth: 8 bit, chroma subsampling: 4:2:0, 4:2:2, 4:4:4
> > > > profile 2
> > > > color depth: 10–12 bit, chroma subsampling: 4:2:0
> > > > profile 3
> > > > color depth: 10–12 bit, chroma subsampling: 4:2:0, 4:2:2, 4:4:4
> > > >
> > > > (reference: https://en.wikipedia.org/wiki/VP9#Profiles)
> > >
> > > I think it would be fair to expect userspace to select the right
> > > destination format (and maybe have the driver error if there's a
> > > mismatch with the meta-data) instead of having the driver somewhat
> > > expose what format should be used.
> > >
> > > But maybe this would be an API violation, since all the enumerated
> > > formats are probably supposed to be selectable?
> > >
> > > We could also look at it the other way round and consider that selecting
> > > an exposed format is always legit, but that it implies passing a
> > > bitstream that matches it or the driver will error (because of an
> > > invalid bitstream passed, not because of a "wrong" selected format).
> > >
> >
> > The API requires the user to negotiate via TRY_FMT/S_FMT. The driver
> > usually does not return error on invalid formats, and simply return
> > a format it can work with. I think the kernel-user contract has to
> > guarantee if the driver accepted a given format, it won't fail to
> > encoder or decode.
>
> Well, the issue here is that in order to correctly enumerate the
> formats, the driver needs to be aware of:
> 1. in what destination format the bitstream data is decoded to;
> 2. what format convesions the VPU can do.
>
> Step 1 is known by userspace but is only passed to the driver with the
> bitstream metadata from controls. This is much too late for trimming the
> list of supported formats.

That's not true. See my previous reply. The user space only knows the
physical representation of samples in the stream, i.e. YUV 4:2:0 or
4:2:2. It doesn't know anything about hardware constraints on the
memory representation.

>
> I'm not sure that passing an extra information to the driver early to
> trim the list would make sense, because it comes to down to telling the
> driver what format to target and then asking the driver was format it
> supports, which is rather redundant.
>
> I think the information we need to expose to userspace is whether the
> driver supports a destination format that does not match the bitstream
> format. We could make it part of the spec that, by lack of this
> indication, the provided bitstream is expected to match the format that
> was selected.
>
> > I think that's why it makes sense for stateless drivers to set the
> > profile/levels for a given job, in order to properly negotiate
> > the input and output formats (for both encoders and decoders).
>
> I still fail to follow this logic. As far as I understood, profile/level
> are indications of hardware capabilities required to decode the video,
> not an indication of the precise features selected for decoding,
> especially when it comes the format. As Tomasz mentionned in the
> previous email, various formats may be supported by the same profile, so
> setting the profile/level does not indicate which format is appropriate
> for decoding the bitstream.

Indeed, after thinking a bit more about this, profile/levels alone are
not a good indicator for this. I'm afraid we may need an
initialization sequence that involves user space providing to the
driver any necessary state needed for it to determine the exact set of
supported memory representations (pixel formats).

Best regards,
Tomasz

Re: [RFC] Request API and V4L2 capabilities

[Tomasz Figa]

On Fri, Aug 24, 2018 at 2:33 AM Nicolas Dufresne <nicolas@ndufresne.ca> wrote:
>
> Le jeudi 23 août 2018 à 10:05 +0200, Paul Kocialkowski a écrit :
> > Hi,
> >
> > On Wed, 2018-08-22 at 14:33 -0300, Ezequiel Garcia wrote:
> > > On Wed, 2018-08-22 at 16:10 +0200, Paul Kocialkowski wrote:
> > > > Hi,
> > > >
> > > > On Tue, 2018-08-21 at 17:52 +0900, Tomasz Figa wrote:
> > > > > Hi Hans, Paul,
> > > > >
> > > > > On Mon, Aug 6, 2018 at 6:29 PM Paul Kocialkowski
> > > > > <paul.kocialkowski@bootlin.com> wrote:
> > > > > >
> > > > > > On Mon, 2018-08-06 at 11:23 +0200, Hans Verkuil wrote:
> > > > > > > On 08/06/2018 11:13 AM, Paul Kocialkowski wrote:
> > > > > > > > Hi,
> > > > > > > >
> > > > > > > > On Mon, 2018-08-06 at 10:32 +0200, Hans Verkuil wrote:
> > > > > > > > > On 08/06/2018 10:16 AM, Paul Kocialkowski wrote:
> > > > > > > > > > On Sat, 2018-08-04 at 15:50 +0200, Hans Verkuil wrote:
> > > > > > > > > > > Regarding point 3: I think this should be documented next to the pixel format. I.e.
> > > > > > > > > > > the MPEG-2 Slice format used by the stateless cedrus codec requires the request API
> > > > > > > > > > > and that two MPEG-2 controls (slice params and quantization matrices) must be present
> > > > > > > > > > > in each request.
> > > > > > > > > > >
> > > > > > > > > > > I am not sure a control flag (e.g. V4L2_CTRL_FLAG_REQUIRED_IN_REQ) is needed here.
> > > > > > > > > > > It's really implied by the fact that you use a stateless codec. It doesn't help
> > > > > > > > > > > generic applications like v4l2-ctl or qv4l2 either since in order to support
> > > > > > > > > > > stateless codecs they will have to know about the details of these controls anyway.
> > > > > > > > > > >
> > > > > > > > > > > So I am inclined to say that it is not necessary to expose this information in
> > > > > > > > > > > the API, but it has to be documented together with the pixel format documentation.
> > > > > > > > > >
> > > > > > > > > > I think this is affected by considerations about codec profile/level
> > > > > > > > > > support. More specifically, some controls will only be required for
> > > > > > > > > > supporting advanced codec profiles/levels, so they can only be
> > > > > > > > > > explicitly marked with appropriate flags by the driver when the target
> > > > > > > > > > profile/level is known. And I don't think it would be sane for userspace
> > > > > > > > > > to explicitly set what profile/level it's aiming at. As a result, I
> > > > > > > > > > don't think we can explicitly mark controls as required or optional.
> > > > >
> > > > > I'm not sure this is entirely true. The hardware may need to be
> > > > > explicitly told what profile the video is. It may even not be the
> > > > > hardware, but the driver itself too, given that the profile may imply
> > > > > the CAPTURE pixel format, e.g. for VP9 profiles:
> > > > >
> > > > > profile 0
> > > > > color depth: 8 bit/sample, chroma subsampling: 4:2:0
> > > > > profile 1
> > > > > color depth: 8 bit, chroma subsampling: 4:2:0, 4:2:2, 4:4:4
> > > > > profile 2
> > > > > color depth: 10–12 bit, chroma subsampling: 4:2:0
> > > > > profile 3
> > > > > color depth: 10–12 bit, chroma subsampling: 4:2:0, 4:2:2, 4:4:4
> > > > >
> > > > > (reference: https://en.wikipedia.org/wiki/VP9#Profiles)
> > > >
> > > > I think it would be fair to expect userspace to select the right
> > > > destination format (and maybe have the driver error if there's a
> > > > mismatch with the meta-data) instead of having the driver somewhat
> > > > expose what format should be used.
> > > >
> > > > But maybe this would be an API violation, since all the enumerated
> > > > formats are probably supposed to be selectable?
> > > >
> > > > We could also look at it the other way round and consider that selecting
> > > > an exposed format is always legit, but that it implies passing a
> > > > bitstream that matches it or the driver will error (because of an
> > > > invalid bitstream passed, not because of a "wrong" selected format).
> > > >
> > >
> > > The API requires the user to negotiate via TRY_FMT/S_FMT. The driver
> > > usually does not return error on invalid formats, and simply return
> > > a format it can work with. I think the kernel-user contract has to
> > > guarantee if the driver accepted a given format, it won't fail to
> > > encoder or decode.
> >
> > Well, the issue here is that in order to correctly enumerate the
> > formats, the driver needs to be aware of:
> > 1. in what destination format the bitstream data is decoded to;
>
> This is covered by the state-full specification patch if I remember
> correctly. So the driver, if it's a multi-format, will first return all
> possible formats, and later on, will return the proper subset. So let's
> take an encoder, after setting the capture format, the enumeration of
> the raw formats could then be limited to what is supported for this
> output. For an H264 encoder, what could also affect this list is the
> profile that being set. For decoder, this list is reduced after
> sufficient headers information has been given. Though for decoder it's
> also limited case, since it only apply to decoder that do a conversion
> before releasing the output buffer (like CODA does).
>
> What isn't so nice with this approach, is that it adds an implicit
> uninitialized state after open() which isn't usual to the V4L2 API and
> might not be that trivial or nice to handle in drivers.

I don't think we defined it this way and as you pointed, it's against
the general V4L2 API semantics. After open, there is a default coded
format set and ENUM_FMT/FRAME_SIZES returns what's available for the
default format.

>
> > 2. what format convesions the VPU can do.
>
> whenever possible, exposing the color conversion as a seperate m2m
> driver is better approach. Makes the driver simpler and avoids having
> to add this double enumeration support.

How about the overhead of having the decoded data stored to memory
once, read once and then written yet again, even though the hardware
supports doing it on the fly?

>
> >
> > Step 1 is known by userspace but is only passed to the driver with the
> > bitstream metadata from controls. This is much too late for trimming the
> > list of supported formats.
> >
> > I'm not sure that passing an extra information to the driver early to
> > trim the list would make sense, because it comes to down to telling the
> > driver what format to target and then asking the driver was format it
> > supports, which is rather redundant.
> >
> > I think the information we need to expose to userspace is whether the
> > driver supports a destination format that does not match the bitstream
> > format. We could make it part of the spec that, by lack of this
> > indication, the provided bitstream is expected to match the format that
> > was selected.
>
> I'm not sure why you consider this too late. With decoder, the OUTPUT
> and CAPTURE stream is asynchronous. So we start streaming on the OUTPUT
> until the driver notify (V4L2_EVENT_SOURCE_CHANGE). We then enumerate
> the formats again at that moment, and then configure and start the
> CAPTURE.

Yeah, I think we may eventually need this kind of initialization
sequence, similar to stateful hardware...

Best regards,
Tomasz

Re: [RFC] Request API and V4L2 capabilities

[Tomasz Figa]

On Fri, Aug 24, 2018 at 4:30 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>
> On 08/23/2018 07:37 PM, Nicolas Dufresne wrote:
> > Le jeudi 23 août 2018 à 16:31 +0200, Hans Verkuil a écrit :
> >>> I propose adding these capabilities:
> >>>
> >>> #define V4L2_BUF_CAP_HAS_REQUESTS     0x00000001
> >>> #define V4L2_BUF_CAP_REQUIRES_REQUESTS        0x00000002
> >>> #define V4L2_BUF_CAP_HAS_MMAP         0x00000100
> >>> #define V4L2_BUF_CAP_HAS_USERPTR      0x00000200
> >>> #define V4L2_BUF_CAP_HAS_DMABUF               0x00000400
> >>
> >> I substituted SUPPORTS for HAS and dropped the REQUIRES_REQUESTS capability.
> >> As Tomasz mentioned, technically (at least for stateless codecs) you could
> >> handle just one frame at a time without using requests. It's very inefficient,
> >> but it would work.
> >
> > I thought the request was providing a data structure to refer back to
> > the frames, so each codec don't have to implement one. Do you mean that
> > the framework will implicitly request requests in that mode ? or simply
> > that there is no such helper ?
>
> Yes, that's done through controls as well.
>
> The idea would be that you set the necessary controls, queue a buffer to
> the output queue, dequeue a buffer from the output queue, read back any
> new state information and repeat the process.
>
> That said, I'm not sure if the cedrus driver for example can handle this
> at the moment. It is also inefficient and it won't work if codecs require
> more than one buffer in the queue for whatever reason.
>
> Tomasz, Paul, please correct me if I am wrong.
>
> In any case, I think we can do without this proposed capability since it is
> simply a requirement when implementing the pixelformat for the stateless
> codec that the Request API will be available and it should be documented
> as such in the spec.

No correction needed. :)

Best regards,
Tomasz

Re: [RFC] Request API and V4L2 capabilities

[Hans Verkuil]

On 08/29/2018 06:30 AM, Tomasz Figa wrote:
> On Fri, Aug 24, 2018 at 2:33 AM Nicolas Dufresne <nicolas@ndufresne.ca> wrote:
>>
>> Le jeudi 23 août 2018 à 10:05 +0200, Paul Kocialkowski a écrit :
>>> Hi,
>>>
>>> On Wed, 2018-08-22 at 14:33 -0300, Ezequiel Garcia wrote:
>>>> On Wed, 2018-08-22 at 16:10 +0200, Paul Kocialkowski wrote:
>>>>> Hi,
>>>>>
>>>>> On Tue, 2018-08-21 at 17:52 +0900, Tomasz Figa wrote:
>>>>>> Hi Hans, Paul,
>>>>>>
>>>>>> On Mon, Aug 6, 2018 at 6:29 PM Paul Kocialkowski
>>>>>> <paul.kocialkowski@bootlin.com> wrote:
>>>>>>>
>>>>>>> On Mon, 2018-08-06 at 11:23 +0200, Hans Verkuil wrote:
>>>>>>>> On 08/06/2018 11:13 AM, Paul Kocialkowski wrote:
>>>>>>>>> Hi,
>>>>>>>>>
>>>>>>>>> On Mon, 2018-08-06 at 10:32 +0200, Hans Verkuil wrote:
>>>>>>>>>> On 08/06/2018 10:16 AM, Paul Kocialkowski wrote:
>>>>>>>>>>> On Sat, 2018-08-04 at 15:50 +0200, Hans Verkuil wrote:
>>>>>>>>>>>> Regarding point 3: I think this should be documented next to the pixel format. I.e.
>>>>>>>>>>>> the MPEG-2 Slice format used by the stateless cedrus codec requires the request API
>>>>>>>>>>>> and that two MPEG-2 controls (slice params and quantization matrices) must be present
>>>>>>>>>>>> in each request.
>>>>>>>>>>>>
>>>>>>>>>>>> I am not sure a control flag (e.g. V4L2_CTRL_FLAG_REQUIRED_IN_REQ) is needed here.
>>>>>>>>>>>> It's really implied by the fact that you use a stateless codec. It doesn't help
>>>>>>>>>>>> generic applications like v4l2-ctl or qv4l2 either since in order to support
>>>>>>>>>>>> stateless codecs they will have to know about the details of these controls anyway.
>>>>>>>>>>>>
>>>>>>>>>>>> So I am inclined to say that it is not necessary to expose this information in
>>>>>>>>>>>> the API, but it has to be documented together with the pixel format documentation.
>>>>>>>>>>>
>>>>>>>>>>> I think this is affected by considerations about codec profile/level
>>>>>>>>>>> support. More specifically, some controls will only be required for
>>>>>>>>>>> supporting advanced codec profiles/levels, so they can only be
>>>>>>>>>>> explicitly marked with appropriate flags by the driver when the target
>>>>>>>>>>> profile/level is known. And I don't think it would be sane for userspace
>>>>>>>>>>> to explicitly set what profile/level it's aiming at. As a result, I
>>>>>>>>>>> don't think we can explicitly mark controls as required or optional.
>>>>>>
>>>>>> I'm not sure this is entirely true. The hardware may need to be
>>>>>> explicitly told what profile the video is. It may even not be the
>>>>>> hardware, but the driver itself too, given that the profile may imply
>>>>>> the CAPTURE pixel format, e.g. for VP9 profiles:
>>>>>>
>>>>>> profile 0
>>>>>> color depth: 8 bit/sample, chroma subsampling: 4:2:0
>>>>>> profile 1
>>>>>> color depth: 8 bit, chroma subsampling: 4:2:0, 4:2:2, 4:4:4
>>>>>> profile 2
>>>>>> color depth: 10–12 bit, chroma subsampling: 4:2:0
>>>>>> profile 3
>>>>>> color depth: 10–12 bit, chroma subsampling: 4:2:0, 4:2:2, 4:4:4
>>>>>>
>>>>>> (reference: https://en.wikipedia.org/wiki/VP9#Profiles)
>>>>>
>>>>> I think it would be fair to expect userspace to select the right
>>>>> destination format (and maybe have the driver error if there's a
>>>>> mismatch with the meta-data) instead of having the driver somewhat
>>>>> expose what format should be used.
>>>>>
>>>>> But maybe this would be an API violation, since all the enumerated
>>>>> formats are probably supposed to be selectable?
>>>>>
>>>>> We could also look at it the other way round and consider that selecting
>>>>> an exposed format is always legit, but that it implies passing a
>>>>> bitstream that matches it or the driver will error (because of an
>>>>> invalid bitstream passed, not because of a "wrong" selected format).
>>>>>
>>>>
>>>> The API requires the user to negotiate via TRY_FMT/S_FMT. The driver
>>>> usually does not return error on invalid formats, and simply return
>>>> a format it can work with. I think the kernel-user contract has to
>>>> guarantee if the driver accepted a given format, it won't fail to
>>>> encoder or decode.
>>>
>>> Well, the issue here is that in order to correctly enumerate the
>>> formats, the driver needs to be aware of:
>>> 1. in what destination format the bitstream data is decoded to;
>>
>> This is covered by the state-full specification patch if I remember
>> correctly. So the driver, if it's a multi-format, will first return all
>> possible formats, and later on, will return the proper subset. So let's
>> take an encoder, after setting the capture format, the enumeration of
>> the raw formats could then be limited to what is supported for this
>> output. For an H264 encoder, what could also affect this list is the
>> profile that being set. For decoder, this list is reduced after
>> sufficient headers information has been given. Though for decoder it's
>> also limited case, since it only apply to decoder that do a conversion
>> before releasing the output buffer (like CODA does).
>>
>> What isn't so nice with this approach, is that it adds an implicit
>> uninitialized state after open() which isn't usual to the V4L2 API and
>> might not be that trivial or nice to handle in drivers.
> 
> I don't think we defined it this way and as you pointed, it's against
> the general V4L2 API semantics. After open, there is a default coded
> format set and ENUM_FMT/FRAME_SIZES returns what's available for the
> default format.

Right. The list of enumerated formats must be valid. So if the list depends
on the profile/level in a decoder, then there are two cases: either we know
up front what the profile/level of the stream is, and we can set the
controls accordingly and get a valid list of formats. If it turns out that
the stream actually uses a different profile/level, then that probably means
userspace has to stop streaming, re-alloc buffers and restart streaming.

If userspace doesn't know this, then the driver needs to know this so it
can select a set of formats that work for all profiles/levels.

It's been suggested before, but drivers can provide bitmask controls
with the set of profiles/levels that they support. By default all bits
of the supported profiles/levels are set to 1. But userspace can set it
to another value if it knows the set of profiles/levels that can be
supported by the stream it wants to decode. This will update the list
of supported formats.

The same should be done with any other controls that impact the format
list.

Regards,

	Hans

> 
>>
>>> 2. what format convesions the VPU can do.
>>
>> whenever possible, exposing the color conversion as a seperate m2m
>> driver is better approach. Makes the driver simpler and avoids having
>> to add this double enumeration support.
> 
> How about the overhead of having the decoded data stored to memory
> once, read once and then written yet again, even though the hardware
> supports doing it on the fly?
> 
>>
>>>
>>> Step 1 is known by userspace but is only passed to the driver with the
>>> bitstream metadata from controls. This is much too late for trimming the
>>> list of supported formats.
>>>
>>> I'm not sure that passing an extra information to the driver early to
>>> trim the list would make sense, because it comes to down to telling the
>>> driver what format to target and then asking the driver was format it
>>> supports, which is rather redundant.
>>>
>>> I think the information we need to expose to userspace is whether the
>>> driver supports a destination format that does not match the bitstream
>>> format. We could make it part of the spec that, by lack of this
>>> indication, the provided bitstream is expected to match the format that
>>> was selected.
>>
>> I'm not sure why you consider this too late. With decoder, the OUTPUT
>> and CAPTURE stream is asynchronous. So we start streaming on the OUTPUT
>> until the driver notify (V4L2_EVENT_SOURCE_CHANGE). We then enumerate
>> the formats again at that moment, and then configure and start the
>> CAPTURE.
> 
> Yeah, I think we may eventually need this kind of initialization
> sequence, similar to stateful hardware...
> 
> Best regards,
> Tomasz
>