usb: gadget: uvc: configfs: Add bFrameIndex attributes

PATCH New Laurent Pinchart 1 participant 1 message Sep 24, 2018 USB

lore

Share & tools

lore

Laurent Pinchart

author

2018-09-24 16:00 UTC #0 lore raw

PATCH v2 7/8 usb: gadget: uvc: configfs: Add bFrameIndex attributes

Hi Kieran,

Quoted 30 lines · On Monday, 24 September 2018 15:22:57 EEST Kieran Bingham wrote:

Quoted 11 lines · Laurent Pinchart
From: Joel Pepper <[email protected]>
- 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?)
Quoted 4 lines
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/
Quoted 8 lines
Signed-off-by Joel Pepper <[email protected]>
[Simplified documentation, renamed function, blank space update]
Signed-off-by Laurent Pinchart <[email protected]>
---
 Documentation/ABI/testing/configfs-usb-gadget-uvc |  8 ++++
 drivers/usb/gadget/function/uvc_configfs.c        | 56 ++++++++++++++++++
 2 files changed, 64 insertions(+)

[snip]

Quoted 11 lines

Quoted 11 lines
(quoted diff) +0 -0
b/drivers/usb/gadget/function/uvc_configfs.c index 5cee8aca3734..b8763343dcae 100644
drivers/usb/gadget/function/uvc_configfs.c +0 -0
@@ -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);
 
 }
 

Quoted 7 lines

drivers/usb/gadget/function/uvc_configfs.c static struct uvcg_streaming_header · line 874
Quoted 2 lines
drivers/usb/gadget/function/uvc_configfs.c +2 -0
+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 :-(

Quoted 3 lines

With a decision made on that, and the typo fixed in the commit message:
Reviewed-by Kieran Bingham <[email protected]>

Thank you. I'll keep the forward declaration for now. I might give it a try again later.

Quoted 106 lines

drivers/usb/gadget/function/uvc_configfs.c +0 -0

 static int uvcg_streaming_header_allow_link(struct config_item *src,

drivers/usb/gadget/function/uvc_configfs.c +0 -0

 					    struct config_item *target)
 
 {

@@ -915,6 +917,8 @@ static int uvcg_streaming_header_allow_link(struct

config_item *src,>

drivers/usb/gadget/function/uvc_configfs.c +37 -0

 	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,

drivers/usb/gadget/function/uvc_configfs.c +0 -0

@@ -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[] = {

drivers/usb/gadget/function/uvc_configfs.c +17 -0

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