All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2 1/2] drm/amdgpu: Fix a bug on flag table_freed
@ 2021-06-01  0:06 Eric Huang
  2021-06-01  0:06 ` [PATCH v2 2/2] drm/amdgpu: Don't flush HDP on A+A Eric Huang
  2021-06-01  7:02 ` [PATCH v2 1/2] drm/amdgpu: Fix a bug on flag table_freed Christian König
  0 siblings, 2 replies; 6+ messages in thread
From: Eric Huang @ 2021-06-01  0:06 UTC (permalink / raw)
  To: amd-gfx; +Cc: Eric Huang

table_freed will be always true when mapping a memory with size
bigger than 2MB. The problem is page table's entries are always
existed, but existing mapping depends on page talbe's bo, so
using a check of page table's bo existed will resolve the issue.

Signed-off-by: Eric Huang <jinhuieric.huang@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 539c117906cc..2c20bba7dc1a 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -1582,9 +1582,12 @@ static int amdgpu_vm_update_ptes(struct amdgpu_vm_update_params *params,
 			 * completely covered by the range and so potentially still in use.
 			 */
 			while (cursor.pfn < frag_start) {
-				amdgpu_vm_free_pts(adev, params->vm, &cursor);
+				/* Make sure previous mapping is freed */
+				if (cursor.entry->base.bo) {
+					params->table_freed = true;
+					amdgpu_vm_free_pts(adev, params->vm, &cursor);
+				}
 				amdgpu_vm_pt_next(adev, &cursor);
-				params->table_freed = true;
 			}
 
 		} else if (frag >= shift) {
-- 
2.25.1

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

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH v2 2/2] drm/amdgpu: Don't flush HDP on A+A
  2021-06-01  0:06 [PATCH v2 1/2] drm/amdgpu: Fix a bug on flag table_freed Eric Huang
@ 2021-06-01  0:06 ` Eric Huang
  2021-06-01  7:05   ` Christian König
  2021-06-01  7:02 ` [PATCH v2 1/2] drm/amdgpu: Fix a bug on flag table_freed Christian König
  1 sibling, 1 reply; 6+ messages in thread
From: Eric Huang @ 2021-06-01  0:06 UTC (permalink / raw)
  To: amd-gfx; +Cc: Eric Huang

With XGMI connection flushing HDP on PCIe is unnecessary,
it is also to optimize memory allocation latency.

Signed-off-by: Eric Huang <jinhuieric.huang@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_hdp.h | 1 +
 drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c  | 3 ++-
 drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c   | 1 +
 drivers/gpu/drm/amd/amdgpu/hdp_v4_0.c   | 3 +++
 4 files changed, 7 insertions(+), 1 deletion(-)

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..f31eae2931f3 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
@@ -226,7 +226,8 @@ int amdgpu_ib_schedule(struct amdgpu_ring *ring, unsigned num_ibs,
 	if (!(adev->flags & AMD_IS_APU))
 #endif
 	{
-		if (ring->funcs->emit_hdp_flush)
+		if (ring->funcs->emit_hdp_flush &&
+			!adev->hdp.no_flush)
 			amdgpu_ring_emit_hdp_flush(ring);
 		else
 			amdgpu_asic_flush_hdp(adev, ring);
diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
index 2749621d5f63..6e1eab615914 100644
--- a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
@@ -1223,6 +1223,7 @@ static int gmc_v9_0_early_init(void *handle)
 		adev->gmc.xgmi.supported = true;
 		adev->gmc.xgmi.connected_to_cpu =
 			adev->smuio.funcs->is_host_gpu_xgmi_supported(adev);
+		adev->hdp.no_flush = adev->gmc.xgmi.connected_to_cpu;
 	}
 
 	gmc_v9_0_set_gmc_funcs(adev);
diff --git a/drivers/gpu/drm/amd/amdgpu/hdp_v4_0.c b/drivers/gpu/drm/amd/amdgpu/hdp_v4_0.c
index 74b90cc2bf48..e1b2face8656 100644
--- a/drivers/gpu/drm/amd/amdgpu/hdp_v4_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/hdp_v4_0.c
@@ -40,6 +40,9 @@
 static void hdp_v4_0_flush_hdp(struct amdgpu_device *adev,
 				struct amdgpu_ring *ring)
 {
+	if (adev->hdp.no_flush)
+		return;
+
 	if (!ring || !ring->funcs->emit_wreg)
 		WREG32_NO_KIQ((adev->rmmio_remap.reg_offset + KFD_MMIO_REMAP_HDP_MEM_FLUSH_CNTL) >> 2, 0);
 	else
-- 
2.25.1

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

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH v2 1/2] drm/amdgpu: Fix a bug on flag table_freed
  2021-06-01  0:06 [PATCH v2 1/2] drm/amdgpu: Fix a bug on flag table_freed Eric Huang
  2021-06-01  0:06 ` [PATCH v2 2/2] drm/amdgpu: Don't flush HDP on A+A Eric Huang
@ 2021-06-01  7:02 ` Christian König
  1 sibling, 0 replies; 6+ messages in thread
From: Christian König @ 2021-06-01  7:02 UTC (permalink / raw)
  To: Eric Huang, amd-gfx



Am 01.06.21 um 02:06 schrieb Eric Huang:
> table_freed will be always true when mapping a memory with size
> bigger than 2MB. The problem is page table's entries are always
> existed, but existing mapping depends on page talbe's bo, so
> using a check of page table's bo existed will resolve the issue.
>
> Signed-off-by: Eric Huang <jinhuieric.huang@amd.com>

Reviewed-by: Christian König <christian.koenig@amd.com> for this one.

> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 7 +++++--
>   1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index 539c117906cc..2c20bba7dc1a 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -1582,9 +1582,12 @@ static int amdgpu_vm_update_ptes(struct amdgpu_vm_update_params *params,
>   			 * completely covered by the range and so potentially still in use.
>   			 */
>   			while (cursor.pfn < frag_start) {
> -				amdgpu_vm_free_pts(adev, params->vm, &cursor);
> +				/* Make sure previous mapping is freed */
> +				if (cursor.entry->base.bo) {
> +					params->table_freed = true;
> +					amdgpu_vm_free_pts(adev, params->vm, &cursor);
> +				}
>   				amdgpu_vm_pt_next(adev, &cursor);
> -				params->table_freed = true;
>   			}
>   
>   		} else if (frag >= shift) {

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2 2/2] drm/amdgpu: Don't flush HDP on A+A
  2021-06-01  0:06 ` [PATCH v2 2/2] drm/amdgpu: Don't flush HDP on A+A Eric Huang
@ 2021-06-01  7:05   ` Christian König
  2021-06-01 14:45     ` Eric Huang
  0 siblings, 1 reply; 6+ messages in thread
From: Christian König @ 2021-06-01  7:05 UTC (permalink / raw)
  To: Eric Huang, amd-gfx

Am 01.06.21 um 02:06 schrieb Eric Huang:
> With XGMI connection flushing HDP on PCIe is unnecessary,
> it is also to optimize memory allocation latency.
>
> Signed-off-by: Eric Huang <jinhuieric.huang@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_hdp.h | 1 +
>   drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c  | 3 ++-
>   drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c   | 1 +
>   drivers/gpu/drm/amd/amdgpu/hdp_v4_0.c   | 3 +++
>   4 files changed, 7 insertions(+), 1 deletion(-)
>
> 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..f31eae2931f3 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
> @@ -226,7 +226,8 @@ int amdgpu_ib_schedule(struct amdgpu_ring *ring, unsigned num_ibs,
>   	if (!(adev->flags & AMD_IS_APU))
>   #endif
>   	{
> -		if (ring->funcs->emit_hdp_flush)
> +		if (ring->funcs->emit_hdp_flush &&
> +			!adev->hdp.no_flush)

This still emits the flush through MMIO.

What you need to do is to initialize the hdp.no_flush field for all 
asics and architectures and then use that here in the if above this one.

>   			amdgpu_ring_emit_hdp_flush(ring);
>   		else
>   			amdgpu_asic_flush_hdp(adev, ring);
> diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> index 2749621d5f63..6e1eab615914 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> @@ -1223,6 +1223,7 @@ static int gmc_v9_0_early_init(void *handle)
>   		adev->gmc.xgmi.supported = true;
>   		adev->gmc.xgmi.connected_to_cpu =
>   			adev->smuio.funcs->is_host_gpu_xgmi_supported(adev);
> +		adev->hdp.no_flush = adev->gmc.xgmi.connected_to_cpu;
>   	}
>   
>   	gmc_v9_0_set_gmc_funcs(adev);
> diff --git a/drivers/gpu/drm/amd/amdgpu/hdp_v4_0.c b/drivers/gpu/drm/amd/amdgpu/hdp_v4_0.c
> index 74b90cc2bf48..e1b2face8656 100644
> --- a/drivers/gpu/drm/amd/amdgpu/hdp_v4_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/hdp_v4_0.c
> @@ -40,6 +40,9 @@
>   static void hdp_v4_0_flush_hdp(struct amdgpu_device *adev,
>   				struct amdgpu_ring *ring)
>   {
> +	if (adev->hdp.no_flush)
> +		return;
> +

Just to be clear once more, this approach is a NAK.

Checks like this should not be in the hardware specific function.

Regards,
Christian.

>   	if (!ring || !ring->funcs->emit_wreg)
>   		WREG32_NO_KIQ((adev->rmmio_remap.reg_offset + KFD_MMIO_REMAP_HDP_MEM_FLUSH_CNTL) >> 2, 0);
>   	else

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2 2/2] drm/amdgpu: Don't flush HDP on A+A
  2021-06-01  7:05   ` Christian König
@ 2021-06-01 14:45     ` Eric Huang
  2021-06-01 17:38       ` Christian König
  0 siblings, 1 reply; 6+ messages in thread
From: Eric Huang @ 2021-06-01 14:45 UTC (permalink / raw)
  To: Christian König, amd-gfx


On 2021-06-01 3:05 a.m., Christian König wrote:
> Am 01.06.21 um 02:06 schrieb Eric Huang:
>> With XGMI connection flushing HDP on PCIe is unnecessary,
>> it is also to optimize memory allocation latency.
>>
>> Signed-off-by: Eric Huang <jinhuieric.huang@amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_hdp.h | 1 +
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c  | 3 ++-
>>   drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c   | 1 +
>>   drivers/gpu/drm/amd/amdgpu/hdp_v4_0.c   | 3 +++
>>   4 files changed, 7 insertions(+), 1 deletion(-)
>>
>> 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..f31eae2931f3 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
>> @@ -226,7 +226,8 @@ int amdgpu_ib_schedule(struct amdgpu_ring *ring, 
>> unsigned num_ibs,
>>       if (!(adev->flags & AMD_IS_APU))
>>   #endif
>>       {
>> -        if (ring->funcs->emit_hdp_flush)
>> +        if (ring->funcs->emit_hdp_flush &&
>> +            !adev->hdp.no_flush)
>
> This still emits the flush through MMIO.

As matter of fact, it doesn't, because amdgpu_asic_flush_hdp will check 
the flag again in hdp_v4_0.c. I even think the check here is unnecessary 
for previous asic, because asic other than Aldeberan A+A has to flush 
tlbs according to hardware specific.
>
> What you need to do is to initialize the hdp.no_flush field for all 
> asics and architectures and then use that here in the if above this one.

I don't understand why it should be for all asics. Currently only 
Aldeberan A+A need no TLB flush, we don't have to consider other asics. 
And hdp.no_flush is a common flag for all asics, Is there an issue in 
other asics for this flag on tlb flush?
>
>> amdgpu_ring_emit_hdp_flush(ring);
>>           else
>>               amdgpu_asic_flush_hdp(adev, ring);
>> diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c 
>> b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
>> index 2749621d5f63..6e1eab615914 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
>> @@ -1223,6 +1223,7 @@ static int gmc_v9_0_early_init(void *handle)
>>           adev->gmc.xgmi.supported = true;
>>           adev->gmc.xgmi.connected_to_cpu =
>> adev->smuio.funcs->is_host_gpu_xgmi_supported(adev);
>> +        adev->hdp.no_flush = adev->gmc.xgmi.connected_to_cpu;
>>       }
>>         gmc_v9_0_set_gmc_funcs(adev);
>> diff --git a/drivers/gpu/drm/amd/amdgpu/hdp_v4_0.c 
>> b/drivers/gpu/drm/amd/amdgpu/hdp_v4_0.c
>> index 74b90cc2bf48..e1b2face8656 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/hdp_v4_0.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/hdp_v4_0.c
>> @@ -40,6 +40,9 @@
>>   static void hdp_v4_0_flush_hdp(struct amdgpu_device *adev,
>>                   struct amdgpu_ring *ring)
>>   {
>> +    if (adev->hdp.no_flush)
>> +        return;
>> +
>
> Just to be clear once more, this approach is a NAK.
>
> Checks like this should not be in the hardware specific function.

As I mention above, TLB flush should be specific for Asic for my opinion.

Regards,
Eric
>
> Regards,
> Christian.
>
>>       if (!ring || !ring->funcs->emit_wreg)
>>           WREG32_NO_KIQ((adev->rmmio_remap.reg_offset + 
>> KFD_MMIO_REMAP_HDP_MEM_FLUSH_CNTL) >> 2, 0);
>>       else
>

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2 2/2] drm/amdgpu: Don't flush HDP on A+A
  2021-06-01 14:45     ` Eric Huang
@ 2021-06-01 17:38       ` Christian König
  0 siblings, 0 replies; 6+ messages in thread
From: Christian König @ 2021-06-01 17:38 UTC (permalink / raw)
  To: Eric Huang, amd-gfx

Am 01.06.21 um 16:45 schrieb Eric Huang:
>
> On 2021-06-01 3:05 a.m., Christian König wrote:
>> Am 01.06.21 um 02:06 schrieb Eric Huang:
>>> With XGMI connection flushing HDP on PCIe is unnecessary,
>>> it is also to optimize memory allocation latency.
>>>
>>> Signed-off-by: Eric Huang <jinhuieric.huang@amd.com>
>>> ---
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_hdp.h | 1 +
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c  | 3 ++-
>>>   drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c   | 1 +
>>>   drivers/gpu/drm/amd/amdgpu/hdp_v4_0.c   | 3 +++
>>>   4 files changed, 7 insertions(+), 1 deletion(-)
>>>
>>> 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..f31eae2931f3 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
>>> @@ -226,7 +226,8 @@ int amdgpu_ib_schedule(struct amdgpu_ring *ring, 
>>> unsigned num_ibs,
>>>       if (!(adev->flags & AMD_IS_APU))
>>>   #endif
>>>       {
>>> -        if (ring->funcs->emit_hdp_flush)
>>> +        if (ring->funcs->emit_hdp_flush &&
>>> +            !adev->hdp.no_flush)
>>
>> This still emits the flush through MMIO.
>
> As matter of fact, it doesn't, because amdgpu_asic_flush_hdp will 
> check the flag again in hdp_v4_0.c. I even think the check here is 
> unnecessary for previous asic, because asic other than Aldeberan A+A 
> has to flush tlbs according to hardware specific.

And exactly that is what I'm trying to explain here.

Skipping HDP flushing is something we do for APUs for over 10 years now, 
that is pretty common on other ASICs and not Aldeberan specific at all.

Aldeberan is just the first dGPU we skip this flushing because it has an 
XGMI connection to the CPU.

>>
>> What you need to do is to initialize the hdp.no_flush field for all 
>> asics and architectures and then use that here in the if above this one.
>
> I don't understand why it should be for all asics. Currently only 
> Aldeberan A+A need no TLB flush, we don't have to consider other 
> asics. And hdp.no_flush is a common flag for all asics, Is there an 
> issue in other asics for this flag on tlb flush?

See the code in the IB handling as well. What you should do is to 
generalize this code and not re-invent the wheel specific for Aldeberan.

In other words modify or extend the existing code which skips HDP 
flushing/invalidation.

Cleaning that up by putting the no_flush into the hdp structure sounds 
like a good start to me.

>>
>>> amdgpu_ring_emit_hdp_flush(ring);
>>>           else
>>>               amdgpu_asic_flush_hdp(adev, ring);
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c 
>>> b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
>>> index 2749621d5f63..6e1eab615914 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
>>> @@ -1223,6 +1223,7 @@ static int gmc_v9_0_early_init(void *handle)
>>>           adev->gmc.xgmi.supported = true;
>>>           adev->gmc.xgmi.connected_to_cpu =
>>> adev->smuio.funcs->is_host_gpu_xgmi_supported(adev);
>>> +        adev->hdp.no_flush = adev->gmc.xgmi.connected_to_cpu;
>>>       }
>>>         gmc_v9_0_set_gmc_funcs(adev);
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/hdp_v4_0.c 
>>> b/drivers/gpu/drm/amd/amdgpu/hdp_v4_0.c
>>> index 74b90cc2bf48..e1b2face8656 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/hdp_v4_0.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/hdp_v4_0.c
>>> @@ -40,6 +40,9 @@
>>>   static void hdp_v4_0_flush_hdp(struct amdgpu_device *adev,
>>>                   struct amdgpu_ring *ring)
>>>   {
>>> +    if (adev->hdp.no_flush)
>>> +        return;
>>> +
>>
>> Just to be clear once more, this approach is a NAK.
>>
>> Checks like this should not be in the hardware specific function.
>
> As I mention above, TLB flush should be specific for Asic for my opinion.

No, that is certainly wrong. This is btw not related to the TLB in any 
way, the HDP is part of the PCIe bus interface.

Regards,
Christian.

>
> Regards,
> Eric
>>
>> Regards,
>> Christian.
>>
>>>       if (!ring || !ring->funcs->emit_wreg)
>>>           WREG32_NO_KIQ((adev->rmmio_remap.reg_offset + 
>>> KFD_MMIO_REMAP_HDP_MEM_FLUSH_CNTL) >> 2, 0);
>>>       else
>>
>

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2021-06-01 17:38 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-01  0:06 [PATCH v2 1/2] drm/amdgpu: Fix a bug on flag table_freed Eric Huang
2021-06-01  0:06 ` [PATCH v2 2/2] drm/amdgpu: Don't flush HDP on A+A Eric Huang
2021-06-01  7:05   ` Christian König
2021-06-01 14:45     ` Eric Huang
2021-06-01 17:38       ` Christian König
2021-06-01  7:02 ` [PATCH v2 1/2] drm/amdgpu: Fix a bug on flag table_freed Christian König

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.