Re: usb full speed isochronous bug ?

> > 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?


> >	(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
 
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 EHCI_TT_NEWSCHED clear.  Does this
issue crop up with that set?


> > 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.

- 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.



> >  #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.


> >                 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;
> > 
> > 
> > 
> > 
> >        

-------------------------------------------------------------------------
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