Re: [PATCH 1/2] drm/amdgpu: make duplicated EOP packet for GFX7/8 have real content

[Christian König <christian.koenig@xxxxxxx>]

Am 17.06.24 um 12:58 schrieb Icenowy Zheng:
> The duplication of EOP packets for GFX7/8, with the former one have
> seq-1 written and the latter one have seq written, seems to confuse some
> hardware platform (e.g. Loongson 7A series PCIe controllers).
>
> Make the content of the duplicated EOP packet the same with the real
> one, only masking any possible interrupts.

Well completely NAK to that, exactly that disables the workaround.

The CPU needs to see two different values written here.

Regards,
Christian.

> Fixes: bf26da927a1c ("drm/amdgpu: add cache flush workaround to gfx8 emit_fence")
> Fixes: a2e73f56fa62 ("drm/amdgpu: Add support for CIK parts")
> Signed-off-by: Icenowy Zheng <uwu@xxxxxxxxxx>
> ---
>   drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c | 12 +++++-------
>   drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c | 12 ++++--------
>   2 files changed, 9 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c b/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c
> index 541dbd70d8c75..778f27f1a34fe 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c
> @@ -2117,9 +2117,8 @@ static void gfx_v7_0_ring_emit_fence_gfx(struct amdgpu_ring *ring, u64 addr,
>   {
>   	bool write64bit = flags & AMDGPU_FENCE_FLAG_64BIT;
>   	bool int_sel = flags & AMDGPU_FENCE_FLAG_INT;
> -	/* Workaround for cache flush problems. First send a dummy EOP
> -	 * event down the pipe with seq one below.
> -	 */
> +
> +	/* Workaround for cache flush problems, send EOP twice. */
>   	amdgpu_ring_write(ring, PACKET3(PACKET3_EVENT_WRITE_EOP, 4));
>   	amdgpu_ring_write(ring, (EOP_TCL1_ACTION_EN |
>   				 EOP_TC_ACTION_EN |
> @@ -2127,11 +2126,10 @@ static void gfx_v7_0_ring_emit_fence_gfx(struct amdgpu_ring *ring, u64 addr,
>   				 EVENT_INDEX(5)));
>   	amdgpu_ring_write(ring, addr & 0xfffffffc);
>   	amdgpu_ring_write(ring, (upper_32_bits(addr) & 0xffff) |
> -				DATA_SEL(1) | INT_SEL(0));
> -	amdgpu_ring_write(ring, lower_32_bits(seq - 1));
> -	amdgpu_ring_write(ring, upper_32_bits(seq - 1));
> +				DATA_SEL(write64bit ? 2 : 1) | INT_SEL(0));
> +	amdgpu_ring_write(ring, lower_32_bits(seq));
> +	amdgpu_ring_write(ring, upper_32_bits(seq));
>   
> -	/* Then send the real EOP event down the pipe. */
>   	amdgpu_ring_write(ring, PACKET3(PACKET3_EVENT_WRITE_EOP, 4));
>   	amdgpu_ring_write(ring, (EOP_TCL1_ACTION_EN |
>   				 EOP_TC_ACTION_EN |
> diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c b/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
> index 2f0e72caee1af..39a7d60f1fd69 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
> @@ -6153,9 +6153,7 @@ static void gfx_v8_0_ring_emit_fence_gfx(struct amdgpu_ring *ring, u64 addr,
>   	bool write64bit = flags & AMDGPU_FENCE_FLAG_64BIT;
>   	bool int_sel = flags & AMDGPU_FENCE_FLAG_INT;
>   
> -	/* Workaround for cache flush problems. First send a dummy EOP
> -	 * event down the pipe with seq one below.
> -	 */
> +	/* Workaround for cache flush problems, send EOP twice. */
>   	amdgpu_ring_write(ring, PACKET3(PACKET3_EVENT_WRITE_EOP, 4));
>   	amdgpu_ring_write(ring, (EOP_TCL1_ACTION_EN |
>   				 EOP_TC_ACTION_EN |
> @@ -6164,12 +6162,10 @@ static void gfx_v8_0_ring_emit_fence_gfx(struct amdgpu_ring *ring, u64 addr,
>   				 EVENT_INDEX(5)));
>   	amdgpu_ring_write(ring, addr & 0xfffffffc);
>   	amdgpu_ring_write(ring, (upper_32_bits(addr) & 0xffff) |
> -				DATA_SEL(1) | INT_SEL(0));
> -	amdgpu_ring_write(ring, lower_32_bits(seq - 1));
> -	amdgpu_ring_write(ring, upper_32_bits(seq - 1));
> +			  DATA_SEL(write64bit ? 2 : 1) | INT_SEL(0));
> +	amdgpu_ring_write(ring, lower_32_bits(seq));
> +	amdgpu_ring_write(ring, upper_32_bits(seq));
>   
> -	/* Then send the real EOP event down the pipe:
> -	 * EVENT_WRITE_EOP - flush caches, send int */
>   	amdgpu_ring_write(ring, PACKET3(PACKET3_EVENT_WRITE_EOP, 4));
>   	amdgpu_ring_write(ring, (EOP_TCL1_ACTION_EN |
>   				 EOP_TC_ACTION_EN |