[Intel-gfx] [PATCH] drm/i915: Configurable GT idle frequency

[Vanshidhar Konda]

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