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

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

[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

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

[Eugeni Dodonov]

From: Eugeni Dodonov <eugeni.dodonov@intel.com>

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

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..475eff3 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_WARNING "drm: i2c told us that device %s is not there\n",
+					adapter->name);
+			break;
+		}
 	} while (ret != 2 && --retries);
 
 	return ret == 2 ? 0 : -1;
-- 
1.7.6.3

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

[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

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

[Jesse Barnes]

On Thu,  6 Oct 2011 20:30:35 -0300
Eugeni Dodonov <eugeni@dodonov.net> wrote:

> From: Eugeni Dodonov <eugeni.dodonov@intel.com>
> 
> 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

I think I like this, assuming i2c doesn't lie to us.  But won't we spam
the log quite a bit?  We do detection a lot because userspace often
polls it and performs detection at app startup a lot.

Jesse

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

[Eugeni Dodonov]

[-- Attachment #1.1: Type: text/plain, Size: 1183 bytes --]

On Fri, Oct 7, 2011 at 11:08, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:

> On Thu,  6 Oct 2011 20:30:35 -0300
> Eugeni Dodonov <eugeni@dodonov.net> wrote:
>
> > From: Eugeni Dodonov <eugeni.dodonov@intel.com>
> >
> > 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
>
> I think I like this, assuming i2c doesn't lie to us.  But won't we spam
> the log quite a bit?  We do detection a lot because userspace often
> polls it and performs detection at app startup a lot.
>

This extra logging is there just for making it easy to see when the outputs
come and go. For the final version, we could use a KERN_DEBUG instead.

-- 
Eugeni Dodonov
<http://eugeni.dodonov.net/>

[-- Attachment #1.2: Type: text/html, Size: 1799 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx