* [PATCH 0/2] RFC: Potential improvements in edid detection timings (v2) @ 2011-10-07 22:00 Eugeni Dodonov 2011-10-07 22:00 ` [PATCH 1/2] Give up on edid retries when i2c tells us that bus is not there Eugeni Dodonov 2011-10-07 22:00 ` [PATCH 2/2] Check if the bus is valid prior to discovering edid Eugeni Dodonov 0 siblings, 2 replies; 5+ 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] 5+ messages in thread
* [PATCH 1/2] Give up on edid retries when i2c tells us that bus is not there 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 2011-10-07 22:00 ` [PATCH 2/2] Check if the bus is valid prior to discovering edid Eugeni Dodonov 1 sibling, 0 replies; 5+ messages in thread From: Eugeni Dodonov @ 2011-10-07 22:00 UTC (permalink / raw) To: intel-gfx; +Cc: Eugeni Dodonov, dri-devel 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. As the disadvantage comes the fact that I only tested it on Intel cards, so I am not sure whether it would work on nouveau and radeon. This change should potentially 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/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..5ed34f2 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) { + printk(KERN_DEBUG "drm: skipping non-existent adapter %s\n", + adapter->name); + break; + } } while (ret != 2 && --retries); return ret == 2 ? 0 : -1; -- 1.7.6.4 ^ permalink raw reply related [flat|nested] 5+ 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 ` [PATCH 1/2] Give up on edid retries when i2c tells us that bus is not there Eugeni Dodonov @ 2011-10-07 22:00 ` Eugeni Dodonov 2011-10-08 22:13 ` [Intel-gfx] " Ben Widawsky 1 sibling, 1 reply; 5+ 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] 5+ messages in thread
* Re: [Intel-gfx] [PATCH 2/2] Check if the bus is valid prior to discovering edid. 2011-10-07 22:00 ` [PATCH 2/2] Check if the bus is valid prior to discovering edid Eugeni Dodonov @ 2011-10-08 22:13 ` Ben Widawsky 2011-10-10 12:20 ` Eugeni Dodonov 0 siblings, 1 reply; 5+ messages in thread From: Ben Widawsky @ 2011-10-08 22:13 UTC (permalink / raw) To: Eugeni Dodonov; +Cc: intel-gfx, dri-devel On Fri, Oct 07, 2011 at 07:00:42PM -0300, Eugeni Dodonov wrote: > 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> > --- > > 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); > +} I think you may as well optimistically try to get the edid_data here. The problem is, in the success case you add ~10 i2c clocks because you next call drm_get_edid. If you optimistacally try to do both you should receive the -ENXIO after the slaves ignore the address byte, and not lose time. (So win on exists case, lose a *slight* amount of CPU time in fail case). Now if you do that I think most of the code can be taken from intel_ddc_probe. Just modify that function to take the args you need (dev_priv, and adatper I am thinking). I only see one caller of that function, which can easily be modified. Ben ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Intel-gfx] [PATCH 2/2] Check if the bus is valid prior to discovering edid. 2011-10-08 22:13 ` [Intel-gfx] " Ben Widawsky @ 2011-10-10 12:20 ` Eugeni Dodonov 0 siblings, 0 replies; 5+ messages in thread From: Eugeni Dodonov @ 2011-10-10 12:20 UTC (permalink / raw) To: Ben Widawsky; +Cc: intel-gfx, dri-devel, Eugeni Dodonov [-- Attachment #1.1: Type: text/plain, Size: 821 bytes --] On Sat, Oct 8, 2011 at 19:13, Ben Widawsky <ben@bwidawsk.net> wrote: > I think you may as well optimistically try to get the edid_data here. > The problem is, in the success case you add ~10 i2c clocks because you > next call drm_get_edid. If you optimistacally try to do both you should > receive the -ENXIO after the slaves ignore the address byte, and not > lose time. (So win on exists case, lose a *slight* amount of CPU time in > fail case). > Yep, good idea! I was hoping about that drm_edid patch - it is much smaller, gives slightly better results in all cases, and works with all the cards which use drm. But until I find some non-intel cards to test it (or someone with such cards volunteers to do such testing), I am a bit sceptical about having it merged. -- Eugeni Dodonov <http://eugeni.dodonov.net/> [-- Attachment #1.2: Type: text/html, Size: 1137 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] 5+ messages in thread
end of thread, other threads:[~2011-10-10 12:20 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2011-10-07 22:00 [PATCH 0/2] RFC: Potential improvements in edid detection timings (v2) Eugeni Dodonov 2011-10-07 22:00 ` [PATCH 1/2] Give up on edid retries when i2c tells us that bus is not there Eugeni Dodonov 2011-10-07 22:00 ` [PATCH 2/2] Check if the bus is valid prior to discovering edid Eugeni Dodonov 2011-10-08 22:13 ` [Intel-gfx] " Ben Widawsky 2011-10-10 12:20 ` Eugeni Dodonov
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox