[PATCH 0/2] Live status detection fixes

[PATCH 0/2] Live status detection fixes

[David Weinehall]

With the introduction of the extended timeouts for live status
detection to accomodate for displays that does not immediately
answer to live status requests, we also introduced rather large
overhead whenever we run intel_hdmi_detect() -- the overhead
is multiplied by the number of HDMI-ports.

The first patch here skips the live status check completely for
platforms where the results are unreliable (without the patch
we'd do the check, then ignore the result). The second patch
removes the extended timeouts by default, but offers options
to either use the extended timeouts, or completely disable
live status check.

David Weinehall (2):
  drm/i915: Skip live status if not supported
  drm/i915: add module param for live_status

 drivers/gpu/drm/i915/i915_params.c |  8 ++++++++
 drivers/gpu/drm/i915/i915_params.h |  1 +
 drivers/gpu/drm/i915/intel_hdmi.c  | 36 ++++++++++++++++++++++++++----------
 3 files changed, 35 insertions(+), 10 deletions(-)

-- 
2.8.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 1/2] drm/i915: Skip live status if not supported

[David Weinehall]

Rather than first attempting to use live status on platforms where
its functionality is spotty, and then forcing it to true no matter
what, we first check whether we're on a supported platform,
*then* try to detect whether there's anything connected or not.

This doesn't improve things for newer platforms, but for older
platforms we save ~80ms / HDMI-port with no display connected
whenever intel_hdmi_detect() is called.

Signed-off-by: David Weinehall <david.weinehall@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_hdmi.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index 4df9f384910c..713c91ce7f70 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -1475,6 +1475,14 @@ intel_hdmi_detect(struct drm_connector *connector, bool force)
 
 	intel_display_power_get(dev_priv, POWER_DOMAIN_GMBUS);
 
+	/*
+	 * Live status register is not reliable on all intel platforms.
+	 * So consider live_status only for certain platforms, for
+	 * others, read EDID to determine presence of sink.
+	 */
+	if (INTEL_INFO(dev_priv)->gen < 7 || IS_IVYBRIDGE(dev_priv))
+		live_status = true;
+
 	for (try = 0; !live_status && try < 9; try++) {
 		if (try)
 			msleep(10);
@@ -1482,16 +1490,8 @@ intel_hdmi_detect(struct drm_connector *connector, bool force)
 				hdmi_to_dig_port(intel_hdmi));
 	}
 
-	if (!live_status) {
+	if (!live_status)
 		DRM_DEBUG_KMS("HDMI live status down\n");
-		/*
-		 * Live status register is not reliable on all intel platforms.
-		 * So consider live_status only for certain platforms, for
-		 * others, read EDID to determine presence of sink.
-		 */
-		if (INTEL_INFO(dev_priv)->gen < 7 || IS_IVYBRIDGE(dev_priv))
-			live_status = true;
-	}
 
 	intel_hdmi_unset_edid(connector);
 
-- 
2.8.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/2] drm/i915: add module param for live_status

[David Weinehall]

Since the workaround for buggy displays that do not reply to
live status detect immediately affects a rather limited set of
displays, and since the price paid (almost 100ms per HDMI-port),
we should have that hack disabled by default.

Rather than leaving people with these kinds of broken displays
out in the cold completely, add a module parameter, defaulting
to -1 (live status detection on supported platforms, but without
the extra delays), that allows for re-enabling this hack.

Signed-off-by: David Weinehall <david.weinehall@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_params.c |  8 ++++++++
 drivers/gpu/drm/i915/i915_params.h |  1 +
 drivers/gpu/drm/i915/intel_hdmi.c  | 20 ++++++++++++++++++--
 3 files changed, 27 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
index 768ad89d9cd4..ad5988b17656 100644
--- a/drivers/gpu/drm/i915/i915_params.c
+++ b/drivers/gpu/drm/i915/i915_params.c
@@ -62,6 +62,7 @@ struct i915_params i915 __read_mostly = {
 	.inject_load_failure = 0,
 	.enable_dpcd_backlight = false,
 	.enable_gvt = false,
+	.live_status = -1,
 };
 
 module_param_named(modeset, i915.modeset, int, 0400);
@@ -233,3 +234,10 @@ MODULE_PARM_DESC(enable_dpcd_backlight,
 module_param_named(enable_gvt, i915.enable_gvt, bool, 0400);
 MODULE_PARM_DESC(enable_gvt,
 	"Enable support for Intel GVT-g graphics virtualization host support(default:false)");
+
+module_param_named(live_status, i915.live_status, int, 0600);
+MODULE_PARM_DESC(live_status,
+	"Enable live status detection "
+	"(-1=auto [default], "
+	  "0=disabled, "
+	  "1=enabled with extra delay)");
diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h
index 3a0dd78ddb38..2ff2083936e3 100644
--- a/drivers/gpu/drm/i915/i915_params.h
+++ b/drivers/gpu/drm/i915/i915_params.h
@@ -52,6 +52,7 @@ struct i915_params {
 	int mmio_debug;
 	int edp_vswing;
 	unsigned int inject_load_failure;
+	int live_status;
 	/* leave bools at the end to not create holes */
 	bool enable_hangcheck;
 	bool fastboot;
diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index 713c91ce7f70..b7d88728335a 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -1468,6 +1468,7 @@ intel_hdmi_detect(struct drm_connector *connector, bool force)
 	struct intel_hdmi *intel_hdmi = intel_attached_hdmi(connector);
 	struct drm_i915_private *dev_priv = to_i915(connector->dev);
 	bool live_status = false;
+	int attempts = 1;
 	unsigned int try;
 
 	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
@@ -1478,12 +1479,27 @@ intel_hdmi_detect(struct drm_connector *connector, bool force)
 	/*
 	 * Live status register is not reliable on all intel platforms.
 	 * So consider live_status only for certain platforms, for
-	 * others, read EDID to determine presence of sink.
+	 * others, read EDID to determine presence of sink, unless
+	 * the users explicitly disables live status reads. For users
+	 * who have broken displays we offer the option to use
+	 * live status with an extra delay.
 	 */
+	switch (i915.live_status) {
+	case 0:
+		live_status = true;
+		break;
+	case 1:
+		attempts = 9;
+		break;
+	default:
+	case -1:
+		break;
+	}
+
 	if (INTEL_INFO(dev_priv)->gen < 7 || IS_IVYBRIDGE(dev_priv))
 		live_status = true;
 
-	for (try = 0; !live_status && try < 9; try++) {
+	for (try = 0; !live_status && try < attempts; try++) {
 		if (try)
 			msleep(10);
 		live_status = intel_digital_port_connected(dev_priv,
-- 
2.8.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915: add module param for live_status

[Chris Wilson]

On Wed, Aug 17, 2016 at 11:02:32AM +0300, David Weinehall wrote:
> Since the workaround for buggy displays that do not reply to
> live status detect immediately affects a rather limited set of
> displays, and since the price paid (almost 100ms per HDMI-port),
> we should have that hack disabled by default.
> 
> Rather than leaving people with these kinds of broken displays
> out in the cold completely, add a module parameter, defaulting
> to -1 (live status detection on supported platforms, but without
> the extra delays), that allows for re-enabling this hack.

No. It is a regression. We revert back to the previous status quo,
and then try to introduce live-status checking in a way that works if at
all possible.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Ro.CI.BAT: failure for Live status detection fixes

[Patchwork]

== Series Details ==

Series: Live status detection fixes
URL   : https://patchwork.freedesktop.org/series/11188/
State : failure

== Summary ==

Series 11188v1 Live status detection fixes
http://patchwork.freedesktop.org/api/1.0/series/11188/revisions/1/mbox

Test gem_exec_gttfill:
        Subgroup basic:
                skip       -> PASS       (fi-snb-i7-2600)
Test gem_exec_suspend:
        Subgroup basic-s3:
                dmesg-warn -> PASS       (ro-bdw-i7-5600u)
Test kms_cursor_legacy:
        Subgroup basic-flip-vs-cursor-legacy:
                pass       -> FAIL       (ro-byt-n2820)
                pass       -> FAIL       (fi-hsw-i7-4770k)
        Subgroup basic-flip-vs-cursor-varying-size:
                pass       -> FAIL       (ro-bdw-i5-5250u)
Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-b:
                pass       -> INCOMPLETE (fi-hsw-i7-4770k)

fi-hsw-i7-4770k  total:207  pass:185  dwarn:0   dfail:0   fail:1   skip:20 
fi-snb-i7-2600   total:244  pass:202  dwarn:0   dfail:0   fail:0   skip:42 
ro-bdw-i5-5250u  total:240  pass:218  dwarn:2   dfail:0   fail:2   skip:18 
ro-bdw-i7-5557U  total:240  pass:220  dwarn:3   dfail:0   fail:0   skip:17 
ro-bdw-i7-5600u  total:240  pass:207  dwarn:0   dfail:0   fail:1   skip:32 
ro-bsw-n3050     total:240  pass:194  dwarn:0   dfail:0   fail:4   skip:42 
ro-byt-n2820     total:240  pass:197  dwarn:0   dfail:0   fail:3   skip:40 
ro-hsw-i3-4010u  total:240  pass:214  dwarn:0   dfail:0   fail:0   skip:26 
ro-hsw-i7-4770r  total:240  pass:185  dwarn:0   dfail:0   fail:0   skip:55 
ro-ilk1-i5-650   total:235  pass:174  dwarn:0   dfail:0   fail:1   skip:60 
ro-ivb-i7-3770   total:240  pass:205  dwarn:0   dfail:0   fail:0   skip:35 
ro-ivb2-i7-3770  total:240  pass:209  dwarn:0   dfail:0   fail:0   skip:31 
ro-skl3-i5-6260u total:240  pass:222  dwarn:0   dfail:0   fail:4   skip:14 

Results at /archive/results/CI_IGT_test/RO_Patchwork_1896/

a363b72 drm-intel-nightly: 2016y-08m-17d-05h-29m-02s UTC integration manifest
a78e123 drm/i915: add module param for live_status
989ac09 drm/i915: Skip live status if not supported

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915: add module param for live_status

[David Weinehall]

On Wed, Aug 17, 2016 at 09:08:58AM +0100, Chris Wilson wrote:
> On Wed, Aug 17, 2016 at 11:02:32AM +0300, David Weinehall wrote:
> > Since the workaround for buggy displays that do not reply to
> > live status detect immediately affects a rather limited set of
> > displays, and since the price paid (almost 100ms per HDMI-port),
> > we should have that hack disabled by default.
> > 
> > Rather than leaving people with these kinds of broken displays
> > out in the cold completely, add a module parameter, defaulting
> > to -1 (live status detection on supported platforms, but without
> > the extra delays), that allows for re-enabling this hack.
> 
> No. It is a regression. We revert back to the previous status quo,
> and then try to introduce live-status checking in a way that works if at
> all possible.

The way I understand it, the only approaches that would allow for
combining live status checking with buggy displays are:

* The current behaviour (unconditionally waiting until we're sure
  that even the buggiest displays replies; wastes ~90ms per port
  on working setups when there's no display connected)

or

* live status check as in my patch, with the additional delay
  configurable

The other option is not to bother with with live status check at all.
It seems to work just fine for anything < gen 7; I'm not sure
if it's necessary for any newer setups either.

Seeing as I'm trying to optimise suspend/resume times, I'm kinda
biased towards any solutions that removes the delays by default.
Whether we remove them by doing the live status check without retries
by default (forcing users with buggy displays to enable the workaround)
or by ripping out the live status check completely isn't really
all that important to me. As long as we can get rid of the current
overhead.


Kind regards, David Weienhall
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915: add module param for live_status

[Chris Wilson]

On Wed, Aug 17, 2016 at 12:25:40PM +0300, David Weinehall wrote:
> On Wed, Aug 17, 2016 at 09:08:58AM +0100, Chris Wilson wrote:
> > On Wed, Aug 17, 2016 at 11:02:32AM +0300, David Weinehall wrote:
> > > Since the workaround for buggy displays that do not reply to
> > > live status detect immediately affects a rather limited set of
> > > displays, and since the price paid (almost 100ms per HDMI-port),
> > > we should have that hack disabled by default.
> > > 
> > > Rather than leaving people with these kinds of broken displays
> > > out in the cold completely, add a module parameter, defaulting
> > > to -1 (live status detection on supported platforms, but without
> > > the extra delays), that allows for re-enabling this hack.
> > 
> > No. It is a regression. We revert back to the previous status quo,
> > and then try to introduce live-status checking in a way that works if at
> > all possible.
> 
> The way I understand it, the only approaches that would allow for
> combining live status checking with buggy displays are:

I haven't seen any convincing analysis as to why live-status works
better than ddc 0x50. Certainly not in the changelogs.

The rule is that even if you fix one system, if you break anything else
in the process and you cannot fix it promptly you revert back to the
previous state and try again.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915: add module param for live_status

[Daniel Vetter]

On Wed, Aug 17, 2016 at 10:35:31AM +0100, Chris Wilson wrote:
> On Wed, Aug 17, 2016 at 12:25:40PM +0300, David Weinehall wrote:
> > On Wed, Aug 17, 2016 at 09:08:58AM +0100, Chris Wilson wrote:
> > > On Wed, Aug 17, 2016 at 11:02:32AM +0300, David Weinehall wrote:
> > > > Since the workaround for buggy displays that do not reply to
> > > > live status detect immediately affects a rather limited set of
> > > > displays, and since the price paid (almost 100ms per HDMI-port),
> > > > we should have that hack disabled by default.
> > > > 
> > > > Rather than leaving people with these kinds of broken displays
> > > > out in the cold completely, add a module parameter, defaulting
> > > > to -1 (live status detection on supported platforms, but without
> > > > the extra delays), that allows for re-enabling this hack.
> > > 
> > > No. It is a regression. We revert back to the previous status quo,
> > > and then try to introduce live-status checking in a way that works if at
> > > all possible.
> > 
> > The way I understand it, the only approaches that would allow for
> > combining live status checking with buggy displays are:
> 
> I haven't seen any convincing analysis as to why live-status works
> better than ddc 0x50. Certainly not in the changelogs.
> 
> The rule is that even if you fix one system, if you break anything else
> in the process and you cannot fix it promptly you revert back to the
> previous state and try again.

+1, and mine counts a thousands since magic maintainer powers.

We added this to make it hdmi complaint, because vpg asked for it. It
breaks shit, out it goes again. End of discussion.

The linux community is _very_ clear that when a standard disagrees with
experimental reality, reality wins. On top of that I'm very clear that
we're not going to add module parameters to fine-tune things to appease
different people and groups. Either it works, and we enable it, or it
doesn't, and then we should throw it out again. There's some grey area
with big features like fbc/psr where it makes sense to keep the code until
it's fixed, but definitely not for something as small as this here.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx