[Intel-gfx] [PATCH 2/7] drm/i915/hwmon: Add HWMON current voltage support

[Gupta, Anshuman]

> -----Original Message-----
> From: Dixit, Ashutosh <ashutosh.dixit at intel.com>
> Sent: Tuesday, September 13, 2022 8:49 PM
> To: Gupta, Anshuman <anshuman.gupta at intel.com>
> Cc: Nilawar, Badal <badal.nilawar at intel.com>; intel-gfx at lists.freedesktop.org;
> Tauro, Riana <riana.tauro at intel.com>; Ewins, Jon <jon.ewins at intel.com>;
> linux-hwmon at vger.kernel.org
> Subject: Re: [PATCH 2/7] drm/i915/hwmon: Add HWMON current voltage
> support
> 
> On Tue, 13 Sep 2022 01:11:57 -0700, Gupta, Anshuman wrote:
> >
> 
> Hi Anshuman,
> 
> > > -----Original Message-----
> > > From: Dixit, Ashutosh <ashutosh.dixit at intel.com>
> > > Sent: Monday, September 12, 2022 10:08 PM
> > > To: Gupta, Anshuman <anshuman.gupta at intel.com>
> > > Cc: Nilawar, Badal <badal.nilawar at intel.com>;
> > > intel-gfx at lists.freedesktop.org; Tauro, Riana
> > > <riana.tauro at intel.com>; Ewins, Jon <jon.ewins at intel.com>;
> > > linux-hwmon at vger.kernel.org
> > > Subject: Re: [PATCH 2/7] drm/i915/hwmon: Add HWMON current voltage
> > > support
> > >
> > > On Mon, 12 Sep 2022 07:09:28 -0700, Gupta, Anshuman wrote:
> > > >
> > > > > +static int
> > > > > +hwm_in_read(struct hwm_drvdata *ddat, u32 attr, long *val) {
> > > > > +	struct i915_hwmon *hwmon = ddat->hwmon;
> > > > > +	intel_wakeref_t wakeref;
> > > > > +	u32 reg_value;
> > > > > +
> > > > > +	switch (attr) {
> > > > > +	case hwmon_in_input:
> > > >
> > > > Other attributes in this series take hwmon->lock before accessing
> > > > i915 registers , So do we need lock here as well ?
> > >
> > > The lock is being taken only for RMW and for making sure energy
> > > counter updates happen atomically. We are not taking the lock for
> > > just reads so IMO no lock is needed here.
> >
> > If that is the case, then why it needs to grab a lock for rmw on
> > different Register ? Like in patch 3 of this series grabs
> > hwmon->howmon_lock for rmw on two different register
> > hwmon->pkg_power_sku_unit
> > and pkg_rapl_limit.
> 
> I don't see this happening, where do you see it?
> 
> > One register rmw should be independent of other register here ?
> 
> Yes each register RMW is independent. In Patch 3 only hwm_power_write (not
> hwm_power_read) is taking the lock for RMW on pkg_rapl_limit (lock is not
> taken for pkg_power_sku_unit). So the assumption is that RMW of a single
> register are not atomic and must be serialized with a lock. Reads are considered
> to not need a lock.
Thanks for explanation , and my apologies for the noise.
Br,
Anshuman Gupta.
> 
> Thanks.
> --
> Ashutosh
> 
> 
> > >
> > > > > +		with_intel_runtime_pm(ddat->uncore->rpm, wakeref)
> > > > > +			reg_value = intel_uncore_read(ddat->uncore,
> hwmon-
> > > > > >rg.gt_perf_status);
> > > > > +		/* In units of 2.5 millivolt */
> > > > > +		*val =
> > > > > DIV_ROUND_CLOSEST(REG_FIELD_GET(GEN12_VOLTAGE_MASK,
> reg_value)
> > > * 25,
> > > > > 10);
> > > > > +		return 0;
> > > > > +	default:
> > > > > +		return -EOPNOTSUPP;
> > > > > +	}
> > > > > +}
> > >
> > > Thanks.
> > > --
> > > Ashutosh