[PATCH 1/6] drm/amdgpu: Power up UVD 3 for FW validation
Timur Kristóf
timur.kristof at gmail.com
Wed Aug 6 00:35:07 UTC 2025
On Mon, 2025-08-04 at 20:59 +0200, Christian König wrote:
> On 04.08.25 19:45, 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.
>
> Oh my, that stuff was last at the front of my head a long long time
> ago.
Thanks for taking the time to reply anyway!
>
> > > >
> > > > 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.
>
> >
> > 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?
Thanks,
Timur
>
> Regards,
> Christian.
>
> >
> > 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
> > > > >
More information about the amd-gfx
mailing list