drm/i915: use correct node for handling cache domain eviction
Subsystems (2)
PATCH 1/2 drm/i915: use correct node for handling cache domain eviction
It looks like we were incorrectly comparing vma->node against itself instead of the target node, when evicting for a node on systems where we need guard pages between regions with different cache domains. As a consequence we can end up trying to needlessly evict neighbouring nodes, even if they have the same cache domain, and if they were pinned we would fail the eviction.
--- drivers/gpu/drm/i915/i915_gem_evict.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
drivers/gpu/drm/i915/i915_gem_evict.c +4 -4
@@ -299,12 +299,12 @@ int i915_gem_evict_for_node(struct i915_address_space *vm,* those as well to make room for our guard pages. */ if (check_color) { -if (vma->node.start + vma->node.size == node->start) { -if (vma->node.color == node->color) +if (node->start + node->size == target->start) { +if (node->color == target->color)continue ; } -if (vma->node.start == node->start + node->size) { -if (vma->node.color == node->color) +if (node->start == target->start + target->size) { +if (node->color == target->color)continue ; } }
Signature · 6 lines
2.9.3 _______________________________________________ Intel-gfx mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/intel-gfx
PATCH 2/2 drm/i915/selftests: exercise cache domain eviction
--- drivers/gpu/drm/i915/selftests/i915_gem_evict.c | 80 +++++++++++++++++++++++++ 1 file changed, 80 insertions(+)
drivers/gpu/drm/i915/selftests/i915_gem_evict.c +80 -0
@@ -202,6 +202,85 @@ static int igt_evict_for_vma(void *arg)return err; } +static void mock_color_adjust (const struct drm_mm_node *node, +unsigned long color, +u64 *start, +u64 *end) +{ +} + +static int igt_evict_for_cache_color (void *arg) +{ +struct drm_i915_private *i915 = arg; +struct i915_ggtt *ggtt = &i915->ggtt; +const unsigned long flags =PIN_OFFSET_FIXED ; +struct drm_mm_node target = { + .start =I915_GTT_PAGE_SIZE *2 , + .size =I915_GTT_PAGE_SIZE , + .color =I915_CACHE_LLC , + }; +struct drm_i915_gem_object *obj; +struct i915_vma *vma; +int err; + + ggtt->base.mm.color_adjust = mock_color_adjust; + + obj =i915_gem_object_create_internal (i915,I915_GTT_PAGE_SIZE ); +if (IS_ERR (obj)) { + err =PTR_ERR (obj); + goto cleanup; + } +i915_gem_object_set_cache_level (obj,I915_CACHE_LLC ); + + vma =i915_gem_object_ggtt_pin (obj,NULL ,0 ,0 , +I915_GTT_PAGE_SIZE | flags); +if (IS_ERR (vma)) { +pr_err ("[0]i915_gem_object_ggtt_pin failed\n" ); + err =PTR_ERR (vma); + goto cleanup; + } + + obj =i915_gem_object_create_internal (i915,I915_GTT_PAGE_SIZE ); +if (IS_ERR (obj)) { + err =PTR_ERR (obj); + goto cleanup; + } +i915_gem_object_set_cache_level (obj,I915_CACHE_LLC ); + + vma =i915_gem_object_ggtt_pin (obj,NULL ,0 ,0 , + (I915_GTT_PAGE_SIZE *2 ) | flags); +if (IS_ERR (vma)) { +pr_err ("[1]i915_gem_object_ggtt_pin failed\n" ); + err =PTR_ERR (vma); + goto cleanup; + } + +i915_vma_unpin (vma); + + err =i915_gem_evict_for_node (&ggtt->base, &target,0 ); +if (err) { +pr_err ("[0]i915_gem_evict_for_node returned err=%d\n" , err); + goto cleanup; + } + + target.color =I915_CACHE_L3_LLC ; + + err =i915_gem_evict_for_node (&ggtt->base, &target,0 ); +if (!err) { +pr_err ("[1]i915_gem_evict_for_node returned err=%d\n" , err); + err = -EINVAL ; + goto cleanup; + } + + err =0 ; + +cleanup: +unpin_ggtt (i915); +cleanup_objects (i915); + ggtt->base.mm.color_adjust =NULL ; +return err; +} +static int igt_evict_vm (void *arg) {struct drm_i915_private *i915 = arg;
@@ -241,6 +320,7 @@ int i915_gem_evict_mock_selftests(void)static const struct i915_subtest tests[] = {SUBTEST (igt_evict_something),SUBTEST (igt_evict_for_vma), +SUBTEST (igt_evict_for_cache_color),SUBTEST (igt_evict_vm),SUBTEST (igt_overcommit), };
Signature · 6 lines
2.9.3 _______________________________________________ Intel-gfx mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [PATCH 2/2] drm/i915/selftests: exercise cache domain eviction
Quoted 10 lines · Matthew Auld
Signed-off-by Matthew Auld <[email protected]>--- drivers/gpu/drm/i915/selftests/i915_gem_evict.c | 80 +++++++++++++++++++++++++ 1 file changed, 80 insertions(+)drivers/gpu/drm/i915/selftests/i915_gem_evict.c +0
-0
drivers/gpu/drm/i915/selftests/i915_gem_evict.c static int igt_evict_for_vma(void *arg) · line 209
Quoted 8 lines
drivers/gpu/drm/i915/selftests/i915_gem_evict.c +5
-0@@ -202,6 +202,85 @@ static int igt_evict_for_vma(void *arg)return err; } +static void mock_color_adjust (const struct drm_mm_node *node, +unsigned long color, +u64 *start, +u64 *end) +{
Ah. I see, this is because evict_for_node makes the assumption that any color_adjust is i915_ggtt_color_adjust. I was wondering. Please add a note here about the magic.
Quoted 30 lines
drivers/gpu/drm/i915/selftests/i915_gem_evict.c +30
-0+} + +static int igt_evict_for_cache_color (void *arg) +{ +struct drm_i915_private *i915 = arg; +struct i915_ggtt *ggtt = &i915->ggtt; +const unsigned long flags =PIN_OFFSET_FIXED ; +struct drm_mm_node target = { + .start =I915_GTT_PAGE_SIZE *2 , + .size =I915_GTT_PAGE_SIZE , + .color =I915_CACHE_LLC , + }; +struct drm_i915_gem_object *obj; +struct i915_vma *vma; +int err; + + ggtt->base.mm.color_adjust = mock_color_adjust; + + obj =i915_gem_object_create_internal (i915,I915_GTT_PAGE_SIZE ); +if (IS_ERR (obj)) { + err =PTR_ERR (obj); + goto cleanup; + } +i915_gem_object_set_cache_level (obj,I915_CACHE_LLC ); + + vma =i915_gem_object_ggtt_pin (obj,NULL ,0 ,0 , +I915_GTT_PAGE_SIZE | flags); +if (IS_ERR (vma)) { +pr_err ("[0]i915_gem_object_ggtt_pin failed\n" ); + err =PTR_ERR (vma);
drivers/gpu/drm/i915/selftests/i915_gem_evict.c static int igt_evict_for_vma(void *arg) · line 249
Quoted 10 lines
drivers/gpu/drm/i915/selftests/i915_gem_evict.c +10
-0+ goto cleanup; + } + + obj =i915_gem_object_create_internal (i915,I915_GTT_PAGE_SIZE ); +if (IS_ERR (obj)) { + err =PTR_ERR (obj); + goto cleanup; + } +i915_gem_object_set_cache_level (obj,I915_CACHE_LLC ); +
/* Neighbouring; same colour - should fit */
drivers/gpu/drm/i915/selftests/i915_gem_evict.c static int igt_evict_for_vma(void *arg) · line 259
Quoted 10 lines
drivers/gpu/drm/i915/selftests/i915_gem_evict.c +10
-0+ vma =i915_gem_object_ggtt_pin (obj,NULL ,0 ,0 , + (I915_GTT_PAGE_SIZE *2 ) | flags); +if (IS_ERR (vma)) { +pr_err ("[1]i915_gem_object_ggtt_pin failed\n" ); + err =PTR_ERR (vma); + goto cleanup; + } + +i915_vma_unpin (vma); +
/* Remove just the second vma */
drivers/gpu/drm/i915/selftests/i915_gem_evict.c static int igt_evict_for_vma(void *arg) · line 265
Quoted 6 lines
drivers/gpu/drm/i915/selftests/i915_gem_evict.c +6
-0+ err =i915_gem_evict_for_node (&ggtt->base, &target,0 ); +if (err) { +pr_err ("[0]i915_gem_evict_for_node returned err=%d\n" , err); + goto cleanup; + } +
/* Attempt to remove the first *pinned* vma, by removing the (empty) neighbour */
drivers/gpu/drm/i915/selftests/i915_gem_evict.c static int igt_evict_for_vma(void *arg) · line 272
Quoted 7 lines
drivers/gpu/drm/i915/selftests/i915_gem_evict.c +7
-0+ target.color =I915_CACHE_L3_LLC ; + + err =i915_gem_evict_for_node (&ggtt->base, &target,0 ); +if (!err) { +pr_err ("[1]i915_gem_evict_for_node returned err=%d\n" , err); + err = -EINVAL ; + goto cleanup;
Your pr_err("[]") are as bad as mine ;)
I see what you're testing, looks ok. So just a couple of comments for the next reader.
-Chris
Signature · 5 lines
Chris Wilson, Intel Open Source Technology Centre _______________________________________________ Intel-gfx mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Quoted 11 lines · Matthew Auld
It looks like we were incorrectly comparing
vma->nodeagainst itself instead of the target node, when evicting for a node on systems where we need guard pages between regions with different cache domains. As a consequence we can end up trying to needlessly evict neighbouring nodes, even if they have the same cache domain, and if they were pinned we would fail the eviction.Fixes 625d988acc28 ("drm/i915: Extract reserving space in the GTT to a helper")Signed-off-by Matthew Auld <[email protected]>Cc Chris Wilson <[email protected]>Cc Joonas Lahtinen <[email protected]>
-Chris
Signature · 5 lines
Chris Wilson, Intel Open Source Technology Centre _______________________________________________ Intel-gfx mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Joonas Lahtinen
maintainerQuoted 9 lines · Chris Wilson
Quoted 8 lines · Matthew Auld
It looks like we were incorrectly comparing
vma->nodeagainst itself instead of the target node, when evicting for a node on systems where we need guard pages between regions with different cache domains. As a consequence we can end up trying to needlessly evict neighbouring nodes, even if they have the same cache domain, and if they were pinned we would fail the eviction.
Quoted 7 lines
Quoted 4 lines
Fixes 625d988acc28 ("drm/i915: Extract reserving space in the GTT to a helper")Signed-off-by Matthew Auld <[email protected]>Cc Chris Wilson <[email protected]>Cc Joonas Lahtinen <[email protected]>Reviewed-by Chris Wilson <[email protected]> /o\-Chris
Good catch, will merge the series once the comments on patch 2 are addressed.
Regards, Joonas
Signature · 7 lines
Joonas Lahtinen Open Source Technology Center Intel Corporation _______________________________________________ Intel-gfx mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Patchwork
CI✓ Fi.CI.BAT: success for series starting with [1/2] drm/i915: use correct node for handling cache domain eviction
In reply to Matthew Auld== Series Details ==
== Series Details ==
Series: series starting with [1/2] drm/i915: use correct node for handling cache domain eviction
URL : https://patchwork.freedesktop.org/series/20646/
State : success
== Summary ==
Series 20646v1 Series without cover letter
https://patchwork.freedesktop.org/api/1.0/series/20646/revisions/1/mbox/
Test gem_exec_flush:
Subgroup basic-batch-kernel-default-uc:
pass -> FAIL (fi-snb-2600) fdo#100007fdo#100007 https://bugs.freedesktop.org/show_bug.cgi?id=100007
fi-bdw-5557u total:278 pass:267 dwarn:0 dfail:0 fail:0 skip:11 fi-bsw-n3050 total:278 pass:239 dwarn:0 dfail:0 fail:0 skip:39 fi-bxt-j4205 total:278 pass:259 dwarn:0 dfail:0 fail:0 skip:19 fi-bxt-t5700 total:278 pass:258 dwarn:0 dfail:0 fail:0 skip:20 fi-byt-j1900 total:278 pass:251 dwarn:0 dfail:0 fail:0 skip:27 fi-byt-n2820 total:278 pass:247 dwarn:0 dfail:0 fail:0 skip:31 fi-hsw-4770 total:278 pass:262 dwarn:0 dfail:0 fail:0 skip:16 fi-hsw-4770r total:278 pass:262 dwarn:0 dfail:0 fail:0 skip:16 fi-ilk-650 total:278 pass:228 dwarn:0 dfail:0 fail:0 skip:50 fi-ivb-3520m total:278 pass:260 dwarn:0 dfail:0 fail:0 skip:18 fi-ivb-3770 total:278 pass:260 dwarn:0 dfail:0 fail:0 skip:18 fi-kbl-7500u total:278 pass:259 dwarn:1 dfail:0 fail:0 skip:18 fi-skl-6260u total:278 pass:268 dwarn:0 dfail:0 fail:0 skip:10 fi-skl-6700hq total:278 pass:261 dwarn:0 dfail:0 fail:0 skip:17 fi-skl-6700k total:278 pass:256 dwarn:4 dfail:0 fail:0 skip:18 fi-skl-6770hq total:278 pass:268 dwarn:0 dfail:0 fail:0 skip:10 fi-snb-2520m total:278 pass:250 dwarn:0 dfail:0 fail:0 skip:28 fi-snb-2600 total:278 pass:248 dwarn:0 dfail:0 fail:1 skip:29
1d7915e7823628a0d548c6ecc5ad5b9a4ae4a6ff drm-tip: 2017y-03m-03d-15h-54m-06s UTC integration manifest dcfd30d drm/i915/selftests: exercise cache domain eviction
60905a6 drm/i915: use correct node for handling cache domain eviction
== Logs ==
For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_4060/
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx