[PATCH] drm/i915: Reorder context-close to avoid calling i915_vma_close() under RCU

[PATCH] drm/i915: Reorder context-close to avoid calling i915_vma_close() under RCU

[Chris Wilson]

When we close the VMA, we unbind it from the ppgtt and tear down the
page directory pointing at it. That may trigger us to return WC pages
back to the system, requiring conversion back to WB which itself may
sleep. That makes i915_vma_close() unsuitable for use inside the RCU
read lock, which we need to hold to iterate the radixtree.

The fix is quite simple, we can close all the VMA as we close the ppgtt,
we only need to do that instead of closing them during destruction of
the LUT.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=103638
Fixes: 547da76b5777 ("drm/i915: Hold rcu_read_lock when iterating over the radixtree (vma idr)")
Fixes: d1b48c1e7184 ("drm/i915: Replace execbuf vma ht with an idr")
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Matthew Auld <matthew.william.auld@gmail.com>
---
 drivers/gpu/drm/i915/i915_gem_context.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 10affb35ac56..0e1c4626870e 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -107,14 +107,9 @@ static void lut_close(struct i915_gem_context *ctx)
 	rcu_read_lock();
 	radix_tree_for_each_slot(slot, &ctx->handles_vma, &iter, 0) {
 		struct i915_vma *vma = rcu_dereference_raw(*slot);
-		struct drm_i915_gem_object *obj = vma->obj;
 
 		radix_tree_iter_delete(&ctx->handles_vma, &iter, slot);
-
-		if (!i915_vma_is_ggtt(vma))
-			i915_vma_close(vma);
-
-		__i915_gem_object_release_unless_active(obj);
+		__i915_gem_object_release_unless_active(vma->obj);
 	}
 	rcu_read_unlock();
 }
@@ -200,10 +195,11 @@ static void context_close(struct i915_gem_context *ctx)
 {
 	i915_gem_context_set_closed(ctx);
 
-	lut_close(ctx);
 	if (ctx->ppgtt)
 		i915_ppgtt_close(&ctx->ppgtt->base);
 
+	lut_close(ctx);
+
 	ctx->file_priv = ERR_PTR(-EBADF);
 	i915_gem_context_put(ctx);
 }
-- 
2.15.0

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

Re: [PATCH] drm/i915: Reorder context-close to avoid calling i915_vma_close() under RCU

[Tvrtko Ursulin]

On 09/11/2017 08:20, Chris Wilson wrote:
> When we close the VMA, we unbind it from the ppgtt and tear down the
> page directory pointing at it. That may trigger us to return WC pages
> back to the system, requiring conversion back to WB which itself may
> sleep. That makes i915_vma_close() unsuitable for use inside the RCU
> read lock, which we need to hold to iterate the radixtree.
> 
> The fix is quite simple, we can close all the VMA as we close the ppgtt,
> we only need to do that instead of closing them during destruction of
> the LUT.
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=103638
> Fixes: 547da76b5777 ("drm/i915: Hold rcu_read_lock when iterating over the radixtree (vma idr)")
> Fixes: d1b48c1e7184 ("drm/i915: Replace execbuf vma ht with an idr")
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Matthew Auld <matthew.william.auld@gmail.com>
> ---
>   drivers/gpu/drm/i915/i915_gem_context.c | 10 +++-------
>   1 file changed, 3 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index 10affb35ac56..0e1c4626870e 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -107,14 +107,9 @@ static void lut_close(struct i915_gem_context *ctx)
>   	rcu_read_lock();
>   	radix_tree_for_each_slot(slot, &ctx->handles_vma, &iter, 0) {
>   		struct i915_vma *vma = rcu_dereference_raw(*slot);
> -		struct drm_i915_gem_object *obj = vma->obj;
>   
>   		radix_tree_iter_delete(&ctx->handles_vma, &iter, slot);
> -
> -		if (!i915_vma_is_ggtt(vma))
> -			i915_vma_close(vma);
> -
> -		__i915_gem_object_release_unless_active(obj);
> +		__i915_gem_object_release_unless_active(vma->obj);
>   	}
>   	rcu_read_unlock();
>   }
> @@ -200,10 +195,11 @@ static void context_close(struct i915_gem_context *ctx)
>   {
>   	i915_gem_context_set_closed(ctx);
>   
> -	lut_close(ctx);
>   	if (ctx->ppgtt)
>   		i915_ppgtt_close(&ctx->ppgtt->base);
>   
> +	lut_close(ctx);
> +
>   	ctx->file_priv = ERR_PTR(-EBADF);
>   	i915_gem_context_put(ctx);
>   }
> 

Looks straightforward enough. Was a bit unsure about how the pre-ppgtt 
world works but that too looks fine.

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

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

✗ Fi.CI.BAT: failure for drm/i915: Reorder context-close to avoid calling i915_vma_close() under RCU

[Patchwork]

== Series Details ==

Series: drm/i915: Reorder context-close to avoid calling i915_vma_close() under RCU
URL   : https://patchwork.freedesktop.org/series/33491/
State : failure

== Summary ==

Series 33491v1 drm/i915: Reorder context-close to avoid calling i915_vma_close() under RCU
https://patchwork.freedesktop.org/api/1.0/series/33491/revisions/1/mbox/

Test gem_ctx_switch:
        Subgroup basic-default:
                pass       -> INCOMPLETE (fi-bdw-5557u)
                pass       -> INCOMPLETE (fi-bdw-gvtdvm)
                pass       -> INCOMPLETE (fi-bsw-n3050)
        Subgroup basic-default-heavy:
                pass       -> INCOMPLETE (fi-kbl-7567u) fdo#103165

fdo#103165 https://bugs.freedesktop.org/show_bug.cgi?id=103165

fi-bdw-5557u     total:31   pass:21   dwarn:0   dfail:0   fail:0   skip:9  
fi-bdw-gvtdvm    total:31   pass:21   dwarn:0   dfail:0   fail:0   skip:9  
fi-blb-e6850     total:289  pass:223  dwarn:1   dfail:0   fail:0   skip:65  time:376s
fi-bsw-n3050     total:31   pass:21   dwarn:0   dfail:0   fail:0   skip:9  
fi-bwr-2160      total:289  pass:183  dwarn:0   dfail:0   fail:0   skip:106 time:275s
fi-bxt-dsi       total:289  pass:259  dwarn:0   dfail:0   fail:0   skip:30  time:504s
fi-bxt-j4205     total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:502s
fi-byt-j1900     total:289  pass:254  dwarn:0   dfail:0   fail:0   skip:35  time:504s
fi-byt-n2820     total:289  pass:250  dwarn:0   dfail:0   fail:0   skip:39  time:488s
fi-elk-e7500     total:289  pass:229  dwarn:0   dfail:0   fail:0   skip:60  time:436s
fi-gdg-551       total:289  pass:178  dwarn:1   dfail:0   fail:1   skip:109 time:263s
fi-glk-1         total:289  pass:261  dwarn:0   dfail:0   fail:0   skip:28  time:541s
fi-hsw-4770      total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:459s
fi-hsw-4770r     total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:438s
fi-ilk-650       total:289  pass:228  dwarn:0   dfail:0   fail:0   skip:61  time:428s
fi-ivb-3520m     total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:485s
fi-ivb-3770      total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:463s
fi-kbl-7500u     total:289  pass:264  dwarn:1   dfail:0   fail:0   skip:24  time:491s
fi-kbl-7560u     total:289  pass:270  dwarn:0   dfail:0   fail:0   skip:19  time:524s
fi-kbl-7567u     total:32   pass:22   dwarn:0   dfail:0   fail:0   skip:9  
fi-kbl-r         total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:533s
fi-pnv-d510      total:289  pass:222  dwarn:1   dfail:0   fail:0   skip:66  time:569s
fi-skl-6260u     total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:465s
fi-skl-6600u     total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:546s
fi-skl-6700hq    total:289  pass:263  dwarn:0   dfail:0   fail:0   skip:26  time:560s
fi-skl-6700k     total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:522s
fi-skl-6770hq    total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:501s
fi-skl-gvtdvm    total:289  pass:266  dwarn:0   dfail:0   fail:0   skip:23  time:455s
fi-snb-2520m     total:289  pass:250  dwarn:0   dfail:0   fail:0   skip:39  time:562s
fi-snb-2600      total:289  pass:249  dwarn:0   dfail:0   fail:0   skip:40  time:424s
Blacklisted hosts:
fi-cnl-y         total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:584s

1446a30948d7c5570e5cfc40678dbdd74e152498 drm-tip: 2017y-11m-08d-21h-46m-48s UTC integration manifest
69d76561965b drm/i915: Reorder context-close to avoid calling i915_vma_close() under RCU

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7025/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Reorder context-close to avoid calling i915_vma_close() under RCU

[Chris Wilson]

Quoting Chris Wilson (2017-11-09 08:20:48)
> When we close the VMA, we unbind it from the ppgtt and tear down the
> page directory pointing at it. That may trigger us to return WC pages
> back to the system, requiring conversion back to WB which itself may
> sleep. That makes i915_vma_close() unsuitable for use inside the RCU
> read lock, which we need to hold to iterate the radixtree.
> 
> The fix is quite simple, we can close all the VMA as we close the ppgtt,
> we only need to do that instead of closing them during destruction of
> the LUT.

The flaw is that i915_vma_close() frees the vma; destroying our
backpointer from the lut to the object. Definitely time for tea. :(
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2] drm/i915: Reorder context-close to avoid calling i915_vma_close() under RCU

[Chris Wilson]

When we close the VMA, we unbind it from the ppgtt and tear down the
page directory pointing at it. That may trigger us to return WC pages
back to the system, requiring conversion back to WB which itself may
sleep. That makes i915_vma_close() unsuitable for use inside the RCU
read lock, which we need to hold to iterate the radixtree.

The fix is quite simple, we can close all the VMA as we close the ppgtt,
we only need to do that instead of closing them during destruction of
the LUT.

v2: Order between closing the LUT and the ppgtt is important; we use the
vma inside the LUT as a means of retrieving the object, and so we must
clear the LUT before freeing the VMA when closing the ppgtt.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=103638
Fixes: 547da76b5777 ("drm/i915: Hold rcu_read_lock when iterating over the radixtree (vma idr)")
Fixes: d1b48c1e7184 ("drm/i915: Replace execbuf vma ht with an idr")
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Matthew Auld <matthew.william.auld@gmail.com>
---
 drivers/gpu/drm/i915/i915_gem_context.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 10affb35ac56..c05c3d7d21a5 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -107,14 +107,9 @@ static void lut_close(struct i915_gem_context *ctx)
 	rcu_read_lock();
 	radix_tree_for_each_slot(slot, &ctx->handles_vma, &iter, 0) {
 		struct i915_vma *vma = rcu_dereference_raw(*slot);
-		struct drm_i915_gem_object *obj = vma->obj;
 
 		radix_tree_iter_delete(&ctx->handles_vma, &iter, slot);
-
-		if (!i915_vma_is_ggtt(vma))
-			i915_vma_close(vma);
-
-		__i915_gem_object_release_unless_active(obj);
+		__i915_gem_object_release_unless_active(vma->obj);
 	}
 	rcu_read_unlock();
 }
@@ -200,6 +195,11 @@ static void context_close(struct i915_gem_context *ctx)
 {
 	i915_gem_context_set_closed(ctx);
 
+	/*
+	 * The LUT uses the VMA as a backpointer to unref the object,
+	 * so we need to clear the LUT before we close all the VMA (inside
+	 * the ppgtt).
+	 */
 	lut_close(ctx);
 	if (ctx->ppgtt)
 		i915_ppgtt_close(&ctx->ppgtt->base);
-- 
2.15.0

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

Re: [PATCH v2] drm/i915: Reorder context-close to avoid calling i915_vma_close() under RCU

[Tvrtko Ursulin]

On 09/11/2017 08:55, Chris Wilson wrote:
> When we close the VMA, we unbind it from the ppgtt and tear down the
> page directory pointing at it. That may trigger us to return WC pages
> back to the system, requiring conversion back to WB which itself may
> sleep. That makes i915_vma_close() unsuitable for use inside the RCU
> read lock, which we need to hold to iterate the radixtree.
> 
> The fix is quite simple, we can close all the VMA as we close the ppgtt,
> we only need to do that instead of closing them during destruction of
> the LUT.
> 
> v2: Order between closing the LUT and the ppgtt is important; we use the
> vma inside the LUT as a means of retrieving the object, and so we must
> clear the LUT before freeing the VMA when closing the ppgtt.
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=103638
> Fixes: 547da76b5777 ("drm/i915: Hold rcu_read_lock when iterating over the radixtree (vma idr)")
> Fixes: d1b48c1e7184 ("drm/i915: Replace execbuf vma ht with an idr")
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Matthew Auld <matthew.william.auld@gmail.com>
> ---
>   drivers/gpu/drm/i915/i915_gem_context.c | 12 ++++++------
>   1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index 10affb35ac56..c05c3d7d21a5 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -107,14 +107,9 @@ static void lut_close(struct i915_gem_context *ctx)
>   	rcu_read_lock();
>   	radix_tree_for_each_slot(slot, &ctx->handles_vma, &iter, 0) {
>   		struct i915_vma *vma = rcu_dereference_raw(*slot);
> -		struct drm_i915_gem_object *obj = vma->obj;
>   
>   		radix_tree_iter_delete(&ctx->handles_vma, &iter, slot);
> -
> -		if (!i915_vma_is_ggtt(vma))
> -			i915_vma_close(vma);
> -
> -		__i915_gem_object_release_unless_active(obj);
> +		__i915_gem_object_release_unless_active(vma->obj);
>   	}
>   	rcu_read_unlock();
>   }
> @@ -200,6 +195,11 @@ static void context_close(struct i915_gem_context *ctx)
>   {
>   	i915_gem_context_set_closed(ctx);
>   
> +	/*
> +	 * The LUT uses the VMA as a backpointer to unref the object,
> +	 * so we need to clear the LUT before we close all the VMA (inside
> +	 * the ppgtt).
> +	 */
>   	lut_close(ctx);
>   	if (ctx->ppgtt)
>   		i915_ppgtt_close(&ctx->ppgtt->base);
> 

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

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

✓ Fi.CI.BAT: success for drm/i915: Reorder context-close to avoid calling i915_vma_close() under RCU (rev2)

[Patchwork]

== Series Details ==

Series: drm/i915: Reorder context-close to avoid calling i915_vma_close() under RCU (rev2)
URL   : https://patchwork.freedesktop.org/series/33491/
State : success

== Summary ==

Series 33491v2 drm/i915: Reorder context-close to avoid calling i915_vma_close() under RCU
https://patchwork.freedesktop.org/api/1.0/series/33491/revisions/2/mbox/

Test gem_ctx_basic:
                fail       -> SKIP       (fi-gdg-551)
Test gem_exec_reloc:
        Subgroup basic-cpu-active:
                fail       -> PASS       (fi-gdg-551) fdo#102582 +4
Test gem_sync:
        Subgroup basic-store-all:
                fail       -> PASS       (fi-ivb-3520m) fdo#100007
Test drv_module_reload:
        Subgroup basic-reload:
                pass       -> DMESG-WARN (fi-bsw-n3050) fdo#103479

fdo#102582 https://bugs.freedesktop.org/show_bug.cgi?id=102582
fdo#100007 https://bugs.freedesktop.org/show_bug.cgi?id=100007
fdo#103479 https://bugs.freedesktop.org/show_bug.cgi?id=103479

fi-bdw-5557u     total:289  pass:268  dwarn:0   dfail:0   fail:0   skip:21  time:442s
fi-bdw-gvtdvm    total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:456s
fi-blb-e6850     total:289  pass:223  dwarn:1   dfail:0   fail:0   skip:65  time:381s
fi-bsw-n3050     total:289  pass:242  dwarn:1   dfail:0   fail:0   skip:46  time:537s
fi-bwr-2160      total:289  pass:183  dwarn:0   dfail:0   fail:0   skip:106 time:273s
fi-bxt-dsi       total:289  pass:259  dwarn:0   dfail:0   fail:0   skip:30  time:505s
fi-bxt-j4205     total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:508s
fi-byt-j1900     total:289  pass:254  dwarn:0   dfail:0   fail:0   skip:35  time:493s
fi-byt-n2820     total:289  pass:250  dwarn:0   dfail:0   fail:0   skip:39  time:477s
fi-elk-e7500     total:289  pass:229  dwarn:0   dfail:0   fail:0   skip:60  time:426s
fi-gdg-551       total:289  pass:178  dwarn:1   dfail:0   fail:1   skip:109 time:263s
fi-glk-1         total:289  pass:261  dwarn:0   dfail:0   fail:0   skip:28  time:540s
fi-hsw-4770      total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:429s
fi-hsw-4770r     total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:440s
fi-ilk-650       total:289  pass:228  dwarn:0   dfail:0   fail:0   skip:61  time:428s
fi-ivb-3520m     total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:482s
fi-ivb-3770      total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:461s
fi-kbl-7500u     total:289  pass:264  dwarn:1   dfail:0   fail:0   skip:24  time:485s
fi-kbl-7560u     total:289  pass:270  dwarn:0   dfail:0   fail:0   skip:19  time:523s
fi-kbl-7567u     total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:472s
fi-kbl-r         total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:532s
fi-pnv-d510      total:289  pass:222  dwarn:1   dfail:0   fail:0   skip:66  time:568s
fi-skl-6260u     total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:452s
fi-skl-6600u     total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:548s
fi-skl-6700hq    total:289  pass:263  dwarn:0   dfail:0   fail:0   skip:26  time:562s
fi-skl-6700k     total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:515s
fi-skl-6770hq    total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:494s
fi-skl-gvtdvm    total:289  pass:266  dwarn:0   dfail:0   fail:0   skip:23  time:465s
fi-snb-2520m     total:289  pass:250  dwarn:0   dfail:0   fail:0   skip:39  time:558s
fi-snb-2600      total:289  pass:249  dwarn:0   dfail:0   fail:0   skip:40  time:424s
Blacklisted hosts:
fi-cnl-y         total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:561s
fi-glk-dsi       total:289  pass:259  dwarn:0   dfail:0   fail:0   skip:30  time:491s

65dc54b704d3ee0486f9f5b11f00c28973f783a2 drm-tip: 2017y-11m-09d-08h-53m-46s UTC integration manifest
b5ede6ccb319 drm/i915: Reorder context-close to avoid calling i915_vma_close() under RCU

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7027/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2] drm/i915: Reorder context-close to avoid calling i915_vma_close() under RCU

[Tvrtko Ursulin]

On 09/11/2017 09:32, Tvrtko Ursulin wrote:
> 
> On 09/11/2017 08:55, Chris Wilson wrote:
>> When we close the VMA, we unbind it from the ppgtt and tear down the
>> page directory pointing at it. That may trigger us to return WC pages
>> back to the system, requiring conversion back to WB which itself may
>> sleep. That makes i915_vma_close() unsuitable for use inside the RCU
>> read lock, which we need to hold to iterate the radixtree.

Would it be worth adding might_sleep to i915_vma_close? Or at a lower layer?

Regards,

Tvrtko

>>
>> The fix is quite simple, we can close all the VMA as we close the ppgtt,
>> we only need to do that instead of closing them during destruction of
>> the LUT.
>>
>> v2: Order between closing the LUT and the ppgtt is important; we use the
>> vma inside the LUT as a means of retrieving the object, and so we must
>> clear the LUT before freeing the VMA when closing the ppgtt.
>>
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=103638
>> Fixes: 547da76b5777 ("drm/i915: Hold rcu_read_lock when iterating over 
>> the radixtree (vma idr)")
>> Fixes: d1b48c1e7184 ("drm/i915: Replace execbuf vma ht with an idr")
>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>> Cc: Matthew Auld <matthew.william.auld@gmail.com>
>> ---
>>   drivers/gpu/drm/i915/i915_gem_context.c | 12 ++++++------
>>   1 file changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c 
>> b/drivers/gpu/drm/i915/i915_gem_context.c
>> index 10affb35ac56..c05c3d7d21a5 100644
>> --- a/drivers/gpu/drm/i915/i915_gem_context.c
>> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
>> @@ -107,14 +107,9 @@ static void lut_close(struct i915_gem_context *ctx)
>>       rcu_read_lock();
>>       radix_tree_for_each_slot(slot, &ctx->handles_vma, &iter, 0) {
>>           struct i915_vma *vma = rcu_dereference_raw(*slot);
>> -        struct drm_i915_gem_object *obj = vma->obj;
>>           radix_tree_iter_delete(&ctx->handles_vma, &iter, slot);
>> -
>> -        if (!i915_vma_is_ggtt(vma))
>> -            i915_vma_close(vma);
>> -
>> -        __i915_gem_object_release_unless_active(obj);
>> +        __i915_gem_object_release_unless_active(vma->obj);
>>       }
>>       rcu_read_unlock();
>>   }
>> @@ -200,6 +195,11 @@ static void context_close(struct i915_gem_context 
>> *ctx)
>>   {
>>       i915_gem_context_set_closed(ctx);
>> +    /*
>> +     * The LUT uses the VMA as a backpointer to unref the object,
>> +     * so we need to clear the LUT before we close all the VMA (inside
>> +     * the ppgtt).
>> +     */
>>       lut_close(ctx);
>>       if (ctx->ppgtt)
>>           i915_ppgtt_close(&ctx->ppgtt->base);
>>
> 
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> Regards,
> 
> Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2] drm/i915: Reorder context-close to avoid calling i915_vma_close() under RCU

[Chris Wilson]

Quoting Tvrtko Ursulin (2017-11-09 10:00:27)
> 
> On 09/11/2017 09:32, Tvrtko Ursulin wrote:
> > 
> > On 09/11/2017 08:55, Chris Wilson wrote:
> >> When we close the VMA, we unbind it from the ppgtt and tear down the
> >> page directory pointing at it. That may trigger us to return WC pages
> >> back to the system, requiring conversion back to WB which itself may
> >> sleep. That makes i915_vma_close() unsuitable for use inside the RCU
> >> read lock, which we need to hold to iterate the radixtree.
> 
> Would it be worth adding might_sleep to i915_vma_close? Or at a lower layer?

vm_free_page() would be my first choice. i915_vma_close() is also a good
candidate, but I'm worrying if that's too disconnected with the reason
why it might_sleep(). Most of the work within (unbind if not active)
could be atomic as it is just poking at our pagetables. It's just that
changing the PAT is not lazy and uses a stop_machine to change the view
on all CPUs at once. For our purposes we could do with an arch/x86 WC
allocator. Fortunately (or unfortunately depending on pov) we don't need
them in enough bulk to justify pursuing a global scheme, though if one
turned up we would happily make use of it.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.IGT: success for drm/i915: Reorder context-close to avoid calling i915_vma_close() under RCU (rev2)

[Patchwork]

== Series Details ==

Series: drm/i915: Reorder context-close to avoid calling i915_vma_close() under RCU (rev2)
URL   : https://patchwork.freedesktop.org/series/33491/
State : success

== Summary ==

Test perf:
        Subgroup polling:
                pass       -> FAIL       (shard-hsw) fdo#102252
Test kms_flip:
        Subgroup dpms-vs-vblank-race-interruptible:
                pass       -> FAIL       (shard-hsw) fdo#103060
Test kms_pipe_crc_basic:
        Subgroup nonblocking-crc-pipe-a:
                skip       -> PASS       (shard-hsw)
Test pm_lpsp:
        Subgroup screens-disabled:
                fail       -> PASS       (shard-hsw) fdo#102579

fdo#102252 https://bugs.freedesktop.org/show_bug.cgi?id=102252
fdo#103060 https://bugs.freedesktop.org/show_bug.cgi?id=103060
fdo#102579 https://bugs.freedesktop.org/show_bug.cgi?id=102579

shard-hsw        total:2540 pass:1429 dwarn:0   dfail:0   fail:12  skip:1099 time:9278s

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7027/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx