Re: [PATCH] drm/i915: Resume DP MST before doing any kind of modesetting

[Daniel Vetter <daniel@ffwll.ch>]

On Mon, Feb 29, 2016 at 06:33:42PM -0500, Rob Clark wrote:
> On Mon, Feb 29, 2016 at 11:12 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Wed, Feb 24, 2016 at 08:03:04AM +0530, Thulasimani, Sivakumar wrote:
> >>
> >>
> >> On 2/24/2016 3:41 AM, Lyude wrote:
> >> >As it turns out, resuming DP MST is racey since we don't make sure MST
> >> >is ready before we start modesetting, it just usually happens to be
> >> >ready in time. This isn't the case on all systems, particularly a
> >> >ThinkPad T560 with displays connected through the dock. On these
> >> >systems, resuming the laptop while connected to the dock usually results
> >> >in blank monitors. Making sure MST is ready before doing any kind of
> >> >modesetting fixes this issue.
> >> basic question since i haven't worked on MST much. MST should work like any
> >> other digital panel on resume. i.e detect followed by modeset. in the
> >> modified
> >> commit mentioned below is it failing to detect the panel or failing at the
> >> modeset ?
> >> if we are depending on the intel_display_resume, how about moving the
> >> intel_dp_mst_resume just above intel_display_resume?
> >>
> >>
> >> Generic question to others in mail list on i915_drm_resume
> >> we are doing a modeset and then doing the detect/hpd init.
> >> shouldn't this be the other way round ? almost all displays
> >> will pass a modeset even if display is not connected so we
> >> are spending time on modeset even for displays that were
> >> removed during the suspend state. if this is to simply
> >> drm_state being saved and restored,
> >
> > We must restore anyway, we're not really allowed to shut down a display
> > without userspace's consent. DP mst breaks this, and it's not fun really.
> 
> well, that isn't completely true.. I mean, if the user unplugs (for
> example) an hdmi monitor, it isn't with userspace's consent..

But the pipe keeps running, which means the next pageflip or vblank wait
won't just fail and result in hilarity.

> I wonder if we could just have flag per connector, ie.
> connector->no_auto_resume and just automatically send unplug events
> for those to userspace (followed shortly by a plug event if it is
> still connected and the normal hpd mechanism kicks in?  I mean, for
> all we know, the user unplugged the DP monitor/hub/etc and plugged in
> a different one while we were suspended..

We need to be able to restore mst to avoid upsetting the desktop. I don't
think this would work.
-Daniel

> 
> BR,
> -R
> 
> > So for hotunplug the flow should always be:
> > 1. kernel notices something has changed in the output config.
> > 2. kernel sends out uevent
> > 3. userspace figures out what changed, and what needs to be done
> > 4. userspace asks the kernel to change display configuration through
> > setCrtc and Atomic ioctl calls.
> >
> > Irrespective of hotunplug handling, the kernel also _must_ restore the
> > entire display configuration before userspace resumes. We can do that
> > asynchronously (and there's plans for that), but even then we must stall
> > userspace on the first KMS ioclt to keep up the illusion that a system s/r
> > is transparent.
> >
> > In short, even if we do hpd processing before resuming the display,
> > nothing will be faster - we must wait for userspace to make up its mind,
> > and that can only happen once we've restored the display config.
> >
> > And again, mst is kinda breaking this, since and mst unplug results in a
> > force-disable. Which can upset userspace and in general results in the
> > need for lots of fragile error handling all over.
> >
> >> >We originally changed the resume order in
> >> >
> >> >     commit e7d6f7d70829 ("drm/i915: resume MST after reading back hw state")
> >> >
> >> >to fix a ton of WARN_ON's after resume, but this doesn't seem to be an
> >> >issue now anyhow.
> >> >
> >> >CC: stable@vger.kernel.org
> >> >Signed-off-by: Lyude <cpaul@redhat.com>
> >
> > Wrt the patch itself: I think only in 4.6 we've actually fixed up all
> > these mst WARN_ON. Cc: stable seems extremely risky on this one. Also, the
> > modeset state readout for mst is still not entirely correct (it still
> > relies on software state), so I'm not sure we've actually managed to shut
> > up all the WARNINGs. Iirc the way to repro them is to hot-unplug something
> > while suspended. In short the get_hw_state functions for mst should not
> > rely on any mst software state, but instead just look at hw registers and
> > dp aux registers to figure out what's going on. But not sure whether this
> > will result on WARNINGs on resume, since generally the bios doesn't enable
> > anything in that case.
> >
> > Furthermore MST still does a force-modeset when processing a hotunplug.
> > Doing that before we've resumed the display is likely a very bad idea.
> > What we need to fix that part is to separate the dp mst connector
> > hotplug/unplugging from actually updating the modeset change. This needs
> > reference-counting on drm_connector (so that we can lazily free
> > drm_connector structs after hot-unplug), and is a major change.
> >
> > In short: I'm scared about this patch ;-)
> >
> > Thanks, Daniel
> >
> >
> >> >---
> >> >  drivers/gpu/drm/i915/i915_drv.c | 10 ++++++++--
> >> >  1 file changed, 8 insertions(+), 2 deletions(-)
> >> >
> >> >diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> >> >index f357058..4dcf3dd 100644
> >> >--- a/drivers/gpu/drm/i915/i915_drv.c
> >> >+++ b/drivers/gpu/drm/i915/i915_drv.c
> >> >@@ -733,6 +733,14 @@ static int i915_drm_resume(struct drm_device *dev)
> >> >     intel_opregion_setup(dev);
> >> >     intel_init_pch_refclk(dev);
> >> >+
> >> >+    /*
> >> >+     * We need to make sure that we resume MST before doing anything
> >> >+     * display related, otherwise we risk trying to bring up a display on
> >> >+     * MST before the hub is actually ready
> >> >+     */
> >> >+    intel_dp_mst_resume(dev);
> >> >+
> >> This does not look proper. if the CD clock is turned off during suspend
> >> our dpcd read itself might fail till we enable CD Clock.
> >>
> >> regards,
> >> Sivakumar
> >> >     drm_mode_config_reset(dev);
> >> >     /*
> >> >@@ -765,8 +773,6 @@ static int i915_drm_resume(struct drm_device *dev)
> >> >     intel_display_resume(dev);
> >> >     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
> >>
> >> _______________________________________________
> >> Intel-gfx mailing list
> >> Intel-gfx@lists.freedesktop.org
> >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx