[PATCH] drm/amdgpu: PRT support for gfx9

[Nicolai Hähnle]

On 18.04.2017 05:13, Zhang, Jerry (Junwei) wrote:
> On 04/18/2017 10:47 AM, zhoucm1 wrote:
>>
>>
>> On 2017年04月18日 09:51, Zhang, Jerry (Junwei) wrote:
>>>
>>> Anyone could help to review it?
>>>
>>> On 04/17/2017 05:04 PM, Junwei Zhang wrote:
>>>> Signed-off-by: Junwei Zhang <Jerry.Zhang at amd.com>
>>>> ---
>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 5 +++++
>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h | 1 +
>>>>   drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c  | 2 +-
>>>>   3 files changed, 7 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>> index 9ff445c..51aedf9 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>> @@ -1269,6 +1269,11 @@ int amdgpu_vm_bo_update(struct amdgpu_device
>>>> *adev,
>>>>       spin_unlock(&vm->status_lock);
>>>>
>>>>       list_for_each_entry(mapping, &bo_va->invalids, list) {
>>>> +        if (mapping->flags & AMDGPU_PTE_TILED) {
>>>> +            flags |= AMDGPU_PTE_TILED;
>>>> +            flags &= ~AMDGPU_PTE_VALID;

Why do you need to explicitly clear the VALID bit here? I'd expect 
whoever creates the mapping to already ensure that the VALID bit is cleared.


>>>> +        }
>>>> +
>> How about clear operation?
>
> CLEAR op will clear all mapping with flag=0, put into free list, and
> then clear them by amdgpu_vm_clear_freed().
>
> When amdgpu_vm_bo_update() is performed, the mapping's flag is 0 now.
>
>>
>>>>           r = amdgpu_vm_bo_split_mapping(adev, exclusive,
>>>>                              gtt_flags, pages_addr, vm,
>>>>                              mapping, flags, mem,
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>>>> index 4904740..8d25914 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>>>> @@ -70,6 +70,7 @@
>>>>   /* VEGA10 only */
>>>>   #define AMDGPU_PTE_MTYPE(a)    ((uint64_t)a << 57)
>>>>   #define AMDGPU_PTE_MTYPE_MASK    AMDGPU_PTE_MTYPE(3ULL)
>>>> +#define AMDGPU_PTE_TILED    (1ULL << 51)
>>>>
>>>>   /* How to programm VM fault handling */
>>>>   #define AMDGPU_VM_FAULT_STOP_NEVER    0
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
>>>> b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
>>>> index 51a1919..6d033ae 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
>>>> @@ -324,7 +324,7 @@ static uint64_t gmc_v9_0_get_vm_pte_flags(struct
>>>> amdgpu_device *adev,
>>>>       }
>>>>
>>>>       if (flags & AMDGPU_VM_PAGE_PRT)
>>>> -        pte_flag |= AMDGPU_PTE_PRT;
>>>> +        pte_flag |= AMDGPU_PTE_TILED;
>> PTE_PRT name doesn't make sense?
>
> This naming is tricky for PRT feature, as there is not PRT bit on PTE
> for pre-gfx9.
> So PTE_PRT is used in reserve bit of PTE.
> But for gfx9, there is a actual bit for PTE, it's better to use the real
> one.

Wouldn't it be better to keep the AMDGPU_PTE_PRT name, and just make 
sure that the bit isn't set on older ASICs (unless the bit is simply 
ignored on older ASICs, in which case we don't have to bother either way)?

Cheers,
Nicolai


>
> BTW, all ASICs will use the general flag in UMD now.
> But different handling inside PTE.
>
> Jerry
>
>>
>> Regards,
>> David Zhou
>>>>
>>>>       return pte_flag;
>>>>   }
>>>>
>>> _______________________________________________
>>> amd-gfx mailing list
>>> amd-gfx at lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>>
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
> _______________________________________________
> amd-gfx mailing list
> amd-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx


-- 
Lerne, wie die Welt wirklich ist,
Aber vergiss niemals, wie sie sein sollte.