Message ID | 4B124BDF.50309@freemail.hu (mailing list archive) |
---|---|
State | Superseded, archived |
Headers |
Return-path: <linux-media-owner@vger.kernel.org> Envelope-to: mchehab@infradead.org Delivery-date: Sun, 29 Nov 2009 10:24:39 +0000 Received: from bombadil.infradead.org [18.85.46.34] by pedra with IMAP (fetchmail-6.3.6) for <mchehab@localhost> (single-drop); Sun, 29 Nov 2009 10:03:45 -0200 (BRST) Received: from vger.kernel.org ([209.132.176.167]) by bombadil.infradead.org with esmtp (Exim 4.69 #1 (Red Hat Linux)) id 1NEgxL-0005ml-Gl; Sun, 29 Nov 2009 10:24:39 +0000 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753275AbZK2KYb (ORCPT <rfc822; kmpark@infradead.org> + 1 other); Sun, 29 Nov 2009 05:24:31 -0500 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1753080AbZK2KYb (ORCPT <rfc822;linux-media-outgoing>); Sun, 29 Nov 2009 05:24:31 -0500 Received: from mail01d.mail.t-online.hu ([84.2.42.6]:62973 "EHLO mail01d.mail.t-online.hu" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752227AbZK2KYa (ORCPT <rfc822;linux-media@vger.kernel.org>); Sun, 29 Nov 2009 05:24:30 -0500 Received: from [192.168.1.64] (dsl5402C4C9.pool.t-online.hu [84.2.196.201]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) by mail01d.mail.t-online.hu (Postfix) with ESMTPSA id 81A05758D00; Sun, 29 Nov 2009 11:23:35 +0100 (CET) Message-ID: <4B124BDF.50309@freemail.hu> Date: Sun, 29 Nov 2009 11:24:31 +0100 From: =?UTF-8?B?TsOpbWV0aCBNw6FydG9u?= <nm127@freemail.hu> User-Agent: Mozilla/5.0 (X11; U; Linux i686; hu-HU; rv:1.8.1.21) Gecko/20090402 SeaMonkey/1.1.16 MIME-Version: 1.0 To: Jean-Francois Moine <moinejf@free.fr>, V4L Mailing List <linux-media@vger.kernel.org> Subject: [PATCH] gspca main: reorganize loop Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-DCC-mail.t-online.hu-Metrics: mail01d.mail.t-online.hu 32721; Body=2 Fuz1=2 Fuz2=2 Sender: linux-media-owner@vger.kernel.org Precedence: bulk List-ID: <linux-media.vger.kernel.org> X-Mailing-List: linux-media@vger.kernel.org |
Commit Message
Németh Márton
Nov. 29, 2009, 10:24 a.m. UTC
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> --- -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Comments
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.
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 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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: