Bug 205393

Summary: [amdgpu powerplay] vega10: custom pp_table, AVFS accidentally reenabled after display powersave
Product: Drivers Reporter: haro41
Component: Video(DRI - non Intel)Assignee: drivers_video-dri
Status: RESOLVED CODE_FIX    
Severity: normal CC: alexdeucher, haro41, tempel.julian
Priority: P1 Keywords: trivial
Hardware: All   
OS: Linux   
Kernel Version: ~agd5f/linux (drm-next), 5.4-rc5, earlier kernels Subsystem:
Regression: No Bisected commit-id:
Attachments: patch to prevent AVFS reenabling (against 5.4-rc5)
patch to prevent AVFS reenabling (against 5.4-rc5)
patch to prevent AVFS reenabling, when custom pptable is loaded
possible fix
patch for smu7

Description haro41 2019-11-02 21:26:42 UTC
Created attachment 285755 [details]
patch to prevent AVFS reenabling (against 5.4-rc5)

When uploading a customized powerplay table, AVFS feature becomes correctly disabled.

But after a power save (blanc screen) event, AVFS becomes accidentally enabled again. This results in unwanted gfx voltage changes.

Example:

vddc = 1.075V after pp table upload (correct).

vddc = 1.150V after power save event (set by smc because AVFS feature reenabled).

I have debugged this issue and found a simple solution. There is a patch attached, that solves the problem.
Comment 1 haro41 2019-11-03 09:26:21 UTC
Created attachment 285757 [details]
patch to prevent AVFS reenabling (against 5.4-rc5)

If a custom pp table is loaded AVFS feature is correctly disabled.
AVFS may become accidently reenabled later, i.e. by a display power save wakeup event. This patch fixes this unwanted behavior for vega10.
Comment 2 haro41 2019-11-04 13:58:50 UTC
Created attachment 285775 [details]
patch to prevent AVFS reenabling, when custom pptable is loaded

patch refined (diff -upr6)
Comment 3 Alex Deucher 2019-11-07 17:17:16 UTC
Created attachment 285821 [details]
possible fix

Does this patch work?  I think it would be better to move the change above the call to vega10_update_avfs() to avoid enabling avfs only to disable it again later.
Comment 4 haro41 2019-11-07 18:18:16 UTC
Yes, your patch works and has the same effect, apparently.

What confused me and the reason why i prefered to leave the vega10_update_avfs() call before the flag modification, was the code inside vega10_update_avfs():

static int vega10_update_avfs(struct pp_hwmgr *hwmgr)
{
	struct vega10_hwmgr *data = hwmgr->backend;

	if (data->need_update_dpm_table & DPMTABLE_OD_UPDATE_VDDC) {
		vega10_avfs_enable(hwmgr, false);
	} else if (data->need_update_dpm_table) {
		vega10_avfs_enable(hwmgr, false);
		vega10_avfs_enable(hwmgr, true);
	} else {
		vega10_avfs_enable(hwmgr, true);
	}

	return 0;
}

Here is a disable/enable sequence inserted, perhaps for a reason?
Comment 5 Alex Deucher 2019-11-07 20:17:43 UTC
(In reply to haro41 from comment #4)
> Yes, your patch works and has the same effect, apparently.
> 
> What confused me and the reason why i prefered to leave the
> vega10_update_avfs() call before the flag modification, was the code inside
> vega10_update_avfs():
> 
> static int vega10_update_avfs(struct pp_hwmgr *hwmgr)
> {
>       struct vega10_hwmgr *data = hwmgr->backend;
> 
>       if (data->need_update_dpm_table & DPMTABLE_OD_UPDATE_VDDC) {
>               vega10_avfs_enable(hwmgr, false);
>       } else if (data->need_update_dpm_table) {
>               vega10_avfs_enable(hwmgr, false);
>               vega10_avfs_enable(hwmgr, true);
>       } else {
>               vega10_avfs_enable(hwmgr, true);
>       }
> 
>       return 0;
> }
> 
> Here is a disable/enable sequence inserted, perhaps for a reason?

It's needed to disable the current AFVS settings before reapplying the new ones.  That shouldn't be necessary with the DPMTABLE_OD_UPDATE_VDDC flag set because in that case, we just disable AVFS.
Comment 6 haro41 2019-11-07 20:39:02 UTC
(In reply to Alex Deucher from comment #5)
> (In reply to haro41 from comment #4)
> > Yes, your patch works and has the same effect, apparently.
> > 
> > What confused me and the reason why i prefered to leave the
> > vega10_update_avfs() call before the flag modification, was the code inside
> > vega10_update_avfs():
> > 
> > static int vega10_update_avfs(struct pp_hwmgr *hwmgr)
> > {
> >       struct vega10_hwmgr *data = hwmgr->backend;
> > 
> >       if (data->need_update_dpm_table & DPMTABLE_OD_UPDATE_VDDC) {
> >               vega10_avfs_enable(hwmgr, false);
> >       } else if (data->need_update_dpm_table) {
> >               vega10_avfs_enable(hwmgr, false);
> >               vega10_avfs_enable(hwmgr, true);
> >       } else {
> >               vega10_avfs_enable(hwmgr, true);
> >       }
> > 
> >       return 0;
> > }
> > 
> > Here is a disable/enable sequence inserted, perhaps for a reason?
> 
> It's needed to disable the current AFVS settings before reapplying the new
> ones.  That shouldn't be necessary with the DPMTABLE_OD_UPDATE_VDDC flag set
> because in that case, we just disable AVFS.

Ok, that makes sense. Thanks for clarification and for applying the fix.
Comment 7 tempel.julian 2019-11-07 23:57:14 UTC
Is this really only relevant for Vega 10?
Also on Polaris voltage is reset to default when using a customized powerplay table e.g. after a modeline switch (happens when closing Sway session with ctr + shift + e).
Comment 8 tempel.julian 2019-11-08 00:00:34 UTC
Small correction: It's actually $mod (meta/win key by default) + shift + e.
Comment 9 haro41 2019-11-08 10:35:35 UTC
(In reply to tempel.julian from comment #7)
> Is this really only relevant for Vega 10?
> Also on Polaris voltage is reset to default when using a customized
> powerplay table e.g. after a modeline switch (happens when closing Sway
> session with ctr + shift + e).

I don't have a polaris card for debugging, but looking at the code, i think that a similar bug could be relevant too. Vega10 is smu9, while Polaris is smu7, but both firmwares generally support AVFS and enabling/disabling this feature.
Comment 10 tempel.julian 2019-11-08 10:49:13 UTC
I'd try out patches if you want. :)
Though it's probably not really important, as the regular overdrive functionality works fine with Polaris.

Much more important would be situation for Navi, as regular overdrive doesn't work with it yet and one is dependent on the powerplay table method:
https://bugs.freedesktop.org/show_bug.cgi?id=111762
Comment 11 Alex Deucher 2019-11-08 16:18:32 UTC
Created attachment 285835 [details]
patch for smu7

Try this.
Comment 12 tempel.julian 2019-11-08 17:16:11 UTC
(In reply to Alex Deucher from comment #11)
> Created attachment 285835 [details]
> patch for smu7
> 
> Try this.

Thank you, I can confirm that it fixes the issue on Polaris.

Would the same be required for Navi as well?
Comment 13 Alex Deucher 2019-11-08 17:22:00 UTC
(In reply to tempel.julian from comment #12)
> 
> Would the same be required for Navi as well?

No.  IIRC, vega12 and newer handle voltage differently.
Comment 14 tempel.julian 2019-11-08 17:25:13 UTC
(In reply to Alex Deucher from comment #13)
> (In reply to tempel.julian from comment #12)
> > 
> > Would the same be required for Navi as well?
> 
> No.  IIRC, vega12 and newer handle voltage differently.

Ok, thanks. Maybe I can give it a try with someone else's Navi card before the end of the year. I'd report back in case something odd shows up.
Comment 15 haro41 2019-11-09 09:48:58 UTC
Considering this bug as resolved. Thanks everybody.