| Message ID | 20211101043937.35747-2-vinay.belgaumkar@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/guc/slpc: Implement waitboost for SLPC | expand |
On Sun, 31 Oct 2021 21:39:35 -0700, Belgaumkar, Vinay wrote: > > Define helpers and struct members required to record boost info. > Boost frequency is initialized to RP0 at SLPC init. Also define num_waiters > which can track the pending boost requests. > > Boost will be done by scheduling a worker thread. This will allow > us to make H2G calls inside an interrupt context. Initialize the "to not make H2G calls from interrupt context" is probably better. > +static int slpc_force_min_freq(struct intel_guc_slpc *slpc, u32 freq) > +{ > + struct drm_i915_private *i915 = slpc_to_i915(slpc); > + intel_wakeref_t wakeref; > + int ret = 0; > + > + lockdep_assert_held(&slpc->lock); > + > + /** nit: this I believe should just be /* /** I believe shows up in kerneldoc so shouldn't be used unless we want something in kerneldoc. > + * This function is a little different as compared to > + * intel_guc_slpc_set_min_freq(). Softlimit will not be updated > + * here since this is used to temporarily change min freq, > + * for example, during a waitboost. Caller is responsible for > + * checking bounds. > + */ > + > + with_intel_runtime_pm(&i915->runtime_pm, wakeref) { > + ret = slpc_set_param(slpc, > + SLPC_PARAM_GLOBAL_MIN_GT_UNSLICE_FREQ_MHZ, > + freq); > + if (ret) > + drm_err(&i915->drm, "Unable to force min freq to %u: %d", Probably drm_err_ratelimited since it's called at run time not only at init? Not sure if drm_err_once suffizes, probably not. > + freq, ret); > + } > + > + return ret; > +} > + > +static void slpc_boost_work(struct work_struct *work) > +{ > + struct intel_guc_slpc *slpc = container_of(work, typeof(*slpc), boost_work); > + > + /* Raise min freq to boost. It's possible that > + * this is greater than current max. But it will > + * certainly be limited by RP0. An error setting > + * the min param is not fatal. > + */ nit: do we follow the following format for multi-line comments, Documentation/process/coding-style.rst mentions this: /* * Line 1 * Line 2 */
On 11/1/2021 1:26 PM, Dixit, Ashutosh wrote: > On Sun, 31 Oct 2021 21:39:35 -0700, Belgaumkar, Vinay wrote: >> >> Define helpers and struct members required to record boost info. >> Boost frequency is initialized to RP0 at SLPC init. Also define num_waiters >> which can track the pending boost requests. >> >> Boost will be done by scheduling a worker thread. This will allow >> us to make H2G calls inside an interrupt context. Initialize the > > "to not make H2G calls from interrupt context" is probably better. > >> +static int slpc_force_min_freq(struct intel_guc_slpc *slpc, u32 freq) >> +{ >> + struct drm_i915_private *i915 = slpc_to_i915(slpc); >> + intel_wakeref_t wakeref; >> + int ret = 0; >> + >> + lockdep_assert_held(&slpc->lock); >> + >> + /** > > nit: this I believe should just be > > /* ok. > > /** I believe shows up in kerneldoc so shouldn't be used unless we want > something in kerneldoc. > >> + * This function is a little different as compared to >> + * intel_guc_slpc_set_min_freq(). Softlimit will not be updated >> + * here since this is used to temporarily change min freq, >> + * for example, during a waitboost. Caller is responsible for >> + * checking bounds. >> + */ >> + >> + with_intel_runtime_pm(&i915->runtime_pm, wakeref) { >> + ret = slpc_set_param(slpc, >> + SLPC_PARAM_GLOBAL_MIN_GT_UNSLICE_FREQ_MHZ, >> + freq); >> + if (ret) >> + drm_err(&i915->drm, "Unable to force min freq to %u: %d", > > Probably drm_err_ratelimited since it's called at run time not only at > init? Not sure if drm_err_once suffizes, probably not. Keeping it drm_err as discussed offline. > >> + freq, ret); >> + } >> + >> + return ret; >> +} >> + >> +static void slpc_boost_work(struct work_struct *work) >> +{ >> + struct intel_guc_slpc *slpc = container_of(work, typeof(*slpc), boost_work); >> + >> + /* Raise min freq to boost. It's possible that >> + * this is greater than current max. But it will >> + * certainly be limited by RP0. An error setting >> + * the min param is not fatal. >> + */ > > nit: do we follow the following format for multi-line comments, > Documentation/process/coding-style.rst mentions this: > > /* > * Line 1 > * Line 2 > */ Ok. Thanks, Vinay. >
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c index 65a3e7fdb2b2..cc51987b2535 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c @@ -79,29 +79,6 @@ static void slpc_mem_set_disabled(struct slpc_shared_data *data, slpc_mem_set_param(data, enable_id, 0); } -int intel_guc_slpc_init(struct intel_guc_slpc *slpc) -{ - struct intel_guc *guc = slpc_to_guc(slpc); - struct drm_i915_private *i915 = slpc_to_i915(slpc); - u32 size = PAGE_ALIGN(sizeof(struct slpc_shared_data)); - int err; - - GEM_BUG_ON(slpc->vma); - - err = intel_guc_allocate_and_map_vma(guc, size, &slpc->vma, (void **)&slpc->vaddr); - if (unlikely(err)) { - drm_err(&i915->drm, - "Failed to allocate SLPC struct (err=%pe)\n", - ERR_PTR(err)); - return err; - } - - slpc->max_freq_softlimit = 0; - slpc->min_freq_softlimit = 0; - - return err; -} - static u32 slpc_get_state(struct intel_guc_slpc *slpc) { struct slpc_shared_data *data; @@ -203,6 +180,81 @@ static int slpc_unset_param(struct intel_guc_slpc *slpc, return guc_action_slpc_unset_param(guc, id); } +static int slpc_force_min_freq(struct intel_guc_slpc *slpc, u32 freq) +{ + struct drm_i915_private *i915 = slpc_to_i915(slpc); + intel_wakeref_t wakeref; + int ret = 0; + + lockdep_assert_held(&slpc->lock); + + /** + * This function is a little different as compared to + * intel_guc_slpc_set_min_freq(). Softlimit will not be updated + * here since this is used to temporarily change min freq, + * for example, during a waitboost. Caller is responsible for + * checking bounds. + */ + + with_intel_runtime_pm(&i915->runtime_pm, wakeref) { + ret = slpc_set_param(slpc, + SLPC_PARAM_GLOBAL_MIN_GT_UNSLICE_FREQ_MHZ, + freq); + if (ret) + drm_err(&i915->drm, "Unable to force min freq to %u: %d", + freq, ret); + } + + return ret; +} + +static void slpc_boost_work(struct work_struct *work) +{ + struct intel_guc_slpc *slpc = container_of(work, typeof(*slpc), boost_work); + + /* Raise min freq to boost. It's possible that + * this is greater than current max. But it will + * certainly be limited by RP0. An error setting + * the min param is not fatal. + */ + mutex_lock(&slpc->lock); + if (atomic_read(&slpc->num_waiters)) { + slpc_force_min_freq(slpc, slpc->boost_freq); + slpc->num_boosts++; + } + mutex_unlock(&slpc->lock); +} + +int intel_guc_slpc_init(struct intel_guc_slpc *slpc) +{ + struct intel_guc *guc = slpc_to_guc(slpc); + struct drm_i915_private *i915 = slpc_to_i915(slpc); + u32 size = PAGE_ALIGN(sizeof(struct slpc_shared_data)); + int err; + + GEM_BUG_ON(slpc->vma); + + err = intel_guc_allocate_and_map_vma(guc, size, &slpc->vma, (void **)&slpc->vaddr); + if (unlikely(err)) { + drm_err(&i915->drm, + "Failed to allocate SLPC struct (err=%pe)\n", + ERR_PTR(err)); + return err; + } + + slpc->max_freq_softlimit = 0; + slpc->min_freq_softlimit = 0; + + slpc->boost_freq = 0; + atomic_set(&slpc->num_waiters, 0); + slpc->num_boosts = 0; + + mutex_init(&slpc->lock); + INIT_WORK(&slpc->boost_work, slpc_boost_work); + + return err; +} + static const char *slpc_global_state_to_string(enum slpc_global_state state) { switch (state) { @@ -522,6 +574,9 @@ static void slpc_get_rp_values(struct intel_guc_slpc *slpc) GT_FREQUENCY_MULTIPLIER; slpc->min_freq = REG_FIELD_GET(RPN_CAP_MASK, rp_state_cap) * GT_FREQUENCY_MULTIPLIER; + + if (!slpc->boost_freq) + slpc->boost_freq = slpc->rp0_freq; } /* diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.h b/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.h index e45054d5b9b4..b62528647770 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.h +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.h @@ -38,5 +38,6 @@ int intel_guc_slpc_get_max_freq(struct intel_guc_slpc *slpc, u32 *val); int intel_guc_slpc_get_min_freq(struct intel_guc_slpc *slpc, u32 *val); int intel_guc_slpc_print_info(struct intel_guc_slpc *slpc, struct drm_printer *p); void intel_guc_pm_intrmsk_enable(struct intel_gt *gt); +void intel_guc_slpc_boost(struct intel_guc_slpc *slpc); #endif diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc_types.h b/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc_types.h index 41d13527666f..bf5b9a563c09 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc_types.h +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_slpc_types.h @@ -6,6 +6,9 @@ #ifndef _INTEL_GUC_SLPC_TYPES_H_ #define _INTEL_GUC_SLPC_TYPES_H_ +#include <linux/atomic.h> +#include <linux/workqueue.h> +#include <linux/mutex.h> #include <linux/types.h> #define SLPC_RESET_TIMEOUT_MS 5 @@ -20,10 +23,20 @@ struct intel_guc_slpc { u32 min_freq; u32 rp0_freq; u32 rp1_freq; + u32 boost_freq; /* frequency softlimits */ u32 min_freq_softlimit; u32 max_freq_softlimit; + + /* Protects set/reset of boost freq + * and value of num_waiters + */ + struct mutex lock; + + struct work_struct boost_work; + atomic_t num_waiters; + u32 num_boosts; }; #endif
Define helpers and struct members required to record boost info. Boost frequency is initialized to RP0 at SLPC init. Also define num_waiters which can track the pending boost requests. Boost will be done by scheduling a worker thread. This will allow us to make H2G calls inside an interrupt context. Initialize the worker function during SLPC init as well. Had to move intel_guc_slpc_init a few lines below to accomodate this. v2: Add a workqueue to handle waitboost Cc: Ashutosh Dixit <ashutosh.dixit@intel.com> Signed-off-by: Vinay Belgaumkar <vinay.belgaumkar@intel.com> --- drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.c | 101 ++++++++++++++---- drivers/gpu/drm/i915/gt/uc/intel_guc_slpc.h | 1 + .../gpu/drm/i915/gt/uc/intel_guc_slpc_types.h | 13 +++ 3 files changed, 92 insertions(+), 23 deletions(-)