[PATCH] media: uvcvideo: Pick first best alternate setting insteed of last

[PATCH] media: uvcvideo: Pick first best alternate setting insteed of last

[Laurent Pinchart]

When selecting an alternate setting, the driver loops over all available
alternate settings to find the one with the lowest bandwidth high enough
for the selected format and resolution. While all alternate settings
should have different packet sizes, some buggy devices report multiple
alternate settings with the same size. The driver happens to pick the
last one in this case.

In theory this should work fine, but in real life we have device bugs.
The Ali Corp. Newmine Camera (0402:8841) exposes four alternate
settings with the same packet size. The first three seem to work fine,
while selecting the last one results in lots of transmission errors.

Switch to using the first best alternate setting when multiple are
present. This should be safe (last famous words), as sniffing USB
traffic with the faulty device shows that Windows 10 picks the first
alternate setting, and devices are typically tested on Windows.

Reported-by: Karel Janda <karel1@tutanota.com>
Closes: https://lore.kernel.org/linux-media/Nh6D0WI--3-9@tutanota.com/
Suggested-by: Karel Janda <karel1@tutanota.com>
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
Karel, could you please test this patch ? Please also let me know if you
are fine having your name recorded in the kernel git history with the
Reported-by and Suggested-by tags above. If not, I will drop them.
---
 drivers/media/usb/uvc/uvc_video.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
index 28dde08ec6c5..7cbf4692bd87 100644
--- a/drivers/media/usb/uvc/uvc_video.c
+++ b/drivers/media/usb/uvc/uvc_video.c
@@ -1954,7 +1954,7 @@ static int uvc_video_start_transfer(struct uvc_streaming *stream,
 
 			/* Check if the bandwidth is high enough. */
 			psize = uvc_endpoint_max_bpi(stream->dev->udev, ep);
-			if (psize >= bandwidth && psize <= best_psize) {
+			if (psize >= bandwidth && psize < best_psize) {
 				altsetting = alts->desc.bAlternateSetting;
 				best_psize = psize;
 				best_ep = ep;
-- 
Regards,

Laurent Pinchart

Re: [PATCH] media: uvcvideo: Pick first best alternate setting insteed of last

[Karel Janda]

Hi Laurent,

 you pleased me very much, also for your quick reply.

1)
  The patch is exactly the same as final state of my debugging.
I just succesfully probe it against uvc_video.c <https://elixir.bootlin.com/linux/v6.6-rc6/source/drivers/media/usb/uvc/uvc_video.c> source on my PC running
 Linux HP-Pro-A-MT 6.5.0-9-generic #9-Ubuntu SMP PREEMPT_DYNAMIC 
Sat Oct  7 01:35:40 UTC 2023 x86_64 x86_64 x86_64 GNU/Linux
 with Newmine camera. 
/The patch also doesn't hurt my other UVC 1.0 camera, Genius Qcam 6000./
All my 6.5.0 uvc*.c  files in uvc directory are clean of any other changes I used for tests before,
only the path of one single line in one file is there now.

  Now I also carefully compare 6.5.0 uvc_video.c <https://elixir.bootlin.com/linux/v6.6-rc6/source/drivers/media/usb/uvc/uvc_video.c> original  (before the patch) with currect Linux sources at
https://elixir.bootlin.com/linux/v6.6-rc6/source/drivers/media/usb/uvc/uvc_video.c
and also at
https://github.com/torvalds/linux/blob/master/drivers/media/usb/uvc/uvc_video.c
   These are different only in few blank characters :)).

  I am not able to run last git 6.6 kernel just now, but I hope that my verification by 6.5.0 will be enough.
If is necessery - let me now, I will try compile last kernel on different, dedicated computer.
(I never work with such a big git project. I sometimes used other's ppa results.)

2)
  You can mention my name in linux git history, I will be glad.

best regards

Karel Janda


20. 10. 2023 0:24 od laurent.pinchart@ideasonboard.com:

> ---
> Karel, could you please test this patch ? Please also let me know if you
> are fine having your name recorded in the kernel git history with the
> Reported-by and Suggested-by tags above. If not, I will drop them.
> ---
>  drivers/media/usb/uvc/uvc_video.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
> index 28dde08ec6c5..7cbf4692bd87 100644
> --- a/drivers/media/usb/uvc/uvc_video.c
> +++ b/drivers/media/usb/uvc/uvc_video.c
> @@ -1954,7 +1954,7 @@ static int uvc_video_start_transfer(struct uvc_streaming *stream,
>  
>  /* Check if the bandwidth is high enough. */
>  psize = uvc_endpoint_max_bpi(stream->dev->udev, ep);
> -			if (psize >= bandwidth && psize <= best_psize) {
> +			if (psize >= bandwidth && psize < best_psize) {
>  altsetting = alts->desc.bAlternateSetting;
>  best_psize = psize;
>  best_ep = ep;
> -- 
>

Re: [PATCH] media: uvcvideo: Pick first best alternate setting insteed of last

[Karel Janda]

Hi Laurent,

   I think again and I have more good news:
I installed patch for uvc_video.c  on running kernel from
 https://kernel.ubuntu.com/mainline/v6.6-rc5/

Patch works as expected, with working video on Newmine camera  at my
Linux karel1-HP-630 6.6.0-060600rc5-generic #202310081731 SMP PREEMPT_DYNAMIC 
Sun Oct  8 21:38:25 UTC 2023 x86_64 x86_64 x86_64 GNU/Linux

insmod uvcvideo trace=0x400 #logs into kernel.log:
[ 1223.890865] usb 2-1.1: Found UVC 1.00 device Newmine Camera (0402:8841)
[ 1223.917166] usbcore: registered new interface driver uvcvideo
[ 1339.107555] usb 2-1.1: Device requested 320 B/frame bandwidth
[ 1339.107566] usb 2-1.1: Selecting alternate setting 1 (1024 B/frame bandwidth)
[ 1339.115788] usb 2-1.1: Allocated 5 URB buffers of 32x1024 bytes each
So we all can be sure, that patch is OK also for 6.6 Linux kernel.

sincerely

Karel Janda


20. 10. 2023 16:09 od karel1@tutanota.com:

> Hi Laurent,
>
>  you pleased me very much, also for your quick reply.
>
> 1)
>   The patch is exactly the same as final state of my debugging.
> I just succesfully probe it against > uvc_video.c <https://elixir.bootlin.com/linux/v6.6-rc6/source/drivers/media/usb/uvc/uvc_video.c>>  source on my PC running
> Linux HP-Pro-A-MT 6.5.0-9-generic #9-Ubuntu SMP PREEMPT_DYNAMIC
> Sat Oct  7 01:35:40 UTC 2023 x86_64 x86_64 x86_64 GNU/Linux
> with Newmine camera.
> /The patch also doesn't hurt my other UVC 1.0 camera, Genius Qcam 6000./
> All my 6.5.0 uvc*.c  files in uvc directory are clean of any other changes I used for tests before,
> only the path of one single line in one file is there now.
>
>   Now I also carefully compare 6.5.0 > uvc_video.c <https://elixir.bootlin.com/linux/v6.6-rc6/source/drivers/media/usb/uvc/uvc_video.c>>  original  (before the patch) with currect Linux sources at
> https://elixir.bootlin.com/linux/v6.6-rc6/source/drivers/media/usb/uvc/uvc_video.c
> and also at
> https://github.com/torvalds/linux/blob/master/drivers/media/usb/uvc/uvc_video.c
>    These are different only in few blank characters :)).
>
>   I am not able to run last git 6.6 kernel just now, but I hope that my verification by 6.5.0 will be enough.
> If is necessery - let me now, I will try compile last kernel on different, dedicated computer.
> (I never work with such a big git project. I sometimes used other's ppa results.)
>
> 2)
>   You can mention my name in linux git history, I will be glad.
>
> best regards
>
> Karel Janda
>
>
> 20. 10. 2023 0:24 od laurent.pinchart@ideasonboard.com:
>
>> ---
>> Karel, could you please test this patch ? Please also let me know if you
>> are fine having your name recorded in the kernel git history with the
>> Reported-by and Suggested-by tags above. If not, I will drop them.
>> ---
>> drivers/media/usb/uvc/uvc_video.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
>> index 28dde08ec6c5..7cbf4692bd87 100644
>> --- a/drivers/media/usb/uvc/uvc_video.c
>> +++ b/drivers/media/usb/uvc/uvc_video.c
>> @@ -1954,7 +1954,7 @@ static int uvc_video_start_transfer(struct uvc_streaming *stream,
>>
>> /* Check if the bandwidth is high enough. */
>> psize = uvc_endpoint_max_bpi(stream->dev->udev, ep);
>> -			if (psize >= bandwidth && psize <= best_psize) {
>> +			if (psize >= bandwidth && psize < best_psize) {
>> altsetting = alts->desc.bAlternateSetting;
>> best_psize = psize;
>> best_ep = ep;
>> --
>>
>
>