[Intel-gfx] [PATCH 1/2] drm/i915/bdw: Implement a basic PM interrupt handler
S, Deepak
deepak.s at intel.com
Fri Apr 18 14:39:40 CEST 2014
On 4/16/2014 2:11 PM, Ville Syrjälä wrote:
> On Tue, Apr 15, 2014 at 07:43:07PM -0700, Ben Widawsky wrote:
>> 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 at intel.com wrote:
>>>> From: Ben Widawsky <benjamin.widawsky at intel.com>
>>>>
>>>> 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 at intel.com>
>>>> 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 at bwidawsk.net>
>>>>
>>>> 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.
>
> I don't see a reason for bdw to be different here. There are other bits
> in the relevant IIR registers, so the same WARN should be fine.
>
>>
>>
>>>> -
>>>> 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.
>
> I'm actually wondering how/if you managed to test this before Deepak's
> PMINTRMSK interrupt redirect bit fix. W/o that you shouldn't be
> getting any interrupt AFAICS.
>
>>
>>>> + } 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?
>
> If you enable before clearing and there's already an IIR bit high the
> interrupt should trigger immediately before you get to clear it. And
> then on a SMP machine the write to clear it would even race with the
> interrupt handler if there's no locking. So the clearing after
> enabling+unmasking is pointless. The normal approach for enabling
> interrupts is to clear first and then enable.
>
> Hmm. Actually here we alreay have the interrupts enabled in IER, so
> if any relevant IIR bit would be high, we should have already gotten
> the interrupt. But since we always clear the IIR after masking the
> interrupts in IMR, we should never have any relevant IIR bit set here.
>
> So looks to me like we can just drop the IIR writes here. They serve no
> purpose other than racing with the interrupt handler if a real interrupt
> happens just after we've unmasked it.
I think we should create a separate patch to fix this issue for both
gen6 and gen8
>>
>>>> + 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
>
Thanks for the review.
More information about the Intel-gfx
mailing list