[PATCH 3/6] drm/amdgpu: reserve/unreserve vmid by vm ioctl v3
Christian König
deathsimple at vodafone.de
Thu Apr 27 09:13:11 UTC 2017
Am 27.04.2017 um 04:24 schrieb Zhang, Jerry (Junwei):
> On 04/27/2017 10:13 AM, zhoucm1 wrote:
>>
>>
>> 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.
>
> Just the same thing looking at different views.
>
> IMHO, it's reserved vmid from global group, naming "reserved_vmid".
> And in Patch#4, it constrains the number, possibly naming
> "reserved_vmid_seq" or "reserved_vmid_ref".
>
> Any of them looks reasonable, select the one you like. :)
Yeah, agree. More like a gut feeling that reserved is more common
wording, but I don't have a strong opinion on that either.
Christian.
>
> Jerry
>
>>
>> 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;
>>>
>>>
>>
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
More information about the amd-gfx
mailing list