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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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: signature.asc
Description: PGP signature


[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux