[PATCH 1/2] drm/i915: Only mark as map-and-fenceable when bound into the GGTT

[PATCH 1/2] drm/i915: Only mark as map-and-fenceable when bound into the GGTT

[Chris Wilson]

We use the obj->map_and_fenceable hint for when we already have a
valid mapping of this object in the aperture. This hint can only apply
to the GGTT and not to the aliasing-ppGTT. One user of the hint is
execbuffer relocation, which began to fail when it tried to follow the
hint and perform the relocate through the non-existent GGTT mapping.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=85671
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gem.c | 39 +++++++++++++++++++++++++--------------
 1 file changed, 25 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 7e919784b112..fe6c602a2a00 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3477,20 +3477,6 @@ search_free:
 	list_move_tail(&obj->global_list, &dev_priv->mm.bound_list);
 	list_add_tail(&vma->mm_list, &vm->inactive_list);
 
-	if (i915_is_ggtt(vm)) {
-		bool mappable, fenceable;
-
-		fenceable = (vma->node.size == fence_size &&
-			     (vma->node.start & (fence_alignment - 1)) == 0);
-
-		mappable = (vma->node.start + obj->base.size <=
-			    dev_priv->gtt.mappable_end);
-
-		obj->map_and_fenceable = mappable && fenceable;
-	}
-
-	WARN_ON(flags & PIN_MAPPABLE && !obj->map_and_fenceable);
-
 	trace_i915_vma_bind(vma, flags);
 	vma->bind_vma(vma, obj->cache_level,
 		      flags & (PIN_MAPPABLE | PIN_GLOBAL) ? GLOBAL_BIND : 0);
@@ -4062,6 +4048,7 @@ i915_gem_object_pin(struct drm_i915_gem_object *obj,
 {
 	struct drm_i915_private *dev_priv = obj->base.dev->dev_private;
 	struct i915_vma *vma;
+	unsigned bound;
 	int ret;
 
 	if (WARN_ON(vm == &dev_priv->mm.aliasing_ppgtt->base))
@@ -4091,6 +4078,7 @@ i915_gem_object_pin(struct drm_i915_gem_object *obj,
 		}
 	}
 
+	bound = vma ? vma->bound : 0;
 	if (vma == NULL || !drm_mm_node_allocated(&vma->node)) {
 		vma = i915_gem_object_bind_to_vm(obj, vm, alignment, flags);
 		if (IS_ERR(vma))
@@ -4100,6 +4088,29 @@ i915_gem_object_pin(struct drm_i915_gem_object *obj,
 	if (flags & PIN_GLOBAL && !(vma->bound & GLOBAL_BIND))
 		vma->bind_vma(vma, obj->cache_level, GLOBAL_BIND);
 
+	if ((bound ^ vma->bound) & GLOBAL_BIND) {
+		bool mappable, fenceable;
+		u32 fence_size, fence_alignment;
+
+		fence_size = i915_gem_get_gtt_size(obj->base.dev,
+						   obj->base.size,
+						   obj->tiling_mode);
+		fence_alignment = i915_gem_get_gtt_alignment(obj->base.dev,
+							     obj->base.size,
+							     obj->tiling_mode,
+							     true);
+
+		fenceable = (vma->node.size == fence_size &&
+			     (vma->node.start & (fence_alignment - 1)) == 0);
+
+		mappable = (vma->node.start + obj->base.size <=
+			    dev_priv->gtt.mappable_end);
+
+		obj->map_and_fenceable = mappable && fenceable;
+	}
+
+	WARN_ON(flags & PIN_MAPPABLE && !obj->map_and_fenceable);
+
 	vma->pin_count++;
 	if (flags & PIN_MAPPABLE)
 		obj->pin_mappable |= true;
-- 
2.1.1

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

[PATCH 2/2] drm/i915: Request PIN_GLOBAL when pinning a vma for GTT relocations

[Chris Wilson]

Always require PIN_GLOBAL when we want a mappable offset (PIN_MAPPABLE).
This causes the pin to fixup the global binding in cases were the vma
was already bound (and due to the proceeding bug, we considered it to be
already mappable).

References: https://bugs.freedesktop.org/show_bug.cgi?id=85671
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gem.c            | 2 +-
 drivers/gpu/drm/i915/i915_gem_execbuffer.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index fe6c602a2a00..0c82a4d2cd0c 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3479,7 +3479,7 @@ search_free:
 
 	trace_i915_vma_bind(vma, flags);
 	vma->bind_vma(vma, obj->cache_level,
-		      flags & (PIN_MAPPABLE | PIN_GLOBAL) ? GLOBAL_BIND : 0);
+		      flags & PIN_GLOBAL ? GLOBAL_BIND : 0);
 
 	return vma;
 
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 4b7f5c104ce0..e1ed85a6dc6d 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -528,7 +528,7 @@ i915_gem_execbuffer_reserve_vma(struct i915_vma *vma,
 
 	flags = 0;
 	if (entry->flags & __EXEC_OBJECT_NEEDS_MAP)
-		flags |= PIN_MAPPABLE;
+		flags |= PIN_GLOBAL | PIN_MAPPABLE;
 	if (entry->flags & EXEC_OBJECT_NEEDS_GTT)
 		flags |= PIN_GLOBAL;
 	if (entry->flags & __EXEC_OBJECT_NEEDS_BIAS)
-- 
2.1.1

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

Re: [PATCH 1/2] drm/i915: Only mark as map-and-fenceable when bound into the GGTT

[Chris Wilson]

On Fri, Oct 31, 2014 at 01:53:52PM +0000, Chris Wilson wrote:
> We use the obj->map_and_fenceable hint for when we already have a
> valid mapping of this object in the aperture. This hint can only apply
> to the GGTT and not to the aliasing-ppGTT. One user of the hint is
> execbuffer relocation, which began to fail when it tried to follow the
> hint and perform the relocate through the non-existent GGTT mapping.
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=85671
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Tested-by: huax.lu@intel.com

I have further patches to move the accounting here to the vma
bind/unbind callbacks, but this is the simpler bug fix.
-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 1/2] drm/i915: Only mark as map-and-fenceable when bound into the GGTT

[Daniel Vetter]

On Fri, Oct 31, 2014 at 01:53:52PM +0000, Chris Wilson wrote:
> @@ -4091,6 +4078,7 @@ i915_gem_object_pin(struct drm_i915_gem_object *obj,
>  		}
>  	}
>  
> +	bound = vma ? vma->bound : 0;
>  	if (vma == NULL || !drm_mm_node_allocated(&vma->node)) {
>  		vma = i915_gem_object_bind_to_vm(obj, vm, alignment, flags);
>  		if (IS_ERR(vma))
> @@ -4100,6 +4088,29 @@ i915_gem_object_pin(struct drm_i915_gem_object *obj,
>  	if (flags & PIN_GLOBAL && !(vma->bound & GLOBAL_BIND))
>  		vma->bind_vma(vma, obj->cache_level, GLOBAL_BIND);
>  
> +	if ((bound ^ vma->bound) & GLOBAL_BIND) {

Shouldn't we have a && i915_is_ggtt(vma->vm) check here too?

Then we could just look at the vma's drm_mm node instead of jumping
through the helpers here. Which gets us one inch closer to tracking
mappable in the ggtt vma, without increasing the diff ;-)
-Daniel

> +		bool mappable, fenceable;
> +		u32 fence_size, fence_alignment;
> +
> +		fence_size = i915_gem_get_gtt_size(obj->base.dev,
> +						   obj->base.size,
> +						   obj->tiling_mode);
> +		fence_alignment = i915_gem_get_gtt_alignment(obj->base.dev,
> +							     obj->base.size,
> +							     obj->tiling_mode,
> +							     true);
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915: Request PIN_GLOBAL when pinning a vma for GTT relocations

[Daniel Vetter]

On Fri, Oct 31, 2014 at 01:53:53PM +0000, Chris Wilson wrote:
> Always require PIN_GLOBAL when we want a mappable offset (PIN_MAPPABLE).
> This causes the pin to fixup the global binding in cases were the vma
> was already bound (and due to the proceeding bug, we considered it to be
> already mappable).
> 
> References: https://bugs.freedesktop.org/show_bug.cgi?id=85671
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/i915_gem.c            | 2 +-
>  drivers/gpu/drm/i915/i915_gem_execbuffer.c | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index fe6c602a2a00..0c82a4d2cd0c 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -3479,7 +3479,7 @@ search_free:
>  
>  	trace_i915_vma_bind(vma, flags);
>  	vma->bind_vma(vma, obj->cache_level,
> -		      flags & (PIN_MAPPABLE | PIN_GLOBAL) ? GLOBAL_BIND : 0);
> +		      flags & PIN_GLOBAL ? GLOBAL_BIND : 0);

Hm, why this? If we want to reduce the interface complexity maybe we
should throw in a WARN_ON if PIN_MAPPABLE is set, but PIN_GLOBAL isnt?
Just removing this safeguard make me a bit uneasy ...
-Daniel

>  
>  	return vma;
>  
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index 4b7f5c104ce0..e1ed85a6dc6d 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -528,7 +528,7 @@ i915_gem_execbuffer_reserve_vma(struct i915_vma *vma,
>  
>  	flags = 0;
>  	if (entry->flags & __EXEC_OBJECT_NEEDS_MAP)
> -		flags |= PIN_MAPPABLE;
> +		flags |= PIN_GLOBAL | PIN_MAPPABLE;
>  	if (entry->flags & EXEC_OBJECT_NEEDS_GTT)
>  		flags |= PIN_GLOBAL;
>  	if (entry->flags & __EXEC_OBJECT_NEEDS_BIAS)
> -- 
> 2.1.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/i915: Only mark as map-and-fenceable when bound into the GGTT

[Chris Wilson]

On Mon, Nov 03, 2014 at 04:17:57PM +0100, Daniel Vetter wrote:
> On Fri, Oct 31, 2014 at 01:53:52PM +0000, Chris Wilson wrote:
> > @@ -4091,6 +4078,7 @@ i915_gem_object_pin(struct drm_i915_gem_object *obj,
> >  		}
> >  	}
> >  
> > +	bound = vma ? vma->bound : 0;
> >  	if (vma == NULL || !drm_mm_node_allocated(&vma->node)) {
> >  		vma = i915_gem_object_bind_to_vm(obj, vm, alignment, flags);
> >  		if (IS_ERR(vma))
> > @@ -4100,6 +4088,29 @@ i915_gem_object_pin(struct drm_i915_gem_object *obj,
> >  	if (flags & PIN_GLOBAL && !(vma->bound & GLOBAL_BIND))
> >  		vma->bind_vma(vma, obj->cache_level, GLOBAL_BIND);
> >  
> > +	if ((bound ^ vma->bound) & GLOBAL_BIND) {
> 
> Shouldn't we have a && i915_is_ggtt(vma->vm) check here too?

vma->bound & GLOBAL_BIND => i915_is_ggtt().
 
> Then we could just look at the vma's drm_mm node instead of jumping
> through the helpers here. Which gets us one inch closer to tracking
> mappable in the ggtt vma, without increasing the diff ;-)

Oh, that's just copy and paste without thinking.
-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 2/2] drm/i915: Request PIN_GLOBAL when pinning a vma for GTT relocations

[Chris Wilson]

On Mon, Nov 03, 2014 at 04:20:58PM +0100, Daniel Vetter wrote:
> On Fri, Oct 31, 2014 at 01:53:53PM +0000, Chris Wilson wrote:
> > Always require PIN_GLOBAL when we want a mappable offset (PIN_MAPPABLE).
> > This causes the pin to fixup the global binding in cases were the vma
> > was already bound (and due to the proceeding bug, we considered it to be
> > already mappable).
> > 
> > References: https://bugs.freedesktop.org/show_bug.cgi?id=85671
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > ---
> >  drivers/gpu/drm/i915/i915_gem.c            | 2 +-
> >  drivers/gpu/drm/i915/i915_gem_execbuffer.c | 2 +-
> >  2 files changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index fe6c602a2a00..0c82a4d2cd0c 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -3479,7 +3479,7 @@ search_free:
> >  
> >  	trace_i915_vma_bind(vma, flags);
> >  	vma->bind_vma(vma, obj->cache_level,
> > -		      flags & (PIN_MAPPABLE | PIN_GLOBAL) ? GLOBAL_BIND : 0);
> > +		      flags & PIN_GLOBAL ? GLOBAL_BIND : 0);
> 
> Hm, why this? If we want to reduce the interface complexity maybe we
> should throw in a WARN_ON if PIN_MAPPABLE is set, but PIN_GLOBAL isnt?
> Just removing this safeguard make me a bit uneasy ...

Just review all the users, takes less than 2 minutes ;)
It's a wart in the interface, which should do as I say, not guess, and
PIN_MAPPABLE doesn't translate well to GLOBAL_BIND imo.
-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 1/2] drm/i915: Only mark as map-and-fenceable when bound into the GGTT

[Chris Wilson]

On Mon, Nov 03, 2014 at 04:17:57PM +0100, Daniel Vetter wrote:
> Then we could just look at the vma's drm_mm node instead of jumping
> through the helpers here. Which gets us one inch closer to tracking
> mappable in the ggtt vma, without increasing the diff ;-)

> 
> > +		bool mappable, fenceable;
> > +		u32 fence_size, fence_alignment;
> > +
> > +		fence_size = i915_gem_get_gtt_size(obj->base.dev,
> > +						   obj->base.size,
> > +						   obj->tiling_mode);
> > +		fence_alignment = i915_gem_get_gtt_alignment(obj->base.dev,
> > +							     obj->base.size,
> > +							     obj->tiling_mode,
> > +							     true);

I thought you were referring to these? But you are not, so what do you
mean?
-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 2/2] drm/i915: Request PIN_GLOBAL when pinning a vma for GTT relocations

[Daniel Vetter]

On Mon, Nov 03, 2014 at 08:49:08PM +0000, Chris Wilson wrote:
> On Mon, Nov 03, 2014 at 04:20:58PM +0100, Daniel Vetter wrote:
> > On Fri, Oct 31, 2014 at 01:53:53PM +0000, Chris Wilson wrote:
> > > Always require PIN_GLOBAL when we want a mappable offset (PIN_MAPPABLE).
> > > This causes the pin to fixup the global binding in cases were the vma
> > > was already bound (and due to the proceeding bug, we considered it to be
> > > already mappable).
> > > 
> > > References: https://bugs.freedesktop.org/show_bug.cgi?id=85671
> > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > ---
> > >  drivers/gpu/drm/i915/i915_gem.c            | 2 +-
> > >  drivers/gpu/drm/i915/i915_gem_execbuffer.c | 2 +-
> > >  2 files changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > > index fe6c602a2a00..0c82a4d2cd0c 100644
> > > --- a/drivers/gpu/drm/i915/i915_gem.c
> > > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > > @@ -3479,7 +3479,7 @@ search_free:
> > >  
> > >  	trace_i915_vma_bind(vma, flags);
> > >  	vma->bind_vma(vma, obj->cache_level,
> > > -		      flags & (PIN_MAPPABLE | PIN_GLOBAL) ? GLOBAL_BIND : 0);
> > > +		      flags & PIN_GLOBAL ? GLOBAL_BIND : 0);
> > 
> > Hm, why this? If we want to reduce the interface complexity maybe we
> > should throw in a WARN_ON if PIN_MAPPABLE is set, but PIN_GLOBAL isnt?
> > Just removing this safeguard make me a bit uneasy ...
> 
> Just review all the users, takes less than 2 minutes ;)
> It's a wart in the interface, which should do as I say, not guess, and
> PIN_MAPPABLE doesn't translate well to GLOBAL_BIND imo.

I don't object to the functional change, it makes sense. But I think
throwing in the safeguard WARN_ON would be useful.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915: Request PIN_GLOBAL when pinning a vma for GTT relocations

[Daniel Vetter]

On Tue, Nov 04, 2014 at 10:21:36AM +0100, Daniel Vetter wrote:
> On Mon, Nov 03, 2014 at 08:49:08PM +0000, Chris Wilson wrote:
> > On Mon, Nov 03, 2014 at 04:20:58PM +0100, Daniel Vetter wrote:
> > > On Fri, Oct 31, 2014 at 01:53:53PM +0000, Chris Wilson wrote:
> > > > Always require PIN_GLOBAL when we want a mappable offset (PIN_MAPPABLE).
> > > > This causes the pin to fixup the global binding in cases were the vma
> > > > was already bound (and due to the proceeding bug, we considered it to be
> > > > already mappable).
> > > > 
> > > > References: https://bugs.freedesktop.org/show_bug.cgi?id=85671
> > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > > ---
> > > >  drivers/gpu/drm/i915/i915_gem.c            | 2 +-
> > > >  drivers/gpu/drm/i915/i915_gem_execbuffer.c | 2 +-
> > > >  2 files changed, 2 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > > > index fe6c602a2a00..0c82a4d2cd0c 100644
> > > > --- a/drivers/gpu/drm/i915/i915_gem.c
> > > > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > > > @@ -3479,7 +3479,7 @@ search_free:
> > > >  
> > > >  	trace_i915_vma_bind(vma, flags);
> > > >  	vma->bind_vma(vma, obj->cache_level,
> > > > -		      flags & (PIN_MAPPABLE | PIN_GLOBAL) ? GLOBAL_BIND : 0);
> > > > +		      flags & PIN_GLOBAL ? GLOBAL_BIND : 0);
> > > 
> > > Hm, why this? If we want to reduce the interface complexity maybe we
> > > should throw in a WARN_ON if PIN_MAPPABLE is set, but PIN_GLOBAL isnt?
> > > Just removing this safeguard make me a bit uneasy ...
> > 
> > Just review all the users, takes less than 2 minutes ;)
> > It's a wart in the interface, which should do as I say, not guess, and
> > PIN_MAPPABLE doesn't translate well to GLOBAL_BIND imo.
> 
> I don't object to the functional change, it makes sense. But I think
> throwing in the safeguard WARN_ON would be useful.

Done as per our irc discussion and merged them all.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx