Re: [PATCH 3/3] usb: gadget: uvc: Replace plain printk() with dev_*()
Hi Laurent,
On 18/09/18 11:35, Laurent Pinchart wrote:
> Adding device context to the kernel log messages make them more useful.
> Add new uvcg_* macros based on dev_*() that print both the gadget device
> name and the function name.
>
> While at it, remove a commented out printk statement and an unused
> printk-based macro.
>
> Signed-off-by: Laurent Pinchart
Great - looks good to me.
Reviewed-by: Kieran Bingham
> ---
> drivers/usb/gadget/function/f_uvc.c | 41
> ++---
> drivers/usb/gadget/function/uvc.h | 16 ++---
> drivers/usb/gadget/function/uvc_v4l2.c | 4 ++--
> drivers/usb/gadget/function/uvc_video.c | 18 +--
> drivers/usb/gadget/function/uvc_video.h | 2 +-
> 5 files changed, 39 insertions(+), 42 deletions(-)
>
> diff --git a/drivers/usb/gadget/function/f_uvc.c
> b/drivers/usb/gadget/function/f_uvc.c
> index 4ea987741e6e..0cc4a6220050 100644
> --- a/drivers/usb/gadget/function/f_uvc.c
> +++ b/drivers/usb/gadget/function/f_uvc.c
> @@ -232,13 +232,8 @@ uvc_function_setup(struct usb_function *f, const struct
> usb_ctrlrequest *ctrl)
> struct v4l2_event v4l2_event;
> struct uvc_event *uvc_event = (void *)_event.u.data;
>
> - /* printk(KERN_INFO "setup request %02x %02x value %04x index %04x
> %04x\n",
> - * ctrl->bRequestType, ctrl->bRequest, le16_to_cpu(ctrl->wValue),
> - * le16_to_cpu(ctrl->wIndex), le16_to_cpu(ctrl->wLength));
> - */
> -
> if ((ctrl->bRequestType & USB_TYPE_MASK) != USB_TYPE_CLASS) {
> - INFO(f->config->cdev, "invalid request type\n");
> + uvcg_info(f, "invalid request type\n");
> return -EINVAL;
> }
>
> @@ -272,7 +267,7 @@ uvc_function_get_alt(struct usb_function *f, unsigned
> interface)
> {
> struct uvc_device *uvc = to_uvc(f);
>
> - INFO(f->config->cdev, "uvc_function_get_alt(%u)\n", interface);
> + uvcg_info(f, "%s(%u)\n", __func__, interface);
>
> if (interface == uvc->control_intf)
> return 0;
> @@ -291,13 +286,13 @@ uvc_function_set_alt(struct usb_function *f, unsigned
> interface, unsigned alt)
> struct uvc_event *uvc_event = (void *)_event.u.data;
> int ret;
>
> - INFO(cdev, "uvc_function_set_alt(%u, %u)\n", interface, alt);
> + uvcg_info(f, "%s(%u, %u)\n", __func__, interface, alt);
>
> if (interface == uvc->control_intf) {
> if (alt)
> return -EINVAL;
>
> - INFO(cdev, "reset UVC Control\n");
> + uvcg_info(f, "reset UVC Control\n");
> usb_ep_disable(uvc->control_ep);
>
> if (!uvc->control_ep->desc)
> @@ -348,7 +343,7 @@ uvc_function_set_alt(struct usb_function *f, unsigned
> interface, unsigned alt)
> if (!uvc->video.ep)
> return -EINVAL;
>
> - INFO(cdev, "reset UVC\n");
> + uvcg_info(f, "reset UVC\n");
> usb_ep_disable(uvc->video.ep);
>
> ret = config_ep_by_speed(f->config->cdev->gadget,
> @@ -373,7 +368,7 @@ uvc_function_disable(struct usb_function *f)
> struct uvc_device *uvc = to_uvc(f);
> struct v4l2_event v4l2_event;
>
> - INFO(f->config->cdev, "uvc_function_disable\n");
> + uvcg_info(f, "%s()\n", __func__);
>
> memset(_event, 0, sizeof(v4l2_event));
> v4l2_event.type = UVC_EVENT_DISCONNECT;
> @@ -392,21 +387,19 @@ uvc_function_disable(struct usb_function *f)
> void
> uvc_function_connect(struct uvc_device *uvc)
> {
> - struct usb_composite_dev *cdev = uvc->func.config->cdev;
> int ret;
>
> if ((ret = usb_function_activate(>func)) < 0)
> - INFO(cdev, "UVC connect failed with %d\n", ret);
> + uvcg_info(>func, "UVC connect failed with %d\n", ret);
> }
>
> void
> uvc_function_disconnect(struct uvc_device *uvc)
> {
> - struct usb_composite_dev *cdev = uvc->func.config->cdev;
> int ret;
>
> if ((ret = usb_function_deactivate(>func)) < 0)
> - INFO(cdev, "UVC disconnect failed with %d\n", ret);
> + uvcg_info(>func, "UVC disconnect failed with %d\n", ret);
> }
>
> /* --
> @@ -605,7 +598,7 @@ uvc_function_bind(struct usb_configuration *c, struct
> usb_functio
Re: [PATCH 2/3] usb: gadget: uvc: Only halt video streaming endpoint in bulk mode
Hi Laurent,
On 18/09/18 11:35, Laurent Pinchart wrote:
> When USB requests for video data fail to be submitted, the driver
> signals a problem to the host by halting the video streaming endpoint.
> This is only valid in bulk mode, as isochronous transfers have no
> handshake phase and can't thus report a stall. The usb_ep_set_halt()
> call returns an error when using isochronous endpoints, which we happily
> ignore, but some UDCs complain in the kernel log. Fix this by only
> trying to halt the endpoint in bulk mode.
>
Agreed,
> Signed-off-by: Laurent Pinchart
Reviewed-by: Kieran Bingham
> ---
> drivers/usb/gadget/function/uvc_video.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/gadget/function/uvc_video.c
> b/drivers/usb/gadget/function/uvc_video.c
> index a95c8e2364ed..2c9821ec836e 100644
> --- a/drivers/usb/gadget/function/uvc_video.c
> +++ b/drivers/usb/gadget/function/uvc_video.c
> @@ -132,7 +132,9 @@ static int uvcg_video_ep_queue(struct uvc_video *video,
> struct usb_request *req)
> ret = usb_ep_queue(video->ep, req, GFP_ATOMIC);
> if (ret < 0) {
> printk(KERN_INFO "Failed to queue request (%d).\n", ret);
> - usb_ep_set_halt(video->ep);
> + /* Isochronous endpoints can't be halted. */
> + if (usb_endpoint_xfer_bulk(video->ep->desc))
> + usb_ep_set_halt(video->ep);
> }
>
> return ret;
>
--
Regards
--
Kieran
Re: [PATCH 1/3] usb: gadget: uvc: Factor out video USB request queueing
Hi Laurent,
On 18/09/18 11:35, Laurent Pinchart wrote:
> USB requests for video data are queued from two different locations in
> the driver, with the same code block occurring twice. Factor it out to a
> function.
>
> Signed-off-by: Laurent Pinchart
Looks good, and simplifies locking. Win win.
Reviewed-by: Kieran Bingham
> ---
> drivers/usb/gadget/function/uvc_video.c | 30 --
> 1 file changed, 20 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/usb/gadget/function/uvc_video.c
> b/drivers/usb/gadget/function/uvc_video.c
> index d3567b90343a..a95c8e2364ed 100644
> --- a/drivers/usb/gadget/function/uvc_video.c
> +++ b/drivers/usb/gadget/function/uvc_video.c
> @@ -125,6 +125,19 @@ uvc_video_encode_isoc(struct usb_request *req, struct
> uvc_video *video,
> * Request handling
> */
>
> +static int uvcg_video_ep_queue(struct uvc_video *video, struct usb_request
> *req)
> +{
> + int ret;
> +
> + ret = usb_ep_queue(video->ep, req, GFP_ATOMIC);
> + if (ret < 0) {
> + printk(KERN_INFO "Failed to queue request (%d).\n", ret);
> + usb_ep_set_halt(video->ep);
> + }
> +
> + return ret;
> +}
> +
> /*
> * I somehow feel that synchronisation won't be easy to achieve here. We have
> * three events that control USB requests submission:
> @@ -189,14 +202,13 @@ uvc_video_complete(struct usb_ep *ep, struct
> usb_request *req)
>
> video->encode(req, video, buf);
>
> - if ((ret = usb_ep_queue(ep, req, GFP_ATOMIC)) < 0) {
> - printk(KERN_INFO "Failed to queue request (%d).\n", ret);
> - usb_ep_set_halt(ep);
> - spin_unlock_irqrestore(>queue.irqlock, flags);
> + ret = uvcg_video_ep_queue(video, req);
> + spin_unlock_irqrestore(>queue.irqlock, flags);
> +
> + if (ret < 0) {
> uvcg_queue_cancel(queue, 0);
> goto requeue;
> }
> - spin_unlock_irqrestore(>queue.irqlock, flags);
>
> return;
>
> @@ -316,15 +328,13 @@ int uvcg_video_pump(struct uvc_video *video)
> video->encode(req, video, buf);
>
> /* Queue the USB request */
> - ret = usb_ep_queue(video->ep, req, GFP_ATOMIC);
> + ret = uvcg_video_ep_queue(video, req);
> + spin_unlock_irqrestore(>irqlock, flags);
> +
> if (ret < 0) {
> - printk(KERN_INFO "Failed to queue request (%d)\n", ret);
> - usb_ep_set_halt(video->ep);
> - spin_unlock_irqrestore(>irqlock, flags);
> uvcg_queue_cancel(queue, 0);
> break;
> }
> - spin_unlock_irqrestore(>irqlock, flags);
> }
>
> spin_lock_irqsave(>req_lock, flags);
>
--
Regards
--
Kieran
Re: [PATCH] usb: gadget: uvc: Remove uvc_set_trace_param() function
Hi Laurent,
On 18/09/18 13:25, Laurent Pinchart wrote:
> The function is never called, remove it.
It does seem that way :)
> Signed-off-by: Laurent Pinchart
Reviewed-by: Kieran Bingham
> ---
> drivers/usb/gadget/function/f_uvc.c | 6 --
> 1 file changed, 6 deletions(-)
>
> diff --git a/drivers/usb/gadget/function/f_uvc.c
> b/drivers/usb/gadget/function/f_uvc.c
> index 0cc4a6220050..8c99392df593 100644
> --- a/drivers/usb/gadget/function/f_uvc.c
> +++ b/drivers/usb/gadget/function/f_uvc.c
> @@ -197,12 +197,6 @@ static const struct usb_descriptor_header * const
> uvc_ss_streaming[] = {
> NULL,
> };
>
> -void uvc_set_trace_param(unsigned int trace)
> -{
> - uvc_gadget_trace_param = trace;
> -}
> -EXPORT_SYMBOL(uvc_set_trace_param);
> -
> /* --
> * Control requests
> */
>
--
Regards
--
Kieran
Re: [PATCH v2 7/8] usb: gadget: uvc: configfs: Add bFrameIndex attributes
Hi Laurent,
On 24/09/18 17:00, Laurent Pinchart wrote:
> Hi Kieran,
>
> On Monday, 24 September 2018 15:22:57 EEST Kieran Bingham wrote:
>> On 01/08/18 22:55, Laurent Pinchart wrote:
>>> From: Joel Pepper
>>>
>>> - Add bFrameIndex as a UVCG_FRAME_ATTR_RO for each frame size.
>>> - Automatically assign ascending bFrameIndex to each frame in a format.
>>>
>>> Before all "bFrameindex" attributes were set to "1" with no way to
>>> configure the gadget otherwise. This resulted in the host always
>>> negotiating for bFrameIndex 1 (i.e. the first framesize of the gadget).
>>> After the negotiation the host driver will set the user or application
>>> selected framesize, while the gadget is actually set to the first
>>> framesize.
>>
>> s/framesize/frame size/ *3 above ? (or alternatively frame-size?)
>>
>>> Now, when the containing format is linked into the streaming header,
>>> iterate over all child frame descriptors and assign ascending indices.
>>> The automatically assigned indices can be read from the new read only
>>> bFrameIndex configsfs attribute in each frame descriptor item.
>>
>> s/configsfs/configfs/
>>
>>> Signed-off-by: Joel Pepper
>>> [Simplified documentation, renamed function, blank space update]
>>> Signed-off-by: Laurent Pinchart
>>> ---
>>>
>>> Documentation/ABI/testing/configfs-usb-gadget-uvc | 8
>>> drivers/usb/gadget/function/uvc_configfs.c| 56 ++
>>> 2 files changed, 64 insertions(+)
>
> [snip]
>
>>> diff --git a/drivers/usb/gadget/function/uvc_configfs.c
>>> b/drivers/usb/gadget/function/uvc_configfs.c index
>>> 5cee8aca3734..b8763343dcae 100644
>>> --- a/drivers/usb/gadget/function/uvc_configfs.c
>>> +++ b/drivers/usb/gadget/function/uvc_configfs.c
>>> @@ -868,6 +868,8 @@ static struct uvcg_streaming_header
>>> *to_uvcg_streaming_header(struct config_item>
>>> return container_of(item, struct uvcg_streaming_header, item);
>>>
>>> }
>>>
>>> +static void uvcg_format_set_indices(struct config_group *fmt);
>>> +
>>
>> Do we need to forward declare here?
>>
>> Couldn't we move the uvcg_format_set_indices() implementation up ?
>> Or is it preferred to keep that code down in the lower section.
>
> You know how much I dislike forward-declarations. I tried to fix this one,
> but
> uvcg_format_set_indices() can't be moved up as-is as it depends on the
> definition of the uvcg_frame structure. I attempted to reorganize the code in
> a more logical way but gave up given how large the resulting change was even
> before I got it to compile, and how the new organization was less logical :-(
Aha - I missed that it was because of the structure definition. I had
only looked at the call hierarchy :-)
>
>> With a decision made on that, and the typo fixed in the commit message:
>>
>> Reviewed-by: Kieran Bingham
>
> Thank you. I'll keep the forward declaration for now. I might give it a try
> again later.
No problem.
--
Kieran
>
>>> static int uvcg_streaming_header_allow_link(struct config_item *src,
>>>
>>> struct config_item *target)
>>>
>>> {
>>>
>>> @@ -915,6 +917,8 @@ static int uvcg_streaming_header_allow_link(struct
>>> config_item *src,>
>>> if (!target_fmt)
>>>
>>> goto out;
>>>
>>> + uvcg_format_set_indices(to_config_group(target));
>>> +
>>>
>>> format_ptr = kzalloc(sizeof(*format_ptr), GFP_KERNEL);
>>> if (!format_ptr) {
>>>
>>> ret = -ENOMEM;
>>>
>>> @@ -1146,6 +1150,41 @@ end:
>>> \
>>>
>>> \
>>>
>>> UVC_ATTR(uvcg_frame_, cname, aname);
>>>
>>> +static ssize_t uvcg_frame_b_frame_index_show(struct config_item *item,
>>> +char *page)
>>> +{
>>> + struct uvcg_frame *f = to_uvcg_frame(item);
>>> + struct uvcg_format *fmt;
>>> + struct f_uvc_opts *opts;
>>> + struct config_item *opts_item;
>>> + struct config_item *fmt_item;
>>> + struct mutex *su_mutex = >item.ci_group->cg_subsys->su_mutex;
>>> + int result;
>>> +
>
Re: [PATCH v2 7/8] usb: gadget: uvc: configfs: Add bFrameIndex attributes
Hi Laurent, Joel
On 01/08/18 22:55, Laurent Pinchart wrote:
> From: Joel Pepper
>
> - Add bFrameIndex as a UVCG_FRAME_ATTR_RO for each frame size.
> - Automatically assign ascending bFrameIndex to each frame in a format.
>
> Before all "bFrameindex" attributes were set to "1" with no way to
> configure the gadget otherwise. This resulted in the host always
> negotiating for bFrameIndex 1 (i.e. the first framesize of the gadget).
> After the negotiation the host driver will set the user or application
> selected framesize, while the gadget is actually set to the first
> framesize.
s/framesize/frame size/ *3 above ? (or alternatively frame-size?)
>
> Now, when the containing format is linked into the streaming header,
> iterate over all child frame descriptors and assign ascending indices.
> The automatically assigned indices can be read from the new read only
> bFrameIndex configsfs attribute in each frame descriptor item.
s/configsfs/configfs/
>
> Signed-off-by: Joel Pepper
> [Simplified documentation, renamed function, blank space update]
> Signed-off-by: Laurent Pinchart
> ---
> Documentation/ABI/testing/configfs-usb-gadget-uvc | 8
> drivers/usb/gadget/function/uvc_configfs.c| 56
> +++
> 2 files changed, 64 insertions(+)
>
> diff --git a/Documentation/ABI/testing/configfs-usb-gadget-uvc
> b/Documentation/ABI/testing/configfs-usb-gadget-uvc
> index a6cc8d6d398e..809765bd9573 100644
> --- a/Documentation/ABI/testing/configfs-usb-gadget-uvc
> +++ b/Documentation/ABI/testing/configfs-usb-gadget-uvc
> @@ -189,6 +189,10 @@ Date:Dec 2014
> KernelVersion: 4.0
> Description: Specific MJPEG frame descriptors
>
> + bFrameIndex - unique id for this framedescriptor;
> + only defined after parent format is
> + linked into the streaming header;
> + read-only
> dwFrameInterval - indicates how frame interval can be
> programmed; a number of values
> separated by newline can be specified
> @@ -240,6 +244,10 @@ Date:Dec 2014
> KernelVersion: 4.0
> Description: Specific uncompressed frame descriptors
>
> + bFrameIndex - unique id for this framedescriptor;
> + only defined after parent format is
> + linked into the streaming header;
> + read-only
> dwFrameInterval - indicates how frame interval can be
> programmed; a number of values
> separated by newline can be specified
> diff --git a/drivers/usb/gadget/function/uvc_configfs.c
> b/drivers/usb/gadget/function/uvc_configfs.c
> index 5cee8aca3734..b8763343dcae 100644
> --- a/drivers/usb/gadget/function/uvc_configfs.c
> +++ b/drivers/usb/gadget/function/uvc_configfs.c
> @@ -868,6 +868,8 @@ static struct uvcg_streaming_header
> *to_uvcg_streaming_header(struct config_item
> return container_of(item, struct uvcg_streaming_header, item);
> }
>
> +static void uvcg_format_set_indices(struct config_group *fmt);
> +
Do we need to forward declare here?
Couldn't we move the uvcg_format_set_indices() implementation up ?
Or is it preferred to keep that code down in the lower section.
With a decision made on that, and the typo fixed in the commit message:
Reviewed-by: Kieran Bingham
> static int uvcg_streaming_header_allow_link(struct config_item *src,
> struct config_item *target)
> {
> @@ -915,6 +917,8 @@ static int uvcg_streaming_header_allow_link(struct
> config_item *src,
> if (!target_fmt)
> goto out;
>
> + uvcg_format_set_indices(to_config_group(target));
> +
> format_ptr = kzalloc(sizeof(*format_ptr), GFP_KERNEL);
> if (!format_ptr) {
> ret = -ENOMEM;
> @@ -1146,6 +1150,41 @@ end:
> \
> \
> UVC_ATTR(uvcg_frame_, cname, aname);
>
> +static ssize_t uvcg_frame_b_frame_index_show(struct config_item *item,
> + char *page)
> +{
> + struct uvcg_frame *f = to_uvcg_frame(item);
> + struct uvcg_format *fmt;
> + struct f_uvc_opts *opts;
> + struct config_item *opts_item;
> + struct config_item *fmt_item;
> + struct mutex *su_mutex = >it
Re: [PATCH v2 8/8] usb: gadget: uvc: configfs: Prevent format changes after linking header
Hi Laurent, Joel, On 01/08/18 22:55, Laurent Pinchart wrote: > From: Joel Pepper > > While checks are in place to avoid attributes and children of a format > being manipulated after the format is linked into the streaming header, > the linked flag was never actually set, invalidating the protections. That explains what's wrong, but not what we do about it. How about adding: "Update the flag as appropriate in the header link calls." > > Signed-off-by: Joel Pepper Missing and S-o-B from Laurent here? (I guess that will be added later?) Otherwise, Reviewed-by: Kieran Bingham > --- > drivers/usb/gadget/function/uvc_configfs.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/usb/gadget/function/uvc_configfs.c > b/drivers/usb/gadget/function/uvc_configfs.c > index b8763343dcae..799dc32c5bc7 100644 > --- a/drivers/usb/gadget/function/uvc_configfs.c > +++ b/drivers/usb/gadget/function/uvc_configfs.c > @@ -928,6 +928,7 @@ static int uvcg_streaming_header_allow_link(struct > config_item *src, > format_ptr->fmt = target_fmt; > list_add_tail(_ptr->entry, _hdr->formats); > ++src_hdr->num_fmt; > + ++target_fmt->linked; > > out: > mutex_unlock(>lock); > @@ -965,6 +966,8 @@ static void uvcg_streaming_header_drop_link(struct > config_item *src, > break; > } > > + --target_fmt->linked; > + > out: > mutex_unlock(>lock); > mutex_unlock(su_mutex); > -- Regards -- Kieran
Re: [PATCH v2 6/8] usb: gadget: uvc: configfs: Add bFormatIndex attributes
Hi Laurent,
On 01/08/18 22:55, Laurent Pinchart wrote:
> The UVC format description are numbered using the descriptor's
> bFormatIndex field. The index is used in UVC requests, and is thus
> needed to handle requests in userspace. Make it dynamically discoverable
> by exposing it in a bFormatIndex configfs attribute of the uncompressed
> and mjpeg format config items.
>
> The bFormatIndex value exposed through the attribute is stored in the
> config item private data. However, that value is never set: the driver
> instead computes the bFormatIndex value when linking the stream class
> header in the configfs hierarchy and stores it directly in the class
> descriptors in a separate structure. In order to expose the value
> through the configfs attribute, store it in the config item private data
> as well. This results in a small code simplification.
>
> Signed-off-by: Laurent Pinchart
> ---
>
> Changes since v1:
>
> - Squash patch "usb: gadget: uvc: configfs: Document the bFormatIndex
> attribute".
Thanks,
Reviewed-by: Kieran Bingham
> ---
> Documentation/ABI/testing/configfs-usb-gadget-uvc | 8
> drivers/usb/gadget/function/uvc_configfs.c| 14 --
> 2 files changed, 16 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/ABI/testing/configfs-usb-gadget-uvc
> b/Documentation/ABI/testing/configfs-usb-gadget-uvc
> index 490a0136fb02..a6cc8d6d398e 100644
> --- a/Documentation/ABI/testing/configfs-usb-gadget-uvc
> +++ b/Documentation/ABI/testing/configfs-usb-gadget-uvc
> @@ -168,6 +168,10 @@ Description: Specific MJPEG format descriptors
>
> All attributes read only,
> except bmaControls and bDefaultFrameIndex:
> + bFormatIndex- unique id for this format descriptor;
> + only defined after parent header is
> + linked into the streaming class;
> + read-only
> bmaControls - this format's data for bmaControls in
> the streaming header
> bmInterfaceFlags- specifies interlace information,
> @@ -212,6 +216,10 @@ Date:Dec 2014
> KernelVersion: 4.0
> Description: Specific uncompressed format descriptors
>
> + bFormatIndex- unique id for this format descriptor;
> + only defined after parent header is
> + linked into the streaming class;
> + read-only
> bmaControls - this format's data for bmaControls in
> the streaming header
> bmInterfaceFlags- specifies interlace information,
> diff --git a/drivers/usb/gadget/function/uvc_configfs.c
> b/drivers/usb/gadget/function/uvc_configfs.c
> index fa8d2e1f54ba..5cee8aca3734 100644
> --- a/drivers/usb/gadget/function/uvc_configfs.c
> +++ b/drivers/usb/gadget/function/uvc_configfs.c
> @@ -1538,6 +1538,7 @@ UVC_ATTR(uvcg_uncompressed_, cname, aname);
>
> #define identity_conv(x) (x)
>
> +UVCG_UNCOMPRESSED_ATTR_RO(b_format_index, bFormatIndex, identity_conv);
> UVCG_UNCOMPRESSED_ATTR(b_bits_per_pixel, bBitsPerPixel, identity_conv);
> UVCG_UNCOMPRESSED_ATTR(b_default_frame_index, bDefaultFrameIndex,
> identity_conv);
> @@ -1568,6 +1569,7 @@ uvcg_uncompressed_bma_controls_store(struct config_item
> *item,
> UVC_ATTR(uvcg_uncompressed_, bma_controls, bmaControls);
>
> static struct configfs_attribute *uvcg_uncompressed_attrs[] = {
> + _uncompressed_attr_b_format_index,
> _uncompressed_attr_guid_format,
> _uncompressed_attr_b_bits_per_pixel,
> _uncompressed_attr_b_default_frame_index,
> @@ -1738,6 +1740,7 @@ UVC_ATTR(uvcg_mjpeg_, cname, aname)
>
> #define identity_conv(x) (x)
>
> +UVCG_MJPEG_ATTR_RO(b_format_index, bFormatIndex, identity_conv);
> UVCG_MJPEG_ATTR(b_default_frame_index, bDefaultFrameIndex,
> identity_conv);
> UVCG_MJPEG_ATTR_RO(bm_flags, bmFlags, identity_conv);
> @@ -1768,6 +1771,7 @@ uvcg_mjpeg_bma_controls_store(struct config_item *item,
> UVC_ATTR(uvcg_mjpeg_, bma_controls, bmaControls);
>
> static struct configfs_attribute *uvcg_mjpeg_attrs[] = {
> + _mjpeg_attr_b_format_index,
> _mjpeg_attr_b_default_frame_index,
> _mjpeg_attr_bm_flags,
> _mjpeg_attr_b_aspect_ratio_x,
> @@ -2079,24 +2083,22 @@ static int __uvcg_fill_strm(void *priv1, void *priv2,
> void *priv3, int n,
> struct uvcg_format *fmt = priv1;
>
>
Re: [PATCH v2 5/8] usb: gadget: uvc: configfs: Add interface number attributes
Hi Laurent,
On 01/08/18 22:55, Laurent Pinchart wrote:
> The video control and video streaming interface numbers are needed in
> the UVC gadget userspace stack to reply to UVC requests. They are
> hardcoded to fixed values at the moment, preventing configurations with
> multiple functions.
>
> To fix this, make them dynamically discoverable by userspace through
> read-only configfs attributes in /control/bInterfaceNumber and
> /streaming/bInterfaceNumber respectively.
>
> Signed-off-by: Laurent Pinchart
> ---
> Changes since v1:
>
> - Document the new attribute
Perfect, Thank you.
Reviewed-by: Kieran Bingham
> ---
> Documentation/ABI/testing/configfs-usb-gadget-uvc | 8 +++
> drivers/usb/gadget/function/f_uvc.c | 2 +
> drivers/usb/gadget/function/u_uvc.h | 3 ++
> drivers/usb/gadget/function/uvc_configfs.c| 62
> +++
> 4 files changed, 75 insertions(+)
>
> diff --git a/Documentation/ABI/testing/configfs-usb-gadget-uvc
> b/Documentation/ABI/testing/configfs-usb-gadget-uvc
> index 9281e2aa38df..490a0136fb02 100644
> --- a/Documentation/ABI/testing/configfs-usb-gadget-uvc
> +++ b/Documentation/ABI/testing/configfs-usb-gadget-uvc
> @@ -12,6 +12,10 @@ Date: Dec 2014
> KernelVersion: 4.0
> Description: Control descriptors
>
> + All attributes read only:
> + bInterfaceNumber- USB interface number for this
> + streaming interface
> +
> What:
> /config/usb-gadget/gadget/functions/uvc.name/control/class
> Date:Dec 2014
> KernelVersion: 4.0
> @@ -109,6 +113,10 @@ Date:Dec 2014
> KernelVersion: 4.0
> Description: Streaming descriptors
>
> + All attributes read only:
> + bInterfaceNumber- USB interface number for this
> + streaming interface
> +
> What:
> /config/usb-gadget/gadget/functions/uvc.name/streaming/class
> Date:Dec 2014
> KernelVersion: 4.0
> diff --git a/drivers/usb/gadget/function/f_uvc.c
> b/drivers/usb/gadget/function/f_uvc.c
> index 95cb1b5f5ffe..4ea987741e6e 100644
> --- a/drivers/usb/gadget/function/f_uvc.c
> +++ b/drivers/usb/gadget/function/f_uvc.c
> @@ -699,12 +699,14 @@ uvc_function_bind(struct usb_configuration *c, struct
> usb_function *f)
> uvc_iad.bFirstInterface = ret;
> uvc_control_intf.bInterfaceNumber = ret;
> uvc->control_intf = ret;
> + opts->control_interface = ret;
>
> if ((ret = usb_interface_id(c, f)) < 0)
> goto error;
> uvc_streaming_intf_alt0.bInterfaceNumber = ret;
> uvc_streaming_intf_alt1.bInterfaceNumber = ret;
> uvc->streaming_intf = ret;
> + opts->streaming_interface = ret;
>
> /* Copy descriptors */
> f->fs_descriptors = uvc_copy_descriptors(uvc, USB_SPEED_FULL);
> diff --git a/drivers/usb/gadget/function/u_uvc.h
> b/drivers/usb/gadget/function/u_uvc.h
> index 2ed292e94fbc..5242d489e20a 100644
> --- a/drivers/usb/gadget/function/u_uvc.h
> +++ b/drivers/usb/gadget/function/u_uvc.h
> @@ -25,6 +25,9 @@ struct f_uvc_opts {
> unsigned intstreaming_maxpacket;
> unsigned intstreaming_maxburst;
>
> + unsigned intcontrol_interface;
> + unsigned intstreaming_interface;
> +
> /*
>* Control descriptors array pointers for full-/high-speed and
>* super-speed. They point by default to the uvc_fs_control_cls and
> diff --git a/drivers/usb/gadget/function/uvc_configfs.c
> b/drivers/usb/gadget/function/uvc_configfs.c
> index ae722549eabc..fa8d2e1f54ba 100644
> --- a/drivers/usb/gadget/function/uvc_configfs.c
> +++ b/drivers/usb/gadget/function/uvc_configfs.c
> @@ -713,9 +713,40 @@ static const struct uvcg_config_group_type
> uvcg_control_class_grp_type = {
> * control
> */
>
> +static ssize_t uvcg_default_control_b_interface_number_show(
> + struct config_item *item, char *page)
> +{
> + struct config_group *group = to_config_group(item);
> + struct mutex *su_mutex = >cg_subsys->su_mutex;
> + struct config_item *opts_item;
> + struct f_uvc_opts *opts;
> + int result = 0;
> +
> + mutex_lock(su_mutex); /* for navigating configfs hierarchy */
> +
> + opts_item = item->ci_parent;
> + opts = to_f_uvc_opts(opts_item);
> +
> + mutex_lock(>lock);
> + result += sprintf(page, "
Re: [PATCH v2 4/8] usb: gadget: uvc: configfs: Allocate groups dynamically
Hi Laurent,
On 01/08/18 22:55, Laurent Pinchart wrote:
> The UVC configfs implementation creates all groups as global static
> variables. This prevents creation of multiple UVC function instances,
> as they would all require their own configfs group instances.
>
> Fix this by allocating all groups dynamically. To avoid duplicating code
> around, extend the config_item_type structure with group name and
> children, and implement helper functions to create children
> automatically for most groups.
>
> Signed-off-by: Laurent Pinchart
> ---
> Changes since v1:
>
> - Free groups by implementing .release() handler and removing children
> explicitly.
Great - that resolves my concerns from the previous iteration.
Reviewed-by: Kieran Bingham
> ---
> drivers/usb/gadget/function/f_uvc.c| 8 +-
> drivers/usb/gadget/function/uvc_configfs.c | 581
> -
> 2 files changed, 338 insertions(+), 251 deletions(-)
>
> diff --git a/drivers/usb/gadget/function/f_uvc.c
> b/drivers/usb/gadget/function/f_uvc.c
> index d8ce7868fe22..95cb1b5f5ffe 100644
> --- a/drivers/usb/gadget/function/f_uvc.c
> +++ b/drivers/usb/gadget/function/f_uvc.c
> @@ -792,6 +792,7 @@ static struct usb_function_instance *uvc_alloc_inst(void)
> struct uvc_output_terminal_descriptor *od;
> struct uvc_color_matching_descriptor *md;
> struct uvc_descriptor_header **ctl_cls;
> + int ret;
>
> opts = kzalloc(sizeof(*opts), GFP_KERNEL);
> if (!opts)
> @@ -868,7 +869,12 @@ static struct usb_function_instance *uvc_alloc_inst(void)
> opts->streaming_interval = 1;
> opts->streaming_maxpacket = 1024;
>
> - uvcg_attach_configfs(opts);
> + ret = uvcg_attach_configfs(opts);
> + if (ret < 0) {
> + kfree(opts);
> + return ERR_PTR(ret);
> + }
> +
> return >func_inst;
> }
>
> diff --git a/drivers/usb/gadget/function/uvc_configfs.c
> b/drivers/usb/gadget/function/uvc_configfs.c
> index 8d513cc6fb8c..ae722549eabc 100644
> --- a/drivers/usb/gadget/function/uvc_configfs.c
> +++ b/drivers/usb/gadget/function/uvc_configfs.c
> @@ -41,6 +41,71 @@ static inline struct f_uvc_opts *to_f_uvc_opts(struct
> config_item *item)
> func_inst.group);
> }
>
> +struct uvcg_config_group_type {
> + struct config_item_type type;
> + const char *name;
> + const struct uvcg_config_group_type **children;
> + int (*create_children)(struct config_group *group);
> +};
> +
> +static void uvcg_config_item_release(struct config_item *item)
> +{
> + struct config_group *group = to_config_group(item);
> +
> + kfree(group);
> +}
> +
> +static struct configfs_item_operations uvcg_config_item_ops = {
> + .release= uvcg_config_item_release,
> +};
> +
> +static int uvcg_config_create_group(struct config_group *parent,
> + const struct uvcg_config_group_type *type);
> +
> +static int uvcg_config_create_children(struct config_group *group,
> + const struct uvcg_config_group_type *type)
> +{
> + const struct uvcg_config_group_type **child;
> + int ret;
> +
> + if (type->create_children)
> + return type->create_children(group);
> +
> + for (child = type->children; child && *child; ++child) {
> + ret = uvcg_config_create_group(group, *child);
> + if (ret < 0)
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static int uvcg_config_create_group(struct config_group *parent,
> + const struct uvcg_config_group_type *type)
> +{
> + struct config_group *group;
> +
> + group = kzalloc(sizeof(*group), GFP_KERNEL);
> + if (!group)
> + return -ENOMEM;
> +
> + config_group_init_type_name(group, type->name, >type);
> + configfs_add_default_group(group, parent);
> +
> + return uvcg_config_create_children(group, type);
> +}
> +
> +static void uvcg_config_remove_children(struct config_group *group)
> +{
> + struct config_group *child, *n;
> +
> + list_for_each_entry_safe(child, n, >default_groups, group_entry)
> {
> + list_del(>group_entry);
> + uvcg_config_remove_children(child);
> + config_item_put(>cg_item);
> + }
> +}
> +
> /*
> -
> * control/header/
> * control/header
> @@ -137,6 +202,7 @@ static struct configfs_attribute
> *uvcg_control_header_
Re: [PATCH v2 3/8] usb: gadget: uvc: configfs: Drop leaked references to config items
Hi Laurent, On 01/08/18 22:55, Laurent Pinchart wrote: > Some of the .allow_link() and .drop_link() operations implementations > call config_group_find_item() and then leak the reference to the > returned item. Fix this by dropping those references where needed. > > Signed-off-by: Laurent Pinchart Reviewed-by: Kieran Bingham > --- > drivers/usb/gadget/function/uvc_configfs.c | 4 > 1 file changed, 4 insertions(+) > > diff --git a/drivers/usb/gadget/function/uvc_configfs.c > b/drivers/usb/gadget/function/uvc_configfs.c > index dbc95c9558de..8d513cc6fb8c 100644 > --- a/drivers/usb/gadget/function/uvc_configfs.c > +++ b/drivers/usb/gadget/function/uvc_configfs.c > @@ -529,6 +529,7 @@ static int uvcg_control_class_allow_link(struct > config_item *src, > unlock: > mutex_unlock(>lock); > out: > + config_item_put(header); > mutex_unlock(su_mutex); > return ret; > } > @@ -564,6 +565,7 @@ static void uvcg_control_class_drop_link(struct > config_item *src, > unlock: > mutex_unlock(>lock); > out: > + config_item_put(header); > mutex_unlock(su_mutex); > } > > @@ -2026,6 +2028,7 @@ static int uvcg_streaming_class_allow_link(struct > config_item *src, > unlock: > mutex_unlock(>lock); > out: > + config_item_put(header); > mutex_unlock(su_mutex); > return ret; > } > @@ -2066,6 +2069,7 @@ static void uvcg_streaming_class_drop_link(struct > config_item *src, > unlock: > mutex_unlock(>lock); > out: > + config_item_put(header); > mutex_unlock(su_mutex); > } > > -- Regards -- Kieran
Re: [PATCH 6/8] usb: gadget: uvc: configfs: Document the bFormatIndex attribute
Hi Laurent, On 01/08/18 01:29, Laurent Pinchart wrote: > Document for the bFormatIndex attribute of the format descriptors is > missing. Add it. It might be missing, but only just since the previous patch. Perhaps this should squash into [5/8] ? > Signed-off-by: Laurent Pinchart > --- > Documentation/ABI/testing/configfs-usb-gadget-uvc | 8 > 1 file changed, 8 insertions(+) > > diff --git a/Documentation/ABI/testing/configfs-usb-gadget-uvc > b/Documentation/ABI/testing/configfs-usb-gadget-uvc > index 9281e2aa38df..9896c8aa0e14 100644 > --- a/Documentation/ABI/testing/configfs-usb-gadget-uvc > +++ b/Documentation/ABI/testing/configfs-usb-gadget-uvc > @@ -160,6 +160,10 @@ Description: Specific MJPEG format descriptors > > All attributes read only, > except bmaControls and bDefaultFrameIndex: > + bFormatIndex- unique id for this format descriptor; > + only defined after parent header is > + linked into the streaming class; > + read-only > bmaControls - this format's data for bmaControls in > the streaming header > bmInterfaceFlags- specifies interlace information, > @@ -204,6 +208,10 @@ Date:Dec 2014 > KernelVersion: 4.0 > Description: Specific uncompressed format descriptors > > + bFormatIndex- unique id for this format descriptor; > + only defined after parent header is > + linked into the streaming class; > + read-only > bmaControls - this format's data for bmaControls in > the streaming header > bmInterfaceFlags- specifies interlace information, > -- Regards -- Kieran -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/8] usb: gadget: uvc: configfs: Add interface number attributes
Hi Laurent,
Thank you for the patch,
On 01/08/18 01:29, Laurent Pinchart wrote:
> The video control and video streaming interface numbers are needed in
> the UVC gadget userspace stack to reply to UVC requests. They are
> hardcoded to fixed values at the moment, preventing configurations with
> multiple functions.
>
> To fix this, make them dynamically discoverable by userspace through
> read-only configfs attributes in /control/bInterfaceNumber and
> /streaming/bInterfaceNumber respectively.
>
> Signed-off-by: Laurent Pinchart
> ---
> drivers/usb/gadget/function/f_uvc.c| 2 +
> drivers/usb/gadget/function/u_uvc.h| 3 ++
> drivers/usb/gadget/function/uvc_configfs.c | 62
> ++
It sounds like this is extending the userspace ABI.
Do we need to document this anywhere?
Documentation/ABI/testing/configfs-usb-gadget-uvc ?
> 3 files changed, 67 insertions(+)
>
> diff --git a/drivers/usb/gadget/function/f_uvc.c
> b/drivers/usb/gadget/function/f_uvc.c
> index 95cb1b5f5ffe..4ea987741e6e 100644
> --- a/drivers/usb/gadget/function/f_uvc.c
> +++ b/drivers/usb/gadget/function/f_uvc.c
> @@ -699,12 +699,14 @@ uvc_function_bind(struct usb_configuration *c, struct
> usb_function *f)
> uvc_iad.bFirstInterface = ret;
> uvc_control_intf.bInterfaceNumber = ret;
> uvc->control_intf = ret;
> + opts->control_interface = ret;
>
> if ((ret = usb_interface_id(c, f)) < 0)
> goto error;
> uvc_streaming_intf_alt0.bInterfaceNumber = ret;
> uvc_streaming_intf_alt1.bInterfaceNumber = ret;
> uvc->streaming_intf = ret;
> + opts->streaming_interface = ret;
>
> /* Copy descriptors */
> f->fs_descriptors = uvc_copy_descriptors(uvc, USB_SPEED_FULL);
> diff --git a/drivers/usb/gadget/function/u_uvc.h
> b/drivers/usb/gadget/function/u_uvc.h
> index 2ed292e94fbc..5242d489e20a 100644
> --- a/drivers/usb/gadget/function/u_uvc.h
> +++ b/drivers/usb/gadget/function/u_uvc.h
> @@ -25,6 +25,9 @@ struct f_uvc_opts {
> unsigned intstreaming_maxpacket;
> unsigned intstreaming_maxburst;
>
> + unsigned intcontrol_interface;
> + unsigned intstreaming_interface;
> +
> /*
>* Control descriptors array pointers for full-/high-speed and
>* super-speed. They point by default to the uvc_fs_control_cls and
> diff --git a/drivers/usb/gadget/function/uvc_configfs.c
> b/drivers/usb/gadget/function/uvc_configfs.c
> index e019ea317c7a..801117686108 100644
> --- a/drivers/usb/gadget/function/uvc_configfs.c
> +++ b/drivers/usb/gadget/function/uvc_configfs.c
> @@ -687,8 +687,39 @@ static const struct uvcg_config_group_type
> uvcg_control_class_grp_type = {
> * control
> */
>
> +static ssize_t uvcg_default_control_b_interface_number_show(
> + struct config_item *item, char *page)
> +{
> + struct config_group *group = to_config_group(item);
> + struct mutex *su_mutex = >cg_subsys->su_mutex;
> + struct config_item *opts_item;
> + struct f_uvc_opts *opts;
> + int result = 0;
> +
> + mutex_lock(su_mutex); /* for navigating configfs hierarchy */
> +
> + opts_item = item->ci_parent;
> + opts = to_f_uvc_opts(opts_item);
> +
> + mutex_lock(>lock);
> + result += sprintf(page, "%u\n", opts->control_interface);
> + mutex_unlock(>lock);
> +
> + mutex_unlock(su_mutex);
> +
> + return result;
> +}
> +
> +UVC_ATTR_RO(uvcg_default_control_, b_interface_number, bInterfaceNumber);
> +
> +static struct configfs_attribute *uvcg_default_control_attrs[] = {
> + _default_control_attr_b_interface_number,
> + NULL,
> +};
> +
> static const struct uvcg_config_group_type uvcg_control_grp_type = {
> .type = {
> + .ct_attrs = uvcg_default_control_attrs,
> .ct_owner = THIS_MODULE,
> },
> .name = "control",
> @@ -2246,8 +2277,39 @@ static const struct uvcg_config_group_type
> uvcg_streaming_class_grp_type = {
> * streaming
> */
>
> +static ssize_t uvcg_default_streaming_b_interface_number_show(
> + struct config_item *item, char *page)
> +{
> + struct config_group *group = to_config_group(item);
> + struct mutex *su_mutex = >cg_subsys->su_mutex;
> + struct config_item *opts_item;
> + struct f_uvc_opts *opts;
> + int result = 0;
> +
> + mutex_lock(su_mutex); /* for navigating configfs hierarchy */
> +
> + opts_item = item->ci_parent;
> + opts = to_f_uvc_opts(opts_item);
> +
> + mutex_lock(>lock);
> + result += sprintf(page, "%u\n", opts->streaming_interface);
> + mutex_unlock(>lock);
> +
> + mutex_unlock(su_mutex);
> +
> + return result;
> +}
That looks like a lot of common boilerplate code copied for each file
which prints a single %u.
Perhaps we should convert those to a macro - but for now they look
Re: [PATCH 3/8] usb: gadget: uvc: configfs: Allocate groups dynamically
Hi Laurent,
Thank you for the patch,
On 01/08/18 01:29, Laurent Pinchart wrote:
> The UVC configfs implementation creates all groups as global static
> variables. This prevents creationg of multiple UVC function instances,
/creationg/creation/
> as they would all require their own configfs group instances.
>
> Fix this by allocating all groups dynamically. To avoid duplicating code
> around, extend the config_item_type structure with group name and
> children, and implement helper functions to create children
> automatically for most groups.
>
> Signed-off-by: Laurent Pinchart
I'm struggling to see what paths free all of the dynamically created
children in this patch.
Is this already supported by the config_group framework?
I see a reference to config_group_put(>func_inst.group); in one
error path - but that's about it.
Am I missing something nice and obvious? (or is it already handled by
framework code not in this patch)
In fact, I can't see how it could be handled by core - as the children
are added to a new structure you have created.
I'll let you look into this :)
> ---
> drivers/usb/gadget/function/f_uvc.c| 8 +-
> drivers/usb/gadget/function/uvc_configfs.c | 480
> +
> 2 files changed, 282 insertions(+), 206 deletions(-)
>
> diff --git a/drivers/usb/gadget/function/f_uvc.c
> b/drivers/usb/gadget/function/f_uvc.c
> index d8ce7868fe22..95cb1b5f5ffe 100644
> --- a/drivers/usb/gadget/function/f_uvc.c
> +++ b/drivers/usb/gadget/function/f_uvc.c
> @@ -792,6 +792,7 @@ static struct usb_function_instance *uvc_alloc_inst(void)
> struct uvc_output_terminal_descriptor *od;
> struct uvc_color_matching_descriptor *md;
> struct uvc_descriptor_header **ctl_cls;
> + int ret;
>
> opts = kzalloc(sizeof(*opts), GFP_KERNEL);
> if (!opts)
> @@ -868,7 +869,12 @@ static struct usb_function_instance *uvc_alloc_inst(void)
> opts->streaming_interval = 1;
> opts->streaming_maxpacket = 1024;
>
> - uvcg_attach_configfs(opts);
> + ret = uvcg_attach_configfs(opts);
> + if (ret < 0) {
> + kfree(opts);
> + return ERR_PTR(ret);
> + }
> +
> return >func_inst;
> }
>
> diff --git a/drivers/usb/gadget/function/uvc_configfs.c
> b/drivers/usb/gadget/function/uvc_configfs.c
> index dbc95c9558de..e019ea317c7a 100644
> --- a/drivers/usb/gadget/function/uvc_configfs.c
> +++ b/drivers/usb/gadget/function/uvc_configfs.c
> @@ -41,6 +41,49 @@ static inline struct f_uvc_opts *to_f_uvc_opts(struct
> config_item *item)
> func_inst.group);
> }
>
> +struct uvcg_config_group_type {
> + struct config_item_type type;
> + const char *name;
> + const struct uvcg_config_group_type **children;
> + int (*create_children)(struct config_group *group);
> +};
> +
> +static int uvcg_config_create_group(struct config_group *parent,
> + const struct uvcg_config_group_type *type);
> +
> +static int uvcg_config_create_children(struct config_group *group,
> + const struct uvcg_config_group_type *type)
> +{
> + const struct uvcg_config_group_type **child;
> + int ret;
> +
> + if (type->create_children)
> + return type->create_children(group);
> +
> + for (child = type->children; child && *child; ++child) {
> + ret = uvcg_config_create_group(group, *child);
> + if (ret < 0)
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static int uvcg_config_create_group(struct config_group *parent,
> + const struct uvcg_config_group_type *type)
> +{
> + struct config_group *group;
> +
> + group = kzalloc(sizeof(*group), GFP_KERNEL);
> + if (!group)
> + return -ENOMEM;
> +
> + config_group_init_type_name(group, type->name, >type);
> + configfs_add_default_group(group, parent);
> +
> + return uvcg_config_create_children(group, type);
> +}
> +
> /*
> -
> * control/header/
> * control/header
> @@ -169,24 +212,23 @@ static void uvcg_control_header_drop(struct
> config_group *group,
> kfree(h);
> }
>
> -static struct config_group uvcg_control_header_grp;
> -
> static struct configfs_group_operations uvcg_control_header_grp_ops = {
> .make_item = uvcg_control_header_make,
> .drop_item = uvcg_control_header_drop,
> };
>
> -static const struct config_item_type uvcg_control_header_grp_type = {
> - .ct_group_ops = _control_header_grp_ops,
> - .ct_owner = THIS_MODULE,
> +static const struct uvcg_config_group_type uvcg_control_header_grp_type = {
> + .type = {
> + .ct_group_ops = _control_header_grp_ops,
> + .ct_owner = THIS_MODULE,
> + },
> + .name = "header",
> };
>
> /*
>
Re: [PATCH 2/8] usb: gadget: uvc: configfs: Add section header comments
Hi Laurent,
A trivial but nice improvement.
On 01/08/18 01:29, Laurent Pinchart wrote:
> The UVC configfs implementation is large and difficult to navigate. Add
> a bit more air to the code to make it easier to read.
>
> Signed-off-by: Laurent Pinchart
Acked-by: Kieran Bingham
> ---
> drivers/usb/gadget/function/uvc_configfs.c | 120
> ++---
> 1 file changed, 91 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/usb/gadget/function/uvc_configfs.c
> b/drivers/usb/gadget/function/uvc_configfs.c
> index 1df94b25abe1..dbc95c9558de 100644
> --- a/drivers/usb/gadget/function/uvc_configfs.c
> +++ b/drivers/usb/gadget/function/uvc_configfs.c
> @@ -12,6 +12,10 @@
> #include "u_uvc.h"
> #include "uvc_configfs.h"
>
> +/*
> -
> + * Global Utility Structures and Macros
> + */
> +
> #define UVCG_STREAMING_CONTROL_SIZE 1
>
> #define UVC_ATTR(prefix, cname, aname) \
> @@ -37,7 +41,11 @@ static inline struct f_uvc_opts *to_f_uvc_opts(struct
> config_item *item)
> func_inst.group);
> }
>
> -/* control/header/ */
> +/*
> -
> + * control/header/
> + * control/header
> + */
> +
> DECLARE_UVC_HEADER_DESCRIPTOR(1);
>
> struct uvcg_control_header {
> @@ -161,7 +169,6 @@ static void uvcg_control_header_drop(struct config_group
> *group,
> kfree(h);
> }
>
> -/* control/header */
> static struct config_group uvcg_control_header_grp;
>
> static struct configfs_group_operations uvcg_control_header_grp_ops = {
> @@ -174,7 +181,10 @@ static const struct config_item_type
> uvcg_control_header_grp_type = {
> .ct_owner = THIS_MODULE,
> };
>
> -/* control/processing/default */
> +/*
> -
> + * control/processing/default
> + */
> +
> static struct config_group uvcg_default_processing_grp;
>
> #define UVCG_DEFAULT_PROCESSING_ATTR(cname, aname, conv) \
> @@ -260,16 +270,20 @@ static const struct config_item_type
> uvcg_default_processing_type = {
> .ct_owner = THIS_MODULE,
> };
>
> -/* struct uvcg_processing {}; */
> +/*
> -
> + * control/processing
> + */
>
> -/* control/processing */
> static struct config_group uvcg_processing_grp;
>
> static const struct config_item_type uvcg_processing_grp_type = {
> .ct_owner = THIS_MODULE,
> };
>
> -/* control/terminal/camera/default */
> +/*
> -
> + * control/terminal/camera/default
> + */
> +
> static struct config_group uvcg_default_camera_grp;
>
> #define UVCG_DEFAULT_CAMERA_ATTR(cname, aname, conv) \
> @@ -366,16 +380,20 @@ static const struct config_item_type
> uvcg_default_camera_type = {
> .ct_owner = THIS_MODULE,
> };
>
> -/* struct uvcg_camera {}; */
> +/*
> -
> + * control/terminal/camera
> + */
>
> -/* control/terminal/camera */
> static struct config_group uvcg_camera_grp;
>
> static const struct config_item_type uvcg_camera_grp_type = {
> .ct_owner = THIS_MODULE,
> };
>
> -/* control/terminal/output/default */
> +/*
> -
> + * control/terminal/output/default
> + */
> +
> static struct config_group uvcg_default_output_grp;
>
> #define UVCG_DEFAULT_OUTPUT_ATTR(cname, aname, conv) \
> @@ -433,23 +451,30 @@ static const struct config_item_type
> uvcg_default_output_type = {
> .ct_owner = THIS_MODULE,
> };
>
> -/* struct uvcg_output {}; */
> +/*
> -
> + * control/terminal/output
> + */
>
> -/* control/terminal/output */
> static struct config_group uvcg_output_grp;
>
> static const struct config_item_type uvcg_output_grp_type = {
> .ct_owner = THIS_MODULE,
> };
>
> -/* control/terminal */
> +/*
> -
> + * control/terminal
> + */
> +
> static struct config_group uvcg_terminal_grp;
>
> static const struct config_item_type uvcg_terminal_grp_type = {
> .ct_owner
Re: [PATCH 1/8] usb: gadget: uvc: configfs: Don't wrap groups unnecessarily
Hi Laurent,
Thank you for the patch,
On 01/08/18 01:29, Laurent Pinchart wrote:
> Various configfs groups (represented by config_group) as wrapped in
/as wrapped/are wrapped/
> structures that they're the only member of. This allows adding other
> data fields to groups, but is unnecessarily makes the code more complex.
s/is/it/
> Remove the outter structures and use config_group directly to simplify
/outter/outer/
> the code. Groups can still be wrapped individually in the future if
> other data fields need to be added.
>
> Signed-off-by: Laurent Pinchart
phew. Well that is some terse reading of macro modifications.
But except for the trivial spellings above, I can't see anything wrong.
And this definitely moves the right way :D
Reviewed-by: Kieran Bingham
> ---
> drivers/usb/gadget/function/uvc_configfs.c | 302
> +++--
> 1 file changed, 117 insertions(+), 185 deletions(-)
>
> diff --git a/drivers/usb/gadget/function/uvc_configfs.c
> b/drivers/usb/gadget/function/uvc_configfs.c
> index b51f0d278826..1df94b25abe1 100644
> --- a/drivers/usb/gadget/function/uvc_configfs.c
> +++ b/drivers/usb/gadget/function/uvc_configfs.c
> @@ -162,9 +162,7 @@ static void uvcg_control_header_drop(struct config_group
> *group,
> }
>
> /* control/header */
> -static struct uvcg_control_header_grp {
> - struct config_group group;
> -} uvcg_control_header_grp;
> +static struct config_group uvcg_control_header_grp;
>
> static struct configfs_group_operations uvcg_control_header_grp_ops = {
> .make_item = uvcg_control_header_make,
> @@ -177,31 +175,22 @@ static const struct config_item_type
> uvcg_control_header_grp_type = {
> };
>
> /* control/processing/default */
> -static struct uvcg_default_processing {
> - struct config_group group;
> -} uvcg_default_processing;
> -
> -static inline struct uvcg_default_processing
> -*to_uvcg_default_processing(struct config_item *item)
> -{
> - return container_of(to_config_group(item),
> - struct uvcg_default_processing, group);
> -}
> +static struct config_group uvcg_default_processing_grp;
>
> #define UVCG_DEFAULT_PROCESSING_ATTR(cname, aname, conv) \
> static ssize_t uvcg_default_processing_##cname##_show(
> \
> struct config_item *item, char *page) \
> {\
> - struct uvcg_default_processing *dp = to_uvcg_default_processing(item); \
> + struct config_group *group = to_config_group(item); \
> struct f_uvc_opts *opts;\
> struct config_item *opts_item; \
> - struct mutex *su_mutex = >group.cg_subsys->su_mutex;\
> + struct mutex *su_mutex = >cg_subsys->su_mutex; \
> struct uvc_processing_unit_descriptor *pd; \
> int result; \
> \
> mutex_lock(su_mutex); /* for navigating configfs hierarchy */ \
> \
> - opts_item = dp->group.cg_item.ci_parent->ci_parent->ci_parent; \
> + opts_item = group->cg_item.ci_parent->ci_parent->ci_parent; \
> opts = to_f_uvc_opts(opts_item);\
> pd = >uvc_processing; \
> \
> @@ -229,17 +218,17 @@ UVCG_DEFAULT_PROCESSING_ATTR(i_processing, iProcessing,
> identity_conv);
> static ssize_t uvcg_default_processing_bm_controls_show(
> struct config_item *item, char *page)
> {
> - struct uvcg_default_processing *dp = to_uvcg_default_processing(item);
> + struct config_group *group = to_config_group(item);
> struct f_uvc_opts *opts;
> struct config_item *opts_item;
> - struct mutex *su_mutex = >group.cg_subsys->su_mutex;
> + struct mutex *su_mutex = >cg_subsys->su_mutex;
> struct uvc_processing_unit_descriptor *pd;
> int result, i;
> char *pg = page;
>
> mutex_lock(su_mutex); /* for navigating configfs hierarchy */
>
> - opts_item = dp->group.cg_item.ci_parent->ci_parent->ci_parent;
> + opts_item = group->cg_item.ci_parent->ci_parent->ci_parent;
> opts = to_f_uvc_opts(opts_item);
> pd = >uvc_processing;
>
> @@ -274,40 +263,29 @@ static const struct config_item_type
> uvcg
[PATCH v2] usb: gadget: uvc: Expose configuration name through video node
From: Kieran Bingham <kieran.bing...@ideasonboard.com>
When utilising multiple instantiations of a UVC gadget on a composite
device, there is no clear method to link a particular configuration to
its respective video node.
Provide a means for identifying the correct video node by exposing the
name of the function configuration through sysfs.
Signed-off-by: Kieran Bingham <kieran.bing...@ideasonboard.com>
---
v2:
- Fix commit title (f_uvc -> uvc)
- Change identifier file name (now function_name)
- Document in ABI
.../ABI/testing/configfs-usb-gadget-uvc | 5
drivers/usb/gadget/function/f_uvc.c | 24 ++-
2 files changed, 28 insertions(+), 1 deletion(-)
diff --git a/Documentation/ABI/testing/configfs-usb-gadget-uvc
b/Documentation/ABI/testing/configfs-usb-gadget-uvc
index 1ba0d0fda9c0..9281e2aa38df 100644
--- a/Documentation/ABI/testing/configfs-usb-gadget-uvc
+++ b/Documentation/ABI/testing/configfs-usb-gadget-uvc
@@ -263,3 +263,8 @@ Description:Specific streaming header descriptors
is connected
bmInfo - capabilities of this video streaming
interface
+
+What:
/sys/class/udc/udc.name/device/gadget/video4linux/video.name/function_name
+Date: May 2018
+KernelVersion: 4.19
+Description: UVC configfs function instance name
diff --git a/drivers/usb/gadget/function/f_uvc.c
b/drivers/usb/gadget/function/f_uvc.c
index d82cd61676d3..c8627cc698f8 100644
--- a/drivers/usb/gadget/function/f_uvc.c
+++ b/drivers/usb/gadget/function/f_uvc.c
@@ -421,10 +421,21 @@ uvc_function_disconnect(struct uvc_device *uvc)
* USB probe and disconnect
*/
+static ssize_t function_name_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct uvc_device *uvc = dev_get_drvdata(dev);
+
+ return sprintf(buf, "%s\n", uvc->func.fi->group.cg_item.ci_name);
+}
+
+static DEVICE_ATTR_RO(function_name);
+
static int
uvc_register_video(struct uvc_device *uvc)
{
struct usb_composite_dev *cdev = uvc->func.config->cdev;
+ int ret;
/* TODO reference counting. */
uvc->vdev.v4l2_dev = >v4l2_dev;
@@ -437,7 +448,17 @@ uvc_register_video(struct uvc_device *uvc)
video_set_drvdata(>vdev, uvc);
- return video_register_device(>vdev, VFL_TYPE_GRABBER, -1);
+ ret = video_register_device(>vdev, VFL_TYPE_GRABBER, -1);
+ if (ret < 0)
+ return ret;
+
+ ret = device_create_file(>vdev.dev, _attr_function_name);
+ if (ret < 0) {
+ video_unregister_device(>vdev);
+ return ret;
+ }
+
+ return 0;
}
#define UVC_COPY_DESCRIPTOR(mem, dst, desc) \
@@ -877,6 +898,7 @@ static void uvc_unbind(struct usb_configuration *c, struct
usb_function *f)
INFO(cdev, "%s\n", __func__);
+ device_remove_file(>vdev.dev, _attr_function_name);
video_unregister_device(>vdev);
v4l2_device_unregister(>v4l2_dev);
--
2.17.0
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] usb: gadget: f_uvc: Expose configuration name through video node
From: Kieran Bingham <kieran.bing...@ideasonboard.com>
When utilising multiple instantiations of a UVC gadget on a composite
device, there is no clear method to link a particular configuration to
its respective video node.
Provide a means for identifying the correct video node by exposing the
name of the function configuration through sysfs.
Signed-off-by: Kieran Bingham <kieran.bing...@ideasonboard.com>
---
drivers/usb/gadget/function/f_uvc.c | 24 +++-
1 file changed, 23 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/gadget/function/f_uvc.c
b/drivers/usb/gadget/function/f_uvc.c
index 1affa8e3a974..f326d1707633 100644
--- a/drivers/usb/gadget/function/f_uvc.c
+++ b/drivers/usb/gadget/function/f_uvc.c
@@ -421,10 +421,21 @@ uvc_function_disconnect(struct uvc_device *uvc)
* USB probe and disconnect
*/
+static ssize_t config_show(struct device *dev, struct device_attribute *attr,
+ char *buf)
+{
+ struct uvc_device *uvc = dev_get_drvdata(dev);
+
+ return sprintf(buf, "%s\n", uvc->func.fi->group.cg_item.ci_name);
+}
+
+static DEVICE_ATTR_RO(config);
+
static int
uvc_register_video(struct uvc_device *uvc)
{
struct usb_composite_dev *cdev = uvc->func.config->cdev;
+ int ret;
/* TODO reference counting. */
uvc->vdev.v4l2_dev = >v4l2_dev;
@@ -437,7 +448,17 @@ uvc_register_video(struct uvc_device *uvc)
video_set_drvdata(>vdev, uvc);
- return video_register_device(>vdev, VFL_TYPE_GRABBER, -1);
+ ret = video_register_device(>vdev, VFL_TYPE_GRABBER, -1);
+ if (ret < 0)
+ return ret;
+
+ ret = device_create_file(>vdev.dev, _attr_config);
+ if (ret < 0) {
+ video_unregister_device(>vdev);
+ return ret;
+ }
+
+ return 0;
}
#define UVC_COPY_DESCRIPTOR(mem, dst, desc) \
@@ -875,6 +896,7 @@ static void uvc_unbind(struct usb_configuration *c, struct
usb_function *f)
INFO(cdev, "%s\n", __func__);
+ device_remove_file(>vdev.dev, _attr_config);
video_unregister_device(>vdev);
v4l2_device_unregister(>v4l2_dev);
--
2.17.0
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] usb: gadget: uvc: Utilise instance name in video name
From: Kieran Bingham <kieran.bing...@ideasonboard.com> With multiple UVC gadgets on a composite device, the device names become indistinguishable from one another. Extend the gadget video name to incorporate the function instance name, along side the existing UDC controller name. Signed-off-by: Kieran Bingham <kieran.bing...@ideasonboard.com> --- drivers/usb/gadget/function/f_uvc.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/drivers/usb/gadget/function/f_uvc.c b/drivers/usb/gadget/function/f_uvc.c index 1affa8e3a974..65454a31ad68 100644 --- a/drivers/usb/gadget/function/f_uvc.c +++ b/drivers/usb/gadget/function/f_uvc.c @@ -433,7 +433,9 @@ uvc_register_video(struct uvc_device *uvc) uvc->vdev.release = video_device_release_empty; uvc->vdev.vfl_dir = VFL_DIR_TX; uvc->vdev.lock = >video.mutex; - strlcpy(uvc->vdev.name, cdev->gadget->name, sizeof(uvc->vdev.name)); + + snprintf(uvc->vdev.name, sizeof(uvc->vdev.name), "%s:%s", +cdev->gadget->name, uvc->func.fi->group.cg_item.ci_name); video_set_drvdata(>vdev, uvc); -- 2.17.0 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Pandaboard OMAP4 MUSB short packet hang on UVC gadget
Hi Bin, Felipe, We have been trying to get a UVC gadget operational on the Pandaboard ES platform, and we believe we've hit an issue with short packets on the MUSB in DMA mode. Using the g_webcam module, I can instantiate a UVC video node, and use the uvc-gadget helper application [0] to handle the frame generation. With DMA disabled using CONFIG_MUSB_PIO_ONLY I am able to process frames on the host using yavta [1] However when DMA is enabled, only the first frame is transmitted to the host, and I get an error from the musb_log debug prints which indicate that the MUSB_TXCSR_TXPKTRDY is not cleared following a short packet. A capture of the musb_log trace-points [2] shows the following at the end of the log: The 'mode' has been set to '0' instead of '1', buffer length is 902 instead of 1024 (expected behaviour for the last packet): "ep13-Tx pkt_sz 1024, dma_addr 0xadfbbc00 length 902, mode 0" And then there is a fault reported: "ep13 old packet still ready , txcsr 2003" The host (linux, running the uvc driver with extra debug prints) reports: [24517.711147] uvcvideo: decode_data: len: 1022, maxlen 460800 [24517.759117] uvcvideo: decode_data: len: 1022, maxlen 459778 ... [24529.375018] uvcvideo: decode_data: len: 1022, maxlen 2944 [24529.399073] uvcvideo: decode_data: len: 1022, maxlen 1922 [24529.427014] uvcvideo: decode_data: len: 900, maxlen 900 And receives no further data following the end of the first frame. Are there any known issues on the musb-gadget with short-packet handling - or any other tests I can perform to check this ? Should I be looking to try to get the txstate() call to re-execute after a delay (using musb_queue_resume_work() perhaps?) or does this indicate that the hardware has jammed ? Stopping the pipeline (the yavta capture) and restarting, will successfully transfer (only) the first frame again. Any other hints and tips appreciated! [0] git://git.ideasonboard.org/uvc-gadget.git [1] git://git.ideasonboard.org/yavta.git [2] http://paste.ubuntu.com/p/QHrh5XzZv8/ -- Regards Kieran signature.asc Description: OpenPGP digital signature
