Re: [PATCH] drm/i915: Initialize the aliasing ppgtt as part of global gtt

[Daniel Vetter <daniel@ffwll.ch>]

On Wed, Aug 06, 2014 at 08:44:34AM +0000, Thierry, Michel wrote:
> 
> 
> > -----Original Message-----
> > From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel
> > Vetter
> > Sent: Wednesday, August 06, 2014 9:31 AM
> > To: Thierry, Michel
> > Cc: Daniel Vetter; Intel Graphics Development
> > Subject: Re: [PATCH] drm/i915: Initialize the aliasing ppgtt as part of
> global gtt
> > 
> > On Wed, Aug 06, 2014 at 08:18:52AM +0000, Thierry, Michel wrote:
> > >
> > >
> > > > -----Original Message-----
> > > > From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch]
> > > > Sent: Monday, August 04, 2014 3:19 PM
> > > > To: Intel Graphics Development
> > > > Cc: Daniel Vetter; Thierry, Michel; Ville Syrjälä
> > > > Subject: [PATCH] drm/i915: Initialize the aliasing ppgtt as part of
> global gtt
> > > >
> > > > Stuffing this into the context setup code doesn't make a lot of sense.
> > > > Also reusing the real ppgtt setup code makes even less sense since the
> > > > aliasing ppgtt isn't a real address space. Leaving all that stuff
> > > > unitialized will make sure that we catch any abusers promptly.
> > > >
> > > > This is also a prep work to clean up the context->ppgtt link.
> > > >
> > > > v2: Fix up the logic fail, I've fumbled it so badly to completely
> > > > disable ppgtt on gen6. Spotted by Ville and Michel. Also move around
> > > > the pde write into the gen6 init function, since otherwise it won't
> > > > work at all.
> > > >
> > > > Cc: "Thierry, Michel" <michel.thierry@intel.com>
> > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > > ---
> > > >  drivers/gpu/drm/i915/i915_gem_context.c | 13 +---------
> > > >  drivers/gpu/drm/i915/i915_gem_gtt.c     | 42
> > +++++++++++++++++++++++-
> > > > ---------
> > > >  2 files changed, 31 insertions(+), 24 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c
> > > > b/drivers/gpu/drm/i915/i915_gem_context.c
> > > > index 3b8367aa8404..7a455fcee3a7 100644
> > > > --- a/drivers/gpu/drm/i915/i915_gem_context.c
> > > > +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> > > > @@ -276,17 +276,6 @@ i915_gem_create_context(struct drm_device
> > *dev,
> > > >  			goto err_unpin;
> > > >  		} else
> > > >  			ctx->vm = &ppgtt->base;
> > > > -
> > > > -		/* This case is reserved for the global default context and
> > > > -		 * should only happen once. */
> > > > -		if (is_global_default_ctx) {
> > > > -			if (WARN_ON(dev_priv->mm.aliasing_ppgtt)) {
> > > > -				ret = -EEXIST;
> > > > -				goto err_unpin;
> > > > -			}
> > > > -
> > > > -			dev_priv->mm.aliasing_ppgtt = ppgtt;
> > > > -		}
> > >
> > > I expect some problems with full ppgtt & this (in some places, the
> > > driver is still making some decisions based on
> > > dev_priv->mm.aliasing_ppgtt, which now will be null).  Should I address
> > > these problems in a subsequent patch?
> > 
> > Oh, good catch. I've done a bit a review and found two cases:
> > - Driver unload code. Already fairly broken, made much worse by my series
> >   here. I've fixed that up yesterday and will resend the series with those
> >   additional patches and revised patches.
> > - cmd parser. I guess that should be a fixup patch on top. I'll also do
> >   that.
> > 
> > Have you spotted any other places?
> The other place is gen8_ring_dispatch_execbuffer, it won't set the ppgtt
> flag in MI_BATCH_BUFFER_START.

Indeed. I've also found a few other really fishy places. Will follow up
with revised patches.
-Daniel

> 
> > 
> > Thanks, Daniel
> > 
> > >
> > > >  	} else if (USES_PPGTT(dev)) {
> > > >  		/* For platforms which only have aliasing PPGTT, we fake the
> > > >  		 * address space and refcounting. */
> > > > @@ -361,7 +350,7 @@ int i915_gem_context_init(struct drm_device
> > *dev)
> > > >  		}
> > > >  	}
> > > >
> > > > -	ctx = i915_gem_create_context(dev, NULL, USES_PPGTT(dev));
> > > > +	ctx = i915_gem_create_context(dev, NULL,
> > > > USES_FULL_PPGTT(dev));
> > > >  	if (IS_ERR(ctx)) {
> > > >  		DRM_ERROR("Failed to create default global context (error
> > > > %ld)\n",
> > > >  			  PTR_ERR(ctx));
> > > >  }
> > > >
> > > > --
> > > > 1.9.3
> > >
> > 
> > 
> > 
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > +41 (0) 79 365 57 48 - http://blog.ffwll.ch



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