[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 > >
