Re: [PATCH 11/17] drm/i915: Fix up the vma aliasing ppgtt binding

From: Daniel Vetter <daniel@ffwll.ch>
To: Chris Wilson <chris@chris-wilson.co.uk>,
	Daniel Vetter <daniel@ffwll.ch>,
	Daniel Vetter <daniel.vetter@ffwll.ch>,
	Intel Graphics Development <intel-gfx@lists.freedesktop.org>,
	Daniel Vetter <daniel.vetter@intel.com>
Subject: Re: [PATCH 11/17] drm/i915: Fix up the vma aliasing ppgtt binding
Date: Thu, 16 Apr 2015 10:57:34 +0200	[thread overview]
Message-ID: <20150416085733.GJ6092@phenom.ffwll.local> (raw)
In-Reply-To: <20150416080757.GE6193@nuc-i3427.alporthouse.com>

On Thu, Apr 16, 2015 at 09:07:57AM +0100, Chris Wilson wrote:
> On Thu, Apr 16, 2015 at 10:01:31AM +0200, Daniel Vetter wrote:
> > On Wed, Apr 15, 2015 at 11:47:02AM +0100, Chris Wilson wrote:
> > > On Tue, Apr 14, 2015 at 05:35:21PM +0200, Daniel Vetter wrote:
> > > > Currently we have the problem that the decision whether ptes need to
> > > > be (re)written is splattered all over the codebase. Move all that into
> > > > i915_vma_bind. This needs a few changes:
> > > > - Just reuse the PIN_* flags for i915_vma_bind and do the conversion
> > > >   to vma->bound in there to avoid duplicating the conversion code all
> > > >   over.
> > > > - We need to make binding for EXECBUF (i.e. pick aliasing ppgtt if
> > > >   around) explicit, add PIN_EXECBUF for that.
> > > 
> > > I am in favour of making the PIN_GLOBAL | PIN_LOCAL explicit, but
> > > PIN_EXECBUF doesn't seem descriptive of what happens, nor why it should
> > > be execbuf specific. Just use PIN_LOCAL with the execbuf oring in
> > > PIN_GLOBAL as it needs for workarounds + relocations.
> > 
> > Well I wasnt' too happy with LOCAL_BIND tbh since that sounds awfully
> > close to gen1 local memory ;-) My idea behind PIN_EXECBUF (and hiding the
> > ggtt vs. aliasing ppgtt binding in the low-level code) is that imo
> > conceptually global vs. aliasing ppgtt is just a pte bit with crazy
> > storage. We might as well have 2 bits to control access for priviledged
> > clients and for unpriviledge GT access, and in that case it would make
> > sense to hide that in the lower levels. ggtt+aliasing ppgtt isnt' anything
> > else, except for a rather peculariar storage format of these two bits.
> > 
> > Essentially we have:
> > - PIN_GLOBAL: Please set up pte so that system agent, display and
> >   priviledged GT can access it.
> > - PIN_EXECBUF: Please set up pte so that unpriveledged GT access is
> >   possible.
> > 
> > Maybe PIN_UNPRIV_GT or something else would be better? Pushing the
> > decision of how that access control is down up into the execbuf code is
> > imo a layering violation and exactly the kind of trouble that imo has lead
> > to rebinding or too-much-binding bugs we've had. With my approach it's all
> > together in the low-level gtt binding code.
> 
> Yeah, I'm just arguing that EXECBUF doesn't tell me anything at all
> about what is happening. It is just not descriptive enough and is liable
> to change meanings and end up in confusion.
> 
> How about PIN_GLOBAL (aka PIN_SYSTEM) and PIN_USER?

PIN_USER sounds really good indeed. Will change.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx