| Message ID | 20180608191256.739-1-laurent.pinchart@ideasonboard.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Joel and Paul, On Friday, 8 June 2018 22:12:56 EEST Laurent Pinchart wrote: > From: Paul Elder <paul.elder@ideasonboard.com> > > The UVC frame descriptions are numbered using the descriptor's > bFrameIndex 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 bFrameIndex configfs attribute of the frame > config item. > > However, the bFrameIndex value is never set to begin with. Set these > values when linking the stream class header in the configfs hierarchy, > and store it in the config item private data. > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > --- > drivers/usb/gadget/function/uvc_configfs.c | 10 ++++++++-- > 1 file changed, 8 insertions(+), 2 deletions(-) > > diff --git a/drivers/usb/gadget/function/uvc_configfs.c > b/drivers/usb/gadget/function/uvc_configfs.c index > 0ad6ea57d0b7..882d9963b244 100644 > --- a/drivers/usb/gadget/function/uvc_configfs.c > +++ b/drivers/usb/gadget/function/uvc_configfs.c > @@ -1152,6 +1152,8 @@ UVC_ATTR(uvcg_frame_, cname, aname); > > #define noop_conversion(x) (x) > > +UVCG_FRAME_ATTR_RO(b_frame_index, bFrameIndex, noop_conversion, > + noop_conversion, 8); > UVCG_FRAME_ATTR(bm_capabilities, bmCapabilities, noop_conversion, > noop_conversion, 8); > UVCG_FRAME_ATTR(w_width, wWidth, le16_to_cpu, cpu_to_le16, 16); > @@ -1298,6 +1300,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, > @@ -2130,12 +2133,15 @@ static int __uvcg_fill_strm(void *priv1, void > *priv2, void *priv3, int n, sizeof(*frm->dw_frame_interval); > memcpy(*dest, frm->dw_frame_interval, sz); > *dest += sz; > - if (frm->fmt_type == UVCG_UNCOMPRESSED) > + if (frm->fmt_type == UVCG_UNCOMPRESSED) { > h->bLength = UVC_DT_FRAME_UNCOMPRESSED_SIZE( > frm->frame.b_frame_interval_type); > - else if (frm->fmt_type == UVCG_MJPEG) > + frm->frame.b_frame_index = n + 1; > + } else if (frm->fmt_type == UVCG_MJPEG) { > h->bLength = UVC_DT_FRAME_MJPEG_SIZE( > frm->frame.b_frame_interval_type); > + frm->frame.b_frame_index = n + 1; > + } > } > break; > } This approach looks simpler to me than the patch that Joel submitted. However, this patch lacks ABI documentation, which is included in Joel's patch. Joel, as you were first, I propose taking your patch and modifying it with the approach followed here, using the UVCG_FRAME_ATTR_RO macro and setting b_frame_index in __uvcg_fill_strm(). Would that be OK with you ? Or is there a reason why the approach taken in your patch would be better ?
@Laurent, I accidentally hit "Reply" instead of "Reply All", please disregard the previous copy of this email Hi Laurent and Paul, On 08.06.2018 21:16, Laurent Pinchart wrote: > Hi Joel and Paul, > > On Friday, 8 June 2018 22:12:56 EEST Laurent Pinchart wrote: >> From: Paul Elder <paul.elder@ideasonboard.com> >> >> The UVC frame descriptions are numbered using the descriptor's >> bFrameIndex 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 bFrameIndex configfs attribute of the frame >> config item. >> >> However, the bFrameIndex value is never set to begin with. Set these >> values when linking the stream class header in the configfs hierarchy, >> and store it in the config item private data. >> >> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> >> --- >> drivers/usb/gadget/function/uvc_configfs.c | 10 ++++++++-- >> 1 file changed, 8 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/usb/gadget/function/uvc_configfs.c >> b/drivers/usb/gadget/function/uvc_configfs.c index >> 0ad6ea57d0b7..882d9963b244 100644 >> --- a/drivers/usb/gadget/function/uvc_configfs.c >> +++ b/drivers/usb/gadget/function/uvc_configfs.c >> @@ -1152,6 +1152,8 @@ UVC_ATTR(uvcg_frame_, cname, aname); >> >> #define noop_conversion(x) (x) >> >> +UVCG_FRAME_ATTR_RO(b_frame_index, bFrameIndex, noop_conversion, >> + noop_conversion, 8); >> UVCG_FRAME_ATTR(bm_capabilities, bmCapabilities, noop_conversion, >> noop_conversion, 8); >> UVCG_FRAME_ATTR(w_width, wWidth, le16_to_cpu, cpu_to_le16, 16); >> @@ -1298,6 +1300,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, >> @@ -2130,12 +2133,15 @@ static int __uvcg_fill_strm(void *priv1, void >> *priv2, void *priv3, int n, sizeof(*frm->dw_frame_interval); >> memcpy(*dest, frm->dw_frame_interval, sz); >> *dest += sz; >> - if (frm->fmt_type == UVCG_UNCOMPRESSED) >> + if (frm->fmt_type == UVCG_UNCOMPRESSED) { >> h->bLength = UVC_DT_FRAME_UNCOMPRESSED_SIZE( >> frm->frame.b_frame_interval_type); >> - else if (frm->fmt_type == UVCG_MJPEG) >> + frm->frame.b_frame_index = n + 1; >> + } else if (frm->fmt_type == UVCG_MJPEG) { >> h->bLength = UVC_DT_FRAME_MJPEG_SIZE( >> frm->frame.b_frame_interval_type); >> + frm->frame.b_frame_index = n + 1; >> + } >> } >> break; >> } > > This approach looks simpler to me than the patch that Joel submitted. However, > this patch lacks ABI documentation, which is included in Joel's patch. > > Joel, as you were first, I propose taking your patch and modifying it with the > approach followed here, using the UVCG_FRAME_ATTR_RO macro and setting > b_frame_index in __uvcg_fill_strm(). Would that be OK with you ? Or is there a > reason why the approach taken in your patch would be better ? > I've compared Paul's implementation to mine, and as far as I can tell, they are functionally equivalent except for two points: 1) I specifically opted against using a generic UVCG_FRAME_ATTR_RO macro, because the index has no well defined value until they are all set at once, as such reading the attribute beforehand should return -EBUSY before that point (instead of 1 for all indexes, which would be the case now). All other attributes can be read immediately after creation. Because my approach sets the frame indexes when the parent format is linked, I check the target_fmt->linked flag; For Paul's approach we would have to check whether __uvcg_fill_strm() has already been called. Considering, that the format indices are set in the same fashion, we should probably do the same for bFormatIndex. 2) My approach creates the bFrameIndexes as soon as a format has been finalized and linked. Depending on how an application sets up the gadget, this might make it easier to take note of the created indices at the earliest possible time instead of rescanning everything at the end. I do not think however this is a significant enough advantage to warrant taking a different approach compared to the way format indices are already being set. Cheers, Joël Pepper -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Joel, On Sunday, 10 June 2018 14:06:28 EEST Joel Pepper wrote: > @Laurent, I accidentally hit "Reply" instead of "Reply All", please > disregard the previous copy of this email > > Hi Laurent and Paul, > > On 08.06.2018 21:16, Laurent Pinchart wrote: > > Hi Joel and Paul, > > > > On Friday, 8 June 2018 22:12:56 EEST Laurent Pinchart wrote: > >> From: Paul Elder <paul.elder@ideasonboard.com> > >> > >> The UVC frame descriptions are numbered using the descriptor's > >> bFrameIndex 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 bFrameIndex configfs attribute of the frame > >> config item. > >> > >> However, the bFrameIndex value is never set to begin with. Set these > >> values when linking the stream class header in the configfs hierarchy, > >> and store it in the config item private data. > >> > >> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > >> --- > >> > >> drivers/usb/gadget/function/uvc_configfs.c | 10 ++++++++-- > >> 1 file changed, 8 insertions(+), 2 deletions(-) > >> > >> diff --git a/drivers/usb/gadget/function/uvc_configfs.c > >> b/drivers/usb/gadget/function/uvc_configfs.c index > >> 0ad6ea57d0b7..882d9963b244 100644 > >> --- a/drivers/usb/gadget/function/uvc_configfs.c > >> +++ b/drivers/usb/gadget/function/uvc_configfs.c > >> @@ -1152,6 +1152,8 @@ UVC_ATTR(uvcg_frame_, cname, aname); > >> > >> #define noop_conversion(x) (x) > >> > >> +UVCG_FRAME_ATTR_RO(b_frame_index, bFrameIndex, noop_conversion, > >> + noop_conversion, 8); > >> > >> UVCG_FRAME_ATTR(bm_capabilities, bmCapabilities, noop_conversion, > >> > >> noop_conversion, 8); > >> > >> UVCG_FRAME_ATTR(w_width, wWidth, le16_to_cpu, cpu_to_le16, 16); > >> > >> @@ -1298,6 +1300,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, > >> > >> @@ -2130,12 +2133,15 @@ static int __uvcg_fill_strm(void *priv1, void > >> *priv2, void *priv3, int n, sizeof(*frm->dw_frame_interval); > >> > >> memcpy(*dest, frm->dw_frame_interval, sz); > >> *dest += sz; > >> > >> - if (frm->fmt_type == UVCG_UNCOMPRESSED) > >> + if (frm->fmt_type == UVCG_UNCOMPRESSED) { > >> > >> h->bLength = UVC_DT_FRAME_UNCOMPRESSED_SIZE( > >> > >> frm->frame.b_frame_interval_type); > >> > >> - else if (frm->fmt_type == UVCG_MJPEG) > >> + frm->frame.b_frame_index = n + 1; > >> + } else if (frm->fmt_type == UVCG_MJPEG) { > >> > >> h->bLength = UVC_DT_FRAME_MJPEG_SIZE( > >> > >> frm->frame.b_frame_interval_type); > >> > >> + frm->frame.b_frame_index = n + 1; > >> + } > >> > >> } > >> break; > >> } > > > > This approach looks simpler to me than the patch that Joel submitted. > > However, > > > this patch lacks ABI documentation, which is included in Joel's patch. > > > > Joel, as you were first, I propose taking your patch and modifying it > > with the > > > approach followed here, using the UVCG_FRAME_ATTR_RO macro and setting > > b_frame_index in __uvcg_fill_strm(). Would that be OK with you ? Or is > > there a > > > reason why the approach taken in your patch would be better ? > > I've compared Paul's implementation to mine, and as far as I can tell, > they are functionally equivalent except for two points: > > 1) I specifically opted against using a generic UVCG_FRAME_ATTR_RO > macro, because the index has no well defined value until they are all > set at once, as such reading the attribute beforehand should return > -EBUSY before that point (instead of 1 for all indexes, which would be > the case now). All other attributes can be read immediately after > creation. Because my approach sets the frame indexes when the parent > format is linked, I check the target_fmt->linked flag; For Paul's > approach we would have to check whether __uvcg_fill_strm() has already > been called. Considering, that the format indices are set in the same > fashion, we should probably do the same for bFormatIndex. > > 2) My approach creates the bFrameIndexes as soon as a format has been > finalized and linked. Depending on how an application sets up the > gadget, this might make it easier to take note of the created indices at > the earliest possible time instead of rescanning everything at the end. > I do not think however this is a significant enough advantage to warrant > taking a different approach compared to the way format indices are > already being set. Those are very valid points. I agree that your version is better.
diff --git a/drivers/usb/gadget/function/uvc_configfs.c b/drivers/usb/gadget/function/uvc_configfs.c index 0ad6ea57d0b7..882d9963b244 100644 --- a/drivers/usb/gadget/function/uvc_configfs.c +++ b/drivers/usb/gadget/function/uvc_configfs.c @@ -1152,6 +1152,8 @@ UVC_ATTR(uvcg_frame_, cname, aname); #define noop_conversion(x) (x) +UVCG_FRAME_ATTR_RO(b_frame_index, bFrameIndex, noop_conversion, + noop_conversion, 8); UVCG_FRAME_ATTR(bm_capabilities, bmCapabilities, noop_conversion, noop_conversion, 8); UVCG_FRAME_ATTR(w_width, wWidth, le16_to_cpu, cpu_to_le16, 16); @@ -1298,6 +1300,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, @@ -2130,12 +2133,15 @@ static int __uvcg_fill_strm(void *priv1, void *priv2, void *priv3, int n, sizeof(*frm->dw_frame_interval); memcpy(*dest, frm->dw_frame_interval, sz); *dest += sz; - if (frm->fmt_type == UVCG_UNCOMPRESSED) + if (frm->fmt_type == UVCG_UNCOMPRESSED) { h->bLength = UVC_DT_FRAME_UNCOMPRESSED_SIZE( frm->frame.b_frame_interval_type); - else if (frm->fmt_type == UVCG_MJPEG) + frm->frame.b_frame_index = n + 1; + } else if (frm->fmt_type == UVCG_MJPEG) { h->bLength = UVC_DT_FRAME_MJPEG_SIZE( frm->frame.b_frame_interval_type); + frm->frame.b_frame_index = n + 1; + } } break; }