[AMD Official Use Only - AMD Internal Distribution Only] >-----Original Message----- >From: amd-gfx <[email protected]> On Behalf Of Mario >Limonciello >Sent: Monday, October 13, 2025 7:34 PM >To: Lazar, Lijo <[email protected]>; [email protected]; Deucher, >Alexander <[email protected]> >Cc: Limonciello, Mario <[email protected]>; Lee, Peyton ><[email protected]>; Sultan Alsawaf <[email protected]> >Subject: Re: [PATCH v3] drm/amd: Check that VPE has reached DPM0 in idle >handler > >On 10/13/25 8:59 AM, Lazar, Lijo wrote: >> [AMD Official Use Only - AMD Internal Distribution Only] >> >>> -----Original Message----- >>> From: Mario Limonciello <[email protected]> >>> Sent: Monday, October 13, 2025 7:21 PM >>> To: Lazar, Lijo <[email protected]>; [email protected] >>> Cc: Limonciello, Mario <[email protected]>; Lee, Peyton >>> <[email protected]>; Sultan Alsawaf <[email protected]> >>> Subject: Re: [PATCH v3] drm/amd: Check that VPE has reached DPM0 in >>> idle handler >>> >>> On 10/12/25 11:54 PM, Lazar, Lijo wrote: >>>> [Public] >>>> >>>> Doesn't this translate to just a higher idle timeout >>>> (VPE_IDLE_TIMEOUT ) for >>> the particular VPE version? >>>> >>>> Thanks, >>>> Lijo >>> >>> Yes if the VPE microcode adjusts DPM at runtime this makes sure that >>> it has settled when workload is complete. >>> >>> I expect that a higher VPE_IDLE_TIMEOUT would work too, but it seems >>> less scalable to me. >>> >> [lijo] >> >> I guess VPE firmware behavior could vary across generations. For ex: even if >> it >doesn't lower the clocks in this generation, it could do so after seeing a >power >gate (any handshake with PMFW). Or, even if it doesn't lower the clock, it may >adjust the clocks after powering up. >> >> So probably just keeping vpe.idle_timeout as a variable based on IP version >may be good enough for the current issue. > >"Ideally" PMFW would lower clocks before turning off VPE, but that's not the >case right now on Strix Halo. We just get lucky with older VPE microcode that >it >doesn't change this. > [lijo] PMFW could be changing clocks only on VPE requests. Ideally, it's not required to lower DPM clock before power-off, just keeping it at deep sleep is good enough which goes even lower than DPM0 levels. Deep sleep these days is taken care by hardware.
Thanks, Lijo >My thought was this current solution will work properly on all microcode >version on all products. If PMFW changes behavior we could add conditional >code later to only do this check for DPM level if on older PMFW. > >Alex, > >What do you want to see here? >> >> Thanks, >> Lijo >> >>>>> -----Original Message----- >>>>> From: amd-gfx <[email protected]> On Behalf Of >>>>> Mario Limonciello (AMD) >>>>> Sent: Monday, October 13, 2025 12:48 AM >>>>> To: [email protected] >>>>> Cc: Limonciello, Mario <[email protected]>; Lee, Peyton >>>>> <[email protected]>; Sultan Alsawaf <[email protected]> >>>>> Subject: [PATCH v3] drm/amd: Check that VPE has reached DPM0 in >>>>> idle handler >>>>> >>>>> From: Mario Limonciello <[email protected]> >>>>> >>>>> [Why] >>>>> Newer VPE microcode has functionality that will decrease DPM level >>>>> only when a workload has run for 2 or more seconds. If VPE is >>>>> turned off before this DPM decrease, the SOC can get stuck with a higher >DPM level. >>>>> >>>>> This can happen from amdgpu's ring buffer test because it's a short >>>>> quick workload for VPE and VPE is turned off after 1s. >>>>> >>>>> [How] >>>>> In idle handler besides checking fences are drained check that VPE >>>>> DPM level is really is at DPM0. If not, schedule delayed work again until >>>>> it >is. >>>>> >>>>> Cc: [email protected] >>>>> Reported-by: Sultan Alsawaf <[email protected]> >>>>> Closes: https://gitlab.freedesktop.org/drm/amd/-/issues/4615 >>>>> Signed-off-by: Mario Limonciello <[email protected]> >>>>> --- >>>>> v3: >>>>> * Use label to avoid a register read if fences active >>>>> --- >>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_vpe.c | 15 ++++++++++++--- >>>>> 1 file changed, 12 insertions(+), 3 deletions(-) >>>>> >>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vpe.c >>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vpe.c >>>>> index 474bfe36c0c2f..e8e512de5992a 100644 >>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vpe.c >>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vpe.c >>>>> @@ -326,14 +326,23 @@ static void vpe_idle_work_handler(struct >>>>> work_struct *work) { >>>>> struct amdgpu_device *adev = >>>>> container_of(work, struct amdgpu_device, >>>>> vpe.idle_work.work); >>>>> + struct amdgpu_vpe *vpe = &adev->vpe; >>>>> unsigned int fences = 0; >>>>> + uint32_t dpm_level; >>>>> >>>>> fences += amdgpu_fence_count_emitted(&adev->vpe.ring); >>>>> + if (fences) >>>>> + goto reschedule; >>>>> >>>>> - if (fences == 0) >>>>> + dpm_level = adev->pm.dpm_enabled ? >>>>> + RREG32(vpe_get_reg_offset(vpe, 0, vpe- >>>>>> regs.dpm_request_lv)) : 0; >>>>> + if (!dpm_level) { >>>>> amdgpu_device_ip_set_powergating_state(adev, >>>>> AMD_IP_BLOCK_TYPE_VPE, AMD_PG_STATE_GATE); >>>>> - else >>>>> - schedule_delayed_work(&adev->vpe.idle_work, >>>>> VPE_IDLE_TIMEOUT); >>>>> + return; >>>>> + } >>>>> + >>>>> +reschedule: >>>>> + schedule_delayed_work(&adev->vpe.idle_work, >>>>> +VPE_IDLE_TIMEOUT); >>>>> } >>>>> >>>>> static int vpe_common_init(struct amdgpu_vpe *vpe) >>>>> -- >>>>> 2.43.0 >>>> >>
