Hi Fritz, On 2/22/22 16:10, Fritz Koenig wrote: > On Thu, Feb 17, 2022 at 9:35 AM Hans Verkuil <hverkuil@xxxxxxxxx> 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@xxxxxxxxx> >> --- >> 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));
