Bug 205393

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.

Created attachment 285775 [details]
patch to prevent AVFS reenabling, when custom pptable is loaded

patch refined (diff -upr6)

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.

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?

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

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

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

Small correction: It's actually $mod (meta/win key by default) + shift + e.

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

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

Created attachment 285835 [details]
patch for smu7

Try this.

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

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

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

Considering this bug as resolved. Thanks everybody.