On Wed, Mar 2, 2016 at 4:29 AM, Daniel Vetter daniel@ffwll.ch wrote:
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.
well, then shut it down? But what do we do on hot unplug? It seems like we should be able to make it look like an unplug plus later replug.
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.
I think the desktop already supports hotplug, or we'd have bigger issues.
What happens if the DP topology changes while suspended. Ie. user plugs in a different monitor with a hub in between, or things like that? Seems like you can end up in scenarios where previous mode is no longer valid/possible.
BR, -R
-Daniel
BR, -R
So for hotunplug the flow should always be:
- kernel notices something has changed in the output config.
- kernel sends out uevent
- userspace figures out what changed, and what needs to be done
- 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