| Message ID | 1452023588-3857-1-git-send-email-cpaul@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Jan 05, 2016 at 02:53:08PM -0500, Lyude wrote: > There's no reason to check for an active encoder when setting up hpd, > especially since a disconnected connector isn't going to have an encoder > in the first place. This fixes reprobing display connectors on resume. > > Signed-off-by: Lyude <cpaul@redhat.com> > --- > After some talking with danvet on IRC, I learned that calling > drm_helper_hpd_irq_event() does actually trigger a full reprobe of each > connector's status. It turns out this is the actual reason reprobing on resume > hasn't been working (this was observed on a T440s): > > - We call hpd_init() > - We check each connector for a couple of things before marking > connector->polled with DRM_CONNECTOR_POLL_HPD, one of which is an > active encoder. Of course, a disconnected port won't have an active > encoder, so we don't add the flag to any of the connector's. > - We call drm_helper_hpd_irq_event() > - drm_helper_irq_event() checks each connector for the > DRM_CONNECTOR_POLL_HPD flag. The only one that has it is eDP-1, so we > skip reprobing each connector except that one. The above explanation should be part of the commit message, otherwise it's hard to understand the impact of this fix. Fixes: 0e32b39ceed6 ("drm/i915: add DP 1.2 MST support (v0.7)") Cc: stable@vger.kernel.org Unfortunately Dave doesn't explain in his MST commit why he's done this. Dave? I can't come up with a reason, but better to test that. Lyude? Anyway looks good to me. Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > drivers/gpu/drm/i915/intel_hotplug.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/intel_hotplug.c b/drivers/gpu/drm/i915/intel_hotplug.c > index b177857..18797bc 100644 > --- a/drivers/gpu/drm/i915/intel_hotplug.c > +++ b/drivers/gpu/drm/i915/intel_hotplug.c > @@ -468,7 +468,7 @@ void intel_hpd_init(struct drm_i915_private *dev_priv) > 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->encoder && !connector->polled && I915_HAS_HOTPLUG(dev) && intel_connector->encoder->hpd_pin > HPD_NONE) > + if (!connector->polled && I915_HAS_HOTPLUG(dev) && intel_connector->encoder->hpd_pin > HPD_NONE) > connector->polled = DRM_CONNECTOR_POLL_HPD; > if (intel_connector->mst_port) > connector->polled = DRM_CONNECTOR_POLL_HPD; > -- > 2.5.0 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Tue, Jan 05, 2016 at 09:05:49PM +0100, Daniel Vetter wrote: > On Tue, Jan 05, 2016 at 02:53:08PM -0500, Lyude wrote: > > There's no reason to check for an active encoder when setting up hpd, > > especially since a disconnected connector isn't going to have an encoder > > in the first place. This fixes reprobing display connectors on resume. > > > > Signed-off-by: Lyude <cpaul@redhat.com> > > --- > > After some talking with danvet on IRC, I learned that calling > > drm_helper_hpd_irq_event() does actually trigger a full reprobe of each > > connector's status. It turns out this is the actual reason reprobing on resume > > hasn't been working (this was observed on a T440s): > > > > - We call hpd_init() > > - We check each connector for a couple of things before marking > > connector->polled with DRM_CONNECTOR_POLL_HPD, one of which is an > > active encoder. Of course, a disconnected port won't have an active > > encoder, so we don't add the flag to any of the connector's. > > - We call drm_helper_hpd_irq_event() > > - drm_helper_irq_event() checks each connector for the > > DRM_CONNECTOR_POLL_HPD flag. The only one that has it is eDP-1, so we > > skip reprobing each connector except that one. > > The above explanation should be part of the commit message, otherwise it's > hard to understand the impact of this fix. > > Fixes: 0e32b39ceed6 ("drm/i915: add DP 1.2 MST support (v0.7)") > Cc: stable@vger.kernel.org > > Unfortunately Dave doesn't explain in his MST commit why he's done this. > Dave? I can't come up with a reason, but better to test that. Lyude? > > Anyway looks good to me. > > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> Need to retract my r-b. MST connectors don't have a fixed fake encoder, and the intel_connector->encoder link is done at link time and not static. We need to filter out MST connectors here, otherwise this will go boom. While at it, can you please line-break this properly to appease checkpatch? -Daniel > > > > > drivers/gpu/drm/i915/intel_hotplug.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_hotplug.c b/drivers/gpu/drm/i915/intel_hotplug.c > > index b177857..18797bc 100644 > > --- a/drivers/gpu/drm/i915/intel_hotplug.c > > +++ b/drivers/gpu/drm/i915/intel_hotplug.c > > @@ -468,7 +468,7 @@ void intel_hpd_init(struct drm_i915_private *dev_priv) > > 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->encoder && !connector->polled && I915_HAS_HOTPLUG(dev) && intel_connector->encoder->hpd_pin > HPD_NONE) > > + if (!connector->polled && I915_HAS_HOTPLUG(dev) && intel_connector->encoder->hpd_pin > HPD_NONE) > > connector->polled = DRM_CONNECTOR_POLL_HPD; > > if (intel_connector->mst_port) > > connector->polled = DRM_CONNECTOR_POLL_HPD; > > -- > > 2.5.0 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch
diff --git a/drivers/gpu/drm/i915/intel_hotplug.c b/drivers/gpu/drm/i915/intel_hotplug.c index b177857..18797bc 100644 --- a/drivers/gpu/drm/i915/intel_hotplug.c +++ b/drivers/gpu/drm/i915/intel_hotplug.c @@ -468,7 +468,7 @@ void intel_hpd_init(struct drm_i915_private *dev_priv) 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->encoder && !connector->polled && I915_HAS_HOTPLUG(dev) && intel_connector->encoder->hpd_pin > HPD_NONE) + if (!connector->polled && I915_HAS_HOTPLUG(dev) && intel_connector->encoder->hpd_pin > HPD_NONE) connector->polled = DRM_CONNECTOR_POLL_HPD; if (intel_connector->mst_port) connector->polled = DRM_CONNECTOR_POLL_HPD;
There's no reason to check for an active encoder when setting up hpd, especially since a disconnected connector isn't going to have an encoder in the first place. This fixes reprobing display connectors on resume. Signed-off-by: Lyude <cpaul@redhat.com> --- After some talking with danvet on IRC, I learned that calling drm_helper_hpd_irq_event() does actually trigger a full reprobe of each connector's status. It turns out this is the actual reason reprobing on resume hasn't been working (this was observed on a T440s): - We call hpd_init() - We check each connector for a couple of things before marking connector->polled with DRM_CONNECTOR_POLL_HPD, one of which is an active encoder. Of course, a disconnected port won't have an active encoder, so we don't add the flag to any of the connector's. - We call drm_helper_hpd_irq_event() - drm_helper_irq_event() checks each connector for the DRM_CONNECTOR_POLL_HPD flag. The only one that has it is eDP-1, so we skip reprobing each connector except that one. drivers/gpu/drm/i915/intel_hotplug.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)