Venus v4l2-compliance failures

Venus v4l2-compliance failures

[Stanimir Varbanov]

Hi Hans,

Presently we have two failures while running v4l2-compliance on venus
stateful decoder:

1. fail: v4l2-compliance.cpp(753): !ok
        test for unlimited opens: FAIL

2. fail: v4l2-test-formats.cpp(1668): IS_DECODER(node)
        test Cropping: FAIL
   fail: v4l2-test-codecs.cpp(104): node->function !=
MEDIA_ENT_F_PROC_VIDEO_DECODER
        test VIDIOC_(TRY_)DECODER_CMD: FAIL

Failure #1 is related to the limitation we made in decoder open(). The
maximum parallel decoding sessions is limited to 16 and the check
for this maximum is made in decoder open() because the clients wanted to
know that earlier. For example, Chromium browser can open 16 hw
accelerated decoder sessions (in separate Tabs) and from 17 and upward
it will fallback to sw decoder. I wonder how that failure can be fixed.


Failure #2 is related to a commit [1] which add checks for
MEDIA_ENT_F_PROC_VIDEO_ENCODER, I think this entity flag is applicable
for stateless encoders (Request API) but Venus driver has no use of
media-controller API. Did I miss something?


[1]
https://git.linuxtv.org/v4l-utils.git/commit/?id=493af03f3c576fad69c050d33215d1f4fc0d532d

-- 
regards,
Stan

Re: Venus v4l2-compliance failures

[Hans Verkuil]

On 17/02/2022 15:12, Stanimir Varbanov wrote:
> Hi Hans,
> 
> Presently we have two failures while running v4l2-compliance on venus
> stateful decoder:
> 
> 1. fail: v4l2-compliance.cpp(753): !ok
>         test for unlimited opens: FAIL
> 
> 2. fail: v4l2-test-formats.cpp(1668): IS_DECODER(node)
>         test Cropping: FAIL
>    fail: v4l2-test-codecs.cpp(104): node->function !=
> MEDIA_ENT_F_PROC_VIDEO_DECODER
>         test VIDIOC_(TRY_)DECODER_CMD: FAIL
> 
> Failure #1 is related to the limitation we made in decoder open(). The
> maximum parallel decoding sessions is limited to 16 and the check
> for this maximum is made in decoder open() because the clients wanted to
> know that earlier. For example, Chromium browser can open 16 hw
> accelerated decoder sessions (in separate Tabs) and from 17 and upward
> it will fallback to sw decoder. I wonder how that failure can be fixed.

I'm wondering if this isn't better done via a read-only control that
reports the max number of parallel sessions.

I really hate artificial open() limitations, it's very much against the
v4l2 philosophy.

Reporting it with a standard control makes it also much easier for software
to anticipate when it needs to switch to SW en/decoding.

> 
> 
> Failure #2 is related to a commit [1] which add checks for
> MEDIA_ENT_F_PROC_VIDEO_ENCODER, I think this entity flag is applicable
> for stateless encoders (Request API) but Venus driver has no use of
> media-controller API. Did I miss something?

For item 2, can you try the patch below?

Regards,

	Hans

-----------------------------------------------------------------------

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
diff --git a/utils/v4l2-compliance/v4l2-test-codecs.cpp b/utils/v4l2-compliance/v4l2-test-codecs.cpp
index 22175eef..3f06070f 100644
--- a/utils/v4l2-compliance/v4l2-test-codecs.cpp
+++ b/utils/v4l2-compliance/v4l2-test-codecs.cpp
@@ -29,9 +29,10 @@ int testEncoder(struct node *node)
 {
 	struct v4l2_encoder_cmd cmd;
 	bool is_encoder = node->codec_mask & (STATEFUL_ENCODER | JPEG_ENCODER);
+	bool is_stateless_encoder = node->codec_mask & STATELESS_ENCODER;
 	int ret;

-	if (IS_ENCODER(node))
+	if (is_stateless_encoder)
 		fail_on_test(node->function != MEDIA_ENT_F_PROC_VIDEO_ENCODER);
 	memset(&cmd, 0xff, sizeof(cmd));
 	memset(&cmd.raw, 0, sizeof(cmd.raw));
@@ -98,9 +99,10 @@ int testDecoder(struct node *node)
 {
 	struct v4l2_decoder_cmd cmd;
 	bool is_decoder = node->codec_mask & (STATEFUL_DECODER | JPEG_DECODER);
+	bool is_stateless_decoder = node->codec_mask & STATELESS_DECODER;
 	int ret;

-	if (IS_DECODER(node))
+	if (is_stateless_decoder)
 		fail_on_test(node->function != MEDIA_ENT_F_PROC_VIDEO_DECODER);
 	memset(&cmd, 0xff, sizeof(cmd));
 	memset(&cmd.raw, 0, sizeof(cmd.raw));

Re: Venus v4l2-compliance failures

[Nícolas F. R. A. Prado]

Hi Hans,

On Thu, Feb 17, 2022 at 03:34:51PM +0100, Hans Verkuil wrote:
> On 17/02/2022 15:12, Stanimir Varbanov wrote:
> > Failure #2 is related to a commit [1] which add checks for
> > MEDIA_ENT_F_PROC_VIDEO_ENCODER, I think this entity flag is applicable
> > for stateless encoders (Request API) but Venus driver has no use of
> > media-controller API. Did I miss something?
> 
> For item 2, can you try the patch below?

I faced the same issue with the mtk-vcodec-enc driver on mt8173-elm-hana. The
patch below does fix the test for me, so

Tested-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>

Thanks,
Nícolas

> 
> Regards,
> 
> 	Hans
> 
> -----------------------------------------------------------------------
> 
> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> ---
> diff --git a/utils/v4l2-compliance/v4l2-test-codecs.cpp b/utils/v4l2-compliance/v4l2-test-codecs.cpp
> index 22175eef..3f06070f 100644
> --- a/utils/v4l2-compliance/v4l2-test-codecs.cpp
> +++ b/utils/v4l2-compliance/v4l2-test-codecs.cpp
> @@ -29,9 +29,10 @@ int testEncoder(struct node *node)
>  {
>  	struct v4l2_encoder_cmd cmd;
>  	bool is_encoder = node->codec_mask & (STATEFUL_ENCODER | JPEG_ENCODER);
> +	bool is_stateless_encoder = node->codec_mask & STATELESS_ENCODER;
>  	int ret;
> 
> -	if (IS_ENCODER(node))
> +	if (is_stateless_encoder)
>  		fail_on_test(node->function != MEDIA_ENT_F_PROC_VIDEO_ENCODER);
>  	memset(&cmd, 0xff, sizeof(cmd));
>  	memset(&cmd.raw, 0, sizeof(cmd.raw));
> @@ -98,9 +99,10 @@ int testDecoder(struct node *node)
>  {
>  	struct v4l2_decoder_cmd cmd;
>  	bool is_decoder = node->codec_mask & (STATEFUL_DECODER | JPEG_DECODER);
> +	bool is_stateless_decoder = node->codec_mask & STATELESS_DECODER;
>  	int ret;
> 
> -	if (IS_DECODER(node))
> +	if (is_stateless_decoder)
>  		fail_on_test(node->function != MEDIA_ENT_F_PROC_VIDEO_DECODER);
>  	memset(&cmd, 0xff, sizeof(cmd));
>  	memset(&cmd.raw, 0, sizeof(cmd.raw));
> 

Re: Venus v4l2-compliance failures

[Hans Verkuil]

Hi Nicolas,

On 18/02/2022 03:24, Nícolas F. R. A. Prado wrote:
> Hi Hans,
> 
> On Thu, Feb 17, 2022 at 03:34:51PM +0100, Hans Verkuil wrote:
>> On 17/02/2022 15:12, Stanimir Varbanov wrote:
>>> Failure #2 is related to a commit [1] which add checks for
>>> MEDIA_ENT_F_PROC_VIDEO_ENCODER, I think this entity flag is applicable
>>> for stateless encoders (Request API) but Venus driver has no use of
>>> media-controller API. Did I miss something?
>>
>> For item 2, can you try the patch below?
> 
> I faced the same issue with the mtk-vcodec-enc driver on mt8173-elm-hana. The
> patch below does fix the test for me, so
> 
> Tested-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>

Thank you. But I've posted a v2 that is a bit smarter (i.e. it only skips the
test for stateful codecs without an MC).

If you can give it a quick spin, then that would be great.

Regards,

	Hans

> 
> Thanks,
> Nícolas
> 
>>
>> Regards,
>>
>> 	Hans
>>
>> -----------------------------------------------------------------------
>>
>> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
>> ---
>> diff --git a/utils/v4l2-compliance/v4l2-test-codecs.cpp b/utils/v4l2-compliance/v4l2-test-codecs.cpp
>> index 22175eef..3f06070f 100644
>> --- a/utils/v4l2-compliance/v4l2-test-codecs.cpp
>> +++ b/utils/v4l2-compliance/v4l2-test-codecs.cpp
>> @@ -29,9 +29,10 @@ int testEncoder(struct node *node)
>>  {
>>  	struct v4l2_encoder_cmd cmd;
>>  	bool is_encoder = node->codec_mask & (STATEFUL_ENCODER | JPEG_ENCODER);
>> +	bool is_stateless_encoder = node->codec_mask & STATELESS_ENCODER;
>>  	int ret;
>>
>> -	if (IS_ENCODER(node))
>> +	if (is_stateless_encoder)
>>  		fail_on_test(node->function != MEDIA_ENT_F_PROC_VIDEO_ENCODER);
>>  	memset(&cmd, 0xff, sizeof(cmd));
>>  	memset(&cmd.raw, 0, sizeof(cmd.raw));
>> @@ -98,9 +99,10 @@ int testDecoder(struct node *node)
>>  {
>>  	struct v4l2_decoder_cmd cmd;
>>  	bool is_decoder = node->codec_mask & (STATEFUL_DECODER | JPEG_DECODER);
>> +	bool is_stateless_decoder = node->codec_mask & STATELESS_DECODER;
>>  	int ret;
>>
>> -	if (IS_DECODER(node))
>> +	if (is_stateless_decoder)
>>  		fail_on_test(node->function != MEDIA_ENT_F_PROC_VIDEO_DECODER);
>>  	memset(&cmd, 0xff, sizeof(cmd));
>>  	memset(&cmd.raw, 0, sizeof(cmd.raw));
>>

Re: Venus v4l2-compliance failures

[Fritz Koenig]

On Thu, Feb 17, 2022 at 9:35 AM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>
> On 17/02/2022 15:12, Stanimir Varbanov wrote:
> > Hi Hans,
> >
> > Presently we have two failures while running v4l2-compliance on venus
> > stateful decoder:
> >
> > 1. fail: v4l2-compliance.cpp(753): !ok
> >         test for unlimited opens: FAIL
> >
> > 2. fail: v4l2-test-formats.cpp(1668): IS_DECODER(node)
> >         test Cropping: FAIL
> >    fail: v4l2-test-codecs.cpp(104): node->function !=
> > MEDIA_ENT_F_PROC_VIDEO_DECODER
> >         test VIDIOC_(TRY_)DECODER_CMD: FAIL
> >
> > Failure #1 is related to the limitation we made in decoder open(). The
> > maximum parallel decoding sessions is limited to 16 and the check
> > for this maximum is made in decoder open() because the clients wanted to
> > know that earlier. For example, Chromium browser can open 16 hw
> > accelerated decoder sessions (in separate Tabs) and from 17 and upward
> > it will fallback to sw decoder. I wonder how that failure can be fixed.
>
> I'm wondering if this isn't better done via a read-only control that
> reports the max number of parallel sessions.
>
Do you see this as a constant value?  It would be burdensome if the
client had to keep track of how many contexts are in use.  Or do you
see this as a number of currently available contexts?

> I really hate artificial open() limitations, it's very much against the
> v4l2 philosophy.
>
From a client stand point it just seems like extra round trips.
Everytime the device is opened another call needs to be made right
away to check and see if there are resources available.

If that's the philosophy, the client can adapt.  If the control was
queried and it returned 0 for the number of available contexts, then
the handle could be closed and then a sw codec could be used instead.

> Reporting it with a standard control makes it also much easier for software
> to anticipate when it needs to switch to SW en/decoding.
>
I think you are talking about the codec controls[1], correct? I didn't
see an existing control present that would report the number of
currently open contexts and/or the number of maximum contexts.

[1]: https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/ext-ctrls-codec.html
> >
> >
> > Failure #2 is related to a commit [1] which add checks for
> > MEDIA_ENT_F_PROC_VIDEO_ENCODER, I think this entity flag is applicable
> > for stateless encoders (Request API) but Venus driver has no use of
> > media-controller API. Did I miss something?
>
> For item 2, can you try the patch below?
>
> Regards,
>
>         Hans
>
> -----------------------------------------------------------------------
>
> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> ---
> diff --git a/utils/v4l2-compliance/v4l2-test-codecs.cpp b/utils/v4l2-compliance/v4l2-test-codecs.cpp
> index 22175eef..3f06070f 100644
> --- a/utils/v4l2-compliance/v4l2-test-codecs.cpp
> +++ b/utils/v4l2-compliance/v4l2-test-codecs.cpp
> @@ -29,9 +29,10 @@ int testEncoder(struct node *node)
>  {
>         struct v4l2_encoder_cmd cmd;
>         bool is_encoder = node->codec_mask & (STATEFUL_ENCODER | JPEG_ENCODER);
> +       bool is_stateless_encoder = node->codec_mask & STATELESS_ENCODER;
>         int ret;
>
> -       if (IS_ENCODER(node))
> +       if (is_stateless_encoder)
>                 fail_on_test(node->function != MEDIA_ENT_F_PROC_VIDEO_ENCODER);
>         memset(&cmd, 0xff, sizeof(cmd));
>         memset(&cmd.raw, 0, sizeof(cmd.raw));
> @@ -98,9 +99,10 @@ int testDecoder(struct node *node)
>  {
>         struct v4l2_decoder_cmd cmd;
>         bool is_decoder = node->codec_mask & (STATEFUL_DECODER | JPEG_DECODER);
> +       bool is_stateless_decoder = node->codec_mask & STATELESS_DECODER;
>         int ret;
>
> -       if (IS_DECODER(node))
> +       if (is_stateless_decoder)
>                 fail_on_test(node->function != MEDIA_ENT_F_PROC_VIDEO_DECODER);
>         memset(&cmd, 0xff, sizeof(cmd));
>         memset(&cmd.raw, 0, sizeof(cmd.raw));

Re: Venus v4l2-compliance failures

[Hans Verkuil]

Hi Fritz,

On 2/22/22 16:10, Fritz Koenig wrote:
> On Thu, Feb 17, 2022 at 9:35 AM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>>
>> On 17/02/2022 15:12, Stanimir Varbanov wrote:
>>> Hi Hans,
>>>
>>> Presently we have two failures while running v4l2-compliance on venus
>>> stateful decoder:
>>>
>>> 1. fail: v4l2-compliance.cpp(753): !ok
>>>         test for unlimited opens: FAIL
>>>
>>> 2. fail: v4l2-test-formats.cpp(1668): IS_DECODER(node)
>>>         test Cropping: FAIL
>>>    fail: v4l2-test-codecs.cpp(104): node->function !=
>>> MEDIA_ENT_F_PROC_VIDEO_DECODER
>>>         test VIDIOC_(TRY_)DECODER_CMD: FAIL
>>>
>>> Failure #1 is related to the limitation we made in decoder open(). The
>>> maximum parallel decoding sessions is limited to 16 and the check
>>> for this maximum is made in decoder open() because the clients wanted to
>>> know that earlier. For example, Chromium browser can open 16 hw
>>> accelerated decoder sessions (in separate Tabs) and from 17 and upward
>>> it will fallback to sw decoder. I wonder how that failure can be fixed.
>>
>> I'm wondering if this isn't better done via a read-only control that
>> reports the max number of parallel sessions.
>>
> Do you see this as a constant value?  It would be burdensome if the
> client had to keep track of how many contexts are in use.  Or do you
> see this as a number of currently available contexts?

I haven't really thought about it. It probably depends on the HW design:
i.e. it might be a hard limit as per the number of instances, or more
of a resource (bandwidth/memory) limitation that also depends on the
bitrate etc. From what I gather it is a hard limit to the number of
instances in the case of the venus driver. So here it would be a read-only
control that has a constant value.

> 
>> I really hate artificial open() limitations, it's very much against the
>> v4l2 philosophy.
>>
> From a client stand point it just seems like extra round trips.
> Everytime the device is opened another call needs to be made right
> away to check and see if there are resources available.
> 
> If that's the philosophy, the client can adapt.  If the control was
> queried and it returned 0 for the number of available contexts, then
> the handle could be closed and then a sw codec could be used instead.
> 
>> Reporting it with a standard control makes it also much easier for software
>> to anticipate when it needs to switch to SW en/decoding.
>>
> I think you are talking about the codec controls[1], correct? I didn't
> see an existing control present that would report the number of
> currently open contexts and/or the number of maximum contexts.

Yes, this would be a new codec control.

Regards,

	Hans

> 
> [1]: https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/ext-ctrls-codec.html
>>>
>>>
>>> Failure #2 is related to a commit [1] which add checks for
>>> MEDIA_ENT_F_PROC_VIDEO_ENCODER, I think this entity flag is applicable
>>> for stateless encoders (Request API) but Venus driver has no use of
>>> media-controller API. Did I miss something?
>>
>> For item 2, can you try the patch below?
>>
>> Regards,
>>
>>         Hans
>>
>> -----------------------------------------------------------------------
>>
>> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
>> ---
>> diff --git a/utils/v4l2-compliance/v4l2-test-codecs.cpp b/utils/v4l2-compliance/v4l2-test-codecs.cpp
>> index 22175eef..3f06070f 100644
>> --- a/utils/v4l2-compliance/v4l2-test-codecs.cpp
>> +++ b/utils/v4l2-compliance/v4l2-test-codecs.cpp
>> @@ -29,9 +29,10 @@ int testEncoder(struct node *node)
>>  {
>>         struct v4l2_encoder_cmd cmd;
>>         bool is_encoder = node->codec_mask & (STATEFUL_ENCODER | JPEG_ENCODER);
>> +       bool is_stateless_encoder = node->codec_mask & STATELESS_ENCODER;
>>         int ret;
>>
>> -       if (IS_ENCODER(node))
>> +       if (is_stateless_encoder)
>>                 fail_on_test(node->function != MEDIA_ENT_F_PROC_VIDEO_ENCODER);
>>         memset(&cmd, 0xff, sizeof(cmd));
>>         memset(&cmd.raw, 0, sizeof(cmd.raw));
>> @@ -98,9 +99,10 @@ int testDecoder(struct node *node)
>>  {
>>         struct v4l2_decoder_cmd cmd;
>>         bool is_decoder = node->codec_mask & (STATEFUL_DECODER | JPEG_DECODER);
>> +       bool is_stateless_decoder = node->codec_mask & STATELESS_DECODER;
>>         int ret;
>>
>> -       if (IS_DECODER(node))
>> +       if (is_stateless_decoder)
>>                 fail_on_test(node->function != MEDIA_ENT_F_PROC_VIDEO_DECODER);
>>         memset(&cmd, 0xff, sizeof(cmd));
>>         memset(&cmd.raw, 0, sizeof(cmd.raw));

Re: Venus v4l2-compliance failures

[Fritz Koenig]

Hi Hans,

On Tue, Feb 22, 2022 at 10:50 AM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>
> Hi Fritz,
>
> On 2/22/22 16:10, Fritz Koenig wrote:
> > On Thu, Feb 17, 2022 at 9:35 AM Hans Verkuil <hverkuil@xs4all.nl> wrote:
> >>
> >> On 17/02/2022 15:12, Stanimir Varbanov wrote:
> >>> Hi Hans,
> >>>
> >>> Presently we have two failures while running v4l2-compliance on venus
> >>> stateful decoder:
> >>>
> >>> 1. fail: v4l2-compliance.cpp(753): !ok
> >>>         test for unlimited opens: FAIL
> >>>
> >>> 2. fail: v4l2-test-formats.cpp(1668): IS_DECODER(node)
> >>>         test Cropping: FAIL
> >>>    fail: v4l2-test-codecs.cpp(104): node->function !=
> >>> MEDIA_ENT_F_PROC_VIDEO_DECODER
> >>>         test VIDIOC_(TRY_)DECODER_CMD: FAIL
> >>>
> >>> Failure #1 is related to the limitation we made in decoder open(). The
> >>> maximum parallel decoding sessions is limited to 16 and the check
> >>> for this maximum is made in decoder open() because the clients wanted to
> >>> know that earlier. For example, Chromium browser can open 16 hw
> >>> accelerated decoder sessions (in separate Tabs) and from 17 and upward
> >>> it will fallback to sw decoder. I wonder how that failure can be fixed.
> >>
> >> I'm wondering if this isn't better done via a read-only control that
> >> reports the max number of parallel sessions.
> >>
> > Do you see this as a constant value?  It would be burdensome if the
> > client had to keep track of how many contexts are in use.  Or do you
> > see this as a number of currently available contexts?
>
> I haven't really thought about it. It probably depends on the HW design:
> i.e. it might be a hard limit as per the number of instances, or more
> of a resource (bandwidth/memory) limitation that also depends on the
> bitrate etc. From what I gather it is a hard limit to the number of
> instances in the case of the venus driver. So here it would be a read-only
> control that has a constant value.
>
I might be misunderstanding you here.  In my mind a constant value
read-only control would be difficult to use with a multiprocess
system.  The client would read how many contexts could be open, but
wouldn't be easily able to track how many are currently in use.

I see a control that could return the number of contexts currently in
use, and the maximum number available.  I think that would be
preferable to a control that only returns the number of currently
available contexts.  But maybe that is a nuance of the client or the
driver doing the subtraction.

-Fritz

> >
> >> I really hate artificial open() limitations, it's very much against the
> >> v4l2 philosophy.
> >>
> > From a client stand point it just seems like extra round trips.
> > Everytime the device is opened another call needs to be made right
> > away to check and see if there are resources available.
> >
> > If that's the philosophy, the client can adapt.  If the control was
> > queried and it returned 0 for the number of available contexts, then
> > the handle could be closed and then a sw codec could be used instead.
> >
> >> Reporting it with a standard control makes it also much easier for software
> >> to anticipate when it needs to switch to SW en/decoding.
> >>
> > I think you are talking about the codec controls[1], correct? I didn't
> > see an existing control present that would report the number of
> > currently open contexts and/or the number of maximum contexts.
>
> Yes, this would be a new codec control.
>
> Regards,
>
>         Hans
>
> >
> > [1]: https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/ext-ctrls-codec.html
> >>>
> >>>
> >>> Failure #2 is related to a commit [1] which add checks for
> >>> MEDIA_ENT_F_PROC_VIDEO_ENCODER, I think this entity flag is applicable
> >>> for stateless encoders (Request API) but Venus driver has no use of
> >>> media-controller API. Did I miss something?
> >>
> >> For item 2, can you try the patch below?
> >>
> >> Regards,
> >>
> >>         Hans
> >>
> >> -----------------------------------------------------------------------
> >>
> >> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> >> ---
> >> diff --git a/utils/v4l2-compliance/v4l2-test-codecs.cpp b/utils/v4l2-compliance/v4l2-test-codecs.cpp
> >> index 22175eef..3f06070f 100644
> >> --- a/utils/v4l2-compliance/v4l2-test-codecs.cpp
> >> +++ b/utils/v4l2-compliance/v4l2-test-codecs.cpp
> >> @@ -29,9 +29,10 @@ int testEncoder(struct node *node)
> >>  {
> >>         struct v4l2_encoder_cmd cmd;
> >>         bool is_encoder = node->codec_mask & (STATEFUL_ENCODER | JPEG_ENCODER);
> >> +       bool is_stateless_encoder = node->codec_mask & STATELESS_ENCODER;
> >>         int ret;
> >>
> >> -       if (IS_ENCODER(node))
> >> +       if (is_stateless_encoder)
> >>                 fail_on_test(node->function != MEDIA_ENT_F_PROC_VIDEO_ENCODER);
> >>         memset(&cmd, 0xff, sizeof(cmd));
> >>         memset(&cmd.raw, 0, sizeof(cmd.raw));
> >> @@ -98,9 +99,10 @@ int testDecoder(struct node *node)
> >>  {
> >>         struct v4l2_decoder_cmd cmd;
> >>         bool is_decoder = node->codec_mask & (STATEFUL_DECODER | JPEG_DECODER);
> >> +       bool is_stateless_decoder = node->codec_mask & STATELESS_DECODER;
> >>         int ret;
> >>
> >> -       if (IS_DECODER(node))
> >> +       if (is_stateless_decoder)
> >>                 fail_on_test(node->function != MEDIA_ENT_F_PROC_VIDEO_DECODER);
> >>         memset(&cmd, 0xff, sizeof(cmd));
> >>         memset(&cmd.raw, 0, sizeof(cmd.raw));

Re: Venus v4l2-compliance failures

[Hans Verkuil]

On 2/22/22 17:11, Fritz Koenig wrote:
> Hi Hans,
> 
> On Tue, Feb 22, 2022 at 10:50 AM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>>
>> Hi Fritz,
>>
>> On 2/22/22 16:10, Fritz Koenig wrote:
>>> On Thu, Feb 17, 2022 at 9:35 AM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>>>>
>>>> On 17/02/2022 15:12, Stanimir Varbanov wrote:
>>>>> Hi Hans,
>>>>>
>>>>> Presently we have two failures while running v4l2-compliance on venus
>>>>> stateful decoder:
>>>>>
>>>>> 1. fail: v4l2-compliance.cpp(753): !ok
>>>>>         test for unlimited opens: FAIL
>>>>>
>>>>> 2. fail: v4l2-test-formats.cpp(1668): IS_DECODER(node)
>>>>>         test Cropping: FAIL
>>>>>    fail: v4l2-test-codecs.cpp(104): node->function !=
>>>>> MEDIA_ENT_F_PROC_VIDEO_DECODER
>>>>>         test VIDIOC_(TRY_)DECODER_CMD: FAIL
>>>>>
>>>>> Failure #1 is related to the limitation we made in decoder open(). The
>>>>> maximum parallel decoding sessions is limited to 16 and the check
>>>>> for this maximum is made in decoder open() because the clients wanted to
>>>>> know that earlier. For example, Chromium browser can open 16 hw
>>>>> accelerated decoder sessions (in separate Tabs) and from 17 and upward
>>>>> it will fallback to sw decoder. I wonder how that failure can be fixed.
>>>>
>>>> I'm wondering if this isn't better done via a read-only control that
>>>> reports the max number of parallel sessions.
>>>>
>>> Do you see this as a constant value?  It would be burdensome if the
>>> client had to keep track of how many contexts are in use.  Or do you
>>> see this as a number of currently available contexts?
>>
>> I haven't really thought about it. It probably depends on the HW design:
>> i.e. it might be a hard limit as per the number of instances, or more
>> of a resource (bandwidth/memory) limitation that also depends on the
>> bitrate etc. From what I gather it is a hard limit to the number of
>> instances in the case of the venus driver. So here it would be a read-only
>> control that has a constant value.
>>
> I might be misunderstanding you here.  In my mind a constant value
> read-only control would be difficult to use with a multiprocess
> system.  The client would read how many contexts could be open, but
> wouldn't be easily able to track how many are currently in use.
> 
> I see a control that could return the number of contexts currently in
> use, and the maximum number available.  I think that would be
> preferable to a control that only returns the number of currently
> available contexts.  But maybe that is a nuance of the client or the
> driver doing the subtraction.

Yes, that's actually what I had in mind. A read-only control that
reports the number of instances in use, and the maximum value (as
returned by QUERYCTRL) would be the max number of instances.

Regards,

	Hans

> 
> -Fritz
> 
>>>
>>>> I really hate artificial open() limitations, it's very much against the
>>>> v4l2 philosophy.
>>>>
>>> From a client stand point it just seems like extra round trips.
>>> Everytime the device is opened another call needs to be made right
>>> away to check and see if there are resources available.
>>>
>>> If that's the philosophy, the client can adapt.  If the control was
>>> queried and it returned 0 for the number of available contexts, then
>>> the handle could be closed and then a sw codec could be used instead.
>>>
>>>> Reporting it with a standard control makes it also much easier for software
>>>> to anticipate when it needs to switch to SW en/decoding.
>>>>
>>> I think you are talking about the codec controls[1], correct? I didn't
>>> see an existing control present that would report the number of
>>> currently open contexts and/or the number of maximum contexts.
>>
>> Yes, this would be a new codec control.
>>
>> Regards,
>>
>>         Hans
>>
>>>
>>> [1]: https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/ext-ctrls-codec.html
>>>>>
>>>>>
>>>>> Failure #2 is related to a commit [1] which add checks for
>>>>> MEDIA_ENT_F_PROC_VIDEO_ENCODER, I think this entity flag is applicable
>>>>> for stateless encoders (Request API) but Venus driver has no use of
>>>>> media-controller API. Did I miss something?
>>>>
>>>> For item 2, can you try the patch below?
>>>>
>>>> Regards,
>>>>
>>>>         Hans
>>>>
>>>> -----------------------------------------------------------------------
>>>>
>>>> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
>>>> ---
>>>> diff --git a/utils/v4l2-compliance/v4l2-test-codecs.cpp b/utils/v4l2-compliance/v4l2-test-codecs.cpp
>>>> index 22175eef..3f06070f 100644
>>>> --- a/utils/v4l2-compliance/v4l2-test-codecs.cpp
>>>> +++ b/utils/v4l2-compliance/v4l2-test-codecs.cpp
>>>> @@ -29,9 +29,10 @@ int testEncoder(struct node *node)
>>>>  {
>>>>         struct v4l2_encoder_cmd cmd;
>>>>         bool is_encoder = node->codec_mask & (STATEFUL_ENCODER | JPEG_ENCODER);
>>>> +       bool is_stateless_encoder = node->codec_mask & STATELESS_ENCODER;
>>>>         int ret;
>>>>
>>>> -       if (IS_ENCODER(node))
>>>> +       if (is_stateless_encoder)
>>>>                 fail_on_test(node->function != MEDIA_ENT_F_PROC_VIDEO_ENCODER);
>>>>         memset(&cmd, 0xff, sizeof(cmd));
>>>>         memset(&cmd.raw, 0, sizeof(cmd.raw));
>>>> @@ -98,9 +99,10 @@ int testDecoder(struct node *node)
>>>>  {
>>>>         struct v4l2_decoder_cmd cmd;
>>>>         bool is_decoder = node->codec_mask & (STATEFUL_DECODER | JPEG_DECODER);
>>>> +       bool is_stateless_decoder = node->codec_mask & STATELESS_DECODER;
>>>>         int ret;
>>>>
>>>> -       if (IS_DECODER(node))
>>>> +       if (is_stateless_decoder)
>>>>                 fail_on_test(node->function != MEDIA_ENT_F_PROC_VIDEO_DECODER);
>>>>         memset(&cmd, 0xff, sizeof(cmd));
>>>>         memset(&cmd.raw, 0, sizeof(cmd.raw));