[Intel-gfx] [PATCH] drm/i915/eb: Fix pagefault disabling in the first slowpath

[Maarten Lankhorst]

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 at 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 at 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 at 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 at 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 at 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 at intel.com>
>
>>
>> Signed-off-by: Daniel Vetter <daniel.vetter at intel.com>
>> Cc: Jon Bloomfield <jon.bloomfield at intel.com>
>> Cc: Chris Wilson <chris at chris-wilson.co.uk>
>> Cc: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
>> Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
>> Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
>> Cc: "Thomas Hellström" <thomas.hellstrom at linux.intel.com>
>> Cc: Matthew Auld <matthew.auld at intel.com>
>> Cc: Lionel Landwerlin <lionel.g.landwerlin at intel.com>
>> Cc: Dave Airlie <airlied at redhat.com>
>> Cc: Jason Ekstrand <jason at 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 at linux.intel.com>