Skip to contentSkip to conversation

usb/gadget/uvc-configfs Fix host unable to negotiate framesizes other than first

PATCH RESEND New Laurent Pinchart 1 participant 1 message Jun 8, 2018 USB

lore
Share & tools
lore

Laurent Pinchart

author
#0 lore raw

PATCH RESEND v6 2/2 usb/gadget/uvc-configfs Fix host unable to negotiate framesizes other than first

Hi Joel,

Thank you for the patch, and sorry for the late reply.

Quoted 40 lines · On Tuesday, 29 May 2018 22:02:13 EEST Joel Pepper wrote:

- 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.

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.

Signed-off-by Joel Pepper <[email protected]>

---

v2: Add the new attribute to both MJPEG and uncompressed frame descriptors in Documentation/ABI, with note that it was added only in a later kernel version

v3: Changed from simply allowing user to set the value for bFrameIndex to automatically assigning correct distinct frame indexes. Changed bFrameIndex from RW to RO

v4: Actually include updated patch

v5: bFrameIndex now returns -EBUSY if the parent fmt is not linked yet

 Documentation/ABI/testing/configfs-usb-gadget-uvc | 18 +++++++
 drivers/usb/gadget/function/uvc_configfs.c        | 64 ++++++++++++++++++++
 2 files changed, 82 insertions(+)
(quoted diff) +0 -0

b/Documentation/ABI/testing/configfs-usb-gadget-uvc index 1ba0d0f..86c9b2e 100644

Documentation/ABI/testing/configfs-usb-gadget-uvc Description: Specific MJPEG frame descriptors · line 200

Quoted 7 lines
Documentation/ABI/testing/configfs-usb-gadget-uvc +4 -0
@@ -194,6 +194,14 @@ Description:	Specific MJPEG frame descriptors
 		bmCapabilities		- still image support, fixed frame-rate
 					support
 
+Date:		May 2018
+KernelVersion:	4.17
+
+		bFrameIndex		- unique id for this framedescriptor;

This should be spelled "frame descriptor", not "framedescriptor".

Documentation/ABI/testing/configfs-usb-gadget-uvc Description: Specific MJPEG frame descriptors · line 205

Quoted 5 lines
Documentation/ABI/testing/configfs-usb-gadget-uvc +4 -0
+					only defined after parent format is
+					linked into the streaming header;
+					read-only
+
 What:		/config/usb-gadget/gadget/functions/uvc.name/streaming/

uncompressed

Documentation/ABI/testing/configfs-usb-gadget-uvc Description: Specific uncompressed frame descriptors · line 256

Quoted 11 lines
Documentation/ABI/testing/configfs-usb-gadget-uvc +4 -0
 Date:		Dec 2014
 KernelVersion:	4.0

@@ -241,6 +249,16 @@ Description:	Specific uncompressed frame descriptors
 		bmCapabilities		- still image support, fixed frame-rate
 					support
 
+Date:           May 2018
+KernelVersion:  4.17
+
+                bFrameIndex             - unique id for this
 framedescriptor;

Same here.

Documentation/ABI/testing/configfs-usb-gadget-uvc Description: Specific uncompressed frame descriptors · line 259

Quoted 3 lines
Documentation/ABI/testing/configfs-usb-gadget-uvc +3 -0
+                                        only defined after parent format is
+                                        linked into the streaming header;
+                                        read-only

For some reason tabs got converted to space.

Documentation/ABI/testing/configfs-usb-gadget-uvc Description: Specific uncompressed frame descriptors · line 262

Quoted 3 lines
Documentation/ABI/testing/configfs-usb-gadget-uvc +3 -0
+
+
+

One blank line is enough.

drivers/usb/gadget/function/uvc_configfs.c static struct configfs_attribute prefix##attr_##cname = { · line 35

Quoted 12 lines
Documentation/ABI/testing/configfs-usb-gadget-uvc +0 -0
 What:		/config/usb-gadget/gadget/functions/uvc.name/streaming/header
 Date:		Dec 2014

KernelVersion: 4.0

(quoted diff) +0 -0

b/drivers/usb/gadget/function/uvc_configfs.c index 7a98f9f..37b4a14 100644

drivers/usb/gadget/function/uvc_configfs.c +1 -0
@@ -32,6 +32,7 @@ static struct configfs_attribute prefix##attr_##cname = {
 \ }
 
 static inline struct f_uvc_opts *to_f_uvc_opts(struct config_item *item);
+static void __uvcg_fmt_set_indices(struct config_group *fmt);

Please move the function before it gets called to avoid forward declarations.

drivers/usb/gadget/function/uvc_configfs.c static int uvcg_streaming_header_allow_link(struct · line 756

Quoted 8 lines
drivers/usb/gadget/function/uvc_configfs.c +2 -0
 /* control/header/<NAME> */
 DECLARE_UVC_HEADER_DESCRIPTOR(1);

@@ -751,6 +752,8 @@ static int uvcg_streaming_header_allow_link(struct
 config_item *src, if (!target_fmt)
 		goto out;
 
+	__uvcg_fmt_set_indices(to_config_group(target));
+

Is there a need to prefix the function name with __ ?

drivers/usb/gadget/function/uvc_configfs.c end: \ · line 997

Quoted 8 lines
drivers/usb/gadget/function/uvc_configfs.c +1 -0
 	format_ptr = kzalloc(sizeof(*format_ptr), GFP_KERNEL);
 	if (!format_ptr) {
 		ret = -ENOMEM;

@@ -991,8 +994,46 @@ end:									\
 									\
 UVC_ATTR(uvcg_frame_, cname, aname);
 
+

No need for a new blank line.

Quoted 25 lines
drivers/usb/gadget/function/uvc_configfs.c +25 -0
+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);

drivers/usb/gadget/function/uvc_configfs.c end: \ · line 1032

Quoted 10 lines
drivers/usb/gadget/function/uvc_configfs.c +10 -0
+	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);
+

As we already have UVCG_FRAME_ATTR, should we turn this into a UVCG_FRAME_ATTR_RO macro ?

Quoted 17 lines
drivers/usb/gadget/function/uvc_configfs.c +3 -0
 #define noop_conversion(x) (x)
 
+
+
 UVCG_FRAME_ATTR(bm_capabilities, bmCapabilities, noop_conversion,
 		noop_conversion, 8);
 UVCG_FRAME_ATTR(w_width, wWidth, le16_to_cpu, cpu_to_le16, 16);

@@ -1140,6 +1181,7 @@ UVC_ATTR(uvcg_frame_, dw_frame_interval,
 dwFrameInterval);
 
 static struct configfs_attribute *uvcg_frame_attrs[] = {
 	&uvcg_frame_attr_bm_capabilities,
+	&uvcg_frame_attr_b_frame_index,
 	&uvcg_frame_attr_w_width,
 	&uvcg_frame_attr_w_height,
drivers/usb/gadget/function/uvc_configfs.c +0 -0
 	&uvcg_frame_attr_dw_min_bit_rate,

drivers/usb/gadget/function/uvc_configfs.c static void uvcg_frame_drop(struct config_group · line 1264

Quoted 6 lines
drivers/usb/gadget/function/uvc_configfs.c +3 -0
@@ -1217,6 +1259,28 @@ static void uvcg_frame_drop(struct config_group
 *group, struct config_item *item mutex_unlock(&opts->lock);
 }
 
+static void __uvcg_fmt_set_indices(struct config_group *fmt)
+{
+	u8 i = 1;

I think you can simply use an unsigned int.

drivers/usb/gadget/function/uvc_configfs.c static void uvcg_frame_drop(struct config_group · line 1267

Quoted 3 lines
drivers/usb/gadget/function/uvc_configfs.c +3 -0
+	struct list_head *entry;
+	struct config_item *ci;
+	struct uvcg_frame *frm;

You could declare the last two variables inside the loop. I'll let you decide on this change, as the function is small and the variables are thus already close to the code that uses them.

drivers/usb/gadget/function/uvc_configfs.c static void uvcg_frame_drop(struct config_group · line 1270

Quoted 3 lines
drivers/usb/gadget/function/uvc_configfs.c +3 -0
+
+	list_for_each(entry, &(fmt->cg_children)) {
+

No need for a blank line here.

drivers/usb/gadget/function/uvc_configfs.c static void uvcg_frame_drop(struct config_group · line 1278

Quoted 8 lines
drivers/usb/gadget/function/uvc_configfs.c +8 -0
+		ci = container_of(entry, struct config_item, ci_entry);
+
+		if (ci->ci_type != &uvcg_frame_type)
+			continue;
+
+		frm = to_uvcg_frame(ci);
+		frm->frame.b_frame_index = i;
+		i++;

You could write this

		frm->frame.b_frame_index = i++;

drivers/usb/gadget/function/uvc_configfs.c static void uvcg_frame_drop(struct config_group · line 1279

Quoted 1 line
drivers/usb/gadget/function/uvc_configfs.c +1 -0
+

And no need for a blank line here either.

drivers/usb/gadget/function/uvc_configfs.c static void uvcg_frame_drop(struct config_group · line 1281

Quoted 2 lines
drivers/usb/gadget/function/uvc_configfs.c +2 -0
+	}
+

Same here.

drivers/usb/gadget/function/uvc_configfs.c static void uvcg_frame_drop(struct config_group · line 1286

Quoted 5 lines
drivers/usb/gadget/function/uvc_configfs.c +2 -0
+}
+
 /* streaming/uncompressed/<NAME> */
 struct uvcg_uncompressed {
 	struct uvcg_format		fmt;

Can't you set b_frame_index from __uvcg_fill_strm(), in a similar way as the b_format_index is set ? I've received a competing implementation from Paul in parallel to your patch that does so, I'll post it as a reply to this e-mail to discuss which approach is best.