* [PATCH] Improvements in edid detection timings (final)
@ 2011-10-17 13:12 Eugeni Dodonov
2011-10-17 13:12 ` [PATCH 1/2] Give up on edid retries when i2c tells us that bus is not there Eugeni Dodonov
2011-10-17 13:12 ` [PATCH 2/2] Check if the bus is valid prior to discovering edid Eugeni Dodonov
0 siblings, 2 replies; 10+ messages in thread
From: Eugeni Dodonov @ 2011-10-17 13:12 UTC (permalink / raw)
To: intel-gfx; +Cc: dri-devel, Eugeni Dodonov
Those are two identical fixes for improving EDID detection timings, which
also fix extremely slow xrandr queries in case of phantom outputs
(https://bugs.freedesktop.org/show_bug.cgi?id=41059)
The first fix is a small change to drm_edid, which prevents it from talking to
non-existent adapters by detecting them faster.
The second fix replicates the first one within the i915 driver. It does the
same thing, but without touching core DRM files.
Those are some of the testing results from our QA team:
Regressions and functional testing:
Machine+ports result
Ironlake(mobile) eDP/VGA pass
Ironlake(desktop) DP/VGA pass
Ironlake(mobile) LVDS/VGA/DP pass
G45(desktop) VGA/HDMI pass
Pineview(mobile) LVDS/VGA pass
xrandr performance:
Without patch with patch
E6510(Ironlake mobile) 0.119 0.111
PK1(Ironlake desktop) 0.101 0.080
T410b(Ironlake mobile) 0.406 0.114
G45b( G45 desktop) 0.121 0.091
Pnv1(Pineview mobile) 0.043 0.040
Those are the results for machines affected by phantom outputs issue, based on
fd.o #41059 feedback:
xrandr performance:
Without patch with patch
System 1 0.840 0.290
System 2 0.690 0.140
System 3 0.315 0.280
System 4 0.175 0.140
System 6 (original issue) 4s 0.184
We have observed no regressions in any cases, and performance improvements
of 20-30% for edid detection timing. Combining it with the results obtained
at https://bugs.freedesktop.org/show_bug.cgi?id=41059, besides those
improvements it also improves xrandr timing by up to 20x in the worst case
of phantom outputs.
I believe that the better way to fix this is via the drm_get_edid() fix, as
it is a one-line fix and could benefit all other chipsets. And we won't have
to reinvent the wheel with intel_drm_get_edid, which only duplicates the
existent functionality with no additional benefits.
Could we have any feedback or reviewed-by or from non-intel drm maintainers?
Thanks!
Eugeni Dodonov (2):
Give up on edid retries when i2c tells us that bus is not there
Check if the bus is valid prior to discovering edid.
drivers/gpu/drm/drm_edid.c | 5 ++++
drivers/gpu/drm/i915/intel_crt.c | 46 ++++++++++++++++++------------------
drivers/gpu/drm/i915/intel_dp.c | 4 +-
drivers/gpu/drm/i915/intel_drv.h | 3 +-
drivers/gpu/drm/i915/intel_hdmi.c | 4 +-
drivers/gpu/drm/i915/intel_i2c.c | 42 ++++++++++++++++++++++++++++++++
drivers/gpu/drm/i915/intel_lvds.c | 2 +-
drivers/gpu/drm/i915/intel_modes.c | 29 +----------------------
drivers/gpu/drm/i915/intel_sdvo.c | 4 +-
9 files changed, 80 insertions(+), 59 deletions(-)
--
1.7.7
^ permalink raw reply [flat|nested] 10+ messages in thread* [PATCH 1/2] Give up on edid retries when i2c tells us that bus is not there 2011-10-17 13:12 [PATCH] Improvements in edid detection timings (final) Eugeni Dodonov @ 2011-10-17 13:12 ` Eugeni Dodonov 2011-10-17 20:41 ` Keith Packard 2011-10-31 19:45 ` [Intel-gfx] " Chris Wilson 2011-10-17 13:12 ` [PATCH 2/2] Check if the bus is valid prior to discovering edid Eugeni Dodonov 1 sibling, 2 replies; 10+ messages in thread From: Eugeni Dodonov @ 2011-10-17 13:12 UTC (permalink / raw) To: intel-gfx; +Cc: dri-devel, Eugeni Dodonov This allows to avoid talking to a non-existent bus repeatedly until we finally timeout. The non-existent bus is signalled by -ENXIO error, provided by i2c_algo_bit:bit_doAddress call. As the advantage of such change, all the other routines which use drm_get_edid would benefit for this timeout. This change should fix https://bugs.freedesktop.org/show_bug.cgi?id=41059 and improve overall edid detection timing by 10-30% in most cases, and by a much larger margin in case of phantom outputs. Signed-off-by: Eugeni Dodonov <eugeni.dodonov@intel.com> --- drivers/gpu/drm/drm_edid.c | 5 +++++ 1 files changed, 5 insertions(+), 0 deletions(-) diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index 7425e5c..1bca6d7 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -265,6 +265,11 @@ drm_do_probe_ddc_edid(struct i2c_adapter *adapter, unsigned char *buf, } }; ret = i2c_transfer(adapter, msgs, 2); + if (ret == -ENXIO) { + DRM_DEBUG_KMS("drm: skipping non-existent adapter %s\n", + adapter->name); + break; + } } while (ret != 2 && --retries); return ret == 2 ? 0 : -1; -- 1.7.7 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] Give up on edid retries when i2c tells us that bus is not there 2011-10-17 13:12 ` [PATCH 1/2] Give up on edid retries when i2c tells us that bus is not there Eugeni Dodonov @ 2011-10-17 20:41 ` Keith Packard 2011-10-17 21:07 ` Eugeni Dodonov 2011-10-31 19:45 ` [Intel-gfx] " Chris Wilson 1 sibling, 1 reply; 10+ messages in thread From: Keith Packard @ 2011-10-17 20:41 UTC (permalink / raw) To: intel-gfx; +Cc: dri-devel, Eugeni Dodonov [-- Attachment #1.1: Type: text/plain, Size: 397 bytes --] On Mon, 17 Oct 2011 11:12:29 -0200, Eugeni Dodonov <eugeni.dodonov@intel.com> wrote: > + if (ret == -ENXIO) { > + DRM_DEBUG_KMS("drm: skipping non-existent adapter %s\n", > + adapter->name); > + break; > + } This seems good to me; are there additional error values which should also be considered fatal and not subject to retry? Reviewed-by: Keith Packard <keithp@keithp.com> -keith [-- Attachment #1.2: Type: application/pgp-signature, Size: 827 bytes --] [-- Attachment #2: Type: text/plain, Size: 159 bytes --] _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] Give up on edid retries when i2c tells us that bus is not there 2011-10-17 20:41 ` Keith Packard @ 2011-10-17 21:07 ` Eugeni Dodonov 2011-10-17 22:41 ` Keith Packard 0 siblings, 1 reply; 10+ messages in thread From: Eugeni Dodonov @ 2011-10-17 21:07 UTC (permalink / raw) To: Keith Packard; +Cc: intel-gfx, dri-devel, Eugeni Dodonov [-- Attachment #1.1: Type: text/plain, Size: 739 bytes --] On Mon, Oct 17, 2011 at 18:41, Keith Packard <keithp@keithp.com> wrote: > On Mon, 17 Oct 2011 11:12:29 -0200, Eugeni Dodonov < > eugeni.dodonov@intel.com> wrote: > > > + if (ret == -ENXIO) { > > + DRM_DEBUG_KMS("drm: skipping non-existent adapter > %s\n", > > + adapter->name); > > + break; > > + } > > This seems good to me; are there additional error values which should > also be considered fatal and not subject to retry? > >From what I've checked, the other return error value in this context could be -EREMOTEIO, which could be caused by transmission error so it should be retried. -- Eugeni Dodonov <http://eugeni.dodonov.net/> [-- Attachment #1.2: Type: text/html, Size: 1149 bytes --] [-- Attachment #2: Type: text/plain, Size: 159 bytes --] _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] Give up on edid retries when i2c tells us that bus is not there 2011-10-17 21:07 ` Eugeni Dodonov @ 2011-10-17 22:41 ` Keith Packard 2011-10-18 0:06 ` Eugeni Dodonov 0 siblings, 1 reply; 10+ messages in thread From: Keith Packard @ 2011-10-17 22:41 UTC (permalink / raw) To: Eugeni Dodonov; +Cc: intel-gfx, dri-devel, Eugeni Dodonov [-- Attachment #1.1: Type: text/plain, Size: 439 bytes --] On Mon, 17 Oct 2011 19:07:51 -0200, Eugeni Dodonov <eugeni@dodonov.net> wrote: > From what I've checked, the other return error value in this context could > be -EREMOTEIO, which could be caused by transmission error so it should be > retried. Oh, there's -ENOMEM, -EINVAL and probably a few others down in the bowels of the kernel i2c bits. Starting with the obvious (ENXIO) seems safest to me. -- keith.packard@intel.com [-- Attachment #1.2: Type: application/pgp-signature, Size: 827 bytes --] [-- Attachment #2: Type: text/plain, Size: 159 bytes --] _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] Give up on edid retries when i2c tells us that bus is not there 2011-10-17 22:41 ` Keith Packard @ 2011-10-18 0:06 ` Eugeni Dodonov 0 siblings, 0 replies; 10+ messages in thread From: Eugeni Dodonov @ 2011-10-18 0:06 UTC (permalink / raw) To: Keith Packard; +Cc: intel-gfx, dri-devel, Eugeni Dodonov [-- Attachment #1.1: Type: text/plain, Size: 684 bytes --] On Mon, Oct 17, 2011 at 20:41, Keith Packard <keithp@keithp.com> wrote: > On Mon, 17 Oct 2011 19:07:51 -0200, Eugeni Dodonov <eugeni@dodonov.net> > wrote: > > > From what I've checked, the other return error value in this context > could > > be -EREMOTEIO, which could be caused by transmission error so it should > be > > retried. > > Oh, there's -ENOMEM, -EINVAL and probably a few others down in the > bowels of the kernel i2c bits. Starting with the obvious (ENXIO) seems > safest to me. > Yes, of course, but I was referring to the values which could be returned through the i2c-algo-bit call used in this edid detection call. -- Eugeni Dodonov <http://eugeni.dodonov.net/> [-- Attachment #1.2: Type: text/html, Size: 1085 bytes --] [-- Attachment #2: Type: text/plain, Size: 159 bytes --] _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Intel-gfx] [PATCH 1/2] Give up on edid retries when i2c tells us that bus is not there 2011-10-17 13:12 ` [PATCH 1/2] Give up on edid retries when i2c tells us that bus is not there Eugeni Dodonov 2011-10-17 20:41 ` Keith Packard @ 2011-10-31 19:45 ` Chris Wilson 1 sibling, 0 replies; 10+ messages in thread From: Chris Wilson @ 2011-10-31 19:45 UTC (permalink / raw) To: intel-gfx; +Cc: dri-devel, Eugeni Dodonov On Mon, 17 Oct 2011 11:12:29 -0200, Eugeni Dodonov <eugeni.dodonov@intel.com> wrote: > This allows to avoid talking to a non-existent bus repeatedly until we > finally timeout. The non-existent bus is signalled by -ENXIO error, > provided by i2c_algo_bit:bit_doAddress call. > > As the advantage of such change, all the other routines which use > drm_get_edid would benefit for this timeout. > > This change should fix > https://bugs.freedesktop.org/show_bug.cgi?id=41059 and improve overall > edid detection timing by 10-30% in most cases, and by a much larger margin > in case of phantom outputs. > > Signed-off-by: Eugeni Dodonov <eugeni.dodonov@intel.com> Looks like we have reached the conclusion that this simple patch is the least likely to cause problems and easiest to fix if it does. :) Reviewed-by: Chris Wilson <chris@hchris-wilson.co.uk> -Chris -- Chris Wilson, Intel Open Source Technology Centre ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 2/2] Check if the bus is valid prior to discovering edid. 2011-10-17 13:12 [PATCH] Improvements in edid detection timings (final) Eugeni Dodonov 2011-10-17 13:12 ` [PATCH 1/2] Give up on edid retries when i2c tells us that bus is not there Eugeni Dodonov @ 2011-10-17 13:12 ` Eugeni Dodonov 1 sibling, 0 replies; 10+ messages in thread From: Eugeni Dodonov @ 2011-10-17 13:12 UTC (permalink / raw) To: intel-gfx; +Cc: dri-devel, Eugeni Dodonov This adds a new function intel_drm_get_valid_edid, which attempts to detect EDID values from available devices and returns quickly in case no connection is present. We detect non-existing devices by checking the i2c_transfer status, and if it fails with the -ENXIO result, we know that the i2c_algo_bit was unable to locate the hardware, so we give up on further edid discovery. This should improve the edid detection timeouts by about 10-30% in most cases, and by a much larger margin in case of broken or phantom outputs https://bugs.freedesktop.org/show_bug.cgi?id=41059. This also removes intel_ddc_probe, whose functionality is now done by intel_drm_get_valid_edid. Signed-off-by: Eugeni Dodonov <eugeni.dodonov@intel.com> --- drivers/gpu/drm/i915/intel_crt.c | 46 ++++++++++++++++++------------------ drivers/gpu/drm/i915/intel_dp.c | 4 +- drivers/gpu/drm/i915/intel_drv.h | 3 +- drivers/gpu/drm/i915/intel_hdmi.c | 4 +- drivers/gpu/drm/i915/intel_i2c.c | 42 ++++++++++++++++++++++++++++++++ drivers/gpu/drm/i915/intel_lvds.c | 2 +- drivers/gpu/drm/i915/intel_modes.c | 29 +---------------------- drivers/gpu/drm/i915/intel_sdvo.c | 4 +- 8 files changed, 75 insertions(+), 59 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c index 0979d88..3b55fdf 100644 --- a/drivers/gpu/drm/i915/intel_crt.c +++ b/drivers/gpu/drm/i915/intel_crt.c @@ -273,36 +273,36 @@ static bool intel_crt_detect_ddc(struct drm_connector *connector) { struct intel_crt *crt = intel_attached_crt(connector); struct drm_i915_private *dev_priv = crt->base.base.dev->dev_private; + struct edid *edid = NULL; + bool is_digital = false; /* CRT should always be at 0, but check anyway */ if (crt->base.type != INTEL_OUTPUT_ANALOG) return false; - if (intel_ddc_probe(&crt->base, dev_priv->crt_ddc_pin)) { - struct edid *edid; - bool is_digital = false; + edid = intel_drm_get_valid_edid(connector, + &dev_priv->gmbus[dev_priv->crt_ddc_pin].adapter); + if (!edid) + return false; - edid = drm_get_edid(connector, - &dev_priv->gmbus[dev_priv->crt_ddc_pin].adapter); - /* - * This may be a DVI-I connector with a shared DDC - * link between analog and digital outputs, so we - * have to check the EDID input spec of the attached device. - * - * On the other hand, what should we do if it is a broken EDID? - */ - if (edid != NULL) { - is_digital = edid->input & DRM_EDID_INPUT_DIGITAL; - connector->display_info.raw_edid = NULL; - kfree(edid); - } + /* + * This may be a DVI-I connector with a shared DDC + * link between analog and digital outputs, so we + * have to check the EDID input spec of the attached device. + * + * On the other hand, what should we do if it is a broken EDID? + */ + if (edid != NULL) { + is_digital = edid->input & DRM_EDID_INPUT_DIGITAL; + connector->display_info.raw_edid = NULL; + kfree(edid); + } - if (!is_digital) { - DRM_DEBUG_KMS("CRT detected via DDC:0x50 [EDID]\n"); - return true; - } else { - DRM_DEBUG_KMS("CRT not detected via DDC:0x50 [EDID reports a digital panel]\n"); - } + if (!is_digital) { + DRM_DEBUG_KMS("CRT detected via DDC:0x50 [EDID]\n"); + return true; + } else { + DRM_DEBUG_KMS("CRT not detected via DDC:0x50 [EDID reports a digital panel]\n"); } return false; diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 44fef5e..dd0d8b1 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -1715,7 +1715,7 @@ intel_dp_detect(struct drm_connector *connector, bool force) if (intel_dp->force_audio) { intel_dp->has_audio = intel_dp->force_audio > 0; } else { - edid = drm_get_edid(connector, &intel_dp->adapter); + edid = intel_drm_get_valid_edid(connector, &intel_dp->adapter); if (edid) { intel_dp->has_audio = drm_detect_monitor_audio(edid); connector->display_info.raw_edid = NULL; @@ -1772,7 +1772,7 @@ intel_dp_detect_audio(struct drm_connector *connector) struct edid *edid; bool has_audio = false; - edid = drm_get_edid(connector, &intel_dp->adapter); + edid = intel_drm_get_valid_edid(connector, &intel_dp->adapter); if (edid) { has_audio = drm_detect_monitor_audio(edid); diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index fe1099d..0e4b823 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -264,8 +264,9 @@ struct intel_fbc_work { int interval; }; +struct edid * intel_drm_get_valid_edid(struct drm_connector *connector, + struct i2c_adapter *adapter); int intel_ddc_get_modes(struct drm_connector *c, struct i2c_adapter *adapter); -extern bool intel_ddc_probe(struct intel_encoder *intel_encoder, int ddc_bus); extern void intel_attach_force_audio_property(struct drm_connector *connector); extern void intel_attach_broadcast_rgb_property(struct drm_connector *connector); diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c index 226ba83..714f2fb 100644 --- a/drivers/gpu/drm/i915/intel_hdmi.c +++ b/drivers/gpu/drm/i915/intel_hdmi.c @@ -329,7 +329,7 @@ intel_hdmi_detect(struct drm_connector *connector, bool force) intel_hdmi->has_hdmi_sink = false; intel_hdmi->has_audio = false; - edid = drm_get_edid(connector, + edid = intel_drm_get_valid_edid(connector, &dev_priv->gmbus[intel_hdmi->ddc_bus].adapter); if (edid) { @@ -371,7 +371,7 @@ intel_hdmi_detect_audio(struct drm_connector *connector) struct edid *edid; bool has_audio = false; - edid = drm_get_edid(connector, + edid = intel_drm_get_valid_edid(connector, &dev_priv->gmbus[intel_hdmi->ddc_bus].adapter); if (edid) { if (edid->input & DRM_EDID_INPUT_DIGITAL) diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c index d98cee6..b3a6eda 100644 --- a/drivers/gpu/drm/i915/intel_i2c.c +++ b/drivers/gpu/drm/i915/intel_i2c.c @@ -470,3 +470,45 @@ void intel_teardown_gmbus(struct drm_device *dev) kfree(dev_priv->gmbus); dev_priv->gmbus = NULL; } + +/** + * intel_drm_get_valid_edid - gets edid from existent adapters only + * @connector: DRM connector device to use + * @adapter: i2c adapter + * + * Verifies if the i2c adapter is responding to our queries before + * attempting to do proper communication with it. If it does, + * retreive the EDID with help of drm_get_edid + */ +struct edid * +intel_drm_get_valid_edid(struct drm_connector *connector, + struct i2c_adapter *adapter) +{ + int ret; + u8 out_buf[] = { 0x0, 0x0}; + u8 buf[2]; + struct i2c_msg msgs[] = { + { + .addr = 0x50, + .flags = 0, + .len = 1, + .buf = out_buf, + }, + { + .addr = 0x50, + .flags = I2C_M_RD, + .len = 1, + .buf = buf, + } + }; + + /* We just check for -ENXIO - drm_get_edid checks if the transfer + * works and manages the remaining parts of the EDID */ + ret = i2c_transfer(adapter, msgs, 2); + if (ret == -ENXIO) { + DRM_DEBUG_KMS("i915: adapter %s not responding: %d\n", + adapter->name, ret); + return NULL; + } + return drm_get_edid(connector, adapter); +} diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c index 31da77f..8f59a8e 100644 --- a/drivers/gpu/drm/i915/intel_lvds.c +++ b/drivers/gpu/drm/i915/intel_lvds.c @@ -921,7 +921,7 @@ bool intel_lvds_init(struct drm_device *dev) * Attempt to get the fixed panel mode from DDC. Assume that the * preferred mode is the right one. */ - intel_lvds->edid = drm_get_edid(connector, + intel_lvds->edid = intel_drm_get_valid_edid(connector, &dev_priv->gmbus[pin].adapter); if (intel_lvds->edid) { if (drm_add_edid_modes(connector, diff --git a/drivers/gpu/drm/i915/intel_modes.c b/drivers/gpu/drm/i915/intel_modes.c index 3b26a3b..ae1a989 100644 --- a/drivers/gpu/drm/i915/intel_modes.c +++ b/drivers/gpu/drm/i915/intel_modes.c @@ -31,33 +31,6 @@ #include "i915_drv.h" /** - * intel_ddc_probe - * - */ -bool intel_ddc_probe(struct intel_encoder *intel_encoder, int ddc_bus) -{ - struct drm_i915_private *dev_priv = intel_encoder->base.dev->dev_private; - u8 out_buf[] = { 0x0, 0x0}; - u8 buf[2]; - struct i2c_msg msgs[] = { - { - .addr = 0x50, - .flags = 0, - .len = 1, - .buf = out_buf, - }, - { - .addr = 0x50, - .flags = I2C_M_RD, - .len = 1, - .buf = buf, - } - }; - - return i2c_transfer(&dev_priv->gmbus[ddc_bus].adapter, msgs, 2) == 2; -} - -/** * intel_ddc_get_modes - get modelist from monitor * @connector: DRM connector device to use * @adapter: i2c adapter @@ -70,7 +43,7 @@ int intel_ddc_get_modes(struct drm_connector *connector, struct edid *edid; int ret = 0; - edid = drm_get_edid(connector, adapter); + edid = intel_drm_get_valid_edid(connector, adapter); if (edid) { drm_mode_connector_update_edid_property(connector, edid); ret = drm_add_edid_modes(connector, edid); diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c index 6348c49..8b4ac61 100644 --- a/drivers/gpu/drm/i915/intel_sdvo.c +++ b/drivers/gpu/drm/i915/intel_sdvo.c @@ -1240,7 +1240,7 @@ static struct edid * intel_sdvo_get_edid(struct drm_connector *connector) { struct intel_sdvo *sdvo = intel_attached_sdvo(connector); - return drm_get_edid(connector, &sdvo->ddc); + return intel_drm_get_valid_edid(connector, &sdvo->ddc); } /* Mac mini hack -- use the same DDC as the analog connector */ @@ -1249,7 +1249,7 @@ intel_sdvo_get_analog_edid(struct drm_connector *connector) { struct drm_i915_private *dev_priv = connector->dev->dev_private; - return drm_get_edid(connector, + return intel_drm_get_valid_edid(connector, &dev_priv->gmbus[dev_priv->crt_ddc_pin].adapter); } -- 1.7.7 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 0/2] RFC: Potential improvements in edid detection timings (v2) @ 2011-10-07 22:00 Eugeni Dodonov 2011-10-07 22:00 ` [PATCH 2/2] Check if the bus is valid prior to discovering edid Eugeni Dodonov 0 siblings, 1 reply; 10+ messages in thread From: Eugeni Dodonov @ 2011-10-07 22:00 UTC (permalink / raw) To: intel-gfx; +Cc: Eugeni Dodonov, dri-devel (Resending with small improvements - also sending to dri-devel for feedback). This is the the forth iteration of potential fixes for slow edid detection issues over non-existent outputs (https://bugs.freedesktop.org/show_bug.cgi?id=41059) - the previous versions were posted to the bug and were used mostly for debugging the problem. The problem I attempted to fix here is that some systems would take a very long time trying to locate all the possible video outputs - in in cases where such outputs were bogus, this lead to timing out after all the possible delays were done. After investigation, I came to think on two different ways to fix the issue: - The first patch does a one-line fix in drm_edid.c. I added a check for the return value of i2c_transfer - and, if it is -ENXIO, we give up on further attempts as the bus is not there. A drawback to this approach is that it affects all the devices out there which use drm_get_edid. From my testing, the -ENXIO gave no false positives, but I haven't tested it on non-Intel cards. So I'd like to hear what other drm developers think about that. - The second patch does a similar procedure within the i915 driver, in case the proposed change to drm_edid.c won't be adequate for other drivers. It adds a new function - intel_drm_get_valid_edid - which attempts to do a simple i2c transfer over the bus prior to calling drm_get_edid. In case such transfer fails with -ENXIO, it is a signal that the bus is not there, so we shouldn't waste any time trying to communicate with it further. Note that those patches provide lots of dmesg pollution - for the final version those printk'ss should probably be removed, but I left them intentionally with KERN_DEBUG in order to see when the adapters come and go during the debugging and testing. According to testing performed at https://bugs.freedesktop.org/show_bug.cgi?id=41059, the following results were obtained with those patches: System1: (testing all possible outputs) ubuntu 3.0.0-12.19: 840ms 3.0.0-12.19 + drm patch: 290ms 3.0.0-12.19 + i915 patch: 290ms (manually ignoring phantom outputs) ubuntu 3.0.0-12.19: 690ms 3.0.0-12.19 + drm patch: 140ms 3.0.0-12.19 + i915 patch: 140ms System2: (testing all possible outputs) ubuntu 3.0.0-12.19: 315ms 3.0.0-12.19 + drm patch: 280ms 3.0.0-12.19 + i915 patch: 280ms (manually ignoring phantom outputs) ubuntu 3.0.0-12.19: 175ms 3.0.0-12.19 + drm patch: 140ms 3.0.0-12.19 + i915 patch: 140ms Eugeni Dodonov (2): Give up on edid retries when i2c tells us that bus is not there Check if the bus is valid prior to discovering edid. drivers/gpu/drm/drm_edid.c | 5 +++++ drivers/gpu/drm/i915/intel_dp.c | 4 ++-- drivers/gpu/drm/i915/intel_drv.h | 2 ++ drivers/gpu/drm/i915/intel_hdmi.c | 4 ++-- drivers/gpu/drm/i915/intel_i2c.c | 33 +++++++++++++++++++++++++++++++++ drivers/gpu/drm/i915/intel_lvds.c | 2 +- drivers/gpu/drm/i915/intel_modes.c | 2 +- drivers/gpu/drm/i915/intel_sdvo.c | 4 ++-- 8 files changed, 48 insertions(+), 8 deletions(-) -- 1.7.6.3 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 2/2] Check if the bus is valid prior to discovering edid. 2011-10-07 22:00 [PATCH 0/2] RFC: Potential improvements in edid detection timings (v2) Eugeni Dodonov @ 2011-10-07 22:00 ` Eugeni Dodonov 0 siblings, 0 replies; 10+ messages in thread From: Eugeni Dodonov @ 2011-10-07 22:00 UTC (permalink / raw) To: intel-gfx; +Cc: Eugeni Dodonov, dri-devel This adds a new function intel_drm_get_valid_edid, which is used instead of drm_get_edid within the i915 driver. It does a similar check to the one in previous patch, but it is limited to i915 driver. The check is similar to the first part of EDID discovery performed by the drm_do_probe_ddc_edid. In case the i2c_transfer fails with the -ENXIO result, we know that the i2c_algo_bit was unable to locate the hardware, so we give up on further edid discovery. They should also fix https://bugs.freedesktop.org/show_bug.cgi?id=41059 v2: change printk level to KERN_DEBUG to avoid filling up dmesg Signed-off-by: Eugeni Dodonov <eugeni.dodonov@intel.com> --- drivers/gpu/drm/i915/intel_dp.c | 4 ++-- drivers/gpu/drm/i915/intel_drv.h | 2 ++ drivers/gpu/drm/i915/intel_hdmi.c | 4 ++-- drivers/gpu/drm/i915/intel_i2c.c | 33 +++++++++++++++++++++++++++++++++ drivers/gpu/drm/i915/intel_lvds.c | 2 +- drivers/gpu/drm/i915/intel_modes.c | 2 +- drivers/gpu/drm/i915/intel_sdvo.c | 4 ++-- 7 files changed, 43 insertions(+), 8 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 44fef5e..dd0d8b1 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -1715,7 +1715,7 @@ intel_dp_detect(struct drm_connector *connector, bool force) if (intel_dp->force_audio) { intel_dp->has_audio = intel_dp->force_audio > 0; } else { - edid = drm_get_edid(connector, &intel_dp->adapter); + edid = intel_drm_get_valid_edid(connector, &intel_dp->adapter); if (edid) { intel_dp->has_audio = drm_detect_monitor_audio(edid); connector->display_info.raw_edid = NULL; @@ -1772,7 +1772,7 @@ intel_dp_detect_audio(struct drm_connector *connector) struct edid *edid; bool has_audio = false; - edid = drm_get_edid(connector, &intel_dp->adapter); + edid = intel_drm_get_valid_edid(connector, &intel_dp->adapter); if (edid) { has_audio = drm_detect_monitor_audio(edid); diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index fe1099d..d80a9b0 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -264,6 +264,8 @@ struct intel_fbc_work { int interval; }; +struct edid * intel_drm_get_valid_edid(struct drm_connector *connector, + struct i2c_adapter *adapter); int intel_ddc_get_modes(struct drm_connector *c, struct i2c_adapter *adapter); extern bool intel_ddc_probe(struct intel_encoder *intel_encoder, int ddc_bus); diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c index 226ba83..714f2fb 100644 --- a/drivers/gpu/drm/i915/intel_hdmi.c +++ b/drivers/gpu/drm/i915/intel_hdmi.c @@ -329,7 +329,7 @@ intel_hdmi_detect(struct drm_connector *connector, bool force) intel_hdmi->has_hdmi_sink = false; intel_hdmi->has_audio = false; - edid = drm_get_edid(connector, + edid = intel_drm_get_valid_edid(connector, &dev_priv->gmbus[intel_hdmi->ddc_bus].adapter); if (edid) { @@ -371,7 +371,7 @@ intel_hdmi_detect_audio(struct drm_connector *connector) struct edid *edid; bool has_audio = false; - edid = drm_get_edid(connector, + edid = intel_drm_get_valid_edid(connector, &dev_priv->gmbus[intel_hdmi->ddc_bus].adapter); if (edid) { if (edid->input & DRM_EDID_INPUT_DIGITAL) diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c index d98cee6..77115b9 100644 --- a/drivers/gpu/drm/i915/intel_i2c.c +++ b/drivers/gpu/drm/i915/intel_i2c.c @@ -470,3 +470,36 @@ void intel_teardown_gmbus(struct drm_device *dev) kfree(dev_priv->gmbus); dev_priv->gmbus = NULL; } + +/** + * intel_drm_get_valid_edid - gets edid from existent adapters only + * @connector: DRM connector device to use + * @adapter: i2c adapter + * + * Verifies if the i2c adapter is responding to our queries before + * attempting to do proper communication with it. If it does, + * retreive the EDID with help of drm_get_edid + */ +struct edid * +intel_drm_get_valid_edid(struct drm_connector *connector, + struct i2c_adapter *adapter) +{ + unsigned char out; + int ret; + struct i2c_msg msgs[] = { + { + .addr = 0x50, + .flags = 0, + .len = 1, + .buf = &out, + } + }; + /* Let's try one edid transfer */ + ret = i2c_transfer(adapter, msgs, 1); + if (ret == -ENXIO) { + printk(KERN_DEBUG "i915: adapter %s not responding: %d\n", + adapter->name, ret); + return NULL; + } + return drm_get_edid(connector, adapter); +} diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c index 31da77f..8f59a8e 100644 --- a/drivers/gpu/drm/i915/intel_lvds.c +++ b/drivers/gpu/drm/i915/intel_lvds.c @@ -921,7 +921,7 @@ bool intel_lvds_init(struct drm_device *dev) * Attempt to get the fixed panel mode from DDC. Assume that the * preferred mode is the right one. */ - intel_lvds->edid = drm_get_edid(connector, + intel_lvds->edid = intel_drm_get_valid_edid(connector, &dev_priv->gmbus[pin].adapter); if (intel_lvds->edid) { if (drm_add_edid_modes(connector, diff --git a/drivers/gpu/drm/i915/intel_modes.c b/drivers/gpu/drm/i915/intel_modes.c index 3b26a3b..9280343 100644 --- a/drivers/gpu/drm/i915/intel_modes.c +++ b/drivers/gpu/drm/i915/intel_modes.c @@ -70,7 +70,7 @@ int intel_ddc_get_modes(struct drm_connector *connector, struct edid *edid; int ret = 0; - edid = drm_get_edid(connector, adapter); + edid = intel_drm_get_valid_edid(connector, adapter); if (edid) { drm_mode_connector_update_edid_property(connector, edid); ret = drm_add_edid_modes(connector, edid); diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c index 6348c49..8b4ac61 100644 --- a/drivers/gpu/drm/i915/intel_sdvo.c +++ b/drivers/gpu/drm/i915/intel_sdvo.c @@ -1240,7 +1240,7 @@ static struct edid * intel_sdvo_get_edid(struct drm_connector *connector) { struct intel_sdvo *sdvo = intel_attached_sdvo(connector); - return drm_get_edid(connector, &sdvo->ddc); + return intel_drm_get_valid_edid(connector, &sdvo->ddc); } /* Mac mini hack -- use the same DDC as the analog connector */ @@ -1249,7 +1249,7 @@ intel_sdvo_get_analog_edid(struct drm_connector *connector) { struct drm_i915_private *dev_priv = connector->dev->dev_private; - return drm_get_edid(connector, + return intel_drm_get_valid_edid(connector, &dev_priv->gmbus[dev_priv->crt_ddc_pin].adapter); } -- 1.7.6.4 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 0/2] RFC: Potential improvements in edid detection timings @ 2011-10-06 23:30 Eugeni Dodonov 2011-10-06 23:30 ` [PATCH 2/2] Check if the bus is valid prior to discovering edid Eugeni Dodonov 0 siblings, 1 reply; 10+ messages in thread From: Eugeni Dodonov @ 2011-10-06 23:30 UTC (permalink / raw) To: intel-gfx; +Cc: Eugeni Dodonov From: Eugeni Dodonov <eugeni.dodonov@intel.com> This is the the forth iteration of potential fixes for slow edid detection issues over non-existent outputs (https://bugs.freedesktop.org/show_bug.cgi?id=41059) - the previous versions were posted to the bug and were used mostly for debugging the problem. After investigation, I came to think on two different ways to fix the issue: in PATCH1, I added a check for the return value of i2c_transfer - and, if it is -ENXIO, we give up on further attempts as the bus is not there. A drawback to this approach is that it affects all the devices out there which use drm_get_edid. From my testing, the -ENXIO gave no false positives, but I haven't tested it on non-Intel cards. The second patch does a similar procedure within the i915 driver. It adds a new function - intel_drm_get_valid_edid - which attempts to do a simple i2c transfer over the bus prior to calling drm_get_edid. In case such transfer fails with -ENXIO, it is a signal that the bus is not there, so we shouldn't waste any time trying to communicate with it further. Note that those patches provide lots of dmesg pollution - I just wanted to send them out to get an overall feedback on the proposed approach. Eugeni Dodonov (2): Give up on edid retries when i2c tells us that bus is not there Check if the bus is valid prior to discovering edid. drivers/gpu/drm/drm_edid.c | 5 +++++ drivers/gpu/drm/i915/intel_dp.c | 4 ++-- drivers/gpu/drm/i915/intel_drv.h | 2 ++ drivers/gpu/drm/i915/intel_hdmi.c | 4 ++-- drivers/gpu/drm/i915/intel_i2c.c | 33 +++++++++++++++++++++++++++++++++ drivers/gpu/drm/i915/intel_lvds.c | 2 +- drivers/gpu/drm/i915/intel_modes.c | 2 +- drivers/gpu/drm/i915/intel_sdvo.c | 4 ++-- 8 files changed, 48 insertions(+), 8 deletions(-) -- 1.7.6.3 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 2/2] Check if the bus is valid prior to discovering edid. 2011-10-06 23:30 [PATCH 0/2] RFC: Potential improvements in edid detection timings Eugeni Dodonov @ 2011-10-06 23:30 ` Eugeni Dodonov 0 siblings, 0 replies; 10+ messages in thread From: Eugeni Dodonov @ 2011-10-06 23:30 UTC (permalink / raw) To: intel-gfx; +Cc: Eugeni Dodonov From: Eugeni Dodonov <eugeni.dodonov@intel.com> This adds a new function intel_drm_get_valid_edid, which is used instead of drm_get_edid within the i915 driver. It does a similar check to the one in previous patch, but it is limited to i915 driver. The check is similar to the first part of EDID discovery performed by the drm_do_probe_ddc_edid. In case the i2c_transfer fails with the -ENXIO result, we know that the i2c_algo_bit was unable to locate the hardware, so we give up on further edid discovery. This should also fix https://bugs.freedesktop.org/show_bug.cgi?id=41059 Signed-off-by: Eugeni Dodonov <eugeni.dodonov@intel.com> --- drivers/gpu/drm/i915/intel_dp.c | 4 ++-- drivers/gpu/drm/i915/intel_drv.h | 2 ++ drivers/gpu/drm/i915/intel_hdmi.c | 4 ++-- drivers/gpu/drm/i915/intel_i2c.c | 33 +++++++++++++++++++++++++++++++++ drivers/gpu/drm/i915/intel_lvds.c | 2 +- drivers/gpu/drm/i915/intel_modes.c | 2 +- drivers/gpu/drm/i915/intel_sdvo.c | 4 ++-- 7 files changed, 43 insertions(+), 8 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 44fef5e..dd0d8b1 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -1715,7 +1715,7 @@ intel_dp_detect(struct drm_connector *connector, bool force) if (intel_dp->force_audio) { intel_dp->has_audio = intel_dp->force_audio > 0; } else { - edid = drm_get_edid(connector, &intel_dp->adapter); + edid = intel_drm_get_valid_edid(connector, &intel_dp->adapter); if (edid) { intel_dp->has_audio = drm_detect_monitor_audio(edid); connector->display_info.raw_edid = NULL; @@ -1772,7 +1772,7 @@ intel_dp_detect_audio(struct drm_connector *connector) struct edid *edid; bool has_audio = false; - edid = drm_get_edid(connector, &intel_dp->adapter); + edid = intel_drm_get_valid_edid(connector, &intel_dp->adapter); if (edid) { has_audio = drm_detect_monitor_audio(edid); diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index fe1099d..d80a9b0 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -264,6 +264,8 @@ struct intel_fbc_work { int interval; }; +struct edid * intel_drm_get_valid_edid(struct drm_connector *connector, + struct i2c_adapter *adapter); int intel_ddc_get_modes(struct drm_connector *c, struct i2c_adapter *adapter); extern bool intel_ddc_probe(struct intel_encoder *intel_encoder, int ddc_bus); diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c index 226ba83..714f2fb 100644 --- a/drivers/gpu/drm/i915/intel_hdmi.c +++ b/drivers/gpu/drm/i915/intel_hdmi.c @@ -329,7 +329,7 @@ intel_hdmi_detect(struct drm_connector *connector, bool force) intel_hdmi->has_hdmi_sink = false; intel_hdmi->has_audio = false; - edid = drm_get_edid(connector, + edid = intel_drm_get_valid_edid(connector, &dev_priv->gmbus[intel_hdmi->ddc_bus].adapter); if (edid) { @@ -371,7 +371,7 @@ intel_hdmi_detect_audio(struct drm_connector *connector) struct edid *edid; bool has_audio = false; - edid = drm_get_edid(connector, + edid = intel_drm_get_valid_edid(connector, &dev_priv->gmbus[intel_hdmi->ddc_bus].adapter); if (edid) { if (edid->input & DRM_EDID_INPUT_DIGITAL) diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c index d98cee6..ac3723c 100644 --- a/drivers/gpu/drm/i915/intel_i2c.c +++ b/drivers/gpu/drm/i915/intel_i2c.c @@ -470,3 +470,36 @@ void intel_teardown_gmbus(struct drm_device *dev) kfree(dev_priv->gmbus); dev_priv->gmbus = NULL; } + +/** + * intel_check_if_adapter_exists - verifies if an i2c adapter is there + * @connector: DRM connector device to use + * @adapter: i2c adapter + * + * Verifies if the i2c adapter is responding to our queries before + * attempting to do proper communication with it. If it does, + * retreive the EDID with help of drm_get_edid + */ +struct edid * +intel_drm_get_valid_edid(struct drm_connector *connector, + struct i2c_adapter *adapter) +{ + unsigned char out; + int ret; + struct i2c_msg msgs[] = { + { + .addr = 0x50, + .flags = 0, + .len = 1, + .buf = &out, + } + }; + /* Let's try one edid transfer */ + ret = i2c_transfer(adapter, msgs, 1); + if (ret == -ENXIO) { + printk(KERN_WARNING "i915: adapter %s not responding: %d\n", + adapter->name, ret); + return NULL; + } + return drm_get_edid(connector, adapter); +} diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c index 31da77f..8f59a8e 100644 --- a/drivers/gpu/drm/i915/intel_lvds.c +++ b/drivers/gpu/drm/i915/intel_lvds.c @@ -921,7 +921,7 @@ bool intel_lvds_init(struct drm_device *dev) * Attempt to get the fixed panel mode from DDC. Assume that the * preferred mode is the right one. */ - intel_lvds->edid = drm_get_edid(connector, + intel_lvds->edid = intel_drm_get_valid_edid(connector, &dev_priv->gmbus[pin].adapter); if (intel_lvds->edid) { if (drm_add_edid_modes(connector, diff --git a/drivers/gpu/drm/i915/intel_modes.c b/drivers/gpu/drm/i915/intel_modes.c index 3b26a3b..9280343 100644 --- a/drivers/gpu/drm/i915/intel_modes.c +++ b/drivers/gpu/drm/i915/intel_modes.c @@ -70,7 +70,7 @@ int intel_ddc_get_modes(struct drm_connector *connector, struct edid *edid; int ret = 0; - edid = drm_get_edid(connector, adapter); + edid = intel_drm_get_valid_edid(connector, adapter); if (edid) { drm_mode_connector_update_edid_property(connector, edid); ret = drm_add_edid_modes(connector, edid); diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c index 6348c49..8b4ac61 100644 --- a/drivers/gpu/drm/i915/intel_sdvo.c +++ b/drivers/gpu/drm/i915/intel_sdvo.c @@ -1240,7 +1240,7 @@ static struct edid * intel_sdvo_get_edid(struct drm_connector *connector) { struct intel_sdvo *sdvo = intel_attached_sdvo(connector); - return drm_get_edid(connector, &sdvo->ddc); + return intel_drm_get_valid_edid(connector, &sdvo->ddc); } /* Mac mini hack -- use the same DDC as the analog connector */ @@ -1249,7 +1249,7 @@ intel_sdvo_get_analog_edid(struct drm_connector *connector) { struct drm_i915_private *dev_priv = connector->dev->dev_private; - return drm_get_edid(connector, + return intel_drm_get_valid_edid(connector, &dev_priv->gmbus[dev_priv->crt_ddc_pin].adapter); } -- 1.7.6.3 ^ permalink raw reply related [flat|nested] 10+ messages in thread
end of thread, other threads:[~2011-10-31 19:45 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2011-10-17 13:12 [PATCH] Improvements in edid detection timings (final) Eugeni Dodonov 2011-10-17 13:12 ` [PATCH 1/2] Give up on edid retries when i2c tells us that bus is not there Eugeni Dodonov 2011-10-17 20:41 ` Keith Packard 2011-10-17 21:07 ` Eugeni Dodonov 2011-10-17 22:41 ` Keith Packard 2011-10-18 0:06 ` Eugeni Dodonov 2011-10-31 19:45 ` [Intel-gfx] " Chris Wilson 2011-10-17 13:12 ` [PATCH 2/2] Check if the bus is valid prior to discovering edid Eugeni Dodonov -- strict thread matches above, loose matches on Subject: below -- 2011-10-07 22:00 [PATCH 0/2] RFC: Potential improvements in edid detection timings (v2) Eugeni Dodonov 2011-10-07 22:00 ` [PATCH 2/2] Check if the bus is valid prior to discovering edid Eugeni Dodonov 2011-10-06 23:30 [PATCH 0/2] RFC: Potential improvements in edid detection timings Eugeni Dodonov 2011-10-06 23:30 ` [PATCH 2/2] Check if the bus is valid prior to discovering edid Eugeni Dodonov
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox