Cool, now we have the last dupe for ivb, for reference see bug #67462 	and bug #54687 Docking station unplugging is like cable pulling in this way ;-)

I encounter a similar stack in ChromeOS when unplugging DP cable.
The reason it happens for me is that intel_dp_link_down() is called first time from intel_dp_check_link_status() from intel_dp_hot_plug().
Then, during hot plug uevent processing, we decide to DPMS OFF the DP, and then intel_dp_link_down() is called a second time, so there is a warning for
WARN_ON((I915_READ(intel_dp->output_reg) & DP_PORT_EN) == 0).

I am also getting a warning in intel_modeset_check_state(), since we reconfigure internal LVDS before we DPMS OFF the DP, and during the check we hit the warning for DP:
active = encoder->get_hw_state(encoder, &pipe);
WARN(active != encoder->connectors_active,

I'd be happy to upload a fix for that, but I am not sure what would be the right direction to take with it.
From reading commit messages for intel_dp_hot_plug(), I understand that it calls intel_dp_check_link_status() "for userspace apps that don't respond to hot plug uevents". But that causes the warnings for apps that do respond to them.

I think that the warnings are irrelevant in this case. I'd like to keep track of hot plug detection, and then not warn about the DP link being down as a result of hot unplug.
Daniel, what do you think?

(In reply to comment #2)
> I think that the warnings are irrelevant in this case. I'd like to keep
> track of hot plug detection, and then not warn about the DP link being down
> as a result of hot unplug.

The dp link training code is botched since if it disables the port if it doesn't work out. Which results in warns like yours but on some systems also in deadlocks since the display pipe stops working when we expect it to run. The fix is to gracefully fail the link training by simple enabling the link at conservative settings and keep on pumping pixels into the air (like we do with all other outputs).

The comment in dp_hotplug is simply historical bonghits.

But wouldn't "enabling the link at conservative settings and keep on pumping pixels into the air" waste power?
Sounds to me that bringing the link down when unplug is detected is the power efficient way to go.
That is, if there is a safe way to do so.

As I understand, the problem with display pipe stopping is not a hardware one, but the other parts of the driver, right?
I've tried to DPMS OFF the crtc instead of intel_dp_link_down(), and it worked for me.
But, marcheu@ told me that X caches DPMS state, and the only reason it worked is because in ChromeOS we re-probe once we receive hotplug uevent, so the cache is updated properly.

Sounds to me that a better solution would be to devise a mechanism by which drm state can be updated from the hardware side.
I mean, the usual configuration flow is that userspace configures crtc, which enables / disables encoder and connector.
But, it sound reasonable to me that when a connector detects that the hardware configuration changed, it will reconfigure encoder / crtc on it's own, and notify the userspace about the change.

I've tried calling intel_modeset_setup_hw_state(force_restore==true) for that effect, and it worked, but it took a few seconds and the internal screen turned on / off twice instead of once, that I'd expect from reconfiguration.

Perhaps what we need is a more light-weight intel_modeset_setup_hw_state(), which would not touch hardware which is already in the right state, but only turn off the relevant one?

How does that sound from "right thing to do" and feasibility perspectives?

If I am just talking nonsense here, what would you suggest as "conservative settings"? 640x480@60Hz?

Please try current drm-intel-nightly, I think we've merged some patches to address this.

Yuly, are you still there? Will close if not.

Sorry, I didn't have time to test this.
Would be helpful if I knew what commits address this, so I could cherry-pick them into our branch.
BTW, I am not the original reporter of the bug, Jan-Michael is.

Jani, would you be able to come up with a list of potential commits that may already fix the issue?

I've been trying to find commits that might affect it by myself, looking at drm-intel-next branch, and I believe with the current code, the warning will still occur. The sequence of events leading to the warning is as follows:

ironlake_irq_handler()
  ilk_display_irq_handler()
    cpt_irq_handler()
      intel_hpd_irq_handler()
        i915_hotplug_work_func()
          intel_dp_hot_plug()
            intel_dp_check_link_status()
              intel_dp_get_link_status()
              intel_dp_link_down() 
          drm_helper_hpd_irq_event()
            drm_kms_helper_hotplug_event()
              drm_sysfs_hotplug_event()
                kobject_uevent_env()
<userspace uevent processing>
drm_mode_obj_set_property_ioctl()
  drm_mode_connector_set_obj_prop()
    intel_connector_dpms()
      intel_encoder_dpms()
        intel_crtc_update_dpms()
          ironlake_crtc_disable()
            intel_disable_dp()
              intel_dp_link_down()

There were several changes on these paths, but I fail to find there a change that would prevent from intel_dp_link_down() to be called twice on SNB or IVB.

(In reply to comment #8)
> Jani, would you be able to come up with a list of potential commits that may
> already fix the issue?

commit 5d6a1116c6475404e6505b708320f9579ae19acd
Author: Imre Deak <imre.deak@intel.com>
Date:   Thu Jan 16 18:35:57 2014 +0200

    drm/i915: don't disable the DP port if the link is lost

comes to mind, and it does change the analysis in comment #9. Do note that the branch to test is drm-intel-nightly, not -next.

Ah, sorry that I was looking in a wrong branch.
Indeed, the commit in question will remove the warning, as it removes the call to intel_dp_link_down() from intel_dp_check_link_status().
However, my understanding is that this call was there to support cases where userspace doesn't handle uevents, and removing it will leave DP link on and consuming power in such systems, and probably lead to errors when one monitor is plugged out and another is plugged in. Hope you are keeping track of this in some other bug.
But, for me the problem is fixed, so you can close this one.

BTW, if relying on uevents is the direction you want to proceed with, I think you can remove intel_dp_hot_plug() altogether, since currently it does nothing in case of unplug, and in case of plug, the dp link will be enabled in intel_enable_dp() anyway, when userspace decides to DPMS_ON.

Thanks, closing.

Imre, please check comment #11 and see if there's anything to worry about.

(In reply to comment #11)
> Ah, sorry that I was looking in a wrong branch.
> Indeed, the commit in question will remove the warning, as it removes the
> call to intel_dp_link_down() from intel_dp_check_link_status().
> However, my understanding is that this call was there to support cases where
> userspace doesn't handle uevents, and removing it will leave DP link on and
> consuming power in such systems, and probably lead to errors when one
> monitor is plugged out and another is plugged in. Hope you are keeping track
> of this in some other bug.

In general we do depend on user space handling uevents, I don't know any reason why guaranteeing this would pose any problem. So the subsequent modeset disable should take care of disabling the port. Note that there is still a TODO identified by Chris, for the case where a modeset fails due to link training errors and we leave the whole pipeline enabled. But that's a somewhat separate issue.

> But, for me the problem is fixed, so you can close this one.
> 
> BTW, if relying on uevents is the direction you want to proceed with, I
> think you can remove intel_dp_hot_plug() altogether, since currently it does
> nothing in case of unplug, and in case of plug, the dp link will be enabled
> in intel_enable_dp() anyway, when userspace decides to DPMS_ON.

Afaics intel_dp_hot_plug() also handles link retraining.