| Message ID | 20210618214503.1773805-1-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/eb: Fix pagefault disabling in the first slowpath | expand |
On 18/06/2021 22:45, Daniel Vetter wrote: > In > > commit ebc0808fa2da0548a78e715858024cb81cd732bc > Author: Chris Wilson <chris@chris-wilson.co.uk> > 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@chris-wilson.co.uk> > 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@chris-wilson.co.uk> > 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@chris-wilson.co.uk> > 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@linux.intel.com> > 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@intel.com> > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > Cc: Jon Bloomfield <jon.bloomfield@intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > Cc: "Thomas Hellström" <thomas.hellstrom@linux.intel.com> > Cc: Matthew Auld <matthew.auld@intel.com> > Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com> > Cc: Dave Airlie <airlied@redhat.com> > Cc: Jason Ekstrand <jason@jlekstrand.net> > --- > 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 { >
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@chris-wilson.co.uk> >> 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@chris-wilson.co.uk> >> 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@chris-wilson.co.uk> >> 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@chris-wilson.co.uk> >> 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@linux.intel.com> >> 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@intel.com> > >> >> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> >> Cc: Jon Bloomfield <jon.bloomfield@intel.com> >> Cc: Chris Wilson <chris@chris-wilson.co.uk> >> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> >> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> >> Cc: "Thomas Hellström" <thomas.hellstrom@linux.intel.com> >> Cc: Matthew Auld <matthew.auld@intel.com> >> Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com> >> Cc: Dave Airlie <airlied@redhat.com> >> Cc: Jason Ekstrand <jason@jlekstrand.net> >> --- >> 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@linux.intel.com>
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@chris-wilson.co.uk> > >> 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@chris-wilson.co.uk> > >> 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@chris-wilson.co.uk> > >> 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@chris-wilson.co.uk> > >> 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@linux.intel.com> > >> 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@intel.com> > > > >> > >> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > >> Cc: Jon Bloomfield <jon.bloomfield@intel.com> > >> Cc: Chris Wilson <chris@chris-wilson.co.uk> > >> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > >> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > >> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > >> Cc: "Thomas Hellström" <thomas.hellstrom@linux.intel.com> > >> Cc: Matthew Auld <matthew.auld@intel.com> > >> Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com> > >> Cc: Dave Airlie <airlied@redhat.com> > >> Cc: Jason Ekstrand <jason@jlekstrand.net> > >> --- > >> 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@linux.intel.com> Pushed to drm-intel-gt-next, thanks to both of you for taking a look. -Daniel > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
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 {
In commit ebc0808fa2da0548a78e715858024cb81cd732bc Author: Chris Wilson <chris@chris-wilson.co.uk> 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@chris-wilson.co.uk> 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@chris-wilson.co.uk> 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@chris-wilson.co.uk> 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@linux.intel.com> 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. Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> Cc: Jon Bloomfield <jon.bloomfield@intel.com> Cc: Chris Wilson <chris@chris-wilson.co.uk> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> Cc: "Thomas Hellström" <thomas.hellstrom@linux.intel.com> Cc: Matthew Auld <matthew.auld@intel.com> Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com> Cc: Dave Airlie <airlied@redhat.com> Cc: Jason Ekstrand <jason@jlekstrand.net> --- drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c | 2 -- 1 file changed, 2 deletions(-)