Re: [RFC 00/21] Replace seqno values with request structures

[Daniel Vetter <daniel@ffwll.ch>]

On Mon, Oct 20, 2014 at 08:19:18AM +0100, Chris Wilson wrote:
> On Sun, Oct 19, 2014 at 07:15:53PM +0200, Daniel Vetter wrote:
> > On Mon, Oct 6, 2014 at 5:17 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > > On Mon, Oct 06, 2014 at 03:15:04PM +0100, John.C.Harrison@Intel.com wrote:
> > >> From: John Harrison <John.C.Harrison@Intel.com>
> > >>
> > >> Work in progress for replacing seqno usage with requst structures.
> > >
> > > You fail to end up with my earlier code. Nak.
> > 
> > Well I've tried to split up your patch into small independent changes
> > and failed at that. And given how many changes there are in there I
> > simply can't merge your patch as-is. I know that it does seem to fix
> > some random hangs, but with massive behaviour changes like the
> > read-read stuff in there this could very well just be a random fluke.
> 
> Nope. Try reading it again. The most invasive change is defining the
> order of engine/context/ppgtt/ring creation (and doing the setup/enable
> split). Everything else is about making a request a transaction and
> using that to fix bugs in our state tacking.

I guess the setup changes only showed up in the revised and much bigger
version. At least I don't remember seeing that in the first iteration I've
tried to split up. Or maybe I've been blind. In any case I want to
untangle that web too, just with a much more gradual approach.
 
> The fundamental change, that I am going to be a stickler for, is that the
> request is the ring access. It defines the context, owner and interface
> to both the ring and the engine/scheduler. In doing so it drops the
> inconsistent and unmaintainable approach of introducing separate but almost
> duplicate code paths inside the core of GEM.

So for a clean slate design I agree with you. But given where we are I
still think that the duplication with the execlist code in intel_lrc.c is
a good approach. It will lead to a lot more ugliness, and there's a good
chance that we can't get rid of the final bits in years. But given that
execlist absolutely had to get in I really didn't see any other way to
merge it, without the inevitable reworks totally wreaking the other parts
of gem.

It's definitely not a great option, but there's also limits to how much
flak I can absorb without crumbling completely.

> > But if there's anything amiss in John's work for the plain
> > s/seqno/request/ change, or anything else that we absolutely need to
> > have then I very much want your opinion on this. But a flat outright
> > nak without further content isn't useful to move forward.
> 
> It doesn't reduce the technical debt of execlists, which was the
> motivation for the changes. It even brings a breath of sanity to
> contexts and ppgtt as well, a massive boon.

I agree on that. It should give us a start though to clean things up by
shifting a lot tracking execlist does over to requests. Or at least I hope
so. And there's still the issue that I really need to avoid another dept
pile-up excercise with the scheduler, so for just that part I think we
need this. Even when it doesn't address any of the issues with execlist.
But I do think this will allow us to at least move into the right
direction a bit, so please scream if I'm totally off the tracks.

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch