| Message ID | 20221121092517.225242-5-dan.scally@ideasonboard.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add XU support to UVC Gadget | expand |
Hi all On 21/11/2022 09:25, Daniel Scally wrote: > Now that extension unit support is available through configfs we need > to copy the descriptors for the XUs during uvc_function_bind() so that > they're exposed to the usb subsystem. > > Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com> > --- > Changes in v2: > > - none > > drivers/usb/gadget/function/f_uvc.c | 35 +++++++++++++++++++++++++++++ > drivers/usb/gadget/function/uvc.h | 1 + > 2 files changed, 36 insertions(+) > > diff --git a/drivers/usb/gadget/function/f_uvc.c b/drivers/usb/gadget/function/f_uvc.c > index eca5f36dfa74..e0a308f1355c 100644 > --- a/drivers/usb/gadget/function/f_uvc.c > +++ b/drivers/usb/gadget/function/f_uvc.c > @@ -464,6 +464,25 @@ uvc_register_video(struct uvc_device *uvc) > } \ > } while (0) > > +#define UVC_COPY_XU_DESCRIPTOR(mem, dst, desc) \ > + do { \ > + *(dst)++ = mem; \ > + memcpy(mem, desc, 22); /* bLength to bNrInPins */ \ > + mem += 22; \ > + \ > + memcpy(mem, desc->baSourceID, desc->bNrInPins); \ > + mem += desc->bNrInPins; \ > + \ > + memcpy(mem, &desc->bControlSize, 1); \ > + mem++; \ > + \ > + memcpy(mem, desc->bmControls, desc->bControlSize); \ > + mem += desc->bControlSize; \ > + \ > + memcpy(mem, &desc->iExtension, 1); \ > + mem++; \ > + } while (0) > + |I don't especially like UVC_COPY_XU_DESCRIPTOR(), but the need to vary the array size for baSourceID and bmControls plus the requirement for the struct to be copied to consecutive bytes of memory constrained it a bit. An alternative might be to replace baSourceID, bControlSize, bmControls and iExtension in struct uvcg_extension_unit_descriptor with a single flexible array member (called data[] or something). That would allow the copy to be a much more straight forward memcpy(mem, desc, desc->bLength); - but the cost would be reallocating the entire struct each time the baSourceID or bmControls attributes was changed. That might be a better method, but I thought I'd stick with this for this submission at least on the grounds that it's less confusing.| > static struct usb_descriptor_header ** > uvc_copy_descriptors(struct uvc_device *uvc, enum usb_device_speed speed) > { > @@ -475,6 +494,7 @@ uvc_copy_descriptors(struct uvc_device *uvc, enum usb_device_speed speed) > const struct usb_descriptor_header * const *src; > struct usb_descriptor_header **dst; > struct usb_descriptor_header **hdr; > + struct uvcg_extension *xu; > unsigned int control_size; > unsigned int streaming_size; > unsigned int n_desc; > @@ -539,6 +559,13 @@ uvc_copy_descriptors(struct uvc_device *uvc, enum usb_device_speed speed) > bytes += (*src)->bLength; > n_desc++; > } > + > + list_for_each_entry(xu, uvc->desc.extension_units, list) { > + control_size += xu->desc.bLength; > + bytes += xu->desc.bLength; > + n_desc++; > + } > + > for (src = (const struct usb_descriptor_header **)uvc_streaming_cls; > *src; ++src) { > streaming_size += (*src)->bLength; > @@ -565,6 +592,13 @@ uvc_copy_descriptors(struct uvc_device *uvc, enum usb_device_speed speed) > uvc_control_header = mem; > UVC_COPY_DESCRIPTORS(mem, dst, > (const struct usb_descriptor_header **)uvc_control_desc); > + > + list_for_each_entry(xu, uvc->desc.extension_units, list) { > + struct uvcg_extension_unit_descriptor *desc = &xu->desc; > + > + UVC_COPY_XU_DESCRIPTOR(mem, dst, desc); > + } > + > uvc_control_header->wTotalLength = cpu_to_le16(control_size); > uvc_control_header->bInCollection = 1; > uvc_control_header->baInterfaceNr[0] = uvc->streaming_intf; > @@ -988,6 +1022,7 @@ static struct usb_function *uvc_alloc(struct usb_function_instance *fi) > uvc->desc.fs_streaming = opts->fs_streaming; > uvc->desc.hs_streaming = opts->hs_streaming; > uvc->desc.ss_streaming = opts->ss_streaming; > + uvc->desc.extension_units = &opts->extension_units; > > streaming = config_group_find_item(&opts->func_inst.group, "streaming"); > if (!streaming) > diff --git a/drivers/usb/gadget/function/uvc.h b/drivers/usb/gadget/function/uvc.h > index 40226b1f7e14..f1a016d20bb6 100644 > --- a/drivers/usb/gadget/function/uvc.h > +++ b/drivers/usb/gadget/function/uvc.h > @@ -143,6 +143,7 @@ struct uvc_device { > const struct uvc_descriptor_header * const *fs_streaming; > const struct uvc_descriptor_header * const *hs_streaming; > const struct uvc_descriptor_header * const *ss_streaming; > + struct list_head *extension_units; > } desc; > > unsigned int control_intf;
Hi Dan, Thank you for the patch. On Mon, Nov 21, 2022 at 09:51:32AM +0000, Dan Scally wrote: > On 21/11/2022 09:25, Daniel Scally wrote: > > Now that extension unit support is available through configfs we need > > to copy the descriptors for the XUs during uvc_function_bind() so that > > they're exposed to the usb subsystem. > > > > Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com> > > --- > > Changes in v2: > > > > - none > > > > drivers/usb/gadget/function/f_uvc.c | 35 +++++++++++++++++++++++++++++ > > drivers/usb/gadget/function/uvc.h | 1 + > > 2 files changed, 36 insertions(+) > > > > diff --git a/drivers/usb/gadget/function/f_uvc.c b/drivers/usb/gadget/function/f_uvc.c > > index eca5f36dfa74..e0a308f1355c 100644 > > --- a/drivers/usb/gadget/function/f_uvc.c > > +++ b/drivers/usb/gadget/function/f_uvc.c > > @@ -464,6 +464,25 @@ uvc_register_video(struct uvc_device *uvc) > > } \ > > } while (0) > > > > +#define UVC_COPY_XU_DESCRIPTOR(mem, dst, desc) \ > > + do { \ > > + *(dst)++ = mem; \ > > + memcpy(mem, desc, 22); /* bLength to bNrInPins */ \ > > + mem += 22; \ > > + \ > > + memcpy(mem, desc->baSourceID, desc->bNrInPins); \ > > + mem += desc->bNrInPins; \ > > + \ > > + memcpy(mem, &desc->bControlSize, 1); \ > > + mem++; \ > > + \ > > + memcpy(mem, desc->bmControls, desc->bControlSize); \ > > + mem += desc->bControlSize; \ > > + \ > > + memcpy(mem, &desc->iExtension, 1); \ > > + mem++; \ > > + } while (0) > > + > > |I don't especially like UVC_COPY_XU_DESCRIPTOR(), but the need to vary > the array size for baSourceID and bmControls plus the requirement for > the struct to be copied to consecutive bytes of memory constrained it a > bit. An alternative might be to replace baSourceID, bControlSize, > bmControls and iExtension in struct uvcg_extension_unit_descriptor with > a single flexible array member (called data[] or something). That would > allow the copy to be a much more straight forward memcpy(mem, desc, > desc->bLength); - but the cost would be reallocating the entire struct > each time the baSourceID or bmControls attributes was changed. That > might be a better method, but I thought I'd stick with this for this > submission at least on the grounds that it's less confusing.| I think it's fine. I'm tempted to turn the macro into a function though. We can't do that with UVC_COPY_DESCRIPTOR() as the macro accepts different types of descriptors, but here you know that desc will be an XU descriptor. > > static struct usb_descriptor_header ** > > uvc_copy_descriptors(struct uvc_device *uvc, enum usb_device_speed speed) > > { > > @@ -475,6 +494,7 @@ uvc_copy_descriptors(struct uvc_device *uvc, enum usb_device_speed speed) > > const struct usb_descriptor_header * const *src; > > struct usb_descriptor_header **dst; > > struct usb_descriptor_header **hdr; > > + struct uvcg_extension *xu; > > unsigned int control_size; > > unsigned int streaming_size; > > unsigned int n_desc; > > @@ -539,6 +559,13 @@ uvc_copy_descriptors(struct uvc_device *uvc, enum usb_device_speed speed) > > bytes += (*src)->bLength; > > n_desc++; > > } > > + > > + list_for_each_entry(xu, uvc->desc.extension_units, list) { > > + control_size += xu->desc.bLength; > > + bytes += xu->desc.bLength; > > + n_desc++; > > + } > > + > > for (src = (const struct usb_descriptor_header **)uvc_streaming_cls; > > *src; ++src) { > > streaming_size += (*src)->bLength; > > @@ -565,6 +592,13 @@ uvc_copy_descriptors(struct uvc_device *uvc, enum usb_device_speed speed) > > uvc_control_header = mem; > > UVC_COPY_DESCRIPTORS(mem, dst, > > (const struct usb_descriptor_header **)uvc_control_desc); > > + > > + list_for_each_entry(xu, uvc->desc.extension_units, list) { > > + struct uvcg_extension_unit_descriptor *desc = &xu->desc; > > + > > + UVC_COPY_XU_DESCRIPTOR(mem, dst, desc); You could possibly skip the local variable UVC_COPY_XU_DESCRIPTOR(mem, dst, &xu->desc); Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > + } > > + > > uvc_control_header->wTotalLength = cpu_to_le16(control_size); > > uvc_control_header->bInCollection = 1; > > uvc_control_header->baInterfaceNr[0] = uvc->streaming_intf; > > @@ -988,6 +1022,7 @@ static struct usb_function *uvc_alloc(struct usb_function_instance *fi) > > uvc->desc.fs_streaming = opts->fs_streaming; > > uvc->desc.hs_streaming = opts->hs_streaming; > > uvc->desc.ss_streaming = opts->ss_streaming; > > + uvc->desc.extension_units = &opts->extension_units; > > > > streaming = config_group_find_item(&opts->func_inst.group, "streaming"); > > if (!streaming) > > diff --git a/drivers/usb/gadget/function/uvc.h b/drivers/usb/gadget/function/uvc.h > > index 40226b1f7e14..f1a016d20bb6 100644 > > --- a/drivers/usb/gadget/function/uvc.h > > +++ b/drivers/usb/gadget/function/uvc.h > > @@ -143,6 +143,7 @@ struct uvc_device { > > const struct uvc_descriptor_header * const *fs_streaming; > > const struct uvc_descriptor_header * const *hs_streaming; > > const struct uvc_descriptor_header * const *ss_streaming; > > + struct list_head *extension_units; > > } desc; > > > > unsigned int control_intf;
diff --git a/drivers/usb/gadget/function/f_uvc.c b/drivers/usb/gadget/function/f_uvc.c index eca5f36dfa74..e0a308f1355c 100644 --- a/drivers/usb/gadget/function/f_uvc.c +++ b/drivers/usb/gadget/function/f_uvc.c @@ -464,6 +464,25 @@ uvc_register_video(struct uvc_device *uvc) } \ } while (0) +#define UVC_COPY_XU_DESCRIPTOR(mem, dst, desc) \ + do { \ + *(dst)++ = mem; \ + memcpy(mem, desc, 22); /* bLength to bNrInPins */ \ + mem += 22; \ + \ + memcpy(mem, desc->baSourceID, desc->bNrInPins); \ + mem += desc->bNrInPins; \ + \ + memcpy(mem, &desc->bControlSize, 1); \ + mem++; \ + \ + memcpy(mem, desc->bmControls, desc->bControlSize); \ + mem += desc->bControlSize; \ + \ + memcpy(mem, &desc->iExtension, 1); \ + mem++; \ + } while (0) + static struct usb_descriptor_header ** uvc_copy_descriptors(struct uvc_device *uvc, enum usb_device_speed speed) { @@ -475,6 +494,7 @@ uvc_copy_descriptors(struct uvc_device *uvc, enum usb_device_speed speed) const struct usb_descriptor_header * const *src; struct usb_descriptor_header **dst; struct usb_descriptor_header **hdr; + struct uvcg_extension *xu; unsigned int control_size; unsigned int streaming_size; unsigned int n_desc; @@ -539,6 +559,13 @@ uvc_copy_descriptors(struct uvc_device *uvc, enum usb_device_speed speed) bytes += (*src)->bLength; n_desc++; } + + list_for_each_entry(xu, uvc->desc.extension_units, list) { + control_size += xu->desc.bLength; + bytes += xu->desc.bLength; + n_desc++; + } + for (src = (const struct usb_descriptor_header **)uvc_streaming_cls; *src; ++src) { streaming_size += (*src)->bLength; @@ -565,6 +592,13 @@ uvc_copy_descriptors(struct uvc_device *uvc, enum usb_device_speed speed) uvc_control_header = mem; UVC_COPY_DESCRIPTORS(mem, dst, (const struct usb_descriptor_header **)uvc_control_desc); + + list_for_each_entry(xu, uvc->desc.extension_units, list) { + struct uvcg_extension_unit_descriptor *desc = &xu->desc; + + UVC_COPY_XU_DESCRIPTOR(mem, dst, desc); + } + uvc_control_header->wTotalLength = cpu_to_le16(control_size); uvc_control_header->bInCollection = 1; uvc_control_header->baInterfaceNr[0] = uvc->streaming_intf; @@ -988,6 +1022,7 @@ static struct usb_function *uvc_alloc(struct usb_function_instance *fi) uvc->desc.fs_streaming = opts->fs_streaming; uvc->desc.hs_streaming = opts->hs_streaming; uvc->desc.ss_streaming = opts->ss_streaming; + uvc->desc.extension_units = &opts->extension_units; streaming = config_group_find_item(&opts->func_inst.group, "streaming"); if (!streaming) diff --git a/drivers/usb/gadget/function/uvc.h b/drivers/usb/gadget/function/uvc.h index 40226b1f7e14..f1a016d20bb6 100644 --- a/drivers/usb/gadget/function/uvc.h +++ b/drivers/usb/gadget/function/uvc.h @@ -143,6 +143,7 @@ struct uvc_device { const struct uvc_descriptor_header * const *fs_streaming; const struct uvc_descriptor_header * const *hs_streaming; const struct uvc_descriptor_header * const *ss_streaming; + struct list_head *extension_units; } desc; unsigned int control_intf;
Now that extension unit support is available through configfs we need to copy the descriptors for the XUs during uvc_function_bind() so that they're exposed to the usb subsystem. Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com> --- Changes in v2: - none drivers/usb/gadget/function/f_uvc.c | 35 +++++++++++++++++++++++++++++ drivers/usb/gadget/function/uvc.h | 1 + 2 files changed, 36 insertions(+)