[PATCH 3/6] drm/amdgpu: reserve/unreserve vmid by vm ioctl v3

[zhoucm1]

On 2017年04月26日 20:26, Christian König wrote:
> Am 26.04.2017 um 13:10 schrieb Chunming Zhou:
>> add reserve/unreserve vmid funtions.
>> v3:
>> only reserve vmid from gfxhub
>>
>> Change-Id: I5f80dc39dc9d44660a96a2b710b0dbb4d3b9039d
>> Signed-off-by: Chunming Zhou <David1.Zhou at amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 70 
>> +++++++++++++++++++++++++++-------
>>   1 file changed, 57 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> index ec87d64..a783e8e 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> @@ -397,6 +397,11 @@ static bool amdgpu_vm_had_gpu_reset(struct 
>> amdgpu_device *adev,
>>           atomic_read(&adev->gpu_reset_counter);
>>   }
>>   +static bool amdgpu_vm_dedicated_vmid_ready(struct amdgpu_vm *vm, 
>> unsigned vmhub)
>> +{
>> +    return !!vm->dedicated_vmid[vmhub];
>> +}
>> +
>>   /**
>>    * amdgpu_vm_grab_id - allocate the next free VMID
>>    *
>> @@ -546,6 +551,47 @@ int amdgpu_vm_grab_id(struct amdgpu_vm *vm, 
>> struct amdgpu_ring *ring,
>>       return r;
>>   }
>>   +static void amdgpu_vm_free_dedicated_vmid(struct amdgpu_device *adev,
>> +                      struct amdgpu_vm *vm,
>> +                      unsigned vmhub)
>> +{
>> +    struct amdgpu_vm_id_manager *id_mgr = 
>> &adev->vm_manager.id_mgr[vmhub];
>> +
>> +    mutex_lock(&id_mgr->lock);
>> +    if (vm->dedicated_vmid[vmhub]) {
>> +        list_add(&vm->dedicated_vmid[vmhub]->list,
>> +            &id_mgr->ids_lru);
>> +        vm->dedicated_vmid[vmhub] = NULL;
>> +    }
>> +    mutex_unlock(&id_mgr->lock);
>> +}
>> +
>> +static int amdgpu_vm_alloc_dedicated_vmid(struct amdgpu_device *adev,
>> +                      struct amdgpu_vm *vm,
>> +                      unsigned vmhub)
>> +{
>> +    struct amdgpu_vm_id_manager *id_mgr;
>> +    struct amdgpu_vm_id *idle;
>> +    int r;
>> +
>> +    id_mgr = &adev->vm_manager.id_mgr[vmhub];
>> +    mutex_lock(&id_mgr->lock);
>> +    /* Select the first entry VMID */
>> +    idle = list_first_entry(&id_mgr->ids_lru, struct amdgpu_vm_id, 
>> list);
>> +    list_del_init(&idle->list);
>> +    vm->dedicated_vmid[vmhub] = idle;
>
> We need a check here if there isn't an ID already reserved for the VM.
>
> Additional to that I would still rename the field to reserved_vmid, 
> that describes better what it is actually doing.
the vmid is global, reserve it from global lru, so it makes sense to 
name 'reserved_vmid' like in patch#4.
the reserved vmid is dedicated to one vm, so the field in vm is 
'dedicated_vmid' like here, which is my reason to name it.

Anyway, the alternative is ok to me although I prefer mine, I hope we 
can deliver this solution by today, many RGP issues depend on it.

Thanks,
David Zhou
>
>> +    mutex_unlock(&id_mgr->lock);
>> +
>> +    r = amdgpu_sync_wait(&idle->active);
>> +    if (r)
>> +        goto err;
>
> This is racy. A command submission could happen while we wait for the 
> id to become idle.
>
> The easiest way to solve this is to hold the lock while waiting for 
> the ID to become available.
>
>> +
>> +    return 0;
>> +err:
>> +    amdgpu_vm_free_dedicated_vmid(adev, vm, vmhub);
>> +    return r;
>> +}
>> +
>>   static bool amdgpu_vm_ring_has_compute_vm_bug(struct amdgpu_ring 
>> *ring)
>>   {
>>       struct amdgpu_device *adev = ring->adev;
>> @@ -2316,18 +2362,8 @@ void amdgpu_vm_fini(struct amdgpu_device 
>> *adev, struct amdgpu_vm *vm)
>>         amdgpu_vm_free_levels(&vm->root);
>>       fence_put(vm->last_dir_update);
>> -    for (i = 0; i < AMDGPU_MAX_VMHUBS; i++) {
>> -        struct amdgpu_vm_id_manager *id_mgr =
>> -            &adev->vm_manager.id_mgr[i];
>> -
>> -        mutex_lock(&id_mgr->lock);
>> -        if (vm->dedicated_vmid[i]) {
>> -            list_add(&vm->dedicated_vmid[i]->list,
>> -                 &id_mgr->ids_lru);
>> -            vm->dedicated_vmid[i] = NULL;
>> -        }
>> -        mutex_unlock(&id_mgr->lock);
>> -    }
>> +    for (i = 0; i < AMDGPU_MAX_VMHUBS; i++)
>> +        amdgpu_vm_free_dedicated_vmid(adev, vm, i);
>>   }
>>     /**
>> @@ -2400,11 +2436,19 @@ int amdgpu_vm_ioctl(struct drm_device *dev, 
>> void *data, struct drm_file *filp)
>>       union drm_amdgpu_vm *args = data;
>>       struct amdgpu_device *adev = dev->dev_private;
>>       struct amdgpu_fpriv *fpriv = filp->driver_priv;
>> +    int r;
>>         switch (args->in.op) {
>>       case AMDGPU_VM_OP_RESERVE_VMID:
>> +        /* current, we only have requirement to reserve vmid from 
>> gfxhub */
>> +        if (!amdgpu_vm_dedicated_vmid_ready(&fpriv->vm, 0)) {
>> +            r = amdgpu_vm_alloc_dedicated_vmid(adev, &fpriv->vm, 0);
>
> This is racy as well.
>
> Two threads could try to reserve a VMID at the same time. You need to 
> integrate the check if it's already allocated into 
> amdgpu_vm_alloc_dedicated_vmid().
>
> Regards,
> Christian.
>
>> +            if (r)
>> +                return r;
>> +        }
>> +        break;
>>       case AMDGPU_VM_OP_UNRESERVE_VMID:
>> -        return -EINVAL;
>> +        amdgpu_vm_free_dedicated_vmid(adev, &fpriv->vm, 0);
>>           break;
>>       default:
>>           return -EINVAL;
>
>