Re: [PATCH 2/2] drm/amdgpu: allow APU to send power gate message when dpm is disabled

[Lang Yu <Lang.Yu@amd.com>]

On 12/06/ , Lazar, Lijo wrote:
> 
> 
> On 12/6/2021 12:18 PM, Yu, Lang wrote:
> > [Public]
> > 
> > A typo.
> > 
> > > -----Original Message-----
> > > From: Yu, Lang
> > > Sent: Monday, December 6, 2021 2:47 PM
> > > To: Lazar, Lijo <Lijo.Lazar@amd.com>; amd-gfx@lists.freedesktop.org
> > > Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; Huang, Ray
> > > <Ray.Huang@amd.com>
> > > Subject: RE: [PATCH 2/2] drm/amdgpu: allow APU to send power gate message
> > > when dpm is disabled
> > > 
> > > [Public]
> > > 
> > > 
> > > 
> > > > -----Original Message-----
> > > > From: Lazar, Lijo <Lijo.Lazar@amd.com>
> > > > Sent: Monday, December 6, 2021 11:41 AM
> > > > To: Yu, Lang <Lang.Yu@amd.com>; amd-gfx@lists.freedesktop.org
> > > > Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; Huang, Ray
> > > > <Ray.Huang@amd.com>
> > > > Subject: Re: [PATCH 2/2] drm/amdgpu: allow APU to send power gate
> > > > message when dpm is disabled
> > > > 
> > > > 
> > > > 
> > > > On 12/6/2021 8:19 AM, Yu, Lang wrote:
> > > > > [Public]
> > > > > 
> > > > > 
> > > > > 
> > > > > > -----Original Message-----
> > > > > > From: Lazar, Lijo <Lijo.Lazar@amd.com>
> > > > > > Sent: Friday, December 3, 2021 5:52 PM
> > > > > > To: Yu, Lang <Lang.Yu@amd.com>; amd-gfx@lists.freedesktop.org
> > > > > > Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; Huang, Ray
> > > > > > <Ray.Huang@amd.com>
> > > > > > Subject: Re: [PATCH 2/2] drm/amdgpu: allow APU to send power gate
> > > > > > message when dpm is disabled
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > On 12/3/2021 12:24 PM, Lang Yu wrote:
> > > > > > > The general hw fini sequence is SMU-> ... ->SDMA-> ...
> > > > > > > We need to send power gate message to power off SDMA(in SDMA
> > > > > > > hw_fini()) afer dpm is disabled(in SMU hw_fini()). Allow that for APU.
> > > > > > 
> > > > > > This message is not right. In APUs there is no message provided by
> > > > > > FW to enable/disable DPM, it is done in BIOS. Rephrase to something
> > > > > > like after smu hw_fini is completed.
> > > > > 
> > > > > It is power on/off SDMA message. Not enable/disable DPM.
> > > > > 
> > > > Bad choice of word :) I didn't mean FW message, it was about this line
> > > > in "commit message" - "afer dpm is disabled".
> > > 
> > > Ok. I got it.
> > > 
> > > > 
> > > > > > > 
> > > > > > > Signed-off-by: Lang Yu <lang.yu@amd.com>
> > > > > > > ---
> > > > > > >     drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c | 2 +-
> > > > > > >     1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > > > 
> > > > > > > diff --git a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> > > > > > > b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> > > > > > > index 2d718c30c8eb..285a237f3605 100644
> > > > > > > --- a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> > > > > > > +++ b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> > > > > > > @@ -277,7 +277,7 @@ static int smu_dpm_set_power_gate(void *handle,
> > > > > > >     	struct smu_context *smu = handle;
> > > > > > >     	int ret = 0;
> > > > > > > 
> > > > > > > -	if (!smu->pm_enabled || !smu->adev->pm.dpm_enabled) {
> > > > > > > +	if (!smu->pm_enabled || (!smu->is_apu &&
> > > > > > > +!smu->adev->pm.dpm_enabled)) {
> > > > > > 
> > > > > > 
> > > > > > This check was there before also, only the WARN is added. That means
> > > > > > it was skipping sending messages in APUs also and so far this was
> > > > > > working fine (until this gets noticed because of the warning).
> > > > > > 
> > > > > > Now this would try to send the message to APU without any check.
> > > > > > That doesn't look good. Ideal way should be to fix the sequence.
> > > > > > Otherwise, suggest to do something like below as the last step of
> > > > > > smu hw cleanup rather than sending the message blindly.
> > > > > > 
> > > > > > 	if (smu->is_apu)
> > > > > > 		smu->pm.dpm_enabled = smu_is_dpm_running(smu);
> > > > > 
> > > > > smu_is_dpm_running(smu) will cause errors in suspend.
> > > > > 
> > > > That is interesting. What is the error you get?
> > > 
> > > [drm:amdgpu_dpm_enable_uvd [amdgpu]] *ERROR* Dpm enable uvd failed, ret =
> > > -95 That means EOPNOTSUPP.
> > > 
> > > Actually, in resume process, but adev->in_suspend  is still true.
> > > For Renoir series APU, smu_is_dpm_running is hardcoded as following,
> > > 
> > > static bool renoir_is_dpm_running(struct smu_context *smu) {
> > > 	struct amdgpu_device *adev = smu->adev;
> > > 
> > > 	/*
> > > 	 * Until now, the pmfw hasn't exported the interface of SMU
> > > 	 * feature mask to APU SKU so just force on all the feature
> > > 	 * at early initial stage.
> > > 	 */
> > > 	if (adev->in_suspend)
> > > 		return false;
> > > 	else
> 
> Renoir suspend shouldn't be a special case. FW should keep running with
> features enabled after driver suspend. Could you try with a return true all
> the time for this?

That worked.

But we just send an IP power on/off message here.

Do we really need dpm running?

Regards,
Lang

> Thanks,
> Lijo
> 
> > > 		return true;
> > > 
> > > }
> > > 
> > > So we got such an error.
> > > 
> > > Regards,
> > > Lang
> > > 
> > > > Thanks,
> > > > Lijo
> > > > 
> > > > > Here we just  send some IP power on/off messages.
> > > > > Is it necessary to enable DPM to send such messages?
> > > > > 
> > > > > Regards,
> > > > > Lang
> > > > > 
> > > > > > Thanks,
> > > > > > Lijo
> > > > > > 
> > > > > > >     		dev_WARN(smu->adev->dev,
> > > > > > >     			 "SMU uninitialized but power %s requested for %u!\n",
> > > > > > >     			 gate ? "gate" : "ungate", block_type);
> > > > > > >