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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



[AMD Official Use Only]



> -----Original Message-----
> From: Lazar, Lijo <Lijo.Lazar@xxxxxxx>
> Sent: Monday, November 15, 2021 12:34 PM
> To: Quan, Evan <Evan.Quan@xxxxxxx>; amd-gfx@xxxxxxxxxxxxxxxxxxxxx
> Cc: Deucher, Alexander <Alexander.Deucher@xxxxxxx>; Borislav Petkov
> <bp@xxxxxxx>
> Subject: Re: [PATCH V3] drm/amd/pm: avoid duplicate powergate/ungate
> setting
> 
> 
> 
> On 11/15/2021 8:21 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
> > sometimes may cause unexpected issues.
> >
> > Link: https://lore.kernel.org/all/YV81vidWQLWvATMM@xxxxxxx/
> >
> > Change-Id: I66346c69f121df0f5ee20182451313ae4fda2d04
> > Signed-off-by: Evan Quan <evan.quan@xxxxxxx>
> > Tested-by: Borislav Petkov <bp@xxxxxxx>
> > --
> > 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)
> > v2->v3:
> >    - stick to original logics on handling unmentioned IP blocks(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        | 10 ++++++++++
> >   drivers/gpu/drm/amd/pm/inc/amdgpu_dpm.h    |  8 ++++++++
> >   4 files changed, 23 insertions(+), 1 deletion(-)
> >
> > 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..08362d506534 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:
> > @@ -979,6 +986,9 @@ int amdgpu_dpm_set_powergating_by_smu(struct
> amdgpu_device *adev, uint32_t block
> >   		break;
> >   	}
> >
> > +	if (!ret)
> > +		atomic_set(&adev->pm.pwr_state[block_type], pwr_state);
> > +
> 
> Once the default case is skipped, this will set the state for unhandled blocks.
> Should be fine if such blocks are not expected. 
[Quan, Evan] Yes, that's the case.

Thanks, 
Evan
> Otherwise, just return 0 such
> that they hold unknown power states.
> 
> Reviewed-by: Lijo Lazar <lijo.lazar@xxxxxxx>
> 
> Thanks,
> Lijo
> >   	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
> >




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux