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>
