| Message ID | 20230925081842.3566834-6-badal.nilawar@intel.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | Add HWMON support for DGFX | expand |
Hi Badal, [...] > +static ssize_t > +xe_hwmon_power1_max_interval_store(struct device *dev, struct device_attribute *attr, > + const char *buf, size_t count) > +{ > + struct xe_hwmon *hwmon = dev_get_drvdata(dev); > + u32 x, y, rxy, x_w = 2; /* 2 bits */ > + u64 tau4, r, max_win; > + unsigned long val; > + int ret; > + > + ret = kstrtoul(buf, 0, &val); > + if (ret) > + return ret; > + > + /* > + * Max HW supported tau in '1.x * power(2,y)' format, x = 0, y = 0x12 > + * The hwmon->scl_shift_time default of 0xa results in a max tau of 256 seconds > + */ > +#define PKG_MAX_WIN_DEFAULT 0x12ull > + > + /* > + * val must be < max in hwmon interface units. The steps below are > + * explained in xe_hwmon_power1_max_interval_show() > + */ > + r = FIELD_PREP(PKG_MAX_WIN, PKG_MAX_WIN_DEFAULT); > + x = REG_FIELD_GET(PKG_MAX_WIN_X, r); > + y = REG_FIELD_GET(PKG_MAX_WIN_Y, r); > + tau4 = ((1 << x_w) | x) << y; > + max_win = mul_u64_u32_shr(tau4, SF_TIME, hwmon->scl_shift_time + x_w); > + > + if (val > max_win) > + return -EINVAL; > + > + /* val in hw units */ > + val = DIV_ROUND_CLOSEST_ULL((u64)val << hwmon->scl_shift_time, SF_TIME); > + /* Convert to 1.x * power(2,y) */ > + if (!val) { > + /* Avoid ilog2(0) */ > + y = 0; > + x = 0; > + } else { > + y = ilog2(val); > + /* x = (val - (1 << y)) >> (y - 2); */ this is some spurious development comment, can you please remove it? > + x = (val - (1ul << y)) << x_w >> y; > + } > + > + rxy = REG_FIELD_PREP(PKG_PWR_LIM_1_TIME_X, x) | REG_FIELD_PREP(PKG_PWR_LIM_1_TIME_Y, y); > + > + xe_device_mem_access_get(gt_to_xe(hwmon->gt)); > + > + mutex_lock(&hwmon->hwmon_lock); > + > + xe_hwmon_process_reg(hwmon, REG_PKG_RAPL_LIMIT, REG_RMW, (u32 *)&r, > + PKG_PWR_LIM_1_TIME, rxy); > + > + mutex_unlock(&hwmon->hwmon_lock); why are we locking here? Andi > + > + xe_device_mem_access_put(gt_to_xe(hwmon->gt)); > + > + return count; > +}
Hi Badal, > > > + /* val in hw units */ > > > + val = DIV_ROUND_CLOSEST_ULL((u64)val << hwmon->scl_shift_time, SF_TIME); > > > + /* Convert to 1.x * power(2,y) */ > > > + if (!val) { > > > + /* Avoid ilog2(0) */ > > > + y = 0; > > > + x = 0; > > > + } else { > > > + y = ilog2(val); > > > + /* x = (val - (1 << y)) >> (y - 2); */ > > > > this is some spurious development comment, can you please remove > > it? > > This is kept intentionally to help to understand the calculations. then this is confusing... Can you please expand the concept? As it is it's not understandable and I would expect someone sending a patch with title: [PATCH] drm/xe/hwmon: Remove spurious comment Because it just looks forgotten from previous development. > > > + x = (val - (1ul << y)) << x_w >> y; > > > + } > > > + > > > + rxy = REG_FIELD_PREP(PKG_PWR_LIM_1_TIME_X, x) | REG_FIELD_PREP(PKG_PWR_LIM_1_TIME_Y, y); > > > + > > > + xe_device_mem_access_get(gt_to_xe(hwmon->gt)); > > > + > > > + mutex_lock(&hwmon->hwmon_lock); > > > + > > > + xe_hwmon_process_reg(hwmon, REG_PKG_RAPL_LIMIT, REG_RMW, (u32 *)&r, > > > + PKG_PWR_LIM_1_TIME, rxy); > > > + > > > + mutex_unlock(&hwmon->hwmon_lock); > > > > why are we locking here? > > Since it is rmw operation we are using lock here. OK... so what you are trying to protect here is the read -> update -> write and it makes sense. The problem is that if this is a generic rule, which means that everyone who will do a rmw operation has to take the lock, why not take the lock directly in xe_hwmon_process_reg()? But also this can be a bit confusing, because a function is either locked or unlocked and purists might complain. A suggestion would be to do something like: static int xe_hwmon_process_reg(..., enum xe_hwmon_reg_operation operation) { ... } static int xe_hwmon_reg_read(...); { return xe_hwmon_process_reg(..., REG_READ); } static int xe_hwmon_reg_write(...); { return xe_hwmon_process_reg(..., REG_WRITE); } static int xe_hwmon_reg_rmw(...); { int ret; /* * Optional: you can check that the lock is not taken * to shout loud if potential deadlocks arise. */ /* * We want to protect the register update with the * lock blah blah blah... explanatory comment. */ mutex_lock(&hwmon->hwmon_lock); ret = xe_hwmon_process_reg(..., REG_RMW); mutex_unlock(&hwmon->hwmon_lock); return ret; } What do you think? It looks much clearer to me. Andi
Hi Andi, On 26-09-2023 13:31, Andi Shyti wrote: > Hi Badal, > >>>> + /* val in hw units */ >>>> + val = DIV_ROUND_CLOSEST_ULL((u64)val << hwmon->scl_shift_time, SF_TIME); >>>> + /* Convert to 1.x * power(2,y) */ >>>> + if (!val) { >>>> + /* Avoid ilog2(0) */ >>>> + y = 0; >>>> + x = 0; >>>> + } else { >>>> + y = ilog2(val); >>>> + /* x = (val - (1 << y)) >> (y - 2); */ >>> >>> this is some spurious development comment, can you please remove >>> it? >> >> This is kept intentionally to help to understand the calculations. > > then this is confusing... Can you please expand the concept? > As it is it's not understandable and I would expect someone > sending a patch with title: > > [PATCH] drm/xe/hwmon: Remove spurious comment > > Because it just looks forgotten from previous development. I will add this comment inside the comment at the top of if. So it will look like. /* * Convert to 1.x * power(2,y) * y = ilog(val); * x = (val - (1 << y)) >> (y-2); */ > >>>> + x = (val - (1ul << y)) << x_w >> y; >>>> + } >>>> + >>>> + rxy = REG_FIELD_PREP(PKG_PWR_LIM_1_TIME_X, x) | REG_FIELD_PREP(PKG_PWR_LIM_1_TIME_Y, y); >>>> + >>>> + xe_device_mem_access_get(gt_to_xe(hwmon->gt)); >>>> + >>>> + mutex_lock(&hwmon->hwmon_lock); >>>> + >>>> + xe_hwmon_process_reg(hwmon, REG_PKG_RAPL_LIMIT, REG_RMW, (u32 *)&r, >>>> + PKG_PWR_LIM_1_TIME, rxy); >>>> + >>>> + mutex_unlock(&hwmon->hwmon_lock); >>> >>> why are we locking here? >> >> Since it is rmw operation we are using lock here. > > OK... so what you are trying to protect here is the > > read -> update -> write > > and it makes sense. The problem is that if this is a generic > rule, which means that everyone who will do a rmw operation has > to take the lock, why not take the lock directly in > xe_hwmon_process_reg()? > > But also this can be a bit confusing, because a function is > either locked or unlocked and purists might complain. > > A suggestion would be to do something like: > > static int xe_hwmon_process_reg(..., enum xe_hwmon_reg_operation operation) > { > ... > } > > static int xe_hwmon_reg_read(...); > { > return xe_hwmon_process_reg(..., REG_READ); > } > > static int xe_hwmon_reg_write(...); > { > return xe_hwmon_process_reg(..., REG_WRITE); > } > > static int xe_hwmon_reg_rmw(...); > { > int ret; > > /* > * Optional: you can check that the lock is not taken > * to shout loud if potential deadlocks arise. > */ > > /* > * We want to protect the register update with the > * lock blah blah blah... explanatory comment. > */ > mutex_lock(&hwmon->hwmon_lock); > ret = xe_hwmon_process_reg(..., REG_RMW); > mutex_unlock(&hwmon->hwmon_lock); > > return ret; > } > > What do you think? It looks much clearer to me. REG_PKG_RAPL_LIMIT register is being written in xe_hwmon_power_max_write also, that's why lock is taken. But some how while cleaning up I forgot to take it in xe_hwmon_power_max_write(), thanks for catching it up. I will update xe_hwmon_power_max_write() and resend series. Thanks, Badal > > Andi
Hi Badal, > > > > > + /* val in hw units */ > > > > > + val = DIV_ROUND_CLOSEST_ULL((u64)val << hwmon->scl_shift_time, SF_TIME); > > > > > + /* Convert to 1.x * power(2,y) */ > > > > > + if (!val) { > > > > > + /* Avoid ilog2(0) */ > > > > > + y = 0; > > > > > + x = 0; > > > > > + } else { > > > > > + y = ilog2(val); > > > > > + /* x = (val - (1 << y)) >> (y - 2); */ > > > > > > > > this is some spurious development comment, can you please remove > > > > it? > > > > > > This is kept intentionally to help to understand the calculations. > > > > then this is confusing... Can you please expand the concept? > > As it is it's not understandable and I would expect someone > > sending a patch with title: > > > > [PATCH] drm/xe/hwmon: Remove spurious comment > > > > Because it just looks forgotten from previous development. > I will add this comment inside the comment at the top of if. So it will look > like. > /* > * Convert to 1.x * power(2,y) > * y = ilog(val); > * x = (val - (1 << y)) >> (y-2); > */ All right. > > > > > + x = (val - (1ul << y)) << x_w >> y; > > > > > + } > > > > > + > > > > > + rxy = REG_FIELD_PREP(PKG_PWR_LIM_1_TIME_X, x) | REG_FIELD_PREP(PKG_PWR_LIM_1_TIME_Y, y); > > > > > + > > > > > + xe_device_mem_access_get(gt_to_xe(hwmon->gt)); > > > > > + > > > > > + mutex_lock(&hwmon->hwmon_lock); > > > > > + > > > > > + xe_hwmon_process_reg(hwmon, REG_PKG_RAPL_LIMIT, REG_RMW, (u32 *)&r, > > > > > + PKG_PWR_LIM_1_TIME, rxy); > > > > > + > > > > > + mutex_unlock(&hwmon->hwmon_lock); > > > > > > > > why are we locking here? > > > > > > Since it is rmw operation we are using lock here. > > > > OK... so what you are trying to protect here is the > > > > read -> update -> write > > > > and it makes sense. The problem is that if this is a generic > > rule, which means that everyone who will do a rmw operation has > > to take the lock, why not take the lock directly in > > xe_hwmon_process_reg()? > > > > But also this can be a bit confusing, because a function is > > either locked or unlocked and purists might complain. > > > > A suggestion would be to do something like: > > > > static int xe_hwmon_process_reg(..., enum xe_hwmon_reg_operation operation) > > { > > ... > > } > > > > static int xe_hwmon_reg_read(...); > > { > > return xe_hwmon_process_reg(..., REG_READ); > > } > > > > static int xe_hwmon_reg_write(...); > > { > > return xe_hwmon_process_reg(..., REG_WRITE); > > } > > > > static int xe_hwmon_reg_rmw(...); > > { > > int ret; > > > > /* > > * Optional: you can check that the lock is not taken > > * to shout loud if potential deadlocks arise. > > */ > > > > /* > > * We want to protect the register update with the > > * lock blah blah blah... explanatory comment. > > */ > > mutex_lock(&hwmon->hwmon_lock); > > ret = xe_hwmon_process_reg(..., REG_RMW); > > mutex_unlock(&hwmon->hwmon_lock); > > > > return ret; > > } > > > > What do you think? It looks much clearer to me. > > REG_PKG_RAPL_LIMIT register is being written in xe_hwmon_power_max_write > also, that's why lock is taken. But some how while cleaning up I forgot to > take it in xe_hwmon_power_max_write(), thanks for catching it up. I will > update xe_hwmon_power_max_write() and resend series. yes... OK... then, please add the lock also in the write case. But still... thinking of hwmon running in more threads don't you think we might need a more generic locking? This might mean to lock all around xe_hwmon_process_reg()... maybe it's an overkill. For the time being I'm OK with your current solution. If you don't like my suggestion above, feel free to ignore it. Andi
On Tue, 26 Sep 2023 14:01:06 -0700, Andi Shyti wrote: > Hi Badal/Andi, > > > > > > > + /* val in hw units */ > > > > > > + val = DIV_ROUND_CLOSEST_ULL((u64)val << hwmon->scl_shift_time, SF_TIME); > > > > > > + /* Convert to 1.x * power(2,y) */ > > > > > > + if (!val) { > > > > > > + /* Avoid ilog2(0) */ > > > > > > + y = 0; > > > > > > + x = 0; > > > > > > + } else { > > > > > > + y = ilog2(val); > > > > > > + /* x = (val - (1 << y)) >> (y - 2); */ > > > > > > > > > > this is some spurious development comment, can you please remove > > > > > it? > > > > > > > > This is kept intentionally to help to understand the calculations. > > > > > > then this is confusing... Can you please expand the concept? > > > As it is it's not understandable and I would expect someone > > > sending a patch with title: > > > > > > [PATCH] drm/xe/hwmon: Remove spurious comment > > > > > > Because it just looks forgotten from previous development. > > I will add this comment inside the comment at the top of if. So it will look > > like. > > /* > > * Convert to 1.x * power(2,y) > > * y = ilog(val); > > * x = (val - (1 << y)) >> (y-2); > > */ > > All right. That comment is explaining that one line of code. I think we should just leave it where it is, it doesn't make sense to move it above the if. How else can we write it so that the comment doesn't look like a leftover? If the code is clear we can remove the comment, but I think the code is hard to understand. So try to understand the code and then you will need the comment. > > > > > > > + x = (val - (1ul << y)) << x_w >> y; > > > > > > + } > > > > > > + > > > > > > + rxy = REG_FIELD_PREP(PKG_PWR_LIM_1_TIME_X, x) | REG_FIELD_PREP(PKG_PWR_LIM_1_TIME_Y, y); > > > > > > + > > > > > > + xe_device_mem_access_get(gt_to_xe(hwmon->gt)); > > > > > > + > > > > > > + mutex_lock(&hwmon->hwmon_lock); > > > > > > + > > > > > > + xe_hwmon_process_reg(hwmon, REG_PKG_RAPL_LIMIT, REG_RMW, (u32 *)&r, > > > > > > + PKG_PWR_LIM_1_TIME, rxy); > > > > > > + > > > > > > + mutex_unlock(&hwmon->hwmon_lock); > > > > > > > > > > why are we locking here? > > > > > > > > Since it is rmw operation we are using lock here. > > > > > > OK... so what you are trying to protect here is the > > > > > > read -> update -> write > > > > > > and it makes sense. The problem is that if this is a generic > > > rule, which means that everyone who will do a rmw operation has > > > to take the lock, why not take the lock directly in > > > xe_hwmon_process_reg()? > > > > > > But also this can be a bit confusing, because a function is > > > either locked or unlocked and purists might complain. > > > > > > A suggestion would be to do something like: > > > > > > static int xe_hwmon_process_reg(..., enum xe_hwmon_reg_operation operation) > > > { > > > ... > > > } > > > > > > static int xe_hwmon_reg_read(...); > > > { > > > return xe_hwmon_process_reg(..., REG_READ); > > > } > > > > > > static int xe_hwmon_reg_write(...); > > > { > > > return xe_hwmon_process_reg(..., REG_WRITE); > > > } > > > > > > static int xe_hwmon_reg_rmw(...); > > > { > > > int ret; > > > > > > /* > > > * Optional: you can check that the lock is not taken > > > * to shout loud if potential deadlocks arise. > > > */ > > > > > > /* > > > * We want to protect the register update with the > > > * lock blah blah blah... explanatory comment. > > > */ > > > mutex_lock(&hwmon->hwmon_lock); > > > ret = xe_hwmon_process_reg(..., REG_RMW); > > > mutex_unlock(&hwmon->hwmon_lock); > > > > > > return ret; > > > } > > > > > > What do you think? It looks much clearer to me. > > > > REG_PKG_RAPL_LIMIT register is being written in xe_hwmon_power_max_write > > also, that's why lock is taken. But some how while cleaning up I forgot to > > take it in xe_hwmon_power_max_write(), thanks for catching it up. I will > > update xe_hwmon_power_max_write() and resend series. > > yes... OK... then, please add the lock also in the write case. > > But still... thinking of hwmon running in more threads don't you > think we might need a more generic locking? This might mean to > lock all around xe_hwmon_process_reg()... maybe it's an overkill. > > For the time being I'm OK with your current solution. > > If you don't like my suggestion above, feel free to ignore it. Yeah agree, might as well take the lock around the switch statement in xe_hwmon_process_reg(). > > Andi Thanks. -- Ashutosh
Hi Ashutosh, On 27-09-2023 09:02, Dixit, Ashutosh wrote: > On Tue, 26 Sep 2023 14:01:06 -0700, Andi Shyti wrote: >> > > Hi Badal/Andi, > >> >>>>>>> + /* val in hw units */ >>>>>>> + val = DIV_ROUND_CLOSEST_ULL((u64)val << hwmon->scl_shift_time, SF_TIME); >>>>>>> + /* Convert to 1.x * power(2,y) */ >>>>>>> + if (!val) { >>>>>>> + /* Avoid ilog2(0) */ >>>>>>> + y = 0; >>>>>>> + x = 0; >>>>>>> + } else { >>>>>>> + y = ilog2(val); >>>>>>> + /* x = (val - (1 << y)) >> (y - 2); */ >>>>>> >>>>>> this is some spurious development comment, can you please remove >>>>>> it? >>>>> >>>>> This is kept intentionally to help to understand the calculations. >>>> >>>> then this is confusing... Can you please expand the concept? >>>> As it is it's not understandable and I would expect someone >>>> sending a patch with title: >>>> >>>> [PATCH] drm/xe/hwmon: Remove spurious comment >>>> >>>> Because it just looks forgotten from previous development. >>> I will add this comment inside the comment at the top of if. So it will look >>> like. >>> /* >>> * Convert to 1.x * power(2,y) >>> * y = ilog(val); >>> * x = (val - (1 << y)) >> (y-2); >>> */ >> >> All right. > > That comment is explaining that one line of code. I think we should just > leave it where it is, it doesn't make sense to move it above the if. How > else can we write it so that the comment doesn't look like a leftover? > > If the code is clear we can remove the comment, but I think the code is > hard to understand. So try to understand the code and then you will need > the comment. Agreed, I will keep this comment as it is. > >> >>>>>>> + x = (val - (1ul << y)) << x_w >> y; >>>>>>> + } >>>>>>> + >>>>>>> + rxy = REG_FIELD_PREP(PKG_PWR_LIM_1_TIME_X, x) | REG_FIELD_PREP(PKG_PWR_LIM_1_TIME_Y, y); >>>>>>> + >>>>>>> + xe_device_mem_access_get(gt_to_xe(hwmon->gt)); >>>>>>> + >>>>>>> + mutex_lock(&hwmon->hwmon_lock); >>>>>>> + >>>>>>> + xe_hwmon_process_reg(hwmon, REG_PKG_RAPL_LIMIT, REG_RMW, (u32 *)&r, >>>>>>> + PKG_PWR_LIM_1_TIME, rxy); >>>>>>> + >>>>>>> + mutex_unlock(&hwmon->hwmon_lock); >>>>>> >>>>>> why are we locking here? >>>>> >>>>> Since it is rmw operation we are using lock here. >>>> >>>> OK... so what you are trying to protect here is the >>>> >>>> read -> update -> write >>>> >>>> and it makes sense. The problem is that if this is a generic >>>> rule, which means that everyone who will do a rmw operation has >>>> to take the lock, why not take the lock directly in >>>> xe_hwmon_process_reg()? >>>> >>>> But also this can be a bit confusing, because a function is >>>> either locked or unlocked and purists might complain. >>>> >>>> A suggestion would be to do something like: >>>> >>>> static int xe_hwmon_process_reg(..., enum xe_hwmon_reg_operation operation) >>>> { >>>> ... >>>> } >>>> >>>> static int xe_hwmon_reg_read(...); >>>> { >>>> return xe_hwmon_process_reg(..., REG_READ); >>>> } >>>> >>>> static int xe_hwmon_reg_write(...); >>>> { >>>> return xe_hwmon_process_reg(..., REG_WRITE); >>>> } >>>> >>>> static int xe_hwmon_reg_rmw(...); >>>> { >>>> int ret; >>>> >>>> /* >>>> * Optional: you can check that the lock is not taken >>>> * to shout loud if potential deadlocks arise. >>>> */ >>>> >>>> /* >>>> * We want to protect the register update with the >>>> * lock blah blah blah... explanatory comment. >>>> */ >>>> mutex_lock(&hwmon->hwmon_lock); >>>> ret = xe_hwmon_process_reg(..., REG_RMW); >>>> mutex_unlock(&hwmon->hwmon_lock); >>>> >>>> return ret; >>>> } >>>> >>>> What do you think? It looks much clearer to me. >>> >>> REG_PKG_RAPL_LIMIT register is being written in xe_hwmon_power_max_write >>> also, that's why lock is taken. But some how while cleaning up I forgot to >>> take it in xe_hwmon_power_max_write(), thanks for catching it up. I will >>> update xe_hwmon_power_max_write() and resend series. >> >> yes... OK... then, please add the lock also in the write case. >> >> But still... thinking of hwmon running in more threads don't you >> think we might need a more generic locking? This might mean to >> lock all around xe_hwmon_process_reg()... maybe it's an overkill. >> >> For the time being I'm OK with your current solution. >> >> If you don't like my suggestion above, feel free to ignore it. > > Yeah agree, might as well take the lock around the switch statement in > xe_hwmon_process_reg(). Will there be a possibility where two different threads will access power attributes power1_max and power1_max_interval simultaneously and frequently. I am not able to think such scenario. If not then we can remove lock from here. Regards. Badal > >> >> Andi > > Thanks. > -- > Ashutosh
> -----Original Message----- > From: Nilawar, Badal <badal.nilawar@intel.com> > Sent: Wednesday, September 27, 2023 2:35 PM > To: Dixit, Ashutosh <ashutosh.dixit@intel.com>; Andi Shyti > <andi.shyti@linux.intel.com> > Cc: intel-xe@lists.freedesktop.org; linux-hwmon@vger.kernel.org; Gupta, > Anshuman <anshuman.gupta@intel.com>; linux@roeck-us.net; Tauro, Riana > <riana.tauro@intel.com>; Brost, Matthew <matthew.brost@intel.com>; Vivi, > Rodrigo <rodrigo.vivi@intel.com> > Subject: Re: [PATCH v6 5/5] drm/xe/hwmon: Expose power1_max_interval > > Hi Ashutosh, > > On 27-09-2023 09:02, Dixit, Ashutosh wrote: > > On Tue, 26 Sep 2023 14:01:06 -0700, Andi Shyti wrote: > >> > > > > Hi Badal/Andi, > > > >> > >>>>>>> + /* val in hw units */ > >>>>>>> + val = DIV_ROUND_CLOSEST_ULL((u64)val << hwmon- > >scl_shift_time, SF_TIME); > >>>>>>> + /* Convert to 1.x * power(2,y) */ > >>>>>>> + if (!val) { > >>>>>>> + /* Avoid ilog2(0) */ > >>>>>>> + y = 0; > >>>>>>> + x = 0; > >>>>>>> + } else { > >>>>>>> + y = ilog2(val); > >>>>>>> + /* x = (val - (1 << y)) >> (y - 2); */ > >>>>>> > >>>>>> this is some spurious development comment, can you please remove > >>>>>> it? > >>>>> > >>>>> This is kept intentionally to help to understand the calculations. > >>>> > >>>> then this is confusing... Can you please expand the concept? > >>>> As it is it's not understandable and I would expect someone sending > >>>> a patch with title: > >>>> > >>>> [PATCH] drm/xe/hwmon: Remove spurious comment > >>>> > >>>> Because it just looks forgotten from previous development. > >>> I will add this comment inside the comment at the top of if. So it > >>> will look like. > >>> /* > >>> * Convert to 1.x * power(2,y) > >>> * y = ilog(val); > >>> * x = (val - (1 << y)) >> (y-2); > >>> */ > >> > >> All right. > > > > That comment is explaining that one line of code. I think we should > > just leave it where it is, it doesn't make sense to move it above the > > if. How else can we write it so that the comment doesn't look like a leftover? > > > > If the code is clear we can remove the comment, but I think the code > > is hard to understand. So try to understand the code and then you will > > need the comment. > Agreed, I will keep this comment as it is. > > > >> > >>>>>>> + x = (val - (1ul << y)) << x_w >> y; > >>>>>>> + } > >>>>>>> + > >>>>>>> + rxy = REG_FIELD_PREP(PKG_PWR_LIM_1_TIME_X, x) | > >>>>>>> +REG_FIELD_PREP(PKG_PWR_LIM_1_TIME_Y, y); > >>>>>>> + > >>>>>>> + xe_device_mem_access_get(gt_to_xe(hwmon->gt)); > >>>>>>> + > >>>>>>> + mutex_lock(&hwmon->hwmon_lock); > >>>>>>> + > >>>>>>> + xe_hwmon_process_reg(hwmon, REG_PKG_RAPL_LIMIT, > REG_RMW, (u32 *)&r, > >>>>>>> + PKG_PWR_LIM_1_TIME, rxy); > >>>>>>> + > >>>>>>> + mutex_unlock(&hwmon->hwmon_lock); > >>>>>> > >>>>>> why are we locking here? > >>>>> > >>>>> Since it is rmw operation we are using lock here. > >>>> > >>>> OK... so what you are trying to protect here is the > >>>> > >>>> read -> update -> write > >>>> > >>>> and it makes sense. The problem is that if this is a generic rule, > >>>> which means that everyone who will do a rmw operation has to take > >>>> the lock, why not take the lock directly in xe_hwmon_process_reg()? > >>>> > >>>> But also this can be a bit confusing, because a function is either > >>>> locked or unlocked and purists might complain. > >>>> > >>>> A suggestion would be to do something like: > >>>> > >>>> static int xe_hwmon_process_reg(..., enum xe_hwmon_reg_operation > operation) > >>>> { > >>>> ... > >>>> } > >>>> > >>>> static int xe_hwmon_reg_read(...); > >>>> { > >>>> return xe_hwmon_process_reg(..., REG_READ); > >>>> } > >>>> > >>>> static int xe_hwmon_reg_write(...); > >>>> { > >>>> return xe_hwmon_process_reg(..., REG_WRITE); > >>>> } > >>>> > >>>> static int xe_hwmon_reg_rmw(...); > >>>> { > >>>> int ret; > >>>> > >>>> /* > >>>> * Optional: you can check that the lock is not taken > >>>> * to shout loud if potential deadlocks arise. > >>>> */ > >>>> > >>>> /* > >>>> * We want to protect the register update with the > >>>> * lock blah blah blah... explanatory comment. > >>>> */ > >>>> mutex_lock(&hwmon->hwmon_lock); > >>>> ret = xe_hwmon_process_reg(..., REG_RMW); > >>>> mutex_unlock(&hwmon->hwmon_lock); > >>>> > >>>> return ret; > >>>> } > >>>> > >>>> What do you think? It looks much clearer to me. > >>> > >>> REG_PKG_RAPL_LIMIT register is being written in > >>> xe_hwmon_power_max_write also, that's why lock is taken. But some > >>> how while cleaning up I forgot to take it in > >>> xe_hwmon_power_max_write(), thanks for catching it up. I will update > xe_hwmon_power_max_write() and resend series. > >> > >> yes... OK... then, please add the lock also in the write case. > >> > >> But still... thinking of hwmon running in more threads don't you > >> think we might need a more generic locking? This might mean to lock > >> all around xe_hwmon_process_reg()... maybe it's an overkill. > >> > >> For the time being I'm OK with your current solution. > >> > >> If you don't like my suggestion above, feel free to ignore it. > > > > Yeah agree, might as well take the lock around the switch statement in > > xe_hwmon_process_reg(). > Will there be a possibility where two different threads will access power > attributes power1_max and power1_max_interval simultaneously and > frequently. I am not able to think such scenario. If not then we can remove > lock from here. There are read and write cases, as far as I can see the seq_read_iter always takes seq_file->lock So read cases like hwm_energy won't need any lock in my opinion, we are protected by above sysfs layer. https://elixir.bootlin.com/linux/latest/source/fs/seq_file.c#L171 But seq_write on another hand does not use any lock, so I also fees for any ATTR does any read/write operation on REG_PKG_RAPL_LIMIT register need a lock here. Thanks, Anshuman Gupta. > > Regards. > Badal > > > >> > >> Andi > > > > Thanks. > > -- > > Ashutosh
diff --git a/Documentation/ABI/testing/sysfs-driver-intel-xe-hwmon b/Documentation/ABI/testing/sysfs-driver-intel-xe-hwmon index 1a7a6c23e141..9ceb9c04b52b 100644 --- a/Documentation/ABI/testing/sysfs-driver-intel-xe-hwmon +++ b/Documentation/ABI/testing/sysfs-driver-intel-xe-hwmon @@ -59,3 +59,14 @@ Contact: intel-xe@lists.freedesktop.org Description: RO. Energy input of device in microjoules. Only supported for particular Intel xe graphics platforms. + +What: /sys/devices/.../hwmon/hwmon<i>/power1_max_interval +Date: September 2023 +KernelVersion: 6.5 +Contact: intel-xe@lists.freedesktop.org +Description: RW. Sustained power limit interval (Tau in PL1/Tau) in + milliseconds over which sustained power is averaged. + + Only supported for particular Intel xe graphics platforms. + + diff --git a/drivers/gpu/drm/xe/regs/xe_mchbar_regs.h b/drivers/gpu/drm/xe/regs/xe_mchbar_regs.h index d8ecbe1858d1..519dd1067a19 100644 --- a/drivers/gpu/drm/xe/regs/xe_mchbar_regs.h +++ b/drivers/gpu/drm/xe/regs/xe_mchbar_regs.h @@ -22,15 +22,23 @@ #define PKG_TDP GENMASK_ULL(14, 0) #define PKG_MIN_PWR GENMASK_ULL(30, 16) #define PKG_MAX_PWR GENMASK_ULL(46, 32) +#define PKG_MAX_WIN GENMASK_ULL(54, 48) +#define PKG_MAX_WIN_X GENMASK_ULL(54, 53) +#define PKG_MAX_WIN_Y GENMASK_ULL(52, 48) + #define PCU_CR_PACKAGE_POWER_SKU_UNIT XE_REG(MCHBAR_MIRROR_BASE_SNB + 0x5938) #define PKG_PWR_UNIT REG_GENMASK(3, 0) #define PKG_ENERGY_UNIT REG_GENMASK(12, 8) +#define PKG_TIME_UNIT REG_GENMASK(19, 16) #define PCU_CR_PACKAGE_ENERGY_STATUS XE_REG(MCHBAR_MIRROR_BASE_SNB + 0x593c) #define PCU_CR_PACKAGE_RAPL_LIMIT XE_REG(MCHBAR_MIRROR_BASE_SNB + 0x59a0) #define PKG_PWR_LIM_1 REG_GENMASK(14, 0) #define PKG_PWR_LIM_1_EN REG_BIT(15) +#define PKG_PWR_LIM_1_TIME REG_GENMASK(23, 17) +#define PKG_PWR_LIM_1_TIME_X REG_GENMASK(23, 22) +#define PKG_PWR_LIM_1_TIME_Y REG_GENMASK(21, 17) #endif /* _XE_MCHBAR_REGS_H_ */ diff --git a/drivers/gpu/drm/xe/xe_hwmon.c b/drivers/gpu/drm/xe/xe_hwmon.c index 1deb5007e1e2..5bd5e5e9958b 100644 --- a/drivers/gpu/drm/xe/xe_hwmon.c +++ b/drivers/gpu/drm/xe/xe_hwmon.c @@ -38,6 +38,7 @@ enum xe_hwmon_reg_operation { #define SF_CURR 1000 /* milliamperes */ #define SF_VOLTAGE 1000 /* millivolts */ #define SF_ENERGY 1000000 /* microjoules */ +#define SF_TIME 1000 /* milliseconds */ struct xe_hwmon_energy_info { u32 reg_val_prev; @@ -50,6 +51,7 @@ struct xe_hwmon { struct mutex hwmon_lock; /* rmw operations*/ int scl_shift_power; int scl_shift_energy; + int scl_shift_time; struct xe_hwmon_energy_info ei; /* Energy info for energy1_input */ }; @@ -256,6 +258,138 @@ xe_hwmon_energy_get(struct xe_hwmon *hwmon, long *energy) xe_device_mem_access_put(gt_to_xe(hwmon->gt)); } +static ssize_t +xe_hwmon_power1_max_interval_show(struct device *dev, struct device_attribute *attr, + char *buf) +{ + struct xe_hwmon *hwmon = dev_get_drvdata(dev); + u32 r, x, y, x_w = 2; /* 2 bits */ + u64 tau4, out; + + xe_device_mem_access_get(gt_to_xe(hwmon->gt)); + + xe_hwmon_process_reg(hwmon, REG_PKG_RAPL_LIMIT, + REG_READ, &r, 0, 0); + + xe_device_mem_access_put(gt_to_xe(hwmon->gt)); + + x = REG_FIELD_GET(PKG_PWR_LIM_1_TIME_X, r); + y = REG_FIELD_GET(PKG_PWR_LIM_1_TIME_Y, r); + /* + * tau = 1.x * power(2,y), x = bits(23:22), y = bits(21:17) + * = (4 | x) << (y - 2) + * where (y - 2) ensures a 1.x fixed point representation of 1.x + * However because y can be < 2, we compute + * tau4 = (4 | x) << y + * but add 2 when doing the final right shift to account for units + */ + tau4 = ((1 << x_w) | x) << y; + /* val in hwmon interface units (millisec) */ + out = mul_u64_u32_shr(tau4, SF_TIME, hwmon->scl_shift_time + x_w); + + return sysfs_emit(buf, "%llu\n", out); +} + +static ssize_t +xe_hwmon_power1_max_interval_store(struct device *dev, struct device_attribute *attr, + const char *buf, size_t count) +{ + struct xe_hwmon *hwmon = dev_get_drvdata(dev); + u32 x, y, rxy, x_w = 2; /* 2 bits */ + u64 tau4, r, max_win; + unsigned long val; + int ret; + + ret = kstrtoul(buf, 0, &val); + if (ret) + return ret; + + /* + * Max HW supported tau in '1.x * power(2,y)' format, x = 0, y = 0x12 + * The hwmon->scl_shift_time default of 0xa results in a max tau of 256 seconds + */ +#define PKG_MAX_WIN_DEFAULT 0x12ull + + /* + * val must be < max in hwmon interface units. The steps below are + * explained in xe_hwmon_power1_max_interval_show() + */ + r = FIELD_PREP(PKG_MAX_WIN, PKG_MAX_WIN_DEFAULT); + x = REG_FIELD_GET(PKG_MAX_WIN_X, r); + y = REG_FIELD_GET(PKG_MAX_WIN_Y, r); + tau4 = ((1 << x_w) | x) << y; + max_win = mul_u64_u32_shr(tau4, SF_TIME, hwmon->scl_shift_time + x_w); + + if (val > max_win) + return -EINVAL; + + /* val in hw units */ + val = DIV_ROUND_CLOSEST_ULL((u64)val << hwmon->scl_shift_time, SF_TIME); + /* Convert to 1.x * power(2,y) */ + if (!val) { + /* Avoid ilog2(0) */ + y = 0; + x = 0; + } else { + y = ilog2(val); + /* x = (val - (1 << y)) >> (y - 2); */ + x = (val - (1ul << y)) << x_w >> y; + } + + rxy = REG_FIELD_PREP(PKG_PWR_LIM_1_TIME_X, x) | REG_FIELD_PREP(PKG_PWR_LIM_1_TIME_Y, y); + + xe_device_mem_access_get(gt_to_xe(hwmon->gt)); + + mutex_lock(&hwmon->hwmon_lock); + + xe_hwmon_process_reg(hwmon, REG_PKG_RAPL_LIMIT, REG_RMW, (u32 *)&r, + PKG_PWR_LIM_1_TIME, rxy); + + mutex_unlock(&hwmon->hwmon_lock); + + xe_device_mem_access_put(gt_to_xe(hwmon->gt)); + + return count; +} + +static SENSOR_DEVICE_ATTR(power1_max_interval, 0664, + xe_hwmon_power1_max_interval_show, + xe_hwmon_power1_max_interval_store, 0); + +static struct attribute *hwmon_attributes[] = { + &sensor_dev_attr_power1_max_interval.dev_attr.attr, + NULL +}; + +static umode_t xe_hwmon_attributes_visible(struct kobject *kobj, + struct attribute *attr, int index) +{ + struct device *dev = kobj_to_dev(kobj); + struct xe_hwmon *hwmon = dev_get_drvdata(dev); + u32 reg_val; + int ret = 0; + + xe_device_mem_access_get(gt_to_xe(hwmon->gt)); + + if (attr == &sensor_dev_attr_power1_max_interval.dev_attr.attr) + ret = xe_hwmon_process_reg(hwmon, REG_PKG_RAPL_LIMIT, + REG_READ, ®_val, 0, 0) ? 0 : attr->mode; + + xe_device_mem_access_put(gt_to_xe(hwmon->gt)); + + return ret; +} + +static const struct attribute_group hwmon_attrgroup = { + .attrs = hwmon_attributes, + .is_visible = xe_hwmon_attributes_visible, +}; + +static const struct attribute_group *hwmon_groups[] = { + &hwmon_attrgroup, + NULL +}; + static const struct hwmon_channel_info *hwmon_info[] = { HWMON_CHANNEL_INFO(power, HWMON_P_MAX | HWMON_P_RATED_MAX | HWMON_P_CRIT), HWMON_CHANNEL_INFO(curr, HWMON_C_CRIT), @@ -574,6 +708,7 @@ xe_hwmon_get_preregistration_info(struct xe_device *xe) if (!ret) { hwmon->scl_shift_power = REG_FIELD_GET(PKG_PWR_UNIT, val_sku_unit); hwmon->scl_shift_energy = REG_FIELD_GET(PKG_ENERGY_UNIT, val_sku_unit); + hwmon->scl_shift_time = REG_FIELD_GET(PKG_TIME_UNIT, val_sku_unit); } /* @@ -611,7 +746,8 @@ void xe_hwmon_register(struct xe_device *xe) /* hwmon_dev points to device hwmon<i> */ hwmon->hwmon_dev = devm_hwmon_device_register_with_info(dev, "xe", hwmon, &hwmon_chip_info, - NULL); + hwmon_groups); + if (IS_ERR(hwmon->hwmon_dev)) { drm_warn(&xe->drm, "Failed to register xe hwmon (%pe)\n", hwmon->hwmon_dev); xe->hwmon = NULL;