| Message ID | 1364341470-1106-1-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Mar 27, 2013 at 12:44:30AM +0100, Daniel Vetter wrote: > Due to the irq setup rework in 3.9 Egbert's hpd rework blows up on > pch-split platforms. The new init sequence is: > > - irq enabling > - modeset init > - hpd setup > > We need to move around the ibx setup a bit to fix this. > > This needs to be squashed into a commit on dinq. > > Cc: Egbert Eich <eich@suse.de> > Cc: Jesse Barnes <jbarnes@virtuousgeek.org> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > --- > drivers/gpu/drm/i915/i915_irq.c | 21 ++++++++++++++++++--- > 1 file changed, 18 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c > index 43436e0..1279a44 100644 > --- a/drivers/gpu/drm/i915/i915_irq.c > +++ b/drivers/gpu/drm/i915/i915_irq.c > @@ -2084,7 +2084,7 @@ static void ibx_enable_hotplug(struct drm_device *dev) > I915_WRITE(PCH_PORT_HOTPLUG, hotplug); > } > > -static void ibx_irq_postinstall(struct drm_device *dev) > +static void ibx_hpd_irq_setup(struct drm_device *dev) > { > drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private; > struct drm_mode_config *mode_config = &dev->mode_config; > @@ -2095,12 +2095,10 @@ static void ibx_irq_postinstall(struct drm_device *dev) > mask &= ~SDE_HOTPLUG_MASK; > list_for_each_entry(intel_encoder, &mode_config->encoder_list, base.head) > mask |= hpd_ibx[intel_encoder->hpd_pin]; > - mask |= SDE_GMBUS | SDE_AUX_MASK; > } else { > mask &= ~SDE_HOTPLUG_MASK_CPT; > list_for_each_entry(intel_encoder, &mode_config->encoder_list, base.head) > mask |= hpd_cpt[intel_encoder->hpd_pin]; > - mask |= SDE_GMBUS_CPT | SDE_AUX_MASK_CPT; > } > I915_WRITE(SDEIIR, I915_READ(SDEIIR)); > I915_WRITE(SDEIMR, ~mask); > @@ -2110,6 +2108,21 @@ static void ibx_irq_postinstall(struct drm_device *dev) > ibx_enable_hotplug(dev); > } > > +static void ibx_irq_postinstall(struct drm_device *dev) > +{ > + drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private; > + u32 mask = I915_READ(SDEIER); > + > + if (HAS_PCH_IBX(dev)) > + mask |= SDE_GMBUS | SDE_AUX_MASK; > + else > + mask |= SDE_GMBUS_CPT | SDE_AUX_MASK_CPT; > + I915_WRITE(SDEIIR, I915_READ(SDEIIR)); > + I915_WRITE(SDEIMR, ~mask); > + I915_WRITE(SDEIER, mask); > + POSTING_READ(SDEIER); > +} Should we clear just the HPD bits here? Then again, I suppose enable_hotplug_processing should make sure we don't do aux/gmbus transfers at the same time, so maybe it doesn't matter. But now I started to wondee what are chances that we'd get some other interrupt while executing this function. That could lead to a corrupted SDEIER now that the irq handler touches it as well. > + > static int ironlake_irq_postinstall(struct drm_device *dev) > { > drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private; > @@ -2960,6 +2973,7 @@ void intel_irq_init(struct drm_device *dev) > dev->driver->irq_uninstall = ironlake_irq_uninstall; > dev->driver->enable_vblank = ivybridge_enable_vblank; > dev->driver->disable_vblank = ivybridge_disable_vblank; > + dev_priv->display.hpd_irq_setup = ibx_hpd_irq_setup; > } else if (HAS_PCH_SPLIT(dev)) { > dev->driver->irq_handler = ironlake_irq_handler; > dev->driver->irq_preinstall = ironlake_irq_preinstall; > @@ -2967,6 +2981,7 @@ void intel_irq_init(struct drm_device *dev) > dev->driver->irq_uninstall = ironlake_irq_uninstall; > dev->driver->enable_vblank = ironlake_enable_vblank; > dev->driver->disable_vblank = ironlake_disable_vblank; > + dev_priv->display.hpd_irq_setup = ibx_hpd_irq_setup; > } else { > if (INTEL_INFO(dev)->gen == 2) { > dev->driver->irq_preinstall = i8xx_irq_preinstall; > -- > 1.7.11.7 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Sorry for replying so late, I wasn't able to task switch my brain towards this when it was discussed: Daniel Vetter writes: > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c > index 43436e0..1279a44 100644 > --- a/drivers/gpu/drm/i915/i915_irq.c > +++ b/drivers/gpu/drm/i915/i915_irq.c > @@ -2084,7 +2084,7 @@ static void ibx_enable_hotplug(struct drm_device *dev) > I915_WRITE(PCH_PORT_HOTPLUG, hotplug); > } > > -static void ibx_irq_postinstall(struct drm_device *dev) > +static void ibx_hpd_irq_setup(struct drm_device *dev) > { > drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private; > struct drm_mode_config *mode_config = &dev->mode_config; > @@ -2095,12 +2095,10 @@ static void ibx_irq_postinstall(struct drm_device *dev) > mask &= ~SDE_HOTPLUG_MASK; ^^^^^^^^^^^^^^^^^^^^^^^ I'm missing those lines in the committed version of the patch. > list_for_each_entry(intel_encoder, &mode_config->encoder_list, base.head) > mask |= hpd_ibx[intel_encoder->hpd_pin]; > - mask |= SDE_GMBUS | SDE_AUX_MASK; > } else { > mask &= ~SDE_HOTPLUG_MASK_CPT; ^^^^^^^^^^^^^^^^^^^^^^^ > list_for_each_entry(intel_encoder, &mode_config->encoder_list, base.head) > mask |= hpd_cpt[intel_encoder->hpd_pin]; > - mask |= SDE_GMBUS_CPT | SDE_AUX_MASK_CPT; > } > I915_WRITE(SDEIIR, I915_READ(SDEIIR)); These are not really relevant in the present code, however they are important once I've got the hotplug stuff refitted as one needs to be able to turn off individual interrupts. I'm going to prepare a commit for this and will send it with the hpd irq storm patches. Cheers, Egbert.
On Fri, Mar 29, 2013 at 5:35 PM, Egbert Eich <eich@suse.com> wrote: > Daniel Vetter writes: > > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c > > index 43436e0..1279a44 100644 > > --- a/drivers/gpu/drm/i915/i915_irq.c > > +++ b/drivers/gpu/drm/i915/i915_irq.c > > @@ -2084,7 +2084,7 @@ static void ibx_enable_hotplug(struct drm_device *dev) > > I915_WRITE(PCH_PORT_HOTPLUG, hotplug); > > } > > > > -static void ibx_irq_postinstall(struct drm_device *dev) > > +static void ibx_hpd_irq_setup(struct drm_device *dev) > > { > > drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private; > > struct drm_mode_config *mode_config = &dev->mode_config; > > @@ -2095,12 +2095,10 @@ static void ibx_irq_postinstall(struct drm_device *dev) > > mask &= ~SDE_HOTPLUG_MASK; > ^^^^^^^^^^^^^^^^^^^^^^^ > I'm missing those lines in the committed version of the patch. > > > list_for_each_entry(intel_encoder, &mode_config->encoder_list, base.head) > > mask |= hpd_ibx[intel_encoder->hpd_pin]; > > - mask |= SDE_GMBUS | SDE_AUX_MASK; > > } else { > > mask &= ~SDE_HOTPLUG_MASK_CPT; > ^^^^^^^^^^^^^^^^^^^^^^^ > > list_for_each_entry(intel_encoder, &mode_config->encoder_list, base.head) > > mask |= hpd_cpt[intel_encoder->hpd_pin]; > > - mask |= SDE_GMBUS_CPT | SDE_AUX_MASK_CPT; > > } > > I915_WRITE(SDEIIR, I915_READ(SDEIIR)); > > These are not really relevant in the present code, however they are > important once I've got the hotplug stuff refitted as one needs to be > able to turn off individual interrupts. > I'm going to prepare a commit for this and will send it with the hpd > irq storm patches. Yeah, makes sense now that I think about it - I've simply didn't look ahead in your patch series while writing this little fixup ;-) Can you just re-add this when resending your patches again please? Thanks, Daniel
Hi Daniel, Daniel Vetter writes: > On Fri, Mar 29, 2013 at 5:35 PM, Egbert Eich <eich@suse.com> wrote: > > Yeah, makes sense now that I think about it - I've simply didn't look > ahead in your patch series while writing this little fixup ;-) Can you > just re-add this when resending your patches again please? > Sure, I have prepared all the patches. I just wanted to give them a try before sending them. Unfortunately I did not get around to do so over the Easter holidays. Cheers, Egbert.
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index 43436e0..1279a44 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -2084,7 +2084,7 @@ static void ibx_enable_hotplug(struct drm_device *dev) I915_WRITE(PCH_PORT_HOTPLUG, hotplug); } -static void ibx_irq_postinstall(struct drm_device *dev) +static void ibx_hpd_irq_setup(struct drm_device *dev) { drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private; struct drm_mode_config *mode_config = &dev->mode_config; @@ -2095,12 +2095,10 @@ static void ibx_irq_postinstall(struct drm_device *dev) mask &= ~SDE_HOTPLUG_MASK; list_for_each_entry(intel_encoder, &mode_config->encoder_list, base.head) mask |= hpd_ibx[intel_encoder->hpd_pin]; - mask |= SDE_GMBUS | SDE_AUX_MASK; } else { mask &= ~SDE_HOTPLUG_MASK_CPT; list_for_each_entry(intel_encoder, &mode_config->encoder_list, base.head) mask |= hpd_cpt[intel_encoder->hpd_pin]; - mask |= SDE_GMBUS_CPT | SDE_AUX_MASK_CPT; } I915_WRITE(SDEIIR, I915_READ(SDEIIR)); I915_WRITE(SDEIMR, ~mask); @@ -2110,6 +2108,21 @@ static void ibx_irq_postinstall(struct drm_device *dev) ibx_enable_hotplug(dev); } +static void ibx_irq_postinstall(struct drm_device *dev) +{ + drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private; + u32 mask = I915_READ(SDEIER); + + if (HAS_PCH_IBX(dev)) + mask |= SDE_GMBUS | SDE_AUX_MASK; + else + mask |= SDE_GMBUS_CPT | SDE_AUX_MASK_CPT; + I915_WRITE(SDEIIR, I915_READ(SDEIIR)); + I915_WRITE(SDEIMR, ~mask); + I915_WRITE(SDEIER, mask); + POSTING_READ(SDEIER); +} + static int ironlake_irq_postinstall(struct drm_device *dev) { drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private; @@ -2960,6 +2973,7 @@ void intel_irq_init(struct drm_device *dev) dev->driver->irq_uninstall = ironlake_irq_uninstall; dev->driver->enable_vblank = ivybridge_enable_vblank; dev->driver->disable_vblank = ivybridge_disable_vblank; + dev_priv->display.hpd_irq_setup = ibx_hpd_irq_setup; } else if (HAS_PCH_SPLIT(dev)) { dev->driver->irq_handler = ironlake_irq_handler; dev->driver->irq_preinstall = ironlake_irq_preinstall; @@ -2967,6 +2981,7 @@ void intel_irq_init(struct drm_device *dev) dev->driver->irq_uninstall = ironlake_irq_uninstall; dev->driver->enable_vblank = ironlake_enable_vblank; dev->driver->disable_vblank = ironlake_disable_vblank; + dev_priv->display.hpd_irq_setup = ibx_hpd_irq_setup; } else { if (INTEL_INFO(dev)->gen == 2) { dev->driver->irq_preinstall = i8xx_irq_preinstall;
Due to the irq setup rework in 3.9 Egbert's hpd rework blows up on pch-split platforms. The new init sequence is: - irq enabling - modeset init - hpd setup We need to move around the ibx setup a bit to fix this. This needs to be squashed into a commit on dinq. Cc: Egbert Eich <eich@suse.de> Cc: Jesse Barnes <jbarnes@virtuousgeek.org> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> --- drivers/gpu/drm/i915/i915_irq.c | 21 ++++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-)