| Message ID | 20220422133752.1370964-1-badal.nilawar@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] drm/i915/rc6: Access RC6 regs with forcewake | expand |
On Fri, Apr 22, 2022 at 07:07:52PM +0530, Badal Nilawar wrote: > To access RC6 related MMIO regs use intel_uncore_write(), which grabs > forcewake if reg requires a forcewake. > > Signed-off-by: Badal Nilawar <badal.nilawar@intel.com> > --- > drivers/gpu/drm/i915/gt/intel_rc6.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/gt/intel_rc6.c b/drivers/gpu/drm/i915/gt/intel_rc6.c > index b4770690e794..9edaad3f19b5 100644 > --- a/drivers/gpu/drm/i915/gt/intel_rc6.c > +++ b/drivers/gpu/drm/i915/gt/intel_rc6.c > @@ -55,7 +55,7 @@ static struct drm_i915_private *rc6_to_i915(struct intel_rc6 *rc) > > static void set(struct intel_uncore *uncore, i915_reg_t reg, u32 val) > { > - intel_uncore_write_fw(uncore, reg, val); > + intel_uncore_write(uncore, reg, val); The set() function is primarily used within the *_rc6_enable() functions. Those are all called via intel_rc6_enable() which has already grabbed explicit forcewake before calling them: intel_uncore_forcewake_get(uncore, FORCEWAKE_ALL); so there's no need to grab an additional reference on every register access. Likewise, the call in vlv_residency_raw() doesn't need forcewake because its own caller (intel_rc6_residency_ns) has already acquired it. I think the call in intel_rc6_unpark() is the only one that might be questionable from a very quick skim of the code. So rather than needlessly grabbing forcewake in all the other spots, it's probably better to just replace that single call with a direct call to intel_uncore_write() if it actually is problematic. Even better, we might just want to drop the set() wrapper completely and replace all instances with the appropriate intel_uncore_write[_fw] calls; I don't really see the slightly shorter lines the wrapper gives us as being worth the deviation from the rest of the driver's code (especially if it's causing us confusion about what the intendended forcewake semantics are). Matt > } > > static void gen11_rc6_enable(struct intel_rc6 *rc6) > -- > 2.25.1 >
On 22-04-2022 21:04, Matt Roper wrote: > On Fri, Apr 22, 2022 at 07:07:52PM +0530, Badal Nilawar wrote: >> To access RC6 related MMIO regs use intel_uncore_write(), which grabs >> forcewake if reg requires a forcewake. >> >> Signed-off-by: Badal Nilawar <badal.nilawar@intel.com> >> --- >> drivers/gpu/drm/i915/gt/intel_rc6.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/i915/gt/intel_rc6.c b/drivers/gpu/drm/i915/gt/intel_rc6.c >> index b4770690e794..9edaad3f19b5 100644 >> --- a/drivers/gpu/drm/i915/gt/intel_rc6.c >> +++ b/drivers/gpu/drm/i915/gt/intel_rc6.c >> @@ -55,7 +55,7 @@ static struct drm_i915_private *rc6_to_i915(struct intel_rc6 *rc) >> >> static void set(struct intel_uncore *uncore, i915_reg_t reg, u32 val) >> { >> - intel_uncore_write_fw(uncore, reg, val); >> + intel_uncore_write(uncore, reg, val); > > The set() function is primarily used within the *_rc6_enable() functions. > Those are all called via intel_rc6_enable() which has already > grabbed explicit forcewake before calling them: > > intel_uncore_forcewake_get(uncore, FORCEWAKE_ALL); > > so there's no need to grab an additional reference on every register > access. Likewise, the call in vlv_residency_raw() doesn't need > forcewake because its own caller (intel_rc6_residency_ns) has already > acquired it. > > I think the call in intel_rc6_unpark() is the only one that might be > questionable from a very quick skim of the code. So rather than > needlessly grabbing forcewake in all the other spots, it's probably > better to just replace that single call with a direct call to > intel_uncore_write() if it actually is problematic. > > Even better, we might just want to drop the set() wrapper completely and > replace all instances with the appropriate intel_uncore_write[_fw] > calls; I don't really see the slightly shorter lines the wrapper gives > us as being worth the deviation from the rest of the driver's code > (especially if it's causing us confusion about what the intendended > forcewake semantics are). > Thanks for clarification. The change in set() function was done for inte_rc6_unpark/park() only since 0xA090 RC6_CTRL is getting written without forcewake. For now as you suggested in inte_rc6_unpark/park() I will replace set() with intel_uncore_write(). Regards, Badal > > Matt > >> } >> >> static void gen11_rc6_enable(struct intel_rc6 *rc6) >> -- >> 2.25.1 >> >
diff --git a/drivers/gpu/drm/i915/gt/intel_rc6.c b/drivers/gpu/drm/i915/gt/intel_rc6.c index b4770690e794..9edaad3f19b5 100644 --- a/drivers/gpu/drm/i915/gt/intel_rc6.c +++ b/drivers/gpu/drm/i915/gt/intel_rc6.c @@ -55,7 +55,7 @@ static struct drm_i915_private *rc6_to_i915(struct intel_rc6 *rc) static void set(struct intel_uncore *uncore, i915_reg_t reg, u32 val) { - intel_uncore_write_fw(uncore, reg, val); + intel_uncore_write(uncore, reg, val); } static void gen11_rc6_enable(struct intel_rc6 *rc6)
To access RC6 related MMIO regs use intel_uncore_write(), which grabs forcewake if reg requires a forcewake. Signed-off-by: Badal Nilawar <badal.nilawar@intel.com> --- drivers/gpu/drm/i915/gt/intel_rc6.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)