Hi Laurent,

looks good to me; I have no complaints.


On 13.06.2018 00:58, Laurent Pinchart wrote:
> From: Joel Pepper <joel.pep...@rwth-aachen.de>
>
> - 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.
>
> 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.
>
> Signed-off-by: Joel Pepper <joel.pep...@rwth-aachen.de>
> [Simplified documentation, renamed function, blank space update]
> Signed-off-by: Laurent Pinchart <laurent.pinch...@ideasonboard.com>
> ---
>  Documentation/ABI/testing/configfs-usb-gadget-uvc |  8 ++++
>  drivers/usb/gadget/function/uvc_configfs.c        | 56 
> +++++++++++++++++++++++
>  2 files changed, 64 insertions(+)
>
> Hi Joel,
>
> What do you think about this patch ? It retains your approach and
> addresses the few minor reviw comments I sent.
>
> diff --git a/Documentation/ABI/testing/configfs-usb-gadget-uvc 
> b/Documentation/ABI/testing/configfs-usb-gadget-uvc
> index 9896c8aa0e14..1efeb2e835ea 100644
> --- a/Documentation/ABI/testing/configfs-usb-gadget-uvc
> +++ b/Documentation/ABI/testing/configfs-usb-gadget-uvc
> @@ -181,6 +181,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
> @@ -232,6 +236,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 5c3ea5f89201..cac249e8a7ec 100644
> --- a/drivers/usb/gadget/function/uvc_configfs.c
> +++ b/drivers/usb/gadget/function/uvc_configfs.c
> @@ -841,6 +841,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);
> +
>  static int uvcg_streaming_header_allow_link(struct config_item *src,
>                                           struct config_item *target)
>  {
> @@ -888,6 +890,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;
> @@ -1129,6 +1133,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 = &f->item.ci_group->cg_subsys->su_mutex;
> +     int result;
> +
> +     mutex_lock(su_mutex); /* for navigating configfs hierarchy */
> +
> +     fmt_item = f->item.ci_parent;
> +     fmt = to_uvcg_format(fmt_item);
> +
> +     if (!fmt->linked) {
> +             result = -EBUSY;
> +             goto out;
> +     }
> +
> +     opts_item = fmt_item->ci_parent->ci_parent->ci_parent;
> +     opts = to_f_uvc_opts(opts_item);
> +
> +     mutex_lock(&opts->lock);
> +     result = sprintf(page, "%d\n", f->frame.b_frame_index);
> +     mutex_unlock(&opts->lock);
> +
> +out:
> +     mutex_unlock(su_mutex);
> +     return result;
> +}
> +
> +UVC_ATTR_RO(uvcg_frame_, b_frame_index, bFrameIndex);
> +
>  #define noop_conversion(x) (x)
>  
>  UVCG_FRAME_ATTR(bm_capabilities, bmCapabilities, noop_conversion,
> @@ -1277,6 +1316,7 @@ static ssize_t 
> uvcg_frame_dw_frame_interval_store(struct config_item *item,
>  UVC_ATTR(uvcg_frame_, dw_frame_interval, dwFrameInterval);
>  
>  static struct configfs_attribute *uvcg_frame_attrs[] = {
> +     &uvcg_frame_attr_b_frame_index,
>       &uvcg_frame_attr_bm_capabilities,
>       &uvcg_frame_attr_w_width,
>       &uvcg_frame_attr_w_height,
> @@ -1355,6 +1395,22 @@ static void uvcg_frame_drop(struct config_group 
> *group, struct config_item *item
>       mutex_unlock(&opts->lock);
>  }
>  
> +static void uvcg_format_set_indices(struct config_group *fmt)
> +{
> +     struct config_item *ci;
> +     unsigned int i = 1;
> +
> +     list_for_each_entry(ci, &fmt->cg_children, ci_entry) {
> +             struct uvcg_frame *frm;
> +
> +             if (ci->ci_type != &uvcg_frame_type)
> +                     continue;
> +
> +             frm = to_uvcg_frame(ci);
> +             frm->frame.b_frame_index = i++;
> +     }
> +}
> +
>  /* 
> -----------------------------------------------------------------------------
>   * streaming/uncompressed/<NAME>
>   */

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

Reply via email to