[Intel-gfx] [PATCH 08/12] drm/i915: ensure the HW is powered when accessing the CRC HW block

[Daniel Vetter]

On Wed, Feb 17, 2016 at 02:20:49PM +0200, Imre Deak wrote:
> On ti, 2016-02-16 at 17:02 +0100, Daniel Vetter wrote:
> > On Fri, Feb 12, 2016 at 06:55:17PM +0200, Imre Deak wrote:
> > > The assumption when adding the intel_display_power_is_enabled()
> > > checks
> > > was that if it returns success the power can't be turned off
> > > afterwards
> > > during the HW access, which is guaranteed by modeset locks. This
> > > isn't
> > > always true, so make sure we hold a dedicated reference for the
> > > time of
> > > the access.
> > > 
> > > Signed-off-by: Imre Deak <imre.deak at intel.com>
> > 
> > subject needs to be fixed to mention that this is for all the stuff
> > in
> > i915_debugfs.c.
> 
> Ah, yea i915_interrupt_info() is also changed, will updated that in the
> log.

Ah right makes sense. I thought I counted more to convert in
intel_display.c than what was in your patches, plus the error capture
stuff. And not converting error capture is indeed what we need to do ...
-Daniel

> 
> > With that fixed for patches 4-8:
> > 
> > Reviewed-by: Daniel Vetter <daniel.vetter at ffwll.ch>
> > 
> > 
> > > ---
> > >  drivers/gpu/drm/i915/i915_debugfs.c | 28 +++++++++++++++++++++--
> > > -----
> > >  1 file changed, 21 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> > > b/drivers/gpu/drm/i915/i915_debugfs.c
> > > index ec0c2a05e..9e19cf0 100644
> > > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > > @@ -825,8 +825,11 @@ static int i915_interrupt_info(struct seq_file
> > > *m, void *data)
> > >  		}
> > >  
> > >  		for_each_pipe(dev_priv, pipe) {
> > > -			if
> > > (!intel_display_power_is_enabled(dev_priv,
> > > -						POWER_DOMAIN_PIPE(
> > > pipe))) {
> > > +			enum intel_display_power_domain
> > > power_domain;
> > > +
> > > +			power_domain = POWER_DOMAIN_PIPE(pipe);
> > > +			if
> > > (!intel_display_power_get_if_enabled(dev_priv,
> > > +								po
> > > wer_domain)) {
> > >  				seq_printf(m, "Pipe %c power
> > > disabled\n",
> > >  					   pipe_name(pipe));
> > >  				continue;
> > > @@ -840,6 +843,8 @@ static int i915_interrupt_info(struct seq_file
> > > *m, void *data)
> > >  			seq_printf(m, "Pipe %c IER:\t%08x\n",
> > >  				   pipe_name(pipe),
> > >  				   I915_READ(GEN8_DE_PIPE_IER(pipe
> > > )));
> > > +
> > > +			intel_display_power_put(dev_priv,
> > > power_domain);
> > >  		}
> > >  
> > >  		seq_printf(m, "Display Engine port interrupt
> > > mask:\t%08x\n",
> > > @@ -4004,6 +4009,7 @@ static int pipe_crc_set_source(struct
> > > drm_device *dev, enum pipe pipe,
> > >  	struct intel_pipe_crc *pipe_crc = &dev_priv-
> > > >pipe_crc[pipe];
> > >  	struct intel_crtc *crtc =
> > > to_intel_crtc(intel_get_crtc_for_pipe(dev,
> > >  									
> > > pipe));
> > > +	enum intel_display_power_domain power_domain;
> > >  	u32 val = 0; /* shut up gcc */
> > >  	int ret;
> > >  
> > > @@ -4014,7 +4020,8 @@ static int pipe_crc_set_source(struct
> > > drm_device *dev, enum pipe pipe,
> > >  	if (pipe_crc->source && source)
> > >  		return -EINVAL;
> > >  
> > > -	if (!intel_display_power_is_enabled(dev_priv,
> > > POWER_DOMAIN_PIPE(pipe))) {
> > > +	power_domain = POWER_DOMAIN_PIPE(pipe);
> > > +	if (!intel_display_power_get_if_enabled(dev_priv,
> > > power_domain)) {
> > >  		DRM_DEBUG_KMS("Trying to capture CRC while pipe is
> > > off\n");
> > >  		return -EIO;
> > >  	}
> > > @@ -4031,7 +4038,7 @@ static int pipe_crc_set_source(struct
> > > drm_device *dev, enum pipe pipe,
> > >  		ret = ivb_pipe_crc_ctl_reg(dev, pipe, &source,
> > > &val);
> > >  
> > >  	if (ret != 0)
> > > -		return ret;
> > > +		goto out;
> > >  
> > >  	/* none -> real source transition */
> > >  	if (source) {
> > > @@ -4043,8 +4050,10 @@ static int pipe_crc_set_source(struct
> > > drm_device *dev, enum pipe pipe,
> > >  		entries = kcalloc(INTEL_PIPE_CRC_ENTRIES_NR,
> > >  				  sizeof(pipe_crc->entries[0]),
> > >  				  GFP_KERNEL);
> > > -		if (!entries)
> > > -			return -ENOMEM;
> > > +		if (!entries) {
> > > +			ret = -ENOMEM;
> > > +			goto out;
> > > +		}
> > >  
> > >  		/*
> > >  		 * When IPS gets enabled, the pipe CRC changes.
> > > Since IPS gets
> > > @@ -4100,7 +4109,12 @@ static int pipe_crc_set_source(struct
> > > drm_device *dev, enum pipe pipe,
> > >  		hsw_enable_ips(crtc);
> > >  	}
> > >  
> > > -	return 0;
> > > +	ret = 0;
> > > +
> > > +out:
> > > +	intel_display_power_put(dev_priv, power_domain);
> > > +
> > > +	return ret;
> > >  }
> > >  
> > >  /*
> > > -- 
> > > 2.5.0
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx at lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch