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

[Rob Clark]

On Mon, Feb 29, 2016 at 11:12 AM, Daniel Vetter <daniel at 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..

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..

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 at vger.kernel.org
>> >Signed-off-by: Lyude <cpaul at 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 at 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 at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx