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

Daniel Vetter
2015-04-16T07:59:32Z

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.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation