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

[Alex Deucher]

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

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

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