[AMD Official Use Only]
> -----Original Message-----
> From: Lazar, Lijo <Lijo.Lazar@xxxxxxx>
> Sent: Monday, November 8, 2021 7:16 PM
> To: Quan, Evan <Evan.Quan@xxxxxxx>; amd-gfx@xxxxxxxxxxxxxxxxxxxxx
> Cc: Deucher, Alexander <Alexander.Deucher@xxxxxxx>; Borislav Petkov
> <bp@xxxxxxx>
> Subject: Re: [PATCH] drm/amd/pm: avoid duplicate powergate/ungate
> setting
>
>
>
> On 11/8/2021 10:17 AM, Evan Quan wrote:
> > 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.
> >
> > Change-Id: I66346c69f121df0f5ee20182451313ae4fda2d04
> > Signed-off-by: Evan Quan <evan.quan@xxxxxxx>
> > Tested-by: Borislav Petkov <bp@xxxxxxx>
> > ---
> > 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,
> > +};
> > +
>
> To reflect the current state, they could be named like
> POWERGATE_STATE_UNKNOWN/ON/OFF.
[Quan, Evan] This may be more confusing. POWERGATE_STATE_ON means "gfx on" or "gate on which means gfx off"?
>
>
> > struct amd_powerplay {
> > void *pp_handle;
> > const struct amd_pm_funcs *pp_funcs;
> > + atomic_t pg_state[AMD_IP_BLOCK_TYPE_NUM];
>
> Maybe add another field in amdgpu_ip_block_status? Downside is it won't
> reflect the PG state achieved through paths other than PMFW control and
> ipblock needs to be queried through amdgpu_device_ip_get_ip_block()
[Quan, Evan] Yes, we will need to track pg state other than PMFW control then.
That will need extra effort and seems unnecessary considering there is no such use case(need to know the PG state out of PMFW control).
BR
Evan
>
> Thanks,
> Lijo
>
> > };
> >
> > /* 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;
> >
> > 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);
> > }
> >
