Re: [Intel-gfx] [PATCH 1/2] drm/i915: Fix assert in i915_ggtt_pin

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 25/02/2022 17:58, Ville Syrjälä wrote:
> On Fri, Feb 25, 2022 at 05:41:17PM +0000, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> Use lockdep_assert_not_held to simplify and correct the code. Otherwise
>> false positive are hit if lock state is uknown like after a previous
>> taint.
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> Reported-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_vma.c | 4 +---
>>   1 file changed, 1 insertion(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c
>> index 3558b16a929c..4469b7f52853 100644
>> --- a/drivers/gpu/drm/i915/i915_vma.c
>> +++ b/drivers/gpu/drm/i915/i915_vma.c
>> @@ -1552,9 +1552,7 @@ int i915_ggtt_pin(struct i915_vma *vma, struct i915_gem_ww_ctx *ww,
>>   	if (ww)
>>   		return __i915_ggtt_pin(vma, ww, align, flags);
>>   
>> -#ifdef CONFIG_LOCKDEP
>> -	WARN_ON(dma_resv_held(vma->obj->base.resv));
>> -#endif
>> +	lockdep_assert_not_held(&vma->obj->base.resv->lock.base);
> 
> Should there be a dma_resv wrapper for that? Shrug.

Probably.. it is really ugly being this verbose.

> Makes sense to me:
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> I'll leave the other one to someone how knows how it's actually used.

Yeah I think that one is likely wrong. If lockdep is compiled in but not 
enabled it would make some asserts fire since state will be 'unknown' 
then and so it would always say "not locked" to the callers.

Regards,

Tvrtko

> 
>>   
>>   	for_i915_gem_ww(&_ww, err, true) {
>>   		err = i915_gem_object_lock(vma->obj, &_ww);
>> -- 
>> 2.32.0
>