On Wed, Dec 13, 2023 at 2:32 PM Mario Limonciello
<mario.limoncie...@amd.com> wrote:
>
> On 12/13/2023 13:12, Mario Limonciello wrote:
> > On 12/13/2023 13:07, Alex Deucher wrote:
> >> On Wed, Dec 13, 2023 at 1:00 PM Mario Limonciello
> >> <mario.limoncie...@amd.com> wrote:
> >>>
> >>> Some systems with MP1 13.0.4 or 13.0.11 have a firmware bug that
> >>> causes the first MES packet after resume to fail. This packet is
> >>> used to flush the TLB when GART is enabled.
> >>>
> >>> This issue is fixed in newer firmware, but as OEMs may not roll this
> >>> out to the field, introduce a workaround that will retry the flush
> >>> when detecting running on an older firmware and decrease relevant
> >>> error messages to debug while workaround is in use.
> >>>
> >>> Cc: sta...@vger.kernel.org # 6.1+
> >>> Cc: Tim Huang <tim.hu...@amd.com>
> >>> Closes: https://gitlab.freedesktop.org/drm/amd/-/issues/3045
> >>> Signed-off-by: Mario Limonciello <mario.limoncie...@amd.com>
> >>> ---
> >>> drivers/gpu/drm/amd/amdgpu/amdgpu_mes.c | 10 ++++++++--
> >>> drivers/gpu/drm/amd/amdgpu/amdgpu_mes.h | 2 ++
> >>> drivers/gpu/drm/amd/amdgpu/gmc_v11_0.c | 17 ++++++++++++++++-
> >>> drivers/gpu/drm/amd/amdgpu/mes_v11_0.c | 8 ++++++--
> >>> 4 files changed, 32 insertions(+), 5 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_mes.c
> >>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_mes.c
> >>> index 9ddbf1494326..6ce3f6e6b6de 100644
> >>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_mes.c
> >>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_mes.c
> >>> @@ -836,8 +836,14 @@ int amdgpu_mes_reg_write_reg_wait(struct
> >>> amdgpu_device *adev,
> >>> }
> >>>
> >>> r = adev->mes.funcs->misc_op(&adev->mes, &op_input);
> >>> - if (r)
> >>> - DRM_ERROR("failed to reg_write_reg_wait\n");
> >>> + if (r) {
> >>> + const char *msg = "failed to reg_write_reg_wait\n";
> >>> +
> >>> + if (adev->mes.suspend_workaround)
> >>> + DRM_DEBUG(msg);
> >>> + else
> >>> + DRM_ERROR(msg);
> >>> + }
> >>>
> >>> error:
> >>> return r;
> >>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_mes.h
> >>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_mes.h
> >>> index a27b424ffe00..90f2bba3b12b 100644
> >>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_mes.h
> >>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_mes.h
> >>> @@ -135,6 +135,8 @@ struct amdgpu_mes {
> >>>
> >>> /* ip specific functions */
> >>> const struct amdgpu_mes_funcs *funcs;
> >>> +
> >>> + bool suspend_workaround;
> >>> };
> >>>
> >>> struct amdgpu_mes_process {
> >>> diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v11_0.c
> >>> b/drivers/gpu/drm/amd/amdgpu/gmc_v11_0.c
> >>> index 23d7b548d13f..e810c7bb3156 100644
> >>> --- a/drivers/gpu/drm/amd/amdgpu/gmc_v11_0.c
> >>> +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v11_0.c
> >>> @@ -889,7 +889,11 @@ static int gmc_v11_0_gart_enable(struct
> >>> amdgpu_device *adev)
> >>> false : true;
> >>>
> >>> adev->mmhub.funcs->set_fault_enable_default(adev, value);
> >>> - gmc_v11_0_flush_gpu_tlb(adev, 0, AMDGPU_MMHUB0(0), 0);
> >>> +
> >>> + do {
> >>> + gmc_v11_0_flush_gpu_tlb(adev, 0, AMDGPU_MMHUB0(0), 0);
> >>> + adev->mes.suspend_workaround = false;
> >>> + } while (adev->mes.suspend_workaround);
> >>
> >> Shouldn't this be something like:
> >>
> >>> + do {
> >>> + gmc_v11_0_flush_gpu_tlb(adev, 0, AMDGPU_MMHUB0(0), 0);
> >>> + adev->mes.suspend_workaround = false;
> >>> + gmc_v11_0_flush_gpu_tlb(adev, 0, AMDGPU_MMHUB0(0), 0);
> >>> + } while (adev->mes.suspend_workaround);
> >>
> >> If we actually need the flush. Maybe a better approach would be to
> >> check if we are in s0ix in
> >
> > Ah you're right; I had shifted this around to keep less stateful
> > variables and push them up the stack from when I first made it and that
> > logic is wrong now.
> >
> > I don't think the one you suggested is right either; it's going to apply
> > twice on ASICs that only need it once.
> >
> > I guess pending on what Christian comments on below I'll respin to logic
> > that only calls twice on resume for these ASICs.
>
> One more comment. Tim and I both did an experiment for this (skipping
> the flush) separately. The problem isn't the flush itself, rather it's
> the first MES packet after exiting GFXOFF.
>
> So it seems that it pushes off the issue to the next thing which is a
> ring buffer test:
>
> [drm:amdgpu_ib_ring_tests [amdgpu]] *ERROR* IB test failed on comp_1.0.0
> (-110).
> [drm:process_one_work] *ERROR* ib ring test failed (-110).
>
> So maybe a better workaround is a "dummy" command that is only sent for
> the broken firmware that we don't care about the outcome and discard errors.
>
> Then the workaround doesn't need to get as entangled with correct flow.
Yeah. Something like that seems cleaner. Just a question of where to
put it since we skip GC and MES for s0ix. Probably somewhere in
gmc_v11_0_resume() before gmc_v11_0_gart_enable(). Maybe add a new
mes callback.
Alex
>
> >
> >>
> >> diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v11_0.c
> >> b/drivers/gpu/drm/amd/amdgpu/gmc_v11_0.c in gmc_v11_0_flush_gpu_tlb():
> >> index 23d7b548d13f..bd6d9953a80e 100644
> >> --- a/drivers/gpu/drm/amd/amdgpu/gmc_v11_0.c
> >> +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v11_0.c
> >> @@ -227,7 +227,8 @@ static void gmc_v11_0_flush_gpu_tlb(struct
> >> amdgpu_device *adev, uint32_t vmid,
> >> * Directly use kiq to do the vm invalidation instead
> >> */
> >> if ((adev->gfx.kiq[0].ring.sched.ready ||
> >> adev->mes.ring.sched.ready) &&
> >> - (amdgpu_sriov_runtime(adev) || !amdgpu_sriov_vf(adev))) {
> >> + (amdgpu_sriov_runtime(adev) || !amdgpu_sriov_vf(adev)) ||
> >> + !adev->in_s0ix) {
> >> amdgpu_virt_kiq_reg_write_reg_wait(adev, req, ack,
> >> inv_req,
> >> 1 << vmid, GET_INST(GC, 0));
> >> return;
> >>
> >> @Christian Koenig is this logic correct?
> >>
> >> /* For SRIOV run time, driver shouldn't access the register
> >> through MMIO
> >> * Directly use kiq to do the vm invalidation instead
> >> */
> >> if ((adev->gfx.kiq[0].ring.sched.ready ||
> >> adev->mes.ring.sched.ready) &&
> >> (amdgpu_sriov_runtime(adev) || !amdgpu_sriov_vf(adev))) {
> >> amdgpu_virt_kiq_reg_write_reg_wait(adev, req, ack,
> >> inv_req,
> >> 1 << vmid, GET_INST(GC, 0));
> >> return;
> >> }
> >>
> >> We basically always use the MES with that logic. If that is the case,
> >> we should just drop the rest of that function. Shouldn't we only use
> >> KIQ or MES for SR-IOV? gmc v10 has similar logic which also seems
> >> wrong.
> >>
> >> Alex
> >>
> >>
> >>>
> >>> DRM_INFO("PCIE GART of %uM enabled (table at 0x%016llX).\n",
> >>> (unsigned int)(adev->gmc.gart_size >> 20),
> >>> @@ -960,6 +964,17 @@ static int gmc_v11_0_resume(void *handle)
> >>> int r;
> >>> struct amdgpu_device *adev = (struct amdgpu_device *)handle;
> >>>
> >>> + switch (amdgpu_ip_version(adev, MP1_HWIP, 0)) {
> >>> + case IP_VERSION(13, 0, 4):
> >>> + case IP_VERSION(13, 0, 11):
> >>> + /* avoid problems with first TLB flush after resume */
> >>> + if ((adev->pm.fw_version & 0x00FFFFFF) < 0x004c4900)
> >>> + adev->mes.suspend_workaround = adev->in_s0ix;
> >>> + break;
> >>> + default:
> >>> + break;
> >>> + }
> >>> +
> >>> r = gmc_v11_0_hw_init(adev);
> >>> if (r)
> >>> return r;
> >>> diff --git a/drivers/gpu/drm/amd/amdgpu/mes_v11_0.c
> >>> b/drivers/gpu/drm/amd/amdgpu/mes_v11_0.c
> >>> index 4dfec56e1b7f..84ab8c611e5e 100644
> >>> --- a/drivers/gpu/drm/amd/amdgpu/mes_v11_0.c
> >>> +++ b/drivers/gpu/drm/amd/amdgpu/mes_v11_0.c
> >>> @@ -137,8 +137,12 @@ static int
> >>> mes_v11_0_submit_pkt_and_poll_completion(struct amdgpu_mes *mes,
> >>> r = amdgpu_fence_wait_polling(ring, ring->fence_drv.sync_seq,
> >>> timeout);
> >>> if (r < 1) {
> >>> - DRM_ERROR("MES failed to response msg=%d\n",
> >>> - x_pkt->header.opcode);
> >>> + if (mes->suspend_workaround)
> >>> + DRM_DEBUG("MES failed to response msg=%d\n",
> >>> + x_pkt->header.opcode);
> >>> + else
> >>> + DRM_ERROR("MES failed to response msg=%d\n",
> >>> + x_pkt->header.opcode);
> >>>
> >>> while (halt_if_hws_hang)
> >>> schedule();
> >>> --
> >>> 2.34.1
> >>>
> >
>
