| Message ID | 1466456253-30493-1-git-send-email-cpaul@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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,
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,