[PATCH] drm/amd/pm: avoid duplicate powergate/ungate setting

[Quan, Evan]

[AMD Official Use Only]



> -----Original Message-----
> From: Lazar, Lijo <Lijo.Lazar at amd.com>
> Sent: Friday, November 12, 2021 4:46 PM
> To: Quan, Evan <Evan.Quan at amd.com>; amd-gfx at lists.freedesktop.org
> Cc: Deucher, Alexander <Alexander.Deucher at amd.com>; Borislav Petkov
> <bp at suse.de>
> Subject: Re: [PATCH] drm/amd/pm: avoid duplicate powergate/ungate
> setting
> 
> 
> 
> On 11/11/2021 1:27 PM, 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
> > sometimes may cause unexpected issues.
> >
> > Link: https://lore.kernel.org/all/YV81vidWQLWvATMM@zn.tnic/
> >
> > Change-Id: I66346c69f121df0f5ee20182451313ae4fda2d04
> > Signed-off-by: Evan Quan <evan.quan at amd.com>
> > Tested-by: Borislav Petkov <bp at suse.de>
> > --
> > v1->v2:
> >    - typo fix and add link for the issue referred in commit
> >      message(Paul/Boris)
> >    - update the data type to uint32_t(Paul)
> >    - better Macro naming(Lijo)
> > ---
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_device.c |  3 +++
> >   drivers/gpu/drm/amd/include/amd_shared.h   |  3 ++-
> >   drivers/gpu/drm/amd/pm/amdgpu_dpm.c        | 12 +++++++++++-
> >   drivers/gpu/drm/amd/pm/inc/amdgpu_dpm.h    |  8 ++++++++
> >   4 files changed, 24 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > index 0bd90ec9e43e..fca592394fa1 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > @@ -3508,6 +3508,9 @@ int amdgpu_device_init(struct amdgpu_device
> *adev,
> >   		adev->rmmio_size = pci_resource_len(adev->pdev, 2);
> >   	}
> >
> > +	for (i = 0; i < AMD_IP_BLOCK_TYPE_NUM; i++)
> > +		atomic_set(&adev->pm.pwr_state[i],
> POWER_STATE_UNKNOWN);
> > +
> >   	adev->rmmio = ioremap(adev->rmmio_base, adev->rmmio_size);
> >   	if (adev->rmmio == NULL) {
> >   		return -ENOMEM;
> > 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..40fda199a86f 100644
> > --- a/drivers/gpu/drm/amd/pm/amdgpu_dpm.c
> > +++ b/drivers/gpu/drm/amd/pm/amdgpu_dpm.c
> > @@ -927,6 +927,13 @@ 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 ip_power_state pwr_state = gate ? POWER_STATE_OFF :
> > +POWER_STATE_ON;
> > +
> > +	if (atomic_read(&adev->pm.pwr_state[block_type]) == pwr_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 +983,12 @@ int
> amdgpu_dpm_set_powergating_by_smu(struct amdgpu_device *adev,
> uint32_t block
> >   		}
> >   		break;
> >   	default:
> > -		break;
> > +		return -EINVAL;
> 
> IP blocks which were not handled by PMFW were default pass before. Will
> this be a problem or return 0 here to keep the legacy behavior?
[Quan, Evan] Although I do not think there will be a problem with such change(considering there is no such IP block). I will update the patch
to stick to its original logic.

Thanks,
Evan
> 
> Thanks,
> Lijo
> 
> >   	}
> >
> > +	if (!ret)
> > +		atomic_set(&adev->pm.pwr_state[block_type], pwr_state);
> > +
> >   	return ret;
> >   }
> >
> > diff --git a/drivers/gpu/drm/amd/pm/inc/amdgpu_dpm.h
> > b/drivers/gpu/drm/amd/pm/inc/amdgpu_dpm.h
> > index 98f1b3d8c1d5..16e3f72d31b9 100644
> > --- a/drivers/gpu/drm/amd/pm/inc/amdgpu_dpm.h
> > +++ b/drivers/gpu/drm/amd/pm/inc/amdgpu_dpm.h
> > @@ -417,6 +417,12 @@ struct amdgpu_dpm {
> >   	enum amd_dpm_forced_level forced_level;
> >   };
> >
> > +enum ip_power_state {
> > +	POWER_STATE_UNKNOWN,
> > +	POWER_STATE_ON,
> > +	POWER_STATE_OFF,
> > +};
> > +
> >   struct amdgpu_pm {
> >   	struct mutex		mutex;
> >   	u32                     current_sclk;
> > @@ -452,6 +458,8 @@ struct amdgpu_pm {
> >   	struct i2c_adapter smu_i2c;
> >   	struct mutex		smu_i2c_mutex;
> >   	struct list_head	pm_attr_list;
> > +
> > +	atomic_t		pwr_state[AMD_IP_BLOCK_TYPE_NUM];
> >   };
> >
> >   #define R600_SSTU_DFLT                               0
> >