[Intel-gfx] [PATCH v4 20/61] drm/i915: Rework clflush to work correctly without obj->mm.lock.

[Thomas Hellström]

On 11/2/20 10:22 AM, Thomas Hellström wrote:
>
> On 11/2/20 9:48 AM, Maarten Lankhorst wrote:
>> Op 30-10-2020 om 16:08 schreef Thomas Hellström:
>>> On 10/16/20 12:44 PM, Maarten Lankhorst wrote:
>>>> Pin in the caller, not in the work itself. This should also
>>>> work better for dma-fence annotations.
>>>>
>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
>>>> ---
>>>>    drivers/gpu/drm/i915/gem/i915_gem_clflush.c | 15 +++++++--------
>>>>    1 file changed, 7 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_clflush.c 
>>>> b/drivers/gpu/drm/i915/gem/i915_gem_clflush.c
>>>> index bc0223716906..daf9284ef1f5 100644
>>>> --- a/drivers/gpu/drm/i915/gem/i915_gem_clflush.c
>>>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_clflush.c
>>>> @@ -27,15 +27,8 @@ static void __do_clflush(struct 
>>>> drm_i915_gem_object *obj)
>>>>    static int clflush_work(struct dma_fence_work *base)
>>>>    {
>>>>        struct clflush *clflush = container_of(base, 
>>>> typeof(*clflush), base);
>>>> -    struct drm_i915_gem_object *obj = clflush->obj;
>>>> -    int err;
>>>>    -    err = i915_gem_object_pin_pages(obj);
>>>> -    if (err)
>>>> -        return err;
>>>> -
>>>> -    __do_clflush(obj);
>>>> -    i915_gem_object_unpin_pages(obj);
>>>> +    __do_clflush(clflush->obj);
>>>>          return 0;
>>>>    }
>>>> @@ -44,6 +37,7 @@ static void clflush_release(struct dma_fence_work 
>>>> *base)
>>>>    {
>>>>        struct clflush *clflush = container_of(base, 
>>>> typeof(*clflush), base);
>>>>    +    i915_gem_object_unpin_pages(clflush->obj);
>>> Hmm, Could we do without pinning here? Pages present are protected 
>>> first by the object lock, then by the fence?
>>>
>>> /Thomas
>>>
>>>
>>>
>> I felt the least chance of regressions was to copy the existing code. :)
>>
>> At least until we have completed obj->mm.lock removal, we shouldn't 
>> try to be smart with these kind of things.
>>
>> Likely, we've already pinned the pages, so it's a simple ref increase.
>>
>> Later on, we can try to be smart, but whenever I tried to change 
>> behavior so far, I was forced to hunt down bugs.
>
> OK, sounds good.
>
Reviewed-by: Thomas Hellström <thomas.hellstrom at linux.intel.com>


> /Thomas
>
>
>
>>
>> ~Maarten
>>