| Message ID | 20190415230526.1145629-1-bob.j.paauwe@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: Configurable GT idle frequency | expand |
On Mon, Apr 15, 2019 at 04:05:26PM -0700, Bob Paauwe wrote: >There are real-time use cases where having deterministic CPU processes >can be more important than GPU power/performance. Parking the GPU at a >specific freqency by setting idle, min and max prohibits the normal >dynamic GPU frequency switching which can introduce significant PCI-E >latency. This adds the ability to configure the GPU "idle" frequecy >using the same method that already exists for minimum and maximum >frequencies. > >In addition, parking the idle frequency may reduce spool up latencies >on GPU workloads. > >Signed-off-by: Bob Paauwe <bob.j.paauwe@intel.com> >--- > drivers/gpu/drm/i915/i915_sysfs.c | 60 +++++++++++++++++++++++++++++++ > 1 file changed, 60 insertions(+) > >diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c >index 41313005af42..62612c23d514 100644 >--- a/drivers/gpu/drm/i915/i915_sysfs.c >+++ b/drivers/gpu/drm/i915/i915_sysfs.c >@@ -454,11 +454,69 @@ static ssize_t gt_min_freq_mhz_store(struct device *kdev, > return ret ?: count; > } > >+static ssize_t gt_idle_freq_mhz_show(struct device *kdev, struct device_attribute *attr, char *buf) >+{ >+ struct drm_i915_private *dev_priv = kdev_minor_to_i915(kdev); >+ >+ return snprintf(buf, PAGE_SIZE, "%d\n", >+ intel_gpu_freq(dev_priv, >+ dev_priv->gt_pm.rps.idle_freq)); >+} >+ >+static ssize_t gt_idle_freq_mhz_store(struct device *kdev, >+ struct device_attribute *attr, >+ const char *buf, size_t count) >+{ >+ struct drm_i915_private *dev_priv = kdev_minor_to_i915(kdev); >+ struct intel_rps *rps = &dev_priv->gt_pm.rps; >+ intel_wakeref_t wakeref; >+ u32 val; val can probably just be u8. max_freq, min_freq, etc. are only u8 in struct intel_rps *rps. >+ ssize_t ret; >+ >+ ret = kstrtou32(buf, 0, &val); >+ if (ret) >+ return ret; >+ >+ wakeref = intel_runtime_pm_get(dev_priv); >+ >+ mutex_lock(&dev_priv->pcu_lock); >+ >+ val = intel_freq_opcode(dev_priv, val); >+ >+ if (val < rps->min_freq || >+ val > rps->max_freq || >+ val > rps->max_freq_softlimit) { >+ mutex_unlock(&dev_priv->pcu_lock); >+ intel_runtime_pm_put(dev_priv, wakeref); >+ return -EINVAL; >+ } >+ >+ rps->idle_freq = val; >+ >+ val = clamp_t(int, rps->cur_freq, >+ rps->idle_freq, >+ rps->max_freq_softlimit); This should probably be clamped to u8 instead of int. Vanshi >+ >+ /* >+ * If the current freq is at the old idle freq we should >+ * ajust it to the new idle. Calling *_set_rps will also >+ * update the interrupt limits and PMINTRMSK if ncessary. >+ */ >+ ret = intel_set_rps(dev_priv, val); >+ >+ mutex_unlock(&dev_priv->pcu_lock); >+ >+ intel_runtime_pm_put(dev_priv, wakeref); >+ >+ return ret ?: count; >+} >+ > static DEVICE_ATTR_RO(gt_act_freq_mhz); > static DEVICE_ATTR_RO(gt_cur_freq_mhz); > static DEVICE_ATTR_RW(gt_boost_freq_mhz); > static DEVICE_ATTR_RW(gt_max_freq_mhz); > static DEVICE_ATTR_RW(gt_min_freq_mhz); >+static DEVICE_ATTR_RW(gt_idle_freq_mhz); > > static DEVICE_ATTR_RO(vlv_rpe_freq_mhz); > >@@ -492,6 +550,7 @@ static const struct attribute * const gen6_attrs[] = { > &dev_attr_gt_boost_freq_mhz.attr, > &dev_attr_gt_max_freq_mhz.attr, > &dev_attr_gt_min_freq_mhz.attr, >+ &dev_attr_gt_idle_freq_mhz.attr, > &dev_attr_gt_RP0_freq_mhz.attr, > &dev_attr_gt_RP1_freq_mhz.attr, > &dev_attr_gt_RPn_freq_mhz.attr, >@@ -504,6 +563,7 @@ static const struct attribute * const vlv_attrs[] = { > &dev_attr_gt_boost_freq_mhz.attr, > &dev_attr_gt_max_freq_mhz.attr, > &dev_attr_gt_min_freq_mhz.attr, >+ &dev_attr_gt_idle_freq_mhz.attr, > &dev_attr_gt_RP0_freq_mhz.attr, > &dev_attr_gt_RP1_freq_mhz.attr, > &dev_attr_gt_RPn_freq_mhz.attr, >-- >2.19.2 > >_______________________________________________ >Intel-gfx mailing list >Intel-gfx@lists.freedesktop.org >https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Mon, 15 Apr 2019 17:33:30 -0700 Vanshidhar Konda <vanshidhar.r.konda@intel.com> wrote: > On Mon, Apr 15, 2019 at 04:05:26PM -0700, Bob Paauwe wrote: > >There are real-time use cases where having deterministic CPU processes > >can be more important than GPU power/performance. Parking the GPU at a > >specific freqency by setting idle, min and max prohibits the normal > >dynamic GPU frequency switching which can introduce significant PCI-E > >latency. This adds the ability to configure the GPU "idle" frequecy > >using the same method that already exists for minimum and maximum > >frequencies. > > > >In addition, parking the idle frequency may reduce spool up latencies > >on GPU workloads. > > > >Signed-off-by: Bob Paauwe <bob.j.paauwe@intel.com> > >--- > > drivers/gpu/drm/i915/i915_sysfs.c | 60 +++++++++++++++++++++++++++++++ > > 1 file changed, 60 insertions(+) > > > >diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c > >index 41313005af42..62612c23d514 100644 > >--- a/drivers/gpu/drm/i915/i915_sysfs.c > >+++ b/drivers/gpu/drm/i915/i915_sysfs.c > >@@ -454,11 +454,69 @@ static ssize_t gt_min_freq_mhz_store(struct device *kdev, > > return ret ?: count; > > } > > > >+static ssize_t gt_idle_freq_mhz_show(struct device *kdev, struct device_attribute *attr, char *buf) > >+{ > >+ struct drm_i915_private *dev_priv = kdev_minor_to_i915(kdev); > >+ > >+ return snprintf(buf, PAGE_SIZE, "%d\n", > >+ intel_gpu_freq(dev_priv, > >+ dev_priv->gt_pm.rps.idle_freq)); > >+} > >+ > >+static ssize_t gt_idle_freq_mhz_store(struct device *kdev, > >+ struct device_attribute *attr, > >+ const char *buf, size_t count) > >+{ > >+ struct drm_i915_private *dev_priv = kdev_minor_to_i915(kdev); > >+ struct intel_rps *rps = &dev_priv->gt_pm.rps; > >+ intel_wakeref_t wakeref; > >+ u32 val; > > val can probably just be u8. max_freq, min_freq, etc. are only u8 in > struct intel_rps *rps. Using u32 is consistent with all the other _store functions in the file and changing it would also mean changing the kstrtou32 call below. I'd rather this function stay consistent with the min/max/boost frequency functions. Changing to u8 would be a separate change and should be applied to all the similar functions. > > >+ ssize_t ret; > >+ > >+ ret = kstrtou32(buf, 0, &val); > >+ if (ret) > >+ return ret; > >+ > >+ wakeref = intel_runtime_pm_get(dev_priv); > >+ > >+ mutex_lock(&dev_priv->pcu_lock); > >+ > >+ val = intel_freq_opcode(dev_priv, val); > >+ > >+ if (val < rps->min_freq || > >+ val > rps->max_freq || > >+ val > rps->max_freq_softlimit) { > >+ mutex_unlock(&dev_priv->pcu_lock); > >+ intel_runtime_pm_put(dev_priv, wakeref); > >+ return -EINVAL; > >+ } > >+ > >+ rps->idle_freq = val; > >+ > >+ val = clamp_t(int, rps->cur_freq, > >+ rps->idle_freq, > >+ rps->max_freq_softlimit); > > This should probably be clamped to u8 instead of int. Similar to above, this is consistent with the other similar functions. > > Vanshi > > >+ > >+ /* > >+ * If the current freq is at the old idle freq we should > >+ * ajust it to the new idle. Calling *_set_rps will also > >+ * update the interrupt limits and PMINTRMSK if ncessary. > >+ */ > >+ ret = intel_set_rps(dev_priv, val); > >+ > >+ mutex_unlock(&dev_priv->pcu_lock); > >+ > >+ intel_runtime_pm_put(dev_priv, wakeref); > >+ > >+ return ret ?: count; > >+} > >+ > > static DEVICE_ATTR_RO(gt_act_freq_mhz); > > static DEVICE_ATTR_RO(gt_cur_freq_mhz); > > static DEVICE_ATTR_RW(gt_boost_freq_mhz); > > static DEVICE_ATTR_RW(gt_max_freq_mhz); > > static DEVICE_ATTR_RW(gt_min_freq_mhz); > >+static DEVICE_ATTR_RW(gt_idle_freq_mhz); > > > > static DEVICE_ATTR_RO(vlv_rpe_freq_mhz); > > > >@@ -492,6 +550,7 @@ static const struct attribute * const gen6_attrs[] = { > > &dev_attr_gt_boost_freq_mhz.attr, > > &dev_attr_gt_max_freq_mhz.attr, > > &dev_attr_gt_min_freq_mhz.attr, > >+ &dev_attr_gt_idle_freq_mhz.attr, > > &dev_attr_gt_RP0_freq_mhz.attr, > > &dev_attr_gt_RP1_freq_mhz.attr, > > &dev_attr_gt_RPn_freq_mhz.attr, > >@@ -504,6 +563,7 @@ static const struct attribute * const vlv_attrs[] = { > > &dev_attr_gt_boost_freq_mhz.attr, > > &dev_attr_gt_max_freq_mhz.attr, > > &dev_attr_gt_min_freq_mhz.attr, > >+ &dev_attr_gt_idle_freq_mhz.attr, > > &dev_attr_gt_RP0_freq_mhz.attr, > > &dev_attr_gt_RP1_freq_mhz.attr, > > &dev_attr_gt_RPn_freq_mhz.attr, > >-- > >2.19.2 > > > >_______________________________________________ > >Intel-gfx mailing list > >Intel-gfx@lists.freedesktop.org > >https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Tue, Apr 16, 2019 at 08:30:22AM -0700, Bob Paauwe wrote: >On Mon, 15 Apr 2019 17:33:30 -0700 >Vanshidhar Konda <vanshidhar.r.konda@intel.com> wrote: > >> On Mon, Apr 15, 2019 at 04:05:26PM -0700, Bob Paauwe wrote: >> >There are real-time use cases where having deterministic CPU processes >> >can be more important than GPU power/performance. Parking the GPU at a >> >specific freqency by setting idle, min and max prohibits the normal >> >dynamic GPU frequency switching which can introduce significant PCI-E >> >latency. This adds the ability to configure the GPU "idle" frequecy >> >using the same method that already exists for minimum and maximum >> >frequencies. >> > >> >In addition, parking the idle frequency may reduce spool up latencies >> >on GPU workloads. >> > >> >Signed-off-by: Bob Paauwe <bob.j.paauwe@intel.com> >> >--- >> > drivers/gpu/drm/i915/i915_sysfs.c | 60 +++++++++++++++++++++++++++++++ >> > 1 file changed, 60 insertions(+) >> > >> >diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c >> >index 41313005af42..62612c23d514 100644 >> >--- a/drivers/gpu/drm/i915/i915_sysfs.c >> >+++ b/drivers/gpu/drm/i915/i915_sysfs.c >> >@@ -454,11 +454,69 @@ static ssize_t gt_min_freq_mhz_store(struct device *kdev, >> > return ret ?: count; >> > } >> > >> >+static ssize_t gt_idle_freq_mhz_show(struct device *kdev, struct device_attribute *attr, char *buf) >> >+{ >> >+ struct drm_i915_private *dev_priv = kdev_minor_to_i915(kdev); >> >+ >> >+ return snprintf(buf, PAGE_SIZE, "%d\n", >> >+ intel_gpu_freq(dev_priv, >> >+ dev_priv->gt_pm.rps.idle_freq)); >> >+} >> >+ >> >+static ssize_t gt_idle_freq_mhz_store(struct device *kdev, >> >+ struct device_attribute *attr, >> >+ const char *buf, size_t count) >> >+{ >> >+ struct drm_i915_private *dev_priv = kdev_minor_to_i915(kdev); >> >+ struct intel_rps *rps = &dev_priv->gt_pm.rps; >> >+ intel_wakeref_t wakeref; >> >+ u32 val; >> >> val can probably just be u8. max_freq, min_freq, etc. are only u8 in >> struct intel_rps *rps. > >Using u32 is consistent with all the other _store functions in the file >and changing it would also mean changing the kstrtou32 call below. I'd >rather this function stay consistent with the min/max/boost frequency >functions. Changing to u8 would be a separate change and should be >applied to all the similar functions. > Thanks for pointing that out. I recently joined the team, so not sure if you would like someone else to review as well. Reviewed-by: Vanshidhar Konda <vanshidhar.r.konda@intel.com> >> >> >+ ssize_t ret; >> >+ >> >+ ret = kstrtou32(buf, 0, &val); >> >+ if (ret) >> >+ return ret; >> >+ >> >+ wakeref = intel_runtime_pm_get(dev_priv); >> >+ >> >+ mutex_lock(&dev_priv->pcu_lock); >> >+ >> >+ val = intel_freq_opcode(dev_priv, val); >> >+ >> >+ if (val < rps->min_freq || >> >+ val > rps->max_freq || >> >+ val > rps->max_freq_softlimit) { >> >+ mutex_unlock(&dev_priv->pcu_lock); >> >+ intel_runtime_pm_put(dev_priv, wakeref); >> >+ return -EINVAL; >> >+ } >> >+ >> >+ rps->idle_freq = val; >> >+ >> >+ val = clamp_t(int, rps->cur_freq, >> >+ rps->idle_freq, >> >+ rps->max_freq_softlimit); >> >> This should probably be clamped to u8 instead of int. > >Similar to above, this is consistent with the other similar functions. > >> >> Vanshi >> >> >+ >> >+ /* >> >+ * If the current freq is at the old idle freq we should >> >+ * ajust it to the new idle. Calling *_set_rps will also >> >+ * update the interrupt limits and PMINTRMSK if ncessary. >> >+ */ >> >+ ret = intel_set_rps(dev_priv, val); >> >+ >> >+ mutex_unlock(&dev_priv->pcu_lock); >> >+ >> >+ intel_runtime_pm_put(dev_priv, wakeref); >> >+ >> >+ return ret ?: count; >> >+} >> >+ >> > static DEVICE_ATTR_RO(gt_act_freq_mhz); >> > static DEVICE_ATTR_RO(gt_cur_freq_mhz); >> > static DEVICE_ATTR_RW(gt_boost_freq_mhz); >> > static DEVICE_ATTR_RW(gt_max_freq_mhz); >> > static DEVICE_ATTR_RW(gt_min_freq_mhz); >> >+static DEVICE_ATTR_RW(gt_idle_freq_mhz); >> > >> > static DEVICE_ATTR_RO(vlv_rpe_freq_mhz); >> > >> >@@ -492,6 +550,7 @@ static const struct attribute * const gen6_attrs[] = { >> > &dev_attr_gt_boost_freq_mhz.attr, >> > &dev_attr_gt_max_freq_mhz.attr, >> > &dev_attr_gt_min_freq_mhz.attr, >> >+ &dev_attr_gt_idle_freq_mhz.attr, >> > &dev_attr_gt_RP0_freq_mhz.attr, >> > &dev_attr_gt_RP1_freq_mhz.attr, >> > &dev_attr_gt_RPn_freq_mhz.attr, >> >@@ -504,6 +563,7 @@ static const struct attribute * const vlv_attrs[] = { >> > &dev_attr_gt_boost_freq_mhz.attr, >> > &dev_attr_gt_max_freq_mhz.attr, >> > &dev_attr_gt_min_freq_mhz.attr, >> >+ &dev_attr_gt_idle_freq_mhz.attr, >> > &dev_attr_gt_RP0_freq_mhz.attr, >> > &dev_attr_gt_RP1_freq_mhz.attr, >> > &dev_attr_gt_RPn_freq_mhz.attr, >> >-- >> >2.19.2 >> > >> >_______________________________________________ >> >Intel-gfx mailing list >> >Intel-gfx@lists.freedesktop.org >> >https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > >-- >-- >Bob Paauwe >Bob.J.Paauwe@intel.com >IOTG / PED Software Organization >Intel Corp. Folsom, CA >(916) 356-6193 >
Quoting Bob Paauwe (2019-04-16 00:05:26) > There are real-time use cases where having deterministic CPU processes > can be more important than GPU power/performance. Parking the GPU at a > specific freqency by setting idle, min and max prohibits the normal > dynamic GPU frequency switching which can introduce significant PCI-E > latency. This adds the ability to configure the GPU "idle" frequecy > using the same method that already exists for minimum and maximum > frequencies. What exactly is the problem? We never use idle frequency while active, always restarting at max(cur, rpe). So for the simple minded among us, where is the igt demonstrating the issue? -Chris
On Tue, 16 Apr 2019 16:56:26 +0100 Chris Wilson <chris@chris-wilson.co.uk> wrote: > Quoting Bob Paauwe (2019-04-16 00:05:26) > > There are real-time use cases where having deterministic CPU processes > > can be more important than GPU power/performance. Parking the GPU at a > > specific freqency by setting idle, min and max prohibits the normal > > dynamic GPU frequency switching which can introduce significant PCI-E > > latency. This adds the ability to configure the GPU "idle" frequecy > > using the same method that already exists for minimum and maximum > > frequencies. > > What exactly is the problem? We never use idle frequency while active, > always restarting at max(cur, rpe). So for the simple minded among us, > where is the igt demonstrating the issue? > -Chris To follow up and close this. When I requested more details on the use case and data for this request, I was informed that a different solution is being pursued and this patch is no longer needed. Thanks for the reviews and comments. Bob
diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c index 41313005af42..62612c23d514 100644 --- a/drivers/gpu/drm/i915/i915_sysfs.c +++ b/drivers/gpu/drm/i915/i915_sysfs.c @@ -454,11 +454,69 @@ static ssize_t gt_min_freq_mhz_store(struct device *kdev, return ret ?: count; } +static ssize_t gt_idle_freq_mhz_show(struct device *kdev, struct device_attribute *attr, char *buf) +{ + struct drm_i915_private *dev_priv = kdev_minor_to_i915(kdev); + + return snprintf(buf, PAGE_SIZE, "%d\n", + intel_gpu_freq(dev_priv, + dev_priv->gt_pm.rps.idle_freq)); +} + +static ssize_t gt_idle_freq_mhz_store(struct device *kdev, + struct device_attribute *attr, + const char *buf, size_t count) +{ + struct drm_i915_private *dev_priv = kdev_minor_to_i915(kdev); + struct intel_rps *rps = &dev_priv->gt_pm.rps; + intel_wakeref_t wakeref; + u32 val; + ssize_t ret; + + ret = kstrtou32(buf, 0, &val); + if (ret) + return ret; + + wakeref = intel_runtime_pm_get(dev_priv); + + mutex_lock(&dev_priv->pcu_lock); + + val = intel_freq_opcode(dev_priv, val); + + if (val < rps->min_freq || + val > rps->max_freq || + val > rps->max_freq_softlimit) { + mutex_unlock(&dev_priv->pcu_lock); + intel_runtime_pm_put(dev_priv, wakeref); + return -EINVAL; + } + + rps->idle_freq = val; + + val = clamp_t(int, rps->cur_freq, + rps->idle_freq, + rps->max_freq_softlimit); + + /* + * If the current freq is at the old idle freq we should + * ajust it to the new idle. Calling *_set_rps will also + * update the interrupt limits and PMINTRMSK if ncessary. + */ + ret = intel_set_rps(dev_priv, val); + + mutex_unlock(&dev_priv->pcu_lock); + + intel_runtime_pm_put(dev_priv, wakeref); + + return ret ?: count; +} + static DEVICE_ATTR_RO(gt_act_freq_mhz); static DEVICE_ATTR_RO(gt_cur_freq_mhz); static DEVICE_ATTR_RW(gt_boost_freq_mhz); static DEVICE_ATTR_RW(gt_max_freq_mhz); static DEVICE_ATTR_RW(gt_min_freq_mhz); +static DEVICE_ATTR_RW(gt_idle_freq_mhz); static DEVICE_ATTR_RO(vlv_rpe_freq_mhz); @@ -492,6 +550,7 @@ static const struct attribute * const gen6_attrs[] = { &dev_attr_gt_boost_freq_mhz.attr, &dev_attr_gt_max_freq_mhz.attr, &dev_attr_gt_min_freq_mhz.attr, + &dev_attr_gt_idle_freq_mhz.attr, &dev_attr_gt_RP0_freq_mhz.attr, &dev_attr_gt_RP1_freq_mhz.attr, &dev_attr_gt_RPn_freq_mhz.attr, @@ -504,6 +563,7 @@ static const struct attribute * const vlv_attrs[] = { &dev_attr_gt_boost_freq_mhz.attr, &dev_attr_gt_max_freq_mhz.attr, &dev_attr_gt_min_freq_mhz.attr, + &dev_attr_gt_idle_freq_mhz.attr, &dev_attr_gt_RP0_freq_mhz.attr, &dev_attr_gt_RP1_freq_mhz.attr, &dev_attr_gt_RPn_freq_mhz.attr,
There are real-time use cases where having deterministic CPU processes can be more important than GPU power/performance. Parking the GPU at a specific freqency by setting idle, min and max prohibits the normal dynamic GPU frequency switching which can introduce significant PCI-E latency. This adds the ability to configure the GPU "idle" frequecy using the same method that already exists for minimum and maximum frequencies. In addition, parking the idle frequency may reduce spool up latencies on GPU workloads. Signed-off-by: Bob Paauwe <bob.j.paauwe@intel.com> --- drivers/gpu/drm/i915/i915_sysfs.c | 60 +++++++++++++++++++++++++++++++ 1 file changed, 60 insertions(+)