Re: [Intel-xe] [PATCH v6 5/5] drm/xe/hwmon: Expose power1_max_interval

["Nilawar, Badal" <badal.nilawar@intel.com>]

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