[PATCH] drm/amd/amdgpu: Enable IH Retry CAM by register read

[Alam, Dewan]

[AMD Official Use Only - General]

Looping in + at 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.

Thanks,
Dewan

-----Original Message-----
From: Kuehling, Felix <Felix.Kuehling at amd.com>
Sent: Wednesday, March 13, 2024 3:46 PM
To: Alam, Dewan <Dewan.Alam at amd.com>; amd-gfx at lists.freedesktop.org
Cc: Zhang, Hawking <Hawking.Zhang at amd.com>
Subject: Re: [PATCH] drm/amd/amdgpu: Enable IH Retry CAM by register read

On 2024-03-13 13:43, Dewan Alam wrote:
> IH Retry CAM should be enabled by register reads instead of always being set to true.
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 ...


>
> Signed-off-by: Dewan Alam <dewan.alam at amd.com>
> ---
>   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 {

Indentation looks wrong here.

>               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);
> +             }

Wrong indentation.

Regards,
   Felix

>
>       /* enable interrupts */
>       ret = vega20_ih_toggle_interrupts(adev, true);