Re: [PATCH] drm/amdgpu: PRT support for gfx9

["Zhang, Jerry (Junwei)" <Jerry.Zhang-5C7GfCeVMHo@public.gmane.org>]

On 04/18/2017 07:25 PM, Nicolai Hähnle wrote:
> 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@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.

Yes, the VALID bit is clear actually.
Here clearing it explicitly is to notice that TILED bit should work with V=0.
Just double confirm and reminder to the reader.

>
>
>>>>> +        }
>>>>> +
>>> 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)?

Thanks for you advice.
It's a way to unify the PRT bit in PTE and it doesn't bother older ASIC as 
reserved bit.
I will revise the patch.

BTW, actually I have discussed it with Christian before.
Then we thinked AMDGPU_PTE_PRT was a little hacking to set dummy bit in PTE and 
consider to clear it next. Actually TILED bit is the real one for gfx9 PRT.

Jerry

>
> 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@lists.freedesktop.org
>>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>>>
>>> _______________________________________________
>>> amd-gfx mailing list
>>> amd-gfx@lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx