Re: [PATCH v8 4/4] usb: gadget: uvc: add format/frame handling code

[Michael Grzeschik <mgr@pengutronix.de>]

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

On Wed, Sep 07, 2022 at 06:32:09PM +0300, Laurent Pinchart wrote:
>Hi Michael,
>
>On Wed, Sep 07, 2022 at 05:15:16PM +0200, Michael Grzeschik wrote:
>> On Wed, Sep 07, 2022 at 06:07:34PM +0300, Laurent Pinchart wrote:
>> > On Wed, Sep 07, 2022 at 04:02:54PM +0200, Michael Grzeschik wrote:
>> >> The Hostside format selection is currently only done in userspace, as
>> >> the events for SET_CUR and GET_CUR are always moved to the application
>> >> layer. Since the v4l2 device parses the configfs data, the format
>> >> negotiation can be done in the kernel. This patch adds the functions to
>> >> set the current configuration while continuing to forward all unknown
>> >> events to the userspace level.
>> >
>> > Why do you think this is better done in kernel space, given that
>> > userspace has to process the event anyway ? It's more complexity in the
>> > kernel side, for little added value as far as I can see. It will also
>> > make it more difficult to handle different UVC versions (in particular
>> > UVC 1.5). I'd rather not go in this direction if there's no clear
>> > benefit.
>>
>> The current case is to set configfs from userspace to ensure
>> the host sees only what we configured. Then the userspace has to parse
>> this configfs again, to be sure not to allow something on
>> SET_CUR/GET_CUR that is not valid from configfs configuration. Since the
>> configfs-Setup could be done from another application comming from
>> somewhere in the userspace this limit will always stay.
>
>That really depends on the architecture of the userspace stack, when the
>same userspace application configures the gadget and handles the runtime
>operations, it won't have to parse configfs. I'd also argue that in
>practical use cases, the application will likely need to know the list
>of formats and resolutions that are exposed by the gadget in order to
>prepare for supporting them properly (for instance, allocating buffers
>large enough to store the largest resolution is common when you want to
>lower stream start delays if your system is not memory-constrained).

The userspace can for sure can do resolution and format parsing that
is exposed by the gadget. Even more standardized, now with the simple
v4l2 API, without having to parse the whole configfs again.

>> Since the kernel knows the configfs-setup it was given in the beginning
>> it can ensure that SET_CUR/GET_CUR will only handle valid setups.
>>
>> When done in Kernel, we can also use simple v4l2 API calls like try_format to
>> ask the driver what the host side did configure. So we can use simple
>> abstracion-libs like gstreamer for this.
>
>This will cause trouble when extending support to UVC 1.5 as the
>complexity will grow on the kernel side. Furthermore, by handling the
>video probe and commit control in kernel space, you'd removing the
>possibility for userspace to decide on how to handle the bHint flags, or
>how to negociate dwFrameInterval key frame rate, compression quality, or
>the additional fields specific to UVC 1.5. This effectively hardcodes
>one particular policy in the driver, and that shouldn't be the role of
>the kernel.

When you look closely, you will see that for later user cases, I decided
the userspace appliaction will still be able to subscribe for DATA and
SETUP.

It will only fall back to the kernel side of operation if there is
nobody that is able to operate. In those cases, it will take over. I
tested this with your uvc-gadget appliaction. Since it subscribes more
than STREAMON and STREAMOFF, it is still working like before with this
patchstack applied.

Regards,
Michael

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]