All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
To: Imre Deak <imre.deak@intel.com>
Cc: intel-gfx@lists.freedesktop.org, "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:16:18 +0200	[thread overview]
Message-ID: <20160316181618.GT4329@intel.com> (raw)
In-Reply-To: <1458151623.4473.52.camel@intel.com>

On Wed, Mar 16, 2016 at 08:07:03PM +0200, Imre Deak wrote:
> 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.

Yeah, at least that would get rid of the mutex + rpm. And I suppose I
should look at the debugfs side too.

> 
> > ---
> >  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();
> >  

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

  reply	other threads:[~2016-03-16 18:16 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
2016-03-16 18:16     ` Ville Syrjälä [this message]
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=20160316181618.GT4329@intel.com \
    --to=ville.syrjala@linux.intel.com \
    --cc=Tom.O'Rourke@intel.com \
    --cc=imre.deak@intel.com \
    --cc=intel-gfx@lists.freedesktop.org \
    /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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.