From: Imre Deak <imre.deak@intel.com>
To: ville.syrjala@linux.intel.com, intel-gfx@lists.freedesktop.org
Cc: "O'Rourke, Tom" <Tom.O'Rourke@intel.com>
Subject: Re: [PATCH 3/3] drm/i915: Drop locking/rpm resume/flush_delayed_work for cur/min/max freq sysfs read
Date: Wed, 16 Mar 2016 20:07:03 +0200 [thread overview]
Message-ID: <1458151623.4473.52.camel@intel.com> (raw)
In-Reply-To: <1457120584-26080-4-git-send-email-ville.syrjala@linux.intel.com>
On Fri, 2016-03-04 at 21:43 +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> cur_freq, min/max_freq_softlimit, efficient_freq are just single
> values
> stored under dev_priv.rps, so there's no real point in locking,
> resuming
> the devices and flushing the delayed resume work just to print them
> out.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
This makes things clearer, so would it make sense to still keep the
flush wq and leave out patch 4 until somebody benchmarks things? That
would address Tom's concern as well.
> ---
> drivers/gpu/drm/i915/i915_sysfs.c | 84 ++++++-----------------------
> ----------
> 1 file changed, 13 insertions(+), 71 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_sysfs.c
> b/drivers/gpu/drm/i915/i915_sysfs.c
> index 2d576b7ff299..9e39d7a18fdb 100644
> --- a/drivers/gpu/drm/i915/i915_sysfs.c
> +++ b/drivers/gpu/drm/i915/i915_sysfs.c
> @@ -305,55 +305,6 @@ static ssize_t gt_act_freq_mhz_show(struct
> device *kdev,
> return snprintf(buf, PAGE_SIZE, "%d\n", ret);
> }
>
> -static ssize_t gt_cur_freq_mhz_show(struct device *kdev,
> - struct device_attribute *attr,
> char *buf)
> -{
> - struct drm_minor *minor = dev_to_drm_minor(kdev);
> - struct drm_device *dev = minor->dev;
> - struct drm_i915_private *dev_priv = dev->dev_private;
> - int ret;
> -
> - flush_delayed_work(&dev_priv->rps.delayed_resume_work);
> -
> - intel_runtime_pm_get(dev_priv);
> -
> - mutex_lock(&dev_priv->rps.hw_lock);
> - ret = intel_gpu_freq(dev_priv, dev_priv->rps.cur_freq);
> - mutex_unlock(&dev_priv->rps.hw_lock);
> -
> - intel_runtime_pm_put(dev_priv);
> -
> - return snprintf(buf, PAGE_SIZE, "%d\n", ret);
> -}
> -
> -static ssize_t vlv_rpe_freq_mhz_show(struct device *kdev,
> - struct device_attribute *attr,
> char *buf)
> -{
> - struct drm_minor *minor = dev_to_drm_minor(kdev);
> - struct drm_device *dev = minor->dev;
> - struct drm_i915_private *dev_priv = dev->dev_private;
> -
> - return snprintf(buf, PAGE_SIZE,
> - "%d\n",
> - intel_gpu_freq(dev_priv, dev_priv-
> >rps.efficient_freq));
> -}
> -
> -static ssize_t gt_max_freq_mhz_show(struct device *kdev, struct
> device_attribute *attr, char *buf)
> -{
> - struct drm_minor *minor = dev_to_drm_minor(kdev);
> - struct drm_device *dev = minor->dev;
> - struct drm_i915_private *dev_priv = dev->dev_private;
> - int ret;
> -
> - flush_delayed_work(&dev_priv->rps.delayed_resume_work);
> -
> - mutex_lock(&dev_priv->rps.hw_lock);
> - ret = intel_gpu_freq(dev_priv, dev_priv-
> >rps.max_freq_softlimit);
> - mutex_unlock(&dev_priv->rps.hw_lock);
> -
> - return snprintf(buf, PAGE_SIZE, "%d\n", ret);
> -}
> -
> static ssize_t gt_max_freq_mhz_store(struct device *kdev,
> struct device_attribute *attr,
> const char *buf, size_t count)
> @@ -406,22 +357,6 @@ static ssize_t gt_max_freq_mhz_store(struct
> device *kdev,
> return count;
> }
>
> -static ssize_t gt_min_freq_mhz_show(struct device *kdev, struct
> device_attribute *attr, char *buf)
> -{
> - struct drm_minor *minor = dev_to_drm_minor(kdev);
> - struct drm_device *dev = minor->dev;
> - struct drm_i915_private *dev_priv = dev->dev_private;
> - int ret;
> -
> - flush_delayed_work(&dev_priv->rps.delayed_resume_work);
> -
> - mutex_lock(&dev_priv->rps.hw_lock);
> - ret = intel_gpu_freq(dev_priv, dev_priv-
> >rps.min_freq_softlimit);
> - mutex_unlock(&dev_priv->rps.hw_lock);
> -
> - return snprintf(buf, PAGE_SIZE, "%d\n", ret);
> -}
> -
> static ssize_t gt_min_freq_mhz_store(struct device *kdev,
> struct device_attribute *attr,
> const char *buf, size_t count)
> @@ -472,16 +407,15 @@ static ssize_t gt_min_freq_mhz_store(struct
> device *kdev,
> }
>
> static DEVICE_ATTR(gt_act_freq_mhz, S_IRUGO, gt_act_freq_mhz_show,
> NULL);
> -static DEVICE_ATTR(gt_cur_freq_mhz, S_IRUGO, gt_cur_freq_mhz_show,
> NULL);
> -static DEVICE_ATTR(gt_max_freq_mhz, S_IRUGO | S_IWUSR,
> gt_max_freq_mhz_show, gt_max_freq_mhz_store);
> -static DEVICE_ATTR(gt_min_freq_mhz, S_IRUGO | S_IWUSR,
> gt_min_freq_mhz_show, gt_min_freq_mhz_store);
> -
> -static DEVICE_ATTR(vlv_rpe_freq_mhz, S_IRUGO, vlv_rpe_freq_mhz_show,
> NULL);
>
> static ssize_t gt_rp_mhz_show(struct device *kdev, struct
> device_attribute *attr, char *buf);
> +static DEVICE_ATTR(gt_cur_freq_mhz, S_IRUGO, gt_rp_mhz_show, NULL);
> +static DEVICE_ATTR(gt_max_freq_mhz, S_IRUGO | S_IWUSR,
> gt_rp_mhz_show, gt_max_freq_mhz_store);
> +static DEVICE_ATTR(gt_min_freq_mhz, S_IRUGO | S_IWUSR,
> gt_rp_mhz_show, gt_min_freq_mhz_store);
> static DEVICE_ATTR(gt_RP0_freq_mhz, S_IRUGO, gt_rp_mhz_show, NULL);
> static DEVICE_ATTR(gt_RP1_freq_mhz, S_IRUGO, gt_rp_mhz_show, NULL);
> static DEVICE_ATTR(gt_RPn_freq_mhz, S_IRUGO, gt_rp_mhz_show, NULL);
> +static DEVICE_ATTR(vlv_rpe_freq_mhz, S_IRUGO, gt_rp_mhz_show, NULL);
>
> /* For now we have a static number of RP states */
> static ssize_t gt_rp_mhz_show(struct device *kdev, struct
> device_attribute *attr, char *buf)
> @@ -491,12 +425,20 @@ static ssize_t gt_rp_mhz_show(struct device
> *kdev, struct device_attribute *attr
> struct drm_i915_private *dev_priv = dev->dev_private;
> u32 val;
>
> - if (attr == &dev_attr_gt_RP0_freq_mhz)
> + if (attr == &dev_attr_gt_cur_freq_mhz)
> + val = intel_gpu_freq(dev_priv, dev_priv-
> >rps.cur_freq);
> + else if (attr == &dev_attr_gt_max_freq_mhz)
> + val = intel_gpu_freq(dev_priv, dev_priv-
> >rps.max_freq_softlimit);
> + else if (attr == &dev_attr_gt_min_freq_mhz)
> + val = intel_gpu_freq(dev_priv, dev_priv-
> >rps.min_freq_softlimit);
> + else if (attr == &dev_attr_gt_RP0_freq_mhz)
> val = intel_gpu_freq(dev_priv, dev_priv-
> >rps.rp0_freq);
> else if (attr == &dev_attr_gt_RP1_freq_mhz)
> val = intel_gpu_freq(dev_priv, dev_priv-
> >rps.rp1_freq);
> else if (attr == &dev_attr_gt_RPn_freq_mhz)
> val = intel_gpu_freq(dev_priv, dev_priv-
> >rps.min_freq);
> + else if (attr == &dev_attr_vlv_rpe_freq_mhz)
> + val = intel_gpu_freq(dev_priv, dev_priv-
> >rps.efficient_freq);
> else
> BUG();
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
next prev parent reply other threads:[~2016-03-16 18:07 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-03-04 19:43 [PATCH 0/3] drm/i915: Some GT freq stuff ville.syrjala
2016-03-04 19:43 ` [PATCH 1/3] drm/i915: Use GPLL ref clock to calculate GPU freqs on VLV/CHV ville.syrjala
2016-03-16 17:17 ` Imre Deak
2016-03-16 17:47 ` Ville Syrjälä
2016-03-16 17:51 ` Imre Deak
2016-04-05 19:09 ` Ville Syrjälä
2016-03-04 19:43 ` [PATCH 2/3] drm/i915: Set GPU freq to idle_freq initially ville.syrjala
2016-03-16 17:56 ` Imre Deak
2016-03-16 18:05 ` Chris Wilson
2016-03-04 19:43 ` [PATCH 3/3] drm/i915: Drop locking/rpm resume/flush_delayed_work for cur/min/max freq sysfs read ville.syrjala
2016-03-16 18:07 ` Imre Deak [this message]
2016-03-16 18:16 ` Ville Syrjälä
2016-03-07 10:34 ` ✗ Fi.CI.BAT: failure for drm/i915: Some GT freq stuff Patchwork
2016-03-07 15:07 ` Ville Syrjälä
2016-03-07 17:57 ` Ville Syrjälä
2016-03-07 18:57 ` [PATCH 4/3] drm/i915: Read RPS frequencies earlier on non-VLV/CHV ville.syrjala
2016-03-08 0:49 ` O'Rourke, Tom
2016-03-08 13:34 ` Ville Syrjälä
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1458151623.4473.52.camel@intel.com \
--to=imre.deak@intel.com \
--cc=Tom.O'Rourke@intel.com \
--cc=intel-gfx@lists.freedesktop.org \
--cc=ville.syrjala@linux.intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox