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@xxxxxxx>; amd-gfx@xxxxxxxxxxxxxxxxxxxxx
> > > Cc: Deucher, Alexander <Alexander.Deucher@xxxxxxx>; Huang, Ray
> > > <Ray.Huang@xxxxxxx>
> > > 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@xxxxxxx>
> > > > Sent: Monday, December 6, 2021 11:41 AM
> > > > To: Yu, Lang <Lang.Yu@xxxxxxx>; amd-gfx@xxxxxxxxxxxxxxxxxxxxx
> > > > Cc: Deucher, Alexander <Alexander.Deucher@xxxxxxx>; Huang, Ray
> > > > <Ray.Huang@xxxxxxx>
> > > > 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@xxxxxxx>
> > > > > > Sent: Friday, December 3, 2021 5:52 PM
> > > > > > To: Yu, Lang <Lang.Yu@xxxxxxx>; amd-gfx@xxxxxxxxxxxxxxxxxxxxx
> > > > > > Cc: Deucher, Alexander <Alexander.Deucher@xxxxxxx>; Huang, Ray
> > > > > > <Ray.Huang@xxxxxxx>
> > > > > > 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@xxxxxxx>
> > > > > > > ---
> > > > > > > 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);
> > > > > > >
