[Intel-gfx] [PATCH] drm/i915: properly restore the ppgtt page directory on resume

[Ben Widawsky]

On Thu, 22 Mar 2012 00:14:43 +0100
Daniel Vetter <daniel.vetter at 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 at 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?).

> +
> +	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?