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

Rob Clark robdclark at gmail.com
Tue Mar 1 01:38:14 UTC 2016 

On Mon, Feb 29, 2016 at 7:47 PM, Thulasimani, Sivakumar
<sivakumar.thulasimani at intel.com> wrote:
>
>
> 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.

that seems like a good idea for HDMI/DVI/etc.. although I suspect for
DP there may be 50 more shades of grey.. ie. same monitor re-attached
through different sequence of hubs.. :-(

BR,
-R


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

More information about the Intel-gfx mailing list