[PATCH 06/13] drm/amdgpu: use the new drm_exec object for CS v2

[Tatsuyuki Ishi]

On 6/20/23 17:12, Christian König wrote:
> Am 20.06.23 um 06:07 schrieb Tatsuyuki Ishi:
>> +Boris and +Matthew in case you want to take over this patch set.
>>
>> Here are some review / testing comments, including those I posted before to ease tracking.
>>
>> On 5/4/23 20:51, Christian König wrote:
>>> Use the new component here as well and remove the old handling.
>>>
>>> v2: drop dupplicate handling
>>>
>>> Signed-off-by: Christian König <christian.koenig at amd.com>
>>> ---
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu.h         |   1 -
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c |  71 ++-----
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.h |   5 +-
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c      | 210 +++++++++-----------
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_cs.h      |   7 +-
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c      |  22 --
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h      |   3 -
>>>   7 files changed, 115 insertions(+), 204 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>> index 02b827785e39..eba3e4f01ea6 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>> @@ -133,6 +141,8 @@ int amdgpu_bo_list_create(struct amdgpu_device *adev, struct drm_file *filp,
>>>         list->first_userptr = first_userptr;
>>>       list->num_entries = num_entries;
>>> +    sort(array, last_entry, sizeof(struct amdgpu_bo_list_entry),
>>> +         amdgpu_bo_list_entry_cmp, NULL);
>>
>> Previously amdgpu_bo_list_get_list sorted all entries, but this one only sorts userptr entries. I think this changes behavior?
> 
> The intention here is to sort all entries except the userptrs. Need to double check.

Sorry, I mistyped. You're right that it sorts all entries except the userptrs. The previous code seems to sort all entries including userptrs.

>>
>>> @@ -928,18 +874,56 @@ static int amdgpu_cs_parser_bos(struct amdgpu_cs_parser *p,
>>>           e->user_invalidated = userpage_invalidated;
>>>       }
>>>   -    r = ttm_eu_reserve_buffers(&p->ticket, &p->validated, true,
>>> -                   &duplicates);
>>> -    if (unlikely(r != 0)) {
>>> -        if (r != -ERESTARTSYS)
>>> -            DRM_ERROR("ttm_eu_reserve_buffers failed.\n");
>>> -        goto out_free_user_pages;
>>> +    drm_exec_while_not_all_locked(&p->exec) {
>>> +        r = amdgpu_vm_lock_pd(&fpriv->vm, &p->exec);
>>> +        drm_exec_continue_on_contention(&p->exec);
>>
>> Duplicate handling is needed for pretty much every call of amdgpu_vm_lock_pd, as bo->tbo.base.resv == vm->root.bo->tbo.base.resv for AMDGPU_GEM_CREATE_VM_ALWAYS_VALID.
> 
> Well no. AMDGPU_GEM_CREATE_VM_ALWAYS_VALID means that BOs should *not* be part of the relocation list. So when those cause an EALREADY here then userspace has a bug.

Sounds fair, lemme check how RADV is handling this again.

Tatsuyuki