[PATCH] drm/amdgpu: Use PSP to program IH_RB_CNTL_RING1/2 on SRIOV

[Khaire, Rohit]

[AMD Public Use]

OK.

I will just skip the function call for SRIOV and resend.

Rohit

-----Original Message-----
From: Koenig, Christian <Christian.Koenig at amd.com> 
Sent: June 7, 2021 12:42 PM
To: Kuehling, Felix <Felix.Kuehling at amd.com>; Khaire, Rohit <Rohit.Khaire at amd.com>; amd-gfx at lists.freedesktop.org; Deucher, Alexander <Alexander.Deucher at amd.com>; Zhang, Hawking <Hawking.Zhang at amd.com>; Deng, Emily <Emily.Deng at amd.com>; Liu, Monk <Monk.Liu at amd.com>; Zhou, Peng Ju <PengJu.Zhou at amd.com>; Chen, Horace <Horace.Chen at amd.com>
Cc: Ming, Davis <Davis.Ming at amd.com>
Subject: Re: [PATCH] drm/amdgpu: Use PSP to program IH_RB_CNTL_RING1/2 on SRIOV

Ah, good point. In this case we should probably rather save than sorry.

Then I suggest to clean up this patch, repeating the psp_reg_program() and error message is pretty horrible coding style.

Christian.

Am 07.06.21 um 18:36 schrieb Felix Kuehling:
> With SRIOV, the interrupt routing is setup by the hypervisor driver. 
> We need the secondary IH rings in case the hypervisor enabled 
> rerouting of page fault interrupts. I'm not sure what the hypervisor driver does today.
>
> Regards,
>    Felix
>
>
> Am 2021-06-07 um 12:29 p.m. schrieb Christian König:
>> That's a workaround for bare metal and as far as I know doesn't apply 
>> to SRIOV.
>>
>> We only need the additional IH rings for page fault handling or log 
>> handling and as far as I know that is incompatible with SRIOV for the 
>> moment. But Felix might have some more updates on this.
>>
>> So as long as we don't support that under SRIOV we don't need this 
>> patch either.
>>
>> Christian.
>>
>> Am 07.06.21 um 17:59 schrieb Khaire, Rohit:
>>> [AMD Public Use]
>>>
>>> The hash is 5ea6f9c
>>>
>>> Rohit
>>>
>>> -----Original Message-----
>>> From: Koenig, Christian <Christian.Koenig at amd.com>
>>> Sent: June 7, 2021 11:58 AM
>>> To: Khaire, Rohit <Rohit.Khaire at amd.com>; 
>>> amd-gfx at lists.freedesktop.org; Deucher, Alexander 
>>> <Alexander.Deucher at amd.com>; Zhang, Hawking <Hawking.Zhang at amd.com>; 
>>> Deng, Emily <Emily.Deng at amd.com>; Liu, Monk <Monk.Liu at amd.com>; 
>>> Zhou, Peng Ju <PengJu.Zhou at amd.com>; Chen, Horace 
>>> <Horace.Chen at amd.com>
>>> Cc: Ming, Davis <Davis.Ming at amd.com>
>>> Subject: Re: [PATCH] drm/amdgpu: Use PSP to program
>>> IH_RB_CNTL_RING1/2 on SRIOV
>>>
>>> Do you have the hash for this commit?
>>>
>>> Thanks,
>>> Christian.
>>>
>>> Am 07.06.21 um 17:30 schrieb Khaire, Rohit:
>>>> [AMD Public Use]
>>>>
>>>> We don't need RING1 and RING2 functionality for SRIOV afaik.
>>>>
>>>> But looking at the description of the original commit message it 
>>>> affects RING0 too?
>>>>
>>>> " drm/amdgpu: add timeout flush mechanism to update wptr for self 
>>>> interrupt (v2)
>>>>
>>>> outstanding log reaches threshold will trigger IH ring1/2's wptr 
>>>> reported, that will avoid generating interrupts to ring0 too frequent.
>>>> But if ring1/2's wptr hasn't been increased for a long time, the 
>>>> outstanding log can't reach threshold so that driver can't get 
>>>> latest wptr info and miss some interrupts."
>>>>
>>>> Rohit
>>>>
>>>> -----Original Message-----
>>>> From: Koenig, Christian <Christian.Koenig at amd.com>
>>>> Sent: June 7, 2021 10:31 AM
>>>> To: Khaire, Rohit <Rohit.Khaire at amd.com>; 
>>>> amd-gfx at lists.freedesktop.org; Deucher, Alexander 
>>>> <Alexander.Deucher at amd.com>; Zhang, Hawking 
>>>> <Hawking.Zhang at amd.com>; Deng, Emily <Emily.Deng at amd.com>; Liu, 
>>>> Monk <Monk.Liu at amd.com>; Zhou, Peng Ju <PengJu.Zhou at amd.com>; Chen, 
>>>> Horace <Horace.Chen at amd.com>
>>>> Cc: Ming, Davis <Davis.Ming at amd.com>
>>>> Subject: Re: [PATCH] drm/amdgpu: Use PSP to program 
>>>> IH_RB_CNTL_RING1/2 on SRIOV
>>>>
>>>> Why are the ring 1&2 enabled on SRIOV in the first place?
>>>>
>>>> Christian.
>>>>
>>>> Am 07.06.21 um 16:23 schrieb Rohit Khaire:
>>>>> This is similar to IH_RB_CNTL programming in 
>>>>> navi10_ih_toggle_ring_interrupts
>>>>>
>>>>> Signed-off-by: Rohit Khaire <rohit.khaire at amd.com>
>>>>> ---
>>>>>      drivers/gpu/drm/amd/amdgpu/navi10_ih.c | 20 
>>>>> ++++++++++++++++++--
>>>>>      1 file changed, 18 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/navi10_ih.c
>>>>> b/drivers/gpu/drm/amd/amdgpu/navi10_ih.c
>>>>> index eac564e8dd52..e41188c04846 100644
>>>>> --- a/drivers/gpu/drm/amd/amdgpu/navi10_ih.c
>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/navi10_ih.c
>>>>> @@ -120,11 +120,27 @@ force_update_wptr_for_self_int(struct
>>>>> amdgpu_device *adev,
>>>>>          ih_rb_cntl = REG_SET_FIELD(ih_rb_cntl, IH_RB_CNTL_RING1,
>>>>>                         RB_USED_INT_THRESHOLD, threshold);
>>>>>      -    WREG32_SOC15(OSSSYS, 0, mmIH_RB_CNTL_RING1, ih_rb_cntl);
>>>>> +    if (amdgpu_sriov_vf(adev) &&
>>>>> amdgpu_sriov_reg_indirect_ih(adev)) {
>>>>> +        if (psp_reg_program(&adev->psp, PSP_REG_IH_RB_CNTL_RING1,
>>>>> ih_rb_cntl)) {
>>>>> +            DRM_ERROR("PSP program IH_RB_CNTL_RING1 failed!\n");
>>>>> +            return;
>>>>> +        }
>>>>> +    } else {
>>>>> +        WREG32_SOC15(OSSSYS, 0, mmIH_RB_CNTL_RING1, ih_rb_cntl);
>>>>> +    }
>>>>> +
>>>>>          ih_rb_cntl = RREG32_SOC15(OSSSYS, 0, mmIH_RB_CNTL_RING2);
>>>>>          ih_rb_cntl = REG_SET_FIELD(ih_rb_cntl, IH_RB_CNTL_RING2,
>>>>>                         RB_USED_INT_THRESHOLD, threshold);
>>>>> -    WREG32_SOC15(OSSSYS, 0, mmIH_RB_CNTL_RING2, ih_rb_cntl);
>>>>> +    if (amdgpu_sriov_vf(adev) &&
>>>>> amdgpu_sriov_reg_indirect_ih(adev)) {
>>>>> +        if (psp_reg_program(&adev->psp, PSP_REG_IH_RB_CNTL_RING2,
>>>>> ih_rb_cntl)) {
>>>>> +            DRM_ERROR("PSP program IH_RB_CNTL_RING2 failed!\n");
>>>>> +            return;
>>>>> +        }
>>>>> +    } else {
>>>>> +        WREG32_SOC15(OSSSYS, 0, mmIH_RB_CNTL_RING2, ih_rb_cntl);
>>>>> +    }
>>>>> +
>>>>>          WREG32_SOC15(OSSSYS, 0, mmIH_CNTL2, ih_cntl);
>>>>>      }
>>>>>