Re: [PATCH 1/2] drm/i915/bdw: Implement a basic PM interrupt handler

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, Apr 14, 2014 at 10:55:53PM +0300, Ville Syrjälä wrote:
> On Mon, Apr 14, 2014 at 10:41:14PM +0530, deepak.s@xxxxxxxxx wrote:
> > From: Ben Widawsky <benjamin.widawsky@xxxxxxxxx>
> > 
> > Almost all of it is reusable from the existing code. The primary
> > difference is we need to do even less in the interrupt handler, since
> > interrupts are not shared in the same way.
> > 
> > The patch is mostly a copy-paste of the existing snb+ code, with updates
> > to the relevant parts requiring changes to the interrupt handling. As
> > such it /should/ be relatively trivial. It's highly likely that I missed
> > some places where I need a gen8 version of the PM interrupts, but it has
> > become invisible to me by now.
> > 
> > This patch could probably be split into adding the new functions,
> > followed by actually handling the interrupts. Since the code is
> > currently disabled (and broken) I think the patch stands better by
> > itself.
> > 
> > v2: Move the commit about not touching the ringbuffer interrupt to the
> > snb_* function where it belongs (Rodrigo)
> > 
> > v3: Rebased on Paulo's runtime PM changes
> > 
> > v4: Not well validated, but rebase on
> > commit 730488b2eddded4497f63f70867b1256cd9e117c
> > Author: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx>
> > Date:   Fri Mar 7 20:12:32 2014 -0300
> > 
> >     drm/i915: kill dev_priv->pm.regsave
> > 
> > v5: Rebased on latest code base. (Deepak)
> > 
> > Signed-off-by: Ben Widawsky <ben@xxxxxxxxxxxx>
> > 
> > Conflicts:
> > 	drivers/gpu/drm/i915/i915_irq.c
> 
> IIRC Daniel doesn't like these conflict markers. So should be dropped.
> 

I like the conflict markers generally. Daniel can kill it if he likes,
but thanks for the input. I've killed it this time around, but I don't
plan on it for the future.

> > ---
> >  drivers/gpu/drm/i915/i915_irq.c  | 81 +++++++++++++++++++++++++++++++++++++---
> >  drivers/gpu/drm/i915/i915_reg.h  |  1 +
> >  drivers/gpu/drm/i915/intel_drv.h |  3 +-
> >  drivers/gpu/drm/i915/intel_pm.c  | 38 ++++++++++++++++++-
> >  4 files changed, 115 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > index 7a4d3ae..96c459a 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -248,6 +248,50 @@ static bool ivb_can_enable_err_int(struct drm_device *dev)
> >  	return true;
> >  }
> >  
> > +/**
> > +  * bdw_update_pm_irq - update GT interrupt 2
> > +  * @dev_priv: driver private
> > +  * @interrupt_mask: mask of interrupt bits to update
> > +  * @enabled_irq_mask: mask of interrupt bits to enable
> > +  *
> > +  * Copied from the snb function, updated with relevant register offsets
> > +  */
> > +static void bdw_update_pm_irq(struct drm_i915_private *dev_priv,
> > +			      uint32_t interrupt_mask,
> > +			      uint32_t enabled_irq_mask)
> > +{
> > +	uint32_t new_val;
> > +
> > +	assert_spin_locked(&dev_priv->irq_lock);
> > +
> > +	if (dev_priv->pm.irqs_disabled) {
> > +		WARN(1, "IRQs disabled\n");
> > +		return;
> > +	}
> > +
> > +	new_val = dev_priv->pm_irq_mask;
> > +	new_val &= ~interrupt_mask;
> > +	new_val |= (~enabled_irq_mask & interrupt_mask);
> > +
> > +	if (new_val != dev_priv->pm_irq_mask) {
> > +		dev_priv->pm_irq_mask = new_val;
> > +		I915_WRITE(GEN8_GT_IMR(2), I915_READ(GEN8_GT_IMR(2)) |
> > +					   dev_priv->pm_irq_mask);
> > +		POSTING_READ(GEN8_GT_IMR(2));
> > +	}
> > +}
> > +
> > +void bdw_enable_pm_irq(struct drm_i915_private *dev_priv, uint32_t mask)
> > +{
> > +	bdw_update_pm_irq(dev_priv, mask, mask);
> > +}
> > +
> > +void bdw_disable_pm_irq(struct drm_i915_private *dev_priv, uint32_t mask)
> > +{
> > +	bdw_update_pm_irq(dev_priv, mask, 0);
> > +}
> > +
> > +
> 
> Unnecessary empty line.
> 

Got it, thanks.

> >  static bool cpt_can_enable_serr_int(struct drm_device *dev)
> >  {
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > @@ -1126,13 +1170,17 @@ static void gen6_pm_rps_work(struct work_struct *work)
> >  	spin_lock_irq(&dev_priv->irq_lock);
> >  	pm_iir = dev_priv->rps.pm_iir;
> >  	dev_priv->rps.pm_iir = 0;
> > -	/* Make sure not to corrupt PMIMR state used by ringbuffer code */
> > -	snb_enable_pm_irq(dev_priv, dev_priv->pm_rps_events);
> > +	if (IS_BROADWELL(dev_priv->dev))
> > +		bdw_enable_pm_irq(dev_priv, dev_priv->pm_rps_events);
> > +	else {
> > +		/* Make sure not to corrupt PMIMR state used by ringbuffer */
> > +		snb_enable_pm_irq(dev_priv, dev_priv->pm_rps_events);
> > +		/* Make sure we didn't queue anything we're not going to
> > +		 * process. */
> > +		WARN_ON(pm_iir & ~dev_priv->pm_rps_events);
> > +	}
> >  	spin_unlock_irq(&dev_priv->irq_lock);
> >  
> > -	/* Make sure we didn't queue anything we're not going to process. */
> > -	WARN_ON(pm_iir & ~dev_priv->pm_rps_events);
> 
> Isn't this WARN equally valid for bdw?
> 

So first, let me just mention, this has moved slightly in my latest
version of this patch, but it's still a valid question.

The answer is ambiguous actually. The WARN is always valid technically
The distinction in BDW (at least for the time being) is that unlike
pre-BDW, PM IIR isn't shared. I can add it, but for BDW, at least right
now, DRM_ERROR (or BUG) is more appropriate.


> > -
> >  	if ((pm_iir & dev_priv->pm_rps_events) == 0)
> >  		return;
> >  
> > @@ -1324,6 +1372,19 @@ static void snb_gt_irq_handler(struct drm_device *dev,
> >  		ivybridge_parity_error_irq_handler(dev, gt_iir);
> >  }
> >  
> > +static void gen8_rps_irq_handler(struct drm_i915_private *dev_priv, u32 pm_iir)
> > +{
> > +	if ((pm_iir & dev_priv->pm_rps_events) == 0)
> > +		return;
> > +
> > +	spin_lock(&dev_priv->irq_lock);
> > +	dev_priv->rps.pm_iir |= pm_iir & dev_priv->pm_rps_events;
> > +	bdw_disable_pm_irq(dev_priv, pm_iir & dev_priv->pm_rps_events);
> > +	spin_unlock(&dev_priv->irq_lock);
> > +
> > +	queue_work(dev_priv->wq, &dev_priv->rps.work);
> > +}
> > +
> >  static irqreturn_t gen8_gt_irq_handler(struct drm_device *dev,
> >  				       struct drm_i915_private *dev_priv,
> >  				       u32 master_ctl)
> > @@ -1359,6 +1420,16 @@ static irqreturn_t gen8_gt_irq_handler(struct drm_device *dev,
> >  			DRM_ERROR("The master control interrupt lied (GT1)!\n");
> >  	}
> >  
> > +	if (master_ctl & GEN8_GT_PM_IRQ) {
> > +		tmp = I915_READ(GEN8_GT_IIR(2));
> > +		if (tmp & dev_priv->pm_rps_events) {
> > +			ret = IRQ_HANDLED;
> > +			gen8_rps_irq_handler(dev_priv, tmp);
> > +			I915_WRITE(GEN8_GT_IIR(1), tmp & dev_priv->pm_rps_events);
>                                    ^^^^^^^^^^^^^^
> 
> GEN8_GT_IIR(2)
> 

Nice catch, I guess I need to retest after this one.

> > +		} else
> > +			DRM_ERROR("The master control interrupt lied (PM)!\n");
> > +	}
> > +
> >  	if (master_ctl & GEN8_GT_VECS_IRQ) {
> >  		tmp = I915_READ(GEN8_GT_IIR(3));
> >  		if (tmp) {
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index 8f84555..c2dd436 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -4193,6 +4193,7 @@ enum punit_power_well {
> >  #define  GEN8_DE_PIPE_A_IRQ		(1<<16)
> >  #define  GEN8_DE_PIPE_IRQ(pipe)		(1<<(16+pipe))
> >  #define  GEN8_GT_VECS_IRQ		(1<<6)
> > +#define  GEN8_GT_PM_IRQ			(1<<4)
> >  #define  GEN8_GT_VCS2_IRQ		(1<<3)
> >  #define  GEN8_GT_VCS1_IRQ		(1<<2)
> >  #define  GEN8_GT_BCS_IRQ		(1<<1)
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index c551472..68a078c 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -650,10 +650,11 @@ void ilk_enable_gt_irq(struct drm_i915_private *dev_priv, uint32_t mask);
> >  void ilk_disable_gt_irq(struct drm_i915_private *dev_priv, uint32_t mask);
> >  void snb_enable_pm_irq(struct drm_i915_private *dev_priv, uint32_t mask);
> >  void snb_disable_pm_irq(struct drm_i915_private *dev_priv, uint32_t mask);
> > +void bdw_enable_pm_irq(struct drm_i915_private *dev_priv, uint32_t mask);
> > +void bdw_disable_pm_irq(struct drm_i915_private *dev_priv, uint32_t mask);
> >  void intel_runtime_pm_disable_interrupts(struct drm_device *dev);
> >  void intel_runtime_pm_restore_interrupts(struct drm_device *dev);
> >  
> > -
> 
> Unrelated whitespace change. IIRC someone left these two empty lines
> here on purpose.
> 

Got it, thanks.

> >  /* intel_crt.c */
> >  void intel_crt_init(struct drm_device *dev);
> >  
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index 75c1c76..27b64ab 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -3210,6 +3210,25 @@ void valleyview_set_rps(struct drm_device *dev, u8 val)
> >  	trace_intel_gpu_freq_change(vlv_gpu_freq(dev_priv, val));
> >  }
> >  
> > +static void gen8_disable_rps_interrupts(struct drm_device *dev)
> > +{
> > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > +
> > +	I915_WRITE(GEN6_PMINTRMSK, 0xffffffff);
> > +	I915_WRITE(GEN8_GT_IER(2), I915_READ(GEN8_GT_IER(2)) &
> > +				   ~dev_priv->pm_rps_events);
> > +	/* Complete PM interrupt masking here doesn't race with the rps work
> > +	 * item again unmasking PM interrupts because that is using a different
> > +	 * register (PMIMR) to mask PM interrupts. The only risk is in leaving
> > +	 * stale bits in PMIIR and PMIMR which gen6_enable_rps will clean up. */
> 
> This comment would need a bit of update for gen8.
> 

I've killed this comment locally. Somewhere I had written that we can
rewrite some of this for gen8, but I seem to have lost that in the sands
of time.

> > +
> > +	spin_lock_irq(&dev_priv->irq_lock);
> > +	dev_priv->rps.pm_iir = 0;
> > +	spin_unlock_irq(&dev_priv->irq_lock);
> > +
> > +	I915_WRITE(GEN8_GT_IIR(2), dev_priv->pm_rps_events);
> > +}
> > +
> >  static void gen6_disable_rps_interrupts(struct drm_device *dev)
> >  {
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > @@ -3236,7 +3255,10 @@ static void gen6_disable_rps(struct drm_device *dev)
> >  	I915_WRITE(GEN6_RC_CONTROL, 0);
> >  	I915_WRITE(GEN6_RPNSWREQ, 1 << 31);
> >  
> > -	gen6_disable_rps_interrupts(dev);
> > +	if (IS_BROADWELL(dev))
> > +		gen8_disable_rps_interrupts(dev);
> > +	else
> > +		gen6_disable_rps_interrupts(dev);
> >  }
> >  
> >  static void valleyview_disable_rps(struct drm_device *dev)
> > @@ -3276,6 +3298,18 @@ int intel_enable_rc6(const struct drm_device *dev)
> >  	return INTEL_RC6_ENABLE;
> >  }
> >  
> > +static void gen8_enable_rps_interrupts(struct drm_device *dev)
> > +{
> > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > +
> > +	spin_lock_irq(&dev_priv->irq_lock);
> > +	WARN_ON(dev_priv->rps.pm_iir);
> > +	bdw_enable_pm_irq(dev_priv, dev_priv->pm_rps_events);
> > +	I915_WRITE(GEN8_GT_IIR(2), dev_priv->pm_rps_events);
> 
> The order of "unmask first then clear" seems just wrong. The same issues is
> already present in the gen6 code however. So this should be fixed in
> both places, or if it's like that on purpose it needs a comment.
> 

Well, if you want to fix it, we can do it in a separate patch. Although
I don't think I agree. Why does it seem wrong to you?

> > +	spin_unlock_irq(&dev_priv->irq_lock);
> > +
> 
> Useless empty line.
> 

If I was in an arguing mood, I would do, but fine.

> > +}
> > +
> >  static void gen6_enable_rps_interrupts(struct drm_device *dev)
> >  {
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > @@ -3378,7 +3412,7 @@ static void gen8_enable_rps(struct drm_device *dev)
> >  
> >  	gen6_set_rps(dev, (I915_READ(GEN6_GT_PERF_STATUS) & 0xff00) >> 8);
> >  
> > -	gen6_enable_rps_interrupts(dev);
> > +	gen8_enable_rps_interrupts(dev);
> >  
> >  	gen6_gt_force_wake_put(dev_priv, FORCEWAKE_ALL);
> >  }

Thanks for the review.

-- 
Ben Widawsky, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx





[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux