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

[Gupta, Anshuman]

> -----Original Message-----
> From: Nilawar, Badal <badal.nilawar at intel.com>
> Sent: Wednesday, September 27, 2023 2:35 PM
> To: Dixit, Ashutosh <ashutosh.dixit at intel.com>; Andi Shyti
> <andi.shyti at linux.intel.com>
> Cc: intel-xe at lists.freedesktop.org; linux-hwmon at vger.kernel.org; Gupta,
> Anshuman <anshuman.gupta at intel.com>; linux at roeck-us.net; Tauro, Riana
> <riana.tauro at intel.com>; Brost, Matthew <matthew.brost at intel.com>; Vivi,
> Rodrigo <rodrigo.vivi at 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