[PATCH 1/2] drm: allow drivers to provide their own EDID fetching routine

[PATCH 1/2] drm: allow drivers to provide their own EDID fetching routine

[Jesse Barnes]

Make drm_edid_read take a new argument, edid_read, to allow drivers to
provide their own EDID fetch routine.  Export the bit banging DDC over
i2c version of the EDID fetching routine and make the drivers use it.
This sets the stage for GMBUS support in the Intel driver.

Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/drm_edid.c                  |   42 +++++++++++---------------
 drivers/gpu/drm/i915/intel_drv.h            |    1 +
 drivers/gpu/drm/i915/intel_hdmi.c           |    2 +-
 drivers/gpu/drm/i915/intel_modes.c          |    3 +-
 drivers/gpu/drm/i915/intel_sdvo.c           |    9 ++++--
 drivers/gpu/drm/nouveau/nouveau_connector.c |    8 ++++-
 drivers/gpu/drm/radeon/radeon_connectors.c  |    5 ++-
 drivers/gpu/drm/radeon/radeon_display.c     |    6 ++--
 include/drm/drm_crtc.h                      |    5 ++-
 include/drm/drm_edid.h                      |    3 ++
 10 files changed, 47 insertions(+), 37 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 83d8072..fb720e2 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -214,10 +214,10 @@ EXPORT_SYMBOL(drm_edid_is_valid);
  *
  * Try to fetch EDID information by calling i2c driver function.
  */
-static int
-drm_do_probe_ddc_edid(struct i2c_adapter *adapter, unsigned char *buf,
-		      int block, int len)
+int
+drm_ddc_i2c_edid_read(void *data, unsigned char *buf, int block, int len)
 {
+	struct i2c_adapter *adapter = data;
 	unsigned char start = block * EDID_LENGTH;
 	struct i2c_msg msgs[] = {
 		{
@@ -238,9 +238,13 @@ drm_do_probe_ddc_edid(struct i2c_adapter *adapter, unsigned char *buf,
 
 	return -1;
 }
+EXPORT_SYMBOL(drm_ddc_i2c_edid_read);
 
 static u8 *
-drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
+drm_do_get_edid(struct drm_connector *connector,
+		int (*edid_read)(void *data, unsigned char *buf, int block,
+				 int len),
+		void *data)
 {
 	int i, j = 0;
 	u8 *block, *new;
@@ -250,7 +254,7 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
 
 	/* base block fetch */
 	for (i = 0; i < 4; i++) {
-		if (drm_do_probe_ddc_edid(adapter, block, 0, EDID_LENGTH))
+		if (edid_read(data, block, 0, EDID_LENGTH))
 			goto out;
 		if (drm_edid_block_valid(block))
 			break;
@@ -269,8 +273,7 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
 
 	for (j = 1; j <= block[0x7e]; j++) {
 		for (i = 0; i < 4; i++) {
-			if (drm_do_probe_ddc_edid(adapter, block, j,
-						  EDID_LENGTH))
+			if (edid_read(data, block, j, EDID_LENGTH))
 				goto out;
 			if (drm_edid_block_valid(block + j * EDID_LENGTH))
 				break;
@@ -291,20 +294,6 @@ out:
 }
 
 /**
- * Probe DDC presence.
- *
- * \param adapter : i2c device adaptor
- * \return 1 on success
- */
-static bool
-drm_probe_ddc(struct i2c_adapter *adapter)
-{
-	unsigned char out;
-
-	return (drm_do_probe_ddc_edid(adapter, &out, 0, 1) == 0);
-}
-
-/**
  * drm_get_edid - get EDID data, if available
  * @connector: connector we're probing
  * @adapter: i2c adapter to use for DDC
@@ -315,12 +304,17 @@ drm_probe_ddc(struct i2c_adapter *adapter)
  * Return edid data or NULL if we couldn't find any.
  */
 struct edid *drm_get_edid(struct drm_connector *connector,
-			  struct i2c_adapter *adapter)
+			  int (*edid_read)(void *data, unsigned char *buf,
+					   int block, int len),
+			  void *data)
 {
 	struct edid *edid = NULL;
+	unsigned char out;
 
-	if (drm_probe_ddc(adapter))
-		edid = (struct edid *)drm_do_get_edid(connector, adapter);
+	/* Check for presence first */
+	if (edid_read(data, &out, 0, 1) == 0)
+		edid = (struct edid *)drm_do_get_edid(connector, edid_read,
+						      data);
 
 	connector->display_info.raw_edid = (char *)edid;
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 3fbedd8..75c7161 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -167,6 +167,7 @@ int intel_ddc_get_modes(struct drm_connector *c, struct i2c_adapter *adapter);
 extern bool intel_ddc_probe(struct intel_encoder *intel_encoder);
 void intel_i2c_quirk_set(struct drm_device *dev, bool enable);
 void intel_i2c_reset_gmbus(struct drm_device *dev);
+int intel_gmbus_get_modes(struct drm_connector *c, int pin);
 
 extern void intel_crt_init(struct drm_device *dev);
 extern void intel_hdmi_init(struct drm_device *dev, int sdvox_reg);
diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index 83bd764..f1c8b30 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -143,7 +143,7 @@ intel_hdmi_detect(struct drm_connector *connector)
 	enum drm_connector_status status = connector_status_disconnected;
 
 	hdmi_priv->has_hdmi_sink = false;
-	edid = drm_get_edid(connector,
+	edid = drm_get_edid(connector, drm_ddc_i2c_edid_read,
 			    intel_encoder->ddc_bus);
 
 	if (edid) {
diff --git a/drivers/gpu/drm/i915/intel_modes.c b/drivers/gpu/drm/i915/intel_modes.c
index 4b1fd3d..35d15f8 100644
--- a/drivers/gpu/drm/i915/intel_modes.c
+++ b/drivers/gpu/drm/i915/intel_modes.c
@@ -26,6 +26,7 @@
 #include <linux/slab.h>
 #include <linux/i2c.h>
 #include <linux/fb.h>
+#include <drm/drm_edid.h>
 #include "drmP.h"
 #include "intel_drv.h"
 #include "i915_drv.h"
@@ -77,7 +78,7 @@ int intel_ddc_get_modes(struct drm_connector *connector,
 	int ret = 0;
 
 	intel_i2c_quirk_set(connector->dev, true);
-	edid = drm_get_edid(connector, adapter);
+	edid = drm_get_edid(connector, drm_ddc_i2c_edid_read, adapter);
 	intel_i2c_quirk_set(connector->dev, false);
 	if (edid) {
 		drm_mode_connector_update_edid_property(connector, edid);
diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
index 76993ac..0bdb860 100644
--- a/drivers/gpu/drm/i915/intel_sdvo.c
+++ b/drivers/gpu/drm/i915/intel_sdvo.c
@@ -1514,7 +1514,8 @@ intel_sdvo_hdmi_sink_detect(struct drm_connector *connector)
 	enum drm_connector_status status = connector_status_connected;
 	struct edid *edid = NULL;
 
-	edid = drm_get_edid(connector, intel_encoder->ddc_bus);
+	edid = drm_get_edid(connector, drm_ddc_i2c_edid_read,
+			    intel_encoder->ddc_bus);
 
 	/* This is only applied to SDVO cards with multiple outputs */
 	if (edid == NULL && intel_sdvo_multifunc_encoder(intel_encoder)) {
@@ -1527,7 +1528,8 @@ intel_sdvo_hdmi_sink_detect(struct drm_connector *connector)
 		 */
 		while(temp_ddc > 1) {
 			sdvo_priv->ddc_bus = temp_ddc;
-			edid = drm_get_edid(connector, intel_encoder->ddc_bus);
+			edid = drm_get_edid(connector, drm_ddc_i2c_edid_read,
+					    intel_encoder->ddc_bus);
 			if (edid) {
 				/*
 				 * When we can get the EDID, maybe it is the
@@ -1546,7 +1548,8 @@ intel_sdvo_hdmi_sink_detect(struct drm_connector *connector)
 	 */
 	if (edid == NULL && sdvo_priv->analog_ddc_bus &&
 	    !intel_analog_is_connected(connector->dev))
-		edid = drm_get_edid(connector, sdvo_priv->analog_ddc_bus);
+		edid = drm_get_edid(connector, drm_ddc_i2c_edid_read,
+				    sdvo_priv->analog_ddc_bus);
 
 	if (edid != NULL) {
 		bool is_digital = !!(edid->input & DRM_EDID_INPUT_DIGITAL);
diff --git a/drivers/gpu/drm/nouveau/nouveau_connector.c b/drivers/gpu/drm/nouveau/nouveau_connector.c
index 149ed22..69c86e7 100644
--- a/drivers/gpu/drm/nouveau/nouveau_connector.c
+++ b/drivers/gpu/drm/nouveau/nouveau_connector.c
@@ -260,7 +260,9 @@ nouveau_connector_detect(struct drm_connector *connector)
 	i2c = nouveau_connector_ddc_detect(connector, &nv_encoder);
 	if (i2c) {
 		nouveau_connector_ddc_prepare(connector, &flags);
-		nv_connector->edid = drm_get_edid(connector, &i2c->adapter);
+		nv_connector->edid = drm_get_edid(connector,
+						  drm_ddc_i2c_edid_read,
+						  &i2c->adapter);
 		nouveau_connector_ddc_finish(connector, flags);
 		drm_mode_connector_update_edid_property(connector,
 							nv_connector->edid);
@@ -693,7 +695,9 @@ nouveau_connector_create_lvds(struct drm_device *dev,
 
 	if (i2c) {
 		nouveau_connector_ddc_prepare(connector, &flags);
-		nv_connector->edid = drm_get_edid(connector, &i2c->adapter);
+		nv_connector->edid = drm_get_edid(connector,
+						  drm_ddc_i2c_edid_read,
+						  &i2c->adapter);
 		nouveau_connector_ddc_finish(connector, flags);
 	}
 
diff --git a/drivers/gpu/drm/radeon/radeon_connectors.c b/drivers/gpu/drm/radeon/radeon_connectors.c
index f58f8bd..8e129c2 100644
--- a/drivers/gpu/drm/radeon/radeon_connectors.c
+++ b/drivers/gpu/drm/radeon/radeon_connectors.c
@@ -489,6 +489,7 @@ static enum drm_connector_status radeon_lvds_detect(struct drm_connector *connec
 	else {
 		if (radeon_connector->ddc_bus) {
 			radeon_connector->edid = drm_get_edid(&radeon_connector->base,
+							      drm_ddc_i2c_edid_read,
 							      &radeon_connector->ddc_bus->adapter);
 			if (radeon_connector->edid)
 				ret = connector_status_connected;
@@ -601,7 +602,7 @@ static enum drm_connector_status radeon_vga_detect(struct drm_connector *connect
 			kfree(radeon_connector->edid);
 			radeon_connector->edid = NULL;
 		}
-		radeon_connector->edid = drm_get_edid(&radeon_connector->base, &radeon_connector->ddc_bus->adapter);
+		radeon_connector->edid = drm_get_edid(&radeon_connector->base, drm_ddc_i2c_edid_read, &radeon_connector->ddc_bus->adapter);
 
 		if (!radeon_connector->edid) {
 			DRM_ERROR("%s: probed a monitor but no|invalid EDID\n",
@@ -753,7 +754,7 @@ static enum drm_connector_status radeon_dvi_detect(struct drm_connector *connect
 			kfree(radeon_connector->edid);
 			radeon_connector->edid = NULL;
 		}
-		radeon_connector->edid = drm_get_edid(&radeon_connector->base, &radeon_connector->ddc_bus->adapter);
+		radeon_connector->edid = drm_get_edid(&radeon_connector->base, drm_ddc_i2c_edid_read, &radeon_connector->ddc_bus->adapter);
 
 		if (!radeon_connector->edid) {
 			DRM_ERROR("%s: probed a monitor but no|invalid EDID\n",
diff --git a/drivers/gpu/drm/radeon/radeon_display.c b/drivers/gpu/drm/radeon/radeon_display.c
index 8154cdf..0f40c93 100644
--- a/drivers/gpu/drm/radeon/radeon_display.c
+++ b/drivers/gpu/drm/radeon/radeon_display.c
@@ -400,12 +400,12 @@ int radeon_ddc_get_modes(struct radeon_connector *radeon_connector)
 		struct radeon_connector_atom_dig *dig = radeon_connector->con_priv;
 		if ((dig->dp_sink_type == CONNECTOR_OBJECT_ID_DISPLAYPORT ||
 		     dig->dp_sink_type == CONNECTOR_OBJECT_ID_eDP) && dig->dp_i2c_bus)
-			radeon_connector->edid = drm_get_edid(&radeon_connector->base, &dig->dp_i2c_bus->adapter);
+			radeon_connector->edid = drm_get_edid(&radeon_connector->base, drm_ddc_i2c_edid_read, &dig->dp_i2c_bus->adapter);
 	}
 	if (!radeon_connector->ddc_bus)
 		return -1;
 	if (!radeon_connector->edid) {
-		radeon_connector->edid = drm_get_edid(&radeon_connector->base, &radeon_connector->ddc_bus->adapter);
+		radeon_connector->edid = drm_get_edid(&radeon_connector->base, drm_ddc_i2c_edid_read, &radeon_connector->ddc_bus->adapter);
 	}
 	/* some servers provide a hardcoded edid in rom for KVMs */
 	if (!radeon_connector->edid)
@@ -427,7 +427,7 @@ static int radeon_ddc_dump(struct drm_connector *connector)
 
 	if (!radeon_connector->ddc_bus)
 		return -1;
-	edid = drm_get_edid(connector, &radeon_connector->ddc_bus->adapter);
+	edid = drm_get_edid(connector, drm_ddc_i2c_edid_read, &radeon_connector->ddc_bus->adapter);
 	if (edid) {
 		kfree(edid);
 	}
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 93a1a31..027286e 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -664,7 +664,10 @@ extern char *drm_get_tv_select_name(int val);
 extern void drm_fb_release(struct drm_file *file_priv);
 extern int drm_mode_group_init_legacy_group(struct drm_device *dev, struct drm_mode_group *group);
 extern struct edid *drm_get_edid(struct drm_connector *connector,
-				 struct i2c_adapter *adapter);
+				 int (*edid_read)(void *data,
+						  unsigned char *buf,
+						  int block, int len),
+				 void *data);
 extern int drm_add_edid_modes(struct drm_connector *connector, struct edid *edid);
 extern void drm_mode_probed_add(struct drm_connector *connector, struct drm_display_mode *mode);
 extern void drm_mode_remove(struct drm_connector *connector, struct drm_display_mode *mode);
diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
index 39e2cc5..af6ccfa 100644
--- a/include/drm/drm_edid.h
+++ b/include/drm/drm_edid.h
@@ -201,4 +201,7 @@ struct edid {
 
 #define EDID_PRODUCT_ID(e) ((e)->prod_code[0] | ((e)->prod_code[1] << 8))
 
+extern int drm_ddc_i2c_edid_read(void *data, unsigned char *buf, int block,
+				 int len);
+
 #endif /* __DRM_EDID_H__ */
-- 
1.7.0.4

[PATCH 2/2] drm/i915: use GMBUS for EDID fetching

[Jesse Barnes]

Use the GMBUS interface rather than direct bit banging to grab the EDID
over DDC.  The hope is that this method will be more reliable than bit
banging for fetching EDIDs from buggy monitors or through switches.

Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/i915/i915_reg.h    |   52 ++++++++++--
 drivers/gpu/drm/i915/intel_crt.c   |    7 ++
 drivers/gpu/drm/i915/intel_modes.c |  164 +++++++++++++++++++++++++++++++++++-
 3 files changed, 216 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 42c6024..b89599c 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -507,12 +507,52 @@
 # define GPIO_DATA_VAL_IN		(1 << 12)
 # define GPIO_DATA_PULLUP_DISABLE	(1 << 13)
 
-#define GMBUS0			0x5100
-#define GMBUS1			0x5104
-#define GMBUS2			0x5108
-#define GMBUS3			0x510c
-#define GMBUS4			0x5110
-#define GMBUS5			0x5120
+#define GMBUS0			0x5100 /* clock/port select */
+#define   GMBUS_RATE_100KHZ	(0<<8)
+#define   GMBUS_RATE_50KHZ	(1<<8)
+#define   GMBUS_RATE_400KHZ	(2<<8) /* reserved on Pineview */
+#define   GMBUS_RATE_1MHZ	(3<<8) /* reserved on Pineview */
+#define   GMBUS_HOLD_EXT	(1<<7) /* 300ns hold time, rsvd on Pineview */
+#define   GMBUS_PORT_DISABLED	0
+#define   GMBUS_PORT_SSC	1
+#define   GMBUS_PORT_VGADDC	2
+#define   GMBUS_PORT_PANEL	3
+#define   GMBUS_PORT_DPC	4 /* HDMIC */
+#define   GMBUS_PORT_DPB	5 /* SDVO, HDMIB */
+#define   GMBUS_PORT_DPD	6 /* HDMID */
+				  /* 7 reserved */
+#define GMBUS1			0x5104 /* command/status */
+#define   GMBUS_SW_CLR_INT	(1<<31)
+#define   GMBUS_SW_RDY		(1<<30)
+#define   GMBUS_ENT		(1<<29) /* enable timeout */
+#define   GMBUS_CYCLE_NONE	(0<<25)
+#define   GMBUS_CYCLE_NI_NS_WAIT (1<<25)
+#define   GMBUS_CYCLE_I_NS_WAIT	(3<<25)
+#define   GMBUS_CYCLE_STOP	(4<<25)
+#define   GMBUS_CYCLE_NI_STOP	(5<<25)
+#define   GMBUS_CYCLE_I_STOP	(7<<25)
+#define   GMBUS_BYTE_COUNT_SHIFT 16
+#define   GMBUS_SLAVE_INDEX_SHIFT 8
+#define   GMBUS_SLAVE_ADDR_SHIFT 1
+#define   GMBUS_SLAVE_READ	(1<<0)
+#define   GMBUS_SLAVE_WRITE	(0<<0)
+#define GMBUS2			0x5108 /* status */
+#define   GMBUS_INUSE		(1<<15)
+#define   GMBUS_HW_WAIT_PHASE	(1<<14)
+#define   GMBUS_STALL_TIMEOUT	(1<<13)
+#define   GMBUS_INT		(1<<12)
+#define   GMBUS_HW_RDY		(1<<11)
+#define   GMBUS_SATOER		(1<<10)
+#define   GMBUS_ACTIVE		(1<<9)
+#define GMBUS3			0x510c /* data buffer bytes 3-0 */
+#define GMBUS4			0x5110 /* interrupt mask (Pineview+) */
+#define   GMBUS_SLAVE_TIMEOUT_EN (1<<4)
+#define   GMBUS_NAK_EN		(1<<3)
+#define   GMBUS_IDLE_EN		(1<<2)
+#define   GMBUS_HW_WAIT_EN	(1<<1)
+#define   GMBUS_HW_RDY_EN	(1<<0)
+#define GMBUS5			0x5120 /* byte index */
+#define   GMBUS_2BYTE_INDEX_EN	(1<<31)
 
 /*
  * Clock control & power management
diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c
index ee0732b..60dc11b 100644
--- a/drivers/gpu/drm/i915/intel_crt.c
+++ b/drivers/gpu/drm/i915/intel_crt.c
@@ -453,6 +453,12 @@ static int intel_crt_get_modes(struct drm_connector *connector)
 	struct i2c_adapter *ddc_bus;
 	struct drm_device *dev = connector->dev;
 
+	/* Try GMBUS first */
+	ret = intel_gmbus_get_modes(connector, 0);
+	if (ret) {
+		DRM_DEBUG_DRIVER("got EDID from GMBUS\n");
+		goto end;
+	}
 
 	ret = intel_ddc_get_modes(connector, intel_encoder->ddc_bus);
 	if (ret || !IS_G4X(dev))
@@ -466,6 +472,7 @@ static int intel_crt_get_modes(struct drm_connector *connector)
 			   "DDC bus registration failed for CRTDDC_D.\n");
 		goto end;
 	}
+
 	/* Try to get modes by GPIOD port */
 	ret = intel_ddc_get_modes(connector, ddc_bus);
 	intel_i2c_destroy(ddc_bus);
diff --git a/drivers/gpu/drm/i915/intel_modes.c b/drivers/gpu/drm/i915/intel_modes.c
index 35d15f8..c9b946d 100644
--- a/drivers/gpu/drm/i915/intel_modes.c
+++ b/drivers/gpu/drm/i915/intel_modes.c
@@ -1,6 +1,6 @@
 /*
  * Copyright (c) 2007 Dave Airlie <airlied@linux.ie>
- * Copyright (c) 2007 Intel Corporation
+ * Copyright (c) 2007, 2010 Intel Corporation
  *   Jesse Barnes <jesse.barnes@intel.com>
  *
  * Permission is hereby granted, free of charge, to any person obtaining a
@@ -89,3 +89,165 @@ int intel_ddc_get_modes(struct drm_connector *connector,
 
 	return ret;
 }
+
+static void intel_dump_gmbus(drm_i915_private_t *dev_priv)
+{
+	DRM_DEBUG_DRIVER("GMBUS0: 0x%08x\n", I915_READ(GMBUS0));
+	DRM_DEBUG_DRIVER("GMBUS1: 0x%08x\n", I915_READ(GMBUS1));
+	DRM_DEBUG_DRIVER("GMBUS2: 0x%08x\n", I915_READ(GMBUS2));
+	DRM_DEBUG_DRIVER("GMBUS3: 0x%08x\n", I915_READ(GMBUS3));
+	DRM_DEBUG_DRIVER("GMBUS4: 0x%08x\n", I915_READ(GMBUS4));
+	DRM_DEBUG_DRIVER("GMBUS5: 0x%08x\n", I915_READ(GMBUS5));
+}
+
+static bool intel_gmbus_wait_hw(drm_i915_private_t *dev_priv)
+{
+	unsigned long timeout = jiffies + msecs_to_jiffies(500);
+	u32 gmbus2;
+	bool ret = false;
+
+	do {
+		gmbus2 = I915_READ(GMBUS2);
+
+		if (time_after(jiffies, timeout) ||
+		    (gmbus2 & GMBUS_SATOER)) {
+			DRM_DEBUG_DRIVER("timeout waiting for GMBUS hw\n");
+			intel_dump_gmbus(dev_priv);
+			ret = true;
+			break;
+		}
+	} while(!(gmbus2 & GMBUS_HW_RDY));
+
+	return ret;
+}
+
+static bool intel_gmbus_wait_idle(drm_i915_private_t *dev_priv)
+{
+	unsigned long timeout = jiffies + msecs_to_jiffies(500);
+	bool ret = false;
+
+	while (I915_READ(GMBUS2) & GMBUS_ACTIVE) {
+		if (time_after(jiffies, timeout)) {
+			DRM_DEBUG_DRIVER("timeout waiting for GMBUS idle\n");
+			intel_dump_gmbus(dev_priv);
+			ret = true;
+			break;
+		}
+	}
+
+	return ret;
+}
+
+static void intel_gmbus_reset(drm_i915_private_t *dev_priv)
+{
+	I915_WRITE(GMBUS1, GMBUS_SW_CLR_INT);
+	POSTING_READ(GMBUS1);
+	I915_WRITE(GMBUS1, 0);
+
+	if (intel_gmbus_wait_idle(dev_priv))
+		DRM_DEBUG_DRIVER("warning: gmbus reset timed out\n");
+}
+
+struct intel_gmbus_edid_data {
+	struct drm_device *dev;
+	int pin;
+};
+
+static int intel_gmbus_edid_read(void *data, unsigned char *buf, int block,
+				 int len)
+{
+	struct intel_gmbus_edid_data *edid_read_data = data;
+	drm_i915_private_t *dev_priv = edid_read_data->dev->dev_private;
+	u32 gmbus_port;
+	int pin = edid_read_data->pin, i;
+	unsigned char start = block * EDID_LENGTH;
+
+	if (pin != 0) {
+		DRM_DEBUG_DRIVER("pin not supported\n");
+		return -EINVAL;
+	}
+
+	gmbus_port = GMBUS_PORT_VGADDC;
+
+	intel_gmbus_reset(dev_priv);
+
+	I915_WRITE(GMBUS0, GMBUS_RATE_100KHZ | gmbus_port);
+
+	/* Write start address to DDC port */
+	I915_WRITE(GMBUS3, start);
+	I915_WRITE(GMBUS1, GMBUS_CYCLE_NI_STOP | (1 << GMBUS_BYTE_COUNT_SHIFT) |
+		   (DDC_ADDR << GMBUS_SLAVE_ADDR_SHIFT) |
+		   GMBUS_SLAVE_WRITE);
+	I915_WRITE(GMBUS1, I915_READ(GMBUS1) | GMBUS_SW_RDY);
+	if (intel_gmbus_wait_hw(dev_priv)) {
+		DRM_DEBUG_DRIVER("GMBUS timeout waiting for hw ready\n");
+		return -ETIMEDOUT;
+	}
+
+	/* Start data transfer into buf */
+	I915_WRITE(GMBUS1, GMBUS_CYCLE_NI_STOP |
+		   (len << GMBUS_BYTE_COUNT_SHIFT) |
+		   (DDC_ADDR << GMBUS_SLAVE_ADDR_SHIFT) |
+		   GMBUS_SLAVE_READ);
+	I915_WRITE(GMBUS1, I915_READ(GMBUS1) | GMBUS_SW_RDY);
+	for (i = 0; i < len; i += 4) {
+		u32 data;
+		if (intel_gmbus_wait_hw(dev_priv))
+			break;
+		data = I915_READ(GMBUS3);
+		buf[i] = data & 0xff;
+		if (i + 1 < len)
+			buf[i+1] = (data >> 8) & 0xff;
+		if (i + 2 < len)
+			buf[i+2] = (data >> 16) & 0xff;
+		if (i + 3 < len)
+			buf[i+3] = (data >> 24) & 0xff;
+	}
+
+	/* Send STOP (ignore timeouts) and clear GMBUS0 */
+	intel_gmbus_wait_hw(dev_priv);
+	I915_WRITE(GMBUS1, GMBUS_CYCLE_STOP |
+		   (DDC_ADDR << GMBUS_SLAVE_ADDR_SHIFT));
+	I915_WRITE(GMBUS1, I915_READ(GMBUS1) | GMBUS_SW_RDY);
+	intel_gmbus_wait_idle(dev_priv);
+	I915_WRITE(GMBUS0, 0);
+
+	return 0;
+}
+
+/**
+ * intel_gmbus_get_modes - get modes using DDC over GMBUS
+ * @connector: connector in question
+ * @pin: pin to use
+ *
+ * There are 8 pins available:
+ *   0 - VGA DAC DDC
+ *   1 - i2c data (SSC clock control)
+ *   2 - LVDS DDC
+ *   3 - DVOA DDC (reserved in 9xx+)
+ *   4 - i2c data (add2 control)
+ *   5 - DVOA data (reserved in 9xx+)
+ *   6 - thermal sensor (reserved in 9xx+)
+ *   7 - trusted output
+ * The above are the recommended pin connections, they may vary from
+ * platform to platform; the VBT should contain the actual mappings.
+ */
+int intel_gmbus_get_modes(struct drm_connector *connector, int pin)
+{
+	struct edid *edid;
+	struct intel_gmbus_edid_data data = {
+		.dev = connector->dev,
+		.pin = pin,
+	};
+	int ret = 0;
+
+	edid = drm_get_edid(connector, intel_gmbus_edid_read, &data);
+	if (edid) {
+		drm_mode_connector_update_edid_property(connector, edid);
+		ret = drm_add_edid_modes(connector, edid);
+		connector->display_info.raw_edid = NULL;
+		kfree(edid);
+	}
+
+	return ret;
+}
-- 
1.7.0.4

Re: [PATCH 1/2] drm: allow drivers to provide their own EDID fetching routine

[Dave Airlie]

On Tue, 2010-07-20 at 15:44 -0700, Jesse Barnes wrote:
> Make drm_edid_read take a new argument, edid_read, to allow drivers to
> provide their own EDID fetch routine.  Export the bit banging DDC over
> i2c version of the EDID fetching routine and make the drivers use it.
> This sets the stage for GMBUS support in the Intel driver.
> 

I think this needs some rework.

You might want to checkout what the radeon driver does for hw i2c
engine. You should set up your own i2c hw handlers and use those instead
of bypassing the i2c stack. GMBUS is just another i2c hw block.

Dave.

> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> ---
>  drivers/gpu/drm/drm_edid.c                  |   42 +++++++++++---------------
>  drivers/gpu/drm/i915/intel_drv.h            |    1 +
>  drivers/gpu/drm/i915/intel_hdmi.c           |    2 +-
>  drivers/gpu/drm/i915/intel_modes.c          |    3 +-
>  drivers/gpu/drm/i915/intel_sdvo.c           |    9 ++++--
>  drivers/gpu/drm/nouveau/nouveau_connector.c |    8 ++++-
>  drivers/gpu/drm/radeon/radeon_connectors.c  |    5 ++-
>  drivers/gpu/drm/radeon/radeon_display.c     |    6 ++--
>  include/drm/drm_crtc.h                      |    5 ++-
>  include/drm/drm_edid.h                      |    3 ++
>  10 files changed, 47 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 83d8072..fb720e2 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -214,10 +214,10 @@ EXPORT_SYMBOL(drm_edid_is_valid);
>   *
>   * Try to fetch EDID information by calling i2c driver function.
>   */
> -static int
> -drm_do_probe_ddc_edid(struct i2c_adapter *adapter, unsigned char *buf,
> -		      int block, int len)
> +int
> +drm_ddc_i2c_edid_read(void *data, unsigned char *buf, int block, int len)
>  {
> +	struct i2c_adapter *adapter = data;
>  	unsigned char start = block * EDID_LENGTH;
>  	struct i2c_msg msgs[] = {
>  		{
> @@ -238,9 +238,13 @@ drm_do_probe_ddc_edid(struct i2c_adapter *adapter, unsigned char *buf,
>  
>  	return -1;
>  }
> +EXPORT_SYMBOL(drm_ddc_i2c_edid_read);
>  
>  static u8 *
> -drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
> +drm_do_get_edid(struct drm_connector *connector,
> +		int (*edid_read)(void *data, unsigned char *buf, int block,
> +				 int len),
> +		void *data)
>  {
>  	int i, j = 0;
>  	u8 *block, *new;
> @@ -250,7 +254,7 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
>  
>  	/* base block fetch */
>  	for (i = 0; i < 4; i++) {
> -		if (drm_do_probe_ddc_edid(adapter, block, 0, EDID_LENGTH))
> +		if (edid_read(data, block, 0, EDID_LENGTH))
>  			goto out;
>  		if (drm_edid_block_valid(block))
>  			break;
> @@ -269,8 +273,7 @@ drm_do_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
>  
>  	for (j = 1; j <= block[0x7e]; j++) {
>  		for (i = 0; i < 4; i++) {
> -			if (drm_do_probe_ddc_edid(adapter, block, j,
> -						  EDID_LENGTH))
> +			if (edid_read(data, block, j, EDID_LENGTH))
>  				goto out;
>  			if (drm_edid_block_valid(block + j * EDID_LENGTH))
>  				break;
> @@ -291,20 +294,6 @@ out:
>  }
>  
>  /**
> - * Probe DDC presence.
> - *
> - * \param adapter : i2c device adaptor
> - * \return 1 on success
> - */
> -static bool
> -drm_probe_ddc(struct i2c_adapter *adapter)
> -{
> -	unsigned char out;
> -
> -	return (drm_do_probe_ddc_edid(adapter, &out, 0, 1) == 0);
> -}
> -
> -/**
>   * drm_get_edid - get EDID data, if available
>   * @connector: connector we're probing
>   * @adapter: i2c adapter to use for DDC
> @@ -315,12 +304,17 @@ drm_probe_ddc(struct i2c_adapter *adapter)
>   * Return edid data or NULL if we couldn't find any.
>   */
>  struct edid *drm_get_edid(struct drm_connector *connector,
> -			  struct i2c_adapter *adapter)
> +			  int (*edid_read)(void *data, unsigned char *buf,
> +					   int block, int len),
> +			  void *data)
>  {
>  	struct edid *edid = NULL;
> +	unsigned char out;
>  
> -	if (drm_probe_ddc(adapter))
> -		edid = (struct edid *)drm_do_get_edid(connector, adapter);
> +	/* Check for presence first */
> +	if (edid_read(data, &out, 0, 1) == 0)
> +		edid = (struct edid *)drm_do_get_edid(connector, edid_read,
> +						      data);
>  
>  	connector->display_info.raw_edid = (char *)edid;
>  
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 3fbedd8..75c7161 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -167,6 +167,7 @@ int intel_ddc_get_modes(struct drm_connector *c, struct i2c_adapter *adapter);
>  extern bool intel_ddc_probe(struct intel_encoder *intel_encoder);
>  void intel_i2c_quirk_set(struct drm_device *dev, bool enable);
>  void intel_i2c_reset_gmbus(struct drm_device *dev);
> +int intel_gmbus_get_modes(struct drm_connector *c, int pin);
>  
>  extern void intel_crt_init(struct drm_device *dev);
>  extern void intel_hdmi_init(struct drm_device *dev, int sdvox_reg);
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> index 83bd764..f1c8b30 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -143,7 +143,7 @@ intel_hdmi_detect(struct drm_connector *connector)
>  	enum drm_connector_status status = connector_status_disconnected;
>  
>  	hdmi_priv->has_hdmi_sink = false;
> -	edid = drm_get_edid(connector,
> +	edid = drm_get_edid(connector, drm_ddc_i2c_edid_read,
>  			    intel_encoder->ddc_bus);
>  
>  	if (edid) {
> diff --git a/drivers/gpu/drm/i915/intel_modes.c b/drivers/gpu/drm/i915/intel_modes.c
> index 4b1fd3d..35d15f8 100644
> --- a/drivers/gpu/drm/i915/intel_modes.c
> +++ b/drivers/gpu/drm/i915/intel_modes.c
> @@ -26,6 +26,7 @@
>  #include <linux/slab.h>
>  #include <linux/i2c.h>
>  #include <linux/fb.h>
> +#include <drm/drm_edid.h>
>  #include "drmP.h"
>  #include "intel_drv.h"
>  #include "i915_drv.h"
> @@ -77,7 +78,7 @@ int intel_ddc_get_modes(struct drm_connector *connector,
>  	int ret = 0;
>  
>  	intel_i2c_quirk_set(connector->dev, true);
> -	edid = drm_get_edid(connector, adapter);
> +	edid = drm_get_edid(connector, drm_ddc_i2c_edid_read, adapter);
>  	intel_i2c_quirk_set(connector->dev, false);
>  	if (edid) {
>  		drm_mode_connector_update_edid_property(connector, edid);
> diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
> index 76993ac..0bdb860 100644
> --- a/drivers/gpu/drm/i915/intel_sdvo.c
> +++ b/drivers/gpu/drm/i915/intel_sdvo.c
> @@ -1514,7 +1514,8 @@ intel_sdvo_hdmi_sink_detect(struct drm_connector *connector)
>  	enum drm_connector_status status = connector_status_connected;
>  	struct edid *edid = NULL;
>  
> -	edid = drm_get_edid(connector, intel_encoder->ddc_bus);
> +	edid = drm_get_edid(connector, drm_ddc_i2c_edid_read,
> +			    intel_encoder->ddc_bus);
>  
>  	/* This is only applied to SDVO cards with multiple outputs */
>  	if (edid == NULL && intel_sdvo_multifunc_encoder(intel_encoder)) {
> @@ -1527,7 +1528,8 @@ intel_sdvo_hdmi_sink_detect(struct drm_connector *connector)
>  		 */
>  		while(temp_ddc > 1) {
>  			sdvo_priv->ddc_bus = temp_ddc;
> -			edid = drm_get_edid(connector, intel_encoder->ddc_bus);
> +			edid = drm_get_edid(connector, drm_ddc_i2c_edid_read,
> +					    intel_encoder->ddc_bus);
>  			if (edid) {
>  				/*
>  				 * When we can get the EDID, maybe it is the
> @@ -1546,7 +1548,8 @@ intel_sdvo_hdmi_sink_detect(struct drm_connector *connector)
>  	 */
>  	if (edid == NULL && sdvo_priv->analog_ddc_bus &&
>  	    !intel_analog_is_connected(connector->dev))
> -		edid = drm_get_edid(connector, sdvo_priv->analog_ddc_bus);
> +		edid = drm_get_edid(connector, drm_ddc_i2c_edid_read,
> +				    sdvo_priv->analog_ddc_bus);
>  
>  	if (edid != NULL) {
>  		bool is_digital = !!(edid->input & DRM_EDID_INPUT_DIGITAL);
> diff --git a/drivers/gpu/drm/nouveau/nouveau_connector.c b/drivers/gpu/drm/nouveau/nouveau_connector.c
> index 149ed22..69c86e7 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_connector.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_connector.c
> @@ -260,7 +260,9 @@ nouveau_connector_detect(struct drm_connector *connector)
>  	i2c = nouveau_connector_ddc_detect(connector, &nv_encoder);
>  	if (i2c) {
>  		nouveau_connector_ddc_prepare(connector, &flags);
> -		nv_connector->edid = drm_get_edid(connector, &i2c->adapter);
> +		nv_connector->edid = drm_get_edid(connector,
> +						  drm_ddc_i2c_edid_read,
> +						  &i2c->adapter);
>  		nouveau_connector_ddc_finish(connector, flags);
>  		drm_mode_connector_update_edid_property(connector,
>  							nv_connector->edid);
> @@ -693,7 +695,9 @@ nouveau_connector_create_lvds(struct drm_device *dev,
>  
>  	if (i2c) {
>  		nouveau_connector_ddc_prepare(connector, &flags);
> -		nv_connector->edid = drm_get_edid(connector, &i2c->adapter);
> +		nv_connector->edid = drm_get_edid(connector,
> +						  drm_ddc_i2c_edid_read,
> +						  &i2c->adapter);
>  		nouveau_connector_ddc_finish(connector, flags);
>  	}
>  
> diff --git a/drivers/gpu/drm/radeon/radeon_connectors.c b/drivers/gpu/drm/radeon/radeon_connectors.c
> index f58f8bd..8e129c2 100644
> --- a/drivers/gpu/drm/radeon/radeon_connectors.c
> +++ b/drivers/gpu/drm/radeon/radeon_connectors.c
> @@ -489,6 +489,7 @@ static enum drm_connector_status radeon_lvds_detect(struct drm_connector *connec
>  	else {
>  		if (radeon_connector->ddc_bus) {
>  			radeon_connector->edid = drm_get_edid(&radeon_connector->base,
> +							      drm_ddc_i2c_edid_read,
>  							      &radeon_connector->ddc_bus->adapter);
>  			if (radeon_connector->edid)
>  				ret = connector_status_connected;
> @@ -601,7 +602,7 @@ static enum drm_connector_status radeon_vga_detect(struct drm_connector *connect
>  			kfree(radeon_connector->edid);
>  			radeon_connector->edid = NULL;
>  		}
> -		radeon_connector->edid = drm_get_edid(&radeon_connector->base, &radeon_connector->ddc_bus->adapter);
> +		radeon_connector->edid = drm_get_edid(&radeon_connector->base, drm_ddc_i2c_edid_read, &radeon_connector->ddc_bus->adapter);
>  
>  		if (!radeon_connector->edid) {
>  			DRM_ERROR("%s: probed a monitor but no|invalid EDID\n",
> @@ -753,7 +754,7 @@ static enum drm_connector_status radeon_dvi_detect(struct drm_connector *connect
>  			kfree(radeon_connector->edid);
>  			radeon_connector->edid = NULL;
>  		}
> -		radeon_connector->edid = drm_get_edid(&radeon_connector->base, &radeon_connector->ddc_bus->adapter);
> +		radeon_connector->edid = drm_get_edid(&radeon_connector->base, drm_ddc_i2c_edid_read, &radeon_connector->ddc_bus->adapter);
>  
>  		if (!radeon_connector->edid) {
>  			DRM_ERROR("%s: probed a monitor but no|invalid EDID\n",
> diff --git a/drivers/gpu/drm/radeon/radeon_display.c b/drivers/gpu/drm/radeon/radeon_display.c
> index 8154cdf..0f40c93 100644
> --- a/drivers/gpu/drm/radeon/radeon_display.c
> +++ b/drivers/gpu/drm/radeon/radeon_display.c
> @@ -400,12 +400,12 @@ int radeon_ddc_get_modes(struct radeon_connector *radeon_connector)
>  		struct radeon_connector_atom_dig *dig = radeon_connector->con_priv;
>  		if ((dig->dp_sink_type == CONNECTOR_OBJECT_ID_DISPLAYPORT ||
>  		     dig->dp_sink_type == CONNECTOR_OBJECT_ID_eDP) && dig->dp_i2c_bus)
> -			radeon_connector->edid = drm_get_edid(&radeon_connector->base, &dig->dp_i2c_bus->adapter);
> +			radeon_connector->edid = drm_get_edid(&radeon_connector->base, drm_ddc_i2c_edid_read, &dig->dp_i2c_bus->adapter);
>  	}
>  	if (!radeon_connector->ddc_bus)
>  		return -1;
>  	if (!radeon_connector->edid) {
> -		radeon_connector->edid = drm_get_edid(&radeon_connector->base, &radeon_connector->ddc_bus->adapter);
> +		radeon_connector->edid = drm_get_edid(&radeon_connector->base, drm_ddc_i2c_edid_read, &radeon_connector->ddc_bus->adapter);
>  	}
>  	/* some servers provide a hardcoded edid in rom for KVMs */
>  	if (!radeon_connector->edid)
> @@ -427,7 +427,7 @@ static int radeon_ddc_dump(struct drm_connector *connector)
>  
>  	if (!radeon_connector->ddc_bus)
>  		return -1;
> -	edid = drm_get_edid(connector, &radeon_connector->ddc_bus->adapter);
> +	edid = drm_get_edid(connector, drm_ddc_i2c_edid_read, &radeon_connector->ddc_bus->adapter);
>  	if (edid) {
>  		kfree(edid);
>  	}
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 93a1a31..027286e 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -664,7 +664,10 @@ extern char *drm_get_tv_select_name(int val);
>  extern void drm_fb_release(struct drm_file *file_priv);
>  extern int drm_mode_group_init_legacy_group(struct drm_device *dev, struct drm_mode_group *group);
>  extern struct edid *drm_get_edid(struct drm_connector *connector,
> -				 struct i2c_adapter *adapter);
> +				 int (*edid_read)(void *data,
> +						  unsigned char *buf,
> +						  int block, int len),
> +				 void *data);
>  extern int drm_add_edid_modes(struct drm_connector *connector, struct edid *edid);
>  extern void drm_mode_probed_add(struct drm_connector *connector, struct drm_display_mode *mode);
>  extern void drm_mode_remove(struct drm_connector *connector, struct drm_display_mode *mode);
> diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
> index 39e2cc5..af6ccfa 100644
> --- a/include/drm/drm_edid.h
> +++ b/include/drm/drm_edid.h
> @@ -201,4 +201,7 @@ struct edid {
>  
>  #define EDID_PRODUCT_ID(e) ((e)->prod_code[0] | ((e)->prod_code[1] << 8))
>  
> +extern int drm_ddc_i2c_edid_read(void *data, unsigned char *buf, int block,
> +				 int len);
> +
>  #endif /* __DRM_EDID_H__ */

Re: [PATCH 1/2] drm: allow drivers to provide their own EDID fetching routine

[Jesse Barnes]

On Wed, 21 Jul 2010 08:54:30 +1000
Dave Airlie <airlied@redhat.com> wrote:

> On Tue, 2010-07-20 at 15:44 -0700, Jesse Barnes wrote:
> > Make drm_edid_read take a new argument, edid_read, to allow drivers to
> > provide their own EDID fetch routine.  Export the bit banging DDC over
> > i2c version of the EDID fetching routine and make the drivers use it.
> > This sets the stage for GMBUS support in the Intel driver.
> > 
> 
> I think this needs some rework.
> 
> You might want to checkout what the radeon driver does for hw i2c
> engine. You should set up your own i2c hw handlers and use those instead
> of bypassing the i2c stack. GMBUS is just another i2c hw block.

I'll check it out, but I don't see what using the i2c stack buys us
here except for obfuscation...

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 1/2] drm: allow drivers to provide their own EDID fetching routine

[Dave Airlie]

On Tue, 2010-07-20 at 16:05 -0700, Jesse Barnes wrote:
> On Wed, 21 Jul 2010 08:54:30 +1000
> Dave Airlie <airlied@redhat.com> wrote:
> 
> > On Tue, 2010-07-20 at 15:44 -0700, Jesse Barnes wrote:
> > > Make drm_edid_read take a new argument, edid_read, to allow drivers to
> > > provide their own EDID fetch routine.  Export the bit banging DDC over
> > > i2c version of the EDID fetching routine and make the drivers use it.
> > > This sets the stage for GMBUS support in the Intel driver.
> > > 
> > 
> > I think this needs some rework.
> > 
> > You might want to checkout what the radeon driver does for hw i2c
> > engine. You should set up your own i2c hw handlers and use those instead
> > of bypassing the i2c stack. GMBUS is just another i2c hw block.
> 
> I'll check it out, but I don't see what using the i2c stack buys us
> here except for obfuscation...
> 

You'll want to use GMBUS for SDVO at some point in the future, or
something else, or you'll want to expose it to userspace for DDC/CI
users. Lots of reasons, its not obfuscation at all, what you are doing
is dodgy shortcuts.

Dave.

Re: [PATCH 1/2] drm: allow drivers to provide their own EDID fetching routine

[Jesse Barnes]

On Wed, 21 Jul 2010 09:27:54 +1000
Dave Airlie <airlied@redhat.com> wrote:

> On Tue, 2010-07-20 at 16:05 -0700, Jesse Barnes wrote:
> > On Wed, 21 Jul 2010 08:54:30 +1000
> > Dave Airlie <airlied@redhat.com> wrote:
> > 
> > > On Tue, 2010-07-20 at 15:44 -0700, Jesse Barnes wrote:
> > > > Make drm_edid_read take a new argument, edid_read, to allow drivers to
> > > > provide their own EDID fetch routine.  Export the bit banging DDC over
> > > > i2c version of the EDID fetching routine and make the drivers use it.
> > > > This sets the stage for GMBUS support in the Intel driver.
> > > > 
> > > 
> > > I think this needs some rework.
> > > 
> > > You might want to checkout what the radeon driver does for hw i2c
> > > engine. You should set up your own i2c hw handlers and use those instead
> > > of bypassing the i2c stack. GMBUS is just another i2c hw block.
> > 
> > I'll check it out, but I don't see what using the i2c stack buys us
> > here except for obfuscation...
> > 
> 
> You'll want to use GMBUS for SDVO at some point in the future, or
> something else, or you'll want to expose it to userspace for DDC/CI
> users. Lots of reasons, its not obfuscation at all, what you are doing
> is dodgy shortcuts.

Using it for SDVO and other things means some other changes to the
GMBUS code unfortunately.  Still not seeing how using i2c makes
userspace exposure or SDVO usage easier, but I don't care, I'll switch
it around to use i2c core code.

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 1/2] drm: allow drivers to provide their own EDID fetching routine

[Dave Airlie]

On Tue, 2010-07-20 at 16:34 -0700, Jesse Barnes wrote:
> On Wed, 21 Jul 2010 09:27:54 +1000
> Dave Airlie <airlied@redhat.com> wrote:
> 
> > On Tue, 2010-07-20 at 16:05 -0700, Jesse Barnes wrote:
> > > On Wed, 21 Jul 2010 08:54:30 +1000
> > > Dave Airlie <airlied@redhat.com> wrote:
> > > 
> > > > On Tue, 2010-07-20 at 15:44 -0700, Jesse Barnes wrote:
> > > > > Make drm_edid_read take a new argument, edid_read, to allow drivers to
> > > > > provide their own EDID fetch routine.  Export the bit banging DDC over
> > > > > i2c version of the EDID fetching routine and make the drivers use it.
> > > > > This sets the stage for GMBUS support in the Intel driver.
> > > > > 
> > > > 
> > > > I think this needs some rework.
> > > > 
> > > > You might want to checkout what the radeon driver does for hw i2c
> > > > engine. You should set up your own i2c hw handlers and use those instead
> > > > of bypassing the i2c stack. GMBUS is just another i2c hw block.
> > > 
> > > I'll check it out, but I don't see what using the i2c stack buys us
> > > here except for obfuscation...
> > > 
> > 
> > You'll want to use GMBUS for SDVO at some point in the future, or
> > something else, or you'll want to expose it to userspace for DDC/CI
> > users. Lots of reasons, its not obfuscation at all, what you are doing
> > is dodgy shortcuts.
> 
> Using it for SDVO and other things means some other changes to the
> GMBUS code unfortunately.  Still not seeing how using i2c makes
> userspace exposure or SDVO usage easier, but I don't care, I'll switch
> it around to use i2c core code.
> 

You can expose /dev/i2c devices properly to userspace, you can't do that
with the code you proposed.

Dave.

Re: [PATCH 1/2] drm: allow drivers to provide their own EDID fetching routine

[Jesse Barnes]

On Tue, 20 Jul 2010 16:34:39 -0700
Jesse Barnes <jbarnes@virtuousgeek.org> wrote:

> On Wed, 21 Jul 2010 09:27:54 +1000
> Dave Airlie <airlied@redhat.com> wrote:
> 
> > On Tue, 2010-07-20 at 16:05 -0700, Jesse Barnes wrote:
> > > On Wed, 21 Jul 2010 08:54:30 +1000
> > > Dave Airlie <airlied@redhat.com> wrote:
> > > 
> > > > On Tue, 2010-07-20 at 15:44 -0700, Jesse Barnes wrote:
> > > > > Make drm_edid_read take a new argument, edid_read, to allow drivers to
> > > > > provide their own EDID fetch routine.  Export the bit banging DDC over
> > > > > i2c version of the EDID fetching routine and make the drivers use it.
> > > > > This sets the stage for GMBUS support in the Intel driver.
> > > > > 
> > > > 
> > > > I think this needs some rework.
> > > > 
> > > > You might want to checkout what the radeon driver does for hw i2c
> > > > engine. You should set up your own i2c hw handlers and use those instead
> > > > of bypassing the i2c stack. GMBUS is just another i2c hw block.
> > > 
> > > I'll check it out, but I don't see what using the i2c stack buys us
> > > here except for obfuscation...
> > > 
> > 
> > You'll want to use GMBUS for SDVO at some point in the future, or
> > something else, or you'll want to expose it to userspace for DDC/CI
> > users. Lots of reasons, its not obfuscation at all, what you are doing
> > is dodgy shortcuts.
> 
> Using it for SDVO and other things means some other changes to the
> GMBUS code unfortunately.  Still not seeing how using i2c makes
> userspace exposure or SDVO usage easier, but I don't care, I'll switch
> it around to use i2c core code.

Assuming it helps a all of course.  My hope here was that it would be
able to get EDID out of configurations where we currently have trouble,
like monitor switches or just plain crappy monitors.  If it doesn't
help I don't think there's much need to keep the code around...

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [Intel-gfx] [PATCH 1/2] drm: allow drivers to provide their own EDID fetching routine

[Alex Deucher]

On Tue, Jul 20, 2010 at 7:43 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> On Tue, 20 Jul 2010 16:34:39 -0700
> Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
>
>> On Wed, 21 Jul 2010 09:27:54 +1000
>> Dave Airlie <airlied@redhat.com> wrote:
>>
>> > On Tue, 2010-07-20 at 16:05 -0700, Jesse Barnes wrote:
>> > > On Wed, 21 Jul 2010 08:54:30 +1000
>> > > Dave Airlie <airlied@redhat.com> wrote:
>> > >
>> > > > On Tue, 2010-07-20 at 15:44 -0700, Jesse Barnes wrote:
>> > > > > Make drm_edid_read take a new argument, edid_read, to allow drivers to
>> > > > > provide their own EDID fetch routine.  Export the bit banging DDC over
>> > > > > i2c version of the EDID fetching routine and make the drivers use it.
>> > > > > This sets the stage for GMBUS support in the Intel driver.
>> > > > >
>> > > >
>> > > > I think this needs some rework.
>> > > >
>> > > > You might want to checkout what the radeon driver does for hw i2c
>> > > > engine. You should set up your own i2c hw handlers and use those instead
>> > > > of bypassing the i2c stack. GMBUS is just another i2c hw block.
>> > >
>> > > I'll check it out, but I don't see what using the i2c stack buys us
>> > > here except for obfuscation...
>> > >
>> >
>> > You'll want to use GMBUS for SDVO at some point in the future, or
>> > something else, or you'll want to expose it to userspace for DDC/CI
>> > users. Lots of reasons, its not obfuscation at all, what you are doing
>> > is dodgy shortcuts.
>>
>> Using it for SDVO and other things means some other changes to the
>> GMBUS code unfortunately.  Still not seeing how using i2c makes
>> userspace exposure or SDVO usage easier, but I don't care, I'll switch
>> it around to use i2c core code.
>
> Assuming it helps a all of course.  My hope here was that it would be
> able to get EDID out of configurations where we currently have trouble,
> like monitor switches or just plain crappy monitors.  If it doesn't
> help I don't think there's much need to keep the code around...

>From my experience, I doubt you'll see any improvement; KVMs and
crappy monitors are still crappy.  The hw i2c engines are mostly there
to reduce CPU usage and allow i2c transactions to be interrupt driven.

Alex