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