[PATCH 0/2] RFC: Potential improvements in edid detection timings (v2)

[PATCH 0/2] RFC: Potential improvements in edid detection timings (v2)

[Eugeni Dodonov]

(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

[PATCH 1/2] Give up on edid retries when i2c tells us that bus is not there

[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.

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

[PATCH 2/2] Check if the bus is valid prior to discovering edid.

[Eugeni Dodonov]

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

Re: [Intel-gfx] [PATCH 2/2] Check if the bus is valid prior to discovering edid.

[Ben Widawsky]

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

Re: [Intel-gfx] [PATCH 2/2] Check if the bus is valid prior to discovering edid.

[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