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

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

[Daniel Vetter]

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);
+
+	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;
 
-- 
1.7.9

Re: [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@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?).

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

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

[Daniel Vetter]

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.

> > +
> > +	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.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

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

[Ben Widawsky]

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>

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

[Ben Widawsky]

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