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@xxxxxxxxx> 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@xxxxxxxxx> >--- > 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@xxxxxxxxx>
>+ 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@xxxxxxxxxxxxxxxxxxxxx >https://lists.freedesktop.org/mailman/listinfo/intel-gfx-- -- Bob Paauwe Bob.J.Paauwe@xxxxxxxxx IOTG / PED Software Organization Intel Corp. Folsom, CA (916) 356-6193
_______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
