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
