[PATCH 00/12] PPGTT pre-work

[PATCH 00/12] PPGTT pre-work

[Ben Widawsky]

These are all the patches that I have which shouldn't be too controversial for
full PPGTT support. I'd prefer if we kept bikesheds to a minimum since much of
this code will be either modified, or used in ways which it is not currently
used.

The first two seem to fix BDW reset for me, and are needed for upcoming PPGTT
work. All the rest are random stuff I found alone the way.

Ben Widawsky (12):
  drm/i915: Fix BDW PPGTT error path
  drm/i915: Provide PDP updates via MMIO
  drm/i915: Add a few missed bits to the mm
  drm/i915: Don't unconditionally try to deref aliasing ppgtt
  drm/i915: Disallow dynamic ppgtt param modification
  drm/i915: Demote drop_caches_set print
  drm/i915: Removed unused vm args
  drm/i915: Remove defunct ctx switch comments
  drm/i915: Missed dropped VMA conversion
  drm/i915: Allow ggtt lookups to not WARN
  drm/i915: Takedown drm_mm on failed gtt setup
  drm/i915: Move the gtt mm takedown to cleanup

 drivers/gpu/drm/i915/i915_debugfs.c        |  2 +-
 drivers/gpu/drm/i915/i915_dma.c            |  1 -
 drivers/gpu/drm/i915/i915_drv.c            |  2 +-
 drivers/gpu/drm/i915/i915_gem.c            |  9 ++++++---
 drivers/gpu/drm/i915/i915_gem_context.c    |  2 --
 drivers/gpu/drm/i915/i915_gem_execbuffer.c | 11 +++++------
 drivers/gpu/drm/i915/i915_gem_gtt.c        | 28 +++++++++++++++++++++++++---
 7 files changed, 38 insertions(+), 17 deletions(-)

-- 
1.8.4.2

[PATCH 01/12] drm/i915: Fix BDW PPGTT error path

[Ben Widawsky]

When we fail for some reason on loading the PDPs, it would be wise to
disable the PPGTT in the ring registers. If we do not do this, we have
undefined results.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_gem_gtt.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index efb5dab..1a5272c 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -238,10 +238,16 @@ static int gen8_ppgtt_enable(struct drm_device *dev)
 		for_each_ring(ring, dev_priv, j) {
 			ret = gen8_write_pdp(ring, i, addr);
 			if (ret)
-				return ret;
+				goto err_out;
 		}
 	}
 	return 0;
+
+err_out:
+	for_each_ring(ring, dev_priv, j)
+		I915_WRITE(RING_MODE_GEN7(ring),
+			   _MASKED_BIT_DISABLE(GFX_PPGTT_ENABLE));
+	return ret;
 }
 
 static void gen8_ppgtt_clear_range(struct i915_address_space *vm,
-- 
1.8.4.2

Re: [PATCH 01/12] drm/i915: Fix BDW PPGTT error path

[Chris Wilson]

On Mon, Nov 25, 2013 at 09:54:32AM -0800, Ben Widawsky wrote:
> When we fail for some reason on loading the PDPs, it would be wise to
> disable the PPGTT in the ring registers. If we do not do this, we have
> undefined results.
> 
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH 02/12] drm/i915: Provide PDP updates via MMIO

[Ben Widawsky]

The initial implementation of this function used MMIO to write the PDPs.
Upon review it was determined (correctly) that the docs say to use LRI.
The issue is there are times where we want to do a synchronous write
(GPU reset).

I've tested this, and it works. I've verified with as many people as
possible that it should work.

This should fix the failing reset problems.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_gem_gtt.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 1a5272c..96dbf3d 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -197,12 +197,19 @@ static gen6_gtt_pte_t iris_pte_encode(dma_addr_t addr,
 
 /* Broadwell Page Directory Pointer Descriptors */
 static int gen8_write_pdp(struct intel_ring_buffer *ring, unsigned entry,
-			   uint64_t val)
+			   uint64_t val, bool synchronous)
 {
+	struct drm_i915_private *dev_priv = ring->dev->dev_private;
 	int ret;
 
 	BUG_ON(entry >= 4);
 
+	if (synchronous) {
+		I915_WRITE(GEN8_RING_PDP_UDW(ring, entry), val >> 32);
+		I915_WRITE(GEN8_RING_PDP_LDW(ring, entry), (u32)val);
+		return 0;
+	}
+
 	ret = intel_ring_begin(ring, 6);
 	if (ret)
 		return ret;
@@ -236,7 +243,8 @@ static int gen8_ppgtt_enable(struct drm_device *dev)
 	for (i = used_pd - 1; i >= 0; i--) {
 		dma_addr_t addr = ppgtt->pd_dma_addr[i];
 		for_each_ring(ring, dev_priv, j) {
-			ret = gen8_write_pdp(ring, i, addr);
+			ret = gen8_write_pdp(ring, i, addr,
+					     i915_reset_in_progress(&dev_priv->gpu_error));
 			if (ret)
 				goto err_out;
 		}
-- 
1.8.4.2

Re: [PATCH 02/12] drm/i915: Provide PDP updates via MMIO

[Ben Widawsky]

On Mon, Nov 25, 2013 at 09:54:33AM -0800, Ben Widawsky wrote:
> The initial implementation of this function used MMIO to write the PDPs.
> Upon review it was determined (correctly) that the docs say to use LRI.
> The issue is there are times where we want to do a synchronous write
> (GPU reset).
> 
> I've tested this, and it works. I've verified with as many people as
> possible that it should work.
> 
> This should fix the failing reset problems.
> 
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>

One addition to the commit message. I haven't tried this patch alone for
the reset fix. I have a bunch of other stuff on top. I can try to
isolate if this was all that was required ASAP.

[snip]
 

-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH 02/12] drm/i915: Provide PDP updates via MMIO

[Chris Wilson]

On Mon, Nov 25, 2013 at 09:54:33AM -0800, Ben Widawsky wrote:
> The initial implementation of this function used MMIO to write the PDPs.
> Upon review it was determined (correctly) that the docs say to use LRI.
> The issue is there are times where we want to do a synchronous write
> (GPU reset).
> 
> I've tested this, and it works. I've verified with as many people as
> possible that it should work.
> 
> This should fix the failing reset problems.
> 
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> ---
>  drivers/gpu/drm/i915/i915_gem_gtt.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 1a5272c..96dbf3d 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -197,12 +197,19 @@ static gen6_gtt_pte_t iris_pte_encode(dma_addr_t addr,
>  
>  /* Broadwell Page Directory Pointer Descriptors */
>  static int gen8_write_pdp(struct intel_ring_buffer *ring, unsigned entry,
> -			   uint64_t val)
> +			   uint64_t val, bool synchronous)
>  {
> +	struct drm_i915_private *dev_priv = ring->dev->dev_private;
>  	int ret;
>  

i915_reset_in_progress(&dev_priv->gpu_error));
doesn't actually mean that we are in the middle of a reset. Or does
it?  Anyway intel_ring_begin() returns EIO/EAGAIN so we do not need
to pass down the parameter. But the issue with mixing and matching LRI
vs mmio is that if this was not a reset call, then we just upset the
GPU even further.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 02/12] drm/i915: Provide PDP updates via MMIO

[Ben Widawsky]

On Mon, Nov 25, 2013 at 06:18:52PM +0000, Chris Wilson wrote:
> On Mon, Nov 25, 2013 at 09:54:33AM -0800, Ben Widawsky wrote:
> > The initial implementation of this function used MMIO to write the PDPs.
> > Upon review it was determined (correctly) that the docs say to use LRI.
> > The issue is there are times where we want to do a synchronous write
> > (GPU reset).
> > 
> > I've tested this, and it works. I've verified with as many people as
> > possible that it should work.
> > 
> > This should fix the failing reset problems.
> > 
> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> > ---
> >  drivers/gpu/drm/i915/i915_gem_gtt.c | 12 ++++++++++--
> >  1 file changed, 10 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > index 1a5272c..96dbf3d 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > @@ -197,12 +197,19 @@ static gen6_gtt_pte_t iris_pte_encode(dma_addr_t addr,
> >  
> >  /* Broadwell Page Directory Pointer Descriptors */
> >  static int gen8_write_pdp(struct intel_ring_buffer *ring, unsigned entry,
> > -			   uint64_t val)
> > +			   uint64_t val, bool synchronous)
> >  {
> > +	struct drm_i915_private *dev_priv = ring->dev->dev_private;
> >  	int ret;
> >  
> 
> i915_reset_in_progress(&dev_priv->gpu_error));
> doesn't actually mean that we are in the middle of a reset. Or does
> it?  Anyway intel_ring_begin() returns EIO/EAGAIN so we do not need
> to pass down the parameter. But the issue with mixing and matching LRI
> vs mmio is that if this was not a reset call, then we just upset the
> GPU even further.
> -Chris

/me sighs

The synchronous argument comes from the future. There are
three places where one could conceivably use it:
1. before rings are up
2. during hang
3. after rings are shut down

With the current code, only #2 is actually possible. I don't like
checking EIO/EAGAIN as there are cases where we expect failure, and
cases where we do not. Being able to strain out which one is which, is
helpful, and [in the future] the caller is the one that can take
appropriate action. Also I found also using the argument a bit nicer
since every gen would have to implement the same logic to determine if
the rings were usage.

You'll have to trust me that I won't use MMIO when I shouldn't be using
it.

I can understand your comment at this stage, but I hope my reasoning
makes sense. (Feel free to view my ppgtt branch if you'd like)

> 
> -- 
> Chris Wilson, Intel Open Source Technology Centre

-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH 02/12] drm/i915: Provide PDP updates via MMIO

[Chris Wilson]

On Mon, Nov 25, 2013 at 10:28:26AM -0800, Ben Widawsky wrote:
> On Mon, Nov 25, 2013 at 06:18:52PM +0000, Chris Wilson wrote:
> > On Mon, Nov 25, 2013 at 09:54:33AM -0800, Ben Widawsky wrote:
> > > The initial implementation of this function used MMIO to write the PDPs.
> > > Upon review it was determined (correctly) that the docs say to use LRI.
> > > The issue is there are times where we want to do a synchronous write
> > > (GPU reset).
> > > 
> > > I've tested this, and it works. I've verified with as many people as
> > > possible that it should work.
> > > 
> > > This should fix the failing reset problems.
> > > 
> > > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> > > ---
> > >  drivers/gpu/drm/i915/i915_gem_gtt.c | 12 ++++++++++--
> > >  1 file changed, 10 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > > index 1a5272c..96dbf3d 100644
> > > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> > > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > > @@ -197,12 +197,19 @@ static gen6_gtt_pte_t iris_pte_encode(dma_addr_t addr,
> > >  
> > >  /* Broadwell Page Directory Pointer Descriptors */
> > >  static int gen8_write_pdp(struct intel_ring_buffer *ring, unsigned entry,
> > > -			   uint64_t val)
> > > +			   uint64_t val, bool synchronous)
> > >  {
> > > +	struct drm_i915_private *dev_priv = ring->dev->dev_private;
> > >  	int ret;
> > >  
> > 
> > i915_reset_in_progress(&dev_priv->gpu_error));
> > doesn't actually mean that we are in the middle of a reset. Or does
> > it?  Anyway intel_ring_begin() returns EIO/EAGAIN so we do not need
> > to pass down the parameter. But the issue with mixing and matching LRI
> > vs mmio is that if this was not a reset call, then we just upset the
> > GPU even further.
> > -Chris
> 
> /me sighs
> 
> The synchronous argument comes from the future. There are
> three places where one could conceivably use it:
> 1. before rings are up
> 2. during hang
> 3. after rings are shut down
> 
> With the current code, only #2 is actually possible. I don't like
> checking EIO/EAGAIN as there are cases where we expect failure, and
> cases where we do not. Being able to strain out which one is which, is
> helpful, and [in the future] the caller is the one that can take
> appropriate action. Also I found also using the argument a bit nicer
> since every gen would have to implement the same logic to determine if
> the rings were usage.
> 
> You'll have to trust me that I won't use MMIO when I shouldn't be using
> it.
> 
> I can understand your comment at this stage, but I hope my reasoning
> makes sense. (Feel free to view my ppgtt branch if you'd like)

Sure having a parameter will be useful to do other things, I am just not
convinced that what you want to do in this patch is sane. Currently, we
only call ->enable in i915_gem_init_hw(), so what you do here could
work. But presumably, with full-ppgtt it will then be possible to call
between resets making us vulnerable to weird races?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 02/12] drm/i915: Provide PDP updates via MMIO

[Ben Widawsky]

On Mon, Nov 25, 2013 at 06:44:09PM +0000, Chris Wilson wrote:
> On Mon, Nov 25, 2013 at 10:28:26AM -0800, Ben Widawsky wrote:
> > On Mon, Nov 25, 2013 at 06:18:52PM +0000, Chris Wilson wrote:
> > > On Mon, Nov 25, 2013 at 09:54:33AM -0800, Ben Widawsky wrote:
> > > > The initial implementation of this function used MMIO to write the PDPs.
> > > > Upon review it was determined (correctly) that the docs say to use LRI.
> > > > The issue is there are times where we want to do a synchronous write
> > > > (GPU reset).
> > > > 
> > > > I've tested this, and it works. I've verified with as many people as
> > > > possible that it should work.
> > > > 
> > > > This should fix the failing reset problems.
> > > > 
> > > > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> > > > ---
> > > >  drivers/gpu/drm/i915/i915_gem_gtt.c | 12 ++++++++++--
> > > >  1 file changed, 10 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > > > index 1a5272c..96dbf3d 100644
> > > > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> > > > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > > > @@ -197,12 +197,19 @@ static gen6_gtt_pte_t iris_pte_encode(dma_addr_t addr,
> > > >  
> > > >  /* Broadwell Page Directory Pointer Descriptors */
> > > >  static int gen8_write_pdp(struct intel_ring_buffer *ring, unsigned entry,
> > > > -			   uint64_t val)
> > > > +			   uint64_t val, bool synchronous)
> > > >  {
> > > > +	struct drm_i915_private *dev_priv = ring->dev->dev_private;
> > > >  	int ret;
> > > >  
> > > 
> > > i915_reset_in_progress(&dev_priv->gpu_error));
> > > doesn't actually mean that we are in the middle of a reset. Or does
> > > it?  Anyway intel_ring_begin() returns EIO/EAGAIN so we do not need
> > > to pass down the parameter. But the issue with mixing and matching LRI
> > > vs mmio is that if this was not a reset call, then we just upset the
> > > GPU even further.
> > > -Chris
> > 
> > /me sighs
> > 
> > The synchronous argument comes from the future. There are
> > three places where one could conceivably use it:
> > 1. before rings are up
> > 2. during hang
> > 3. after rings are shut down
> > 
> > With the current code, only #2 is actually possible. I don't like
> > checking EIO/EAGAIN as there are cases where we expect failure, and
> > cases where we do not. Being able to strain out which one is which, is
> > helpful, and [in the future] the caller is the one that can take
> > appropriate action. Also I found also using the argument a bit nicer
> > since every gen would have to implement the same logic to determine if
> > the rings were usage.
> > 
> > You'll have to trust me that I won't use MMIO when I shouldn't be using
> > it.
> > 
> > I can understand your comment at this stage, but I hope my reasoning
> > makes sense. (Feel free to view my ppgtt branch if you'd like)
> 
> Sure having a parameter will be useful to do other things, I am just not
> convinced that what you want to do in this patch is sane. Currently, we
> only call ->enable in i915_gem_init_hw(), so what you do here could
> work. But presumably, with full-ppgtt it will then be possible to call
> between resets making us vulnerable to weird races?
> -Chris
> 

In the future case, as long as the MMIO always follows the GPU reset
(before we're using the rings), it should be race free.

FWIW, in the future case, MMIO is still always relegated to case #2 (for
exactly one patch, it is both #1, and #2.) The other case is
aliasing PPGTT only ofc.

-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH 02/12] drm/i915: Provide PDP updates via MMIO

[Daniel Vetter]

On Mon, Nov 25, 2013 at 09:54:33AM -0800, Ben Widawsky wrote:
> The initial implementation of this function used MMIO to write the PDPs.
> Upon review it was determined (correctly) that the docs say to use LRI.
> The issue is there are times where we want to do a synchronous write
> (GPU reset).
> 
> I've tested this, and it works. I've verified with as many people as
> possible that it should work.
> 
> This should fix the failing reset problems.
> 
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>

This makes me nervous, for two reasons:
- Historically we've _always_ ended up in hilarity, bloodshed and tears
  when having different setup/teardown code in the different parts of the
  driver. Always.
- Looking at the reset code we should be initializing the rings before we
  try to re-enable ppgtt. So if I don't misread stuff horribly this needs
  at least one giant comment explaining what's exactly going on here -
  either the hw is busted or we have some other issue.

Aside: Mika noticed that we're loosing a bunch of the w/a settings since a
few of the per-ring workarounds have been errornously put into
init_clock_gating. We need to move those to the correct per-ring init
function. This is even more important once we have per-ring reset.

Cheers, Daniel
> ---
>  drivers/gpu/drm/i915/i915_gem_gtt.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 1a5272c..96dbf3d 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -197,12 +197,19 @@ static gen6_gtt_pte_t iris_pte_encode(dma_addr_t addr,
>  
>  /* Broadwell Page Directory Pointer Descriptors */
>  static int gen8_write_pdp(struct intel_ring_buffer *ring, unsigned entry,
> -			   uint64_t val)
> +			   uint64_t val, bool synchronous)
>  {
> +	struct drm_i915_private *dev_priv = ring->dev->dev_private;
>  	int ret;
>  
>  	BUG_ON(entry >= 4);
>  
> +	if (synchronous) {
> +		I915_WRITE(GEN8_RING_PDP_UDW(ring, entry), val >> 32);
> +		I915_WRITE(GEN8_RING_PDP_LDW(ring, entry), (u32)val);
> +		return 0;
> +	}
> +
>  	ret = intel_ring_begin(ring, 6);
>  	if (ret)
>  		return ret;
> @@ -236,7 +243,8 @@ static int gen8_ppgtt_enable(struct drm_device *dev)
>  	for (i = used_pd - 1; i >= 0; i--) {
>  		dma_addr_t addr = ppgtt->pd_dma_addr[i];
>  		for_each_ring(ring, dev_priv, j) {
> -			ret = gen8_write_pdp(ring, i, addr);
> +			ret = gen8_write_pdp(ring, i, addr,
> +					     i915_reset_in_progress(&dev_priv->gpu_error));
>  			if (ret)
>  				goto err_out;
>  		}
> -- 
> 1.8.4.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@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

[PATCH 03/12] drm/i915: Add a few missed bits to the mm

[Ben Widawsky]

This should really have been added in BDW integration, as well as:

commit 93bd8649dba3155d1a0ba2a902d9c49f1c75a1da
Author: Ben Widawsky <ben@bwidawsk.net>
Date:   Tue Jul 16 16:50:06 2013 -0700

    drm/i915: Put the mm in the parent address space

It didn't really matter before, but it will in the future.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_gem_gtt.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 96dbf3d..5ec4ba0 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -330,6 +330,8 @@ static void gen8_ppgtt_cleanup(struct i915_address_space *vm)
 		container_of(vm, struct i915_hw_ppgtt, base);
 	int i, j;
 
+	drm_mm_takedown(&vm->mm);
+
 	for (i = 0; i < ppgtt->num_pd_pages ; i++) {
 		if (ppgtt->pd_dma_addr[i]) {
 			pci_unmap_page(ppgtt->base.dev->pdev,
@@ -393,6 +395,8 @@ static int gen8_ppgtt_init(struct i915_hw_ppgtt *ppgtt, uint64_t size)
 	ppgtt->base.clear_range = gen8_ppgtt_clear_range;
 	ppgtt->base.insert_entries = gen8_ppgtt_insert_entries;
 	ppgtt->base.cleanup = gen8_ppgtt_cleanup;
+	ppgtt->base.start = 0;
+	ppgtt->base.total = ppgtt->num_pt_pages * GEN8_PTES_PER_PAGE * PAGE_SIZE;
 
 	BUG_ON(ppgtt->num_pd_pages > GEN8_LEGACY_PDPS);
 
@@ -644,6 +648,8 @@ static int gen6_ppgtt_init(struct i915_hw_ppgtt *ppgtt)
 	ppgtt->base.insert_entries = gen6_ppgtt_insert_entries;
 	ppgtt->base.cleanup = gen6_ppgtt_cleanup;
 	ppgtt->base.scratch = dev_priv->gtt.base.scratch;
+	ppgtt->base.start = 0;
+	ppgtt->base.total = GEN6_PPGTT_PD_ENTRIES * I915_PPGTT_PT_ENTRIES * PAGE_SIZE;
 	ppgtt->pt_pages = kcalloc(ppgtt->num_pd_entries, sizeof(struct page *),
 				  GFP_KERNEL);
 	if (!ppgtt->pt_pages)
-- 
1.8.4.2

[PATCH 04/12] drm/i915: Don't unconditionally try to deref aliasing ppgtt

[Ben Widawsky]

Since the beginning, the functions which try to properly reference the
aliasing PPGTT have deferences a potentially null aliasing_ppgtt member.
Since the accessors are meant to be global, this will not do.

Introduced originally in:
commit a70a3148b0c61cb7c588ea650db785b261b378a3
Author: Ben Widawsky <ben@bwidawsk.net>
Date:   Wed Jul 31 16:59:56 2013 -0700

    drm/i915: Make proper functions for VMs

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_gem.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 40d9dcf..bc5c865 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4971,7 +4971,8 @@ 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 (vm == &dev_priv->mm.aliasing_ppgtt->base)
+	if (!dev_priv->mm.aliasing_ppgtt ||
+	    vm == &dev_priv->mm.aliasing_ppgtt->base)
 		vm = &dev_priv->gtt.base;
 
 	BUG_ON(list_empty(&o->vma_list));
@@ -5012,7 +5013,8 @@ 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 (vm == &dev_priv->mm.aliasing_ppgtt->base)
+	if (!dev_priv->mm.aliasing_ppgtt ||
+	    vm == &dev_priv->mm.aliasing_ppgtt->base)
 		vm = &dev_priv->gtt.base;
 
 	BUG_ON(list_empty(&o->vma_list));
-- 
1.8.4.2

Re: [PATCH 04/12] drm/i915: Don't unconditionally try to deref aliasing ppgtt

[Chris Wilson]

On Mon, Nov 25, 2013 at 09:54:35AM -0800, Ben Widawsky wrote:
> Since the beginning, the functions which try to properly reference the
> aliasing PPGTT have deferences a potentially null aliasing_ppgtt member.
> Since the accessors are meant to be global, this will not do.
> 
> Introduced originally in:
> commit a70a3148b0c61cb7c588ea650db785b261b378a3
> Author: Ben Widawsky <ben@bwidawsk.net>
> Date:   Wed Jul 31 16:59:56 2013 -0700
> 
>     drm/i915: Make proper functions for VMs
> 
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> ---
>  drivers/gpu/drm/i915/i915_gem.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 40d9dcf..bc5c865 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -4971,7 +4971,8 @@ 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 (vm == &dev_priv->mm.aliasing_ppgtt->base)
> +	if (!dev_priv->mm.aliasing_ppgtt ||
> +	    vm == &dev_priv->mm.aliasing_ppgtt->base)

Where's the dereference? gcc is smarter than your average bear.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 04/12] drm/i915: Don't unconditionally try to deref aliasing ppgtt

[Ben Widawsky]

On Mon, Nov 25, 2013 at 06:06:18PM +0000, Chris Wilson wrote:
> On Mon, Nov 25, 2013 at 09:54:35AM -0800, Ben Widawsky wrote:
> > Since the beginning, the functions which try to properly reference the
> > aliasing PPGTT have deferences a potentially null aliasing_ppgtt member.
> > Since the accessors are meant to be global, this will not do.
> > 
> > Introduced originally in:
> > commit a70a3148b0c61cb7c588ea650db785b261b378a3
> > Author: Ben Widawsky <ben@bwidawsk.net>
> > Date:   Wed Jul 31 16:59:56 2013 -0700
> > 
> >     drm/i915: Make proper functions for VMs
> > 
> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> > ---
> >  drivers/gpu/drm/i915/i915_gem.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index 40d9dcf..bc5c865 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -4971,7 +4971,8 @@ 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 (vm == &dev_priv->mm.aliasing_ppgtt->base)
> > +	if (!dev_priv->mm.aliasing_ppgtt ||
> > +	    vm == &dev_priv->mm.aliasing_ppgtt->base)
> 
> Where's the dereference? gcc is smarter than your average bear.
> -Chris

I had assumed: dev_priv->mm.aliasing_ppgtt->base in cases when
aliasing_ppgtt was NULL. Given that I never actually hit this, I agree
GCC must be doing something.

Is such behavior documented somewhere? (forgive the lazy)

> 
> -- 
> Chris Wilson, Intel Open Source Technology Centre

-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH 04/12] drm/i915: Don't unconditionally try to deref aliasing ppgtt

[Ville Syrjälä]

On Mon, Nov 25, 2013 at 10:10:28AM -0800, Ben Widawsky wrote:
> On Mon, Nov 25, 2013 at 06:06:18PM +0000, Chris Wilson wrote:
> > On Mon, Nov 25, 2013 at 09:54:35AM -0800, Ben Widawsky wrote:
> > > Since the beginning, the functions which try to properly reference the
> > > aliasing PPGTT have deferences a potentially null aliasing_ppgtt member.
> > > Since the accessors are meant to be global, this will not do.
> > > 
> > > Introduced originally in:
> > > commit a70a3148b0c61cb7c588ea650db785b261b378a3
> > > Author: Ben Widawsky <ben@bwidawsk.net>
> > > Date:   Wed Jul 31 16:59:56 2013 -0700
> > > 
> > >     drm/i915: Make proper functions for VMs
> > > 
> > > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> > > ---
> > >  drivers/gpu/drm/i915/i915_gem.c | 6 ++++--
> > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > > index 40d9dcf..bc5c865 100644
> > > --- a/drivers/gpu/drm/i915/i915_gem.c
> > > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > > @@ -4971,7 +4971,8 @@ 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 (vm == &dev_priv->mm.aliasing_ppgtt->base)
> > > +	if (!dev_priv->mm.aliasing_ppgtt ||
> > > +	    vm == &dev_priv->mm.aliasing_ppgtt->base)
> > 
> > Where's the dereference? gcc is smarter than your average bear.
> > -Chris
> 
> I had assumed: dev_priv->mm.aliasing_ppgtt->base in cases when
> aliasing_ppgtt was NULL. Given that I never actually hit this, I agree
> GCC must be doing something.

Sounds like another discussion on the implementation of offsetof().

> 
> Is such behavior documented somewhere? (forgive the lazy)

IIRC the C standard does say that for &*foo it's as if both & and *
weren't there (and IIRC the same for &foo[x]), but doesn't really
say that kind of thing for &foo->bar. I guess it's a gray area,
and just happens work that way on certain compilers.

In this particular case I think there's one slight issue. If
aliasing_pggtt == NULL, and someone passes in vm == NULL by
accident, then it'll take the branch and use ggtt because
&aliasing_ppgtt->base will be NULL too (due to base being at
offset 0 in the struct). Now, I don't know if a NULL
vm could survive this far into the code, but it it can, then it
might make debugging a bit more "fun".

As a side note, we actually depend on such things in the mode
setting code. Ie. &((struct intel_crtc*)NULL)->base must be
NULL, otherwise interesting bugs would happen.

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 04/12] drm/i915: Don't unconditionally try to deref aliasing ppgtt

[Daniel Vetter]

On Mon, Nov 25, 2013 at 09:08:50PM +0200, Ville Syrjälä wrote:
> On Mon, Nov 25, 2013 at 10:10:28AM -0800, Ben Widawsky wrote:
> > On Mon, Nov 25, 2013 at 06:06:18PM +0000, Chris Wilson wrote:
> > > On Mon, Nov 25, 2013 at 09:54:35AM -0800, Ben Widawsky wrote:
> > > > Since the beginning, the functions which try to properly reference the
> > > > aliasing PPGTT have deferences a potentially null aliasing_ppgtt member.
> > > > Since the accessors are meant to be global, this will not do.
> > > > 
> > > > Introduced originally in:
> > > > commit a70a3148b0c61cb7c588ea650db785b261b378a3
> > > > Author: Ben Widawsky <ben@bwidawsk.net>
> > > > Date:   Wed Jul 31 16:59:56 2013 -0700
> > > > 
> > > >     drm/i915: Make proper functions for VMs
> > > > 
> > > > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> > > > ---
> > > >  drivers/gpu/drm/i915/i915_gem.c | 6 ++++--
> > > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > > > index 40d9dcf..bc5c865 100644
> > > > --- a/drivers/gpu/drm/i915/i915_gem.c
> > > > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > > > @@ -4971,7 +4971,8 @@ 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 (vm == &dev_priv->mm.aliasing_ppgtt->base)
> > > > +	if (!dev_priv->mm.aliasing_ppgtt ||
> > > > +	    vm == &dev_priv->mm.aliasing_ppgtt->base)
> > > 
> > > Where's the dereference? gcc is smarter than your average bear.
> > > -Chris
> > 
> > I had assumed: dev_priv->mm.aliasing_ppgtt->base in cases when
> > aliasing_ppgtt was NULL. Given that I never actually hit this, I agree
> > GCC must be doing something.
> 
> Sounds like another discussion on the implementation of offsetof().
> 
> > 
> > Is such behavior documented somewhere? (forgive the lazy)
> 
> IIRC the C standard does say that for &*foo it's as if both & and *
> weren't there (and IIRC the same for &foo[x]), but doesn't really
> say that kind of thing for &foo->bar. I guess it's a gray area,
> and just happens work that way on certain compilers.
> 
> In this particular case I think there's one slight issue. If
> aliasing_pggtt == NULL, and someone passes in vm == NULL by
> accident, then it'll take the branch and use ggtt because
> &aliasing_ppgtt->base will be NULL too (due to base being at
> offset 0 in the struct). Now, I don't know if a NULL
> vm could survive this far into the code, but it it can, then it
> might make debugging a bit more "fun".

If you pass in a NULL vm then you get to keep both pieces. And wrt
portability atm you can pick any compiler for the linux kernel as long as
it's gcc compatible. So imo nothing to really worry about.

Aside: We really should start to ditch all these (vm, obj) pair lookups
and move to using vmas everywhere ...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH 05/12] drm/i915: Disallow dynamic ppgtt param modification

[Ben Widawsky]

This would have never worked.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_drv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 583adcb..cef0422 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -114,7 +114,7 @@ MODULE_PARM_DESC(enable_hangcheck,
 		"(default: true)");
 
 int i915_enable_ppgtt __read_mostly = -1;
-module_param_named(i915_enable_ppgtt, i915_enable_ppgtt, int, 0600);
+module_param_named(i915_enable_ppgtt, i915_enable_ppgtt, int, 0400);
 MODULE_PARM_DESC(i915_enable_ppgtt,
 		"Enable PPGTT (default: true)");
 
-- 
1.8.4.2

Re: [PATCH 05/12] drm/i915: Disallow dynamic ppgtt param modification

[Chris Wilson]

On Mon, Nov 25, 2013 at 09:54:36AM -0800, Ben Widawsky wrote:
> This would have never worked.
> 
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH 06/12] drm/i915: Demote drop_caches_set print

[Ben Widawsky]

Many tests call this ad naseum now (in an infinite loop, very often).
It clutters the logs. Actually, I'd rather drop it completely...

Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 97c304e..ee4d465 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2744,7 +2744,7 @@ i915_drop_caches_set(void *data, u64 val)
 	struct i915_vma *vma, *x;
 	int ret;
 
-	DRM_DEBUG_DRIVER("Dropping caches: 0x%08llx\n", val);
+	DRM_DEBUG("Dropping caches: 0x%08llx\n", val);
 
 	/* No need to check and wait for gpu resets, only libdrm auto-restarts
 	 * on ioctls on -EAGAIN. */
-- 
1.8.4.2

Re: [PATCH 06/12] drm/i915: Demote drop_caches_set print

[Chris Wilson]

On Mon, Nov 25, 2013 at 09:54:37AM -0800, Ben Widawsky wrote:
> Many tests call this ad naseum now (in an infinite loop, very often).
> It clutters the logs. Actually, I'd rather drop it completely...
> 
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>

Acked-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH 07/12] drm/i915: Removed unused vm args

[Ben Widawsky]

i915_gem_execbuffer_relocate became defunct in:
commit 27173f1f95db5e74ceb35fe9a2f2f348ea11bac9
Author: Ben Widawsky <ben@bwidawsk.net>
Date:   Wed Aug 14 11:38:36 2013 +0200

    drm/i915: Convert execbuf code to use vmas

eb_create: never used?

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_gem_execbuffer.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 885d595..740fc1d 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -43,7 +43,7 @@ struct eb_vmas {
 };
 
 static struct eb_vmas *
-eb_create(struct drm_i915_gem_execbuffer2 *args, struct i915_address_space *vm)
+eb_create(struct drm_i915_gem_execbuffer2 *args)
 {
 	struct eb_vmas *eb = NULL;
 
@@ -454,8 +454,7 @@ i915_gem_execbuffer_relocate_vma_slow(struct i915_vma *vma,
 }
 
 static int
-i915_gem_execbuffer_relocate(struct eb_vmas *eb,
-			     struct i915_address_space *vm)
+i915_gem_execbuffer_relocate(struct eb_vmas *eb)
 {
 	struct i915_vma *vma;
 	int ret = 0;
@@ -1102,7 +1101,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 		goto pre_mutex_err;
 	}
 
-	eb = eb_create(args, vm);
+	eb = eb_create(args);
 	if (eb == NULL) {
 		mutex_unlock(&dev->struct_mutex);
 		ret = -ENOMEM;
@@ -1125,7 +1124,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 
 	/* The objects are in their final locations, apply the relocations. */
 	if (need_relocs)
-		ret = i915_gem_execbuffer_relocate(eb, vm);
+		ret = i915_gem_execbuffer_relocate(eb);
 	if (ret) {
 		if (ret == -EFAULT) {
 			ret = i915_gem_execbuffer_relocate_slow(dev, args, file, ring,
-- 
1.8.4.2

[PATCH 08/12] drm/i915: Remove defunct ctx switch comments

[Ben Widawsky]

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_gem_context.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 2ec122a..4187704 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -492,8 +492,6 @@ static int do_switch(struct i915_hw_context *to)
  * @ring: ring for which we'll execute the context switch
  * @file_priv: file_priv associated with the context, may be NULL
  * @id: context id number
- * @seqno: sequence number by which the new context will be switched to
- * @flags:
  *
  * The context life cycle is simple. The context refcount is incremented and
  * decremented by 1 and create and destroy. If the context is in use by the GPU,
-- 
1.8.4.2

[PATCH 09/12] drm/i915: Missed dropped VMA conversion

[Ben Widawsky]

This belonged in
commit 07fe0b12800d4752d729d4122c01f41f80a5ba5a
Author: Ben Widawsky <ben@bwidawsk.net>
Date:   Wed Jul 31 17:00:10 2013 -0700

    drm/i915: plumb VM into bind/unbind code

But it was somehow missed along the way.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_gem_execbuffer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 740fc1d..b800fe4 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -307,7 +307,7 @@ i915_gem_execbuffer_relocate_entry(struct drm_i915_gem_object *obj,
 	target_i915_obj = target_vma->obj;
 	target_obj = &target_vma->obj->base;
 
-	target_offset = i915_gem_obj_ggtt_offset(target_i915_obj);
+	target_offset = target_vma->node.start;
 
 	/* Sandybridge PPGTT errata: We need a global gtt mapping for MI and
 	 * pipe_control writes because the gpu doesn't properly redirect them
-- 
1.8.4.2

[PATCH 10/12] drm/i915: Allow ggtt lookups to not WARN

[Ben Widawsky]

To be able to effectively use the GGTT object lookup function, we don't
want to warn when there is no GGTT mapping. Let the caller deal with it
instead.

Originally, I had intended to have this behavior, and has not
introduced the WARN. It was introduced during review with the addition
of the follow commit

commit 5c2abbeab798154166d42fce4f71790caa6dd9bc
Author: Ben Widawsky <benjamin.widawsky@intel.com>
Date:   Tue Sep 24 09:57:57 2013 -0700

    drm/i915: Provide a cheap ggtt vma lookup

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_gem.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index bc5c865..6388706 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -5069,7 +5069,7 @@ struct i915_vma *i915_gem_obj_to_ggtt(struct drm_i915_gem_object *obj)
 		return NULL;
 
 	vma = list_first_entry(&obj->vma_list, typeof(*vma), vma_link);
-	if (WARN_ON(vma->vm != obj_to_ggtt(obj)))
+	if (vma->vm != obj_to_ggtt(obj))
 		return NULL;
 
 	return vma;
-- 
1.8.4.2

Re: [PATCH 10/12] drm/i915: Allow ggtt lookups to not WARN

[Daniel Vetter]

On Mon, Nov 25, 2013 at 09:54:41AM -0800, Ben Widawsky wrote:
> To be able to effectively use the GGTT object lookup function, we don't
> want to warn when there is no GGTT mapping. Let the caller deal with it
> instead.
> 
> Originally, I had intended to have this behavior, and has not
> introduced the WARN. It was introduced during review with the addition
> of the follow commit
> 
> commit 5c2abbeab798154166d42fce4f71790caa6dd9bc
> Author: Ben Widawsky <benjamin.widawsky@intel.com>
> Date:   Tue Sep 24 09:57:57 2013 -0700
> 
>     drm/i915: Provide a cheap ggtt vma lookup
> 
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> ---
>  drivers/gpu/drm/i915/i915_gem.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index bc5c865..6388706 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -5069,7 +5069,7 @@ struct i915_vma *i915_gem_obj_to_ggtt(struct drm_i915_gem_object *obj)
>  		return NULL;
>  
>  	vma = list_first_entry(&obj->vma_list, typeof(*vma), vma_link);
> -	if (WARN_ON(vma->vm != obj_to_ggtt(obj)))
> +	if (vma->vm != obj_to_ggtt(obj))

Makes sense, but then please add a must_check annotation to this function.
That will point you at the user in the context code which imo deserves a
BUG_ON or similar. I guess we should also convert over the little ggtt
helpers to use this function.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH 11/12] drm/i915: Takedown drm_mm on failed gtt setup

[Ben Widawsky]

This was found by code inspection. If the GTT setup fails then we are
left without properly tearing down the drm_mm.

Hopefully this never happens.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_gem.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 6388706..d85bc3e 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4504,6 +4504,7 @@ int i915_gem_init(struct drm_device *dev)
 	mutex_unlock(&dev->struct_mutex);
 	if (ret) {
 		i915_gem_cleanup_aliasing_ppgtt(dev);
+		drm_mm_takedown(&dev_priv->gtt.base.mm);
 		return ret;
 	}
 
-- 
1.8.4.2

Re: [PATCH 11/12] drm/i915: Takedown drm_mm on failed gtt setup

[Daniel Vetter]

On Mon, Nov 25, 2013 at 09:54:42AM -0800, Ben Widawsky wrote:
> This was found by code inspection. If the GTT setup fails then we are
> left without properly tearing down the drm_mm.
> 
> Hopefully this never happens.
> 
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> ---
>  drivers/gpu/drm/i915/i915_gem.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 6388706..d85bc3e 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -4504,6 +4504,7 @@ int i915_gem_init(struct drm_device *dev)
>  	mutex_unlock(&dev->struct_mutex);
>  	if (ret) {
>  		i915_gem_cleanup_aliasing_ppgtt(dev);
> +		drm_mm_takedown(&dev_priv->gtt.base.mm);

Sprinkling stuff all over the place feels bad, I guess our gem init code
is ripe for an overhaul. I've pulled this in anyway.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH 12/12] drm/i915: Move the gtt mm takedown to cleanup

[Ben Widawsky]

Our VM code already has a cleanup function, and this is a nice place to
put the drm_mm_takedown. This should have no functional impact, it just
leaves the unload function a bit cleaer, and is more logical IMO

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_dma.c     | 1 -
 drivers/gpu/drm/i915/i915_gem_gtt.c | 2 ++
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 25acbb5..5aeb103 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1777,7 +1777,6 @@ int i915_driver_unload(struct drm_device *dev)
 
 	list_del(&dev_priv->gtt.base.global_link);
 	WARN_ON(!list_empty(&dev_priv->vm_list));
-	drm_mm_takedown(&dev_priv->gtt.base.mm);
 
 	drm_vblank_cleanup(dev);
 
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 5ec4ba0..0560337 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -1419,6 +1419,8 @@ static void gen6_gmch_remove(struct i915_address_space *vm)
 {
 
 	struct i915_gtt *gtt = container_of(vm, struct i915_gtt, base);
+
+	drm_mm_takedown(&vm->mm);
 	iounmap(gtt->gsm);
 	teardown_scratch_page(vm->dev);
 }
-- 
1.8.4.2