[PATCH] drm/i915: Fixup hpd irq register setup ordering

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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;




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux