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
> >>>
> >
>

Reply via email to