[PATCH 1/6] drm/amdgpu: Power up UVD 3 for FW validation

[Christian König]

On 04.08.25 19:45, Alex Deucher wrote:
> On Mon, Aug 4, 2025 at 12:00 PM Timur Kristóf <timur.kristof at gmail.com> wrote:
>>
>> On Mon, 2025-08-04 at 11:20 -0400, Alex Deucher wrote:
>>> On Mon, Aug 4, 2025 at 9:58 AM Timur Kristóf
>>> <timur.kristof at gmail.com> wrote:
>>>>
>>>> Unlike later versions, UVD 3 has firmware validation.
>>>> For this to work, the UVD should be powered up correctly.
>>>>
>>>> When DPM is enabled and the display clock is off,
>>>> the SMU may choose a power state which doesn't power
>>>> the UVD, which can result in failure to initialize UVD.
>>>
>>> + Christian, Leo
>>>
>>> That doesn't seem right to me.  IIRC, the driver always set the UVD
>>> PLL directly on SI and I don't think SI supported any kind of UVD
>>> power gating. I guess it's probably some sort of subtle sequencing
>>> difference between radeon and amdgpu.  Unless Christian or Leo have
>>> any ideas, I think the patch is probably fine.

Oh my, that stuff was last at the front of my head a long long time ago.

>>>
>>> Alex
>>
>> Hi,
>>
>> These are my observations about how the UVD clock works on SI:
>>
>> 1. It seems that the SMC needs to know whether UVD is enabled or not,
>> and the UVD clocks are included as part of the power states. See:
>> si_convert_power_state_to_smc
>> si_convert_power_level_to_smc

Correct, yes. The design was that either the KMD or the SMC could program the PLLs.

>>
>> On SI the default power state doesn't set the UVD clocks,
>> and SI has a specific power state to be used with UVD. Actually
>> amdgpu_dpm_enable_uvd has a special case code path for SI, where it
>> sets this power state. If I print the power states from
>> si_parse_power_table, it indeed confirms that there is only one power
>> state that has non-zero UVD clocks, and the rest of them just have the
>> UVD clocks at zero.
>>
>> It's unclear to me what happens if we try to enable UVD clocks when we
>> selected a power state that doesn't include them (ie. when we don't
>> tell the SMC that UVD is active).

IIRC there were two possibilities.

Either you let the SMC handle the clocks in which case it would lower the GFX clock in favor of stable UVD clocks.

Or the KMD would lock the SMC to the highest level and then program the UVD clocks manually.

The later was not really validated but requested by a lot of people because otherwise you got a GFX performance reduction whenever you used UVD.

>>
>> 2. When setting a power state that enables UVD, the UVD clock is
>> enabled either before or after the engine clock by si_dpm. This is done
>> so in both radeon and amdgpu, see:
>> si_dpm_set_power_state
>> ni_set_uvd_clock_before_set_eng_clock
>> ni_set_uvd_clock_after_set_eng_clock
>>
>> The specific sequence in which the UVD clock is enabled by
>> si_dpm_set_power_state leads me to the conclusion that
>> amdgpu_asic_set_uvd_clocks should not be directly called on SI outside
>> of the DPM code.
>>
>> Please correct me if I misunderstood the code.

That sounds correct to me.

> 
> Yeah, I don't remember the clock dependencies.  I thought that you
> should be able to program the UVD PLLs any time you wanted and the
> ordering only mattered when you were also changing the sclk.
> Programming the PLLs directly works as is in radeon, but I guess maybe
> we init DPM in a different order in radeon vs amdgpu.
> 
> It would also probably be a good idea to disable the UVD clocks again
> after IP init to save power. E.g., something like:
> 
>        if (adev->pm.dpm_enabled)
>                amdgpu_dpm_enable_uvd(adev, false);
>        else
>                amdgpu_asic_set_uvd_clocks(adev, 0, 0);

IIRC we always disabled the PLL manually when UVD was unused because the SMC sometimes failed to do this.

Regards,
Christian.

> 
> Alex
> 
> 
>>
>> Thanks,
>> Timur
>>
>>
>>>
>>>>
>>>> Fixes: b38f3e80ecec ("drm amdgpu: SI UVD v3_1 (v2)")
>>>> Signed-off-by: Timur Kristóf <timur.kristof at gmail.com>
>>>> ---
>>>>  drivers/gpu/drm/amd/amdgpu/uvd_v3_1.c | 9 ++++++---
>>>>  1 file changed, 6 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/uvd_v3_1.c
>>>> b/drivers/gpu/drm/amd/amdgpu/uvd_v3_1.c
>>>> index 5dbaebb592b3..9ad06c1e150d 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/uvd_v3_1.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/uvd_v3_1.c
>>>> @@ -633,6 +633,12 @@ static int uvd_v3_1_hw_init(struct
>>>> amdgpu_ip_block *ip_block)
>>>>         int r;
>>>>
>>>>         uvd_v3_1_mc_resume(adev);
>>>> +       uvd_v3_1_enable_mgcg(adev, true);
>>>> +
>>>> +       if (adev->pm.dpm_enabled)
>>>> +               amdgpu_dpm_enable_uvd(adev, true);
>>>> +       else
>>>> +               amdgpu_asic_set_uvd_clocks(adev, 53300, 40000);
>>>>
>>>>         r = uvd_v3_1_fw_validate(adev);
>>>>         if (r) {
>>>> @@ -640,9 +646,6 @@ static int uvd_v3_1_hw_init(struct
>>>> amdgpu_ip_block *ip_block)
>>>>                 return r;
>>>>         }
>>>>
>>>> -       uvd_v3_1_enable_mgcg(adev, true);
>>>> -       amdgpu_asic_set_uvd_clocks(adev, 53300, 40000);
>>>> -
>>>>         uvd_v3_1_start(adev);
>>>>
>>>>         r = amdgpu_ring_test_helper(ring);
>>>> --
>>>> 2.50.1
>>>>