usb: device_next: new USB Video Class (UVC) implementation #76798

josuah · 2024-08-07T14:08:09Z

How to test:

The method to test with any device_next board:

west build --board <board> samples/subsys/usb/uvc -- \
    -DEXTRA_DTC_OVERLAY_FILE=video-emul.overlay

The method to test with any device with a dcmi node:

west build --board <board> samples/subsys/usb/uvc -- \
    -DEXTRA_DTC_OVERLAY_FILE=video-dcmi.overlay

Build the sample for the selected board like above and load it in the board
Connect it to a host USB port and expect a video interface to show-up
Play the feed using one of the method listed on the sample documentation

It will use a very small test pattern as video source, so no camera is required.
To connect a camera instead of a test pattern, connect a different video source through the devicetree.
The USB descriptors will be updated by querying the driver.

Dependencies:

~~drivers: video: introduce "GET" sub-operations #78603~~ replaced by something else to come
~~drivers: video: allow allocation with a header preceding the buffer #79168~~ not required anymore
video: emul: virtual driver for an imager and RX engine #79482

This is a preview of an USB Video Class implementation for Zephyr that was only tested on custom devices insofar.

Proper Zephyr samples will be provided on upcoming commits. The API is simply to submit frames to the UVC device like to any Zephyr video device.

There is an unsolved challenge around the Video API: there is no set_format because the Zephyr application cannot decide what the host uses, only get_format for what the host does support. But there is a missing video API to allow the driver to warn the application about a forced format change requested by the host. I thought of maybe reusing set_signal() to also warn about format changes and not just buffer events.

	.get_format = uvc_get_format,
	.get_caps = uvc_get_caps,

I will now work on building examples for existing Zephyr devices, as this was built for a custom USB3 board.
Here is the devicetree configuration used insofar:

#include <zephyr/dt-bindings/usb/video.h>

&zephyr_udc0 {
	uvc: uvc {
		compatible = "zephyr,uvc-device";

		port {
			uvc_ep_in: endpoint {
				remote-endpoint-label = "mipi0_ep_out";
			};
		};
	};
};

The sizes and FPS are to be selected by the developer. The USB descriptors get configured as described above, and the host will request a particular format. Once that is done, the USB Video Class driver can let the application know which of these was selected through the video.h get_format() API.

This is still a draft PR, but I am grateful for comments and suggestions. I am willing to do the work of refactoring this as much as needed.

[EDIT: see this comment for latest description]

josuah · 2024-08-07T14:09:22Z

I am grateful for the work on the USB and Video stacks of Zephyr, as well as the entire Zephyr tree, on the shoulder of which this is built.

josuah · 2024-08-08T02:16:03Z

Force push:

fixes to make it compile with west build -b frdm_mcxn947/mcxn947/cpu0 (a board I happened to have, open to suggestions)
added another commit (with I hope proper attributions!) to introduce fragments, which UVC and this implementation support.

josuah · 2024-08-08T02:22:20Z

Force-push:

Add a sample to illustrate the usage. It is not yet tested but compiles.

XenuIsWatching · 2024-08-08T18:56:44Z

subsys/usb/device_next/class/usbd_uvc.c

+		.dwMinBitRate = sys_cpu_to_le32(15360000),                                         \
+		.dwMaxBitRate = sys_cpu_to_le32(15360000),                                         \


Where are these values coming from?

These are chosen arbitrarily, and I need to think about what should I do here: pick reasonable defaults? Deduce from other values? Let the user figure out by exposing a devicetree option?

In future work, it will be possible to ask the sensor for VIDEO_CID_PIXEL_RATE in combination with video_bits_per_pixel() to generate this field.

XenuIsWatching · 2024-08-08T18:57:48Z

subsys/usb/device_next/class/usbd_uvc.c

+				.bLength = sizeof(struct usb_ep_descriptor),                       \
+				.bDescriptorType = USB_DESC_ENDPOINT,                              \
+				.bEndpointAddress = 0x81,                                          \
+				.bmAttributes = USB_EP_TYPE_BULK,                                  \


I believe UVC can be BULK or ISO.... but I only see BULK supported here. Do you plan to have a way to select which type of EP?

I forgot to mention that only BULK is supported through this early version. I will need to spend more time investigating the device_next APIs for how ISO is implemented.

I cannot guarantee ETAs as I would do this feature on free time, but will add it to the roadmap: this will be needed at some point.

include/zephyr/usb/class/usb_uvc.h

josuah · 2024-08-08T22:10:23Z

Very grateful for your reviews @XenuIsWatching I will force-push with the changes as soon as I get a chance to do so.

josuah · 2024-08-21T11:35:12Z

Force-push:

control a selected source = <&dev>; device from the UVC stack directly
wait that a format got selected before queuing video buffers to the USB
use "discrete" frame intervals (FPS) values, which seemed to help getting it to work under Windows
USB3CV compliant descriptors

josuah · 2024-09-07T15:47:51Z

force push: rebased on main

josuah · 2024-09-07T15:56:50Z

force push: changed the way descriptors are declared and introduce video controls at the descriptor level (no support for controls commands yet)

josuah · 2024-09-12T00:44:11Z

force push: implemented the UVC controls for the supported Video class controls.

I did not test this yet with the samples, but on the internal fork, we could get an IMX219 sensor with exposure and gain control from the host, format selection at startup (but not runtime, see [1]).

I believe that the last step for me is to test the sample on several boards that support device_next and fix the CI.

josuah · 2024-09-12T01:50:49Z

Known limitations:

[1]: there is currently no <video.h> API to let the application know that a format change occurred. For now, the sample waits that the host makes an initial choice, and does not reconsider its format selection.
A trade-off between supported feature and sample complexity.

[2]: there is currently no <video.h> API to let the video device communicate their min/max/step, so 0 is always picked as min, INT32/16/8_MAX as max, and 1 as step. Maybe the solution is to define an unit for each of the controls and have every sensor map this to their local definition. For instance, 1 µs for exposure, 1/1000 for gain, etc, and have the max value implicit. DONE

[3]: ~~The "default" value is an arbitrary value of 1 instead of querying the controls of each sensor.~~ DONE

[4]: ~~There is no device const struct uvc_conf and only struct uvc_data, which wastes precious memory.~~ DONE.

[5]: There are missing implementations for selector unit, extensions unit, encoding unit. TODO.

[6]: UVC introduces dependencies between some controls, i.e. auto-expopsure needs to be off for exposure to be accepted. TODO.

[7]: ~~UVC has "error" control type reporting the last error.~~ DONE

[8]: Only a single endpoint and output/streaming interface is supported per UVC interface. Composite devices (multiple UVC classes per device) are used instead for supporting multiple video streams per device. DONE

[9]: ~~No documentation outside the devicetree bindings~~. DONE (no documentation needed anymore as no configuration required).

[10]: Supports custom header size, but not passing custom header data yet.

[11]: Still image capture (capturing one frame at full resolution) not supported.

[12]: USB3CV compliance tests not all passing since last rebase. TODO.

[13]: Announcing different resolutions/FPS for different connection speed not supported.

[14]: Asynchronous controls (the host setting a control, and a notification interrupt alert of completion) not supported.

Supported features:

[A]: Class API and enumeration

[B]: ~~Custom control chains descriptors built from the devicetree layout~~ Not supported by Linux

[C]: ~~Selection of which controls to expose to the host via devicetree toggles~~ Now controlled by the drivers.

[D]: ~~Per-control entity tuning~~ Sending all the controls requests to the first driver of the pipeline.

[E]: Handling of control commands end-to-end from host to Zephyr video device

[F]: Zephyr native Video API that allows to enqueue/dequeue frames like any other Zephyr video device.

[G]: Supports fragmented frames as discussed in #66994 and #72827

[H]: ~~Can configure pixel format and resolution on the devicetree~~ Done automatically by asking the video drivers.

[I]: Supports querying the min/max/current values of every controls end-to-end from host to Zephyr video driver.

subsys/usb/device_next/class/usbd_uvc.c

IAmAnkur · 2025-02-10T21:06:28Z

subsys/usb/device_next/class/usbd_uvc.c

+	union uvc_stream_descriptor *desc = &strm->desc->if_vs_formats[*id];
+	uint32_t pixfmt = cap->pixelformat;
+
+	if (*id >= CONFIG_USBD_VIDEO_MAX_VS_DESC) {
+		LOG_ERR("Out of descriptors, consider increasing CONFIG_USBD_VIDEO_MAX_VS_DESC");
+		return -ENOMEM;
+	}


since we are assigning desc based on id, i believe the if (*id >= CONFIG_USBD_VIDEO_MAX_VS_DESC) check should be prior to the desc assignment.

IAmAnkur · 2025-02-10T21:10:01Z

subsys/usb/device_next/class/usbd_uvc.c

+		strm->desc->if_vs_header.bNumFormats + 1,
+		pixfmt >> 0 & 0xff, pixfmt >> 8 & 0xff, pixfmt >> 16 & 0xff, pixfmt >> 24 & 0xff);
+
+	memset(desc, 0, sizeof(*desc));


More of a suggestion, If desc is guaranteed to be Non NULL at this point, it is fine, otherwise it is good to keep NULL check before doing any memsets in order to avoid undefined behavior (memset on NULL pointer).

It's desc = &array[*id];, but I added a __ASSERT_NO_MSG() to raise awareness i.e. in case of refactoring.

Thank you!

IAmAnkur · 2025-02-10T21:10:43Z

subsys/usb/device_next/class/usbd_uvc.c

+	struct video_format fmt = {.pixelformat = cap->pixelformat,
+				   .width = w, .height = h, .pitch = p};
+	struct video_frmival_enum fie = {.format = &fmt};
+	union uvc_stream_descriptor *desc = &strm->desc->if_vs_formats[*id];


since we are assigning desc based on id, i believe the if (*id >= CONFIG_USBD_VIDEO_MAX_VS_DESC) check should be prior to the desc assignment.

IAmAnkur · 2025-02-10T21:14:52Z

subsys/usb/device_next/class/usbd_uvc.c

+			return -EINVAL;
+		}
+		strm->format_id = probe->bFormatIndex;
+		break;


Is it possible to handle default case for request incase of invalid request?

I originally had one, but factorized it from all locations here:
5fbd6d7#diff-8741d252cf20802cf806752bfd74a7e120f74445aa181cfd85ff04c9e2dc8ec0R889-R893

However, this leads to loose coupling between code and associated checks, better have a default!

IAmAnkur · 2025-02-10T21:15:21Z

subsys/usb/device_next/class/usbd_uvc.c

+			return -EINVAL;
+		}
+		strm->frame_id = probe->bFrameIndex;
+		break;


Is it possible to handle default case for request incase of invalid request?

Same as here. Thanks!

IAmAnkur · 2025-02-10T21:15:54Z

subsys/usb/device_next/class/usbd_uvc.c

+	case SET_CUR:
+		strm->video_frmival.numerator = sys_le32_to_cpu(probe->dwFrameInterval);
+		break;
+	}


Is it possible to handle default case for request incase of invalid request?

Same as here. Thanks!

IAmAnkur · 2025-02-10T21:16:12Z

subsys/usb/device_next/class/usbd_uvc.c

+		    sys_le32_to_cpu(probe->dwMaxVideoFrameSize) != max_frame_size) {
+			LOG_WRN("probe: dwMaxVideoFrameSize is read-only");
+		}
+		break;


Is it possible to handle default case for request incase of invalid request?

Same as here. Thanks!

josuah · 2025-02-10T23:02:45Z

force-push:

Working west twister without passing extra arguments,
Added a boards/ for the Nicla Vision (with device_next and an image sensor on it)
Other bugfixes following @jfischer-no instructions
Improve robustness following @IAmAnkur review

jfischer-no · 2025-02-11T17:45:59Z

samples/subsys/usb/uvc/sample.yaml

+  sample.subsys.usb.uvc.camera:
+    depends_on:
+      - usbd
+    tags: usb video
+    filter: dt_chosen_enabled("zephyr,camera")
+    integration_platforms:
+      - arduino_nicla_vision/stm32h747xx/m7


I see that it depends on video-dcmi.overlay. I thought the source was abstracted by the video driver/subsystem. If there is no generic way to get the video source, then you cannot use filter: dt_chosen_enabled("zephyr,camera") and integration_platform because it would fail on other platforms.

I thought the source was abstracted by the video driver/subsystem.

Not yet... so I removed the sample.subsys.usb.uvc.camera hoping that it is the best thing to do until a long term solution solves it.

Thank you once again for the guidance.

josuah · 2025-02-12T00:41:33Z

force-push:

remove sample.subsys.usb.uvc.camera until a really hardware-agnostic solution can be provided

jfischer-no · 2025-02-12T09:41:17Z

@josuah It is broken by the b341d9d for no reason, you need to rebase on latest main and rework uvc_stream_start/uvc_stream_stop.

jfischer-no · 2025-02-12T12:42:26Z

@josuah It is broken by the b341d9d for no reason, you need to rebase on latest main and rework uvc_stream_start/uvc_stream_stop.

@josuah Still not fixed.

josuah · 2025-02-12T18:30:18Z

CI just finished. Thanks for waiting.

Force-push:

.stream_start()/.stream_stop() is now .set_stream(true)/.set_stream(false), bugfix while also adding basic support for it

jfischer-no · 2025-02-13T07:34:46Z

subsys/usb/device_next/class/usbd_uvc.c

+#define SET_CUR					0x01
+#define GET_CUR					0x81
+#define GET_MIN					0x82
+#define GET_MAX					0x83
+#define GET_RES					0x84
+#define GET_LEN					0x85
+#define GET_INFO				0x86
+#define GET_DEF					0x87


I suggest to prefix them with UVC_ to minimize the risk of colliding with whatever madness the vendor's HALs contain.

jfischer-no · 2025-02-13T07:40:19Z

subsys/usb/device_next/class/usbd_uvc.c

+/**
+ * @brief Get the currently selected format and frame descriptors.
+ *
+ * @param strm The VideoStreaming interface from which search the descriptors
+ * @param format_desc Pointer to be set to a format descriptor pointer.
+ * @param frame_desc Pointer to be set to a frame descriptor pointer.
+ */


Please never use doxygen format in c or local header files.

jfischer-no · 2025-02-13T07:42:12Z

subsys/usb/device_next/class/usbd_uvc.c

+	case GET_RES:
+	case GET_MIN:
+		probe->bFormatIndex = 1;
+		break;
+	case GET_MAX:
+		probe->bFormatIndex = max;
+		break;
+	case GET_CUR:
+		probe->bFormatIndex = strm->format_id;
+		break;
+	case SET_CUR:


GET_/SET_ must not be mixed, but must be handled in separate handlers. See my comment below about net_buf pointer.

subsys/usb/device_next/class/usbd_uvc.c

jfischer-no · 2025-02-13T10:31:21Z

subsys/usb/device_next/class/usbd_uvc.c

+
+static void uvc_update(struct usbd_class_data *const c_data, uint8_t iface, uint8_t alternate)
+{
+	LOG_DBG("update");


LOG_DBG("Select alternate %u for interface %u", alternate, iface);

jfischer-no · 2025-02-13T10:35:33Z

subsys/usb/device_next/class/usbd_uvc.c

+	uint32_t max_size = MAX(p, w) * h;
+
+	LOG_DBG("Adding frame descriptor #%u for %ux%u",
+		format_desc->format.bNumFrameDescriptors + 1, (unsigned int)w, (unsigned int)h);


LOG_DBG("Adding frame descriptor #%u for %ux%u", format_desc->format.bNumFrameDescriptors + 1, w, h);

jfischer-no · 2025-02-13T10:37:23Z

boards/arduino/nicla_vision/arduino_nicla_vision_stm32h747xx_m7.yaml

@@ -11,4 +11,5 @@ supported:
  - gpio
  - spi
  - i2c
+  - usbd


Please move it to a separate commit "boards: nicla_vision: add usbd test feature"

josuah · 2025-02-13T20:57:34Z

Force-push:

Apply the style change suggested above
Refactor everything to separate the SET and GET operations (.control_to_host() and .control_to_dev()).

This is an early preview that was not tested yet, only there to give an idea.
Another force-push might come once tested.

Introduce a new USB Video Class (UVC) implementation from scratch. It exposes a native Zephyr Video driver interface, allowing to call the video_enqueue()/video_dequeue() interface. It will query the attached video device to learn about the pipeline capabilities, and use this to configure the USB descriptors. At runtime, this UVC implementation will send this device all the control requests, which it can then dispatch to the rest of the pipeline. The application can poll the format currently selected by the host, but will not be alerted when the host configures a new format, as there is no video.h API for it yet. Signed-off-by: Josuah Demangeon <[email protected]>

The Arduino Nicla Vision board supports the new device_next USB stack and can be used for testing USB Device features such as UVC. Signed-off-by: Josuah Demangeon <[email protected]>

Following the addition of USB Video Class, this adds a sample that makes use of the &zephyr,camera chosen node of any board to stream the video source to the host. A fallback video-emul.overlay is provided for test and debugging purpose for devices without a camera. Signed-off-by: Josuah Demangeon <[email protected]>

josuah · 2025-02-14T00:57:08Z

Force-push:

Fix bugs after refactoring
More testing

Current limitations (likely not due to this refactoring):

Race conditions observed
Depending on the log level, the feed gets stuck after some time, or cannot be closed/started (as pointed here)

High-level outline:

/* UVC helper functions */
/* UVC control handling */
/* UVC descriptor handling */
/* UVC data handling */
/* UVC video API */

kartben · 2025-02-14T17:13:15Z

Kind of sad this won't make it in 4.1 😢
FWIW, assuming parties involved (mainly @jfischer-no and @josuah) are fine with that and this gets to mergeable state next week or so, it might make sense to ask for an exception to include this in 4.1. Just a thought :)

josuah added priority: low Low impact/importance bug area: USB Universal Serial Bus area: Video Video subsystem area: Devicetree Binding PR modifies or adds a Device Tree binding labels Aug 7, 2024

josuah force-pushed the pr-usb-uvc branch from 2a90efe to f28a24d Compare August 8, 2024 02:14

XenuIsWatching reviewed Aug 8, 2024

View reviewed changes

This was referenced Aug 12, 2024

drivers: video: "remote-endpoint" does nothing #72311

Closed

drivers: video: API enhancement #72959

Open

josuah force-pushed the pr-usb-uvc branch from 81a14d9 to 9446edc Compare August 21, 2024 11:32

josuah force-pushed the pr-usb-uvc branch 2 times, most recently from 182ac7c to cec26cc Compare August 21, 2024 11:43

josuah force-pushed the pr-usb-uvc branch 2 times, most recently from 02c1087 to 4bdc1ea Compare September 7, 2024 15:46

josuah force-pushed the pr-usb-uvc branch from 4bdc1ea to 5d36783 Compare September 7, 2024 15:55

josuah force-pushed the pr-usb-uvc branch from 5d36783 to d4209ae Compare September 12, 2024 00:29

josuah changed the title ~~usb: device_next: new USB Video Class implementation~~ usb: device_next: new USB Video Class (UVC) implementation Sep 12, 2024

josuah commented Sep 16, 2024

View reviewed changes

subsys/usb/device_next/class/usbd_uvc.c Outdated Show resolved Hide resolved

josuah mentioned this pull request Sep 18, 2024

drivers: video: introduce "GET" sub-operations #78603

Closed

josuah force-pushed the pr-usb-uvc branch 3 times, most recently from a7cd6cb to ea60ec0 Compare September 22, 2024 01:26

IAmAnkur reviewed Feb 10, 2025

View reviewed changes

josuah force-pushed the pr-usb-uvc branch from 65ee4be to d4ec26d Compare February 10, 2025 22:14

jfischer-no reviewed Feb 11, 2025

View reviewed changes

josuah force-pushed the pr-usb-uvc branch from d4ec26d to 6213ee6 Compare February 12, 2025 00:34

josuah force-pushed the pr-usb-uvc branch from 6213ee6 to 8c93fd9 Compare February 12, 2025 10:28

josuah force-pushed the pr-usb-uvc branch from 8c93fd9 to 2860f21 Compare February 12, 2025 15:21

jfischer-no requested changes Feb 13, 2025

View reviewed changes

josuah force-pushed the pr-usb-uvc branch from 2860f21 to d999a46 Compare February 13, 2025 20:55

josuah added the DNM This PR should not be merged (Do Not Merge) label Feb 13, 2025

josuah added 3 commits February 14, 2025 00:52

boards: nicla_vision: add usbd test feature

9732e9c

The Arduino Nicla Vision board supports the new device_next USB stack and can be used for testing USB Device features such as UVC. Signed-off-by: Josuah Demangeon <[email protected]>

josuah force-pushed the pr-usb-uvc branch from d999a46 to df89766 Compare February 14, 2025 00:52

josuah removed the DNM This PR should not be merged (Do Not Merge) label Feb 14, 2025

jfischer-no modified the milestones: v4.1.0, v4.2.0 Feb 14, 2025

		.dwMinBitRate = sys_cpu_to_le32(15360000), \
		.dwMaxBitRate = sys_cpu_to_le32(15360000), \

@@ @@ -11,4 +11,5 @@ supported: @@
                 - gpio
                 - spi
                 - i2c
+                - usbd

usb: device_next: new USB Video Class (UVC) implementation #76798

Are you sure you want to change the base?

usb: device_next: new USB Video Class (UVC) implementation #76798

Conversation

josuah commented Aug 7, 2024 • edited Loading

josuah commented Aug 7, 2024

josuah commented Aug 8, 2024

josuah commented Aug 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josuah commented Aug 8, 2024

josuah commented Aug 21, 2024 • edited Loading

josuah commented Sep 7, 2024

josuah commented Sep 7, 2024

josuah commented Sep 12, 2024

josuah commented Sep 12, 2024 • edited Loading

Known limitations:

Supported features:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josuah commented Feb 10, 2025

Choose a reason for hiding this comment

josuah Feb 12, 2025 • edited Loading

Choose a reason for hiding this comment

josuah commented Feb 12, 2025

jfischer-no commented Feb 12, 2025

jfischer-no commented Feb 12, 2025

josuah commented Feb 12, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josuah commented Feb 13, 2025

josuah commented Feb 14, 2025 • edited Loading

kartben commented Feb 14, 2025

josuah commented Aug 7, 2024 •

edited

Loading

josuah commented Aug 21, 2024 •

edited

Loading

josuah commented Sep 12, 2024 •

edited

Loading

josuah Feb 12, 2025 •

edited

Loading

josuah commented Feb 14, 2025 •

edited

Loading