[PATCH 2/2] drm/amdgpu: Auto-validate DMABuf imports in compute VMs

[Felix Kuehling]

On 2023-12-04 03:40, Christian König wrote:
>>   @@ -416,6 +423,28 @@ int amdgpu_vm_validate_pt_bos(struct 
>> amdgpu_device *adev, struct amdgpu_vm *vm,
>>           }
>>           spin_lock(&vm->status_lock);
>>       }
>> +    while (ticket && !list_empty(&vm->evicted_user)) {
>> +        bo_base = list_first_entry(&vm->evicted_user,
>> +                       struct amdgpu_vm_bo_base,
>> +                       vm_status);
>> +        spin_unlock(&vm->status_lock);
>> +
>> +        bo = bo_base->bo;
>> +        resv = bo->tbo.base.resv;
>> +
>> +        if (dma_resv_locking_ctx(resv) != ticket) {
>> +            pr_warn_ratelimited("Evicted user BO is not reserved in 
>> pid %d\n",
>> +                        vm->task_info.pid);
>> +            return -EINVAL;
>> +        }
>> +
>> +        r = validate(param, bo);
>> +        if (r)
>> +            return r;
>> +
>> +        amdgpu_vm_bo_invalidated(bo_base);
>> +        spin_lock(&vm->status_lock);
>> +    }
>
> I see two main differences to the evicted state here:
>
> 1. We now handle BOs which don't use the shared reservation object and 
> needs to be locked manually with the ticket before calling this function.
>
> 2. The BOs are then moved into a different state after validating them.
>
> That could be handled with just an additional if if I'm not completely 
> mistaken which would make the extra state superfluous. 

I tried this, and I think it's working. But if I don't have a ticket for 
validating user BOs, I have to move the BO back to the vm->invalidated 
list anyway, to avoid an infinite while(!list_empty()) loop. So the BO 
may ping-pong between invalidated and evicted, and lead to some 
redundant page table clearing in amdgpu_vm_handle_moved in cases where 
amdgpu_vm_validate was called without a ticket and there happen to be 
invalidated DMABuf imports.

I'm not sure if this will be a problem in practice. If it is, I think 
keeping a separate state for evicted user BOs may still make sense.

Regards,
   Felix