Re: [PATCH v2 4/9] usb: gadget: uvc: Copy XU descriptors during .bind()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxxxxxxxx>

> > +	}
> > +
> >   	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;

-- 
Regards,

Laurent Pinchart



[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux