| Message ID | 1471420952-24482-3-git-send-email-david.weinehall@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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
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,
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(-)