From de752a1f8fdfab5f929767b6e9fb2c5fd6fb8af8 Mon Sep 17 00:00:00 2001 From: Taylor R Campbell Date: Sun, 9 Jan 2022 17:28:16 +0000 Subject: [PATCH 1/6] uvideo(4): Fix USB interface numbering. Don't try to be clever and count -- just use bInterfaceNumber. The previous logic to count interface descriptors failed to consider interfaces with alternate settings, which led it to pass an invalid interface number to usbd_device2interface_handle. It is simpler to just use the recorded bInterfaceNumber, which is guaranteed by the USB spec to be zero-indexed and below bNumInterfaces as we need. --- sys/dev/usb/uvideo.c | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-) diff --git a/sys/dev/usb/uvideo.c b/sys/dev/usb/uvideo.c index 6c263b047ded..a1f8aca18cda 100644 --- a/sys/dev/usb/uvideo.c +++ b/sys/dev/usb/uvideo.c @@ -328,8 +328,7 @@ static struct uvideo_stream * uvideo_stream_alloc(void); static usbd_status uvideo_stream_init( struct uvideo_stream *, struct uvideo_softc *, - const usb_interface_descriptor_t *, - uint8_t); + const usb_interface_descriptor_t *); static usbd_status uvideo_stream_init_desc( struct uvideo_stream *, const usb_interface_descriptor_t *, @@ -496,7 +495,6 @@ uvideo_attach(device_t parent, device_t self, void *aux) const usb_interface_descriptor_t *ifdesc; struct uvideo_stream *vs; usbd_status err; - uint8_t ifaceidx; sc->sc_dev = self; @@ -528,10 +526,7 @@ uvideo_attach(device_t parent, device_t self, void *aux) /* iterate through interface descriptors and initialize softc */ usb_desc_iter_init(sc->sc_udev, &iter); - for (ifaceidx = 0; - (ifdesc = usb_desc_iter_next_interface(&iter)) != NULL; - ++ifaceidx) - { + while ((ifdesc = usb_desc_iter_next_interface(&iter)) != NULL) { if (ifdesc->bInterfaceClass != UICLASS_VIDEO) { DPRINTFN(50, ("uvideo_attach: " "ignoring non-uvc interface: " @@ -568,8 +563,7 @@ uvideo_attach(device_t parent, device_t self, void *aux) err = USBD_NOMEM; goto bad; } - err = uvideo_stream_init(vs, sc, ifdesc, - ifaceidx); + err = uvideo_stream_init(vs, sc, ifdesc); if (err != USBD_NORMAL_COMPLETION) { DPRINTF(("uvideo_attach: " "error initializing stream: " @@ -1022,8 +1016,7 @@ uvideo_unit_free_controls(struct uvideo_unit *vu) static usbd_status uvideo_stream_init(struct uvideo_stream *vs, struct uvideo_softc *sc, - const usb_interface_descriptor_t *ifdesc, - uint8_t idx) + const usb_interface_descriptor_t *ifdesc) { uWord len; usbd_status err; @@ -1039,7 +1032,8 @@ uvideo_stream_init(struct uvideo_stream *vs, vs->vs_current_format.priv = -1; vs->vs_xfer_type = 0; - err = usbd_device2interface_handle(sc->sc_udev, idx, &vs->vs_iface); + err = usbd_device2interface_handle(sc->sc_udev, vs->vs_ifaceno, + &vs->vs_iface); if (err != USBD_NORMAL_COMPLETION) { DPRINTF(("uvideo_stream_init: " "error getting vs interface: " From 264fabfe4cdc9fd308f421dc2c55c1cae5132915 Mon Sep 17 00:00:00 2001 From: Taylor R Campbell Date: Mon, 10 Jan 2022 01:22:10 +0000 Subject: [PATCH 2/6] uvideo(4): Use __nothing for empty DPRINTF, not actually empty. --- sys/dev/usb/uvideo.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/sys/dev/usb/uvideo.c b/sys/dev/usb/uvideo.c index a1f8aca18cda..8a78ce647c79 100644 --- a/sys/dev/usb/uvideo.c +++ b/sys/dev/usb/uvideo.c @@ -91,8 +91,8 @@ __KERNEL_RCSID(0, "$NetBSD: uvideo.c,v 1.47.2.1 2019/12/29 11:13:48 martin Exp $ #define DPRINTFN(n,x) do { if (uvideodebug>(n)) printf x; } while (0) int uvideodebug = 20; #else -#define DPRINTF(x) -#define DPRINTFN(n,x) +#define DPRINTF(x) __nothing +#define DPRINTFN(n,x) __nothing #endif typedef enum { From 51c2f812117afd8463b0db6277974e3545a6defc Mon Sep 17 00:00:00 2001 From: Taylor R Campbell Date: Mon, 10 Jan 2022 01:23:30 +0000 Subject: [PATCH 3/6] uvideo(4): Sprinkle debug messages. --- sys/dev/usb/uvideo.c | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/sys/dev/usb/uvideo.c b/sys/dev/usb/uvideo.c index 8a78ce647c79..ba9d15481991 100644 --- a/sys/dev/usb/uvideo.c +++ b/sys/dev/usb/uvideo.c @@ -1021,6 +1021,11 @@ uvideo_stream_init(struct uvideo_stream *vs, uWord len; usbd_status err; + DPRINTF(("%s: %s ifaceno=%d vs=%p\n", __func__, + device_xname(sc->sc_dev), + ifdesc->bInterfaceNumber, + vs)); + SLIST_INSERT_HEAD(&sc->sc_stream_list, vs, entries); memset(vs, 0, sizeof(*vs)); vs->vs_parent = sc; @@ -1097,6 +1102,9 @@ uvideo_stream_init_desc(struct uvideo_stream *vs, uint8_t bmAttributes, bEndpointAddress; int i; + DPRINTF(("%s: bInterfaceNumber=%d bAlternateSetting=%d\n", __func__, + ifdesc->bInterfaceNumber, ifdesc->bAlternateSetting)); + /* Iterate until the next interface descriptor. All * descriptors until then belong to this streaming * interface. */ @@ -1215,6 +1223,10 @@ uvideo_stream_init_desc(struct uvideo_stream *vs, } } + DPRINTF(("%s: bInterfaceNumber=%d bAlternateSetting=%d done\n", + __func__, + ifdesc->bInterfaceNumber, ifdesc->bAlternateSetting)); + return USBD_NORMAL_COMPLETION; } @@ -1263,10 +1275,14 @@ uvideo_stream_init_frame_based_format(struct uvideo_stream *vs, uint32_t frame_interval; const usb_guid_t *guid; + DPRINTF(("%s: ifaceno=%d subtype=%d probelen=%d\n", __func__, + vs->vs_ifaceno, vs->vs_subtype, vs->vs_probelen)); + pixel_format = VIDEO_FORMAT_UNDEFINED; switch (format_desc->bDescriptorSubtype) { case UDESC_VS_FORMAT_UNCOMPRESSED: + DPRINTF(("%s: uncompressed\n", __func__)); subtype = UDESC_VS_FRAME_UNCOMPRESSED; default_index = GET(uvideo_vs_format_uncompressed_descriptor_t, format_desc, @@ -1280,14 +1296,21 @@ uvideo_stream_init_frame_based_format(struct uvideo_stream *vs, pixel_format = VIDEO_FORMAT_NV12; else if (usb_guid_cmp(guid, &uvideo_guid_format_uyvy) == 0) pixel_format = VIDEO_FORMAT_UYVY; + else { + DPRINTF(("%s: unknown format: ", __func__)); + usb_guid_print(guid); + DPRINTF(("\n")); + } break; case UDESC_VS_FORMAT_FRAME_BASED: + DPRINTF(("%s: frame-based\n", __func__)); subtype = UDESC_VS_FRAME_FRAME_BASED; default_index = GET(uvideo_format_frame_based_descriptor_t, format_desc, bDefaultFrameIndex); break; case UDESC_VS_FORMAT_MJPEG: + DPRINTF(("%s: mjpeg\n", __func__)); subtype = UDESC_VS_FRAME_MJPEG; default_index = GET(uvideo_vs_format_mjpeg_descriptor_t, format_desc, From 4cb22302f8a4650b9d25cb589c5a7fee5a245435 Mon Sep 17 00:00:00 2001 From: Taylor R Campbell Date: Mon, 10 Jan 2022 01:26:06 +0000 Subject: [PATCH 4/6] video(4): Allow drivers to pass the softc explicitly. This way one device driver can have multiple video0, video1, &c., interfaces attached, using independent state and a common parent. --- sys/dev/video.c | 14 +++++++++++++- sys/dev/video_if.h | 2 ++ 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/sys/dev/video.c b/sys/dev/video.c index 2046b03a1f03..db60e2ca74fd 100644 --- a/sys/dev/video.c +++ b/sys/dev/video.c @@ -351,7 +351,7 @@ video_attach(device_t parent, device_t self, void *aux) sc->sc_dev = self; sc->hw_dev = parent; sc->hw_if = args->hw_if; - sc->hw_softc = device_private(parent); + sc->hw_softc = args->hw_softc; sc->sc_open = 0; sc->sc_refcnt = 0; @@ -433,6 +433,18 @@ video_attach_mi(const struct video_hw_if *hw_if, device_t parent) struct video_attach_args args; args.hw_if = hw_if; + args.hw_softc = device_private(parent); + return config_found_ia(parent, "videobus", &args, video_print); +} + +device_t +video_attach_mi_softc(const struct video_hw_if *hw_if, device_t parent, + void *sc) +{ + struct video_attach_args args; + + args.hw_if = hw_if; + args.hw_softc = sc; return config_found_ia(parent, "videobus", &args, video_print); } diff --git a/sys/dev/video_if.h b/sys/dev/video_if.h index 8a7cd3f8dbc4..b66579494fef 100644 --- a/sys/dev/video_if.h +++ b/sys/dev/video_if.h @@ -501,9 +501,11 @@ struct video_hw_if { struct video_attach_args { const struct video_hw_if *hw_if; + void *hw_softc; }; device_t video_attach_mi(const struct video_hw_if *, device_t); +device_t video_attach_mi_softc(const struct video_hw_if *, device_t, void *); void video_submit_payload(device_t, const struct video_payload *); #endif /* _SYS_DEV_VIDEO_IF_H_ */ From bcf8737b21fe2b999c4d688f8ae1015794321f28 Mon Sep 17 00:00:00 2001 From: Taylor R Campbell Date: Mon, 10 Jan 2022 01:31:04 +0000 Subject: [PATCH 5/6] uvideo(4): Fix zero initialization of uvideo_stream. Just use kmem_zalloc; don't memset it to zero, especially not after we just inserted it into the list, with the side effect of deleting the rest of the list! --- sys/dev/usb/uvideo.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/sys/dev/usb/uvideo.c b/sys/dev/usb/uvideo.c index ba9d15481991..f1a4647726cd 100644 --- a/sys/dev/usb/uvideo.c +++ b/sys/dev/usb/uvideo.c @@ -758,7 +758,7 @@ uvideo_stream_guess_format(struct uvideo_stream *vs, static struct uvideo_stream * uvideo_stream_alloc(void) { - return kmem_alloc(sizeof(struct uvideo_stream), KM_NOSLEEP); + return kmem_zalloc(sizeof(struct uvideo_stream), KM_NOSLEEP); } @@ -1027,7 +1027,6 @@ uvideo_stream_init(struct uvideo_stream *vs, vs)); SLIST_INSERT_HEAD(&sc->sc_stream_list, vs, entries); - memset(vs, 0, sizeof(*vs)); vs->vs_parent = sc; vs->vs_ifaceno = ifdesc->bInterfaceNumber; vs->vs_subtype = 0; From 666e00505bfc0e1bd9ebd6c6d10b3661a357d18c Mon Sep 17 00:00:00 2001 From: Taylor R Campbell Date: Mon, 10 Jan 2022 01:36:50 +0000 Subject: [PATCH 6/6] uvideo(4): Attach one video(4) per independent stream. --- sys/dev/usb/uvideo.c | 115 +++++++++++++++++++------------------------ 1 file changed, 51 insertions(+), 64 deletions(-) diff --git a/sys/dev/usb/uvideo.c b/sys/dev/usb/uvideo.c index f1a4647726cd..d28dbd38d1bd 100644 --- a/sys/dev/usb/uvideo.c +++ b/sys/dev/usb/uvideo.c @@ -210,6 +210,7 @@ struct uvideo_bulk_xfer { }; struct uvideo_stream { + device_t vs_videodev; struct uvideo_softc *vs_parent; struct usbd_interface *vs_iface; uint8_t vs_ifaceno; @@ -236,6 +237,8 @@ struct uvideo_stream { uint32_t vs_max_payload_size; uint32_t vs_frame_interval; SLIST_ENTRY(uvideo_stream) entries; + + uvideo_state vs_state; }; SLIST_HEAD(uvideo_stream_list, uvideo_stream); @@ -246,16 +249,11 @@ struct uvideo_softc { int sc_ifaceno; /* interface number */ char *sc_devname; - device_t sc_videodev; - int sc_dying; - uvideo_state sc_state; uint8_t sc_nunits; struct uvideo_unit **sc_unit; - struct uvideo_stream *sc_stream_in; - struct uvideo_stream_list sc_stream_list; char sc_businfo[32]; @@ -507,7 +505,6 @@ uvideo_attach(device_t parent, device_t self, void *aux) sc->sc_iface = uiaa->uiaa_iface; sc->sc_ifaceno = uiaa->uiaa_ifaceno; sc->sc_dying = 0; - sc->sc_state = UVIDEO_STATE_CLOSED; SLIST_INIT(&sc->sc_stream_list); snprintf(sc->sc_businfo, sizeof(sc->sc_businfo), "usb:%08x", sc->sc_udev->ud_cookie.cookie); @@ -580,8 +577,6 @@ uvideo_attach(device_t parent, device_t self, void *aux) usbd_errstr(err), err)); goto bad; } - /* TODO: for now, set (each) stream to stream_in. */ - sc->sc_stream_in = vs; break; case UISUBCLASS_VIDEOCOLLECTION: err = uvideo_init_collection(sc, ifdesc, &iter); @@ -612,9 +607,11 @@ uvideo_attach(device_t parent, device_t self, void *aux) if (!pmf_device_register(self, NULL, NULL)) aprint_error_dev(self, "couldn't establish power handler\n"); - sc->sc_videodev = video_attach_mi(&uvideo_hw_if, sc->sc_dev); - DPRINTF(("uvideo_attach: attached video driver at %p\n", - sc->sc_videodev)); + SLIST_FOREACH(vs, &sc->sc_stream_list, entries) { + /* XXX initialization of vs_videodev is racy */ + vs->vs_videodev = video_attach_mi_softc(&uvideo_hw_if, + sc->sc_dev, vs); + } return; @@ -648,21 +645,29 @@ void uvideo_childdet(device_t self, device_t child) { struct uvideo_softc *sc = device_private(self); + struct uvideo_stream *vs; - KASSERT(sc->sc_videodev == child); - sc->sc_videodev = NULL; + SLIST_FOREACH(vs, &sc->sc_stream_list, entries) { + if (child == vs->vs_videodev) { + vs->vs_videodev = NULL; + break; + } + } + KASSERTMSG(vs != NULL, "unknown child of %s detached: %s @ %p", + device_xname(self), device_xname(child), child); } int uvideo_detach(device_t self, int flags) { - struct uvideo_softc *sc; + struct uvideo_softc *sc = device_private(self); struct uvideo_stream *vs; - int rv; + int error; - sc = device_private(self); - rv = 0; + error = config_detach_children(self, flags); + if (error) + return error; sc->sc_dying = 1; @@ -687,14 +692,11 @@ uvideo_detach(device_t self, int flags) DPRINTFN(15, ("uvideo: detaching from %s\n", device_xname(sc->sc_dev))); - if (sc->sc_videodev != NULL) - rv = config_detach(sc->sc_videodev, flags); - usbd_add_drv_event(USB_EVENT_DRIVER_DETACH, sc->sc_udev, sc->sc_dev); usbd_devinfo_free(sc->sc_devname); - return rv; + return 0; } /* Search the stream list for a stream matching the interface number. @@ -1035,6 +1037,7 @@ uvideo_stream_init(struct uvideo_stream *vs, vs->vs_default_format = NULL; vs->vs_current_format.priv = -1; vs->vs_xfer_type = 0; + vs->vs_state = UVIDEO_STATE_CLOSED; err = usbd_device2interface_handle(sc->sc_udev, vs->vs_ifaceno, &vs->vs_iface); @@ -1773,7 +1776,7 @@ uvideo_stream_recv_process(struct uvideo_stream *vs, uint8_t *buf, uint32_t len) payload.frameno = hdr->bmHeaderInfo & UV_FRAME_ID; payload.end_of_frame = hdr->bmHeaderInfo & UV_END_OF_FRAME; - video_submit_payload(vs->vs_parent->sc_videodev, &payload); + video_submit_payload(vs->vs_videodev, &payload); return USBD_NORMAL_COMPLETION; } @@ -1871,13 +1874,10 @@ uvideo_stream_recv_bulk_transfer(void *addr) static int uvideo_open(void *addr, int flags) { - struct uvideo_softc *sc; - struct uvideo_stream *vs; + struct uvideo_stream *vs = addr; + struct uvideo_softc *sc = vs->vs_parent; struct video_format fmt; - sc = addr; - vs = sc->sc_stream_in; - DPRINTF(("uvideo_open: sc=%p\n", sc)); if (sc->sc_dying) return EIO; @@ -1891,36 +1891,36 @@ uvideo_open(void *addr, int flags) static void uvideo_close(void *addr) { - struct uvideo_softc *sc; - - sc = addr; + struct uvideo_stream *vs = addr; uvideo_stop_transfer(addr); - if (sc->sc_state != UVIDEO_STATE_CLOSED) { - sc->sc_state = UVIDEO_STATE_CLOSED; + if (vs->vs_state != UVIDEO_STATE_CLOSED) { + vs->vs_state = UVIDEO_STATE_CLOSED; } } static const char * uvideo_get_devname(void *addr) { - struct uvideo_softc *sc = addr; - return sc->sc_devname; + struct uvideo_stream *vs = addr; + + return vs->vs_parent->sc_devname; } static const char * uvideo_get_businfo(void *addr) { - struct uvideo_softc *sc = addr; - return sc->sc_businfo; + struct uvideo_stream *vs = addr; + + return vs->vs_parent->sc_businfo; } static int uvideo_enum_format(void *addr, uint32_t index, struct video_format *format) { - struct uvideo_softc *sc = addr; - struct uvideo_stream *vs = sc->sc_stream_in; + struct uvideo_stream *vs = addr; + struct uvideo_softc *sc = vs->vs_parent; struct uvideo_format *video_format; int off; @@ -1946,8 +1946,8 @@ uvideo_enum_format(void *addr, uint32_t index, struct video_format *format) static int uvideo_get_format(void *addr, struct video_format *format) { - struct uvideo_softc *sc = addr; - struct uvideo_stream *vs = sc->sc_stream_in; + struct uvideo_stream *vs = addr; + struct uvideo_softc *sc = vs->vs_parent; if (sc->sc_dying) return EIO; @@ -1963,20 +1963,16 @@ uvideo_get_format(void *addr, struct video_format *format) static int uvideo_set_format(void *addr, struct video_format *format) { - struct uvideo_softc *sc; - struct uvideo_stream *vs; + struct uvideo_stream *vs = addr; + struct uvideo_softc *sc = vs->vs_parent; struct uvideo_format *uvfmt; uvideo_probe_and_commit_data_t probe, maxprobe; usbd_status err; - sc = addr; - DPRINTF(("uvideo_set_format: sc=%p\n", sc)); if (sc->sc_dying) return EIO; - vs = sc->sc_stream_in; - uvfmt = uvideo_stream_guess_format(vs, format->pixel_format, format->width, format->height); if (uvfmt == NULL) { @@ -2080,8 +2076,7 @@ uvideo_set_format(void *addr, struct video_format *format) static int uvideo_try_format(void *addr, struct video_format *format) { - struct uvideo_softc *sc = addr; - struct uvideo_stream *vs = sc->sc_stream_in; + struct uvideo_stream *vs = addr; struct uvideo_format *uvfmt; uvfmt = uvideo_stream_guess_format(vs, format->pixel_format, @@ -2096,8 +2091,7 @@ uvideo_try_format(void *addr, struct video_format *format) static int uvideo_get_framerate(void *addr, struct video_fract *fract) { - struct uvideo_softc *sc = addr; - struct uvideo_stream *vs = sc->sc_stream_in; + struct uvideo_stream *vs = addr; switch (vs->vs_frame_interval) { case 41666: /* 240 */ @@ -2139,12 +2133,9 @@ uvideo_set_framerate(void *addr, struct video_fract *fract) static int uvideo_start_transfer(void *addr) { - struct uvideo_softc *sc = addr; - struct uvideo_stream *vs; + struct uvideo_stream *vs = addr; int s, err; - /* FIXME: this function should be stream specific */ - vs = SLIST_FIRST(&sc->sc_stream_list); s = splusb(); err = uvideo_stream_start_xfer(vs); splx(s); @@ -2155,13 +2146,11 @@ uvideo_start_transfer(void *addr) static int uvideo_stop_transfer(void *addr) { - struct uvideo_softc *sc; + struct uvideo_stream *vs = addr; int err, s; - sc = addr; - s = splusb(); - err = uvideo_stream_stop_xfer(sc->sc_stream_in); + err = uvideo_stream_stop_xfer(vs); splx(s); return err; @@ -2171,15 +2160,14 @@ uvideo_stop_transfer(void *addr) static int uvideo_get_control_group(void *addr, struct video_control_group *group) { - struct uvideo_softc *sc; + struct uvideo_stream *vs = addr; + struct uvideo_softc *sc = vs->vs_parent; usb_device_request_t req; usbd_status err; uint8_t control_id, ent_id, data[16]; uint16_t len; int s; - sc = addr; - /* request setup */ switch (group->group_id) { case VIDEO_CONTROL_PANTILT_RELATIVE: @@ -2233,15 +2221,14 @@ uvideo_get_control_group(void *addr, struct video_control_group *group) static int uvideo_set_control_group(void *addr, const struct video_control_group *group) { - struct uvideo_softc *sc; + struct uvideo_stream *vs = addr; + struct uvideo_softc *sc = vs->vs_parent; usb_device_request_t req; usbd_status err; uint8_t control_id, ent_id, data[16]; /* long enough for all controls */ uint16_t len; int s; - sc = addr; - switch (group->group_id) { case VIDEO_CONTROL_PANTILT_RELATIVE: if (group->length != 4)