| Message ID | 20180801215505.7658-8-laurent.pinchart@ideasonboard.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | usb: gadget: uvc: Improve configfs support | expand |
Hi Laurent, Joel On 01/08/18 22:55, Laurent Pinchart wrote: > From: Joel Pepper <joel.pepper@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. 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 <joel.pepper@rwth-aachen.de> > [Simplified documentation, renamed function, blank space update] > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > 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 <kieran.bingham@ideasonboard.com> > 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 = &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, > @@ -1294,6 +1333,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, > @@ -1373,6 +1413,22 @@ static void uvcg_frame_drop(struct config_group *group, struct config_item *item > config_item_put(item); > } > > +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> > */ >
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 <joel.pepper@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. > > 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 <joel.pepper@rwth-aachen.de> > > [Simplified documentation, renamed function, blank space update] > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > > > 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 :-( > With a decision made on that, and the typo fixed in the commit message: > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> Thank you. I'll keep the forward declaration for now. I might give it a try again later. > > 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 = &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, > > > > @@ -1294,6 +1333,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, > > > > @@ -1373,6 +1413,22 @@ static void uvcg_frame_drop(struct config_group > > *group, struct config_item *item> > > config_item_put(item); > > > > } > > > > +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> > > */
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 <joel.pepper@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. >> >> 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 <joel.pepper@rwth-aachen.de> >>> [Simplified documentation, renamed function, blank space update] >>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >>> --- >>> >>> 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 <kieran.bingham@ideasonboard.com> > > 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 = &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, >>> >>> @@ -1294,6 +1333,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, >>> >>> @@ -1373,6 +1413,22 @@ static void uvcg_frame_drop(struct config_group >>> *group, struct config_item *item> >>> config_item_put(item); >>> >>> } >>> >>> +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> >>> */ > >
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); + 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 = &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, @@ -1294,6 +1333,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, @@ -1373,6 +1413,22 @@ static void uvcg_frame_drop(struct config_group *group, struct config_item *item config_item_put(item); } +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> */