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

[Andi Shyti <andi.shyti@linux.intel.com>]

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);
>  */

All right.

> > > > > +		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.

Andi