Re: [PATCH v2 3/3] drm/i915: Enable polling when we don't have hpd

[Lyude Paul <cpaul@redhat.com>]

As a forewarning: I have confirmed the CRT hpd loops Vsyrjala has mentioned, and
they are triggered by this patch. I'm working on a patch for this atm.

On Mon, 2016-06-20 at 16:57 -0400, Lyude wrote:
> Unfortunately, there's two situations where we lose hpd right now:
> - Runtime suspend
> - When we've shut off all of the power wells on Valleyview/Cherryview
> 
> While it would be nice if this didn't cause issues, this has the
> ability to get us in some awkward states where a user won't be able to
> get their display to turn on. For instance; if we boot a Valleyview
> system without any monitors connected, it won't need any of it's power
> wells and thus shut them off. Since this causes us to lose HPD, this
> means that unless the user knows how to ssh into their machine and do a
> manual reprobe for monitors, none of the monitors they connect after
> booting will actually work.
> 
> Eventually we should come up with a better fix then having to enable
> polling for this, since this makes rpm a lot less useful, but for now
> the infrastructure in i915 just isn't there yet to get hpd in these
> situations.
> 
> Changes since v1:
>  - Add comment explaining the addition of the if
>    (!mode_config->poll_running) in intel_hpd_init()
>  - Remove unneeded if (!dev->mode_config.poll_enabled) in
>    i915_hpd_poll_init_work()
>  - Call to drm_helper_hpd_irq_event() after we disable polling
>  - Add cancel_work_sync() call to intel_hpd_cancel_work()
> 
> Cc: stable@vger.kernel.org
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> Signed-off-by: Lyude <cpaul@redhat.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.c         |  7 ++-
>  drivers/gpu/drm/i915/i915_drv.h         |  3 ++
>  drivers/gpu/drm/i915/intel_drv.h        |  2 +
>  drivers/gpu/drm/i915/intel_hotplug.c    | 82 +++++++++++++++++++++++++++++---
> -
>  drivers/gpu/drm/i915/intel_runtime_pm.c |  3 ++
>  5 files changed, 86 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 3eb47fb..5381d9e 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1612,6 +1612,9 @@ static int intel_runtime_suspend(struct device *device)
>  
>  	assert_forcewakes_inactive(dev_priv);
>  
> +	if (!IS_VALLEYVIEW(dev_priv) || !IS_CHERRYVIEW(dev_priv))
> +		intel_hpd_poll_enable(dev_priv, true);
> +
>  	DRM_DEBUG_KMS("Device suspended\n");
>  	return 0;
>  }
> @@ -1667,8 +1670,10 @@ static int intel_runtime_resume(struct device *device)
>  	 * power well, so hpd is reinitialized from there. For
>  	 * everyone else do it here.
>  	 */
> -	if (!IS_VALLEYVIEW(dev_priv) && !IS_CHERRYVIEW(dev_priv))
> +	if (!IS_VALLEYVIEW(dev_priv) && !IS_CHERRYVIEW(dev_priv)) {
>  		intel_hpd_init(dev_priv);
> +		intel_hpd_poll_enable(dev_priv, false);
> +	}
>  
>  	intel_enable_gt_powersave(dev_priv);
>  
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 1eef5ef..6f7075d 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -283,6 +283,9 @@ struct i915_hotplug {
>  	u32 short_port_mask;
>  	struct work_struct dig_port_work;
>  
> +	struct work_struct poll_enable_work;
> +	bool poll_enabled;
> +
>  	/*
>  	 * if we get a HPD irq from DP and a HPD irq from non-DP
>  	 * the non-DP HPD could block the workqueue on a mode config
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index cd14751..87c5143 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1384,6 +1384,8 @@ int intel_dsi_dcs_init_backlight_funcs(struct
> intel_connector *intel_connector);
>  
>  /* intel_dvo.c */
>  void intel_dvo_init(struct drm_device *dev);
> +/* intel_hotplug.c */
> +void intel_hpd_poll_enable(struct drm_i915_private *dev_priv, bool enabled);
>  
>  
>  /* legacy fbdev emulation in intel_fbdev.c */
> diff --git a/drivers/gpu/drm/i915/intel_hotplug.c
> b/drivers/gpu/drm/i915/intel_hotplug.c
> index ec3285f..3ef5096 100644
> --- a/drivers/gpu/drm/i915/intel_hotplug.c
> +++ b/drivers/gpu/drm/i915/intel_hotplug.c
> @@ -464,20 +464,34 @@ void intel_hpd_init(struct drm_i915_private *dev_priv)
>  		dev_priv->hotplug.stats[i].count = 0;
>  		dev_priv->hotplug.stats[i].state = HPD_ENABLED;
>  	}
> -	list_for_each_entry(connector, &mode_config->connector_list, head) {
> -		struct intel_connector *intel_connector =
> to_intel_connector(connector);
> -		connector->polled = intel_connector->polled;
>  
> -		/* MST has a dynamic intel_connector->encoder and it's
> reprobing
> -		 * is all handled by the MST helpers. */
> -		if (intel_connector->mst_port)
> -			continue;
> +	/*
> +	 * When we're in the midst of doing connector polling in situations
> +	 * where we don't have working hpd, we need to make sure we don't try
> +	 * changing the polling settings of any connectors, since this can
> lead
> +	 * to us setting connector->polled to DRM_CONNECTOR_POLL_HPD,
> resulting
> +	 * in DRM's polling helpers automatically skipping the rest of the
> +	 * connectors that still need polling
> +	 */
> +	if (!mode_config->poll_running) {
> +		list_for_each_entry(connector, &mode_config->connector_list,
> head) {
> +			struct intel_connector *intel_connector =
> +				to_intel_connector(connector);
> +			connector->polled = intel_connector->polled;
>  
> -		if (!connector->polled && I915_HAS_HOTPLUG(dev) &&
> -		    intel_connector->encoder->hpd_pin > HPD_NONE)
> -			connector->polled = DRM_CONNECTOR_POLL_HPD;
> +			/* MST has a dynamic intel_connector->encoder and
> it's
> +			 * reprobing is all handled by the MST helpers. */
> +			if (intel_connector->mst_port)
> +				continue;
> +
> +			if (!connector->polled && I915_HAS_HOTPLUG(dev) &&
> +			    intel_connector->encoder->hpd_pin > HPD_NONE)
> +				connector->polled = DRM_CONNECTOR_POLL_HPD;
> +		}
>  	}
>  
> +	intel_hpd_poll_enable(dev_priv, false);
> +
>  	/*
>  	 * Interrupt setup is already guaranteed to be single-threaded, this
> is
>  	 * just to make the assert_spin_locked checks happy.
> @@ -488,10 +502,57 @@ void intel_hpd_init(struct drm_i915_private *dev_priv)
>  	spin_unlock_irq(&dev_priv->irq_lock);
>  }
>  
> +void i915_hpd_poll_init_work(struct work_struct *work) {
> +	struct drm_i915_private *dev_priv =
> +		container_of(work, struct drm_i915_private,
> +			     hotplug.poll_enable_work);
> +	struct drm_device *dev = dev_priv->dev;
> +	struct intel_connector *intel_connector;
> +
> +	mutex_lock(&dev->mode_config.mutex);
> +
> +	for_each_intel_connector(dev, intel_connector) {
> +		struct drm_connector *connector = &intel_connector->base;
> +
> +		if (dev_priv->hotplug.poll_enabled) {
> +			connector->polled = DRM_CONNECTOR_POLL_CONNECT |
> +				DRM_CONNECTOR_POLL_DISCONNECT;
> +
> +		} else if (!intel_connector->polled && I915_HAS_HOTPLUG(dev)
> &&
> +			   intel_connector->encoder->hpd_pin > HPD_NONE) {
> +			connector->polled = DRM_CONNECTOR_POLL_HPD;
> +		}
> +	}
> +
> +	if (dev_priv->hotplug.poll_enabled)
> +		drm_kms_helper_poll_enable_locked(dev);
> +
> +	mutex_unlock(&dev->mode_config.mutex);
> +
> +	/*
> +	 * We might have missed any hotplugs that happened while we were
> +	 * in the middle of disabling polling
> +	 */
> +	if (!dev_priv->hotplug.poll_enabled)
> +		drm_helper_hpd_irq_event(dev);
> +}
> +
> +void intel_hpd_poll_enable(struct drm_i915_private *dev_priv, bool enabled)
> +{
> +	dev_priv->hotplug.poll_enabled = enabled;
> +
> +	/*
> +	 * We might already be holding dev->mode_config.mutex, so do this in
> a
> +	 * seperate worker
> +	 */
> +	schedule_work(&dev_priv->hotplug.poll_enable_work);
> +}
> +
>  void intel_hpd_init_work(struct drm_i915_private *dev_priv)
>  {
>  	INIT_WORK(&dev_priv->hotplug.hotplug_work, i915_hotplug_work_func);
>  	INIT_WORK(&dev_priv->hotplug.dig_port_work, i915_digport_work_func);
> +	INIT_WORK(&dev_priv->hotplug.poll_enable_work,
> i915_hpd_poll_init_work);
>  	INIT_DELAYED_WORK(&dev_priv->hotplug.reenable_work,
>  			  intel_hpd_irq_storm_reenable_work);
>  }
> @@ -508,6 +569,7 @@ void intel_hpd_cancel_work(struct drm_i915_private
> *dev_priv)
>  
>  	cancel_work_sync(&dev_priv->hotplug.dig_port_work);
>  	cancel_work_sync(&dev_priv->hotplug.hotplug_work);
> +	cancel_work_sync(&dev_priv->hotplug.poll_enable_work);
>  	cancel_delayed_work_sync(&dev_priv->hotplug.reenable_work);
>  }
>  
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c
> b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index 6f3d019..592cb7d 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -1099,6 +1099,7 @@ static void vlv_display_power_well_init(struct
> drm_i915_private *dev_priv)
>  	if (dev_priv->power_domains.initializing)
>  		return;
>  
> +	intel_hpd_poll_enable(dev_priv, false);
>  	intel_hpd_init(dev_priv);
>  
>  	/* Re-enable the ADPA, if we have one */
> @@ -1120,6 +1121,8 @@ static void vlv_display_power_well_deinit(struct
> drm_i915_private *dev_priv)
>  	synchronize_irq(dev_priv->dev->irq);
>  
>  	vlv_power_sequencer_reset(dev_priv);
> +
> +	intel_hpd_poll_enable(dev_priv, true);
>  }
>  
>  static void vlv_display_power_well_enable(struct drm_i915_private *dev_priv,
-- 
Cheers,
	Lyude

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx