<!DOCTYPE html><html><head> <meta http-equiv="Content-Type" content="text/html; charset=utf-8"> </head> <body> <div class="moz-cite-prefix">On 2024-03-26 12:04, Alam, Dewan wrote: </div> <blockquote type="cite" cite="mid:SA3PR12MB790204F7976709B88A9302BF8B352@SA3PR12MB7902.namprd12.prod.outlook.com"> <pre class="moz-quote-pre" wrap="">[AMD Official Use Only - General] Looping in +@Zhang, Zhaochen CAM control register can only be written by PF. VF can only read the register. In SRIOV VF, the write won't work. In SRIOV case, CAM's enablement is controlled by the host. Hence, we think the enablement status should be decided by the register reading.</pre> </blockquote> Thank you for clarifying that. With that in mind, I would suggest changes to the commit headline and description to avoid confusion: <pre>drm/amdgpu: Confirm IH retry CAM enablement by reading the register Under SRIOV, the IH CAM cannot be enabled by the guest. The host controls this register. In the guest driver, read the register to confirm whether the CAM was enabled. </pre> Regards, Felix <blockquote type="cite" cite="mid:SA3PR12MB790204F7976709B88A9302BF8B352@SA3PR12MB7902.namprd12.prod.outlook.com"> <pre class="moz-quote-pre" wrap=""> Thanks, Dewan -----Original Message----- From: Kuehling, Felix <a class="moz-txt-link-rfc2396E" href="mailto:Felix.Kuehling@amd.com"><Felix.Kuehling@amd.com></a> Sent: Wednesday, March 13, 2024 3:46 PM To: Alam, Dewan <a class="moz-txt-link-rfc2396E" href="mailto:Dewan.Alam@amd.com"><Dewan.Alam@amd.com></a>; <a class="moz-txt-link-abbreviated" href="mailto:amd-gfx@lists.freedesktop.org">amd-gfx@lists.freedesktop.org</a> Cc: Zhang, Hawking <a class="moz-txt-link-rfc2396E" href="mailto:Hawking.Zhang@amd.com"><Hawking.Zhang@amd.com></a> Subject: Re: [PATCH] drm/amd/amdgpu: Enable IH Retry CAM by register read On 2024-03-13 13:43, Dewan Alam wrote: </pre> <blockquote type="cite"> <pre class="moz-quote-pre" wrap="">IH Retry CAM should be enabled by register reads instead of always being set to true. </pre> </blockquote> <pre class="moz-quote-pre" wrap="">This explanation sounds odd. Your code is still writing the register first. What's the reason for reading back the register? I assume it's not needed for enabling the CAM, but to check whether it was enabled successfully. What are the configurations where it cannot be enabled successfully? Two more nit-picks inline ... </pre> <blockquote type="cite"> <pre class="moz-quote-pre" wrap=""> Signed-off-by: Dewan Alam <a class="moz-txt-link-rfc2396E" href="mailto:dewan.alam@amd.com"><dewan.alam@amd.com></a> --- drivers/gpu/drm/amd/amdgpu/vega20_ih.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/amd/amdgpu/vega20_ih.c b/drivers/gpu/drm/amd/amdgpu/vega20_ih.c index b9e785846637..c330f5a88a06 100644 --- a/drivers/gpu/drm/amd/amdgpu/vega20_ih.c +++ b/drivers/gpu/drm/amd/amdgpu/vega20_ih.c @@ -337,13 +337,20 @@ static int vega20_ih_irq_init(struct amdgpu_device *adev) /* Enable IH Retry CAM */ if (amdgpu_ip_version(adev, OSSSYS_HWIP, 0) == IP_VERSION(4, 4, 0) || - amdgpu_ip_version(adev, OSSSYS_HWIP, 0) == IP_VERSION(4, 4, 2)) + amdgpu_ip_version(adev, OSSSYS_HWIP, 0) == IP_VERSION(4, 4, 2)) +{ WREG32_FIELD15(OSSSYS, 0, IH_RETRY_INT_CAM_CNTL_ALDEBARAN, ENABLE, 1); - else + adev->irq.retry_cam_enabled = REG_GET_FIELD( + RREG32_SOC15(OSSSYS, 0, + mmIH_RETRY_INT_CAM_CNTL_ALDEBARAN), + IH_RETRY_INT_CAM_CNTL_ALDEBARAN, ENABLE); + } else { </pre> </blockquote> <pre class="moz-quote-pre" wrap=""> Indentation looks wrong here. </pre> <blockquote type="cite"> <pre class="moz-quote-pre" wrap=""> WREG32_FIELD15(OSSSYS, 0, IH_RETRY_INT_CAM_CNTL, ENABLE, 1); - - adev->irq.retry_cam_enabled = true; + adev->irq.retry_cam_enabled = REG_GET_FIELD( + RREG32_SOC15(OSSSYS, 0, + mmIH_RETRY_INT_CAM_CNTL), + IH_RETRY_INT_CAM_CNTL, ENABLE); + } </pre> </blockquote> <pre class="moz-quote-pre" wrap=""> Wrong indentation. Regards, Felix </pre> <blockquote type="cite"> <pre class="moz-quote-pre" wrap=""> /* enable interrupts */ ret = vega20_ih_toggle_interrupts(adev, true); </pre> </blockquote> </blockquote> </body> </html>