On 21-10-2021 19:39, Matthew Auld wrote:
> On Thu, 21 Oct 2021 at 11:37, Maarten Lankhorst
> <maarten.lankhorst@xxxxxxxxxxxxxxx> wrote:
>> i915_vma_wait_for_bind needs the vma lock held, fix the caller.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
>> ---
>> 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
