The idea was to treat it like a somewhat normal file.  So every 4 bytes is a 
new sensor.  So I do "*pos >>= 2" to map an offset to an index.  Linux doesn't 
care that I modified *pos though because we're not allowing the user to read 
more than a 4 bytes at a time (so if they tried to read more than a page it 
would just fail anyways).


Tom


________________________________
From: Edward O'Callaghan <funfunc...@folklore1984.net>
Sent: Friday, September 16, 2016 01:24
To: Tom St Denis; amd-gfx@lists.freedesktop.org
Cc: StDenis, Tom
Subject: Re: [PATCH] drm/amd/amdgpu: Add sensors debugfs support



On 09/15/2016 11:22 PM, Tom St Denis wrote:
> This patch adds a callback to powerplay which
> reads specific PP sensors (vdd/clocks/load) which is then
> accessible via debugfs.  The idea being is it'll be a standard
> interface between different ASICs that userland tools can
> read.
>
> Currently only CZ/ST is supported but the others are
> NULL'ed off so they shouldn't cause any sort of oops.
>
> Signed-off-by: Tom St Denis <tom.stde...@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_device.c         | 31 +++++++
>  drivers/gpu/drm/amd/powerplay/amd_powerplay.c      | 20 +++++
>  drivers/gpu/drm/amd/powerplay/hwmgr/cz_hwmgr.c     | 96 
> ++++++++++++++++++++++
>  drivers/gpu/drm/amd/powerplay/hwmgr/fiji_hwmgr.c   |  1 +
>  .../gpu/drm/amd/powerplay/hwmgr/iceland_hwmgr.c    |  1 +
>  .../gpu/drm/amd/powerplay/hwmgr/polaris10_hwmgr.c  |  1 +
>  drivers/gpu/drm/amd/powerplay/hwmgr/tonga_hwmgr.c  |  1 +
>  drivers/gpu/drm/amd/powerplay/inc/amd_powerplay.h  | 10 +++
>  drivers/gpu/drm/amd/powerplay/inc/hwmgr.h          |  1 +
>  9 files changed, 162 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index 9103e7baf26e..b6a4588c95ee 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -2841,6 +2841,29 @@ static ssize_t amdgpu_debugfs_gca_config_read(struct 
> file *f, char __user *buf,
>        return result;
>  }
>
> +static ssize_t amdgpu_debugfs_sensor_read(struct file *f, char __user *buf,
> +                                     size_t size, loff_t *pos)
> +{
> +     struct amdgpu_device *adev = f->f_inode->i_private;
> +     int r;
> +     int32_t value;
> +
> +     if (size != 4 || *pos & 0x3)

Just some minor questions,

maybe I miss-read but maybe dereference pos the once and have it const?

> +             return -EINVAL;
> +
> +     /* convert offset to sensor number */
> +     *pos >>= 2;
Is the intent here just a local mutation or a in-place global state
transition?

Kind Regards,
Edward.

> +
> +     if (adev->powerplay.pp_funcs && adev->powerplay.pp_funcs->read_sensor)
> +             r = 
> adev->powerplay.pp_funcs->read_sensor(adev->powerplay.pp_handle, *pos, 
> &value);
> +     else
> +             r = -EINVAL;
> +
> +     if (!r)
> +             r = put_user(value, (int32_t *)buf);
> +
> +     return !r ? 4 : r;
> +}
>
>  static const struct file_operations amdgpu_debugfs_regs_fops = {
>        .owner = THIS_MODULE,
> @@ -2873,12 +2896,19 @@ static const struct file_operations 
> amdgpu_debugfs_gca_config_fops = {
>        .llseek = default_llseek
>  };
>
> +static const struct file_operations amdgpu_debugfs_sensors_fops = {
> +     .owner = THIS_MODULE,
> +     .read = amdgpu_debugfs_sensor_read,
> +     .llseek = default_llseek
> +};
> +
>  static const struct file_operations *debugfs_regs[] = {
>        &amdgpu_debugfs_regs_fops,
>        &amdgpu_debugfs_regs_didt_fops,
>        &amdgpu_debugfs_regs_pcie_fops,
>        &amdgpu_debugfs_regs_smc_fops,
>        &amdgpu_debugfs_gca_config_fops,
> +     &amdgpu_debugfs_sensors_fops,
>  };
>
>  static const char *debugfs_regs_names[] = {
> @@ -2887,6 +2917,7 @@ static const char *debugfs_regs_names[] = {
>        "amdgpu_regs_pcie",
>        "amdgpu_regs_smc",
>        "amdgpu_gca_config",
> +     "amdgpu_sensors",
>  };
>
>  static int amdgpu_debugfs_regs_init(struct amdgpu_device *adev)
> diff --git a/drivers/gpu/drm/amd/powerplay/amd_powerplay.c 
> b/drivers/gpu/drm/amd/powerplay/amd_powerplay.c
> index b1d19409bf86..ee0368381e82 100644
> --- a/drivers/gpu/drm/amd/powerplay/amd_powerplay.c
> +++ b/drivers/gpu/drm/amd/powerplay/amd_powerplay.c
> @@ -894,6 +894,25 @@ static int pp_dpm_set_mclk_od(void *handle, uint32_t 
> value)
>        return hwmgr->hwmgr_func->set_mclk_od(hwmgr, value);
>  }
>
> +static int pp_dpm_read_sensor(void *handle, int idx, int32_t *value)
> +{
> +     struct pp_hwmgr *hwmgr;
> +
> +     if (!handle)
> +             return -EINVAL;
> +
> +     hwmgr = ((struct pp_instance *)handle)->hwmgr;
> +
> +     PP_CHECK_HW(hwmgr);
> +
> +     if (hwmgr->hwmgr_func->read_sensor == NULL) {
> +             printk(KERN_INFO "%s was not implemented.\n", __func__);
> +             return 0;
> +     }
> +
> +     return hwmgr->hwmgr_func->read_sensor(hwmgr, idx, value);
> +}
> +
>  const struct amd_powerplay_funcs pp_dpm_funcs = {
>        .get_temperature = pp_dpm_get_temperature,
>        .load_firmware = pp_dpm_load_fw,
> @@ -920,6 +939,7 @@ const struct amd_powerplay_funcs pp_dpm_funcs = {
>        .set_sclk_od = pp_dpm_set_sclk_od,
>        .get_mclk_od = pp_dpm_get_mclk_od,
>        .set_mclk_od = pp_dpm_set_mclk_od,
> +     .read_sensor = pp_dpm_read_sensor,
>  };
>
>  static int amd_pp_instance_init(struct amd_pp_init *pp_init,
> diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/cz_hwmgr.c 
> b/drivers/gpu/drm/amd/powerplay/hwmgr/cz_hwmgr.c
> index 5ecef1732e20..9f3c5a8a903c 100644
> --- a/drivers/gpu/drm/amd/powerplay/hwmgr/cz_hwmgr.c
> +++ b/drivers/gpu/drm/amd/powerplay/hwmgr/cz_hwmgr.c
> @@ -1857,6 +1857,101 @@ static int cz_get_max_high_clocks(struct pp_hwmgr 
> *hwmgr, struct amd_pp_simple_c
>        return 0;
>  }
>
> +static int cz_read_sensor(struct pp_hwmgr *hwmgr, int idx, int32_t *value)
> +{
> +     struct cz_hwmgr *cz_hwmgr = (struct cz_hwmgr *)(hwmgr->backend);
> +
> +     struct phm_clock_voltage_dependency_table *table =
> +                             hwmgr->dyn_state.vddc_dependency_on_sclk;
> +
> +     struct phm_vce_clock_voltage_dependency_table *vce_table =
> +             hwmgr->dyn_state.vce_clock_voltage_dependency_table;
> +
> +     struct phm_uvd_clock_voltage_dependency_table *uvd_table =
> +             hwmgr->dyn_state.uvd_clock_voltage_dependency_table;
> +
> +     uint32_t sclk_index = 
> PHM_GET_FIELD(cgs_read_ind_register(hwmgr->device, CGS_IND_REG__SMC, 
> ixTARGET_AND_CURRENT_PROFILE_INDEX),
> +                                     TARGET_AND_CURRENT_PROFILE_INDEX, 
> CURR_SCLK_INDEX);
> +     uint32_t uvd_index = PHM_GET_FIELD(cgs_read_ind_register(hwmgr->device, 
> CGS_IND_REG__SMC, ixTARGET_AND_CURRENT_PROFILE_INDEX_2),
> +                                     TARGET_AND_CURRENT_PROFILE_INDEX_2, 
> CURR_UVD_INDEX);
> +     uint32_t vce_index = PHM_GET_FIELD(cgs_read_ind_register(hwmgr->device, 
> CGS_IND_REG__SMC, ixTARGET_AND_CURRENT_PROFILE_INDEX_2),
> +                                     TARGET_AND_CURRENT_PROFILE_INDEX_2, 
> CURR_VCE_INDEX);
> +
> +     uint32_t sclk, vclk, dclk, ecclk, tmp, activity_percent;
> +     uint16_t vddnb, vddgfx;
> +     int result;
> +
> +     switch (idx) {
> +     case AMDGPU_PP_SENSOR_GFX_SCLK:
> +             if (sclk_index < NUM_SCLK_LEVELS) {
> +                     sclk = table->entries[sclk_index].clk;
> +                     *value = sclk;
> +                     return 0;
> +             }
> +             return -EINVAL;
> +     case AMDGPU_PP_SENSOR_VDDNB:
> +             tmp = (cgs_read_ind_register(hwmgr->device, CGS_IND_REG__SMC, 
> ixSMUSVI_NB_CURRENTVID) &
> +                     CURRENT_NB_VID_MASK) >> CURRENT_NB_VID__SHIFT;
> +             vddnb = cz_convert_8Bit_index_to_voltage(hwmgr, tmp);
> +             *value = vddnb;
> +             return 0;
> +     case AMDGPU_PP_SENSOR_VDDGFX:
> +             tmp = (cgs_read_ind_register(hwmgr->device, CGS_IND_REG__SMC, 
> ixSMUSVI_GFX_CURRENTVID) &
> +                     CURRENT_GFX_VID_MASK) >> CURRENT_GFX_VID__SHIFT;
> +             vddgfx = cz_convert_8Bit_index_to_voltage(hwmgr, (u16)tmp);
> +             *value = vddgfx;
> +             return 0;
> +     case AMDGPU_PP_SENSOR_UVD_VCLK:
> +             if (!cz_hwmgr->uvd_power_gated) {
> +                     if (uvd_index >= CZ_MAX_HARDWARE_POWERLEVELS) {
> +                             return -EINVAL;
> +                     } else {
> +                             vclk = uvd_table->entries[uvd_index].vclk;
> +                             *value = vclk;
> +                             return 0;
> +                     }
> +             }
> +             *value = 0;
> +             return 0;
> +     case AMDGPU_PP_SENSOR_UVD_DCLK:
> +             if (!cz_hwmgr->uvd_power_gated) {
> +                     if (uvd_index >= CZ_MAX_HARDWARE_POWERLEVELS) {
> +                             return -EINVAL;
> +                     } else {
> +                             dclk = uvd_table->entries[uvd_index].dclk;
> +                             *value = dclk;
> +                             return 0;
> +                     }
> +             }
> +             *value = 0;
> +             return 0;
> +     case AMDGPU_PP_SENSOR_VCE_ECCLK:
> +             if (!cz_hwmgr->vce_power_gated) {
> +                     if (vce_index >= CZ_MAX_HARDWARE_POWERLEVELS) {
> +                             return -EINVAL;
> +                     } else {
> +                             ecclk = vce_table->entries[vce_index].ecclk;
> +                             *value = ecclk;
> +                             return 0;
> +                     }
> +             }
> +             *value = 0;
> +             return 0;
> +     case AMDGPU_PP_SENSOR_GPU_LOAD:
> +             result = smum_send_msg_to_smc(hwmgr->smumgr, 
> PPSMC_MSG_GetAverageGraphicsActivity);
> +             if (0 == result) {
> +                     activity_percent = cgs_read_register(hwmgr->device, 
> mmSMU_MP1_SRBM2P_ARG_0);
> +                     activity_percent = activity_percent > 100 ? 100 : 
> activity_percent;
> +             } else {
> +                     activity_percent = 50;
> +             }
> +             *value = activity_percent;
> +             return 0;
> +     default:
> +             return -EINVAL;
> +     }
> +}
> +
>  static const struct pp_hwmgr_func cz_hwmgr_funcs = {
>        .backend_init = cz_hwmgr_backend_init,
>        .backend_fini = cz_hwmgr_backend_fini,
> @@ -1882,6 +1977,7 @@ static const struct pp_hwmgr_func cz_hwmgr_funcs = {
>        .get_current_shallow_sleep_clocks = 
> cz_get_current_shallow_sleep_clocks,
>        .get_clock_by_type = cz_get_clock_by_type,
>        .get_max_high_clocks = cz_get_max_high_clocks,
> +     .read_sensor = cz_read_sensor,
>  };
>
>  int cz_hwmgr_init(struct pp_hwmgr *hwmgr)
> diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/fiji_hwmgr.c 
> b/drivers/gpu/drm/amd/powerplay/hwmgr/fiji_hwmgr.c
> index 0d4c99b9e3f9..c64def1884c9 100644
> --- a/drivers/gpu/drm/amd/powerplay/hwmgr/fiji_hwmgr.c
> +++ b/drivers/gpu/drm/amd/powerplay/hwmgr/fiji_hwmgr.c
> @@ -5590,6 +5590,7 @@ static const struct pp_hwmgr_func fiji_hwmgr_funcs = {
>        .set_sclk_od = fiji_set_sclk_od,
>        .get_mclk_od = fiji_get_mclk_od,
>        .set_mclk_od = fiji_set_mclk_od,
> +     .read_sensor = NULL,
>  };
>
>  int fiji_hwmgr_init(struct pp_hwmgr *hwmgr)
> diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/iceland_hwmgr.c 
> b/drivers/gpu/drm/amd/powerplay/hwmgr/iceland_hwmgr.c
> index 5abe43360ec0..d7a1410402d4 100644
> --- a/drivers/gpu/drm/amd/powerplay/hwmgr/iceland_hwmgr.c
> +++ b/drivers/gpu/drm/amd/powerplay/hwmgr/iceland_hwmgr.c
> @@ -5663,6 +5663,7 @@ static const struct pp_hwmgr_func iceland_hwmgr_funcs = 
> {
>        .set_sclk_od = iceland_set_sclk_od,
>        .get_mclk_od = iceland_get_mclk_od,
>        .set_mclk_od = iceland_set_mclk_od,
> +     .read_sensor = NULL,
>  };
>
>  int iceland_hwmgr_init(struct pp_hwmgr *hwmgr)
> diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/polaris10_hwmgr.c 
> b/drivers/gpu/drm/amd/powerplay/hwmgr/polaris10_hwmgr.c
> index 970e3930452d..8da1d0f66240 100644
> --- a/drivers/gpu/drm/amd/powerplay/hwmgr/polaris10_hwmgr.c
> +++ b/drivers/gpu/drm/amd/powerplay/hwmgr/polaris10_hwmgr.c
> @@ -5278,6 +5278,7 @@ static const struct pp_hwmgr_func polaris10_hwmgr_funcs 
> = {
>        .set_sclk_od = polaris10_set_sclk_od,
>        .get_mclk_od = polaris10_get_mclk_od,
>        .set_mclk_od = polaris10_set_mclk_od,
> +     .read_sensor = NULL,
>  };
>
>  int polaris10_hwmgr_init(struct pp_hwmgr *hwmgr)
> diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/tonga_hwmgr.c 
> b/drivers/gpu/drm/amd/powerplay/hwmgr/tonga_hwmgr.c
> index 42783bf7647c..a5175ea5bb46 100644
> --- a/drivers/gpu/drm/amd/powerplay/hwmgr/tonga_hwmgr.c
> +++ b/drivers/gpu/drm/amd/powerplay/hwmgr/tonga_hwmgr.c
> @@ -6359,6 +6359,7 @@ static const struct pp_hwmgr_func tonga_hwmgr_funcs = {
>        .set_sclk_od = tonga_set_sclk_od,
>        .get_mclk_od = tonga_get_mclk_od,
>        .set_mclk_od = tonga_set_mclk_od,
> +     .read_sensor = NULL,
>  };
>
>  int tonga_hwmgr_init(struct pp_hwmgr *hwmgr)
> diff --git a/drivers/gpu/drm/amd/powerplay/inc/amd_powerplay.h 
> b/drivers/gpu/drm/amd/powerplay/inc/amd_powerplay.h
> index 18f39e89a7aa..bafb593b568a 100644
> --- a/drivers/gpu/drm/amd/powerplay/inc/amd_powerplay.h
> +++ b/drivers/gpu/drm/amd/powerplay/inc/amd_powerplay.h
> @@ -29,6 +29,15 @@
>  #include "amd_shared.h"
>  #include "cgs_common.h"
>
> +enum amd_pp_sensors {
> +     AMDGPU_PP_SENSOR_GFX_SCLK = 0,
> +     AMDGPU_PP_SENSOR_VDDNB,
> +     AMDGPU_PP_SENSOR_VDDGFX,
> +     AMDGPU_PP_SENSOR_UVD_VCLK,
> +     AMDGPU_PP_SENSOR_UVD_DCLK,
> +     AMDGPU_PP_SENSOR_VCE_ECCLK,
> +     AMDGPU_PP_SENSOR_GPU_LOAD,
> +};
>
>  enum amd_pp_event {
>        AMD_PP_EVENT_INITIALIZE = 0,
> @@ -346,6 +355,7 @@ struct amd_powerplay_funcs {
>        int (*set_sclk_od)(void *handle, uint32_t value);
>        int (*get_mclk_od)(void *handle);
>        int (*set_mclk_od)(void *handle, uint32_t value);
> +     int (*read_sensor)(void *handle, int idx, int32_t *value);
>  };
>
>  struct amd_powerplay {
> diff --git a/drivers/gpu/drm/amd/powerplay/inc/hwmgr.h 
> b/drivers/gpu/drm/amd/powerplay/inc/hwmgr.h
> index e98748344801..310ba0ce2934 100644
> --- a/drivers/gpu/drm/amd/powerplay/inc/hwmgr.h
> +++ b/drivers/gpu/drm/amd/powerplay/inc/hwmgr.h
> @@ -359,6 +359,7 @@ struct pp_hwmgr_func {
>        int (*set_sclk_od)(struct pp_hwmgr *hwmgr, uint32_t value);
>        int (*get_mclk_od)(struct pp_hwmgr *hwmgr);
>        int (*set_mclk_od)(struct pp_hwmgr *hwmgr, uint32_t value);
> +     int (*read_sensor)(struct pp_hwmgr *hwmgr, int idx, int32_t *value);
>  };
>
>  struct pp_table_func {
>

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Reply via email to