OK, thanks for the advice. I will try to put pp_feature in the struct amdgpu_pm and send out the changed patch latter. Regards, Likun -----Original Message----- From: amd-gfx <amd-gfx-bounces@xxxxxxxxxxxxxxxxxxxxx> On Behalf Of Huang, Ray Sent: Tuesday, February 26, 2019 2:02 PM To: Alex Deucher <alexdeucher@xxxxxxxxx> Cc: Gao, Likun <Likun.Gao@xxxxxxx>; Wang, Kevin(Yang) <Kevin1.Wang@xxxxxxx>; Gui, Jack <Jack.Gui@xxxxxxx>; amd-gfx list <amd-gfx@xxxxxxxxxxxxxxxxxxxxx> Subject: RE: [PATCH 13/26] drm/amd/powerplay: add limit of pp_feature for smu > -----Original Message----- > From: Alex Deucher [mailto:alexdeucher@xxxxxxxxx] > Sent: Tuesday, February 26, 2019 12:08 PM > To: Huang, Ray <Ray.Huang@xxxxxxx> > Cc: amd-gfx list <amd-gfx@xxxxxxxxxxxxxxxxxxxxx>; Gao, Likun > <Likun.Gao@xxxxxxx>; Wang, Kevin(Yang) <Kevin1.Wang@xxxxxxx>; Gui, > Jack <Jack.Gui@xxxxxxx> > Subject: Re: [PATCH 13/26] drm/amd/powerplay: add limit of pp_feature > for smu > > On Mon, Feb 25, 2019 at 7:13 AM Huang Rui <ray.huang@xxxxxxx> wrote: > > > > From: Likun Gao <Likun.Gao@xxxxxxx> > > > > Move pp_feature from the struct of amd_powerplay to amdgpu_device. > > I think we can probably drop this change. If you do want to move it > out of powerplay, maybe put it in struct amdgpu_pm? I don't like > adding random stuff to amdgpu_device. While we init sw smu, the powerplay structure won't be initialized. So we move the pp_feature out of powerplay. And yes, putting it in struct amdgpu_pm is better than in struct amdgpu_device. Thanks, Ray > > > Add pp_feature limit for overdrive interface. > > > > Signed-off-by: Likun Gao <Likun.Gao@xxxxxxx> > > Reviewed-by: Kevin Wang <kevin1.wang@xxxxxxx> > > --- > > drivers/gpu/drm/amd/amdgpu/amdgpu.h | 4 +++- > > drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 2 +- > > drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c | 2 +- > > drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c | 6 ++++-- > > drivers/gpu/drm/amd/amdgpu/kv_dpm.c | 2 +- > > drivers/gpu/drm/amd/amdgpu/soc15.c | 2 +- > > drivers/gpu/drm/amd/powerplay/amd_powerplay.c | 2 +- > > drivers/gpu/drm/amd/powerplay/amdgpu_smu.c | 4 ++++ > > drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h | 3 +++ > > 9 files changed, 19 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h > > b/drivers/gpu/drm/amd/amdgpu/amdgpu.h > > index d1c02fa..f96b6e9 100644 > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h > > @@ -706,7 +706,6 @@ enum amd_hw_ip_block_type { struct > amd_powerplay > > { > > void *pp_handle; > > const struct amd_pm_funcs *pp_funcs; > > - uint32_t pp_feature; > > }; > > > > #define AMDGPU_RESET_MAGIC_NUM 64 > > @@ -842,6 +841,9 @@ struct amdgpu_device { > > /* interrupts */ > > struct amdgpu_irq irq; > > > > + /* powerplay feature */ > > + uint32_t pp_feature; > > + > > /* powerplay */ > > struct amd_powerplay powerplay; > > bool pp_force_state_enabled; > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c > > b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c > > index fcab1fe..c8fe5e5 100644 > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c > > @@ -1506,7 +1506,7 @@ static int amdgpu_device_ip_early_init(struct > amdgpu_device *adev) > > return -EAGAIN; > > } > > > > - adev->powerplay.pp_feature = amdgpu_pp_feature_mask; > > + adev->pp_feature = amdgpu_pp_feature_mask; > > > > for (i = 0; i < adev->num_ip_blocks; i++) { > > if ((amdgpu_ip_block_mask & (1 << i)) == 0) { diff > > --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c > > b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c > > index 97a60da..bcc732d 100644 > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c > > @@ -390,7 +390,7 @@ void amdgpu_gfx_compute_mqd_sw_fini(struct > > amdgpu_device *adev) > > > > void amdgpu_gfx_off_ctrl(struct amdgpu_device *adev, bool enable) { > > - if (!(adev->powerplay.pp_feature & PP_GFXOFF_MASK)) > > + if (!(adev->pp_feature & PP_GFXOFF_MASK)) > > return; > > > > if (!adev->powerplay.pp_funcs || > > !adev->powerplay.pp_funcs->set_powergating_by_smu) > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c > > b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c > > index 6bc80c1..fe1b0c4 100644 > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c > > @@ -2558,7 +2558,8 @@ int amdgpu_pm_sysfs_init(struct amdgpu_device > *adev) > > "pp_power_profile_mode\n"); > > return ret; > > } > > - if (is_support_sw_smu(adev) || hwmgr->od_enabled) { > > + if ((is_support_sw_smu(adev) && adev->smu.od_enabled) || > > + (!is_support_sw_smu(adev) && hwmgr->od_enabled)) { > > ret = device_create_file(adev->dev, > > &dev_attr_pp_od_clk_voltage); > > if (ret) { > > @@ -2634,7 +2635,8 @@ void amdgpu_pm_sysfs_fini(struct > amdgpu_device *adev) > > device_remove_file(adev->dev, &dev_attr_pp_mclk_od); > > device_remove_file(adev->dev, > > &dev_attr_pp_power_profile_mode); > > - if (hwmgr->od_enabled) > > + if ((is_support_sw_smu(adev) && adev->smu.od_enabled) || > > + (!is_support_sw_smu(adev) && hwmgr->od_enabled)) > > device_remove_file(adev->dev, > > &dev_attr_pp_od_clk_voltage); > > device_remove_file(adev->dev, &dev_attr_gpu_busy_percent); > > diff --git a/drivers/gpu/drm/amd/amdgpu/kv_dpm.c > > b/drivers/gpu/drm/amd/amdgpu/kv_dpm.c > > index 0c9a2c0..9022f42 100644 > > --- a/drivers/gpu/drm/amd/amdgpu/kv_dpm.c > > +++ b/drivers/gpu/drm/amd/amdgpu/kv_dpm.c > > @@ -2824,7 +2824,7 @@ static int kv_dpm_init(struct amdgpu_device > *adev) > > pi->caps_tcp_ramping = true; > > } > > > > - if (adev->powerplay.pp_feature & PP_SCLK_DEEP_SLEEP_MASK) > > + if (adev->pp_feature & PP_SCLK_DEEP_SLEEP_MASK) > > pi->caps_sclk_ds = true; > > else > > pi->caps_sclk_ds = false; diff --git > > a/drivers/gpu/drm/amd/amdgpu/soc15.c > > b/drivers/gpu/drm/amd/amdgpu/soc15.c > > index 9f6ce6e..c296f6c 100644 > > --- a/drivers/gpu/drm/amd/amdgpu/soc15.c > > +++ b/drivers/gpu/drm/amd/amdgpu/soc15.c > > @@ -933,7 +933,7 @@ static int soc15_common_early_init(void *handle) > > adev->pg_flags = AMD_PG_SUPPORT_SDMA | > AMD_PG_SUPPORT_VCN; > > } > > > > - if (adev->powerplay.pp_feature & PP_GFXOFF_MASK) > > + if (adev->pp_feature & PP_GFXOFF_MASK) > > adev->pg_flags |= AMD_PG_SUPPORT_GFX_PG | > > AMD_PG_SUPPORT_CP | > > AMD_PG_SUPPORT_RLC_SMU_HS; diff > > --git a/drivers/gpu/drm/amd/powerplay/amd_powerplay.c > > b/drivers/gpu/drm/amd/powerplay/amd_powerplay.c > > index 3f73f7c..ea5689a 100644 > > --- a/drivers/gpu/drm/amd/powerplay/amd_powerplay.c > > +++ b/drivers/gpu/drm/amd/powerplay/amd_powerplay.c > > @@ -53,7 +53,7 @@ static int amd_powerplay_create(struct > amdgpu_device *adev) > > mutex_init(&hwmgr->smu_lock); > > hwmgr->chip_family = adev->family; > > hwmgr->chip_id = adev->asic_type; > > - hwmgr->feature_mask = adev->powerplay.pp_feature; > > + hwmgr->feature_mask = adev->pp_feature; > > hwmgr->display_config = &adev->pm.pm_display_cfg; > > adev->powerplay.pp_handle = hwmgr; > > adev->powerplay.pp_funcs = &pp_dpm_funcs; diff --git > > a/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c > > b/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c > > index 9cb45fe..fa0af71 100644 > > --- a/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c > > +++ b/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c > > @@ -290,6 +290,9 @@ static int smu_set_funcs(struct amdgpu_device > > *adev) > > > > switch (adev->asic_type) { > > case CHIP_VEGA20: > > + smu->feature_mask &= ~PP_GFXOFF_MASK; > > + if (smu->feature_mask & PP_OVERDRIVE_MASK) > > + smu->od_enabled = true; > > smu_v11_0_set_smu_funcs(smu); > > break; > > default: > > @@ -306,6 +309,7 @@ static int smu_early_init(void *handle) > > > > smu->adev = adev; > > mutex_init(&smu->mutex); > > + smu->feature_mask = adev->pp_feature; > > > > return smu_set_funcs(adev); > > } > > diff --git a/drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h > > b/drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h > > index 00ef6f1..8c7eac9 100644 > > --- a/drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h > > +++ b/drivers/gpu/drm/amd/powerplay/inc/amdgpu_smu.h > > @@ -384,6 +384,7 @@ struct smu_context > > uint32_t pstate_sclk; > > uint32_t pstate_mclk; > > > > + bool od_enabled; > > uint32_t power_limit; > > uint32_t default_power_limit; > > > > @@ -399,6 +400,8 @@ struct smu_context > > uint32_t workload_setting[WORKLOAD_POLICY_MAX]; > > uint32_t power_profile_mode; > > uint32_t default_power_profile_mode; > > + > > + uint32_t feature_mask; > > }; > > > > struct pptable_funcs { > > -- > > 2.7.4 > > > > _______________________________________________ > > amd-gfx mailing list > > amd-gfx@xxxxxxxxxxxxxxxxxxxxx > > https://lists.freedesktop.org/mailman/listinfo/amd-gfx _______________________________________________ amd-gfx mailing list amd-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/amd-gfx _______________________________________________ amd-gfx mailing list amd-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/amd-gfx
