Re: [PATCH] drm/xe: Check empty pinned BO list with lock held.

[Nirmoy Das <nirmoy.das@linux.intel.com>]

Hi Matt,

On 5/28/2024 10:56 PM, Matthew Brost wrote:
> On Tue, May 28, 2024 at 01:54:08PM +0200, Nirmoy Das wrote:
>> Use lock that is meant to use for accessing the BO pin list.
>>
>> Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>
> Agree with this patch removing removing micro optimization of bypassing
> &xe->pinned.lock, we should avoid thing like this.
>
> Curious if this was an actual problem (i.e. was their a bug report which
> lead to this change)?


There was a warning from static analyzer tool so no real bug report yet.

>
> Anyways the patch LGTM. With that:
> Reviewed-by: Matthew Brost <matthew.brost@intel.com>


Thanks,

Nirmoy

>
>> ---
>>   drivers/gpu/drm/xe/xe_bo.c | 10 ++++------
>>   1 file changed, 4 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_bo.c b/drivers/gpu/drm/xe/xe_bo.c
>> index 03f7fe7acf8c..2bae01ce4e5b 100644
>> --- a/drivers/gpu/drm/xe/xe_bo.c
>> +++ b/drivers/gpu/drm/xe/xe_bo.c
>> @@ -1758,11 +1758,10 @@ void xe_bo_unpin_external(struct xe_bo *bo)
>>   	xe_assert(xe, xe_bo_is_pinned(bo));
>>   	xe_assert(xe, xe_bo_is_user(bo));
>>   
>> -	if (bo->ttm.pin_count == 1 && !list_empty(&bo->pinned_link)) {
>> -		spin_lock(&xe->pinned.lock);
>> +	spin_lock(&xe->pinned.lock);
>> +	if (bo->ttm.pin_count == 1 && !list_empty(&bo->pinned_link))
>>   		list_del_init(&bo->pinned_link);
>> -		spin_unlock(&xe->pinned.lock);
>> -	}
>> +	spin_unlock(&xe->pinned.lock);
>>   
>>   	ttm_bo_unpin(&bo->ttm);
>>   
>> @@ -1785,9 +1784,8 @@ void xe_bo_unpin(struct xe_bo *bo)
>>   		struct ttm_place *place = &(bo->placements[0]);
>>   
>>   		if (mem_type_is_vram(place->mem_type)) {
>> -			xe_assert(xe, !list_empty(&bo->pinned_link));
>> -
>>   			spin_lock(&xe->pinned.lock);
>> +			xe_assert(xe, !list_empty(&bo->pinned_link));
>>   			list_del_init(&bo->pinned_link);
>>   			spin_unlock(&xe->pinned.lock);
>>   		}
>> -- 
>> 2.42.0
>>