[PATCH 08/12] drm/amdgpu: rework synchronization of VM updates

[Christian König]

Am 06.01.20 um 22:38 schrieb Felix Kuehling:
> On 2020-01-06 10:03 a.m., Christian König wrote:
>> [SNIP]
>> @@ -1557,7 +1557,9 @@ static int amdgpu_vm_bo_update_mapping(struct 
>> amdgpu_device *adev,
>>         /* sync to everything except eviction fences on unmapping */
>
> I think this comment needs to be updated. Something like "Implicitly 
> sync to command submissions in the same VM before unmapping. Sync to 
> moving fences before mapping."

Good point, just updated this.

>
>
>> [SNIP]
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_cpu.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_cpu.c
>> index 68b013be3837..0be72660db4c 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_cpu.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_cpu.c
>> @@ -44,26 +44,21 @@ static int amdgpu_vm_cpu_map_table(struct 
>> amdgpu_bo *table)
>>    * Returns:
>>    * Negativ errno, 0 for success.
>>    */
>> -static int amdgpu_vm_cpu_prepare(struct amdgpu_vm_update_params *p, 
>> void *owner,
>> -                 struct dma_fence *exclusive)
>> +static int amdgpu_vm_cpu_prepare(struct amdgpu_vm_update_params *p,
>> +                 struct dma_resv *resv,
>> +                 enum amdgpu_sync_mode sync_mode)
>>   {
>> +    struct amdgpu_sync sync;
>>       int r;
>>   -    /* Wait for any BO move to be completed */
>> -    if (exclusive) {
>> -        r = dma_fence_wait(exclusive, true);
>> -        if (unlikely(r))
>> -            return r;
>> -    }
>> -
>> -    /* Don't wait for submissions during page fault */
>> -    if (p->direct)
>> +    if (!resv)
>>           return 0;
>>   -    /* Wait for PT BOs to be idle. PTs share the same resv. object
>> -     * as the root PD BO
>> -     */
>> -    return amdgpu_bo_sync_wait(p->vm->root.base.bo, owner, true);
>> +    amdgpu_sync_create(&sync);
>> +    amdgpu_sync_resv(p->adev, &sync, resv, sync_mode, p->vm);
>> +    r = amdgpu_sync_wait(&sync, true);
>> +    amdgpu_sync_free(&sync);
>
> This looks like it's pretty much duplicating amdgpu_bo_sync_wait. In 
> fact, when I first wrote that function it was meant as 
> amdgpu_sync_wait_resv (and the comment above that function still 
> incorrectly has that name).

Question is where is the best place to put this? It doesn't really fit 
into amdgpu_sync.c because it uses the syncobject and doesn't implements it.

But if we give the reservation object as parameter amdgpu_object.c 
doesn't fits either as well.

Regards,
Christian.

>
> Regards,
>   Felix
>
>
>> +    return r;
>>   }
>>     /**
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c
>> index 3b61317c0f08..e7a383134521 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c
>> @@ -58,9 +58,9 @@ static int amdgpu_vm_sdma_map_table(struct 
>> amdgpu_bo *table)
>>    * Negativ errno, 0 for success.
>>    */
>>   static int amdgpu_vm_sdma_prepare(struct amdgpu_vm_update_params *p,
>> -                  void *owner, struct dma_fence *exclusive)
>> +                  struct dma_resv *resv,
>> +                  enum amdgpu_sync_mode sync_mode)
>>   {
>> -    struct amdgpu_bo *root = p->vm->root.base.bo;
>>       unsigned int ndw = AMDGPU_VM_SDMA_MIN_NUM_DW;
>>       int r;
>>   @@ -70,17 +70,10 @@ static int amdgpu_vm_sdma_prepare(struct 
>> amdgpu_vm_update_params *p,
>>         p->num_dw_left = ndw;
>>   -    /* Wait for moves to be completed */
>> -    r = amdgpu_sync_fence(&p->job->sync, exclusive, false);
>> -    if (r)
>> -        return r;
>> -
>> -    /* Don't wait for any submissions during page fault handling */
>> -    if (p->direct)
>> +    if (!resv)
>>           return 0;
>>   -    return amdgpu_sync_resv(p->adev, &p->job->sync, 
>> root->tbo.base.resv,
>> -                AMDGPU_SYNC_NE_OWNER, owner);
>> +    return amdgpu_sync_resv(p->adev, &p->job->sync, resv, sync_mode, 
>> p->vm);
>>   }
>>     /**