[PATCH 10/12] drm/amdgpu: immedially invalidate PTEs

[Christian König]

Am 06.01.20 um 22:04 schrieb Felix Kuehling:
> On 2020-01-06 10:03 a.m., Christian König wrote:
>> When a BO is evicted immedially invalidate the mapped PTEs.
>>
>> Signed-off-by: Christian König <christian.koenig at amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 17 ++++++++++++++++-
>>   1 file changed, 16 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> index a03cfbe670c4..6844ba7467a6 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> @@ -2569,6 +2569,7 @@ void amdgpu_vm_bo_invalidate(struct 
>> amdgpu_device *adev,
>>                    struct amdgpu_bo *bo, bool evicted)
>>   {
>>       struct amdgpu_vm_bo_base *bo_base;
>> +    int r;
>>         /* shadow bo doesn't have bo base, its validation needs its 
>> parent */
>>       if (bo->parent && bo->parent->shadow == bo)
>> @@ -2576,8 +2577,22 @@ void amdgpu_vm_bo_invalidate(struct 
>> amdgpu_device *adev,
>>         for (bo_base = bo->vm_bo; bo_base; bo_base = bo_base->next) {
>>           struct amdgpu_vm *vm = bo_base->vm;
>> +        struct dma_resv *resv = vm->root.base.bo->tbo.base.resv;
>> +
>> +        if (bo->tbo.type != ttm_bo_type_kernel) {
>> +            struct amdgpu_bo_va *bo_va;
>> +
>> +            bo_va = container_of(bo_base, struct amdgpu_bo_va,
>> +                         base);
>> +            r = amdgpu_vm_bo_update(adev, bo_va,
>> +                        bo->tbo.base.resv != resv);
>
> Will this update PTEs for per-VM BOs without validating them first?

Yes and that's intentional. Essentially this should allows moving per-VM 
BOs around when they aren't fenced.

>> +            if (!r) {
>> +                amdgpu_vm_bo_idle(bo_base);
>
> Is this the right state? The description of amdgpu_vm_bo_idle says 
> that this is for "PDs/PTs and per VM BOs". For regular BOs, I think 
> this should call amdgpu_vm_bo_done.

Good point, that's indeed not correct for per-VM BOs. Looks like I need 
to rework the whole state logic.

Thanks,
Christian.

>
>
>> +                continue;
>
> This skips a bunch of state machine logic below. Maybe some of that 
> could be cleaned up.
>
>
>> +            }
>> +        }
>>   -        if (evicted && bo->tbo.base.resv == 
>> vm->root.base.bo->tbo.base.resv) {
>> +        if (evicted && bo->tbo.base.resv == resv) {
>>               amdgpu_vm_bo_evicted(bo_base);
>
> It will never get here for per-VM BOs now (except if bo_update 
> failed). Not sure if that's a problem.
>
>
>>               continue;
>>           }
>>
>>                  if (bo_base->moved)
>>                          continue;
>>                  bo_base->moved = true;
>
> Maybe the whole PT invalidation should be after this to avoid multiple 
> invalidations off the same BO?
>
>
>>
>>                  if (bo->tbo.type == ttm_bo_type_kernel)
>>                          amdgpu_vm_bo_relocated(bo_base);
>>                  else if (bo->tbo.resv == vm->root.base.bo->tbo.resv)
>>                          amdgpu_vm_bo_moved(bo_base);
>>                  else
>>                          amdgpu_vm_bo_invalidated(bo_base);
>
> I believe the last two cases are unreachable now (except if bo_update 
> failed).
>
> Regards,
>   Felix
>
>