On Wed, 2025-08-06 at 14:27 +0200, Christian König wrote: > 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.
Hi Christian, Sounds good. I will add a comment when I submit a next version of this series. > > > > > Thanks, > > Timur
