[Intel-xe] [PATCH v2 2/3] drm/xe/hwmon: Protect hwmon rw attributes with hwmon_lock
Gupta, Anshuman
anshuman.gupta at intel.com
Thu Oct 26 10:47:27 UTC 2023
> -----Original Message-----
> From: Nilawar, Badal <badal.nilawar at intel.com>
> Sent: Thursday, October 26, 2023 3:47 PM
> To: Gupta, Anshuman <anshuman.gupta at intel.com>; intel-
> xe at lists.freedesktop.org
> Cc: 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: Re: [PATCH v2 2/3] drm/xe/hwmon: Protect hwmon rw attributes with
> hwmon_lock
>
>
>
> 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.
Pl you can keep it in the same file.
Thanks,
Anshuman Gupta.
>
> 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