| Message ID | 20230316035954.2593843-1-ashutosh.dixit@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/guc: Disable PL1 power limit when loading GuC firmware | expand |
On 3/15/2023 8:59 PM, Ashutosh Dixit wrote: > On dGfx, the PL1 power limit being enabled and set to a low value results > in a low GPU operating freq. It also negates the freq raise operation which > is done before GuC firmware load. As a result GuC firmware load can time > out. Such timeouts were seen in the GL #8062 bug below (where the PL1 power > limit was enabled and set to a low value). Therefore disable the PL1 power > limit when allowed by HW when loading GuC firmware. v3 label missing in subject. > > v2: > - Take mutex (to disallow writes to power1_max) across GuC reset/fw load > - Add hwm_power_max_restore to error return code path > > v3 (Jani N): > - Add/remove explanatory comments > - Function renames > - Type corrections > - Locking annotation > > Link: https://gitlab.freedesktop.org/drm/intel/-/issues/8062 > Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com> > --- > drivers/gpu/drm/i915/gt/uc/intel_uc.c | 9 +++++++ > drivers/gpu/drm/i915/i915_hwmon.c | 39 +++++++++++++++++++++++++++ > drivers/gpu/drm/i915/i915_hwmon.h | 7 +++++ > 3 files changed, 55 insertions(+) > > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c b/drivers/gpu/drm/i915/gt/uc/intel_uc.c > index 4ccb4be4c9cba..aa8e35a5636a0 100644 > --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c > +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c > @@ -18,6 +18,7 @@ > #include "intel_uc.h" > > #include "i915_drv.h" > +#include "i915_hwmon.h" > > static const struct intel_uc_ops uc_ops_off; > static const struct intel_uc_ops uc_ops_on; > @@ -461,6 +462,7 @@ static int __uc_init_hw(struct intel_uc *uc) > struct intel_guc *guc = &uc->guc; > struct intel_huc *huc = &uc->huc; > int ret, attempts; > + bool pl1en; Init to 'false' here > > GEM_BUG_ON(!intel_uc_supports_guc(uc)); > GEM_BUG_ON(!intel_uc_wants_guc(uc)); > @@ -491,6 +493,9 @@ static int __uc_init_hw(struct intel_uc *uc) > else > attempts = 1; > > + /* Disable a potentially low PL1 power limit to allow freq to be raised */ > + i915_hwmon_power_max_disable(gt->i915, &pl1en); > + > intel_rps_raise_unslice(&uc_to_gt(uc)->rps); > > while (attempts--) { > @@ -547,6 +552,8 @@ static int __uc_init_hw(struct intel_uc *uc) > intel_rps_lower_unslice(&uc_to_gt(uc)->rps); > } > > + i915_hwmon_power_max_restore(gt->i915, pl1en); > + > guc_info(guc, "submission %s\n", str_enabled_disabled(intel_uc_uses_guc_submission(uc))); > guc_info(guc, "SLPC %s\n", str_enabled_disabled(intel_uc_uses_guc_slpc(uc))); > > @@ -563,6 +570,8 @@ static int __uc_init_hw(struct intel_uc *uc) > /* Return GT back to RPn */ > intel_rps_lower_unslice(&uc_to_gt(uc)->rps); > > + i915_hwmon_power_max_restore(gt->i915, pl1en); if (pl1en) i915_hwmon_power_max_enable(). > + > __uc_sanitize(uc); > > if (!ret) { > diff --git a/drivers/gpu/drm/i915/i915_hwmon.c b/drivers/gpu/drm/i915/i915_hwmon.c > index ee63a8fd88fc1..769b5bda4d53f 100644 > --- a/drivers/gpu/drm/i915/i915_hwmon.c > +++ b/drivers/gpu/drm/i915/i915_hwmon.c > @@ -444,6 +444,45 @@ hwm_power_write(struct hwm_drvdata *ddat, u32 attr, int chan, long val) > } > } > > +void i915_hwmon_power_max_disable(struct drm_i915_private *i915, bool *old) Shouldn't we call this i915_hwmon_package_pl1_disable()? > + __acquires(i915->hwmon->hwmon_lock) > +{ > + struct i915_hwmon *hwmon = i915->hwmon; > + intel_wakeref_t wakeref; > + u32 r; > + > + if (!hwmon || !i915_mmio_reg_valid(hwmon->rg.pkg_rapl_limit)) > + return; > + > + /* Take mutex to prevent concurrent hwm_power_max_write */ > + mutex_lock(&hwmon->hwmon_lock); > + > + with_intel_runtime_pm(hwmon->ddat.uncore->rpm, wakeref) > + r = intel_uncore_rmw(hwmon->ddat.uncore, > + hwmon->rg.pkg_rapl_limit, > + PKG_PWR_LIM_1_EN, 0); Most of this code (lock and rmw parts) is already inside static void hwm_locked_with_pm_intel_uncore_rmw() , can we reuse that here? > + > + *old = !!(r & PKG_PWR_LIM_1_EN); > +} > + > +void i915_hwmon_power_max_restore(struct drm_i915_private *i915, bool old) > + __releases(i915->hwmon->hwmon_lock) We can just call this i915_hwmon_power_max_enable() and call whenever the old value was actually enabled. That way, we have proper mirror functions. > +{ > + struct i915_hwmon *hwmon = i915->hwmon; > + intel_wakeref_t wakeref; > + > + if (!hwmon || !i915_mmio_reg_valid(hwmon->rg.pkg_rapl_limit)) > + return; > + > + with_intel_runtime_pm(hwmon->ddat.uncore->rpm, wakeref) > + intel_uncore_rmw(hwmon->ddat.uncore, > + hwmon->rg.pkg_rapl_limit, > + PKG_PWR_LIM_1_EN, > + old ? PKG_PWR_LIM_1_EN : 0); 3rd param should be 0 here, else we will end up clearing other bits. Thanks, Vinay. > + > + mutex_unlock(&hwmon->hwmon_lock); > +} > + > static umode_t > hwm_energy_is_visible(const struct hwm_drvdata *ddat, u32 attr) > { > diff --git a/drivers/gpu/drm/i915/i915_hwmon.h b/drivers/gpu/drm/i915/i915_hwmon.h > index 7ca9cf2c34c96..0fcb7de844061 100644 > --- a/drivers/gpu/drm/i915/i915_hwmon.h > +++ b/drivers/gpu/drm/i915/i915_hwmon.h > @@ -7,14 +7,21 @@ > #ifndef __I915_HWMON_H__ > #define __I915_HWMON_H__ > > +#include <linux/types.h> > + > struct drm_i915_private; > +struct intel_gt; > > #if IS_REACHABLE(CONFIG_HWMON) > void i915_hwmon_register(struct drm_i915_private *i915); > void i915_hwmon_unregister(struct drm_i915_private *i915); > +void i915_hwmon_power_max_disable(struct drm_i915_private *i915, bool *old); > +void i915_hwmon_power_max_restore(struct drm_i915_private *i915, bool old); > #else > static inline void i915_hwmon_register(struct drm_i915_private *i915) { }; > static inline void i915_hwmon_unregister(struct drm_i915_private *i915) { }; > +static inline void i915_hwmon_power_max_disable(struct drm_i915_private *i915, bool *old) { }; > +static inline void i915_hwmon_power_max_restore(struct drm_i915_private *i915, bool old) { }; > #endif > > #endif /* __I915_HWMON_H__ */
On Fri, 24 Mar 2023 11:15:02 -0700, Belgaumkar, Vinay wrote: > Hi Vinay, Thanks for the review. Comments inline below. > On 3/15/2023 8:59 PM, Ashutosh Dixit wrote: > > On dGfx, the PL1 power limit being enabled and set to a low value results > > in a low GPU operating freq. It also negates the freq raise operation which > > is done before GuC firmware load. As a result GuC firmware load can time > > out. Such timeouts were seen in the GL #8062 bug below (where the PL1 power > > limit was enabled and set to a low value). Therefore disable the PL1 power > > limit when allowed by HW when loading GuC firmware. > v3 label missing in subject. > > > > v2: > > - Take mutex (to disallow writes to power1_max) across GuC reset/fw load > > - Add hwm_power_max_restore to error return code path > > > > v3 (Jani N): > > - Add/remove explanatory comments > > - Function renames > > - Type corrections > > - Locking annotation > > > > Link: https://gitlab.freedesktop.org/drm/intel/-/issues/8062 > > Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com> > > --- > > drivers/gpu/drm/i915/gt/uc/intel_uc.c | 9 +++++++ > > drivers/gpu/drm/i915/i915_hwmon.c | 39 +++++++++++++++++++++++++++ > > drivers/gpu/drm/i915/i915_hwmon.h | 7 +++++ > > 3 files changed, 55 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c b/drivers/gpu/drm/i915/gt/uc/intel_uc.c > > index 4ccb4be4c9cba..aa8e35a5636a0 100644 > > --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c > > +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c > > @@ -18,6 +18,7 @@ > > #include "intel_uc.h" > > #include "i915_drv.h" > > +#include "i915_hwmon.h" > > static const struct intel_uc_ops uc_ops_off; > > static const struct intel_uc_ops uc_ops_on; > > @@ -461,6 +462,7 @@ static int __uc_init_hw(struct intel_uc *uc) > > struct intel_guc *guc = &uc->guc; > > struct intel_huc *huc = &uc->huc; > > int ret, attempts; > > + bool pl1en; > > Init to 'false' here See next comment. > > > > GEM_BUG_ON(!intel_uc_supports_guc(uc)); > > GEM_BUG_ON(!intel_uc_wants_guc(uc)); > > @@ -491,6 +493,9 @@ static int __uc_init_hw(struct intel_uc *uc) > > else > > attempts = 1; > > + /* Disable a potentially low PL1 power limit to allow freq to be > > raised */ > > + i915_hwmon_power_max_disable(gt->i915, &pl1en); > > + > > intel_rps_raise_unslice(&uc_to_gt(uc)->rps); > > while (attempts--) { > > @@ -547,6 +552,8 @@ static int __uc_init_hw(struct intel_uc *uc) > > intel_rps_lower_unslice(&uc_to_gt(uc)->rps); > > } > > + i915_hwmon_power_max_restore(gt->i915, pl1en); > > + > > guc_info(guc, "submission %s\n", str_enabled_disabled(intel_uc_uses_guc_submission(uc))); > > guc_info(guc, "SLPC %s\n", str_enabled_disabled(intel_uc_uses_guc_slpc(uc))); > > @@ -563,6 +570,8 @@ static int __uc_init_hw(struct intel_uc *uc) > > /* Return GT back to RPn */ > > intel_rps_lower_unslice(&uc_to_gt(uc)->rps); > > + i915_hwmon_power_max_restore(gt->i915, pl1en); > > if (pl1en) > > i915_hwmon_power_max_enable(). IMO it's better not to have checks in the main __uc_init_hw() function (if we do this we'll need to add 2 checks in __uc_init_hw()). If you really want we could do something like this inside i915_hwmon_power_max_disable/i915_hwmon_power_max_restore. But for now I am not making any changes. (I can send a patch with the changes if you want to take a look but IMO it will add more logic/code but without real benefits (it will save a rmw if the limit was already disabled, but IMO this code is called so infrequently (only during GuC resets) as to not have any significant impact)). > > > + > > __uc_sanitize(uc); > > if (!ret) { > > diff --git a/drivers/gpu/drm/i915/i915_hwmon.c b/drivers/gpu/drm/i915/i915_hwmon.c > > index ee63a8fd88fc1..769b5bda4d53f 100644 > > --- a/drivers/gpu/drm/i915/i915_hwmon.c > > +++ b/drivers/gpu/drm/i915/i915_hwmon.c > > @@ -444,6 +444,45 @@ hwm_power_write(struct hwm_drvdata *ddat, u32 attr, int chan, long val) > > } > > } > > +void i915_hwmon_power_max_disable(struct drm_i915_private *i915, bool > > *old) > Shouldn't we call this i915_hwmon_package_pl1_disable()? I did think of using "pl1" in the function name but then decided to retain "power_max" because other hwmon functions for PL1 limit also use "power_max" (hwm_power_max_read/hwm_power_max_write) and currently "hwmon_power_max" is mapped to the PL1 limit. So "power_max" is used to show that all these functions deal with the PL1 power limit. There is a comment in __uc_init_hw() explaining "power_max" means the PL1 power limit. > > + __acquires(i915->hwmon->hwmon_lock) > > +{ > > + struct i915_hwmon *hwmon = i915->hwmon; > > + intel_wakeref_t wakeref; > > + u32 r; > > + > > + if (!hwmon || !i915_mmio_reg_valid(hwmon->rg.pkg_rapl_limit)) > > + return; > > + > > + /* Take mutex to prevent concurrent hwm_power_max_write */ > > + mutex_lock(&hwmon->hwmon_lock); > > + > > + with_intel_runtime_pm(hwmon->ddat.uncore->rpm, wakeref) > > + r = intel_uncore_rmw(hwmon->ddat.uncore, > > + hwmon->rg.pkg_rapl_limit, > > + PKG_PWR_LIM_1_EN, 0); > Most of this code (lock and rmw parts) is already inside static void > hwm_locked_with_pm_intel_uncore_rmw() , can we reuse that here? This was the case in v1 of the patch: https://patchwork.freedesktop.org/patch/526393/?series=115003&rev=1 But now this cannot be done because if you notice we acquire the mutex in i915_hwmon_power_max_disable() and release the mutex in i915_hwmon_power_max_restore(). I explained the reason why this the mutex is handled this way in my reply to Jani Nikula here: https://patchwork.freedesktop.org/patch/526598/?series=115003&rev=2 Quoting below: ``` > > + /* hwmon_lock mutex is unlocked in hwm_power_max_restore */ > > Not too happy about that... any better ideas? Afais, taking the mutex is the only fully correct solution (when we disable the power limit, userspace can go re-enable it). Examples of partly incorrect solutions (which don't take the mutex) include: a. Don't take the mutex, don't do anything, ignore any changes to the value if it has changed during GuC reset/fw load (just overwrite the changed value). Con: changed value is lost. b. Detect if the value has changed (the limit has been re-enabled) after we have disabled the limit and in that case skip restoring the value. But then someone can say why do we allow enabling the PL1 limit since we want to disable it. Both these are very unlikely scenarios so they might work. But I would first like to explore if holding a mutex across GuC reset is prolebmatic since that is /the/ correct solution. But if anyone comes up with a reason why that cannot be done we can look at these other not completely correct options. ``` > > + > > + *old = !!(r & PKG_PWR_LIM_1_EN); > > +} > > + > > +void i915_hwmon_power_max_restore(struct drm_i915_private *i915, bool old) > > + __releases(i915->hwmon->hwmon_lock) > We can just call this i915_hwmon_power_max_enable() and call whenever the > old value was actually enabled. That way, we have proper mirror functions. As I explained that would mean adding two checks in the main __uc_init_hw() function which I am trying to avoid. So we have disable/restore pair. > > +{ > > + struct i915_hwmon *hwmon = i915->hwmon; > > + intel_wakeref_t wakeref; > > + > > + if (!hwmon || !i915_mmio_reg_valid(hwmon->rg.pkg_rapl_limit)) > > + return; > > + > > + with_intel_runtime_pm(hwmon->ddat.uncore->rpm, wakeref) > > + intel_uncore_rmw(hwmon->ddat.uncore, > > + hwmon->rg.pkg_rapl_limit, > > + PKG_PWR_LIM_1_EN, > > + old ? PKG_PWR_LIM_1_EN : 0); > > 3rd param should be 0 here, else we will end up clearing other bits. No see intel_uncore_rmw(), it will only clear the PKG_PWR_LIM_1_EN bit, so the code here is correct. intel_uncore_rmw() does: val = (old & ~clear) | set; So for now I am not making any changes, if you feel strongly about something one way or another let me know. Anyway these comments should help you understand the patch better so take a look and we can go from there. Thanks. -- Ashutosh > > + > > + mutex_unlock(&hwmon->hwmon_lock); > > +} > > + > > static umode_t > > hwm_energy_is_visible(const struct hwm_drvdata *ddat, u32 attr) > > { > > diff --git a/drivers/gpu/drm/i915/i915_hwmon.h b/drivers/gpu/drm/i915/i915_hwmon.h > > index 7ca9cf2c34c96..0fcb7de844061 100644 > > --- a/drivers/gpu/drm/i915/i915_hwmon.h > > +++ b/drivers/gpu/drm/i915/i915_hwmon.h > > @@ -7,14 +7,21 @@ > > #ifndef __I915_HWMON_H__ > > #define __I915_HWMON_H__ > > +#include <linux/types.h> > > + > > struct drm_i915_private; > > +struct intel_gt; > > #if IS_REACHABLE(CONFIG_HWMON) > > void i915_hwmon_register(struct drm_i915_private *i915); > > void i915_hwmon_unregister(struct drm_i915_private *i915); > > +void i915_hwmon_power_max_disable(struct drm_i915_private *i915, bool *old); > > +void i915_hwmon_power_max_restore(struct drm_i915_private *i915, bool old); > > #else > > static inline void i915_hwmon_register(struct drm_i915_private *i915) { }; > > static inline void i915_hwmon_unregister(struct drm_i915_private *i915) { }; > > +static inline void i915_hwmon_power_max_disable(struct drm_i915_private *i915, bool *old) { }; > > +static inline void i915_hwmon_power_max_restore(struct drm_i915_private *i915, bool old) { }; > > #endif > > #endif /* __I915_HWMON_H__ */
On 3/24/2023 4:31 PM, Dixit, Ashutosh wrote: > On Fri, 24 Mar 2023 11:15:02 -0700, Belgaumkar, Vinay wrote: > Hi Vinay, > > Thanks for the review. Comments inline below. Sorry about asking the same questions all over again :) Didn't look at previous versions. > >> On 3/15/2023 8:59 PM, Ashutosh Dixit wrote: >>> On dGfx, the PL1 power limit being enabled and set to a low value results >>> in a low GPU operating freq. It also negates the freq raise operation which >>> is done before GuC firmware load. As a result GuC firmware load can time >>> out. Such timeouts were seen in the GL #8062 bug below (where the PL1 power >>> limit was enabled and set to a low value). Therefore disable the PL1 power >>> limit when allowed by HW when loading GuC firmware. >> v3 label missing in subject. >>> v2: >>> - Take mutex (to disallow writes to power1_max) across GuC reset/fw load >>> - Add hwm_power_max_restore to error return code path >>> >>> v3 (Jani N): >>> - Add/remove explanatory comments >>> - Function renames >>> - Type corrections >>> - Locking annotation >>> >>> Link: https://gitlab.freedesktop.org/drm/intel/-/issues/8062 >>> Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com> >>> --- >>> drivers/gpu/drm/i915/gt/uc/intel_uc.c | 9 +++++++ >>> drivers/gpu/drm/i915/i915_hwmon.c | 39 +++++++++++++++++++++++++++ >>> drivers/gpu/drm/i915/i915_hwmon.h | 7 +++++ >>> 3 files changed, 55 insertions(+) >>> >>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c b/drivers/gpu/drm/i915/gt/uc/intel_uc.c >>> index 4ccb4be4c9cba..aa8e35a5636a0 100644 >>> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c >>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c >>> @@ -18,6 +18,7 @@ >>> #include "intel_uc.h" >>> #include "i915_drv.h" >>> +#include "i915_hwmon.h" >>> static const struct intel_uc_ops uc_ops_off; >>> static const struct intel_uc_ops uc_ops_on; >>> @@ -461,6 +462,7 @@ static int __uc_init_hw(struct intel_uc *uc) >>> struct intel_guc *guc = &uc->guc; >>> struct intel_huc *huc = &uc->huc; >>> int ret, attempts; >>> + bool pl1en; >> Init to 'false' here > See next comment. > >> >>> GEM_BUG_ON(!intel_uc_supports_guc(uc)); >>> GEM_BUG_ON(!intel_uc_wants_guc(uc)); >>> @@ -491,6 +493,9 @@ static int __uc_init_hw(struct intel_uc *uc) >>> else >>> attempts = 1; >>> + /* Disable a potentially low PL1 power limit to allow freq to be >>> raised */ >>> + i915_hwmon_power_max_disable(gt->i915, &pl1en); >>> + >>> intel_rps_raise_unslice(&uc_to_gt(uc)->rps); >>> while (attempts--) { >>> @@ -547,6 +552,8 @@ static int __uc_init_hw(struct intel_uc *uc) >>> intel_rps_lower_unslice(&uc_to_gt(uc)->rps); >>> } >>> + i915_hwmon_power_max_restore(gt->i915, pl1en); >>> + >>> guc_info(guc, "submission %s\n", str_enabled_disabled(intel_uc_uses_guc_submission(uc))); >>> guc_info(guc, "SLPC %s\n", str_enabled_disabled(intel_uc_uses_guc_slpc(uc))); >>> @@ -563,6 +570,8 @@ static int __uc_init_hw(struct intel_uc *uc) >>> /* Return GT back to RPn */ >>> intel_rps_lower_unslice(&uc_to_gt(uc)->rps); >>> + i915_hwmon_power_max_restore(gt->i915, pl1en); >> if (pl1en) >> >> i915_hwmon_power_max_enable(). > IMO it's better not to have checks in the main __uc_init_hw() function (if > we do this we'll need to add 2 checks in __uc_init_hw()). If you really > want we could do something like this inside > i915_hwmon_power_max_disable/i915_hwmon_power_max_restore. But for now I > am not making any changes. ok. > > (I can send a patch with the changes if you want to take a look but IMO it > will add more logic/code but without real benefits (it will save a rmw if > the limit was already disabled, but IMO this code is called so infrequently > (only during GuC resets) as to not have any significant impact)). > >>> + >>> __uc_sanitize(uc); >>> if (!ret) { >>> diff --git a/drivers/gpu/drm/i915/i915_hwmon.c b/drivers/gpu/drm/i915/i915_hwmon.c >>> index ee63a8fd88fc1..769b5bda4d53f 100644 >>> --- a/drivers/gpu/drm/i915/i915_hwmon.c >>> +++ b/drivers/gpu/drm/i915/i915_hwmon.c >>> @@ -444,6 +444,45 @@ hwm_power_write(struct hwm_drvdata *ddat, u32 attr, int chan, long val) >>> } >>> } >>> +void i915_hwmon_power_max_disable(struct drm_i915_private *i915, bool >>> *old) >> Shouldn't we call this i915_hwmon_package_pl1_disable()? > I did think of using "pl1" in the function name but then decided to retain > "power_max" because other hwmon functions for PL1 limit also use > "power_max" (hwm_power_max_read/hwm_power_max_write) and currently > "hwmon_power_max" is mapped to the PL1 limit. So "power_max" is used to > show that all these functions deal with the PL1 power limit. > > There is a comment in __uc_init_hw() explaining "power_max" means the PL1 > power limit. ok. > >>> + __acquires(i915->hwmon->hwmon_lock) >>> +{ >>> + struct i915_hwmon *hwmon = i915->hwmon; >>> + intel_wakeref_t wakeref; >>> + u32 r; >>> + >>> + if (!hwmon || !i915_mmio_reg_valid(hwmon->rg.pkg_rapl_limit)) >>> + return; >>> + >>> + /* Take mutex to prevent concurrent hwm_power_max_write */ >>> + mutex_lock(&hwmon->hwmon_lock); >>> + >>> + with_intel_runtime_pm(hwmon->ddat.uncore->rpm, wakeref) >>> + r = intel_uncore_rmw(hwmon->ddat.uncore, >>> + hwmon->rg.pkg_rapl_limit, >>> + PKG_PWR_LIM_1_EN, 0); >> Most of this code (lock and rmw parts) is already inside static void >> hwm_locked_with_pm_intel_uncore_rmw() , can we reuse that here? > This was the case in v1 of the patch: > > https://patchwork.freedesktop.org/patch/526393/?series=115003&rev=1 > > But now this cannot be done because if you notice we acquire the mutex in > i915_hwmon_power_max_disable() and release the mutex in > i915_hwmon_power_max_restore(). > > I explained the reason why this the mutex is handled this way in my reply > to Jani Nikula here: > > https://patchwork.freedesktop.org/patch/526598/?series=115003&rev=2 > > Quoting below: > > ``` >>> + /* hwmon_lock mutex is unlocked in hwm_power_max_restore */ >> Not too happy about that... any better ideas? > Afais, taking the mutex is the only fully correct solution (when we disable > the power limit, userspace can go re-enable it). Examples of partly > incorrect solutions (which don't take the mutex) include: > > a. Don't take the mutex, don't do anything, ignore any changes to the value > if it has changed during GuC reset/fw load (just overwrite the changed > value). Con: changed value is lost. > > b. Detect if the value has changed (the limit has been re-enabled) after we > have disabled the limit and in that case skip restoring the value. But > then someone can say why do we allow enabling the PL1 limit since we > want to disable it. > > Both these are very unlikely scenarios so they might work. But I would > first like to explore if holding a mutex across GuC reset is prolebmatic > since that is /the/ correct solution. But if anyone comes up with a reason > why that cannot be done we can look at these other not completely correct > options. Well, one reason is that this is adding a lot of duplicate/non-reusable code needlessly. If it gets re-used elsewhere, that could lead to some weird situations where the lock could be held for an extended period of time and introduce dependencies. Also, how/why would the user modify this PL1 during guc load? The sysfs interfaces are not even ready at this point? Even if we consider this during a resume, the terminal will not be available to the user. Thanks, Vinay. > ``` > >>> + >>> + *old = !!(r & PKG_PWR_LIM_1_EN); >>> +} >>> + >>> +void i915_hwmon_power_max_restore(struct drm_i915_private *i915, bool old) >>> + __releases(i915->hwmon->hwmon_lock) >> We can just call this i915_hwmon_power_max_enable() and call whenever the >> old value was actually enabled. That way, we have proper mirror functions. > As I explained that would mean adding two checks in the main __uc_init_hw() > function which I am trying to avoid. So we have disable/restore pair. > >>> +{ >>> + struct i915_hwmon *hwmon = i915->hwmon; >>> + intel_wakeref_t wakeref; >>> + >>> + if (!hwmon || !i915_mmio_reg_valid(hwmon->rg.pkg_rapl_limit)) >>> + return; >>> + >>> + with_intel_runtime_pm(hwmon->ddat.uncore->rpm, wakeref) >>> + intel_uncore_rmw(hwmon->ddat.uncore, >>> + hwmon->rg.pkg_rapl_limit, >>> + PKG_PWR_LIM_1_EN, >>> + old ? PKG_PWR_LIM_1_EN : 0); >> 3rd param should be 0 here, else we will end up clearing other bits. > No see intel_uncore_rmw(), it will only clear the PKG_PWR_LIM_1_EN bit, so > the code here is correct. intel_uncore_rmw() does: > > val = (old & ~clear) | set; Ok, just confusing, since you are also setting it with the 4th param. > > So for now I am not making any changes, if you feel strongly about > something one way or another let me know. Anyway these comments should help > you understand the patch better so take a look and we can go from there. > > Thanks. > -- > Ashutosh > >>> + >>> + mutex_unlock(&hwmon->hwmon_lock); >>> +} >>> + >>> static umode_t >>> hwm_energy_is_visible(const struct hwm_drvdata *ddat, u32 attr) >>> { >>> diff --git a/drivers/gpu/drm/i915/i915_hwmon.h b/drivers/gpu/drm/i915/i915_hwmon.h >>> index 7ca9cf2c34c96..0fcb7de844061 100644 >>> --- a/drivers/gpu/drm/i915/i915_hwmon.h >>> +++ b/drivers/gpu/drm/i915/i915_hwmon.h >>> @@ -7,14 +7,21 @@ >>> #ifndef __I915_HWMON_H__ >>> #define __I915_HWMON_H__ >>> +#include <linux/types.h> >>> + >>> struct drm_i915_private; >>> +struct intel_gt; >>> #if IS_REACHABLE(CONFIG_HWMON) >>> void i915_hwmon_register(struct drm_i915_private *i915); >>> void i915_hwmon_unregister(struct drm_i915_private *i915); >>> +void i915_hwmon_power_max_disable(struct drm_i915_private *i915, bool *old); >>> +void i915_hwmon_power_max_restore(struct drm_i915_private *i915, bool old); >>> #else >>> static inline void i915_hwmon_register(struct drm_i915_private *i915) { }; >>> static inline void i915_hwmon_unregister(struct drm_i915_private *i915) { }; >>> +static inline void i915_hwmon_power_max_disable(struct drm_i915_private *i915, bool *old) { }; >>> +static inline void i915_hwmon_power_max_restore(struct drm_i915_private *i915, bool old) { }; >>> #endif >>> #endif /* __I915_HWMON_H__ */
On Fri, Mar 24, 2023 at 04:31:22PM -0700, Dixit, Ashutosh wrote: > On Fri, 24 Mar 2023 11:15:02 -0700, Belgaumkar, Vinay wrote: > > > > Hi Vinay, > > Thanks for the review. Comments inline below. > > > On 3/15/2023 8:59 PM, Ashutosh Dixit wrote: > > > On dGfx, the PL1 power limit being enabled and set to a low value results > > > in a low GPU operating freq. It also negates the freq raise operation which > > > is done before GuC firmware load. As a result GuC firmware load can time > > > out. Such timeouts were seen in the GL #8062 bug below (where the PL1 power > > > limit was enabled and set to a low value). Therefore disable the PL1 power > > > limit when allowed by HW when loading GuC firmware. > > v3 label missing in subject. > > > > > > v2: > > > - Take mutex (to disallow writes to power1_max) across GuC reset/fw load > > > - Add hwm_power_max_restore to error return code path > > > > > > v3 (Jani N): > > > - Add/remove explanatory comments > > > - Function renames > > > - Type corrections > > > - Locking annotation > > > > > > Link: https://gitlab.freedesktop.org/drm/intel/-/issues/8062 > > > Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com> > > > --- > > > drivers/gpu/drm/i915/gt/uc/intel_uc.c | 9 +++++++ > > > drivers/gpu/drm/i915/i915_hwmon.c | 39 +++++++++++++++++++++++++++ > > > drivers/gpu/drm/i915/i915_hwmon.h | 7 +++++ > > > 3 files changed, 55 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c b/drivers/gpu/drm/i915/gt/uc/intel_uc.c > > > index 4ccb4be4c9cba..aa8e35a5636a0 100644 > > > --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c > > > +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c > > > @@ -18,6 +18,7 @@ > > > #include "intel_uc.h" > > > #include "i915_drv.h" > > > +#include "i915_hwmon.h" > > > static const struct intel_uc_ops uc_ops_off; > > > static const struct intel_uc_ops uc_ops_on; > > > @@ -461,6 +462,7 @@ static int __uc_init_hw(struct intel_uc *uc) > > > struct intel_guc *guc = &uc->guc; > > > struct intel_huc *huc = &uc->huc; > > > int ret, attempts; > > > + bool pl1en; > > > > Init to 'false' here > > See next comment. > > > > > > > > GEM_BUG_ON(!intel_uc_supports_guc(uc)); > > > GEM_BUG_ON(!intel_uc_wants_guc(uc)); > > > @@ -491,6 +493,9 @@ static int __uc_init_hw(struct intel_uc *uc) > > > else > > > attempts = 1; > > > + /* Disable a potentially low PL1 power limit to allow freq to be > > > raised */ > > > + i915_hwmon_power_max_disable(gt->i915, &pl1en); > > > + > > > intel_rps_raise_unslice(&uc_to_gt(uc)->rps); > > > while (attempts--) { > > > @@ -547,6 +552,8 @@ static int __uc_init_hw(struct intel_uc *uc) > > > intel_rps_lower_unslice(&uc_to_gt(uc)->rps); > > > } > > > + i915_hwmon_power_max_restore(gt->i915, pl1en); > > > + > > > guc_info(guc, "submission %s\n", str_enabled_disabled(intel_uc_uses_guc_submission(uc))); > > > guc_info(guc, "SLPC %s\n", str_enabled_disabled(intel_uc_uses_guc_slpc(uc))); > > > @@ -563,6 +570,8 @@ static int __uc_init_hw(struct intel_uc *uc) > > > /* Return GT back to RPn */ > > > intel_rps_lower_unslice(&uc_to_gt(uc)->rps); > > > + i915_hwmon_power_max_restore(gt->i915, pl1en); > > > > if (pl1en) > > > > i915_hwmon_power_max_enable(). > > IMO it's better not to have checks in the main __uc_init_hw() function (if > we do this we'll need to add 2 checks in __uc_init_hw()). If you really > want we could do something like this inside > i915_hwmon_power_max_disable/i915_hwmon_power_max_restore. But for now I > am not making any changes. > > (I can send a patch with the changes if you want to take a look but IMO it > will add more logic/code but without real benefits (it will save a rmw if > the limit was already disabled, but IMO this code is called so infrequently > (only during GuC resets) as to not have any significant impact)). > > > > > > + > > > __uc_sanitize(uc); > > > if (!ret) { > > > diff --git a/drivers/gpu/drm/i915/i915_hwmon.c b/drivers/gpu/drm/i915/i915_hwmon.c > > > index ee63a8fd88fc1..769b5bda4d53f 100644 > > > --- a/drivers/gpu/drm/i915/i915_hwmon.c > > > +++ b/drivers/gpu/drm/i915/i915_hwmon.c > > > @@ -444,6 +444,45 @@ hwm_power_write(struct hwm_drvdata *ddat, u32 attr, int chan, long val) > > > } > > > } > > > +void i915_hwmon_power_max_disable(struct drm_i915_private *i915, bool > > > *old) > > Shouldn't we call this i915_hwmon_package_pl1_disable()? > > I did think of using "pl1" in the function name but then decided to retain > "power_max" because other hwmon functions for PL1 limit also use > "power_max" (hwm_power_max_read/hwm_power_max_write) and currently > "hwmon_power_max" is mapped to the PL1 limit. So "power_max" is used to > show that all these functions deal with the PL1 power limit. > > There is a comment in __uc_init_hw() explaining "power_max" means the PL1 > power limit. > > > > + __acquires(i915->hwmon->hwmon_lock) > > > +{ > > > + struct i915_hwmon *hwmon = i915->hwmon; > > > + intel_wakeref_t wakeref; > > > + u32 r; > > > + > > > + if (!hwmon || !i915_mmio_reg_valid(hwmon->rg.pkg_rapl_limit)) > > > + return; > > > + > > > + /* Take mutex to prevent concurrent hwm_power_max_write */ > > > + mutex_lock(&hwmon->hwmon_lock); > > > + > > > + with_intel_runtime_pm(hwmon->ddat.uncore->rpm, wakeref) > > > + r = intel_uncore_rmw(hwmon->ddat.uncore, > > > + hwmon->rg.pkg_rapl_limit, > > > + PKG_PWR_LIM_1_EN, 0); > > Most of this code (lock and rmw parts) is already inside static void > > hwm_locked_with_pm_intel_uncore_rmw() , can we reuse that here? > > This was the case in v1 of the patch: > > https://patchwork.freedesktop.org/patch/526393/?series=115003&rev=1 > > But now this cannot be done because if you notice we acquire the mutex in > i915_hwmon_power_max_disable() and release the mutex in > i915_hwmon_power_max_restore(). > > I explained the reason why this the mutex is handled this way in my reply > to Jani Nikula here: > > https://patchwork.freedesktop.org/patch/526598/?series=115003&rev=2 > > Quoting below: > > ``` > > > + /* hwmon_lock mutex is unlocked in hwm_power_max_restore */ > > > > Not too happy about that... any better ideas? > > Afais, taking the mutex is the only fully correct solution (when we disable > the power limit, userspace can go re-enable it). Examples of partly > incorrect solutions (which don't take the mutex) include: > > a. Don't take the mutex, don't do anything, ignore any changes to the value > if it has changed during GuC reset/fw load (just overwrite the changed > value). Con: changed value is lost. > > b. Detect if the value has changed (the limit has been re-enabled) after we > have disabled the limit and in that case skip restoring the value. But > then someone can say why do we allow enabling the PL1 limit since we > want to disable it. > > Both these are very unlikely scenarios so they might work. But I would > first like to explore if holding a mutex across GuC reset is prolebmatic > since that is /the/ correct solution. But if anyone comes up with a reason > why that cannot be done we can look at these other not completely correct > options. I see what you are doing and it looks indeed a very safe approach to ensure the pl1 won't be toggled by other paths while we need some guaranteed state here, or hw init fails badly. But in the end you are making your lock to protect the code from another path and not protecting the data itself. The data was already protected in the first version with the lock in the rmw. maybe we need to have some kind of a state check with other state-lock and then if we are in this forced state for init path, the request for the normal path ignores and move one, or maybe we queue some request... > ``` > > > > + > > > + *old = !!(r & PKG_PWR_LIM_1_EN); > > > +} > > > + > > > +void i915_hwmon_power_max_restore(struct drm_i915_private *i915, bool old) > > > + __releases(i915->hwmon->hwmon_lock) > > We can just call this i915_hwmon_power_max_enable() and call whenever the > > old value was actually enabled. That way, we have proper mirror functions. > > As I explained that would mean adding two checks in the main __uc_init_hw() > function which I am trying to avoid. So we have disable/restore pair. > > > > +{ > > > + struct i915_hwmon *hwmon = i915->hwmon; > > > + intel_wakeref_t wakeref; > > > + > > > + if (!hwmon || !i915_mmio_reg_valid(hwmon->rg.pkg_rapl_limit)) > > > + return; > > > + > > > + with_intel_runtime_pm(hwmon->ddat.uncore->rpm, wakeref) > > > + intel_uncore_rmw(hwmon->ddat.uncore, > > > + hwmon->rg.pkg_rapl_limit, > > > + PKG_PWR_LIM_1_EN, > > > + old ? PKG_PWR_LIM_1_EN : 0); > > > > 3rd param should be 0 here, else we will end up clearing other bits. > > No see intel_uncore_rmw(), it will only clear the PKG_PWR_LIM_1_EN bit, so > the code here is correct. intel_uncore_rmw() does: > > val = (old & ~clear) | set; > > So for now I am not making any changes, if you feel strongly about > something one way or another let me know. Anyway these comments should help > you understand the patch better so take a look and we can go from there. > > Thanks. > -- > Ashutosh > > > > + > > > + mutex_unlock(&hwmon->hwmon_lock); > > > +} > > > + > > > static umode_t > > > hwm_energy_is_visible(const struct hwm_drvdata *ddat, u32 attr) > > > { > > > diff --git a/drivers/gpu/drm/i915/i915_hwmon.h b/drivers/gpu/drm/i915/i915_hwmon.h > > > index 7ca9cf2c34c96..0fcb7de844061 100644 > > > --- a/drivers/gpu/drm/i915/i915_hwmon.h > > > +++ b/drivers/gpu/drm/i915/i915_hwmon.h > > > @@ -7,14 +7,21 @@ > > > #ifndef __I915_HWMON_H__ > > > #define __I915_HWMON_H__ > > > +#include <linux/types.h> > > > + > > > struct drm_i915_private; > > > +struct intel_gt; > > > #if IS_REACHABLE(CONFIG_HWMON) > > > void i915_hwmon_register(struct drm_i915_private *i915); > > > void i915_hwmon_unregister(struct drm_i915_private *i915); > > > +void i915_hwmon_power_max_disable(struct drm_i915_private *i915, bool *old); > > > +void i915_hwmon_power_max_restore(struct drm_i915_private *i915, bool old); > > > #else > > > static inline void i915_hwmon_register(struct drm_i915_private *i915) { }; > > > static inline void i915_hwmon_unregister(struct drm_i915_private *i915) { }; > > > +static inline void i915_hwmon_power_max_disable(struct drm_i915_private *i915, bool *old) { }; > > > +static inline void i915_hwmon_power_max_restore(struct drm_i915_private *i915, bool old) { }; > > > #endif > > > #endif /* __I915_HWMON_H__ */
On Fri, 24 Mar 2023 17:06:33 -0700, Belgaumkar, Vinay wrote: > Hi Vinay, > On 3/24/2023 4:31 PM, Dixit, Ashutosh wrote: > > On Fri, 24 Mar 2023 11:15:02 -0700, Belgaumkar, Vinay wrote: > > Hi Vinay, > > > > Thanks for the review. Comments inline below. > Sorry about asking the same questions all over again :) Didn't look at > previous versions. Np, the previous versions were buried somewhere anyway that's why I provided the link. > > > >> On 3/15/2023 8:59 PM, Ashutosh Dixit wrote: > >>> On dGfx, the PL1 power limit being enabled and set to a low value results > >>> in a low GPU operating freq. It also negates the freq raise operation which > >>> is done before GuC firmware load. As a result GuC firmware load can time > >>> out. Such timeouts were seen in the GL #8062 bug below (where the PL1 power > >>> limit was enabled and set to a low value). Therefore disable the PL1 power > >>> limit when allowed by HW when loading GuC firmware. > >> v3 label missing in subject. > >>> v2: > >>> - Take mutex (to disallow writes to power1_max) across GuC reset/fw load > >>> - Add hwm_power_max_restore to error return code path > >>> > >>> v3 (Jani N): > >>> - Add/remove explanatory comments > >>> - Function renames > >>> - Type corrections > >>> - Locking annotation > >>> > >>> Link: https://gitlab.freedesktop.org/drm/intel/-/issues/8062 > >>> Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com> > >>> --- > >>> drivers/gpu/drm/i915/gt/uc/intel_uc.c | 9 +++++++ > >>> drivers/gpu/drm/i915/i915_hwmon.c | 39 +++++++++++++++++++++++++++ > >>> drivers/gpu/drm/i915/i915_hwmon.h | 7 +++++ > >>> 3 files changed, 55 insertions(+) > >>> > >>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c b/drivers/gpu/drm/i915/gt/uc/intel_uc.c > >>> index 4ccb4be4c9cba..aa8e35a5636a0 100644 > >>> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c > >>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c > >>> @@ -18,6 +18,7 @@ > >>> #include "intel_uc.h" > >>> #include "i915_drv.h" > >>> +#include "i915_hwmon.h" > >>> static const struct intel_uc_ops uc_ops_off; > >>> static const struct intel_uc_ops uc_ops_on; > >>> @@ -461,6 +462,7 @@ static int __uc_init_hw(struct intel_uc *uc) > >>> struct intel_guc *guc = &uc->guc; > >>> struct intel_huc *huc = &uc->huc; > >>> int ret, attempts; > >>> + bool pl1en; > >> Init to 'false' here > > See next comment. > > > >> > >>> GEM_BUG_ON(!intel_uc_supports_guc(uc)); > >>> GEM_BUG_ON(!intel_uc_wants_guc(uc)); > >>> @@ -491,6 +493,9 @@ static int __uc_init_hw(struct intel_uc *uc) > >>> else > >>> attempts = 1; > >>> + /* Disable a potentially low PL1 power limit to allow freq to be > >>> raised */ > >>> + i915_hwmon_power_max_disable(gt->i915, &pl1en); > >>> + > >>> intel_rps_raise_unslice(&uc_to_gt(uc)->rps); > >>> while (attempts--) { > >>> @@ -547,6 +552,8 @@ static int __uc_init_hw(struct intel_uc *uc) > >>> intel_rps_lower_unslice(&uc_to_gt(uc)->rps); > >>> } > >>> + i915_hwmon_power_max_restore(gt->i915, pl1en); > >>> + > >>> guc_info(guc, "submission %s\n", str_enabled_disabled(intel_uc_uses_guc_submission(uc))); > >>> guc_info(guc, "SLPC %s\n", str_enabled_disabled(intel_uc_uses_guc_slpc(uc))); > >>> @@ -563,6 +570,8 @@ static int __uc_init_hw(struct intel_uc *uc) > >>> /* Return GT back to RPn */ > >>> intel_rps_lower_unslice(&uc_to_gt(uc)->rps); > >>> + i915_hwmon_power_max_restore(gt->i915, pl1en); > >> if (pl1en) > >> > >> i915_hwmon_power_max_enable(). > > IMO it's better not to have checks in the main __uc_init_hw() function (if > > we do this we'll need to add 2 checks in __uc_init_hw()). If you really > > want we could do something like this inside > > i915_hwmon_power_max_disable/i915_hwmon_power_max_restore. But for now I > > am not making any changes. > ok. > > > > (I can send a patch with the changes if you want to take a look but IMO it > > will add more logic/code but without real benefits (it will save a rmw if > > the limit was already disabled, but IMO this code is called so infrequently > > (only during GuC resets) as to not have any significant impact)). > > > >>> + > >>> __uc_sanitize(uc); > >>> if (!ret) { > >>> diff --git a/drivers/gpu/drm/i915/i915_hwmon.c b/drivers/gpu/drm/i915/i915_hwmon.c > >>> index ee63a8fd88fc1..769b5bda4d53f 100644 > >>> --- a/drivers/gpu/drm/i915/i915_hwmon.c > >>> +++ b/drivers/gpu/drm/i915/i915_hwmon.c > >>> @@ -444,6 +444,45 @@ hwm_power_write(struct hwm_drvdata *ddat, u32 attr, int chan, long val) > >>> } > >>> } > >>> +void i915_hwmon_power_max_disable(struct drm_i915_private *i915, bool > >>> *old) > >> Shouldn't we call this i915_hwmon_package_pl1_disable()? > > I did think of using "pl1" in the function name but then decided to retain > > "power_max" because other hwmon functions for PL1 limit also use > > "power_max" (hwm_power_max_read/hwm_power_max_write) and currently > > "hwmon_power_max" is mapped to the PL1 limit. So "power_max" is used to > > show that all these functions deal with the PL1 power limit. > > > > There is a comment in __uc_init_hw() explaining "power_max" means the PL1 > > power limit. > ok. > > > >>> + __acquires(i915->hwmon->hwmon_lock) > >>> +{ > >>> + struct i915_hwmon *hwmon = i915->hwmon; > >>> + intel_wakeref_t wakeref; > >>> + u32 r; > >>> + > >>> + if (!hwmon || !i915_mmio_reg_valid(hwmon->rg.pkg_rapl_limit)) > >>> + return; > >>> + > >>> + /* Take mutex to prevent concurrent hwm_power_max_write */ > >>> + mutex_lock(&hwmon->hwmon_lock); > >>> + > >>> + with_intel_runtime_pm(hwmon->ddat.uncore->rpm, wakeref) > >>> + r = intel_uncore_rmw(hwmon->ddat.uncore, > >>> + hwmon->rg.pkg_rapl_limit, > >>> + PKG_PWR_LIM_1_EN, 0); > >> Most of this code (lock and rmw parts) is already inside static void > >> hwm_locked_with_pm_intel_uncore_rmw() , can we reuse that here? > > This was the case in v1 of the patch: > > > > https://patchwork.freedesktop.org/patch/526393/?series=115003&rev=1 > > > > But now this cannot be done because if you notice we acquire the mutex in > > i915_hwmon_power_max_disable() and release the mutex in > > i915_hwmon_power_max_restore(). > > > > I explained the reason why this the mutex is handled this way in my reply > > to Jani Nikula here: > > > > https://patchwork.freedesktop.org/patch/526598/?series=115003&rev=2 > > > > Quoting below: > > > > ``` > >>> + /* hwmon_lock mutex is unlocked in hwm_power_max_restore */ > >> Not too happy about that... any better ideas? > > Afais, taking the mutex is the only fully correct solution (when we disable > > the power limit, userspace can go re-enable it). Examples of partly > > incorrect solutions (which don't take the mutex) include: > > > > a. Don't take the mutex, don't do anything, ignore any changes to the value > > if it has changed during GuC reset/fw load (just overwrite the changed > > value). Con: changed value is lost. > > > > b. Detect if the value has changed (the limit has been re-enabled) after we > > have disabled the limit and in that case skip restoring the value. But > > then someone can say why do we allow enabling the PL1 limit since we > > want to disable it. > > > > Both these are very unlikely scenarios so they might work. But I would > > first like to explore if holding a mutex across GuC reset is prolebmatic > > since that is /the/ correct solution. But if anyone comes up with a reason > > why that cannot be done we can look at these other not completely correct > > options. > > Well, one reason is that this is adding a lot of duplicate/non-reusable > code needlessly. If it gets re-used elsewhere, that could lead to some > weird situations where the lock could be held for an extended period of > time and introduce dependencies. The lock will only be held if userspace tries to set the PL1 limit while GuC load is in progress. The chance of this is low (though not zero). Otherwise the lock will be uncontended. But otherwise yes the lock is being held across GuC load. I would think as long as GuC load completes in the 200 ms (or even a few seconds) it should not be an issue. These hwmon operations are happening very infrequently, say in a matter of seconds, at least 100's of milliseconds. We could add a different lock here but I don't think it's worth it. > Also, how/why would the user modify this > PL1 during guc load? The sysfs interfaces are not even ready at this point? > Even if we consider this during a resume, the terminal will not be > available to the user. A few points: * Agree about probe, the sysfs doesn't exist * About resume, again agreed. But in general a userspace process (say a daemon) might be writing to hwmon sysfs even when a terminal is not available. hwmon is accessed by L0 sysman e.g. * Finally, GuC is not loaded only during probe and resume but GuC can also get reset because of error conditions. When the sysfs and the terminal are available (e.g what if you set a low PL1 limit and then run i915_hangman, it fails currently as was seen in CI, I will try to write an IGT for this). So that's the reason for taking the lock. I think it's not a big problem even if we take the lock that is why it is in the patch. > > ``` > > > >>> + > >>> + *old = !!(r & PKG_PWR_LIM_1_EN); > >>> +} > >>> + > >>> +void i915_hwmon_power_max_restore(struct drm_i915_private *i915, bool old) > >>> + __releases(i915->hwmon->hwmon_lock) > >> We can just call this i915_hwmon_power_max_enable() and call whenever the > >> old value was actually enabled. That way, we have proper mirror functions. > > As I explained that would mean adding two checks in the main __uc_init_hw() > > function which I am trying to avoid. So we have disable/restore pair. > > > >>> +{ > >>> + struct i915_hwmon *hwmon = i915->hwmon; > >>> + intel_wakeref_t wakeref; > >>> + > >>> + if (!hwmon || !i915_mmio_reg_valid(hwmon->rg.pkg_rapl_limit)) > >>> + return; > >>> + > >>> + with_intel_runtime_pm(hwmon->ddat.uncore->rpm, wakeref) > >>> + intel_uncore_rmw(hwmon->ddat.uncore, > >>> + hwmon->rg.pkg_rapl_limit, > >>> + PKG_PWR_LIM_1_EN, > >>> + old ? PKG_PWR_LIM_1_EN : 0); > >> 3rd param should be 0 here, else we will end up clearing other bits. > > No see intel_uncore_rmw(), it will only clear the PKG_PWR_LIM_1_EN bit, so > > the code here is correct. intel_uncore_rmw() does: > > > > val = (old & ~clear) | set; > Ok, just confusing, since you are also setting it with the 4th param. No, the 3rd param is clearing (old & ~clear) and the 4th param is setting (| set). Look at intel_uncore_rmw(). Also I've tested this patch ;-) Thanks. -- Ashutosh > > > > So for now I am not making any changes, if you feel strongly about > > something one way or another let me know. Anyway these comments should help > > you understand the patch better so take a look and we can go from there. > > > > Thanks. > > -- > > Ashutosh > > > >>> + > >>> + mutex_unlock(&hwmon->hwmon_lock); > >>> +} > >>> + > >>> static umode_t > >>> hwm_energy_is_visible(const struct hwm_drvdata *ddat, u32 attr) > >>> { > >>> diff --git a/drivers/gpu/drm/i915/i915_hwmon.h b/drivers/gpu/drm/i915/i915_hwmon.h > >>> index 7ca9cf2c34c96..0fcb7de844061 100644 > >>> --- a/drivers/gpu/drm/i915/i915_hwmon.h > >>> +++ b/drivers/gpu/drm/i915/i915_hwmon.h > >>> @@ -7,14 +7,21 @@ > >>> #ifndef __I915_HWMON_H__ > >>> #define __I915_HWMON_H__ > >>> +#include <linux/types.h> > >>> + > >>> struct drm_i915_private; > >>> +struct intel_gt; > >>> #if IS_REACHABLE(CONFIG_HWMON) > >>> void i915_hwmon_register(struct drm_i915_private *i915); > >>> void i915_hwmon_unregister(struct drm_i915_private *i915); > >>> +void i915_hwmon_power_max_disable(struct drm_i915_private *i915, bool *old); > >>> +void i915_hwmon_power_max_restore(struct drm_i915_private *i915, bool old); > >>> #else > >>> static inline void i915_hwmon_register(struct drm_i915_private *i915) { }; > >>> static inline void i915_hwmon_unregister(struct drm_i915_private *i915) { }; > >>> +static inline void i915_hwmon_power_max_disable(struct drm_i915_private *i915, bool *old) { }; > >>> +static inline void i915_hwmon_power_max_restore(struct drm_i915_private *i915, bool old) { }; > >>> #endif > >>> #endif /* __I915_HWMON_H__ */
On Sun, 26 Mar 2023 04:52:59 -0700, Rodrigo Vivi wrote: > Hi Rodrigo, > On Fri, Mar 24, 2023 at 04:31:22PM -0700, Dixit, Ashutosh wrote: > > On Fri, 24 Mar 2023 11:15:02 -0700, Belgaumkar, Vinay wrote: > > > > > > > Hi Vinay, > > > > Thanks for the review. Comments inline below. > > > > > On 3/15/2023 8:59 PM, Ashutosh Dixit wrote: > > > > On dGfx, the PL1 power limit being enabled and set to a low value results > > > > in a low GPU operating freq. It also negates the freq raise operation which > > > > is done before GuC firmware load. As a result GuC firmware load can time > > > > out. Such timeouts were seen in the GL #8062 bug below (where the PL1 power > > > > limit was enabled and set to a low value). Therefore disable the PL1 power > > > > limit when allowed by HW when loading GuC firmware. > > > v3 label missing in subject. > > > > > > > > v2: > > > > - Take mutex (to disallow writes to power1_max) across GuC reset/fw load > > > > - Add hwm_power_max_restore to error return code path > > > > > > > > v3 (Jani N): > > > > - Add/remove explanatory comments > > > > - Function renames > > > > - Type corrections > > > > - Locking annotation > > > > > > > > Link: https://gitlab.freedesktop.org/drm/intel/-/issues/8062 > > > > Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com> > > > > --- > > > > drivers/gpu/drm/i915/gt/uc/intel_uc.c | 9 +++++++ > > > > drivers/gpu/drm/i915/i915_hwmon.c | 39 +++++++++++++++++++++++++++ > > > > drivers/gpu/drm/i915/i915_hwmon.h | 7 +++++ > > > > 3 files changed, 55 insertions(+) > > > > > > > > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c b/drivers/gpu/drm/i915/gt/uc/intel_uc.c > > > > index 4ccb4be4c9cba..aa8e35a5636a0 100644 > > > > --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c > > > > +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c > > > > @@ -18,6 +18,7 @@ > > > > #include "intel_uc.h" > > > > #include "i915_drv.h" > > > > +#include "i915_hwmon.h" > > > > static const struct intel_uc_ops uc_ops_off; > > > > static const struct intel_uc_ops uc_ops_on; > > > > @@ -461,6 +462,7 @@ static int __uc_init_hw(struct intel_uc *uc) > > > > struct intel_guc *guc = &uc->guc; > > > > struct intel_huc *huc = &uc->huc; > > > > int ret, attempts; > > > > + bool pl1en; > > > > > > Init to 'false' here > > > > See next comment. > > > > > > > > > > > > GEM_BUG_ON(!intel_uc_supports_guc(uc)); > > > > GEM_BUG_ON(!intel_uc_wants_guc(uc)); > > > > @@ -491,6 +493,9 @@ static int __uc_init_hw(struct intel_uc *uc) > > > > else > > > > attempts = 1; > > > > + /* Disable a potentially low PL1 power limit to allow freq to be > > > > raised */ > > > > + i915_hwmon_power_max_disable(gt->i915, &pl1en); > > > > + > > > > intel_rps_raise_unslice(&uc_to_gt(uc)->rps); > > > > while (attempts--) { > > > > @@ -547,6 +552,8 @@ static int __uc_init_hw(struct intel_uc *uc) > > > > intel_rps_lower_unslice(&uc_to_gt(uc)->rps); > > > > } > > > > + i915_hwmon_power_max_restore(gt->i915, pl1en); > > > > + > > > > guc_info(guc, "submission %s\n", str_enabled_disabled(intel_uc_uses_guc_submission(uc))); > > > > guc_info(guc, "SLPC %s\n", str_enabled_disabled(intel_uc_uses_guc_slpc(uc))); > > > > @@ -563,6 +570,8 @@ static int __uc_init_hw(struct intel_uc *uc) > > > > /* Return GT back to RPn */ > > > > intel_rps_lower_unslice(&uc_to_gt(uc)->rps); > > > > + i915_hwmon_power_max_restore(gt->i915, pl1en); > > > > > > if (pl1en) > > > > > > i915_hwmon_power_max_enable(). > > > > IMO it's better not to have checks in the main __uc_init_hw() function (if > > we do this we'll need to add 2 checks in __uc_init_hw()). If you really > > want we could do something like this inside > > i915_hwmon_power_max_disable/i915_hwmon_power_max_restore. But for now I > > am not making any changes. > > > > (I can send a patch with the changes if you want to take a look but IMO it > > will add more logic/code but without real benefits (it will save a rmw if > > the limit was already disabled, but IMO this code is called so infrequently > > (only during GuC resets) as to not have any significant impact)). > > > > > > > > > + > > > > __uc_sanitize(uc); > > > > if (!ret) { > > > > diff --git a/drivers/gpu/drm/i915/i915_hwmon.c b/drivers/gpu/drm/i915/i915_hwmon.c > > > > index ee63a8fd88fc1..769b5bda4d53f 100644 > > > > --- a/drivers/gpu/drm/i915/i915_hwmon.c > > > > +++ b/drivers/gpu/drm/i915/i915_hwmon.c > > > > @@ -444,6 +444,45 @@ hwm_power_write(struct hwm_drvdata *ddat, u32 attr, int chan, long val) > > > > } > > > > } > > > > +void i915_hwmon_power_max_disable(struct drm_i915_private *i915, bool > > > > *old) > > > Shouldn't we call this i915_hwmon_package_pl1_disable()? > > > > I did think of using "pl1" in the function name but then decided to retain > > "power_max" because other hwmon functions for PL1 limit also use > > "power_max" (hwm_power_max_read/hwm_power_max_write) and currently > > "hwmon_power_max" is mapped to the PL1 limit. So "power_max" is used to > > show that all these functions deal with the PL1 power limit. > > > > There is a comment in __uc_init_hw() explaining "power_max" means the PL1 > > power limit. > > > > > > + __acquires(i915->hwmon->hwmon_lock) > > > > +{ > > > > + struct i915_hwmon *hwmon = i915->hwmon; > > > > + intel_wakeref_t wakeref; > > > > + u32 r; > > > > + > > > > + if (!hwmon || !i915_mmio_reg_valid(hwmon->rg.pkg_rapl_limit)) > > > > + return; > > > > + > > > > + /* Take mutex to prevent concurrent hwm_power_max_write */ > > > > + mutex_lock(&hwmon->hwmon_lock); > > > > + > > > > + with_intel_runtime_pm(hwmon->ddat.uncore->rpm, wakeref) > > > > + r = intel_uncore_rmw(hwmon->ddat.uncore, > > > > + hwmon->rg.pkg_rapl_limit, > > > > + PKG_PWR_LIM_1_EN, 0); > > > Most of this code (lock and rmw parts) is already inside static void > > > hwm_locked_with_pm_intel_uncore_rmw() , can we reuse that here? > > > > This was the case in v1 of the patch: > > > > https://patchwork.freedesktop.org/patch/526393/?series=115003&rev=1 > > > > But now this cannot be done because if you notice we acquire the mutex in > > i915_hwmon_power_max_disable() and release the mutex in > > i915_hwmon_power_max_restore(). > > > > I explained the reason why this the mutex is handled this way in my reply > > to Jani Nikula here: > > > > https://patchwork.freedesktop.org/patch/526598/?series=115003&rev=2 > > > > Quoting below: > > > > ``` > > > > + /* hwmon_lock mutex is unlocked in hwm_power_max_restore */ > > > > > > Not too happy about that... any better ideas? > > > > Afais, taking the mutex is the only fully correct solution (when we disable > > the power limit, userspace can go re-enable it). Examples of partly > > incorrect solutions (which don't take the mutex) include: > > > > a. Don't take the mutex, don't do anything, ignore any changes to the value > > if it has changed during GuC reset/fw load (just overwrite the changed > > value). Con: changed value is lost. > > > > b. Detect if the value has changed (the limit has been re-enabled) after we > > have disabled the limit and in that case skip restoring the value. But > > then someone can say why do we allow enabling the PL1 limit since we > > want to disable it. > > > > Both these are very unlikely scenarios so they might work. But I would > > first like to explore if holding a mutex across GuC reset is prolebmatic > > since that is /the/ correct solution. But if anyone comes up with a reason > > why that cannot be done we can look at these other not completely correct > > options. > > I see what you are doing and it looks indeed a very safe approach to ensure > the pl1 won't be toggled by other paths while we need some guaranteed state > here, or hw init fails badly. > > But in the end you are making your lock to protect the code from another path > and not protecting the data itself. The data was already protected in the > first version with the lock in the rmw. Sorry I am not really following. Daniel had mentioned this "protecting code vs protecting data" but I am wondering how it is applicable in this case. IMO here the data we are protecting is the register which we don't want written to by userland while GuC load is in progress. To do that we need to block the code path writing to register. So what we have here seems to me to be the simplest and cleanest approach for solving this issue. > maybe we need to have some kind of a state check with other state-lock and > then if we are in this forced state for init path, the request for the normal path > ignores and move one, I don't see how this will *not* be racy... > or maybe we queue some request... Queuing a request will not be enough (even if this is possible), the request will need to wait to complete till GuC load completes. So we'll have to complete the request when GuC load completes, similar to releasing the mutex in the current patch. Looks like a much more complicated way of doing what the mutex does very simply. So: a. What is the real problem with the current implementation? b. What would be the correct solution for it? That is how, specifically, should we implement it? Some more guidance will be helpful if you think this patch has issues. Thanks. -- Ashutosh > > ``` > > > > > > + > > > > + *old = !!(r & PKG_PWR_LIM_1_EN); > > > > +} > > > > + > > > > +void i915_hwmon_power_max_restore(struct drm_i915_private *i915, bool old) > > > > + __releases(i915->hwmon->hwmon_lock) > > > We can just call this i915_hwmon_power_max_enable() and call whenever the > > > old value was actually enabled. That way, we have proper mirror functions. > > > > As I explained that would mean adding two checks in the main __uc_init_hw() > > function which I am trying to avoid. So we have disable/restore pair. > > > > > > +{ > > > > + struct i915_hwmon *hwmon = i915->hwmon; > > > > + intel_wakeref_t wakeref; > > > > + > > > > + if (!hwmon || !i915_mmio_reg_valid(hwmon->rg.pkg_rapl_limit)) > > > > + return; > > > > + > > > > + with_intel_runtime_pm(hwmon->ddat.uncore->rpm, wakeref) > > > > + intel_uncore_rmw(hwmon->ddat.uncore, > > > > + hwmon->rg.pkg_rapl_limit, > > > > + PKG_PWR_LIM_1_EN, > > > > + old ? PKG_PWR_LIM_1_EN : 0); > > > > > > 3rd param should be 0 here, else we will end up clearing other bits. > > > > No see intel_uncore_rmw(), it will only clear the PKG_PWR_LIM_1_EN bit, so > > the code here is correct. intel_uncore_rmw() does: > > > > val = (old & ~clear) | set; > > > > So for now I am not making any changes, if you feel strongly about > > something one way or another let me know. Anyway these comments should help > > you understand the patch better so take a look and we can go from there. > > > > Thanks. > > -- > > Ashutosh > > > > > > + > > > > + mutex_unlock(&hwmon->hwmon_lock); > > > > +} > > > > + > > > > static umode_t > > > > hwm_energy_is_visible(const struct hwm_drvdata *ddat, u32 attr) > > > > { > > > > diff --git a/drivers/gpu/drm/i915/i915_hwmon.h b/drivers/gpu/drm/i915/i915_hwmon.h > > > > index 7ca9cf2c34c96..0fcb7de844061 100644 > > > > --- a/drivers/gpu/drm/i915/i915_hwmon.h > > > > +++ b/drivers/gpu/drm/i915/i915_hwmon.h > > > > @@ -7,14 +7,21 @@ > > > > #ifndef __I915_HWMON_H__ > > > > #define __I915_HWMON_H__ > > > > +#include <linux/types.h> > > > > + > > > > struct drm_i915_private; > > > > +struct intel_gt; > > > > #if IS_REACHABLE(CONFIG_HWMON) > > > > void i915_hwmon_register(struct drm_i915_private *i915); > > > > void i915_hwmon_unregister(struct drm_i915_private *i915); > > > > +void i915_hwmon_power_max_disable(struct drm_i915_private *i915, bool *old); > > > > +void i915_hwmon_power_max_restore(struct drm_i915_private *i915, bool old); > > > > #else > > > > static inline void i915_hwmon_register(struct drm_i915_private *i915) { }; > > > > static inline void i915_hwmon_unregister(struct drm_i915_private *i915) { }; > > > > +static inline void i915_hwmon_power_max_disable(struct drm_i915_private *i915, bool *old) { }; > > > > +static inline void i915_hwmon_power_max_restore(struct drm_i915_private *i915, bool old) { }; > > > > #endif > > > > #endif /* __I915_HWMON_H__ */
+Daniel On Mon, Mar 27, 2023 at 09:58:52AM -0700, Dixit, Ashutosh wrote: > On Sun, 26 Mar 2023 04:52:59 -0700, Rodrigo Vivi wrote: > > > > Hi Rodrigo, > > > On Fri, Mar 24, 2023 at 04:31:22PM -0700, Dixit, Ashutosh wrote: > > > On Fri, 24 Mar 2023 11:15:02 -0700, Belgaumkar, Vinay wrote: > > > > > > > > > > Hi Vinay, > > > > > > Thanks for the review. Comments inline below. > > > > > > > On 3/15/2023 8:59 PM, Ashutosh Dixit wrote: > > > > > On dGfx, the PL1 power limit being enabled and set to a low value results > > > > > in a low GPU operating freq. It also negates the freq raise operation which > > > > > is done before GuC firmware load. As a result GuC firmware load can time > > > > > out. Such timeouts were seen in the GL #8062 bug below (where the PL1 power > > > > > limit was enabled and set to a low value). Therefore disable the PL1 power > > > > > limit when allowed by HW when loading GuC firmware. > > > > v3 label missing in subject. > > > > > > > > > > v2: > > > > > - Take mutex (to disallow writes to power1_max) across GuC reset/fw load > > > > > - Add hwm_power_max_restore to error return code path > > > > > > > > > > v3 (Jani N): > > > > > - Add/remove explanatory comments > > > > > - Function renames > > > > > - Type corrections > > > > > - Locking annotation > > > > > > > > > > Link: https://gitlab.freedesktop.org/drm/intel/-/issues/8062 > > > > > Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com> > > > > > --- > > > > > drivers/gpu/drm/i915/gt/uc/intel_uc.c | 9 +++++++ > > > > > drivers/gpu/drm/i915/i915_hwmon.c | 39 +++++++++++++++++++++++++++ > > > > > drivers/gpu/drm/i915/i915_hwmon.h | 7 +++++ > > > > > 3 files changed, 55 insertions(+) > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c b/drivers/gpu/drm/i915/gt/uc/intel_uc.c > > > > > index 4ccb4be4c9cba..aa8e35a5636a0 100644 > > > > > --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c > > > > > +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c > > > > > @@ -18,6 +18,7 @@ > > > > > #include "intel_uc.h" > > > > > #include "i915_drv.h" > > > > > +#include "i915_hwmon.h" > > > > > static const struct intel_uc_ops uc_ops_off; > > > > > static const struct intel_uc_ops uc_ops_on; > > > > > @@ -461,6 +462,7 @@ static int __uc_init_hw(struct intel_uc *uc) > > > > > struct intel_guc *guc = &uc->guc; > > > > > struct intel_huc *huc = &uc->huc; > > > > > int ret, attempts; > > > > > + bool pl1en; > > > > > > > > Init to 'false' here > > > > > > See next comment. > > > > > > > > > > > > > > > > GEM_BUG_ON(!intel_uc_supports_guc(uc)); > > > > > GEM_BUG_ON(!intel_uc_wants_guc(uc)); > > > > > @@ -491,6 +493,9 @@ static int __uc_init_hw(struct intel_uc *uc) > > > > > else > > > > > attempts = 1; > > > > > + /* Disable a potentially low PL1 power limit to allow freq to be > > > > > raised */ > > > > > + i915_hwmon_power_max_disable(gt->i915, &pl1en); > > > > > + > > > > > intel_rps_raise_unslice(&uc_to_gt(uc)->rps); > > > > > while (attempts--) { > > > > > @@ -547,6 +552,8 @@ static int __uc_init_hw(struct intel_uc *uc) > > > > > intel_rps_lower_unslice(&uc_to_gt(uc)->rps); > > > > > } > > > > > + i915_hwmon_power_max_restore(gt->i915, pl1en); > > > > > + > > > > > guc_info(guc, "submission %s\n", str_enabled_disabled(intel_uc_uses_guc_submission(uc))); > > > > > guc_info(guc, "SLPC %s\n", str_enabled_disabled(intel_uc_uses_guc_slpc(uc))); > > > > > @@ -563,6 +570,8 @@ static int __uc_init_hw(struct intel_uc *uc) > > > > > /* Return GT back to RPn */ > > > > > intel_rps_lower_unslice(&uc_to_gt(uc)->rps); > > > > > + i915_hwmon_power_max_restore(gt->i915, pl1en); > > > > > > > > if (pl1en) > > > > > > > > i915_hwmon_power_max_enable(). > > > > > > IMO it's better not to have checks in the main __uc_init_hw() function (if > > > we do this we'll need to add 2 checks in __uc_init_hw()). If you really > > > want we could do something like this inside > > > i915_hwmon_power_max_disable/i915_hwmon_power_max_restore. But for now I > > > am not making any changes. > > > > > > (I can send a patch with the changes if you want to take a look but IMO it > > > will add more logic/code but without real benefits (it will save a rmw if > > > the limit was already disabled, but IMO this code is called so infrequently > > > (only during GuC resets) as to not have any significant impact)). > > > > > > > > > > > > + > > > > > __uc_sanitize(uc); > > > > > if (!ret) { > > > > > diff --git a/drivers/gpu/drm/i915/i915_hwmon.c b/drivers/gpu/drm/i915/i915_hwmon.c > > > > > index ee63a8fd88fc1..769b5bda4d53f 100644 > > > > > --- a/drivers/gpu/drm/i915/i915_hwmon.c > > > > > +++ b/drivers/gpu/drm/i915/i915_hwmon.c > > > > > @@ -444,6 +444,45 @@ hwm_power_write(struct hwm_drvdata *ddat, u32 attr, int chan, long val) > > > > > } > > > > > } > > > > > +void i915_hwmon_power_max_disable(struct drm_i915_private *i915, bool > > > > > *old) > > > > Shouldn't we call this i915_hwmon_package_pl1_disable()? > > > > > > I did think of using "pl1" in the function name but then decided to retain > > > "power_max" because other hwmon functions for PL1 limit also use > > > "power_max" (hwm_power_max_read/hwm_power_max_write) and currently > > > "hwmon_power_max" is mapped to the PL1 limit. So "power_max" is used to > > > show that all these functions deal with the PL1 power limit. > > > > > > There is a comment in __uc_init_hw() explaining "power_max" means the PL1 > > > power limit. > > > > > > > > + __acquires(i915->hwmon->hwmon_lock) > > > > > +{ > > > > > + struct i915_hwmon *hwmon = i915->hwmon; > > > > > + intel_wakeref_t wakeref; > > > > > + u32 r; > > > > > + > > > > > + if (!hwmon || !i915_mmio_reg_valid(hwmon->rg.pkg_rapl_limit)) > > > > > + return; > > > > > + > > > > > + /* Take mutex to prevent concurrent hwm_power_max_write */ > > > > > + mutex_lock(&hwmon->hwmon_lock); > > > > > + > > > > > + with_intel_runtime_pm(hwmon->ddat.uncore->rpm, wakeref) > > > > > + r = intel_uncore_rmw(hwmon->ddat.uncore, > > > > > + hwmon->rg.pkg_rapl_limit, > > > > > + PKG_PWR_LIM_1_EN, 0); > > > > Most of this code (lock and rmw parts) is already inside static void > > > > hwm_locked_with_pm_intel_uncore_rmw() , can we reuse that here? > > > > > > This was the case in v1 of the patch: > > > > > > https://patchwork.freedesktop.org/patch/526393/?series=115003&rev=1 > > > > > > But now this cannot be done because if you notice we acquire the mutex in > > > i915_hwmon_power_max_disable() and release the mutex in > > > i915_hwmon_power_max_restore(). > > > > > > I explained the reason why this the mutex is handled this way in my reply > > > to Jani Nikula here: > > > > > > https://patchwork.freedesktop.org/patch/526598/?series=115003&rev=2 > > > > > > Quoting below: > > > > > > ``` > > > > > + /* hwmon_lock mutex is unlocked in hwm_power_max_restore */ > > > > > > > > Not too happy about that... any better ideas? > > > > > > Afais, taking the mutex is the only fully correct solution (when we disable > > > the power limit, userspace can go re-enable it). Examples of partly > > > incorrect solutions (which don't take the mutex) include: > > > > > > a. Don't take the mutex, don't do anything, ignore any changes to the value > > > if it has changed during GuC reset/fw load (just overwrite the changed > > > value). Con: changed value is lost. > > > > > > b. Detect if the value has changed (the limit has been re-enabled) after we > > > have disabled the limit and in that case skip restoring the value. But > > > then someone can say why do we allow enabling the PL1 limit since we > > > want to disable it. > > > > > > Both these are very unlikely scenarios so they might work. But I would > > > first like to explore if holding a mutex across GuC reset is prolebmatic > > > since that is /the/ correct solution. But if anyone comes up with a reason > > > why that cannot be done we can look at these other not completely correct > > > options. > > > > I see what you are doing and it looks indeed a very safe approach to ensure > > the pl1 won't be toggled by other paths while we need some guaranteed state > > here, or hw init fails badly. > > > > But in the end you are making your lock to protect the code from another path > > and not protecting the data itself. The data was already protected in the > > first version with the lock in the rmw. > > Sorry I am not really following. Daniel had mentioned this "protecting code > vs protecting data" but I am wondering how it is applicable in this > case. IMO here the data we are protecting is the register which we don't > want written to by userland while GuC load is in progress. To do that we > need to block the code path writing to register. So what we have here seems > to me to be the simplest and cleanest approach for solving this issue. I believe your cases here is exactly what Daniel had mentioned as protecting code and not data. Well, in the end we are of course protecting data to be modified, but in your case you use that mutex to also protect the code path and avoid other calls while you are in this guc_init_path... Please Daniel, correct me here if I got it wrong. What I don't like here is that we lock from one function and keep that for a while and unlock from the other function. To protect the data itself in general we just need for a very minimal time while we are modifying the data itself. > > > maybe we need to have some kind of a state check with other state-lock and > > then if we are in this forced state for init path, the request for the normal path > > ignores and move one, > > I don't see how this will *not* be racy... maybe something like this?: at power_max_disable: mutex_lock(data_lock); mutex_lock(state_lock); state = in_use; mutex_unlock(state_lock); mmio_rmw(); mutex_unlock(data_lock); at power_max_restoration: at power_max_disable: mutex_lock(data_lock); mutex_lock(state_lock); state = available; mutex_unlock(state_lock); mmio_rmw(); mutex_unlock(data_lock); at sysfs fn: mutex_lock(data_lock); mutex_lock(state_lock); if (state == in_use) { ret = -EAGAIN goto out; } mutex_unlock(state_lock); .... out: mutex_unlock(data_lock); > > > or maybe we queue some request... > > Queuing a request will not be enough (even if this is possible), the > request will need to wait to complete till GuC load completes. So we'll > have to complete the request when GuC load completes, similar to releasing > the mutex in the current patch. Looks like a much more complicated way of > doing what the mutex does very simply. The wq would sleep/delay while state == in_use, then process the next request... > > So: > > a. What is the real problem with the current implementation? probably the big lock used to protect the state machinery... but if other folks believe that we don't have an actual problem here and this big lock is acceptable as long as it has the annotation for the static analyzers, I'm okay to just let it go... > > b. What would be the correct solution for it? That is how, specifically, > should we implement it? state handling with separated lock from the data itself is my suggestion. > > Some more guidance will be helpful if you think this patch has issues. I hope Daniel and/or other i915 maintainers can jump here. Specially if I'm being to paranoid and the current patch is enough... > > Thanks. > -- > Ashutosh > > > > ``` > > > > > > > > + > > > > > + *old = !!(r & PKG_PWR_LIM_1_EN); > > > > > +} > > > > > + > > > > > +void i915_hwmon_power_max_restore(struct drm_i915_private *i915, bool old) > > > > > + __releases(i915->hwmon->hwmon_lock) > > > > We can just call this i915_hwmon_power_max_enable() and call whenever the > > > > old value was actually enabled. That way, we have proper mirror functions. > > > > > > As I explained that would mean adding two checks in the main __uc_init_hw() > > > function which I am trying to avoid. So we have disable/restore pair. > > > > > > > > +{ > > > > > + struct i915_hwmon *hwmon = i915->hwmon; > > > > > + intel_wakeref_t wakeref; > > > > > + > > > > > + if (!hwmon || !i915_mmio_reg_valid(hwmon->rg.pkg_rapl_limit)) > > > > > + return; > > > > > + > > > > > + with_intel_runtime_pm(hwmon->ddat.uncore->rpm, wakeref) > > > > > + intel_uncore_rmw(hwmon->ddat.uncore, > > > > > + hwmon->rg.pkg_rapl_limit, > > > > > + PKG_PWR_LIM_1_EN, > > > > > + old ? PKG_PWR_LIM_1_EN : 0); > > > > > > > > 3rd param should be 0 here, else we will end up clearing other bits. > > > > > > No see intel_uncore_rmw(), it will only clear the PKG_PWR_LIM_1_EN bit, so > > > the code here is correct. intel_uncore_rmw() does: > > > > > > val = (old & ~clear) | set; > > > > > > So for now I am not making any changes, if you feel strongly about > > > something one way or another let me know. Anyway these comments should help > > > you understand the patch better so take a look and we can go from there. > > > > > > Thanks. > > > -- > > > Ashutosh > > > > > > > > + > > > > > + mutex_unlock(&hwmon->hwmon_lock); > > > > > +} > > > > > + > > > > > static umode_t > > > > > hwm_energy_is_visible(const struct hwm_drvdata *ddat, u32 attr) > > > > > { > > > > > diff --git a/drivers/gpu/drm/i915/i915_hwmon.h b/drivers/gpu/drm/i915/i915_hwmon.h > > > > > index 7ca9cf2c34c96..0fcb7de844061 100644 > > > > > --- a/drivers/gpu/drm/i915/i915_hwmon.h > > > > > +++ b/drivers/gpu/drm/i915/i915_hwmon.h > > > > > @@ -7,14 +7,21 @@ > > > > > #ifndef __I915_HWMON_H__ > > > > > #define __I915_HWMON_H__ > > > > > +#include <linux/types.h> > > > > > + > > > > > struct drm_i915_private; > > > > > +struct intel_gt; > > > > > #if IS_REACHABLE(CONFIG_HWMON) > > > > > void i915_hwmon_register(struct drm_i915_private *i915); > > > > > void i915_hwmon_unregister(struct drm_i915_private *i915); > > > > > +void i915_hwmon_power_max_disable(struct drm_i915_private *i915, bool *old); > > > > > +void i915_hwmon_power_max_restore(struct drm_i915_private *i915, bool old); > > > > > #else > > > > > static inline void i915_hwmon_register(struct drm_i915_private *i915) { }; > > > > > static inline void i915_hwmon_unregister(struct drm_i915_private *i915) { }; > > > > > +static inline void i915_hwmon_power_max_disable(struct drm_i915_private *i915, bool *old) { }; > > > > > +static inline void i915_hwmon_power_max_restore(struct drm_i915_private *i915, bool old) { }; > > > > > #endif > > > > > #endif /* __I915_HWMON_H__ */
On 27/03/2023 18:47, Rodrigo Vivi wrote: > > +Daniel > > On Mon, Mar 27, 2023 at 09:58:52AM -0700, Dixit, Ashutosh wrote: >> On Sun, 26 Mar 2023 04:52:59 -0700, Rodrigo Vivi wrote: >>> >> >> Hi Rodrigo, >> >>> On Fri, Mar 24, 2023 at 04:31:22PM -0700, Dixit, Ashutosh wrote: >>>> On Fri, 24 Mar 2023 11:15:02 -0700, Belgaumkar, Vinay wrote: >>>>> >>>> >>>> Hi Vinay, >>>> >>>> Thanks for the review. Comments inline below. >>>> >>>>> On 3/15/2023 8:59 PM, Ashutosh Dixit wrote: >>>>>> On dGfx, the PL1 power limit being enabled and set to a low value results >>>>>> in a low GPU operating freq. It also negates the freq raise operation which >>>>>> is done before GuC firmware load. As a result GuC firmware load can time >>>>>> out. Such timeouts were seen in the GL #8062 bug below (where the PL1 power >>>>>> limit was enabled and set to a low value). Therefore disable the PL1 power >>>>>> limit when allowed by HW when loading GuC firmware. >>>>> v3 label missing in subject. >>>>>> >>>>>> v2: >>>>>> - Take mutex (to disallow writes to power1_max) across GuC reset/fw load >>>>>> - Add hwm_power_max_restore to error return code path >>>>>> >>>>>> v3 (Jani N): >>>>>> - Add/remove explanatory comments >>>>>> - Function renames >>>>>> - Type corrections >>>>>> - Locking annotation >>>>>> >>>>>> Link: https://gitlab.freedesktop.org/drm/intel/-/issues/8062 >>>>>> Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com> >>>>>> --- >>>>>> drivers/gpu/drm/i915/gt/uc/intel_uc.c | 9 +++++++ >>>>>> drivers/gpu/drm/i915/i915_hwmon.c | 39 +++++++++++++++++++++++++++ >>>>>> drivers/gpu/drm/i915/i915_hwmon.h | 7 +++++ >>>>>> 3 files changed, 55 insertions(+) >>>>>> >>>>>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c b/drivers/gpu/drm/i915/gt/uc/intel_uc.c >>>>>> index 4ccb4be4c9cba..aa8e35a5636a0 100644 >>>>>> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c >>>>>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c >>>>>> @@ -18,6 +18,7 @@ >>>>>> #include "intel_uc.h" >>>>>> #include "i915_drv.h" >>>>>> +#include "i915_hwmon.h" >>>>>> static const struct intel_uc_ops uc_ops_off; >>>>>> static const struct intel_uc_ops uc_ops_on; >>>>>> @@ -461,6 +462,7 @@ static int __uc_init_hw(struct intel_uc *uc) >>>>>> struct intel_guc *guc = &uc->guc; >>>>>> struct intel_huc *huc = &uc->huc; >>>>>> int ret, attempts; >>>>>> + bool pl1en; >>>>> >>>>> Init to 'false' here >>>> >>>> See next comment. >>>> >>>>> >>>>> >>>>>> GEM_BUG_ON(!intel_uc_supports_guc(uc)); >>>>>> GEM_BUG_ON(!intel_uc_wants_guc(uc)); >>>>>> @@ -491,6 +493,9 @@ static int __uc_init_hw(struct intel_uc *uc) >>>>>> else >>>>>> attempts = 1; >>>>>> + /* Disable a potentially low PL1 power limit to allow freq to be >>>>>> raised */ >>>>>> + i915_hwmon_power_max_disable(gt->i915, &pl1en); >>>>>> + >>>>>> intel_rps_raise_unslice(&uc_to_gt(uc)->rps); >>>>>> while (attempts--) { >>>>>> @@ -547,6 +552,8 @@ static int __uc_init_hw(struct intel_uc *uc) >>>>>> intel_rps_lower_unslice(&uc_to_gt(uc)->rps); >>>>>> } >>>>>> + i915_hwmon_power_max_restore(gt->i915, pl1en); >>>>>> + >>>>>> guc_info(guc, "submission %s\n", str_enabled_disabled(intel_uc_uses_guc_submission(uc))); >>>>>> guc_info(guc, "SLPC %s\n", str_enabled_disabled(intel_uc_uses_guc_slpc(uc))); >>>>>> @@ -563,6 +570,8 @@ static int __uc_init_hw(struct intel_uc *uc) >>>>>> /* Return GT back to RPn */ >>>>>> intel_rps_lower_unslice(&uc_to_gt(uc)->rps); >>>>>> + i915_hwmon_power_max_restore(gt->i915, pl1en); >>>>> >>>>> if (pl1en) >>>>> >>>>> i915_hwmon_power_max_enable(). >>>> >>>> IMO it's better not to have checks in the main __uc_init_hw() function (if >>>> we do this we'll need to add 2 checks in __uc_init_hw()). If you really >>>> want we could do something like this inside >>>> i915_hwmon_power_max_disable/i915_hwmon_power_max_restore. But for now I >>>> am not making any changes. >>>> >>>> (I can send a patch with the changes if you want to take a look but IMO it >>>> will add more logic/code but without real benefits (it will save a rmw if >>>> the limit was already disabled, but IMO this code is called so infrequently >>>> (only during GuC resets) as to not have any significant impact)). >>>> >>>>> >>>>>> + >>>>>> __uc_sanitize(uc); >>>>>> if (!ret) { >>>>>> diff --git a/drivers/gpu/drm/i915/i915_hwmon.c b/drivers/gpu/drm/i915/i915_hwmon.c >>>>>> index ee63a8fd88fc1..769b5bda4d53f 100644 >>>>>> --- a/drivers/gpu/drm/i915/i915_hwmon.c >>>>>> +++ b/drivers/gpu/drm/i915/i915_hwmon.c >>>>>> @@ -444,6 +444,45 @@ hwm_power_write(struct hwm_drvdata *ddat, u32 attr, int chan, long val) >>>>>> } >>>>>> } >>>>>> +void i915_hwmon_power_max_disable(struct drm_i915_private *i915, bool >>>>>> *old) >>>>> Shouldn't we call this i915_hwmon_package_pl1_disable()? >>>> >>>> I did think of using "pl1" in the function name but then decided to retain >>>> "power_max" because other hwmon functions for PL1 limit also use >>>> "power_max" (hwm_power_max_read/hwm_power_max_write) and currently >>>> "hwmon_power_max" is mapped to the PL1 limit. So "power_max" is used to >>>> show that all these functions deal with the PL1 power limit. >>>> >>>> There is a comment in __uc_init_hw() explaining "power_max" means the PL1 >>>> power limit. >>>> >>>>>> + __acquires(i915->hwmon->hwmon_lock) >>>>>> +{ >>>>>> + struct i915_hwmon *hwmon = i915->hwmon; >>>>>> + intel_wakeref_t wakeref; >>>>>> + u32 r; >>>>>> + >>>>>> + if (!hwmon || !i915_mmio_reg_valid(hwmon->rg.pkg_rapl_limit)) >>>>>> + return; >>>>>> + >>>>>> + /* Take mutex to prevent concurrent hwm_power_max_write */ >>>>>> + mutex_lock(&hwmon->hwmon_lock); >>>>>> + >>>>>> + with_intel_runtime_pm(hwmon->ddat.uncore->rpm, wakeref) >>>>>> + r = intel_uncore_rmw(hwmon->ddat.uncore, >>>>>> + hwmon->rg.pkg_rapl_limit, >>>>>> + PKG_PWR_LIM_1_EN, 0); >>>>> Most of this code (lock and rmw parts) is already inside static void >>>>> hwm_locked_with_pm_intel_uncore_rmw() , can we reuse that here? >>>> >>>> This was the case in v1 of the patch: >>>> >>>> https://patchwork.freedesktop.org/patch/526393/?series=115003&rev=1 >>>> >>>> But now this cannot be done because if you notice we acquire the mutex in >>>> i915_hwmon_power_max_disable() and release the mutex in >>>> i915_hwmon_power_max_restore(). >>>> >>>> I explained the reason why this the mutex is handled this way in my reply >>>> to Jani Nikula here: >>>> >>>> https://patchwork.freedesktop.org/patch/526598/?series=115003&rev=2 >>>> >>>> Quoting below: >>>> >>>> ``` >>>>>> + /* hwmon_lock mutex is unlocked in hwm_power_max_restore */ >>>>> >>>>> Not too happy about that... any better ideas? >>>> >>>> Afais, taking the mutex is the only fully correct solution (when we disable >>>> the power limit, userspace can go re-enable it). Examples of partly >>>> incorrect solutions (which don't take the mutex) include: >>>> >>>> a. Don't take the mutex, don't do anything, ignore any changes to the value >>>> if it has changed during GuC reset/fw load (just overwrite the changed >>>> value). Con: changed value is lost. >>>> >>>> b. Detect if the value has changed (the limit has been re-enabled) after we >>>> have disabled the limit and in that case skip restoring the value. But >>>> then someone can say why do we allow enabling the PL1 limit since we >>>> want to disable it. >>>> >>>> Both these are very unlikely scenarios so they might work. But I would >>>> first like to explore if holding a mutex across GuC reset is prolebmatic >>>> since that is /the/ correct solution. But if anyone comes up with a reason >>>> why that cannot be done we can look at these other not completely correct >>>> options. >>> >>> I see what you are doing and it looks indeed a very safe approach to ensure >>> the pl1 won't be toggled by other paths while we need some guaranteed state >>> here, or hw init fails badly. >>> >>> But in the end you are making your lock to protect the code from another path >>> and not protecting the data itself. The data was already protected in the >>> first version with the lock in the rmw. >> >> Sorry I am not really following. Daniel had mentioned this "protecting code >> vs protecting data" but I am wondering how it is applicable in this >> case. IMO here the data we are protecting is the register which we don't >> want written to by userland while GuC load is in progress. To do that we >> need to block the code path writing to register. So what we have here seems >> to me to be the simplest and cleanest approach for solving this issue. > > I believe your cases here is exactly what Daniel had mentioned as protecting > code and not data. Well, in the end we are of course protecting data to be > modified, but in your case you use that mutex to also protect the code path > and avoid other calls while you are in this guc_init_path... > > Please Daniel, correct me here if I got it wrong. > > What I don't like here is that we lock from one function and keep that for a > while and unlock from the other function. To protect the data itself in general > we just need for a very minimal time while we are modifying the data itself. > >> >>> maybe we need to have some kind of a state check with other state-lock and >>> then if we are in this forced state for init path, the request for the normal path >>> ignores and move one, >> >> I don't see how this will *not* be racy... > > maybe something like this?: > > at power_max_disable: > mutex_lock(data_lock); > > mutex_lock(state_lock); > state = in_use; > mutex_unlock(state_lock); > > mmio_rmw(); > mutex_unlock(data_lock); > > > at power_max_restoration: > > at power_max_disable: > mutex_lock(data_lock); > > mutex_lock(state_lock); > state = available; > mutex_unlock(state_lock); > > mmio_rmw(); > mutex_unlock(data_lock); > > at sysfs fn: > > mutex_lock(data_lock); > mutex_lock(state_lock); > if (state == in_use) { > ret = -EAGAIN > goto out; > } > mutex_unlock(state_lock); > > .... > > out: > > mutex_unlock(data_lock); I agree holding the mutex across functions to cover the GuC init path is not the nicest pattern. Above looks a plausible improvement, although I don't know if EAGAIN is correct for hwmon, or if blocking is. Is something expected to be configuring those fields during boot and can it even handle EAGAIN? One advantage of the solution from this patch I can see though is that I think it eliminates data races (restoring the stale value) with fw reload triggered by a potential full GPU reset happening in parallel to sysfs writes. Another thing to check would be if the inversions between hwmon_lock->rpm_get and rpm_get->hwmon_lock are okay. In fact, I am not sure rpm_get in this patch is needed? Seems to be running under paths which guarantee holding it already, if I am not missing something. If not needed then there is obviously no inversion in any way. Regards, Tvrtko P.S. Do some of the exiting mutex_lock need actually be mutex_lock_interruptible so sysfs reads/write can Ctrl-C, in theory at least. >>> or maybe we queue some request... >> >> Queuing a request will not be enough (even if this is possible), the >> request will need to wait to complete till GuC load completes. So we'll >> have to complete the request when GuC load completes, similar to releasing >> the mutex in the current patch. Looks like a much more complicated way of >> doing what the mutex does very simply. > > The wq would sleep/delay while state == in_use, then process the next request... > >> >> So: >> >> a. What is the real problem with the current implementation? > > probably the big lock used to protect the state machinery... > > but if other folks believe that we don't have an actual problem here > and this big lock is acceptable as long as it has the annotation for > the static analyzers, I'm okay to just let it go... > > >> >> b. What would be the correct solution for it? That is how, specifically, >> should we implement it? > > state handling with separated lock from the data itself is my suggestion. > >> >> Some more guidance will be helpful if you think this patch has issues. > > I hope Daniel and/or other i915 maintainers can jump here. Specially if > I'm being to paranoid and the current patch is enough... > >> >> Thanks. >> -- >> Ashutosh >> >>>> ``` >>>> >>>>>> + >>>>>> + *old = !!(r & PKG_PWR_LIM_1_EN); >>>>>> +} >>>>>> + >>>>>> +void i915_hwmon_power_max_restore(struct drm_i915_private *i915, bool old) >>>>>> + __releases(i915->hwmon->hwmon_lock) >>>>> We can just call this i915_hwmon_power_max_enable() and call whenever the >>>>> old value was actually enabled. That way, we have proper mirror functions. >>>> >>>> As I explained that would mean adding two checks in the main __uc_init_hw() >>>> function which I am trying to avoid. So we have disable/restore pair. >>>> >>>>>> +{ >>>>>> + struct i915_hwmon *hwmon = i915->hwmon; >>>>>> + intel_wakeref_t wakeref; >>>>>> + >>>>>> + if (!hwmon || !i915_mmio_reg_valid(hwmon->rg.pkg_rapl_limit)) >>>>>> + return; >>>>>> + >>>>>> + with_intel_runtime_pm(hwmon->ddat.uncore->rpm, wakeref) >>>>>> + intel_uncore_rmw(hwmon->ddat.uncore, >>>>>> + hwmon->rg.pkg_rapl_limit, >>>>>> + PKG_PWR_LIM_1_EN, >>>>>> + old ? PKG_PWR_LIM_1_EN : 0); >>>>> >>>>> 3rd param should be 0 here, else we will end up clearing other bits. >>>> >>>> No see intel_uncore_rmw(), it will only clear the PKG_PWR_LIM_1_EN bit, so >>>> the code here is correct. intel_uncore_rmw() does: >>>> >>>> val = (old & ~clear) | set; >>>> >>>> So for now I am not making any changes, if you feel strongly about >>>> something one way or another let me know. Anyway these comments should help >>>> you understand the patch better so take a look and we can go from there. >>>> >>>> Thanks. >>>> -- >>>> Ashutosh >>>> >>>>>> + >>>>>> + mutex_unlock(&hwmon->hwmon_lock); >>>>>> +} >>>>>> + >>>>>> static umode_t >>>>>> hwm_energy_is_visible(const struct hwm_drvdata *ddat, u32 attr) >>>>>> { >>>>>> diff --git a/drivers/gpu/drm/i915/i915_hwmon.h b/drivers/gpu/drm/i915/i915_hwmon.h >>>>>> index 7ca9cf2c34c96..0fcb7de844061 100644 >>>>>> --- a/drivers/gpu/drm/i915/i915_hwmon.h >>>>>> +++ b/drivers/gpu/drm/i915/i915_hwmon.h >>>>>> @@ -7,14 +7,21 @@ >>>>>> #ifndef __I915_HWMON_H__ >>>>>> #define __I915_HWMON_H__ >>>>>> +#include <linux/types.h> >>>>>> + >>>>>> struct drm_i915_private; >>>>>> +struct intel_gt; >>>>>> #if IS_REACHABLE(CONFIG_HWMON) >>>>>> void i915_hwmon_register(struct drm_i915_private *i915); >>>>>> void i915_hwmon_unregister(struct drm_i915_private *i915); >>>>>> +void i915_hwmon_power_max_disable(struct drm_i915_private *i915, bool *old); >>>>>> +void i915_hwmon_power_max_restore(struct drm_i915_private *i915, bool old); >>>>>> #else >>>>>> static inline void i915_hwmon_register(struct drm_i915_private *i915) { }; >>>>>> static inline void i915_hwmon_unregister(struct drm_i915_private *i915) { }; >>>>>> +static inline void i915_hwmon_power_max_disable(struct drm_i915_private *i915, bool *old) { }; >>>>>> +static inline void i915_hwmon_power_max_restore(struct drm_i915_private *i915, bool old) { }; >>>>>> #endif >>>>>> #endif /* __I915_HWMON_H__ */
On Mon, 27 Mar 2023 10:47:25 -0700, Rodrigo Vivi wrote: > Hi Rodrigo, Sorry for the delay, I got pulled away into a couple of other things and could only now get back to this. > > +Daniel > > On Mon, Mar 27, 2023 at 09:58:52AM -0700, Dixit, Ashutosh wrote: > > On Sun, 26 Mar 2023 04:52:59 -0700, Rodrigo Vivi wrote: > > > > > > > Hi Rodrigo, > > > > > On Fri, Mar 24, 2023 at 04:31:22PM -0700, Dixit, Ashutosh wrote: > > > > On Fri, 24 Mar 2023 11:15:02 -0700, Belgaumkar, Vinay wrote: > > > > > > > > > > > > > Hi Vinay, > > > > > > > > Thanks for the review. Comments inline below. > > > > > > > > > On 3/15/2023 8:59 PM, Ashutosh Dixit wrote: > > > > > > On dGfx, the PL1 power limit being enabled and set to a low value results > > > > > > in a low GPU operating freq. It also negates the freq raise operation which > > > > > > is done before GuC firmware load. As a result GuC firmware load can time > > > > > > out. Such timeouts were seen in the GL #8062 bug below (where the PL1 power > > > > > > limit was enabled and set to a low value). Therefore disable the PL1 power > > > > > > limit when allowed by HW when loading GuC firmware. > > > > > v3 label missing in subject. > > > > > > > > > > > > v2: > > > > > > - Take mutex (to disallow writes to power1_max) across GuC reset/fw load > > > > > > - Add hwm_power_max_restore to error return code path > > > > > > > > > > > > v3 (Jani N): > > > > > > - Add/remove explanatory comments > > > > > > - Function renames > > > > > > - Type corrections > > > > > > - Locking annotation > > > > > > > > > > > > Link: https://gitlab.freedesktop.org/drm/intel/-/issues/8062 > > > > > > Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com> > > > > > > --- > > > > > > drivers/gpu/drm/i915/gt/uc/intel_uc.c | 9 +++++++ > > > > > > drivers/gpu/drm/i915/i915_hwmon.c | 39 +++++++++++++++++++++++++++ > > > > > > drivers/gpu/drm/i915/i915_hwmon.h | 7 +++++ > > > > > > 3 files changed, 55 insertions(+) > > > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c b/drivers/gpu/drm/i915/gt/uc/intel_uc.c > > > > > > index 4ccb4be4c9cba..aa8e35a5636a0 100644 > > > > > > --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c > > > > > > +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c > > > > > > @@ -18,6 +18,7 @@ > > > > > > #include "intel_uc.h" > > > > > > #include "i915_drv.h" > > > > > > +#include "i915_hwmon.h" > > > > > > static const struct intel_uc_ops uc_ops_off; > > > > > > static const struct intel_uc_ops uc_ops_on; > > > > > > @@ -461,6 +462,7 @@ static int __uc_init_hw(struct intel_uc *uc) > > > > > > struct intel_guc *guc = &uc->guc; > > > > > > struct intel_huc *huc = &uc->huc; > > > > > > int ret, attempts; > > > > > > + bool pl1en; > > > > > > > > > > Init to 'false' here > > > > > > > > See next comment. > > > > > > > > > > > > > > > > > > > > GEM_BUG_ON(!intel_uc_supports_guc(uc)); > > > > > > GEM_BUG_ON(!intel_uc_wants_guc(uc)); > > > > > > @@ -491,6 +493,9 @@ static int __uc_init_hw(struct intel_uc *uc) > > > > > > else > > > > > > attempts = 1; > > > > > > + /* Disable a potentially low PL1 power limit to allow freq to be > > > > > > raised */ > > > > > > + i915_hwmon_power_max_disable(gt->i915, &pl1en); > > > > > > + > > > > > > intel_rps_raise_unslice(&uc_to_gt(uc)->rps); > > > > > > while (attempts--) { > > > > > > @@ -547,6 +552,8 @@ static int __uc_init_hw(struct intel_uc *uc) > > > > > > intel_rps_lower_unslice(&uc_to_gt(uc)->rps); > > > > > > } > > > > > > + i915_hwmon_power_max_restore(gt->i915, pl1en); > > > > > > + > > > > > > guc_info(guc, "submission %s\n", str_enabled_disabled(intel_uc_uses_guc_submission(uc))); > > > > > > guc_info(guc, "SLPC %s\n", str_enabled_disabled(intel_uc_uses_guc_slpc(uc))); > > > > > > @@ -563,6 +570,8 @@ static int __uc_init_hw(struct intel_uc *uc) > > > > > > /* Return GT back to RPn */ > > > > > > intel_rps_lower_unslice(&uc_to_gt(uc)->rps); > > > > > > + i915_hwmon_power_max_restore(gt->i915, pl1en); > > > > > > > > > > if (pl1en) > > > > > > > > > > i915_hwmon_power_max_enable(). > > > > > > > > IMO it's better not to have checks in the main __uc_init_hw() function (if > > > > we do this we'll need to add 2 checks in __uc_init_hw()). If you really > > > > want we could do something like this inside > > > > i915_hwmon_power_max_disable/i915_hwmon_power_max_restore. But for now I > > > > am not making any changes. > > > > > > > > (I can send a patch with the changes if you want to take a look but IMO it > > > > will add more logic/code but without real benefits (it will save a rmw if > > > > the limit was already disabled, but IMO this code is called so infrequently > > > > (only during GuC resets) as to not have any significant impact)). > > > > > > > > > > > > > > > + > > > > > > __uc_sanitize(uc); > > > > > > if (!ret) { > > > > > > diff --git a/drivers/gpu/drm/i915/i915_hwmon.c b/drivers/gpu/drm/i915/i915_hwmon.c > > > > > > index ee63a8fd88fc1..769b5bda4d53f 100644 > > > > > > --- a/drivers/gpu/drm/i915/i915_hwmon.c > > > > > > +++ b/drivers/gpu/drm/i915/i915_hwmon.c > > > > > > @@ -444,6 +444,45 @@ hwm_power_write(struct hwm_drvdata *ddat, u32 attr, int chan, long val) > > > > > > } > > > > > > } > > > > > > +void i915_hwmon_power_max_disable(struct drm_i915_private *i915, bool > > > > > > *old) > > > > > Shouldn't we call this i915_hwmon_package_pl1_disable()? > > > > > > > > I did think of using "pl1" in the function name but then decided to retain > > > > "power_max" because other hwmon functions for PL1 limit also use > > > > "power_max" (hwm_power_max_read/hwm_power_max_write) and currently > > > > "hwmon_power_max" is mapped to the PL1 limit. So "power_max" is used to > > > > show that all these functions deal with the PL1 power limit. > > > > > > > > There is a comment in __uc_init_hw() explaining "power_max" means the PL1 > > > > power limit. > > > > > > > > > > + __acquires(i915->hwmon->hwmon_lock) > > > > > > +{ > > > > > > + struct i915_hwmon *hwmon = i915->hwmon; > > > > > > + intel_wakeref_t wakeref; > > > > > > + u32 r; > > > > > > + > > > > > > + if (!hwmon || !i915_mmio_reg_valid(hwmon->rg.pkg_rapl_limit)) > > > > > > + return; > > > > > > + > > > > > > + /* Take mutex to prevent concurrent hwm_power_max_write */ > > > > > > + mutex_lock(&hwmon->hwmon_lock); > > > > > > + > > > > > > + with_intel_runtime_pm(hwmon->ddat.uncore->rpm, wakeref) > > > > > > + r = intel_uncore_rmw(hwmon->ddat.uncore, > > > > > > + hwmon->rg.pkg_rapl_limit, > > > > > > + PKG_PWR_LIM_1_EN, 0); > > > > > Most of this code (lock and rmw parts) is already inside static void > > > > > hwm_locked_with_pm_intel_uncore_rmw() , can we reuse that here? > > > > > > > > This was the case in v1 of the patch: > > > > > > > > https://patchwork.freedesktop.org/patch/526393/?series=115003&rev=1 > > > > > > > > But now this cannot be done because if you notice we acquire the mutex in > > > > i915_hwmon_power_max_disable() and release the mutex in > > > > i915_hwmon_power_max_restore(). > > > > > > > > I explained the reason why this the mutex is handled this way in my reply > > > > to Jani Nikula here: > > > > > > > > https://patchwork.freedesktop.org/patch/526598/?series=115003&rev=2 > > > > > > > > Quoting below: > > > > > > > > ``` > > > > > > + /* hwmon_lock mutex is unlocked in hwm_power_max_restore */ > > > > > > > > > > Not too happy about that... any better ideas? > > > > > > > > Afais, taking the mutex is the only fully correct solution (when we disable > > > > the power limit, userspace can go re-enable it). Examples of partly > > > > incorrect solutions (which don't take the mutex) include: > > > > > > > > a. Don't take the mutex, don't do anything, ignore any changes to the value > > > > if it has changed during GuC reset/fw load (just overwrite the changed > > > > value). Con: changed value is lost. > > > > > > > > b. Detect if the value has changed (the limit has been re-enabled) after we > > > > have disabled the limit and in that case skip restoring the value. But > > > > then someone can say why do we allow enabling the PL1 limit since we > > > > want to disable it. > > > > > > > > Both these are very unlikely scenarios so they might work. But I would > > > > first like to explore if holding a mutex across GuC reset is prolebmatic > > > > since that is /the/ correct solution. But if anyone comes up with a reason > > > > why that cannot be done we can look at these other not completely correct > > > > options. > > > > > > I see what you are doing and it looks indeed a very safe approach to ensure > > > the pl1 won't be toggled by other paths while we need some guaranteed state > > > here, or hw init fails badly. > > > > > > But in the end you are making your lock to protect the code from another path > > > and not protecting the data itself. The data was already protected in the > > > first version with the lock in the rmw. > > > > Sorry I am not really following. Daniel had mentioned this "protecting code > > vs protecting data" but I am wondering how it is applicable in this > > case. IMO here the data we are protecting is the register which we don't > > want written to by userland while GuC load is in progress. To do that we > > need to block the code path writing to register. So what we have here seems > > to me to be the simplest and cleanest approach for solving this issue. > > I believe your cases here is exactly what Daniel had mentioned as protecting > code and not data. Well, in the end we are of course protecting data to be > modified, but in your case you use that mutex to also protect the code path > and avoid other calls while you are in this guc_init_path... > > Please Daniel, correct me here if I got it wrong. > > What I don't like here is that we lock from one function and keep that for a > while and unlock from the other function. To protect the data itself in general > we just need for a very minimal time while we are modifying the data itself. > > > > > > maybe we need to have some kind of a state check with other state-lock and > > > then if we are in this forced state for init path, the request for the normal path > > > ignores and move one, > > > > I don't see how this will *not* be racy... > > maybe something like this?: > > at power_max_disable: > mutex_lock(data_lock); > > mutex_lock(state_lock); > state = in_use; > mutex_unlock(state_lock); > > mmio_rmw(); > mutex_unlock(data_lock); > > > at power_max_restoration: > > at power_max_disable: > mutex_lock(data_lock); > > mutex_lock(state_lock); > state = available; > mutex_unlock(state_lock); > > mmio_rmw(); > mutex_unlock(data_lock); > > at sysfs fn: > > mutex_lock(data_lock); > mutex_lock(state_lock); > if (state == in_use) { > ret = -EAGAIN > goto out; > } > mutex_unlock(state_lock); > > .... > > out: > > mutex_unlock(data_lock); Thanks for suggesting this, actually it worked out quite nicely and I have implemented this in the latest version. Though I believe state_lock is not needed (data_lock is sufficient) so I have skipped that. Please take a look at: https://patchwork.freedesktop.org/series/116172/ Thanks. -- Ashutosh > > > > > > or maybe we queue some request... > > > > Queuing a request will not be enough (even if this is possible), the > > request will need to wait to complete till GuC load completes. So we'll > > have to complete the request when GuC load completes, similar to releasing > > the mutex in the current patch. Looks like a much more complicated way of > > doing what the mutex does very simply. > > The wq would sleep/delay while state == in_use, then process the next request... > > > > > So: > > > > a. What is the real problem with the current implementation? > > probably the big lock used to protect the state machinery... > > but if other folks believe that we don't have an actual problem here > and this big lock is acceptable as long as it has the annotation for > the static analyzers, I'm okay to just let it go... > > > > > > b. What would be the correct solution for it? That is how, specifically, > > should we implement it? > > state handling with separated lock from the data itself is my suggestion. > > > > > Some more guidance will be helpful if you think this patch has issues. > > I hope Daniel and/or other i915 maintainers can jump here. Specially if > I'm being to paranoid and the current patch is enough... > > > > > Thanks. > > -- > > Ashutosh > > > > > > ``` > > > > > > > > > > + > > > > > > + *old = !!(r & PKG_PWR_LIM_1_EN); > > > > > > +} > > > > > > + > > > > > > +void i915_hwmon_power_max_restore(struct drm_i915_private *i915, bool old) > > > > > > + __releases(i915->hwmon->hwmon_lock) > > > > > We can just call this i915_hwmon_power_max_enable() and call whenever the > > > > > old value was actually enabled. That way, we have proper mirror functions. > > > > > > > > As I explained that would mean adding two checks in the main __uc_init_hw() > > > > function which I am trying to avoid. So we have disable/restore pair. > > > > > > > > > > +{ > > > > > > + struct i915_hwmon *hwmon = i915->hwmon; > > > > > > + intel_wakeref_t wakeref; > > > > > > + > > > > > > + if (!hwmon || !i915_mmio_reg_valid(hwmon->rg.pkg_rapl_limit)) > > > > > > + return; > > > > > > + > > > > > > + with_intel_runtime_pm(hwmon->ddat.uncore->rpm, wakeref) > > > > > > + intel_uncore_rmw(hwmon->ddat.uncore, > > > > > > + hwmon->rg.pkg_rapl_limit, > > > > > > + PKG_PWR_LIM_1_EN, > > > > > > + old ? PKG_PWR_LIM_1_EN : 0); > > > > > > > > > > 3rd param should be 0 here, else we will end up clearing other bits. > > > > > > > > No see intel_uncore_rmw(), it will only clear the PKG_PWR_LIM_1_EN bit, so > > > > the code here is correct. intel_uncore_rmw() does: > > > > > > > > val = (old & ~clear) | set; > > > > > > > > So for now I am not making any changes, if you feel strongly about > > > > something one way or another let me know. Anyway these comments should help > > > > you understand the patch better so take a look and we can go from there. > > > > > > > > Thanks. > > > > -- > > > > Ashutosh > > > > > > > > > > + > > > > > > + mutex_unlock(&hwmon->hwmon_lock); > > > > > > +} > > > > > > + > > > > > > static umode_t > > > > > > hwm_energy_is_visible(const struct hwm_drvdata *ddat, u32 attr) > > > > > > { > > > > > > diff --git a/drivers/gpu/drm/i915/i915_hwmon.h b/drivers/gpu/drm/i915/i915_hwmon.h > > > > > > index 7ca9cf2c34c96..0fcb7de844061 100644 > > > > > > --- a/drivers/gpu/drm/i915/i915_hwmon.h > > > > > > +++ b/drivers/gpu/drm/i915/i915_hwmon.h > > > > > > @@ -7,14 +7,21 @@ > > > > > > #ifndef __I915_HWMON_H__ > > > > > > #define __I915_HWMON_H__ > > > > > > +#include <linux/types.h> > > > > > > + > > > > > > struct drm_i915_private; > > > > > > +struct intel_gt; > > > > > > #if IS_REACHABLE(CONFIG_HWMON) > > > > > > void i915_hwmon_register(struct drm_i915_private *i915); > > > > > > void i915_hwmon_unregister(struct drm_i915_private *i915); > > > > > > +void i915_hwmon_power_max_disable(struct drm_i915_private *i915, bool *old); > > > > > > +void i915_hwmon_power_max_restore(struct drm_i915_private *i915, bool old); > > > > > > #else > > > > > > static inline void i915_hwmon_register(struct drm_i915_private *i915) { }; > > > > > > static inline void i915_hwmon_unregister(struct drm_i915_private *i915) { }; > > > > > > +static inline void i915_hwmon_power_max_disable(struct drm_i915_private *i915, bool *old) { }; > > > > > > +static inline void i915_hwmon_power_max_restore(struct drm_i915_private *i915, bool old) { }; > > > > > > #endif > > > > > > #endif /* __I915_HWMON_H__ */
On Tue, 28 Mar 2023 02:14:42 -0700, Tvrtko Ursulin wrote: > Hi Tvrtko, > On 27/03/2023 18:47, Rodrigo Vivi wrote: > > > > +Daniel > > > > On Mon, Mar 27, 2023 at 09:58:52AM -0700, Dixit, Ashutosh wrote: > >> On Sun, 26 Mar 2023 04:52:59 -0700, Rodrigo Vivi wrote: > >>> > >> > >> Hi Rodrigo, > >> > >>> On Fri, Mar 24, 2023 at 04:31:22PM -0700, Dixit, Ashutosh wrote: > >>>> On Fri, 24 Mar 2023 11:15:02 -0700, Belgaumkar, Vinay wrote: > >>>>> > >>>> > >>>> Hi Vinay, > >>>> > >>>> Thanks for the review. Comments inline below. > >>>> > >>>>> On 3/15/2023 8:59 PM, Ashutosh Dixit wrote: > >>>>>> On dGfx, the PL1 power limit being enabled and set to a low value results > >>>>>> in a low GPU operating freq. It also negates the freq raise operation which > >>>>>> is done before GuC firmware load. As a result GuC firmware load can time > >>>>>> out. Such timeouts were seen in the GL #8062 bug below (where the PL1 power > >>>>>> limit was enabled and set to a low value). Therefore disable the PL1 power > >>>>>> limit when allowed by HW when loading GuC firmware. > >>>>> v3 label missing in subject. > >>>>>> > >>>>>> v2: > >>>>>> - Take mutex (to disallow writes to power1_max) across GuC reset/fw load > >>>>>> - Add hwm_power_max_restore to error return code path > >>>>>> > >>>>>> v3 (Jani N): > >>>>>> - Add/remove explanatory comments > >>>>>> - Function renames > >>>>>> - Type corrections > >>>>>> - Locking annotation > >>>>>> > >>>>>> Link: https://gitlab.freedesktop.org/drm/intel/-/issues/8062 > >>>>>> Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com> > >>>>>> --- > >>>>>> drivers/gpu/drm/i915/gt/uc/intel_uc.c | 9 +++++++ > >>>>>> drivers/gpu/drm/i915/i915_hwmon.c | 39 +++++++++++++++++++++++++++ > >>>>>> drivers/gpu/drm/i915/i915_hwmon.h | 7 +++++ > >>>>>> 3 files changed, 55 insertions(+) > >>>>>> > >>>>>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c b/drivers/gpu/drm/i915/gt/uc/intel_uc.c > >>>>>> index 4ccb4be4c9cba..aa8e35a5636a0 100644 > >>>>>> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c > >>>>>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c > >>>>>> @@ -18,6 +18,7 @@ > >>>>>> #include "intel_uc.h" > >>>>>> #include "i915_drv.h" > >>>>>> +#include "i915_hwmon.h" > >>>>>> static const struct intel_uc_ops uc_ops_off; > >>>>>> static const struct intel_uc_ops uc_ops_on; > >>>>>> @@ -461,6 +462,7 @@ static int __uc_init_hw(struct intel_uc *uc) > >>>>>> struct intel_guc *guc = &uc->guc; > >>>>>> struct intel_huc *huc = &uc->huc; > >>>>>> int ret, attempts; > >>>>>> + bool pl1en; > >>>>> > >>>>> Init to 'false' here > >>>> > >>>> See next comment. > >>>> > >>>>> > >>>>> > >>>>>> GEM_BUG_ON(!intel_uc_supports_guc(uc)); > >>>>>> GEM_BUG_ON(!intel_uc_wants_guc(uc)); > >>>>>> @@ -491,6 +493,9 @@ static int __uc_init_hw(struct intel_uc *uc) > >>>>>> else > >>>>>> attempts = 1; > >>>>>> + /* Disable a potentially low PL1 power limit to allow freq to be > >>>>>> raised */ > >>>>>> + i915_hwmon_power_max_disable(gt->i915, &pl1en); > >>>>>> + > >>>>>> intel_rps_raise_unslice(&uc_to_gt(uc)->rps); > >>>>>> while (attempts--) { > >>>>>> @@ -547,6 +552,8 @@ static int __uc_init_hw(struct intel_uc *uc) > >>>>>> intel_rps_lower_unslice(&uc_to_gt(uc)->rps); > >>>>>> } > >>>>>> + i915_hwmon_power_max_restore(gt->i915, pl1en); > >>>>>> + > >>>>>> guc_info(guc, "submission %s\n", str_enabled_disabled(intel_uc_uses_guc_submission(uc))); > >>>>>> guc_info(guc, "SLPC %s\n", str_enabled_disabled(intel_uc_uses_guc_slpc(uc))); > >>>>>> @@ -563,6 +570,8 @@ static int __uc_init_hw(struct intel_uc *uc) > >>>>>> /* Return GT back to RPn */ > >>>>>> intel_rps_lower_unslice(&uc_to_gt(uc)->rps); > >>>>>> + i915_hwmon_power_max_restore(gt->i915, pl1en); > >>>>> > >>>>> if (pl1en) > >>>>> > >>>>> i915_hwmon_power_max_enable(). > >>>> > >>>> IMO it's better not to have checks in the main __uc_init_hw() function (if > >>>> we do this we'll need to add 2 checks in __uc_init_hw()). If you really > >>>> want we could do something like this inside > >>>> i915_hwmon_power_max_disable/i915_hwmon_power_max_restore. But for now I > >>>> am not making any changes. > >>>> > >>>> (I can send a patch with the changes if you want to take a look but IMO it > >>>> will add more logic/code but without real benefits (it will save a rmw if > >>>> the limit was already disabled, but IMO this code is called so infrequently > >>>> (only during GuC resets) as to not have any significant impact)). > >>>> > >>>>> > >>>>>> + > >>>>>> __uc_sanitize(uc); > >>>>>> if (!ret) { > >>>>>> diff --git a/drivers/gpu/drm/i915/i915_hwmon.c b/drivers/gpu/drm/i915/i915_hwmon.c > >>>>>> index ee63a8fd88fc1..769b5bda4d53f 100644 > >>>>>> --- a/drivers/gpu/drm/i915/i915_hwmon.c > >>>>>> +++ b/drivers/gpu/drm/i915/i915_hwmon.c > >>>>>> @@ -444,6 +444,45 @@ hwm_power_write(struct hwm_drvdata *ddat, u32 attr, int chan, long val) > >>>>>> } > >>>>>> } > >>>>>> +void i915_hwmon_power_max_disable(struct drm_i915_private *i915, bool > >>>>>> *old) > >>>>> Shouldn't we call this i915_hwmon_package_pl1_disable()? > >>>> > >>>> I did think of using "pl1" in the function name but then decided to retain > >>>> "power_max" because other hwmon functions for PL1 limit also use > >>>> "power_max" (hwm_power_max_read/hwm_power_max_write) and currently > >>>> "hwmon_power_max" is mapped to the PL1 limit. So "power_max" is used to > >>>> show that all these functions deal with the PL1 power limit. > >>>> > >>>> There is a comment in __uc_init_hw() explaining "power_max" means the PL1 > >>>> power limit. > >>>> > >>>>>> + __acquires(i915->hwmon->hwmon_lock) > >>>>>> +{ > >>>>>> + struct i915_hwmon *hwmon = i915->hwmon; > >>>>>> + intel_wakeref_t wakeref; > >>>>>> + u32 r; > >>>>>> + > >>>>>> + if (!hwmon || !i915_mmio_reg_valid(hwmon->rg.pkg_rapl_limit)) > >>>>>> + return; > >>>>>> + > >>>>>> + /* Take mutex to prevent concurrent hwm_power_max_write */ > >>>>>> + mutex_lock(&hwmon->hwmon_lock); > >>>>>> + > >>>>>> + with_intel_runtime_pm(hwmon->ddat.uncore->rpm, wakeref) > >>>>>> + r = intel_uncore_rmw(hwmon->ddat.uncore, > >>>>>> + hwmon->rg.pkg_rapl_limit, > >>>>>> + PKG_PWR_LIM_1_EN, 0); > >>>>> Most of this code (lock and rmw parts) is already inside static void > >>>>> hwm_locked_with_pm_intel_uncore_rmw() , can we reuse that here? > >>>> > >>>> This was the case in v1 of the patch: > >>>> > >>>> https://patchwork.freedesktop.org/patch/526393/?series=115003&rev=1 > >>>> > >>>> But now this cannot be done because if you notice we acquire the mutex in > >>>> i915_hwmon_power_max_disable() and release the mutex in > >>>> i915_hwmon_power_max_restore(). > >>>> > >>>> I explained the reason why this the mutex is handled this way in my reply > >>>> to Jani Nikula here: > >>>> > >>>> https://patchwork.freedesktop.org/patch/526598/?series=115003&rev=2 > >>>> > >>>> Quoting below: > >>>> > >>>> ``` > >>>>>> + /* hwmon_lock mutex is unlocked in hwm_power_max_restore */ > >>>>> > >>>>> Not too happy about that... any better ideas? > >>>> > >>>> Afais, taking the mutex is the only fully correct solution (when we disable > >>>> the power limit, userspace can go re-enable it). Examples of partly > >>>> incorrect solutions (which don't take the mutex) include: > >>>> > >>>> a. Don't take the mutex, don't do anything, ignore any changes to the value > >>>> if it has changed during GuC reset/fw load (just overwrite the changed > >>>> value). Con: changed value is lost. > >>>> > >>>> b. Detect if the value has changed (the limit has been re-enabled) after we > >>>> have disabled the limit and in that case skip restoring the value. But > >>>> then someone can say why do we allow enabling the PL1 limit since we > >>>> want to disable it. > >>>> > >>>> Both these are very unlikely scenarios so they might work. But I would > >>>> first like to explore if holding a mutex across GuC reset is prolebmatic > >>>> since that is /the/ correct solution. But if anyone comes up with a reason > >>>> why that cannot be done we can look at these other not completely correct > >>>> options. > >>> > >>> I see what you are doing and it looks indeed a very safe approach to ensure > >>> the pl1 won't be toggled by other paths while we need some guaranteed state > >>> here, or hw init fails badly. > >>> > >>> But in the end you are making your lock to protect the code from another path > >>> and not protecting the data itself. The data was already protected in the > >>> first version with the lock in the rmw. > >> > >> Sorry I am not really following. Daniel had mentioned this "protecting code > >> vs protecting data" but I am wondering how it is applicable in this > >> case. IMO here the data we are protecting is the register which we don't > >> want written to by userland while GuC load is in progress. To do that we > >> need to block the code path writing to register. So what we have here seems > >> to me to be the simplest and cleanest approach for solving this issue. > > > > I believe your cases here is exactly what Daniel had mentioned as protecting > > code and not data. Well, in the end we are of course protecting data to be > > modified, but in your case you use that mutex to also protect the code path > > and avoid other calls while you are in this guc_init_path... > > > > Please Daniel, correct me here if I got it wrong. > > > > What I don't like here is that we lock from one function and keep that for a > > while and unlock from the other function. To protect the data itself in general > > we just need for a very minimal time while we are modifying the data itself. > > > >> > >>> maybe we need to have some kind of a state check with other state-lock and > >>> then if we are in this forced state for init path, the request for the normal path > >>> ignores and move one, > >> > >> I don't see how this will *not* be racy... > > > > maybe something like this?: > > > > at power_max_disable: > > mutex_lock(data_lock); > > > > mutex_lock(state_lock); > > state = in_use; > > mutex_unlock(state_lock); > > > > mmio_rmw(); > > mutex_unlock(data_lock); > > > > > > at power_max_restoration: > > > > at power_max_disable: > > mutex_lock(data_lock); > > > > mutex_lock(state_lock); > > state = available; > > mutex_unlock(state_lock); > > > > mmio_rmw(); > > mutex_unlock(data_lock); > > > > at sysfs fn: > > > > mutex_lock(data_lock); > > mutex_lock(state_lock); > > if (state == in_use) { > > ret = -EAGAIN > > goto out; > > } > > mutex_unlock(state_lock); > > > > .... > > > > out: > > > > mutex_unlock(data_lock); > > I agree holding the mutex across functions to cover the GuC init path is > not the nicest pattern. Above looks a plausible improvement, Agree, the above turned out rather nice. > although I don't know if EAGAIN is correct for hwmon, or if blocking is. Yes blocking is the correct uapi behavior, Patch 3/3 in the latest version does this: https://patchwork.freedesktop.org/series/116172/ > Is something expected to be configuring those fields during boot and can > it even handle EAGAIN? > > One advantage of the solution from this patch I can see though is that I > think it eliminates data races (restoring the stale value) with fw reload > triggered by a potential full GPU reset happening in parallel to sysfs > writes. > > Another thing to check would be if the inversions between > hwmon_lock->rpm_get and rpm_get->hwmon_lock are okay. > > In fact, I am not sure rpm_get in this patch is needed? Seems to be running > under paths which guarantee holding it already, if I am not missing > something. If not needed then there is obviously no inversion in any way. Yes, I checked and believe you are right. So I have eliminated rpm_get in the disable/restore functions in the latest version. Thanks. -- Ashutosh > P.S. > Do some of the exiting mutex_lock need actually be mutex_lock_interruptible > so sysfs reads/write can Ctrl-C, in theory at least. > > > >>> or maybe we queue some request... > >> > >> Queuing a request will not be enough (even if this is possible), the > >> request will need to wait to complete till GuC load completes. So we'll > >> have to complete the request when GuC load completes, similar to releasing > >> the mutex in the current patch. Looks like a much more complicated way of > >> doing what the mutex does very simply. > > > > The wq would sleep/delay while state == in_use, then process the next request... > > > >> > >> So: > >> > >> a. What is the real problem with the current implementation? > > > > probably the big lock used to protect the state machinery... > > > > but if other folks believe that we don't have an actual problem here > > and this big lock is acceptable as long as it has the annotation for > > the static analyzers, I'm okay to just let it go... > > > > > >> > >> b. What would be the correct solution for it? That is how, specifically, > >> should we implement it? > > > > state handling with separated lock from the data itself is my suggestion. > > > >> > >> Some more guidance will be helpful if you think this patch has issues. > > > > I hope Daniel and/or other i915 maintainers can jump here. Specially if > > I'm being to paranoid and the current patch is enough... > > > >> > >> Thanks. > >> -- > >> Ashutosh > >> > >>>> ``` > >>>> > >>>>>> + > >>>>>> + *old = !!(r & PKG_PWR_LIM_1_EN); > >>>>>> +} > >>>>>> + > >>>>>> +void i915_hwmon_power_max_restore(struct drm_i915_private *i915, bool old) > >>>>>> + __releases(i915->hwmon->hwmon_lock) > >>>>> We can just call this i915_hwmon_power_max_enable() and call whenever the > >>>>> old value was actually enabled. That way, we have proper mirror functions. > >>>> > >>>> As I explained that would mean adding two checks in the main __uc_init_hw() > >>>> function which I am trying to avoid. So we have disable/restore pair. > >>>> > >>>>>> +{ > >>>>>> + struct i915_hwmon *hwmon = i915->hwmon; > >>>>>> + intel_wakeref_t wakeref; > >>>>>> + > >>>>>> + if (!hwmon || !i915_mmio_reg_valid(hwmon->rg.pkg_rapl_limit)) > >>>>>> + return; > >>>>>> + > >>>>>> + with_intel_runtime_pm(hwmon->ddat.uncore->rpm, wakeref) > >>>>>> + intel_uncore_rmw(hwmon->ddat.uncore, > >>>>>> + hwmon->rg.pkg_rapl_limit, > >>>>>> + PKG_PWR_LIM_1_EN, > >>>>>> + old ? PKG_PWR_LIM_1_EN : 0); > >>>>> > >>>>> 3rd param should be 0 here, else we will end up clearing other bits. > >>>> > >>>> No see intel_uncore_rmw(), it will only clear the PKG_PWR_LIM_1_EN bit, so > >>>> the code here is correct. intel_uncore_rmw() does: > >>>> > >>>> val = (old & ~clear) | set; > >>>> > >>>> So for now I am not making any changes, if you feel strongly about > >>>> something one way or another let me know. Anyway these comments should help > >>>> you understand the patch better so take a look and we can go from there. > >>>> > >>>> Thanks. > >>>> -- > >>>> Ashutosh > >>>> > >>>>>> + > >>>>>> + mutex_unlock(&hwmon->hwmon_lock); > >>>>>> +} > >>>>>> + > >>>>>> static umode_t > >>>>>> hwm_energy_is_visible(const struct hwm_drvdata *ddat, u32 attr) > >>>>>> { > >>>>>> diff --git a/drivers/gpu/drm/i915/i915_hwmon.h b/drivers/gpu/drm/i915/i915_hwmon.h > >>>>>> index 7ca9cf2c34c96..0fcb7de844061 100644 > >>>>>> --- a/drivers/gpu/drm/i915/i915_hwmon.h > >>>>>> +++ b/drivers/gpu/drm/i915/i915_hwmon.h > >>>>>> @@ -7,14 +7,21 @@ > >>>>>> #ifndef __I915_HWMON_H__ > >>>>>> #define __I915_HWMON_H__ > >>>>>> +#include <linux/types.h> > >>>>>> + > >>>>>> struct drm_i915_private; > >>>>>> +struct intel_gt; > >>>>>> #if IS_REACHABLE(CONFIG_HWMON) > >>>>>> void i915_hwmon_register(struct drm_i915_private *i915); > >>>>>> void i915_hwmon_unregister(struct drm_i915_private *i915); > >>>>>> +void i915_hwmon_power_max_disable(struct drm_i915_private *i915, bool *old); > >>>>>> +void i915_hwmon_power_max_restore(struct drm_i915_private *i915, bool old); > >>>>>> #else > >>>>>> static inline void i915_hwmon_register(struct drm_i915_private *i915) { }; > >>>>>> static inline void i915_hwmon_unregister(struct drm_i915_private *i915) { }; > >>>>>> +static inline void i915_hwmon_power_max_disable(struct drm_i915_private *i915, bool *old) { }; > >>>>>> +static inline void i915_hwmon_power_max_restore(struct drm_i915_private *i915, bool old) { }; > >>>>>> #endif > >>>>>> #endif /* __I915_HWMON_H__ */
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c b/drivers/gpu/drm/i915/gt/uc/intel_uc.c index 4ccb4be4c9cba..aa8e35a5636a0 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c @@ -18,6 +18,7 @@ #include "intel_uc.h" #include "i915_drv.h" +#include "i915_hwmon.h" static const struct intel_uc_ops uc_ops_off; static const struct intel_uc_ops uc_ops_on; @@ -461,6 +462,7 @@ static int __uc_init_hw(struct intel_uc *uc) struct intel_guc *guc = &uc->guc; struct intel_huc *huc = &uc->huc; int ret, attempts; + bool pl1en; GEM_BUG_ON(!intel_uc_supports_guc(uc)); GEM_BUG_ON(!intel_uc_wants_guc(uc)); @@ -491,6 +493,9 @@ static int __uc_init_hw(struct intel_uc *uc) else attempts = 1; + /* Disable a potentially low PL1 power limit to allow freq to be raised */ + i915_hwmon_power_max_disable(gt->i915, &pl1en); + intel_rps_raise_unslice(&uc_to_gt(uc)->rps); while (attempts--) { @@ -547,6 +552,8 @@ static int __uc_init_hw(struct intel_uc *uc) intel_rps_lower_unslice(&uc_to_gt(uc)->rps); } + i915_hwmon_power_max_restore(gt->i915, pl1en); + guc_info(guc, "submission %s\n", str_enabled_disabled(intel_uc_uses_guc_submission(uc))); guc_info(guc, "SLPC %s\n", str_enabled_disabled(intel_uc_uses_guc_slpc(uc))); @@ -563,6 +570,8 @@ static int __uc_init_hw(struct intel_uc *uc) /* Return GT back to RPn */ intel_rps_lower_unslice(&uc_to_gt(uc)->rps); + i915_hwmon_power_max_restore(gt->i915, pl1en); + __uc_sanitize(uc); if (!ret) { diff --git a/drivers/gpu/drm/i915/i915_hwmon.c b/drivers/gpu/drm/i915/i915_hwmon.c index ee63a8fd88fc1..769b5bda4d53f 100644 --- a/drivers/gpu/drm/i915/i915_hwmon.c +++ b/drivers/gpu/drm/i915/i915_hwmon.c @@ -444,6 +444,45 @@ hwm_power_write(struct hwm_drvdata *ddat, u32 attr, int chan, long val) } } +void i915_hwmon_power_max_disable(struct drm_i915_private *i915, bool *old) + __acquires(i915->hwmon->hwmon_lock) +{ + struct i915_hwmon *hwmon = i915->hwmon; + intel_wakeref_t wakeref; + u32 r; + + if (!hwmon || !i915_mmio_reg_valid(hwmon->rg.pkg_rapl_limit)) + return; + + /* Take mutex to prevent concurrent hwm_power_max_write */ + mutex_lock(&hwmon->hwmon_lock); + + with_intel_runtime_pm(hwmon->ddat.uncore->rpm, wakeref) + r = intel_uncore_rmw(hwmon->ddat.uncore, + hwmon->rg.pkg_rapl_limit, + PKG_PWR_LIM_1_EN, 0); + + *old = !!(r & PKG_PWR_LIM_1_EN); +} + +void i915_hwmon_power_max_restore(struct drm_i915_private *i915, bool old) + __releases(i915->hwmon->hwmon_lock) +{ + struct i915_hwmon *hwmon = i915->hwmon; + intel_wakeref_t wakeref; + + if (!hwmon || !i915_mmio_reg_valid(hwmon->rg.pkg_rapl_limit)) + return; + + with_intel_runtime_pm(hwmon->ddat.uncore->rpm, wakeref) + intel_uncore_rmw(hwmon->ddat.uncore, + hwmon->rg.pkg_rapl_limit, + PKG_PWR_LIM_1_EN, + old ? PKG_PWR_LIM_1_EN : 0); + + mutex_unlock(&hwmon->hwmon_lock); +} + static umode_t hwm_energy_is_visible(const struct hwm_drvdata *ddat, u32 attr) { diff --git a/drivers/gpu/drm/i915/i915_hwmon.h b/drivers/gpu/drm/i915/i915_hwmon.h index 7ca9cf2c34c96..0fcb7de844061 100644 --- a/drivers/gpu/drm/i915/i915_hwmon.h +++ b/drivers/gpu/drm/i915/i915_hwmon.h @@ -7,14 +7,21 @@ #ifndef __I915_HWMON_H__ #define __I915_HWMON_H__ +#include <linux/types.h> + struct drm_i915_private; +struct intel_gt; #if IS_REACHABLE(CONFIG_HWMON) void i915_hwmon_register(struct drm_i915_private *i915); void i915_hwmon_unregister(struct drm_i915_private *i915); +void i915_hwmon_power_max_disable(struct drm_i915_private *i915, bool *old); +void i915_hwmon_power_max_restore(struct drm_i915_private *i915, bool old); #else static inline void i915_hwmon_register(struct drm_i915_private *i915) { }; static inline void i915_hwmon_unregister(struct drm_i915_private *i915) { }; +static inline void i915_hwmon_power_max_disable(struct drm_i915_private *i915, bool *old) { }; +static inline void i915_hwmon_power_max_restore(struct drm_i915_private *i915, bool old) { }; #endif #endif /* __I915_HWMON_H__ */
On dGfx, the PL1 power limit being enabled and set to a low value results in a low GPU operating freq. It also negates the freq raise operation which is done before GuC firmware load. As a result GuC firmware load can time out. Such timeouts were seen in the GL #8062 bug below (where the PL1 power limit was enabled and set to a low value). Therefore disable the PL1 power limit when allowed by HW when loading GuC firmware. v2: - Take mutex (to disallow writes to power1_max) across GuC reset/fw load - Add hwm_power_max_restore to error return code path v3 (Jani N): - Add/remove explanatory comments - Function renames - Type corrections - Locking annotation Link: https://gitlab.freedesktop.org/drm/intel/-/issues/8062 Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com> --- drivers/gpu/drm/i915/gt/uc/intel_uc.c | 9 +++++++ drivers/gpu/drm/i915/i915_hwmon.c | 39 +++++++++++++++++++++++++++ drivers/gpu/drm/i915/i915_hwmon.h | 7 +++++ 3 files changed, 55 insertions(+)