RE: [PATCH 2/7] drm/i915/hwmon: Add HWMON current voltage support

From: "Gupta, Anshuman" <anshuman.gupta@intel.com>
To: "Dixit, Ashutosh" <ashutosh.dixit@intel.com>
Cc: "Nilawar, Badal" <badal.nilawar@intel.com>,
	"intel-gfx@lists.freedesktop.org"
	<intel-gfx@lists.freedesktop.org>,
	"Tauro, Riana" <riana.tauro@intel.com>,
	"Ewins, Jon" <jon.ewins@intel.com>,
	"linux-hwmon@vger.kernel.org" <linux-hwmon@vger.kernel.org>
Subject: RE: [PATCH 2/7] drm/i915/hwmon: Add HWMON current voltage support
Date: Tue, 13 Sep 2022 08:11:57 +0000	[thread overview]
Message-ID: <CY5PR11MB62114DF624728E35804635D495479@CY5PR11MB6211.namprd11.prod.outlook.com> (raw)
In-Reply-To: <87edwg5z1b.wl-ashutosh.dixit@intel.com>


> -----Original Message-----
> From: Dixit, Ashutosh <ashutosh.dixit@intel.com>
> Sent: Monday, September 12, 2022 10:08 PM
> To: Gupta, Anshuman <anshuman.gupta@intel.com>
> Cc: Nilawar, Badal <badal.nilawar@intel.com>; intel-gfx@lists.freedesktop.org;
> Tauro, Riana <riana.tauro@intel.com>; Ewins, Jon <jon.ewins@intel.com>;
> linux-hwmon@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 pkg_power_sku_unit and pkg_rapl_limit. One register rmw 
should be independent of other register here ?
Thanks,
Anshuman Gupta

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