Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

josuah
Copy link
Collaborator

@josuah josuah commented Aug 7, 2024

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
  1. Build the sample for the selected board like above and load it in the board
  2. Connect it to a host USB port and expect a video interface to show-up
  3. 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:

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
Copy link
Collaborator Author

josuah commented Aug 7, 2024

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 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
Copy link
Collaborator Author

josuah commented Aug 8, 2024

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
Copy link
Collaborator Author

josuah commented Aug 8, 2024

Force-push:

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

Comment on lines 920 to 961
.dwMinBitRate = sys_cpu_to_le32(15360000), \
.dwMaxBitRate = sys_cpu_to_le32(15360000), \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where are these values coming from?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@josuah
Copy link
Collaborator Author

josuah commented Aug 8, 2024

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

@josuah
Copy link
Collaborator Author

josuah commented Aug 21, 2024

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 josuah force-pushed the pr-usb-uvc branch 2 times, most recently from 182ac7c to cec26cc Compare August 21, 2024 11:43
@josuah josuah force-pushed the pr-usb-uvc branch 2 times, most recently from 02c1087 to 4bdc1ea Compare September 7, 2024 15:46
@josuah
Copy link
Collaborator Author

josuah commented Sep 7, 2024

force push: rebased on main

@josuah
Copy link
Collaborator Author

josuah commented Sep 7, 2024

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

@josuah 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
Copy link
Collaborator Author

josuah commented Sep 12, 2024

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
Copy link
Collaborator Author

josuah commented Sep 12, 2024

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.

@josuah
Copy link
Collaborator Author

josuah commented Feb 12, 2025

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

Comment on lines 30 to 37
#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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines 604 to 610
/**
* @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.
*/
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines 732 to 742
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:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@josuah
Copy link
Collaborator Author

josuah commented Feb 13, 2025

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.

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

josuah commented Feb 14, 2025

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 */

@josuah josuah removed the DNM This PR should not be merged (Do Not Merge) label Feb 14, 2025
@jfischer-no jfischer-no modified the milestones: v4.1.0, v4.2.0 Feb 14, 2025
@kartben
Copy link
Collaborator

kartben commented Feb 14, 2025

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
Copy link
Collaborator Author

josuah commented Feb 14, 2025

Then maybe this is the opportunity to complete this refactoring, and introduce a more stable API right from the start...
Which also has a chance to address the race condition (I think happening on descriptor initialization done twice in parallel).

Comment on lines 30 to 54
uint32_t dwMaxBitRate;
uint32_t dwMaxVideoFrameBufferSize;
uint32_t dwDefaultFrameInterval;
uint8_t bFrameIntervalType;
uint32_t dwMinFrameInterval;
uint32_t dwMaxFrameInterval;
uint32_t dwFrameIntervalStep;
} __packed;

struct uvc_frame_discrete_descriptor {
uint8_t bLength;
uint8_t bDescriptorType;
uint8_t bDescriptorSubtype;
uint8_t bFrameIndex;
uint8_t bmCapabilities;
uint16_t wWidth;
uint16_t wHeight;
uint32_t dwMinBitRate;
uint32_t dwMaxBitRate;
uint32_t dwMaxVideoFrameBufferSize;
uint32_t dwDefaultFrameInterval;
uint8_t bFrameIntervalType;
uint32_t dwFrameInterval[CONFIG_USBD_VIDEO_MAX_FRMIVAL];
} __packed;

union uvc_stream_descriptor {
struct uvc_format_descriptor format;
struct uvc_format_uncomp_descriptor format_uncomp;
struct uvc_format_mjpeg_descriptor format_mjpeg;
struct uvc_frame_discrete_descriptor frame_discrete;
struct uvc_frame_continuous_descriptor frame_continuous;
} __packed;

struct uvc_color_descriptor {
uint8_t bLength;
uint8_t bDescriptorType;
uint8_t bDescriptorSubtype;
uint8_t bColorPrimaries;
uint8_t bTransferCharacteristics;
uint8_t bMatrixCoefficients;
} __packed;

struct usbd_uvc_desc {
struct usb_association_descriptor iad;
struct usb_if_descriptor if_vc;
struct uvc_control_header_descriptor if_vc_header;
struct usb_desc_header nil_desc;
};

struct usbd_uvc_strm_desc {
struct uvc_camera_terminal_descriptor if_vc_ct;
struct uvc_selector_unit_descriptor if_vc_su;
struct uvc_processing_unit_descriptor if_vc_pu;
struct uvc_extension_unit_descriptor if_vc_xu;
struct uvc_output_terminal_descriptor if_vc_ot;

struct usb_if_descriptor if_vs;
struct uvc_stream_header_descriptor if_vs_header;
union uvc_stream_descriptor *if_vs_formats;
size_t if_vs_format_num;
struct usb_ep_descriptor if_vs_ep_fs;
struct usb_ep_descriptor if_vs_ep_hs;
struct uvc_color_descriptor if_vs_color;
};

struct uvc_probe {
uint16_t bmHint;
uint8_t bFormatIndex;
uint8_t bFrameIndex;
uint32_t dwFrameInterval;
uint16_t wKeyFrameRate;
uint16_t wPFrameRate;
uint16_t wCompQuality;
uint16_t wCompWindowSize;
uint16_t wDelay;
uint32_t dwMaxVideoFrameSize;
uint32_t dwMaxPayloadTransferSize;
uint32_t dwClockFrequency;
uint8_t bmFramingInfo;
#define UVC_BMFRAMING_INFO_FID BIT(0)
#define UVC_BMFRAMING_INFO_EOF BIT(1)
#define UVC_BMFRAMING_INFO_EOS BIT(2)
uint8_t bPreferedVersion;
uint8_t bMinVersion;
uint8_t bMaxVersion;
uint8_t bUsage;
uint8_t bBitDepthLuma;
uint8_t bmSettings;
uint8_t bMaxNumberOfRefFramesPlus1;
uint16_t bmRateControlModes;
uint64_t bmLayoutPerStream;
} __packed;

struct uvc_payload_header {
uint8_t bHeaderLength;
uint8_t bmHeaderInfo;
uint32_t dwPresentationTime; /* optional */
uint32_t scrSourceClockSTC; /* optional */
uint16_t scrSourceClockSOF; /* optional */
} __packed;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please consider moving these 400+ lines into the subsys/usb/device_next/class/uvc.h header. This could be used later to write uvc tests and perhaps host driver implementation.


struct uvc_guid_quirk {
uint32_t fourcc;
uint8_t guid[16];
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why 16? Please add comment.

Comment on lines 539 to 543
uint8_t selector;
uint8_t size;
uint8_t bit;
enum uvc_op op;
uint32_t param;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

?

	uint32_t param;
	enum uvc_op op;
	uint8_t selector;
	uint8_t size;
	uint8_t bit;

Comment on lines 554 to 100
NET_BUF_POOL_VAR_DEFINE(uvc_pool, DT_NUM_INST_STATUS_OKAY(DT_DRV_COMPAT) * 16,
512 * DT_NUM_INST_STATUS_OKAY(DT_DRV_COMPAT) * 16,
sizeof(struct uvc_buf_info), NULL);
Copy link
Collaborator

@jfischer-no jfischer-no Feb 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a problem. It is not a DCACHE-aware buffer pool. I will look in the next days how to provide a macro that also takes into account user data length argument.

Comment on lines 595 to 596
{.size = 1, .bit = 1, .selector = UVC_CT_AE_MODE_CONTROL,
.op = UVC_OP_VC_FIXED, .param = BIT(0)},
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are only a few places with these squeezed initialization. This is sometimes okay, but it does not follow the usual style and is inconsistent, please rework them.

static const struct uvc_control_map uvc_control_map_ct[] = {
	{
		.size = 1,
		.bit = 1,
		.selector = UVC_CT_AE_MODE_CONTROL,
	 	.op = UVC_OP_VC_FIXED,
		.param = BIT(0)
	},

@jfischer-no
Copy link
Collaborator

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 :)

I have noticed some, mostly minor, issues, but fixing them during the stabilization period would probably be too stressful. @josuah I apologize for not noticing them earlier. Let's get this merged right after the release.

@kartben
Copy link
Collaborator

kartben commented Feb 15, 2025

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 :)

I have noticed some, mostly minor, issues, but fixing them during the stabilization period would probably be too stressful. @josuah I apologize for not noticing them earlier. Let's get this merged right after the release.

Thanks @jfischer-no - makes sense.

@josuah
Copy link
Collaborator Author

josuah commented Feb 15, 2025

On my side, I have no issue with delay as tinyVision.ai uses an SDK where patches are cooked.

About other users wanting to try it, I am still glad that the review is comprehensive, it will help making things JustWork™ out of the box, and fit better in device_next once merged. Thank all for the time spent on this review!

P.S.: more force-push coming tonight soon

@josuah josuah force-pushed the pr-usb-uvc branch 2 times, most recently from 248d236 to 7516ec4 Compare February 18, 2025 21:29
@josuah
Copy link
Collaborator Author

josuah commented Feb 18, 2025

force-push:

  • Refactor to have all descripitors dynamically allocated (from a static array)
  • Refactor to have a macro declare a stream from the application (i.e. the sample main.c)
  • Refactor to not have any interface configuration on the devicetree
  • Add support for stepwise frame interval desscriptors
  • Runtime API to select which driver to load
  • These 5 previous items enable use of the VIDEO_SW_GENERATOR
  • If a control unit is empty (no controls), it is not added saving memory
  • Bugfixes

known-limitations and issues:

  • Closing the stream or restarting it sometimes leads to issues
  • Some few adjustments needed to get Windows working again
  • All "range" video formats (frame interval, resolution) are handled by only encoding the min and max
  • Does not yet support custom extension units (coming in a separate PR)
  • Does not yet support Isochronous endpoints (coming in a separate PR)
  • Does not yet support still image capture over endpoint 0 (coming in a separate PR)

The feedback helped improve the quality and reduce memory usage a little bit. 🙏

@josuah josuah force-pushed the pr-usb-uvc branch 3 times, most recently from 4a9347f to 568ce73 Compare February 27, 2025 02:44
@josuah
Copy link
Collaborator Author

josuah commented Feb 27, 2025

Force-push:

  • Implement the Video polling API
  • Fix the stream not restarting correctly
  • Fix inversion between frame interval and frame rate
  • Fix build failure on CI (Arduino Nicla asked for 2 buffers since "recent" commits)
  • Fix race condition upon accessing the FIFO from multiple locations
  • Remove the workqueue and process the video buffers directly from the parent context. The USB driver queue is where data is really queued for I/O

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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Devicetree Binding PR modifies or adds a Device Tree binding area: Devicetree area: Samples Samples area: USB Universal Serial Bus area: Video Video subsystem Experimental Experimental features not enabled by default
Projects
None yet
Development

Successfully merging this pull request may close these issues.