| Message ID | 20140520141016.3dcd5658@jbarnes-desktop (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 20 May 2014 14:10:16 -0700 Jesse Barnes <jbarnes@virtuousgeek.org> wrote: > On Tue, 20 May 2014 22:15:45 +0200 > Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > > > On Tue, May 20, 2014 at 9:58 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote: > > > Ah, poll_enable is false until after _thaw finishes, so > > > our hotplug_resume call of hpd_irq_event does nothing. So aside from > > > the encoder hot_plug callbacks (which really just check dp link status, > > > which ought to be a no-op) our resume_hotplug function doesn't do > > > anything right now. May as well kill it and just send an unconditional > > > uevent? > > > > That's expensive since the reprobe will likely result in a few dropped > > frames (if the edid reading takes a long time). Better to do that in > > the kernel and avoid sending the uevent if nothing changed. Which iirc > > hpd_irq_event does for us. > > Well delaying the resume by a long time isn't much better, but I guess > we need to fix this somehow. > > Knut, does this patch also address your issue? It should reset the > connector status fields correctly so the X driver doesn't have to work > around things... > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > index b948c71..e725235 100644 > --- a/drivers/gpu/drm/i915/i915_drv.c > +++ b/drivers/gpu/drm/i915/i915_drv.c > @@ -623,8 +623,6 @@ static int __i915_drm_thaw(struct drm_device *dev, bool restore_gtt_mappings) > * */ > intel_hpd_init(dev); > dev_priv->enable_hotplug_processing = true; > - /* Config may have changed between suspend and resume */ > - intel_resume_hotplug(dev); > } > > intel_opregion_init(dev); > @@ -694,6 +692,11 @@ int i915_resume(struct drm_device *dev) > return ret; > > drm_kms_helper_poll_enable(dev); > + if (drm_core_check_feature(dev, DRIVER_MODESET)) { > + /* Config may have changed between suspend and resume */ > + intel_resume_hotplug(dev); > + } > + > return 0; > } > Bah nevermind, this won't do anything. I was thinking that poll_enable controlled the poll_enabled field, but it doesn't, it just schedules or cancels the work for it. So we're still left wondering why the connector status fields are incorrect on resume in this case...
On Tue, 20 May 2014 14:18:07 -0700 Jesse Barnes <jbarnes@virtuousgeek.org> wrote: > On Tue, 20 May 2014 14:10:16 -0700 > Jesse Barnes <jbarnes@virtuousgeek.org> wrote: > > > On Tue, 20 May 2014 22:15:45 +0200 > > Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > > > > > On Tue, May 20, 2014 at 9:58 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote: > > > > Ah, poll_enable is false until after _thaw finishes, so > > > > our hotplug_resume call of hpd_irq_event does nothing. So aside from > > > > the encoder hot_plug callbacks (which really just check dp link status, > > > > which ought to be a no-op) our resume_hotplug function doesn't do > > > > anything right now. May as well kill it and just send an unconditional > > > > uevent? > > > > > > That's expensive since the reprobe will likely result in a few dropped > > > frames (if the edid reading takes a long time). Better to do that in > > > the kernel and avoid sending the uevent if nothing changed. Which iirc > > > hpd_irq_event does for us. > > > > Well delaying the resume by a long time isn't much better, but I guess > > we need to fix this somehow. > > > > Knut, does this patch also address your issue? It should reset the > > connector status fields correctly so the X driver doesn't have to work > > around things... > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > > index b948c71..e725235 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.c > > +++ b/drivers/gpu/drm/i915/i915_drv.c > > @@ -623,8 +623,6 @@ static int __i915_drm_thaw(struct drm_device *dev, bool restore_gtt_mappings) > > * */ > > intel_hpd_init(dev); > > dev_priv->enable_hotplug_processing = true; > > - /* Config may have changed between suspend and resume */ > > - intel_resume_hotplug(dev); > > } > > > > intel_opregion_init(dev); > > @@ -694,6 +692,11 @@ int i915_resume(struct drm_device *dev) > > return ret; > > > > drm_kms_helper_poll_enable(dev); > > + if (drm_core_check_feature(dev, DRIVER_MODESET)) { > > + /* Config may have changed between suspend and resume */ > > + intel_resume_hotplug(dev); > > + } > > + > > return 0; > > } > > > > Bah nevermind, this won't do anything. I was thinking that poll_enable > controlled the poll_enabled field, but it doesn't, it just schedules or > cancels the work for it. > > So we're still left wondering why the connector status fields are > incorrect on resume in this case... And on top of that I can't reproduce your issue here on my BYT which shares many of these code paths. It could be something i915 specific where we're not doing teardown correctly... Knut, can you run testdisplay -i from intel-gpu-tools after you kill the X server following the resume? It shows connected for me both on eDP and HDMI as expected. Thanks,
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index b948c71..e725235 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -623,8 +623,6 @@ static int __i915_drm_thaw(struct drm_device *dev, bool restore_gtt_mappings) * */ intel_hpd_init(dev); dev_priv->enable_hotplug_processing = true; - /* Config may have changed between suspend and resume */ - intel_resume_hotplug(dev); } intel_opregion_init(dev); @@ -694,6 +692,11 @@ int i915_resume(struct drm_device *dev) return ret; drm_kms_helper_poll_enable(dev); + if (drm_core_check_feature(dev, DRIVER_MODESET)) { + /* Config may have changed between suspend and resume */ + intel_resume_hotplug(dev); + } + return 0; }