[PATCH] drm/amdgpu/display: flush the HDP when setting up DMCUB firmware

[PATCH] drm/amdgpu/display: flush the HDP when setting up DMCUB firmware

[Alex Deucher]

When data is written to VRAM via the PCI BAR, the data goes
through a block called HDP which has a write queue and a
read cache.  When the driver writes to VRAM, it needs to flush
the HDP write queue to make sure all the data written has
actually hit VRAM.

When we write the DMCUB firmware to vram, we never flushed the
HDP.  In theory this could cause DMCUB errors if we try and
start the DMCUB firmware without making sure the data has hit
memory.

This doesn't fix any known issues, but is the right thing to do.

Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
---
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index a6c3e1d74124..5c1fd3a91cd5 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -1133,6 +1133,10 @@ static int dm_dmub_hw_init(struct amdgpu_device *adev)
 		break;
 	}
 
+	/* flush HDP */
+	mb();
+	amdgpu_device_flush_hdp(adev, NULL);
+
 	status = dmub_srv_hw_init(dmub_srv, &hw_params);
 	if (status != DMUB_STATUS_OK) {
 		DRM_ERROR("Error initializing DMUB HW: %d\n", status);
-- 
2.35.1

Re: [PATCH] drm/amdgpu/display: flush the HDP when setting up DMCUB firmware

[Kazlauskas, Nicholas]

[-- Attachment #1: Type: text/plain, Size: 2979 bytes --]

[Public]

This bug previously existed, and we have a solution in place for it.

The solution we picked was to force a stall through reading back the memory. You'll see this implemented in dmub_srv.c and the dmub_cmd.h header - through use of a volatile read over the region written. We do this for both the initial allocation for the cache windows and on every command submission to ensure DMCUB doesn't wakeup before the writes are in VRAM.

The issue on dGPU is the latency through the HDP path, but on APU the issue is out of order writes. We saw this problem on both DCN30/DCN21 when DMCUB was first introduced.

The writes we do happen within dmub_hw_init and on every command execution, but this patch adds the flush before HW init. I think the only issue this potentially fixes is the initial writeout in the SW PSP code to VRAM, but they already have flushes in place for that. The signature validation would cause firmware to fail to load if it wasn't at least.

So from a correctness perspective I don't think this patch causes any issue, but from a performance perspective this probably adds at least 100us to boot, if not more. My recommendation is to leave things as-is for now.

Regards,
Nicholas Kazlauskas
________________________________
From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> on behalf of Alex Deucher <alexander.deucher@amd.com>
Sent: Thursday, April 28, 2022 6:13 PM
To: amd-gfx@lists.freedesktop.org <amd-gfx@lists.freedesktop.org>
Cc: Deucher, Alexander <Alexander.Deucher@amd.com>
Subject: [PATCH] drm/amdgpu/display: flush the HDP when setting up DMCUB firmware

When data is written to VRAM via the PCI BAR, the data goes
through a block called HDP which has a write queue and a
read cache.  When the driver writes to VRAM, it needs to flush
the HDP write queue to make sure all the data written has
actually hit VRAM.

When we write the DMCUB firmware to vram, we never flushed the
HDP.  In theory this could cause DMCUB errors if we try and
start the DMCUB firmware without making sure the data has hit
memory.

This doesn't fix any known issues, but is the right thing to do.

Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
---
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index a6c3e1d74124..5c1fd3a91cd5 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -1133,6 +1133,10 @@ static int dm_dmub_hw_init(struct amdgpu_device *adev)
                 break;
         }

+       /* flush HDP */
+       mb();
+       amdgpu_device_flush_hdp(adev, NULL);
+
         status = dmub_srv_hw_init(dmub_srv, &hw_params);
         if (status != DMUB_STATUS_OK) {
                 DRM_ERROR("Error initializing DMUB HW: %d\n", status);
--
2.35.1


[-- Attachment #2: Type: text/html, Size: 6191 bytes --]

RE: [PATCH] drm/amdgpu/display: flush the HDP when setting up DMCUB firmware

[Zhang, Hawking]

[AMD Official Use Only - General]

Reviewed-by: Hawking Zhang <Hawking.Zhang@amd.com>

Regards,
Hawking
-----Original Message-----
From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of Alex Deucher
Sent: Friday, April 29, 2022 06:13
To: amd-gfx@lists.freedesktop.org
Cc: Deucher, Alexander <Alexander.Deucher@amd.com>
Subject: [PATCH] drm/amdgpu/display: flush the HDP when setting up DMCUB firmware

When data is written to VRAM via the PCI BAR, the data goes through a block called HDP which has a write queue and a read cache.  When the driver writes to VRAM, it needs to flush the HDP write queue to make sure all the data written has actually hit VRAM.

When we write the DMCUB firmware to vram, we never flushed the HDP.  In theory this could cause DMCUB errors if we try and start the DMCUB firmware without making sure the data has hit memory.

This doesn't fix any known issues, but is the right thing to do.

Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
---
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index a6c3e1d74124..5c1fd3a91cd5 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -1133,6 +1133,10 @@ static int dm_dmub_hw_init(struct amdgpu_device *adev)
                break;
        }

+       /* flush HDP */
+       mb();
+       amdgpu_device_flush_hdp(adev, NULL);
+
        status = dmub_srv_hw_init(dmub_srv, &hw_params);
        if (status != DMUB_STATUS_OK) {
                DRM_ERROR("Error initializing DMUB HW: %d\n", status);
--
2.35.1

Re: [PATCH] drm/amdgpu/display: flush the HDP when setting up DMCUB firmware

[Alex Deucher]

On Thu, Apr 28, 2022 at 9:41 PM Kazlauskas, Nicholas
<Nicholas.Kazlauskas@amd.com> wrote:
>
> [Public]
>
>
> This bug previously existed, and we have a solution in place for it.
>
> The solution we picked was to force a stall through reading back the memory. You'll see this implemented in dmub_srv.c and the dmub_cmd.h header - through use of a volatile read over the region written. We do this for both the initial allocation for the cache windows and on every command submission to ensure DMCUB doesn't wakeup before the writes are in VRAM.
>
> The issue on dGPU is the latency through the HDP path, but on APU the issue is out of order writes. We saw this problem on both DCN30/DCN21 when DMCUB was first introduced.
>
> The writes we do happen within dmub_hw_init and on every command execution, but this patch adds the flush before HW init. I think the only issue this potentially fixes is the initial writeout in the SW PSP code to VRAM, but they already have flushes in place for that. The signature validation would cause firmware to fail to load if it wasn't at least.
>
> So from a correctness perspective I don't think this patch causes any issue, but from a performance perspective this probably adds at least 100us to boot, if not more. My recommendation is to leave things as-is for now.

Thanks for the background.  I'll drop that patch.

Alex


>
> Regards,
> Nicholas Kazlauskas
> ________________________________
> From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> on behalf of Alex Deucher <alexander.deucher@amd.com>
> Sent: Thursday, April 28, 2022 6:13 PM
> To: amd-gfx@lists.freedesktop.org <amd-gfx@lists.freedesktop.org>
> Cc: Deucher, Alexander <Alexander.Deucher@amd.com>
> Subject: [PATCH] drm/amdgpu/display: flush the HDP when setting up DMCUB firmware
>
> When data is written to VRAM via the PCI BAR, the data goes
> through a block called HDP which has a write queue and a
> read cache.  When the driver writes to VRAM, it needs to flush
> the HDP write queue to make sure all the data written has
> actually hit VRAM.
>
> When we write the DMCUB firmware to vram, we never flushed the
> HDP.  In theory this could cause DMCUB errors if we try and
> start the DMCUB firmware without making sure the data has hit
> memory.
>
> This doesn't fix any known issues, but is the right thing to do.
>
> Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
> ---
>  drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index a6c3e1d74124..5c1fd3a91cd5 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -1133,6 +1133,10 @@ static int dm_dmub_hw_init(struct amdgpu_device *adev)
>                  break;
>          }
>
> +       /* flush HDP */
> +       mb();
> +       amdgpu_device_flush_hdp(adev, NULL);
> +
>          status = dmub_srv_hw_init(dmub_srv, &hw_params);
>          if (status != DMUB_STATUS_OK) {
>                  DRM_ERROR("Error initializing DMUB HW: %d\n", status);
> --
> 2.35.1
>