Re: [PATCH 1/7] drm/amdgpu: cleanup adjust_mc_addr handling v2

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

On 05/24/2017 05:18 PM, Christian König wrote:
> Am 18.05.2017 um 07:33 schrieb Zhang, Jerry (Junwei):
>> On 05/17/2017 05:22 PM, Christian König wrote:
>>> [SNIP]
>>> +static uint64_t gmc_v6_0_get_vm_pde(struct amdgpu_device *adev, uint64_t addr)
>>> +{
>>> +    BUG_ON(addr & 0xFFFFF0000000FFFULL);
>>> +    return addr;
>>> +}
>>
>> Just wonder why we didn't return the address as below?
>> adev->vm_manager.vram_base_offset + addr
>
> That is a really good question and I couldn't figure out all the details either.
>
> I've tested it on an Kaveri and it still seems to work fine, so the logic is
> indeed correct.

Thanks for your verification.

Then it's a makeshift change for now, although it looks a little suspicious.
Let's dig it more in the future with HW team.
Please feel free to add my RB for the v3 one.

Reviewed-by: Junwei Zhang <Jerry.Zhang@amd.com>

Jerry

>
>> Does that cause gmc6~gmc8 has no APU support officially?
>
> No, only the pro driver doesn't support APUs. The open source driver works
> perfectly fine on them.
>
>> Or I miss any info about them?
>
> The documentation for gfx6-8 is very unclear on this.
>
> It says that the registers and PDE should use physical addresses, but from my
> testing that isn't the case.
>
> Going to ping the hardware dev on this once more when I have time.
>
> Regards,
> Christian.
>
>>
>> Jerry
>>
>>> +
>>>   static void gmc_v6_0_set_fault_enable_default(struct amdgpu_device *adev,
>>>                             bool value)
>>>   {
>>> @@ -1127,6 +1133,7 @@ static const struct amdgpu_gart_funcs
>>> gmc_v6_0_gart_funcs = {
>>>       .flush_gpu_tlb = gmc_v6_0_gart_flush_gpu_tlb,
>>>       .set_pte_pde = gmc_v6_0_gart_set_pte_pde,
>>>       .set_prt = gmc_v6_0_set_prt,
>>> +    .get_vm_pde = gmc_v6_0_get_vm_pde,
>>>       .get_vm_pte_flags = gmc_v6_0_get_vm_pte_flags
>>>   };
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c
>>> b/drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c
>>> index 967505b..4f140e8 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c
>>> @@ -472,6 +472,12 @@ static uint64_t gmc_v7_0_get_vm_pte_flags(struct
>>> amdgpu_device *adev,
>>>       return pte_flag;
>>>   }
>>>
>>> +static uint64_t gmc_v7_0_get_vm_pde(struct amdgpu_device *adev, uint64_t addr)
>>> +{
>>> +    BUG_ON(addr & 0xFFFFF0000000FFFULL);
>>> +    return addr;
>>> +}
>>> +
>>>   /**
>>>    * gmc_v8_0_set_fault_enable_default - update VM fault handling
>>>    *
>>> @@ -1293,7 +1299,8 @@ static const struct amdgpu_gart_funcs
>>> gmc_v7_0_gart_funcs = {
>>>       .flush_gpu_tlb = gmc_v7_0_gart_flush_gpu_tlb,
>>>       .set_pte_pde = gmc_v7_0_gart_set_pte_pde,
>>>       .set_prt = gmc_v7_0_set_prt,
>>> -    .get_vm_pte_flags = gmc_v7_0_get_vm_pte_flags
>>> +    .get_vm_pte_flags = gmc_v7_0_get_vm_pte_flags,
>>> +    .get_vm_pde = gmc_v7_0_get_vm_pde
>>>   };
>>>
>>>   static const struct amdgpu_irq_src_funcs gmc_v7_0_irq_funcs = {
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c
>>> b/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c
>>> index 3b5ea0f..f05c034 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v8_0.c
>>> @@ -656,6 +656,12 @@ static uint64_t gmc_v8_0_get_vm_pte_flags(struct
>>> amdgpu_device *adev,
>>>       return pte_flag;
>>>   }
>>>
>>> +static uint64_t gmc_v8_0_get_vm_pde(struct amdgpu_device *adev, uint64_t addr)
>>> +{
>>> +    BUG_ON(addr & 0xFFF0000000000FFFULL);
>>> +    return addr;
>>> +}
>>> +
>>>   /**
>>>    * gmc_v8_0_set_fault_enable_default - update VM fault handling
>>>    *
>>> @@ -1612,7 +1618,8 @@ static const struct amdgpu_gart_funcs
>>> gmc_v8_0_gart_funcs = {
>>>       .flush_gpu_tlb = gmc_v8_0_gart_flush_gpu_tlb,
>>>       .set_pte_pde = gmc_v8_0_gart_set_pte_pde,
>>>       .set_prt = gmc_v8_0_set_prt,
>>> -    .get_vm_pte_flags = gmc_v8_0_get_vm_pte_flags
>>> +    .get_vm_pte_flags = gmc_v8_0_get_vm_pte_flags,
>>> +    .get_vm_pde = gmc_v8_0_get_vm_pde
>>>   };
>>>
>>>   static const struct amdgpu_irq_src_funcs gmc_v8_0_irq_funcs = {
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
>>> b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
>>> index 19e1027..047b1a7 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
>>> @@ -358,17 +358,19 @@ static uint64_t gmc_v9_0_get_vm_pte_flags(struct
>>> amdgpu_device *adev,
>>>       return pte_flag;
>>>   }
>>>
>>> -static u64 gmc_v9_0_adjust_mc_addr(struct amdgpu_device *adev, u64 mc_addr)
>>> +static u64 gmc_v9_0_get_vm_pde(struct amdgpu_device *adev, u64 addr)
>>>   {
>>> -    return adev->vm_manager.vram_base_offset + mc_addr - adev->mc.vram_start;
>>> +    addr = adev->vm_manager.vram_base_offset + addr - adev->mc.vram_start;
>>> +    BUG_ON(addr & 0xFFFF00000000003FULL);
>>> +    return addr;
>>>   }
>>>
>>>   static const struct amdgpu_gart_funcs gmc_v9_0_gart_funcs = {
>>>       .flush_gpu_tlb = gmc_v9_0_gart_flush_gpu_tlb,
>>>       .set_pte_pde = gmc_v9_0_gart_set_pte_pde,
>>> -    .get_vm_pte_flags = gmc_v9_0_get_vm_pte_flags,
>>> -    .adjust_mc_addr = gmc_v9_0_adjust_mc_addr,
>>>       .get_invalidate_req = gmc_v9_0_get_invalidate_req,
>>> +    .get_vm_pte_flags = gmc_v9_0_get_vm_pte_flags,
>>> +    .get_vm_pde = gmc_v9_0_get_vm_pde
>>>   };
>>>
>>>   static void gmc_v9_0_set_gart_funcs(struct amdgpu_device *adev)
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c
>>> b/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c
>>> index 91cf7e6..fb6f4b3 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c
>>> @@ -1143,10 +1143,8 @@ static void sdma_v4_0_ring_emit_vm_flush(struct
>>> amdgpu_ring *ring,
>>>       uint32_t req = ring->adev->gart.gart_funcs->get_invalidate_req(vm_id);
>>>       unsigned eng = ring->vm_inv_eng;
>>>
>>> -    pd_addr = ring->adev->gart.gart_funcs->adjust_mc_addr(ring->adev,
>>> pd_addr);
>>> -    pd_addr = pd_addr | 0x1; /* valid bit */
>>> -    /* now only use physical base address of PDE and valid */
>>> -    BUG_ON(pd_addr & 0xFFFF00000000003EULL);
>>> +    pd_addr = amdgpu_gart_get_vm_pde(ring->adev, pd_addr);
>>> +    pd_addr |= AMDGPU_PTE_VALID;
>>>
>>>       amdgpu_ring_write(ring, SDMA_PKT_HEADER_OP(SDMA_OP_SRBM_WRITE) |
>>>                 SDMA_PKT_SRBM_WRITE_HEADER_BYTE_EN(0xf));
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/uvd_v7_0.c
>>> b/drivers/gpu/drm/amd/amdgpu/uvd_v7_0.c
>>> index 22f42f3..1997ec8 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/uvd_v7_0.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/uvd_v7_0.c
>>> @@ -1316,10 +1316,8 @@ static void uvd_v7_0_ring_emit_vm_flush(struct
>>> amdgpu_ring *ring,
>>>       uint32_t data0, data1, mask;
>>>       unsigned eng = ring->vm_inv_eng;
>>>
>>> -    pd_addr = ring->adev->gart.gart_funcs->adjust_mc_addr(ring->adev,
>>> pd_addr);
>>> -    pd_addr = pd_addr | 0x1; /* valid bit */
>>> -    /* now only use physical base address of PDE and valid */
>>> -    BUG_ON(pd_addr & 0xFFFF00000000003EULL);
>>> +    pd_addr = amdgpu_gart_get_vm_pde(ring->adev, pd_addr);
>>> +    pd_addr |= AMDGPU_PTE_VALID;
>>>
>>>       data0 = (hub->ctx0_ptb_addr_hi32 + vm_id * 2) << 2;
>>>       data1 = upper_32_bits(pd_addr);
>>> @@ -1358,10 +1356,8 @@ static void uvd_v7_0_enc_ring_emit_vm_flush(struct
>>> amdgpu_ring *ring,
>>>       uint32_t req = ring->adev->gart.gart_funcs->get_invalidate_req(vm_id);
>>>       unsigned eng = ring->vm_inv_eng;
>>>
>>> -    pd_addr = ring->adev->gart.gart_funcs->adjust_mc_addr(ring->adev,
>>> pd_addr);
>>> -    pd_addr = pd_addr | 0x1; /* valid bit */
>>> -    /* now only use physical base address of PDE and valid */
>>> -    BUG_ON(pd_addr & 0xFFFF00000000003EULL);
>>> +    pd_addr = amdgpu_gart_get_vm_pde(ring->adev, pd_addr);
>>> +    pd_addr |= AMDGPU_PTE_VALID;
>>>
>>>       amdgpu_ring_write(ring, HEVC_ENC_CMD_REG_WRITE);
>>>       amdgpu_ring_write(ring,    (hub->ctx0_ptb_addr_hi32 + vm_id * 2) << 2);
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/vce_v4_0.c
>>> b/drivers/gpu/drm/amd/amdgpu/vce_v4_0.c
>>> index 07b2ac7..8dbd0b7 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/vce_v4_0.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/vce_v4_0.c
>>> @@ -926,10 +926,8 @@ static void vce_v4_0_emit_vm_flush(struct amdgpu_ring
>>> *ring,
>>>       uint32_t req = ring->adev->gart.gart_funcs->get_invalidate_req(vm_id);
>>>       unsigned eng = ring->vm_inv_eng;
>>>
>>> -    pd_addr = ring->adev->gart.gart_funcs->adjust_mc_addr(ring->adev,
>>> pd_addr);
>>> -    pd_addr = pd_addr | 0x1; /* valid bit */
>>> -    /* now only use physical base address of PDE and valid */
>>> -    BUG_ON(pd_addr & 0xFFFF00000000003EULL);
>>> +    pd_addr = amdgpu_gart_get_vm_pde(ring->adev, pd_addr);
>>> +    pd_addr |= AMDGPU_PTE_VALID;
>>>
>>>       amdgpu_ring_write(ring, VCE_CMD_REG_WRITE);
>>>       amdgpu_ring_write(ring,    (hub->ctx0_ptb_addr_hi32 + vm_id * 2) << 2);
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c
>>> b/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c
>>> index ad1862f..b2d995b 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c
>>> @@ -882,9 +882,8 @@ static void vcn_v1_0_dec_ring_emit_vm_flush(struct
>>> amdgpu_ring *ring,
>>>       uint32_t data0, data1, mask;
>>>       unsigned eng = ring->vm_inv_eng;
>>>
>>> -    pd_addr = pd_addr | 0x1; /* valid bit */
>>> -    /* now only use physical base address of PDE and valid */
>>> -    BUG_ON(pd_addr & 0xFFFF00000000003EULL);
>>> +    pd_addr = amdgpu_gart_get_vm_pde(ring->adev, pd_addr);
>>> +    pd_addr |= AMDGPU_PTE_VALID;
>>>
>>>       data0 = (hub->ctx0_ptb_addr_hi32 + vm_id * 2) << 2;
>>>       data1 = upper_32_bits(pd_addr);
>>>
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx