On Tue, 13 Sep 2022 01:11:57 -0700, Gupta, Anshuman wrote: > Hi Anshuman,
> > -----Original Message----- > > From: Dixit, Ashutosh <ashutosh.di...@intel.com> > > Sent: Monday, September 12, 2022 10:08 PM > > To: Gupta, Anshuman <anshuman.gu...@intel.com> > > Cc: Nilawar, Badal <badal.nila...@intel.com>; > > intel-gfx@lists.freedesktop.org; > > Tauro, Riana <riana.ta...@intel.com>; Ewins, Jon <jon.ew...@intel.com>; > > linux-hw...@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. 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