Bug 89008

Summary: [BYT BSW] No HDMI hotplugging
Product: DRI Reporter: Harald Jagenteufel <h.jagenteufel.dev>
Component: DRM/IntelAssignee: Intel GFX Bugs mailing list <intel-gfx-bugs>
Status: CLOSED FIXED QA Contact: Intel GFX Bugs mailing list <intel-gfx-bugs>
Severity: normal    
Priority: medium CC: christophe.prigent, eich, imre.deak, intel-gfx-bugs, nrndda, tjaalton, ville.syrjala
Version: unspecified   
Hardware: x86-64 (AMD64)   
OS: Linux (All)   
See Also: https://bugs.freedesktop.org/show_bug.cgi?id=89461
Whiteboard:
i915 platform: BSW/CHT, BYT i915 features: display/HDMI
Attachments:
Description Flags
dmesg after reloading i915
none
dmesg after suspend-resume and reload i915
none
quick_dump before removing i915
none
quick_dump after modprobe i915 none

Description Harald Jagenteufel 2015-02-06 13:53:24 UTC
Created attachment 113224 [details]
dmesg after reloading i915

System environment:
-- chipset: Baytrail
-- system architecture: 64-bit
-- kernel: 3.16.7-ckt2-1
-- Linux distribution: Debian Jessie
-- Machine or mobo model: Intel NUC DN2820FYK; SA H22962-103 (CPU N2830)
-- Display connector: HDMI

Reproducing steps:
Boot without monitor attached. Plug-in monitor. Monitor receives no input from the NUC.

Additional info:
BIOS version: 0047 (booting without monitor attached with ealrier BIOS versions was not possible)

Workaround:
remove i915 kernel module and insert again:
rmmod -f i915 && modprobe i915
after these steps, the monitor is recognized and works as expected. (see attached dmesg file)
Comment 1 Jani Nikula 2015-02-09 13:55:42 UTC
Does hot unplug/plug work right if you boot with monitor attached?
Comment 2 Harald Jagenteufel 2015-02-09 14:17:20 UTC
(In reply to Jani Nikula from comment #1)
> Does hot unplug/plug work right if you boot with monitor attached?

No, same behaviour. The monitor is recognized on boot, unplugging is recognized as well, but plugging in again doesn't work an shows no message in dmesg.
Comment 3 Jani Nikula 2015-02-09 15:21:37 UTC
Hmm, strange.

Please attach the output of tools/quick_dump/quick_dump.py from intel gpu tools [1], for both before you remove i915 *and* after you've probed it again. Maybe the diff in the registers gives us some clues.


[1] http://cgit.freedesktop.org/xorg/app/intel-gpu-tools/
Comment 4 Jani Nikula 2015-02-09 15:23:09 UTC
Also, independently, does a suspend/resume cycle after you've booted fix things, without reprobe?
Comment 5 Harald Jagenteufel 2015-02-09 16:44:10 UTC
Created attachment 113281 [details]
dmesg after suspend-resume and reload i915

boot without monitor attached
attach monitor, monitor gets no input
suspend to memory, resume
monitor lights up, but with wrong resolution set (lower than the native native 1920x1080)

reloading i915 at this point fixes the resolution issue.
Comment 6 Harald Jagenteufel 2015-02-09 17:10:53 UTC
Created attachment 113282 [details]
quick_dump before removing i915

boot without monitor attached
attach monitor
./quick_dump.py
Comment 7 Harald Jagenteufel 2015-02-09 17:11:50 UTC
Created attachment 113283 [details]
quick_dump after modprobe i915

rmmod -f i915
modprobe i915
./quick_dump.py
Comment 8 Jani Nikula 2015-02-10 11:25:35 UTC
Ville on #intel-gfx:

 22:40        vsyrjala   j4ni: hotplug needs disp2d well on byt. if there are 
                         no active display we turn off the power well. also 
                         most display regs are in the power well which explains 
                         the 0xffffffff
Comment 9 Egbert Eich 2015-02-18 17:11:32 UTC
(In reply to Jani Nikula from comment #8)
> Ville on #intel-gfx:
> 
>  22:40        vsyrjala   j4ni: hotplug needs disp2d well on byt. if there
> are 
>                          no active display we turn off the power well. also 
>                          most display regs are in the power well which
> explains 
>                          the 0xffffffff

Indeed, I was able to bisect this back to commit 77961eb984c7e5394bd29cc7be2ab0bf0cc7e7b1.

This begs the question if the disp2d power wells need to be disabled quite as aggressively: maybe there is only a subset needed for hotplug which can stay enabled.
The same is true for the display interrupts: HPD may be one of them.

I do understand that it is important to power down the chip as much as possible whenever feasable for ultra mobile use cases. However a lot of Baytrail chips are used in TCs or other desktop like devices which are connected to the mains - at least my system with 8086:0f31 is.
Comment 10 Ville Syrjala 2015-02-18 19:08:54 UTC
(In reply to Egbert Eich from comment #9)
> (In reply to Jani Nikula from comment #8)
> > Ville on #intel-gfx:
> > 
> >  22:40        vsyrjala   j4ni: hotplug needs disp2d well on byt. if there
> > are 
> >                          no active display we turn off the power well. also 
> >                          most display regs are in the power well which
> > explains 
> >                          the 0xffffffff
> 
> Indeed, I was able to bisect this back to commit
> 77961eb984c7e5394bd29cc7be2ab0bf0cc7e7b1.
> 
> This begs the question if the disp2d power wells need to be disabled quite
> as aggressively: maybe there is only a subset needed for hotplug which can
> stay enabled.
> The same is true for the display interrupts: HPD may be one of them.
> 
> I do understand that it is important to power down the chip as much as
> possible whenever feasable for ultra mobile use cases. However a lot of
> Baytrail chips are used in TCs or other desktop like devices which are
> connected to the mains - at least my system with 8086:0f31 is.

Sadly there's no sub-partitioning of the power well, so it's either all or nothing. Also we may even further runtime suspend the entire device. That means we should actually have the same problem on all platforms supporting runtime PM. I suppose the only thing that saves most other platforms is the default 10s delay on runtime susped we have (which we really should reduce to something like 1s).

So at the moment i915.disable_power_well=0 is the only way to guarantee that HPD keeps working at all times, but that will keep all the power wells on (assuming the platform has any), as well as disable runtime PM of course.

Anyway there are two solutions that have been considered: polling and GPIO interrupts

Polling should be fairly easy to do I suppose, but it itself wastes power, and to minimize that you need to not poll too often which may still give you an uncomfortably long delay until the display lights up. Although maybe a long delay is better than an infinite delay. Just has to short enough by default so that most users wouldn't go yank out the cable again to blow away the dust and whatnot thinking it didn't work. The nice thing about polling is that it's a platform independent solution.

The GPIO approach would involve changing the pin muxing depending on the state of the power well (or maybe leave them as GPIO pins all the time?). I can't recall if Imre actually tried this approach, but I do recall we discussed it a few time when he was adding the BYT power well stuff. The bad thing is that it's a very platform specific solution. I believe we should be able to do it on BYT/CHV, but probably not on other platforms.

I guess one idea would be to implement both. Use GPIO when available to minimize power consumption (BYT/CHV are the low power platforms anyway), and fall back to polling otherwise. I seem to recall that some upcoming platforms might have a bit better support for HPD while otherwise powered down, but could be I just dreamed it.

I don't think anyone is really opposed to solving this, just there are plenty of higher priority things to do. And if someone could come up with some great plan D that has no downsides that would be great ;)
Comment 11 Ville Syrjala 2015-02-18 19:15:05 UTC
(In reply to Ville Syrjala from comment #10)
> The GPIO approach would involve changing the pin muxing depending on the
> state of the power well (or maybe leave them as GPIO pins all the time?).

Oh yeah we can't leave them as GPIO pins since we'll be wanting the hardware long/short pulse detection to work.
Comment 12 Imre Deak 2015-02-18 21:24:57 UTC
(In reply to Ville Syrjala from comment #10)
> (In reply to Egbert Eich from comment #9)
> > (In reply to Jani Nikula from comment #8)
> > > Ville on #intel-gfx:
> > > ... 
> The GPIO approach would involve changing the pin muxing depending on the
> state of the power well (or maybe leave them as GPIO pins all the time?). I
> can't recall if Imre actually tried this approach, but I do recall we
> discussed it a few time when he was adding the BYT power well stuff. The bad
> thing is that it's a very platform specific solution. I believe we should be
> able to do it on BYT/CHV, but probably not on other platforms.

Antti investigated the GPIO approach. The result of that was that using GPIOs in this way is an unofficial configuration (not explicitly described by the spec) and that the i915 driver would need to interface with the kernel's pin control/GPIO subsystem which makes it more difficult to implement. There wasn't either a clear conclusion if it's physically doable on CHV/BYT at all; based on what I understood from the GPIO/MUX specs it is, but I haven't actually tried. There is no information on whether we can use GPIOs in this way on non-BYT/CHV platforms. As a sidenote besides D3 that Ville mentioned for non-BYT/CHV platforms HPD also goes away on those if we turn off their display power well (at least on HSW/BDW).

Another issue is S0ix. It's possible that in that state even the GPIO subsystem is powered down. Granted, it would be somewhat strange.

All that said, I think it's still worth investigating this further for someone with the time.

And yes, brand new platforms (at least Gen9LP based) have explicit HW support to keep HPD working even during low power states.
Comment 13 Jani Nikula 2015-02-19 09:04:40 UTC
Hmm, what does Windows do?
Comment 14 Egbert Eich 2015-02-19 14:20:35 UTC
(In reply to Ville Syrjala from comment #10)
> 
> Sadly there's no sub-partitioning of the power well, so it's either all or
> nothing. Also we may even further runtime suspend the entire device. That

Right. I was in a rush and therefore reading the code wrong: the power domains 
are just a software concept to group the different subsystems onto the power wells which is platform dependent.
It looks like the display2d power well on vlv is actually affecting all subsystems. 
 
> means we should actually have the same problem on all platforms supporting
> runtime PM. I suppose the only thing that saves most other platforms is the
> default 10s delay on runtime susped we have (which we really should reduce
> to something like 1s).Therefore I was under the impression that HPD was not affected by runtime PM on these platforms. (On the other hand I didn't experiement with HPD too much on these).

Well, on other platforms you have a lot of subsystems 'always on'. On HSW and BDW the display2d power well powers fewer subsystems.

> 
> So at the moment i915.disable_power_well=0 is the only way to guarantee that
> HPD keeps working at all times, but that will keep all the power wells on
> (assuming the platform has any), as well as disable runtime PM of course.

Ok, this is still better than not having no HPD at all after disconnecting all displays. For my use case no HPD would be unacceptible.

It looks a bit like a design flaw that you cannot power down the display part without affecting HPD. On DVI/HDMI as well as on DP HPD signaling is done on line that's separate to all other signaling lines on the PHY. It should be possible power this subsystem separately.

> 
> Anyway there are two solutions that have been considered: polling and GPIO
> interrupts
> 
> Polling should be fairly easy to do I suppose, but it itself wastes power,
> and to minimize that you need to not poll too often which may still give you
> an uncomfortably long delay until the display lights up. Although maybe a
> long delay is better than an infinite delay. Just has to short enough by
> default so that most users wouldn't go yank out the cable again to blow away
> the dust and whatnot thinking it didn't work. The nice thing about polling
> is that it's a platform independent solution.

Polling is still used as fallback with 10 sec interval in case of problems (HPD storm) or if IRQ signalling is not supported by the chip. However signalling is preferrable.

> 
> The GPIO approach would involve changing the pin muxing depending on the
> state of the power well (or maybe leave them as GPIO pins all the time?). I
> can't recall if Imre actually tried this approach, but I do recall we
> discussed it a few time when he was adding the BYT power well stuff. The bad
> thing is that it's a very platform specific solution. I believe we should be
> able to do it on BYT/CHV, but probably not on other platforms.
> 
> I guess one idea would be to implement both. Use GPIO when available to
> minimize power consumption (BYT/CHV are the low power platforms anyway), and
> fall back to polling otherwise. I seem to recall that some upcoming
> platforms might have a bit better support for HPD while otherwise powered
> down, but could be I just dreamed it.

Frankly, I don't care about powering down the display engines at all.
The systems I care about are no phones nor tablets running on a skimpy little battery. In the use cases I care about the displays need to go on when connected.
I would be happy to get by without any runtime PM stuff in favor of yet more complicated handling of HPD and wait with saving the planet from gobla warming until the Intel hardware designer have come up with a less flawed HPD PM solution.
> 
> I don't think anyone is really opposed to solving this, just there are
> plenty of higher priority things to do. And if someone could come up with
> some great plan D that has no downsides that would be great ;)

Currently the kernels I have to support don't support runtime PM, yet. Eventually they will, though.
Comment 15 Egbert Eich 2015-02-19 14:34:28 UTC
(In reply to Ville Syrjala from comment #11)
> (In reply to Ville Syrjala from comment #10)
> > The GPIO approach would involve changing the pin muxing depending on the
> > state of the power well (or maybe leave them as GPIO pins all the time?).
> 
> Oh yeah we can't leave them as GPIO pins since we'll be wanting the hardware
> long/short pulse detection to work.

In the case where the power wells are off (ie. no displays connected and link down) this is probably  irrelevant as in this situation you would only expect a long pulse, short pulses (ie sink issuing an IRQ) should only happen when the link is up, shouldn't they?
Comment 16 Egbert Eich 2015-02-19 14:47:32 UTC
Also, for HPD to work on the sink side the source must supply trickle power. Would not be surprised if this doesn't get turned off by agressive runtime PM as well - in which case the GPIO trick would not work either.
In this case when doing polling the relevant wells would have to be ungated before the HPD status could be read.
Comment 17 Jesse Barnes 2015-03-31 18:53:24 UTC
There's probably a nicer way to do this, but it seems like preserving HPD behavior by default is the right thing to do.  Having a runtime way to disable it would be nice though, or a config option, so if your userspace can handle the polling we could allow the DISP2D power well to shut down.

Egbert, can you give it a try?

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 92f8300..1ffeef2 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2441,6 +2441,7 @@ struct i915_params {
 	bool mmio_debug;
 	bool verbose_state_checks;
 	bool nuclear_pageflip;
+	bool keep_hpd_enabled;
 };
 extern struct i915_params i915 __read_mostly;
 
diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
index 44f2262..48acf4b 100644
--- a/drivers/gpu/drm/i915/i915_params.c
+++ b/drivers/gpu/drm/i915/i915_params.c
@@ -53,6 +53,7 @@ struct i915_params i915 __read_mostly = {
 	.mmio_debug = 0,
 	.verbose_state_checks = 1,
 	.nuclear_pageflip = 0,
+	.keep_hpd_enabled = 1,
 };
 
 module_param_named(modeset, i915.modeset, int, 0400);
@@ -183,3 +184,6 @@ MODULE_PARM_DESC(verbose_state_checks,
 module_param_named_unsafe(nuclear_pageflip, i915.nuclear_pageflip, bool, 0600);
 MODULE_PARM_DESC(nuclear_pageflip,
 		 "Force atomic modeset functionality; only planes work for now (default: false).");
+
+module_param_named(keep_hpd_enabled, i915.keep_hpd_enabled, bool, 0600);
+MODULE_PARM_DESC(keep_hpd_enabled, "Keep hotplug detection enabled (costs some power) (default: true)");
diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index 6d8e29a..1d45609 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -544,6 +544,9 @@ static void vlv_display_power_well_disable(struct drm_i915_private *dev_priv,
 {
 	WARN_ON_ONCE(power_well->data != PUNIT_POWER_WELL_DISP2D);
 
+	if (i915.keep_hpd_enabled)
+		return;
+
 	spin_lock_irq(&dev_priv->irq_lock);
 	valleyview_disable_display_irqs(dev_priv);
 	spin_unlock_irq(&dev_priv->irq_lock);
Comment 18 Jani Nikula 2015-04-01 09:36:37 UTC
Jesse, correct me if I'm wrong, but I thought we need the power well to be able to handle the irq.
Comment 19 Jani Nikula 2015-05-12 08:53:49 UTC
There's a lot of probably valid talk about power wells here, but this is worth trying too: http://patchwork.freedesktop.org/patch/49042/
Comment 20 Timo Aaltonen 2016-01-21 16:41:01 UTC
happens with latest -next kernel too which of course has that patch
Comment 21 Ville Syrjala 2016-03-17 14:59:03 UTC
*** Bug 94592 has been marked as a duplicate of this bug. ***
Comment 22 Timo Aaltonen 2016-09-09 06:24:12 UTC
this is fixed in v4.8~

84c8e0963da434d drm/i915: Enable polling when we don't have hpd
21842ea84f161ae drm/i915/vlv: Disable HPD in valleyview_crt_detect_hotplug()
4c732e6ee9e7190 drm/i915/vlv: Reset the ADPA in vlv_display_power_well_init()
4570d833390b100 drm/i915/vlv: Make intel_crt_reset() per-encoder

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.