Re: [PATCH] drm/i915: properly restore the ppgtt page directory on resume

[Ben Widawsky <ben@bwidawsk.net>]

NVM, Jesse has corrected me on IRC.

On Wed, 28 Mar 2012 10:04:09 -0700
Ben Widawsky <ben@bwidawsk.net> wrote:

> On Wed, 28 Mar 2012 09:05:52 +0200
> Daniel Vetter <daniel@ffwll.ch> wrote:
> 
> > On Tue, Mar 27, 2012 at 07:30:11PM -0700, Ben Widawsky wrote:
> > > On Thu, 22 Mar 2012 00:14:43 +0100
> > > Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> > > 
> > > > The ppgtt page directory lives in a snatched part of the gtt pte
> > > > range. Which naturally gets cleared on hibernate when we pull
> > > > the power. Suspend to ram (which is what I've tested) works
> > > > because despite the fact that this is a mmio region, it is
> > > > actually back by system ram.
> > > > 
> > > > Fix this by moving the page directory setup code to the ppgtt
> > > > init code (which gets called on resume).
> > > > 
> > > > This fixes hibernate on my ivb and snb.
> > > > 
> > > > Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > > ---
> > > >  drivers/gpu/drm/i915/i915_gem.c     |   22
> > > > +++++++++++++++++++++- drivers/gpu/drm/i915/i915_gem_gtt.c |
> > > > 9 --------- 2 files changed, 21 insertions(+), 10 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/i915_gem.c
> > > > b/drivers/gpu/drm/i915/i915_gem.c index 863e14a..6ab44be 100644
> > > > --- a/drivers/gpu/drm/i915/i915_gem.c
> > > > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > > > @@ -3766,12 +3766,32 @@ void i915_gem_init_ppgtt(struct
> > > > drm_device *dev) drm_i915_private_t *dev_priv =
> > > > dev->dev_private; uint32_t pd_offset;
> > > >  	struct intel_ring_buffer *ring;
> > > > +	struct i915_hw_ppgtt *ppgtt =
> > > > dev_priv->mm.aliasing_ppgtt;
> > > > +	uint32_t __iomem *pd_addr;
> > > > +	uint32_t pd_entry;
> > > >  	int i;
> > > >  
> > > >  	if (!dev_priv->mm.aliasing_ppgtt)
> > > >  		return;
> > > >  
> > > > -	pd_offset = dev_priv->mm.aliasing_ppgtt->pd_offset;
> > > > +
> > > > +	pd_addr = dev_priv->mm.gtt->gtt +
> > > > ppgtt->pd_offset/sizeof(uint32_t);
> > > > +	for (i = 0; i < ppgtt->num_pd_entries; i++) {
> > > > +		dma_addr_t pt_addr;
> > > > +
> > > > +		if (dev_priv->mm.gtt->needs_dmar)
> > > > +			pt_addr = ppgtt->pt_dma_addr[i];
> > > > +		else
> > > > +			pt_addr =
> > > > page_to_phys(ppgtt->pt_pages[i]); +
> > > > +		pd_entry = GEN6_PDE_ADDR_ENCODE(pt_addr);
> > > > +		pd_entry |= GEN6_PDE_VALID;
> > > > +
> > > > +		writel(pd_entry, pd_addr + i);
> > > > +	}
> > > > +	readl(pd_addr);
> > > 
> > > I know I reviewed the other usage of this before, but remind me
> > > why we need readl again (comment it, maybe?).
> > 
> > The hw intercepts these mmio writes before it forwards them to
> > memory to shoot down any system agent tlbs (i.e. for cpu access).
> > Before we can clain to have set up global gtt entries we need to
> > read back (so that the hw can stall us until it has finished tlb
> > flushing). Standard procedure to ensure the hw sees the mmio
> > ordering we want it to see, no comment needed imo.
> 
> Since your patch is simply moving this logic around, I don't think
> it's worth debating this too much further...
> I didn't understand your explanation. Are you saying you are using
> this as an I/O barrier?  Any reason why wmb() is not sufficient?
> 
> > 
> > > > +
> > > > +	pd_offset = ppgtt->pd_offset;
> > > >  	pd_offset /= 64; /* in cachelines, */
> > > >  	pd_offset <<= 16;
> > > >  
> > > > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c
> > > > b/drivers/gpu/drm/i915/i915_gem_gtt.c index 72be806..99b27ff
> > > > 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> > > > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > > > @@ -65,9 +65,7 @@ int i915_gem_init_aliasing_ppgtt(struct
> > > > drm_device *dev) {
> > > >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > > >  	struct i915_hw_ppgtt *ppgtt;
> > > > -	uint32_t pd_entry;
> > > >  	unsigned first_pd_entry_in_global_pt;
> > > > -	uint32_t __iomem *pd_addr;
> > > >  	int i;
> > > >  	int ret = -ENOMEM;
> > > >  
> > > > @@ -100,7 +98,6 @@ int i915_gem_init_aliasing_ppgtt(struct
> > > > drm_device *dev) goto err_pt_alloc;
> > > >  	}
> > > >  
> > > > -	pd_addr = dev_priv->mm.gtt->gtt +
> > > > first_pd_entry_in_global_pt; for (i = 0; i <
> > > > ppgtt->num_pd_entries; i++) { dma_addr_t pt_addr;
> > > >  		if (dev_priv->mm.gtt->needs_dmar) {
> > > > @@ -117,13 +114,7 @@ int i915_gem_init_aliasing_ppgtt(struct
> > > > drm_device *dev) ppgtt->pt_dma_addr[i] = pt_addr;
> > > >  		} else
> > > >  			pt_addr =
> > > > page_to_phys(ppgtt->pt_pages[i]); -
> > > > -		pd_entry = GEN6_PDE_ADDR_ENCODE(pt_addr);
> > > > -		pd_entry |= GEN6_PDE_VALID;
> > > > -
> > > > -		writel(pd_entry, pd_addr + i);
> > > >  	}
> > > > -	readl(pd_addr);
> > > >  
> > > >  	ppgtt->scratch_page_dma_addr =
> > > > dev_priv->mm.gtt->scratch_page_dma; 
> > > 
> > > It's kind of hard for me to tell that the GPU won't access
> > > anything on resume that might kill things before hitting
> > > init_ppgtt. Do you think it makes sense to disable ppgtt before
> > > suspending?
> > 
> > Afaik yanking the power does that for us - before I've re-enabled
> > ppgtt on the resume path (the bit frobbing, not the page directory
> > restoring) the hw just kept on using global gtt.
> 
> Again since this is just moving code around, I don't think further
> discussion here is required.
> 
> > -Daniel
> 
> Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx