[2/2] usb: gadget: uvc: configfs: Add bFrameIndex attributes

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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.