On 12.05.2025 12:38, Greg KH wrote:
On Mon, May 12, 2025 at 12:19:07PM +0200, Krzysztof Opasiak wrote:
Hi Greg,
On 4.12.2022 09:29, Greg KH wrote:
On Sat, Dec 03, 2022 at 11:26:14PM +0200, Laurent Pinchart wrote:
Hi Michael,
On Sat, Sep 10, 2022 at 12:13:31AM +0200, Michael Grzeschik wrote:
This series improves the uvc video gadget by parsing the configfs
entries. With the configfs data, the userspace now is able to use simple
v4l2 api calls like enum and try_format to check for valid configurations
initially set by configfs.
I've realized that this whole series got merged, despite my multiple
attempts to explain why I think it's not a good idea. The UVC gadget
requires userspace support, and there's no point in trying to move all
these things to the kernel side. It only bloats the kernel, makes the
code more complex, more difficult to maintain, and will make UVC 1.5
support more difficult.
I can easily revert them, but I did not see any objections to them
originally and so I merged them as is the normal method :)
I know that it's already 2025 and I'm very late to the game but this series
breaks our userspace scripts as it implicitly adds a requirement to name a
streaming header directory as "h" which previously was a user-selected name.
This requirement is coming from here:
+
+ streaming = config_group_find_item(&opts->func_inst.group,
"streaming");
+ if (!streaming)
+ goto err_config;
+
+ header = config_group_find_item(to_config_group(streaming),
"header");
+ config_item_put(streaming);
+ if (!header)
+ goto err_config;
+
+ h = config_group_find_item(to_config_group(header), "h");
+ config_item_put(header);
+ if (!h)
+ goto err_config;
If you name this directory as "header" gadget just fails to link to a
configuration. Although this isn't a big deal on its own as we could simply
rename the dir in our code but this is just the tip of the iceberg.
To my understanding, this patch broke an important feature of UVC ConfigFS
interface which is allowing to present different list of available formats
for different USB speeds as for example, it does not make sense to expose
1080p30 in high speed as it simply just does not fit into the ISO bandwidth
of HS. For example what we've been using previously:
mkdir uvc.nf/streaming/uncompressed/hsu
mkdir uvc.nf/streaming/uncompressed/hsu/360p
mkdir uvc.nf/streaming/uncompressed/ssu
mkdir uvc.nf/streaming/uncompressed/ssu/360p
mkdir uvc.nf/streaming/uncompressed/ssu/720p
mkdir uvc.nf/streaming/uncompressed/ssu/1080p
symlink uvc.nf/streaming/uncompressed/hsu \
uvc.nf/streaming/header/hsh/hsu
symlink uvc.nf/streaming/uncompressed/ssu \
uvc.nf/streaming/header/ssh/hsu
symlink uvc.nf/streaming/header/hsh \
uvc.nf/streaming/class/hs/h
symlink uvc.nf/streaming/header/ssh \
uvc.nf/streaming/class/ss/h
no longer works as the patch requires presence of "h" directory inside
"streaming/header" and even if we rename one of the headers directory to h
the patch would be actually wrong as it would expose via v4l2 calls only
formats available in the "h" directory and not in any other. (and at least
on our adroid kernel it makes the kernel crash but haven't tested if that
would be the case for mainline as well)
Given the limitations of the v4l2 interface I kind of don't see a way how we
could fix this series to present proper formats to the userspace using v4l2
calls as the list of formats can change when the speed changes and userspace
would have no way of knowing that.
Given that I'd like to suggest that it seems to actually make sense to
revert this unless there are some ideas how to fix it.
Sorry about this, can you submit a patch series that reverts the
offending commits? As it was years ago, I don't exactly know what you
are referring to anymore.
Sure! Will do.
--
Krzysztof Opasiak | R&D Team
neat.
