[PATCHv2] usb/gadget/uvc-configs Fix host unable to negotiate framesizes other than first

[PATCHv2] usb/gadget/uvc-configs Fix host unable to negotiate framesizes other than first

[Joel Pepper]

Add bFrameIndex as a UVCG_FRAME_ATTR for each frame size.
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.

Note that this still requires the gadget to be configured with unique
bFrameIndexes for each frameSize of each format through configfs. An
alternative might be to automatically assign ascending indices when the
format is linked into the streaming header, but the user space gadget
would need a way to check or predict the indices so that it can properly
interpret to PROBE/COMMIT CONTROL requests

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

Signed-off-by: Joel Pepper <joel.pepper@rwth-aachen.de>
---
 Documentation/ABI/testing/configfs-usb-gadget-uvc | 17 +++++++++++++++++
 drivers/usb/gadget/function/uvc_configfs.c        |  3 +++
 2 files changed, 20 insertions(+)

diff --git a/Documentation/ABI/testing/configfs-usb-gadget-uvc b/Documentation/ABI/testing/configfs-usb-gadget-uvc
index 1ba0d0f..d435cf7 100644
--- a/Documentation/ABI/testing/configfs-usb-gadget-uvc
+++ b/Documentation/ABI/testing/configfs-usb-gadget-uvc
@@ -194,6 +194,14 @@ Description:	Specific MJPEG frame descriptors
 		bmCapabilities		- still image support, fixed frame-rate
 					support
 
+Date:		Mar 2018
+KernelVersion:	4.16
+
+		bFrameIndex		- unique id for this framedescriptor;
+					if using multiple framedescriptors for
+					same format, user needs to set distinct
+					value for each frame descriptor
+
 What:		/config/usb-gadget/gadget/functions/uvc.name/streaming/uncompressed
 Date:		Dec 2014
 KernelVersion:	4.0
@@ -241,6 +249,15 @@ Description:	Specific uncompressed frame descriptors
 		bmCapabilities		- still image support, fixed frame-rate
 					support
 
+Date:           Mar 2018
+KernelVersion:  4.16
+
+                bFrameIndex             - unique id for this framedescriptor;
+                                        if using multiple framedescriptors for
+                                        same format, user needs to set distinct
+                                        value for each frame descriptor
+
+
 What:		/config/usb-gadget/gadget/functions/uvc.name/streaming/header
 Date:		Dec 2014
 KernelVersion:	4.0
diff --git a/drivers/usb/gadget/function/uvc_configfs.c b/drivers/usb/gadget/function/uvc_configfs.c
index c9b8cc4a..5966d65 100644
--- a/drivers/usb/gadget/function/uvc_configfs.c
+++ b/drivers/usb/gadget/function/uvc_configfs.c
@@ -992,6 +992,8 @@ UVC_ATTR(uvcg_frame_, cname, aname);
 
 UVCG_FRAME_ATTR(bm_capabilities, bmCapabilities, noop_conversion,
 		noop_conversion, 8);
+UVCG_FRAME_ATTR(b_frame_index, bFrameIndex, noop_conversion,
+		noop_conversion, 8);
 UVCG_FRAME_ATTR(w_width, wWidth, le16_to_cpu, cpu_to_le16, 16);
 UVCG_FRAME_ATTR(w_height, wHeight, le16_to_cpu, cpu_to_le16, 16);
 UVCG_FRAME_ATTR(dw_min_bit_rate, dwMinBitRate, le32_to_cpu, cpu_to_le32, 32);
@@ -1137,6 +1139,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,
 	&uvcg_frame_attr_dw_min_bit_rate,

[PATCHv2] usb/gadget/uvc-configs Fix host unable to negotiate framesizes other than first

[Laurent Pinchart]

Hi Joel,

On Tuesday, 20 March 2018 12:30:00 EET Joel Pepper wrote:
> On 20.03.2018 10:02, Laurent Pinchart wrote:
> > Hi Joel,
> > 
> > (CC'ing Paul Elder who is working on the UVC gadget driver)
> > 
> > Thank you for the patch.
> > 
> > On Monday, 19 March 2018 21:36:41 EET Joel Pepper wrote:
> >> Add bFrameIndex as a UVCG_FRAME_ATTR for each frame size.
> >> 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.
> >> 
> >> Note that this still requires the gadget to be configured with unique
> >> bFrameIndexes for each frameSize of each format through configfs. An
> >> alternative might be to automatically assign ascending indices when the
> >> format is linked into the streaming header, but the user space gadget
> >> would need a way to check or predict the indices so that it can properly
> >> interpret to PROBE/COMMIT CONTROL requests
> > 
> > It would indeed be nice if the indices could be automatically generated,
> > to avoid giving userspace another possibility to create invalid
> > descriptors. As you've correctly noted, that would require a way for the
> > userspace helper application to coordinate with the UVC gadget driver.
> > Would it be difficult to do so by defining the bFrameIndex attribute as
> > read-only ? There's the additional issue of finding a place to store the
> > index counter locally to the format, I'm not sure how that's done with the
> > configfs API, but if it's not too difficult I think it would be a good
> > option.
> 
> Setting bFrameIndex to be read-only will suffice in allowing the
> userspace gadget to coordinate with the driver at the (slight) expense of
> the userspace gadget not being able to dictate an order which is
> semantically meaningful to it.
> 
> There are two approaches I see towards assigning indices automatically:
> 1) Assign a new unique index in "uvcg_frame_make" (i.e. config item
> creation)
> 2) Assign indices when exact set of frame descriptors is finalized (i.e.
> linking into the streaming header)
> 
> As for 1):
> I think this is the approach you are referring to with regards to
> storing the index counter in the format. There already is a 'num_frames'
> unsigned int in the uvcg_format struct, which could be used as index,
> however this approach does not handle deleting frame descriptors well,
> because num_frames is decremented then. Unless frames are always deleted in
> reverse order of creation this will produce index collisions. If we instead
> use a new monotonously incrementing counter, deleting frames will produce
> "holes" in the indices (I think UVC does allow for that but it might trip
> up host drivers not expecting that). The counter would also overflow after
> creating 255 frames even if the final number is much lower.
> 
> 
> As for 2):
> I personally prefer this approach as it avoids the problems of 1). It
> depends on identifying a point in time when the set of frame descriptors
> cannot change anymore I think this is given when the format is linked into
> the streaming header. It should then suffice to iterate of the contained
> frames and assign ascending indices. A small problem however is that the
> attribute will have no meaningful value to be read before the format is
> linked. This fact will need to be communicated in the documentation and
> potentially through returning EBUSY on premature read attempts.
> 
> I'll prepare a patch implementing 2), but in the meantime I'm grateful
> for more input, as there is not necessarily a single best approach to be
> taken here.

I like the second approach for the reasons you've described above.

Returning -EBUSY might not even be necessary, as before the gadget is fully 
setup there will be no V4L2 device registered anyway, and the userspace helper 
application will thus not be able to proceed. It however seems a good idea to 
let userspace know that the attribute isn't ready yet, so I agree with your 
proposal.

> >> v2: Add the new attribute to both MJPEG and uncompressedframe descriptos
> >> in Documentation/ABI, with note that it was added only in a later
> >> kernel version
> >> 
> >> Signed-off-by: Joel Pepper <joel.pepper@rwth-aachen.de>
> >> ---
> >> 
> >>  Documentation/ABI/testing/configfs-usb-gadget-uvc | 17 +++++++++++++++++
> >>  drivers/usb/gadget/function/uvc_configfs.c        |  3 +++
> >>  2 files changed, 20 insertions(+)
> >> 
> >> diff --git a/Documentation/ABI/testing/configfs-usb-gadget-uvc
> >> b/Documentation/ABI/testing/configfs-usb-gadget-uvc index
> >> 1ba0d0f..d435cf7
> >> 100644
> >> --- a/Documentation/ABI/testing/configfs-usb-gadget-uvc
> >> +++ b/Documentation/ABI/testing/configfs-usb-gadget-uvc
> >> @@ -194,6 +194,14 @@ Description:	Specific MJPEG frame descriptors
> >> 
> >>  		bmCapabilities		- still image support, fixed frame-rate
> >>  		
> >>  					support
> >> 
> >> +Date:		Mar 2018
> >> +KernelVersion:	4.16
> >> +
> >> +		bFrameIndex		- unique id for this framedescriptor;
> >> +					if using multiple framedescriptors for
> >> +					same format, user needs to set distinct
> >> +					value for each frame descriptor
> >> +
> >> 
> >>  What:		/config/usb-gadget/gadget/functions/uvc.name/streaming/
> > 
> > uncompressed
> > 
> >>  Date:		Dec 2014
> >>  KernelVersion:	4.0
> >> 
> >> @@ -241,6 +249,15 @@ Description:	Specific uncompressed frame 
descriptors
> >> 
> >>  		bmCapabilities		- still image support, fixed frame-rate
> >>  		
> >>  					support
> >> 
> >> +Date:           Mar 2018
> >> +KernelVersion:  4.16
> >> +
> >> +                bFrameIndex             - unique id for this
> >> framedescriptor; +                                        if using
> >> multiple
> >> framedescriptors for +                                        same
> >> format,
> >> user needs to set distinct +                                        value
> >> for each frame descriptor +
> >> +
> >> 
> >>  What:		/config/usb-gadget/gadget/functions/uvc.name/streaming/header
> >>  Date:		Dec 2014
> >>  KernelVersion:	4.0
> >> 
> >> diff --git a/drivers/usb/gadget/function/uvc_configfs.c
> >> b/drivers/usb/gadget/function/uvc_configfs.c index c9b8cc4a..5966d65
> >> 100644
> >> --- a/drivers/usb/gadget/function/uvc_configfs.c
> >> +++ b/drivers/usb/gadget/function/uvc_configfs.c
> >> @@ -992,6 +992,8 @@ UVC_ATTR(uvcg_frame_, cname, aname);
> >> 
> >>  UVCG_FRAME_ATTR(bm_capabilities, bmCapabilities, noop_conversion,
> >>  
> >>  		noop_conversion, 8);
> >> 
> >> +UVCG_FRAME_ATTR(b_frame_index, bFrameIndex, noop_conversion,
> >> +		noop_conversion, 8);
> >> 
> >>  UVCG_FRAME_ATTR(w_width, wWidth, le16_to_cpu, cpu_to_le16, 16);
> >>  UVCG_FRAME_ATTR(w_height, wHeight, le16_to_cpu, cpu_to_le16, 16);
> >>  UVCG_FRAME_ATTR(dw_min_bit_rate, dwMinBitRate, le32_to_cpu, cpu_to_le32,
> >> 
> >> 32);
> >> @@ -1137,6 +1139,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,
> >>  	&uvcg_frame_attr_dw_min_bit_rate,

[PATCHv2] usb/gadget/uvc-configs Fix host unable to negotiate framesizes other than first

[Joel Pepper]

Hi Lauren,


On 20.03.2018 10:02, Laurent Pinchart wrote:
> Hi Joel,
>
> (CC'ing Paul Elder who is working on the UVC gadget driver)
>
> Thank you for the patch.
>
> On Monday, 19 March 2018 21:36:41 EET Joel Pepper wrote:
>> Add bFrameIndex as a UVCG_FRAME_ATTR for each frame size.
>> 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.
>>
>> Note that this still requires the gadget to be configured with unique
>> bFrameIndexes for each frameSize of each format through configfs. An
>> alternative might be to automatically assign ascending indices when the
>> format is linked into the streaming header, but the user space gadget
>> would need a way to check or predict the indices so that it can properly
>> interpret to PROBE/COMMIT CONTROL requests
> It would indeed be nice if the indices could be automatically generated, to 
> avoid giving userspace another possibility to create invalid descriptors. As 
> you've correctly noted, that would require a way for the userspace helper 
> application to coordinate with the UVC gadget driver. Would it be difficult to 
> do so by defining the bFrameIndex attribute as read-only ? There's the 
> additional issue of finding a place to store the index counter locally to the 
> format, I'm not sure how that's done with the configfs API, but if it's not 
> too difficult I think it would be a good option.

Setting bFrameIndex to be read-only will suffice in allowing the
userspace gadget
to coordinate with the driver at the (slight) expense of the userspace
gadget not
being able to dictate an order which is semantically meaningful to it.

There are two approaches I see towards assigning indices automatically:
1) Assign a new unique index in "uvcg_frame_make" (i.e. config item
creation)
2) Assign indices when exact set of frame descriptors is finalized (i.e.
linking
   into the streaming header)

As for 1):
I think this is the approach you are referring to with regards to
storing the
index counter in the format. There already is a 'num_frames' unsigned
int in the
uvcg_format struct, which could be used as index, however this approach
does not
handle deleting frame descriptors well, because num_frames is
decremented then.
Unless frames are always deleted in reverse order of creation this will
produce
index collisions.
If we instead use a new monotonously incrementing counter, deleting
frames will
produce "holes" in the indices (I think UVC does allow for that but it
might trip
up host drivers not expecting that). The counter would also overflow after
creating 255 frames even if the final number is much lower.


As for 2):
I personally prefer this approach as it avoids the problems of 1). It
depends on
identifying a point in time when the set of frame descriptors cannot
change anymore
I think this is given when the format is linked into the streaming
header. It should
then suffice to iterate of the contained frames and assign ascending
indices.
A small problem however is that the attribute will have no meaningful
value to be
read before the format is linked. This fact will need to be communicated
in the
documentation and potentially through returning EBUSY on premature read
attempts.

I'll prepare a patch implementing 2), but in the meantime I'm grateful
for more input,
as there is not necessarily a single best approach to be taken here.

>> v2: Add the new attribute to both MJPEG and uncompressedframe descriptos
>> in Documentation/ABI, with note that it was added only in a later
>> kernel version
>>
>> Signed-off-by: Joel Pepper <joel.pepper@rwth-aachen.de>
>> ---
>>  Documentation/ABI/testing/configfs-usb-gadget-uvc | 17 +++++++++++++++++
>>  drivers/usb/gadget/function/uvc_configfs.c        |  3 +++
>>  2 files changed, 20 insertions(+)
>>
>> diff --git a/Documentation/ABI/testing/configfs-usb-gadget-uvc
>> b/Documentation/ABI/testing/configfs-usb-gadget-uvc index 1ba0d0f..d435cf7
>> 100644
>> --- a/Documentation/ABI/testing/configfs-usb-gadget-uvc
>> +++ b/Documentation/ABI/testing/configfs-usb-gadget-uvc
>> @@ -194,6 +194,14 @@ Description:	Specific MJPEG frame descriptors
>>  		bmCapabilities		- still image support, fixed frame-rate
>>  					support
>>
>> +Date:		Mar 2018
>> +KernelVersion:	4.16
>> +
>> +		bFrameIndex		- unique id for this framedescriptor;
>> +					if using multiple framedescriptors for
>> +					same format, user needs to set distinct
>> +					value for each frame descriptor
>> +
>>  What:		/config/usb-gadget/gadget/functions/uvc.name/streaming/
> uncompressed
>>  Date:		Dec 2014
>>  KernelVersion:	4.0
>> @@ -241,6 +249,15 @@ Description:	Specific uncompressed frame descriptors
>>  		bmCapabilities		- still image support, fixed frame-rate
>>  					support
>>
>> +Date:           Mar 2018
>> +KernelVersion:  4.16
>> +
>> +                bFrameIndex             - unique id for this
>> framedescriptor; +                                        if using multiple
>> framedescriptors for +                                        same format,
>> user needs to set distinct +                                        value
>> for each frame descriptor +
>> +
>>  What:		/config/usb-gadget/gadget/functions/uvc.name/streaming/header
>>  Date:		Dec 2014
>>  KernelVersion:	4.0
>> diff --git a/drivers/usb/gadget/function/uvc_configfs.c
>> b/drivers/usb/gadget/function/uvc_configfs.c index c9b8cc4a..5966d65 100644
>> --- a/drivers/usb/gadget/function/uvc_configfs.c
>> +++ b/drivers/usb/gadget/function/uvc_configfs.c
>> @@ -992,6 +992,8 @@ UVC_ATTR(uvcg_frame_, cname, aname);
>>
>>  UVCG_FRAME_ATTR(bm_capabilities, bmCapabilities, noop_conversion,
>>  		noop_conversion, 8);
>> +UVCG_FRAME_ATTR(b_frame_index, bFrameIndex, noop_conversion,
>> +		noop_conversion, 8);
>>  UVCG_FRAME_ATTR(w_width, wWidth, le16_to_cpu, cpu_to_le16, 16);
>>  UVCG_FRAME_ATTR(w_height, wHeight, le16_to_cpu, cpu_to_le16, 16);
>>  UVCG_FRAME_ATTR(dw_min_bit_rate, dwMinBitRate, le32_to_cpu, cpu_to_le32,
>> 32);
>> @@ -1137,6 +1139,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,
>>  	&uvcg_frame_attr_dw_min_bit_rate,
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCHv2] usb/gadget/uvc-configs Fix host unable to negotiate framesizes other than first

[Laurent Pinchart]

Hi Joel,

(CC'ing Paul Elder who is working on the UVC gadget driver)

Thank you for the patch.

On Monday, 19 March 2018 21:36:41 EET Joel Pepper wrote:
> Add bFrameIndex as a UVCG_FRAME_ATTR for each frame size.
> 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.
> 
> Note that this still requires the gadget to be configured with unique
> bFrameIndexes for each frameSize of each format through configfs. An
> alternative might be to automatically assign ascending indices when the
> format is linked into the streaming header, but the user space gadget
> would need a way to check or predict the indices so that it can properly
> interpret to PROBE/COMMIT CONTROL requests

It would indeed be nice if the indices could be automatically generated, to 
avoid giving userspace another possibility to create invalid descriptors. As 
you've correctly noted, that would require a way for the userspace helper 
application to coordinate with the UVC gadget driver. Would it be difficult to 
do so by defining the bFrameIndex attribute as read-only ? There's the 
additional issue of finding a place to store the index counter locally to the 
format, I'm not sure how that's done with the configfs API, but if it's not 
too difficult I think it would be a good option.

> v2: Add the new attribute to both MJPEG and uncompressedframe descriptos
> in Documentation/ABI, with note that it was added only in a later
> kernel version
> 
> Signed-off-by: Joel Pepper <joel.pepper@rwth-aachen.de>
> ---
>  Documentation/ABI/testing/configfs-usb-gadget-uvc | 17 +++++++++++++++++
>  drivers/usb/gadget/function/uvc_configfs.c        |  3 +++
>  2 files changed, 20 insertions(+)
> 
> diff --git a/Documentation/ABI/testing/configfs-usb-gadget-uvc
> b/Documentation/ABI/testing/configfs-usb-gadget-uvc index 1ba0d0f..d435cf7
> 100644
> --- a/Documentation/ABI/testing/configfs-usb-gadget-uvc
> +++ b/Documentation/ABI/testing/configfs-usb-gadget-uvc
> @@ -194,6 +194,14 @@ Description:	Specific MJPEG frame descriptors
>  		bmCapabilities		- still image support, fixed frame-rate
>  					support
> 
> +Date:		Mar 2018
> +KernelVersion:	4.16
> +
> +		bFrameIndex		- unique id for this framedescriptor;
> +					if using multiple framedescriptors for
> +					same format, user needs to set distinct
> +					value for each frame descriptor
> +
>  What:		/config/usb-gadget/gadget/functions/uvc.name/streaming/
uncompressed
>  Date:		Dec 2014
>  KernelVersion:	4.0
> @@ -241,6 +249,15 @@ Description:	Specific uncompressed frame descriptors
>  		bmCapabilities		- still image support, fixed frame-rate
>  					support
> 
> +Date:           Mar 2018
> +KernelVersion:  4.16
> +
> +                bFrameIndex             - unique id for this
> framedescriptor; +                                        if using multiple
> framedescriptors for +                                        same format,
> user needs to set distinct +                                        value
> for each frame descriptor +
> +
>  What:		/config/usb-gadget/gadget/functions/uvc.name/streaming/header
>  Date:		Dec 2014
>  KernelVersion:	4.0
> diff --git a/drivers/usb/gadget/function/uvc_configfs.c
> b/drivers/usb/gadget/function/uvc_configfs.c index c9b8cc4a..5966d65 100644
> --- a/drivers/usb/gadget/function/uvc_configfs.c
> +++ b/drivers/usb/gadget/function/uvc_configfs.c
> @@ -992,6 +992,8 @@ UVC_ATTR(uvcg_frame_, cname, aname);
> 
>  UVCG_FRAME_ATTR(bm_capabilities, bmCapabilities, noop_conversion,
>  		noop_conversion, 8);
> +UVCG_FRAME_ATTR(b_frame_index, bFrameIndex, noop_conversion,
> +		noop_conversion, 8);
>  UVCG_FRAME_ATTR(w_width, wWidth, le16_to_cpu, cpu_to_le16, 16);
>  UVCG_FRAME_ATTR(w_height, wHeight, le16_to_cpu, cpu_to_le16, 16);
>  UVCG_FRAME_ATTR(dw_min_bit_rate, dwMinBitRate, le32_to_cpu, cpu_to_le32,
> 32);
> @@ -1137,6 +1139,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,
>  	&uvcg_frame_attr_dw_min_bit_rate,