[Intel-gfx] [PATCH 08/15] drm/i915: Fix up checks for aliasing ppgtt

[Daniel Vetter]

On Fri, Aug 08, 2014 at 01:03:53PM +0000, Thierry, Michel wrote:
> 
> > -----Original Message-----
> > From: Intel-gfx [mailto:intel-gfx-bounces at lists.freedesktop.org] On Behalf
> > Of Daniel Vetter
> > Sent: Wednesday, August 06, 2014 2:05 PM
> > To: Intel Graphics Development
> > Cc: Daniel Vetter
> > Subject: [Intel-gfx] [PATCH 08/15] drm/i915: Fix up checks for aliasing
> ppgtt
> >
> > A subsequent patch will no longer initialize the aliasing ppgtt if we
> > have full ppgtt enabled, since we simply don't need that any more.
> >
> > Unfortunately a few places check for the aliasing ppgtt instead of
> > checking for ppgtt in general. Fix them up.
> >
> > One special case are the gtt offset and size macros, which have some
> > code to remap the aliasing ppgtt to the global gtt. The aliasing ppgtt
> > is _not_ a logical address space, so passing that in as the vm is
> > plain and simple a bug. So just WARN about it and carry on - we have a
> > gracefully fall-through anyway if we can't find the vma.
> >
> > Signed-off-by: Daniel Vetter <daniel.vetter at ffwll.ch>
> > ---
> >  drivers/gpu/drm/i915/i915_cmd_parser.c  | 4 +---
> >  drivers/gpu/drm/i915/i915_dma.c         | 2 +-
> >  drivers/gpu/drm/i915/i915_gem.c         | 8 ++------
> >  drivers/gpu/drm/i915/intel_ringbuffer.c | 4 +---
> >  4 files changed, 5 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c
> > b/drivers/gpu/drm/i915/i915_cmd_parser.c
> > index dea99d92fb4a..c45856bcc8b9 100644
> > --- a/drivers/gpu/drm/i915/i915_cmd_parser.c
> > +++ b/drivers/gpu/drm/i915/i915_cmd_parser.c
> > @@ -842,8 +842,6 @@ finish:
> >   */
> >  bool i915_needs_cmd_parser(struct intel_engine_cs *ring)
> >  {
> > -	struct drm_i915_private *dev_priv = ring->dev->dev_private;
> > -
> >  	if (!ring->needs_cmd_parser)
> >  		return false;
> >
> > @@ -852,7 +850,7 @@ bool i915_needs_cmd_parser(struct intel_engine_cs
> > *ring)
> >  	 * disabled. That will cause all of the parser's PPGTT checks to
> >  	 * fail. For now, disable parsing when PPGTT is off.
> >  	 */
> > -	if (!dev_priv->mm.aliasing_ppgtt)
> > +	if (USES_PPGTT(ring->dev))
> >  		return false;
> >
> >  	return (i915.enable_cmd_parser == 1);
> > diff --git a/drivers/gpu/drm/i915/i915_dma.c
> > b/drivers/gpu/drm/i915/i915_dma.c
> > index 2e7f03ad5ee2..e5ac1a6e9d26 100644
> > --- a/drivers/gpu/drm/i915/i915_dma.c
> > +++ b/drivers/gpu/drm/i915/i915_dma.c
> > @@ -999,7 +999,7 @@ static int i915_getparam(struct drm_device *dev, void
> > *data,
> >  		value = HAS_WT(dev);
> >  		break;
> >  	case I915_PARAM_HAS_ALIASING_PPGTT:
> > -		value = dev_priv->mm.aliasing_ppgtt ||
> > USES_FULL_PPGTT(dev);
> > +		value = USES_PPGTT(dev);
> >  		break;
> >  	case I915_PARAM_HAS_WAIT_TIMEOUT:
> >  		value = 1;
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c
> > b/drivers/gpu/drm/i915/i915_gem.c
> > index d8399ee622b9..a79deb189146 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -5114,9 +5114,7 @@ unsigned long i915_gem_obj_offset(struct
> > drm_i915_gem_object *o,
> >  	struct drm_i915_private *dev_priv = o->base.dev->dev_private;
> >  	struct i915_vma *vma;
> >
> > -	if (!dev_priv->mm.aliasing_ppgtt ||
> > -	    vm == &dev_priv->mm.aliasing_ppgtt->base)
> > -		vm = &dev_priv->gtt.base;
> > +	WARN_ON(vm == &dev_priv->mm.aliasing_ppgtt->base);
> >
> >  	list_for_each_entry(vma, &o->vma_list, vma_link) {
> >  		if (vma->vm == vm)
> > @@ -5157,9 +5155,7 @@ unsigned long i915_gem_obj_size(struct
> > drm_i915_gem_object *o,
> >  	struct drm_i915_private *dev_priv = o->base.dev->dev_private;
> >  	struct i915_vma *vma;
> >
> > -	if (!dev_priv->mm.aliasing_ppgtt ||
> > -	    vm == &dev_priv->mm.aliasing_ppgtt->base)
> > -		vm = &dev_priv->gtt.base;
> > +	WARN_ON(vm == &dev_priv->mm.aliasing_ppgtt->base);
> >
> >  	BUG_ON(list_empty(&o->vma_list));
> >
> > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c
> > b/drivers/gpu/drm/i915/intel_ringbuffer.c
> > index 05969f03c0c1..390e38325b37 100644
> > --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> > @@ -1977,9 +1977,7 @@ gen8_ring_dispatch_execbuffer(struct
> > intel_engine_cs *ring,
> >  			      u64 offset, u32 len,
> >  			      unsigned flags)
> >  {
> > -	struct drm_i915_private *dev_priv = ring->dev->dev_private;
> > -	bool ppgtt = dev_priv->mm.aliasing_ppgtt != NULL &&
> > -		!(flags & I915_DISPATCH_SECURE);
> > +	bool ppgtt = USES_PPGTT(ring->dev) && !(flags &
> > I915_DISPATCH_SECURE);
> >  	int ret;
> >
> >  	ret = intel_ring_begin(ring, 4);
> > --
> > 1.9.3
> >
> Sorry, I spotted another instance in gen8_ppgtt_info (debugfs). It is more 
> gen8_ppgtt_info's fault, so I'm sending a follow up patch for it.
> Unless you want to combine them...
> Reviewed-by: Michel Thierry <michel.thierry at intel.com>

Actually I spotted that one and decided that I can't break things worse
than it is - the aliasing ppgtt for full ppgtt is completely unused
(except for these checks), so dumping it doesn't add value.

To check: Does your r-b apply here still even without any fixup for gen8
ppgtt_info?
-Daniel

> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx



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