drm/i915: resume MST after reading back hw state

Commit Message

Dave Airlie Dec. 8, 2014, 3:23 a.m. UTC

From: Dave Airlie <[e-mail]>

Otherwise the MST resume paths can hit DPMS paths
which hit state checker paths, which hit WARN_ON,
because the state checker is inconsistent with the
hw.

This fixes a bunch of WARN_ON's on resume after
undocking.

Signed-off-by: Dave Airlie <[e-mail]>
---
 drivers/gpu/drm/i915/i915_drv.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Daniel Vetter Dec. 8, 2014, 9:44 a.m. UTC | #1

On Mon, Dec 08, 2014 at 01:23:37PM +1000, Dave Airlie wrote:
> From: Dave Airlie <[e-mail]>
> 
> Otherwise the MST resume paths can hit DPMS paths
> which hit state checker paths, which hit WARN_ON,
> because the state checker is inconsistent with the
> hw.
> 
> This fixes a bunch of WARN_ON's on resume after
> undocking.
> 
> Signed-off-by: Dave Airlie <[e-mail]>

Makes sense, so Reviewed-by: Daniel Vetter <[e-mail]>

And Cc: [e-mail] when Jani picks it up.

While reading around I've noticed though that we might miss dp mst changes
after resume:
- intel_dp_mst_resume checks for can_mst, so will skip if we didn't plug
  in an mst thing before suspend.
- drm_helper_hpd_irq_event only does the locked detect dance and doesn't
  do the unlocked mst dance we do before calling down into ->detect
  callbacks.

So I think if we plug in an mst dock while suspended and then resume
we'll miss the hotplug in the kernel. Or do I miss something?
-Daniel

Daniel Vetter Dec. 8, 2014, 9:51 a.m. UTC | #2

On Mon, Dec 08, 2014 at 10:44:54AM +0100, Daniel Vetter wrote:
> On Mon, Dec 08, 2014 at 01:23:37PM +1000, Dave Airlie wrote:
> > From: Dave Airlie <[e-mail]>
> > 
> > Otherwise the MST resume paths can hit DPMS paths
> > which hit state checker paths, which hit WARN_ON,
> > because the state checker is inconsistent with the
> > hw.
> > 
> > This fixes a bunch of WARN_ON's on resume after
> > undocking.
> > 
> > Signed-off-by: Dave Airlie <[e-mail]>
> 
> Makes sense, so Reviewed-by: Daniel Vetter <[e-mail]>
> 
> And Cc: [e-mail] when Jani picks it up.
> 
> While reading around I've noticed though that we might miss dp mst changes
> after resume:
> - intel_dp_mst_resume checks for can_mst, so will skip if we didn't plug
>   in an mst thing before suspend.
> - drm_helper_hpd_irq_event only does the locked detect dance and doesn't
>   do the unlocked mst dance we do before calling down into ->detect
>   callbacks.
> 
> So I think if we plug in an mst dock while suspended and then resume
> we'll miss the hotplug in the kernel. Or do I miss something?

Aside: On some ports/platforms hpd pins don't work until hpd interrupts
are enabled. So maybe we should take the full reprobe out of
intel_dp_mst_resume anyway and replace drm_helper_hpd_irq_event with a
call to schedule our async hpd worker with all port bits set?
-Daniel

Jani Nikula Dec. 8, 2014, 12:32 p.m. UTC | #3

On Mon, 08 Dec 2014, Daniel Vetter <[e-mail]> wrote:
> On Mon, Dec 08, 2014 at 01:23:37PM +1000, Dave Airlie wrote:
>> From: Dave Airlie <[e-mail]>
>> 
>> Otherwise the MST resume paths can hit DPMS paths
>> which hit state checker paths, which hit WARN_ON,
>> because the state checker is inconsistent with the
>> hw.
>> 
>> This fixes a bunch of WARN_ON's on resume after
>> undocking.
>> 
>> Signed-off-by: Dave Airlie <[e-mail]>
>
> Makes sense, so Reviewed-by: Daniel Vetter <[e-mail]>

Pushed to drm-intel-next-fixes, thanks for the patch and review.

BR,
Jani.

>
> And Cc: [e-mail] when Jani picks it up.
>
> While reading around I've noticed though that we might miss dp mst changes
> after resume:
> - intel_dp_mst_resume checks for can_mst, so will skip if we didn't plug
>   in an mst thing before suspend.
> - drm_helper_hpd_irq_event only does the locked detect dance and doesn't
>   do the unlocked mst dance we do before calling down into ->detect
>   callbacks.
>
> So I think if we plug in an mst dock while suspended and then resume
> we'll miss the hotplug in the kernel. Or do I miss something?
> -Daniel
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> [phone] - http://blog.ffwll.ch

Shuang He Dec. 9, 2014, 1 p.m. UTC | #4

Tested-By: PRC QA PRTS (Patch Regression Test System Contact: [e-mail])
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                                  364/364              364/364
ILK              +3                 362/366              365/366
SNB                                  448/450              448/450
IVB                                  497/498              497/498
BYT                                  289/289              289/289
HSW                                  563/564              563/564
BDW                                  417/417              417/417
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
 ILK  igt_kms_flip_nonblocking-read      DMESG_WARN(1, M26)PASS(3, M26M37)      PASS(1, M37)
 ILK  igt_kms_setmode_invalid-clone-exclusive-crtc      DMESG_WARN(1, M26)PASS(3, M26M37)      PASS(1, M37)
 ILK  igt_kms_flip_flip-vs-rmfb-interruptible      DMESG_WARN(1, M26)PASS(3, M26M37)      PASS(1, M37)
Note: You need to pay more attention to line start with '*'

Patch

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 1e9c136..f990ab4 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -706,11 +706,12 @@  static int i915_drm_resume(struct drm_device *dev)
 			dev_priv->display.hpd_irq_setup(dev);
 		spin_unlock_irq(&dev_priv->irq_lock);
 
-		intel_dp_mst_resume(dev);
 		drm_modeset_lock_all(dev);
 		intel_modeset_setup_hw_state(dev, true);
 		drm_modeset_unlock_all(dev);
 
+		intel_dp_mst_resume(dev);
+
 		/*
 		 * ... but also need to make sure that hotplug processing
 		 * doesn't cause havoc. Like in the driver load code we don't