* [Intel-gfx] [PATCH v4 0/3] drm/i915/guc: Disable PL1 power limit when loading GuC firmware @ 2023-04-06 4:45 ` Ashutosh Dixit 0 siblings, 0 replies; 31+ messages in thread From: Ashutosh Dixit @ 2023-04-06 4:45 UTC (permalink / raw) To: intel-gfx; +Cc: dri-devel, Rodrigo Vivi Split the v3 patch into 3 patches for easier review, can squash later if needed. Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> Ashutosh Dixit (3): drm/i915/hwmon: Get mutex and rpm ref just once in hwm_power_max_write drm/i915/guc: Disable PL1 power limit when loading GuC firmware drm/i915/hwmon: Block waiting for GuC reset to complete drivers/gpu/drm/i915/gt/uc/intel_uc.c | 9 ++++ drivers/gpu/drm/i915/i915_hwmon.c | 75 ++++++++++++++++++++++----- drivers/gpu/drm/i915/i915_hwmon.h | 7 +++ 3 files changed, 78 insertions(+), 13 deletions(-) -- 2.38.0 ^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH v4 0/3] drm/i915/guc: Disable PL1 power limit when loading GuC firmware @ 2023-04-06 4:45 ` Ashutosh Dixit 0 siblings, 0 replies; 31+ messages in thread From: Ashutosh Dixit @ 2023-04-06 4:45 UTC (permalink / raw) To: intel-gfx; +Cc: Tvrtko Ursulin, dri-devel, Rodrigo Vivi Split the v3 patch into 3 patches for easier review, can squash later if needed. Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> Ashutosh Dixit (3): drm/i915/hwmon: Get mutex and rpm ref just once in hwm_power_max_write drm/i915/guc: Disable PL1 power limit when loading GuC firmware drm/i915/hwmon: Block waiting for GuC reset to complete drivers/gpu/drm/i915/gt/uc/intel_uc.c | 9 ++++ drivers/gpu/drm/i915/i915_hwmon.c | 75 ++++++++++++++++++++++----- drivers/gpu/drm/i915/i915_hwmon.h | 7 +++ 3 files changed, 78 insertions(+), 13 deletions(-) -- 2.38.0 ^ permalink raw reply [flat|nested] 31+ messages in thread
* [Intel-gfx] [PATCH 1/3] drm/i915/hwmon: Get mutex and rpm ref just once in hwm_power_max_write 2023-04-06 4:45 ` Ashutosh Dixit @ 2023-04-06 4:45 ` Ashutosh Dixit -1 siblings, 0 replies; 31+ messages in thread From: Ashutosh Dixit @ 2023-04-06 4:45 UTC (permalink / raw) To: intel-gfx; +Cc: dri-devel, Rodrigo Vivi In preparation for follow-on patches, refactor hwm_power_max_write to take hwmon_lock and runtime pm wakeref at start of the function and release them at the end, therefore acquiring these just once each. Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com> --- drivers/gpu/drm/i915/i915_hwmon.c | 28 +++++++++++++++------------- 1 file changed, 15 insertions(+), 13 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_hwmon.c b/drivers/gpu/drm/i915/i915_hwmon.c index 8e7dccc8d3a0e..7f44e809ca155 100644 --- a/drivers/gpu/drm/i915/i915_hwmon.c +++ b/drivers/gpu/drm/i915/i915_hwmon.c @@ -396,31 +396,33 @@ hwm_power_max_write(struct hwm_drvdata *ddat, long val) { struct i915_hwmon *hwmon = ddat->hwmon; intel_wakeref_t wakeref; + int ret = 0; u32 nval; + mutex_lock(&hwmon->hwmon_lock); + wakeref = intel_runtime_pm_get(ddat->uncore->rpm); + /* Disable PL1 limit and verify, because the limit cannot be disabled on all platforms */ if (val == PL1_DISABLE) { - mutex_lock(&hwmon->hwmon_lock); - with_intel_runtime_pm(ddat->uncore->rpm, wakeref) { - intel_uncore_rmw(ddat->uncore, hwmon->rg.pkg_rapl_limit, - PKG_PWR_LIM_1_EN, 0); - nval = intel_uncore_read(ddat->uncore, hwmon->rg.pkg_rapl_limit); - } - mutex_unlock(&hwmon->hwmon_lock); + intel_uncore_rmw(ddat->uncore, hwmon->rg.pkg_rapl_limit, + PKG_PWR_LIM_1_EN, 0); + nval = intel_uncore_read(ddat->uncore, hwmon->rg.pkg_rapl_limit); if (nval & PKG_PWR_LIM_1_EN) - return -ENODEV; - return 0; + ret = -ENODEV; + goto exit; } /* Computation in 64-bits to avoid overflow. Round to nearest. */ nval = DIV_ROUND_CLOSEST_ULL((u64)val << hwmon->scl_shift_power, SF_POWER); nval = PKG_PWR_LIM_1_EN | REG_FIELD_PREP(PKG_PWR_LIM_1, nval); - hwm_locked_with_pm_intel_uncore_rmw(ddat, hwmon->rg.pkg_rapl_limit, - PKG_PWR_LIM_1_EN | PKG_PWR_LIM_1, - nval); - return 0; + intel_uncore_rmw(ddat->uncore, hwmon->rg.pkg_rapl_limit, + PKG_PWR_LIM_1_EN | PKG_PWR_LIM_1, nval); +exit: + intel_runtime_pm_put(ddat->uncore->rpm, wakeref); + mutex_unlock(&hwmon->hwmon_lock); + return ret; } static int -- 2.38.0 ^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH 1/3] drm/i915/hwmon: Get mutex and rpm ref just once in hwm_power_max_write @ 2023-04-06 4:45 ` Ashutosh Dixit 0 siblings, 0 replies; 31+ messages in thread From: Ashutosh Dixit @ 2023-04-06 4:45 UTC (permalink / raw) To: intel-gfx; +Cc: Tvrtko Ursulin, dri-devel, Rodrigo Vivi In preparation for follow-on patches, refactor hwm_power_max_write to take hwmon_lock and runtime pm wakeref at start of the function and release them at the end, therefore acquiring these just once each. Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com> --- drivers/gpu/drm/i915/i915_hwmon.c | 28 +++++++++++++++------------- 1 file changed, 15 insertions(+), 13 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_hwmon.c b/drivers/gpu/drm/i915/i915_hwmon.c index 8e7dccc8d3a0e..7f44e809ca155 100644 --- a/drivers/gpu/drm/i915/i915_hwmon.c +++ b/drivers/gpu/drm/i915/i915_hwmon.c @@ -396,31 +396,33 @@ hwm_power_max_write(struct hwm_drvdata *ddat, long val) { struct i915_hwmon *hwmon = ddat->hwmon; intel_wakeref_t wakeref; + int ret = 0; u32 nval; + mutex_lock(&hwmon->hwmon_lock); + wakeref = intel_runtime_pm_get(ddat->uncore->rpm); + /* Disable PL1 limit and verify, because the limit cannot be disabled on all platforms */ if (val == PL1_DISABLE) { - mutex_lock(&hwmon->hwmon_lock); - with_intel_runtime_pm(ddat->uncore->rpm, wakeref) { - intel_uncore_rmw(ddat->uncore, hwmon->rg.pkg_rapl_limit, - PKG_PWR_LIM_1_EN, 0); - nval = intel_uncore_read(ddat->uncore, hwmon->rg.pkg_rapl_limit); - } - mutex_unlock(&hwmon->hwmon_lock); + intel_uncore_rmw(ddat->uncore, hwmon->rg.pkg_rapl_limit, + PKG_PWR_LIM_1_EN, 0); + nval = intel_uncore_read(ddat->uncore, hwmon->rg.pkg_rapl_limit); if (nval & PKG_PWR_LIM_1_EN) - return -ENODEV; - return 0; + ret = -ENODEV; + goto exit; } /* Computation in 64-bits to avoid overflow. Round to nearest. */ nval = DIV_ROUND_CLOSEST_ULL((u64)val << hwmon->scl_shift_power, SF_POWER); nval = PKG_PWR_LIM_1_EN | REG_FIELD_PREP(PKG_PWR_LIM_1, nval); - hwm_locked_with_pm_intel_uncore_rmw(ddat, hwmon->rg.pkg_rapl_limit, - PKG_PWR_LIM_1_EN | PKG_PWR_LIM_1, - nval); - return 0; + intel_uncore_rmw(ddat->uncore, hwmon->rg.pkg_rapl_limit, + PKG_PWR_LIM_1_EN | PKG_PWR_LIM_1, nval); +exit: + intel_runtime_pm_put(ddat->uncore->rpm, wakeref); + mutex_unlock(&hwmon->hwmon_lock); + return ret; } static int -- 2.38.0 ^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [Intel-gfx] [PATCH 1/3] drm/i915/hwmon: Get mutex and rpm ref just once in hwm_power_max_write 2023-04-06 4:45 ` Ashutosh Dixit (?) @ 2023-04-07 11:08 ` Rodrigo Vivi -1 siblings, 0 replies; 31+ messages in thread From: Rodrigo Vivi @ 2023-04-07 11:08 UTC (permalink / raw) To: Ashutosh Dixit; +Cc: intel-gfx, dri-devel On Wed, Apr 05, 2023 at 09:45:20PM -0700, Ashutosh Dixit wrote: > In preparation for follow-on patches, refactor hwm_power_max_write to take > hwmon_lock and runtime pm wakeref at start of the function and release them > at the end, therefore acquiring these just once each. > > Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > --- > drivers/gpu/drm/i915/i915_hwmon.c | 28 +++++++++++++++------------- > 1 file changed, 15 insertions(+), 13 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_hwmon.c b/drivers/gpu/drm/i915/i915_hwmon.c > index 8e7dccc8d3a0e..7f44e809ca155 100644 > --- a/drivers/gpu/drm/i915/i915_hwmon.c > +++ b/drivers/gpu/drm/i915/i915_hwmon.c > @@ -396,31 +396,33 @@ hwm_power_max_write(struct hwm_drvdata *ddat, long val) > { > struct i915_hwmon *hwmon = ddat->hwmon; > intel_wakeref_t wakeref; > + int ret = 0; > u32 nval; > > + mutex_lock(&hwmon->hwmon_lock); > + wakeref = intel_runtime_pm_get(ddat->uncore->rpm); > + > /* Disable PL1 limit and verify, because the limit cannot be disabled on all platforms */ > if (val == PL1_DISABLE) { > - mutex_lock(&hwmon->hwmon_lock); > - with_intel_runtime_pm(ddat->uncore->rpm, wakeref) { > - intel_uncore_rmw(ddat->uncore, hwmon->rg.pkg_rapl_limit, > - PKG_PWR_LIM_1_EN, 0); > - nval = intel_uncore_read(ddat->uncore, hwmon->rg.pkg_rapl_limit); > - } > - mutex_unlock(&hwmon->hwmon_lock); > + intel_uncore_rmw(ddat->uncore, hwmon->rg.pkg_rapl_limit, > + PKG_PWR_LIM_1_EN, 0); > + nval = intel_uncore_read(ddat->uncore, hwmon->rg.pkg_rapl_limit); > > if (nval & PKG_PWR_LIM_1_EN) > - return -ENODEV; > - return 0; > + ret = -ENODEV; > + goto exit; > } > > /* Computation in 64-bits to avoid overflow. Round to nearest. */ > nval = DIV_ROUND_CLOSEST_ULL((u64)val << hwmon->scl_shift_power, SF_POWER); > nval = PKG_PWR_LIM_1_EN | REG_FIELD_PREP(PKG_PWR_LIM_1, nval); > > - hwm_locked_with_pm_intel_uncore_rmw(ddat, hwmon->rg.pkg_rapl_limit, > - PKG_PWR_LIM_1_EN | PKG_PWR_LIM_1, > - nval); > - return 0; > + intel_uncore_rmw(ddat->uncore, hwmon->rg.pkg_rapl_limit, > + PKG_PWR_LIM_1_EN | PKG_PWR_LIM_1, nval); > +exit: > + intel_runtime_pm_put(ddat->uncore->rpm, wakeref); > + mutex_unlock(&hwmon->hwmon_lock); > + return ret; > } > > static int > -- > 2.38.0 > ^ permalink raw reply [flat|nested] 31+ messages in thread
* [Intel-gfx] [PATCH 2/3] drm/i915/guc: Disable PL1 power limit when loading GuC firmware 2023-04-06 4:45 ` Ashutosh Dixit @ 2023-04-06 4:45 ` Ashutosh Dixit -1 siblings, 0 replies; 31+ messages in thread From: Ashutosh Dixit @ 2023-04-06 4:45 UTC (permalink / raw) To: intel-gfx; +Cc: dri-devel, Rodrigo Vivi 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 v4: - Don't hold the lock across GuC reset (Rodrigo) - New locking scheme (suggested by Rodrigo) - Eliminate rpm_get in power_max_disable/restore, not needed (Tvrtko) 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 | 40 +++++++++++++++++++++++++++ drivers/gpu/drm/i915/i915_hwmon.h | 7 +++++ 3 files changed, 56 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; 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 7f44e809ca155..9ab8971679fe3 100644 --- a/drivers/gpu/drm/i915/i915_hwmon.c +++ b/drivers/gpu/drm/i915/i915_hwmon.c @@ -50,6 +50,7 @@ struct hwm_drvdata { struct hwm_energy_info ei; /* Energy info for energy1_input */ char name[12]; int gt_n; + bool reset_in_progress; }; struct i915_hwmon { @@ -400,6 +401,10 @@ hwm_power_max_write(struct hwm_drvdata *ddat, long val) u32 nval; mutex_lock(&hwmon->hwmon_lock); + if (hwmon->ddat.reset_in_progress) { + ret = -EAGAIN; + goto unlock; + } wakeref = intel_runtime_pm_get(ddat->uncore->rpm); /* Disable PL1 limit and verify, because the limit cannot be disabled on all platforms */ @@ -421,6 +426,7 @@ hwm_power_max_write(struct hwm_drvdata *ddat, long val) PKG_PWR_LIM_1_EN | PKG_PWR_LIM_1, nval); exit: intel_runtime_pm_put(ddat->uncore->rpm, wakeref); +unlock: mutex_unlock(&hwmon->hwmon_lock); return ret; } @@ -472,6 +478,40 @@ 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) +{ + struct i915_hwmon *hwmon = i915->hwmon; + u32 r; + + if (!hwmon || !i915_mmio_reg_valid(hwmon->rg.pkg_rapl_limit)) + return; + + mutex_lock(&hwmon->hwmon_lock); + + hwmon->ddat.reset_in_progress = true; + 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); + + mutex_unlock(&hwmon->hwmon_lock); +} + +void i915_hwmon_power_max_restore(struct drm_i915_private *i915, bool old) +{ + struct i915_hwmon *hwmon = i915->hwmon; + + if (!hwmon || !i915_mmio_reg_valid(hwmon->rg.pkg_rapl_limit)) + return; + + mutex_lock(&hwmon->hwmon_lock); + + intel_uncore_rmw(hwmon->ddat.uncore, hwmon->rg.pkg_rapl_limit, + PKG_PWR_LIM_1_EN, old ? PKG_PWR_LIM_1_EN : 0); + hwmon->ddat.reset_in_progress = false; + + 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__ */ -- 2.38.0 ^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH 2/3] drm/i915/guc: Disable PL1 power limit when loading GuC firmware @ 2023-04-06 4:45 ` Ashutosh Dixit 0 siblings, 0 replies; 31+ messages in thread From: Ashutosh Dixit @ 2023-04-06 4:45 UTC (permalink / raw) To: intel-gfx; +Cc: Tvrtko Ursulin, dri-devel, Rodrigo Vivi 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 v4: - Don't hold the lock across GuC reset (Rodrigo) - New locking scheme (suggested by Rodrigo) - Eliminate rpm_get in power_max_disable/restore, not needed (Tvrtko) 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 | 40 +++++++++++++++++++++++++++ drivers/gpu/drm/i915/i915_hwmon.h | 7 +++++ 3 files changed, 56 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; 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 7f44e809ca155..9ab8971679fe3 100644 --- a/drivers/gpu/drm/i915/i915_hwmon.c +++ b/drivers/gpu/drm/i915/i915_hwmon.c @@ -50,6 +50,7 @@ struct hwm_drvdata { struct hwm_energy_info ei; /* Energy info for energy1_input */ char name[12]; int gt_n; + bool reset_in_progress; }; struct i915_hwmon { @@ -400,6 +401,10 @@ hwm_power_max_write(struct hwm_drvdata *ddat, long val) u32 nval; mutex_lock(&hwmon->hwmon_lock); + if (hwmon->ddat.reset_in_progress) { + ret = -EAGAIN; + goto unlock; + } wakeref = intel_runtime_pm_get(ddat->uncore->rpm); /* Disable PL1 limit and verify, because the limit cannot be disabled on all platforms */ @@ -421,6 +426,7 @@ hwm_power_max_write(struct hwm_drvdata *ddat, long val) PKG_PWR_LIM_1_EN | PKG_PWR_LIM_1, nval); exit: intel_runtime_pm_put(ddat->uncore->rpm, wakeref); +unlock: mutex_unlock(&hwmon->hwmon_lock); return ret; } @@ -472,6 +478,40 @@ 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) +{ + struct i915_hwmon *hwmon = i915->hwmon; + u32 r; + + if (!hwmon || !i915_mmio_reg_valid(hwmon->rg.pkg_rapl_limit)) + return; + + mutex_lock(&hwmon->hwmon_lock); + + hwmon->ddat.reset_in_progress = true; + 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); + + mutex_unlock(&hwmon->hwmon_lock); +} + +void i915_hwmon_power_max_restore(struct drm_i915_private *i915, bool old) +{ + struct i915_hwmon *hwmon = i915->hwmon; + + if (!hwmon || !i915_mmio_reg_valid(hwmon->rg.pkg_rapl_limit)) + return; + + mutex_lock(&hwmon->hwmon_lock); + + intel_uncore_rmw(hwmon->ddat.uncore, hwmon->rg.pkg_rapl_limit, + PKG_PWR_LIM_1_EN, old ? PKG_PWR_LIM_1_EN : 0); + hwmon->ddat.reset_in_progress = false; + + 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__ */ -- 2.38.0 ^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [Intel-gfx] [PATCH 2/3] drm/i915/guc: Disable PL1 power limit when loading GuC firmware 2023-04-06 4:45 ` Ashutosh Dixit @ 2023-04-06 9:16 ` kernel test robot -1 siblings, 0 replies; 31+ messages in thread From: kernel test robot @ 2023-04-06 9:16 UTC (permalink / raw) To: Ashutosh Dixit, intel-gfx; +Cc: Rodrigo Vivi, llvm, dri-devel, oe-kbuild-all Hi Ashutosh, kernel test robot noticed the following build warnings: [auto build test WARNING on drm-tip/drm-tip] url: https://github.com/intel-lab-lkp/linux/commits/Ashutosh-Dixit/drm-i915-hwmon-Get-mutex-and-rpm-ref-just-once-in-hwm_power_max_write/20230406-124659 base: git://anongit.freedesktop.org/drm/drm-tip drm-tip patch link: https://lore.kernel.org/r/20230406044522.3108359-3-ashutosh.dixit%40intel.com patch subject: [Intel-gfx] [PATCH 2/3] drm/i915/guc: Disable PL1 power limit when loading GuC firmware config: i386-randconfig-a002-20230403 (https://download.01.org/0day-ci/archive/20230406/202304061654.yjntbbxy-lkp@intel.com/config) compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project f28c006a5895fc0e329fe15fead81e37457cb1d1) reproduce (this is a W=1 build): wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # https://github.com/intel-lab-lkp/linux/commit/b4aa935db7f0b46437cdaa39f0149ad835ceb73c git remote add linux-review https://github.com/intel-lab-lkp/linux git fetch --no-tags linux-review Ashutosh-Dixit/drm-i915-hwmon-Get-mutex-and-rpm-ref-just-once-in-hwm_power_max_write/20230406-124659 git checkout b4aa935db7f0b46437cdaa39f0149ad835ceb73c # save the config file mkdir build_dir && cp config build_dir/.config COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 olddefconfig COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 SHELL=/bin/bash drivers/gpu/drm/i915/ If you fix the issue, kindly add following tag where applicable | Reported-by: kernel test robot <lkp@intel.com> | Link: https://lore.kernel.org/oe-kbuild-all/202304061654.yjntbbxy-lkp@intel.com/ All warnings (new ones prefixed by >>): >> drivers/gpu/drm/i915/gt/uc/intel_uc.c:484:6: warning: variable 'pl1en' is used uninitialized whenever 'if' condition is true [-Wsometimes-uninitialized] if (ret) ^~~ drivers/gpu/drm/i915/gt/uc/intel_uc.c:573:41: note: uninitialized use occurs here i915_hwmon_power_max_restore(gt->i915, pl1en); ^~~~~ drivers/gpu/drm/i915/gt/uc/intel_uc.c:484:2: note: remove the 'if' if its condition is always false if (ret) ^~~~~~~~ drivers/gpu/drm/i915/gt/uc/intel_uc.c:475:6: warning: variable 'pl1en' is used uninitialized whenever 'if' condition is true [-Wsometimes-uninitialized] if (!intel_uc_fw_is_loadable(&guc->fw)) { ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ drivers/gpu/drm/i915/gt/uc/intel_uc.c:573:41: note: uninitialized use occurs here i915_hwmon_power_max_restore(gt->i915, pl1en); ^~~~~ drivers/gpu/drm/i915/gt/uc/intel_uc.c:475:2: note: remove the 'if' if its condition is always false if (!intel_uc_fw_is_loadable(&guc->fw)) { ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ drivers/gpu/drm/i915/gt/uc/intel_uc.c:465:12: note: initialize the variable 'pl1en' to silence this warning bool pl1en; ^ = 0 2 warnings generated. vim +484 drivers/gpu/drm/i915/gt/uc/intel_uc.c afd088ac05f120d drivers/gpu/drm/i915/gt/uc/intel_uc.c John Harrison 2022-01-06 457 6fbeda0bfd210f9 drivers/gpu/drm/i915/gt/uc/intel_uc.c Michal Wajdeczko 2020-01-10 458 static int __uc_init_hw(struct intel_uc *uc) 6fbeda0bfd210f9 drivers/gpu/drm/i915/gt/uc/intel_uc.c Michal Wajdeczko 2020-01-10 459 { 2f8c06cb6622b55 drivers/gpu/drm/i915/gt/uc/intel_uc.c Michal Wajdeczko 2023-01-28 460 struct intel_gt *gt = uc_to_gt(uc); 2f8c06cb6622b55 drivers/gpu/drm/i915/gt/uc/intel_uc.c Michal Wajdeczko 2023-01-28 461 struct drm_i915_private *i915 = gt->i915; 6fbeda0bfd210f9 drivers/gpu/drm/i915/gt/uc/intel_uc.c Michal Wajdeczko 2020-01-10 462 struct intel_guc *guc = &uc->guc; 6fbeda0bfd210f9 drivers/gpu/drm/i915/gt/uc/intel_uc.c Michal Wajdeczko 2020-01-10 463 struct intel_huc *huc = &uc->huc; 6fbeda0bfd210f9 drivers/gpu/drm/i915/gt/uc/intel_uc.c Michal Wajdeczko 2020-01-10 464 int ret, attempts; b4aa935db7f0b46 drivers/gpu/drm/i915/gt/uc/intel_uc.c Ashutosh Dixit 2023-04-05 465 bool pl1en; 6fbeda0bfd210f9 drivers/gpu/drm/i915/gt/uc/intel_uc.c Michal Wajdeczko 2020-01-10 466 6fbeda0bfd210f9 drivers/gpu/drm/i915/gt/uc/intel_uc.c Michal Wajdeczko 2020-01-10 467 GEM_BUG_ON(!intel_uc_supports_guc(uc)); bfe5a40a7b9a967 drivers/gpu/drm/i915/gt/uc/intel_uc.c Daniele Ceraolo Spurio 2020-02-18 468 GEM_BUG_ON(!intel_uc_wants_guc(uc)); 356c484822e6ac9 drivers/gpu/drm/i915/gt/uc/intel_uc.c Michal Wajdeczko 2019-08-16 469 2f8c06cb6622b55 drivers/gpu/drm/i915/gt/uc/intel_uc.c Michal Wajdeczko 2023-01-28 470 print_fw_ver(gt, &guc->fw); afd088ac05f120d drivers/gpu/drm/i915/gt/uc/intel_uc.c John Harrison 2022-01-06 471 afd088ac05f120d drivers/gpu/drm/i915/gt/uc/intel_uc.c John Harrison 2022-01-06 472 if (intel_uc_uses_huc(uc)) 2f8c06cb6622b55 drivers/gpu/drm/i915/gt/uc/intel_uc.c Michal Wajdeczko 2023-01-28 473 print_fw_ver(gt, &huc->fw); afd088ac05f120d drivers/gpu/drm/i915/gt/uc/intel_uc.c John Harrison 2022-01-06 474 42f96e5bd41e91f drivers/gpu/drm/i915/gt/uc/intel_uc.c Daniele Ceraolo Spurio 2020-02-18 475 if (!intel_uc_fw_is_loadable(&guc->fw)) { 6fbeda0bfd210f9 drivers/gpu/drm/i915/gt/uc/intel_uc.c Michal Wajdeczko 2020-01-10 476 ret = __uc_check_hw(uc) || ee402140274e870 drivers/gpu/drm/i915/gt/uc/intel_uc.c Michal Wajdeczko 2019-08-18 477 intel_uc_fw_is_overridden(&guc->fw) || 202c98e71692484 drivers/gpu/drm/i915/gt/uc/intel_uc.c Daniele Ceraolo Spurio 2020-02-18 478 intel_uc_wants_guc_submission(uc) ? ee402140274e870 drivers/gpu/drm/i915/gt/uc/intel_uc.c Michal Wajdeczko 2019-08-18 479 intel_uc_fw_status_to_error(guc->fw.status) : 0; ae7a3166a708bee drivers/gpu/drm/i915/gt/uc/intel_uc.c Michal Wajdeczko 2019-08-11 480 goto err_out; ae7a3166a708bee drivers/gpu/drm/i915/gt/uc/intel_uc.c Michal Wajdeczko 2019-08-11 481 } ae7a3166a708bee drivers/gpu/drm/i915/gt/uc/intel_uc.c Michal Wajdeczko 2019-08-11 482 63064d822c964c0 drivers/gpu/drm/i915/gt/uc/intel_uc.c Daniele Ceraolo Spurio 2019-07-30 483 ret = uc_init_wopcm(uc); 63064d822c964c0 drivers/gpu/drm/i915/gt/uc/intel_uc.c Daniele Ceraolo Spurio 2019-07-30 @484 if (ret) 63064d822c964c0 drivers/gpu/drm/i915/gt/uc/intel_uc.c Daniele Ceraolo Spurio 2019-07-30 485 goto err_out; 63064d822c964c0 drivers/gpu/drm/i915/gt/uc/intel_uc.c Daniele Ceraolo Spurio 2019-07-30 486 e5a1ad035938e60 drivers/gpu/drm/i915/gt/uc/intel_uc.c Matthew Brost 2021-07-26 487 intel_guc_reset_interrupts(guc); 61b5c1587dd82a8 drivers/gpu/drm/i915/intel_uc.c Michał Winiarski 2017-12-13 488 6cd5a72c357732e drivers/gpu/drm/i915/intel_uc.c Arkadiusz Hiler 2017-03-14 489 /* WaEnableuKernelHeaderValidFix:skl */ 6cd5a72c357732e drivers/gpu/drm/i915/intel_uc.c Arkadiusz Hiler 2017-03-14 490 /* WaEnableGuCBootHashCheckNotSet:skl,bxt,kbl */ c816723b6b8a627 drivers/gpu/drm/i915/gt/uc/intel_uc.c Lucas De Marchi 2021-06-05 491 if (GRAPHICS_VER(i915) == 9) 6cd5a72c357732e drivers/gpu/drm/i915/intel_uc.c Arkadiusz Hiler 2017-03-14 492 attempts = 3; 6cd5a72c357732e drivers/gpu/drm/i915/intel_uc.c Arkadiusz Hiler 2017-03-14 493 else 6cd5a72c357732e drivers/gpu/drm/i915/intel_uc.c Arkadiusz Hiler 2017-03-14 494 attempts = 1; 6cd5a72c357732e drivers/gpu/drm/i915/intel_uc.c Arkadiusz Hiler 2017-03-14 495 b4aa935db7f0b46 drivers/gpu/drm/i915/gt/uc/intel_uc.c Ashutosh Dixit 2023-04-05 496 /* Disable a potentially low PL1 power limit to allow freq to be raised */ b4aa935db7f0b46 drivers/gpu/drm/i915/gt/uc/intel_uc.c Ashutosh Dixit 2023-04-05 497 i915_hwmon_power_max_disable(gt->i915, &pl1en); b4aa935db7f0b46 drivers/gpu/drm/i915/gt/uc/intel_uc.c Ashutosh Dixit 2023-04-05 498 1c40d40f6835cde drivers/gpu/drm/i915/gt/uc/intel_uc.c Vinay Belgaumkar 2021-12-16 499 intel_rps_raise_unslice(&uc_to_gt(uc)->rps); 1c40d40f6835cde drivers/gpu/drm/i915/gt/uc/intel_uc.c Vinay Belgaumkar 2021-12-16 500 6cd5a72c357732e drivers/gpu/drm/i915/intel_uc.c Arkadiusz Hiler 2017-03-14 501 while (attempts--) { 6cd5a72c357732e drivers/gpu/drm/i915/intel_uc.c Arkadiusz Hiler 2017-03-14 502 /* 6cd5a72c357732e drivers/gpu/drm/i915/intel_uc.c Arkadiusz Hiler 2017-03-14 503 * Always reset the GuC just before (re)loading, so 6cd5a72c357732e drivers/gpu/drm/i915/intel_uc.c Arkadiusz Hiler 2017-03-14 504 * that the state and timing are fairly predictable 6cd5a72c357732e drivers/gpu/drm/i915/intel_uc.c Arkadiusz Hiler 2017-03-14 505 */ 771051eaa74661f drivers/gpu/drm/i915/gt/uc/intel_uc.c Michal Wajdeczko 2019-08-02 506 ret = __uc_sanitize(uc); 6cd5a72c357732e drivers/gpu/drm/i915/intel_uc.c Arkadiusz Hiler 2017-03-14 507 if (ret) 61b5c1587dd82a8 drivers/gpu/drm/i915/intel_uc.c Michał Winiarski 2017-12-13 508 goto err_out; 6cd5a72c357732e drivers/gpu/drm/i915/intel_uc.c Arkadiusz Hiler 2017-03-14 509 a8dc0f6d187bccc drivers/gpu/drm/i915/gt/uc/intel_uc.c Michal Wajdeczko 2019-08-18 510 intel_huc_fw_upload(huc); 386e300fe9fae7e drivers/gpu/drm/i915/intel_uc.c Michal Wajdeczko 2019-05-27 511 intel_guc_ads_reset(guc); 2bf8fb39eb70b6c drivers/gpu/drm/i915/gt/uc/intel_uc.c Daniele Ceraolo Spurio 2019-07-24 512 intel_guc_write_params(guc); e8668bbcb0f91c7 drivers/gpu/drm/i915/intel_uc.c Michal Wajdeczko 2017-10-16 513 ret = intel_guc_fw_upload(guc); 52b832606038c5b drivers/gpu/drm/i915/intel_uc.c Robert M. Fosha 2019-03-29 514 if (ret == 0) 6cd5a72c357732e drivers/gpu/drm/i915/intel_uc.c Arkadiusz Hiler 2017-03-14 515 break; 6cd5a72c357732e drivers/gpu/drm/i915/intel_uc.c Arkadiusz Hiler 2017-03-14 516 2f8c06cb6622b55 drivers/gpu/drm/i915/gt/uc/intel_uc.c Michal Wajdeczko 2023-01-28 517 gt_dbg(gt, "GuC fw load failed (%pe) will reset and retry %d more time(s)\n", 2f8c06cb6622b55 drivers/gpu/drm/i915/gt/uc/intel_uc.c Michal Wajdeczko 2023-01-28 518 ERR_PTR(ret), attempts); 6cd5a72c357732e drivers/gpu/drm/i915/intel_uc.c Arkadiusz Hiler 2017-03-14 519 } 6cd5a72c357732e drivers/gpu/drm/i915/intel_uc.c Arkadiusz Hiler 2017-03-14 520 6cd5a72c357732e drivers/gpu/drm/i915/intel_uc.c Arkadiusz Hiler 2017-03-14 521 /* Did we succeded or run out of retries? */ 6cd5a72c357732e drivers/gpu/drm/i915/intel_uc.c Arkadiusz Hiler 2017-03-14 522 if (ret) ac58d2ab0ad9c8b drivers/gpu/drm/i915/intel_uc.c Daniele Ceraolo Spurio 2017-05-22 523 goto err_log_capture; 6cd5a72c357732e drivers/gpu/drm/i915/intel_uc.c Arkadiusz Hiler 2017-03-14 524 789a625158b0c0c drivers/gpu/drm/i915/intel_uc.c Michal Wajdeczko 2017-05-02 525 ret = guc_enable_communication(guc); 789a625158b0c0c drivers/gpu/drm/i915/intel_uc.c Michal Wajdeczko 2017-05-02 526 if (ret) ac58d2ab0ad9c8b drivers/gpu/drm/i915/intel_uc.c Daniele Ceraolo Spurio 2017-05-22 527 goto err_log_capture; 789a625158b0c0c drivers/gpu/drm/i915/intel_uc.c Michal Wajdeczko 2017-05-02 528 6f67930af78f10a drivers/gpu/drm/i915/gt/uc/intel_uc.c Daniele Ceraolo Spurio 2022-05-04 529 /* 6f67930af78f10a drivers/gpu/drm/i915/gt/uc/intel_uc.c Daniele Ceraolo Spurio 2022-05-04 530 * GSC-loaded HuC is authenticated by the GSC, so we don't need to 6f67930af78f10a drivers/gpu/drm/i915/gt/uc/intel_uc.c Daniele Ceraolo Spurio 2022-05-04 531 * trigger the auth here. However, given that the HuC loaded this way 6f67930af78f10a drivers/gpu/drm/i915/gt/uc/intel_uc.c Daniele Ceraolo Spurio 2022-05-04 532 * survive GT reset, we still need to update our SW bookkeeping to make 6f67930af78f10a drivers/gpu/drm/i915/gt/uc/intel_uc.c Daniele Ceraolo Spurio 2022-05-04 533 * sure it reflects the correct HW status. 6f67930af78f10a drivers/gpu/drm/i915/gt/uc/intel_uc.c Daniele Ceraolo Spurio 2022-05-04 534 */ 6f67930af78f10a drivers/gpu/drm/i915/gt/uc/intel_uc.c Daniele Ceraolo Spurio 2022-05-04 535 if (intel_huc_is_loaded_by_gsc(huc)) 6f67930af78f10a drivers/gpu/drm/i915/gt/uc/intel_uc.c Daniele Ceraolo Spurio 2022-05-04 536 intel_huc_update_auth_status(huc); 6f67930af78f10a drivers/gpu/drm/i915/gt/uc/intel_uc.c Daniele Ceraolo Spurio 2022-05-04 537 else a8dc0f6d187bccc drivers/gpu/drm/i915/gt/uc/intel_uc.c Michal Wajdeczko 2019-08-18 538 intel_huc_auth(huc); 0dfa1cee613e03c drivers/gpu/drm/i915/intel_uc.c Michal Wajdeczko 2017-12-06 539 cd414f4f59f64d7 drivers/gpu/drm/i915/gt/uc/intel_uc.c John Harrison 2023-02-17 540 if (intel_uc_uses_guc_submission(uc)) { cd414f4f59f64d7 drivers/gpu/drm/i915/gt/uc/intel_uc.c John Harrison 2023-02-17 541 ret = intel_guc_submission_enable(guc); cd414f4f59f64d7 drivers/gpu/drm/i915/gt/uc/intel_uc.c John Harrison 2023-02-17 542 if (ret) cd414f4f59f64d7 drivers/gpu/drm/i915/gt/uc/intel_uc.c John Harrison 2023-02-17 543 goto err_log_capture; cd414f4f59f64d7 drivers/gpu/drm/i915/gt/uc/intel_uc.c John Harrison 2023-02-17 544 } 6cd5a72c357732e drivers/gpu/drm/i915/intel_uc.c Arkadiusz Hiler 2017-03-14 545 63c0eb30bfe9269 drivers/gpu/drm/i915/gt/uc/intel_uc.c Vinay Belgaumkar 2021-07-30 546 if (intel_uc_uses_guc_slpc(uc)) { 63c0eb30bfe9269 drivers/gpu/drm/i915/gt/uc/intel_uc.c Vinay Belgaumkar 2021-07-30 547 ret = intel_guc_slpc_enable(&guc->slpc); 63c0eb30bfe9269 drivers/gpu/drm/i915/gt/uc/intel_uc.c Vinay Belgaumkar 2021-07-30 548 if (ret) 63c0eb30bfe9269 drivers/gpu/drm/i915/gt/uc/intel_uc.c Vinay Belgaumkar 2021-07-30 549 goto err_submission; 1c40d40f6835cde drivers/gpu/drm/i915/gt/uc/intel_uc.c Vinay Belgaumkar 2021-12-16 550 } else { 1c40d40f6835cde drivers/gpu/drm/i915/gt/uc/intel_uc.c Vinay Belgaumkar 2021-12-16 551 /* Restore GT back to RPn for non-SLPC path */ 1c40d40f6835cde drivers/gpu/drm/i915/gt/uc/intel_uc.c Vinay Belgaumkar 2021-12-16 552 intel_rps_lower_unslice(&uc_to_gt(uc)->rps); 63c0eb30bfe9269 drivers/gpu/drm/i915/gt/uc/intel_uc.c Vinay Belgaumkar 2021-07-30 553 } 63c0eb30bfe9269 drivers/gpu/drm/i915/gt/uc/intel_uc.c Vinay Belgaumkar 2021-07-30 554 b4aa935db7f0b46 drivers/gpu/drm/i915/gt/uc/intel_uc.c Ashutosh Dixit 2023-04-05 555 i915_hwmon_power_max_restore(gt->i915, pl1en); b4aa935db7f0b46 drivers/gpu/drm/i915/gt/uc/intel_uc.c Ashutosh Dixit 2023-04-05 556 4fd4fde8e42e164 drivers/gpu/drm/i915/gt/uc/intel_uc.c John Harrison 2023-02-06 557 guc_info(guc, "submission %s\n", str_enabled_disabled(intel_uc_uses_guc_submission(uc))); 4fd4fde8e42e164 drivers/gpu/drm/i915/gt/uc/intel_uc.c John Harrison 2023-02-06 558 guc_info(guc, "SLPC %s\n", str_enabled_disabled(intel_uc_uses_guc_slpc(uc))); 63c0eb30bfe9269 drivers/gpu/drm/i915/gt/uc/intel_uc.c Vinay Belgaumkar 2021-07-30 559 6cd5a72c357732e drivers/gpu/drm/i915/intel_uc.c Arkadiusz Hiler 2017-03-14 560 return 0; 6cd5a72c357732e drivers/gpu/drm/i915/intel_uc.c Arkadiusz Hiler 2017-03-14 561 6cd5a72c357732e drivers/gpu/drm/i915/intel_uc.c Arkadiusz Hiler 2017-03-14 562 /* 6cd5a72c357732e drivers/gpu/drm/i915/intel_uc.c Arkadiusz Hiler 2017-03-14 563 * We've failed to load the firmware :( 6cd5a72c357732e drivers/gpu/drm/i915/intel_uc.c Arkadiusz Hiler 2017-03-14 564 */ 63c0eb30bfe9269 drivers/gpu/drm/i915/gt/uc/intel_uc.c Vinay Belgaumkar 2021-07-30 565 err_submission: 63c0eb30bfe9269 drivers/gpu/drm/i915/gt/uc/intel_uc.c Vinay Belgaumkar 2021-07-30 566 intel_guc_submission_disable(guc); ac58d2ab0ad9c8b drivers/gpu/drm/i915/intel_uc.c Daniele Ceraolo Spurio 2017-05-22 567 err_log_capture: 32ff76e80c2400c drivers/gpu/drm/i915/gt/uc/intel_uc.c Michal Wajdeczko 2019-08-02 568 __uc_capture_load_err_log(uc); 121981fafe699d9 drivers/gpu/drm/i915/intel_uc.c Michal Wajdeczko 2017-12-06 569 err_out: 1c40d40f6835cde drivers/gpu/drm/i915/gt/uc/intel_uc.c Vinay Belgaumkar 2021-12-16 570 /* Return GT back to RPn */ 1c40d40f6835cde drivers/gpu/drm/i915/gt/uc/intel_uc.c Vinay Belgaumkar 2021-12-16 571 intel_rps_lower_unslice(&uc_to_gt(uc)->rps); 1c40d40f6835cde drivers/gpu/drm/i915/gt/uc/intel_uc.c Vinay Belgaumkar 2021-12-16 572 b4aa935db7f0b46 drivers/gpu/drm/i915/gt/uc/intel_uc.c Ashutosh Dixit 2023-04-05 573 i915_hwmon_power_max_restore(gt->i915, pl1en); b4aa935db7f0b46 drivers/gpu/drm/i915/gt/uc/intel_uc.c Ashutosh Dixit 2023-04-05 574 ca7b2c1bbede618 drivers/gpu/drm/i915/gt/uc/intel_uc.c Daniele Ceraolo Spurio 2019-07-13 575 __uc_sanitize(uc); 89195bab5d8c540 drivers/gpu/drm/i915/intel_uc.c Michal Wajdeczko 2019-05-22 576 ee402140274e870 drivers/gpu/drm/i915/gt/uc/intel_uc.c Michal Wajdeczko 2019-08-18 577 if (!ret) { 2f8c06cb6622b55 drivers/gpu/drm/i915/gt/uc/intel_uc.c Michal Wajdeczko 2023-01-28 578 gt_notice(gt, "GuC is uninitialized\n"); ee402140274e870 drivers/gpu/drm/i915/gt/uc/intel_uc.c Michal Wajdeczko 2019-08-18 579 /* We want to run without GuC submission */ ee402140274e870 drivers/gpu/drm/i915/gt/uc/intel_uc.c Michal Wajdeczko 2019-08-18 580 return 0; ee402140274e870 drivers/gpu/drm/i915/gt/uc/intel_uc.c Michal Wajdeczko 2019-08-18 581 } ee402140274e870 drivers/gpu/drm/i915/gt/uc/intel_uc.c Michal Wajdeczko 2019-08-18 582 2f8c06cb6622b55 drivers/gpu/drm/i915/gt/uc/intel_uc.c Michal Wajdeczko 2023-01-28 583 gt_probe_error(gt, "GuC initialization failed %pe\n", ERR_PTR(ret)); a5f978c3609f02a drivers/gpu/drm/i915/gt/uc/intel_uc.c Michal Wajdeczko 2019-08-11 584 a5f978c3609f02a drivers/gpu/drm/i915/gt/uc/intel_uc.c Michal Wajdeczko 2019-08-11 585 /* We want to keep KMS alive */ a5f978c3609f02a drivers/gpu/drm/i915/gt/uc/intel_uc.c Michal Wajdeczko 2019-08-11 586 return -EIO; 6cd5a72c357732e drivers/gpu/drm/i915/intel_uc.c Arkadiusz Hiler 2017-03-14 587 } 6cd5a72c357732e drivers/gpu/drm/i915/intel_uc.c Arkadiusz Hiler 2017-03-14 588 -- 0-DAY CI Kernel Test Service https://github.com/intel/lkp-tests ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [Intel-gfx] [PATCH 2/3] drm/i915/guc: Disable PL1 power limit when loading GuC firmware @ 2023-04-06 9:16 ` kernel test robot 0 siblings, 0 replies; 31+ messages in thread From: kernel test robot @ 2023-04-06 9:16 UTC (permalink / raw) To: Ashutosh Dixit, intel-gfx; +Cc: llvm, oe-kbuild-all, dri-devel, Rodrigo Vivi Hi Ashutosh, kernel test robot noticed the following build warnings: [auto build test WARNING on drm-tip/drm-tip] url: https://github.com/intel-lab-lkp/linux/commits/Ashutosh-Dixit/drm-i915-hwmon-Get-mutex-and-rpm-ref-just-once-in-hwm_power_max_write/20230406-124659 base: git://anongit.freedesktop.org/drm/drm-tip drm-tip patch link: https://lore.kernel.org/r/20230406044522.3108359-3-ashutosh.dixit%40intel.com patch subject: [Intel-gfx] [PATCH 2/3] drm/i915/guc: Disable PL1 power limit when loading GuC firmware config: i386-randconfig-a002-20230403 (https://download.01.org/0day-ci/archive/20230406/202304061654.yjntbbxy-lkp@intel.com/config) compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project f28c006a5895fc0e329fe15fead81e37457cb1d1) reproduce (this is a W=1 build): wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # https://github.com/intel-lab-lkp/linux/commit/b4aa935db7f0b46437cdaa39f0149ad835ceb73c git remote add linux-review https://github.com/intel-lab-lkp/linux git fetch --no-tags linux-review Ashutosh-Dixit/drm-i915-hwmon-Get-mutex-and-rpm-ref-just-once-in-hwm_power_max_write/20230406-124659 git checkout b4aa935db7f0b46437cdaa39f0149ad835ceb73c # save the config file mkdir build_dir && cp config build_dir/.config COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 olddefconfig COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 SHELL=/bin/bash drivers/gpu/drm/i915/ If you fix the issue, kindly add following tag where applicable | Reported-by: kernel test robot <lkp@intel.com> | Link: https://lore.kernel.org/oe-kbuild-all/202304061654.yjntbbxy-lkp@intel.com/ All warnings (new ones prefixed by >>): >> drivers/gpu/drm/i915/gt/uc/intel_uc.c:484:6: warning: variable 'pl1en' is used uninitialized whenever 'if' condition is true [-Wsometimes-uninitialized] if (ret) ^~~ drivers/gpu/drm/i915/gt/uc/intel_uc.c:573:41: note: uninitialized use occurs here i915_hwmon_power_max_restore(gt->i915, pl1en); ^~~~~ drivers/gpu/drm/i915/gt/uc/intel_uc.c:484:2: note: remove the 'if' if its condition is always false if (ret) ^~~~~~~~ drivers/gpu/drm/i915/gt/uc/intel_uc.c:475:6: warning: variable 'pl1en' is used uninitialized whenever 'if' condition is true [-Wsometimes-uninitialized] if (!intel_uc_fw_is_loadable(&guc->fw)) { ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ drivers/gpu/drm/i915/gt/uc/intel_uc.c:573:41: note: uninitialized use occurs here i915_hwmon_power_max_restore(gt->i915, pl1en); ^~~~~ drivers/gpu/drm/i915/gt/uc/intel_uc.c:475:2: note: remove the 'if' if its condition is always false if (!intel_uc_fw_is_loadable(&guc->fw)) { ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ drivers/gpu/drm/i915/gt/uc/intel_uc.c:465:12: note: initialize the variable 'pl1en' to silence this warning bool pl1en; ^ = 0 2 warnings generated. vim +484 drivers/gpu/drm/i915/gt/uc/intel_uc.c afd088ac05f120d drivers/gpu/drm/i915/gt/uc/intel_uc.c John Harrison 2022-01-06 457 6fbeda0bfd210f9 drivers/gpu/drm/i915/gt/uc/intel_uc.c Michal Wajdeczko 2020-01-10 458 static int __uc_init_hw(struct intel_uc *uc) 6fbeda0bfd210f9 drivers/gpu/drm/i915/gt/uc/intel_uc.c Michal Wajdeczko 2020-01-10 459 { 2f8c06cb6622b55 drivers/gpu/drm/i915/gt/uc/intel_uc.c Michal Wajdeczko 2023-01-28 460 struct intel_gt *gt = uc_to_gt(uc); 2f8c06cb6622b55 drivers/gpu/drm/i915/gt/uc/intel_uc.c Michal Wajdeczko 2023-01-28 461 struct drm_i915_private *i915 = gt->i915; 6fbeda0bfd210f9 drivers/gpu/drm/i915/gt/uc/intel_uc.c Michal Wajdeczko 2020-01-10 462 struct intel_guc *guc = &uc->guc; 6fbeda0bfd210f9 drivers/gpu/drm/i915/gt/uc/intel_uc.c Michal Wajdeczko 2020-01-10 463 struct intel_huc *huc = &uc->huc; 6fbeda0bfd210f9 drivers/gpu/drm/i915/gt/uc/intel_uc.c Michal Wajdeczko 2020-01-10 464 int ret, attempts; b4aa935db7f0b46 drivers/gpu/drm/i915/gt/uc/intel_uc.c Ashutosh Dixit 2023-04-05 465 bool pl1en; 6fbeda0bfd210f9 drivers/gpu/drm/i915/gt/uc/intel_uc.c Michal Wajdeczko 2020-01-10 466 6fbeda0bfd210f9 drivers/gpu/drm/i915/gt/uc/intel_uc.c Michal Wajdeczko 2020-01-10 467 GEM_BUG_ON(!intel_uc_supports_guc(uc)); bfe5a40a7b9a967 drivers/gpu/drm/i915/gt/uc/intel_uc.c Daniele Ceraolo Spurio 2020-02-18 468 GEM_BUG_ON(!intel_uc_wants_guc(uc)); 356c484822e6ac9 drivers/gpu/drm/i915/gt/uc/intel_uc.c Michal Wajdeczko 2019-08-16 469 2f8c06cb6622b55 drivers/gpu/drm/i915/gt/uc/intel_uc.c Michal Wajdeczko 2023-01-28 470 print_fw_ver(gt, &guc->fw); afd088ac05f120d drivers/gpu/drm/i915/gt/uc/intel_uc.c John Harrison 2022-01-06 471 afd088ac05f120d drivers/gpu/drm/i915/gt/uc/intel_uc.c John Harrison 2022-01-06 472 if (intel_uc_uses_huc(uc)) 2f8c06cb6622b55 drivers/gpu/drm/i915/gt/uc/intel_uc.c Michal Wajdeczko 2023-01-28 473 print_fw_ver(gt, &huc->fw); afd088ac05f120d drivers/gpu/drm/i915/gt/uc/intel_uc.c John Harrison 2022-01-06 474 42f96e5bd41e91f drivers/gpu/drm/i915/gt/uc/intel_uc.c Daniele Ceraolo Spurio 2020-02-18 475 if (!intel_uc_fw_is_loadable(&guc->fw)) { 6fbeda0bfd210f9 drivers/gpu/drm/i915/gt/uc/intel_uc.c Michal Wajdeczko 2020-01-10 476 ret = __uc_check_hw(uc) || ee402140274e870 drivers/gpu/drm/i915/gt/uc/intel_uc.c Michal Wajdeczko 2019-08-18 477 intel_uc_fw_is_overridden(&guc->fw) || 202c98e71692484 drivers/gpu/drm/i915/gt/uc/intel_uc.c Daniele Ceraolo Spurio 2020-02-18 478 intel_uc_wants_guc_submission(uc) ? ee402140274e870 drivers/gpu/drm/i915/gt/uc/intel_uc.c Michal Wajdeczko 2019-08-18 479 intel_uc_fw_status_to_error(guc->fw.status) : 0; ae7a3166a708bee drivers/gpu/drm/i915/gt/uc/intel_uc.c Michal Wajdeczko 2019-08-11 480 goto err_out; ae7a3166a708bee drivers/gpu/drm/i915/gt/uc/intel_uc.c Michal Wajdeczko 2019-08-11 481 } ae7a3166a708bee drivers/gpu/drm/i915/gt/uc/intel_uc.c Michal Wajdeczko 2019-08-11 482 63064d822c964c0 drivers/gpu/drm/i915/gt/uc/intel_uc.c Daniele Ceraolo Spurio 2019-07-30 483 ret = uc_init_wopcm(uc); 63064d822c964c0 drivers/gpu/drm/i915/gt/uc/intel_uc.c Daniele Ceraolo Spurio 2019-07-30 @484 if (ret) 63064d822c964c0 drivers/gpu/drm/i915/gt/uc/intel_uc.c Daniele Ceraolo Spurio 2019-07-30 485 goto err_out; 63064d822c964c0 drivers/gpu/drm/i915/gt/uc/intel_uc.c Daniele Ceraolo Spurio 2019-07-30 486 e5a1ad035938e60 drivers/gpu/drm/i915/gt/uc/intel_uc.c Matthew Brost 2021-07-26 487 intel_guc_reset_interrupts(guc); 61b5c1587dd82a8 drivers/gpu/drm/i915/intel_uc.c Michał Winiarski 2017-12-13 488 6cd5a72c357732e drivers/gpu/drm/i915/intel_uc.c Arkadiusz Hiler 2017-03-14 489 /* WaEnableuKernelHeaderValidFix:skl */ 6cd5a72c357732e drivers/gpu/drm/i915/intel_uc.c Arkadiusz Hiler 2017-03-14 490 /* WaEnableGuCBootHashCheckNotSet:skl,bxt,kbl */ c816723b6b8a627 drivers/gpu/drm/i915/gt/uc/intel_uc.c Lucas De Marchi 2021-06-05 491 if (GRAPHICS_VER(i915) == 9) 6cd5a72c357732e drivers/gpu/drm/i915/intel_uc.c Arkadiusz Hiler 2017-03-14 492 attempts = 3; 6cd5a72c357732e drivers/gpu/drm/i915/intel_uc.c Arkadiusz Hiler 2017-03-14 493 else 6cd5a72c357732e drivers/gpu/drm/i915/intel_uc.c Arkadiusz Hiler 2017-03-14 494 attempts = 1; 6cd5a72c357732e drivers/gpu/drm/i915/intel_uc.c Arkadiusz Hiler 2017-03-14 495 b4aa935db7f0b46 drivers/gpu/drm/i915/gt/uc/intel_uc.c Ashutosh Dixit 2023-04-05 496 /* Disable a potentially low PL1 power limit to allow freq to be raised */ b4aa935db7f0b46 drivers/gpu/drm/i915/gt/uc/intel_uc.c Ashutosh Dixit 2023-04-05 497 i915_hwmon_power_max_disable(gt->i915, &pl1en); b4aa935db7f0b46 drivers/gpu/drm/i915/gt/uc/intel_uc.c Ashutosh Dixit 2023-04-05 498 1c40d40f6835cde drivers/gpu/drm/i915/gt/uc/intel_uc.c Vinay Belgaumkar 2021-12-16 499 intel_rps_raise_unslice(&uc_to_gt(uc)->rps); 1c40d40f6835cde drivers/gpu/drm/i915/gt/uc/intel_uc.c Vinay Belgaumkar 2021-12-16 500 6cd5a72c357732e drivers/gpu/drm/i915/intel_uc.c Arkadiusz Hiler 2017-03-14 501 while (attempts--) { 6cd5a72c357732e drivers/gpu/drm/i915/intel_uc.c Arkadiusz Hiler 2017-03-14 502 /* 6cd5a72c357732e drivers/gpu/drm/i915/intel_uc.c Arkadiusz Hiler 2017-03-14 503 * Always reset the GuC just before (re)loading, so 6cd5a72c357732e drivers/gpu/drm/i915/intel_uc.c Arkadiusz Hiler 2017-03-14 504 * that the state and timing are fairly predictable 6cd5a72c357732e drivers/gpu/drm/i915/intel_uc.c Arkadiusz Hiler 2017-03-14 505 */ 771051eaa74661f drivers/gpu/drm/i915/gt/uc/intel_uc.c Michal Wajdeczko 2019-08-02 506 ret = __uc_sanitize(uc); 6cd5a72c357732e drivers/gpu/drm/i915/intel_uc.c Arkadiusz Hiler 2017-03-14 507 if (ret) 61b5c1587dd82a8 drivers/gpu/drm/i915/intel_uc.c Michał Winiarski 2017-12-13 508 goto err_out; 6cd5a72c357732e drivers/gpu/drm/i915/intel_uc.c Arkadiusz Hiler 2017-03-14 509 a8dc0f6d187bccc drivers/gpu/drm/i915/gt/uc/intel_uc.c Michal Wajdeczko 2019-08-18 510 intel_huc_fw_upload(huc); 386e300fe9fae7e drivers/gpu/drm/i915/intel_uc.c Michal Wajdeczko 2019-05-27 511 intel_guc_ads_reset(guc); 2bf8fb39eb70b6c drivers/gpu/drm/i915/gt/uc/intel_uc.c Daniele Ceraolo Spurio 2019-07-24 512 intel_guc_write_params(guc); e8668bbcb0f91c7 drivers/gpu/drm/i915/intel_uc.c Michal Wajdeczko 2017-10-16 513 ret = intel_guc_fw_upload(guc); 52b832606038c5b drivers/gpu/drm/i915/intel_uc.c Robert M. Fosha 2019-03-29 514 if (ret == 0) 6cd5a72c357732e drivers/gpu/drm/i915/intel_uc.c Arkadiusz Hiler 2017-03-14 515 break; 6cd5a72c357732e drivers/gpu/drm/i915/intel_uc.c Arkadiusz Hiler 2017-03-14 516 2f8c06cb6622b55 drivers/gpu/drm/i915/gt/uc/intel_uc.c Michal Wajdeczko 2023-01-28 517 gt_dbg(gt, "GuC fw load failed (%pe) will reset and retry %d more time(s)\n", 2f8c06cb6622b55 drivers/gpu/drm/i915/gt/uc/intel_uc.c Michal Wajdeczko 2023-01-28 518 ERR_PTR(ret), attempts); 6cd5a72c357732e drivers/gpu/drm/i915/intel_uc.c Arkadiusz Hiler 2017-03-14 519 } 6cd5a72c357732e drivers/gpu/drm/i915/intel_uc.c Arkadiusz Hiler 2017-03-14 520 6cd5a72c357732e drivers/gpu/drm/i915/intel_uc.c Arkadiusz Hiler 2017-03-14 521 /* Did we succeded or run out of retries? */ 6cd5a72c357732e drivers/gpu/drm/i915/intel_uc.c Arkadiusz Hiler 2017-03-14 522 if (ret) ac58d2ab0ad9c8b drivers/gpu/drm/i915/intel_uc.c Daniele Ceraolo Spurio 2017-05-22 523 goto err_log_capture; 6cd5a72c357732e drivers/gpu/drm/i915/intel_uc.c Arkadiusz Hiler 2017-03-14 524 789a625158b0c0c drivers/gpu/drm/i915/intel_uc.c Michal Wajdeczko 2017-05-02 525 ret = guc_enable_communication(guc); 789a625158b0c0c drivers/gpu/drm/i915/intel_uc.c Michal Wajdeczko 2017-05-02 526 if (ret) ac58d2ab0ad9c8b drivers/gpu/drm/i915/intel_uc.c Daniele Ceraolo Spurio 2017-05-22 527 goto err_log_capture; 789a625158b0c0c drivers/gpu/drm/i915/intel_uc.c Michal Wajdeczko 2017-05-02 528 6f67930af78f10a drivers/gpu/drm/i915/gt/uc/intel_uc.c Daniele Ceraolo Spurio 2022-05-04 529 /* 6f67930af78f10a drivers/gpu/drm/i915/gt/uc/intel_uc.c Daniele Ceraolo Spurio 2022-05-04 530 * GSC-loaded HuC is authenticated by the GSC, so we don't need to 6f67930af78f10a drivers/gpu/drm/i915/gt/uc/intel_uc.c Daniele Ceraolo Spurio 2022-05-04 531 * trigger the auth here. However, given that the HuC loaded this way 6f67930af78f10a drivers/gpu/drm/i915/gt/uc/intel_uc.c Daniele Ceraolo Spurio 2022-05-04 532 * survive GT reset, we still need to update our SW bookkeeping to make 6f67930af78f10a drivers/gpu/drm/i915/gt/uc/intel_uc.c Daniele Ceraolo Spurio 2022-05-04 533 * sure it reflects the correct HW status. 6f67930af78f10a drivers/gpu/drm/i915/gt/uc/intel_uc.c Daniele Ceraolo Spurio 2022-05-04 534 */ 6f67930af78f10a drivers/gpu/drm/i915/gt/uc/intel_uc.c Daniele Ceraolo Spurio 2022-05-04 535 if (intel_huc_is_loaded_by_gsc(huc)) 6f67930af78f10a drivers/gpu/drm/i915/gt/uc/intel_uc.c Daniele Ceraolo Spurio 2022-05-04 536 intel_huc_update_auth_status(huc); 6f67930af78f10a drivers/gpu/drm/i915/gt/uc/intel_uc.c Daniele Ceraolo Spurio 2022-05-04 537 else a8dc0f6d187bccc drivers/gpu/drm/i915/gt/uc/intel_uc.c Michal Wajdeczko 2019-08-18 538 intel_huc_auth(huc); 0dfa1cee613e03c drivers/gpu/drm/i915/intel_uc.c Michal Wajdeczko 2017-12-06 539 cd414f4f59f64d7 drivers/gpu/drm/i915/gt/uc/intel_uc.c John Harrison 2023-02-17 540 if (intel_uc_uses_guc_submission(uc)) { cd414f4f59f64d7 drivers/gpu/drm/i915/gt/uc/intel_uc.c John Harrison 2023-02-17 541 ret = intel_guc_submission_enable(guc); cd414f4f59f64d7 drivers/gpu/drm/i915/gt/uc/intel_uc.c John Harrison 2023-02-17 542 if (ret) cd414f4f59f64d7 drivers/gpu/drm/i915/gt/uc/intel_uc.c John Harrison 2023-02-17 543 goto err_log_capture; cd414f4f59f64d7 drivers/gpu/drm/i915/gt/uc/intel_uc.c John Harrison 2023-02-17 544 } 6cd5a72c357732e drivers/gpu/drm/i915/intel_uc.c Arkadiusz Hiler 2017-03-14 545 63c0eb30bfe9269 drivers/gpu/drm/i915/gt/uc/intel_uc.c Vinay Belgaumkar 2021-07-30 546 if (intel_uc_uses_guc_slpc(uc)) { 63c0eb30bfe9269 drivers/gpu/drm/i915/gt/uc/intel_uc.c Vinay Belgaumkar 2021-07-30 547 ret = intel_guc_slpc_enable(&guc->slpc); 63c0eb30bfe9269 drivers/gpu/drm/i915/gt/uc/intel_uc.c Vinay Belgaumkar 2021-07-30 548 if (ret) 63c0eb30bfe9269 drivers/gpu/drm/i915/gt/uc/intel_uc.c Vinay Belgaumkar 2021-07-30 549 goto err_submission; 1c40d40f6835cde drivers/gpu/drm/i915/gt/uc/intel_uc.c Vinay Belgaumkar 2021-12-16 550 } else { 1c40d40f6835cde drivers/gpu/drm/i915/gt/uc/intel_uc.c Vinay Belgaumkar 2021-12-16 551 /* Restore GT back to RPn for non-SLPC path */ 1c40d40f6835cde drivers/gpu/drm/i915/gt/uc/intel_uc.c Vinay Belgaumkar 2021-12-16 552 intel_rps_lower_unslice(&uc_to_gt(uc)->rps); 63c0eb30bfe9269 drivers/gpu/drm/i915/gt/uc/intel_uc.c Vinay Belgaumkar 2021-07-30 553 } 63c0eb30bfe9269 drivers/gpu/drm/i915/gt/uc/intel_uc.c Vinay Belgaumkar 2021-07-30 554 b4aa935db7f0b46 drivers/gpu/drm/i915/gt/uc/intel_uc.c Ashutosh Dixit 2023-04-05 555 i915_hwmon_power_max_restore(gt->i915, pl1en); b4aa935db7f0b46 drivers/gpu/drm/i915/gt/uc/intel_uc.c Ashutosh Dixit 2023-04-05 556 4fd4fde8e42e164 drivers/gpu/drm/i915/gt/uc/intel_uc.c John Harrison 2023-02-06 557 guc_info(guc, "submission %s\n", str_enabled_disabled(intel_uc_uses_guc_submission(uc))); 4fd4fde8e42e164 drivers/gpu/drm/i915/gt/uc/intel_uc.c John Harrison 2023-02-06 558 guc_info(guc, "SLPC %s\n", str_enabled_disabled(intel_uc_uses_guc_slpc(uc))); 63c0eb30bfe9269 drivers/gpu/drm/i915/gt/uc/intel_uc.c Vinay Belgaumkar 2021-07-30 559 6cd5a72c357732e drivers/gpu/drm/i915/intel_uc.c Arkadiusz Hiler 2017-03-14 560 return 0; 6cd5a72c357732e drivers/gpu/drm/i915/intel_uc.c Arkadiusz Hiler 2017-03-14 561 6cd5a72c357732e drivers/gpu/drm/i915/intel_uc.c Arkadiusz Hiler 2017-03-14 562 /* 6cd5a72c357732e drivers/gpu/drm/i915/intel_uc.c Arkadiusz Hiler 2017-03-14 563 * We've failed to load the firmware :( 6cd5a72c357732e drivers/gpu/drm/i915/intel_uc.c Arkadiusz Hiler 2017-03-14 564 */ 63c0eb30bfe9269 drivers/gpu/drm/i915/gt/uc/intel_uc.c Vinay Belgaumkar 2021-07-30 565 err_submission: 63c0eb30bfe9269 drivers/gpu/drm/i915/gt/uc/intel_uc.c Vinay Belgaumkar 2021-07-30 566 intel_guc_submission_disable(guc); ac58d2ab0ad9c8b drivers/gpu/drm/i915/intel_uc.c Daniele Ceraolo Spurio 2017-05-22 567 err_log_capture: 32ff76e80c2400c drivers/gpu/drm/i915/gt/uc/intel_uc.c Michal Wajdeczko 2019-08-02 568 __uc_capture_load_err_log(uc); 121981fafe699d9 drivers/gpu/drm/i915/intel_uc.c Michal Wajdeczko 2017-12-06 569 err_out: 1c40d40f6835cde drivers/gpu/drm/i915/gt/uc/intel_uc.c Vinay Belgaumkar 2021-12-16 570 /* Return GT back to RPn */ 1c40d40f6835cde drivers/gpu/drm/i915/gt/uc/intel_uc.c Vinay Belgaumkar 2021-12-16 571 intel_rps_lower_unslice(&uc_to_gt(uc)->rps); 1c40d40f6835cde drivers/gpu/drm/i915/gt/uc/intel_uc.c Vinay Belgaumkar 2021-12-16 572 b4aa935db7f0b46 drivers/gpu/drm/i915/gt/uc/intel_uc.c Ashutosh Dixit 2023-04-05 573 i915_hwmon_power_max_restore(gt->i915, pl1en); b4aa935db7f0b46 drivers/gpu/drm/i915/gt/uc/intel_uc.c Ashutosh Dixit 2023-04-05 574 ca7b2c1bbede618 drivers/gpu/drm/i915/gt/uc/intel_uc.c Daniele Ceraolo Spurio 2019-07-13 575 __uc_sanitize(uc); 89195bab5d8c540 drivers/gpu/drm/i915/intel_uc.c Michal Wajdeczko 2019-05-22 576 ee402140274e870 drivers/gpu/drm/i915/gt/uc/intel_uc.c Michal Wajdeczko 2019-08-18 577 if (!ret) { 2f8c06cb6622b55 drivers/gpu/drm/i915/gt/uc/intel_uc.c Michal Wajdeczko 2023-01-28 578 gt_notice(gt, "GuC is uninitialized\n"); ee402140274e870 drivers/gpu/drm/i915/gt/uc/intel_uc.c Michal Wajdeczko 2019-08-18 579 /* We want to run without GuC submission */ ee402140274e870 drivers/gpu/drm/i915/gt/uc/intel_uc.c Michal Wajdeczko 2019-08-18 580 return 0; ee402140274e870 drivers/gpu/drm/i915/gt/uc/intel_uc.c Michal Wajdeczko 2019-08-18 581 } ee402140274e870 drivers/gpu/drm/i915/gt/uc/intel_uc.c Michal Wajdeczko 2019-08-18 582 2f8c06cb6622b55 drivers/gpu/drm/i915/gt/uc/intel_uc.c Michal Wajdeczko 2023-01-28 583 gt_probe_error(gt, "GuC initialization failed %pe\n", ERR_PTR(ret)); a5f978c3609f02a drivers/gpu/drm/i915/gt/uc/intel_uc.c Michal Wajdeczko 2019-08-11 584 a5f978c3609f02a drivers/gpu/drm/i915/gt/uc/intel_uc.c Michal Wajdeczko 2019-08-11 585 /* We want to keep KMS alive */ a5f978c3609f02a drivers/gpu/drm/i915/gt/uc/intel_uc.c Michal Wajdeczko 2019-08-11 586 return -EIO; 6cd5a72c357732e drivers/gpu/drm/i915/intel_uc.c Arkadiusz Hiler 2017-03-14 587 } 6cd5a72c357732e drivers/gpu/drm/i915/intel_uc.c Arkadiusz Hiler 2017-03-14 588 -- 0-DAY CI Kernel Test Service https://github.com/intel/lkp-tests ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [Intel-gfx] [PATCH 2/3] drm/i915/guc: Disable PL1 power limit when loading GuC firmware 2023-04-06 4:45 ` Ashutosh Dixit (?) (?) @ 2023-04-07 11:08 ` Rodrigo Vivi 2023-04-10 22:17 ` Dixit, Ashutosh -1 siblings, 1 reply; 31+ messages in thread From: Rodrigo Vivi @ 2023-04-07 11:08 UTC (permalink / raw) To: Ashutosh Dixit; +Cc: intel-gfx, dri-devel On Wed, Apr 05, 2023 at 09:45:21PM -0700, 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. > > 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 > > v4: > - Don't hold the lock across GuC reset (Rodrigo) > - New locking scheme (suggested by Rodrigo) > - Eliminate rpm_get in power_max_disable/restore, not needed (Tvrtko) > > 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 | 40 +++++++++++++++++++++++++++ > drivers/gpu/drm/i915/i915_hwmon.h | 7 +++++ > 3 files changed, 56 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; we need to initialize this to make warn free builds happy... what's our default btw? false? true? we need to read it back? > > 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 7f44e809ca155..9ab8971679fe3 100644 > --- a/drivers/gpu/drm/i915/i915_hwmon.c > +++ b/drivers/gpu/drm/i915/i915_hwmon.c > @@ -50,6 +50,7 @@ struct hwm_drvdata { > struct hwm_energy_info ei; /* Energy info for energy1_input */ > char name[12]; > int gt_n; > + bool reset_in_progress; > }; > > struct i915_hwmon { > @@ -400,6 +401,10 @@ hwm_power_max_write(struct hwm_drvdata *ddat, long val) > u32 nval; > > mutex_lock(&hwmon->hwmon_lock); > + if (hwmon->ddat.reset_in_progress) { > + ret = -EAGAIN; > + goto unlock; > + } > wakeref = intel_runtime_pm_get(ddat->uncore->rpm); > > /* Disable PL1 limit and verify, because the limit cannot be disabled on all platforms */ > @@ -421,6 +426,7 @@ hwm_power_max_write(struct hwm_drvdata *ddat, long val) > PKG_PWR_LIM_1_EN | PKG_PWR_LIM_1, nval); > exit: > intel_runtime_pm_put(ddat->uncore->rpm, wakeref); > +unlock: > mutex_unlock(&hwmon->hwmon_lock); > return ret; > } > @@ -472,6 +478,40 @@ 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) > +{ > + struct i915_hwmon *hwmon = i915->hwmon; > + u32 r; > + > + if (!hwmon || !i915_mmio_reg_valid(hwmon->rg.pkg_rapl_limit)) > + return; > + > + mutex_lock(&hwmon->hwmon_lock); > + > + hwmon->ddat.reset_in_progress = true; > + 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); > + > + mutex_unlock(&hwmon->hwmon_lock); > +} > + > +void i915_hwmon_power_max_restore(struct drm_i915_private *i915, bool old) > +{ > + struct i915_hwmon *hwmon = i915->hwmon; > + > + if (!hwmon || !i915_mmio_reg_valid(hwmon->rg.pkg_rapl_limit)) > + return; > + > + mutex_lock(&hwmon->hwmon_lock); > + > + intel_uncore_rmw(hwmon->ddat.uncore, hwmon->rg.pkg_rapl_limit, > + PKG_PWR_LIM_1_EN, old ? PKG_PWR_LIM_1_EN : 0); > + hwmon->ddat.reset_in_progress = false; > + > + mutex_unlock(&hwmon->hwmon_lock); > +} you could have combined both functions in a i915_hwmon_power_max_set(struct drm_i915_private *i915, bool val, bool *old) then pass NULL to old on the restoration times and have if (old) *old = !!(r & PKG_PWR_LIM_1_EN); But really up to you here, the current code is clear to follow imho so, with the pl1en initialization fixed: Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > + > 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__ */ > -- > 2.38.0 > ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [Intel-gfx] [PATCH 2/3] drm/i915/guc: Disable PL1 power limit when loading GuC firmware 2023-04-07 11:08 ` Rodrigo Vivi @ 2023-04-10 22:17 ` Dixit, Ashutosh 0 siblings, 0 replies; 31+ messages in thread From: Dixit, Ashutosh @ 2023-04-10 22:17 UTC (permalink / raw) To: Rodrigo Vivi; +Cc: intel-gfx, dri-devel On Fri, 07 Apr 2023 04:08:31 -0700, Rodrigo Vivi wrote: > Hi Rodrigo, > On Wed, Apr 05, 2023 at 09:45:21PM -0700, 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. > > > > 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 > > > > v4: > > - Don't hold the lock across GuC reset (Rodrigo) > > - New locking scheme (suggested by Rodrigo) > > - Eliminate rpm_get in power_max_disable/restore, not needed (Tvrtko) > > > > 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 | 40 +++++++++++++++++++++++++++ > > drivers/gpu/drm/i915/i915_hwmon.h | 7 +++++ > > 3 files changed, 56 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; > > we need to initialize this to make warn free builds happy... > what's our default btw? false? true? we need to read it back? Yes this was a real bug caught by the kernel build robot. We don't know the default till we read it back, which would mean exposing a new function. I have avoided exposing the new function, i.e. I have fixed this by creating a new (err_rps) label which will make sure that the variable is not used unless it is initialized. I am not expecting to see warnings from the build robot with this fix now. > > > > 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 7f44e809ca155..9ab8971679fe3 100644 > > --- a/drivers/gpu/drm/i915/i915_hwmon.c > > +++ b/drivers/gpu/drm/i915/i915_hwmon.c > > @@ -50,6 +50,7 @@ struct hwm_drvdata { > > struct hwm_energy_info ei; /* Energy info for energy1_input */ > > char name[12]; > > int gt_n; > > + bool reset_in_progress; > > }; > > > > struct i915_hwmon { > > @@ -400,6 +401,10 @@ hwm_power_max_write(struct hwm_drvdata *ddat, long val) > > u32 nval; > > > > mutex_lock(&hwmon->hwmon_lock); > > + if (hwmon->ddat.reset_in_progress) { > > + ret = -EAGAIN; > > + goto unlock; > > + } > > wakeref = intel_runtime_pm_get(ddat->uncore->rpm); > > > > /* Disable PL1 limit and verify, because the limit cannot be disabled on all platforms */ > > @@ -421,6 +426,7 @@ hwm_power_max_write(struct hwm_drvdata *ddat, long val) > > PKG_PWR_LIM_1_EN | PKG_PWR_LIM_1, nval); > > exit: > > intel_runtime_pm_put(ddat->uncore->rpm, wakeref); > > +unlock: > > mutex_unlock(&hwmon->hwmon_lock); > > return ret; > > } > > @@ -472,6 +478,40 @@ 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) > > +{ > > + struct i915_hwmon *hwmon = i915->hwmon; > > + u32 r; > > + > > + if (!hwmon || !i915_mmio_reg_valid(hwmon->rg.pkg_rapl_limit)) > > + return; > > + > > + mutex_lock(&hwmon->hwmon_lock); > > + > > + hwmon->ddat.reset_in_progress = true; > > + 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); > > + > > + mutex_unlock(&hwmon->hwmon_lock); > > +} > > + > > +void i915_hwmon_power_max_restore(struct drm_i915_private *i915, bool old) > > +{ > > + struct i915_hwmon *hwmon = i915->hwmon; > > + > > + if (!hwmon || !i915_mmio_reg_valid(hwmon->rg.pkg_rapl_limit)) > > + return; > > + > > + mutex_lock(&hwmon->hwmon_lock); > > + > > + intel_uncore_rmw(hwmon->ddat.uncore, hwmon->rg.pkg_rapl_limit, > > + PKG_PWR_LIM_1_EN, old ? PKG_PWR_LIM_1_EN : 0); > > + hwmon->ddat.reset_in_progress = false; > > + > > + mutex_unlock(&hwmon->hwmon_lock); > > +} > > you could have combined both functions in a > i915_hwmon_power_max_set(struct drm_i915_private *i915, bool val, bool *old) > > then pass NULL to old on the restoration times > and have > if (old) > *old = !!(r & PKG_PWR_LIM_1_EN); > > But really up to you here, the current code is clear to follow imho > so, with the pl1en initialization fixed: Yes, left this as is. > > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> Have retained the R-b since the fix in __uc_init_hw is minor. Thanks! Ashutosh > > + > > 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__ */ > > -- > > 2.38.0 > > ^ permalink raw reply [flat|nested] 31+ messages in thread
* [Intel-gfx] [PATCH 3/3] drm/i915/hwmon: Block waiting for GuC reset to complete 2023-04-06 4:45 ` Ashutosh Dixit @ 2023-04-06 4:45 ` Ashutosh Dixit -1 siblings, 0 replies; 31+ messages in thread From: Ashutosh Dixit @ 2023-04-06 4:45 UTC (permalink / raw) To: intel-gfx; +Cc: dri-devel, Rodrigo Vivi Instead of erroring out when GuC reset is in progress, block waiting for GuC reset to complete which is a more reasonable uapi behavior. Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com> --- drivers/gpu/drm/i915/i915_hwmon.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_hwmon.c b/drivers/gpu/drm/i915/i915_hwmon.c index 9ab8971679fe3..4343efb48e61b 100644 --- a/drivers/gpu/drm/i915/i915_hwmon.c +++ b/drivers/gpu/drm/i915/i915_hwmon.c @@ -51,6 +51,7 @@ struct hwm_drvdata { char name[12]; int gt_n; bool reset_in_progress; + wait_queue_head_t wqh; }; struct i915_hwmon { @@ -400,10 +401,15 @@ hwm_power_max_write(struct hwm_drvdata *ddat, long val) int ret = 0; u32 nval; +retry: mutex_lock(&hwmon->hwmon_lock); if (hwmon->ddat.reset_in_progress) { - ret = -EAGAIN; - goto unlock; + mutex_unlock(&hwmon->hwmon_lock); + ret = wait_event_interruptible(ddat->wqh, + !hwmon->ddat.reset_in_progress); + if (ret) + return ret; + goto retry; } wakeref = intel_runtime_pm_get(ddat->uncore->rpm); @@ -426,7 +432,6 @@ hwm_power_max_write(struct hwm_drvdata *ddat, long val) PKG_PWR_LIM_1_EN | PKG_PWR_LIM_1, nval); exit: intel_runtime_pm_put(ddat->uncore->rpm, wakeref); -unlock: mutex_unlock(&hwmon->hwmon_lock); return ret; } @@ -508,6 +513,7 @@ void i915_hwmon_power_max_restore(struct drm_i915_private *i915, bool old) intel_uncore_rmw(hwmon->ddat.uncore, hwmon->rg.pkg_rapl_limit, PKG_PWR_LIM_1_EN, old ? PKG_PWR_LIM_1_EN : 0); hwmon->ddat.reset_in_progress = false; + wake_up_all(&hwmon->ddat.wqh); mutex_unlock(&hwmon->hwmon_lock); } @@ -784,6 +790,7 @@ void i915_hwmon_register(struct drm_i915_private *i915) ddat->uncore = &i915->uncore; snprintf(ddat->name, sizeof(ddat->name), "i915"); ddat->gt_n = -1; + init_waitqueue_head(&ddat->wqh); for_each_gt(gt, i915, i) { ddat_gt = hwmon->ddat_gt + i; -- 2.38.0 ^ permalink raw reply related [flat|nested] 31+ messages in thread
* [PATCH 3/3] drm/i915/hwmon: Block waiting for GuC reset to complete @ 2023-04-06 4:45 ` Ashutosh Dixit 0 siblings, 0 replies; 31+ messages in thread From: Ashutosh Dixit @ 2023-04-06 4:45 UTC (permalink / raw) To: intel-gfx; +Cc: Tvrtko Ursulin, dri-devel, Rodrigo Vivi Instead of erroring out when GuC reset is in progress, block waiting for GuC reset to complete which is a more reasonable uapi behavior. Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com> --- drivers/gpu/drm/i915/i915_hwmon.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_hwmon.c b/drivers/gpu/drm/i915/i915_hwmon.c index 9ab8971679fe3..4343efb48e61b 100644 --- a/drivers/gpu/drm/i915/i915_hwmon.c +++ b/drivers/gpu/drm/i915/i915_hwmon.c @@ -51,6 +51,7 @@ struct hwm_drvdata { char name[12]; int gt_n; bool reset_in_progress; + wait_queue_head_t wqh; }; struct i915_hwmon { @@ -400,10 +401,15 @@ hwm_power_max_write(struct hwm_drvdata *ddat, long val) int ret = 0; u32 nval; +retry: mutex_lock(&hwmon->hwmon_lock); if (hwmon->ddat.reset_in_progress) { - ret = -EAGAIN; - goto unlock; + mutex_unlock(&hwmon->hwmon_lock); + ret = wait_event_interruptible(ddat->wqh, + !hwmon->ddat.reset_in_progress); + if (ret) + return ret; + goto retry; } wakeref = intel_runtime_pm_get(ddat->uncore->rpm); @@ -426,7 +432,6 @@ hwm_power_max_write(struct hwm_drvdata *ddat, long val) PKG_PWR_LIM_1_EN | PKG_PWR_LIM_1, nval); exit: intel_runtime_pm_put(ddat->uncore->rpm, wakeref); -unlock: mutex_unlock(&hwmon->hwmon_lock); return ret; } @@ -508,6 +513,7 @@ void i915_hwmon_power_max_restore(struct drm_i915_private *i915, bool old) intel_uncore_rmw(hwmon->ddat.uncore, hwmon->rg.pkg_rapl_limit, PKG_PWR_LIM_1_EN, old ? PKG_PWR_LIM_1_EN : 0); hwmon->ddat.reset_in_progress = false; + wake_up_all(&hwmon->ddat.wqh); mutex_unlock(&hwmon->hwmon_lock); } @@ -784,6 +790,7 @@ void i915_hwmon_register(struct drm_i915_private *i915) ddat->uncore = &i915->uncore; snprintf(ddat->name, sizeof(ddat->name), "i915"); ddat->gt_n = -1; + init_waitqueue_head(&ddat->wqh); for_each_gt(gt, i915, i) { ddat_gt = hwmon->ddat_gt + i; -- 2.38.0 ^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [Intel-gfx] [PATCH 3/3] drm/i915/hwmon: Block waiting for GuC reset to complete 2023-04-06 4:45 ` Ashutosh Dixit (?) @ 2023-04-07 11:04 ` Rodrigo Vivi 2023-04-10 22:40 ` Dixit, Ashutosh -1 siblings, 1 reply; 31+ messages in thread From: Rodrigo Vivi @ 2023-04-07 11:04 UTC (permalink / raw) To: Ashutosh Dixit; +Cc: intel-gfx, dri-devel On Wed, Apr 05, 2023 at 09:45:22PM -0700, Ashutosh Dixit wrote: > Instead of erroring out when GuC reset is in progress, block waiting for > GuC reset to complete which is a more reasonable uapi behavior. > > Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com> > --- > drivers/gpu/drm/i915/i915_hwmon.c | 13 ++++++++++--- > 1 file changed, 10 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_hwmon.c b/drivers/gpu/drm/i915/i915_hwmon.c > index 9ab8971679fe3..4343efb48e61b 100644 > --- a/drivers/gpu/drm/i915/i915_hwmon.c > +++ b/drivers/gpu/drm/i915/i915_hwmon.c > @@ -51,6 +51,7 @@ struct hwm_drvdata { > char name[12]; > int gt_n; > bool reset_in_progress; > + wait_queue_head_t wqh; > }; > > struct i915_hwmon { > @@ -400,10 +401,15 @@ hwm_power_max_write(struct hwm_drvdata *ddat, long val) > int ret = 0; > u32 nval; > > +retry: > mutex_lock(&hwmon->hwmon_lock); > if (hwmon->ddat.reset_in_progress) { > - ret = -EAGAIN; > - goto unlock; > + mutex_unlock(&hwmon->hwmon_lock); > + ret = wait_event_interruptible(ddat->wqh, > + !hwmon->ddat.reset_in_progress); this is indeed very clever! maybe just use the timeout version to be on the safeside and then return the -EAGAIN on timeout? my fear is probably due to the lack of knowledge on this wait queue, but I'm wondering what could go wrong if due to some funny race you enter this check right after wake_up_all below has passed and then you will be here indefinitely waiting... > + if (ret) > + return ret; > + goto retry; > } > wakeref = intel_runtime_pm_get(ddat->uncore->rpm); > > @@ -426,7 +432,6 @@ hwm_power_max_write(struct hwm_drvdata *ddat, long val) > PKG_PWR_LIM_1_EN | PKG_PWR_LIM_1, nval); > exit: > intel_runtime_pm_put(ddat->uncore->rpm, wakeref); > -unlock: > mutex_unlock(&hwmon->hwmon_lock); > return ret; > } > @@ -508,6 +513,7 @@ void i915_hwmon_power_max_restore(struct drm_i915_private *i915, bool old) > intel_uncore_rmw(hwmon->ddat.uncore, hwmon->rg.pkg_rapl_limit, > PKG_PWR_LIM_1_EN, old ? PKG_PWR_LIM_1_EN : 0); > hwmon->ddat.reset_in_progress = false; > + wake_up_all(&hwmon->ddat.wqh); > > mutex_unlock(&hwmon->hwmon_lock); > } > @@ -784,6 +790,7 @@ void i915_hwmon_register(struct drm_i915_private *i915) > ddat->uncore = &i915->uncore; > snprintf(ddat->name, sizeof(ddat->name), "i915"); > ddat->gt_n = -1; > + init_waitqueue_head(&ddat->wqh); > > for_each_gt(gt, i915, i) { > ddat_gt = hwmon->ddat_gt + i; > -- > 2.38.0 > ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [Intel-gfx] [PATCH 3/3] drm/i915/hwmon: Block waiting for GuC reset to complete 2023-04-07 11:04 ` [Intel-gfx] " Rodrigo Vivi @ 2023-04-10 22:40 ` Dixit, Ashutosh 0 siblings, 0 replies; 31+ messages in thread From: Dixit, Ashutosh @ 2023-04-10 22:40 UTC (permalink / raw) To: Rodrigo Vivi; +Cc: intel-gfx, dri-devel On Fri, 07 Apr 2023 04:04:06 -0700, Rodrigo Vivi wrote: > Hi Rodrigo, > On Wed, Apr 05, 2023 at 09:45:22PM -0700, Ashutosh Dixit wrote: > > Instead of erroring out when GuC reset is in progress, block waiting for > > GuC reset to complete which is a more reasonable uapi behavior. > > > > Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com> > > --- > > drivers/gpu/drm/i915/i915_hwmon.c | 13 ++++++++++--- > > 1 file changed, 10 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_hwmon.c b/drivers/gpu/drm/i915/i915_hwmon.c > > index 9ab8971679fe3..4343efb48e61b 100644 > > --- a/drivers/gpu/drm/i915/i915_hwmon.c > > +++ b/drivers/gpu/drm/i915/i915_hwmon.c > > @@ -51,6 +51,7 @@ struct hwm_drvdata { > > char name[12]; > > int gt_n; > > bool reset_in_progress; > > + wait_queue_head_t wqh; > > }; > > > > struct i915_hwmon { > > @@ -400,10 +401,15 @@ hwm_power_max_write(struct hwm_drvdata *ddat, long val) > > int ret = 0; > > u32 nval; > > > > +retry: > > mutex_lock(&hwmon->hwmon_lock); > > if (hwmon->ddat.reset_in_progress) { > > - ret = -EAGAIN; > > - goto unlock; > > + mutex_unlock(&hwmon->hwmon_lock); > > + ret = wait_event_interruptible(ddat->wqh, > > + !hwmon->ddat.reset_in_progress); > > this is indeed very clever! Not clever, see below :/ > my fear is probably due to the lack of knowledge on this wait queue, but > I'm wondering what could go wrong if due to some funny race you enter this > check right after wake_up_all below has passed and then you will be here > indefinitely waiting... You are absolutely right, there is indeed a race in the patch because in the above code when we drop the mutex (mutex_unlock) the wake_up_all can happen before we have queued ourselves for the wake up. Solving this race needs a more complicated prepare_to_wait/finish_wait sequence which I have gone ahead and implemented in patch v2. The v2 code is also a standard code pattern and the pattern I have implemented is basically the same as that in intel_guc_wait_for_pending_msg() in i915 which I liked. I have read in several places (e.g. in the Advanced Sleeping section in https://static.lwn.net/images/pdf/LDD3/ch06.pdf and in kernel documentation for try_to_wake_up()) that this sequence will avoid the race (between schedule() and wake_up()). The crucial difference from the v1 patch is that in v2 the mutex is dropped after we queue ourselves in prepare_to_wait() just before calling schedule_timeout(). > maybe just use the timeout version to be on the safeside and then return the > -EAGAIN on timeout? Also incorporated timeout in the new version. All code paths in the new patch have been tested. Thanks. -- Ashutosh > > + if (ret) > > + return ret; > > + goto retry; > > } > > wakeref = intel_runtime_pm_get(ddat->uncore->rpm); > > > > @@ -426,7 +432,6 @@ hwm_power_max_write(struct hwm_drvdata *ddat, long val) > > PKG_PWR_LIM_1_EN | PKG_PWR_LIM_1, nval); > > exit: > > intel_runtime_pm_put(ddat->uncore->rpm, wakeref); > > -unlock: > > mutex_unlock(&hwmon->hwmon_lock); > > return ret; > > } > > @@ -508,6 +513,7 @@ void i915_hwmon_power_max_restore(struct drm_i915_private *i915, bool old) > > intel_uncore_rmw(hwmon->ddat.uncore, hwmon->rg.pkg_rapl_limit, > > PKG_PWR_LIM_1_EN, old ? PKG_PWR_LIM_1_EN : 0); > > hwmon->ddat.reset_in_progress = false; > > + wake_up_all(&hwmon->ddat.wqh); > > > > mutex_unlock(&hwmon->hwmon_lock); > > } > > @@ -784,6 +790,7 @@ void i915_hwmon_register(struct drm_i915_private *i915) > > ddat->uncore = &i915->uncore; > > snprintf(ddat->name, sizeof(ddat->name), "i915"); > > ddat->gt_n = -1; > > + init_waitqueue_head(&ddat->wqh); > > > > for_each_gt(gt, i915, i) { > > ddat_gt = hwmon->ddat_gt + i; > > -- > > 2.38.0 > > ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [Intel-gfx] [PATCH 3/3] drm/i915/hwmon: Block waiting for GuC reset to complete @ 2023-04-10 22:40 ` Dixit, Ashutosh 0 siblings, 0 replies; 31+ messages in thread From: Dixit, Ashutosh @ 2023-04-10 22:40 UTC (permalink / raw) To: Rodrigo Vivi; +Cc: Tvrtko Ursulin, intel-gfx, dri-devel On Fri, 07 Apr 2023 04:04:06 -0700, Rodrigo Vivi wrote: > Hi Rodrigo, > On Wed, Apr 05, 2023 at 09:45:22PM -0700, Ashutosh Dixit wrote: > > Instead of erroring out when GuC reset is in progress, block waiting for > > GuC reset to complete which is a more reasonable uapi behavior. > > > > Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com> > > --- > > drivers/gpu/drm/i915/i915_hwmon.c | 13 ++++++++++--- > > 1 file changed, 10 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_hwmon.c b/drivers/gpu/drm/i915/i915_hwmon.c > > index 9ab8971679fe3..4343efb48e61b 100644 > > --- a/drivers/gpu/drm/i915/i915_hwmon.c > > +++ b/drivers/gpu/drm/i915/i915_hwmon.c > > @@ -51,6 +51,7 @@ struct hwm_drvdata { > > char name[12]; > > int gt_n; > > bool reset_in_progress; > > + wait_queue_head_t wqh; > > }; > > > > struct i915_hwmon { > > @@ -400,10 +401,15 @@ hwm_power_max_write(struct hwm_drvdata *ddat, long val) > > int ret = 0; > > u32 nval; > > > > +retry: > > mutex_lock(&hwmon->hwmon_lock); > > if (hwmon->ddat.reset_in_progress) { > > - ret = -EAGAIN; > > - goto unlock; > > + mutex_unlock(&hwmon->hwmon_lock); > > + ret = wait_event_interruptible(ddat->wqh, > > + !hwmon->ddat.reset_in_progress); > > this is indeed very clever! Not clever, see below :/ > my fear is probably due to the lack of knowledge on this wait queue, but > I'm wondering what could go wrong if due to some funny race you enter this > check right after wake_up_all below has passed and then you will be here > indefinitely waiting... You are absolutely right, there is indeed a race in the patch because in the above code when we drop the mutex (mutex_unlock) the wake_up_all can happen before we have queued ourselves for the wake up. Solving this race needs a more complicated prepare_to_wait/finish_wait sequence which I have gone ahead and implemented in patch v2. The v2 code is also a standard code pattern and the pattern I have implemented is basically the same as that in intel_guc_wait_for_pending_msg() in i915 which I liked. I have read in several places (e.g. in the Advanced Sleeping section in https://static.lwn.net/images/pdf/LDD3/ch06.pdf and in kernel documentation for try_to_wake_up()) that this sequence will avoid the race (between schedule() and wake_up()). The crucial difference from the v1 patch is that in v2 the mutex is dropped after we queue ourselves in prepare_to_wait() just before calling schedule_timeout(). > maybe just use the timeout version to be on the safeside and then return the > -EAGAIN on timeout? Also incorporated timeout in the new version. All code paths in the new patch have been tested. Thanks. -- Ashutosh > > + if (ret) > > + return ret; > > + goto retry; > > } > > wakeref = intel_runtime_pm_get(ddat->uncore->rpm); > > > > @@ -426,7 +432,6 @@ hwm_power_max_write(struct hwm_drvdata *ddat, long val) > > PKG_PWR_LIM_1_EN | PKG_PWR_LIM_1, nval); > > exit: > > intel_runtime_pm_put(ddat->uncore->rpm, wakeref); > > -unlock: > > mutex_unlock(&hwmon->hwmon_lock); > > return ret; > > } > > @@ -508,6 +513,7 @@ void i915_hwmon_power_max_restore(struct drm_i915_private *i915, bool old) > > intel_uncore_rmw(hwmon->ddat.uncore, hwmon->rg.pkg_rapl_limit, > > PKG_PWR_LIM_1_EN, old ? PKG_PWR_LIM_1_EN : 0); > > hwmon->ddat.reset_in_progress = false; > > + wake_up_all(&hwmon->ddat.wqh); > > > > mutex_unlock(&hwmon->hwmon_lock); > > } > > @@ -784,6 +790,7 @@ void i915_hwmon_register(struct drm_i915_private *i915) > > ddat->uncore = &i915->uncore; > > snprintf(ddat->name, sizeof(ddat->name), "i915"); > > ddat->gt_n = -1; > > + init_waitqueue_head(&ddat->wqh); > > > > for_each_gt(gt, i915, i) { > > ddat_gt = hwmon->ddat_gt + i; > > -- > > 2.38.0 > > ^ permalink raw reply [flat|nested] 31+ messages in thread
* [Intel-gfx] ✗ Fi.CI.SPARSE: warning for drm/i915/guc: Disable PL1 power limit when loading GuC firmware 2023-04-06 4:45 ` Ashutosh Dixit ` (3 preceding siblings ...) (?) @ 2023-04-06 5:15 ` Patchwork -1 siblings, 0 replies; 31+ messages in thread From: Patchwork @ 2023-04-06 5:15 UTC (permalink / raw) To: Dixit, Ashutosh; +Cc: intel-gfx == Series Details == Series: drm/i915/guc: Disable PL1 power limit when loading GuC firmware URL : https://patchwork.freedesktop.org/series/116172/ State : warning == Summary == Error: dim sparse failed Sparse version: v0.6.2 Fast mode used, each commit won't be checked separately. +./arch/x86/include/asm/bitops.h:117:1: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:117:1: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:148:1: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:148:1: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:150:9: warning: unreplaced symbol 'oldbit' +./arch/x86/include/asm/bitops.h:150:9: warning: unreplaced symbol 'oldbit' +./arch/x86/include/asm/bitops.h:154:26: warning: unreplaced symbol 'oldbit' +./arch/x86/include/asm/bitops.h:154:26: warning: unreplaced symbol 'oldbit' +./arch/x86/include/asm/bitops.h:156:16: warning: unreplaced symbol 'oldbit' +./arch/x86/include/asm/bitops.h:156:16: warning: unreplaced symbol 'oldbit' +./arch/x86/include/asm/bitops.h:156:9: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:156:9: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:174:1: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:174:1: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:176:9: warning: unreplaced symbol 'oldbit' +./arch/x86/include/asm/bitops.h:176:9: warning: unreplaced symbol 'oldbit' +./arch/x86/include/asm/bitops.h:180:35: warning: unreplaced symbol 'oldbit' +./arch/x86/include/asm/bitops.h:180:35: warning: unreplaced symbol 'oldbit' +./arch/x86/include/asm/bitops.h:182:16: warning: unreplaced symbol 'oldbit' +./arch/x86/include/asm/bitops.h:182:16: warning: unreplaced symbol 'oldbit' +./arch/x86/include/asm/bitops.h:182:9: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:182:9: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:186:1: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:186:1: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:188:9: warning: unreplaced symbol 'oldbit' +./arch/x86/include/asm/bitops.h:188:9: warning: unreplaced symbol 'oldbit' +./arch/x86/include/asm/bitops.h:192:35: warning: unreplaced symbol 'oldbit' +./arch/x86/include/asm/bitops.h:192:35: warning: unreplaced symbol 'oldbit' +./arch/x86/include/asm/bitops.h:195:16: warning: unreplaced symbol 'oldbit' +./arch/x86/include/asm/bitops.h:195:16: warning: unreplaced symbol 'oldbit' +./arch/x86/include/asm/bitops.h:195:9: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:195:9: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:237:1: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:237:1: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:239:9: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:239:9: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:66:1: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:66:1: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:92:1: warning: unreplaced symbol 'return' +./arch/x86/include/asm/bitops.h:92:1: warning: unreplaced symbol 'return' +./include/asm-generic/bitops/generic-non-atomic.h:100:17: warning: unreplaced symbol 'old' +./include/asm-generic/bitops/generic-non-atomic.h:100:17: warning: unreplaced symbol 'old' +./include/asm-generic/bitops/generic-non-atomic.h:100:23: warning: unreplaced symbol 'mask' +./include/asm-generic/bitops/generic-non-atomic.h:100:23: warning: unreplaced symbol 'mask' +./include/asm-generic/bitops/generic-non-atomic.h:100:9: warning: unreplaced symbol 'return' +./include/asm-generic/bitops/generic-non-atomic.h:100:9: warning: unreplaced symbol 'return' +./include/asm-generic/bitops/generic-non-atomic.h:105:1: warning: unreplaced symbol 'return' +./include/asm-generic/bitops/generic-non-atomic.h:105:1: warning: unreplaced symbol 'return' +./include/asm-generic/bitops/generic-non-atomic.h:107:9: warning: unreplaced symbol 'mask' +./include/asm-generic/bitops/generic-non-atomic.h:107:9: warning: unreplaced symbol 'mask' +./include/asm-generic/bitops/generic-non-atomic.h:108:9: warning: unreplaced symbol 'p' +./include/asm-generic/bitops/generic-non-atomic.h:108:9: warning: unreplaced symbol 'p' +./include/asm-generic/bitops/generic-non-atomic.h:109:9: warning: unreplaced symbol 'old' +./include/asm-generic/bitops/generic-non-atomic.h:109:9: warning: unreplaced symbol 'old' +./include/asm-generic/bitops/generic-non-atomic.h:111:10: warning: unreplaced symbol 'p' +./include/asm-generic/bitops/generic-non-atomic.h:111:10: warning: unreplaced symbol 'p' +./include/asm-generic/bitops/generic-non-atomic.h:111:14: warning: unreplaced symbol 'old' +./include/asm-generic/bitops/generic-non-atomic.h:111:14: warning: unreplaced symbol 'old' +./include/asm-generic/bitops/generic-non-atomic.h:111:20: warning: unreplaced symbol 'mask' +./include/asm-generic/bitops/generic-non-atomic.h:111:20: warning: unreplaced symbol 'mask' +./include/asm-generic/bitops/generic-non-atomic.h:112:17: warning: unreplaced symbol 'old' +./include/asm-generic/bitops/generic-non-atomic.h:112:17: warning: unreplaced symbol 'old' +./include/asm-generic/bitops/generic-non-atomic.h:112:23: warning: unreplaced symbol 'mask' +./include/asm-generic/bitops/generic-non-atomic.h:112:23: warning: unreplaced symbol 'mask' +./include/asm-generic/bitops/generic-non-atomic.h:112:9: warning: unreplaced symbol 'return' +./include/asm-generic/bitops/generic-non-atomic.h:112:9: warning: unreplaced symbol 'return' +./include/asm-generic/bitops/generic-non-atomic.h:121:1: warning: unreplaced symbol 'return' +./include/asm-generic/bitops/generic-non-atomic.h:121:1: warning: unreplaced symbol 'return' +./include/asm-generic/bitops/generic-non-atomic.h:128:9: warning: unreplaced symbol 'return' +./include/asm-generic/bitops/generic-non-atomic.h:128:9: warning: unreplaced symbol 'return' +./include/asm-generic/bitops/generic-non-atomic.h:166:1: warning: unreplaced symbol 'return' +./include/asm-generic/bitops/generic-non-atomic.h:166:1: warning: unreplaced symbol 'return' +./include/asm-generic/bitops/generic-non-atomic.h:168:9: warning: unreplaced symbol 'p' +./include/asm-generic/bitops/generic-non-atomic.h:168:9: warning: unreplaced symbol 'p' +./include/asm-generic/bitops/generic-non-atomic.h:169:9: warning: unreplaced symbol 'mask' +./include/asm-generic/bitops/generic-non-atomic.h:169:9: warning: unreplaced symbol 'mask' +./include/asm-generic/bitops/generic-non-atomic.h:170:9: warning: unreplaced symbol 'val' +./include/asm-generic/bitops/generic-non-atomic.h:170:9: warning: unreplaced symbol 'val' +./include/asm-generic/bitops/generic-non-atomic.h:172:19: warning: unreplaced symbol 'val' +./include/asm-generic/bitops/generic-non-atomic.h:172:19: warning: unreplaced symbol 'val' +./include/asm-generic/bitops/generic-non-atomic.h:172:25: warning: unreplaced symbol 'mask' +./include/asm-generic/bitops/generic-non-atomic.h:172:25: warning: unreplaced symbol 'mask' +./include/asm-generic/bitops/generic-non-atomic.h:172:9: warning: unreplaced symbol 'return' +./include/asm-generic/bitops/generic-non-atomic.h:172:9: warning: unreplaced symbol 'return' +./include/asm-generic/bitops/generic-non-atomic.h:28:1: warning: unreplaced symbol 'return' +./include/asm-generic/bitops/generic-non-atomic.h:28:1: warning: unreplaced symbol 'return' +./include/asm-generic/bitops/generic-non-atomic.h:30:9: warning: unreplaced symbol 'mask' +./include/asm-generic/bitops/generic-non-atomic.h:30:9: warning: unreplaced symbol 'mask' +./include/asm-generic/bitops/generic-non-atomic.h:31:9: warning: unreplaced symbol 'p' +./include/asm-generic/bitops/generic-non-atomic.h:31:9: warning: unreplaced symbol 'p' +./include/asm-generic/bitops/generic-non-atomic.h:33:10: warning: unreplaced symbol 'p' +./include/asm-generic/bitops/generic-non-atomic.h:33:10: warning: unreplaced symbol 'p' +./include/asm-generic/bitops/generic-non-atomic.h:33:16: warning: unreplaced symbol 'mask' +./include/asm-generic/bitops/generic-non-atomic.h:33:16: warning: unreplaced symbol 'mask' +./include/asm-generic/bitops/generic-non-atomic.h:37:1: warning: unreplaced symbol 'return' +./include/asm-generic/bitops/generic-non-atomic.h:37:1: warning: unreplaced symbol 'return' +./include/asm-generic/bitops/generic-non-atomic.h:39:9: warning: unreplaced symbol 'mask' +./include/asm-generic/bitops/generic-non-atomic.h:39:9: warning: unreplaced symbol 'mask' +./include/asm-generic/bitops/generic-non-atomic.h:40:9: warning: unreplaced symbol 'p' +./include/asm-generic/bitops/generic-non-atomic.h:40:9: warning: unreplaced symbol 'p' +./include/asm-generic/bitops/generic-non-atomic.h:42:10: warning: unreplaced symbol 'p' +./include/asm-generic/bitops/generic-non-atomic.h:42:10: warning: unreplaced symbol 'p' +./include/asm-generic/bitops/generic-non-atomic.h:42:16: warning: unreplaced symbol 'mask' +./include/asm-generic/bitops/generic-non-atomic.h:42:16: warning: unreplaced symbol 'mask' +./include/asm-generic/bitops/generic-non-atomic.h:55:1: warning: unreplaced symbol 'return' +./include/asm-generic/bitops/generic-non-atomic.h:55:1: warning: unreplaced symbol 'return' +./include/asm-generic/bitops/generic-non-atomic.h:57:9: warning: unreplaced symbol 'mask' +./include/asm-generic/bitops/generic-non-atomic.h:57:9: warning: unreplaced symbol 'mask' +./include/asm-generic/bitops/generic-non-atomic.h:58:9: warning: unreplaced symbol 'p' +./include/asm-generic/bitops/generic-non-atomic.h:58:9: warning: unreplaced symbol 'p' +./include/asm-generic/bitops/generic-non-atomic.h:60:10: warning: unreplaced symbol 'p' +./include/asm-generic/bitops/generic-non-atomic.h:60:10: warning: unreplaced symbol 'p' +./include/asm-generic/bitops/generic-non-atomic.h:60:15: warning: unreplaced symbol 'mask' +./include/asm-generic/bitops/generic-non-atomic.h:60:15: warning: unreplaced symbol 'mask' +./include/asm-generic/bitops/generic-non-atomic.h:73:1: warning: unreplaced symbol 'return' +./include/asm-generic/bitops/generic-non-atomic.h:73:1: warning: unreplaced symbol 'return' +./include/asm-generic/bitops/generic-non-atomic.h:75:9: warning: unreplaced symbol 'mask' +./include/asm-generic/bitops/generic-non-atomic.h:75:9: warning: unreplaced symbol 'mask' +./include/asm-generic/bitops/generic-non-atomic.h:76:9: warning: unreplaced symbol 'p' +./include/asm-generic/bitops/generic-non-atomic.h:76:9: warning: unreplaced symbol 'p' +./include/asm-generic/bitops/generic-non-atomic.h:77:9: warning: unreplaced symbol 'old' +./include/asm-generic/bitops/generic-non-atomic.h:77:9: warning: unreplaced symbol 'old' +./include/asm-generic/bitops/generic-non-atomic.h:79:10: warning: unreplaced symbol 'p' +./include/asm-generic/bitops/generic-non-atomic.h:79:10: warning: unreplaced symbol 'p' +./include/asm-generic/bitops/generic-non-atomic.h:79:14: warning: unreplaced symbol 'old' +./include/asm-generic/bitops/generic-non-atomic.h:79:14: warning: unreplaced symbol 'old' +./include/asm-generic/bitops/generic-non-atomic.h:79:20: warning: unreplaced symbol 'mask' +./include/asm-generic/bitops/generic-non-atomic.h:79:20: warning: unreplaced symbol 'mask' +./include/asm-generic/bitops/generic-non-atomic.h:80:17: warning: unreplaced symbol 'old' +./include/asm-generic/bitops/generic-non-atomic.h:80:17: warning: unreplaced symbol 'old' +./include/asm-generic/bitops/generic-non-atomic.h:80:23: warning: unreplaced symbol 'mask' +./include/asm-generic/bitops/generic-non-atomic.h:80:23: warning: unreplaced symbol 'mask' +./include/asm-generic/bitops/generic-non-atomic.h:80:9: warning: unreplaced symbol 'return' +./include/asm-generic/bitops/generic-non-atomic.h:80:9: warning: unreplaced symbol 'return' +./include/asm-generic/bitops/generic-non-atomic.h:93:1: warning: unreplaced symbol 'return' +./include/asm-generic/bitops/generic-non-atomic.h:93:1: warning: unreplaced symbol 'return' +./include/asm-generic/bitops/generic-non-atomic.h:95:9: warning: unreplaced symbol 'mask' +./include/asm-generic/bitops/generic-non-atomic.h:95:9: warning: unreplaced symbol 'mask' +./include/asm-generic/bitops/generic-non-atomic.h:96:9: warning: unreplaced symbol 'p' +./include/asm-generic/bitops/generic-non-atomic.h:96:9: warning: unreplaced symbol 'p' +./include/asm-generic/bitops/generic-non-atomic.h:97:9: warning: unreplaced symbol 'old' +./include/asm-generic/bitops/generic-non-atomic.h:97:9: warning: unreplaced symbol 'old' +./include/asm-generic/bitops/generic-non-atomic.h:99:10: warning: unreplaced symbol 'p' +./include/asm-generic/bitops/generic-non-atomic.h:99:10: warning: unreplaced symbol 'p' +./include/asm-generic/bitops/generic-non-atomic.h:99:14: warning: unreplaced symbol 'old' +./include/asm-generic/bitops/generic-non-atomic.h:99:14: warning: unreplaced symbol 'old' +./include/asm-generic/bitops/generic-non-atomic.h:99:21: warning: unreplaced symbol 'mask' +./include/asm-generic/bitops/generic-non-atomic.h:99:21: warning: unreplaced symbol 'mask' +./include/asm-generic/bitops/instrumented-non-atomic.h:100:9: warning: unreplaced symbol 'return' +./include/asm-generic/bitops/instrumented-non-atomic.h:100:9: warning: unreplaced symbol 'return' +./include/asm-generic/bitops/instrumented-non-atomic.h:112:1: warning: unreplaced symbol 'return' +./include/asm-generic/bitops/instrumented-non-atomic.h:112:1: warning: unreplaced symbol 'return' +./include/asm-generic/bitops/instrumented-non-atomic.h:115:9: warning: unreplaced symbol 'return' +./include/asm-generic/bitops/instrumented-non-atomic.h:115:9: warning: unreplaced symbol 'return' +./include/asm-generic/bitops/instrumented-non-atomic.h:127:1: warning: unreplaced symbol 'return' +./include/asm-generic/bitops/instrumented-non-atomic.h:127:1: warning: unreplaced symbol 'return' +./include/asm-generic/bitops/instrumented-non-atomic.h:130:9: warning: unreplaced symbol 'return' +./include/asm-generic/bitops/instrumented-non-atomic.h:130:9: warning: unreplaced symbol 'return' +./include/asm-generic/bitops/instrumented-non-atomic.h:139:1: warning: unreplaced symbol 'return' +./include/asm-generic/bitops/instrumented-non-atomic.h:139:1: warning: unreplaced symbol 'return' +./include/asm-generic/bitops/instrumented-non-atomic.h:142:9: warning: unreplaced symbol 'return' +./include/asm-generic/bitops/instrumented-non-atomic.h:142:9: warning: unreplaced symbol 'return' +./include/asm-generic/bitops/instrumented-non-atomic.h:26:1: warning: unreplaced symbol 'return' +./include/asm-generic/bitops/instrumented-non-atomic.h:26:1: warning: unreplaced symbol 'return' +./include/asm-generic/bitops/instrumented-non-atomic.h:42:1: warning: unreplaced symbol 'return' +./include/asm-generic/bitops/instrumented-non-atomic.h:42:1: warning: unreplaced symbol 'return' +./include/asm-generic/bitops/instrumented-non-atomic.h:58:1: warning: unreplaced symbol 'return' +./include/asm-generic/bitops/instrumented-non-atomic.h:58:1: warning: unreplaced symbol 'return' +./include/asm-generic/bitops/instrumented-non-atomic.h:97:1: warning: unreplaced symbol 'return' +./include/asm-generic/bitops/instrumented-non-atomic.h:97:1: warning: unreplaced symbol 'return' ^ permalink raw reply [flat|nested] 31+ messages in thread
* [Intel-gfx] ✓ Fi.CI.BAT: success for drm/i915/guc: Disable PL1 power limit when loading GuC firmware 2023-04-06 4:45 ` Ashutosh Dixit ` (4 preceding siblings ...) (?) @ 2023-04-06 5:32 ` Patchwork -1 siblings, 0 replies; 31+ messages in thread From: Patchwork @ 2023-04-06 5:32 UTC (permalink / raw) To: Dixit, Ashutosh; +Cc: intel-gfx [-- Attachment #1: Type: text/plain, Size: 5734 bytes --] == Series Details == Series: drm/i915/guc: Disable PL1 power limit when loading GuC firmware URL : https://patchwork.freedesktop.org/series/116172/ State : success == Summary == CI Bug Log - changes from CI_DRM_12976 -> Patchwork_116172v1 ==================================================== Summary ------- **SUCCESS** No regressions found. External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116172v1/index.html Participating hosts (37 -> 36) ------------------------------ Missing (1): fi-snb-2520m Known issues ------------ Here are the changes found in Patchwork_116172v1 that come from known issues: ### IGT changes ### #### Issues hit #### * igt@gem_exec_suspend@basic-s3@lmem0: - bat-dg2-9: [PASS][1] -> [FAIL][2] ([fdo#103375]) +3 similar issues [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12976/bat-dg2-9/igt@gem_exec_suspend@basic-s3@lmem0.html [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116172v1/bat-dg2-9/igt@gem_exec_suspend@basic-s3@lmem0.html * igt@i915_selftest@live@gt_heartbeat: - fi-kbl-soraka: [PASS][3] -> [DMESG-FAIL][4] ([i915#5334] / [i915#7872]) [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12976/fi-kbl-soraka/igt@i915_selftest@live@gt_heartbeat.html [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116172v1/fi-kbl-soraka/igt@i915_selftest@live@gt_heartbeat.html * igt@i915_selftest@live@slpc: - bat-rpls-2: NOTRUN -> [DMESG-FAIL][5] ([i915#6367] / [i915#7913] / [i915#7996]) [5]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116172v1/bat-rpls-2/igt@i915_selftest@live@slpc.html * igt@kms_chamelium_hpd@common-hpd-after-suspend: - bat-rpls-2: NOTRUN -> [SKIP][6] ([i915#7828]) [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116172v1/bat-rpls-2/igt@kms_chamelium_hpd@common-hpd-after-suspend.html - bat-rpls-1: NOTRUN -> [SKIP][7] ([i915#7828]) [7]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116172v1/bat-rpls-1/igt@kms_chamelium_hpd@common-hpd-after-suspend.html * igt@kms_pipe_crc_basic@nonblocking-crc-frame-sequence@pipe-c-dp-1: - bat-dg2-8: [PASS][8] -> [FAIL][9] ([i915#7932]) [8]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12976/bat-dg2-8/igt@kms_pipe_crc_basic@nonblocking-crc-frame-sequence@pipe-c-dp-1.html [9]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116172v1/bat-dg2-8/igt@kms_pipe_crc_basic@nonblocking-crc-frame-sequence@pipe-c-dp-1.html * igt@kms_pipe_crc_basic@suspend-read-crc: - bat-rpls-1: NOTRUN -> [SKIP][10] ([i915#1845]) [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116172v1/bat-rpls-1/igt@kms_pipe_crc_basic@suspend-read-crc.html - bat-rpls-2: NOTRUN -> [SKIP][11] ([i915#1845]) [11]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116172v1/bat-rpls-2/igt@kms_pipe_crc_basic@suspend-read-crc.html * igt@kms_pipe_crc_basic@suspend-read-crc@pipe-c-dp-3: - bat-dg2-9: [PASS][12] -> [FAIL][13] ([fdo#103375] / [i915#7932]) [12]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12976/bat-dg2-9/igt@kms_pipe_crc_basic@suspend-read-crc@pipe-c-dp-3.html [13]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116172v1/bat-dg2-9/igt@kms_pipe_crc_basic@suspend-read-crc@pipe-c-dp-3.html #### Possible fixes #### * igt@gem_exec_suspend@basic-s3@smem: - bat-rpls-1: [ABORT][14] ([i915#6687] / [i915#7978]) -> [PASS][15] [14]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12976/bat-rpls-1/igt@gem_exec_suspend@basic-s3@smem.html [15]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116172v1/bat-rpls-1/igt@gem_exec_suspend@basic-s3@smem.html * igt@i915_selftest@live@reset: - bat-rpls-2: [ABORT][16] ([i915#4983] / [i915#7913] / [i915#7981]) -> [PASS][17] [16]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12976/bat-rpls-2/igt@i915_selftest@live@reset.html [17]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116172v1/bat-rpls-2/igt@i915_selftest@live@reset.html [fdo#103375]: https://bugs.freedesktop.org/show_bug.cgi?id=103375 [i915#1845]: https://gitlab.freedesktop.org/drm/intel/issues/1845 [i915#4983]: https://gitlab.freedesktop.org/drm/intel/issues/4983 [i915#5334]: https://gitlab.freedesktop.org/drm/intel/issues/5334 [i915#6367]: https://gitlab.freedesktop.org/drm/intel/issues/6367 [i915#6687]: https://gitlab.freedesktop.org/drm/intel/issues/6687 [i915#7828]: https://gitlab.freedesktop.org/drm/intel/issues/7828 [i915#7872]: https://gitlab.freedesktop.org/drm/intel/issues/7872 [i915#7913]: https://gitlab.freedesktop.org/drm/intel/issues/7913 [i915#7932]: https://gitlab.freedesktop.org/drm/intel/issues/7932 [i915#7978]: https://gitlab.freedesktop.org/drm/intel/issues/7978 [i915#7981]: https://gitlab.freedesktop.org/drm/intel/issues/7981 [i915#7996]: https://gitlab.freedesktop.org/drm/intel/issues/7996 Build changes ------------- * Linux: CI_DRM_12976 -> Patchwork_116172v1 CI-20190529: 20190529 CI_DRM_12976: 517e3daa2f8c38df4969591325325de8315685b9 @ git://anongit.freedesktop.org/gfx-ci/linux IGT_7240: ef4550e3b7d3c11ba257006bc7d4f8e421667d46 @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git Patchwork_116172v1: 517e3daa2f8c38df4969591325325de8315685b9 @ git://anongit.freedesktop.org/gfx-ci/linux ### Linux commits 5f9162aad7cf drm/i915/hwmon: Block waiting for GuC reset to complete 8aebbdfa10a0 drm/i915/guc: Disable PL1 power limit when loading GuC firmware 284c2ad65107 drm/i915/hwmon: Get mutex and rpm ref just once in hwm_power_max_write == Logs == For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116172v1/index.html [-- Attachment #2: Type: text/html, Size: 6930 bytes --] ^ permalink raw reply [flat|nested] 31+ messages in thread
* [Intel-gfx] ✓ Fi.CI.IGT: success for drm/i915/guc: Disable PL1 power limit when loading GuC firmware 2023-04-06 4:45 ` Ashutosh Dixit ` (5 preceding siblings ...) (?) @ 2023-04-06 17:42 ` Patchwork -1 siblings, 0 replies; 31+ messages in thread From: Patchwork @ 2023-04-06 17:42 UTC (permalink / raw) To: Dixit, Ashutosh; +Cc: intel-gfx [-- Attachment #1: Type: text/plain, Size: 15187 bytes --] == Series Details == Series: drm/i915/guc: Disable PL1 power limit when loading GuC firmware URL : https://patchwork.freedesktop.org/series/116172/ State : success == Summary == CI Bug Log - changes from CI_DRM_12976_full -> Patchwork_116172v1_full ==================================================== Summary ------- **SUCCESS** No regressions found. Participating hosts (7 -> 7) ------------------------------ No changes in participating hosts Known issues ------------ Here are the changes found in Patchwork_116172v1_full that come from known issues: ### IGT changes ### #### Issues hit #### * igt@gem_exec_fair@basic-pace-solo@rcs0: - shard-apl: [PASS][1] -> [FAIL][2] ([i915#2842]) [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12976/shard-apl3/igt@gem_exec_fair@basic-pace-solo@rcs0.html [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116172v1/shard-apl4/igt@gem_exec_fair@basic-pace-solo@rcs0.html * igt@gem_lmem_swapping@heavy-verify-multi: - shard-apl: NOTRUN -> [SKIP][3] ([fdo#109271] / [i915#4613]) [3]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116172v1/shard-apl7/igt@gem_lmem_swapping@heavy-verify-multi.html - shard-glk: NOTRUN -> [SKIP][4] ([fdo#109271] / [i915#4613]) [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116172v1/shard-glk7/igt@gem_lmem_swapping@heavy-verify-multi.html * igt@kms_ccs@pipe-c-crc-primary-basic-y_tiled_gen12_mc_ccs: - shard-apl: NOTRUN -> [SKIP][5] ([fdo#109271] / [i915#3886]) +1 similar issue [5]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116172v1/shard-apl6/igt@kms_ccs@pipe-c-crc-primary-basic-y_tiled_gen12_mc_ccs.html * igt@kms_cursor_crc@cursor-sliding-512x170: - shard-glk: NOTRUN -> [SKIP][6] ([fdo#109271]) +20 similar issues [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116172v1/shard-glk3/igt@kms_cursor_crc@cursor-sliding-512x170.html * igt@kms_fbcon_fbt@fbc-suspend: - shard-apl: [PASS][7] -> [FAIL][8] ([i915#4767]) [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12976/shard-apl1/igt@kms_fbcon_fbt@fbc-suspend.html [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116172v1/shard-apl6/igt@kms_fbcon_fbt@fbc-suspend.html * igt@kms_plane_scaling@planes-upscale-20x20-downscale-factor-0-75@pipe-c-dp-1: - shard-apl: NOTRUN -> [SKIP][9] ([fdo#109271]) +40 similar issues [9]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116172v1/shard-apl6/igt@kms_plane_scaling@planes-upscale-20x20-downscale-factor-0-75@pipe-c-dp-1.html * igt@kms_psr2_sf@overlay-plane-update-sf-dmg-area: - shard-glk: NOTRUN -> [SKIP][10] ([fdo#109271] / [i915#658]) [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116172v1/shard-glk7/igt@kms_psr2_sf@overlay-plane-update-sf-dmg-area.html * igt@kms_psr2_su@page_flip-p010: - shard-apl: NOTRUN -> [SKIP][11] ([fdo#109271] / [i915#658]) +1 similar issue [11]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116172v1/shard-apl6/igt@kms_psr2_su@page_flip-p010.html #### Possible fixes #### * igt@drm_fdinfo@most-busy-idle-check-all@rcs0: - {shard-rkl}: [FAIL][12] ([i915#7742]) -> [PASS][13] [12]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12976/shard-rkl-3/igt@drm_fdinfo@most-busy-idle-check-all@rcs0.html [13]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116172v1/shard-rkl-4/igt@drm_fdinfo@most-busy-idle-check-all@rcs0.html * igt@gem_ctx_exec@basic-nohangcheck: - {shard-tglu}: [FAIL][14] ([i915#6268]) -> [PASS][15] [14]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12976/shard-tglu-6/igt@gem_ctx_exec@basic-nohangcheck.html [15]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116172v1/shard-tglu-8/igt@gem_ctx_exec@basic-nohangcheck.html * igt@gem_exec_fair@basic-pace-share@rcs0: - {shard-rkl}: [FAIL][16] ([i915#2842]) -> [PASS][17] [16]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12976/shard-rkl-3/igt@gem_exec_fair@basic-pace-share@rcs0.html [17]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116172v1/shard-rkl-4/igt@gem_exec_fair@basic-pace-share@rcs0.html - shard-apl: [FAIL][18] ([i915#2842]) -> [PASS][19] [18]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12976/shard-apl7/igt@gem_exec_fair@basic-pace-share@rcs0.html [19]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116172v1/shard-apl2/igt@gem_exec_fair@basic-pace-share@rcs0.html - {shard-tglu}: [FAIL][20] ([i915#2842]) -> [PASS][21] [20]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12976/shard-tglu-6/igt@gem_exec_fair@basic-pace-share@rcs0.html [21]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116172v1/shard-tglu-10/igt@gem_exec_fair@basic-pace-share@rcs0.html * igt@gen9_exec_parse@allowed-single: - shard-apl: [ABORT][22] ([i915#5566]) -> [PASS][23] [22]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12976/shard-apl6/igt@gen9_exec_parse@allowed-single.html [23]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116172v1/shard-apl7/igt@gen9_exec_parse@allowed-single.html - shard-glk: [ABORT][24] ([i915#5566]) -> [PASS][25] [24]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12976/shard-glk7/igt@gen9_exec_parse@allowed-single.html [25]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116172v1/shard-glk7/igt@gen9_exec_parse@allowed-single.html * igt@i915_pm_dc@dc9-dpms: - {shard-tglu}: [SKIP][26] ([i915#4281]) -> [PASS][27] [26]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12976/shard-tglu-7/igt@i915_pm_dc@dc9-dpms.html [27]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116172v1/shard-tglu-2/igt@i915_pm_dc@dc9-dpms.html * igt@i915_pm_rpm@dpms-mode-unset-non-lpsp: - {shard-rkl}: [SKIP][28] ([i915#1397]) -> [PASS][29] +1 similar issue [28]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12976/shard-rkl-7/igt@i915_pm_rpm@dpms-mode-unset-non-lpsp.html [29]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116172v1/shard-rkl-3/igt@i915_pm_rpm@dpms-mode-unset-non-lpsp.html * igt@i915_pm_rpm@modeset-lpsp-stress-no-wait: - {shard-dg1}: [SKIP][30] ([i915#1397]) -> [PASS][31] [30]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12976/shard-dg1-17/igt@i915_pm_rpm@modeset-lpsp-stress-no-wait.html [31]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116172v1/shard-dg1-14/igt@i915_pm_rpm@modeset-lpsp-stress-no-wait.html * igt@kms_cursor_legacy@flip-vs-cursor-atomic-transitions-varying-size: - shard-apl: [FAIL][32] ([i915#2346]) -> [PASS][33] [32]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12976/shard-apl6/igt@kms_cursor_legacy@flip-vs-cursor-atomic-transitions-varying-size.html [33]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116172v1/shard-apl7/igt@kms_cursor_legacy@flip-vs-cursor-atomic-transitions-varying-size.html * igt@kms_cursor_legacy@single-bo@pipe-b: - {shard-rkl}: [INCOMPLETE][34] ([i915#8011]) -> [PASS][35] [34]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12976/shard-rkl-7/igt@kms_cursor_legacy@single-bo@pipe-b.html [35]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116172v1/shard-rkl-4/igt@kms_cursor_legacy@single-bo@pipe-b.html * igt@kms_flip@2x-plain-flip-ts-check-interruptible@ab-hdmi-a1-hdmi-a2: - shard-glk: [FAIL][36] ([i915#2122]) -> [PASS][37] [36]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12976/shard-glk7/igt@kms_flip@2x-plain-flip-ts-check-interruptible@ab-hdmi-a1-hdmi-a2.html [37]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116172v1/shard-glk7/igt@kms_flip@2x-plain-flip-ts-check-interruptible@ab-hdmi-a1-hdmi-a2.html * igt@kms_flip@flip-vs-suspend-interruptible@b-dp1: - shard-apl: [DMESG-WARN][38] -> [PASS][39] [38]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12976/shard-apl7/igt@kms_flip@flip-vs-suspend-interruptible@b-dp1.html [39]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116172v1/shard-apl3/igt@kms_flip@flip-vs-suspend-interruptible@b-dp1.html * igt@kms_flip@flip-vs-suspend@a-dp1: - shard-apl: [ABORT][40] ([i915#180]) -> [PASS][41] [40]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12976/shard-apl1/igt@kms_flip@flip-vs-suspend@a-dp1.html [41]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116172v1/shard-apl6/igt@kms_flip@flip-vs-suspend@a-dp1.html {name}: This element is suppressed. This means it is ignored when computing the status of the difference (SUCCESS, WARNING, or FAILURE). [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271 [fdo#109285]: https://bugs.freedesktop.org/show_bug.cgi?id=109285 [fdo#109289]: https://bugs.freedesktop.org/show_bug.cgi?id=109289 [fdo#109307]: https://bugs.freedesktop.org/show_bug.cgi?id=109307 [fdo#109506]: https://bugs.freedesktop.org/show_bug.cgi?id=109506 [fdo#111068]: https://bugs.freedesktop.org/show_bug.cgi?id=111068 [fdo#111825]: https://bugs.freedesktop.org/show_bug.cgi?id=111825 [fdo#111827]: https://bugs.freedesktop.org/show_bug.cgi?id=111827 [i915#1072]: https://gitlab.freedesktop.org/drm/intel/issues/1072 [i915#1397]: https://gitlab.freedesktop.org/drm/intel/issues/1397 [i915#180]: https://gitlab.freedesktop.org/drm/intel/issues/180 [i915#1839]: https://gitlab.freedesktop.org/drm/intel/issues/1839 [i915#1902]: https://gitlab.freedesktop.org/drm/intel/issues/1902 [i915#2122]: https://gitlab.freedesktop.org/drm/intel/issues/2122 [i915#2346]: https://gitlab.freedesktop.org/drm/intel/issues/2346 [i915#2437]: https://gitlab.freedesktop.org/drm/intel/issues/2437 [i915#2527]: https://gitlab.freedesktop.org/drm/intel/issues/2527 [i915#2575]: https://gitlab.freedesktop.org/drm/intel/issues/2575 [i915#2587]: https://gitlab.freedesktop.org/drm/intel/issues/2587 [i915#2672]: https://gitlab.freedesktop.org/drm/intel/issues/2672 [i915#280]: https://gitlab.freedesktop.org/drm/intel/issues/280 [i915#2842]: https://gitlab.freedesktop.org/drm/intel/issues/2842 [i915#315]: https://gitlab.freedesktop.org/drm/intel/issues/315 [i915#3281]: https://gitlab.freedesktop.org/drm/intel/issues/3281 [i915#3282]: https://gitlab.freedesktop.org/drm/intel/issues/3282 [i915#3297]: https://gitlab.freedesktop.org/drm/intel/issues/3297 [i915#3359]: https://gitlab.freedesktop.org/drm/intel/issues/3359 [i915#3458]: https://gitlab.freedesktop.org/drm/intel/issues/3458 [i915#3469]: https://gitlab.freedesktop.org/drm/intel/issues/3469 [i915#3539]: https://gitlab.freedesktop.org/drm/intel/issues/3539 [i915#3555]: https://gitlab.freedesktop.org/drm/intel/issues/3555 [i915#3638]: https://gitlab.freedesktop.org/drm/intel/issues/3638 [i915#3689]: https://gitlab.freedesktop.org/drm/intel/issues/3689 [i915#3743]: https://gitlab.freedesktop.org/drm/intel/issues/3743 [i915#3886]: https://gitlab.freedesktop.org/drm/intel/issues/3886 [i915#3952]: https://gitlab.freedesktop.org/drm/intel/issues/3952 [i915#3989]: https://gitlab.freedesktop.org/drm/intel/issues/3989 [i915#4070]: https://gitlab.freedesktop.org/drm/intel/issues/4070 [i915#4077]: https://gitlab.freedesktop.org/drm/intel/issues/4077 [i915#4078]: https://gitlab.freedesktop.org/drm/intel/issues/4078 [i915#4079]: https://gitlab.freedesktop.org/drm/intel/issues/4079 [i915#4083]: https://gitlab.freedesktop.org/drm/intel/issues/4083 [i915#4098]: https://gitlab.freedesktop.org/drm/intel/issues/4098 [i915#4103]: https://gitlab.freedesktop.org/drm/intel/issues/4103 [i915#4212]: https://gitlab.freedesktop.org/drm/intel/issues/4212 [i915#4213]: https://gitlab.freedesktop.org/drm/intel/issues/4213 [i915#4215]: https://gitlab.freedesktop.org/drm/intel/issues/4215 [i915#4270]: https://gitlab.freedesktop.org/drm/intel/issues/4270 [i915#4281]: https://gitlab.freedesktop.org/drm/intel/issues/4281 [i915#4349]: https://gitlab.freedesktop.org/drm/intel/issues/4349 [i915#4538]: https://gitlab.freedesktop.org/drm/intel/issues/4538 [i915#454]: https://gitlab.freedesktop.org/drm/intel/issues/454 [i915#4565]: https://gitlab.freedesktop.org/drm/intel/issues/4565 [i915#4579]: https://gitlab.freedesktop.org/drm/intel/issues/4579 [i915#4613]: https://gitlab.freedesktop.org/drm/intel/issues/4613 [i915#4767]: https://gitlab.freedesktop.org/drm/intel/issues/4767 [i915#4812]: https://gitlab.freedesktop.org/drm/intel/issues/4812 [i915#4816]: https://gitlab.freedesktop.org/drm/intel/issues/4816 [i915#4818]: https://gitlab.freedesktop.org/drm/intel/issues/4818 [i915#4833]: https://gitlab.freedesktop.org/drm/intel/issues/4833 [i915#4852]: https://gitlab.freedesktop.org/drm/intel/issues/4852 [i915#4854]: https://gitlab.freedesktop.org/drm/intel/issues/4854 [i915#4859]: https://gitlab.freedesktop.org/drm/intel/issues/4859 [i915#4860]: https://gitlab.freedesktop.org/drm/intel/issues/4860 [i915#4880]: https://gitlab.freedesktop.org/drm/intel/issues/4880 [i915#5176]: https://gitlab.freedesktop.org/drm/intel/issues/5176 [i915#5235]: https://gitlab.freedesktop.org/drm/intel/issues/5235 [i915#5286]: https://gitlab.freedesktop.org/drm/intel/issues/5286 [i915#5563]: https://gitlab.freedesktop.org/drm/intel/issues/5563 [i915#5566]: https://gitlab.freedesktop.org/drm/intel/issues/5566 [i915#6095]: https://gitlab.freedesktop.org/drm/intel/issues/6095 [i915#6227]: https://gitlab.freedesktop.org/drm/intel/issues/6227 [i915#6268]: https://gitlab.freedesktop.org/drm/intel/issues/6268 [i915#6301]: https://gitlab.freedesktop.org/drm/intel/issues/6301 [i915#6524]: https://gitlab.freedesktop.org/drm/intel/issues/6524 [i915#658]: https://gitlab.freedesktop.org/drm/intel/issues/658 [i915#7116]: https://gitlab.freedesktop.org/drm/intel/issues/7116 [i915#7561]: https://gitlab.freedesktop.org/drm/intel/issues/7561 [i915#7697]: https://gitlab.freedesktop.org/drm/intel/issues/7697 [i915#7701]: https://gitlab.freedesktop.org/drm/intel/issues/7701 [i915#7711]: https://gitlab.freedesktop.org/drm/intel/issues/7711 [i915#7742]: https://gitlab.freedesktop.org/drm/intel/issues/7742 [i915#7828]: https://gitlab.freedesktop.org/drm/intel/issues/7828 [i915#8011]: https://gitlab.freedesktop.org/drm/intel/issues/8011 [i915#8253]: https://gitlab.freedesktop.org/drm/intel/issues/8253 [i915#8292]: https://gitlab.freedesktop.org/drm/intel/issues/8292 [i915#8308]: https://gitlab.freedesktop.org/drm/intel/issues/8308 Build changes ------------- * Linux: CI_DRM_12976 -> Patchwork_116172v1 CI-20190529: 20190529 CI_DRM_12976: 517e3daa2f8c38df4969591325325de8315685b9 @ git://anongit.freedesktop.org/gfx-ci/linux IGT_7240: ef4550e3b7d3c11ba257006bc7d4f8e421667d46 @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git Patchwork_116172v1: 517e3daa2f8c38df4969591325325de8315685b9 @ git://anongit.freedesktop.org/gfx-ci/linux piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit == Logs == For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116172v1/index.html [-- Attachment #2: Type: text/html, Size: 12335 bytes --] ^ permalink raw reply [flat|nested] 31+ messages in thread
* [Intel-gfx] [PATCH 0/3] drm/i915/guc: Disable PL1 power limit when loading GuC firmware @ 2023-04-10 22:35 Ashutosh Dixit 2023-04-10 22:35 ` [Intel-gfx] [PATCH 3/3] drm/i915/hwmon: Block waiting for GuC reset to complete Ashutosh Dixit 0 siblings, 1 reply; 31+ messages in thread From: Ashutosh Dixit @ 2023-04-10 22:35 UTC (permalink / raw) To: intel-gfx; +Cc: dri-devel, Rodrigo Vivi Updates to Patch 2/3 and Patch 3/3 in this version. Ashutosh Dixit (3): drm/i915/hwmon: Get mutex and rpm ref just once in hwm_power_max_write drm/i915/guc: Disable PL1 power limit when loading GuC firmware drm/i915/hwmon: Block waiting for GuC reset to complete drivers/gpu/drm/i915/gt/uc/intel_uc.c | 13 +++- drivers/gpu/drm/i915/i915_hwmon.c | 94 +++++++++++++++++++++++---- drivers/gpu/drm/i915/i915_hwmon.h | 7 ++ 3 files changed, 100 insertions(+), 14 deletions(-) -- 2.38.0 ^ permalink raw reply [flat|nested] 31+ messages in thread
* [Intel-gfx] [PATCH 3/3] drm/i915/hwmon: Block waiting for GuC reset to complete 2023-04-10 22:35 [Intel-gfx] [PATCH 0/3] " Ashutosh Dixit @ 2023-04-10 22:35 ` Ashutosh Dixit 2023-04-18 5:35 ` Rodrigo Vivi 2023-04-19 13:21 ` Tvrtko Ursulin 0 siblings, 2 replies; 31+ messages in thread From: Ashutosh Dixit @ 2023-04-10 22:35 UTC (permalink / raw) To: intel-gfx; +Cc: dri-devel, Rodrigo Vivi Instead of erroring out when GuC reset is in progress, block waiting for GuC reset to complete which is a more reasonable uapi behavior. v2: Avoid race between wake_up_all and waiting for wakeup (Rodrigo) Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com> --- drivers/gpu/drm/i915/i915_hwmon.c | 38 +++++++++++++++++++++++++++---- 1 file changed, 33 insertions(+), 5 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_hwmon.c b/drivers/gpu/drm/i915/i915_hwmon.c index 9ab8971679fe3..8471a667dfc71 100644 --- a/drivers/gpu/drm/i915/i915_hwmon.c +++ b/drivers/gpu/drm/i915/i915_hwmon.c @@ -51,6 +51,7 @@ struct hwm_drvdata { char name[12]; int gt_n; bool reset_in_progress; + wait_queue_head_t waitq; }; struct i915_hwmon { @@ -395,16 +396,41 @@ hwm_power_max_read(struct hwm_drvdata *ddat, long *val) static int hwm_power_max_write(struct hwm_drvdata *ddat, long val) { +#define GUC_RESET_TIMEOUT msecs_to_jiffies(2000) + + int ret = 0, timeout = GUC_RESET_TIMEOUT; struct i915_hwmon *hwmon = ddat->hwmon; intel_wakeref_t wakeref; - int ret = 0; + DEFINE_WAIT(wait); u32 nval; - mutex_lock(&hwmon->hwmon_lock); - if (hwmon->ddat.reset_in_progress) { - ret = -EAGAIN; - goto unlock; + /* Block waiting for GuC reset to complete when needed */ + for (;;) { + mutex_lock(&hwmon->hwmon_lock); + + prepare_to_wait(&ddat->waitq, &wait, TASK_INTERRUPTIBLE); + + if (!hwmon->ddat.reset_in_progress) + break; + + if (signal_pending(current)) { + ret = -EINTR; + break; + } + + if (!timeout) { + ret = -ETIME; + break; + } + + mutex_unlock(&hwmon->hwmon_lock); + + timeout = schedule_timeout(timeout); } + finish_wait(&ddat->waitq, &wait); + if (ret) + goto unlock; + wakeref = intel_runtime_pm_get(ddat->uncore->rpm); /* Disable PL1 limit and verify, because the limit cannot be disabled on all platforms */ @@ -508,6 +534,7 @@ void i915_hwmon_power_max_restore(struct drm_i915_private *i915, bool old) intel_uncore_rmw(hwmon->ddat.uncore, hwmon->rg.pkg_rapl_limit, PKG_PWR_LIM_1_EN, old ? PKG_PWR_LIM_1_EN : 0); hwmon->ddat.reset_in_progress = false; + wake_up_all(&hwmon->ddat.waitq); mutex_unlock(&hwmon->hwmon_lock); } @@ -784,6 +811,7 @@ void i915_hwmon_register(struct drm_i915_private *i915) ddat->uncore = &i915->uncore; snprintf(ddat->name, sizeof(ddat->name), "i915"); ddat->gt_n = -1; + init_waitqueue_head(&ddat->waitq); for_each_gt(gt, i915, i) { ddat_gt = hwmon->ddat_gt + i; -- 2.38.0 ^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [Intel-gfx] [PATCH 3/3] drm/i915/hwmon: Block waiting for GuC reset to complete 2023-04-10 22:35 ` [Intel-gfx] [PATCH 3/3] drm/i915/hwmon: Block waiting for GuC reset to complete Ashutosh Dixit @ 2023-04-18 5:35 ` Rodrigo Vivi 2023-04-18 17:23 ` Dixit, Ashutosh 2023-04-19 13:21 ` Tvrtko Ursulin 1 sibling, 1 reply; 31+ messages in thread From: Rodrigo Vivi @ 2023-04-18 5:35 UTC (permalink / raw) To: Ashutosh Dixit; +Cc: intel-gfx, dri-devel On Mon, Apr 10, 2023 at 03:35:09PM -0700, Ashutosh Dixit wrote: > Instead of erroring out when GuC reset is in progress, block waiting for > GuC reset to complete which is a more reasonable uapi behavior. > > v2: Avoid race between wake_up_all and waiting for wakeup (Rodrigo) > > Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com> > --- > drivers/gpu/drm/i915/i915_hwmon.c | 38 +++++++++++++++++++++++++++---- > 1 file changed, 33 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_hwmon.c b/drivers/gpu/drm/i915/i915_hwmon.c > index 9ab8971679fe3..8471a667dfc71 100644 > --- a/drivers/gpu/drm/i915/i915_hwmon.c > +++ b/drivers/gpu/drm/i915/i915_hwmon.c > @@ -51,6 +51,7 @@ struct hwm_drvdata { > char name[12]; > int gt_n; > bool reset_in_progress; > + wait_queue_head_t waitq; > }; > > struct i915_hwmon { > @@ -395,16 +396,41 @@ hwm_power_max_read(struct hwm_drvdata *ddat, long *val) > static int > hwm_power_max_write(struct hwm_drvdata *ddat, long val) > { > +#define GUC_RESET_TIMEOUT msecs_to_jiffies(2000) > + > + int ret = 0, timeout = GUC_RESET_TIMEOUT; > struct i915_hwmon *hwmon = ddat->hwmon; > intel_wakeref_t wakeref; > - int ret = 0; > + DEFINE_WAIT(wait); > u32 nval; > > - mutex_lock(&hwmon->hwmon_lock); > - if (hwmon->ddat.reset_in_progress) { > - ret = -EAGAIN; > - goto unlock; > + /* Block waiting for GuC reset to complete when needed */ > + for (;;) { > + mutex_lock(&hwmon->hwmon_lock); I'm really afraid of how this mutex is handled with the wait queue. some initial thought it looks like it is trying to reimplement ww_mutex? all other examples of the wait_queue usages like this or didn't use locks or had it in a total different flow that I could not correlate. > + > + prepare_to_wait(&ddat->waitq, &wait, TASK_INTERRUPTIBLE); > + > + if (!hwmon->ddat.reset_in_progress) > + break; If this breaks we never unlock it? > + > + if (signal_pending(current)) { > + ret = -EINTR; > + break; > + } > + > + if (!timeout) { > + ret = -ETIME; > + break; > + } > + > + mutex_unlock(&hwmon->hwmon_lock); do we need to lock the signal pending and timeout as well? or only wrapping it around the hwmon->ddat access would be enough? > + > + timeout = schedule_timeout(timeout); > } > + finish_wait(&ddat->waitq, &wait); > + if (ret) > + goto unlock; > + > wakeref = intel_runtime_pm_get(ddat->uncore->rpm); > > /* Disable PL1 limit and verify, because the limit cannot be disabled on all platforms */ > @@ -508,6 +534,7 @@ void i915_hwmon_power_max_restore(struct drm_i915_private *i915, bool old) > intel_uncore_rmw(hwmon->ddat.uncore, hwmon->rg.pkg_rapl_limit, > PKG_PWR_LIM_1_EN, old ? PKG_PWR_LIM_1_EN : 0); > hwmon->ddat.reset_in_progress = false; > + wake_up_all(&hwmon->ddat.waitq); > > mutex_unlock(&hwmon->hwmon_lock); > } > @@ -784,6 +811,7 @@ void i915_hwmon_register(struct drm_i915_private *i915) > ddat->uncore = &i915->uncore; > snprintf(ddat->name, sizeof(ddat->name), "i915"); > ddat->gt_n = -1; > + init_waitqueue_head(&ddat->waitq); > > for_each_gt(gt, i915, i) { > ddat_gt = hwmon->ddat_gt + i; > -- > 2.38.0 > ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [Intel-gfx] [PATCH 3/3] drm/i915/hwmon: Block waiting for GuC reset to complete 2023-04-18 5:35 ` Rodrigo Vivi @ 2023-04-18 17:23 ` Dixit, Ashutosh 2023-04-19 19:40 ` Rodrigo Vivi 0 siblings, 1 reply; 31+ messages in thread From: Dixit, Ashutosh @ 2023-04-18 17:23 UTC (permalink / raw) To: Rodrigo Vivi; +Cc: intel-gfx, dri-devel On Mon, 17 Apr 2023 22:35:58 -0700, Rodrigo Vivi wrote: > Hi Rodrigo, > On Mon, Apr 10, 2023 at 03:35:09PM -0700, Ashutosh Dixit wrote: > > Instead of erroring out when GuC reset is in progress, block waiting for > > GuC reset to complete which is a more reasonable uapi behavior. > > > > v2: Avoid race between wake_up_all and waiting for wakeup (Rodrigo) > > > > Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com> > > --- > > drivers/gpu/drm/i915/i915_hwmon.c | 38 +++++++++++++++++++++++++++---- > > 1 file changed, 33 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_hwmon.c b/drivers/gpu/drm/i915/i915_hwmon.c > > index 9ab8971679fe3..8471a667dfc71 100644 > > --- a/drivers/gpu/drm/i915/i915_hwmon.c > > +++ b/drivers/gpu/drm/i915/i915_hwmon.c > > @@ -51,6 +51,7 @@ struct hwm_drvdata { > > char name[12]; > > int gt_n; > > bool reset_in_progress; > > + wait_queue_head_t waitq; > > }; > > > > struct i915_hwmon { > > @@ -395,16 +396,41 @@ hwm_power_max_read(struct hwm_drvdata *ddat, long *val) > > static int > > hwm_power_max_write(struct hwm_drvdata *ddat, long val) > > { > > +#define GUC_RESET_TIMEOUT msecs_to_jiffies(2000) > > + > > + int ret = 0, timeout = GUC_RESET_TIMEOUT; > > struct i915_hwmon *hwmon = ddat->hwmon; > > intel_wakeref_t wakeref; > > - int ret = 0; > > + DEFINE_WAIT(wait); > > u32 nval; > > > > - mutex_lock(&hwmon->hwmon_lock); > > - if (hwmon->ddat.reset_in_progress) { > > - ret = -EAGAIN; > > - goto unlock; > > + /* Block waiting for GuC reset to complete when needed */ > > + for (;;) { > > + mutex_lock(&hwmon->hwmon_lock); > > I'm really afraid of how this mutex is handled with the wait queue. > some initial thought it looks like it is trying to reimplement ww_mutex? Sorry, but I am missing the relation with ww_mutex. No such relation is intended. > all other examples of the wait_queue usages like this or didn't use > locks or had it in a total different flow that I could not correlate. Actually there are several examples of prepare_to_wait/finish_wait sequences with both spinlock and mutex in the kernel. See e.g. rpm_suspend(), wait_for_rtrs_disconnection(), softsynthx_read(). Also, as I mentioned, except for the lock, the sequence here is identical to intel_guc_wait_for_pending_msg(). > > > + > > + prepare_to_wait(&ddat->waitq, &wait, TASK_INTERRUPTIBLE); > > + > > + if (!hwmon->ddat.reset_in_progress) > > + break; > > If this breaks we never unlock it? Correct, this is the original case in Patch 2 where the mutex is acquired in the beginning of the function and released just before the final exit from the function (so the mutex is held for the entire duration of the function). > > > + > > + if (signal_pending(current)) { > > + ret = -EINTR; > > + break; > > + } > > + > > + if (!timeout) { > > + ret = -ETIME; > > + break; > > + } > > + > > + mutex_unlock(&hwmon->hwmon_lock); > > do we need to lock the signal pending and timeout as well? > or only wrapping it around the hwmon->ddat access would be > enough? Strictly, the mutex is only needed for the hwmon->ddat.reset_in_progress flag. But because this is not a performance path, implementing it as done in the patch simplifies the code flow (since there are several if/else, goto's, mutex lock/unlock and prepare_to_wait/finish_wait to consider). So if possible I *really* want to not try to over-optimize here (I did try a few other things when writing the patch but it was getting ugly). The only real requirement is to drop the lock before calling schedule_timeout() below (and we are reacquiring the lock as soon as we are scheduled back in, as you can see in the loop above). > > > + > > + timeout = schedule_timeout(timeout); > > } > > + finish_wait(&ddat->waitq, &wait); > > + if (ret) > > + goto unlock; > > + > > wakeref = intel_runtime_pm_get(ddat->uncore->rpm); > > > > /* Disable PL1 limit and verify, because the limit cannot be disabled on all platforms */ > > @@ -508,6 +534,7 @@ void i915_hwmon_power_max_restore(struct drm_i915_private *i915, bool old) > > intel_uncore_rmw(hwmon->ddat.uncore, hwmon->rg.pkg_rapl_limit, > > PKG_PWR_LIM_1_EN, old ? PKG_PWR_LIM_1_EN : 0); > > hwmon->ddat.reset_in_progress = false; > > + wake_up_all(&hwmon->ddat.waitq); > > > > mutex_unlock(&hwmon->hwmon_lock); > > } > > @@ -784,6 +811,7 @@ void i915_hwmon_register(struct drm_i915_private *i915) > > ddat->uncore = &i915->uncore; > > snprintf(ddat->name, sizeof(ddat->name), "i915"); > > ddat->gt_n = -1; > > + init_waitqueue_head(&ddat->waitq); > > > > for_each_gt(gt, i915, i) { > > ddat_gt = hwmon->ddat_gt + i; > > -- > > 2.38.0 > > From what I understand is the locking above is fine and is not the point. The real race is between schedule_timeout() (which suspends the thread) and wake_up_all() (which schedules it back in). But this prepare_to_wait/finish_wait pattern is so widespread that the kernel guarantees that this works correctly as long as you do things in the correct order (otherwise we'd see a lot more kernel hangs/deadlocks). Thanks, Ashutosh ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [Intel-gfx] [PATCH 3/3] drm/i915/hwmon: Block waiting for GuC reset to complete 2023-04-18 17:23 ` Dixit, Ashutosh @ 2023-04-19 19:40 ` Rodrigo Vivi 2023-04-19 22:13 ` Dixit, Ashutosh 0 siblings, 1 reply; 31+ messages in thread From: Rodrigo Vivi @ 2023-04-19 19:40 UTC (permalink / raw) To: Dixit, Ashutosh; +Cc: intel-gfx, dri-devel On Tue, Apr 18, 2023 at 10:23:50AM -0700, Dixit, Ashutosh wrote: > On Mon, 17 Apr 2023 22:35:58 -0700, Rodrigo Vivi wrote: > > > > Hi Rodrigo, > > > On Mon, Apr 10, 2023 at 03:35:09PM -0700, Ashutosh Dixit wrote: > > > Instead of erroring out when GuC reset is in progress, block waiting for > > > GuC reset to complete which is a more reasonable uapi behavior. > > > > > > v2: Avoid race between wake_up_all and waiting for wakeup (Rodrigo) > > > > > > Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com> > > > --- > > > drivers/gpu/drm/i915/i915_hwmon.c | 38 +++++++++++++++++++++++++++---- > > > 1 file changed, 33 insertions(+), 5 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/i915_hwmon.c b/drivers/gpu/drm/i915/i915_hwmon.c > > > index 9ab8971679fe3..8471a667dfc71 100644 > > > --- a/drivers/gpu/drm/i915/i915_hwmon.c > > > +++ b/drivers/gpu/drm/i915/i915_hwmon.c > > > @@ -51,6 +51,7 @@ struct hwm_drvdata { > > > char name[12]; > > > int gt_n; > > > bool reset_in_progress; > > > + wait_queue_head_t waitq; > > > }; > > > > > > struct i915_hwmon { > > > @@ -395,16 +396,41 @@ hwm_power_max_read(struct hwm_drvdata *ddat, long *val) > > > static int > > > hwm_power_max_write(struct hwm_drvdata *ddat, long val) > > > { > > > +#define GUC_RESET_TIMEOUT msecs_to_jiffies(2000) > > > + > > > + int ret = 0, timeout = GUC_RESET_TIMEOUT; > > > struct i915_hwmon *hwmon = ddat->hwmon; > > > intel_wakeref_t wakeref; > > > - int ret = 0; > > > + DEFINE_WAIT(wait); > > > u32 nval; > > > > > > - mutex_lock(&hwmon->hwmon_lock); > > > - if (hwmon->ddat.reset_in_progress) { > > > - ret = -EAGAIN; > > > - goto unlock; > > > + /* Block waiting for GuC reset to complete when needed */ > > > + for (;;) { > > > + mutex_lock(&hwmon->hwmon_lock); > > > > I'm really afraid of how this mutex is handled with the wait queue. > > some initial thought it looks like it is trying to reimplement ww_mutex? > > Sorry, but I am missing the relation with ww_mutex. No such relation is > intended. > > > all other examples of the wait_queue usages like this or didn't use > > locks or had it in a total different flow that I could not correlate. > > Actually there are several examples of prepare_to_wait/finish_wait > sequences with both spinlock and mutex in the kernel. See > e.g. rpm_suspend(), wait_for_rtrs_disconnection(), softsynthx_read(). > > Also, as I mentioned, except for the lock, the sequence here is identical > to intel_guc_wait_for_pending_msg(). > > > > > > + > > > + prepare_to_wait(&ddat->waitq, &wait, TASK_INTERRUPTIBLE); > > > + > > > + if (!hwmon->ddat.reset_in_progress) > > > + break; > > > > If this breaks we never unlock it? > > Correct, this is the original case in Patch 2 where the mutex is acquired > in the beginning of the function and released just before the final exit > from the function (so the mutex is held for the entire duration of the > function). I got really confused here... I looked at the patch 2 again and I don't see any place where the lock remains outside of the function. What was what I asked to remove on the initial versions. But now with this one I'm even more confused because I couldn't follow to understand who will remove the lock and when. > > > > > > + > > > + if (signal_pending(current)) { > > > + ret = -EINTR; > > > + break; > > > + } > > > + > > > + if (!timeout) { > > > + ret = -ETIME; > > > + break; > > > + } > > > + > > > + mutex_unlock(&hwmon->hwmon_lock); > > > > do we need to lock the signal pending and timeout as well? > > or only wrapping it around the hwmon->ddat access would be > > enough? > > Strictly, the mutex is only needed for the hwmon->ddat.reset_in_progress > flag. But because this is not a performance path, implementing it as done > in the patch simplifies the code flow (since there are several if/else, > goto's, mutex lock/unlock and prepare_to_wait/finish_wait to consider). > > So if possible I *really* want to not try to over-optimize here (I did try > a few other things when writing the patch but it was getting ugly). The > only real requirement is to drop the lock before calling schedule_timeout() > below (and we are reacquiring the lock as soon as we are scheduled back in, > as you can see in the loop above). > > > > > > + > > > + timeout = schedule_timeout(timeout); > > > } > > > + finish_wait(&ddat->waitq, &wait); > > > + if (ret) > > > + goto unlock; > > > + > > > wakeref = intel_runtime_pm_get(ddat->uncore->rpm); > > > > > > /* Disable PL1 limit and verify, because the limit cannot be disabled on all platforms */ > > > @@ -508,6 +534,7 @@ void i915_hwmon_power_max_restore(struct drm_i915_private *i915, bool old) > > > intel_uncore_rmw(hwmon->ddat.uncore, hwmon->rg.pkg_rapl_limit, > > > PKG_PWR_LIM_1_EN, old ? PKG_PWR_LIM_1_EN : 0); > > > hwmon->ddat.reset_in_progress = false; > > > + wake_up_all(&hwmon->ddat.waitq); > > > > > > mutex_unlock(&hwmon->hwmon_lock); > > > } > > > @@ -784,6 +811,7 @@ void i915_hwmon_register(struct drm_i915_private *i915) > > > ddat->uncore = &i915->uncore; > > > snprintf(ddat->name, sizeof(ddat->name), "i915"); > > > ddat->gt_n = -1; > > > + init_waitqueue_head(&ddat->waitq); > > > > > > for_each_gt(gt, i915, i) { > > > ddat_gt = hwmon->ddat_gt + i; > > > -- > > > 2.38.0 > > > > > From what I understand is the locking above is fine and is not the > point. The real race is between schedule_timeout() (which suspends the > thread) and wake_up_all() (which schedules it back in). But this > prepare_to_wait/finish_wait pattern is so widespread that the kernel > guarantees that this works correctly as long as you do things in the > correct order (otherwise we'd see a lot more kernel hangs/deadlocks). > > Thanks, > Ashutosh ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [Intel-gfx] [PATCH 3/3] drm/i915/hwmon: Block waiting for GuC reset to complete 2023-04-19 19:40 ` Rodrigo Vivi @ 2023-04-19 22:13 ` Dixit, Ashutosh 2023-04-20 15:45 ` Rodrigo Vivi 0 siblings, 1 reply; 31+ messages in thread From: Dixit, Ashutosh @ 2023-04-19 22:13 UTC (permalink / raw) To: Rodrigo Vivi; +Cc: intel-gfx, dri-devel On Wed, 19 Apr 2023 12:40:44 -0700, Rodrigo Vivi wrote: > Hi Rodrigo, > On Tue, Apr 18, 2023 at 10:23:50AM -0700, Dixit, Ashutosh wrote: > > On Mon, 17 Apr 2023 22:35:58 -0700, Rodrigo Vivi wrote: > > > > > > > Hi Rodrigo, > > > > > On Mon, Apr 10, 2023 at 03:35:09PM -0700, Ashutosh Dixit wrote: > > > > Instead of erroring out when GuC reset is in progress, block waiting for > > > > GuC reset to complete which is a more reasonable uapi behavior. > > > > > > > > v2: Avoid race between wake_up_all and waiting for wakeup (Rodrigo) > > > > > > > > Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com> > > > > --- > > > > drivers/gpu/drm/i915/i915_hwmon.c | 38 +++++++++++++++++++++++++++---- > > > > 1 file changed, 33 insertions(+), 5 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/i915_hwmon.c b/drivers/gpu/drm/i915/i915_hwmon.c > > > > index 9ab8971679fe3..8471a667dfc71 100644 > > > > --- a/drivers/gpu/drm/i915/i915_hwmon.c > > > > +++ b/drivers/gpu/drm/i915/i915_hwmon.c > > > > @@ -51,6 +51,7 @@ struct hwm_drvdata { > > > > char name[12]; > > > > int gt_n; > > > > bool reset_in_progress; > > > > + wait_queue_head_t waitq; > > > > }; > > > > > > > > struct i915_hwmon { > > > > @@ -395,16 +396,41 @@ hwm_power_max_read(struct hwm_drvdata *ddat, long *val) > > > > static int > > > > hwm_power_max_write(struct hwm_drvdata *ddat, long val) > > > > { > > > > +#define GUC_RESET_TIMEOUT msecs_to_jiffies(2000) > > > > + > > > > + int ret = 0, timeout = GUC_RESET_TIMEOUT; > > > > struct i915_hwmon *hwmon = ddat->hwmon; > > > > intel_wakeref_t wakeref; > > > > - int ret = 0; > > > > + DEFINE_WAIT(wait); > > > > u32 nval; > > > > > > > > - mutex_lock(&hwmon->hwmon_lock); > > > > - if (hwmon->ddat.reset_in_progress) { > > > > - ret = -EAGAIN; > > > > - goto unlock; > > > > + /* Block waiting for GuC reset to complete when needed */ > > > > + for (;;) { > > > > + mutex_lock(&hwmon->hwmon_lock); > > > > > > I'm really afraid of how this mutex is handled with the wait queue. > > > some initial thought it looks like it is trying to reimplement ww_mutex? > > > > Sorry, but I am missing the relation with ww_mutex. No such relation is > > intended. > > > > > all other examples of the wait_queue usages like this or didn't use > > > locks or had it in a total different flow that I could not correlate. > > > > Actually there are several examples of prepare_to_wait/finish_wait > > sequences with both spinlock and mutex in the kernel. See > > e.g. rpm_suspend(), wait_for_rtrs_disconnection(), softsynthx_read(). > > > > Also, as I mentioned, except for the lock, the sequence here is identical > > to intel_guc_wait_for_pending_msg(). > > > > > > > > > + > > > > + prepare_to_wait(&ddat->waitq, &wait, TASK_INTERRUPTIBLE); > > > > + > > > > + if (!hwmon->ddat.reset_in_progress) > > > > + break; > > > > > > If this breaks we never unlock it? > > > > Correct, this is the original case in Patch 2 where the mutex is acquired > > in the beginning of the function and released just before the final exit > > from the function (so the mutex is held for the entire duration of the > > function). > > I got really confused here... Sorry, the patch is a little confusing/tricky but I thought I'd better stick to the standard 'for (;;)' loop pattern otherwise it will also be hard to review. > I looked at the patch 2 again and I don't see any place where the lock > remains outside of the function. What was what I asked to remove on the > initial versions. So it was in Patch 1 where we changed the code to take the lock in the beginning of the function and release it at the end of the function (you can see it Patch 1). In Patch 2 the 'unlock' label and 'goto unlock' is introduced and the lock is released at the 'unlock' label (it is visible in Patch 2). > But now with this one I'm even more confused because I couldn't follow > to understand who will remove the lock and when. In Patch 3 again the lock is released at the the 'unlock' label (i.e. the destination of 'goto unlock', not visible in Patch 3). But we execute 'goto unlock' only when 'ret != 0' in the 'for (;;)' loop. But when 'ret == 0' (when 'ddat.reset_in_progress' flag is clear) we hold the mutex, execute the entire function and finally release the lock at the end of the function. Hopefully this helps. Thanks. -- Ashutosh > > > > > > > > > > + > > > > + if (signal_pending(current)) { > > > > + ret = -EINTR; > > > > + break; > > > > + } > > > > + > > > > + if (!timeout) { > > > > + ret = -ETIME; > > > > + break; > > > > + } > > > > + > > > > + mutex_unlock(&hwmon->hwmon_lock); > > > > > > do we need to lock the signal pending and timeout as well? > > > or only wrapping it around the hwmon->ddat access would be > > > enough? > > > > Strictly, the mutex is only needed for the hwmon->ddat.reset_in_progress > > flag. But because this is not a performance path, implementing it as done > > in the patch simplifies the code flow (since there are several if/else, > > goto's, mutex lock/unlock and prepare_to_wait/finish_wait to consider). > > > > So if possible I *really* want to not try to over-optimize here (I did try > > a few other things when writing the patch but it was getting ugly). The > > only real requirement is to drop the lock before calling schedule_timeout() > > below (and we are reacquiring the lock as soon as we are scheduled back in, > > as you can see in the loop above). > > > > > > > > > + > > > > + timeout = schedule_timeout(timeout); > > > > } > > > > + finish_wait(&ddat->waitq, &wait); > > > > + if (ret) > > > > + goto unlock; > > > > + > > > > wakeref = intel_runtime_pm_get(ddat->uncore->rpm); > > > > > > > > /* Disable PL1 limit and verify, because the limit cannot be disabled on all platforms */ > > > > @@ -508,6 +534,7 @@ void i915_hwmon_power_max_restore(struct drm_i915_private *i915, bool old) > > > > intel_uncore_rmw(hwmon->ddat.uncore, hwmon->rg.pkg_rapl_limit, > > > > PKG_PWR_LIM_1_EN, old ? PKG_PWR_LIM_1_EN : 0); > > > > hwmon->ddat.reset_in_progress = false; > > > > + wake_up_all(&hwmon->ddat.waitq); > > > > > > > > mutex_unlock(&hwmon->hwmon_lock); > > > > } > > > > @@ -784,6 +811,7 @@ void i915_hwmon_register(struct drm_i915_private *i915) > > > > ddat->uncore = &i915->uncore; > > > > snprintf(ddat->name, sizeof(ddat->name), "i915"); > > > > ddat->gt_n = -1; > > > > + init_waitqueue_head(&ddat->waitq); > > > > > > > > for_each_gt(gt, i915, i) { > > > > ddat_gt = hwmon->ddat_gt + i; > > > > -- > > > > 2.38.0 > > > > > > > > From what I understand is the locking above is fine and is not the > > point. The real race is between schedule_timeout() (which suspends the > > thread) and wake_up_all() (which schedules it back in). But this > > prepare_to_wait/finish_wait pattern is so widespread that the kernel > > guarantees that this works correctly as long as you do things in the > > correct order (otherwise we'd see a lot more kernel hangs/deadlocks). > > > > Thanks, > > Ashutosh ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [Intel-gfx] [PATCH 3/3] drm/i915/hwmon: Block waiting for GuC reset to complete 2023-04-19 22:13 ` Dixit, Ashutosh @ 2023-04-20 15:45 ` Rodrigo Vivi 0 siblings, 0 replies; 31+ messages in thread From: Rodrigo Vivi @ 2023-04-20 15:45 UTC (permalink / raw) To: Dixit, Ashutosh; +Cc: intel-gfx, dri-devel On Wed, Apr 19, 2023 at 03:13:08PM -0700, Dixit, Ashutosh wrote: > On Wed, 19 Apr 2023 12:40:44 -0700, Rodrigo Vivi wrote: > > > > Hi Rodrigo, > > > On Tue, Apr 18, 2023 at 10:23:50AM -0700, Dixit, Ashutosh wrote: > > > On Mon, 17 Apr 2023 22:35:58 -0700, Rodrigo Vivi wrote: > > > > > > > > > > Hi Rodrigo, > > > > > > > On Mon, Apr 10, 2023 at 03:35:09PM -0700, Ashutosh Dixit wrote: > > > > > Instead of erroring out when GuC reset is in progress, block waiting for > > > > > GuC reset to complete which is a more reasonable uapi behavior. > > > > > > > > > > v2: Avoid race between wake_up_all and waiting for wakeup (Rodrigo) > > > > > > > > > > Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com> > > > > > --- > > > > > drivers/gpu/drm/i915/i915_hwmon.c | 38 +++++++++++++++++++++++++++---- > > > > > 1 file changed, 33 insertions(+), 5 deletions(-) > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/i915_hwmon.c b/drivers/gpu/drm/i915/i915_hwmon.c > > > > > index 9ab8971679fe3..8471a667dfc71 100644 > > > > > --- a/drivers/gpu/drm/i915/i915_hwmon.c > > > > > +++ b/drivers/gpu/drm/i915/i915_hwmon.c > > > > > @@ -51,6 +51,7 @@ struct hwm_drvdata { > > > > > char name[12]; > > > > > int gt_n; > > > > > bool reset_in_progress; > > > > > + wait_queue_head_t waitq; > > > > > }; > > > > > > > > > > struct i915_hwmon { > > > > > @@ -395,16 +396,41 @@ hwm_power_max_read(struct hwm_drvdata *ddat, long *val) > > > > > static int > > > > > hwm_power_max_write(struct hwm_drvdata *ddat, long val) > > > > > { > > > > > +#define GUC_RESET_TIMEOUT msecs_to_jiffies(2000) > > > > > + > > > > > + int ret = 0, timeout = GUC_RESET_TIMEOUT; > > > > > struct i915_hwmon *hwmon = ddat->hwmon; > > > > > intel_wakeref_t wakeref; > > > > > - int ret = 0; > > > > > + DEFINE_WAIT(wait); > > > > > u32 nval; > > > > > > > > > > - mutex_lock(&hwmon->hwmon_lock); > > > > > - if (hwmon->ddat.reset_in_progress) { > > > > > - ret = -EAGAIN; > > > > > - goto unlock; > > > > > + /* Block waiting for GuC reset to complete when needed */ > > > > > + for (;;) { > > > > > + mutex_lock(&hwmon->hwmon_lock); > > > > > > > > I'm really afraid of how this mutex is handled with the wait queue. > > > > some initial thought it looks like it is trying to reimplement ww_mutex? > > > > > > Sorry, but I am missing the relation with ww_mutex. No such relation is > > > intended. > > > > > > > all other examples of the wait_queue usages like this or didn't use > > > > locks or had it in a total different flow that I could not correlate. > > > > > > Actually there are several examples of prepare_to_wait/finish_wait > > > sequences with both spinlock and mutex in the kernel. See > > > e.g. rpm_suspend(), wait_for_rtrs_disconnection(), softsynthx_read(). > > > > > > Also, as I mentioned, except for the lock, the sequence here is identical > > > to intel_guc_wait_for_pending_msg(). > > > > > > > > > > > > + > > > > > + prepare_to_wait(&ddat->waitq, &wait, TASK_INTERRUPTIBLE); > > > > > + > > > > > + if (!hwmon->ddat.reset_in_progress) > > > > > + break; > > > > > > > > If this breaks we never unlock it? > > > > > > Correct, this is the original case in Patch 2 where the mutex is acquired > > > in the beginning of the function and released just before the final exit > > > from the function (so the mutex is held for the entire duration of the > > > function). > > > > I got really confused here... > > Sorry, the patch is a little confusing/tricky but I thought I'd better > stick to the standard 'for (;;)' loop pattern otherwise it will also be > hard to review. > > > I looked at the patch 2 again and I don't see any place where the lock > > remains outside of the function. What was what I asked to remove on the > > initial versions. > > So it was in Patch 1 where we changed the code to take the lock in the > beginning of the function and release it at the end of the function (you > can see it Patch 1). > > In Patch 2 the 'unlock' label and 'goto unlock' is introduced and the lock > is released at the 'unlock' label (it is visible in Patch 2). > > > But now with this one I'm even more confused because I couldn't follow > > to understand who will remove the lock and when. > > In Patch 3 again the lock is released at the the 'unlock' label (i.e. the > destination of 'goto unlock', not visible in Patch 3). But we execute 'goto > unlock' only when 'ret != 0' in the 'for (;;)' loop. But when 'ret == 0' > (when 'ddat.reset_in_progress' flag is clear) we hold the mutex, execute > the entire function and finally release the lock at the end of the > function. > > Hopefully this helps. more coffee also helped! I'm sorry for the noise. with the timeout thing sorted out: Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > Thanks. > -- > Ashutosh > > > > > > > > > > > > > > > + > > > > > + if (signal_pending(current)) { > > > > > + ret = -EINTR; > > > > > + break; > > > > > + } > > > > > + > > > > > + if (!timeout) { > > > > > + ret = -ETIME; > > > > > + break; > > > > > + } > > > > > + > > > > > + mutex_unlock(&hwmon->hwmon_lock); > > > > > > > > do we need to lock the signal pending and timeout as well? > > > > or only wrapping it around the hwmon->ddat access would be > > > > enough? > > > > > > Strictly, the mutex is only needed for the hwmon->ddat.reset_in_progress > > > flag. But because this is not a performance path, implementing it as done > > > in the patch simplifies the code flow (since there are several if/else, > > > goto's, mutex lock/unlock and prepare_to_wait/finish_wait to consider). > > > > > > So if possible I *really* want to not try to over-optimize here (I did try > > > a few other things when writing the patch but it was getting ugly). The > > > only real requirement is to drop the lock before calling schedule_timeout() > > > below (and we are reacquiring the lock as soon as we are scheduled back in, > > > as you can see in the loop above). > > > > > > > > > > > > + > > > > > + timeout = schedule_timeout(timeout); > > > > > } > > > > > + finish_wait(&ddat->waitq, &wait); > > > > > + if (ret) > > > > > + goto unlock; > > > > > + > > > > > wakeref = intel_runtime_pm_get(ddat->uncore->rpm); > > > > > > > > > > /* Disable PL1 limit and verify, because the limit cannot be disabled on all platforms */ > > > > > @@ -508,6 +534,7 @@ void i915_hwmon_power_max_restore(struct drm_i915_private *i915, bool old) > > > > > intel_uncore_rmw(hwmon->ddat.uncore, hwmon->rg.pkg_rapl_limit, > > > > > PKG_PWR_LIM_1_EN, old ? PKG_PWR_LIM_1_EN : 0); > > > > > hwmon->ddat.reset_in_progress = false; > > > > > + wake_up_all(&hwmon->ddat.waitq); > > > > > > > > > > mutex_unlock(&hwmon->hwmon_lock); > > > > > } > > > > > @@ -784,6 +811,7 @@ void i915_hwmon_register(struct drm_i915_private *i915) > > > > > ddat->uncore = &i915->uncore; > > > > > snprintf(ddat->name, sizeof(ddat->name), "i915"); > > > > > ddat->gt_n = -1; > > > > > + init_waitqueue_head(&ddat->waitq); > > > > > > > > > > for_each_gt(gt, i915, i) { > > > > > ddat_gt = hwmon->ddat_gt + i; > > > > > -- > > > > > 2.38.0 > > > > > > > > > > > From what I understand is the locking above is fine and is not the > > > point. The real race is between schedule_timeout() (which suspends the > > > thread) and wake_up_all() (which schedules it back in). But this > > > prepare_to_wait/finish_wait pattern is so widespread that the kernel > > > guarantees that this works correctly as long as you do things in the > > > correct order (otherwise we'd see a lot more kernel hangs/deadlocks). > > > > > > Thanks, > > > Ashutosh ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [Intel-gfx] [PATCH 3/3] drm/i915/hwmon: Block waiting for GuC reset to complete 2023-04-10 22:35 ` [Intel-gfx] [PATCH 3/3] drm/i915/hwmon: Block waiting for GuC reset to complete Ashutosh Dixit 2023-04-18 5:35 ` Rodrigo Vivi @ 2023-04-19 13:21 ` Tvrtko Ursulin 2023-04-19 22:10 ` Dixit, Ashutosh 1 sibling, 1 reply; 31+ messages in thread From: Tvrtko Ursulin @ 2023-04-19 13:21 UTC (permalink / raw) To: Ashutosh Dixit, intel-gfx; +Cc: dri-devel, Rodrigo Vivi On 10/04/2023 23:35, Ashutosh Dixit wrote: > Instead of erroring out when GuC reset is in progress, block waiting for > GuC reset to complete which is a more reasonable uapi behavior. > > v2: Avoid race between wake_up_all and waiting for wakeup (Rodrigo) > > Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com> > --- > drivers/gpu/drm/i915/i915_hwmon.c | 38 +++++++++++++++++++++++++++---- > 1 file changed, 33 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_hwmon.c b/drivers/gpu/drm/i915/i915_hwmon.c > index 9ab8971679fe3..8471a667dfc71 100644 > --- a/drivers/gpu/drm/i915/i915_hwmon.c > +++ b/drivers/gpu/drm/i915/i915_hwmon.c > @@ -51,6 +51,7 @@ struct hwm_drvdata { > char name[12]; > int gt_n; > bool reset_in_progress; > + wait_queue_head_t waitq; > }; > > struct i915_hwmon { > @@ -395,16 +396,41 @@ hwm_power_max_read(struct hwm_drvdata *ddat, long *val) > static int > hwm_power_max_write(struct hwm_drvdata *ddat, long val) > { > +#define GUC_RESET_TIMEOUT msecs_to_jiffies(2000) > + > + int ret = 0, timeout = GUC_RESET_TIMEOUT; Patch looks good to me apart that I am not sure what is the purpose of the timeout? This is just the sysfs write path or has more callers? If the former perhaps it would be better to just use interruptible everything (mutex and sleep) and wait for as long as it takes or until user presses Ctrl-C? Regards, Tvrtko > struct i915_hwmon *hwmon = ddat->hwmon; > intel_wakeref_t wakeref; > - int ret = 0; > + DEFINE_WAIT(wait); > u32 nval; > > - mutex_lock(&hwmon->hwmon_lock); > - if (hwmon->ddat.reset_in_progress) { > - ret = -EAGAIN; > - goto unlock; > + /* Block waiting for GuC reset to complete when needed */ > + for (;;) { > + mutex_lock(&hwmon->hwmon_lock); > + > + prepare_to_wait(&ddat->waitq, &wait, TASK_INTERRUPTIBLE); > + > + if (!hwmon->ddat.reset_in_progress) > + break; > + > + if (signal_pending(current)) { > + ret = -EINTR; > + break; > + } > + > + if (!timeout) { > + ret = -ETIME; > + break; > + } > + > + mutex_unlock(&hwmon->hwmon_lock); > + > + timeout = schedule_timeout(timeout); > } > + finish_wait(&ddat->waitq, &wait); > + if (ret) > + goto unlock; > + > wakeref = intel_runtime_pm_get(ddat->uncore->rpm); > > /* Disable PL1 limit and verify, because the limit cannot be disabled on all platforms */ > @@ -508,6 +534,7 @@ void i915_hwmon_power_max_restore(struct drm_i915_private *i915, bool old) > intel_uncore_rmw(hwmon->ddat.uncore, hwmon->rg.pkg_rapl_limit, > PKG_PWR_LIM_1_EN, old ? PKG_PWR_LIM_1_EN : 0); > hwmon->ddat.reset_in_progress = false; > + wake_up_all(&hwmon->ddat.waitq); > > mutex_unlock(&hwmon->hwmon_lock); > } > @@ -784,6 +811,7 @@ void i915_hwmon_register(struct drm_i915_private *i915) > ddat->uncore = &i915->uncore; > snprintf(ddat->name, sizeof(ddat->name), "i915"); > ddat->gt_n = -1; > + init_waitqueue_head(&ddat->waitq); > > for_each_gt(gt, i915, i) { > ddat_gt = hwmon->ddat_gt + i; ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [Intel-gfx] [PATCH 3/3] drm/i915/hwmon: Block waiting for GuC reset to complete 2023-04-19 13:21 ` Tvrtko Ursulin @ 2023-04-19 22:10 ` Dixit, Ashutosh 2023-04-20 7:57 ` Tvrtko Ursulin 0 siblings, 1 reply; 31+ messages in thread From: Dixit, Ashutosh @ 2023-04-19 22:10 UTC (permalink / raw) To: Tvrtko Ursulin; +Cc: intel-gfx, dri-devel, Rodrigo Vivi On Wed, 19 Apr 2023 06:21:27 -0700, Tvrtko Ursulin wrote: > Hi Tvrtko, > On 10/04/2023 23:35, Ashutosh Dixit wrote: > > Instead of erroring out when GuC reset is in progress, block waiting for > > GuC reset to complete which is a more reasonable uapi behavior. > > > > v2: Avoid race between wake_up_all and waiting for wakeup (Rodrigo) > > > > Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com> > > --- > > drivers/gpu/drm/i915/i915_hwmon.c | 38 +++++++++++++++++++++++++++---- > > 1 file changed, 33 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_hwmon.c b/drivers/gpu/drm/i915/i915_hwmon.c > > index 9ab8971679fe3..8471a667dfc71 100644 > > --- a/drivers/gpu/drm/i915/i915_hwmon.c > > +++ b/drivers/gpu/drm/i915/i915_hwmon.c > > @@ -51,6 +51,7 @@ struct hwm_drvdata { > > char name[12]; > > int gt_n; > > bool reset_in_progress; > > + wait_queue_head_t waitq; > > }; > > struct i915_hwmon { > > @@ -395,16 +396,41 @@ hwm_power_max_read(struct hwm_drvdata *ddat, long *val) > > static int > > hwm_power_max_write(struct hwm_drvdata *ddat, long val) > > { > > +#define GUC_RESET_TIMEOUT msecs_to_jiffies(2000) > > + > > + int ret = 0, timeout = GUC_RESET_TIMEOUT; > > Patch looks good to me Great, thanks :) > apart that I am not sure what is the purpose of the timeout? This is just > the sysfs write path or has more callers? It is just the sysfs path, but the sysfs is accessed also by the oneAPI stack (Level 0). In the initial version I also didn't have the timeout thinking that the app can send a signal to the blocked thread to unblock it. I introduced the timeout after Rodrigo brought it up and I am now thinking maybe it's better to have the timeout in the driver since the app has no knowledge of how long GuC resets can take. But I can remove it if you think it's not needed. > If the > former perhaps it would be better to just use interruptible everything > (mutex and sleep) and wait for as long as it takes or until user presses > Ctrl-C? Now we are not holding the mutexes for long, just long enough do register rmw's. So not holding the mutex across GuC reset as we were originally. Therefore I am thinking mutex_lock_interruptible is not needed? The sleep is already interruptible (TASK_INTERRUPTIBLE). Anyway please let me know if you think we need to change anything. Thanks. -- Ashutosh > > struct i915_hwmon *hwmon = ddat->hwmon; > > intel_wakeref_t wakeref; > > - int ret = 0; > > + DEFINE_WAIT(wait); > > u32 nval; > > - mutex_lock(&hwmon->hwmon_lock); > > - if (hwmon->ddat.reset_in_progress) { > > - ret = -EAGAIN; > > - goto unlock; > > + /* Block waiting for GuC reset to complete when needed */ > > + for (;;) { > > + mutex_lock(&hwmon->hwmon_lock); > > + > > + prepare_to_wait(&ddat->waitq, &wait, TASK_INTERRUPTIBLE); > > + > > + if (!hwmon->ddat.reset_in_progress) > > + break; > > + > > + if (signal_pending(current)) { > > + ret = -EINTR; > > + break; > > + } > > + > > + if (!timeout) { > > + ret = -ETIME; > > + break; > > + } > > + > > + mutex_unlock(&hwmon->hwmon_lock); > > + > > + timeout = schedule_timeout(timeout); > > } > > + finish_wait(&ddat->waitq, &wait); > > + if (ret) > > + goto unlock; > > + > > wakeref = intel_runtime_pm_get(ddat->uncore->rpm); > > /* Disable PL1 limit and verify, because the limit cannot be > > disabled on all platforms */ > > @@ -508,6 +534,7 @@ void i915_hwmon_power_max_restore(struct drm_i915_private *i915, bool old) > > intel_uncore_rmw(hwmon->ddat.uncore, hwmon->rg.pkg_rapl_limit, > > PKG_PWR_LIM_1_EN, old ? PKG_PWR_LIM_1_EN : 0); > > hwmon->ddat.reset_in_progress = false; > > + wake_up_all(&hwmon->ddat.waitq); > > mutex_unlock(&hwmon->hwmon_lock); > > } > > @@ -784,6 +811,7 @@ void i915_hwmon_register(struct drm_i915_private *i915) > > ddat->uncore = &i915->uncore; > > snprintf(ddat->name, sizeof(ddat->name), "i915"); > > ddat->gt_n = -1; > > + init_waitqueue_head(&ddat->waitq); > > for_each_gt(gt, i915, i) { > > ddat_gt = hwmon->ddat_gt + i; ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [Intel-gfx] [PATCH 3/3] drm/i915/hwmon: Block waiting for GuC reset to complete 2023-04-19 22:10 ` Dixit, Ashutosh @ 2023-04-20 7:57 ` Tvrtko Ursulin 2023-04-20 15:43 ` Rodrigo Vivi 0 siblings, 1 reply; 31+ messages in thread From: Tvrtko Ursulin @ 2023-04-20 7:57 UTC (permalink / raw) To: Dixit, Ashutosh; +Cc: intel-gfx, dri-devel, Rodrigo Vivi On 19/04/2023 23:10, Dixit, Ashutosh wrote: > On Wed, 19 Apr 2023 06:21:27 -0700, Tvrtko Ursulin wrote: >> > > Hi Tvrtko, > >> On 10/04/2023 23:35, Ashutosh Dixit wrote: >>> Instead of erroring out when GuC reset is in progress, block waiting for >>> GuC reset to complete which is a more reasonable uapi behavior. >>> >>> v2: Avoid race between wake_up_all and waiting for wakeup (Rodrigo) >>> >>> Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com> >>> --- >>> drivers/gpu/drm/i915/i915_hwmon.c | 38 +++++++++++++++++++++++++++---- >>> 1 file changed, 33 insertions(+), 5 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/i915/i915_hwmon.c b/drivers/gpu/drm/i915/i915_hwmon.c >>> index 9ab8971679fe3..8471a667dfc71 100644 >>> --- a/drivers/gpu/drm/i915/i915_hwmon.c >>> +++ b/drivers/gpu/drm/i915/i915_hwmon.c >>> @@ -51,6 +51,7 @@ struct hwm_drvdata { >>> char name[12]; >>> int gt_n; >>> bool reset_in_progress; >>> + wait_queue_head_t waitq; >>> }; >>> struct i915_hwmon { >>> @@ -395,16 +396,41 @@ hwm_power_max_read(struct hwm_drvdata *ddat, long *val) >>> static int >>> hwm_power_max_write(struct hwm_drvdata *ddat, long val) >>> { >>> +#define GUC_RESET_TIMEOUT msecs_to_jiffies(2000) >>> + >>> + int ret = 0, timeout = GUC_RESET_TIMEOUT; >> >> Patch looks good to me > > Great, thanks :) > >> apart that I am not sure what is the purpose of the timeout? This is just >> the sysfs write path or has more callers? > > It is just the sysfs path, but the sysfs is accessed also by the oneAPI > stack (Level 0). In the initial version I also didn't have the timeout > thinking that the app can send a signal to the blocked thread to unblock > it. I introduced the timeout after Rodrigo brought it up and I am now > thinking maybe it's better to have the timeout in the driver since the app > has no knowledge of how long GuC resets can take. But I can remove it if > you think it's not needed. Maybe I am missing something but I don't get why we would need to provide a timeout facility in sysfs? If the library writes here to configure something it already has to expect a blocking write by the nature of a a write(2) and sysfs contract. It can take long for any reason so I hope we are not guaranteeing some latency number to someone? Or the concern is just about things getting stuck? In which case I think Ctrl-C is the answer because ETIME is not even listed as an errno for write(2). >> If the >> former perhaps it would be better to just use interruptible everything >> (mutex and sleep) and wait for as long as it takes or until user presses >> Ctrl-C? > > Now we are not holding the mutexes for long, just long enough do register > rmw's. So not holding the mutex across GuC reset as we were > originally. Therefore I am thinking mutex_lock_interruptible is not needed? > The sleep is already interruptible (TASK_INTERRUPTIBLE). Ah yes, you are right. Regards, Tvrtko > Anyway please let me know if you think we need to change anything. > > Thanks. > -- > Ashutosh > >>> struct i915_hwmon *hwmon = ddat->hwmon; >>> intel_wakeref_t wakeref; >>> - int ret = 0; >>> + DEFINE_WAIT(wait); >>> u32 nval; >>> - mutex_lock(&hwmon->hwmon_lock); >>> - if (hwmon->ddat.reset_in_progress) { >>> - ret = -EAGAIN; >>> - goto unlock; >>> + /* Block waiting for GuC reset to complete when needed */ >>> + for (;;) { >>> + mutex_lock(&hwmon->hwmon_lock); >>> + >>> + prepare_to_wait(&ddat->waitq, &wait, TASK_INTERRUPTIBLE); >>> + >>> + if (!hwmon->ddat.reset_in_progress) >>> + break; >>> + >>> + if (signal_pending(current)) { >>> + ret = -EINTR; >>> + break; >>> + } >>> + >>> + if (!timeout) { >>> + ret = -ETIME; >>> + break; >>> + } >>> + >>> + mutex_unlock(&hwmon->hwmon_lock); >>> + >>> + timeout = schedule_timeout(timeout); >>> } >>> + finish_wait(&ddat->waitq, &wait); >>> + if (ret) >>> + goto unlock; >>> + >>> wakeref = intel_runtime_pm_get(ddat->uncore->rpm); >>> /* Disable PL1 limit and verify, because the limit cannot be >>> disabled on all platforms */ >>> @@ -508,6 +534,7 @@ void i915_hwmon_power_max_restore(struct drm_i915_private *i915, bool old) >>> intel_uncore_rmw(hwmon->ddat.uncore, hwmon->rg.pkg_rapl_limit, >>> PKG_PWR_LIM_1_EN, old ? PKG_PWR_LIM_1_EN : 0); >>> hwmon->ddat.reset_in_progress = false; >>> + wake_up_all(&hwmon->ddat.waitq); >>> mutex_unlock(&hwmon->hwmon_lock); >>> } >>> @@ -784,6 +811,7 @@ void i915_hwmon_register(struct drm_i915_private *i915) >>> ddat->uncore = &i915->uncore; >>> snprintf(ddat->name, sizeof(ddat->name), "i915"); >>> ddat->gt_n = -1; >>> + init_waitqueue_head(&ddat->waitq); >>> for_each_gt(gt, i915, i) { >>> ddat_gt = hwmon->ddat_gt + i; ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [Intel-gfx] [PATCH 3/3] drm/i915/hwmon: Block waiting for GuC reset to complete 2023-04-20 7:57 ` Tvrtko Ursulin @ 2023-04-20 15:43 ` Rodrigo Vivi 2023-04-20 16:26 ` Dixit, Ashutosh 0 siblings, 1 reply; 31+ messages in thread From: Rodrigo Vivi @ 2023-04-20 15:43 UTC (permalink / raw) To: Tvrtko Ursulin; +Cc: intel-gfx, dri-devel On Thu, Apr 20, 2023 at 08:57:24AM +0100, Tvrtko Ursulin wrote: > > On 19/04/2023 23:10, Dixit, Ashutosh wrote: > > On Wed, 19 Apr 2023 06:21:27 -0700, Tvrtko Ursulin wrote: > > > > > > > Hi Tvrtko, > > > > > On 10/04/2023 23:35, Ashutosh Dixit wrote: > > > > Instead of erroring out when GuC reset is in progress, block waiting for > > > > GuC reset to complete which is a more reasonable uapi behavior. > > > > > > > > v2: Avoid race between wake_up_all and waiting for wakeup (Rodrigo) > > > > > > > > Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com> > > > > --- > > > > drivers/gpu/drm/i915/i915_hwmon.c | 38 +++++++++++++++++++++++++++---- > > > > 1 file changed, 33 insertions(+), 5 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/i915_hwmon.c b/drivers/gpu/drm/i915/i915_hwmon.c > > > > index 9ab8971679fe3..8471a667dfc71 100644 > > > > --- a/drivers/gpu/drm/i915/i915_hwmon.c > > > > +++ b/drivers/gpu/drm/i915/i915_hwmon.c > > > > @@ -51,6 +51,7 @@ struct hwm_drvdata { > > > > char name[12]; > > > > int gt_n; > > > > bool reset_in_progress; > > > > + wait_queue_head_t waitq; > > > > }; > > > > struct i915_hwmon { > > > > @@ -395,16 +396,41 @@ hwm_power_max_read(struct hwm_drvdata *ddat, long *val) > > > > static int > > > > hwm_power_max_write(struct hwm_drvdata *ddat, long val) > > > > { > > > > +#define GUC_RESET_TIMEOUT msecs_to_jiffies(2000) > > > > + > > > > + int ret = 0, timeout = GUC_RESET_TIMEOUT; > > > > > > Patch looks good to me > > > > Great, thanks :) > > > > > apart that I am not sure what is the purpose of the timeout? This is just > > > the sysfs write path or has more callers? > > > > It is just the sysfs path, but the sysfs is accessed also by the oneAPI > > stack (Level 0). In the initial version I also didn't have the timeout > > thinking that the app can send a signal to the blocked thread to unblock > > it. I introduced the timeout after Rodrigo brought it up and I am now > > thinking maybe it's better to have the timeout in the driver since the app > > has no knowledge of how long GuC resets can take. But I can remove it if > > you think it's not needed. > > Maybe I am missing something but I don't get why we would need to provide a > timeout facility in sysfs? If the library writes here to configure something > it already has to expect a blocking write by the nature of a a write(2) and > sysfs contract. It can take long for any reason so I hope we are not > guaranteeing some latency number to someone? Or the concern is just about > things getting stuck? In which case I think Ctrl-C is the answer because > ETIME is not even listed as an errno for write(2). I suggested the timeout on the other version because of that race, which is fixed now with this approach. It is probably better to remove it now to avoid confusions. I'm sorry about that. > > > > If the > > > former perhaps it would be better to just use interruptible everything > > > (mutex and sleep) and wait for as long as it takes or until user presses > > > Ctrl-C? > > > > Now we are not holding the mutexes for long, just long enough do register > > rmw's. So not holding the mutex across GuC reset as we were > > originally. Therefore I am thinking mutex_lock_interruptible is not needed? > > The sleep is already interruptible (TASK_INTERRUPTIBLE). > > Ah yes, you are right. > > Regards, > > Tvrtko > > > Anyway please let me know if you think we need to change anything. > > > > Thanks. > > -- > > Ashutosh > > > > > > struct i915_hwmon *hwmon = ddat->hwmon; > > > > intel_wakeref_t wakeref; > > > > - int ret = 0; > > > > + DEFINE_WAIT(wait); > > > > u32 nval; > > > > - mutex_lock(&hwmon->hwmon_lock); > > > > - if (hwmon->ddat.reset_in_progress) { > > > > - ret = -EAGAIN; > > > > - goto unlock; > > > > + /* Block waiting for GuC reset to complete when needed */ > > > > + for (;;) { > > > > + mutex_lock(&hwmon->hwmon_lock); > > > > + > > > > + prepare_to_wait(&ddat->waitq, &wait, TASK_INTERRUPTIBLE); > > > > + > > > > + if (!hwmon->ddat.reset_in_progress) > > > > + break; > > > > + > > > > + if (signal_pending(current)) { > > > > + ret = -EINTR; > > > > + break; > > > > + } > > > > + > > > > + if (!timeout) { > > > > + ret = -ETIME; > > > > + break; > > > > + } > > > > + > > > > + mutex_unlock(&hwmon->hwmon_lock); > > > > + > > > > + timeout = schedule_timeout(timeout); > > > > } > > > > + finish_wait(&ddat->waitq, &wait); > > > > + if (ret) > > > > + goto unlock; > > > > + > > > > wakeref = intel_runtime_pm_get(ddat->uncore->rpm); > > > > /* Disable PL1 limit and verify, because the limit cannot be > > > > disabled on all platforms */ > > > > @@ -508,6 +534,7 @@ void i915_hwmon_power_max_restore(struct drm_i915_private *i915, bool old) > > > > intel_uncore_rmw(hwmon->ddat.uncore, hwmon->rg.pkg_rapl_limit, > > > > PKG_PWR_LIM_1_EN, old ? PKG_PWR_LIM_1_EN : 0); > > > > hwmon->ddat.reset_in_progress = false; > > > > + wake_up_all(&hwmon->ddat.waitq); > > > > mutex_unlock(&hwmon->hwmon_lock); > > > > } > > > > @@ -784,6 +811,7 @@ void i915_hwmon_register(struct drm_i915_private *i915) > > > > ddat->uncore = &i915->uncore; > > > > snprintf(ddat->name, sizeof(ddat->name), "i915"); > > > > ddat->gt_n = -1; > > > > + init_waitqueue_head(&ddat->waitq); > > > > for_each_gt(gt, i915, i) { > > > > ddat_gt = hwmon->ddat_gt + i; ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [Intel-gfx] [PATCH 3/3] drm/i915/hwmon: Block waiting for GuC reset to complete 2023-04-20 15:43 ` Rodrigo Vivi @ 2023-04-20 16:26 ` Dixit, Ashutosh 0 siblings, 0 replies; 31+ messages in thread From: Dixit, Ashutosh @ 2023-04-20 16:26 UTC (permalink / raw) To: Rodrigo Vivi; +Cc: intel-gfx, dri-devel On Thu, 20 Apr 2023 08:43:52 -0700, Rodrigo Vivi wrote: > Hi Rodrigo, > On Thu, Apr 20, 2023 at 08:57:24AM +0100, Tvrtko Ursulin wrote: > > > > On 19/04/2023 23:10, Dixit, Ashutosh wrote: > > > On Wed, 19 Apr 2023 06:21:27 -0700, Tvrtko Ursulin wrote: > > > > > > > > > > Hi Tvrtko, > > > > > > > On 10/04/2023 23:35, Ashutosh Dixit wrote: > > > > > Instead of erroring out when GuC reset is in progress, block waiting for > > > > > GuC reset to complete which is a more reasonable uapi behavior. > > > > > > > > > > v2: Avoid race between wake_up_all and waiting for wakeup (Rodrigo) > > > > > > > > > > Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com> > > > > > --- > > > > > drivers/gpu/drm/i915/i915_hwmon.c | 38 +++++++++++++++++++++++++++---- > > > > > 1 file changed, 33 insertions(+), 5 deletions(-) > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/i915_hwmon.c b/drivers/gpu/drm/i915/i915_hwmon.c > > > > > index 9ab8971679fe3..8471a667dfc71 100644 > > > > > --- a/drivers/gpu/drm/i915/i915_hwmon.c > > > > > +++ b/drivers/gpu/drm/i915/i915_hwmon.c > > > > > @@ -51,6 +51,7 @@ struct hwm_drvdata { > > > > > char name[12]; > > > > > int gt_n; > > > > > bool reset_in_progress; > > > > > + wait_queue_head_t waitq; > > > > > }; > > > > > struct i915_hwmon { > > > > > @@ -395,16 +396,41 @@ hwm_power_max_read(struct hwm_drvdata *ddat, long *val) > > > > > static int > > > > > hwm_power_max_write(struct hwm_drvdata *ddat, long val) > > > > > { > > > > > +#define GUC_RESET_TIMEOUT msecs_to_jiffies(2000) > > > > > + > > > > > + int ret = 0, timeout = GUC_RESET_TIMEOUT; > > > > > > > > Patch looks good to me > > > > > > Great, thanks :) > > > > > > > apart that I am not sure what is the purpose of the timeout? This is just > > > > the sysfs write path or has more callers? > > > > > > It is just the sysfs path, but the sysfs is accessed also by the oneAPI > > > stack (Level 0). In the initial version I also didn't have the timeout > > > thinking that the app can send a signal to the blocked thread to unblock > > > it. I introduced the timeout after Rodrigo brought it up and I am now > > > thinking maybe it's better to have the timeout in the driver since the app > > > has no knowledge of how long GuC resets can take. But I can remove it if > > > you think it's not needed. > > > > Maybe I am missing something but I don't get why we would need to provide a > > timeout facility in sysfs? If the library writes here to configure something > > it already has to expect a blocking write by the nature of a a write(2) and > > sysfs contract. It can take long for any reason so I hope we are not > > guaranteeing some latency number to someone? Or the concern is just about > > things getting stuck? In which case I think Ctrl-C is the answer because > > ETIME is not even listed as an errno for write(2). Hmm, good point. > I suggested the timeout on the other version because of that race, > which is fixed now with this approach. It is probably better to remove > it now to avoid confusions. I'm sorry about that. No problem, I've removed the timeout in the latest version. Thanks for the R-b. Ashutosh ^ permalink raw reply [flat|nested] 31+ messages in thread
* [Intel-gfx] [PATCH v6 0/3] drm/i915/guc: Disable PL1 power limit when loading GuC firmware @ 2023-04-20 16:40 Ashutosh Dixit 2023-04-20 16:40 ` [Intel-gfx] [PATCH 3/3] drm/i915/hwmon: Block waiting for GuC reset to complete Ashutosh Dixit 0 siblings, 1 reply; 31+ messages in thread From: Ashutosh Dixit @ 2023-04-20 16:40 UTC (permalink / raw) To: intel-gfx; +Cc: dri-devel, Rodrigo Vivi v6: Update Patch 3 to remove the timeout when blocked v1-v5: Please see individual patches for revision history Ashutosh Dixit (3): drm/i915/hwmon: Get mutex and rpm ref just once in hwm_power_max_write drm/i915/guc: Disable PL1 power limit when loading GuC firmware drm/i915/hwmon: Block waiting for GuC reset to complete drivers/gpu/drm/i915/gt/uc/intel_uc.c | 13 +++- drivers/gpu/drm/i915/i915_hwmon.c | 87 +++++++++++++++++++++++---- drivers/gpu/drm/i915/i915_hwmon.h | 7 +++ 3 files changed, 93 insertions(+), 14 deletions(-) -- 2.38.0 ^ permalink raw reply [flat|nested] 31+ messages in thread
* [Intel-gfx] [PATCH 3/3] drm/i915/hwmon: Block waiting for GuC reset to complete 2023-04-20 16:40 [Intel-gfx] [PATCH v6 0/3] drm/i915/guc: Disable PL1 power limit when loading GuC firmware Ashutosh Dixit @ 2023-04-20 16:40 ` Ashutosh Dixit 0 siblings, 0 replies; 31+ messages in thread From: Ashutosh Dixit @ 2023-04-20 16:40 UTC (permalink / raw) To: intel-gfx; +Cc: dri-devel, Rodrigo Vivi Instead of erroring out when GuC reset is in progress, block waiting for GuC reset to complete which is a more reasonable uapi behavior. v2: Avoid race between wake_up_all and waiting for wakeup (Rodrigo) v3: Remove timeout when blocked (Tvrtko) Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> --- drivers/gpu/drm/i915/i915_hwmon.c | 29 +++++++++++++++++++++++++---- 1 file changed, 25 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_hwmon.c b/drivers/gpu/drm/i915/i915_hwmon.c index 9ab8971679fe3..a3bdd9f68a458 100644 --- a/drivers/gpu/drm/i915/i915_hwmon.c +++ b/drivers/gpu/drm/i915/i915_hwmon.c @@ -51,6 +51,7 @@ struct hwm_drvdata { char name[12]; int gt_n; bool reset_in_progress; + wait_queue_head_t waitq; }; struct i915_hwmon { @@ -397,14 +398,32 @@ hwm_power_max_write(struct hwm_drvdata *ddat, long val) { struct i915_hwmon *hwmon = ddat->hwmon; intel_wakeref_t wakeref; + DEFINE_WAIT(wait); int ret = 0; u32 nval; - mutex_lock(&hwmon->hwmon_lock); - if (hwmon->ddat.reset_in_progress) { - ret = -EAGAIN; - goto unlock; + /* Block waiting for GuC reset to complete when needed */ + for (;;) { + mutex_lock(&hwmon->hwmon_lock); + + prepare_to_wait(&ddat->waitq, &wait, TASK_INTERRUPTIBLE); + + if (!hwmon->ddat.reset_in_progress) + break; + + if (signal_pending(current)) { + ret = -EINTR; + break; + } + + mutex_unlock(&hwmon->hwmon_lock); + + schedule(); } + finish_wait(&ddat->waitq, &wait); + if (ret) + goto unlock; + wakeref = intel_runtime_pm_get(ddat->uncore->rpm); /* Disable PL1 limit and verify, because the limit cannot be disabled on all platforms */ @@ -508,6 +527,7 @@ void i915_hwmon_power_max_restore(struct drm_i915_private *i915, bool old) intel_uncore_rmw(hwmon->ddat.uncore, hwmon->rg.pkg_rapl_limit, PKG_PWR_LIM_1_EN, old ? PKG_PWR_LIM_1_EN : 0); hwmon->ddat.reset_in_progress = false; + wake_up_all(&hwmon->ddat.waitq); mutex_unlock(&hwmon->hwmon_lock); } @@ -784,6 +804,7 @@ void i915_hwmon_register(struct drm_i915_private *i915) ddat->uncore = &i915->uncore; snprintf(ddat->name, sizeof(ddat->name), "i915"); ddat->gt_n = -1; + init_waitqueue_head(&ddat->waitq); for_each_gt(gt, i915, i) { ddat_gt = hwmon->ddat_gt + i; -- 2.38.0 ^ permalink raw reply related [flat|nested] 31+ messages in thread
end of thread, other threads:[~2023-04-20 16:44 UTC | newest] Thread overview: 31+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-04-06 4:45 [Intel-gfx] [PATCH v4 0/3] drm/i915/guc: Disable PL1 power limit when loading GuC firmware Ashutosh Dixit 2023-04-06 4:45 ` Ashutosh Dixit 2023-04-06 4:45 ` [Intel-gfx] [PATCH 1/3] drm/i915/hwmon: Get mutex and rpm ref just once in hwm_power_max_write Ashutosh Dixit 2023-04-06 4:45 ` Ashutosh Dixit 2023-04-07 11:08 ` [Intel-gfx] " Rodrigo Vivi 2023-04-06 4:45 ` [Intel-gfx] [PATCH 2/3] drm/i915/guc: Disable PL1 power limit when loading GuC firmware Ashutosh Dixit 2023-04-06 4:45 ` Ashutosh Dixit 2023-04-06 9:16 ` [Intel-gfx] " kernel test robot 2023-04-06 9:16 ` kernel test robot 2023-04-07 11:08 ` Rodrigo Vivi 2023-04-10 22:17 ` Dixit, Ashutosh 2023-04-06 4:45 ` [Intel-gfx] [PATCH 3/3] drm/i915/hwmon: Block waiting for GuC reset to complete Ashutosh Dixit 2023-04-06 4:45 ` Ashutosh Dixit 2023-04-07 11:04 ` [Intel-gfx] " Rodrigo Vivi 2023-04-10 22:40 ` Dixit, Ashutosh 2023-04-10 22:40 ` Dixit, Ashutosh 2023-04-06 5:15 ` [Intel-gfx] ✗ Fi.CI.SPARSE: warning for drm/i915/guc: Disable PL1 power limit when loading GuC firmware Patchwork 2023-04-06 5:32 ` [Intel-gfx] ✓ Fi.CI.BAT: success " Patchwork 2023-04-06 17:42 ` [Intel-gfx] ✓ Fi.CI.IGT: " Patchwork -- strict thread matches above, loose matches on Subject: below -- 2023-04-10 22:35 [Intel-gfx] [PATCH 0/3] " Ashutosh Dixit 2023-04-10 22:35 ` [Intel-gfx] [PATCH 3/3] drm/i915/hwmon: Block waiting for GuC reset to complete Ashutosh Dixit 2023-04-18 5:35 ` Rodrigo Vivi 2023-04-18 17:23 ` Dixit, Ashutosh 2023-04-19 19:40 ` Rodrigo Vivi 2023-04-19 22:13 ` Dixit, Ashutosh 2023-04-20 15:45 ` Rodrigo Vivi 2023-04-19 13:21 ` Tvrtko Ursulin 2023-04-19 22:10 ` Dixit, Ashutosh 2023-04-20 7:57 ` Tvrtko Ursulin 2023-04-20 15:43 ` Rodrigo Vivi 2023-04-20 16:26 ` Dixit, Ashutosh 2023-04-20 16:40 [Intel-gfx] [PATCH v6 0/3] drm/i915/guc: Disable PL1 power limit when loading GuC firmware Ashutosh Dixit 2023-04-20 16:40 ` [Intel-gfx] [PATCH 3/3] drm/i915/hwmon: Block waiting for GuC reset to complete Ashutosh Dixit
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.