[Intel-gfx] [PATCH 18/23] drm/i915: Dirty hack to fix selftests locking inversion

[Tvrtko Ursulin]

On 07/07/2020 11:19, Maarten Lankhorst wrote:
> Op 03-07-2020 om 15:48 schreef Tvrtko Ursulin:
>>
>> On 03/07/2020 13:22, Maarten Lankhorst wrote:
>>> Some i915 selftests still use i915_vma_lock() as inner lock, and
>>> intel_context_create_request() intel_timeline->mutex as outer lock.
>>> Fortunately for selftests this is not an issue, they should be fixed
>>> but we can move ahead and cleanify lockdep now.
>>
>> Mentions and existence of "dirty hacks" will hopefully be removed from the series before it can be considered merge ready?
>>
>> Regards,
>>
>> Tvrtko
>>
>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
>>> ---
>>>    drivers/gpu/drm/i915/gt/intel_context.c | 12 ++++++++++++
>>>    1 file changed, 12 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/i915/gt/intel_context.c b/drivers/gpu/drm/i915/gt/intel_context.c
>>> index 64948386630f..fe9fff5a63b1 100644
>>> --- a/drivers/gpu/drm/i915/gt/intel_context.c
>>> +++ b/drivers/gpu/drm/i915/gt/intel_context.c
>>> @@ -459,6 +459,18 @@ struct i915_request *intel_context_create_request(struct intel_context *ce)
>>>        rq = i915_request_create(ce);
>>>        intel_context_unpin(ce);
>>>    +    if (IS_ERR(rq))
>>> +        return rq;
>>> +
>>> +    /*
>>> +     * timeline->mutex should be the inner lock, but is used as outer lock.
>>> +     * Hack around this to shut up lockdep in selftests..
>>> +     */
>>> +    lockdep_unpin_lock(&ce->timeline->mutex, rq->cookie);
>>> +    mutex_release(&ce->timeline->mutex.dep_map, _RET_IP_);
>>> +    mutex_acquire(&ce->timeline->mutex.dep_map, SINGLE_DEPTH_NESTING, 0, _RET_IP_);
>>> +    rq->cookie = lockdep_pin_lock(&ce->timeline->mutex);
>>> +
>>>        return rq;
>>>    }
>>>   
> 
> Hey,
> 
> We're trying to invert the locking order with vma lock vs request lock, while this is a hack,
> it will not affect normal running code, it's only meant to shut up lockdep in the selftests.
> 
> This is mainly so we can fix the selftests one by one, without breaking the world. Ideally
> when mm.obj lands, we already corrected the lock ordering. We may keep this macro for selftests,
> but until lock reordering in selftests is complete we will need this temporarily.

Is there a relationship between obj->mm.lock and this particular lock 
inversion? I don't see it. It will become critical to have selftests 
adjusted to proper locking order for every which will/can trigger 
eviction. But okay,if you want to stage the pieces perhaps it is acceptable.

As previous patch in the series removes intel_context_create_request 
usages outside selftests I suggest you mention this in this commit 
message, as part of justification why it is safe.

Also it would be good to wrap intel_context_create_request in a selftest 
#ifdef so that accidental usage is prevented in the inter-rim stages of 
refactoring.

Regards,

Tvrtko