[PATCH 00/17] i915_gem_gtt.c polish

[PATCH 00/17] i915_gem_gtt.c polish

[Daniel Vetter]

Hi all,

I ended up reading a bit of i915_gem_gtt.c and spotted a few things to clean up
after the dynamic pagetable stuff landed. I haven't done the checkpatch polish
and kerneldoc, Mika/Michel will be doing that, but overall I think the code
looks fairly tidy now. I also untangled the vma binding logic a bit since it's
related, which means we can finally enable the gen7 cmd parser.

Btw my idea is that we'll move the higher level vma related code in
i915_gem_gtt.c out into a new i915_gem_vma.c file, together with the other vma
code sprinkled in various places. But that's probably better to do after the
partial mmap support from Joonas has landed. With that reorg i915_gem_gtt.c
would only concern itself with the low-level pagetable handling.

Survived light testing on my snb here.

Comments&review highly welcome.

Cheers, Daniel

Daniel Vetter (17):
  drm/i915: Move gen8 clear_range vfunc setup into common code
  drm/i915: Move vma vfuns to adddress_space
  drm/i915: Clean up aliasing ppgtt correctly on error paths
  drm/i915: Unify aliasing ppgtt handling
  drm/i915: Move PTE_READ_ONLY to ->pte_encode vfunc
  drm/i915: Dont clear PIN_GLOBAL in the execbuf pinning fallback
  drm/i915: Drop redundant GGTT rebinding
  drm/i915: Don't look at pg_dirty_rings for aliasing ppgtt
  drm/i915: Don't use atomics for pg_dirty_rings
  drm/i915: Remove misleading comment around bind_to_vm
  drm/i915: Fix up the vma aliasing ppgtt binding
  drm/i915: Arm cmd parser with aliasng ppgtt only
  drm/i915: move i915_gem_restore_gtt_mappings around
  drm/i915: Move ppgtt_bind/unbind around
  drm/i915: Unduplicate i915_ggtt_unbind/bind_vma
  drm/i915: Don't try to outsmart gcc in i915_gem_gtt.c
  drm/i915: Move i915_get_ggtt_vma_pages into ggtt_bind_vma

 drivers/gpu/drm/i915/i915_drv.h            |  11 +-
 drivers/gpu/drm/i915/i915_gem.c            |  17 +-
 drivers/gpu/drm/i915/i915_gem_context.c    |  33 +-
 drivers/gpu/drm/i915/i915_gem_execbuffer.c |  23 +-
 drivers/gpu/drm/i915/i915_gem_gtt.c        | 502 +++++++++++------------------
 drivers/gpu/drm/i915/i915_gem_gtt.h        |  18 +-
 6 files changed, 225 insertions(+), 379 deletions(-)

-- 
2.1.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 01/17] drm/i915: Move gen8 clear_range vfunc setup into common code

[Daniel Vetter]

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_gtt.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 9041f3dfdfb4..1c8ef7c143aa 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -994,6 +994,7 @@ static int gen8_ppgtt_init_common(struct i915_hw_ppgtt *ppgtt, uint64_t size)
 	ppgtt->base.total = size;
 	ppgtt->base.cleanup = gen8_ppgtt_cleanup;
 	ppgtt->base.insert_entries = gen8_ppgtt_insert_entries;
+	ppgtt->base.clear_range = gen8_ppgtt_clear_range;
 
 	ppgtt->switch_mm = gen8_mm_switch;
 
@@ -1022,7 +1023,6 @@ static int gen8_aliasing_ppgtt_init(struct i915_hw_ppgtt *ppgtt)
 	}
 
 	ppgtt->base.allocate_va_range = NULL;
-	ppgtt->base.clear_range = gen8_ppgtt_clear_range;
 	ppgtt->base.clear_range(&ppgtt->base, 0, ppgtt->base.total, true);
 
 	return 0;
@@ -1037,7 +1037,6 @@ static int gen8_ppgtt_init(struct i915_hw_ppgtt *ppgtt)
 		return ret;
 
 	ppgtt->base.allocate_va_range = gen8_alloc_va_range;
-	ppgtt->base.clear_range = gen8_ppgtt_clear_range;
 
 	return 0;
 }
-- 
2.1.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 01/17] drm/i915: Move gen8 clear_range vfunc setup into common code

[Mika Kuoppala]

Daniel Vetter <daniel.vetter@ffwll.ch> writes:

> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>

Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com>

> ---
>  drivers/gpu/drm/i915/i915_gem_gtt.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 9041f3dfdfb4..1c8ef7c143aa 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -994,6 +994,7 @@ static int gen8_ppgtt_init_common(struct i915_hw_ppgtt *ppgtt, uint64_t size)
>  	ppgtt->base.total = size;
>  	ppgtt->base.cleanup = gen8_ppgtt_cleanup;
>  	ppgtt->base.insert_entries = gen8_ppgtt_insert_entries;
> +	ppgtt->base.clear_range = gen8_ppgtt_clear_range;
>  
>  	ppgtt->switch_mm = gen8_mm_switch;
>  
> @@ -1022,7 +1023,6 @@ static int gen8_aliasing_ppgtt_init(struct i915_hw_ppgtt *ppgtt)
>  	}
>  
>  	ppgtt->base.allocate_va_range = NULL;
> -	ppgtt->base.clear_range = gen8_ppgtt_clear_range;
>  	ppgtt->base.clear_range(&ppgtt->base, 0, ppgtt->base.total, true);
>  
>  	return 0;
> @@ -1037,7 +1037,6 @@ static int gen8_ppgtt_init(struct i915_hw_ppgtt *ppgtt)
>  		return ret;
>  
>  	ppgtt->base.allocate_va_range = gen8_alloc_va_range;
> -	ppgtt->base.clear_range = gen8_ppgtt_clear_range;
>  
>  	return 0;
>  }
> -- 
> 2.1.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 02/17] drm/i915: Move vma vfuns to adddress_space

[Daniel Vetter]

They change with the address space and not with each vma, so move them
into the right pile of vfuncs. Save 2 pointers per vma and clarifies
the code.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c     |  2 +-
 drivers/gpu/drm/i915/i915_gem_gtt.c | 28 ++++++++++++----------------
 drivers/gpu/drm/i915/i915_gem_gtt.h | 15 +++++++--------
 3 files changed, 20 insertions(+), 25 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 8ce363aa492c..4578772c5509 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3035,7 +3035,7 @@ int i915_vma_unbind(struct i915_vma *vma)
 
 	trace_i915_vma_unbind(vma);
 
-	vma->unbind_vma(vma);
+	vma->vm->unbind_vma(vma);
 
 	list_del_init(&vma->mm_list);
 	if (i915_is_ggtt(vma->vm)) {
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 1c8ef7c143aa..290db48faf27 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -995,6 +995,8 @@ static int gen8_ppgtt_init_common(struct i915_hw_ppgtt *ppgtt, uint64_t size)
 	ppgtt->base.cleanup = gen8_ppgtt_cleanup;
 	ppgtt->base.insert_entries = gen8_ppgtt_insert_entries;
 	ppgtt->base.clear_range = gen8_ppgtt_clear_range;
+	ppgtt->base.unbind_vma = ppgtt_unbind_vma;
+	ppgtt->base.bind_vma = ppgtt_bind_vma;
 
 	ppgtt->switch_mm = gen8_mm_switch;
 
@@ -1579,6 +1581,8 @@ static int gen6_ppgtt_init(struct i915_hw_ppgtt *ppgtt, bool aliasing)
 	ppgtt->base.allocate_va_range = aliasing ? NULL : gen6_alloc_va_range;
 	ppgtt->base.clear_range = gen6_ppgtt_clear_range;
 	ppgtt->base.insert_entries = gen6_ppgtt_insert_entries;
+	ppgtt->base.unbind_vma = ppgtt_unbind_vma;
+	ppgtt->base.bind_vma = ppgtt_bind_vma;
 	ppgtt->base.cleanup = gen6_ppgtt_cleanup;
 	ppgtt->base.start = 0;
 	ppgtt->base.total = I915_PDES * GEN6_PTES * PAGE_SIZE;
@@ -2573,6 +2577,8 @@ static int gen8_gmch_probe(struct drm_device *dev,
 
 	dev_priv->gtt.base.clear_range = gen8_ggtt_clear_range;
 	dev_priv->gtt.base.insert_entries = gen8_ggtt_insert_entries;
+	dev_priv->gtt.base.bind_vma = ggtt_bind_vma;
+	dev_priv->gtt.base.unbind_vma = ggtt_unbind_vma;
 
 	return ret;
 }
@@ -2613,6 +2619,8 @@ static int gen6_gmch_probe(struct drm_device *dev,
 
 	dev_priv->gtt.base.clear_range = gen6_ggtt_clear_range;
 	dev_priv->gtt.base.insert_entries = gen6_ggtt_insert_entries;
+	dev_priv->gtt.base.bind_vma = ggtt_bind_vma;
+	dev_priv->gtt.base.unbind_vma = ggtt_unbind_vma;
 
 	return ret;
 }
@@ -2645,6 +2653,8 @@ static int i915_gmch_probe(struct drm_device *dev,
 
 	dev_priv->gtt.do_idle_maps = needs_idle_maps(dev_priv->dev);
 	dev_priv->gtt.base.clear_range = i915_ggtt_clear_range;
+	dev_priv->gtt.base.bind_vma = i915_ggtt_bind_vma;
+	dev_priv->gtt.base.unbind_vma = i915_ggtt_unbind_vma;
 
 	if (unlikely(dev_priv->gtt.do_idle_maps))
 		DRM_INFO("applying Ironlake quirks for intel_iommu\n");
@@ -2732,22 +2742,8 @@ __i915_gem_vma_create(struct drm_i915_gem_object *obj,
 	vma->vm = vm;
 	vma->obj = obj;
 
-	if (INTEL_INFO(vm->dev)->gen >= 6) {
-		if (i915_is_ggtt(vm)) {
-			vma->ggtt_view = *ggtt_view;
-
-			vma->unbind_vma = ggtt_unbind_vma;
-			vma->bind_vma = ggtt_bind_vma;
-		} else {
-			vma->unbind_vma = ppgtt_unbind_vma;
-			vma->bind_vma = ppgtt_bind_vma;
-		}
-	} else {
-		BUG_ON(!i915_is_ggtt(vm));
+	if (i915_is_ggtt(vm))
 		vma->ggtt_view = *ggtt_view;
-		vma->unbind_vma = i915_ggtt_unbind_vma;
-		vma->bind_vma = i915_ggtt_bind_vma;
-	}
 
 	list_add_tail(&vma->vma_link, &obj->vma_list);
 	if (!i915_is_ggtt(vm))
@@ -2957,7 +2953,7 @@ int i915_vma_bind(struct i915_vma *vma, enum i915_cache_level cache_level,
 			return ret;
 	}
 
-	vma->bind_vma(vma, cache_level, flags);
+	vma->vm->bind_vma(vma, cache_level, flags);
 
 	return 0;
 }
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
index 29de64d1164e..12d0ded0d823 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.h
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
@@ -196,14 +196,6 @@ struct i915_vma {
 	 * bits with absolutely no headroom. So use 4 bits. */
 	unsigned int pin_count:4;
 #define DRM_I915_GEM_OBJECT_MAX_PIN_COUNT 0xf
-
-	/** Unmap an object from an address space. This usually consists of
-	 * setting the valid PTE entries to a reserved scratch page. */
-	void (*unbind_vma)(struct i915_vma *vma);
-	/* Map an object into an address space with the given cache flags. */
-	void (*bind_vma)(struct i915_vma *vma,
-			 enum i915_cache_level cache_level,
-			 u32 flags);
 };
 
 struct i915_page_table {
@@ -281,6 +273,13 @@ struct i915_address_space {
 			       uint64_t start,
 			       enum i915_cache_level cache_level, u32 flags);
 	void (*cleanup)(struct i915_address_space *vm);
+	/** Unmap an object from an address space. This usually consists of
+	 * setting the valid PTE entries to a reserved scratch page. */
+	void (*unbind_vma)(struct i915_vma *vma);
+	/* Map an object into an address space with the given cache flags. */
+	void (*bind_vma)(struct i915_vma *vma,
+			 enum i915_cache_level cache_level,
+			 u32 flags);
 };
 
 /* The Graphics Translation Table is the way in which GEN hardware translates a
-- 
2.1.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 02/17] drm/i915: Move vma vfuns to adddress_space

[Chris Wilson]

On Tue, Apr 14, 2015 at 05:35:12PM +0200, Daniel Vetter wrote:
> They change with the address space and not with each vma, so move them
> into the right pile of vfuncs. Save 2 pointers per vma and clarifies
> the code.

Using per-vma vfunc allows you make, for example, the pagetables
themselves an ordinary vma in the GGTT and so operate identically wrt to
the shrinker and eviction logic - removing some very fragile code in the
process.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 02/17] drm/i915: Move vma vfuns to adddress_space

[Chris Wilson]

On Tue, Apr 14, 2015 at 05:09:27PM +0100, Chris Wilson wrote:
> On Tue, Apr 14, 2015 at 05:35:12PM +0200, Daniel Vetter wrote:
> > They change with the address space and not with each vma, so move them
> > into the right pile of vfuncs. Save 2 pointers per vma and clarifies
> > the code.
> 
> Using per-vma vfunc allows you make, for example, the pagetables
> themselves an ordinary vma in the GGTT and so operate identically wrt to
> the shrinker and eviction logic - removing some very fragile code in the
> process.

A vma->ops would be an interesting compromise.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 02/17] drm/i915: Move vma vfuns to adddress_space

[Daniel Vetter]

On Tue, Apr 14, 2015 at 05:12:30PM +0100, Chris Wilson wrote:
> On Tue, Apr 14, 2015 at 05:09:27PM +0100, Chris Wilson wrote:
> > On Tue, Apr 14, 2015 at 05:35:12PM +0200, Daniel Vetter wrote:
> > > They change with the address space and not with each vma, so move them
> > > into the right pile of vfuncs. Save 2 pointers per vma and clarifies
> > > the code.
> > 
> > Using per-vma vfunc allows you make, for example, the pagetables
> > themselves an ordinary vma in the GGTT and so operate identically wrt to
> > the shrinker and eviction logic - removing some very fragile code in the
> > process.
> 
> A vma->ops would be an interesting compromise.

Tbh still not really sold on this idea, since the complexit tends to be in
the recursion. E.g. see all the fun we had with gpu_idle and the default
context. So for now I still prefer things to be explicit.

Also that would be a new op to first (try) to unuse the dma. If we don't
do this and it fails then the shrinker gets annoyed since the nice
hole-punching doesn't work correctly. There's also the fairness question,
we'd need to make sure that e.g. the hw context gets cycled through the
active list even when we don't switch contexts.

Given all that I don't think this patch here is a blocker for doing other
vma ops.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 02/17] drm/i915: Move vma vfuns to adddress_space

[Chris Wilson]

On Tue, Apr 14, 2015 at 07:08:35PM +0200, Daniel Vetter wrote:
> On Tue, Apr 14, 2015 at 05:12:30PM +0100, Chris Wilson wrote:
> > On Tue, Apr 14, 2015 at 05:09:27PM +0100, Chris Wilson wrote:
> > > On Tue, Apr 14, 2015 at 05:35:12PM +0200, Daniel Vetter wrote:
> > > > They change with the address space and not with each vma, so move them
> > > > into the right pile of vfuncs. Save 2 pointers per vma and clarifies
> > > > the code.
> > > 
> > > Using per-vma vfunc allows you make, for example, the pagetables
> > > themselves an ordinary vma in the GGTT and so operate identically wrt to
> > > the shrinker and eviction logic - removing some very fragile code in the
> > > process.
> > 
> > A vma->ops would be an interesting compromise.
> 
> Tbh still not really sold on this idea, since the complexit tends to be in
> the recursion. E.g. see all the fun we had with gpu_idle and the default
> context. So for now I still prefer things to be explicit.

Ah, you mean the fun that gpu_idle is broken at the moment. I think that
lends weight to my argument tbh.
 
> Also that would be a new op to first (try) to unuse the dma. If we don't
> do this and it fails then the shrinker gets annoyed since the nice
> hole-punching doesn't work correctly. There's also the fairness question,
> we'd need to make sure that e.g. the hw context gets cycled through the
> active list even when we don't switch contexts.

Yes, all that was taken into account by my patches to sanitize request
handling.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 02/17] drm/i915: Move vma vfuns to adddress_space

[Mika Kuoppala]

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

> On Tue, Apr 14, 2015 at 05:35:12PM +0200, Daniel Vetter wrote:
>> They change with the address space and not with each vma, so move them
>> into the right pile of vfuncs. Save 2 pointers per vma and clarifies
>> the code.
>
> Using per-vma vfunc allows you make, for example, the pagetables
> themselves an ordinary vma in the GGTT and so operate identically wrt
> to

Page tables don't need to be in ggtt. Is this comment about
eviction of page directories (for gen < 8)?

Thanks,
-Mika

> the shrinker and eviction logic - removing some very fragile code in the
> process.
> -Chris
>
> -- 
> Chris Wilson, Intel Open Source Technology Centre
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 02/17] drm/i915: Move vma vfuns to adddress_space

[Chris Wilson]

On Thu, Apr 16, 2015 at 09:18:48AM +0300, Mika Kuoppala wrote:
> Chris Wilson <chris@chris-wilson.co.uk> writes:
> 
> > On Tue, Apr 14, 2015 at 05:35:12PM +0200, Daniel Vetter wrote:
> >> They change with the address space and not with each vma, so move them
> >> into the right pile of vfuncs. Save 2 pointers per vma and clarifies
> >> the code.
> >
> > Using per-vma vfunc allows you make, for example, the pagetables
> > themselves an ordinary vma in the GGTT and so operate identically wrt
> > to
> 
> Page tables don't need to be in ggtt. Is this comment about
> eviction of page directories (for gen < 8)?

Yes. The root table is always in ggtt. But we don't even have correct
context eviction yet...
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 02/17] drm/i915: Move vma vfuns to adddress_space

[Mika Kuoppala]

Daniel Vetter <daniel.vetter@ffwll.ch> writes:

> They change with the address space and not with each vma, so move them
> into the right pile of vfuncs. Save 2 pointers per vma and clarifies
> the code.
>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>

Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com>

> ---
>  drivers/gpu/drm/i915/i915_gem.c     |  2 +-
>  drivers/gpu/drm/i915/i915_gem_gtt.c | 28 ++++++++++++----------------
>  drivers/gpu/drm/i915/i915_gem_gtt.h | 15 +++++++--------
>  3 files changed, 20 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 8ce363aa492c..4578772c5509 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -3035,7 +3035,7 @@ int i915_vma_unbind(struct i915_vma *vma)
>  
>  	trace_i915_vma_unbind(vma);
>  
> -	vma->unbind_vma(vma);
> +	vma->vm->unbind_vma(vma);
>  
>  	list_del_init(&vma->mm_list);
>  	if (i915_is_ggtt(vma->vm)) {
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 1c8ef7c143aa..290db48faf27 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -995,6 +995,8 @@ static int gen8_ppgtt_init_common(struct i915_hw_ppgtt *ppgtt, uint64_t size)
>  	ppgtt->base.cleanup = gen8_ppgtt_cleanup;
>  	ppgtt->base.insert_entries = gen8_ppgtt_insert_entries;
>  	ppgtt->base.clear_range = gen8_ppgtt_clear_range;
> +	ppgtt->base.unbind_vma = ppgtt_unbind_vma;
> +	ppgtt->base.bind_vma = ppgtt_bind_vma;
>  
>  	ppgtt->switch_mm = gen8_mm_switch;
>  
> @@ -1579,6 +1581,8 @@ static int gen6_ppgtt_init(struct i915_hw_ppgtt *ppgtt, bool aliasing)
>  	ppgtt->base.allocate_va_range = aliasing ? NULL : gen6_alloc_va_range;
>  	ppgtt->base.clear_range = gen6_ppgtt_clear_range;
>  	ppgtt->base.insert_entries = gen6_ppgtt_insert_entries;
> +	ppgtt->base.unbind_vma = ppgtt_unbind_vma;
> +	ppgtt->base.bind_vma = ppgtt_bind_vma;
>  	ppgtt->base.cleanup = gen6_ppgtt_cleanup;
>  	ppgtt->base.start = 0;
>  	ppgtt->base.total = I915_PDES * GEN6_PTES * PAGE_SIZE;
> @@ -2573,6 +2577,8 @@ static int gen8_gmch_probe(struct drm_device *dev,
>  
>  	dev_priv->gtt.base.clear_range = gen8_ggtt_clear_range;
>  	dev_priv->gtt.base.insert_entries = gen8_ggtt_insert_entries;
> +	dev_priv->gtt.base.bind_vma = ggtt_bind_vma;
> +	dev_priv->gtt.base.unbind_vma = ggtt_unbind_vma;
>  
>  	return ret;
>  }
> @@ -2613,6 +2619,8 @@ static int gen6_gmch_probe(struct drm_device *dev,
>  
>  	dev_priv->gtt.base.clear_range = gen6_ggtt_clear_range;
>  	dev_priv->gtt.base.insert_entries = gen6_ggtt_insert_entries;
> +	dev_priv->gtt.base.bind_vma = ggtt_bind_vma;
> +	dev_priv->gtt.base.unbind_vma = ggtt_unbind_vma;
>  
>  	return ret;
>  }
> @@ -2645,6 +2653,8 @@ static int i915_gmch_probe(struct drm_device *dev,
>  
>  	dev_priv->gtt.do_idle_maps = needs_idle_maps(dev_priv->dev);
>  	dev_priv->gtt.base.clear_range = i915_ggtt_clear_range;
> +	dev_priv->gtt.base.bind_vma = i915_ggtt_bind_vma;
> +	dev_priv->gtt.base.unbind_vma = i915_ggtt_unbind_vma;
>  
>  	if (unlikely(dev_priv->gtt.do_idle_maps))
>  		DRM_INFO("applying Ironlake quirks for intel_iommu\n");
> @@ -2732,22 +2742,8 @@ __i915_gem_vma_create(struct drm_i915_gem_object *obj,
>  	vma->vm = vm;
>  	vma->obj = obj;
>  
> -	if (INTEL_INFO(vm->dev)->gen >= 6) {
> -		if (i915_is_ggtt(vm)) {
> -			vma->ggtt_view = *ggtt_view;
> -
> -			vma->unbind_vma = ggtt_unbind_vma;
> -			vma->bind_vma = ggtt_bind_vma;
> -		} else {
> -			vma->unbind_vma = ppgtt_unbind_vma;
> -			vma->bind_vma = ppgtt_bind_vma;
> -		}
> -	} else {
> -		BUG_ON(!i915_is_ggtt(vm));
> +	if (i915_is_ggtt(vm))
>  		vma->ggtt_view = *ggtt_view;
> -		vma->unbind_vma = i915_ggtt_unbind_vma;
> -		vma->bind_vma = i915_ggtt_bind_vma;
> -	}
>  
>  	list_add_tail(&vma->vma_link, &obj->vma_list);
>  	if (!i915_is_ggtt(vm))
> @@ -2957,7 +2953,7 @@ int i915_vma_bind(struct i915_vma *vma, enum i915_cache_level cache_level,
>  			return ret;
>  	}
>  
> -	vma->bind_vma(vma, cache_level, flags);
> +	vma->vm->bind_vma(vma, cache_level, flags);
>  
>  	return 0;
>  }
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
> index 29de64d1164e..12d0ded0d823 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.h
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
> @@ -196,14 +196,6 @@ struct i915_vma {
>  	 * bits with absolutely no headroom. So use 4 bits. */
>  	unsigned int pin_count:4;
>  #define DRM_I915_GEM_OBJECT_MAX_PIN_COUNT 0xf
> -
> -	/** Unmap an object from an address space. This usually consists of
> -	 * setting the valid PTE entries to a reserved scratch page. */
> -	void (*unbind_vma)(struct i915_vma *vma);
> -	/* Map an object into an address space with the given cache flags. */
> -	void (*bind_vma)(struct i915_vma *vma,
> -			 enum i915_cache_level cache_level,
> -			 u32 flags);
>  };
>  
>  struct i915_page_table {
> @@ -281,6 +273,13 @@ struct i915_address_space {
>  			       uint64_t start,
>  			       enum i915_cache_level cache_level, u32 flags);
>  	void (*cleanup)(struct i915_address_space *vm);
> +	/** Unmap an object from an address space. This usually consists of
> +	 * setting the valid PTE entries to a reserved scratch page. */
> +	void (*unbind_vma)(struct i915_vma *vma);
> +	/* Map an object into an address space with the given cache flags. */
> +	void (*bind_vma)(struct i915_vma *vma,
> +			 enum i915_cache_level cache_level,
> +			 u32 flags);
>  };
>  
>  /* The Graphics Translation Table is the way in which GEN hardware translates a
> -- 
> 2.1.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 03/17] drm/i915: Clean up aliasing ppgtt correctly on error paths

[Daniel Vetter]

While at it inline the free functions - they don't actually free the
ppgtt, just clean up the allocations done for it.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_gem_gtt.c | 30 ++++++++++--------------------
 1 file changed, 10 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 290db48faf27..abb11f139d25 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -672,8 +672,10 @@ static void gen8_free_page_tables(struct i915_page_directory *pd, struct drm_dev
 	}
 }
 
-static void gen8_ppgtt_free(struct i915_hw_ppgtt *ppgtt)
+static void gen8_ppgtt_cleanup(struct i915_address_space *vm)
 {
+	struct i915_hw_ppgtt *ppgtt =
+		container_of(vm, struct i915_hw_ppgtt, base);
 	int i;
 
 	for_each_set_bit(i, ppgtt->pdp.used_pdpes, GEN8_LEGACY_PDPES) {
@@ -688,14 +690,6 @@ static void gen8_ppgtt_free(struct i915_hw_ppgtt *ppgtt)
 	unmap_and_free_pt(ppgtt->scratch_pt, ppgtt->base.dev);
 }
 
-static void gen8_ppgtt_cleanup(struct i915_address_space *vm)
-{
-	struct i915_hw_ppgtt *ppgtt =
-		container_of(vm, struct i915_hw_ppgtt, base);
-
-	gen8_ppgtt_free(ppgtt);
-}
-
 /**
  * gen8_ppgtt_alloc_pagetabs() - Allocate page tables for VA range.
  * @ppgtt:	Master ppgtt structure.
@@ -1454,11 +1448,16 @@ unwind_out:
 	return ret;
 }
 
-static void gen6_ppgtt_free(struct i915_hw_ppgtt *ppgtt)
+static void gen6_ppgtt_cleanup(struct i915_address_space *vm)
 {
+	struct i915_hw_ppgtt *ppgtt =
+		container_of(vm, struct i915_hw_ppgtt, base);
 	struct i915_page_table *pt;
 	uint32_t pde;
 
+
+	drm_mm_remove_node(&ppgtt->node);
+
 	gen6_for_all_pdes(pt, ppgtt, pde) {
 		if (pt != ppgtt->scratch_pt)
 			unmap_and_free_pt(pt, ppgtt->base.dev);
@@ -1468,16 +1467,6 @@ static void gen6_ppgtt_free(struct i915_hw_ppgtt *ppgtt)
 	unmap_and_free_pd(&ppgtt->pd, ppgtt->base.dev);
 }
 
-static void gen6_ppgtt_cleanup(struct i915_address_space *vm)
-{
-	struct i915_hw_ppgtt *ppgtt =
-		container_of(vm, struct i915_hw_ppgtt, base);
-
-	drm_mm_remove_node(&ppgtt->node);
-
-	gen6_ppgtt_free(ppgtt);
-}
-
 static int gen6_ppgtt_allocate_page_directories(struct i915_hw_ppgtt *ppgtt)
 {
 	struct drm_device *dev = ppgtt->base.dev;
@@ -2268,6 +2257,7 @@ static int i915_gem_setup_global_gtt(struct drm_device *dev,
 
 		ret = __hw_ppgtt_init(dev, ppgtt, true);
 		if (ret) {
+			ppgtt->base.cleanup(&ppgtt->base);
 			kfree(ppgtt);
 			return ret;
 		}
-- 
2.1.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 03/17] drm/i915: Clean up aliasing ppgtt correctly on error paths

[Mika Kuoppala]

Daniel Vetter <daniel.vetter@ffwll.ch> writes:

> While at it inline the free functions - they don't actually free the
> ppgtt, just clean up the allocations done for it.
>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com>

> ---
>  drivers/gpu/drm/i915/i915_gem_gtt.c | 30 ++++++++++--------------------
>  1 file changed, 10 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 290db48faf27..abb11f139d25 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -672,8 +672,10 @@ static void gen8_free_page_tables(struct i915_page_directory *pd, struct drm_dev
>  	}
>  }
>  
> -static void gen8_ppgtt_free(struct i915_hw_ppgtt *ppgtt)
> +static void gen8_ppgtt_cleanup(struct i915_address_space *vm)
>  {
> +	struct i915_hw_ppgtt *ppgtt =
> +		container_of(vm, struct i915_hw_ppgtt, base);
>  	int i;
>  
>  	for_each_set_bit(i, ppgtt->pdp.used_pdpes, GEN8_LEGACY_PDPES) {
> @@ -688,14 +690,6 @@ static void gen8_ppgtt_free(struct i915_hw_ppgtt *ppgtt)
>  	unmap_and_free_pt(ppgtt->scratch_pt, ppgtt->base.dev);
>  }
>  
> -static void gen8_ppgtt_cleanup(struct i915_address_space *vm)
> -{
> -	struct i915_hw_ppgtt *ppgtt =
> -		container_of(vm, struct i915_hw_ppgtt, base);
> -
> -	gen8_ppgtt_free(ppgtt);
> -}
> -
>  /**
>   * gen8_ppgtt_alloc_pagetabs() - Allocate page tables for VA range.
>   * @ppgtt:	Master ppgtt structure.
> @@ -1454,11 +1448,16 @@ unwind_out:
>  	return ret;
>  }
>  
> -static void gen6_ppgtt_free(struct i915_hw_ppgtt *ppgtt)
> +static void gen6_ppgtt_cleanup(struct i915_address_space *vm)
>  {
> +	struct i915_hw_ppgtt *ppgtt =
> +		container_of(vm, struct i915_hw_ppgtt, base);
>  	struct i915_page_table *pt;
>  	uint32_t pde;
>  
> +
> +	drm_mm_remove_node(&ppgtt->node);
> +
>  	gen6_for_all_pdes(pt, ppgtt, pde) {
>  		if (pt != ppgtt->scratch_pt)
>  			unmap_and_free_pt(pt, ppgtt->base.dev);
> @@ -1468,16 +1467,6 @@ static void gen6_ppgtt_free(struct i915_hw_ppgtt *ppgtt)
>  	unmap_and_free_pd(&ppgtt->pd, ppgtt->base.dev);
>  }
>  
> -static void gen6_ppgtt_cleanup(struct i915_address_space *vm)
> -{
> -	struct i915_hw_ppgtt *ppgtt =
> -		container_of(vm, struct i915_hw_ppgtt, base);
> -
> -	drm_mm_remove_node(&ppgtt->node);
> -
> -	gen6_ppgtt_free(ppgtt);
> -}
> -
>  static int gen6_ppgtt_allocate_page_directories(struct i915_hw_ppgtt *ppgtt)
>  {
>  	struct drm_device *dev = ppgtt->base.dev;
> @@ -2268,6 +2257,7 @@ static int i915_gem_setup_global_gtt(struct drm_device *dev,
>  
>  		ret = __hw_ppgtt_init(dev, ppgtt, true);
>  		if (ret) {
> +			ppgtt->base.cleanup(&ppgtt->base);
>  			kfree(ppgtt);
>  			return ret;
>  		}
> -- 
> 2.1.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 04/17] drm/i915: Unify aliasing ppgtt handling

[Daniel Vetter]

With the dynamic pagetable alloc code aliasing ppgtt special-cases
where again mixed in all over the place with the low-level init code.

Extract the va preallocation and clearing again into the common code
where aliasing ppgtt gets set up.

Note that with this we don't set the size of the aliasing ppgtt to the
size of the parent ggtt address space. Which isn't required at all
since except for the ppgtt setup/cleanup code no one ever looks at
this.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_gtt.c | 134 +++++++-----------------------------
 1 file changed, 24 insertions(+), 110 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index abb11f139d25..75787f1d2751 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -387,50 +387,6 @@ fail_bitmap:
 	return ERR_PTR(ret);
 }
 
-/**
- * alloc_pt_range() - Allocate a multiple page tables
- * @pd:		The page directory which will have at least @count entries
- *		available to point to the allocated page tables.
- * @pde:	First page directory entry for which we are allocating.
- * @count:	Number of pages to allocate.
- * @dev:	DRM device.
- *
- * Allocates multiple page table pages and sets the appropriate entries in the
- * page table structure within the page directory. Function cleans up after
- * itself on any failures.
- *
- * Return: 0 if allocation succeeded.
- */
-static int alloc_pt_range(struct i915_page_directory *pd, uint16_t pde, size_t count,
-			  struct drm_device *dev)
-{
-	int i, ret;
-
-	/* 512 is the max page tables per page_directory on any platform. */
-	if (WARN_ON(pde + count > I915_PDES))
-		return -EINVAL;
-
-	for (i = pde; i < pde + count; i++) {
-		struct i915_page_table *pt = alloc_pt_single(dev);
-
-		if (IS_ERR(pt)) {
-			ret = PTR_ERR(pt);
-			goto err_out;
-		}
-		WARN(pd->page_table[i],
-		     "Leaking page directory entry %d (%p)\n",
-		     i, pd->page_table[i]);
-		pd->page_table[i] = pt;
-	}
-
-	return 0;
-
-err_out:
-	while (i-- > pde)
-		unmap_and_free_pt(pd->page_table[i], dev);
-	return ret;
-}
-
 static void unmap_and_free_pd(struct i915_page_directory *pd,
 			      struct drm_device *dev)
 {
@@ -971,7 +927,7 @@ err_out:
  * space.
  *
  */
-static int gen8_ppgtt_init_common(struct i915_hw_ppgtt *ppgtt, uint64_t size)
+static int gen8_ppgtt_init(struct i915_hw_ppgtt *ppgtt)
 {
 	ppgtt->scratch_pt = alloc_pt_single(ppgtt->base.dev);
 	if (IS_ERR(ppgtt->scratch_pt))
@@ -985,8 +941,9 @@ static int gen8_ppgtt_init_common(struct i915_hw_ppgtt *ppgtt, uint64_t size)
 	gen8_initialize_pd(&ppgtt->base, ppgtt->scratch_pd);
 
 	ppgtt->base.start = 0;
-	ppgtt->base.total = size;
+	ppgtt->base.total = 1ULL << 32;
 	ppgtt->base.cleanup = gen8_ppgtt_cleanup;
+	ppgtt->base.allocate_va_range = gen8_alloc_va_range;
 	ppgtt->base.insert_entries = gen8_ppgtt_insert_entries;
 	ppgtt->base.clear_range = gen8_ppgtt_clear_range;
 	ppgtt->base.unbind_vma = ppgtt_unbind_vma;
@@ -997,46 +954,6 @@ static int gen8_ppgtt_init_common(struct i915_hw_ppgtt *ppgtt, uint64_t size)
 	return 0;
 }
 
-static int gen8_aliasing_ppgtt_init(struct i915_hw_ppgtt *ppgtt)
-{
-	struct drm_device *dev = ppgtt->base.dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	uint64_t start = 0, size = dev_priv->gtt.base.total;
-	int ret;
-
-	ret = gen8_ppgtt_init_common(ppgtt, dev_priv->gtt.base.total);
-	if (ret)
-		return ret;
-
-	/* Aliasing PPGTT has to always work and be mapped because of the way we
-	 * use RESTORE_INHIBIT in the context switch. This will be fixed
-	 * eventually. */
-	ret = gen8_alloc_va_range(&ppgtt->base, start, size);
-	if (ret) {
-		unmap_and_free_pd(ppgtt->scratch_pd, ppgtt->base.dev);
-		unmap_and_free_pt(ppgtt->scratch_pt, ppgtt->base.dev);
-		return ret;
-	}
-
-	ppgtt->base.allocate_va_range = NULL;
-	ppgtt->base.clear_range(&ppgtt->base, 0, ppgtt->base.total, true);
-
-	return 0;
-}
-
-static int gen8_ppgtt_init(struct i915_hw_ppgtt *ppgtt)
-{
-	int ret;
-
-	ret = gen8_ppgtt_init_common(ppgtt, (1ULL << 32));
-	if (ret)
-		return ret;
-
-	ppgtt->base.allocate_va_range = gen8_alloc_va_range;
-
-	return 0;
-}
-
 static void gen6_dump_ppgtt(struct i915_hw_ppgtt *ppgtt, struct seq_file *m)
 {
 	struct i915_address_space *vm = &ppgtt->base;
@@ -1533,7 +1450,7 @@ static void gen6_scratch_va_range(struct i915_hw_ppgtt *ppgtt,
 		ppgtt->pd.page_table[pde] = ppgtt->scratch_pt;
 }
 
-static int gen6_ppgtt_init(struct i915_hw_ppgtt *ppgtt, bool aliasing)
+static int gen6_ppgtt_init(struct i915_hw_ppgtt *ppgtt)
 {
 	struct drm_device *dev = ppgtt->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -1556,18 +1473,7 @@ static int gen6_ppgtt_init(struct i915_hw_ppgtt *ppgtt, bool aliasing)
 	if (ret)
 		return ret;
 
-	if (aliasing) {
-		/* preallocate all pts */
-		ret = alloc_pt_range(&ppgtt->pd, 0, I915_PDES,
-				ppgtt->base.dev);
-
-		if (ret) {
-			gen6_ppgtt_cleanup(&ppgtt->base);
-			return ret;
-		}
-	}
-
-	ppgtt->base.allocate_va_range = aliasing ? NULL : gen6_alloc_va_range;
+	ppgtt->base.allocate_va_range = gen6_alloc_va_range;
 	ppgtt->base.clear_range = gen6_ppgtt_clear_range;
 	ppgtt->base.insert_entries = gen6_ppgtt_insert_entries;
 	ppgtt->base.unbind_vma = ppgtt_unbind_vma;
@@ -1583,10 +1489,7 @@ static int gen6_ppgtt_init(struct i915_hw_ppgtt *ppgtt, bool aliasing)
 	ppgtt->pd_addr = (gen6_pte_t __iomem *)dev_priv->gtt.gsm +
 		ppgtt->pd.pd_offset / sizeof(gen6_pte_t);
 
-	if (aliasing)
-		ppgtt->base.clear_range(&ppgtt->base, 0, ppgtt->base.total, true);
-	else
-		gen6_scratch_va_range(ppgtt, 0, ppgtt->base.total);
+	gen6_scratch_va_range(ppgtt, 0, ppgtt->base.total);
 
 	gen6_write_page_range(dev_priv, &ppgtt->pd, 0, ppgtt->base.total);
 
@@ -1600,8 +1503,7 @@ static int gen6_ppgtt_init(struct i915_hw_ppgtt *ppgtt, bool aliasing)
 	return 0;
 }
 
-static int __hw_ppgtt_init(struct drm_device *dev, struct i915_hw_ppgtt *ppgtt,
-		bool aliasing)
+static int __hw_ppgtt_init(struct drm_device *dev, struct i915_hw_ppgtt *ppgtt)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
@@ -1609,9 +1511,7 @@ static int __hw_ppgtt_init(struct drm_device *dev, struct i915_hw_ppgtt *ppgtt,
 	ppgtt->base.scratch = dev_priv->gtt.base.scratch;
 
 	if (INTEL_INFO(dev)->gen < 8)
-		return gen6_ppgtt_init(ppgtt, aliasing);
-	else if (aliasing)
-		return gen8_aliasing_ppgtt_init(ppgtt);
+		return gen6_ppgtt_init(ppgtt);
 	else
 		return gen8_ppgtt_init(ppgtt);
 }
@@ -1620,7 +1520,7 @@ int i915_ppgtt_init(struct drm_device *dev, struct i915_hw_ppgtt *ppgtt)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	int ret = 0;
 
-	ret = __hw_ppgtt_init(dev, ppgtt, false);
+	ret = __hw_ppgtt_init(dev, ppgtt);
 	if (ret == 0) {
 		kref_init(&ppgtt->ref);
 		drm_mm_init(&ppgtt->base.mm, ppgtt->base.start,
@@ -2255,13 +2155,27 @@ static int i915_gem_setup_global_gtt(struct drm_device *dev,
 		if (!ppgtt)
 			return -ENOMEM;
 
-		ret = __hw_ppgtt_init(dev, ppgtt, true);
+		ret = __hw_ppgtt_init(dev, ppgtt);
+		if (ret) {
+			ppgtt->base.cleanup(&ppgtt->base);
+			kfree(ppgtt);
+			return ret;
+		}
+
+		if (ppgtt->base.allocate_va_range)
+			ret = ppgtt->base.allocate_va_range(&ppgtt->base, 0,
+							    ppgtt->base.total);
 		if (ret) {
 			ppgtt->base.cleanup(&ppgtt->base);
 			kfree(ppgtt);
 			return ret;
 		}
 
+		ppgtt->base.clear_range(&ppgtt->base,
+					ppgtt->base.start,
+					ppgtt->base.total,
+					true);
+
 		dev_priv->mm.aliasing_ppgtt = ppgtt;
 	}
 
-- 
2.1.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 04/17] drm/i915: Unify aliasing ppgtt handling

[Mika Kuoppala]

Daniel Vetter <daniel.vetter@ffwll.ch> writes:

> With the dynamic pagetable alloc code aliasing ppgtt special-cases
> where again mixed in all over the place with the low-level init code.
>
> Extract the va preallocation and clearing again into the common code
> where aliasing ppgtt gets set up.
>
> Note that with this we don't set the size of the aliasing ppgtt to the
> size of the parent ggtt address space. Which isn't required at all
> since except for the ppgtt setup/cleanup code no one ever looks at
> this.
>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem_gtt.c | 134 +++++++-----------------------------
>  1 file changed, 24 insertions(+), 110 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index abb11f139d25..75787f1d2751 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -387,50 +387,6 @@ fail_bitmap:
>  	return ERR_PTR(ret);
>  }
>  
> -/**
> - * alloc_pt_range() - Allocate a multiple page tables
> - * @pd:		The page directory which will have at least @count entries
> - *		available to point to the allocated page tables.
> - * @pde:	First page directory entry for which we are allocating.
> - * @count:	Number of pages to allocate.
> - * @dev:	DRM device.
> - *
> - * Allocates multiple page table pages and sets the appropriate entries in the
> - * page table structure within the page directory. Function cleans up after
> - * itself on any failures.
> - *
> - * Return: 0 if allocation succeeded.
> - */
> -static int alloc_pt_range(struct i915_page_directory *pd, uint16_t pde, size_t count,
> -			  struct drm_device *dev)
> -{
> -	int i, ret;
> -
> -	/* 512 is the max page tables per page_directory on any platform. */
> -	if (WARN_ON(pde + count > I915_PDES))
> -		return -EINVAL;
> -
> -	for (i = pde; i < pde + count; i++) {
> -		struct i915_page_table *pt = alloc_pt_single(dev);
> -
> -		if (IS_ERR(pt)) {
> -			ret = PTR_ERR(pt);
> -			goto err_out;
> -		}
> -		WARN(pd->page_table[i],
> -		     "Leaking page directory entry %d (%p)\n",
> -		     i, pd->page_table[i]);
> -		pd->page_table[i] = pt;
> -	}
> -
> -	return 0;
> -
> -err_out:
> -	while (i-- > pde)
> -		unmap_and_free_pt(pd->page_table[i], dev);
> -	return ret;
> -}
> -
>  static void unmap_and_free_pd(struct i915_page_directory *pd,
>  			      struct drm_device *dev)
>  {
> @@ -971,7 +927,7 @@ err_out:
>   * space.
>   *
>   */
> -static int gen8_ppgtt_init_common(struct i915_hw_ppgtt *ppgtt, uint64_t size)
> +static int gen8_ppgtt_init(struct i915_hw_ppgtt *ppgtt)
>  {
>  	ppgtt->scratch_pt = alloc_pt_single(ppgtt->base.dev);
>  	if (IS_ERR(ppgtt->scratch_pt))
> @@ -985,8 +941,9 @@ static int gen8_ppgtt_init_common(struct i915_hw_ppgtt *ppgtt, uint64_t size)
>  	gen8_initialize_pd(&ppgtt->base, ppgtt->scratch_pd);
>  
>  	ppgtt->base.start = 0;
> -	ppgtt->base.total = size;
> +	ppgtt->base.total = 1ULL << 32;

This warns on 32bit builds due to overflow.

Seems that I have already tripped on this when I reviewed 4gb
default ppgtt size patch.

We should do a follow-up patch with:

--- a/drivers/gpu/drm/i915/i915_gem_gtt.h
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
@@ -229,7 +229,7 @@ struct i915_address_space {
        struct drm_device *dev;
        struct list_head global_link;
        unsigned long start;            /* Start offset always 0 for
        dri2 */
-       size_t total;           /* size addr space maps (ex. 2GB for ggtt) */
+       u64 size;               /* size addr space maps (ex. 2GB for ggtt) */

-Mika


>  	ppgtt->base.cleanup = gen8_ppgtt_cleanup;
> +	ppgtt->base.allocate_va_range = gen8_alloc_va_range;
>  	ppgtt->base.insert_entries = gen8_ppgtt_insert_entries;
>  	ppgtt->base.clear_range = gen8_ppgtt_clear_range;
>  	ppgtt->base.unbind_vma = ppgtt_unbind_vma;
> @@ -997,46 +954,6 @@ static int gen8_ppgtt_init_common(struct i915_hw_ppgtt *ppgtt, uint64_t size)
>  	return 0;
>  }
>  
> -static int gen8_aliasing_ppgtt_init(struct i915_hw_ppgtt *ppgtt)
> -{
> -	struct drm_device *dev = ppgtt->base.dev;
> -	struct drm_i915_private *dev_priv = dev->dev_private;
> -	uint64_t start = 0, size = dev_priv->gtt.base.total;
> -	int ret;
> -
> -	ret = gen8_ppgtt_init_common(ppgtt, dev_priv->gtt.base.total);
> -	if (ret)
> -		return ret;
> -
> -	/* Aliasing PPGTT has to always work and be mapped because of the way we
> -	 * use RESTORE_INHIBIT in the context switch. This will be fixed
> -	 * eventually. */
> -	ret = gen8_alloc_va_range(&ppgtt->base, start, size);
> -	if (ret) {
> -		unmap_and_free_pd(ppgtt->scratch_pd, ppgtt->base.dev);
> -		unmap_and_free_pt(ppgtt->scratch_pt, ppgtt->base.dev);
> -		return ret;
> -	}
> -
> -	ppgtt->base.allocate_va_range = NULL;
> -	ppgtt->base.clear_range(&ppgtt->base, 0, ppgtt->base.total, true);
> -
> -	return 0;
> -}
> -
> -static int gen8_ppgtt_init(struct i915_hw_ppgtt *ppgtt)
> -{
> -	int ret;
> -
> -	ret = gen8_ppgtt_init_common(ppgtt, (1ULL << 32));
> -	if (ret)
> -		return ret;
> -
> -	ppgtt->base.allocate_va_range = gen8_alloc_va_range;
> -
> -	return 0;
> -}
> -
>  static void gen6_dump_ppgtt(struct i915_hw_ppgtt *ppgtt, struct seq_file *m)
>  {
>  	struct i915_address_space *vm = &ppgtt->base;
> @@ -1533,7 +1450,7 @@ static void gen6_scratch_va_range(struct i915_hw_ppgtt *ppgtt,
>  		ppgtt->pd.page_table[pde] = ppgtt->scratch_pt;
>  }
>  
> -static int gen6_ppgtt_init(struct i915_hw_ppgtt *ppgtt, bool aliasing)
> +static int gen6_ppgtt_init(struct i915_hw_ppgtt *ppgtt)
>  {
>  	struct drm_device *dev = ppgtt->base.dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -1556,18 +1473,7 @@ static int gen6_ppgtt_init(struct i915_hw_ppgtt *ppgtt, bool aliasing)
>  	if (ret)
>  		return ret;
>  
> -	if (aliasing) {
> -		/* preallocate all pts */
> -		ret = alloc_pt_range(&ppgtt->pd, 0, I915_PDES,
> -				ppgtt->base.dev);
> -
> -		if (ret) {
> -			gen6_ppgtt_cleanup(&ppgtt->base);
> -			return ret;
> -		}
> -	}
> -
> -	ppgtt->base.allocate_va_range = aliasing ? NULL : gen6_alloc_va_range;
> +	ppgtt->base.allocate_va_range = gen6_alloc_va_range;
>  	ppgtt->base.clear_range = gen6_ppgtt_clear_range;
>  	ppgtt->base.insert_entries = gen6_ppgtt_insert_entries;
>  	ppgtt->base.unbind_vma = ppgtt_unbind_vma;
> @@ -1583,10 +1489,7 @@ static int gen6_ppgtt_init(struct i915_hw_ppgtt *ppgtt, bool aliasing)
>  	ppgtt->pd_addr = (gen6_pte_t __iomem *)dev_priv->gtt.gsm +
>  		ppgtt->pd.pd_offset / sizeof(gen6_pte_t);
>  
> -	if (aliasing)
> -		ppgtt->base.clear_range(&ppgtt->base, 0, ppgtt->base.total, true);
> -	else
> -		gen6_scratch_va_range(ppgtt, 0, ppgtt->base.total);
> +	gen6_scratch_va_range(ppgtt, 0, ppgtt->base.total);
>  
>  	gen6_write_page_range(dev_priv, &ppgtt->pd, 0, ppgtt->base.total);
>  
> @@ -1600,8 +1503,7 @@ static int gen6_ppgtt_init(struct i915_hw_ppgtt *ppgtt, bool aliasing)
>  	return 0;
>  }
>  
> -static int __hw_ppgtt_init(struct drm_device *dev, struct i915_hw_ppgtt *ppgtt,
> -		bool aliasing)
> +static int __hw_ppgtt_init(struct drm_device *dev, struct i915_hw_ppgtt *ppgtt)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  
> @@ -1609,9 +1511,7 @@ static int __hw_ppgtt_init(struct drm_device *dev, struct i915_hw_ppgtt *ppgtt,
>  	ppgtt->base.scratch = dev_priv->gtt.base.scratch;
>  
>  	if (INTEL_INFO(dev)->gen < 8)
> -		return gen6_ppgtt_init(ppgtt, aliasing);
> -	else if (aliasing)
> -		return gen8_aliasing_ppgtt_init(ppgtt);
> +		return gen6_ppgtt_init(ppgtt);
>  	else
>  		return gen8_ppgtt_init(ppgtt);
>  }
> @@ -1620,7 +1520,7 @@ int i915_ppgtt_init(struct drm_device *dev, struct i915_hw_ppgtt *ppgtt)
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	int ret = 0;
>  
> -	ret = __hw_ppgtt_init(dev, ppgtt, false);
> +	ret = __hw_ppgtt_init(dev, ppgtt);
>  	if (ret == 0) {
>  		kref_init(&ppgtt->ref);
>  		drm_mm_init(&ppgtt->base.mm, ppgtt->base.start,
> @@ -2255,13 +2155,27 @@ static int i915_gem_setup_global_gtt(struct drm_device *dev,
>  		if (!ppgtt)
>  			return -ENOMEM;
>  
> -		ret = __hw_ppgtt_init(dev, ppgtt, true);
> +		ret = __hw_ppgtt_init(dev, ppgtt);
> +		if (ret) {
> +			ppgtt->base.cleanup(&ppgtt->base);
> +			kfree(ppgtt);
> +			return ret;
> +		}
> +
> +		if (ppgtt->base.allocate_va_range)
> +			ret = ppgtt->base.allocate_va_range(&ppgtt->base, 0,
> +							    ppgtt->base.total);
>  		if (ret) {
>  			ppgtt->base.cleanup(&ppgtt->base);
>  			kfree(ppgtt);
>  			return ret;
>  		}
>  
> +		ppgtt->base.clear_range(&ppgtt->base,
> +					ppgtt->base.start,
> +					ppgtt->base.total,
> +					true);
> +
>  		dev_priv->mm.aliasing_ppgtt = ppgtt;
>  	}
>  
> -- 
> 2.1.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 04/17] drm/i915: Unify aliasing ppgtt handling

[Mika Kuoppala]

Daniel Vetter <daniel.vetter@ffwll.ch> writes:

> With the dynamic pagetable alloc code aliasing ppgtt special-cases
> where again mixed in all over the place with the low-level init code.
>
> Extract the va preallocation and clearing again into the common code
> where aliasing ppgtt gets set up.
>
> Note that with this we don't set the size of the aliasing ppgtt to the
> size of the parent ggtt address space. Which isn't required at all
> since except for the ppgtt setup/cleanup code no one ever looks at
> this.
>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>

Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com>

> ---
>  drivers/gpu/drm/i915/i915_gem_gtt.c | 134 +++++++-----------------------------
>  1 file changed, 24 insertions(+), 110 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index abb11f139d25..75787f1d2751 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -387,50 +387,6 @@ fail_bitmap:
>  	return ERR_PTR(ret);
>  }
>  
> -/**
> - * alloc_pt_range() - Allocate a multiple page tables
> - * @pd:		The page directory which will have at least @count entries
> - *		available to point to the allocated page tables.
> - * @pde:	First page directory entry for which we are allocating.
> - * @count:	Number of pages to allocate.
> - * @dev:	DRM device.
> - *
> - * Allocates multiple page table pages and sets the appropriate entries in the
> - * page table structure within the page directory. Function cleans up after
> - * itself on any failures.
> - *
> - * Return: 0 if allocation succeeded.
> - */
> -static int alloc_pt_range(struct i915_page_directory *pd, uint16_t pde, size_t count,
> -			  struct drm_device *dev)
> -{
> -	int i, ret;
> -
> -	/* 512 is the max page tables per page_directory on any platform. */
> -	if (WARN_ON(pde + count > I915_PDES))
> -		return -EINVAL;
> -
> -	for (i = pde; i < pde + count; i++) {
> -		struct i915_page_table *pt = alloc_pt_single(dev);
> -
> -		if (IS_ERR(pt)) {
> -			ret = PTR_ERR(pt);
> -			goto err_out;
> -		}
> -		WARN(pd->page_table[i],
> -		     "Leaking page directory entry %d (%p)\n",
> -		     i, pd->page_table[i]);
> -		pd->page_table[i] = pt;
> -	}
> -
> -	return 0;
> -
> -err_out:
> -	while (i-- > pde)
> -		unmap_and_free_pt(pd->page_table[i], dev);
> -	return ret;
> -}
> -
>  static void unmap_and_free_pd(struct i915_page_directory *pd,
>  			      struct drm_device *dev)
>  {
> @@ -971,7 +927,7 @@ err_out:
>   * space.
>   *
>   */
> -static int gen8_ppgtt_init_common(struct i915_hw_ppgtt *ppgtt, uint64_t size)
> +static int gen8_ppgtt_init(struct i915_hw_ppgtt *ppgtt)
>  {
>  	ppgtt->scratch_pt = alloc_pt_single(ppgtt->base.dev);
>  	if (IS_ERR(ppgtt->scratch_pt))
> @@ -985,8 +941,9 @@ static int gen8_ppgtt_init_common(struct i915_hw_ppgtt *ppgtt, uint64_t size)
>  	gen8_initialize_pd(&ppgtt->base, ppgtt->scratch_pd);
>  
>  	ppgtt->base.start = 0;
> -	ppgtt->base.total = size;
> +	ppgtt->base.total = 1ULL << 32;
>  	ppgtt->base.cleanup = gen8_ppgtt_cleanup;
> +	ppgtt->base.allocate_va_range = gen8_alloc_va_range;
>  	ppgtt->base.insert_entries = gen8_ppgtt_insert_entries;
>  	ppgtt->base.clear_range = gen8_ppgtt_clear_range;
>  	ppgtt->base.unbind_vma = ppgtt_unbind_vma;
> @@ -997,46 +954,6 @@ static int gen8_ppgtt_init_common(struct i915_hw_ppgtt *ppgtt, uint64_t size)
>  	return 0;
>  }
>  
> -static int gen8_aliasing_ppgtt_init(struct i915_hw_ppgtt *ppgtt)
> -{
> -	struct drm_device *dev = ppgtt->base.dev;
> -	struct drm_i915_private *dev_priv = dev->dev_private;
> -	uint64_t start = 0, size = dev_priv->gtt.base.total;
> -	int ret;
> -
> -	ret = gen8_ppgtt_init_common(ppgtt, dev_priv->gtt.base.total);
> -	if (ret)
> -		return ret;
> -
> -	/* Aliasing PPGTT has to always work and be mapped because of the way we
> -	 * use RESTORE_INHIBIT in the context switch. This will be fixed
> -	 * eventually. */
> -	ret = gen8_alloc_va_range(&ppgtt->base, start, size);
> -	if (ret) {
> -		unmap_and_free_pd(ppgtt->scratch_pd, ppgtt->base.dev);
> -		unmap_and_free_pt(ppgtt->scratch_pt, ppgtt->base.dev);
> -		return ret;
> -	}
> -
> -	ppgtt->base.allocate_va_range = NULL;
> -	ppgtt->base.clear_range(&ppgtt->base, 0, ppgtt->base.total, true);
> -
> -	return 0;
> -}
> -
> -static int gen8_ppgtt_init(struct i915_hw_ppgtt *ppgtt)
> -{
> -	int ret;
> -
> -	ret = gen8_ppgtt_init_common(ppgtt, (1ULL << 32));
> -	if (ret)
> -		return ret;
> -
> -	ppgtt->base.allocate_va_range = gen8_alloc_va_range;
> -
> -	return 0;
> -}
> -
>  static void gen6_dump_ppgtt(struct i915_hw_ppgtt *ppgtt, struct seq_file *m)
>  {
>  	struct i915_address_space *vm = &ppgtt->base;
> @@ -1533,7 +1450,7 @@ static void gen6_scratch_va_range(struct i915_hw_ppgtt *ppgtt,
>  		ppgtt->pd.page_table[pde] = ppgtt->scratch_pt;
>  }
>  
> -static int gen6_ppgtt_init(struct i915_hw_ppgtt *ppgtt, bool aliasing)
> +static int gen6_ppgtt_init(struct i915_hw_ppgtt *ppgtt)
>  {
>  	struct drm_device *dev = ppgtt->base.dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -1556,18 +1473,7 @@ static int gen6_ppgtt_init(struct i915_hw_ppgtt *ppgtt, bool aliasing)
>  	if (ret)
>  		return ret;
>  
> -	if (aliasing) {
> -		/* preallocate all pts */
> -		ret = alloc_pt_range(&ppgtt->pd, 0, I915_PDES,
> -				ppgtt->base.dev);
> -
> -		if (ret) {
> -			gen6_ppgtt_cleanup(&ppgtt->base);
> -			return ret;
> -		}
> -	}
> -
> -	ppgtt->base.allocate_va_range = aliasing ? NULL : gen6_alloc_va_range;
> +	ppgtt->base.allocate_va_range = gen6_alloc_va_range;
>  	ppgtt->base.clear_range = gen6_ppgtt_clear_range;
>  	ppgtt->base.insert_entries = gen6_ppgtt_insert_entries;
>  	ppgtt->base.unbind_vma = ppgtt_unbind_vma;
> @@ -1583,10 +1489,7 @@ static int gen6_ppgtt_init(struct i915_hw_ppgtt *ppgtt, bool aliasing)
>  	ppgtt->pd_addr = (gen6_pte_t __iomem *)dev_priv->gtt.gsm +
>  		ppgtt->pd.pd_offset / sizeof(gen6_pte_t);
>  
> -	if (aliasing)
> -		ppgtt->base.clear_range(&ppgtt->base, 0, ppgtt->base.total, true);
> -	else
> -		gen6_scratch_va_range(ppgtt, 0, ppgtt->base.total);
> +	gen6_scratch_va_range(ppgtt, 0, ppgtt->base.total);
>  
>  	gen6_write_page_range(dev_priv, &ppgtt->pd, 0, ppgtt->base.total);
>  
> @@ -1600,8 +1503,7 @@ static int gen6_ppgtt_init(struct i915_hw_ppgtt *ppgtt, bool aliasing)
>  	return 0;
>  }
>  
> -static int __hw_ppgtt_init(struct drm_device *dev, struct i915_hw_ppgtt *ppgtt,
> -		bool aliasing)
> +static int __hw_ppgtt_init(struct drm_device *dev, struct i915_hw_ppgtt *ppgtt)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  
> @@ -1609,9 +1511,7 @@ static int __hw_ppgtt_init(struct drm_device *dev, struct i915_hw_ppgtt *ppgtt,
>  	ppgtt->base.scratch = dev_priv->gtt.base.scratch;
>  
>  	if (INTEL_INFO(dev)->gen < 8)
> -		return gen6_ppgtt_init(ppgtt, aliasing);
> -	else if (aliasing)
> -		return gen8_aliasing_ppgtt_init(ppgtt);
> +		return gen6_ppgtt_init(ppgtt);
>  	else
>  		return gen8_ppgtt_init(ppgtt);
>  }
> @@ -1620,7 +1520,7 @@ int i915_ppgtt_init(struct drm_device *dev, struct i915_hw_ppgtt *ppgtt)
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	int ret = 0;
>  
> -	ret = __hw_ppgtt_init(dev, ppgtt, false);
> +	ret = __hw_ppgtt_init(dev, ppgtt);
>  	if (ret == 0) {
>  		kref_init(&ppgtt->ref);
>  		drm_mm_init(&ppgtt->base.mm, ppgtt->base.start,
> @@ -2255,13 +2155,27 @@ static int i915_gem_setup_global_gtt(struct drm_device *dev,
>  		if (!ppgtt)
>  			return -ENOMEM;
>  
> -		ret = __hw_ppgtt_init(dev, ppgtt, true);
> +		ret = __hw_ppgtt_init(dev, ppgtt);
> +		if (ret) {
> +			ppgtt->base.cleanup(&ppgtt->base);
> +			kfree(ppgtt);
> +			return ret;
> +		}
> +
> +		if (ppgtt->base.allocate_va_range)
> +			ret = ppgtt->base.allocate_va_range(&ppgtt->base, 0,
> +							    ppgtt->base.total);
>  		if (ret) {
>  			ppgtt->base.cleanup(&ppgtt->base);
>  			kfree(ppgtt);
>  			return ret;
>  		}
>  
> +		ppgtt->base.clear_range(&ppgtt->base,
> +					ppgtt->base.start,
> +					ppgtt->base.total,
> +					true);
> +
>  		dev_priv->mm.aliasing_ppgtt = ppgtt;
>  	}
>  
> -- 
> 2.1.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 05/17] drm/i915: Move PTE_READ_ONLY to ->pte_encode vfunc

[Daniel Vetter]

It's only used as a flag there, so unconfuse things a bit.
Also separate the bind_vma flag space from the pte_encode flag
space in the code.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_gem_gtt.c | 15 +++++++++------
 drivers/gpu/drm/i915/i915_gem_gtt.h |  3 ++-
 2 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 75787f1d2751..4e2caef83772 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -1610,14 +1610,16 @@ void  i915_ppgtt_release(struct kref *kref)
 static void
 ppgtt_bind_vma(struct i915_vma *vma,
 	       enum i915_cache_level cache_level,
-	       u32 flags)
+	       u32 unused)
 {
+	u32 pte_flags = 0;
+
 	/* Currently applicable only to VLV */
 	if (vma->obj->gt_ro)
-		flags |= PTE_READ_ONLY;
+		pte_flags |= PTE_READ_ONLY;
 
 	vma->vm->insert_entries(vma->vm, vma->obj->pages, vma->node.start,
-				cache_level, flags);
+				cache_level, pte_flags);
 }
 
 static void ppgtt_unbind_vma(struct i915_vma *vma)
@@ -1986,10 +1988,11 @@ static void ggtt_bind_vma(struct i915_vma *vma,
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_i915_gem_object *obj = vma->obj;
 	struct sg_table *pages = obj->pages;
+	u32 pte_flags = 0;
 
 	/* Currently applicable only to VLV */
 	if (obj->gt_ro)
-		flags |= PTE_READ_ONLY;
+		pte_flags |= PTE_READ_ONLY;
 
 	if (i915_is_ggtt(vma->vm))
 		pages = vma->ggtt_view.pages;
@@ -2010,7 +2013,7 @@ static void ggtt_bind_vma(struct i915_vma *vma,
 		    (cache_level != obj->cache_level)) {
 			vma->vm->insert_entries(vma->vm, pages,
 						vma->node.start,
-						cache_level, flags);
+						cache_level, pte_flags);
 			vma->bound |= GLOBAL_BIND;
 		}
 	}
@@ -2021,7 +2024,7 @@ static void ggtt_bind_vma(struct i915_vma *vma,
 		struct i915_hw_ppgtt *appgtt = dev_priv->mm.aliasing_ppgtt;
 		appgtt->base.insert_entries(&appgtt->base, pages,
 					    vma->node.start,
-					    cache_level, flags);
+					    cache_level, pte_flags);
 		vma->bound |= LOCAL_BIND;
 	}
 }
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
index 12d0ded0d823..fb0a04aa5363 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.h
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
@@ -158,7 +158,6 @@ struct i915_vma {
 	/** Flags and address space this VMA is bound to */
 #define GLOBAL_BIND	(1<<0)
 #define LOCAL_BIND	(1<<1)
-#define PTE_READ_ONLY	(1<<2)
 	unsigned int bound : 4;
 
 	/**
@@ -261,6 +260,8 @@ struct i915_address_space {
 	gen6_pte_t (*pte_encode)(dma_addr_t addr,
 				 enum i915_cache_level level,
 				 bool valid, u32 flags); /* Create a valid PTE */
+	/* flags for pte_encode */
+#define PTE_READ_ONLY	(1<<0)
 	int (*allocate_va_range)(struct i915_address_space *vm,
 				 uint64_t start,
 				 uint64_t length);
-- 
2.1.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 05/17] drm/i915: Move PTE_READ_ONLY to ->pte_encode vfunc

[Mika Kuoppala]

Daniel Vetter <daniel.vetter@ffwll.ch> writes:

> It's only used as a flag there, so unconfuse things a bit.
> Also separate the bind_vma flag space from the pte_encode flag
> space in the code.
>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com>

> ---
>  drivers/gpu/drm/i915/i915_gem_gtt.c | 15 +++++++++------
>  drivers/gpu/drm/i915/i915_gem_gtt.h |  3 ++-
>  2 files changed, 11 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 75787f1d2751..4e2caef83772 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -1610,14 +1610,16 @@ void  i915_ppgtt_release(struct kref *kref)
>  static void
>  ppgtt_bind_vma(struct i915_vma *vma,
>  	       enum i915_cache_level cache_level,
> -	       u32 flags)
> +	       u32 unused)
>  {
> +	u32 pte_flags = 0;
> +
>  	/* Currently applicable only to VLV */
>  	if (vma->obj->gt_ro)
> -		flags |= PTE_READ_ONLY;
> +		pte_flags |= PTE_READ_ONLY;
>  
>  	vma->vm->insert_entries(vma->vm, vma->obj->pages, vma->node.start,
> -				cache_level, flags);
> +				cache_level, pte_flags);
>  }
>  
>  static void ppgtt_unbind_vma(struct i915_vma *vma)
> @@ -1986,10 +1988,11 @@ static void ggtt_bind_vma(struct i915_vma *vma,
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct drm_i915_gem_object *obj = vma->obj;
>  	struct sg_table *pages = obj->pages;
> +	u32 pte_flags = 0;
>  
>  	/* Currently applicable only to VLV */
>  	if (obj->gt_ro)
> -		flags |= PTE_READ_ONLY;
> +		pte_flags |= PTE_READ_ONLY;
>  
>  	if (i915_is_ggtt(vma->vm))
>  		pages = vma->ggtt_view.pages;
> @@ -2010,7 +2013,7 @@ static void ggtt_bind_vma(struct i915_vma *vma,
>  		    (cache_level != obj->cache_level)) {
>  			vma->vm->insert_entries(vma->vm, pages,
>  						vma->node.start,
> -						cache_level, flags);
> +						cache_level, pte_flags);
>  			vma->bound |= GLOBAL_BIND;
>  		}
>  	}
> @@ -2021,7 +2024,7 @@ static void ggtt_bind_vma(struct i915_vma *vma,
>  		struct i915_hw_ppgtt *appgtt = dev_priv->mm.aliasing_ppgtt;
>  		appgtt->base.insert_entries(&appgtt->base, pages,
>  					    vma->node.start,
> -					    cache_level, flags);
> +					    cache_level, pte_flags);
>  		vma->bound |= LOCAL_BIND;
>  	}
>  }
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
> index 12d0ded0d823..fb0a04aa5363 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.h
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
> @@ -158,7 +158,6 @@ struct i915_vma {
>  	/** Flags and address space this VMA is bound to */
>  #define GLOBAL_BIND	(1<<0)
>  #define LOCAL_BIND	(1<<1)
> -#define PTE_READ_ONLY	(1<<2)
>  	unsigned int bound : 4;
>  
>  	/**
> @@ -261,6 +260,8 @@ struct i915_address_space {
>  	gen6_pte_t (*pte_encode)(dma_addr_t addr,
>  				 enum i915_cache_level level,
>  				 bool valid, u32 flags); /* Create a valid PTE */
> +	/* flags for pte_encode */
> +#define PTE_READ_ONLY	(1<<0)
>  	int (*allocate_va_range)(struct i915_address_space *vm,
>  				 uint64_t start,
>  				 uint64_t length);
> -- 
> 2.1.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 06/17] drm/i915: Dont clear PIN_GLOBAL in the execbuf pinning fallback

[Daniel Vetter]

PIN_GLOBAL is set only when userspace asked for it, and that
is only the case for the gen6 PIPE_CONTROL workaround. We're not
allowed to just clear this.

The important part of the fallback is to drop the restriction to
the mappable range.

This issue has been introduced in

commit edf4427b8055dc93eb5222d8174b07a75ba24fb5
Author: Chris Wilson <chris@chris-wilson.co.uk>
Date:   Wed Jan 14 11:20:56 2015 +0000

    drm/i915: Fallback to using CPU relocations for large batch buffers

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 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 48ac5608481e..a7ed9f695586 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -600,7 +600,7 @@ i915_gem_execbuffer_reserve_vma(struct i915_vma *vma,
 	    only_mappable_for_reloc(entry->flags))
 		ret = i915_gem_object_pin(obj, vma->vm,
 					  entry->alignment,
-					  flags & ~(PIN_GLOBAL | PIN_MAPPABLE));
+					  flags & ~PIN_MAPPABLE);
 	if (ret)
 		return ret;
 
-- 
2.1.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 06/17] drm/i915: Dont clear PIN_GLOBAL in the execbuf pinning fallback

[Chris Wilson]

On Tue, Apr 14, 2015 at 05:35:16PM +0200, Daniel Vetter wrote:
> PIN_GLOBAL is set only when userspace asked for it, and that
> is only the case for the gen6 PIPE_CONTROL workaround. We're not
> allowed to just clear this.

Nope. See only_mappable_for_reloc(). There is an issue here, but this is
not it.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 06/17] drm/i915: Dont clear PIN_GLOBAL in the execbuf pinning fallback

[Chris Wilson]

On Tue, Apr 14, 2015 at 04:53:24PM +0100, Chris Wilson wrote:
> On Tue, Apr 14, 2015 at 05:35:16PM +0200, Daniel Vetter wrote:
> > PIN_GLOBAL is set only when userspace asked for it, and that
> > is only the case for the gen6 PIPE_CONTROL workaround. We're not
> > allowed to just clear this.
> 
> Nope. See only_mappable_for_reloc(). There is an issue here, but this is
> not it.

Less cyptic, I think you want:

diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index a60bfeadc4fb..8599cd87cce5 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -735,14 +735,14 @@ i915_gem_execbuffer_reserve_vma(struct i915_vma *vma,
        int ret;
 
        flags = 0;
+       if (entry->flags & EXEC_OBJECT_NEEDS_GTT)
+               flags |= PIN_GLOBAL;
        if (!drm_mm_node_allocated(&vma->node)) {
                if (entry->flags & __EXEC_OBJECT_NEEDS_MAP) {
                        flags |= PIN_GLOBAL | PIN_MAPPABLE;
                        if (only_mappable_for_reloc(entry->flags))
                                flags |= PIN_NONBLOCK;
                }
-               if (entry->flags & EXEC_OBJECT_NEEDS_GTT)
-                       flags |= PIN_GLOBAL;
                if (entry->flags & __EXEC_OBJECT_NEEDS_BIAS)
                        flags |= BATCH_OFFSET_BIAS | PIN_OFFSET_BIAS;
                if (entry->flags & EXEC_OBJECT_PINNED)

-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH] drm/i915: Dont clear PIN_GLOBAL in the execbuf pinning fallback

[Daniel Vetter]

PIN_GLOBAL is set only when userspace asked for it, and that
is only the case for the gen6 PIPE_CONTROL workaround. We're not
allowed to just clear this.

The important part of the fallback is to drop the restriction to
the mappable range.

This issue has been introduced in

commit edf4427b8055dc93eb5222d8174b07a75ba24fb5
Author: Chris Wilson <chris@chris-wilson.co.uk>
Date:   Wed Jan 14 11:20:56 2015 +0000

    drm/i915: Fallback to using CPU relocations for large batch buffers

v2: Chris pointed out that we also miss to set PIN_GLOBAL when the
buffer is already bound. Fix this up too.

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_gem_execbuffer.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 48ac5608481e..564425fce1a7 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -586,11 +586,12 @@ i915_gem_execbuffer_reserve_vma(struct i915_vma *vma,
 	int ret;
 
 	flags = 0;
+	if (entry->flags & EXEC_OBJECT_NEEDS_GTT)
+		flags |= PIN_GLOBAL;
+
 	if (!drm_mm_node_allocated(&vma->node)) {
 		if (entry->flags & __EXEC_OBJECT_NEEDS_MAP)
 			flags |= PIN_GLOBAL | PIN_MAPPABLE;
-		if (entry->flags & EXEC_OBJECT_NEEDS_GTT)
-			flags |= PIN_GLOBAL;
 		if (entry->flags & __EXEC_OBJECT_NEEDS_BIAS)
 			flags |= BATCH_OFFSET_BIAS | PIN_OFFSET_BIAS;
 	}
@@ -600,7 +601,7 @@ i915_gem_execbuffer_reserve_vma(struct i915_vma *vma,
 	    only_mappable_for_reloc(entry->flags))
 		ret = i915_gem_object_pin(obj, vma->vm,
 					  entry->alignment,
-					  flags & ~(PIN_GLOBAL | PIN_MAPPABLE));
+					  flags & ~PIN_MAPPABLE);
 	if (ret)
 		return ret;
 
-- 
2.1.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Dont clear PIN_GLOBAL in the execbuf pinning fallback

[shuang.he]

Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 6193
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                 -8              276/276              268/276
ILK                                  301/301              301/301
SNB                                  316/316              316/316
IVB                 -4              328/328              324/328
BYT                                  285/285              285/285
HSW                                  394/394              394/394
BDW                                  321/321              321/321
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
*PNV  igt@gem_fence_thrash@bo-write-verify-none      PASS(3)      FAIL(1)PASS(1)
*PNV  igt@gem_fence_thrash@bo-write-verify-threaded-none      PASS(3)      CRASH(1)PASS(1)
 PNV  igt@gem_userptr_blits@coherency-sync      CRASH(2)PASS(6)      CRASH(1)PASS(1)
 PNV  igt@gem_userptr_blits@coherency-unsync      CRASH(2)PASS(7)      CRASH(1)PASS(1)
 PNV  igt@gen3_render_linear_blits      FAIL(4)PASS(9)      FAIL(1)PASS(1)
 PNV  igt@gen3_render_mixed_blits      FAIL(5)PASS(8)      FAIL(1)PASS(1)
 PNV  igt@gen3_render_tiledx_blits      FAIL(5)PASS(9)      FAIL(1)PASS(1)
 PNV  igt@gen3_render_tiledy_blits      FAIL(4)PASS(9)      FAIL(1)PASS(1)
*IVB  igt@kms_cursor_crc@cursor-128x128-onscreen      PASS(2)      TIMEOUT(1)PASS(1)
*IVB  igt@kms_cursor_crc@cursor-128x128-random      PASS(2)      TIMEOUT(1)PASS(1)
*IVB  igt@kms_cursor_crc@cursor-128x128-sliding      PASS(2)      TIMEOUT(1)PASS(1)
*IVB  igt@kms_cursor_crc@cursor-256x256-onscreen      PASS(2)      TIMEOUT(1)PASS(1)
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 07/17] drm/i915: Drop redundant GGTT rebinding

[Daniel Vetter]

Since

commit bf3d149b25f67f241735b91a56b7f070bc0a5407
Author: Daniel Vetter <daniel.vetter@ffwll.ch>
Date:   Fri Feb 14 14:01:12 2014 +0100

    drm/i915: split PIN_GLOBAL out from PIN_MAPPABLE

i915_gem_obj_ggtt_pin always binds into the ggtt, but I've forgotten
to remove the now redundant additional bind call later on. Fix this
up.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_context.c | 11 -----------
 1 file changed, 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index e4c57a3981b3..dd5bff657c9c 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -638,7 +638,6 @@ static int do_switch(struct intel_engine_cs *ring,
 	struct intel_context *from = ring->last_context;
 	u32 hw_flags = 0;
 	bool uninitialized = false;
-	struct i915_vma *vma;
 	int ret, i;
 
 	if (from != NULL && ring == &dev_priv->ring[RCS]) {
@@ -696,16 +695,6 @@ static int do_switch(struct intel_engine_cs *ring,
 	if (ret)
 		goto unpin_out;
 
-	vma = i915_gem_obj_to_ggtt(to->legacy_hw_ctx.rcs_state);
-	if (!(vma->bound & GLOBAL_BIND)) {
-		ret = i915_vma_bind(vma,
-				    to->legacy_hw_ctx.rcs_state->cache_level,
-				    GLOBAL_BIND);
-		/* This shouldn't ever fail. */
-		if (WARN_ONCE(ret, "GGTT context bind failed!"))
-			goto unpin_out;
-	}
-
 	if (!to->legacy_hw_ctx.initialized) {
 		hw_flags |= MI_RESTORE_INHIBIT;
 		/* NB: If we inhibit the restore, the context is not allowed to
-- 
2.1.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 07/17] drm/i915: Drop redundant GGTT rebinding

[Chris Wilson]

On Tue, Apr 14, 2015 at 05:35:17PM +0200, Daniel Vetter wrote:
> Since
> 
> commit bf3d149b25f67f241735b91a56b7f070bc0a5407
> Author: Daniel Vetter <daniel.vetter@ffwll.ch>
> Date:   Fri Feb 14 14:01:12 2014 +0100
> 
>     drm/i915: split PIN_GLOBAL out from PIN_MAPPABLE
> 
> i915_gem_obj_ggtt_pin always binds into the ggtt, but I've forgotten
> to remove the now redundant additional bind call later on. Fix this
> up.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 08/17] drm/i915: Don't look at pg_dirty_rings for aliasing ppgtt

[Daniel Vetter]

We load the ppgtt ptes once per gpu reset/driver load/resume and
that's all that's needed. Note that this only blows up when we're
using the allocate_va_range funcs and not the special-purpose ones
used. With this change we can get rid of that duplication.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_context.c    | 6 ------
 drivers/gpu/drm/i915/i915_gem_execbuffer.c | 3 ---
 2 files changed, 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index dd5bff657c9c..6b0962db2cf7 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -575,8 +575,6 @@ static inline bool should_skip_switch(struct intel_engine_cs *ring,
 				      struct intel_context *from,
 				      struct intel_context *to)
 {
-	struct drm_i915_private *dev_priv = ring->dev->dev_private;
-
 	if (to->remap_slice)
 		return false;
 
@@ -584,10 +582,6 @@ static inline bool should_skip_switch(struct intel_engine_cs *ring,
 		if (from == to && !test_bit(ring->id,
 				&to->ppgtt->pd_dirty_rings))
 			return true;
-	} else if (dev_priv->mm.aliasing_ppgtt) {
-		if (from == to && !test_bit(ring->id,
-				&dev_priv->mm.aliasing_ppgtt->pd_dirty_rings))
-			return true;
 	}
 
 	return false;
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index a7ed9f695586..1d5badf1b887 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -1250,9 +1250,6 @@ i915_gem_ringbuffer_submission(struct drm_device *dev, struct drm_file *file,
 	if (ctx->ppgtt)
 		WARN(ctx->ppgtt->pd_dirty_rings & (1<<ring->id),
 			"%s didn't clear reload\n", ring->name);
-	else if (dev_priv->mm.aliasing_ppgtt)
-		WARN(dev_priv->mm.aliasing_ppgtt->pd_dirty_rings &
-			(1<<ring->id), "%s didn't clear reload\n", ring->name);
 
 	instp_mode = args->flags & I915_EXEC_CONSTANTS_MASK;
 	instp_mask = I915_EXEC_CONSTANTS_MASK;
-- 
2.1.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 08/17] drm/i915: Don't look at pg_dirty_rings for aliasing ppgtt

[Chris Wilson]

On Tue, Apr 14, 2015 at 05:35:18PM +0200, Daniel Vetter wrote:
> We load the ppgtt ptes once per gpu reset/driver load/resume and
> that's all that's needed. Note that this only blows up when we're
> using the allocate_va_range funcs and not the special-purpose ones
> used. With this change we can get rid of that duplication.

Honestly, I would prefer the test to be rewritten so that the
information on which vm was being used was passed along and not that
magic sprinkled in the middle of nowhere. e.g. execbuffer knows exactly
what vm it bound the objects into, and yet towards the end we decide to
guess again. Also, I would rather the execbuffer test be moved to
i915_gem_context since it is scattering internal knowledge about.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 08/17] drm/i915: Don't look at pg_dirty_rings for aliasing ppgtt

[Daniel Vetter]

On Tue, Apr 14, 2015 at 05:06:36PM +0100, Chris Wilson wrote:
> On Tue, Apr 14, 2015 at 05:35:18PM +0200, Daniel Vetter wrote:
> > We load the ppgtt ptes once per gpu reset/driver load/resume and
> > that's all that's needed. Note that this only blows up when we're
> > using the allocate_va_range funcs and not the special-purpose ones
> > used. With this change we can get rid of that duplication.
> 
> Honestly, I would prefer the test to be rewritten so that the
> information on which vm was being used was passed along and not that
> magic sprinkled in the middle of nowhere. e.g. execbuffer knows exactly
> what vm it bound the objects into, and yet towards the end we decide to
> guess again. Also, I would rather the execbuffer test be moved to
> i915_gem_context since it is scattering internal knowledge about.

Yeah I agree that there's more room for cleanup. I've done this minimal
patch purely to shut up the bogus WARN_ONs when I tried to unify the
gen6/7 pt alloc code in the next patch. Since it's bogus.

But I agree that the current pd reloading is hard to understand, and might
even be the reason why gen7 full ppgtt doesn't quite work. Who knows, but
otoh the code isnt' too harmful either (until we decide to can gen7 full
ppgtt for real and remove it all).
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 08/17] drm/i915: Don't look at pg_dirty_rings for aliasing ppgtt

[Chris Wilson]

On Tue, Apr 14, 2015 at 07:11:25PM +0200, Daniel Vetter wrote:
> On Tue, Apr 14, 2015 at 05:06:36PM +0100, Chris Wilson wrote:
> > On Tue, Apr 14, 2015 at 05:35:18PM +0200, Daniel Vetter wrote:
> > > We load the ppgtt ptes once per gpu reset/driver load/resume and
> > > that's all that's needed. Note that this only blows up when we're
> > > using the allocate_va_range funcs and not the special-purpose ones
> > > used. With this change we can get rid of that duplication.
> > 
> > Honestly, I would prefer the test to be rewritten so that the
> > information on which vm was being used was passed along and not that
> > magic sprinkled in the middle of nowhere. e.g. execbuffer knows exactly
> > what vm it bound the objects into, and yet towards the end we decide to
> > guess again. Also, I would rather the execbuffer test be moved to
> > i915_gem_context since it is scattering internal knowledge about.
> 
> Yeah I agree that there's more room for cleanup. I've done this minimal
> patch purely to shut up the bogus WARN_ONs when I tried to unify the
> gen6/7 pt alloc code in the next patch. Since it's bogus.

How about:

diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index ed9232126644..be0f475ee1e5 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -638,22 +638,14 @@ mi_set_context(struct intel_engine_cs *ring,
 
 static inline bool should_skip_switch(struct intel_engine_cs *ring,
                                      struct intel_context *from,
-                                     struct intel_context *to)
+                                     struct intel_context *to,
+                                     struct i915_hw_ppgtt *ppgtt)
 {
-       struct drm_i915_private *dev_priv = ring->dev->dev_private;
-
        if (to->remap_slice)
                return false;
 
-       if (to->ppgtt) {
-               if (from == to && !test_bit(ring->id,
-                               &to->ppgtt->pd_dirty_rings))
-                       return true;
-       } else if (dev_priv->mm.aliasing_ppgtt) {
-               if (from == to && !test_bit(ring->id,
-                               &dev_priv->mm.aliasing_ppgtt->pd_dirty_rings))
-                       return true;
-       }
+       if (from == to && !test_bit(ring->id, &ppgtt->pd_dirty_rings))
+               return true;
 
        return false;
 }
@@ -661,15 +653,13 @@ static inline bool should_skip_switch(struct intel_engine_cs *ring,
 static bool
 needs_pd_load_pre(struct intel_engine_cs *ring, struct intel_context *to)
 {
-       struct drm_i915_private *dev_priv = ring->dev->dev_private;
-
        if (!to->ppgtt)
                return false;
 
        if (INTEL_INFO(ring->dev)->gen < 8)
                return true;
 
-       if (ring != &dev_priv->ring[RCS])
+       if (ring->id != RCS)
                return true;
 
        return false;
@@ -679,15 +669,13 @@ static bool
 needs_pd_load_post(struct intel_engine_cs *ring, struct intel_context *to,
                u32 hw_flags)
 {
-       struct drm_i915_private *dev_priv = ring->dev->dev_private;
-
        if (!to->ppgtt)
                return false;
 
        if (!IS_GEN8(ring->dev))
                return false;
 
-       if (ring != &dev_priv->ring[RCS])
+       if (ring->id != RCS)
                return false;
 
        if (hw_flags & MI_RESTORE_INHIBIT)
@@ -701,14 +689,15 @@ static int do_switch(struct intel_engine_cs *ring,
 {
        struct drm_i915_private *dev_priv = ring->dev->dev_private;
        struct intel_context *from = ring->last_context;
+       struct i915_hw_ppgtt *ppgtt = to->ppgtt ?: dev_priv->mm.aliasing_ppgtt;
        u32 hw_flags = 0;
        bool uninitialized = false;
        int ret, i;
 
-       if (from != NULL && ring == &dev_priv->ring[RCS])
+       if (from != NULL && ring->id == RCS)
                BUG_ON(from->legacy_hw_ctx.rcs_vma == NULL);
 
-       if (should_skip_switch(ring, from, to))
+       if (should_skip_switch(ring, from, to, ppgtt))
                return 0;
 
        /* Trying to pin first makes error handling easier. */
@@ -851,6 +840,9 @@ done:
        i915_gem_context_reference(to);
        ring->last_context = to;
 
+       WARN(ppgtt->pd_dirty_rings & (1<<ring->id),
+            "%s didn't clear reload\n", ring->name);
+
        if (uninitialized) {
                if (ring->init_context) {
                        ret = ring->init_context(ring, to);
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index ef644e7eaac0..595daecb3283 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -1385,13 +1385,6 @@ i915_gem_ringbuffer_submission(struct drm_device *dev, struct drm_file *file,
        if (ret)
                goto error;
 
-       if (ctx->ppgtt)
-               WARN(ctx->ppgtt->pd_dirty_rings & (1<<ring->id),
-                       "%s didn't clear reload\n", ring->name);
-       else if (dev_priv->mm.aliasing_ppgtt)
-               WARN(dev_priv->mm.aliasing_ppgtt->pd_dirty_rings &
-                       (1<<ring->id), "%s didn't clear reload\n", ring->name);
-
        instp_mode = args->flags & I915_EXEC_CONSTANTS_MASK;
        instp_mask = I915_EXEC_CONSTANTS_MASK;
        switch (instp_mode) {


It gets rid of the internal knowledge of address spaces from the execbuf
code, and keeps the symmetry between handling aliasing_ppgtt and
full_ppgtt. (The former should just be a degenerate case where the PD
are all preloaded).
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 08/17] drm/i915: Don't look at pg_dirty_rings for aliasing ppgtt

[Daniel Vetter]

On Tue, Apr 14, 2015 at 06:53:41PM +0100, Chris Wilson wrote:
> On Tue, Apr 14, 2015 at 07:11:25PM +0200, Daniel Vetter wrote:
> > On Tue, Apr 14, 2015 at 05:06:36PM +0100, Chris Wilson wrote:
> > > On Tue, Apr 14, 2015 at 05:35:18PM +0200, Daniel Vetter wrote:
> > > > We load the ppgtt ptes once per gpu reset/driver load/resume and
> > > > that's all that's needed. Note that this only blows up when we're
> > > > using the allocate_va_range funcs and not the special-purpose ones
> > > > used. With this change we can get rid of that duplication.
> > > 
> > > Honestly, I would prefer the test to be rewritten so that the
> > > information on which vm was being used was passed along and not that
> > > magic sprinkled in the middle of nowhere. e.g. execbuffer knows exactly
> > > what vm it bound the objects into, and yet towards the end we decide to
> > > guess again. Also, I would rather the execbuffer test be moved to
> > > i915_gem_context since it is scattering internal knowledge about.
> > 
> > Yeah I agree that there's more room for cleanup. I've done this minimal
> > patch purely to shut up the bogus WARN_ONs when I tried to unify the
> > gen6/7 pt alloc code in the next patch. Since it's bogus.
> 
> How about:

Yeah, but imo there's also more. I tried to understand the gen8 legacy ctx
switch logic and failed, and I wasn't fully convinced that the gen7 one
won't WARN if we actually enable full ppgtt. Given all that I decided to
go with the most minimal patch and just removed the offending bogus WARN.
But Mika/Michel promised to hang around for eventual cleanups?
-Daniel

> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index ed9232126644..be0f475ee1e5 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -638,22 +638,14 @@ mi_set_context(struct intel_engine_cs *ring,
>  
>  static inline bool should_skip_switch(struct intel_engine_cs *ring,
>                                       struct intel_context *from,
> -                                     struct intel_context *to)
> +                                     struct intel_context *to,
> +                                     struct i915_hw_ppgtt *ppgtt)
>  {
> -       struct drm_i915_private *dev_priv = ring->dev->dev_private;
> -
>         if (to->remap_slice)
>                 return false;
>  
> -       if (to->ppgtt) {
> -               if (from == to && !test_bit(ring->id,
> -                               &to->ppgtt->pd_dirty_rings))
> -                       return true;
> -       } else if (dev_priv->mm.aliasing_ppgtt) {
> -               if (from == to && !test_bit(ring->id,
> -                               &dev_priv->mm.aliasing_ppgtt->pd_dirty_rings))
> -                       return true;
> -       }
> +       if (from == to && !test_bit(ring->id, &ppgtt->pd_dirty_rings))
> +               return true;
>  
>         return false;
>  }
> @@ -661,15 +653,13 @@ static inline bool should_skip_switch(struct intel_engine_cs *ring,
>  static bool
>  needs_pd_load_pre(struct intel_engine_cs *ring, struct intel_context *to)
>  {
> -       struct drm_i915_private *dev_priv = ring->dev->dev_private;
> -
>         if (!to->ppgtt)
>                 return false;
>  
>         if (INTEL_INFO(ring->dev)->gen < 8)
>                 return true;
>  
> -       if (ring != &dev_priv->ring[RCS])
> +       if (ring->id != RCS)
>                 return true;
>  
>         return false;
> @@ -679,15 +669,13 @@ static bool
>  needs_pd_load_post(struct intel_engine_cs *ring, struct intel_context *to,
>                 u32 hw_flags)
>  {
> -       struct drm_i915_private *dev_priv = ring->dev->dev_private;
> -
>         if (!to->ppgtt)
>                 return false;
>  
>         if (!IS_GEN8(ring->dev))
>                 return false;
>  
> -       if (ring != &dev_priv->ring[RCS])
> +       if (ring->id != RCS)
>                 return false;
>  
>         if (hw_flags & MI_RESTORE_INHIBIT)
> @@ -701,14 +689,15 @@ static int do_switch(struct intel_engine_cs *ring,
>  {
>         struct drm_i915_private *dev_priv = ring->dev->dev_private;
>         struct intel_context *from = ring->last_context;
> +       struct i915_hw_ppgtt *ppgtt = to->ppgtt ?: dev_priv->mm.aliasing_ppgtt;
>         u32 hw_flags = 0;
>         bool uninitialized = false;
>         int ret, i;
>  
> -       if (from != NULL && ring == &dev_priv->ring[RCS])
> +       if (from != NULL && ring->id == RCS)
>                 BUG_ON(from->legacy_hw_ctx.rcs_vma == NULL);
>  
> -       if (should_skip_switch(ring, from, to))
> +       if (should_skip_switch(ring, from, to, ppgtt))
>                 return 0;
>  
>         /* Trying to pin first makes error handling easier. */
> @@ -851,6 +840,9 @@ done:
>         i915_gem_context_reference(to);
>         ring->last_context = to;
>  
> +       WARN(ppgtt->pd_dirty_rings & (1<<ring->id),
> +            "%s didn't clear reload\n", ring->name);
> +
>         if (uninitialized) {
>                 if (ring->init_context) {
>                         ret = ring->init_context(ring, to);
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index ef644e7eaac0..595daecb3283 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -1385,13 +1385,6 @@ i915_gem_ringbuffer_submission(struct drm_device *dev, struct drm_file *file,
>         if (ret)
>                 goto error;
>  
> -       if (ctx->ppgtt)
> -               WARN(ctx->ppgtt->pd_dirty_rings & (1<<ring->id),
> -                       "%s didn't clear reload\n", ring->name);
> -       else if (dev_priv->mm.aliasing_ppgtt)
> -               WARN(dev_priv->mm.aliasing_ppgtt->pd_dirty_rings &
> -                       (1<<ring->id), "%s didn't clear reload\n", ring->name);
> -
>         instp_mode = args->flags & I915_EXEC_CONSTANTS_MASK;
>         instp_mask = I915_EXEC_CONSTANTS_MASK;
>         switch (instp_mode) {
> 
> 
> It gets rid of the internal knowledge of address spaces from the execbuf
> code, and keeps the symmetry between handling aliasing_ppgtt and
> full_ppgtt. (The former should just be a degenerate case where the PD
> are all preloaded).
> -Chris
> 
> -- 
> Chris Wilson, Intel Open Source Technology Centre

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 08/17] drm/i915: Don't look at pg_dirty_rings for aliasing ppgtt

[Mika Kuoppala]

Daniel Vetter <daniel@ffwll.ch> writes:

> On Tue, Apr 14, 2015 at 06:53:41PM +0100, Chris Wilson wrote:
>> On Tue, Apr 14, 2015 at 07:11:25PM +0200, Daniel Vetter wrote:
>> > On Tue, Apr 14, 2015 at 05:06:36PM +0100, Chris Wilson wrote:
>> > > On Tue, Apr 14, 2015 at 05:35:18PM +0200, Daniel Vetter wrote:
>> > > > We load the ppgtt ptes once per gpu reset/driver load/resume and
>> > > > that's all that's needed. Note that this only blows up when we're
>> > > > using the allocate_va_range funcs and not the special-purpose ones
>> > > > used. With this change we can get rid of that duplication.
>> > > 
>> > > Honestly, I would prefer the test to be rewritten so that the
>> > > information on which vm was being used was passed along and not that
>> > > magic sprinkled in the middle of nowhere. e.g. execbuffer knows exactly
>> > > what vm it bound the objects into, and yet towards the end we decide to
>> > > guess again. Also, I would rather the execbuffer test be moved to
>> > > i915_gem_context since it is scattering internal knowledge about.
>> > 
>> > Yeah I agree that there's more room for cleanup. I've done this minimal
>> > patch purely to shut up the bogus WARN_ONs when I tried to unify the
>> > gen6/7 pt alloc code in the next patch. Since it's bogus.
>> 
>> How about:
>
> Yeah, but imo there's also more. I tried to understand the gen8 legacy ctx
> switch logic and failed, and I wasn't fully convinced that the gen7 one
> won't WARN if we actually enable full ppgtt. Given all that I decided to
> go with the most minimal patch and just removed the offending bogus WARN.
> But Mika/Michel promised to hang around for eventual cleanups?

Yes. There is more to come after this series.
I can include Chris's suggestion.

-Mika

> -Daniel
>
>> 
>> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
>> index ed9232126644..be0f475ee1e5 100644
>> --- a/drivers/gpu/drm/i915/i915_gem_context.c
>> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
>> @@ -638,22 +638,14 @@ mi_set_context(struct intel_engine_cs *ring,
>>  
>>  static inline bool should_skip_switch(struct intel_engine_cs *ring,
>>                                       struct intel_context *from,
>> -                                     struct intel_context *to)
>> +                                     struct intel_context *to,
>> +                                     struct i915_hw_ppgtt *ppgtt)
>>  {
>> -       struct drm_i915_private *dev_priv = ring->dev->dev_private;
>> -
>>         if (to->remap_slice)
>>                 return false;
>>  
>> -       if (to->ppgtt) {
>> -               if (from == to && !test_bit(ring->id,
>> -                               &to->ppgtt->pd_dirty_rings))
>> -                       return true;
>> -       } else if (dev_priv->mm.aliasing_ppgtt) {
>> -               if (from == to && !test_bit(ring->id,
>> -                               &dev_priv->mm.aliasing_ppgtt->pd_dirty_rings))
>> -                       return true;
>> -       }
>> +       if (from == to && !test_bit(ring->id, &ppgtt->pd_dirty_rings))
>> +               return true;
>>  
>>         return false;
>>  }
>> @@ -661,15 +653,13 @@ static inline bool should_skip_switch(struct intel_engine_cs *ring,
>>  static bool
>>  needs_pd_load_pre(struct intel_engine_cs *ring, struct intel_context *to)
>>  {
>> -       struct drm_i915_private *dev_priv = ring->dev->dev_private;
>> -
>>         if (!to->ppgtt)
>>                 return false;
>>  
>>         if (INTEL_INFO(ring->dev)->gen < 8)
>>                 return true;
>>  
>> -       if (ring != &dev_priv->ring[RCS])
>> +       if (ring->id != RCS)
>>                 return true;
>>  
>>         return false;
>> @@ -679,15 +669,13 @@ static bool
>>  needs_pd_load_post(struct intel_engine_cs *ring, struct intel_context *to,
>>                 u32 hw_flags)
>>  {
>> -       struct drm_i915_private *dev_priv = ring->dev->dev_private;
>> -
>>         if (!to->ppgtt)
>>                 return false;
>>  
>>         if (!IS_GEN8(ring->dev))
>>                 return false;
>>  
>> -       if (ring != &dev_priv->ring[RCS])
>> +       if (ring->id != RCS)
>>                 return false;
>>  
>>         if (hw_flags & MI_RESTORE_INHIBIT)
>> @@ -701,14 +689,15 @@ static int do_switch(struct intel_engine_cs *ring,
>>  {
>>         struct drm_i915_private *dev_priv = ring->dev->dev_private;
>>         struct intel_context *from = ring->last_context;
>> +       struct i915_hw_ppgtt *ppgtt = to->ppgtt ?: dev_priv->mm.aliasing_ppgtt;
>>         u32 hw_flags = 0;
>>         bool uninitialized = false;
>>         int ret, i;
>>  
>> -       if (from != NULL && ring == &dev_priv->ring[RCS])
>> +       if (from != NULL && ring->id == RCS)
>>                 BUG_ON(from->legacy_hw_ctx.rcs_vma == NULL);
>>  
>> -       if (should_skip_switch(ring, from, to))
>> +       if (should_skip_switch(ring, from, to, ppgtt))
>>                 return 0;
>>  
>>         /* Trying to pin first makes error handling easier. */
>> @@ -851,6 +840,9 @@ done:
>>         i915_gem_context_reference(to);
>>         ring->last_context = to;
>>  
>> +       WARN(ppgtt->pd_dirty_rings & (1<<ring->id),
>> +            "%s didn't clear reload\n", ring->name);
>> +
>>         if (uninitialized) {
>>                 if (ring->init_context) {
>>                         ret = ring->init_context(ring, to);
>> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
>> index ef644e7eaac0..595daecb3283 100644
>> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
>> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
>> @@ -1385,13 +1385,6 @@ i915_gem_ringbuffer_submission(struct drm_device *dev, struct drm_file *file,
>>         if (ret)
>>                 goto error;
>>  
>> -       if (ctx->ppgtt)
>> -               WARN(ctx->ppgtt->pd_dirty_rings & (1<<ring->id),
>> -                       "%s didn't clear reload\n", ring->name);
>> -       else if (dev_priv->mm.aliasing_ppgtt)
>> -               WARN(dev_priv->mm.aliasing_ppgtt->pd_dirty_rings &
>> -                       (1<<ring->id), "%s didn't clear reload\n", ring->name);
>> -
>>         instp_mode = args->flags & I915_EXEC_CONSTANTS_MASK;
>>         instp_mask = I915_EXEC_CONSTANTS_MASK;
>>         switch (instp_mode) {
>> 
>> 
>> It gets rid of the internal knowledge of address spaces from the execbuf
>> code, and keeps the symmetry between handling aliasing_ppgtt and
>> full_ppgtt. (The former should just be a degenerate case where the PD
>> are all preloaded).
>> -Chris
>> 
>> -- 
>> Chris Wilson, Intel Open Source Technology Centre
>
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 08/17] drm/i915: Don't look at pg_dirty_rings for aliasing ppgtt

[Daniel Vetter]

On Fri, Apr 17, 2015 at 04:49:18PM +0300, Mika Kuoppala wrote:
> Daniel Vetter <daniel@ffwll.ch> writes:
> 
> > On Tue, Apr 14, 2015 at 06:53:41PM +0100, Chris Wilson wrote:
> >> On Tue, Apr 14, 2015 at 07:11:25PM +0200, Daniel Vetter wrote:
> >> > On Tue, Apr 14, 2015 at 05:06:36PM +0100, Chris Wilson wrote:
> >> > > On Tue, Apr 14, 2015 at 05:35:18PM +0200, Daniel Vetter wrote:
> >> > > > We load the ppgtt ptes once per gpu reset/driver load/resume and
> >> > > > that's all that's needed. Note that this only blows up when we're
> >> > > > using the allocate_va_range funcs and not the special-purpose ones
> >> > > > used. With this change we can get rid of that duplication.
> >> > > 
> >> > > Honestly, I would prefer the test to be rewritten so that the
> >> > > information on which vm was being used was passed along and not that
> >> > > magic sprinkled in the middle of nowhere. e.g. execbuffer knows exactly
> >> > > what vm it bound the objects into, and yet towards the end we decide to
> >> > > guess again. Also, I would rather the execbuffer test be moved to
> >> > > i915_gem_context since it is scattering internal knowledge about.
> >> > 
> >> > Yeah I agree that there's more room for cleanup. I've done this minimal
> >> > patch purely to shut up the bogus WARN_ONs when I tried to unify the
> >> > gen6/7 pt alloc code in the next patch. Since it's bogus.
> >> 
> >> How about:
> >
> > Yeah, but imo there's also more. I tried to understand the gen8 legacy ctx
> > switch logic and failed, and I wasn't fully convinced that the gen7 one
> > won't WARN if we actually enable full ppgtt. Given all that I decided to
> > go with the most minimal patch and just removed the offending bogus WARN.
> > But Mika/Michel promised to hang around for eventual cleanups?
> 
> Yes. There is more to come after this series.
> I can include Chris's suggestion.

No r-b as an interim solution to move this patch series forward? Without
this we'll WARN.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 08/17] drm/i915: Don't look at pg_dirty_rings for aliasing ppgtt

[Daniel Vetter]

On Fri, Apr 17, 2015 at 04:49:18PM +0300, Mika Kuoppala wrote:
> Daniel Vetter <daniel@ffwll.ch> writes:
> 
> > On Tue, Apr 14, 2015 at 06:53:41PM +0100, Chris Wilson wrote:
> >> On Tue, Apr 14, 2015 at 07:11:25PM +0200, Daniel Vetter wrote:
> >> > On Tue, Apr 14, 2015 at 05:06:36PM +0100, Chris Wilson wrote:
> >> > > On Tue, Apr 14, 2015 at 05:35:18PM +0200, Daniel Vetter wrote:
> >> > > > We load the ppgtt ptes once per gpu reset/driver load/resume and
> >> > > > that's all that's needed. Note that this only blows up when we're
> >> > > > using the allocate_va_range funcs and not the special-purpose ones
> >> > > > used. With this change we can get rid of that duplication.
> >> > > 
> >> > > Honestly, I would prefer the test to be rewritten so that the
> >> > > information on which vm was being used was passed along and not that
> >> > > magic sprinkled in the middle of nowhere. e.g. execbuffer knows exactly
> >> > > what vm it bound the objects into, and yet towards the end we decide to
> >> > > guess again. Also, I would rather the execbuffer test be moved to
> >> > > i915_gem_context since it is scattering internal knowledge about.
> >> > 
> >> > Yeah I agree that there's more room for cleanup. I've done this minimal
> >> > patch purely to shut up the bogus WARN_ONs when I tried to unify the
> >> > gen6/7 pt alloc code in the next patch. Since it's bogus.
> >> 
> >> How about:
> >
> > Yeah, but imo there's also more. I tried to understand the gen8 legacy ctx
> > switch logic and failed, and I wasn't fully convinced that the gen7 one
> > won't WARN if we actually enable full ppgtt. Given all that I decided to
> > go with the most minimal patch and just removed the offending bogus WARN.
> > But Mika/Michel promised to hang around for eventual cleanups?
> 
> Yes. There is more to come after this series.
> I can include Chris's suggestion.

No r-b on this patch as an interim solution? Without it we'll WARN_ON
unfortunately. Merged all the previous patches meanwhile, thanks for your
review.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 08/17] drm/i915: Don't look at pg_dirty_rings for aliasing ppgtt

[Mika Kuoppala]

Daniel Vetter <daniel@ffwll.ch> writes:

> On Fri, Apr 17, 2015 at 04:49:18PM +0300, Mika Kuoppala wrote:
>> Daniel Vetter <daniel@ffwll.ch> writes:
>> 
>> > On Tue, Apr 14, 2015 at 06:53:41PM +0100, Chris Wilson wrote:
>> >> On Tue, Apr 14, 2015 at 07:11:25PM +0200, Daniel Vetter wrote:
>> >> > On Tue, Apr 14, 2015 at 05:06:36PM +0100, Chris Wilson wrote:
>> >> > > On Tue, Apr 14, 2015 at 05:35:18PM +0200, Daniel Vetter wrote:
>> >> > > > We load the ppgtt ptes once per gpu reset/driver load/resume and
>> >> > > > that's all that's needed. Note that this only blows up when we're
>> >> > > > using the allocate_va_range funcs and not the special-purpose ones
>> >> > > > used. With this change we can get rid of that duplication.
>> >> > > 
>> >> > > Honestly, I would prefer the test to be rewritten so that the
>> >> > > information on which vm was being used was passed along and not that
>> >> > > magic sprinkled in the middle of nowhere. e.g. execbuffer knows exactly
>> >> > > what vm it bound the objects into, and yet towards the end we decide to
>> >> > > guess again. Also, I would rather the execbuffer test be moved to
>> >> > > i915_gem_context since it is scattering internal knowledge about.
>> >> > 
>> >> > Yeah I agree that there's more room for cleanup. I've done this minimal
>> >> > patch purely to shut up the bogus WARN_ONs when I tried to unify the
>> >> > gen6/7 pt alloc code in the next patch. Since it's bogus.
>> >> 
>> >> How about:
>> >
>> > Yeah, but imo there's also more. I tried to understand the gen8 legacy ctx
>> > switch logic and failed, and I wasn't fully convinced that the gen7 one
>> > won't WARN if we actually enable full ppgtt. Given all that I decided to
>> > go with the most minimal patch and just removed the offending bogus WARN.
>> > But Mika/Michel promised to hang around for eventual cleanups?
>> 
>> Yes. There is more to come after this series.
>> I can include Chris's suggestion.
>
> No r-b on this patch as an interim solution? Without it we'll WARN_ON
> unfortunately. Merged all the previous patches meanwhile, thanks for your
> review.

Yes we need this for now.

Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com>

> -Daniel
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 08/17] drm/i915: Don't look at pg_dirty_rings for aliasing ppgtt

[Chris Wilson]

On Fri, Apr 17, 2015 at 04:49:18PM +0300, Mika Kuoppala wrote:
> Daniel Vetter <daniel@ffwll.ch> writes:
> 
> > On Tue, Apr 14, 2015 at 06:53:41PM +0100, Chris Wilson wrote:
> >> On Tue, Apr 14, 2015 at 07:11:25PM +0200, Daniel Vetter wrote:
> >> > On Tue, Apr 14, 2015 at 05:06:36PM +0100, Chris Wilson wrote:
> >> > > On Tue, Apr 14, 2015 at 05:35:18PM +0200, Daniel Vetter wrote:
> >> > > > We load the ppgtt ptes once per gpu reset/driver load/resume and
> >> > > > that's all that's needed. Note that this only blows up when we're
> >> > > > using the allocate_va_range funcs and not the special-purpose ones
> >> > > > used. With this change we can get rid of that duplication.
> >> > > 
> >> > > Honestly, I would prefer the test to be rewritten so that the
> >> > > information on which vm was being used was passed along and not that
> >> > > magic sprinkled in the middle of nowhere. e.g. execbuffer knows exactly
> >> > > what vm it bound the objects into, and yet towards the end we decide to
> >> > > guess again. Also, I would rather the execbuffer test be moved to
> >> > > i915_gem_context since it is scattering internal knowledge about.
> >> > 
> >> > Yeah I agree that there's more room for cleanup. I've done this minimal
> >> > patch purely to shut up the bogus WARN_ONs when I tried to unify the
> >> > gen6/7 pt alloc code in the next patch. Since it's bogus.
> >> 
> >> How about:
> >
> > Yeah, but imo there's also more. I tried to understand the gen8 legacy ctx
> > switch logic and failed, and I wasn't fully convinced that the gen7 one
> > won't WARN if we actually enable full ppgtt. Given all that I decided to
> > go with the most minimal patch and just removed the offending bogus WARN.
> > But Mika/Michel promised to hang around for eventual cleanups?
> 
> Yes. There is more to come after this series.
> I can include Chris's suggestion.

Looking at this WARN that fires continually on gen6/gen7, (it's nice to
have such a blatant WARN to explain the perf decrease), don't we need


diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index a1a9f5dad541..006d4a2610f7 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -1562,6 +1562,8 @@ int i915_ppgtt_init_hw(struct drm_device *dev)
                        ret = ppgtt->switch_mm(ppgtt, ring);
                        if (ret != 0)
                                return ret;
+
+                       ppgtt->pd_dirty_rings &= ~(1 << i);
                }
        }
 
?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 08/17] drm/i915: Don't look at pg_dirty_rings for aliasing ppgtt

[Daniel Vetter]

On Thu, Apr 23, 2015 at 04:43:52PM +0100, Chris Wilson wrote:
> On Fri, Apr 17, 2015 at 04:49:18PM +0300, Mika Kuoppala wrote:
> > Daniel Vetter <daniel@ffwll.ch> writes:
> > 
> > > On Tue, Apr 14, 2015 at 06:53:41PM +0100, Chris Wilson wrote:
> > >> On Tue, Apr 14, 2015 at 07:11:25PM +0200, Daniel Vetter wrote:
> > >> > On Tue, Apr 14, 2015 at 05:06:36PM +0100, Chris Wilson wrote:
> > >> > > On Tue, Apr 14, 2015 at 05:35:18PM +0200, Daniel Vetter wrote:
> > >> > > > We load the ppgtt ptes once per gpu reset/driver load/resume and
> > >> > > > that's all that's needed. Note that this only blows up when we're
> > >> > > > using the allocate_va_range funcs and not the special-purpose ones
> > >> > > > used. With this change we can get rid of that duplication.
> > >> > > 
> > >> > > Honestly, I would prefer the test to be rewritten so that the
> > >> > > information on which vm was being used was passed along and not that
> > >> > > magic sprinkled in the middle of nowhere. e.g. execbuffer knows exactly
> > >> > > what vm it bound the objects into, and yet towards the end we decide to
> > >> > > guess again. Also, I would rather the execbuffer test be moved to
> > >> > > i915_gem_context since it is scattering internal knowledge about.
> > >> > 
> > >> > Yeah I agree that there's more room for cleanup. I've done this minimal
> > >> > patch purely to shut up the bogus WARN_ONs when I tried to unify the
> > >> > gen6/7 pt alloc code in the next patch. Since it's bogus.
> > >> 
> > >> How about:
> > >
> > > Yeah, but imo there's also more. I tried to understand the gen8 legacy ctx
> > > switch logic and failed, and I wasn't fully convinced that the gen7 one
> > > won't WARN if we actually enable full ppgtt. Given all that I decided to
> > > go with the most minimal patch and just removed the offending bogus WARN.
> > > But Mika/Michel promised to hang around for eventual cleanups?
> > 
> > Yes. There is more to come after this series.
> > I can include Chris's suggestion.
> 
> Looking at this WARN that fires continually on gen6/gen7, (it's nice to
> have such a blatant WARN to explain the perf decrease), don't we need
> 
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index a1a9f5dad541..006d4a2610f7 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -1562,6 +1562,8 @@ int i915_ppgtt_init_hw(struct drm_device *dev)
>                         ret = ppgtt->switch_mm(ppgtt, ring);
>                         if (ret != 0)
>                                 return ret;
> +
> +                       ppgtt->pd_dirty_rings &= ~(1 << i);
>                 }
>         }

Yeah, but I didn't really see the point in the debug infrastructure for
aliasing ppgtt. The pde loading is done somewhere completely else than for
full ppgtt. And if we forget to load the pdes the effects are obvious,
whereas failing to reload when changes happen with full ppgtt is much
harder to spot quickly. So I opted to undo those checks again, they have
been defunct before my changes anyway.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 08/17] drm/i915: Don't look at pg_dirty_rings for aliasing ppgtt

[Chris Wilson]

On Thu, Apr 23, 2015 at 08:56:40PM +0200, Daniel Vetter wrote:
> Yeah, but I didn't really see the point in the debug infrastructure for
> aliasing ppgtt. The pde loading is done somewhere completely else than for
> full ppgtt. And if we forget to load the pdes the effects are obvious,
> whereas failing to reload when changes happen with full ppgtt is much
> harder to spot quickly. So I opted to undo those checks again, they have
> been defunct before my changes anyway.

But they did just serve a purpose. They said we were not skipping the
no-op context switches.

I disagree though and that we want to reduce the differences in handling
the two different types of mm. There is no good reason to bake in
knowledge of full-ppgtt vs aliasing-ppgtt here as the decision whether
to do the mm switch is purely decided on the validity of the
current/next PD.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 08/17] drm/i915: Don't look at pg_dirty_rings for aliasing ppgtt

[Chris Wilson]

On Thu, Apr 23, 2015 at 08:56:40PM +0200, Daniel Vetter wrote:
> Yeah, but I didn't really see the point in the debug infrastructure for
> aliasing ppgtt. The pde loading is done somewhere completely else than for
> full ppgtt. And if we forget to load the pdes the effects are obvious,
> whereas failing to reload when changes happen with full ppgtt is much
> harder to spot quickly. So I opted to undo those checks again, they have
> been defunct before my changes anyway.

You removed the WARN, but didn't fix the broken code the WARN was
telling you about.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 08/17] drm/i915: Don't look at pg_dirty_rings for aliasing ppgtt

[Chris Wilson]

On Thu, Apr 23, 2015 at 08:56:40PM +0200, Daniel Vetter wrote:
> On Thu, Apr 23, 2015 at 04:43:52PM +0100, Chris Wilson wrote:
> > On Fri, Apr 17, 2015 at 04:49:18PM +0300, Mika Kuoppala wrote:
> > > Daniel Vetter <daniel@ffwll.ch> writes:
> > > 
> > > > On Tue, Apr 14, 2015 at 06:53:41PM +0100, Chris Wilson wrote:
> > > >> On Tue, Apr 14, 2015 at 07:11:25PM +0200, Daniel Vetter wrote:
> > > >> > On Tue, Apr 14, 2015 at 05:06:36PM +0100, Chris Wilson wrote:
> > > >> > > On Tue, Apr 14, 2015 at 05:35:18PM +0200, Daniel Vetter wrote:
> > > >> > > > We load the ppgtt ptes once per gpu reset/driver load/resume and
> > > >> > > > that's all that's needed. Note that this only blows up when we're
> > > >> > > > using the allocate_va_range funcs and not the special-purpose ones
> > > >> > > > used. With this change we can get rid of that duplication.
> > > >> > > 
> > > >> > > Honestly, I would prefer the test to be rewritten so that the
> > > >> > > information on which vm was being used was passed along and not that
> > > >> > > magic sprinkled in the middle of nowhere. e.g. execbuffer knows exactly
> > > >> > > what vm it bound the objects into, and yet towards the end we decide to
> > > >> > > guess again. Also, I would rather the execbuffer test be moved to
> > > >> > > i915_gem_context since it is scattering internal knowledge about.
> > > >> > 
> > > >> > Yeah I agree that there's more room for cleanup. I've done this minimal
> > > >> > patch purely to shut up the bogus WARN_ONs when I tried to unify the
> > > >> > gen6/7 pt alloc code in the next patch. Since it's bogus.
> > > >> 
> > > >> How about:
> > > >
> > > > Yeah, but imo there's also more. I tried to understand the gen8 legacy ctx
> > > > switch logic and failed, and I wasn't fully convinced that the gen7 one
> > > > won't WARN if we actually enable full ppgtt. Given all that I decided to
> > > > go with the most minimal patch and just removed the offending bogus WARN.
> > > > But Mika/Michel promised to hang around for eventual cleanups?
> > > 
> > > Yes. There is more to come after this series.
> > > I can include Chris's suggestion.
> > 
> > Looking at this WARN that fires continually on gen6/gen7, (it's nice to
> > have such a blatant WARN to explain the perf decrease), don't we need
> > 
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > index a1a9f5dad541..006d4a2610f7 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > @@ -1562,6 +1562,8 @@ int i915_ppgtt_init_hw(struct drm_device *dev)
> >                         ret = ppgtt->switch_mm(ppgtt, ring);
> >                         if (ret != 0)
> >                                 return ret;
> > +
> > +                       ppgtt->pd_dirty_rings &= ~(1 << i);
> >                 }
> >         }
> 
> Yeah, but I didn't really see the point in the debug infrastructure for
> aliasing ppgtt. The pde loading is done somewhere completely else than for
> full ppgtt. And if we forget to load the pdes the effects are obvious,
> whereas failing to reload when changes happen with full ppgtt is much
> harder to spot quickly. So I opted to undo those checks again, they have
> been defunct before my changes anyway.

This performance regression is still unfixed.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 08/17] drm/i915: Don't look at pg_dirty_rings for aliasing ppgtt

[Chris Wilson]

On Fri, Jul 31, 2015 at 05:26:24PM +0100, Chris Wilson wrote:
> This performance regression is still unfixed.

Hah. Just found out that adding the git id to the version also changes
the kernel name - so everytime I was booting the wrong kernel and the
bisect ended up at the wrong patch.

So whilst we still could benefit from clearing the dirty flags after
setting them, I've just wasted a day.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 09/17] drm/i915: Don't use atomics for pg_dirty_rings

[Daniel Vetter]

It's already protected by the bkl^Wdev->struct_mutex. While at it
realign some related code.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_context.c    | 16 ++++++++--------
 drivers/gpu/drm/i915/i915_gem_execbuffer.c |  5 ++---
 2 files changed, 10 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 6b0962db2cf7..5a47eb5e3c5d 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -578,11 +578,9 @@ static inline bool should_skip_switch(struct intel_engine_cs *ring,
 	if (to->remap_slice)
 		return false;
 
-	if (to->ppgtt) {
-		if (from == to && !test_bit(ring->id,
-				&to->ppgtt->pd_dirty_rings))
-			return true;
-	}
+	if (to->ppgtt && from == to &&
+	    !(intel_ring_flag(ring) & to->ppgtt->pd_dirty_rings))
+		return true;
 
 	return false;
 }
@@ -668,7 +666,7 @@ static int do_switch(struct intel_engine_cs *ring,
 			goto unpin_out;
 
 		/* Doing a PD load always reloads the page dirs */
-		clear_bit(ring->id, &to->ppgtt->pd_dirty_rings);
+		to->ppgtt->pd_dirty_rings &= ~intel_ring_flag(ring);
 	}
 
 	if (ring != &dev_priv->ring[RCS]) {
@@ -696,12 +694,14 @@ static int do_switch(struct intel_engine_cs *ring,
 		 * space. This means we must enforce that a page table load
 		 * occur when this occurs. */
 	} else if (to->ppgtt &&
-			test_and_clear_bit(ring->id, &to->ppgtt->pd_dirty_rings))
+		   (intel_ring_flag(ring) & to->ppgtt->pd_dirty_rings)) {
 		hw_flags |= MI_FORCE_RESTORE;
+		to->ppgtt->pd_dirty_rings &= ~intel_ring_flag(ring);
+	}
 
 	/* We should never emit switch_mm more than once */
 	WARN_ON(needs_pd_load_pre(ring, to) &&
-			needs_pd_load_post(ring, to, hw_flags));
+		needs_pd_load_post(ring, to, hw_flags));
 
 	ret = mi_set_context(ring, to, hw_flags);
 	if (ret)
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 1d5badf1b887..0b06c6de27de 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -1247,9 +1247,8 @@ i915_gem_ringbuffer_submission(struct drm_device *dev, struct drm_file *file,
 	if (ret)
 		goto error;
 
-	if (ctx->ppgtt)
-		WARN(ctx->ppgtt->pd_dirty_rings & (1<<ring->id),
-			"%s didn't clear reload\n", ring->name);
+	WARN(ctx->ppgtt && ctx->ppgtt->pd_dirty_rings & (1<<ring->id),
+	     "%s didn't clear reload\n", ring->name);
 
 	instp_mode = args->flags & I915_EXEC_CONSTANTS_MASK;
 	instp_mask = I915_EXEC_CONSTANTS_MASK;
-- 
2.1.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 09/17] drm/i915: Don't use atomics for pg_dirty_rings

[Mika Kuoppala]

Daniel Vetter <daniel.vetter@ffwll.ch> writes:

> It's already protected by the bkl^Wdev->struct_mutex. While at it
> realign some related code.
>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>

Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com>

> ---
>  drivers/gpu/drm/i915/i915_gem_context.c    | 16 ++++++++--------
>  drivers/gpu/drm/i915/i915_gem_execbuffer.c |  5 ++---
>  2 files changed, 10 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index 6b0962db2cf7..5a47eb5e3c5d 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -578,11 +578,9 @@ static inline bool should_skip_switch(struct intel_engine_cs *ring,
>  	if (to->remap_slice)
>  		return false;
>  
> -	if (to->ppgtt) {
> -		if (from == to && !test_bit(ring->id,
> -				&to->ppgtt->pd_dirty_rings))
> -			return true;
> -	}
> +	if (to->ppgtt && from == to &&
> +	    !(intel_ring_flag(ring) & to->ppgtt->pd_dirty_rings))
> +		return true;
>  
>  	return false;
>  }
> @@ -668,7 +666,7 @@ static int do_switch(struct intel_engine_cs *ring,
>  			goto unpin_out;
>  
>  		/* Doing a PD load always reloads the page dirs */
> -		clear_bit(ring->id, &to->ppgtt->pd_dirty_rings);
> +		to->ppgtt->pd_dirty_rings &= ~intel_ring_flag(ring);
>  	}
>  
>  	if (ring != &dev_priv->ring[RCS]) {
> @@ -696,12 +694,14 @@ static int do_switch(struct intel_engine_cs *ring,
>  		 * space. This means we must enforce that a page table load
>  		 * occur when this occurs. */
>  	} else if (to->ppgtt &&
> -			test_and_clear_bit(ring->id, &to->ppgtt->pd_dirty_rings))
> +		   (intel_ring_flag(ring) & to->ppgtt->pd_dirty_rings)) {
>  		hw_flags |= MI_FORCE_RESTORE;
> +		to->ppgtt->pd_dirty_rings &= ~intel_ring_flag(ring);
> +	}
>  
>  	/* We should never emit switch_mm more than once */
>  	WARN_ON(needs_pd_load_pre(ring, to) &&
> -			needs_pd_load_post(ring, to, hw_flags));
> +		needs_pd_load_post(ring, to, hw_flags));
>  
>  	ret = mi_set_context(ring, to, hw_flags);
>  	if (ret)
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index 1d5badf1b887..0b06c6de27de 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -1247,9 +1247,8 @@ i915_gem_ringbuffer_submission(struct drm_device *dev, struct drm_file *file,
>  	if (ret)
>  		goto error;
>  
> -	if (ctx->ppgtt)
> -		WARN(ctx->ppgtt->pd_dirty_rings & (1<<ring->id),
> -			"%s didn't clear reload\n", ring->name);
> +	WARN(ctx->ppgtt && ctx->ppgtt->pd_dirty_rings & (1<<ring->id),
> +	     "%s didn't clear reload\n", ring->name);
>  
>  	instp_mode = args->flags & I915_EXEC_CONSTANTS_MASK;
>  	instp_mask = I915_EXEC_CONSTANTS_MASK;
> -- 
> 2.1.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 10/17] drm/i915: Remove misleading comment around bind_to_vm

[Daniel Vetter]

It's true that we might need to context switch, but both the signalling
and implementation of the same are a few source files away. Remove it.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_gem.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 4578772c5509..10e873c8957f 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4183,10 +4183,6 @@ i915_gem_object_do_pin(struct drm_i915_gem_object *obj,
 
 	bound = vma ? vma->bound : 0;
 	if (vma == NULL || !drm_mm_node_allocated(&vma->node)) {
-		/* In true PPGTT, bind has possibly changed PDEs, which
-		 * means we must do a context switch before the GPU can
-		 * accurately read some of the VMAs.
-		 */
 		vma = i915_gem_object_bind_to_vm(obj, vm, ggtt_view, alignment,
 						 flags);
 		if (IS_ERR(vma))
-- 
2.1.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 10/17] drm/i915: Remove misleading comment around bind_to_vm

[Mika Kuoppala]

Daniel Vetter <daniel.vetter@ffwll.ch> writes:

> It's true that we might need to context switch, but both the signalling
> and implementation of the same are a few source files away. Remove it.
>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com>

> ---
>  drivers/gpu/drm/i915/i915_gem.c | 4 ----
>  1 file changed, 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 4578772c5509..10e873c8957f 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -4183,10 +4183,6 @@ i915_gem_object_do_pin(struct drm_i915_gem_object *obj,
>  
>  	bound = vma ? vma->bound : 0;
>  	if (vma == NULL || !drm_mm_node_allocated(&vma->node)) {
> -		/* In true PPGTT, bind has possibly changed PDEs, which
> -		 * means we must do a context switch before the GPU can
> -		 * accurately read some of the VMAs.
> -		 */
>  		vma = i915_gem_object_bind_to_vm(obj, vm, ggtt_view, alignment,
>  						 flags);
>  		if (IS_ERR(vma))
> -- 
> 2.1.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 11/17] drm/i915: Fix up the vma aliasing ppgtt binding

[Daniel Vetter]

Currently we have the problem that the decision whether ptes need to
be (re)written is splattered all over the codebase. Move all that into
i915_vma_bind. This needs a few changes:
- Just reuse the PIN_* flags for i915_vma_bind and do the conversion
  to vma->bound in there to avoid duplicating the conversion code all
  over.
- We need to make binding for EXECBUF (i.e. pick aliasing ppgtt if
  around) explicit, add PIN_EXECBUF for that.
- Two callers want to update ptes, give them a PIN_UPDATE for that.

Of course we still want to avoid double-binding, but that should be
taken care of:
- A ppgtt vma will only ever see PIN_EXECBUF, so no issue with
  double-binding.
- A ggtt vma with aliasing ppgtt needs both types of binding, and we
  track that properly now.
- A ggtt vma without aliasing ppgtt could be bound twice. In the
  lower-level ->bind_vma functions hence unconditionally set
  GLOBAL_BIND when writing the ggtt ptes.

There's still a bit room for cleanup, but that's for follow-up
patches.

v2: Fixup fumbles.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h            | 11 +++--
 drivers/gpu/drm/i915/i915_gem.c            | 11 ++---
 drivers/gpu/drm/i915/i915_gem_execbuffer.c |  7 ++--
 drivers/gpu/drm/i915/i915_gem_gtt.c        | 65 ++++++++++++------------------
 4 files changed, 40 insertions(+), 54 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 47be4a57e6a9..eaeae041fed9 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2640,10 +2640,13 @@ void i915_init_vm(struct drm_i915_private *dev_priv,
 void i915_gem_free_object(struct drm_gem_object *obj);
 void i915_gem_vma_destroy(struct i915_vma *vma);
 
-#define PIN_MAPPABLE 0x1
-#define PIN_NONBLOCK 0x2
-#define PIN_GLOBAL 0x4
-#define PIN_OFFSET_BIAS 0x8
+/* Flags used by pin/bind&friends. */
+#define PIN_MAPPABLE	(1<<0)
+#define PIN_NONBLOCK	(1<<1)
+#define PIN_GLOBAL	(1<<2)
+#define PIN_OFFSET_BIAS	(1<<3)
+#define PIN_EXECBUF	(1<<4)
+#define PIN_UPDATE	(1<<5)
 #define PIN_OFFSET_MASK (~4095)
 int __must_check
 i915_gem_object_pin(struct drm_i915_gem_object *obj,
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 10e873c8957f..047629b08697 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3557,8 +3557,7 @@ search_free:
 		goto err_remove_node;
 
 	trace_i915_vma_bind(vma, flags);
-	ret = i915_vma_bind(vma, obj->cache_level,
-			    flags & PIN_GLOBAL ? GLOBAL_BIND : 0);
+	ret = i915_vma_bind(vma, obj->cache_level, flags);
 	if (ret)
 		goto err_finish_gtt;
 
@@ -3784,7 +3783,7 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
 		list_for_each_entry(vma, &obj->vma_list, vma_link)
 			if (drm_mm_node_allocated(&vma->node)) {
 				ret = i915_vma_bind(vma, cache_level,
-						    vma->bound & GLOBAL_BIND);
+						    PIN_UPDATE);
 				if (ret)
 					return ret;
 			}
@@ -4187,10 +4186,8 @@ i915_gem_object_do_pin(struct drm_i915_gem_object *obj,
 						 flags);
 		if (IS_ERR(vma))
 			return PTR_ERR(vma);
-	}
-
-	if (flags & PIN_GLOBAL && !(vma->bound & GLOBAL_BIND)) {
-		ret = i915_vma_bind(vma, obj->cache_level, GLOBAL_BIND);
+	} else {
+		ret = i915_vma_bind(vma, obj->cache_level, flags);
 		if (ret)
 			return ret;
 	}
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 0b06c6de27de..f005f3151147 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -400,10 +400,9 @@ i915_gem_execbuffer_relocate_entry(struct drm_i915_gem_object *obj,
 	 * pipe_control writes because the gpu doesn't properly redirect them
 	 * through the ppgtt for non_secure batchbuffers. */
 	if (unlikely(IS_GEN6(dev) &&
-	    reloc->write_domain == I915_GEM_DOMAIN_INSTRUCTION &&
-	    !(target_vma->bound & GLOBAL_BIND))) {
+	    reloc->write_domain == I915_GEM_DOMAIN_INSTRUCTION)) {
 		ret = i915_vma_bind(target_vma, target_i915_obj->cache_level,
-				    GLOBAL_BIND);
+				    PIN_GLOBAL);
 		if (WARN_ONCE(ret, "Unexpected failure to bind target VMA!"))
 			return ret;
 	}
@@ -585,7 +584,7 @@ i915_gem_execbuffer_reserve_vma(struct i915_vma *vma,
 	uint64_t flags;
 	int ret;
 
-	flags = 0;
+	flags = PIN_EXECBUF;
 	if (!drm_mm_node_allocated(&vma->node)) {
 		if (entry->flags & __EXEC_OBJECT_NEEDS_MAP)
 			flags |= PIN_GLOBAL | PIN_MAPPABLE;
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 4e2caef83772..44827090a13f 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -1748,15 +1748,7 @@ void i915_gem_restore_gtt_mappings(struct drm_device *dev)
 			continue;
 
 		i915_gem_clflush_object(obj, obj->pin_display);
-		/* The bind_vma code tries to be smart about tracking mappings.
-		 * Unfortunately above, we've just wiped out the mappings
-		 * without telling our object about it. So we need to fake it.
-		 *
-		 * Bind is not expected to fail since this is only called on
-		 * resume and assumption is all requirements exist already.
-		 */
-		vma->bound &= ~GLOBAL_BIND;
-		WARN_ON(i915_vma_bind(vma, obj->cache_level, GLOBAL_BIND));
+		WARN_ON(i915_vma_bind(vma, obj->cache_level, PIN_UPDATE));
 	}
 
 
@@ -1957,7 +1949,8 @@ static void i915_ggtt_bind_vma(struct i915_vma *vma,
 
 	BUG_ON(!i915_is_ggtt(vma->vm));
 	intel_gtt_insert_sg_entries(vma->ggtt_view.pages, entry, flags);
-	vma->bound = GLOBAL_BIND;
+
+	vma->bound |= GLOBAL_BIND;
 }
 
 static void i915_ggtt_clear_range(struct i915_address_space *vm,
@@ -1976,7 +1969,6 @@ static void i915_ggtt_unbind_vma(struct i915_vma *vma)
 	const unsigned int size = vma->obj->base.size >> PAGE_SHIFT;
 
 	BUG_ON(!i915_is_ggtt(vma->vm));
-	vma->bound = 0;
 	intel_gtt_clear_range(first, size);
 }
 
@@ -1997,35 +1989,19 @@ static void ggtt_bind_vma(struct i915_vma *vma,
 	if (i915_is_ggtt(vma->vm))
 		pages = vma->ggtt_view.pages;
 
-	/* If there is no aliasing PPGTT, or the caller needs a global mapping,
-	 * or we have a global mapping already but the cacheability flags have
-	 * changed, set the global PTEs.
-	 *
-	 * If there is an aliasing PPGTT it is anecdotally faster, so use that
-	 * instead if none of the above hold true.
-	 *
-	 * NB: A global mapping should only be needed for special regions like
-	 * "gtt mappable", SNB errata, or if specified via special execbuf
-	 * flags. At all other times, the GPU will use the aliasing PPGTT.
-	 */
 	if (!dev_priv->mm.aliasing_ppgtt || flags & GLOBAL_BIND) {
-		if (!(vma->bound & GLOBAL_BIND) ||
-		    (cache_level != obj->cache_level)) {
-			vma->vm->insert_entries(vma->vm, pages,
-						vma->node.start,
-						cache_level, pte_flags);
-			vma->bound |= GLOBAL_BIND;
-		}
+		vma->vm->insert_entries(vma->vm, pages,
+					vma->node.start,
+					cache_level, pte_flags);
+
+		vma->bound |= GLOBAL_BIND;
 	}
 
-	if (dev_priv->mm.aliasing_ppgtt &&
-	    (!(vma->bound & LOCAL_BIND) ||
-	     (cache_level != obj->cache_level))) {
+	if (dev_priv->mm.aliasing_ppgtt && flags & LOCAL_BIND) {
 		struct i915_hw_ppgtt *appgtt = dev_priv->mm.aliasing_ppgtt;
 		appgtt->base.insert_entries(&appgtt->base, pages,
 					    vma->node.start,
 					    cache_level, pte_flags);
-		vma->bound |= LOCAL_BIND;
 	}
 }
 
@@ -2040,16 +2016,14 @@ static void ggtt_unbind_vma(struct i915_vma *vma)
 				     vma->node.start,
 				     obj->base.size,
 				     true);
-		vma->bound &= ~GLOBAL_BIND;
 	}
 
-	if (vma->bound & LOCAL_BIND) {
+	if (dev_priv->mm.aliasing_ppgtt && vma->bound & LOCAL_BIND) {
 		struct i915_hw_ppgtt *appgtt = dev_priv->mm.aliasing_ppgtt;
 		appgtt->base.clear_range(&appgtt->base,
 					 vma->node.start,
 					 obj->base.size,
 					 true);
-		vma->bound &= ~LOCAL_BIND;
 	}
 }
 
@@ -2839,6 +2813,7 @@ i915_get_ggtt_vma_pages(struct i915_vma *vma)
 int i915_vma_bind(struct i915_vma *vma, enum i915_cache_level cache_level,
 		  u32 flags)
 {
+	u32 bind_flags = 0;
 	int ret;
 
 	if (vma->vm->allocate_va_range) {
@@ -2855,12 +2830,24 @@ int i915_vma_bind(struct i915_vma *vma, enum i915_cache_level cache_level,
 
 	if (i915_is_ggtt(vma->vm)) {
 		ret = i915_get_ggtt_vma_pages(vma);
-
 		if (ret)
-			return ret;
+			return 0;
 	}
 
-	vma->vm->bind_vma(vma, cache_level, flags);
+	if (flags & PIN_GLOBAL)
+		bind_flags |= GLOBAL_BIND;
+	if (flags & PIN_EXECBUF)
+		bind_flags |= LOCAL_BIND;
+
+	if (flags & PIN_UPDATE)
+		bind_flags |= vma->bound;
+	else
+		bind_flags &= ~vma->bound;
+
+	if (bind_flags)
+		vma->vm->bind_vma(vma, cache_level, bind_flags);
+
+	vma->bound |= bind_flags;
 
 	return 0;
 }
-- 
2.1.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 11/17] drm/i915: Fix up the vma aliasing ppgtt binding

[Chris Wilson]

On Tue, Apr 14, 2015 at 05:35:21PM +0200, Daniel Vetter wrote:
> Currently we have the problem that the decision whether ptes need to
> be (re)written is splattered all over the codebase. Move all that into
> i915_vma_bind. This needs a few changes:
> - Just reuse the PIN_* flags for i915_vma_bind and do the conversion
>   to vma->bound in there to avoid duplicating the conversion code all
>   over.
> - We need to make binding for EXECBUF (i.e. pick aliasing ppgtt if
>   around) explicit, add PIN_EXECBUF for that.

I am in favour of making the PIN_GLOBAL | PIN_LOCAL explicit, but
PIN_EXECBUF doesn't seem descriptive of what happens, nor why it should
be execbuf specific. Just use PIN_LOCAL with the execbuf oring in
PIN_GLOBAL as it needs for workarounds + relocations.
-chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 11/17] drm/i915: Fix up the vma aliasing ppgtt binding

[Daniel Vetter]

On Wed, Apr 15, 2015 at 11:47:02AM +0100, Chris Wilson wrote:
> On Tue, Apr 14, 2015 at 05:35:21PM +0200, Daniel Vetter wrote:
> > Currently we have the problem that the decision whether ptes need to
> > be (re)written is splattered all over the codebase. Move all that into
> > i915_vma_bind. This needs a few changes:
> > - Just reuse the PIN_* flags for i915_vma_bind and do the conversion
> >   to vma->bound in there to avoid duplicating the conversion code all
> >   over.
> > - We need to make binding for EXECBUF (i.e. pick aliasing ppgtt if
> >   around) explicit, add PIN_EXECBUF for that.
> 
> I am in favour of making the PIN_GLOBAL | PIN_LOCAL explicit, but
> PIN_EXECBUF doesn't seem descriptive of what happens, nor why it should
> be execbuf specific. Just use PIN_LOCAL with the execbuf oring in
> PIN_GLOBAL as it needs for workarounds + relocations.

Well I wasnt' too happy with LOCAL_BIND tbh since that sounds awfully
close to gen1 local memory ;-) My idea behind PIN_EXECBUF (and hiding the
ggtt vs. aliasing ppgtt binding in the low-level code) is that imo
conceptually global vs. aliasing ppgtt is just a pte bit with crazy
storage. We might as well have 2 bits to control access for priviledged
clients and for unpriviledge GT access, and in that case it would make
sense to hide that in the lower levels. ggtt+aliasing ppgtt isnt' anything
else, except for a rather peculariar storage format of these two bits.

Essentially we have:
- PIN_GLOBAL: Please set up pte so that system agent, display and
  priviledged GT can access it.
- PIN_EXECBUF: Please set up pte so that unpriveledged GT access is
  possible.

Maybe PIN_UNPRIV_GT or something else would be better? Pushing the
decision of how that access control is down up into the execbuf code is
imo a layering violation and exactly the kind of trouble that imo has lead
to rebinding or too-much-binding bugs we've had. With my approach it's all
together in the low-level gtt binding code.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 11/17] drm/i915: Fix up the vma aliasing ppgtt binding

[Chris Wilson]

On Thu, Apr 16, 2015 at 10:01:31AM +0200, Daniel Vetter wrote:
> On Wed, Apr 15, 2015 at 11:47:02AM +0100, Chris Wilson wrote:
> > On Tue, Apr 14, 2015 at 05:35:21PM +0200, Daniel Vetter wrote:
> > > Currently we have the problem that the decision whether ptes need to
> > > be (re)written is splattered all over the codebase. Move all that into
> > > i915_vma_bind. This needs a few changes:
> > > - Just reuse the PIN_* flags for i915_vma_bind and do the conversion
> > >   to vma->bound in there to avoid duplicating the conversion code all
> > >   over.
> > > - We need to make binding for EXECBUF (i.e. pick aliasing ppgtt if
> > >   around) explicit, add PIN_EXECBUF for that.
> > 
> > I am in favour of making the PIN_GLOBAL | PIN_LOCAL explicit, but
> > PIN_EXECBUF doesn't seem descriptive of what happens, nor why it should
> > be execbuf specific. Just use PIN_LOCAL with the execbuf oring in
> > PIN_GLOBAL as it needs for workarounds + relocations.
> 
> Well I wasnt' too happy with LOCAL_BIND tbh since that sounds awfully
> close to gen1 local memory ;-) My idea behind PIN_EXECBUF (and hiding the
> ggtt vs. aliasing ppgtt binding in the low-level code) is that imo
> conceptually global vs. aliasing ppgtt is just a pte bit with crazy
> storage. We might as well have 2 bits to control access for priviledged
> clients and for unpriviledge GT access, and in that case it would make
> sense to hide that in the lower levels. ggtt+aliasing ppgtt isnt' anything
> else, except for a rather peculariar storage format of these two bits.
> 
> Essentially we have:
> - PIN_GLOBAL: Please set up pte so that system agent, display and
>   priviledged GT can access it.
> - PIN_EXECBUF: Please set up pte so that unpriveledged GT access is
>   possible.
> 
> Maybe PIN_UNPRIV_GT or something else would be better? Pushing the
> decision of how that access control is down up into the execbuf code is
> imo a layering violation and exactly the kind of trouble that imo has lead
> to rebinding or too-much-binding bugs we've had. With my approach it's all
> together in the low-level gtt binding code.

Yeah, I'm just arguing that EXECBUF doesn't tell me anything at all
about what is happening. It is just not descriptive enough and is liable
to change meanings and end up in confusion.

How about PIN_GLOBAL (aka PIN_SYSTEM) and PIN_USER?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 11/17] drm/i915: Fix up the vma aliasing ppgtt binding

[Daniel Vetter]

On Thu, Apr 16, 2015 at 09:07:57AM +0100, Chris Wilson wrote:
> On Thu, Apr 16, 2015 at 10:01:31AM +0200, Daniel Vetter wrote:
> > On Wed, Apr 15, 2015 at 11:47:02AM +0100, Chris Wilson wrote:
> > > On Tue, Apr 14, 2015 at 05:35:21PM +0200, Daniel Vetter wrote:
> > > > Currently we have the problem that the decision whether ptes need to
> > > > be (re)written is splattered all over the codebase. Move all that into
> > > > i915_vma_bind. This needs a few changes:
> > > > - Just reuse the PIN_* flags for i915_vma_bind and do the conversion
> > > >   to vma->bound in there to avoid duplicating the conversion code all
> > > >   over.
> > > > - We need to make binding for EXECBUF (i.e. pick aliasing ppgtt if
> > > >   around) explicit, add PIN_EXECBUF for that.
> > > 
> > > I am in favour of making the PIN_GLOBAL | PIN_LOCAL explicit, but
> > > PIN_EXECBUF doesn't seem descriptive of what happens, nor why it should
> > > be execbuf specific. Just use PIN_LOCAL with the execbuf oring in
> > > PIN_GLOBAL as it needs for workarounds + relocations.
> > 
> > Well I wasnt' too happy with LOCAL_BIND tbh since that sounds awfully
> > close to gen1 local memory ;-) My idea behind PIN_EXECBUF (and hiding the
> > ggtt vs. aliasing ppgtt binding in the low-level code) is that imo
> > conceptually global vs. aliasing ppgtt is just a pte bit with crazy
> > storage. We might as well have 2 bits to control access for priviledged
> > clients and for unpriviledge GT access, and in that case it would make
> > sense to hide that in the lower levels. ggtt+aliasing ppgtt isnt' anything
> > else, except for a rather peculariar storage format of these two bits.
> > 
> > Essentially we have:
> > - PIN_GLOBAL: Please set up pte so that system agent, display and
> >   priviledged GT can access it.
> > - PIN_EXECBUF: Please set up pte so that unpriveledged GT access is
> >   possible.
> > 
> > Maybe PIN_UNPRIV_GT or something else would be better? Pushing the
> > decision of how that access control is down up into the execbuf code is
> > imo a layering violation and exactly the kind of trouble that imo has lead
> > to rebinding or too-much-binding bugs we've had. With my approach it's all
> > together in the low-level gtt binding code.
> 
> Yeah, I'm just arguing that EXECBUF doesn't tell me anything at all
> about what is happening. It is just not descriptive enough and is liable
> to change meanings and end up in confusion.
> 
> How about PIN_GLOBAL (aka PIN_SYSTEM) and PIN_USER?

PIN_USER sounds really good indeed. Will change.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH] drm/i915: Fix up the vma aliasing ppgtt binding

[Daniel Vetter]

Currently we have the problem that the decision whether ptes need to
be (re)written is splattered all over the codebase. Move all that into
i915_vma_bind. This needs a few changes:
- Just reuse the PIN_* flags for i915_vma_bind and do the conversion
  to vma->bound in there to avoid duplicating the conversion code all
  over.
- We need to make binding for EXECBUF (i.e. pick aliasing ppgtt if
  around) explicit, add PIN_USER for that.
- Two callers want to update ptes, give them a PIN_UPDATE for that.

Of course we still want to avoid double-binding, but that should be
taken care of:
- A ppgtt vma will only ever see PIN_USER, so no issue with
  double-binding.
- A ggtt vma with aliasing ppgtt needs both types of binding, and we
  track that properly now.
- A ggtt vma without aliasing ppgtt could be bound twice. In the
  lower-level ->bind_vma functions hence unconditionally set
  GLOBAL_BIND when writing the ggtt ptes.

There's still a bit room for cleanup, but that's for follow-up
patches.

v2: Fixup fumbles.

v3: s/PIN_EXECBUF/PIN_USER/ for clearer meaning, suggested by Chris.

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h            | 11 +++--
 drivers/gpu/drm/i915/i915_gem.c            | 11 ++---
 drivers/gpu/drm/i915/i915_gem_execbuffer.c |  7 ++--
 drivers/gpu/drm/i915/i915_gem_gtt.c        | 65 ++++++++++++------------------
 4 files changed, 40 insertions(+), 54 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 47be4a57e6a9..80afbe3ad669 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2640,10 +2640,13 @@ void i915_init_vm(struct drm_i915_private *dev_priv,
 void i915_gem_free_object(struct drm_gem_object *obj);
 void i915_gem_vma_destroy(struct i915_vma *vma);
 
-#define PIN_MAPPABLE 0x1
-#define PIN_NONBLOCK 0x2
-#define PIN_GLOBAL 0x4
-#define PIN_OFFSET_BIAS 0x8
+/* Flags used by pin/bind&friends. */
+#define PIN_MAPPABLE	(1<<0)
+#define PIN_NONBLOCK	(1<<1)
+#define PIN_GLOBAL	(1<<2)
+#define PIN_OFFSET_BIAS	(1<<3)
+#define PIN_USER	(1<<4)
+#define PIN_UPDATE	(1<<5)
 #define PIN_OFFSET_MASK (~4095)
 int __must_check
 i915_gem_object_pin(struct drm_i915_gem_object *obj,
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 10e873c8957f..047629b08697 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3557,8 +3557,7 @@ search_free:
 		goto err_remove_node;
 
 	trace_i915_vma_bind(vma, flags);
-	ret = i915_vma_bind(vma, obj->cache_level,
-			    flags & PIN_GLOBAL ? GLOBAL_BIND : 0);
+	ret = i915_vma_bind(vma, obj->cache_level, flags);
 	if (ret)
 		goto err_finish_gtt;
 
@@ -3784,7 +3783,7 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
 		list_for_each_entry(vma, &obj->vma_list, vma_link)
 			if (drm_mm_node_allocated(&vma->node)) {
 				ret = i915_vma_bind(vma, cache_level,
-						    vma->bound & GLOBAL_BIND);
+						    PIN_UPDATE);
 				if (ret)
 					return ret;
 			}
@@ -4187,10 +4186,8 @@ i915_gem_object_do_pin(struct drm_i915_gem_object *obj,
 						 flags);
 		if (IS_ERR(vma))
 			return PTR_ERR(vma);
-	}
-
-	if (flags & PIN_GLOBAL && !(vma->bound & GLOBAL_BIND)) {
-		ret = i915_vma_bind(vma, obj->cache_level, GLOBAL_BIND);
+	} else {
+		ret = i915_vma_bind(vma, obj->cache_level, flags);
 		if (ret)
 			return ret;
 	}
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 7f69aa820458..cfdc8c6073aa 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -400,10 +400,9 @@ i915_gem_execbuffer_relocate_entry(struct drm_i915_gem_object *obj,
 	 * pipe_control writes because the gpu doesn't properly redirect them
 	 * through the ppgtt for non_secure batchbuffers. */
 	if (unlikely(IS_GEN6(dev) &&
-	    reloc->write_domain == I915_GEM_DOMAIN_INSTRUCTION &&
-	    !(target_vma->bound & GLOBAL_BIND))) {
+	    reloc->write_domain == I915_GEM_DOMAIN_INSTRUCTION)) {
 		ret = i915_vma_bind(target_vma, target_i915_obj->cache_level,
-				    GLOBAL_BIND);
+				    PIN_GLOBAL);
 		if (WARN_ONCE(ret, "Unexpected failure to bind target VMA!"))
 			return ret;
 	}
@@ -585,7 +584,7 @@ i915_gem_execbuffer_reserve_vma(struct i915_vma *vma,
 	uint64_t flags;
 	int ret;
 
-	flags = 0;
+	flags = PIN_USER;
 	if (entry->flags & EXEC_OBJECT_NEEDS_GTT)
 		flags |= PIN_GLOBAL;
 
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 4e2caef83772..9e06180e206f 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -1748,15 +1748,7 @@ void i915_gem_restore_gtt_mappings(struct drm_device *dev)
 			continue;
 
 		i915_gem_clflush_object(obj, obj->pin_display);
-		/* The bind_vma code tries to be smart about tracking mappings.
-		 * Unfortunately above, we've just wiped out the mappings
-		 * without telling our object about it. So we need to fake it.
-		 *
-		 * Bind is not expected to fail since this is only called on
-		 * resume and assumption is all requirements exist already.
-		 */
-		vma->bound &= ~GLOBAL_BIND;
-		WARN_ON(i915_vma_bind(vma, obj->cache_level, GLOBAL_BIND));
+		WARN_ON(i915_vma_bind(vma, obj->cache_level, PIN_UPDATE));
 	}
 
 
@@ -1957,7 +1949,8 @@ static void i915_ggtt_bind_vma(struct i915_vma *vma,
 
 	BUG_ON(!i915_is_ggtt(vma->vm));
 	intel_gtt_insert_sg_entries(vma->ggtt_view.pages, entry, flags);
-	vma->bound = GLOBAL_BIND;
+
+	vma->bound |= GLOBAL_BIND;
 }
 
 static void i915_ggtt_clear_range(struct i915_address_space *vm,
@@ -1976,7 +1969,6 @@ static void i915_ggtt_unbind_vma(struct i915_vma *vma)
 	const unsigned int size = vma->obj->base.size >> PAGE_SHIFT;
 
 	BUG_ON(!i915_is_ggtt(vma->vm));
-	vma->bound = 0;
 	intel_gtt_clear_range(first, size);
 }
 
@@ -1997,35 +1989,19 @@ static void ggtt_bind_vma(struct i915_vma *vma,
 	if (i915_is_ggtt(vma->vm))
 		pages = vma->ggtt_view.pages;
 
-	/* If there is no aliasing PPGTT, or the caller needs a global mapping,
-	 * or we have a global mapping already but the cacheability flags have
-	 * changed, set the global PTEs.
-	 *
-	 * If there is an aliasing PPGTT it is anecdotally faster, so use that
-	 * instead if none of the above hold true.
-	 *
-	 * NB: A global mapping should only be needed for special regions like
-	 * "gtt mappable", SNB errata, or if specified via special execbuf
-	 * flags. At all other times, the GPU will use the aliasing PPGTT.
-	 */
 	if (!dev_priv->mm.aliasing_ppgtt || flags & GLOBAL_BIND) {
-		if (!(vma->bound & GLOBAL_BIND) ||
-		    (cache_level != obj->cache_level)) {
-			vma->vm->insert_entries(vma->vm, pages,
-						vma->node.start,
-						cache_level, pte_flags);
-			vma->bound |= GLOBAL_BIND;
-		}
+		vma->vm->insert_entries(vma->vm, pages,
+					vma->node.start,
+					cache_level, pte_flags);
+
+		vma->bound |= GLOBAL_BIND;
 	}
 
-	if (dev_priv->mm.aliasing_ppgtt &&
-	    (!(vma->bound & LOCAL_BIND) ||
-	     (cache_level != obj->cache_level))) {
+	if (dev_priv->mm.aliasing_ppgtt && flags & LOCAL_BIND) {
 		struct i915_hw_ppgtt *appgtt = dev_priv->mm.aliasing_ppgtt;
 		appgtt->base.insert_entries(&appgtt->base, pages,
 					    vma->node.start,
 					    cache_level, pte_flags);
-		vma->bound |= LOCAL_BIND;
 	}
 }
 
@@ -2040,16 +2016,14 @@ static void ggtt_unbind_vma(struct i915_vma *vma)
 				     vma->node.start,
 				     obj->base.size,
 				     true);
-		vma->bound &= ~GLOBAL_BIND;
 	}
 
-	if (vma->bound & LOCAL_BIND) {
+	if (dev_priv->mm.aliasing_ppgtt && vma->bound & LOCAL_BIND) {
 		struct i915_hw_ppgtt *appgtt = dev_priv->mm.aliasing_ppgtt;
 		appgtt->base.clear_range(&appgtt->base,
 					 vma->node.start,
 					 obj->base.size,
 					 true);
-		vma->bound &= ~LOCAL_BIND;
 	}
 }
 
@@ -2839,6 +2813,7 @@ i915_get_ggtt_vma_pages(struct i915_vma *vma)
 int i915_vma_bind(struct i915_vma *vma, enum i915_cache_level cache_level,
 		  u32 flags)
 {
+	u32 bind_flags = 0;
 	int ret;
 
 	if (vma->vm->allocate_va_range) {
@@ -2855,12 +2830,24 @@ int i915_vma_bind(struct i915_vma *vma, enum i915_cache_level cache_level,
 
 	if (i915_is_ggtt(vma->vm)) {
 		ret = i915_get_ggtt_vma_pages(vma);
-
 		if (ret)
-			return ret;
+			return 0;
 	}
 
-	vma->vm->bind_vma(vma, cache_level, flags);
+	if (flags & PIN_GLOBAL)
+		bind_flags |= GLOBAL_BIND;
+	if (flags & PIN_USER)
+		bind_flags |= LOCAL_BIND;
+
+	if (flags & PIN_UPDATE)
+		bind_flags |= vma->bound;
+	else
+		bind_flags &= ~vma->bound;
+
+	if (bind_flags)
+		vma->vm->bind_vma(vma, cache_level, bind_flags);
+
+	vma->bound |= bind_flags;
 
 	return 0;
 }
-- 
1.9.3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Fix up the vma aliasing ppgtt binding

[Mika Kuoppala]

Daniel Vetter <daniel.vetter@ffwll.ch> writes:

> Currently we have the problem that the decision whether ptes need to
> be (re)written is splattered all over the codebase. Move all that into
> i915_vma_bind. This needs a few changes:
> - Just reuse the PIN_* flags for i915_vma_bind and do the conversion
>   to vma->bound in there to avoid duplicating the conversion code all
>   over.
> - We need to make binding for EXECBUF (i.e. pick aliasing ppgtt if
>   around) explicit, add PIN_USER for that.
> - Two callers want to update ptes, give them a PIN_UPDATE for that.
>
> Of course we still want to avoid double-binding, but that should be
> taken care of:
> - A ppgtt vma will only ever see PIN_USER, so no issue with
>   double-binding.
> - A ggtt vma with aliasing ppgtt needs both types of binding, and we
>   track that properly now.
> - A ggtt vma without aliasing ppgtt could be bound twice. In the
>   lower-level ->bind_vma functions hence unconditionally set
>   GLOBAL_BIND when writing the ggtt ptes.
>
> There's still a bit room for cleanup, but that's for follow-up
> patches.
>
> v2: Fixup fumbles.
>
> v3: s/PIN_EXECBUF/PIN_USER/ for clearer meaning, suggested by Chris.
>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>

Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com>

> ---
>  drivers/gpu/drm/i915/i915_drv.h            | 11 +++--
>  drivers/gpu/drm/i915/i915_gem.c            | 11 ++---
>  drivers/gpu/drm/i915/i915_gem_execbuffer.c |  7 ++--
>  drivers/gpu/drm/i915/i915_gem_gtt.c        | 65 ++++++++++++------------------
>  4 files changed, 40 insertions(+), 54 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 47be4a57e6a9..80afbe3ad669 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2640,10 +2640,13 @@ void i915_init_vm(struct drm_i915_private *dev_priv,
>  void i915_gem_free_object(struct drm_gem_object *obj);
>  void i915_gem_vma_destroy(struct i915_vma *vma);
>  
> -#define PIN_MAPPABLE 0x1
> -#define PIN_NONBLOCK 0x2
> -#define PIN_GLOBAL 0x4
> -#define PIN_OFFSET_BIAS 0x8
> +/* Flags used by pin/bind&friends. */
> +#define PIN_MAPPABLE	(1<<0)
> +#define PIN_NONBLOCK	(1<<1)
> +#define PIN_GLOBAL	(1<<2)
> +#define PIN_OFFSET_BIAS	(1<<3)
> +#define PIN_USER	(1<<4)
> +#define PIN_UPDATE	(1<<5)
>  #define PIN_OFFSET_MASK (~4095)
>  int __must_check
>  i915_gem_object_pin(struct drm_i915_gem_object *obj,
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 10e873c8957f..047629b08697 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -3557,8 +3557,7 @@ search_free:
>  		goto err_remove_node;
>  
>  	trace_i915_vma_bind(vma, flags);
> -	ret = i915_vma_bind(vma, obj->cache_level,
> -			    flags & PIN_GLOBAL ? GLOBAL_BIND : 0);
> +	ret = i915_vma_bind(vma, obj->cache_level, flags);
>  	if (ret)
>  		goto err_finish_gtt;
>  
> @@ -3784,7 +3783,7 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
>  		list_for_each_entry(vma, &obj->vma_list, vma_link)
>  			if (drm_mm_node_allocated(&vma->node)) {
>  				ret = i915_vma_bind(vma, cache_level,
> -						    vma->bound & GLOBAL_BIND);
> +						    PIN_UPDATE);
>  				if (ret)
>  					return ret;
>  			}
> @@ -4187,10 +4186,8 @@ i915_gem_object_do_pin(struct drm_i915_gem_object *obj,
>  						 flags);
>  		if (IS_ERR(vma))
>  			return PTR_ERR(vma);
> -	}
> -
> -	if (flags & PIN_GLOBAL && !(vma->bound & GLOBAL_BIND)) {
> -		ret = i915_vma_bind(vma, obj->cache_level, GLOBAL_BIND);
> +	} else {
> +		ret = i915_vma_bind(vma, obj->cache_level, flags);
>  		if (ret)
>  			return ret;
>  	}
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index 7f69aa820458..cfdc8c6073aa 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -400,10 +400,9 @@ i915_gem_execbuffer_relocate_entry(struct drm_i915_gem_object *obj,
>  	 * pipe_control writes because the gpu doesn't properly redirect them
>  	 * through the ppgtt for non_secure batchbuffers. */
>  	if (unlikely(IS_GEN6(dev) &&
> -	    reloc->write_domain == I915_GEM_DOMAIN_INSTRUCTION &&
> -	    !(target_vma->bound & GLOBAL_BIND))) {
> +	    reloc->write_domain == I915_GEM_DOMAIN_INSTRUCTION)) {
>  		ret = i915_vma_bind(target_vma, target_i915_obj->cache_level,
> -				    GLOBAL_BIND);
> +				    PIN_GLOBAL);
>  		if (WARN_ONCE(ret, "Unexpected failure to bind target VMA!"))
>  			return ret;
>  	}
> @@ -585,7 +584,7 @@ i915_gem_execbuffer_reserve_vma(struct i915_vma *vma,
>  	uint64_t flags;
>  	int ret;
>  
> -	flags = 0;
> +	flags = PIN_USER;
>  	if (entry->flags & EXEC_OBJECT_NEEDS_GTT)
>  		flags |= PIN_GLOBAL;
>  
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 4e2caef83772..9e06180e206f 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -1748,15 +1748,7 @@ void i915_gem_restore_gtt_mappings(struct drm_device *dev)
>  			continue;
>  
>  		i915_gem_clflush_object(obj, obj->pin_display);
> -		/* The bind_vma code tries to be smart about tracking mappings.
> -		 * Unfortunately above, we've just wiped out the mappings
> -		 * without telling our object about it. So we need to fake it.
> -		 *
> -		 * Bind is not expected to fail since this is only called on
> -		 * resume and assumption is all requirements exist already.
> -		 */
> -		vma->bound &= ~GLOBAL_BIND;
> -		WARN_ON(i915_vma_bind(vma, obj->cache_level, GLOBAL_BIND));
> +		WARN_ON(i915_vma_bind(vma, obj->cache_level, PIN_UPDATE));
>  	}
>  
>  
> @@ -1957,7 +1949,8 @@ static void i915_ggtt_bind_vma(struct i915_vma *vma,
>  
>  	BUG_ON(!i915_is_ggtt(vma->vm));
>  	intel_gtt_insert_sg_entries(vma->ggtt_view.pages, entry, flags);
> -	vma->bound = GLOBAL_BIND;
> +
> +	vma->bound |= GLOBAL_BIND;
>  }
>  
>  static void i915_ggtt_clear_range(struct i915_address_space *vm,
> @@ -1976,7 +1969,6 @@ static void i915_ggtt_unbind_vma(struct i915_vma *vma)
>  	const unsigned int size = vma->obj->base.size >> PAGE_SHIFT;
>  
>  	BUG_ON(!i915_is_ggtt(vma->vm));
> -	vma->bound = 0;
>  	intel_gtt_clear_range(first, size);
>  }
>  
> @@ -1997,35 +1989,19 @@ static void ggtt_bind_vma(struct i915_vma *vma,
>  	if (i915_is_ggtt(vma->vm))
>  		pages = vma->ggtt_view.pages;
>  
> -	/* If there is no aliasing PPGTT, or the caller needs a global mapping,
> -	 * or we have a global mapping already but the cacheability flags have
> -	 * changed, set the global PTEs.
> -	 *
> -	 * If there is an aliasing PPGTT it is anecdotally faster, so use that
> -	 * instead if none of the above hold true.
> -	 *
> -	 * NB: A global mapping should only be needed for special regions like
> -	 * "gtt mappable", SNB errata, or if specified via special execbuf
> -	 * flags. At all other times, the GPU will use the aliasing PPGTT.
> -	 */
>  	if (!dev_priv->mm.aliasing_ppgtt || flags & GLOBAL_BIND) {
> -		if (!(vma->bound & GLOBAL_BIND) ||
> -		    (cache_level != obj->cache_level)) {
> -			vma->vm->insert_entries(vma->vm, pages,
> -						vma->node.start,
> -						cache_level, pte_flags);
> -			vma->bound |= GLOBAL_BIND;
> -		}
> +		vma->vm->insert_entries(vma->vm, pages,
> +					vma->node.start,
> +					cache_level, pte_flags);
> +
> +		vma->bound |= GLOBAL_BIND;
>  	}
>  
> -	if (dev_priv->mm.aliasing_ppgtt &&
> -	    (!(vma->bound & LOCAL_BIND) ||
> -	     (cache_level != obj->cache_level))) {
> +	if (dev_priv->mm.aliasing_ppgtt && flags & LOCAL_BIND) {
>  		struct i915_hw_ppgtt *appgtt = dev_priv->mm.aliasing_ppgtt;
>  		appgtt->base.insert_entries(&appgtt->base, pages,
>  					    vma->node.start,
>  					    cache_level, pte_flags);
> -		vma->bound |= LOCAL_BIND;
>  	}
>  }
>  
> @@ -2040,16 +2016,14 @@ static void ggtt_unbind_vma(struct i915_vma *vma)
>  				     vma->node.start,
>  				     obj->base.size,
>  				     true);
> -		vma->bound &= ~GLOBAL_BIND;
>  	}
>  
> -	if (vma->bound & LOCAL_BIND) {
> +	if (dev_priv->mm.aliasing_ppgtt && vma->bound & LOCAL_BIND) {
>  		struct i915_hw_ppgtt *appgtt = dev_priv->mm.aliasing_ppgtt;
>  		appgtt->base.clear_range(&appgtt->base,
>  					 vma->node.start,
>  					 obj->base.size,
>  					 true);
> -		vma->bound &= ~LOCAL_BIND;
>  	}
>  }
>  
> @@ -2839,6 +2813,7 @@ i915_get_ggtt_vma_pages(struct i915_vma *vma)
>  int i915_vma_bind(struct i915_vma *vma, enum i915_cache_level cache_level,
>  		  u32 flags)
>  {
> +	u32 bind_flags = 0;
>  	int ret;
>  
>  	if (vma->vm->allocate_va_range) {
> @@ -2855,12 +2830,24 @@ int i915_vma_bind(struct i915_vma *vma, enum i915_cache_level cache_level,
>  
>  	if (i915_is_ggtt(vma->vm)) {
>  		ret = i915_get_ggtt_vma_pages(vma);
> -
>  		if (ret)
> -			return ret;
> +			return 0;
>  	}
>  
> -	vma->vm->bind_vma(vma, cache_level, flags);
> +	if (flags & PIN_GLOBAL)
> +		bind_flags |= GLOBAL_BIND;
> +	if (flags & PIN_USER)
> +		bind_flags |= LOCAL_BIND;
> +
> +	if (flags & PIN_UPDATE)
> +		bind_flags |= vma->bound;
> +	else
> +		bind_flags &= ~vma->bound;
> +
> +	if (bind_flags)
> +		vma->vm->bind_vma(vma, cache_level, bind_flags);
> +
> +	vma->bound |= bind_flags;
>  
>  	return 0;
>  }
> -- 
> 1.9.3
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Fix up the vma aliasing ppgtt binding

[Chris Wilson]

On Mon, Apr 20, 2015 at 09:04:05AM -0700, Daniel Vetter wrote:
> Currently we have the problem that the decision whether ptes need to
> be (re)written is splattered all over the codebase. Move all that into
> i915_vma_bind. This needs a few changes:
> - Just reuse the PIN_* flags for i915_vma_bind and do the conversion
>   to vma->bound in there to avoid duplicating the conversion code all
>   over.
> - We need to make binding for EXECBUF (i.e. pick aliasing ppgtt if
>   around) explicit, add PIN_USER for that.
> - Two callers want to update ptes, give them a PIN_UPDATE for that.
> 
> Of course we still want to avoid double-binding, but that should be
> taken care of:
> - A ppgtt vma will only ever see PIN_USER, so no issue with
>   double-binding.
> - A ggtt vma with aliasing ppgtt needs both types of binding, and we
>   track that properly now.
> - A ggtt vma without aliasing ppgtt could be bound twice. In the
>   lower-level ->bind_vma functions hence unconditionally set
>   GLOBAL_BIND when writing the ggtt ptes.
> 
> There's still a bit room for cleanup, but that's for follow-up
> patches.
> 
> v2: Fixup fumbles.
> 
> v3: s/PIN_EXECBUF/PIN_USER/ for clearer meaning, suggested by Chris.
> 
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>

I'm getting lots of GPU hangs from this patch...
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Fix up the vma aliasing ppgtt binding

[Chris Wilson]

On Fri, Apr 24, 2015 at 12:14:17PM +0100, Chris Wilson wrote:
> On Mon, Apr 20, 2015 at 09:04:05AM -0700, Daniel Vetter wrote:
> > Currently we have the problem that the decision whether ptes need to
> > be (re)written is splattered all over the codebase. Move all that into
> > i915_vma_bind. This needs a few changes:
> > - Just reuse the PIN_* flags for i915_vma_bind and do the conversion
> >   to vma->bound in there to avoid duplicating the conversion code all
> >   over.
> > - We need to make binding for EXECBUF (i.e. pick aliasing ppgtt if
> >   around) explicit, add PIN_USER for that.
> > - Two callers want to update ptes, give them a PIN_UPDATE for that.
> > 
> > Of course we still want to avoid double-binding, but that should be
> > taken care of:
> > - A ppgtt vma will only ever see PIN_USER, so no issue with
> >   double-binding.
> > - A ggtt vma with aliasing ppgtt needs both types of binding, and we
> >   track that properly now.
> > - A ggtt vma without aliasing ppgtt could be bound twice. In the
> >   lower-level ->bind_vma functions hence unconditionally set
> >   GLOBAL_BIND when writing the ggtt ptes.
> > 
> > There's still a bit room for cleanup, but that's for follow-up
> > patches.
> > 
> > v2: Fixup fumbles.
> > 
> > v3: s/PIN_EXECBUF/PIN_USER/ for clearer meaning, suggested by Chris.
> > 
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> 
> I'm getting lots of GPU hangs from this patch...

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 91aade7..e3841f8 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3066,8 +3066,8 @@ int i915_vma_unbind(struct i915_vma *vma)
 	}
 
 	trace_i915_vma_unbind(vma);
-
 	vma->vm->unbind_vma(vma);
+	vma->bound = 0;
 
 	list_del_init(&vma->mm_list);
 	if (i915_is_ggtt(vma->vm)) {
@@ -3588,7 +3588,6 @@ search_free:
 	if (ret)
 		goto err_remove_node;
 
-	trace_i915_vma_bind(vma, flags);
 	ret = i915_vma_bind(vma, obj->cache_level, flags);
 	if (ret)
 		goto err_finish_gtt;
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index cfdc8c6..45d74da 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -1563,9 +1563,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 		 * be in ggtt still end up in the aliasing ppgtt. remove
 		 * this check when that is fixed.
 		 */
-		if (USES_FULL_PPGTT(dev))
-			dispatch_flags |= I915_DISPATCH_SECURE;
-
+		dispatch_flags |= I915_DISPATCH_SECURE;
 		exec_start = 0;
 	}
 
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 9e06180..bbeb6c3 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -1949,8 +1949,6 @@ static void i915_ggtt_bind_vma(struct i915_vma *vma,
 
 	BUG_ON(!i915_is_ggtt(vma->vm));
 	intel_gtt_insert_sg_entries(vma->ggtt_view.pages, entry, flags);
-
-	vma->bound |= GLOBAL_BIND;
 }
 
 static void i915_ggtt_clear_range(struct i915_address_space *vm,
@@ -2813,9 +2811,12 @@ i915_get_ggtt_vma_pages(struct i915_vma *vma)
 int i915_vma_bind(struct i915_vma *vma, enum i915_cache_level cache_level,
 		  u32 flags)
 {
-	u32 bind_flags = 0;
+	u32 bind_flags;
 	int ret;
 
+	if (WARN_ON(flags == 0))
+		return -EINVAL;
+
 	if (vma->vm->allocate_va_range) {
 		trace_i915_va_alloc(vma->vm, vma->node.start,
 				    vma->node.size,
@@ -2834,6 +2835,7 @@ int i915_vma_bind(struct i915_vma *vma, enum i915_cache_level cache_level,
 			return 0;
 	}
 
+	bind_flags = 0;
 	if (flags & PIN_GLOBAL)
 		bind_flags |= GLOBAL_BIND;
 	if (flags & PIN_USER)
@@ -2844,10 +2846,11 @@ int i915_vma_bind(struct i915_vma *vma, enum i915_cache_level cache_level,
 	else
 		bind_flags &= ~vma->bound;
 
-	if (bind_flags)
+	if (bind_flags) {
+		trace_i915_vma_bind(vma, bind_flags);
 		vma->vm->bind_vma(vma, cache_level, bind_flags);
-
-	vma->bound |= bind_flags;
+		vma->bound |= bind_flags;
+	}
 
 	return 0;
 }

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Fix up the vma aliasing ppgtt binding

[Daniel Vetter]

On Fri, Apr 24, 2015 at 12:55:57PM +0100, Chris Wilson wrote:
> On Fri, Apr 24, 2015 at 12:14:17PM +0100, Chris Wilson wrote:
> > On Mon, Apr 20, 2015 at 09:04:05AM -0700, Daniel Vetter wrote:
> > > Currently we have the problem that the decision whether ptes need to
> > > be (re)written is splattered all over the codebase. Move all that into
> > > i915_vma_bind. This needs a few changes:
> > > - Just reuse the PIN_* flags for i915_vma_bind and do the conversion
> > >   to vma->bound in there to avoid duplicating the conversion code all
> > >   over.
> > > - We need to make binding for EXECBUF (i.e. pick aliasing ppgtt if
> > >   around) explicit, add PIN_USER for that.
> > > - Two callers want to update ptes, give them a PIN_UPDATE for that.
> > > 
> > > Of course we still want to avoid double-binding, but that should be
> > > taken care of:
> > > - A ppgtt vma will only ever see PIN_USER, so no issue with
> > >   double-binding.
> > > - A ggtt vma with aliasing ppgtt needs both types of binding, and we
> > >   track that properly now.
> > > - A ggtt vma without aliasing ppgtt could be bound twice. In the
> > >   lower-level ->bind_vma functions hence unconditionally set
> > >   GLOBAL_BIND when writing the ggtt ptes.
> > > 
> > > There's still a bit room for cleanup, but that's for follow-up
> > > patches.
> > > 
> > > v2: Fixup fumbles.
> > > 
> > > v3: s/PIN_EXECBUF/PIN_USER/ for clearer meaning, suggested by Chris.
> > > 
> > > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > 
> > I'm getting lots of GPU hangs from this patch...

Oops, I've considered the vma_unbind case but then totally forgot that a
vma can survive over execbuf. Can you please wrap this into a proper
commit message with sob?

> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 91aade7..e3841f8 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -3066,8 +3066,8 @@ int i915_vma_unbind(struct i915_vma *vma)
>  	}
>  
>  	trace_i915_vma_unbind(vma);
> -
>  	vma->vm->unbind_vma(vma);
> +	vma->bound = 0;
>  
>  	list_del_init(&vma->mm_list);
>  	if (i915_is_ggtt(vma->vm)) {
> @@ -3588,7 +3588,6 @@ search_free:
>  	if (ret)
>  		goto err_remove_node;
>  
> -	trace_i915_vma_bind(vma, flags);
>  	ret = i915_vma_bind(vma, obj->cache_level, flags);
>  	if (ret)
>  		goto err_finish_gtt;
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index cfdc8c6..45d74da 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -1563,9 +1563,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
>  		 * be in ggtt still end up in the aliasing ppgtt. remove
>  		 * this check when that is fixed.
>  		 */
> -		if (USES_FULL_PPGTT(dev))
> -			dispatch_flags |= I915_DISPATCH_SECURE;
> -
> +		dispatch_flags |= I915_DISPATCH_SECURE;

Unrelated hunk?

>  		exec_start = 0;
>  	}
>  
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 9e06180..bbeb6c3 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -1949,8 +1949,6 @@ static void i915_ggtt_bind_vma(struct i915_vma *vma,
>  
>  	BUG_ON(!i915_is_ggtt(vma->vm));
>  	intel_gtt_insert_sg_entries(vma->ggtt_view.pages, entry, flags);
> -
> -	vma->bound |= GLOBAL_BIND;

Hm, why remove this? I added this to avoid double-binding on platforms
where one implies the other binding type. Maybe should have been a bit
more consistent with bound |= GLOBAL_BIND | LOCAL_BIND.
-Daniel

>  }
>  
>  static void i915_ggtt_clear_range(struct i915_address_space *vm,
> @@ -2813,9 +2811,12 @@ i915_get_ggtt_vma_pages(struct i915_vma *vma)
>  int i915_vma_bind(struct i915_vma *vma, enum i915_cache_level cache_level,
>  		  u32 flags)
>  {
> -	u32 bind_flags = 0;
> +	u32 bind_flags;
>  	int ret;
>  
> +	if (WARN_ON(flags == 0))
> +		return -EINVAL;
> +
>  	if (vma->vm->allocate_va_range) {
>  		trace_i915_va_alloc(vma->vm, vma->node.start,
>  				    vma->node.size,
> @@ -2834,6 +2835,7 @@ int i915_vma_bind(struct i915_vma *vma, enum i915_cache_level cache_level,
>  			return 0;
>  	}
>  
> +	bind_flags = 0;
>  	if (flags & PIN_GLOBAL)
>  		bind_flags |= GLOBAL_BIND;
>  	if (flags & PIN_USER)
> @@ -2844,10 +2846,11 @@ int i915_vma_bind(struct i915_vma *vma, enum i915_cache_level cache_level,
>  	else
>  		bind_flags &= ~vma->bound;
>  
> -	if (bind_flags)
> +	if (bind_flags) {
> +		trace_i915_vma_bind(vma, bind_flags);
>  		vma->vm->bind_vma(vma, cache_level, bind_flags);
> -
> -	vma->bound |= bind_flags;
> +		vma->bound |= bind_flags;
> +	}
>  
>  	return 0;
>  }
> 
> -- 
> Chris Wilson, Intel Open Source Technology Centre

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Fix up the vma aliasing ppgtt binding

[Chris Wilson]

On Mon, May 04, 2015 at 10:49:30AM +0200, Daniel Vetter wrote:
> On Fri, Apr 24, 2015 at 12:55:57PM +0100, Chris Wilson wrote:
> > On Fri, Apr 24, 2015 at 12:14:17PM +0100, Chris Wilson wrote:
> > > On Mon, Apr 20, 2015 at 09:04:05AM -0700, Daniel Vetter wrote:
> > > > Currently we have the problem that the decision whether ptes need to
> > > > be (re)written is splattered all over the codebase. Move all that into
> > > > i915_vma_bind. This needs a few changes:
> > > > - Just reuse the PIN_* flags for i915_vma_bind and do the conversion
> > > >   to vma->bound in there to avoid duplicating the conversion code all
> > > >   over.
> > > > - We need to make binding for EXECBUF (i.e. pick aliasing ppgtt if
> > > >   around) explicit, add PIN_USER for that.
> > > > - Two callers want to update ptes, give them a PIN_UPDATE for that.
> > > > 
> > > > Of course we still want to avoid double-binding, but that should be
> > > > taken care of:
> > > > - A ppgtt vma will only ever see PIN_USER, so no issue with
> > > >   double-binding.
> > > > - A ggtt vma with aliasing ppgtt needs both types of binding, and we
> > > >   track that properly now.
> > > > - A ggtt vma without aliasing ppgtt could be bound twice. In the
> > > >   lower-level ->bind_vma functions hence unconditionally set
> > > >   GLOBAL_BIND when writing the ggtt ptes.
> > > > 
> > > > There's still a bit room for cleanup, but that's for follow-up
> > > > patches.
> > > > 
> > > > v2: Fixup fumbles.
> > > > 
> > > > v3: s/PIN_EXECBUF/PIN_USER/ for clearer meaning, suggested by Chris.
> > > > 
> > > > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > > 
> > > I'm getting lots of GPU hangs from this patch...
> 
> Oops, I've considered the vma_unbind case but then totally forgot that a
> vma can survive over execbuf. Can you please wrap this into a proper
> commit message with sob?

Mika has and already been applied :)
 
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index 91aade7..e3841f8 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -3066,8 +3066,8 @@ int i915_vma_unbind(struct i915_vma *vma)
> >  	}
> >  
> >  	trace_i915_vma_unbind(vma);
> > -
> >  	vma->vm->unbind_vma(vma);
> > +	vma->bound = 0;
> >  
> >  	list_del_init(&vma->mm_list);
> >  	if (i915_is_ggtt(vma->vm)) {
> > @@ -3588,7 +3588,6 @@ search_free:
> >  	if (ret)
> >  		goto err_remove_node;
> >  
> > -	trace_i915_vma_bind(vma, flags);
> >  	ret = i915_vma_bind(vma, obj->cache_level, flags);
> >  	if (ret)
> >  		goto err_finish_gtt;
> > diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > index cfdc8c6..45d74da 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > @@ -1563,9 +1563,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
> >  		 * be in ggtt still end up in the aliasing ppgtt. remove
> >  		 * this check when that is fixed.
> >  		 */
> > -		if (USES_FULL_PPGTT(dev))
> > -			dispatch_flags |= I915_DISPATCH_SECURE;
> > -
> > +		dispatch_flags |= I915_DISPATCH_SECURE;
> 
> Unrelated hunk?

No, two bugs, this being quite fun and an intentional artifact of your
patches :)

> >  		exec_start = 0;
> >  	}
> >  
> > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > index 9e06180..bbeb6c3 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > @@ -1949,8 +1949,6 @@ static void i915_ggtt_bind_vma(struct i915_vma *vma,
> >  
> >  	BUG_ON(!i915_is_ggtt(vma->vm));
> >  	intel_gtt_insert_sg_entries(vma->ggtt_view.pages, entry, flags);
> > -
> > -	vma->bound |= GLOBAL_BIND;
> 
> Hm, why remove this? I added this to avoid double-binding on platforms
> where one implies the other binding type. Maybe should have been a bit
> more consistent with bound |= GLOBAL_BIND | LOCAL_BIND.

Hmm, didn't we end with doing vma->bound = bind_flags? But I think if
you did if (!HAS_PPGTT) bind_flags |= GLOBAL_BIND in i915_vma_bind, then
we have all the vma->bound processing in the one place.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Fix up the vma aliasing ppgtt binding

[Daniel Vetter]

On Mon, May 04, 2015 at 10:06:51AM +0100, Chris Wilson wrote:
> On Mon, May 04, 2015 at 10:49:30AM +0200, Daniel Vetter wrote:
> > On Fri, Apr 24, 2015 at 12:55:57PM +0100, Chris Wilson wrote:
> > > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > > index 9e06180..bbeb6c3 100644
> > > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> > > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > > @@ -1949,8 +1949,6 @@ static void i915_ggtt_bind_vma(struct i915_vma *vma,
> > >  
> > >  	BUG_ON(!i915_is_ggtt(vma->vm));
> > >  	intel_gtt_insert_sg_entries(vma->ggtt_view.pages, entry, flags);
> > > -
> > > -	vma->bound |= GLOBAL_BIND;
> > 
> > Hm, why remove this? I added this to avoid double-binding on platforms
> > where one implies the other binding type. Maybe should have been a bit
> > more consistent with bound |= GLOBAL_BIND | LOCAL_BIND.
> 
> Hmm, didn't we end with doing vma->bound = bind_flags? But I think if
> you did if (!HAS_PPGTT) bind_flags |= GLOBAL_BIND in i915_vma_bind, then
> we have all the vma->bound processing in the one place.

Yeah I think that's the better option and makes it clearer what's going
on. I'll send out a patch for that.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 12/17] drm/i915: Arm cmd parser with aliasng ppgtt only

[Daniel Vetter]

With the binding regression from the original full ppgtt patches
fixed we can throw the switch. Yay!

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_execbuffer.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index f005f3151147..819f2b2317ff 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -1557,12 +1557,8 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 		 * dispatch_execbuffer implementations. We specifically
 		 * don't want that set when the command parser is
 		 * enabled.
-		 *
-		 * FIXME: with aliasing ppgtt, buffers that should only
-		 * be in ggtt still end up in the aliasing ppgtt. remove
-		 * this check when that is fixed.
 		 */
-		if (USES_FULL_PPGTT(dev))
+		if (USES_PPGTT(dev))
 			dispatch_flags |= I915_DISPATCH_SECURE;
 
 		exec_start = 0;
-- 
2.1.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 12/17] drm/i915: Arm cmd parser with aliasng ppgtt only

[Chris Wilson]

On Tue, Apr 14, 2015 at 05:35:22PM +0200, Daniel Vetter wrote:
> With the binding regression from the original full ppgtt patches
> fixed we can throw the switch. Yay!

This changelog is misleading. The validation part of the command parser
has been running for some time, with people starting to notice the
performance regressions. What is being turned on here is the enabling
part to allow userspace to do more. So shouldn't that also be a bump in
the command parser version?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 12/17] drm/i915: Arm cmd parser with aliasng ppgtt only

[Daniel Vetter]

On Tue, Apr 14, 2015 at 07:10:30PM +0100, Chris Wilson wrote:
> On Tue, Apr 14, 2015 at 05:35:22PM +0200, Daniel Vetter wrote:
> > With the binding regression from the original full ppgtt patches
> > fixed we can throw the switch. Yay!
> 
> This changelog is misleading. The validation part of the command parser
> has been running for some time, with people starting to notice the
> performance regressions. What is being turned on here is the enabling
> part to allow userspace to do more. So shouldn't that also be a bump in
> the command parser version?

mesa has independent checks that the register writes go through, so just
switching the cmd parser to permission granting mode should be all that's
neeeded really.

And yes the cmd parser is enabled already, I thought "to arm" does convey
that it's now going from dummy mode to live.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 12/17] drm/i915: Arm cmd parser with aliasng ppgtt only

[Chris Wilson]

On Wed, Apr 15, 2015 at 11:43:25AM +0200, Daniel Vetter wrote:
> On Tue, Apr 14, 2015 at 07:10:30PM +0100, Chris Wilson wrote:
> > On Tue, Apr 14, 2015 at 05:35:22PM +0200, Daniel Vetter wrote:
> > > With the binding regression from the original full ppgtt patches
> > > fixed we can throw the switch. Yay!
> > 
> > This changelog is misleading. The validation part of the command parser
> > has been running for some time, with people starting to notice the
> > performance regressions. What is being turned on here is the enabling
> > part to allow userspace to do more. So shouldn't that also be a bump in
> > the command parser version?
> 
> mesa has independent checks that the register writes go through, so just
> switching the cmd parser to permission granting mode should be all that's
> neeeded really.

But the issue is the hardware would allow the writes anyway, and that
this patch has no actual effect since mesa can already do pipelined
register writes (at least on ivb/byt).
 
> And yes the cmd parser is enabled already, I thought "to arm" does convey
> that it's now going from dummy mode to live.

"Arm cmd parser" reads to me as a passive actor (parser is just a reader
and doesn't suggest that it enables anything).

"Enable cmd parser to do secure batch promotion for aliasing ppgtt"

or perhaps

"Now witness the firepower of this fully ARMED and OPERATIONAL cmdparser"
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 12/17] drm/i915: Arm cmd parser with aliasng ppgtt only

[Daniel Vetter]

On Wed, Apr 15, 2015 at 11:07:15AM +0100, Chris Wilson wrote:
> On Wed, Apr 15, 2015 at 11:43:25AM +0200, Daniel Vetter wrote:
> > On Tue, Apr 14, 2015 at 07:10:30PM +0100, Chris Wilson wrote:
> > > On Tue, Apr 14, 2015 at 05:35:22PM +0200, Daniel Vetter wrote:
> > > > With the binding regression from the original full ppgtt patches
> > > > fixed we can throw the switch. Yay!
> > > 
> > > This changelog is misleading. The validation part of the command parser
> > > has been running for some time, with people starting to notice the
> > > performance regressions. What is being turned on here is the enabling
> > > part to allow userspace to do more. So shouldn't that also be a bump in
> > > the command parser version?
> > 
> > mesa has independent checks that the register writes go through, so just
> > switching the cmd parser to permission granting mode should be all that's
> > neeeded really.
> 
> But the issue is the hardware would allow the writes anyway, and that
> this patch has no actual effect since mesa can already do pipelined
> register writes (at least on ivb/byt).

Yeah it's only interesting for hsw really.

> > And yes the cmd parser is enabled already, I thought "to arm" does convey
> > that it's now going from dummy mode to live.
> 
> "Arm cmd parser" reads to me as a passive actor (parser is just a reader
> and doesn't suggest that it enables anything).
> 
> "Enable cmd parser to do secure batch promotion for aliasing ppgtt"
> 
> or perhaps
> 
> "Now witness the firepower of this fully ARMED and OPERATIONAL cmdparser"

Yeah not my best commit summary ever. I'll go with the first suggestion.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 12/17] drm/i915: Arm cmd parser with aliasng ppgtt only

[Jani Nikula]

On Wed, 15 Apr 2015, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Wed, Apr 15, 2015 at 11:07:15AM +0100, Chris Wilson wrote:
>> On Wed, Apr 15, 2015 at 11:43:25AM +0200, Daniel Vetter wrote:
>> > On Tue, Apr 14, 2015 at 07:10:30PM +0100, Chris Wilson wrote:
>> > > On Tue, Apr 14, 2015 at 05:35:22PM +0200, Daniel Vetter wrote:
>> > > > With the binding regression from the original full ppgtt patches
>> > > > fixed we can throw the switch. Yay!
>> > > 
>> > > This changelog is misleading. The validation part of the command parser
>> > > has been running for some time, with people starting to notice the
>> > > performance regressions. What is being turned on here is the enabling
>> > > part to allow userspace to do more. So shouldn't that also be a bump in
>> > > the command parser version?
>> > 
>> > mesa has independent checks that the register writes go through, so just
>> > switching the cmd parser to permission granting mode should be all that's
>> > neeeded really.
>> 
>> But the issue is the hardware would allow the writes anyway, and that
>> this patch has no actual effect since mesa can already do pipelined
>> register writes (at least on ivb/byt).
>
> Yeah it's only interesting for hsw really.
>
>> > And yes the cmd parser is enabled already, I thought "to arm" does convey
>> > that it's now going from dummy mode to live.
>> 
>> "Arm cmd parser" reads to me as a passive actor (parser is just a reader
>> and doesn't suggest that it enables anything).
>> 
>> "Enable cmd parser to do secure batch promotion for aliasing ppgtt"
>> 
>> or perhaps
>> 
>> "Now witness the firepower of this fully ARMED and OPERATIONAL cmdparser"
>
> Yeah not my best commit summary ever. I'll go with the first suggestion.

Pushed to drm-intel-next-queued as "drm/i915: Enable cmd parser to do
secure batch promotion for aliasing ppgtt", with

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=90190

and Mika's r-b. Thanks for the patch and review.


BR,
Jani.


> -Daniel
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 12/17] drm/i915: Arm cmd parser with aliasng ppgtt only

[Mika Kuoppala]

Daniel Vetter <daniel.vetter@ffwll.ch> writes:

> With the binding regression from the original full ppgtt patches
> fixed we can throw the switch. Yay!
>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem_execbuffer.c | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index f005f3151147..819f2b2317ff 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -1557,12 +1557,8 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
>  		 * dispatch_execbuffer implementations. We specifically
>  		 * don't want that set when the command parser is
>  		 * enabled.
> -		 *
> -		 * FIXME: with aliasing ppgtt, buffers that should only
> -		 * be in ggtt still end up in the aliasing ppgtt. remove
> -		 * this check when that is fixed.
>  		 */
> -		if (USES_FULL_PPGTT(dev))
> +		if (USES_PPGTT(dev))
>  			dispatch_flags |= I915_DISPATCH_SECURE;
>  

You can remove the check for USES_PPGTT here, like in patch 
that Chris posted. The i915_needs_cmd_parser() checks that
for us.

Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com>

>  		exec_start = 0;
> -- 
> 2.1.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH] drm/i915: Simplify cmd-parser DISPATCH_SECURE check

[Daniel Vetter]

i915_needs_cmd_parser already checks that for us.

Suggested-by: Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com>
Cc: Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_execbuffer.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index d2e21c549756..7ab63d9d7dc5 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -1559,8 +1559,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 		 * don't want that set when the command parser is
 		 * enabled.
 		 */
-		if (USES_PPGTT(dev))
-			dispatch_flags |= I915_DISPATCH_SECURE;
+		dispatch_flags |= I915_DISPATCH_SECURE;
 
 		exec_start = 0;
 	}
-- 
2.1.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Simplify cmd-parser DISPATCH_SECURE check

[Daniel Vetter]

On Mon, May 04, 2015 at 10:54:14AM +0200, Daniel Vetter wrote:
> i915_needs_cmd_parser already checks that for us.
> 
> Suggested-by: Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com>

Oops, r-b obviously shouldn't be here, copypasta-fail.
-Daniel

> Cc: Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem_execbuffer.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index d2e21c549756..7ab63d9d7dc5 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -1559,8 +1559,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
>  		 * don't want that set when the command parser is
>  		 * enabled.
>  		 */
> -		if (USES_PPGTT(dev))
> -			dispatch_flags |= I915_DISPATCH_SECURE;
> +		dispatch_flags |= I915_DISPATCH_SECURE;
>  
>  		exec_start = 0;
>  	}
> -- 
> 2.1.4
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Simplify cmd-parser DISPATCH_SECURE check

[shuang.he]

Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 6310
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                                  276/276              276/276
ILK                                  302/302              302/302
SNB                                  316/316              316/316
IVB                                  342/342              342/342
BYT                                  286/286              286/286
BDW                                  321/321              321/321
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 13/17] drm/i915: move i915_gem_restore_gtt_mappings around

[Daniel Vetter]

Avoids 2 forward declarations.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_gem_gtt.c | 109 ++++++++++++++++++------------------
 1 file changed, 53 insertions(+), 56 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 44827090a13f..00c4a336733f 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -97,9 +97,6 @@ const struct i915_ggtt_view i915_ggtt_view_rotated = {
         .type = I915_GGTT_VIEW_ROTATED
 };
 
-static void bdw_setup_private_ppat(struct drm_i915_private *dev_priv);
-static void chv_setup_private_ppat(struct drm_i915_private *dev_priv);
-
 static int sanitize_enable_ppgtt(struct drm_device *dev, int enable_ppgtt)
 {
 	bool has_aliasing_ppgtt;
@@ -1727,59 +1724,6 @@ void i915_gem_suspend_gtt_mappings(struct drm_device *dev)
 	i915_ggtt_flush(dev_priv);
 }
 
-void i915_gem_restore_gtt_mappings(struct drm_device *dev)
-{
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct drm_i915_gem_object *obj;
-	struct i915_address_space *vm;
-
-	i915_check_and_clear_faults(dev);
-
-	/* First fill our portion of the GTT with scratch pages */
-	dev_priv->gtt.base.clear_range(&dev_priv->gtt.base,
-				       dev_priv->gtt.base.start,
-				       dev_priv->gtt.base.total,
-				       true);
-
-	list_for_each_entry(obj, &dev_priv->mm.bound_list, global_list) {
-		struct i915_vma *vma = i915_gem_obj_to_vma(obj,
-							   &dev_priv->gtt.base);
-		if (!vma)
-			continue;
-
-		i915_gem_clflush_object(obj, obj->pin_display);
-		WARN_ON(i915_vma_bind(vma, obj->cache_level, PIN_UPDATE));
-	}
-
-
-	if (INTEL_INFO(dev)->gen >= 8) {
-		if (IS_CHERRYVIEW(dev) || IS_BROXTON(dev))
-			chv_setup_private_ppat(dev_priv);
-		else
-			bdw_setup_private_ppat(dev_priv);
-
-		return;
-	}
-
-	if (USES_PPGTT(dev)) {
-		list_for_each_entry(vm, &dev_priv->vm_list, global_link) {
-			/* TODO: Perhaps it shouldn't be gen6 specific */
-
-			struct i915_hw_ppgtt *ppgtt =
-					container_of(vm, struct i915_hw_ppgtt,
-						     base);
-
-			if (i915_is_ggtt(vm))
-				ppgtt = dev_priv->mm.aliasing_ppgtt;
-
-			gen6_write_page_range(dev_priv, &ppgtt->pd,
-					      0, ppgtt->base.total);
-		}
-	}
-
-	i915_ggtt_flush(dev_priv);
-}
-
 int i915_gem_gtt_prepare_object(struct drm_i915_gem_object *obj)
 {
 	if (obj->has_dma_mapping)
@@ -2603,6 +2547,59 @@ int i915_gem_gtt_init(struct drm_device *dev)
 	return 0;
 }
 
+void i915_gem_restore_gtt_mappings(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct drm_i915_gem_object *obj;
+	struct i915_address_space *vm;
+
+	i915_check_and_clear_faults(dev);
+
+	/* First fill our portion of the GTT with scratch pages */
+	dev_priv->gtt.base.clear_range(&dev_priv->gtt.base,
+				       dev_priv->gtt.base.start,
+				       dev_priv->gtt.base.total,
+				       true);
+
+	list_for_each_entry(obj, &dev_priv->mm.bound_list, global_list) {
+		struct i915_vma *vma = i915_gem_obj_to_vma(obj,
+							   &dev_priv->gtt.base);
+		if (!vma)
+			continue;
+
+		i915_gem_clflush_object(obj, obj->pin_display);
+		WARN_ON(i915_vma_bind(vma, obj->cache_level, PIN_UPDATE));
+	}
+
+
+	if (INTEL_INFO(dev)->gen >= 8) {
+		if (IS_CHERRYVIEW(dev) || IS_BROXTON(dev))
+			chv_setup_private_ppat(dev_priv);
+		else
+			bdw_setup_private_ppat(dev_priv);
+
+		return;
+	}
+
+	if (USES_PPGTT(dev)) {
+		list_for_each_entry(vm, &dev_priv->vm_list, global_link) {
+			/* TODO: Perhaps it shouldn't be gen6 specific */
+
+			struct i915_hw_ppgtt *ppgtt =
+					container_of(vm, struct i915_hw_ppgtt,
+						     base);
+
+			if (i915_is_ggtt(vm))
+				ppgtt = dev_priv->mm.aliasing_ppgtt;
+
+			gen6_write_page_range(dev_priv, &ppgtt->pd,
+					      0, ppgtt->base.total);
+		}
+	}
+
+	i915_ggtt_flush(dev_priv);
+}
+
 static struct i915_vma *
 __i915_gem_vma_create(struct drm_i915_gem_object *obj,
 		      struct i915_address_space *vm,
-- 
2.1.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 14/17] drm/i915: Move ppgtt_bind/unbind around

[Daniel Vetter]

Again avoids some forward declarations.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_gem_gtt.c | 44 ++++++++++++++++---------------------
 1 file changed, 19 insertions(+), 25 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 00c4a336733f..f07659042928 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -145,8 +145,25 @@ static int sanitize_enable_ppgtt(struct drm_device *dev, int enable_ppgtt)
 
 static void ppgtt_bind_vma(struct i915_vma *vma,
 			   enum i915_cache_level cache_level,
-			   u32 flags);
-static void ppgtt_unbind_vma(struct i915_vma *vma);
+			   u32 unused)
+{
+	u32 pte_flags = 0;
+
+	/* Currently applicable only to VLV */
+	if (vma->obj->gt_ro)
+		pte_flags |= PTE_READ_ONLY;
+
+	vma->vm->insert_entries(vma->vm, vma->obj->pages, vma->node.start,
+				cache_level, pte_flags);
+}
+
+static void ppgtt_unbind_vma(struct i915_vma *vma)
+{
+	vma->vm->clear_range(vma->vm,
+			     vma->node.start,
+			     vma->obj->base.size,
+			     true);
+}
 
 static inline gen8_pte_t gen8_pte_encode(dma_addr_t addr,
 					 enum i915_cache_level level,
@@ -1604,29 +1621,6 @@ void  i915_ppgtt_release(struct kref *kref)
 	kfree(ppgtt);
 }
 
-static void
-ppgtt_bind_vma(struct i915_vma *vma,
-	       enum i915_cache_level cache_level,
-	       u32 unused)
-{
-	u32 pte_flags = 0;
-
-	/* Currently applicable only to VLV */
-	if (vma->obj->gt_ro)
-		pte_flags |= PTE_READ_ONLY;
-
-	vma->vm->insert_entries(vma->vm, vma->obj->pages, vma->node.start,
-				cache_level, pte_flags);
-}
-
-static void ppgtt_unbind_vma(struct i915_vma *vma)
-{
-	vma->vm->clear_range(vma->vm,
-			     vma->node.start,
-			     vma->obj->base.size,
-			     true);
-}
-
 extern int intel_iommu_gfx_mapped;
 /* Certain Gen5 chipsets require require idling the GPU before
  * unmapping anything from the GTT when VT-d is enabled.
-- 
2.1.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 15/17] drm/i915: Unduplicate i915_ggtt_unbind/bind_vma

[Daniel Vetter]

ggtt_bind/unbind_vma already has checks for aliasing ppgtt or not,
there's nothing else magic they do. Resurrect i915_ggtt_insert_entries
to make the reuse possibel.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_gem_gtt.c | 27 ++++++++-------------------
 1 file changed, 8 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index f07659042928..2f8a113cbfb9 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -1876,19 +1876,16 @@ static void gen6_ggtt_clear_range(struct i915_address_space *vm,
 	readl(gtt_base);
 }
 
-
-static void i915_ggtt_bind_vma(struct i915_vma *vma,
-			       enum i915_cache_level cache_level,
-			       u32 unused)
+static void i915_ggtt_insert_entries(struct i915_address_space *vm,
+				     struct sg_table *pages,
+				     uint64_t start,
+				     enum i915_cache_level cache_level, u32 unused)
 {
-	const unsigned long entry = vma->node.start >> PAGE_SHIFT;
 	unsigned int flags = (cache_level == I915_CACHE_NONE) ?
 		AGP_USER_MEMORY : AGP_USER_CACHED_MEMORY;
 
-	BUG_ON(!i915_is_ggtt(vma->vm));
-	intel_gtt_insert_sg_entries(vma->ggtt_view.pages, entry, flags);
+	intel_gtt_insert_sg_entries(pages, start >> PAGE_SHIFT, flags);
 
-	vma->bound |= GLOBAL_BIND;
 }
 
 static void i915_ggtt_clear_range(struct i915_address_space *vm,
@@ -1901,15 +1898,6 @@ static void i915_ggtt_clear_range(struct i915_address_space *vm,
 	intel_gtt_clear_range(first_entry, num_entries);
 }
 
-static void i915_ggtt_unbind_vma(struct i915_vma *vma)
-{
-	const unsigned int first = vma->node.start >> PAGE_SHIFT;
-	const unsigned int size = vma->obj->base.size >> PAGE_SHIFT;
-
-	BUG_ON(!i915_is_ggtt(vma->vm));
-	intel_gtt_clear_range(first, size);
-}
-
 static void ggtt_bind_vma(struct i915_vma *vma,
 			  enum i915_cache_level cache_level,
 			  u32 flags)
@@ -2471,9 +2459,10 @@ static int i915_gmch_probe(struct drm_device *dev,
 	intel_gtt_get(gtt_total, stolen, mappable_base, mappable_end);
 
 	dev_priv->gtt.do_idle_maps = needs_idle_maps(dev_priv->dev);
+	dev_priv->gtt.base.insert_entries = i915_ggtt_insert_entries;
 	dev_priv->gtt.base.clear_range = i915_ggtt_clear_range;
-	dev_priv->gtt.base.bind_vma = i915_ggtt_bind_vma;
-	dev_priv->gtt.base.unbind_vma = i915_ggtt_unbind_vma;
+	dev_priv->gtt.base.bind_vma = ggtt_bind_vma;
+	dev_priv->gtt.base.unbind_vma = ggtt_unbind_vma;
 
 	if (unlikely(dev_priv->gtt.do_idle_maps))
 		DRM_INFO("applying Ironlake quirks for intel_iommu\n");
-- 
2.1.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 16/17] drm/i915: Don't try to outsmart gcc in i915_gem_gtt.c

[Daniel Vetter]

Sprinkling static inline all over the place is carg-culting. Remove
it.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_gem_gtt.c | 38 ++++++++++++++++++-------------------
 1 file changed, 19 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 2f8a113cbfb9..458819b99a0b 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -165,9 +165,9 @@ static void ppgtt_unbind_vma(struct i915_vma *vma)
 			     true);
 }
 
-static inline gen8_pte_t gen8_pte_encode(dma_addr_t addr,
-					 enum i915_cache_level level,
-					 bool valid)
+static gen8_pte_t gen8_pte_encode(dma_addr_t addr,
+				  enum i915_cache_level level,
+				  bool valid)
 {
 	gen8_pte_t pte = valid ? _PAGE_PRESENT | _PAGE_RW : 0;
 	pte |= addr;
@@ -187,9 +187,9 @@ static inline gen8_pte_t gen8_pte_encode(dma_addr_t addr,
 	return pte;
 }
 
-static inline gen8_pde_t gen8_pde_encode(struct drm_device *dev,
-					  dma_addr_t addr,
-					  enum i915_cache_level level)
+static gen8_pde_t gen8_pde_encode(struct drm_device *dev,
+				  dma_addr_t addr,
+				  enum i915_cache_level level)
 {
 	gen8_pde_t pde = _PAGE_PRESENT | _PAGE_RW;
 	pde |= addr;
@@ -299,8 +299,8 @@ static gen6_pte_t iris_pte_encode(dma_addr_t addr,
 #define i915_dma_unmap_single(px, dev) \
 	__i915_dma_unmap_single((px)->daddr, dev)
 
-static inline void __i915_dma_unmap_single(dma_addr_t daddr,
-					struct drm_device *dev)
+static void __i915_dma_unmap_single(dma_addr_t daddr,
+				    struct drm_device *dev)
 {
 	struct device *device = &dev->pdev->dev;
 
@@ -321,9 +321,9 @@ static inline void __i915_dma_unmap_single(dma_addr_t daddr,
 #define i915_dma_map_single(px, dev) \
 	i915_dma_map_page_single((px)->page, (dev), &(px)->daddr)
 
-static inline int i915_dma_map_page_single(struct page *page,
-					   struct drm_device *dev,
-					   dma_addr_t *daddr)
+static int i915_dma_map_page_single(struct page *page,
+				    struct drm_device *dev,
+				    dma_addr_t *daddr)
 {
 	struct device *device = &dev->pdev->dev;
 
@@ -1268,7 +1268,7 @@ static void gen6_ppgtt_insert_entries(struct i915_address_space *vm,
  * are switching between contexts with the same LRCA, we also must do a force
  * restore.
  */
-static inline void mark_tlbs_dirty(struct i915_hw_ppgtt *ppgtt)
+static void mark_tlbs_dirty(struct i915_hw_ppgtt *ppgtt)
 {
 	/* If current vm != vm, */
 	ppgtt->pd_dirty_rings = INTEL_INFO(ppgtt->base.dev)->ring_mask;
@@ -1625,7 +1625,7 @@ extern int intel_iommu_gfx_mapped;
 /* Certain Gen5 chipsets require require idling the GPU before
  * unmapping anything from the GTT when VT-d is enabled.
  */
-static inline bool needs_idle_maps(struct drm_device *dev)
+static bool needs_idle_maps(struct drm_device *dev)
 {
 #ifdef CONFIG_INTEL_IOMMU
 	/* Query intel_iommu to see if we need the workaround. Presumably that
@@ -1731,7 +1731,7 @@ int i915_gem_gtt_prepare_object(struct drm_i915_gem_object *obj)
 	return 0;
 }
 
-static inline void gen8_set_pte(void __iomem *addr, gen8_pte_t pte)
+static void gen8_set_pte(void __iomem *addr, gen8_pte_t pte)
 {
 #ifdef writeq
 	writeq(pte, addr);
@@ -2154,14 +2154,14 @@ static void teardown_scratch_page(struct drm_device *dev)
 	__free_page(page);
 }
 
-static inline unsigned int gen6_get_total_gtt_size(u16 snb_gmch_ctl)
+static unsigned int gen6_get_total_gtt_size(u16 snb_gmch_ctl)
 {
 	snb_gmch_ctl >>= SNB_GMCH_GGMS_SHIFT;
 	snb_gmch_ctl &= SNB_GMCH_GGMS_MASK;
 	return snb_gmch_ctl << 20;
 }
 
-static inline unsigned int gen8_get_total_gtt_size(u16 bdw_gmch_ctl)
+static unsigned int gen8_get_total_gtt_size(u16 bdw_gmch_ctl)
 {
 	bdw_gmch_ctl >>= BDW_GMCH_GGMS_SHIFT;
 	bdw_gmch_ctl &= BDW_GMCH_GGMS_MASK;
@@ -2177,7 +2177,7 @@ static inline unsigned int gen8_get_total_gtt_size(u16 bdw_gmch_ctl)
 	return bdw_gmch_ctl << 20;
 }
 
-static inline unsigned int chv_get_total_gtt_size(u16 gmch_ctrl)
+static unsigned int chv_get_total_gtt_size(u16 gmch_ctrl)
 {
 	gmch_ctrl >>= SNB_GMCH_GGMS_SHIFT;
 	gmch_ctrl &= SNB_GMCH_GGMS_MASK;
@@ -2188,14 +2188,14 @@ static inline unsigned int chv_get_total_gtt_size(u16 gmch_ctrl)
 	return 0;
 }
 
-static inline size_t gen6_get_stolen_size(u16 snb_gmch_ctl)
+static size_t gen6_get_stolen_size(u16 snb_gmch_ctl)
 {
 	snb_gmch_ctl >>= SNB_GMCH_GMS_SHIFT;
 	snb_gmch_ctl &= SNB_GMCH_GMS_MASK;
 	return snb_gmch_ctl << 25; /* 32 MB units */
 }
 
-static inline size_t gen8_get_stolen_size(u16 bdw_gmch_ctl)
+static size_t gen8_get_stolen_size(u16 bdw_gmch_ctl)
 {
 	bdw_gmch_ctl >>= BDW_GMCH_GMS_SHIFT;
 	bdw_gmch_ctl &= BDW_GMCH_GMS_MASK;
-- 
2.1.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 17/17] drm/i915: Move i915_get_ggtt_vma_pages into ggtt_bind_vma

[Daniel Vetter]

We have this neat abstraction between ppgtt and ggtt for (un)bind_vma
and didn't end up using it really. What a shame, so fix this and make
the ->bind_vma hook a bit more useful.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_gtt.c | 41 ++++++++++++++++++++++---------------
 drivers/gpu/drm/i915/i915_gem_gtt.h |  6 +++---
 2 files changed, 27 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 458819b99a0b..763c17dd2118 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -92,6 +92,9 @@
  *
  */
 
+static int
+i915_get_ggtt_vma_pages(struct i915_vma *vma);
+
 const struct i915_ggtt_view i915_ggtt_view_normal;
 const struct i915_ggtt_view i915_ggtt_view_rotated = {
         .type = I915_GGTT_VIEW_ROTATED
@@ -143,9 +146,9 @@ static int sanitize_enable_ppgtt(struct drm_device *dev, int enable_ppgtt)
 		return has_aliasing_ppgtt ? 1 : 0;
 }
 
-static void ppgtt_bind_vma(struct i915_vma *vma,
-			   enum i915_cache_level cache_level,
-			   u32 unused)
+static int ppgtt_bind_vma(struct i915_vma *vma,
+			  enum i915_cache_level cache_level,
+			  u32 unused)
 {
 	u32 pte_flags = 0;
 
@@ -155,6 +158,8 @@ static void ppgtt_bind_vma(struct i915_vma *vma,
 
 	vma->vm->insert_entries(vma->vm, vma->obj->pages, vma->node.start,
 				cache_level, pte_flags);
+
+	return 0;
 }
 
 static void ppgtt_unbind_vma(struct i915_vma *vma)
@@ -1898,22 +1903,26 @@ static void i915_ggtt_clear_range(struct i915_address_space *vm,
 	intel_gtt_clear_range(first_entry, num_entries);
 }
 
-static void ggtt_bind_vma(struct i915_vma *vma,
-			  enum i915_cache_level cache_level,
-			  u32 flags)
+static int ggtt_bind_vma(struct i915_vma *vma,
+			 enum i915_cache_level cache_level,
+			 u32 flags)
 {
 	struct drm_device *dev = vma->vm->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_i915_gem_object *obj = vma->obj;
 	struct sg_table *pages = obj->pages;
 	u32 pte_flags = 0;
+	int ret;
+
+	ret = i915_get_ggtt_vma_pages(vma);
+	if (ret)
+		return ret;
+	pages = vma->ggtt_view.pages;
 
 	/* Currently applicable only to VLV */
 	if (obj->gt_ro)
 		pte_flags |= PTE_READ_ONLY;
 
-	if (i915_is_ggtt(vma->vm))
-		pages = vma->ggtt_view.pages;
 
 	if (!dev_priv->mm.aliasing_ppgtt || flags & GLOBAL_BIND) {
 		vma->vm->insert_entries(vma->vm, pages,
@@ -1929,6 +1938,8 @@ static void ggtt_bind_vma(struct i915_vma *vma,
 					    vma->node.start,
 					    cache_level, pte_flags);
 	}
+
+	return 0;
 }
 
 static void ggtt_unbind_vma(struct i915_vma *vma)
@@ -2749,7 +2760,7 @@ err_st_alloc:
 	return ERR_PTR(ret);
 }
 
-static inline int
+static int
 i915_get_ggtt_vma_pages(struct i915_vma *vma)
 {
 	int ret = 0;
@@ -2793,8 +2804,8 @@ i915_get_ggtt_vma_pages(struct i915_vma *vma)
 int i915_vma_bind(struct i915_vma *vma, enum i915_cache_level cache_level,
 		  u32 flags)
 {
+	int ret = 0;
 	u32 bind_flags = 0;
-	int ret;
 
 	if (vma->vm->allocate_va_range) {
 		trace_i915_va_alloc(vma->vm, vma->node.start,
@@ -2808,12 +2819,6 @@ int i915_vma_bind(struct i915_vma *vma, enum i915_cache_level cache_level,
 			return ret;
 	}
 
-	if (i915_is_ggtt(vma->vm)) {
-		ret = i915_get_ggtt_vma_pages(vma);
-		if (ret)
-			return 0;
-	}
-
 	if (flags & PIN_GLOBAL)
 		bind_flags |= GLOBAL_BIND;
 	if (flags & PIN_EXECBUF)
@@ -2825,7 +2830,9 @@ int i915_vma_bind(struct i915_vma *vma, enum i915_cache_level cache_level,
 		bind_flags &= ~vma->bound;
 
 	if (bind_flags)
-		vma->vm->bind_vma(vma, cache_level, bind_flags);
+		ret = vma->vm->bind_vma(vma, cache_level, bind_flags);
+	if (ret)
+		return ret;
 
 	vma->bound |= bind_flags;
 
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
index fb0a04aa5363..4e6cac575cd8 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.h
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
@@ -278,9 +278,9 @@ struct i915_address_space {
 	 * setting the valid PTE entries to a reserved scratch page. */
 	void (*unbind_vma)(struct i915_vma *vma);
 	/* Map an object into an address space with the given cache flags. */
-	void (*bind_vma)(struct i915_vma *vma,
-			 enum i915_cache_level cache_level,
-			 u32 flags);
+	int (*bind_vma)(struct i915_vma *vma,
+			enum i915_cache_level cache_level,
+			u32 flags);
 };
 
 /* The Graphics Translation Table is the way in which GEN hardware translates a
-- 
2.1.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 17/17] drm/i915: Move i915_get_ggtt_vma_pages into ggtt_bind_vma

[Mika Kuoppala]

Daniel Vetter <daniel.vetter@ffwll.ch> writes:

> We have this neat abstraction between ppgtt and ggtt for (un)bind_vma
> and didn't end up using it really. What a shame, so fix this and make
> the ->bind_vma hook a bit more useful.
>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>

This one I had problems with applying. But it looks simple enough.

Patches 13-17,
Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com>

With 12/17 I don't yet understand all the implications so I left
that out.

-Mika


> ---
>  drivers/gpu/drm/i915/i915_gem_gtt.c | 41 ++++++++++++++++++++++---------------
>  drivers/gpu/drm/i915/i915_gem_gtt.h |  6 +++---
>  2 files changed, 27 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 458819b99a0b..763c17dd2118 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -92,6 +92,9 @@
>   *
>   */
>  
> +static int
> +i915_get_ggtt_vma_pages(struct i915_vma *vma);
> +
>  const struct i915_ggtt_view i915_ggtt_view_normal;
>  const struct i915_ggtt_view i915_ggtt_view_rotated = {
>          .type = I915_GGTT_VIEW_ROTATED
> @@ -143,9 +146,9 @@ static int sanitize_enable_ppgtt(struct drm_device *dev, int enable_ppgtt)
>  		return has_aliasing_ppgtt ? 1 : 0;
>  }
>  
> -static void ppgtt_bind_vma(struct i915_vma *vma,
> -			   enum i915_cache_level cache_level,
> -			   u32 unused)
> +static int ppgtt_bind_vma(struct i915_vma *vma,
> +			  enum i915_cache_level cache_level,
> +			  u32 unused)
>  {
>  	u32 pte_flags = 0;
>  
> @@ -155,6 +158,8 @@ static void ppgtt_bind_vma(struct i915_vma *vma,
>  
>  	vma->vm->insert_entries(vma->vm, vma->obj->pages, vma->node.start,
>  				cache_level, pte_flags);
> +
> +	return 0;
>  }
>  
>  static void ppgtt_unbind_vma(struct i915_vma *vma)
> @@ -1898,22 +1903,26 @@ static void i915_ggtt_clear_range(struct i915_address_space *vm,
>  	intel_gtt_clear_range(first_entry, num_entries);
>  }
>  
> -static void ggtt_bind_vma(struct i915_vma *vma,
> -			  enum i915_cache_level cache_level,
> -			  u32 flags)
> +static int ggtt_bind_vma(struct i915_vma *vma,
> +			 enum i915_cache_level cache_level,
> +			 u32 flags)
>  {
>  	struct drm_device *dev = vma->vm->dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct drm_i915_gem_object *obj = vma->obj;
>  	struct sg_table *pages = obj->pages;
>  	u32 pte_flags = 0;
> +	int ret;
> +
> +	ret = i915_get_ggtt_vma_pages(vma);
> +	if (ret)
> +		return ret;
> +	pages = vma->ggtt_view.pages;
>  
>  	/* Currently applicable only to VLV */
>  	if (obj->gt_ro)
>  		pte_flags |= PTE_READ_ONLY;
>  
> -	if (i915_is_ggtt(vma->vm))
> -		pages = vma->ggtt_view.pages;
>  
>  	if (!dev_priv->mm.aliasing_ppgtt || flags & GLOBAL_BIND) {
>  		vma->vm->insert_entries(vma->vm, pages,
> @@ -1929,6 +1938,8 @@ static void ggtt_bind_vma(struct i915_vma *vma,
>  					    vma->node.start,
>  					    cache_level, pte_flags);
>  	}
> +
> +	return 0;
>  }
>  
>  static void ggtt_unbind_vma(struct i915_vma *vma)
> @@ -2749,7 +2760,7 @@ err_st_alloc:
>  	return ERR_PTR(ret);
>  }
>  
> -static inline int
> +static int
>  i915_get_ggtt_vma_pages(struct i915_vma *vma)
>  {
>  	int ret = 0;
> @@ -2793,8 +2804,8 @@ i915_get_ggtt_vma_pages(struct i915_vma *vma)
>  int i915_vma_bind(struct i915_vma *vma, enum i915_cache_level cache_level,
>  		  u32 flags)
>  {
> +	int ret = 0;
>  	u32 bind_flags = 0;
> -	int ret;
>  
>  	if (vma->vm->allocate_va_range) {
>  		trace_i915_va_alloc(vma->vm, vma->node.start,
> @@ -2808,12 +2819,6 @@ int i915_vma_bind(struct i915_vma *vma, enum i915_cache_level cache_level,
>  			return ret;
>  	}
>  
> -	if (i915_is_ggtt(vma->vm)) {
> -		ret = i915_get_ggtt_vma_pages(vma);
> -		if (ret)
> -			return 0;
> -	}
> -
>  	if (flags & PIN_GLOBAL)
>  		bind_flags |= GLOBAL_BIND;
>  	if (flags & PIN_EXECBUF)
> @@ -2825,7 +2830,9 @@ int i915_vma_bind(struct i915_vma *vma, enum i915_cache_level cache_level,
>  		bind_flags &= ~vma->bound;
>  
>  	if (bind_flags)
> -		vma->vm->bind_vma(vma, cache_level, bind_flags);
> +		ret = vma->vm->bind_vma(vma, cache_level, bind_flags);
> +	if (ret)
> +		return ret;
>  
>  	vma->bound |= bind_flags;
>  
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
> index fb0a04aa5363..4e6cac575cd8 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.h
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
> @@ -278,9 +278,9 @@ struct i915_address_space {
>  	 * setting the valid PTE entries to a reserved scratch page. */
>  	void (*unbind_vma)(struct i915_vma *vma);
>  	/* Map an object into an address space with the given cache flags. */
> -	void (*bind_vma)(struct i915_vma *vma,
> -			 enum i915_cache_level cache_level,
> -			 u32 flags);
> +	int (*bind_vma)(struct i915_vma *vma,
> +			enum i915_cache_level cache_level,
> +			u32 flags);
>  };
>  
>  /* The Graphics Translation Table is the way in which GEN hardware translates a
> -- 
> 2.1.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 17/17] drm/i915: Move i915_get_ggtt_vma_pages into ggtt_bind_vma

[Daniel Vetter]

On Tue, Apr 21, 2015 at 04:36:55PM +0300, Mika Kuoppala wrote:
> Daniel Vetter <daniel.vetter@ffwll.ch> writes:
> 
> > We have this neat abstraction between ppgtt and ggtt for (un)bind_vma
> > and didn't end up using it really. What a shame, so fix this and make
> > the ->bind_vma hook a bit more useful.
> >
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> 
> This one I had problems with applying. But it looks simple enough.
> 
> Patches 13-17,
> Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com>
> 
> With 12/17 I don't yet understand all the implications so I left
> that out.

Thanks for the review, remaining patches except patch 12 pulled in.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 00/17] i915_gem_gtt.c polish

[Chris Wilson]

On Tue, Apr 14, 2015 at 05:35:10PM +0200, Daniel Vetter wrote:
> Hi all,
> 
> I ended up reading a bit of i915_gem_gtt.c and spotted a few things to clean up
> after the dynamic pagetable stuff landed. I haven't done the checkpatch polish
> and kerneldoc, Mika/Michel will be doing that, but overall I think the code
> looks fairly tidy now. I also untangled the vma binding logic a bit since it's
> related, which means we can finally enable the gen7 cmd parser.
> 
> Btw my idea is that we'll move the higher level vma related code in
> i915_gem_gtt.c out into a new i915_gem_vma.c file, together with the other vma
> code sprinkled in various places. But that's probably better to do after the
> partial mmap support from Joonas has landed. With that reorg i915_gem_gtt.c
> would only concern itself with the low-level pagetable handling.
> 
> Survived light testing on my snb here.
> 
> Comments&review highly welcome.

Ok, I think I've commented on everything I want to, the rest lgtm.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx