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

[Timur Kristóf]

On Mon, 2025-08-04 at 13:45 -0400, Alex Deucher wrote:
> 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.

In that case, wouldn't it cause issues that the SMC is not aware that
the UVD is running?

As far as I see from si_convert_power_*_to_smc there are some flags in
the power states which are set differently depending on whether UVD is
active or not.

A secondary issue is that the VBIOS seems to have different clock
values for the UVD compared to what is hardcoded in the driver. This
doesn't mean the driver is incorrect, but si_dpm does use the values
from the VBIOS.

> Programming the PLLs directly works as is in radeon, but I guess
> maybe
> we init DPM in a different order in radeon vs amdgpu.

The order is the same.

Looking at radeon's si.c, in si_init:
It calls radeon_pm_init (which eventually calls si_dpm_init) before
calling si_uvd_init.

Looking at amdgpu's si.c, in si_set_ip_blocks:
The si_smu_ip_block is added before uvd_v3_1_ip_block.

I think the main difference between radeon and amdgpu as far as I see:
- radeon doesn't do the FW validation
- radeon doesn't do a ring test

Hence, radeon doesn't actually need to enable the UVD clocks at
initialization.

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

Note that the UVD clocks were already enabled by uvd_v3_1_hw_init, when
it creates the ring test, that calls amdgpu_uvd_ring_begin_use which
enables the clock by calling amdgpu_dpm_enable_uvd. The only difference
that my patch makes is that it now enables the UVD clock just a little
bit earlier.

I think we can't disable the UVD clocks directly in uvd_v3_1_hw_init,
because the ring test would fail. However, as far as I understand, the
UVD clocks are already disabled automatically anyway after the ring
test is done: amdgpu_ring_commit calls amdgpu_uvd_ring_end_use. This
launches the UVD idle work handler, which automatically disables the UV
clock when all pending fences are signalled.

Please correct me if I misunderstood this.

Thanks,
Timur


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