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

[Christian König]

On 06.08.25 02:35, Timur Kristóf wrote:
>>
>>>>>
>>>>> 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.
> 
> As far as I see the si_dpm code does a mixture of the above two.
> When UVD is enabled, it selects the VBIOS-provided UVD power state and
> then it manually enables the UVD clocks to the value provided by the
> VBIOS.
> 
> When the UVD ring is not used anymore, it then shuts the UVD clock down
> manually.
> 
> (I assume then it goes back to a normal power state but I haven't
> actually verified that.)
> 
>>
>> 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.
> 
> Yes, the UVD power state from the VBIOS indeed has lower shader clocks
> compared to the normal power state.
> 
>>
>>>>
>>>> 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.
> 
> Thanks!
> 
> Sounds like the patch is correct, then.

Most likely yes.

> 
>>
>>>
>>> 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.
> 
> 
> Yes, as I mentioned in my previous mail the PM code does that already
> when the UVD ring is not in use anymore. So it's not necessary to add
> any code to shut it down.
> 
> Maybe I should edit the commit to explain that in a comment?

Code comment please!

That's basically the only chance we have to keep the knowledge why we did something the way we do it for the old HW generations around.

Regards,
Christian.

> 
> Thanks,
> Timur
>