On 18/06/2021 22:45, Daniel Vetter wrote:
In
commit ebc0808fa2da0548a78e715858024cb81cd732bc
Author: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
Date: Tue Oct 18 13:02:51 2016 +0100
drm/i915: Restrict pagefault disabling to just around copy_from_user()
we entirely missed that there's a slow path call to eb_relocate_entry
(or i915_gem_execbuffer_relocate_entry as it was called back then)
which was left fully wrapped by pagefault_disable/enable() calls.
Previously any issues with blocking calls where handled by the
following code:
/* we can't wait for rendering with pagefaults disabled */
if (pagefault_disabled() && !object_is_idle(obj))
return -EFAULT;
Now at this point the prefaulting was still around, which means in
normal applications it was very hard to hit this bug. No idea why the
regressions in igts weren't caught.
Now this all changed big time with 2 patches merged closely together.
First
commit 2889caa9232109afc8881f29a2205abeb5709d0c
Author: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
Date: Fri Jun 16 15:05:19 2017 +0100
drm/i915: Eliminate lots of iterations over the execobjects array
removes the prefaulting from the first relocation path, pushing it into
the first slowpath (of which this patch added a total of 3 escalation
levels). This would have really quickly uncovered the above bug, were
it not for immediate adding a duct-tape on top with
commit 7dd4f6729f9243bd7046c6f04c107a456bda38eb
Author: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
Date: Fri Jun 16 15:05:24 2017 +0100
drm/i915: Async GPU relocation processing
by pushing all all the relocation patching to the gpu if the buffer
was busy, which avoided all the possible blocking calls.
The entire slowpath was then furthermore ditched in
commit 7dc8f1143778a35b190f9413f228b3cf28f67f8d
Author: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
Date: Wed Mar 11 16:03:10 2020 +0000
drm/i915/gem: Drop relocation slowpath
and resurrected in
commit fd1500fcd4420eee06e2c7f3aa6067b78ac05871
Author: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
Date: Wed Aug 19 16:08:43 2020 +0200
Revert "drm/i915/gem: Drop relocation slowpath".
but this did not further impact what's going on.
Since pagefault_disable/enable is an atomic section, any sleeping in
there is prohibited, and we definitely do that without gpu relocations
since we have to wait for the gpu usage to finish before we can patch
up the relocations.
Why do we also need the __copy_from_user_inatomic in eb_relocate_vma()?
Reviewed-by: Matthew Auld <matthew.auld@xxxxxxxxx>
Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxxx>
Cc: Jon Bloomfield <jon.bloomfield@xxxxxxxxx>
Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
Cc: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
Cc: Joonas Lahtinen <joonas.lahtinen@xxxxxxxxxxxxxxx>
Cc: Daniel Vetter <daniel.vetter@xxxxxxxx>
Cc: "Thomas Hellström" <thomas.hellstrom@xxxxxxxxxxxxxxx>
Cc: Matthew Auld <matthew.auld@xxxxxxxxx>
Cc: Lionel Landwerlin <lionel.g.landwerlin@xxxxxxxxx>
Cc: Dave Airlie <airlied@xxxxxxxxxx>
Cc: Jason Ekstrand <jason@xxxxxxxxxxxxxx>
---
drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
index 6539b82dda54..7ff2fc3c0b2c 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
@@ -2082,9 +2082,7 @@ static noinline int eb_relocate_parse_slow(struct i915_execbuffer *eb,
list_for_each_entry(ev, &eb->relocs, reloc_link) {
if (!have_copy) {
- pagefault_disable();
err = eb_relocate_vma(eb, ev);
- pagefault_enable();
if (err)
break;
} else {
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
