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

[Timur Kristóf]

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.
> 
> 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

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).

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.

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
> >