Re: [PATCH] drm/i915: On reset/suspend disable hpd pins & cancel pending delayed work

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

On Fri, Sep 25, 2015 at 08:09:57AM +0200, Egbert Eich wrote:
> This makes sure no hpd interrupt or reenable worker fires when
> resetting or suspending.
> We already call intel_hpd_init() in most cases on resume and
> after reset to undo this.
> 
> Signed-off-by: Egbert Eich <eich@suse.de>
> ---
>  drivers/gpu/drm/i915/i915_drv.c      |  3 +++
>  drivers/gpu/drm/i915/i915_drv.h      |  1 +
>  drivers/gpu/drm/i915/intel_display.c |  5 +++--
>  drivers/gpu/drm/i915/intel_hotplug.c | 28 ++++++++++++++++++++++++++++
>  4 files changed, 35 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index e2bf9e2..cb8d75f 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -620,6 +620,7 @@ static int i915_drm_suspend(struct drm_device *dev)
>  
>  	intel_dp_mst_suspend(dev);
>  
> +	intel_hpd_uninit(dev_priv);
>  	intel_runtime_pm_disable_interrupts(dev_priv);
>  	intel_hpd_cancel_work(dev_priv);
>  
> @@ -1477,12 +1478,14 @@ static int intel_runtime_suspend(struct device *device)
>  	mutex_unlock(&dev->struct_mutex);
>  
>  	intel_suspend_gt_powersave(dev);
> +	intel_hpd_uninit(dev_priv);
>  	intel_runtime_pm_disable_interrupts(dev_priv);
>  
>  	ret = intel_suspend_complete(dev_priv);
>  	if (ret) {
>  		DRM_ERROR("Runtime suspend failed, disabling it (%d)\n", ret);
>  		intel_runtime_pm_enable_interrupts(dev_priv);
> +		intel_hpd_init(dev_priv);
>  
>  		return ret;
>  	}
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index a6b7576..3c06629 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2688,6 +2688,7 @@ void i915_firmware_load_error_print(const char *fw_path, int err);
>  /* intel_hotplug.c */
>  void intel_hpd_irq_handler(struct drm_device *dev, u32 pin_mask, u32 long_mask);
>  void intel_hpd_init(struct drm_i915_private *dev_priv);
> +void intel_hpd_uninit(struct drm_i915_private *dev_priv);
>  void intel_hpd_init_work(struct drm_i915_private *dev_priv);
>  void intel_hpd_cancel_work(struct drm_i915_private *dev_priv);
>  bool intel_hpd_pin_to_port(enum hpd_pin pin, enum port *port);
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index fc00867..fadd4f2 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3207,16 +3207,17 @@ void intel_finish_reset(struct drm_device *dev)
>  	 * The display has been reset as well,
>  	 * so need a full re-initialization.
>  	 */
> +	intel_hpd_uninit(dev_priv);
>  	intel_runtime_pm_disable_interrupts(dev_priv);
>  	intel_runtime_pm_enable_interrupts(dev_priv);
>  
>  	intel_modeset_init_hw(dev);
> -
> +#if 0
>  	spin_lock_irq(&dev_priv->irq_lock);
>  	if (dev_priv->display.hpd_irq_setup)
>  		dev_priv->display.hpd_irq_setup(dev);
>  	spin_unlock_irq(&dev_priv->irq_lock);
> -
> +#endif

This pattern or hdp_irq_setup()+intel_hpd_init() has been cargo culted
all over the place. Can you just kill off the hpd_irq_setup() stuff from
everywhere where it's not needed?

>  	intel_display_resume(dev);
>  
>  	intel_hpd_init(dev_priv);
> diff --git a/drivers/gpu/drm/i915/intel_hotplug.c b/drivers/gpu/drm/i915/intel_hotplug.c
> index b177857..4f2a179 100644
> --- a/drivers/gpu/drm/i915/intel_hotplug.c
> +++ b/drivers/gpu/drm/i915/intel_hotplug.c
> @@ -484,6 +484,34 @@ void intel_hpd_init(struct drm_i915_private *dev_priv)
>  	spin_unlock_irq(&dev_priv->irq_lock);
>  }
>  
> +/**
> + * intel_hpd_uninit - disable and deinitialize hpd support
> + * @dev_priv: i915 device instance
> + *
> + * This function disables any pending delayed work for HPD and
> + * disables all hpd pins. Calling this during suspend/reset
> + * avoids conflicts with the reenable worker of interrupt handler
> + * firing.
> + * At resume we call intel_hpd_init() to reenable things.
> + */
> +void intel_hpd_uninit(struct drm_i915_private *dev_priv)
> +{
> +	struct drm_device *dev = dev_priv->dev;
> +	int i;
> +
> +	cancel_delayed_work_sync(&dev_priv->hotplug.reenable_work);

What if another hpd happens here?

Oh and we already have the intel_hpd_cancel_work() function. Sounds like
we need to rethink how that works rather than rolling another function
to do the same thing.

There's also the matter of VLV/CHV where hpd init gets called from the
display power well enable hook. I was thinking that could race with this
stuff, but since this is only called from the suspend I suppose we
shouldn't be re-enabling the display power well at that time.

> +
> +	spin_lock_irq(&dev_priv->irq_lock);
> +
> +	for_each_hpd_pin(i) {
> +		dev_priv->hotplug.stats[i].state = HPD_DISABLED;
> +	}
> +	if (dev_priv->display.hpd_irq_setup)
> +		dev_priv->display.hpd_irq_setup(dev);
> +
> +	spin_unlock_irq(&dev_priv->irq_lock);
> +}
> +
>  void intel_hpd_init_work(struct drm_i915_private *dev_priv)
>  {
>  	INIT_WORK(&dev_priv->hotplug.hotplug_work, i915_hotplug_work_func);
> -- 
> 1.8.4.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx