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

[Nilawar, Badal]

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