<html><head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
  </head>
  <body>
    <div class="moz-cite-prefix">Yeah, looks fine.<br>
      <br>
      A few nitpicks:<br>
      <br>
      The title seems too long. You can just say "Sienna Cichlid", as
      opposed to using the macro name in the title.<br>
      Perhaps you can add Sienna Cichlid in the description of the
      commit as well.<br>
      <br>
      On 2021-07-20 3:57 a.m., Chengzhe Liu wrote:<br>
    </div>
    <blockquote type="cite" cite="mid:20210720075737.1583824-1-ChengZhe.Liu@amd.com">
      <pre class="moz-quote-pre" wrap="">In passthrough mode, if we unloaded driver in BACO mode(RTPM). Kernel would</pre>
    </blockquote>
    <br>
    Unfinished sentence above. Perhaps you mean to say:<br>
    <br>
    <i>On Sienna Cichlid, in pass-through mode, if we unload the driver<br>
      in BACO mode (RTPM),</i><i> </i><i>then the kernel would receive
      thousands<br>
      of interrupts.</i><br>
    <br>
    Note the use of present (neutral) tense--it's stating a fact.<br>
    <blockquote type="cite" cite="mid:20210720075737.1583824-1-ChengZhe.Liu@amd.com">
      <pre class="moz-quote-pre" wrap="">
receive thousands of unhandled interrupts. That's because there was
doorbell moniter interrupt on BIF, so KVM would keep injecting</pre>
    </blockquote>
    <br>
    Spell: monit<b>o</b>r.<br>
    Use present tense to state a fact: <i>so KVM keeps injecting ...<br>
      <br>
    </i>
    <blockquote type="cite" cite="mid:20210720075737.1583824-1-ChengZhe.Liu@amd.com">
      <pre class="moz-quote-pre" wrap="">
interrupt to guest VM. So we should clear door bell interrupt status</pre>
    </blockquote>
    <br>
    <i>interrupt<b>s</b> to the guest VM. So we should clear the
      doorbell interrupt ...</i><br>
    <br>
    <blockquote type="cite" cite="mid:20210720075737.1583824-1-ChengZhe.Liu@amd.com">
      <pre class="moz-quote-pre" wrap="">
after BACO exit.

Signed-off-by: Chengzhe Liu <a class="moz-txt-link-rfc2396E" href="mailto:ChengZhe.Liu@amd.com"><ChengZhe.Liu@amd.com></a>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c |  3 +++
 drivers/gpu/drm/amd/amdgpu/amdgpu_nbio.h   |  1 +
 drivers/gpu/drm/amd/amdgpu/nbio_v2_3.c     | 20 ++++++++++++++++++++
 3 files changed, 24 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index 37fa199be8b3..109a76ca4535 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -5265,6 +5265,9 @@ int amdgpu_device_baco_exit(struct drm_device *dev)
            adev->nbio.funcs->enable_doorbell_interrupt)
                adev->nbio.funcs->enable_doorbell_interrupt(adev, true);
 
+       if (amdgpu_passthrough(adev) && adev->nbio.funcs->clear_doorbell_interrupt)
+               adev->nbio.funcs->clear_doorbell_interrupt(adev);
+
        return 0;</pre>
    </blockquote>
    <br>
    Seems like a long line--perhaps checkpatch.pl would complain about
    it.<br>
    You can break it after the &&:<br>
    <br>
    <font face="monospace">if (amdgpu_passthrough(adev) &&<br>
          adev->nbio.funcs->...)</font><br>
    <br>
    <blockquote type="cite" cite="mid:20210720075737.1583824-1-ChengZhe.Liu@amd.com">
      <pre class="moz-quote-pre" wrap="">
 }
 
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_nbio.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_nbio.h
index 45295dce5c3e..843052205bd5 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_nbio.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_nbio.h
@@ -95,6 +95,7 @@ struct amdgpu_nbio_funcs {
        void (*program_aspm)(struct amdgpu_device *adev);
        void (*apply_lc_spc_mode_wa)(struct amdgpu_device *adev);
        void (*apply_l1_link_width_reconfig_wa)(struct amdgpu_device *adev);
+       void (*clear_doorbell_interrupt)(struct amdgpu_device *adev);
 };
 
 struct amdgpu_nbio {
diff --git a/drivers/gpu/drm/amd/amdgpu/nbio_v2_3.c b/drivers/gpu/drm/amd/amdgpu/nbio_v2_3.c
index 7b79eeaa88aa..044dc63d2e86 100644
--- a/drivers/gpu/drm/amd/amdgpu/nbio_v2_3.c
+++ b/drivers/gpu/drm/amd/amdgpu/nbio_v2_3.c
@@ -508,6 +508,25 @@ static void nbio_v2_3_apply_l1_link_width_reconfig_wa(struct amdgpu_device *adev
        WREG32_PCIE(smnPCIE_LC_LINK_WIDTH_CNTL, reg_data);
 }
 
+static void nbio_v2_3_clear_doorbell_interrupt(struct amdgpu_device *adev)
+{
+       uint32_t reg, reg_data;
+
+       if (adev->asic_type != CHIP_SIENNA_CICHLID)
+               return;
+
+       reg = RREG32_SOC15(NBIO, 0, mmBIF_RB_CNTL);
+
+       /*Clear Interrupt Status*/</pre>
    </blockquote>
    <br>
    You could make it more readable:<br>
    <br>
    <font face="monospace">/* Clear the interrupt status.<br>
       */<br>
      if ((reg & ...) == 0) {</font><br>
    <br>
    Regards,<br>
    Luben<br>
    <br>
    <blockquote type="cite" cite="mid:20210720075737.1583824-1-ChengZhe.Liu@amd.com">
      <pre class="moz-quote-pre" wrap="">+    if ((reg & BIF_RB_CNTL__RB_ENABLE_MASK) == 0) {
+               reg = RREG32_SOC15(NBIO, 0, mmBIF_DOORBELL_INT_CNTL);
+               if (reg & BIF_DOORBELL_INT_CNTL__DOORBELL_INTERRUPT_STATUS_MASK) {
+                               reg_data = 1 << BIF_DOORBELL_INT_CNTL__DOORBELL_INTERRUPT_CLEAR__SHIFT;
+                               WREG32_SOC15(NBIO, 0, mmBIF_DOORBELL_INT_CNTL, reg_data);
+               }
+       }
+}
+
 const struct amdgpu_nbio_funcs nbio_v2_3_funcs = {
        .get_hdp_flush_req_offset = nbio_v2_3_get_hdp_flush_req_offset,
        .get_hdp_flush_done_offset = nbio_v2_3_get_hdp_flush_done_offset,
@@ -531,4 +550,5 @@ const struct amdgpu_nbio_funcs nbio_v2_3_funcs = {
        .program_aspm =  nbio_v2_3_program_aspm,
        .apply_lc_spc_mode_wa = nbio_v2_3_apply_lc_spc_mode_wa,
        .apply_l1_link_width_reconfig_wa = nbio_v2_3_apply_l1_link_width_reconfig_wa,
+       .clear_doorbell_interrupt = nbio_v2_3_clear_doorbell_interrupt,
 };
</pre>
    </blockquote>
    <br>
  </body>
</html>