Bug 89008

Does hot unplug/plug work right if you boot with monitor attached?

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

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/

Also, independently, does a suspend/resume cycle after you've booted fix things, without reprobe?

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.

Created attachment 113282 [details]
quick_dump before removing i915

boot without monitor attached
attach monitor
./quick_dump.py

Created attachment 113283 [details]
quick_dump after modprobe i915

rmmod -f i915
modprobe i915
./quick_dump.py

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

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

(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 ;)

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

Hmm, what does Windows do?

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

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

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.

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

Jesse, correct me if I'm wrong, but I thought we need the power well to be able to handle the irq.

There's a lot of probably valid talk about power wells here, but this is worth trying too: http://patchwork.freedesktop.org/patch/49042/

happens with latest -next kernel too which of course has that patch

*** Bug 94592 has been marked as a duplicate of this bug. ***

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