[Intel-xe] [PATCH] drm/xe/vm: fix double list add

[Matthew Auld]

Hi,

On 01/06/2023 14:19, Maarten Lankhorst wrote:
> Hey,
> 
> I guess that should make the ignore argument pointless?

IIUC we have something like:

vma = create_vma(vm, bo, ...) -> list_add(..., bo->vmas)
....
vm_insert_extobj(vm, vma)

Here we need the "ignore", otherwise bo_has_vm_references() in 
vm_insert_extobj() might incorrectly think the vma we just added to 
bo->vmas counts as a reference, and then the vma never becomes tracked 
as external. Or am I missing something here?

> 
> Cheers,
> 
> Maarten
> 
> On 2023-06-01 14:35, Matthew Auld wrote:
>> It looks like the driver only wants to track one vma for each external
>> object per vm. However it looks like bo_has_vm_references_locked() will
>> ignore any vma that is marked as vma->destroyed (not actually destroyed
>> yet). If we then mark our externally tracked vma as destroyed and then
>> create a new vma for the same object and vm, we can have two externally
>> tracked vma for the same object and vm. When the destroy actually
>> happens it tries to move the external tracking to a different vma, but
>> in this case it is already being tracked, leading to double list add
>> errors. It should be safe to simply drop the destroyed check in
>> bo_has_vm_references(), since the actual destroy will switch the
>> external tracking to the next available vma.
>>
>> Closes: https://gitlab.freedesktop.org/drm/xe/kernel/-/issues/290
>> Cc: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
>> Cc: Thomas Hellström <thomas.hellstrom at linux.intel.com>
>> Signed-off-by: Matthew Auld <matthew.auld at intel.com>
>> ---
>>   drivers/gpu/drm/xe/xe_vm.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
>> index dd1335e12d4c..5aa2aeb14beb 100644
>> --- a/drivers/gpu/drm/xe/xe_vm.c
>> +++ b/drivers/gpu/drm/xe/xe_vm.c
>> @@ -964,7 +964,7 @@ bo_has_vm_references_locked(struct xe_bo *bo, struct xe_vm *vm,
>>   	struct xe_vma *vma;
>>   
>>   	list_for_each_entry(vma, &bo->vmas, bo_link) {
>> -		if (vma != ignore && vma->vm == vm && !vma->destroyed)
>> +		if (vma != ignore && vma->vm == vm)
>>   			return vma;
>>   	}
>>