[v2,7/8] usb: gadget: uvc: configfs: Add bFrameIndex attributes

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

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