Re: [PATCH] drm/i915: Configurable GT idle frequency

[Bob Paauwe <bob.j.paauwe@intel.com>]

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  



-- 
--
Bob Paauwe                  
Bob.J.Paauwe@intel.com
IOTG / PED Software Organization
Intel Corp.  Folsom, CA
(916) 356-6193    

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx