<html> <head> <meta http-equiv="Content-Type" content="text/html; charset=us-ascii"> <style type="text/css" style="display:none;"></style> </head> <body dir="ltr"> <div id="divtagdefaultwrapper" style="font-size:12pt;color:#000000;font-family:Calibri,Helvetica,sans-serif;" dir="ltr"> Seems reasonable to me. With those changes, Reviewed-by: Alex Deucher <alexander.deucher@amd.com> </div> <hr style="display:inline-block;width:98%" tabindex="-1"> <div id="divRplyFwdMsg" dir="ltr">From: Quan, Evan Sent: Tuesday, January 15, 2019 1:59:42 AM To: Alex Deucher Cc: amd-gfx list; Deucher, Alexander Subject: RE: [PATCH 4/4] drm/amd/powerplay: support retrieving and adjusting dcefclock power levels <div> </div> </div> <div class="BodyFragment"> <div class="PlainText">For ppfeatures, it will be seen only if (adev->asic_type >= CHIP_VEGA10 && !APU). Regards, Evan > -----Original Message----- > From: Quan, Evan > Sent: Tuesday, January 15, 2019 2:44 PM > To: Alex Deucher <alexdeucher@gmail.com> > Cc: amd-gfx list <amd-gfx@lists.freedesktop.org>; Deucher, Alexander > <Alexander.Deucher@amd.com> > Subject: RE: [PATCH 4/4] drm/amd/powerplay: support retrieving and > adjusting dcefclock power levels > > I think we can use asic_type to determine whether to expose these new > interfaces. > If (adev->asic_type >= CHIP_VEGA10), socclk and dcefclk are OK to expose If > (adev->asic_type >= CHIP_VEGA20), fclk is OK to expose > > Regards, > Evan > > -----Original Message----- > > From: Alex Deucher <alexdeucher@gmail.com> > > Sent: Tuesday, January 15, 2019 1:00 AM > > To: Quan, Evan <Evan.Quan@amd.com> > > Cc: amd-gfx list <amd-gfx@lists.freedesktop.org>; Deucher, Alexander > > <Alexander.Deucher@amd.com> > > Subject: Re: [PATCH 4/4] drm/amd/powerplay: support retrieving and > > adjusting dcefclock power levels > > > > On Mon, Jan 14, 2019 at 5:02 AM Evan Quan <evan.quan@amd.com> wrote: > > > > > > User can use "pp_dpm_dcefclk" to retrieve and adjust dcefclock power > > > levels. > > > > > > Change-Id: Ia3f61558ca96104c88d129ba5194103b2fe702ec > > > Signed-off-by: Evan Quan <evan.quan@amd.com> > > > > We should probably find a way to hide these new files on asics which > > don't support them. Other than that, the series looks good to me. > > > > Alex > > > > > --- > > > drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c | 53 > > ++++++++++++++++++- > > > .../gpu/drm/amd/include/kgd_pp_interface.h | 1 + > > > .../drm/amd/powerplay/hwmgr/vega20_hwmgr.c | 52 > > +++++++++++++++++- > > > 3 files changed, 103 insertions(+), 3 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c > > > b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c > > > index f6646a522c06..b7b70f590236 100644 > > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c > > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c > > > @@ -731,11 +731,13 @@ static ssize_t > > > amdgpu_get_ppfeature_status(struct device *dev, } > > > > > > /** > > > - * DOC: pp_dpm_sclk pp_dpm_mclk pp_dpm_socclk pp_dpm_fclk > > pp_dpm_pcie > > > + * DOC: pp_dpm_sclk pp_dpm_mclk pp_dpm_socclk pp_dpm_fclk > > > + pp_dpm_dcefclk > > > + * pp_dpm_pcie > > > * > > > * The amdgpu driver provides a sysfs API for adjusting what power > levels > > > * are enabled for a given power state. The files pp_dpm_sclk, > > > pp_dpm_mclk, > > > - * pp_dpm_socclk, pp_dpm_fclk and pp_dpm_pcie are used for this. > > > + * pp_dpm_socclk, pp_dpm_fclk, pp_dpm_dcefclk and pp_dpm_pcie > are > > > + used for > > > + * this. > > > * > > > * Reading back the files will show you the available power levels within > > > * the power state and the clock information for those levels. > > > @@ -745,6 +747,8 @@ static ssize_t > > > amdgpu_get_ppfeature_status(struct > > device *dev, > > > * Secondly,Enter a new value for each level by inputing a string that > > > * contains " echo xx xx xx > pp_dpm_sclk/mclk/pcie" > > > * E.g., echo 4 5 6 to > pp_dpm_sclk will enable sclk levels 4, 5, and 6. > > > + * > > > + * NOTE: change to the dcefclk max dpm level is not supported now > > > */ > > > > > > static ssize_t amdgpu_get_pp_dpm_sclk(struct device *dev, @@ -927,6 > > > +931,42 @@ static ssize_t amdgpu_set_pp_dpm_fclk(struct device *dev, > > > return count; > > > } > > > > > > +static ssize_t amdgpu_get_pp_dpm_dcefclk(struct device *dev, > > > + struct device_attribute *attr, > > > + char *buf) > > > +{ > > > + struct drm_device *ddev = dev_get_drvdata(dev); > > > + struct amdgpu_device *adev = ddev->dev_private; > > > + > > > + if (adev->powerplay.pp_funcs->print_clock_levels) > > > + return amdgpu_dpm_print_clock_levels(adev, PP_DCEFCLK, > buf); > > > + else > > > + return snprintf(buf, PAGE_SIZE, "\n"); } > > > + > > > +static ssize_t amdgpu_set_pp_dpm_dcefclk(struct device *dev, > > > + struct device_attribute *attr, > > > + const char *buf, > > > + size_t count) > > > +{ > > > + struct drm_device *ddev = dev_get_drvdata(dev); > > > + struct amdgpu_device *adev = ddev->dev_private; > > > + int ret; > > > + uint32_t mask = 0; > > > + > > > + ret = amdgpu_read_mask(buf, count, &mask); > > > + if (ret) > > > + return ret; > > > + > > > + if (adev->powerplay.pp_funcs->force_clock_level) > > > + ret = amdgpu_dpm_force_clock_level(adev, PP_DCEFCLK, > > > + mask); > > > + > > > + if (ret) > > > + return -EINVAL; > > > + > > > + return count; > > > +} > > > + > > > static ssize_t amdgpu_get_pp_dpm_pcie(struct device *dev, > > > struct device_attribute *attr, > > > char *buf) > > > @@ -1216,6 +1256,9 @@ static DEVICE_ATTR(pp_dpm_socclk, S_IRUGO | > > > S_IWUSR, static DEVICE_ATTR(pp_dpm_fclk, S_IRUGO | S_IWUSR, > > > amdgpu_get_pp_dpm_fclk, > > > amdgpu_set_pp_dpm_fclk); > > > +static DEVICE_ATTR(pp_dpm_dcefclk, S_IRUGO | S_IWUSR, > > > + amdgpu_get_pp_dpm_dcefclk, > > > + amdgpu_set_pp_dpm_dcefclk); > > > static DEVICE_ATTR(pp_dpm_pcie, S_IRUGO | S_IWUSR, > > > amdgpu_get_pp_dpm_pcie, > > > amdgpu_set_pp_dpm_pcie); @@ -2387,6 +2430,11 @@ int > > > amdgpu_pm_sysfs_init(struct amdgpu_device *adev) > > > DRM_ERROR("failed to create device file pp_dpm_fclk\n"); > > > return ret; > > > } > > > + ret = device_create_file(adev->dev, &dev_attr_pp_dpm_dcefclk); > > > + if (ret) { > > > + DRM_ERROR("failed to create device file pp_dpm_dcefclk\n"); > > > + return ret; > > > + } > > > ret = device_create_file(adev->dev, &dev_attr_pp_dpm_pcie); > > > if (ret) { > > > DRM_ERROR("failed to create device file > > > pp_dpm_pcie\n"); @@ -2474,6 +2522,7 @@ void > > amdgpu_pm_sysfs_fini(struct amdgpu_device *adev) > > > device_remove_file(adev->dev, &dev_attr_pp_dpm_socclk); > > > device_remove_file(adev->dev, &dev_attr_pp_dpm_pcie); > > > device_remove_file(adev->dev, &dev_attr_pp_dpm_fclk); > > > + device_remove_file(adev->dev, &dev_attr_pp_dpm_dcefclk); > > > device_remove_file(adev->dev, &dev_attr_pp_sclk_od); > > > device_remove_file(adev->dev, &dev_attr_pp_mclk_od); > > > device_remove_file(adev->dev, diff --git > > > a/drivers/gpu/drm/amd/include/kgd_pp_interface.h > > > b/drivers/gpu/drm/amd/include/kgd_pp_interface.h > > > index f82de14f6560..2b579ba9b685 100644 > > > --- a/drivers/gpu/drm/amd/include/kgd_pp_interface.h > > > +++ b/drivers/gpu/drm/amd/include/kgd_pp_interface.h > > > @@ -94,6 +94,7 @@ enum pp_clock_type { > > > PP_PCIE, > > > PP_SOCCLK, > > > PP_FCLK, > > > + PP_DCEFCLK, > > > OD_SCLK, > > > OD_MCLK, > > > OD_VDDC_CURVE, > > > diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/vega20_hwmgr.c > > > b/drivers/gpu/drm/amd/powerplay/hwmgr/vega20_hwmgr.c > > > index 1832dcb965b1..585046c24925 100644 > > > --- a/drivers/gpu/drm/amd/powerplay/hwmgr/vega20_hwmgr.c > > > +++ b/drivers/gpu/drm/amd/powerplay/hwmgr/vega20_hwmgr.c > > > @@ -1746,6 +1746,17 @@ static int > vega20_upload_dpm_min_level(struct > > pp_hwmgr *hwmgr, uint32_t feature_ > > > return ret); > > > } > > > > > > + if (data->smu_features[GNLD_DPM_DCEFCLK].enabled && > > > + (feature_mask & FEATURE_DPM_DCEFCLK_MASK)) { > > > + min_freq = > > > + data->dpm_table.dcef_table.dpm_state.hard_min_level; > > > + > > > + PP_ASSERT_WITH_CODE(!(ret = > > smum_send_msg_to_smc_with_parameter( > > > + hwmgr, PPSMC_MSG_SetHardMinByFreq, > > > + (PPCLK_DCEFCLK << 16) | (min_freq & 0xffff))), > > > + "Failed to set hard min dcefclk!", > > > + return ret); > > > + } > > > + > > > return ret; > > > } > > > > > > @@ -2258,7 +2269,7 @@ static int vega20_force_clock_level(struct > > pp_hwmgr *hwmgr, > > > enum pp_clock_type type, uint32_t mask) { > > > struct vega20_hwmgr *data = (struct vega20_hwmgr *)(hwmgr- > > >backend); > > > - uint32_t soft_min_level, soft_max_level; > > > + uint32_t soft_min_level, soft_max_level, hard_min_level; > > > int ret = 0; > > > > > > switch (type) { > > > @@ -2373,6 +2384,28 @@ static int vega20_force_clock_level(struct > > > pp_hwmgr *hwmgr, > > > > > > break; > > > > > > + case PP_DCEFCLK: > > > + hard_min_level = mask ? (ffs(mask) - 1) : 0; > > > + > > > + if (hard_min_level >= data->dpm_table.dcef_table.count) { > > > + pr_err("Clock level specified %d is over max allowed %d\n", > > > + hard_min_level, > > > + data->dpm_table.dcef_table.count - 1); > > > + return -EINVAL; > > > + } > > > + > > > + data->dpm_table.dcef_table.dpm_state.hard_min_level > > > + = > > > + > > > + data->dpm_table.dcef_table.dpm_levels[hard_min_level].value; > > > + > > > + ret = vega20_upload_dpm_min_level(hwmgr, > > FEATURE_DPM_DCEFCLK_MASK); > > > + PP_ASSERT_WITH_CODE(!ret, > > > + "Failed to upload boot level to lowest!", > > > + return ret); > > > + > > > + //TODO: Setting DCEFCLK max dpm level is not > > > + supported > > > + > > > + break; > > > + > > > case PP_PCIE: > > > soft_min_level = mask ? (ffs(mask) - 1) : 0; > > > soft_max_level = mask ? (fls(mask) - 1) : 0; @@ > > > -3039,6 +3072,23 @@ static int vega20_print_clock_levels(struct > > > pp_hwmgr > > *hwmgr, > > > fclk_dpm_table->dpm_levels[i].value == (now / 100) ? > "*" : > > ""); > > > break; > > > > > > + case PP_DCEFCLK: > > > + ret = vega20_get_current_clk_freq(hwmgr, > > > + PPCLK_DCEFCLK, > > &now); > > > + PP_ASSERT_WITH_CODE(!ret, > > > + "Attempt to get current dcefclk freq Failed!", > > > + return ret); > > > + > > > + ret = vega20_get_dcefclocks(hwmgr, &clocks); > > > + PP_ASSERT_WITH_CODE(!ret, > > > + "Attempt to get dcefclk levels Failed!", > > > + return ret); > > > + > > > + for (i = 0; i < clocks.num_levels; i++) > > > + size += sprintf(buf + size, "%d: %uMhz %s\n", > > > + i, clocks.data[i].clocks_in_khz / 1000, > > > + (clocks.data[i].clocks_in_khz == now * 10) ? "*" : ""); > > > + break; > > > + > > > case PP_PCIE: > > > gen_speed = (RREG32_PCIE(smnPCIE_LC_SPEED_CNTL) & > > > > > > PSWUSP0_PCIE_LC_SPEED_CNTL__LC_CURRENT_DATA_RATE_MASK) > > > -- > > > 2.20.1 > > > > > > _______________________________________________ > > > amd-gfx mailing list > > > amd-gfx@lists.freedesktop.org > > > <a href="https://lists.freedesktop.org/mailman/listinfo/amd-gfx">https://lists.freedesktop.org/mailman/listinfo/amd-gfx</a> </div> </div> </body> </html>