* ✓ Fi.CI.BAT: success for drm/i915: Fix pages pin counting around swizzle quirk
2016-11-02 9:43 [PATCH] drm/i915: Fix pages pin counting around swizzle quirk Chris Wilson
@ 2016-11-02 11:16 ` Patchwork
2016-11-03 9:42 ` [PATCH] " Chris Wilson
` (4 subsequent siblings)
5 siblings, 0 replies; 12+ messages in thread
From: Patchwork @ 2016-11-02 11:16 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
== 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
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH] drm/i915: Fix pages pin counting around swizzle quirk
2016-11-02 9:43 [PATCH] drm/i915: Fix pages pin counting around swizzle quirk Chris Wilson
2016-11-02 11:16 ` ✓ Fi.CI.BAT: success for " Patchwork
@ 2016-11-03 9:42 ` Chris Wilson
2016-11-04 8:50 ` Joonas Lahtinen
` (3 subsequent siblings)
5 siblings, 0 replies; 12+ messages in thread
From: Chris Wilson @ 2016-11-03 9:42 UTC (permalink / raw)
To: intel-gfx
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
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH] drm/i915: Fix pages pin counting around swizzle quirk
2016-11-02 9:43 [PATCH] drm/i915: Fix pages pin counting around swizzle quirk Chris Wilson
2016-11-02 11:16 ` ✓ Fi.CI.BAT: success for " Patchwork
2016-11-03 9:42 ` [PATCH] " Chris Wilson
@ 2016-11-04 8:50 ` Joonas Lahtinen
2016-11-04 9:36 ` Chris Wilson
2016-11-04 10:29 ` [PATCH v2] get-fences-locked Chris Wilson
` (2 subsequent siblings)
5 siblings, 1 reply; 12+ messages in thread
From: Joonas Lahtinen @ 2016-11-04 8:50 UTC (permalink / raw)
To: Chris Wilson, intel-gfx
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
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH] drm/i915: Fix pages pin counting around swizzle quirk
2016-11-04 8:50 ` Joonas Lahtinen
@ 2016-11-04 9:36 ` Chris Wilson
2016-11-04 10:26 ` Chris Wilson
0 siblings, 1 reply; 12+ messages in thread
From: Chris Wilson @ 2016-11-04 9:36 UTC (permalink / raw)
To: Joonas Lahtinen; +Cc: intel-gfx
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
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH] drm/i915: Fix pages pin counting around swizzle quirk
2016-11-04 9:36 ` Chris Wilson
@ 2016-11-04 10:26 ` Chris Wilson
0 siblings, 0 replies; 12+ messages in thread
From: Chris Wilson @ 2016-11-04 10:26 UTC (permalink / raw)
To: Joonas Lahtinen, intel-gfx, Tvrtko Ursulin
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v2] get-fences-locked
2016-11-02 9:43 [PATCH] drm/i915: Fix pages pin counting around swizzle quirk Chris Wilson
` (2 preceding siblings ...)
2016-11-04 8:50 ` Joonas Lahtinen
@ 2016-11-04 10:29 ` Chris Wilson
2016-11-04 11:31 ` Joonas Lahtinen
2016-11-04 10:30 ` [PATCH v2] drm/i915: Fix pages pin counting around swizzle quirk Chris Wilson
2016-11-04 11:15 ` ✓ Fi.CI.BAT: success for drm/i915: Fix pages pin counting around swizzle quirk (rev3) Patchwork
5 siblings, 1 reply; 12+ messages in thread
From: Chris Wilson @ 2016-11-04 10:29 UTC (permalink / raw)
To: intel-gfx
---
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
^ permalink raw reply related [flat|nested] 12+ messages in thread* [PATCH v2] drm/i915: Fix pages pin counting around swizzle quirk
2016-11-02 9:43 [PATCH] drm/i915: Fix pages pin counting around swizzle quirk Chris Wilson
` (3 preceding siblings ...)
2016-11-04 10:29 ` [PATCH v2] get-fences-locked Chris Wilson
@ 2016-11-04 10:30 ` Chris Wilson
2016-11-04 11:43 ` Joonas Lahtinen
2016-11-04 11:15 ` ✓ Fi.CI.BAT: success for drm/i915: Fix pages pin counting around swizzle quirk (rev3) Patchwork
5 siblings, 1 reply; 12+ messages in thread
From: Chris Wilson @ 2016-11-04 10:30 UTC (permalink / raw)
To: intel-gfx
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
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH v2] drm/i915: Fix pages pin counting around swizzle quirk
2016-11-04 10:30 ` [PATCH v2] drm/i915: Fix pages pin counting around swizzle quirk Chris Wilson
@ 2016-11-04 11:43 ` Joonas Lahtinen
2016-11-04 11:57 ` Chris Wilson
0 siblings, 1 reply; 12+ messages in thread
From: Joonas Lahtinen @ 2016-11-04 11:43 UTC (permalink / raw)
To: Chris Wilson, intel-gfx
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
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH v2] drm/i915: Fix pages pin counting around swizzle quirk
2016-11-04 11:43 ` Joonas Lahtinen
@ 2016-11-04 11:57 ` Chris Wilson
0 siblings, 0 replies; 12+ messages in thread
From: Chris Wilson @ 2016-11-04 11:57 UTC (permalink / raw)
To: Joonas Lahtinen; +Cc: intel-gfx
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* ✓ Fi.CI.BAT: success for drm/i915: Fix pages pin counting around swizzle quirk (rev3)
2016-11-02 9:43 [PATCH] drm/i915: Fix pages pin counting around swizzle quirk Chris Wilson
` (4 preceding siblings ...)
2016-11-04 10:30 ` [PATCH v2] drm/i915: Fix pages pin counting around swizzle quirk Chris Wilson
@ 2016-11-04 11:15 ` Patchwork
5 siblings, 0 replies; 12+ messages in thread
From: Patchwork @ 2016-11-04 11:15 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
== 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
^ permalink raw reply [flat|nested] 12+ messages in thread