From mboxrd@z Thu Jan 1 00:00:00 1970 From: "S, Deepak" Subject: Re: [PATCH 1/2] drm/i915/bdw: Implement a basic PM interrupt handler Date: Fri, 18 Apr 2014 18:09:40 +0530 Message-ID: <53511D0C.5060905@intel.com> References: <1397495475-22667-1-git-send-email-deepak.s@intel.com> <1397495475-22667-2-git-send-email-deepak.s@intel.com> <20140414195553.GP18465@intel.com> <20140416024306.GA6425@bwidawsk.net> <20140416084133.GB18465@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1"; Format="flowed" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mga03.intel.com (mga03.intel.com [143.182.124.21]) by gabe.freedesktop.org (Postfix) with ESMTP id 5FE996E681 for ; Fri, 18 Apr 2014 05:39:46 -0700 (PDT) In-Reply-To: <20140416084133.GB18465@intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: =?ISO-8859-1?Q?Ville_Syrj=E4l=E4?= , Ben Widawsky Cc: intel-gfx@lists.freedesktop.org, Ben Widawsky List-Id: intel-gfx@lists.freedesktop.org On 4/16/2014 2:11 PM, Ville Syrj=E4l=E4 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=E4l=E4 wrote: >>> On Mon, Apr 14, 2014 at 10:41:14PM +0530, deepak.s@intel.com wrote: >>>> From: Ben Widawsky >>>> >>>> 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 updat= es >>>> to the relevant parts requiring changes to the interrupt handling. As >>>> such it /should/ be relatively trivial. It's highly likely that I miss= ed >>>> some places where I need a gen8 version of the PM interrupts, but it h= as >>>> 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 >>>> 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 >>>> >>>> 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/i9= 15_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_dev= ice *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 offs= ets >>>> + */ >>>> +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 =3D dev_priv->pm_irq_mask; >>>> + new_val &=3D ~interrupt_mask; >>>> + new_val |=3D (~enabled_irq_mask & interrupt_mask); >>>> + >>>> + if (new_val !=3D dev_priv->pm_irq_mask) { >>>> + dev_priv->pm_irq_mask =3D 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 ma= sk) >>>> +{ >>>> + bdw_update_pm_irq(dev_priv, mask, mask); >>>> +} >>>> + >>>> +void bdw_disable_pm_irq(struct drm_i915_private *dev_priv, uint32_t m= ask) >>>> +{ >>>> + 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 =3D dev->dev_private; >>>> @@ -1126,13 +1170,17 @@ static void gen6_pm_rps_work(struct work_struc= t *work) >>>> spin_lock_irq(&dev_priv->irq_lock); >>>> pm_iir =3D dev_priv->rps.pm_iir; >>>> dev_priv->rps.pm_iir =3D 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) =3D=3D 0) >>>> return; >>>> >>>> @@ -1324,6 +1372,19 @@ static void snb_gt_irq_handler(struct drm_devic= e *dev, >>>> ivybridge_parity_error_irq_handler(dev, gt_iir); >>>> } >>>> >>>> +static void gen8_rps_irq_handler(struct drm_i915_private *dev_priv, u= 32 pm_iir) >>>> +{ >>>> + if ((pm_iir & dev_priv->pm_rps_events) =3D=3D 0) >>>> + return; >>>> + >>>> + spin_lock(&dev_priv->irq_lock); >>>> + dev_priv->rps.pm_iir |=3D 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 d= rm_device *dev, >>>> DRM_ERROR("The master control interrupt lied (GT1)!\n"); >>>> } >>>> >>>> + if (master_ctl & GEN8_GT_PM_IRQ) { >>>> + tmp =3D I915_READ(GEN8_GT_IIR(2)); >>>> + if (tmp & dev_priv->pm_rps_events) { >>>> + ret =3D 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 =3D I915_READ(GEN8_GT_IIR(3)); >>>> if (tmp) { >>>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i9= 15_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/i= ntel_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 m= ask); >>>> 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 ma= sk); >>>> +void bdw_disable_pm_irq(struct drm_i915_private *dev_priv, uint32_t m= ask); >>>> 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/in= tel_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 =3D 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 differ= ent >>>> + * register (PMIMR) to mask PM interrupts. The only risk is in leavi= ng >>>> + * 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 =3D 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 =3D 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 *d= ev) >>>> return INTEL_RC6_ENABLE; >>>> } >>>> >>>> +static void gen8_enable_rps_interrupts(struct drm_device *dev) >>>> +{ >>>> + struct drm_i915_private *dev_priv =3D 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 issue= s 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 =3D dev->dev_private; >>>> @@ -3378,7 +3412,7 @@ static void gen8_enable_rps(struct drm_device *d= ev) >>>> >>>> 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.