[Intel-xe] [PATCH v2 2/3] drm/xe/hwmon: Protect hwmon rw attributes with hwmon_lock
Nilawar, Badal
badal.nilawar at intel.com
Thu Oct 26 10:17:01 UTC 2023
On 19-10-2023 12:04, Gupta, Anshuman wrote:
>
>
>> -----Original Message-----
>> From: Nilawar, Badal <badal.nilawar at intel.com>
>> Sent: Thursday, October 19, 2023 12:34 AM
>> To: intel-xe at lists.freedesktop.org
>> Cc: Gupta, Anshuman <anshuman.gupta at intel.com>; Dixit, Ashutosh
>> <ashutosh.dixit at intel.com>; andi.shyti at linux.intel.com; Tauro, Riana
>> <riana.tauro at intel.com>; Vivi, Rodrigo <rodrigo.vivi at intel.com>
>> Subject: [PATCH v2 2/3] drm/xe/hwmon: Protect hwmon rw attributes with
>> hwmon_lock
>>
>> Take hwmon_lock while accessing hwmon rw attributes. For readonly
>> attributes its not required to take lock as reads are protected by sysfs layer and
>> therefore sequential.
> This patch should only have functionality related to locking, why this is adding
> xe_hwmon_power_curr_crit_write and xe_hwmon_power_curr_crit_read ?
Sure, will add these functions to refactor patch.
>>
>> Cc: Ashutosh Dixit <ashutosh.dixit at intel.com>
>> Cc: Anshuman Gupta <anshuman.gupta at intel.com>
>> Signed-off-by: Badal Nilawar <badal.nilawar at intel.com>
>> ---
>> drivers/gpu/drm/xe/xe_hwmon.c | 92 ++++++++++++++++++++--------------
>> -
>> 1 file changed, 52 insertions(+), 40 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_hwmon.c
>> b/drivers/gpu/drm/xe/xe_hwmon.c index 08c7289723ae..c87d028f1955
>> 100644
>> --- a/drivers/gpu/drm/xe/xe_hwmon.c
>> +++ b/drivers/gpu/drm/xe/xe_hwmon.c
>> @@ -47,7 +47,7 @@ struct xe_hwmon_energy_info { struct xe_hwmon {
>> struct device *hwmon_dev;
>> struct xe_gt *gt;
>> - struct mutex hwmon_lock; /* rmw operations*/
>> + struct mutex hwmon_lock; /* For rw attributes */
> Make this Kernel Doc comment, and this is wrongly placed here in .c , structure are declared
> In headers in all xe files, and requires kernel doc comment.
Sure will add kernel doc comment. Struct added here as it is used only
in this file. Infact in drivers/gpu/drm/xe/xe_mocs.c has structures
defined in it.
Regards,
Badal
> Thanks,
> Anshuman Gupta.
>> int scl_shift_power;
>> int scl_shift_energy;
>> struct xe_hwmon_energy_info ei; /* Energy info for
>> energy1_input */
>> @@ -135,11 +135,13 @@ static void xe_hwmon_power_max_read(struct
>> xe_hwmon *hwmon, long *value) {
>> u64 reg_val, min, max;
>>
>> + mutex_lock(&hwmon->hwmon_lock);
>> +
>> xe_hwmon_process_reg(hwmon, REG_PKG_RAPL_LIMIT,
>> REG_READ32, ®_val, 0, 0);
>> /* Check if PL1 limit is disabled */
>> if (!(reg_val & PKG_PWR_LIM_1_EN)) {
>> *value = PL1_DISABLE;
>> - return;
>> + goto unlock;
>> }
>>
>> reg_val = REG_FIELD_GET(PKG_PWR_LIM_1, reg_val); @@ -153,12
>> +155,17 @@ static void xe_hwmon_power_max_read(struct xe_hwmon
>> *hwmon, long *value)
>>
>> if (min && max)
>> *value = clamp_t(u64, *value, min, max);
>> +unlock:
>> + mutex_unlock(&hwmon->hwmon_lock);
>> }
>>
>> static int xe_hwmon_power_max_write(struct xe_hwmon *hwmon, long
>> value) {
>> + int ret = 0;
>> u64 reg_val;
>>
>> + mutex_lock(&hwmon->hwmon_lock);
>> +
>> /* Disable PL1 limit and verify, as limit cannot be disabled on all
>> platforms */
>> if (value == PL1_DISABLE) {
>> xe_hwmon_process_reg(hwmon, REG_PKG_RAPL_LIMIT,
>> REG_RMW32, ®_val, @@ -166,8 +173,10 @@ static int
>> xe_hwmon_power_max_write(struct xe_hwmon *hwmon, long value)
>> xe_hwmon_process_reg(hwmon, REG_PKG_RAPL_LIMIT,
>> REG_READ32, ®_val,
>> PKG_PWR_LIM_1_EN, 0);
>>
>> - if (reg_val & PKG_PWR_LIM_1_EN)
>> - return -EOPNOTSUPP;
>> + if (reg_val & PKG_PWR_LIM_1_EN) {
>> + ret = -EOPNOTSUPP;
>> + goto unlock;
>> + }
>> }
>>
>> /* Computation in 64-bits to avoid overflow. Round to nearest. */
>> @@ -176,8 +185,9 @@ static int xe_hwmon_power_max_write(struct
>> xe_hwmon *hwmon, long value)
>>
>> xe_hwmon_process_reg(hwmon, REG_PKG_RAPL_LIMIT,
>> REG_RMW32, ®_val,
>> PKG_PWR_LIM_1_EN | PKG_PWR_LIM_1, reg_val);
>> -
>> - return 0;
>> +unlock:
>> + mutex_unlock(&hwmon->hwmon_lock);
>> + return ret;
>> }
>>
>> static void xe_hwmon_power_rated_max_read(struct xe_hwmon *hwmon,
>> long *value) @@ -215,8 +225,6 @@ xe_hwmon_energy_get(struct
>> xe_hwmon *hwmon, long *energy)
>> struct xe_hwmon_energy_info *ei = &hwmon->ei;
>> u64 reg_val;
>>
>> - mutex_lock(&hwmon->hwmon_lock);
>> -
>> xe_hwmon_process_reg(hwmon, REG_PKG_ENERGY_STATUS,
>> REG_READ32,
>> ®_val, 0, 0);
>>
>> @@ -229,8 +237,6 @@ xe_hwmon_energy_get(struct xe_hwmon *hwmon,
>> long *energy)
>>
>> *energy = mul_u64_u32_shr(ei->accum_energy, SF_ENERGY,
>> hwmon->scl_shift_energy);
>> -
>> - mutex_unlock(&hwmon->hwmon_lock);
>> }
>>
>> static const struct hwmon_channel_info *hwmon_info[] = { @@ -260,6
>> +266,38 @@ static int xe_hwmon_pcode_write_i1(struct xe_gt *gt, u32 uval)
>> uval);
>> }
>>
>> +static int xe_hwmon_power_curr_crit_read(struct xe_hwmon *hwmon, long
>> +*value, u32 scale_factor) {
>> + int ret;
>> + u32 uval;
>> +
>> + mutex_lock(&hwmon->hwmon_lock);
>> +
>> + ret = xe_hwmon_pcode_read_i1(hwmon->gt, &uval);
>> + if (ret)
>> + goto unlock;
>> +
>> + *value =
>> mul_u64_u32_shr(REG_FIELD_GET(POWER_SETUP_I1_DATA_MASK, uval),
>> + scale_factor, POWER_SETUP_I1_SHIFT);
>> +unlock:
>> + mutex_unlock(&hwmon->hwmon_lock);
>> + return ret;
>> +}
>> +
>> +static int xe_hwmon_power_curr_crit_write(struct xe_hwmon *hwmon, long
>> +value, u32 scale_factor) {
>> + int ret;
>> + u32 uval;
>> +
>> + mutex_lock(&hwmon->hwmon_lock);
>> +
>> + uval = DIV_ROUND_CLOSEST_ULL(value << POWER_SETUP_I1_SHIFT,
>> scale_factor);
>> + ret = xe_hwmon_pcode_write_i1(hwmon->gt, uval);
>> +
>> + mutex_unlock(&hwmon->hwmon_lock);
>> + return ret;
>> +}
>> +
>> static void xe_hwmon_get_voltage(struct xe_hwmon *hwmon, long *value) {
>> u64 reg_val;
>> @@ -291,9 +329,6 @@ xe_hwmon_power_is_visible(struct xe_hwmon
>> *hwmon, u32 attr, int chan) static int xe_hwmon_power_read(struct
>> xe_hwmon *hwmon, u32 attr, int chan, long *val) {
>> - int ret;
>> - u32 uval;
>> -
>> switch (attr) {
>> case hwmon_power_max:
>> xe_hwmon_power_max_read(hwmon, val);
>> @@ -302,14 +337,7 @@ xe_hwmon_power_read(struct xe_hwmon
>> *hwmon, u32 attr, int chan, long *val)
>> xe_hwmon_power_rated_max_read(hwmon, val);
>> return 0;
>> case hwmon_power_crit:
>> - ret = xe_hwmon_pcode_read_i1(hwmon->gt, &uval);
>> - if (ret)
>> - return ret;
>> - if (!(uval & POWER_SETUP_I1_WATTS))
>> - return -ENODEV;
>> - *val =
>> mul_u64_u32_shr(REG_FIELD_GET(POWER_SETUP_I1_DATA_MASK, uval),
>> - SF_POWER, POWER_SETUP_I1_SHIFT);
>> - return 0;
>> + return xe_hwmon_power_curr_crit_read(hwmon, val,
>> SF_POWER);
>> default:
>> return -EOPNOTSUPP;
>> }
>> @@ -318,14 +346,11 @@ xe_hwmon_power_read(struct xe_hwmon
>> *hwmon, u32 attr, int chan, long *val) static int
>> xe_hwmon_power_write(struct xe_hwmon *hwmon, u32 attr, int chan, long
>> val) {
>> - u32 uval;
>> -
>> switch (attr) {
>> case hwmon_power_max:
>> return xe_hwmon_power_max_write(hwmon, val);
>> case hwmon_power_crit:
>> - uval = DIV_ROUND_CLOSEST_ULL(val <<
>> POWER_SETUP_I1_SHIFT, SF_POWER);
>> - return xe_hwmon_pcode_write_i1(hwmon->gt, uval);
>> + return xe_hwmon_power_curr_crit_write(hwmon, val,
>> SF_POWER);
>> default:
>> return -EOPNOTSUPP;
>> }
>> @@ -348,19 +373,9 @@ xe_hwmon_curr_is_visible(const struct xe_hwmon
>> *hwmon, u32 attr) static int xe_hwmon_curr_read(struct xe_hwmon
>> *hwmon, u32 attr, long *val) {
>> - int ret;
>> - u32 uval;
>> -
>> switch (attr) {
>> case hwmon_curr_crit:
>> - ret = xe_hwmon_pcode_read_i1(hwmon->gt, &uval);
>> - if (ret)
>> - return ret;
>> - if (uval & POWER_SETUP_I1_WATTS)
>> - return -ENODEV;
>> - *val =
>> mul_u64_u32_shr(REG_FIELD_GET(POWER_SETUP_I1_DATA_MASK, uval),
>> - SF_CURR, POWER_SETUP_I1_SHIFT);
>> - return 0;
>> + return xe_hwmon_power_curr_crit_read(hwmon, val,
>> SF_CURR);
>> default:
>> return -EOPNOTSUPP;
>> }
>> @@ -369,12 +384,9 @@ xe_hwmon_curr_read(struct xe_hwmon *hwmon,
>> u32 attr, long *val) static int xe_hwmon_curr_write(struct xe_hwmon
>> *hwmon, u32 attr, long val) {
>> - u32 uval;
>> -
>> switch (attr) {
>> case hwmon_curr_crit:
>> - uval = DIV_ROUND_CLOSEST_ULL(val <<
>> POWER_SETUP_I1_SHIFT, SF_CURR);
>> - return xe_hwmon_pcode_write_i1(hwmon->gt, uval);
>> + return xe_hwmon_power_curr_crit_write(hwmon, val,
>> SF_CURR);
>> default:
>> return -EOPNOTSUPP;
>> }
>> --
>> 2.25.1
>
More information about the Intel-xe
mailing list