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@xxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
