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
