| Message ID | 20160817124748.31208-1-david.weinehall@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Aug 17, 2016 at 03:47:48PM +0300, David Weinehall wrote: > This reverts commit 237ed86c693d8a8e4db476976aeb30df4deac74b. > > Our current implementation of live status check (repeat 9 times > with 10ms delays between each attempt as a workaround for > buggy displays) imposes a rather serious penalty, time wise, > on intel_hdmi_detect(). Since we we already skip live status > checks on platforms before gen 7, and since we seem to have > coped quite well before the live status check was introduced > for newer platforms too, the previous behaviour is probably > preferable, at least unless someone can point to a use-case > that the live status check improves (apart from "Bspec says so". > > Signed-off-by: David Weinehall <david.weinehall@linux.intel.com> Fixes: 237ed86c693d ("drm/i915: Check live status before reading edid") Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97139 Acked-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: stable@vger.kernel.org -Chris
On Wed, 17 Aug 2016, Chris Wilson <chris@chris-wilson.co.uk> wrote: > On Wed, Aug 17, 2016 at 03:47:48PM +0300, David Weinehall wrote: >> This reverts commit 237ed86c693d8a8e4db476976aeb30df4deac74b. >> >> Our current implementation of live status check (repeat 9 times >> with 10ms delays between each attempt as a workaround for >> buggy displays) imposes a rather serious penalty, time wise, >> on intel_hdmi_detect(). Since we we already skip live status >> checks on platforms before gen 7, and since we seem to have >> coped quite well before the live status check was introduced >> for newer platforms too, the previous behaviour is probably >> preferable, at least unless someone can point to a use-case >> that the live status check improves (apart from "Bspec says so". >> >> Signed-off-by: David Weinehall <david.weinehall@linux.intel.com> > > Fixes: 237ed86c693d ("drm/i915: Check live status before reading edid") > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97139 > Acked-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: stable@vger.kernel.org And we've come full circle on live status. Again. References: https://upload.wikimedia.org/wikipedia/commons/3/3b/Paris_Tuileries_Garden_Facepalm_statue.jpg BR, Jani.
On Wed, Aug 17, 2016 at 04:43:36PM +0300, Jani Nikula wrote: > On Wed, 17 Aug 2016, Chris Wilson <chris@chris-wilson.co.uk> wrote: > > On Wed, Aug 17, 2016 at 03:47:48PM +0300, David Weinehall wrote: > >> This reverts commit 237ed86c693d8a8e4db476976aeb30df4deac74b. > >> > >> Our current implementation of live status check (repeat 9 times > >> with 10ms delays between each attempt as a workaround for > >> buggy displays) imposes a rather serious penalty, time wise, > >> on intel_hdmi_detect(). Since we we already skip live status > >> checks on platforms before gen 7, and since we seem to have > >> coped quite well before the live status check was introduced > >> for newer platforms too, the previous behaviour is probably > >> preferable, at least unless someone can point to a use-case > >> that the live status check improves (apart from "Bspec says so". > >> > >> Signed-off-by: David Weinehall <david.weinehall@linux.intel.com> > > > > Fixes: 237ed86c693d ("drm/i915: Check live status before reading edid") > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97139 > > Acked-by: Chris Wilson <chris@chris-wilson.co.uk> > > Cc: stable@vger.kernel.org > > And we've come full circle on live status. Again. > > References: https://upload.wikimedia.org/wikipedia/commons/3/3b/Paris_Tuileries_Garden_Facepalm_statue.jpg Actually, we might have to take (at least) one more lap around the circle. I did some more benchmarks, this time on Cherryview, and two different Skylake models (one ThinkPad 13, one NUC i5). The results are rather frustrating: On Skylake gmbus_wait_hw_status() seems to be really expensive[1]. So expensive that unless we can figure out how to get it down, the live status check is still the lesser of two evils. In a scenario with only sensible displays and no displays, the solution would be simple enough -- just eliminate the retries, keep the live status check, since it seems to work as it should on Skylake (haven't tried on Broxton or Kabylake yet, so I'm not sure whether it's all gen9 platforms, or only Skylake). Reality, however, indicates that not all displays are sensible, so where (almost) back on square one. Clearly we can optimise away the live status checks completely on < (INTEL_GEN(dev_priv) < 7 || IS_IVYBRIDGE(dev_priv))[2]. My testing on Broadwell suggests that this might even be possible to extend to < (INTEL_GEN(dev_priv) < 9). That leaves gen9+ though, and seeing as those platforms are the current and future, respectively, priorities, we clearly need a solution for those too. So, anyone with any theory why gmbus_wait_hw_status() is horribly slow on Skylake? Kind regards, David [1] More than the live_status loop, *if* msleep(10) only sleeps 10ms. If we keep that loop we might want to consider using usleep_range() instead, to get better precision -- I've experienced msleep(10) taking up to 20ms. [2] As in the patch I already submitted earlier.
On Thu, Aug 18, 2016 at 10:29:43AM +0300, David Weinehall wrote: > On Wed, Aug 17, 2016 at 04:43:36PM +0300, Jani Nikula wrote: > > On Wed, 17 Aug 2016, Chris Wilson <chris@chris-wilson.co.uk> wrote: > > > On Wed, Aug 17, 2016 at 03:47:48PM +0300, David Weinehall wrote: > > >> This reverts commit 237ed86c693d8a8e4db476976aeb30df4deac74b. > > >> > > >> Our current implementation of live status check (repeat 9 times > > >> with 10ms delays between each attempt as a workaround for > > >> buggy displays) imposes a rather serious penalty, time wise, > > >> on intel_hdmi_detect(). Since we we already skip live status > > >> checks on platforms before gen 7, and since we seem to have > > >> coped quite well before the live status check was introduced > > >> for newer platforms too, the previous behaviour is probably > > >> preferable, at least unless someone can point to a use-case > > >> that the live status check improves (apart from "Bspec says so". > > >> > > >> Signed-off-by: David Weinehall <david.weinehall@linux.intel.com> > > > > > > Fixes: 237ed86c693d ("drm/i915: Check live status before reading edid") > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97139 > > > Acked-by: Chris Wilson <chris@chris-wilson.co.uk> > > > Cc: stable@vger.kernel.org > > > > And we've come full circle on live status. Again. > > > > References: https://upload.wikimedia.org/wikipedia/commons/3/3b/Paris_Tuileries_Garden_Facepalm_statue.jpg > > Actually, we might have to take (at least) one more > lap around the circle. > > I did some more benchmarks, this time on Cherryview, > and two different Skylake models (one ThinkPad 13, > one NUC i5). > > The results are rather frustrating: > > On Skylake gmbus_wait_hw_status() seems to be > really expensive[1]. [snip] Actually, I think it's safe to do the revert. A bit more testing yields that it's not a generic issue for Skylake, it seems to be specific to ThinkPads (or even a subset of them; I haven't got more than one model to test on). The gmbus never sends a NAK if there's nothing connected, so eventually gmbus_wait_for_status() gives up and we fall back to using the bitbanging method instead. Since things still work (albeit with reduced performance) on ThinkPads, and other platforms don't exhibit this behaviour, I suggest we move forward and revert the live status check. Chris has a partial fix for the problem; it fixes the first port (which is purely an HDMI-port), but the second port -- which I believe is routed through the dock connector and/or the type C USB-port, still remains problematic. Kind regards, David Weinehall
On Fri, 19 Aug 2016, David Weinehall <david.weinehall@linux.intel.com> wrote: > On Thu, Aug 18, 2016 at 10:29:43AM +0300, David Weinehall wrote: >> On Wed, Aug 17, 2016 at 04:43:36PM +0300, Jani Nikula wrote: >> > On Wed, 17 Aug 2016, Chris Wilson <chris@chris-wilson.co.uk> wrote: >> > > On Wed, Aug 17, 2016 at 03:47:48PM +0300, David Weinehall wrote: >> > >> This reverts commit 237ed86c693d8a8e4db476976aeb30df4deac74b. >> > >> >> > >> Our current implementation of live status check (repeat 9 times >> > >> with 10ms delays between each attempt as a workaround for >> > >> buggy displays) imposes a rather serious penalty, time wise, >> > >> on intel_hdmi_detect(). Since we we already skip live status >> > >> checks on platforms before gen 7, and since we seem to have >> > >> coped quite well before the live status check was introduced >> > >> for newer platforms too, the previous behaviour is probably >> > >> preferable, at least unless someone can point to a use-case >> > >> that the live status check improves (apart from "Bspec says so". >> > >> >> > >> Signed-off-by: David Weinehall <david.weinehall@linux.intel.com> >> > > >> > > Fixes: 237ed86c693d ("drm/i915: Check live status before reading edid") >> > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97139 >> > > Acked-by: Chris Wilson <chris@chris-wilson.co.uk> >> > > Cc: stable@vger.kernel.org >> > >> > And we've come full circle on live status. Again. >> > >> > References: https://upload.wikimedia.org/wikipedia/commons/3/3b/Paris_Tuileries_Garden_Facepalm_statue.jpg >> >> Actually, we might have to take (at least) one more >> lap around the circle. >> >> I did some more benchmarks, this time on Cherryview, >> and two different Skylake models (one ThinkPad 13, >> one NUC i5). >> >> The results are rather frustrating: >> >> On Skylake gmbus_wait_hw_status() seems to be >> really expensive[1]. > > [snip] > > Actually, I think it's safe to do the revert. > A bit more testing yields that it's not a generic > issue for Skylake, it seems to be specific to > ThinkPads (or even a subset of them; I haven't > got more than one model to test on). > > The gmbus never sends a NAK if there's nothing connected, > so eventually gmbus_wait_for_status() gives up and we fall > back to using the bitbanging method instead. > > Since things still work (albeit with reduced performance) > on ThinkPads, and other platforms don't exhibit this > behaviour, I suggest we move forward and revert > the live status check. > > Chris has a partial fix for the problem; it fixes > the first port (which is purely an HDMI-port), but > the second port -- which I believe is routed through > the dock connector and/or the type C USB-port, > still remains problematic. Pushed to drm-intel-next-queued after procrastination, thanks for the patch. BR, Jani.
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 364db908c191..dcd8359111be 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -4171,7 +4171,7 @@ static bool bxt_digital_port_connected(struct drm_i915_private *dev_priv, * * Return %true if @port is connected, %false otherwise. */ -bool intel_digital_port_connected(struct drm_i915_private *dev_priv, +static bool intel_digital_port_connected(struct drm_i915_private *dev_priv, struct intel_digital_port *port) { if (HAS_PCH_IBX(dev_priv)) diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 1c700b0c3cea..a81ba75d23f3 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -1400,8 +1400,6 @@ void intel_edp_drrs_invalidate(struct drm_i915_private *dev_priv, unsigned int frontbuffer_bits); void intel_edp_drrs_flush(struct drm_i915_private *dev_priv, unsigned int frontbuffer_bits); -bool intel_digital_port_connected(struct drm_i915_private *dev_priv, - struct intel_digital_port *port); void intel_dp_program_link_training_pattern(struct intel_dp *intel_dp, diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c index 4df9f384910c..c3aa9e670d15 100644 --- a/drivers/gpu/drm/i915/intel_hdmi.c +++ b/drivers/gpu/drm/i915/intel_hdmi.c @@ -1422,24 +1422,22 @@ intel_hdmi_dp_dual_mode_detect(struct drm_connector *connector, bool has_edid) } static bool -intel_hdmi_set_edid(struct drm_connector *connector, bool force) +intel_hdmi_set_edid(struct drm_connector *connector) { struct drm_i915_private *dev_priv = to_i915(connector->dev); struct intel_hdmi *intel_hdmi = intel_attached_hdmi(connector); - struct edid *edid = NULL; + struct edid *edid; bool connected = false; - if (force) { - intel_display_power_get(dev_priv, POWER_DOMAIN_GMBUS); + intel_display_power_get(dev_priv, POWER_DOMAIN_GMBUS); - edid = drm_get_edid(connector, - intel_gmbus_get_adapter(dev_priv, - intel_hdmi->ddc_bus)); + edid = drm_get_edid(connector, + intel_gmbus_get_adapter(dev_priv, + intel_hdmi->ddc_bus)); - intel_hdmi_dp_dual_mode_detect(connector, edid != NULL); + intel_hdmi_dp_dual_mode_detect(connector, edid != NULL); - intel_display_power_put(dev_priv, POWER_DOMAIN_GMBUS); - } + intel_display_power_put(dev_priv, POWER_DOMAIN_GMBUS); to_intel_connector(connector)->detect_edid = edid; if (edid && edid->input & DRM_EDID_INPUT_DIGITAL) { @@ -1465,37 +1463,16 @@ static enum drm_connector_status intel_hdmi_detect(struct drm_connector *connector, bool force) { enum drm_connector_status status; - struct intel_hdmi *intel_hdmi = intel_attached_hdmi(connector); struct drm_i915_private *dev_priv = to_i915(connector->dev); - bool live_status = false; - unsigned int try; DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n", connector->base.id, connector->name); intel_display_power_get(dev_priv, POWER_DOMAIN_GMBUS); - for (try = 0; !live_status && try < 9; try++) { - if (try) - msleep(10); - live_status = intel_digital_port_connected(dev_priv, - hdmi_to_dig_port(intel_hdmi)); - } - - 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); - if (intel_hdmi_set_edid(connector, live_status)) { + if (intel_hdmi_set_edid(connector)) { struct intel_hdmi *intel_hdmi = intel_attached_hdmi(connector); hdmi_to_dig_port(intel_hdmi)->base.type = INTEL_OUTPUT_HDMI; @@ -1521,7 +1498,7 @@ intel_hdmi_force(struct drm_connector *connector) if (connector->status != connector_status_connected) return; - intel_hdmi_set_edid(connector, true); + intel_hdmi_set_edid(connector); hdmi_to_dig_port(intel_hdmi)->base.type = INTEL_OUTPUT_HDMI; }
This reverts commit 237ed86c693d8a8e4db476976aeb30df4deac74b. Our current implementation of live status check (repeat 9 times with 10ms delays between each attempt as a workaround for buggy displays) imposes a rather serious penalty, time wise, on intel_hdmi_detect(). Since we we already skip live status checks on platforms before gen 7, and since we seem to have coped quite well before the live status check was introduced for newer platforms too, the previous behaviour is probably preferable, at least unless someone can point to a use-case that the live status check improves (apart from "Bspec says so". Signed-off-by: David Weinehall <david.weinehall@linux.intel.com> --- drivers/gpu/drm/i915/intel_dp.c | 2 +- drivers/gpu/drm/i915/intel_drv.h | 2 -- drivers/gpu/drm/i915/intel_hdmi.c | 43 +++++++++------------------------------ 3 files changed, 11 insertions(+), 36 deletions(-)