Hi Paul,
> > Furthermore, I wonder about how this scheduler works exactly. What I see
> > is:
> > - the number usecs needed for a single transfer in a periodic qh is
> > calculated
> > - When the qh is scheduled, the first of the 8 microframes with enough
> > usecs available is picked for this qh (disregarding FS transfers that
> > don't fit in 1 microframe for now)
> >
> > However, this seems correct only for endpoints with an interval of 8
> > microframes? If an isoc endpoint has an interval of 1, it should be
> > scheduled in _every_ microframe, right?
> >
> > The old code was pessimistic (the dwc2_check_periodic_bandwidth
> > essentially assumes an interval of 1 for everything) but the new code is
> > actually too optimistic (as it essentially assumes an interval of 8 for
> > everything).
> >
> > This leads me to believe that this code works because it makes the
> > scheduler way to optimistic and because it removes the "one channel per
> > periodic endpoint" policy (which is way too optimistic), but it would
> > break when adding too much (periodic) devices (in nasty ways, no nice
> > "not enough bandwidth" messages).
It occurred to me that there is in fact more merit to this scheduler
than I originally thought. Before, I thought its only actual purpose was
to detect if there was room or not in the schedule for a new periodic
transfer. (and if I'm not mistaken, it does that badly when intervals of
< 8 uframes are involved).
However, what this scheduler also does is (obviously...) scheduling :-)
It makes sure that the various periodic transfers actually get sent to
the hardware spaced over different uframes instead of all at the first
or last uframe in a frame as before. This allows the driver to actually
accept more than 1 uframe worth of periodic transfers.
> > Overall, I don't think this patch is not even nearly ready to be
> > merged...
>
> Well, I disagree :)
I would argue that at least any questions about what this patch does and
why need to be resolved. Even if you did not write the code, you're the
one submitting it. Also, regardless of who wrote the code, the quality
of it should be high enough to accept in mainline. And frankly, I do not
think "But it works" is sufficient indication of quality by itself.
What I'm afraid of, is that if we introduce some code now that isn't
really needed or solves an unknown problem in the wrong way and we'll
end up carrying that code around without properly understanding what
it's for forever.
Of course this is just how I _think_ things should work, I'm not really
in a position to decide anything. So I'll just stick to commenting on
the code now, as far as I understand it, and try to understand better so
I can offer suggestions for improvements.
> > > - chan = list_first_entry(&hsotg->free_hc_list, struct dwc2_host_chan,
> > > - hc_list_entry);
> > > + if (!chan) {
> > > + dev_dbg(hsotg->dev, "No free channel to assign\n");
> > > + return -ENOMEM;
> > > + }
> > As above, I don't think this can ever happen.
>
> Sure it can. If hsotg->free_hc_list is empty on entry to this
> function, for example.
Ah, you're right.
> > Only the addition of nak_frame and frame_usecs seem relevant to this
> > patch, the rest seems noise. And those variables could use some actual
> > documentation. I propose:
> >
> > * @nak_frame: (Micro)frame number in which this qh was
> > re-queued because a NAK was received. Is 0xffff
> > when no NAK was received.
> > * @frame_usecs: Internal variable used by the microframe scheduler
>
> No, its not noise. I took the opportunity while adding the new
> members to reorder the existing ones from smallest to largest, to
> reduce the amount of wasted space due to structure padding. I think
> that's a legitimate thing to do in this patch.
I'm not saying it's a useless change, I meant it is noise for _this_
patch and should go into a separate one.
Gr.
Matthijs
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
