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
