<html><head> <meta http-equiv="Content-Type" content="text/html; charset=utf-8"> </head> <body> <div class="moz-cite-prefix">Yeah, looks fine. A few nitpicks: The title seems too long. You can just say "Sienna Cichlid", as opposed to using the macro name in the title. Perhaps you can add Sienna Cichlid in the description of the commit as well. On 2021-07-20 3:57 a.m., Chengzhe Liu wrote: </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> Unfinished sentence above. Perhaps you mean to say: On Sienna Cichlid, in pass-through mode, if we unload the driver in BACO mode (RTPM), then the kernel would receive thousands of interrupts. Note the use of present (neutral) tense--it's stating a fact. <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> Spell: monitor. Use present tense to state a fact: so KVM keeps injecting ... <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> interrupts to the guest VM. So we should clear the doorbell interrupt ... <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> Seems like a long line--perhaps checkpatch.pl would complain about it. You can break it after the &&: if (amdgpu_passthrough(adev) && adev->nbio.funcs->...) <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> You could make it more readable: /* Clear the interrupt status. */ if ((reg & ...) == 0) { Regards, Luben <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> </body> </html>