On Tue, 2012-12-11 at 14:05 +0100, Daniel Vetter wrote: > For GMCH platforms we set up the hpd irq registers in the irq > postinstall hook. But since we only enable the irq sources we actually > need in PORT_HOTPLUG_EN/STATUS, taking dev_priv->hotplug_supported_mask > into account, no hpd interrupt sources is enabled since > > commit 52d7ecedac3f96fb562cb482c139015372728638 > Author: Daniel Vetter <daniel.vetter at ffwll.ch> > Date: Sat Dec 1 21:03:22 2012 +0100 > > drm/i915: reorder setup sequence to have irqs for output setup > > Wrongly set-up interrupts also lead to broken hw-based load-detection > on at least GM45, resulting in ghost VGA/TV-out outputs. > > To fix this, delay the hotplug register setup until after all outputs > are set up, by moving it into a new dev_priv->display.hpd_irq_callback. > We might also move the PCH_SPLIT platforms to such a setup eventually. > > Another funny part is that we need to delay the fbdev initial config > probing until after the hpd regs are setup, for otherwise it'll detect > ghost outputs. But we can only enable the hpd interrupt handling > itself (and the output polling) _after_ that initial scan, due to > massive locking brain-damage in the fbdev setup code. Add a big > comment to explain this cute little dragon lair. > > v2: Encapsulate all the fbdev handling by wrapping the move call into > intel_fbdev_initial_config in intel_fb.c. Requested by Chris Wilson. > > v3: Applied bikeshed from Jesse Barnes. > > v4: Imre Deak noticed that we also need to call intel_hpd_init after > the drm_irqinstall calls in the gpu reset and resume paths - otherwise > hotplug will be broken. Also improve the comment a bit about why > hpd_init needs to be called before we set up the initial fbdev config. > > Bugzilla: Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=54943 > Reported-by: Chris Wilson <chris at chris-wilson.co.uk> > Signed-off-by: Daniel Vetter <daniel.vetter at ffwll.ch> Looks ok: Reviewed-by: Imre Deak <imre.deak at intel.com> > --- > drivers/gpu/drm/i915/i915_dma.c | 15 +++++++++ > drivers/gpu/drm/i915/i915_drv.c | 2 ++ > drivers/gpu/drm/i915/i915_drv.h | 2 ++ > drivers/gpu/drm/i915/i915_irq.c | 63 ++++++++++++++++++++++++++++++-------- > drivers/gpu/drm/i915/intel_drv.h | 1 + > drivers/gpu/drm/i915/intel_fb.c | 10 +++++- > 6 files changed, 79 insertions(+), 14 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c > index 9363066..0c2ab40 100644 > --- a/drivers/gpu/drm/i915/i915_dma.c > +++ b/drivers/gpu/drm/i915/i915_dma.c > @@ -1319,6 +1319,21 @@ static int i915_load_modeset_init(struct drm_device *dev) > goto cleanup_gem; > > /* Only enable hotplug handling once the fbdev is fully set up. */ > + intel_hpd_init(dev); > + > + /* > + * Some ports require correctly set-up hpd registers for detection to > + * work properly (leading to ghost connected connector status), e.g. VGA > + * on gm45. Hence we can only set up the initial fbdev config after hpd > + * irqs are fully enabled. Now we should scan for the initial config > + * only once hotplug handling is enabled, but due to screwed-up locking > + * around kms/fbdev init we can't protect the fdbev initial config > + * scanning against hotplug events. Hence do this first and ignore the > + * tiny window where we will loose hotplug notifactions. > + */ > + intel_fbdev_initial_config(dev); > + > + /* Only enable hotplug handling once the fbdev is fully set up. */ > dev_priv->enable_hotplug_processing = true; > > drm_kms_helper_poll_init(dev); > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > index a129218..fbd0b28 100644 > --- a/drivers/gpu/drm/i915/i915_drv.c > +++ b/drivers/gpu/drm/i915/i915_drv.c > @@ -566,6 +566,7 @@ static int __i915_drm_thaw(struct drm_device *dev) > intel_modeset_init_hw(dev); > intel_modeset_setup_hw_state(dev, false); > drm_irq_install(dev); > + intel_hpd_init(dev); > } > > intel_opregion_init(dev); > @@ -871,6 +872,7 @@ int i915_reset(struct drm_device *dev) > > drm_irq_uninstall(dev); > drm_irq_install(dev); > + intel_hpd_init(dev); > } else { > mutex_unlock(&dev->struct_mutex); > } > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index e9ac360..36a3bde 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -295,6 +295,7 @@ struct drm_i915_display_funcs { > struct drm_i915_gem_object *obj); > int (*update_plane)(struct drm_crtc *crtc, struct drm_framebuffer *fb, > int x, int y); > + void (*hpd_irq_setup)(struct drm_device *dev); > /* clock updates for mode set */ > /* cursor updates */ > /* render clock increase/decrease */ > @@ -1341,6 +1342,7 @@ void i915_hangcheck_elapsed(unsigned long data); > void i915_handle_error(struct drm_device *dev, bool wedged); > > extern void intel_irq_init(struct drm_device *dev); > +extern void intel_hpd_init(struct drm_device *dev); > extern void intel_gt_init(struct drm_device *dev); > extern void intel_gt_reset(struct drm_device *dev); > > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c > index 2e1d80d..de9947a 100644 > --- a/drivers/gpu/drm/i915/i915_irq.c > +++ b/drivers/gpu/drm/i915/i915_irq.c > @@ -1995,7 +1995,6 @@ static int valleyview_irq_postinstall(struct drm_device *dev) > { > drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private; > u32 enable_mask; > - u32 hotplug_en = I915_READ(PORT_HOTPLUG_EN); > u32 pipestat_enable = PLANE_FLIP_DONE_INT_EN_VLV; > u32 render_irqs; > u16 msid; > @@ -2024,6 +2023,9 @@ static int valleyview_irq_postinstall(struct drm_device *dev) > msid |= (1<<14); > pci_write_config_word(dev_priv->dev->pdev, 0x98, msid); > > + I915_WRITE(PORT_HOTPLUG_EN, 0); > + POSTING_READ(PORT_HOTPLUG_EN); > + > I915_WRITE(VLV_IMR, dev_priv->irq_mask); > I915_WRITE(VLV_IER, enable_mask); > I915_WRITE(VLV_IIR, 0xffffffff); > @@ -2053,6 +2055,15 @@ static int valleyview_irq_postinstall(struct drm_device *dev) > #endif > > I915_WRITE(VLV_MASTER_IER, MASTER_INTERRUPT_ENABLE); > + > + return 0; > +} > + > +static void valleyview_hpd_irq_setup(struct drm_device *dev) > +{ > + drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private; > + u32 hotplug_en = I915_READ(PORT_HOTPLUG_EN); > + > /* Note HDMI and DP share bits */ > if (dev_priv->hotplug_supported_mask & HDMIB_HOTPLUG_INT_STATUS) > hotplug_en |= HDMIB_HOTPLUG_INT_EN; > @@ -2070,8 +2081,6 @@ static int valleyview_irq_postinstall(struct drm_device *dev) > } > > I915_WRITE(PORT_HOTPLUG_EN, hotplug_en); > - > - return 0; > } > > static void valleyview_irq_uninstall(struct drm_device *dev) > @@ -2301,6 +2310,9 @@ static int i915_irq_postinstall(struct drm_device *dev) > I915_USER_INTERRUPT; > > if (I915_HAS_HOTPLUG(dev)) { > + I915_WRITE(PORT_HOTPLUG_EN, 0); > + POSTING_READ(PORT_HOTPLUG_EN); > + > /* Enable in IER... */ > enable_mask |= I915_DISPLAY_PORT_INTERRUPT; > /* and unmask in IMR */ > @@ -2311,8 +2323,18 @@ static int i915_irq_postinstall(struct drm_device *dev) > I915_WRITE(IER, enable_mask); > POSTING_READ(IER); > > + intel_opregion_enable_asle(dev); > + > + return 0; > +} > + > +static void i915_hpd_irq_setup(struct drm_device *dev) > +{ > + drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private; > + u32 hotplug_en; > + > if (I915_HAS_HOTPLUG(dev)) { > - u32 hotplug_en = I915_READ(PORT_HOTPLUG_EN); > + hotplug_en = I915_READ(PORT_HOTPLUG_EN); > > if (dev_priv->hotplug_supported_mask & HDMIB_HOTPLUG_INT_STATUS) > hotplug_en |= HDMIB_HOTPLUG_INT_EN; > @@ -2333,10 +2355,6 @@ static int i915_irq_postinstall(struct drm_device *dev) > > I915_WRITE(PORT_HOTPLUG_EN, hotplug_en); > } > - > - intel_opregion_enable_asle(dev); > - > - return 0; > } > > static irqreturn_t i915_irq_handler(int irq, void *arg) > @@ -2496,7 +2514,6 @@ static void i965_irq_preinstall(struct drm_device * dev) > static int i965_irq_postinstall(struct drm_device *dev) > { > drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private; > - u32 hotplug_en; > u32 enable_mask; > u32 error_mask; > > @@ -2538,6 +2555,19 @@ static int i965_irq_postinstall(struct drm_device *dev) > I915_WRITE(IER, enable_mask); > POSTING_READ(IER); > > + I915_WRITE(PORT_HOTPLUG_EN, 0); > + POSTING_READ(PORT_HOTPLUG_EN); > + > + intel_opregion_enable_asle(dev); > + > + return 0; > +} > + > +static void i965_hpd_irq_setup(struct drm_device *dev) > +{ > + drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private; > + u32 hotplug_en; > + > /* Note HDMI and DP share hotplug bits */ > hotplug_en = 0; > if (dev_priv->hotplug_supported_mask & HDMIB_HOTPLUG_INT_STATUS) > @@ -2572,10 +2602,6 @@ static int i965_irq_postinstall(struct drm_device *dev) > /* Ignore TV since it's buggy */ > > I915_WRITE(PORT_HOTPLUG_EN, hotplug_en); > - > - intel_opregion_enable_asle(dev); > - > - return 0; > } > > static irqreturn_t i965_irq_handler(int irq, void *arg) > @@ -2754,6 +2780,7 @@ void intel_irq_init(struct drm_device *dev) > dev->driver->irq_uninstall = valleyview_irq_uninstall; > dev->driver->enable_vblank = valleyview_enable_vblank; > dev->driver->disable_vblank = valleyview_disable_vblank; > + dev_priv->display.hpd_irq_setup = valleyview_hpd_irq_setup; > } else if (IS_IVYBRIDGE(dev) || IS_HASWELL(dev)) { > /* Share pre & uninstall handlers with ILK/SNB */ > dev->driver->irq_handler = ivybridge_irq_handler; > @@ -2780,13 +2807,23 @@ void intel_irq_init(struct drm_device *dev) > dev->driver->irq_postinstall = i915_irq_postinstall; > dev->driver->irq_uninstall = i915_irq_uninstall; > dev->driver->irq_handler = i915_irq_handler; > + dev_priv->display.hpd_irq_setup = i915_hpd_irq_setup; > } else { > dev->driver->irq_preinstall = i965_irq_preinstall; > dev->driver->irq_postinstall = i965_irq_postinstall; > dev->driver->irq_uninstall = i965_irq_uninstall; > dev->driver->irq_handler = i965_irq_handler; > + dev_priv->display.hpd_irq_setup = i965_hpd_irq_setup; > } > dev->driver->enable_vblank = i915_enable_vblank; > dev->driver->disable_vblank = i915_disable_vblank; > } > } > + > +void intel_hpd_init(struct drm_device *dev) > +{ > + struct drm_i915_private *dev_priv = dev->dev_private; > + > + if (dev_priv->display.hpd_irq_setup) > + dev_priv->display.hpd_irq_setup(dev); > +} > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index 7ca7772..22728f2 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -587,6 +587,7 @@ extern int intel_framebuffer_init(struct drm_device *dev, > struct drm_mode_fb_cmd2 *mode_cmd, > struct drm_i915_gem_object *obj); > extern int intel_fbdev_init(struct drm_device *dev); > +extern void intel_fbdev_initial_config(struct drm_device *dev); > extern void intel_fbdev_fini(struct drm_device *dev); > extern void intel_fbdev_set_suspend(struct drm_device *dev, int state); > extern void intel_prepare_page_flip(struct drm_device *dev, int plane); > diff --git a/drivers/gpu/drm/i915/intel_fb.c b/drivers/gpu/drm/i915/intel_fb.c > index b7773e5..82289678 100644 > --- a/drivers/gpu/drm/i915/intel_fb.c > +++ b/drivers/gpu/drm/i915/intel_fb.c > @@ -243,10 +243,18 @@ int intel_fbdev_init(struct drm_device *dev) > } > > drm_fb_helper_single_add_all_connectors(&ifbdev->helper); > - drm_fb_helper_initial_config(&ifbdev->helper, 32); > + > return 0; > } > > +void intel_fbdev_initial_config(struct drm_device *dev) > +{ > + drm_i915_private_t *dev_priv = dev->dev_private; > + > + /* Due to peculiar init order wrt to hpd handling this is separate. */ > + drm_fb_helper_initial_config(&dev_priv->fbdev->helper, 32); > +} > + > void intel_fbdev_fini(struct drm_device *dev) > { > drm_i915_private_t *dev_priv = dev->dev_private;
