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.
>
> >+ 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
