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

[Timur Kristóf]

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