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

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

On Wed, 27 Sep 2023 01:39:46 -0700, Nilawar, Badal wrote:
>

Hi Badal,

> 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?

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.

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.

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