[PATCH v3 3/3] drm/amdgpu/sdma4.4.2: implement ring reset callback for sdma4.4.2
Lazar, Lijo
lijo.lazar at amd.com
Tue Oct 15 07:26:21 UTC 2024
On 10/15/2024 1:08 AM, Deucher, Alexander wrote:
> [AMD Official Use Only - AMD Internal Distribution Only]
>
>> -----Original Message-----
>> From: Zhu, Jiadong <Jiadong.Zhu at amd.com>
>> Sent: Wednesday, October 9, 2024 5:23 AM
>> To: Lazar, Lijo <Lijo.Lazar at amd.com>; amd-gfx at lists.freedesktop.org
>> Cc: Deucher, Alexander <Alexander.Deucher at amd.com>
>> Subject: RE: [PATCH v3 3/3] drm/amdgpu/sdma4.4.2: implement ring reset callback
>> for sdma4.4.2
>>
>> [AMD Official Use Only - AMD Internal Distribution Only]
>>
>>> -----Original Message-----
>>> From: Lazar, Lijo <Lijo.Lazar at amd.com>
>>> Sent: Wednesday, October 9, 2024 5:13 PM
>>> To: Zhu, Jiadong <Jiadong.Zhu at amd.com>; amd-gfx at lists.freedesktop.org
>>> Cc: Deucher, Alexander <Alexander.Deucher at amd.com>
>>> Subject: Re: [PATCH v3 3/3] drm/amdgpu/sdma4.4.2: implement ring reset
>>> callback for sdma4.4.2
>>>
>>>
>>>
>>> On 10/9/2024 12:52 PM, jiadong.zhu at amd.com wrote:
>>>> From: Jiadong Zhu <Jiadong.Zhu at amd.com>
>>>>
>>>> Implement sdma queue reset callback via SMU interface.
>>>>
>>>> v2: Leverage inst_stop/start functions in reset sequence.
>>>> Use GET_INST for physical SDMA instance.
>>>> Disable apu for sdma reset.
>>>> v3: Rephrase error prints.
>>>>
>>>> Signed-off-by: Jiadong Zhu <Jiadong.Zhu at amd.com>
>>>> ---
>>>> drivers/gpu/drm/amd/amdgpu/sdma_v4_4_2.c | 97
>>>> +++++++++++++++++++-----
>>>> 1 file changed, 79 insertions(+), 18 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v4_4_2.c
>>>> b/drivers/gpu/drm/amd/amdgpu/sdma_v4_4_2.c
>>>> index c77889040760..9a970a3cb908 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/sdma_v4_4_2.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/sdma_v4_4_2.c
>>>> @@ -667,11 +667,12 @@ static uint32_t sdma_v4_4_2_rb_cntl(struct
>>> amdgpu_ring *ring, uint32_t rb_cntl)
>>>> *
>>>> * @adev: amdgpu_device pointer
>>>> * @i: instance to resume
>>>> + * @restore: used to restore wptr when restart
>>>> *
>>>> * Set up the gfx DMA ring buffers and enable them.
>>>> * Returns 0 for success, error for failure.
>>>> */
>>>> -static void sdma_v4_4_2_gfx_resume(struct amdgpu_device *adev,
>>>> unsigned int i)
>>>> +static void sdma_v4_4_2_gfx_resume(struct amdgpu_device *adev,
>>>> +unsigned int i, bool restore)
>>>> {
>>>> struct amdgpu_ring *ring = &adev->sdma.instance[i].ring;
>>>> u32 rb_cntl, ib_cntl, wptr_poll_cntl; @@ -698,16 +699,24 @@
>>>> static void sdma_v4_4_2_gfx_resume(struct amdgpu_device *adev, unsigned
>> int i)
>>>> WREG32_SDMA(i, regSDMA_GFX_RB_BASE, ring->gpu_addr >> 8);
>>>> WREG32_SDMA(i, regSDMA_GFX_RB_BASE_HI, ring->gpu_addr >> 40);
>>>>
>>>> - ring->wptr = 0;
>>>> + if (!restore)
>>>> + ring->wptr = 0;
>>>>
>>>> /* before programing wptr to a less value, need set minor_ptr_update first */
>>>> WREG32_SDMA(i, regSDMA_GFX_MINOR_PTR_UPDATE, 1);
>>>>
>>>> /* Initialize the ring buffer's read and write pointers */
>>>> - WREG32_SDMA(i, regSDMA_GFX_RB_RPTR, 0);
>>>> - WREG32_SDMA(i, regSDMA_GFX_RB_RPTR_HI, 0);
>>>> - WREG32_SDMA(i, regSDMA_GFX_RB_WPTR, 0);
>>>> - WREG32_SDMA(i, regSDMA_GFX_RB_WPTR_HI, 0);
>>>> + if (restore) {
>>>> + WREG32_SDMA(i, regSDMA_GFX_RB_RPTR, lower_32_bits(ring-
>>>> wptr << 2));
>>>> + WREG32_SDMA(i, regSDMA_GFX_RB_RPTR_HI,
>>> upper_32_bits(ring->wptr << 2));
>>>> + WREG32_SDMA(i, regSDMA_GFX_RB_WPTR, lower_32_bits(ring-
>>>> wptr << 2));
>>>> + WREG32_SDMA(i, regSDMA_GFX_RB_WPTR_HI,
>>> upper_32_bits(ring->wptr << 2));
>>>> + } else {
>>>> + WREG32_SDMA(i, regSDMA_GFX_RB_RPTR, 0);
>>>> + WREG32_SDMA(i, regSDMA_GFX_RB_RPTR_HI, 0);
>>>> + WREG32_SDMA(i, regSDMA_GFX_RB_WPTR, 0);
>>>> + WREG32_SDMA(i, regSDMA_GFX_RB_WPTR_HI, 0);
>>>> + }
>>>>
>>>> doorbell = RREG32_SDMA(i, regSDMA_GFX_DOORBELL);
>>>> doorbell_offset = RREG32_SDMA(i,
>>> regSDMA_GFX_DOORBELL_OFFSET); @@
>>>> -759,7 +768,7 @@ static void sdma_v4_4_2_gfx_resume(struct
>>>> amdgpu_device
>>> *adev, unsigned int i)
>>>> * Set up the page DMA ring buffers and enable them.
>>>> * Returns 0 for success, error for failure.
>>>> */
>>>> -static void sdma_v4_4_2_page_resume(struct amdgpu_device *adev,
>>>> unsigned int i)
>>>> +static void sdma_v4_4_2_page_resume(struct amdgpu_device *adev,
>>>> +unsigned int i, bool restore)
>>>> {
>>>> struct amdgpu_ring *ring = &adev->sdma.instance[i].page;
>>>> u32 rb_cntl, ib_cntl, wptr_poll_cntl; @@ -775,10 +784,17 @@
>>>> static void sdma_v4_4_2_page_resume(struct amdgpu_device *adev, unsigned
>> int i)
>>>> WREG32_SDMA(i, regSDMA_PAGE_RB_CNTL, rb_cntl);
>>>>
>>>> /* Initialize the ring buffer's read and write pointers */
>>>> - WREG32_SDMA(i, regSDMA_PAGE_RB_RPTR, 0);
>>>> - WREG32_SDMA(i, regSDMA_PAGE_RB_RPTR_HI, 0);
>>>> - WREG32_SDMA(i, regSDMA_PAGE_RB_WPTR, 0);
>>>> - WREG32_SDMA(i, regSDMA_PAGE_RB_WPTR_HI, 0);
>>>> + if (restore) {
>>>> + WREG32_SDMA(i, regSDMA_GFX_RB_RPTR, lower_32_bits(ring-
>>>> wptr << 2));
>>>> + WREG32_SDMA(i, regSDMA_GFX_RB_RPTR_HI,
>>> upper_32_bits(ring->wptr << 2));
>>>> + WREG32_SDMA(i, regSDMA_GFX_RB_WPTR, lower_32_bits(ring-
>>>> wptr << 2));
>>>> + WREG32_SDMA(i, regSDMA_GFX_RB_WPTR_HI,
>>> upper_32_bits(ring->wptr << 2));
>>>> + } else {
>>>> + WREG32_SDMA(i, regSDMA_PAGE_RB_RPTR, 0);
>>>> + WREG32_SDMA(i, regSDMA_PAGE_RB_RPTR_HI, 0);
>>>> + WREG32_SDMA(i, regSDMA_PAGE_RB_WPTR, 0);
>>>> + WREG32_SDMA(i, regSDMA_PAGE_RB_WPTR_HI, 0);
>>>> + }
>>>>
>>>> /* set the wb address whether it's enabled or not */
>>>> WREG32_SDMA(i, regSDMA_PAGE_RB_RPTR_ADDR_HI, @@ -792,7
>>> +808,8 @@
>>>> static void sdma_v4_4_2_page_resume(struct amdgpu_device *adev,
>>>> unsigned
>>> int i)
>>>> WREG32_SDMA(i, regSDMA_PAGE_RB_BASE, ring->gpu_addr >> 8);
>>>> WREG32_SDMA(i, regSDMA_PAGE_RB_BASE_HI, ring->gpu_addr >>
>>> 40);
>>>>
>>>> - ring->wptr = 0;
>>>> + if (!restore)
>>>> + ring->wptr = 0;
>>>>
>>>> /* before programing wptr to a less value, need set minor_ptr_update first */
>>>> WREG32_SDMA(i, regSDMA_PAGE_MINOR_PTR_UPDATE, 1); @@ -
>>> 916,7 +933,7
>>>> @@ static int sdma_v4_4_2_inst_load_microcode(struct amdgpu_device *adev,
>>>> * Returns 0 for success, error for failure.
>>>> */
>>>> static int sdma_v4_4_2_inst_start(struct amdgpu_device *adev,
>>>> - uint32_t inst_mask)
>>>> + uint32_t inst_mask, bool restore)
>>>> {
>>>> struct amdgpu_ring *ring;
>>>> uint32_t tmp_mask;
>>>> @@ -927,7 +944,7 @@ static int sdma_v4_4_2_inst_start(struct
>>>> amdgpu_device
>>> *adev,
>>>> sdma_v4_4_2_inst_enable(adev, false, inst_mask);
>>>> } else {
>>>> /* bypass sdma microcode loading on Gopher */
>>>> - if (adev->firmware.load_type != AMDGPU_FW_LOAD_PSP &&
>>>> + if (!restore && adev->firmware.load_type !=
>>> AMDGPU_FW_LOAD_PSP &&
>>>> adev->sdma.instance[0].fw) {
>>>> r = sdma_v4_4_2_inst_load_microcode(adev, inst_mask);
>>>> if (r)
>>>> @@ -946,9 +963,9 @@ static int sdma_v4_4_2_inst_start(struct
>>>> amdgpu_device
>>> *adev,
>>>> uint32_t temp;
>>>>
>>>> WREG32_SDMA(i, regSDMA_SEM_WAIT_FAIL_TIMER_CNTL, 0);
>>>> - sdma_v4_4_2_gfx_resume(adev, i);
>>>> + sdma_v4_4_2_gfx_resume(adev, i, restore);
>>>> if (adev->sdma.has_page_queue)
>>>> - sdma_v4_4_2_page_resume(adev, i);
>>>> + sdma_v4_4_2_page_resume(adev, i, restore);
>>>>
>>>> /* set utc l1 enable flag always to 1 */
>>>> temp = RREG32_SDMA(i, regSDMA_CNTL); @@ -1477,7 +1494,7
>>> @@ static
>>>> int sdma_v4_4_2_hw_init(void *handle)
>>>> if (!amdgpu_sriov_vf(adev))
>>>> sdma_v4_4_2_inst_init_golden_registers(adev, inst_mask);
>>>>
>>>> - r = sdma_v4_4_2_inst_start(adev, inst_mask);
>>>> + r = sdma_v4_4_2_inst_start(adev, inst_mask, false);
>>>>
>>>> return r;
>>>> }
>>>> @@ -1566,6 +1583,49 @@ static int sdma_v4_4_2_soft_reset(void *handle)
>>>> return 0;
>>>> }
>>>>
>>>> +static int sdma_v4_4_2_reset_queue(struct amdgpu_ring *ring,
>>>> +unsigned int vmid) {
>>>> + struct amdgpu_device *adev = ring->adev;
>>>> + int i, r;
>>>> + u32 preempt, inst_mask;
>>>> +
>>>> + if ((adev->flags & AMD_IS_APU) || amdgpu_sriov_vf(adev))
>>>> + return -EINVAL;
>>>> +
>>>> + /* stop queue */
>>>> + inst_mask = 1 << ring->me;
>>>> + sdma_v4_4_2_inst_gfx_stop(adev, inst_mask);
>>>> + if (adev->sdma.has_page_queue)
>>>> + sdma_v4_4_2_inst_page_stop(adev, inst_mask);
>>>> +
>>>> + /* soft reset SDMA_GFX_PREEMPT.IB_PREEMPT = 0*/
>>>> + preempt = RREG32_SDMA(ring->me, regSDMA_GFX_PREEMPT);
>>>> + preempt = REG_SET_FIELD(preempt, SDMA_GFX_PREEMPT,
>>> IB_PREEMPT, 0);
>>>> + WREG32_SDMA(ring->me, regSDMA_GFX_PREEMPT, preempt);
>>>
>>> Confirming - should this be RB_PREEMPT?
>>>> +
>>>> + r = amdgpu_dpm_reset_sdma(adev, 1 << GET_INST(SDMA0, ring->me));
>>>> + if (r) {
>>>> + dev_err(adev->dev, "SDMA%d reset failed\n", ring->me);
>>>
>>> My earlier comment was if smu_v13_0_6_reset_sdma() fails, that will
>>> result in this print in addition to the error log in
>>> smu_v13_0_6_reset_sdma(). You may drop one of them.
>>
>> Sure.
>>
>> Thanks,
>> Jiadong
>>
>>>
>>> Apart from that missed to ask one basic question - this looks more
>>> like an SDMA engine reset rather than a SDMA queue reset (ex: only
>>> reset one of the SDMA RLC queues). Is this intended as a queue reset?
>>>
>>> Thanks,
>>> Lijo
>>
>> Yes, OSS4.X does not support to reset a queue for sdma, thus we implement it by
>> per engine reset.
>
> SDMA is single threaded so it is effectively a queue reset even if the entire instance is reset since there will only be one job active on the instance at a given time.
>
Even if timesliced, I guess queue reset purpose is not to affect
processes using other queues. I was thinking it is done through some
sort of forced preempt ("should this be RB_PREEMPT"). Jiandong clarified
offline that it's not using that type of mechanism. Since the instance
is getting reset in this method, that means other queues are also not
usable during that time.
Thanks,
Lijo
> Alex
>
>
>>
>> Thanks,
>> Jiadong
>>
>>>> + return r;
>>>> + }
>>>> +
>>>> + udelay(50);
>>>> +
>>>> + for (i = 0; i < adev->usec_timeout; i++) {
>>>> + if (!REG_GET_FIELD(RREG32_SDMA(ring->me,
>>> regSDMA_F32_CNTL), SDMA_F32_CNTL, HALT))
>>>> + break;
>>>> + udelay(1);
>>>> + }
>>>> +
>>>> + if (i == adev->usec_timeout) {
>>>> + dev_err(adev->dev, "timed out waiting for SDMA%d unhalt
>>>> + after
>>> reset\n",
>>>> + ring->me);
>>>> + return -ETIMEDOUT;
>>>> + }
>>>> +
>>>> + return sdma_v4_4_2_inst_start(adev, inst_mask, true); }
>>>> +
>>>> static int sdma_v4_4_2_set_trap_irq_state(struct amdgpu_device *adev,
>>>> struct amdgpu_irq_src *source,
>>>> unsigned type, @@ -1948,6
>>>> +2008,7 @@ static const struct amdgpu_ring_funcs
>>> sdma_v4_4_2_ring_funcs = {
>>>> .emit_wreg = sdma_v4_4_2_ring_emit_wreg,
>>>> .emit_reg_wait = sdma_v4_4_2_ring_emit_reg_wait,
>>>> .emit_reg_write_reg_wait =
>>>> amdgpu_ring_emit_reg_write_reg_wait_helper,
>>>> + .reset = sdma_v4_4_2_reset_queue,
>>>> };
>>>>
>>>> static const struct amdgpu_ring_funcs sdma_v4_4_2_page_ring_funcs =
>>>> { @@ -2160,7 +2221,7 @@ static int sdma_v4_4_2_xcp_resume(void
>>>> *handle,
>>> uint32_t inst_mask)
>>>> if (!amdgpu_sriov_vf(adev))
>>>> sdma_v4_4_2_inst_init_golden_registers(adev, inst_mask);
>>>>
>>>> - r = sdma_v4_4_2_inst_start(adev, inst_mask);
>>>> + r = sdma_v4_4_2_inst_start(adev, inst_mask, false);
>>>>
>>>> return r;
>>>> }
>
More information about the amd-gfx
mailing list