[PATCH v3] drm/amdgpu: Don't flush/invalidate HDP for APUs and A+A

[PATCH v3] drm/amdgpu: Don't flush/invalidate HDP for APUs and A+A

[Eric Huang]

Integrate two macros into two generic functions and add
no_flush flag to determine if HDP flush is needed for
all Asics.

Signed-off-by: Eric Huang <jinhuieric.huang@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu.h        |  9 ++++---
 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 29 ++++++++++++++++++++++
 drivers/gpu/drm/amd/amdgpu/amdgpu_hdp.h    |  1 +
 drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c     | 15 ++---------
 4 files changed, 37 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
index 1e3cd4ce9e91..224552d38240 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
@@ -1297,10 +1297,6 @@ int emu_soc_asic_init(struct amdgpu_device *adev);
 #define amdgpu_asic_read_bios_from_rom(adev, b, l) (adev)->asic_funcs->read_bios_from_rom((adev), (b), (l))
 #define amdgpu_asic_read_register(adev, se, sh, offset, v)((adev)->asic_funcs->read_register((adev), (se), (sh), (offset), (v)))
 #define amdgpu_asic_get_config_memsize(adev) (adev)->asic_funcs->get_config_memsize((adev))
-#define amdgpu_asic_flush_hdp(adev, r) \
-	((adev)->asic_funcs->flush_hdp ? (adev)->asic_funcs->flush_hdp((adev), (r)) : (adev)->hdp.funcs->flush_hdp((adev), (r)))
-#define amdgpu_asic_invalidate_hdp(adev, r) \
-	((adev)->asic_funcs->invalidate_hdp ? (adev)->asic_funcs->invalidate_hdp((adev), (r)) : (adev)->hdp.funcs->invalidate_hdp((adev), (r)))
 #define amdgpu_asic_need_full_reset(adev) (adev)->asic_funcs->need_full_reset((adev))
 #define amdgpu_asic_init_doorbell_index(adev) (adev)->asic_funcs->init_doorbell_index((adev))
 #define amdgpu_asic_get_pcie_usage(adev, cnt0, cnt1) ((adev)->asic_funcs->get_pcie_usage((adev), (cnt0), (cnt1)))
@@ -1314,6 +1310,11 @@ int emu_soc_asic_init(struct amdgpu_device *adev);
 
 #define amdgpu_inc_vram_lost(adev) atomic_inc(&((adev)->vram_lost_counter));
 
+void amdgpu_asic_flush_hdp(struct amdgpu_device *adev,
+		struct amdgpu_ring *ring);
+void amdgpu_asic_invalidate_hdp(struct amdgpu_device *adev,
+		struct amdgpu_ring *ring);
+
 /* Common functions */
 bool amdgpu_device_has_job_running(struct amdgpu_device *adev);
 bool amdgpu_device_should_recover_gpu(struct amdgpu_device *adev);
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index 9c4d33f649f7..951feefb5e76 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -3505,6 +3505,9 @@ int amdgpu_device_init(struct amdgpu_device *adev,
 	if (r)
 		goto failed_unmap;
 
+	if ((adev->flags & AMD_IS_APU) || adev->gmc.xgmi.connected_to_cpu)
+		adev->hdp.no_flush = true;
+
 	/* doorbell bar mapping and doorbell index init*/
 	amdgpu_device_doorbell_init(adev);
 
@@ -5548,3 +5551,29 @@ bool amdgpu_device_is_headless(struct amdgpu_device *adev)
     /* Check if it is NV's VGA class while VCN is harvest, it is headless*/
     return nv_is_headless_sku(adev->pdev);
 }
+
+void amdgpu_asic_flush_hdp(struct amdgpu_device *adev,
+		struct amdgpu_ring *ring)
+{
+	if (adev->hdp.no_flush)
+		return;
+
+	if (ring->funcs->emit_hdp_flush)
+		amdgpu_ring_emit_hdp_flush(ring);
+	else if (adev->asic_funcs->flush_hdp)
+		adev->asic_funcs->flush_hdp(adev, ring);
+	else
+		adev->hdp.funcs->flush_hdp(adev, ring);
+}
+
+void amdgpu_asic_invalidate_hdp(struct amdgpu_device *adev,
+		struct amdgpu_ring *ring)
+{
+	if (adev->hdp.no_flush)
+		return;
+
+	if (adev->asic_funcs->invalidate_hdp)
+		adev->asic_funcs->invalidate_hdp(adev, ring);
+	else
+		adev->hdp.funcs->invalidate_hdp(adev, ring);
+}
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_hdp.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_hdp.h
index 7ec99d591584..1ca23f2f51d2 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_hdp.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_hdp.h
@@ -44,6 +44,7 @@ struct amdgpu_hdp {
 	struct ras_common_if			*ras_if;
 	const struct amdgpu_hdp_funcs		*funcs;
 	const struct amdgpu_hdp_ras_funcs	*ras_funcs;
+	bool	no_flush;
 };
 
 int amdgpu_hdp_ras_late_init(struct amdgpu_device *adev);
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
index aaa2574ce9bc..6db676c4c7e1 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
@@ -222,15 +222,7 @@ int amdgpu_ib_schedule(struct amdgpu_ring *ring, unsigned num_ibs,
 	if (job && ring->funcs->init_cond_exec)
 		patch_offset = amdgpu_ring_init_cond_exec(ring);
 
-#ifdef CONFIG_X86_64
-	if (!(adev->flags & AMD_IS_APU))
-#endif
-	{
-		if (ring->funcs->emit_hdp_flush)
-			amdgpu_ring_emit_hdp_flush(ring);
-		else
-			amdgpu_asic_flush_hdp(adev, ring);
-	}
+	amdgpu_asic_flush_hdp(adev, ring);
 
 	if (need_ctx_switch)
 		status |= AMDGPU_HAVE_CTX_SWITCH;
@@ -267,10 +259,7 @@ int amdgpu_ib_schedule(struct amdgpu_ring *ring, unsigned num_ibs,
 	if (job && ring->funcs->emit_frame_cntl)
 		amdgpu_ring_emit_frame_cntl(ring, false, secure);
 
-#ifdef CONFIG_X86_64
-	if (!(adev->flags & AMD_IS_APU))
-#endif
-		amdgpu_asic_invalidate_hdp(adev, ring);
+	amdgpu_asic_invalidate_hdp(adev, ring);
 
 	if (ib->flags & AMDGPU_IB_FLAG_TC_WB_NOT_INVALIDATE)
 		fence_flags |= AMDGPU_FENCE_FLAG_TC_WB_ONLY;
-- 
2.25.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH v3] drm/amdgpu: Don't flush/invalidate HDP for APUs and A+A

[Christian König]

Am 02.06.21 um 04:04 schrieb Eric Huang:
> Integrate two macros into two generic functions and add
> no_flush flag to determine if HDP flush is needed for
> all Asics.

Yes that starts looks like it should work, just a few comments below.

>
> Signed-off-by: Eric Huang <jinhuieric.huang@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu.h        |  9 ++++---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 29 ++++++++++++++++++++++
>   drivers/gpu/drm/amd/amdgpu/amdgpu_hdp.h    |  1 +
>   drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c     | 15 ++---------
>   4 files changed, 37 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> index 1e3cd4ce9e91..224552d38240 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> @@ -1297,10 +1297,6 @@ int emu_soc_asic_init(struct amdgpu_device *adev);
>   #define amdgpu_asic_read_bios_from_rom(adev, b, l) (adev)->asic_funcs->read_bios_from_rom((adev), (b), (l))
>   #define amdgpu_asic_read_register(adev, se, sh, offset, v)((adev)->asic_funcs->read_register((adev), (se), (sh), (offset), (v)))
>   #define amdgpu_asic_get_config_memsize(adev) (adev)->asic_funcs->get_config_memsize((adev))
> -#define amdgpu_asic_flush_hdp(adev, r) \
> -	((adev)->asic_funcs->flush_hdp ? (adev)->asic_funcs->flush_hdp((adev), (r)) : (adev)->hdp.funcs->flush_hdp((adev), (r)))
> -#define amdgpu_asic_invalidate_hdp(adev, r) \
> -	((adev)->asic_funcs->invalidate_hdp ? (adev)->asic_funcs->invalidate_hdp((adev), (r)) : (adev)->hdp.funcs->invalidate_hdp((adev), (r)))
>   #define amdgpu_asic_need_full_reset(adev) (adev)->asic_funcs->need_full_reset((adev))
>   #define amdgpu_asic_init_doorbell_index(adev) (adev)->asic_funcs->init_doorbell_index((adev))
>   #define amdgpu_asic_get_pcie_usage(adev, cnt0, cnt1) ((adev)->asic_funcs->get_pcie_usage((adev), (cnt0), (cnt1)))
> @@ -1314,6 +1310,11 @@ int emu_soc_asic_init(struct amdgpu_device *adev);
>   
>   #define amdgpu_inc_vram_lost(adev) atomic_inc(&((adev)->vram_lost_counter));
>   
> +void amdgpu_asic_flush_hdp(struct amdgpu_device *adev,
> +		struct amdgpu_ring *ring);
> +void amdgpu_asic_invalidate_hdp(struct amdgpu_device *adev,
> +		struct amdgpu_ring *ring);
> +
>   /* Common functions */
>   bool amdgpu_device_has_job_running(struct amdgpu_device *adev);
>   bool amdgpu_device_should_recover_gpu(struct amdgpu_device *adev);
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index 9c4d33f649f7..951feefb5e76 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -3505,6 +3505,9 @@ int amdgpu_device_init(struct amdgpu_device *adev,
>   	if (r)
>   		goto failed_unmap;
>   
> +	if ((adev->flags & AMD_IS_APU) || adev->gmc.xgmi.connected_to_cpu)
> +		adev->hdp.no_flush = true;
> +

This is not correct. We can only skip HDP flush on X86_64, but not on 
32bit builds.

Additional to that we might want this somewhere in the GMC code and not 
here.

>   	/* doorbell bar mapping and doorbell index init*/
>   	amdgpu_device_doorbell_init(adev);
>   
> @@ -5548,3 +5551,29 @@ bool amdgpu_device_is_headless(struct amdgpu_device *adev)
>       /* Check if it is NV's VGA class while VCN is harvest, it is headless*/
>       return nv_is_headless_sku(adev->pdev);
>   }
> +
> +void amdgpu_asic_flush_hdp(struct amdgpu_device *adev,
> +		struct amdgpu_ring *ring)

Please name the functions amdgpu_device_, the amdgpu_asic_ names are for 
callbacks only.

> +{
> +	if (adev->hdp.no_flush)
> +		return;

Since we now have a function to check the different conditions I think 
we might want to check for X86_64 & APU and XGMI connection separately here.

But that is not a hard requirement, using the no_flush variable is fine 
with me if you think that this is better.

> +
> +	if (ring->funcs->emit_hdp_flush)
> +		amdgpu_ring_emit_hdp_flush(ring);
> +	else if (adev->asic_funcs->flush_hdp)
> +		adev->asic_funcs->flush_hdp(adev, ring);
> +	else
> +		adev->hdp.funcs->flush_hdp(adev, ring);
> +}
> +
> +void amdgpu_asic_invalidate_hdp(struct amdgpu_device *adev,
> +		struct amdgpu_ring *ring)
> +{
> +	if (adev->hdp.no_flush)
> +		return;
> +
> +	if (adev->asic_funcs->invalidate_hdp)
> +		adev->asic_funcs->invalidate_hdp(adev, ring);
> +	else
> +		adev->hdp.funcs->invalidate_hdp(adev, ring);
> +}
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_hdp.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_hdp.h
> index 7ec99d591584..1ca23f2f51d2 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_hdp.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_hdp.h
> @@ -44,6 +44,7 @@ struct amdgpu_hdp {
>   	struct ras_common_if			*ras_if;
>   	const struct amdgpu_hdp_funcs		*funcs;
>   	const struct amdgpu_hdp_ras_funcs	*ras_funcs;
> +	bool	no_flush;
>   };
>   
>   int amdgpu_hdp_ras_late_init(struct amdgpu_device *adev);
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
> index aaa2574ce9bc..6db676c4c7e1 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
> @@ -222,15 +222,7 @@ int amdgpu_ib_schedule(struct amdgpu_ring *ring, unsigned num_ibs,
>   	if (job && ring->funcs->init_cond_exec)
>   		patch_offset = amdgpu_ring_init_cond_exec(ring);
>   
> -#ifdef CONFIG_X86_64
> -	if (!(adev->flags & AMD_IS_APU))
> -#endif
> -	{
> -		if (ring->funcs->emit_hdp_flush)
> -			amdgpu_ring_emit_hdp_flush(ring);
> -		else
> -			amdgpu_asic_flush_hdp(adev, ring);
> -	}
> +	amdgpu_asic_flush_hdp(adev, ring);

We also have two more HDP flush cases in the GART code and the CPU based 
VM code IIRC.

Thanks,
Christian.

>   
>   	if (need_ctx_switch)
>   		status |= AMDGPU_HAVE_CTX_SWITCH;
> @@ -267,10 +259,7 @@ int amdgpu_ib_schedule(struct amdgpu_ring *ring, unsigned num_ibs,
>   	if (job && ring->funcs->emit_frame_cntl)
>   		amdgpu_ring_emit_frame_cntl(ring, false, secure);
>   
> -#ifdef CONFIG_X86_64
> -	if (!(adev->flags & AMD_IS_APU))
> -#endif
> -		amdgpu_asic_invalidate_hdp(adev, ring);
> +	amdgpu_asic_invalidate_hdp(adev, ring);
>   
>   	if (ib->flags & AMDGPU_IB_FLAG_TC_WB_NOT_INVALIDATE)
>   		fence_flags |= AMDGPU_FENCE_FLAG_TC_WB_ONLY;

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH v3] drm/amdgpu: Don't flush/invalidate HDP for APUs and A+A

[Eric Huang]

On 2021-06-02 3:00 a.m., Christian König wrote:
> Am 02.06.21 um 04:04 schrieb Eric Huang:
>> Integrate two macros into two generic functions and add
>> no_flush flag to determine if HDP flush is needed for
>> all Asics.
>
> Yes that starts looks like it should work, just a few comments below.
>
>>
>> Signed-off-by: Eric Huang <jinhuieric.huang@amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu.h        |  9 ++++---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 29 ++++++++++++++++++++++
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_hdp.h    |  1 +
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c     | 15 ++---------
>>   4 files changed, 37 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>> index 1e3cd4ce9e91..224552d38240 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>> @@ -1297,10 +1297,6 @@ int emu_soc_asic_init(struct amdgpu_device 
>> *adev);
>>   #define amdgpu_asic_read_bios_from_rom(adev, b, l) 
>> (adev)->asic_funcs->read_bios_from_rom((adev), (b), (l))
>>   #define amdgpu_asic_read_register(adev, se, sh, offset, 
>> v)((adev)->asic_funcs->read_register((adev), (se), (sh), (offset), (v)))
>>   #define amdgpu_asic_get_config_memsize(adev) 
>> (adev)->asic_funcs->get_config_memsize((adev))
>> -#define amdgpu_asic_flush_hdp(adev, r) \
>> -    ((adev)->asic_funcs->flush_hdp ? 
>> (adev)->asic_funcs->flush_hdp((adev), (r)) : 
>> (adev)->hdp.funcs->flush_hdp((adev), (r)))
>> -#define amdgpu_asic_invalidate_hdp(adev, r) \
>> -    ((adev)->asic_funcs->invalidate_hdp ? 
>> (adev)->asic_funcs->invalidate_hdp((adev), (r)) : 
>> (adev)->hdp.funcs->invalidate_hdp((adev), (r)))
>>   #define amdgpu_asic_need_full_reset(adev) 
>> (adev)->asic_funcs->need_full_reset((adev))
>>   #define amdgpu_asic_init_doorbell_index(adev) 
>> (adev)->asic_funcs->init_doorbell_index((adev))
>>   #define amdgpu_asic_get_pcie_usage(adev, cnt0, cnt1) 
>> ((adev)->asic_funcs->get_pcie_usage((adev), (cnt0), (cnt1)))
>> @@ -1314,6 +1310,11 @@ int emu_soc_asic_init(struct amdgpu_device 
>> *adev);
>>     #define amdgpu_inc_vram_lost(adev) 
>> atomic_inc(&((adev)->vram_lost_counter));
>>   +void amdgpu_asic_flush_hdp(struct amdgpu_device *adev,
>> +        struct amdgpu_ring *ring);
>> +void amdgpu_asic_invalidate_hdp(struct amdgpu_device *adev,
>> +        struct amdgpu_ring *ring);
>> +
>>   /* Common functions */
>>   bool amdgpu_device_has_job_running(struct amdgpu_device *adev);
>>   bool amdgpu_device_should_recover_gpu(struct amdgpu_device *adev);
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> index 9c4d33f649f7..951feefb5e76 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> @@ -3505,6 +3505,9 @@ int amdgpu_device_init(struct amdgpu_device *adev,
>>       if (r)
>>           goto failed_unmap;
>>   +    if ((adev->flags & AMD_IS_APU) || 
>> adev->gmc.xgmi.connected_to_cpu)
>> +        adev->hdp.no_flush = true;
>> +
>
> This is not correct. We can only skip HDP flush on X86_64, but not on 
> 32bit builds.
>
> Additional to that we might want this somewhere in the GMC code and 
> not here.
>
>>       /* doorbell bar mapping and doorbell index init*/
>>       amdgpu_device_doorbell_init(adev);
>>   @@ -5548,3 +5551,29 @@ bool amdgpu_device_is_headless(struct 
>> amdgpu_device *adev)
>>       /* Check if it is NV's VGA class while VCN is harvest, it is 
>> headless*/
>>       return nv_is_headless_sku(adev->pdev);
>>   }
>> +
>> +void amdgpu_asic_flush_hdp(struct amdgpu_device *adev,
>> +        struct amdgpu_ring *ring)
>
> Please name the functions amdgpu_device_, the amdgpu_asic_ names are 
> for callbacks only.

I was thinking about the naming you mention, then we have to change all 
the caller for amdgpu_asic_flush_hdp(). The idea here is to simplify 
changes. If you think changing name is necessary, I am fine with that.

>
>> +{
>> +    if (adev->hdp.no_flush)
>> +        return;
>
> Since we now have a function to check the different conditions I think 
> we might want to check for X86_64 & APU and XGMI connection separately 
> here.
>
> But that is not a hard requirement, using the no_flush variable is 
> fine with me if you think that this is better.
>
>> +
>> +    if (ring->funcs->emit_hdp_flush)
>> +        amdgpu_ring_emit_hdp_flush(ring);
>> +    else if (adev->asic_funcs->flush_hdp)
>> +        adev->asic_funcs->flush_hdp(adev, ring);
>> +    else
>> +        adev->hdp.funcs->flush_hdp(adev, ring);
>> +}
>> +
>> +void amdgpu_asic_invalidate_hdp(struct amdgpu_device *adev,
>> +        struct amdgpu_ring *ring)
>> +{
>> +    if (adev->hdp.no_flush)
>> +        return;
>> +
>> +    if (adev->asic_funcs->invalidate_hdp)
>> +        adev->asic_funcs->invalidate_hdp(adev, ring);
>> +    else
>> +        adev->hdp.funcs->invalidate_hdp(adev, ring);
>> +}
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_hdp.h 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_hdp.h
>> index 7ec99d591584..1ca23f2f51d2 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_hdp.h
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_hdp.h
>> @@ -44,6 +44,7 @@ struct amdgpu_hdp {
>>       struct ras_common_if            *ras_if;
>>       const struct amdgpu_hdp_funcs        *funcs;
>>       const struct amdgpu_hdp_ras_funcs    *ras_funcs;
>> +    bool    no_flush;
>>   };
>>     int amdgpu_hdp_ras_late_init(struct amdgpu_device *adev);
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
>> index aaa2574ce9bc..6db676c4c7e1 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
>> @@ -222,15 +222,7 @@ int amdgpu_ib_schedule(struct amdgpu_ring *ring, 
>> unsigned num_ibs,
>>       if (job && ring->funcs->init_cond_exec)
>>           patch_offset = amdgpu_ring_init_cond_exec(ring);
>>   -#ifdef CONFIG_X86_64
>> -    if (!(adev->flags & AMD_IS_APU))
>> -#endif
>> -    {
>> -        if (ring->funcs->emit_hdp_flush)
>> -            amdgpu_ring_emit_hdp_flush(ring);
>> -        else
>> -            amdgpu_asic_flush_hdp(adev, ring);
>> -    }
>> +    amdgpu_asic_flush_hdp(adev, ring);
>
> We also have two more HDP flush cases in the GART code and the CPU 
> based VM code IIRC.

Are there calling amdgpu_asic_flush_hdp() in function 
amdgpu_gart_unbind() and amdgpu_vm_cpu_commit()? if so, keeping the same 
name of amdgpu_asic_flush_hdp() will not need to change. That is the 
reason I don't change the name.

Regards,
Eric
>
> Thanks,
> Christian.
>
>>         if (need_ctx_switch)
>>           status |= AMDGPU_HAVE_CTX_SWITCH;
>> @@ -267,10 +259,7 @@ int amdgpu_ib_schedule(struct amdgpu_ring *ring, 
>> unsigned num_ibs,
>>       if (job && ring->funcs->emit_frame_cntl)
>>           amdgpu_ring_emit_frame_cntl(ring, false, secure);
>>   -#ifdef CONFIG_X86_64
>> -    if (!(adev->flags & AMD_IS_APU))
>> -#endif
>> -        amdgpu_asic_invalidate_hdp(adev, ring);
>> +    amdgpu_asic_invalidate_hdp(adev, ring);
>>         if (ib->flags & AMDGPU_IB_FLAG_TC_WB_NOT_INVALIDATE)
>>           fence_flags |= AMDGPU_FENCE_FLAG_TC_WB_ONLY;
>

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH v3] drm/amdgpu: Don't flush/invalidate HDP for APUs and A+A

[Christian König]

Am 02.06.21 um 17:26 schrieb Eric Huang:
>
>
> On 2021-06-02 3:00 a.m., Christian König wrote:
>> Am 02.06.21 um 04:04 schrieb Eric Huang:
>>> Integrate two macros into two generic functions and add
>>> no_flush flag to determine if HDP flush is needed for
>>> all Asics.
>>
>> Yes that starts looks like it should work, just a few comments below.
>>
>>>
>>> Signed-off-by: Eric Huang <jinhuieric.huang@amd.com>
>>> ---
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu.h        |  9 ++++---
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 29 
>>> ++++++++++++++++++++++
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_hdp.h    |  1 +
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c     | 15 ++---------
>>>   4 files changed, 37 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h 
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>> index 1e3cd4ce9e91..224552d38240 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
>>> @@ -1297,10 +1297,6 @@ int emu_soc_asic_init(struct amdgpu_device 
>>> *adev);
>>>   #define amdgpu_asic_read_bios_from_rom(adev, b, l) 
>>> (adev)->asic_funcs->read_bios_from_rom((adev), (b), (l))
>>>   #define amdgpu_asic_read_register(adev, se, sh, offset, 
>>> v)((adev)->asic_funcs->read_register((adev), (se), (sh), (offset), 
>>> (v)))
>>>   #define amdgpu_asic_get_config_memsize(adev) 
>>> (adev)->asic_funcs->get_config_memsize((adev))
>>> -#define amdgpu_asic_flush_hdp(adev, r) \
>>> -    ((adev)->asic_funcs->flush_hdp ? 
>>> (adev)->asic_funcs->flush_hdp((adev), (r)) : 
>>> (adev)->hdp.funcs->flush_hdp((adev), (r)))
>>> -#define amdgpu_asic_invalidate_hdp(adev, r) \
>>> -    ((adev)->asic_funcs->invalidate_hdp ? 
>>> (adev)->asic_funcs->invalidate_hdp((adev), (r)) : 
>>> (adev)->hdp.funcs->invalidate_hdp((adev), (r)))
>>>   #define amdgpu_asic_need_full_reset(adev) 
>>> (adev)->asic_funcs->need_full_reset((adev))
>>>   #define amdgpu_asic_init_doorbell_index(adev) 
>>> (adev)->asic_funcs->init_doorbell_index((adev))
>>>   #define amdgpu_asic_get_pcie_usage(adev, cnt0, cnt1) 
>>> ((adev)->asic_funcs->get_pcie_usage((adev), (cnt0), (cnt1)))
>>> @@ -1314,6 +1310,11 @@ int emu_soc_asic_init(struct amdgpu_device 
>>> *adev);
>>>     #define amdgpu_inc_vram_lost(adev) 
>>> atomic_inc(&((adev)->vram_lost_counter));
>>>   +void amdgpu_asic_flush_hdp(struct amdgpu_device *adev,
>>> +        struct amdgpu_ring *ring);
>>> +void amdgpu_asic_invalidate_hdp(struct amdgpu_device *adev,
>>> +        struct amdgpu_ring *ring);
>>> +
>>>   /* Common functions */
>>>   bool amdgpu_device_has_job_running(struct amdgpu_device *adev);
>>>   bool amdgpu_device_should_recover_gpu(struct amdgpu_device *adev);
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c 
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>> index 9c4d33f649f7..951feefb5e76 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>> @@ -3505,6 +3505,9 @@ int amdgpu_device_init(struct amdgpu_device 
>>> *adev,
>>>       if (r)
>>>           goto failed_unmap;
>>>   +    if ((adev->flags & AMD_IS_APU) || 
>>> adev->gmc.xgmi.connected_to_cpu)
>>> +        adev->hdp.no_flush = true;
>>> +
>>
>> This is not correct. We can only skip HDP flush on X86_64, but not on 
>> 32bit builds.
>>
>> Additional to that we might want this somewhere in the GMC code and 
>> not here.
>>
>>>       /* doorbell bar mapping and doorbell index init*/
>>>       amdgpu_device_doorbell_init(adev);
>>>   @@ -5548,3 +5551,29 @@ bool amdgpu_device_is_headless(struct 
>>> amdgpu_device *adev)
>>>       /* Check if it is NV's VGA class while VCN is harvest, it is 
>>> headless*/
>>>       return nv_is_headless_sku(adev->pdev);
>>>   }
>>> +
>>> +void amdgpu_asic_flush_hdp(struct amdgpu_device *adev,
>>> +        struct amdgpu_ring *ring)
>>
>> Please name the functions amdgpu_device_, the amdgpu_asic_ names are 
>> for callbacks only.
>
> I was thinking about the naming you mention, then we have to change 
> all the caller for amdgpu_asic_flush_hdp(). The idea here is to 
> simplify changes. If you think changing name is necessary, I am fine 
> with that.

I think we have reserved the _asic_ prefix for the macro wrappers only 
and to be honest I think _device_ makes more sense.

>
>>
>>> +{
>>> +    if (adev->hdp.no_flush)
>>> +        return;
>>
>> Since we now have a function to check the different conditions I 
>> think we might want to check for X86_64 & APU and XGMI connection 
>> separately here.
>>
>> But that is not a hard requirement, using the no_flush variable is 
>> fine with me if you think that this is better.
>>
>>> +
>>> +    if (ring->funcs->emit_hdp_flush)
>>> +        amdgpu_ring_emit_hdp_flush(ring);

BTW: You should probably check "ring && ring->funcs->emit_hdp_flush" 
here, cause the ring is only an optional argument.

Christian.

>>> +    else if (adev->asic_funcs->flush_hdp)
>>> +        adev->asic_funcs->flush_hdp(adev, ring);
>>> +    else
>>> +        adev->hdp.funcs->flush_hdp(adev, ring);
>>> +}
>>> +
>>> +void amdgpu_asic_invalidate_hdp(struct amdgpu_device *adev,
>>> +        struct amdgpu_ring *ring)
>>> +{
>>> +    if (adev->hdp.no_flush)
>>> +        return;
>>> +
>>> +    if (adev->asic_funcs->invalidate_hdp)
>>> +        adev->asic_funcs->invalidate_hdp(adev, ring);
>>> +    else
>>> +        adev->hdp.funcs->invalidate_hdp(adev, ring);
>>> +}
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_hdp.h 
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_hdp.h
>>> index 7ec99d591584..1ca23f2f51d2 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_hdp.h
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_hdp.h
>>> @@ -44,6 +44,7 @@ struct amdgpu_hdp {
>>>       struct ras_common_if            *ras_if;
>>>       const struct amdgpu_hdp_funcs        *funcs;
>>>       const struct amdgpu_hdp_ras_funcs    *ras_funcs;
>>> +    bool    no_flush;
>>>   };
>>>     int amdgpu_hdp_ras_late_init(struct amdgpu_device *adev);
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c 
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
>>> index aaa2574ce9bc..6db676c4c7e1 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
>>> @@ -222,15 +222,7 @@ int amdgpu_ib_schedule(struct amdgpu_ring 
>>> *ring, unsigned num_ibs,
>>>       if (job && ring->funcs->init_cond_exec)
>>>           patch_offset = amdgpu_ring_init_cond_exec(ring);
>>>   -#ifdef CONFIG_X86_64
>>> -    if (!(adev->flags & AMD_IS_APU))
>>> -#endif
>>> -    {
>>> -        if (ring->funcs->emit_hdp_flush)
>>> -            amdgpu_ring_emit_hdp_flush(ring);
>>> -        else
>>> -            amdgpu_asic_flush_hdp(adev, ring);
>>> -    }
>>> +    amdgpu_asic_flush_hdp(adev, ring);
>>
>> We also have two more HDP flush cases in the GART code and the CPU 
>> based VM code IIRC.
>
> Are there calling amdgpu_asic_flush_hdp() in function 
> amdgpu_gart_unbind() and amdgpu_vm_cpu_commit()? if so, keeping the 
> same name of amdgpu_asic_flush_hdp() will not need to change. That is 
> the reason I don't change the name.
>
> Regards,
> Eric
>>
>> Thanks,
>> Christian.
>>
>>>         if (need_ctx_switch)
>>>           status |= AMDGPU_HAVE_CTX_SWITCH;
>>> @@ -267,10 +259,7 @@ int amdgpu_ib_schedule(struct amdgpu_ring 
>>> *ring, unsigned num_ibs,
>>>       if (job && ring->funcs->emit_frame_cntl)
>>>           amdgpu_ring_emit_frame_cntl(ring, false, secure);
>>>   -#ifdef CONFIG_X86_64
>>> -    if (!(adev->flags & AMD_IS_APU))
>>> -#endif
>>> -        amdgpu_asic_invalidate_hdp(adev, ring);
>>> +    amdgpu_asic_invalidate_hdp(adev, ring);
>>>         if (ib->flags & AMDGPU_IB_FLAG_TC_WB_NOT_INVALIDATE)
>>>           fence_flags |= AMDGPU_FENCE_FLAG_TC_WB_ONLY;
>>
>

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx