[PATCH] gspca main: reorganize loop

[PATCH] gspca main: reorganize loop

[Németh Márton]

From: Márton Németh <nm127@freemail.hu>

Eliminate redundant code by reorganizing the loop.

Signed-off-by: Márton Németh <nm127@freemail.hu>
---
diff -r 064a82aa2daa linux/drivers/media/video/gspca/gspca.c
--- a/linux/drivers/media/video/gspca/gspca.c	Thu Nov 26 19:36:40 2009 +0100
+++ b/linux/drivers/media/video/gspca/gspca.c	Sun Nov 29 11:09:33 2009 +0100
@@ -623,12 +623,12 @@
 		if (ret < 0)
 			goto out;
 	}
-	ep = get_ep(gspca_dev);
-	if (ep == NULL) {
-		ret = -EIO;
-		goto out;
-	}
 	for (;;) {
+		ep = get_ep(gspca_dev);
+		if (ep == NULL) {
+			ret = -EIO;
+			goto out;
+		}
 		PDEBUG(D_STREAM, "init transfer alt %d", gspca_dev->alt);
 		ret = create_urbs(gspca_dev, ep);
 		if (ret < 0)
@@ -677,12 +677,6 @@
 			ret = gspca_dev->sd_desc->isoc_nego(gspca_dev);
 			if (ret < 0)
 				goto out;
-		} else {
-			ep = get_ep(gspca_dev);
-			if (ep == NULL) {
-				ret = -EIO;
-				goto out;
-			}
 		}
 	}
 out:

Re: [PATCH] gspca main: reorganize loop

[Jean-Francois Moine]

On Sun, 29 Nov 2009 11:24:31 +0100
Németh Márton <nm127@freemail.hu> wrote:

> From: Márton Németh <nm127@freemail.hu>
> 
> Eliminate redundant code by reorganizing the loop.
> 
> Signed-off-by: Márton Németh <nm127@freemail.hu>
> ---
> diff -r 064a82aa2daa linux/drivers/media/video/gspca/gspca.c
> --- a/linux/drivers/media/video/gspca/gspca.c	Thu Nov 26
> 19:36:40 2009 +0100 +++
> b/linux/drivers/media/video/gspca/gspca.c	Sun Nov 29 11:09:33
> 2009 +0100 @@ -623,12 +623,12 @@ if (ret < 0)
>  			goto out;
>  	}
> -	ep = get_ep(gspca_dev);
> -	if (ep == NULL) {
> -		ret = -EIO;
> -		goto out;
> -	}
>  	for (;;) {
> +		ep = get_ep(gspca_dev);
> +		if (ep == NULL) {
> +			ret = -EIO;
> +			goto out;
> +		}
>  		PDEBUG(D_STREAM, "init transfer alt %d",
> gspca_dev->alt); ret = create_urbs(gspca_dev, ep);
>  		if (ret < 0)
> @@ -677,12 +677,6 @@
>  			ret =
> gspca_dev->sd_desc->isoc_nego(gspca_dev); if (ret < 0)
>  				goto out;
> -		} else {
> -			ep = get_ep(gspca_dev);
> -			if (ep == NULL) {
> -				ret = -EIO;
> -				goto out;
> -			}
>  		}
>  	}
>  out:

Hello Márton,

As you may see, in the loop, get_ep() is called only when isoc_nego()
is not called. So, your patch does not work.

Regards.

-- 
Ken ar c'hentañ	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/

Re: [PATCH] gspca main: reorganize loop

[Németh Márton]

Jean-Francois Moine wrote:
> On Sun, 29 Nov 2009 11:24:31 +0100
> Németh Márton <nm127@freemail.hu> wrote:
> 
>> From: Márton Németh <nm127@freemail.hu>
>>
>> Eliminate redundant code by reorganizing the loop.
>>
>> Signed-off-by: Márton Németh <nm127@freemail.hu>
>> ---
>> diff -r 064a82aa2daa linux/drivers/media/video/gspca/gspca.c
>> --- a/linux/drivers/media/video/gspca/gspca.c	Thu Nov 26
>> 19:36:40 2009 +0100 +++
>> b/linux/drivers/media/video/gspca/gspca.c	Sun Nov 29 11:09:33
>> 2009 +0100 @@ -623,12 +623,12 @@ if (ret < 0)
>>  			goto out;
>>  	}
>> -	ep = get_ep(gspca_dev);
>> -	if (ep == NULL) {
>> -		ret = -EIO;
>> -		goto out;
>> -	}
>>  	for (;;) {
>> +		ep = get_ep(gspca_dev);
>> +		if (ep == NULL) {
>> +			ret = -EIO;
>> +			goto out;
>> +		}
>>  		PDEBUG(D_STREAM, "init transfer alt %d",
>> gspca_dev->alt); ret = create_urbs(gspca_dev, ep);
>>  		if (ret < 0)
>> @@ -677,12 +677,6 @@
>>  			ret =
>> gspca_dev->sd_desc->isoc_nego(gspca_dev); if (ret < 0)
>>  				goto out;
>> -		} else {
>> -			ep = get_ep(gspca_dev);
>> -			if (ep == NULL) {
>> -				ret = -EIO;
>> -				goto out;
>> -			}
>>  		}
>>  	}
>>  out:
> 
> Hello Márton,
> 
> As you may see, in the loop, get_ep() is called only when isoc_nego()
> is not called. So, your patch does not work.

You are right, I overseen that.

Is there any subdriver where the isoc_nego() is implemented? I couldn't find
one. What would be the task of the isoc_nego() function? Should it set
the interface by calling usb_set_interface() as the get_ep() does? Should
it create URBs for the endpoint?

Although I found the patch where the isoc_nego() was introduced
( http://linuxtv.org/hg/v4l-dvb/rev/5a5b23605bdb56aec86c9a89de8ca8b8ae9cb925 )
it is not clear how the "ep" pointer is updated when not the isoc_nego() is
called instead of get_ep() in the current implementation.

Regards,

	Márton Németh

Re: [PATCH] gspca main: reorganize loop

[Jean-Francois Moine]

On Sun, 29 Nov 2009 12:19:46 +0100
Németh Márton <nm127@freemail.hu> wrote:

> Is there any subdriver where the isoc_nego() is implemented? I
> couldn't find one. What would be the task of the isoc_nego()
> function? Should it set the interface by calling usb_set_interface()
> as the get_ep() does? Should it create URBs for the endpoint?
> 
> Although I found the patch where the isoc_nego() was introduced
> ( http://linuxtv.org/hg/v4l-dvb/rev/5a5b23605bdb56aec86c9a89de8ca8b8ae9cb925 )
> it is not clear how the "ep" pointer is updated when not the
> isoc_nego() is called instead of get_ep() in the current
> implementation.

Hello Hans,

This function (isoc_nego) was added to fix the Mauro's problem with the
st6422. Was this problem solved in some other way, or is the fix still
waiting to be pulled?

Best regards.

-- 
Ken ar c'hentañ	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/

Re: [PATCH] gspca main: reorganize loop

[Hans de Goede]

Hi,

On 11/29/2009 01:15 PM, Jean-Francois Moine wrote:
> On Sun, 29 Nov 2009 12:19:46 +0100
> Németh Márton<nm127@freemail.hu>  wrote:
>
>> Is there any subdriver where the isoc_nego() is implemented? I
>> couldn't find one. What would be the task of the isoc_nego()
>> function? Should it set the interface by calling usb_set_interface()
>> as the get_ep() does? Should it create URBs for the endpoint?
>>
>> Although I found the patch where the isoc_nego() was introduced
>> ( http://linuxtv.org/hg/v4l-dvb/rev/5a5b23605bdb56aec86c9a89de8ca8b8ae9cb925 )
>> it is not clear how the "ep" pointer is updated when not the
>> isoc_nego() is called instead of get_ep() in the current
>> implementation.
>
> Hello Hans,
>
> This function (isoc_nego) was added to fix the Mauro's problem with the
> st6422. Was this problem solved in some other way, or is the fix still
> waiting to be pulled?
>

The fix is still waiting to be pulled, or actually to be made working :|

First a quick summary of the need for the isoc_nego() function
(and to only call get_ep() once). Some cams, at least those based on stv06xx
bridges, have only 1 alt setting, but they have a register which allows
one to tell it to send isoc frames which are never bigger then the value
set in the register. I've tested this and it works as advertised,
when you create isoc urbs with a size < wMaxPacketSize, then normally
you will get -EMSGSIZE errors (iirc) as the camera sends isoc frames,
larger then the buffers in the isoc urbs created, but if you then write the
size you created the isoc urbs with to the register in question, things
will work. So this works as expected.

The problem is that the usb "scheduler" which checks the bandwidth constrains
will always use wMaxPacketSize to check if there is enough bandwidth instead
of the actual packet size of the isoc packets. So although I have a patch
implementing the isoc_neg call for stv06xx cams, it does not actually help
as although it is scaling back the bandwidth needed, the usb core does not
see this and keeps returning -ENOSPC.

After finding out about this I ran out of time. This reminds me that I need to
send a message about this to the usb mailing list. I will do so now, and
then we will see what will come from this.

Regards,

Hans