Re: [PATCH v2 5/15] drm/i915: take init power domain for sysfs/debugfs entries where needed

[Imre Deak <imre.deak@intel.com>]

On Wed, 2014-04-09 at 19:06 +0300, Ville Syrjälä wrote:
> On Wed, Apr 09, 2014 at 05:21:47PM +0300, Imre Deak wrote:
> > On Wed, 2014-04-09 at 16:15 +0200, Daniel Vetter wrote:
> > > On Tue, Apr 08, 2014 at 10:34:41PM +0300, Imre Deak wrote:
> > > > v2:
> > > > - make it actually compile, I managed to send the wrong version as v1
> > > >   somehow
> > > > 
> > > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > 
> > > Usually if we just want the device to be out of D3 we just use
> > > intel_runtime_pm_get/put. On a quick look it seems like that's enough for
> > > these places here, too?
> > 
> > For drpc yes, but for the others we need the DPIO power wells up too. I
> > got for all VLV case the power well ref for simplicity, I can change it
> > to an RPM ref for drpc, if that's ok.
> 
> The dpio_info file would need DPIO wells up. However I think we should
> just kill that file. It just dumps a small subset of the registers w/o
> decoding anything, and we can get more extensive dumps w/ igt, so I
> don't see the point of this file.

Agreed, there are at least the intel_dpio_* tools and other similar ones
for reading registers in other address spaces, so this file looks a bit
arbitrary. I can remove it if no one objects.

> The error handler should already be able to deal with the wells being
> down no?

i915_handle_error can be called from IRQ context, so it can't get power
domain refs. On an interrupt error detection path this isn't a problem
since there the HW is in D0, but here that's not guaranteed.

I took the separate power domain refs in the error _capture_ function
since that's a deferred work. There again D0 is not guaranteed, for
example we could reset the HW, schedule the capture work, drop the last
RPM ref and only then start running the capture function.

> The gt_cutr_freq file just talks to the punit so no need for any power
> wells either AFAICS.

Hm right, this one needs only D0 as drpc. I'll change this too.

--Imre 

> 
> > 
> > --Imre
> > 
> > > -Daniel
> > > 
> > > > ---
> > > >  drivers/gpu/drm/i915/i915_debugfs.c | 18 ++++++++++++++++--
> > > >  drivers/gpu/drm/i915/i915_sysfs.c   |  4 ++++
> > > >  2 files changed, 20 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> > > > index 02f1b39..ac91a44 100644
> > > > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > > > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > > > @@ -1239,9 +1239,13 @@ static int vlv_drpc_info(struct seq_file *m)
> > > >  	u32 rpmodectl1, rcctl1;
> > > >  	unsigned fw_rendercount = 0, fw_mediacount = 0;
> > > >  
> > > > +	intel_display_power_get(dev_priv, POWER_DOMAIN_INIT);
> > > > +
> > > >  	rpmodectl1 = I915_READ(GEN6_RP_CONTROL);
> > > >  	rcctl1 = I915_READ(GEN6_RC_CONTROL);
> > > >  
> > > > +	intel_display_power_put(dev_priv, POWER_DOMAIN_INIT);
> > > > +
> > > >  	seq_printf(m, "Video Turbo Mode: %s\n",
> > > >  		   yesno(rpmodectl1 & GEN6_RP_MEDIA_TURBO));
> > > >  	seq_printf(m, "Turbo enabled: %s\n",
> > > > @@ -1916,9 +1920,11 @@ static int i915_dpio_info(struct seq_file *m, void *data)
> > > >  		return 0;
> > > >  	}
> > > >  
> > > > +	intel_display_power_get(dev_priv, POWER_DOMAIN_INIT);
> > > > +
> > > >  	ret = mutex_lock_interruptible(&dev_priv->dpio_lock);
> > > >  	if (ret)
> > > > -		return ret;
> > > > +		goto out;
> > > >  
> > > >  	seq_printf(m, "DPIO_CTL: 0x%08x\n", I915_READ(DPIO_CTL));
> > > >  
> > > > @@ -1946,8 +1952,10 @@ static int i915_dpio_info(struct seq_file *m, void *data)
> > > >  		   vlv_dpio_read(dev_priv, PIPE_A, VLV_CMN_DW0));
> > > >  
> > > >  	mutex_unlock(&dev_priv->dpio_lock);
> > > > +out:
> > > > +	intel_display_power_put(dev_priv, POWER_DOMAIN_INIT);
> > > >  
> > > > -	return 0;
> > > > +	return ret;
> > > >  }
> > > >  
> > > >  static int i915_llc(struct seq_file *m, void *data)
> > > > @@ -3304,9 +3312,15 @@ static int
> > > >  i915_wedged_set(void *data, u64 val)
> > > >  {
> > > >  	struct drm_device *dev = data;
> > > > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > > > +
> > > > +	intel_display_power_get(dev_priv, POWER_DOMAIN_INIT);
> > > >  
> > > >  	i915_handle_error(dev, val,
> > > >  			  "Manually setting wedged to %llu", val);
> > > > +
> > > > +	intel_display_power_put(dev_priv, POWER_DOMAIN_INIT);
> > > > +
> > > >  	return 0;
> > > >  }
> > > >  
> > > > diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c
> > > > index 9c57029..552c4ed 100644
> > > > --- a/drivers/gpu/drm/i915/i915_sysfs.c
> > > > +++ b/drivers/gpu/drm/i915/i915_sysfs.c
> > > > @@ -263,6 +263,8 @@ static ssize_t gt_cur_freq_mhz_show(struct device *kdev,
> > > >  
> > > >  	flush_delayed_work(&dev_priv->rps.delayed_resume_work);
> > > >  
> > > > +	intel_display_power_get(dev_priv, POWER_DOMAIN_INIT);
> > > > +
> > > >  	mutex_lock(&dev_priv->rps.hw_lock);
> > > >  	if (IS_VALLEYVIEW(dev_priv->dev)) {
> > > >  		u32 freq;
> > > > @@ -273,6 +275,8 @@ static ssize_t gt_cur_freq_mhz_show(struct device *kdev,
> > > >  	}
> > > >  	mutex_unlock(&dev_priv->rps.hw_lock);
> > > >  
> > > > +	intel_display_power_put(dev_priv, POWER_DOMAIN_INIT);
> > > > +
> > > >  	return snprintf(buf, PAGE_SIZE, "%d\n", ret);
> > > >  }
> > > >  
> > > > -- 
> > > > 1.8.3.2
> > > > 
> > > > _______________________________________________
> > > > Intel-gfx mailing list
> > > > Intel-gfx@lists.freedesktop.org
> > > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > > 
> > 
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 


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