Re: [Intel-xe] [PATCH v6 1/5] drm/xe/hwmon: Expose power attributes

["Dixit, Ashutosh" <ashutosh.dixit@intel.com>]

On Thu, 28 Sep 2023 23:37:35 -0700, Nilawar, Badal wrote:
>

Hi Badal,

> On 28-09-2023 10:25, Dixit, Ashutosh wrote:
> > On Wed, 27 Sep 2023 01:39:46 -0700, Nilawar, Badal wrote:
> >
> >> On 27-09-2023 10:23, Dixit, Ashutosh wrote:
> >>> On Mon, 25 Sep 2023 01:18:38 -0700, Badal Nilawar wrote:
> >>>>
> >>>> +static umode_t
> >>>> +xe_hwmon_is_visible(const void *drvdata, enum hwmon_sensor_types type,
> >>>> +		    u32 attr, int channel)
> >>>> +{
> >>>> +	struct xe_hwmon *hwmon = (struct xe_hwmon *)drvdata;
> >>>> +	int ret;
> >>>> +
> >>>> +	xe_device_mem_access_get(gt_to_xe(hwmon->gt));
> >>>
> >>> Maybe we do xe_device_mem_access_get/put in xe_hwmon_process_reg where it
> >>> is needed? E.g. xe_hwmon_is_visible doesn't need to do this because it
> >>> doesn't read/write registers.
> >> Agreed, but visible function is called only once while registering hwmon
> >> interface, which happen during driver probe. During driver probe device
> >> will be in resumed state. So no harm in keeping
> >> xe_device_mem_access_get/put in visible function.
> >
> > To me it doesn't make any sense to keep xe_device_mem_access_get/put
> > anywhere except in xe_hwmon_process_reg where the HW access actually
> > happens. We can eliminate xe_device_mem_access_get/put's all over the place
> > if we do it. Isn't it?
> Agreed, thought process here suggest that take rpm wakeref at lowest
> possible level. I already tried this in rfc series and in some extent in
> rev2. There is problem with this approach. See my comments below.
> >
> > The only restriction I have heard of (though not sure why) is that
> > xe_device_mem_access_get/put should not be called under lock. Though I am
> > not sure it is for spinlock or also mutex. So as we were saying the locking
> > will also need to move to xe_hwmon_process_reg.
> Yes from rev2 comments its dangerous to take mutex before
> xe_device_mem_access_get/put. With code for "PL1 disable/restore during
> resume" I saw deadlock. Scenario was power1_max write -> mutex lock -> rpm
> resume -> disable pl1 -> mutex lock (dead lock here).

But this is already the wrong order as mentioned below. If we follow the
below order do we still see deadlock?

> >
> > So:
> >
> > xe_hwmon_process_reg()
> > {
> >	xe_device_mem_access_get
> >	mutex_lock
> >	...
> >	mutex_unlock
> >	xe_device_mem_access_put
> > }
> >
> > So once again if this is not possible for some reason let's figure out why.
> There are two problems with this approach.
>
> Problem 1: If you see implementation of xe_hwmon_power_max_write, reg
> access is happening 3 times, so there will be 3 rpm suspend/resume
> cycles. I was observing the same with rfc implementation. So in subsequent
> series xe_device_mem_access_put/get is moved to top level functions
> i.e. hwmon hooks.

This is not exactly correct because there is also a 1 second autosuspend
delay which will prevent such rpm suspend/resume cycles:

xe_pm_runtime_init:
	pm_runtime_set_autosuspend_delay(dev, 1000);


>
> Problem 2: If locking moved inside xe_hwmon_process_reg then between two
> subsequent reg accesses it will open small window during which race can
> happen.
> As Anshuman suggested in other thread for read are sequential and protected
> by sysfs layer. So lets apply locking only for RW attributes.

But what is the locking trying to protect? As far as I understand it is
just the registers which have to be atomically modified/read. So it seems
sufficient to just protect the register accesses with the lock.

So I am still not convinced.

Thanks.
--
Ashutosh


>
> +static int xe_hwmon_power_max_write(struct xe_hwmon *hwmon, long value)
> +{
> +	u32 reg_val;
> +
> +	/* Disable PL1 limit and verify, as limit cannot be disabled on all
> platforms */
> +	if (value == PL1_DISABLE) {
> +		xe_hwmon_process_reg(hwmon, REG_PKG_RAPL_LIMIT, REG_RMW, &reg_val,
> +				     PKG_PWR_LIM_1_EN, 0);
> +		xe_hwmon_process_reg(hwmon, REG_PKG_RAPL_LIMIT, REG_READ, &reg_val,
> +				     PKG_PWR_LIM_1_EN, 0);
> +
> +		if (reg_val & PKG_PWR_LIM_1_EN)
> +			return -EOPNOTSUPP;
> +	}
> +
> +	/* Computation in 64-bits to avoid overflow. Round to nearest. */
> +	reg_val = DIV_ROUND_CLOSEST_ULL((u64)value <<
> hwmon->scl_shift_power, SF_POWER);
> +	reg_val = PKG_PWR_LIM_1_EN | REG_FIELD_PREP(PKG_PWR_LIM_1, reg_val);
> +
> +	xe_hwmon_process_reg(hwmon, REG_PKG_RAPL_LIMIT, REG_RMW, &reg_val,
> +			     PKG_PWR_LIM_1_EN | PKG_PWR_LIM_1, reg_val);
> +
> +	return 0;
> +}
>
> Regards,
> Badal
> >
> >>>
> >>> Also do we need to take forcewake? i915 had forcewake table so it would
> >>> take forcewake automatically but XE doesn't do that.
> >> Hwmon regs doesn't fall under GT domain so doesn't need forcewake.
> >
> > OK, great.
> >
> > Thanks.
> > --
> > Ashutosh