On Mon, Jun 21, 2021 at 04:30:50PM +0200, Maarten Lankhorst wrote:
> Op 21-06-2021 om 11:33 schreef Matthew Auld:
> > 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 {
> >>
> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
Pushed to drm-intel-gt-next, thanks to both of you for taking a look.
-Daniel
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
