On Tue, 13 Sep 2022 01:11:57 -0700, Gupta, Anshuman wrote:
>
Hi Anshuman,
> > -----Original Message-----
> > From: Dixit, Ashutosh <ashutosh.dixit@xxxxxxxxx>
> > Sent: Monday, September 12, 2022 10:08 PM
> > To: Gupta, Anshuman <anshuman.gupta@xxxxxxxxx>
> > Cc: Nilawar, Badal <badal.nilawar@xxxxxxxxx>; intel-gfx@xxxxxxxxxxxxxxxxxxxxx;
> > Tauro, Riana <riana.tauro@xxxxxxxxx>; Ewins, Jon <jon.ewins@xxxxxxxxx>;
> > linux-hwmon@xxxxxxxxxxxxxxx
> > 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.
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.
--
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
