Re: usb full speed isochronous bug ?

David,

Comments inline.

Philip

--- David Brownell <[email protected]> wrote:

> > > We have a full speed usb camera attached to an embedded linux
> solution.
> > > the usb core can support full speed devices without a HS hub even
> > > though it is echi.
> 
> One of the ARC-derived cores?

PMR:  Yes
> 
> 
> > >	(note: using a full speed hub did not solve the
> > > problem) We found that when we plugged the camera in we could get
> one
> > > stream working (video or audio) but not both.
> > > 
> > > We saw similar problem with usb sound cards -- we could record or
> play
> > > back but NOT both.
> 
> I'm not quite clear what's going on here.  Does this behavior
> reproduce *with* a high speed hub?   Or is it only when going
> through the "fullspeed iso on root hub port" logic?
> 
> Point being scheduling the ARC cores really seems to need three
> modes:
> 
>  - high speed
>  - full speed through TT
>  - full speed through root hub port

PMR:  root hub port

>  
> I'm not sure that last mode didn't really exist.  And I suspect
> that's the root cause of the problems here.
> 
> I'm also assuming this is with clear.  Does this
> issue crop up with that set?

PMR: setting EHCI_TT_NEWSCHED does not fix the problem also causes
problem in detecting low speed devices (although not an issue for us). 
Using   EHCI_TT_NEWSCHED does make the performance better in the sense
that the video stream is not as awful as NOT setting it but it does not
make things work like the patch.  Without the patch cannot do QVGA,
with the patch can do VGA.
> 
> 
> > > We talked to our IP provider and they gave us a patch (below)
> that
> > > works.  We are NOT usb experts so it is possible there are issues
> with
> > > the patch but we thought it a good idea to pass it on so.
> > > 
> > > We needed to make one addition to the patch.  the goto next is
> needed !
> >
> > David, what do you think about the patch below?
> 
> Don't much like it, see below.  Also, it seems to be against some
> hacked version of the driver; so maybe those hacks are part of
> the problem being observed.
\

PMR:  Confused -- was NOT against hack version of driver.  maybe the
patch for goto next is missing next:  like you suggested.  My mistake. 
But original patch was against 2.6.16.16 and comparing the code to
2.6.22.x there were no obvious changes (EXCEPT our addition of the
EHCI_TT_NEWSCHED fix from 2.6.17 (which made no difference)
> 
> - Dave
> 
> 
> > thanks,
> >
> > greg k-h
> >
> > > --- ehci-sched.c        2007-09-26 11:03:01.000000000 -0700
> > > +++ /Users/philipr/Desktop/ehci-sched.2.6.22.9.c       
> 2007-10-08
> > > 22:27:06.000000000 -0700
> > > @@ -1265,7 +1265,7 @@
> > >                  */
> > >                 if (!tt_no_collision (ehci, period_uframes << 3,
> > >                                 stream->udev, frame, mask))
> > > -                       return 0;
> > > +                       goto next;
> 
> This can't be right.  For one thing, I don't see such a label, either
> in the driver or added by this patch!  Second, if there's a TT
> collision
> there's no way this slot could be usable.
> 

PMR:  the next label is at the bottom of the routine and moves the
pointer forward one descriptor.  Sorry it was missing:  This patch does
NOT effect the video behavior -- it makes the audio work better with
the video.
> 
> 
> > >  #endif
> > >  
> > >                 /* check starts (OUT uses more than one) */
> > > @@ -2078,6 +2078,7 @@
> > >  {
> > >         unsigned        frame, clock, now_uframe, mod;
> > >         unsigned        modified;
> > > +               u8      uncompleted_td = 0;
> 
> Indent lossage.  (The patch as it arrived here was also line wrapped,
> with tabs turned into spaces.)
> 
> Plus, shouldn't this be set at the beginning of each frame scan?
> 
> 
> > >  
> > >         mod = ehci->periodic_size << 3;
> > >  
> > > @@ -2157,8 +2158,10 @@
> > >                                         break;
> > >                                 }
> > >                                 if (uf != 8)
> > > +                               {
> > > +                                       uncompleted_td = 1;
> 
> Bad CodingStyle.  Plus it's flagging uncomplated *I*TDs, not TDs...
> 
> 
> > >                                         break;
> > > -
> > > +                               }
> > >                                 /* this one's ready ... HC won't
> cache the
> > >                                  * pointer for much longer, if at
> all.
> > >                                  */
> > > @@ -2176,6 +2179,7 @@
> > >                                         hw_p = &q.sitd->hw_next;
> > >                                         type = Q_NEXT_TYPE
> (q.sitd->hw_next);
> > >                                         q = *q_p;
> > > +                                       uncompleted_td = 1;
> > >                                         break;
> > >                                 }
> > >                                 *q_p = q.sitd->sitd_next;
> > > @@ -2207,12 +2211,15 @@
> > >  
> > >                 // FIXME:  likewise assumes HC doesn't halt
> mid-scan
> > >  
> > > +               if((!uncompleted_td) && (HC_IS_RUNNING
> (ehci_to_hcd(ehci)->state)))
> > > +                       ehci->next_uframe = now_uframe;
> > > +
> 
> Again, indent lossage.  (Which I fixed above...)  And other style
> issues.
> 
> The concept behind this test is a bit unclear to me.  Why should only
> ISO transfers getting this treatment?  Previously, "next_uframe"
> would
> be updated whenever the periodic schedule scan caught up to the
> current
> microframe counter from the controller, and hence the scan might
> stop.
> 
> These changes break the simple invariant that this loop scans from
> the
> end of the previous scan up to "now".  Which means that aspect of its
> operation no longer looks "obviously correct".  (IMO, every part of
> this hairy code that can be simple should be simple!)  But there's no
> comment about what this change is trying to do instead ...
> 
> What would be useful here is information about exactly what
> (perceived?)
> issue is being addressed by this change in how the periodic schedule
> gets scanned.  And why that issue should be addressed _here_ rather
> than
> elsewhere in the driver.

PMR:  Patch came from IP provider -- if you tell me how to get the
information you need for a better patch -- will be very happy to
provide data.  Concerning the comment about the issue being addressed. 
Video camera cannot do QVGA nor VGA at 20-30 frames / second WITHOUT
the patch


> 
> 
> > >                 if (now_uframe == clock) {
> > >                         unsigned        now;
> > >  
> > >                         if (!HC_IS_RUNNING
> (ehci_to_hcd(ehci)->state))
> > >                                 break;
> > > -                       ehci->next_uframe = now_uframe;
> > > +
> > >                         now = ehci_readl(ehci,
> &ehci->regs->frame_index) % mod;
> > >                         if (now_uframe == now)
> > >                                 break;
> > > 
> > > 
> > > 
> > > 
> > >        
> 


__________________________________________________
Do You Yahoo!?
Tired of spam?  Yahoo! Mail has the best spam protection around 
http://mail.yahoo.com 

-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/
_______________________________________________
[email protected]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel