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@xxxxxxxxxxxxxxxx>
> >>
> >> 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@xxxxxxxxxxxxxxxx>
> >> ---
> >>
> >> 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.
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
