On 3/1/2016 5:03 AM, Rob Clark wrote:
> 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
i agree. performing a modeset on a display without even confirming
if it is the same one when we had suspended the system will result in
issues such as applying a mode that may not be supported on the
new display or corruption or blankout or simply failing the modeset
restore or worst case of doing modeset without a display connected.
if we will not allow such a scenario during normal operation, i.e prevent
incorrect modeset requests during normal functioning, why allow such
a modeset during suspend-resume alone ?
we can store the edid hash and if the display has the same hash
post resume then we can allow mode to be restored.
regards,
Sivakumar
>> 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
