Re: [Intel-gfx] [PATCH 14/28] drm/i915: Take object lock in i915_ggtt_pin if ww is not set

[Maarten Lankhorst <maarten.lankhorst@linux.intel.com>]

On 21-10-2021 19:39, Matthew Auld wrote:
> On Thu, 21 Oct 2021 at 11:37, Maarten Lankhorst
> <maarten.lankhorst@linux.intel.com> wrote:
>> i915_vma_wait_for_bind needs the vma lock held, fix the caller.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_vma.c | 40 +++++++++++++++++++++++----------
>>  1 file changed, 28 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c
>> index bacc8d68e495..2877dcd62acb 100644
>> --- a/drivers/gpu/drm/i915/i915_vma.c
>> +++ b/drivers/gpu/drm/i915/i915_vma.c
>> @@ -1348,23 +1348,15 @@ static void flush_idle_contexts(struct intel_gt *gt)
>>         intel_gt_wait_for_idle(gt, MAX_SCHEDULE_TIMEOUT);
>>  }
>>
>> -int i915_ggtt_pin(struct i915_vma *vma, struct i915_gem_ww_ctx *ww,
>> -                 u32 align, unsigned int flags)
>> +static int __i915_ggtt_pin(struct i915_vma *vma, struct i915_gem_ww_ctx *ww,
>> +                          u32 align, unsigned int flags)
>>  {
>>         struct i915_address_space *vm = vma->vm;
>>         int err;
>>
>> -       GEM_BUG_ON(!i915_vma_is_ggtt(vma));
>> -
>> -#ifdef CONFIG_LOCKDEP
>> -       WARN_ON(!ww && dma_resv_held(vma->obj->base.resv));
>> -#endif
>> -
>>         do {
>> -               if (ww)
>> -                       err = i915_vma_pin_ww(vma, ww, 0, align, flags | PIN_GLOBAL);
>> -               else
>> -                       err = i915_vma_pin(vma, 0, align, flags | PIN_GLOBAL);
>> +               err = i915_vma_pin_ww(vma, ww, 0, align, flags | PIN_GLOBAL);
>> +
>>                 if (err != -ENOSPC) {
>>                         if (!err) {
>>                                 err = i915_vma_wait_for_bind(vma);
>> @@ -1383,6 +1375,30 @@ int i915_ggtt_pin(struct i915_vma *vma, struct i915_gem_ww_ctx *ww,
>>         } while (1);
>>  }
>>
>> +int i915_ggtt_pin(struct i915_vma *vma, struct i915_gem_ww_ctx *ww,
>> +                 u32 align, unsigned int flags)
>> +{
>> +       struct i915_gem_ww_ctx _ww;
>> +       int err;
>> +
>> +       GEM_BUG_ON(!i915_vma_is_ggtt(vma));
>> +
>> +       if (ww)
>> +               return __i915_ggtt_pin(vma, ww, align, flags);
>> +
>> +#ifdef CONFIG_LOCKDEP
>> +       WARN_ON(dma_resv_held(vma->obj->base.resv));
> Hmm, so this can't trigger, say if shrinker grabs the lock, or some
> other concurrent user?

No, it checks internally in lockdep that the current task doesn't hold the lock. Others can lock just fine.

dma_resv_is_locked() would check if anyone locked it. :)

~Maarten