On Mon, Mar 02, 2015 at 03:37:47PM -0300, Paulo Zanoni wrote:
> 2015-02-13 17:37 GMT-02:00 Damien Lespiau <damien.lespiau@xxxxxxxxx>:
> > The pipe interrupt registers are in the actual pipe power well, so we
> > need to restore them when re-enable the corresponding power well.
> >
> > I've also copied what we do on HSW/BDW for VGA, even if the we haven't
> > enabled unclaimed registers just yet.
> >
> > Signed-off-by: Damien Lespiau <damien.lespiau@xxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/i915_irq.c | 4 ++++
> > drivers/gpu/drm/i915/intel_runtime_pm.c | 31 +++++++++++++++++++++++++++++++
> > 2 files changed, 35 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > index 65fe9e7..292ba89 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -3235,6 +3235,10 @@ void gen8_irq_power_well_post_enable(struct drm_i915_private *dev_priv,
> > uint32_t extra_ier = GEN8_PIPE_VBLANK | GEN8_PIPE_FIFO_UNDERRUN;
> >
> > spin_lock_irq(&dev_priv->irq_lock);
> > + if (pipe_mask & 1 << PIPE_A)
> > + GEN8_IRQ_INIT_NDX(DE_PIPE, PIPE_A,
> > + dev_priv->de_irq_mask[PIPE_A],
> > + ~dev_priv->de_irq_mask[PIPE_A] | extra_ier);
> > if (pipe_mask & 1 << PIPE_B)
> > GEN8_IRQ_INIT_NDX(DE_PIPE, PIPE_B,
> > dev_priv->de_irq_mask[PIPE_B],
> > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > index 35e0cb6..8989747 100644
> > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > @@ -199,6 +199,34 @@ static void hsw_power_well_post_enable(struct drm_i915_private *dev_priv)
> > 1 << PIPE_C | 1 << PIPE_B);
> > }
> >
> > +static void skl_power_well_post_enable(struct drm_i915_private *dev_priv,
> > + struct i915_power_well *power_well)
> > +{
> > + struct drm_device *dev = dev_priv->dev;
> > +
> > + /*
> > + * After we re-enable the power well, if we touch VGA register 0x3d5
> > + * we'll get unclaimed register interrupts. This stops after we write
> > + * anything to the VGA MSR register. The vgacon module uses this
> > + * register all the time, so if we unbind our driver and, as a
> > + * consequence, bind vgacon, we'll get stuck in an infinite loop at
> > + * console_unlock(). So make here we touch the VGA MSR register, making
> > + * sure vgacon can keep working normally without triggering interrupts
> > + * and error messages.
> > + */
> > + if (power_well->data == SKL_DISP_PW_2) {
> > + vga_get_uninterruptible(dev->pdev, VGA_RSRC_LEGACY_IO);
> > + outb(inb(VGA_MSR_READ), VGA_MSR_WRITE);
> > + vga_put(dev->pdev, VGA_RSRC_LEGACY_IO);
> > +
> > + gen8_irq_power_well_post_enable(dev_priv,
> > + 1 << PIPE_C | 1 << PIPE_B);
> > + }
> > +
> > + if (power_well->data == SKL_DISP_PW_1)
> > + gen8_irq_power_well_post_enable(dev_priv, 1 << PIPE_A);
> > +}
> > +
> > static void hsw_set_power_well(struct drm_i915_private *dev_priv,
> > struct i915_power_well *power_well, bool enable)
> > {
> > @@ -359,6 +387,9 @@ static void skl_set_power_well(struct drm_i915_private *dev_priv,
> > DRM_ERROR("PG2 distributing status timeout\n");
> > }
> > }
> > +
> > + if (enable)
> > + skl_power_well_post_enable(dev_priv, power_well);
>
> Please take a look at "drm/i915: only run hsw_power_well_post_enable
> when really needed": http://patchwork.freedesktop.org/patch/34764/ and
> please do the equivalent change here. I also won't complain if you
> create "is_enabled" and "enable_requested" variables on
> skl_set_power_well, just like we have for hsw_set_power_well.
tbh I wonder whether we should move the irq enable/disable into the crtc
enable/disable functions on gen8+. That would have avoided the "have we
enabled this already" logic, which tends to be fragile in general. Otoh
this works too.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
