[AMD Official Use Only]
> -----Original Message-----
> From: Paul Menzel <pmen...@molgen.mpg.de>
> Sent: Monday, November 8, 2021 4:51 PM
> To: Quan, Evan <evan.q...@amd.com>
> Cc: Deucher, Alexander <alexander.deuc...@amd.com>; Lazar, Lijo
> <lijo.la...@amd.com>; Borislav Petkov <b...@suse.de>; amd-
> g...@lists.freedesktop.org
> Subject: Re: [PATCH] drm/amd/pm: avoid duplicate powergate/ungate
> setting
>
> Dear Evan,
>
>
> Am 08.11.21 um 05:47 schrieb Evan Quan:
> > Just bail out if the target IP block is already in the desired
> > powergate/ungate state. This can avoid some duplicate settings which
> > sometime may cause unexpected issues.
>
> sometime*s*
>
> Please elaborate the kind of issues.
>
> On what systems did you test this? Also, there is a new debug log message.
[Quan, Evan] Locally I verified this on my Polaris12 card and Boris helped to
verify on his Carrizo platform. Both show positive results.
The log message was left intentionally. But I downgraded the log level from
INFO to DEBUG to avoid confusing.
>
> > Change-Id: I66346c69f121df0f5ee20182451313ae4fda2d04
> > Signed-off-by: Evan Quan <evan.q...@amd.com>
> > Tested-by: Borislav Petkov <b...@suse.de>
> > ---
> > drivers/gpu/drm/amd/amdgpu/amdgpu.h | 7 +++++++
> > drivers/gpu/drm/amd/include/amd_shared.h | 3 ++-
> > drivers/gpu/drm/amd/pm/amdgpu_dpm.c | 13 ++++++++++++-
> > drivers/gpu/drm/amd/pm/powerplay/amd_powerplay.c | 3 +++
> > drivers/gpu/drm/amd/pm/powerplay/kv_dpm.c | 3 +++
> > drivers/gpu/drm/amd/pm/powerplay/si_dpm.c | 3 +++
> > drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c | 3 +++
> > 7 files changed, 33 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> > index b85b67a88a3d..19fa21ad8a44 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> > @@ -767,9 +767,16 @@ enum amd_hw_ip_block_type {
> > #define HW_ID_MAX 300
> > #define IP_VERSION(mj, mn, rv) (((mj) << 16) | ((mn) << 8) | (rv))
> >
> > +enum amd_ip_powergate_state {
> > + POWERGATE_STATE_INITIAL,
> > + POWERGATE_STATE_GATE,
> > + POWERGATE_STATE_UNGATE,
> > +};
> > +
> > struct amd_powerplay {
> > void *pp_handle;
> > const struct amd_pm_funcs *pp_funcs;
> > + atomic_t pg_state[AMD_IP_BLOCK_TYPE_NUM];
> > };
> >
> > /* polaris10 kickers */
> > diff --git a/drivers/gpu/drm/amd/include/amd_shared.h
> > b/drivers/gpu/drm/amd/include/amd_shared.h
> > index f1a46d16f7ea..4b9e68a79f06 100644
> > --- a/drivers/gpu/drm/amd/include/amd_shared.h
> > +++ b/drivers/gpu/drm/amd/include/amd_shared.h
> > @@ -98,7 +98,8 @@ enum amd_ip_block_type {
> > AMD_IP_BLOCK_TYPE_ACP,
> > AMD_IP_BLOCK_TYPE_VCN,
> > AMD_IP_BLOCK_TYPE_MES,
> > - AMD_IP_BLOCK_TYPE_JPEG
> > + AMD_IP_BLOCK_TYPE_JPEG,
> > + AMD_IP_BLOCK_TYPE_NUM,
> > };
> >
> > enum amd_clockgating_state {
> > diff --git a/drivers/gpu/drm/amd/pm/amdgpu_dpm.c
> > b/drivers/gpu/drm/amd/pm/amdgpu_dpm.c
> > index 03581d5b1836..a6b337b6f4a9 100644
> > --- a/drivers/gpu/drm/amd/pm/amdgpu_dpm.c
> > +++ b/drivers/gpu/drm/amd/pm/amdgpu_dpm.c
> > @@ -927,6 +927,14 @@ int
> amdgpu_dpm_set_powergating_by_smu(struct amdgpu_device *adev,
> uint32_t block
> > {
> > int ret = 0;
> > const struct amd_pm_funcs *pp_funcs = adev->powerplay.pp_funcs;
> > + enum amd_ip_powergate_state pg_state =
> > + gate ? POWERGATE_STATE_GATE :
> POWERGATE_STATE_UNGATE;
> > +
> > + if (atomic_read(&adev->powerplay.pg_state[block_type]) ==
> pg_state) {
> > + dev_dbg(adev->dev, "IP block%d already in the target %s
> state!",
> > + block_type, gate ? "gate" : "ungate");
> > + return 0;
> > + }
> >
> > switch (block_type) {
> > case AMD_IP_BLOCK_TYPE_UVD:
> > @@ -976,9 +984,12 @@ int
> amdgpu_dpm_set_powergating_by_smu(struct amdgpu_device *adev,
> uint32_t block
> > }
> > break;
> > default:
> > - break;
> > + return -EINVAL;
> > }
> >
> > + if (!ret)
> > + atomic_set(&adev->powerplay.pg_state[block_type],
> pg_state);
> > +
> > return ret;
> > }
> >
> > diff --git a/drivers/gpu/drm/amd/pm/powerplay/amd_powerplay.c
> > b/drivers/gpu/drm/amd/pm/powerplay/amd_powerplay.c
> > index bba7479f6265..8d8a7cf615eb 100644
> > --- a/drivers/gpu/drm/amd/pm/powerplay/amd_powerplay.c
> > +++ b/drivers/gpu/drm/amd/pm/powerplay/amd_powerplay.c
> > @@ -38,6 +38,7 @@ static const struct amd_pm_funcs pp_dpm_funcs;
> > static int amd_powerplay_create(struct amdgpu_device *adev)
> > {
> > struct pp_hwmgr *hwmgr;
> > + int i;
>
> I’d use `unsigned int` or `size_t` to make it clear, that they cannot get
> negative.
[Quan, Evan] Sure, I can do that.
BR
Evan
>
> >
> > if (adev == NULL)
> > return -EINVAL;
> > @@ -57,6 +58,8 @@ static int amd_powerplay_create(struct
> amdgpu_device *adev)
> > hwmgr->display_config = &adev->pm.pm_display_cfg;
> > adev->powerplay.pp_handle = hwmgr;
> > adev->powerplay.pp_funcs = &pp_dpm_funcs;
> > + for (i = 0; i < AMD_IP_BLOCK_TYPE_NUM; i++)
> > + atomic_set(&adev->powerplay.pg_state[i],
> POWERGATE_STATE_INITIAL);
> > return 0;
> > }
> >
> > diff --git a/drivers/gpu/drm/amd/pm/powerplay/kv_dpm.c
> > b/drivers/gpu/drm/amd/pm/powerplay/kv_dpm.c
> > index bcae42cef374..f84f56552fd0 100644
> > --- a/drivers/gpu/drm/amd/pm/powerplay/kv_dpm.c
> > +++ b/drivers/gpu/drm/amd/pm/powerplay/kv_dpm.c
> > @@ -2965,9 +2965,12 @@ static int kv_dpm_get_temp(void *handle)
> > static int kv_dpm_early_init(void *handle)
> > {
> > struct amdgpu_device *adev = (struct amdgpu_device *)handle;
> > + int i;
> >
> > adev->powerplay.pp_funcs = &kv_dpm_funcs;
> > adev->powerplay.pp_handle = adev;
> > + for (i = 0; i < AMD_IP_BLOCK_TYPE_NUM; i++)
> > + atomic_set(&adev->powerplay.pg_state[i],
> POWERGATE_STATE_INITIAL);
> > kv_dpm_set_irq_funcs(adev);
> >
> > return 0;
> > diff --git a/drivers/gpu/drm/amd/pm/powerplay/si_dpm.c
> > b/drivers/gpu/drm/amd/pm/powerplay/si_dpm.c
> > index 81f82aa05ec2..f1eb22c9bb59 100644
> > --- a/drivers/gpu/drm/amd/pm/powerplay/si_dpm.c
> > +++ b/drivers/gpu/drm/amd/pm/powerplay/si_dpm.c
> > @@ -7916,9 +7916,12 @@ static int si_dpm_early_init(void *handle)
> > {
> >
> > struct amdgpu_device *adev = (struct amdgpu_device *)handle;
> > + int i;
> >
> > adev->powerplay.pp_funcs = &si_dpm_funcs;
> > adev->powerplay.pp_handle = adev;
> > + for (i = 0; i < AMD_IP_BLOCK_TYPE_NUM; i++)
> > + atomic_set(&adev->powerplay.pg_state[i],
> POWERGATE_STATE_INITIAL);
> > si_dpm_set_irq_funcs(adev);
> > return 0;
> > }
> > diff --git a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> > b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> > index 01168b8955bf..fdc25bae8237 100644
> > --- a/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> > +++ b/drivers/gpu/drm/amd/pm/swsmu/amdgpu_smu.c
> > @@ -627,6 +627,7 @@ static int smu_early_init(void *handle)
> > {
> > struct amdgpu_device *adev = (struct amdgpu_device *)handle;
> > struct smu_context *smu = &adev->smu;
> > + int i;
> >
> > smu->adev = adev;
> > smu->pm_enabled = !!amdgpu_dpm;
> > @@ -639,6 +640,8 @@ static int smu_early_init(void *handle)
> >
> > adev->powerplay.pp_handle = smu;
> > adev->powerplay.pp_funcs = &swsmu_pm_funcs;
> > + for (i = 0; i < AMD_IP_BLOCK_TYPE_NUM; i++)
> > + atomic_set(&adev->powerplay.pg_state[i],
> POWERGATE_STATE_INITIAL);
> >
> > return smu_set_funcs(adev);
> > }
> >
