From mboxrd@z Thu Jan 1 00:00:00 1970 From: Imre Deak Subject: Re: [PATCH 1/6] drm/i915: kill dev_priv->pm.regsave Date: Thu, 20 Mar 2014 14:58:27 +0200 Message-ID: <1395320307.17233.4.camel@intelbox> References: <1394233957-3904-1-git-send-email-przanoni@gmail.com> <1394233957-3904-2-git-send-email-przanoni@gmail.com> Reply-To: imre.deak@intel.com Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============0400291285==" Return-path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by gabe.freedesktop.org (Postfix) with ESMTP id 9D4C66E05A for ; Thu, 20 Mar 2014 05:58:50 -0700 (PDT) In-Reply-To: <1394233957-3904-2-git-send-email-przanoni@gmail.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Paulo Zanoni Cc: intel-gfx@lists.freedesktop.org, Paulo Zanoni List-Id: intel-gfx@lists.freedesktop.org --===============0400291285== Content-Type: multipart/signed; micalg="pgp-sha1"; protocol="application/pgp-signature"; boundary="=-hnfmNuNtCBYGOf/aC68s" --=-hnfmNuNtCBYGOf/aC68s Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Fri, 2014-03-07 at 20:12 -0300, Paulo Zanoni wrote: > From: Paulo Zanoni >=20 > Now that we don't keep the hotplug interrupts enabled anymore, we can > kill the regsave struct and just cal the normal IRQ preinstall, > postinstall and uninstall functions. This makes it easier to add > runtime PM support to non-HSW platforms. >=20 > The only downside is in case we get a request to update interrupts > while they are disabled, won't be able to update the regsave struct. > But this should never happen anyway, so we're not losing too much. >=20 > v2: - Rebase. > v3: - Rebase. > v4: - Rebase. >=20 > Signed-off-by: Paulo Zanoni > --- > drivers/gpu/drm/i915/i915_drv.h | 12 +----- > drivers/gpu/drm/i915/i915_irq.c | 79 +++++-------------------------= ------ > drivers/gpu/drm/i915/intel_display.c | 4 +- > drivers/gpu/drm/i915/intel_drv.h | 4 +- > 4 files changed, 15 insertions(+), 84 deletions(-) >=20 > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_= drv.h > index 70feb61..493579d 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -1351,23 +1351,13 @@ struct ilk_wm_values { > * goes back to false exactly before we reenable the IRQs. We use this v= ariable > * to check if someone is trying to enable/disable IRQs while they're su= pposed > * to be disabled. This shouldn't happen and we'll print some error mess= ages in > - * case it happens, but if it actually happens we'll also update the var= iables > - * inside struct regsave so when we restore the IRQs they will contain t= he > - * latest expected values. > + * case it happens. > * > * For more, read the Documentation/power/runtime_pm.txt. > */ > struct i915_runtime_pm { > bool suspended; > bool irqs_disabled; > - > - struct { > - uint32_t deimr; > - uint32_t sdeimr; > - uint32_t gtimr; > - uint32_t gtier; > - uint32_t gen6_pmimr; > - } regsave; > }; > =20 > enum intel_pipe_crc_source { > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_= irq.c > index dee3a3b..2aefb94 100644 > --- a/drivers/gpu/drm/i915/i915_irq.c > +++ b/drivers/gpu/drm/i915/i915_irq.c > @@ -131,11 +131,8 @@ ironlake_enable_display_irq(drm_i915_private_t *dev_= priv, u32 mask) > { > assert_spin_locked(&dev_priv->irq_lock); > =20 > - if (dev_priv->pm.irqs_disabled) { > - WARN(1, "IRQs disabled\n"); > - dev_priv->pm.regsave.deimr &=3D ~mask; > + if (WARN_ON(dev_priv->pm.irqs_disabled)) > return; > - } > =20 > if ((dev_priv->irq_mask & mask) !=3D 0) { > dev_priv->irq_mask &=3D ~mask; > @@ -149,11 +146,8 @@ ironlake_disable_display_irq(drm_i915_private_t *dev= _priv, u32 mask) > { > assert_spin_locked(&dev_priv->irq_lock); > =20 > - if (dev_priv->pm.irqs_disabled) { > - WARN(1, "IRQs disabled\n"); > - dev_priv->pm.regsave.deimr |=3D mask; > + if (WARN_ON(dev_priv->pm.irqs_disabled)) > return; > - } > =20 > if ((dev_priv->irq_mask & mask) !=3D mask) { > dev_priv->irq_mask |=3D mask; > @@ -174,13 +168,8 @@ static void ilk_update_gt_irq(struct drm_i915_privat= e *dev_priv, > { > assert_spin_locked(&dev_priv->irq_lock); > =20 > - if (dev_priv->pm.irqs_disabled) { > - WARN(1, "IRQs disabled\n"); > - dev_priv->pm.regsave.gtimr &=3D ~interrupt_mask; > - dev_priv->pm.regsave.gtimr |=3D (~enabled_irq_mask & > - interrupt_mask); > + if (WARN_ON(dev_priv->pm.irqs_disabled)) > return; > - } > =20 > dev_priv->gt_irq_mask &=3D ~interrupt_mask; > dev_priv->gt_irq_mask |=3D (~enabled_irq_mask & interrupt_mask); > @@ -212,13 +201,8 @@ static void snb_update_pm_irq(struct drm_i915_privat= e *dev_priv, > =20 > assert_spin_locked(&dev_priv->irq_lock); > =20 > - if (dev_priv->pm.irqs_disabled) { > - WARN(1, "IRQs disabled\n"); > - dev_priv->pm.regsave.gen6_pmimr &=3D ~interrupt_mask; > - dev_priv->pm.regsave.gen6_pmimr |=3D (~enabled_irq_mask & > - interrupt_mask); > + if (WARN_ON(dev_priv->pm.irqs_disabled)) > return; > - } > =20 > new_val =3D dev_priv->pm_irq_mask; > new_val &=3D ~interrupt_mask; > @@ -358,14 +342,8 @@ static void ibx_display_interrupt_update(struct drm_= i915_private *dev_priv, > =20 > assert_spin_locked(&dev_priv->irq_lock); > =20 > - if (dev_priv->pm.irqs_disabled && > - (interrupt_mask & SDE_HOTPLUG_MASK_CPT)) { > - WARN(1, "IRQs disabled\n"); > - dev_priv->pm.regsave.sdeimr &=3D ~interrupt_mask; > - dev_priv->pm.regsave.sdeimr |=3D (~enabled_irq_mask & > - interrupt_mask); > + if (WARN_ON(dev_priv->pm.irqs_disabled)) > return; > - } > =20 > I915_WRITE(SDEIMR, sdeimr); > POSTING_READ(SDEIMR); > @@ -4091,57 +4069,20 @@ void intel_hpd_init(struct drm_device *dev) > } > =20 > /* Disable interrupts so we can allow runtime PM. */ > -void hsw_runtime_pm_disable_interrupts(struct drm_device *dev) > +void intel_runtime_pm_disable_interrupts(struct drm_device *dev) > { > struct drm_i915_private *dev_priv =3D dev->dev_private; > - unsigned long irqflags; > - > - spin_lock_irqsave(&dev_priv->irq_lock, irqflags); > - > - dev_priv->pm.regsave.deimr =3D I915_READ(DEIMR); > - dev_priv->pm.regsave.sdeimr =3D I915_READ(SDEIMR); > - dev_priv->pm.regsave.gtimr =3D I915_READ(GTIMR); > - dev_priv->pm.regsave.gtier =3D I915_READ(GTIER); > - dev_priv->pm.regsave.gen6_pmimr =3D I915_READ(GEN6_PMIMR); > - > - ironlake_disable_display_irq(dev_priv, 0xffffffff); > - ibx_disable_display_interrupt(dev_priv, 0xffffffff); > - ilk_disable_gt_irq(dev_priv, 0xffffffff); > - snb_disable_pm_irq(dev_priv, 0xffffffff); > =20 > + dev->driver->irq_uninstall(dev); > dev_priv->pm.irqs_disabled =3D true; > - > - spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags); It made me think whether removing the locking here is ok. It seems to be ok, as we get here in an idle state where no interrupts should happen. A note about this change in the commit message would have been nice. Otherwise the patch looks ok: Reviewed-by: Imre Deak > } > =20 > /* Restore interrupts so we can recover from runtime PM. */ > -void hsw_runtime_pm_restore_interrupts(struct drm_device *dev) > +void intel_runtime_pm_restore_interrupts(struct drm_device *dev) > { > struct drm_i915_private *dev_priv =3D dev->dev_private; > - unsigned long irqflags; > - uint32_t val; > - > - spin_lock_irqsave(&dev_priv->irq_lock, irqflags); > - > - val =3D I915_READ(DEIMR); > - WARN(val !=3D 0xffffffff, "DEIMR is 0x%08x\n", val); > - > - val =3D I915_READ(SDEIMR); > - WARN(val !=3D 0xffffffff, "SDEIMR is 0x%08x\n", val); > - > - val =3D I915_READ(GTIMR); > - WARN(val !=3D 0xffffffff, "GTIMR is 0x%08x\n", val); > - > - val =3D I915_READ(GEN6_PMIMR); > - WARN(val !=3D 0xffffffff, "GEN6_PMIMR is 0x%08x\n", val); > =20 > dev_priv->pm.irqs_disabled =3D false; > - > - ironlake_enable_display_irq(dev_priv, ~dev_priv->pm.regsave.deimr); > - ibx_enable_display_interrupt(dev_priv, ~dev_priv->pm.regsave.sdeimr); > - ilk_enable_gt_irq(dev_priv, ~dev_priv->pm.regsave.gtimr); > - snb_enable_pm_irq(dev_priv, ~dev_priv->pm.regsave.gen6_pmimr); > - I915_WRITE(GTIER, dev_priv->pm.regsave.gtier); > - > - spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags); > + dev->driver->irq_preinstall(dev); > + dev->driver->irq_postinstall(dev); > } > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/= intel_display.c > index ed9233e..df69866 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -6829,7 +6829,7 @@ void hsw_enable_pc8(struct drm_i915_private *dev_pr= iv) > } > =20 > lpt_disable_clkout_dp(dev); > - hsw_runtime_pm_disable_interrupts(dev); > + intel_runtime_pm_disable_interrupts(dev); > hsw_disable_lcpll(dev_priv, true, true); > } > =20 > @@ -6843,7 +6843,7 @@ void hsw_disable_pc8(struct drm_i915_private *dev_p= riv) > DRM_DEBUG_KMS("Disabling package C8+\n"); > =20 > hsw_restore_lcpll(dev_priv); > - hsw_runtime_pm_restore_interrupts(dev); > + intel_runtime_pm_restore_interrupts(dev); > lpt_init_pch_refclk(dev); > =20 > if (dev_priv->pch_id =3D=3D INTEL_PCH_LPT_LP_DEVICE_ID_TYPE) { > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/inte= l_drv.h > index 9a7db84..0dfb6e9 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -618,8 +618,8 @@ void ilk_enable_gt_irq(struct drm_i915_private *dev_p= riv, 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 hsw_runtime_pm_disable_interrupts(struct drm_device *dev); > -void hsw_runtime_pm_restore_interrupts(struct drm_device *dev); > +void intel_runtime_pm_disable_interrupts(struct drm_device *dev); > +void intel_runtime_pm_restore_interrupts(struct drm_device *dev); > =20 >=20 > /* intel_crt.c */ --=-hnfmNuNtCBYGOf/aC68s Content-Type: application/pgp-signature; name="signature.asc" Content-Description: This is a digitally signed message part Content-Transfer-Encoding: 7bit -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.14 (GNU/Linux) iQEcBAABAgAGBQJTKuX0AAoJEORIIAnNuWDFz14IALe7cgkfBIS7AcjVYGjn7Fza mCXthazlcP0g5flsfFY6/xOfDLjhzVTb3knj+JwI+LD4AjJCL7pHW+GnvVfMXrk5 MV0sDP/T9T8EMAfG7ecdmdJuruoGHiFMrAvW25I7eYYPmnSYsS3EKMmnIVaLs3gX pUODcUIFX8DTcoPXxtUi/yqiPoG70GPqsJM20zk5+XkqiPwLyiSC3ze0IGN8Qand y8+wbZhIf5Rb6MZ5Pm+uw0nHpZrMtbCaFqileCx4BrxOlGs7Lrpuqf3GYeNHuRAU XnWabPwfA9oNajpzEk/7YV7NIIKYQpszLTu9IOt8uQtnRex88qj5BNYhc2wc6Ig= =Zqhu -----END PGP SIGNATURE----- --=-hnfmNuNtCBYGOf/aC68s-- --===============0400291285== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx --===============0400291285==--