[PATCH] drm/i915: Fix pages pin counting around swizzle quirk

[PATCH] drm/i915: Fix pages pin counting around swizzle quirk

[Chris Wilson]

commit bc0629a76726 ("drm/i915: Track pages pinned due to swizzling
quirk") fixed one problem, but revealed a whole lot more. The root cause
of the pin count mismatch for the swizzle quirk (for L-shaped memory on
gen3/4) was that we were incrementing the pages_pin_count upon getting
the backing pages but then overwriting the pages_pin_count to set it to
1 afterwards. With a little bit of adjustment to satisfy the GEM_BUG_ON
sanitychecks, the fix is to replace the explicit atomic_set with an
atomic_inc.

Fixes: 1233e2db199d ("drm/i915: Move object backing storage manipulation")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c        | 40 ++++++++++++++++++----------------
 drivers/gpu/drm/i915/i915_gem_gtt.c    |  1 +
 drivers/gpu/drm/i915/i915_gem_tiling.c |  1 +
 3 files changed, 23 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 993fb90da104..5fe7562aefbd 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2386,12 +2386,6 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
 	if (i915_gem_object_needs_bit17_swizzle(obj))
 		i915_gem_object_do_bit_17_swizzle(obj, st);
 
-	if (i915_gem_object_is_tiled(obj) &&
-	    dev_priv->quirks & QUIRK_PIN_SWIZZLED_PAGES) {
-		__i915_gem_object_pin_pages(obj);
-		obj->mm.quirked = true;
-	}
-
 	return st;
 
 err_pages:
@@ -2424,6 +2418,13 @@ void __i915_gem_object_set_pages(struct drm_i915_gem_object *obj,
 	obj->mm.get_page.sg_idx = 0;
 
 	obj->mm.pages = pages;
+
+	if (i915_gem_object_is_tiled(obj) &&
+	    to_i915(obj->base.dev)->quirks & QUIRK_PIN_SWIZZLED_PAGES) {
+		GEM_BUG_ON(obj->mm.quirked);
+		__i915_gem_object_pin_pages(obj);
+		obj->mm.quirked = true;
+	}
 }
 
 static int ____i915_gem_object_get_pages(struct drm_i915_gem_object *obj)
@@ -2458,17 +2459,16 @@ int __i915_gem_object_get_pages(struct drm_i915_gem_object *obj)
 	if (err)
 		return err;
 
-	if (likely(obj->mm.pages)) {
-		__i915_gem_object_pin_pages(obj);
-		goto unlock;
-	}
-
-	GEM_BUG_ON(i915_gem_object_has_pinned_pages(obj));
+	if (unlikely(!obj->mm.pages)) {
+		GEM_BUG_ON(i915_gem_object_has_pinned_pages(obj));
+		err = ____i915_gem_object_get_pages(obj);
+		if (err)
+			goto unlock;
 
-	err = ____i915_gem_object_get_pages(obj);
-	if (!err)
-		atomic_set_release(&obj->mm.pages_pin_count, 1);
+		smp_mb__before_atomic();
+	}
 
+	__i915_gem_object_pin_pages(obj);
 unlock:
 	mutex_unlock(&obj->mm.lock);
 	return err;
@@ -2542,8 +2542,8 @@ void *i915_gem_object_pin_map(struct drm_i915_gem_object *obj,
 		if (ret)
 			goto err_unlock;
 
-		GEM_BUG_ON(atomic_read(&obj->mm.pages_pin_count));
-		atomic_set_release(&obj->mm.pages_pin_count, 1);
+		smp_mb__before_atomic();
+		__i915_gem_object_pin_pages(obj);
 		pinned = false;
 	}
 	GEM_BUG_ON(!obj->mm.pages);
@@ -2578,7 +2578,7 @@ void *i915_gem_object_pin_map(struct drm_i915_gem_object *obj,
 	return ptr;
 
 err_unpin:
-	atomic_dec(&obj->mm.pages_pin_count);
+	__i915_gem_object_unpin_pages(obj);
 err_unlock:
 	ptr = ERR_PTR(ret);
 	goto out_unlock;
@@ -2996,7 +2996,7 @@ int i915_vma_unbind(struct i915_vma *vma)
 		goto destroy;
 
 	GEM_BUG_ON(obj->bind_count == 0);
-	GEM_BUG_ON(!obj->mm.pages);
+	GEM_BUG_ON(!i915_gem_object_has_pinned_pages(obj));
 
 	if (i915_vma_is_map_and_fenceable(vma)) {
 		/* release the fence reg _after_ flushing */
@@ -3230,6 +3230,7 @@ i915_vma_insert(struct i915_vma *vma, u64 size, u64 alignment, u64 flags)
 	list_move_tail(&obj->global_list, &dev_priv->mm.bound_list);
 	list_move_tail(&vma->vm_link, &vma->vm->inactive_list);
 	obj->bind_count++;
+	GEM_BUG_ON(atomic_read(&obj->mm.pages_pin_count) < obj->bind_count);
 
 	return 0;
 
@@ -4282,6 +4283,7 @@ i915_gem_madvise_ioctl(struct drm_device *dev, void *data,
 			obj->mm.quirked = false;
 		}
 		if (args->madv == I915_MADV_WILLNEED) {
+			GEM_BUG_ON(obj->mm.quirked);
 			__i915_gem_object_pin_pages(obj);
 			obj->mm.quirked = true;
 		}
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index dfc40f16f149..4dc1a463ef4c 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -3707,6 +3707,7 @@ i915_get_ggtt_vma_pages(struct i915_vma *vma)
 {
 	int ret = 0;
 
+	GEM_BUG_ON(!i915_gem_object_has_pinned_pages(vma->obj));
 	if (vma->pages)
 		return 0;
 
diff --git a/drivers/gpu/drm/i915/i915_gem_tiling.c b/drivers/gpu/drm/i915/i915_gem_tiling.c
index 1577e7810cd6..251d51b01174 100644
--- a/drivers/gpu/drm/i915/i915_gem_tiling.c
+++ b/drivers/gpu/drm/i915/i915_gem_tiling.c
@@ -269,6 +269,7 @@ i915_gem_set_tiling(struct drm_device *dev, void *data,
 					obj->mm.quirked = false;
 				}
 				if (!i915_gem_object_is_tiled(obj)) {
+					GEM_BUG_ON(!obj->mm.quirked);
 					__i915_gem_object_pin_pages(obj);
 					obj->mm.quirked = true;
 				}
-- 
2.10.2

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

✓ Fi.CI.BAT: success for drm/i915: Fix pages pin counting around swizzle quirk

[Patchwork]

== Series Details ==

Series: drm/i915: Fix pages pin counting around swizzle quirk
URL   : https://patchwork.freedesktop.org/series/14720/
State : success

== Summary ==

Series 14720v1 drm/i915: Fix pages pin counting around swizzle quirk
https://patchwork.freedesktop.org/api/1.0/series/14720/revisions/1/mbox/

Test gem_ctx_switch:
        Subgroup basic-default:
                timeout    -> PASS       (fi-bsw-n3050)
        Subgroup basic-default-heavy:
                incomplete -> PASS       (fi-bsw-n3050)

fi-bdw-5557u     total:241  pass:226  dwarn:0   dfail:0   fail:0   skip:15 
fi-bsw-n3050     total:241  pass:201  dwarn:0   dfail:0   fail:0   skip:40 
fi-bxt-t5700     total:241  pass:213  dwarn:0   dfail:0   fail:0   skip:28 
fi-byt-j1900     total:241  pass:213  dwarn:0   dfail:0   fail:0   skip:28 
fi-byt-n2820     total:241  pass:209  dwarn:0   dfail:0   fail:0   skip:32 
fi-hsw-4770      total:241  pass:221  dwarn:0   dfail:0   fail:0   skip:20 
fi-hsw-4770r     total:241  pass:220  dwarn:0   dfail:0   fail:0   skip:21 
fi-ilk-650       total:241  pass:187  dwarn:0   dfail:0   fail:0   skip:54 
fi-ivb-3520m     total:241  pass:218  dwarn:0   dfail:0   fail:0   skip:23 
fi-ivb-3770      total:241  pass:218  dwarn:0   dfail:0   fail:0   skip:23 
fi-kbl-7200u     total:241  pass:219  dwarn:0   dfail:0   fail:0   skip:22 
fi-skl-6260u     total:241  pass:227  dwarn:0   dfail:0   fail:0   skip:14 
fi-skl-6700hq    total:241  pass:220  dwarn:0   dfail:0   fail:0   skip:21 
fi-skl-6700k     total:241  pass:219  dwarn:1   dfail:0   fail:0   skip:21 
fi-skl-6770hq    total:241  pass:227  dwarn:0   dfail:0   fail:0   skip:14 
fi-snb-2520m     total:241  pass:208  dwarn:0   dfail:0   fail:0   skip:33 
fi-snb-2600      total:241  pass:207  dwarn:0   dfail:0   fail:0   skip:34 

179f2b207ffc86fad387e7d102912b6c897abc4e drm-intel-nightly: 2016y-11m-02d-10h-06m-52s UTC integration manifest
a2c3d69e drm/i915: Fix pages pin counting around swizzle quirk

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_2887/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Fix pages pin counting around swizzle quirk

[Chris Wilson]

On Wed, Nov 02, 2016 at 09:43:54AM +0000, Chris Wilson wrote:
> commit bc0629a76726 ("drm/i915: Track pages pinned due to swizzling
> quirk") fixed one problem, but revealed a whole lot more. The root cause
> of the pin count mismatch for the swizzle quirk (for L-shaped memory on
> gen3/4) was that we were incrementing the pages_pin_count upon getting
> the backing pages but then overwriting the pages_pin_count to set it to
> 1 afterwards. With a little bit of adjustment to satisfy the GEM_BUG_ON
> sanitychecks, the fix is to replace the explicit atomic_set with an
> atomic_inc.
> 
> Fixes: 1233e2db199d ("drm/i915: Move object backing storage manipulation")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Ping?

> ---
>  drivers/gpu/drm/i915/i915_gem.c        | 40 ++++++++++++++++++----------------
>  drivers/gpu/drm/i915/i915_gem_gtt.c    |  1 +
>  drivers/gpu/drm/i915/i915_gem_tiling.c |  1 +
>  3 files changed, 23 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 993fb90da104..5fe7562aefbd 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2386,12 +2386,6 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
>  	if (i915_gem_object_needs_bit17_swizzle(obj))
>  		i915_gem_object_do_bit_17_swizzle(obj, st);
>  
> -	if (i915_gem_object_is_tiled(obj) &&
> -	    dev_priv->quirks & QUIRK_PIN_SWIZZLED_PAGES) {
> -		__i915_gem_object_pin_pages(obj);
> -		obj->mm.quirked = true;
> -	}
> -
>  	return st;
>  
>  err_pages:
> @@ -2424,6 +2418,13 @@ void __i915_gem_object_set_pages(struct drm_i915_gem_object *obj,
>  	obj->mm.get_page.sg_idx = 0;
>  
>  	obj->mm.pages = pages;
> +
> +	if (i915_gem_object_is_tiled(obj) &&
> +	    to_i915(obj->base.dev)->quirks & QUIRK_PIN_SWIZZLED_PAGES) {
> +		GEM_BUG_ON(obj->mm.quirked);
> +		__i915_gem_object_pin_pages(obj);
> +		obj->mm.quirked = true;
> +	}
>  }
>  
>  static int ____i915_gem_object_get_pages(struct drm_i915_gem_object *obj)
> @@ -2458,17 +2459,16 @@ int __i915_gem_object_get_pages(struct drm_i915_gem_object *obj)
>  	if (err)
>  		return err;
>  
> -	if (likely(obj->mm.pages)) {
> -		__i915_gem_object_pin_pages(obj);
> -		goto unlock;
> -	}
> -
> -	GEM_BUG_ON(i915_gem_object_has_pinned_pages(obj));
> +	if (unlikely(!obj->mm.pages)) {
> +		GEM_BUG_ON(i915_gem_object_has_pinned_pages(obj));
> +		err = ____i915_gem_object_get_pages(obj);
> +		if (err)
> +			goto unlock;
>  
> -	err = ____i915_gem_object_get_pages(obj);
> -	if (!err)
> -		atomic_set_release(&obj->mm.pages_pin_count, 1);
> +		smp_mb__before_atomic();
> +	}
>  
> +	__i915_gem_object_pin_pages(obj);
>  unlock:
>  	mutex_unlock(&obj->mm.lock);
>  	return err;
> @@ -2542,8 +2542,8 @@ void *i915_gem_object_pin_map(struct drm_i915_gem_object *obj,
>  		if (ret)
>  			goto err_unlock;
>  
> -		GEM_BUG_ON(atomic_read(&obj->mm.pages_pin_count));
> -		atomic_set_release(&obj->mm.pages_pin_count, 1);
> +		smp_mb__before_atomic();
> +		__i915_gem_object_pin_pages(obj);
>  		pinned = false;
>  	}
>  	GEM_BUG_ON(!obj->mm.pages);
> @@ -2578,7 +2578,7 @@ void *i915_gem_object_pin_map(struct drm_i915_gem_object *obj,
>  	return ptr;
>  
>  err_unpin:
> -	atomic_dec(&obj->mm.pages_pin_count);
> +	__i915_gem_object_unpin_pages(obj);
>  err_unlock:
>  	ptr = ERR_PTR(ret);
>  	goto out_unlock;
> @@ -2996,7 +2996,7 @@ int i915_vma_unbind(struct i915_vma *vma)
>  		goto destroy;
>  
>  	GEM_BUG_ON(obj->bind_count == 0);
> -	GEM_BUG_ON(!obj->mm.pages);
> +	GEM_BUG_ON(!i915_gem_object_has_pinned_pages(obj));
>  
>  	if (i915_vma_is_map_and_fenceable(vma)) {
>  		/* release the fence reg _after_ flushing */
> @@ -3230,6 +3230,7 @@ i915_vma_insert(struct i915_vma *vma, u64 size, u64 alignment, u64 flags)
>  	list_move_tail(&obj->global_list, &dev_priv->mm.bound_list);
>  	list_move_tail(&vma->vm_link, &vma->vm->inactive_list);
>  	obj->bind_count++;
> +	GEM_BUG_ON(atomic_read(&obj->mm.pages_pin_count) < obj->bind_count);
>  
>  	return 0;
>  
> @@ -4282,6 +4283,7 @@ i915_gem_madvise_ioctl(struct drm_device *dev, void *data,
>  			obj->mm.quirked = false;
>  		}
>  		if (args->madv == I915_MADV_WILLNEED) {
> +			GEM_BUG_ON(obj->mm.quirked);
>  			__i915_gem_object_pin_pages(obj);
>  			obj->mm.quirked = true;
>  		}
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index dfc40f16f149..4dc1a463ef4c 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -3707,6 +3707,7 @@ i915_get_ggtt_vma_pages(struct i915_vma *vma)
>  {
>  	int ret = 0;
>  
> +	GEM_BUG_ON(!i915_gem_object_has_pinned_pages(vma->obj));
>  	if (vma->pages)
>  		return 0;
>  
> diff --git a/drivers/gpu/drm/i915/i915_gem_tiling.c b/drivers/gpu/drm/i915/i915_gem_tiling.c
> index 1577e7810cd6..251d51b01174 100644
> --- a/drivers/gpu/drm/i915/i915_gem_tiling.c
> +++ b/drivers/gpu/drm/i915/i915_gem_tiling.c
> @@ -269,6 +269,7 @@ i915_gem_set_tiling(struct drm_device *dev, void *data,
>  					obj->mm.quirked = false;
>  				}
>  				if (!i915_gem_object_is_tiled(obj)) {
> +					GEM_BUG_ON(!obj->mm.quirked);
>  					__i915_gem_object_pin_pages(obj);
>  					obj->mm.quirked = true;
>  				}
> -- 
> 2.10.2
> 

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

Re: [PATCH] drm/i915: Fix pages pin counting around swizzle quirk

[Joonas Lahtinen]

On ke, 2016-11-02 at 09:43 +0000, Chris Wilson wrote:
> @@ -2458,17 +2459,16 @@ int __i915_gem_object_get_pages(struct drm_i915_gem_object *obj)
>  	if (err)
>  		return err;
>  
> -	if (likely(obj->mm.pages)) {
> -		__i915_gem_object_pin_pages(obj);
> -		goto unlock;
> -	}
> -
> -	GEM_BUG_ON(i915_gem_object_has_pinned_pages(obj));
> +	if (unlikely(!obj->mm.pages)) {
> +		GEM_BUG_ON(i915_gem_object_has_pinned_pages(obj));
> +		err = ____i915_gem_object_get_pages(obj);
> +		if (err)
> +			goto unlock;
>  
> -	err = ____i915_gem_object_get_pages(obj);
> -	if (!err)
> -		atomic_set_release(&obj->mm.pages_pin_count, 1);
> +		smp_mb__before_atomic();

This is not cool without atomic in sight. Inline wrap as
__i915_gem_object_pages_mb() or something.

> @@ -3707,6 +3707,7 @@ i915_get_ggtt_vma_pages(struct i915_vma *vma)
>  {
>  	int ret = 0;
>  
> +	GEM_BUG_ON(!i915_gem_object_has_pinned_pages(vma->obj));

Rather confusing, simple mind would think as
__i915_gem_object_pin_pages has GEM_BUG_ON(!obj->mm.pages),
the next branch would never be taken?

>  	if (vma->pages)
>  		return 0;
>  

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Fix pages pin counting around swizzle quirk

[Chris Wilson]

On Fri, Nov 04, 2016 at 10:50:44AM +0200, Joonas Lahtinen wrote:
> On ke, 2016-11-02 at 09:43 +0000, Chris Wilson wrote:
> > @@ -2458,17 +2459,16 @@ int __i915_gem_object_get_pages(struct drm_i915_gem_object *obj)
> >  	if (err)
> >  		return err;
> >  
> > -	if (likely(obj->mm.pages)) {
> > -		__i915_gem_object_pin_pages(obj);
> > -		goto unlock;
> > -	}
> > -
> > -	GEM_BUG_ON(i915_gem_object_has_pinned_pages(obj));
> > +	if (unlikely(!obj->mm.pages)) {
> > +		GEM_BUG_ON(i915_gem_object_has_pinned_pages(obj));
> > +		err = ____i915_gem_object_get_pages(obj);
> > +		if (err)
> > +			goto unlock;
> >  
> > -	err = ____i915_gem_object_get_pages(obj);
> > -	if (!err)
> > -		atomic_set_release(&obj->mm.pages_pin_count, 1);
> > +		smp_mb__before_atomic();
> 
> This is not cool without atomic in sight. Inline wrap as
> __i915_gem_object_pages_mb() or something.

My first thought was to put in ____i915_gem_object_get_pages() since it
closes the action of setting up the obj->mm.pages and co. I didn't like
that because the association then with the use of the pages_pin_count as
the mutex was not as apparent. Now that you cannot see the atomic_inc()
at all here, you are left confused!

Would you rather this just used the raw atomic_inc() here?

> 
> > @@ -3707,6 +3707,7 @@ i915_get_ggtt_vma_pages(struct i915_vma *vma)
> >  {
> >  	int ret = 0;
> >  
> > +	GEM_BUG_ON(!i915_gem_object_has_pinned_pages(vma->obj));
> 
> Rather confusing, simple mind would think as
> __i915_gem_object_pin_pages has GEM_BUG_ON(!obj->mm.pages),
> the next branch would never be taken?

GEM_BUG_ON(vma == obj) ? Sorry not parsing very well this morning.

GEM_BUG_ON(!obj->mm.pages) would be a weaker form of the above. The
challenge is to express that the vma->page is only valid for the current
lifespan of the obj->mm.pages, should we regenerate that sg_table, we
need to regenerate the vma->pages. So I want to say that we must be
holding a pages_pin_count to utilize the vma->pages.

> >  	if (vma->pages)
> >  		return 0;

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

Re: [PATCH] drm/i915: Fix pages pin counting around swizzle quirk

[Chris Wilson]

On Fri, Nov 04, 2016 at 09:36:31AM +0000, Chris Wilson wrote:
> On Fri, Nov 04, 2016 at 10:50:44AM +0200, Joonas Lahtinen wrote:
> > On ke, 2016-11-02 at 09:43 +0000, Chris Wilson wrote:
> > > @@ -2458,17 +2459,16 @@ int __i915_gem_object_get_pages(struct drm_i915_gem_object *obj)
> > >  	if (err)
> > >  		return err;
> > >  
> > > -	if (likely(obj->mm.pages)) {
> > > -		__i915_gem_object_pin_pages(obj);
> > > -		goto unlock;
> > > -	}
> > > -
> > > -	GEM_BUG_ON(i915_gem_object_has_pinned_pages(obj));
> > > +	if (unlikely(!obj->mm.pages)) {
> > > +		GEM_BUG_ON(i915_gem_object_has_pinned_pages(obj));
> > > +		err = ____i915_gem_object_get_pages(obj);
> > > +		if (err)
> > > +			goto unlock;
> > >  
> > > -	err = ____i915_gem_object_get_pages(obj);
> > > -	if (!err)
> > > -		atomic_set_release(&obj->mm.pages_pin_count, 1);
> > > +		smp_mb__before_atomic();
> > 
> > This is not cool without atomic in sight. Inline wrap as
> > __i915_gem_object_pages_mb() or something.
> 
> My first thought was to put in ____i915_gem_object_get_pages() since it
> closes the action of setting up the obj->mm.pages and co. I didn't like
> that because the association then with the use of the pages_pin_count as
> the mutex was not as apparent. Now that you cannot see the atomic_inc()
> at all here, you are left confused!
> 
> Would you rather this just used the raw atomic_inc() here?

Actually, I like using atomics better here. It is definitely consistent
as we then don't mix the raw atomics and the helpers.
-Chris

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

[PATCH v2] get-fences-locked

[Chris Wilson]

---
 drivers/dma-buf/reservation.c | 58 +++++++++++++++++++++++++++++++++++++++++++
 include/linux/reservation.h   |  4 +++
 2 files changed, 62 insertions(+)

diff --git a/drivers/dma-buf/reservation.c b/drivers/dma-buf/reservation.c
index 3c9ab53be2b9..0f254d0d9bec 100644
--- a/drivers/dma-buf/reservation.c
+++ b/drivers/dma-buf/reservation.c
@@ -133,6 +133,64 @@ void reservation_object_add_excl_fence(struct reservation_object *obj,
 EXPORT_SYMBOL(reservation_object_add_excl_fence);
 
 /**
+ * reservation_object_get_fences_locked - Get an object's shared and exclusive
+ * fences
+ * @obj: the reservation object
+ * @pfence_excl: the returned exclusive fence (or NULL)
+ * @pshared_count: the number of shared fences returned
+ * @pshared: the array of shared fence ptrs returned (array is krealloc'd to
+ * the required size, and must be freed by caller)
+ *
+ * RETURNS
+ * Zero or -errno
+ */
+int reservation_object_get_fences_locked(struct reservation_object *obj,
+				      struct dma_fence **pfence_excl,
+				      unsigned *pshared_count,
+				      struct dma_fence ***pshared)
+{
+	struct dma_fence **shared = NULL;
+	unsigned int count = 0;
+	struct radix_tree_iter iter;
+	void **slot;
+
+	radix_tree_for_each_slot(slot, &obj->shared, &iter, 0) {
+		struct dma_fence *fence = radix_tree_deref_slot(slot);
+
+		if (dma_fence_is_signaled(fence)) {
+			radix_tree_delete(&obj->shared, iter.index);
+			continue;
+		}
+
+		if ((count & -count) == count) {
+			struct dma_fence **nshared;
+			unsigned int sz;
+
+			sz = count ? 2*count : 1;
+			nshared = krealloc(shared,
+					   sz * sizeof(*shared),
+					   GFP_TEMPORARY);
+			if (!nshared) {
+				while (count--)
+					dma_fence_put(shared[count]);
+				kfree(shared);
+				return -ENOMEM;
+			}
+
+			shared = nshared;
+		}
+
+		shared[count++] = dma_fence_get(fence);
+	}
+
+	*pshared_count = count;
+	*pshared = shared;
+	*pfence_excl = dma_fence_get(rcu_dereference(obj->excl));
+	return 0;
+}
+EXPORT_SYMBOL_GPL(reservation_object_get_fences_locked);
+
+/**
  * reservation_object_get_fences_rcu - Get an object's shared and exclusive
  * fences without update side lock held
  * @obj: the reservation object
diff --git a/include/linux/reservation.h b/include/linux/reservation.h
index 697ec52427ca..4f39942906e2 100644
--- a/include/linux/reservation.h
+++ b/include/linux/reservation.h
@@ -161,6 +161,10 @@ void reservation_object_add_shared_fence(struct reservation_object *obj,
 void reservation_object_add_excl_fence(struct reservation_object *obj,
 				       struct dma_fence *fence);
 
+int reservation_object_get_fences_locked(struct reservation_object *obj,
+				      struct dma_fence **pfence_excl,
+				      unsigned *pshared_count,
+				      struct dma_fence ***pshared);
 int reservation_object_get_fences_rcu(struct reservation_object *obj,
 				      struct dma_fence **pfence_excl,
 				      unsigned *pshared_count,
-- 
2.10.2

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

[PATCH v2] drm/i915: Fix pages pin counting around swizzle quirk

[Chris Wilson]

commit bc0629a76726 ("drm/i915: Track pages pinned due to swizzling
quirk") fixed one problem, but revealed a whole lot more. The root cause
of the pin count mismatch for the swizzle quirk (for L-shaped memory on
gen3/4) was that we were incrementing the pages_pin_count upon getting
the backing pages but then overwriting the pages_pin_count to set it to
1 afterwards. With a little bit of adjustment to satisfy the GEM_BUG_ON
sanitychecks, the fix is to replace the explicit atomic_set with an
atomic_inc.

v2: Consistently use atomics (not mix atomics and helpers) within the
lowlevel get_pages routines. This makes the atomic operations much
clearer.

Fixes: 1233e2db199d ("drm/i915: Move object backing storage manipulation")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c        | 47 +++++++++++++++++++---------------
 drivers/gpu/drm/i915/i915_gem_gtt.c    |  7 +++++
 drivers/gpu/drm/i915/i915_gem_tiling.c |  1 +
 3 files changed, 34 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index a7a9ae2c4bce..269e2487c104 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2376,12 +2376,6 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
 	if (i915_gem_object_needs_bit17_swizzle(obj))
 		i915_gem_object_do_bit_17_swizzle(obj, st);
 
-	if (i915_gem_object_is_tiled(obj) &&
-	    dev_priv->quirks & QUIRK_PIN_SWIZZLED_PAGES) {
-		__i915_gem_object_pin_pages(obj);
-		obj->mm.quirked = true;
-	}
-
 	return st;
 
 err_pages:
@@ -2414,12 +2408,21 @@ void __i915_gem_object_set_pages(struct drm_i915_gem_object *obj,
 	obj->mm.get_page.sg_idx = 0;
 
 	obj->mm.pages = pages;
+
+	if (i915_gem_object_is_tiled(obj) &&
+	    to_i915(obj->base.dev)->quirks & QUIRK_PIN_SWIZZLED_PAGES) {
+		GEM_BUG_ON(obj->mm.quirked);
+		__i915_gem_object_pin_pages(obj);
+		obj->mm.quirked = true;
+	}
 }
 
 static int ____i915_gem_object_get_pages(struct drm_i915_gem_object *obj)
 {
 	struct sg_table *pages;
 
+	GEM_BUG_ON(i915_gem_object_has_pinned_pages(obj));
+
 	if (unlikely(obj->mm.madv != I915_MADV_WILLNEED)) {
 		DRM_DEBUG("Attempting to obtain a purgeable object\n");
 		return -EFAULT;
@@ -2448,17 +2451,15 @@ int __i915_gem_object_get_pages(struct drm_i915_gem_object *obj)
 	if (err)
 		return err;
 
-	if (likely(obj->mm.pages)) {
-		__i915_gem_object_pin_pages(obj);
-		goto unlock;
-	}
-
-	GEM_BUG_ON(i915_gem_object_has_pinned_pages(obj));
+	if (unlikely(!obj->mm.pages)) {
+		err = ____i915_gem_object_get_pages(obj);
+		if (err)
+			goto unlock;
 
-	err = ____i915_gem_object_get_pages(obj);
-	if (!err)
-		atomic_set_release(&obj->mm.pages_pin_count, 1);
+		smp_mb__before_atomic();
+	}
 
+	atomic_inc(&obj->mm.pages_pin_count);
 unlock:
 	mutex_unlock(&obj->mm.lock);
 	return err;
@@ -2528,12 +2529,14 @@ void *i915_gem_object_pin_map(struct drm_i915_gem_object *obj,
 
 	pinned = true;
 	if (!atomic_inc_not_zero(&obj->mm.pages_pin_count)) {
-		ret = ____i915_gem_object_get_pages(obj);
-		if (ret)
-			goto err_unlock;
+		if (unlikely(!obj->mm.pages)) {
+			ret = ____i915_gem_object_get_pages(obj);
+			if (ret)
+				goto err_unlock;
 
-		GEM_BUG_ON(atomic_read(&obj->mm.pages_pin_count));
-		atomic_set_release(&obj->mm.pages_pin_count, 1);
+			smp_mb__before_atomic();
+		}
+		atomic_inc(&obj->mm.pages_pin_count);
 		pinned = false;
 	}
 	GEM_BUG_ON(!obj->mm.pages);
@@ -2986,7 +2989,7 @@ int i915_vma_unbind(struct i915_vma *vma)
 		goto destroy;
 
 	GEM_BUG_ON(obj->bind_count == 0);
-	GEM_BUG_ON(!obj->mm.pages);
+	GEM_BUG_ON(!i915_gem_object_has_pinned_pages(obj));
 
 	if (i915_vma_is_map_and_fenceable(vma)) {
 		/* release the fence reg _after_ flushing */
@@ -3220,6 +3223,7 @@ i915_vma_insert(struct i915_vma *vma, u64 size, u64 alignment, u64 flags)
 	list_move_tail(&obj->global_link, &dev_priv->mm.bound_list);
 	list_move_tail(&vma->vm_link, &vma->vm->inactive_list);
 	obj->bind_count++;
+	GEM_BUG_ON(atomic_read(&obj->mm.pages_pin_count) < obj->bind_count);
 
 	return 0;
 
@@ -4272,6 +4276,7 @@ i915_gem_madvise_ioctl(struct drm_device *dev, void *data,
 			obj->mm.quirked = false;
 		}
 		if (args->madv == I915_MADV_WILLNEED) {
+			GEM_BUG_ON(obj->mm.quirked);
 			__i915_gem_object_pin_pages(obj);
 			obj->mm.quirked = true;
 		}
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 3b177c87e862..52999e51a946 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -3711,6 +3711,13 @@ i915_get_ggtt_vma_pages(struct i915_vma *vma)
 {
 	int ret = 0;
 
+	/* The vma->pages are only valid within the lifespan of the borrowed
+	 * obj->mm.pages. When the obj->mm.pages sg_table is regenerated, so
+	 * must be the vma->pages. A simple rule is that vma->pages must only
+	 * be accessed when the obj->mm.pages are pinned.
+	 */
+	GEM_BUG_ON(!i915_gem_object_has_pinned_pages(vma->obj));
+
 	if (vma->pages)
 		return 0;
 
diff --git a/drivers/gpu/drm/i915/i915_gem_tiling.c b/drivers/gpu/drm/i915/i915_gem_tiling.c
index 1577e7810cd6..251d51b01174 100644
--- a/drivers/gpu/drm/i915/i915_gem_tiling.c
+++ b/drivers/gpu/drm/i915/i915_gem_tiling.c
@@ -269,6 +269,7 @@ i915_gem_set_tiling(struct drm_device *dev, void *data,
 					obj->mm.quirked = false;
 				}
 				if (!i915_gem_object_is_tiled(obj)) {
+					GEM_BUG_ON(!obj->mm.quirked);
 					__i915_gem_object_pin_pages(obj);
 					obj->mm.quirked = true;
 				}
-- 
2.10.2

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

✓ Fi.CI.BAT: success for drm/i915: Fix pages pin counting around swizzle quirk (rev3)

[Patchwork]

== Series Details ==

Series: drm/i915: Fix pages pin counting around swizzle quirk (rev3)
URL   : https://patchwork.freedesktop.org/series/14720/
State : success

== Summary ==

Series 14720v3 drm/i915: Fix pages pin counting around swizzle quirk
https://patchwork.freedesktop.org/api/1.0/series/14720/revisions/3/mbox/

Test kms_force_connector_basic:
        Subgroup force-load-detect:
                dmesg-warn -> PASS       (fi-snb-2520m)

fi-bdw-5557u     total:241  pass:226  dwarn:0   dfail:0   fail:0   skip:15 
fi-bsw-n3050     total:241  pass:201  dwarn:0   dfail:0   fail:0   skip:40 
fi-bxt-t5700     total:241  pass:213  dwarn:0   dfail:0   fail:0   skip:28 
fi-byt-j1900     total:241  pass:213  dwarn:0   dfail:0   fail:0   skip:28 
fi-byt-n2820     total:241  pass:209  dwarn:0   dfail:0   fail:0   skip:32 
fi-hsw-4770      total:241  pass:221  dwarn:0   dfail:0   fail:0   skip:20 
fi-hsw-4770r     total:241  pass:221  dwarn:0   dfail:0   fail:0   skip:20 
fi-ilk-650       total:241  pass:188  dwarn:0   dfail:0   fail:0   skip:53 
fi-ivb-3520m     total:241  pass:219  dwarn:0   dfail:0   fail:0   skip:22 
fi-ivb-3770      total:241  pass:219  dwarn:0   dfail:0   fail:0   skip:22 
fi-kbl-7200u     total:241  pass:219  dwarn:0   dfail:0   fail:0   skip:22 
fi-skl-6260u     total:241  pass:227  dwarn:0   dfail:0   fail:0   skip:14 
fi-skl-6700hq    total:241  pass:220  dwarn:0   dfail:0   fail:0   skip:21 
fi-skl-6700k     total:241  pass:219  dwarn:1   dfail:0   fail:0   skip:21 
fi-skl-6770hq    total:241  pass:227  dwarn:0   dfail:0   fail:0   skip:14 
fi-snb-2520m     total:241  pass:209  dwarn:0   dfail:0   fail:0   skip:32 
fi-snb-2600      total:241  pass:208  dwarn:0   dfail:0   fail:0   skip:33 

21f242e536b5077c046df785a8c4c28374941c15 drm-intel-nightly: 2016y-11m-03d-21h-01m-03s UTC integration manifest
25582a0 drm/i915: Fix pages pin counting around swizzle quirk

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_2902/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2] get-fences-locked

[Joonas Lahtinen]

On pe, 2016-11-04 at 10:29 +0000, Chris Wilson wrote:
> ---
>  drivers/dma-buf/reservation.c | 58 +++++++++++++++++++++++++++++++++++++++++++
>  include/linux/reservation.h   |  4 +++
>  2 files changed, 62 insertions(+)

Wrong branch.

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2] drm/i915: Fix pages pin counting around swizzle quirk

[Joonas Lahtinen]

On pe, 2016-11-04 at 10:30 +0000, Chris Wilson wrote:
> @@ -3711,6 +3711,13 @@ i915_get_ggtt_vma_pages(struct i915_vma *vma)
>  {
>  	int ret = 0;
>  
> +	/* The vma->pages are only valid within the lifespan of the borrowed
> +	 * obj->mm.pages. When the obj->mm.pages sg_table is regenerated, so
> +	 * must be the vma->pages. A simple rule is that vma->pages must only
> +	 * be accessed when the obj->mm.pages are pinned.
> +	 */
> +	GEM_BUG_ON(!i915_gem_object_has_pinned_pages(vma->obj));
> +
>  	if (vma->pages)
>  		return 0;

My confusion was vma == obj for the moment, but I think the comment is
still good. The barriers are much more sensible now, too.

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2] drm/i915: Fix pages pin counting around swizzle quirk

[Chris Wilson]

On Fri, Nov 04, 2016 at 01:43:34PM +0200, Joonas Lahtinen wrote:
> On pe, 2016-11-04 at 10:30 +0000, Chris Wilson wrote:
> > @@ -3711,6 +3711,13 @@ i915_get_ggtt_vma_pages(struct i915_vma *vma)
> >  {
> >  	int ret = 0;
> >  
> > +	/* The vma->pages are only valid within the lifespan of the borrowed
> > +	 * obj->mm.pages. When the obj->mm.pages sg_table is regenerated, so
> > +	 * must be the vma->pages. A simple rule is that vma->pages must only
> > +	 * be accessed when the obj->mm.pages are pinned.
> > +	 */
> > +	GEM_BUG_ON(!i915_gem_object_has_pinned_pages(vma->obj));
> > +
> >  	if (vma->pages)
> >  		return 0;
> 
> My confusion was vma == obj for the moment, but I think the comment is
> still good. The barriers are much more sensible now, too.
> 
> Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

* fingers crossed that's the last we see of this quirk.
-Chris

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