From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [RFC 00/21] Replace seqno values with request structures Date: Mon, 20 Oct 2014 17:49:36 +0200 Message-ID: <20141020154936.GS26941@phenom.ffwll.local> References: <1412604925-11290-1-git-send-email-John.C.Harrison@Intel.com> <20141006151739.GC31284@nuc-i3427.alporthouse.com> <20141020071918.GA13512@nuc-i3427.alporthouse.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail-wg0-f46.google.com (mail-wg0-f46.google.com [74.125.82.46]) by gabe.freedesktop.org (Postfix) with ESMTP id 230886E0C9 for ; Mon, 20 Oct 2014 08:49:28 -0700 (PDT) Received: by mail-wg0-f46.google.com with SMTP id l18so5788799wgh.5 for ; Mon, 20 Oct 2014 08:49:28 -0700 (PDT) Content-Disposition: inline In-Reply-To: <20141020071918.GA13512@nuc-i3427.alporthouse.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Chris Wilson , Daniel Vetter , John Harrison , intel-gfx List-Id: intel-gfx@lists.freedesktop.org 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 wrote: > > > On Mon, Oct 06, 2014 at 03:15:04PM +0100, John.C.Harrison@Intel.com wrote: > > >> From: John Harrison > > >> > > >> 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