Re: [PATCH] drm/i915: implement ibx_hpd_irq_setup

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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

-- 
Ville Syrjälä
Intel OTC