[Intel-gfx] [PATCH] drm/i915: Fixup hpd irq register setup ordering
Imre Deak
imre.deak at intel.com
Tue Dec 11 16:58:48 CET 2012
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;
More information about the Intel-gfx
mailing list