Bug 24822

Summary: Embedded DisplayPort is detected wrongly on HP ProBook 5320m
Product: Drivers Reporter: Takashi Iwai (tiwai)
Component: Video(DRI - Intel)Assignee: drivers_video-dri-intel (drivers_video-dri-intel)
Severity: normal CC: chris, eric, florian, keithp, maciej.rutecki, mat, rjw, sndirsch
Priority: P1    
Hardware: All   
OS: Linux   
Kernel Version: 2.6.37-rc5 Subsystem:
Regression: Yes Bisected commit-id:
Bug Depends on:    
Bug Blocks: 21782    
Attachments: dmesg output (with backport of 2.6.37-rc5+ DRM)
Ad hoc fix patch for wrong eDP detections
Verify eDP_A using "the display chicken fuses"
Verify eDP_A using "the display chicken fuses"
Probe the eDP device on init
Probe the eDP device on init
Probe the eDP device on init
Your final fix...

Description Takashi Iwai 2010-12-13 11:09:38 UTC
On 2.6.37-rc5+ (as of commit 6313e3c), a non-existing eDP is detected on HP ProBook 5320m with IronLake.
This results in a constantly flickering screen after loading i915 module.
xrandr reports both LVDS and eDP present.
Comment 1 Takashi Iwai 2010-12-13 11:12:48 UTC
Created attachment 39972 [details]
dmesg output (with backport of 2.6.37-rc5+ DRM)

Running on SLE11-SP1, but DRM part was backported.
The result is same with vanilla 2.6.37-rc5+ kernel.
Comment 2 Takashi Iwai 2010-12-13 11:17:04 UTC
Created attachment 39982 [details]
Ad hoc fix patch for wrong eDP detections

A quick-fix patch for the wrong eDP detections
Comment 3 Chris Wilson 2010-12-13 11:30:53 UTC
Looks equally prone to a false detection of LVDS. We will probably want to validate against the VBT instead.
Comment 4 Chris Wilson 2010-12-14 17:35:35 UTC
DP_A & DP_A_DETECTED: "This bit is qualified with the Embedded DisplayPort A capability fuse."

And no, I have no idea what they mean here either.
Comment 5 Chris Wilson 2010-12-14 19:36:48 UTC
Created attachment 40172 [details]
Verify eDP_A using "the display chicken fuses"

I have no idea if this patch is indeed the correct approach, just something I stumbled across in the docs.
Comment 6 Chris Wilson 2010-12-14 19:38:12 UTC
Created attachment 40182 [details]
Verify eDP_A using "the display chicken fuses"
Comment 7 Rafael J. Wysocki 2010-12-14 19:43:54 UTC
Handled-By : Chris Wilson <chris@chris-wilson.co.uk>
Comment 8 Takashi Iwai 2010-12-15 10:00:41 UTC
Tested the patch in comment 6, but it doesn't work.
Both eDP and LVDS are still detected.
Comment 9 Chris Wilson 2011-02-12 10:38:40 UTC
Created attachment 47502 [details]
Probe the eDP device on init
Comment 10 Takashi Iwai 2011-02-14 13:03:40 UTC
Got an Oops with the patch in intel_dp_aux_ch() during module init.
Isn't it applicable to 2.6.37 kernel?
Comment 11 Chris Wilson 2011-02-14 13:16:37 UTC
No, the patch is bad. The probe is called before enough of the intel_dp is initialized.
Comment 12 Florian Mickler 2011-02-20 13:02:47 UTC
Is this still a problem in 2.6.38-rc5?
Comment 13 Takashi Iwai 2011-02-21 10:03:07 UTC
Yes, the bug still remains.
Comment 14 Chris Wilson 2011-03-06 10:52:11 UTC
Created attachment 50182 [details]
Probe the eDP device on init

Defer probe until after we have initialised the i2c adapter....
Comment 15 Chris Wilson 2011-03-06 10:55:24 UTC
Created attachment 50192 [details]
Probe the eDP device on init

Slightly tidier patch.
Comment 16 Takashi Iwai 2011-03-16 15:18:50 UTC
With the patch, I got an Oops in intel_dp_encoder_destroy().
Looks like the warning message in kobject_put() caused a wrong access, which is called from device_unregister() from i2c_del_adapter().
Comment 17 Chris Wilson 2011-03-16 15:26:36 UTC
Apart from the fact that we completely ignore any potential error when constructing the i2c device (grr), there doesn't seem to be an issue with the order of deletion here.

Can you attach the oops?
Comment 18 Takashi Iwai 2011-03-17 15:34:41 UTC
The order of the calls does matter indeed.  Swapping the call order of intel_dp_destroy() and intel_dp_encoder_destroy() fixes the problem.
This is because i2c_del_adapter unregisters the device which parent is intel_connector, and connectors are removed in intel_dp_destroy().  Thus intel_dp_encoder_destroy() must be called before intel_dp_destroy().

With that fix, now the driver doesn't initialize eDP but activates only LVDS.

The only nitpicking is that DRM_ERROR() is used in that failure path.  Since this is no real error, DRM_INFO() should be used instead, IMO.  Otherwise user would be confused and wonder what he did wrong.
Comment 19 Chris Wilson 2011-03-18 09:09:10 UTC
Created attachment 51162 [details]
Your final fix...

I've queued this patch, thanks!
Comment 20 Florian Mickler 2011-03-18 09:59:06 UTC
Patch: https://bugzilla.kernel.org/attachment.cgi?id=51162
Comment 21 Takashi Iwai 2011-03-18 11:26:35 UTC

Could you prepare the fix patch for 2.6.37/38 kernels and submit to stable kernel once after the fixes are merged to the upstream?  The patch in comment 15 can't be applied cleanly to them.
Comment 22 Florian Mickler 2011-03-28 23:05:52 UTC
A patch referencing this bug report has been merged in v2.6.38-8876-g036a982:

commit 48898b038b69ef4801f0e059026c8f6920684677
Author: Takashi Iwai <tiwai@suse.de>
Date:   Fri Mar 18 09:06:49 2011 +0000

    drm/i915/dp: Correct the order of deletion for ghost eDP devices
Comment 23 Florian Mickler 2011-03-28 23:52:35 UTC
A patch referencing this bug report has been merged in v2.6.38-8876-g036a982:

commit 3d3dc149eda48566619d165f6b34e5eeca00edf1
Author: Chris Wilson <chris@chris-wilson.co.uk>
Date:   Sat Feb 12 10:33:12 2011 +0000

    drm/i915/dp: Sanity check eDP existence
Comment 24 Chris Wilson 2011-03-29 06:41:22 UTC
Created attachment 52412 [details]

For reference, this is the patch that I intend to send to stable.
Comment 25 Takashi Iwai 2011-03-29 06:57:22 UTC
Thanks!  I see slight differences there, though.  Don't we need to change the initial dpms value to -1 for 2.6.38?  Also, in the original patch, panel is set off before returning the error.  I guess it doesn't matter much (as actually no edp is present), but just wondering the difference of code flow.