drm: allow drivers to provide their own EDID fetching routine

PATCH In review Jesse Barnes 3 participants 9 messages Jul 20, 2010 – Jul 21, 2010

Subsystems (5)

DRM drivers DRM misc Intel i915 Radeon and amdgpu DRM drivers +1 more subsystems

lore

Share & tools

lore

Jesse Barnes

author

2010-07-20 22:44 UTC #0 lore raw

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

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 <[email protected]>

---
 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(-)

drivers/gpu/drm/drm_edid.c +18 ~~-24~~

index 83d8072..fb720e2 100644
@@ -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;

drivers/gpu/drm/i915/intel_drv.h +1 -0

index 3fbedd8..75c7161 100644
@@ -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);

drivers/gpu/drm/i915/intel_hdmi.c +1 -1

index 83bd764..f1c8b30 100644
@@ -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) {

drivers/gpu/drm/i915/intel_modes.c +2 -1

index 4b1fd3d..35d15f8 100644
@@ -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);

drivers/gpu/drm/i915/intel_sdvo.c +6 -3

index 76993ac..0bdb860 100644
@@ -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);

drivers/gpu/drm/nouveau/nouveau_connector.c +6 -2

index 149ed22..69c86e7 100644
@@ -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);
 	}

drivers/gpu/drm/radeon/radeon_connectors.c +3 -2

index f58f8bd..8e129c2 100644
@@ -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",

drivers/gpu/drm/radeon/radeon_display.c +3 -3

index 8154cdf..0f40c93 100644
@@ -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);
 	}

include/drm/drm_crtc.h +4 -1

index 93a1a31..027286e 100644
@@ -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);

include/drm/drm_edid.h +3 -0

index 39e2cc5..af6ccfa 100644
@@ -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__ */

Signature · 1 line

1.7.0.4

Jesse Barnes

author

2010-07-20 22:44 UTC #1 lore raw

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

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 <[email protected]>

---
 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(-)

drivers/gpu/drm/i915/i915_reg.h +46 -6

index 42c6024..b89599c 100644
@@ -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

drivers/gpu/drm/i915/intel_crt.c +7 -0

index ee0732b..60dc11b 100644
@@ -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);

drivers/gpu/drm/i915/intel_modes.c +163 -1

172 lines not on this page · raw message

Signature · 1 line

1.7.0.4

Dave Airlie

2010-07-20 22:54 UTC #2 lore raw

In reply to Jesse Barnes

Quoted 5 lines · 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.

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.

Quoted 300 lines

Signed-off-by Jesse Barnes <[email protected]>

---
 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(-)

drivers/gpu/drm/drm_edid.c +18 ~~-24~~

index 83d8072..fb720e2 100644
@@ -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;

drivers/gpu/drm/i915/intel_drv.h +1 -0

index 3fbedd8..75c7161 100644
@@ -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);

drivers/gpu/drm/i915/intel_hdmi.c +1 -1

index 83bd764..f1c8b30 100644
@@ -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) {

drivers/gpu/drm/i915/intel_modes.c +2 -1

index 4b1fd3d..35d15f8 100644
@@ -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);

drivers/gpu/drm/i915/intel_sdvo.c +6 -3

index 76993ac..0bdb860 100644
@@ -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);

drivers/gpu/drm/nouveau/nouveau_connector.c +6 -2

index 149ed22..69c86e7 100644
@@ -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);
 	}

drivers/gpu/drm/radeon/radeon_connectors.c +3 -2

index f58f8bd..8e129c2 100644
@@ -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",

drivers/gpu/drm/radeon/radeon_display.c +3 -3

index 8154cdf..0f40c93 100644
@@ -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);
 	}

include/drm/drm_crtc.h +4 -1

index 93a1a31..027286e 100644
@@ -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);

include/drm/drm_edid.h +3 -0

index 39e2cc5..af6ccfa 100644
@@ -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__ */

Jesse Barnes

author

2010-07-20 23:05 UTC #3 lore raw

Quoted 7 lines · On Wed, 21 Jul 2010 08:54:30 +1000 Dave Airlie <[email protected]> wrote:

Quoted 5 lines · 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.

Quoted 5 lines

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

Signature · 1 line

Jesse Barnes, Intel Open Source Technology Center

Dave Airlie

2010-07-20 23:27 UTC #4 lore raw

Quoted 10 lines · Jesse Barnes

Quoted 7 lines · On Wed, 21 Jul 2010 08:54:30 +1000 Dave Airlie <[email protected]> wrote:
Quoted 5 lines · 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.

Quoted 9 lines

Quoted 5 lines
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.

Jesse Barnes

author

2010-07-20 23:34 UTC #5 lore raw

Quoted 17 lines · On Wed, 21 Jul 2010 09:27:54 +1000 Dave Airlie <[email protected]> wrote:

Quoted 16 lines · Jesse Barnes
Quoted 12 lines · On Wed, 21 Jul 2010 08:54:30 +1000 Dave Airlie <[email protected]> wrote:
Quoted 5 lines · 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.
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.

Quoted 8 lines

Quoted 3 lines
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.

Signature · 1 line

Jesse Barnes, Intel Open Source Technology Center

Dave Airlie

2010-07-20 23:35 UTC #6 lore raw

Quoted 29 lines · Jesse Barnes

Quoted 17 lines · On Wed, 21 Jul 2010 09:27:54 +1000 Dave Airlie <[email protected]> wrote:
Quoted 16 lines · Jesse Barnes
Quoted 12 lines · On Wed, 21 Jul 2010 08:54:30 +1000 Dave Airlie <[email protected]> wrote:
Quoted 5 lines · 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.
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.
Quoted 8 lines
Quoted 3 lines
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.

Quoted 5 lines

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.

Jesse Barnes

author

2010-07-20 23:42 UTC #7 lore raw

In reply to Jesse Barnes

Quoted 29 lines · On Tue, 20 Jul 2010 16:34:39 -0700 Jesse Barnes <[email protected]>…

Quoted 17 lines · On Wed, 21 Jul 2010 09:27:54 +1000 Dave Airlie <[email protected]> wrote:
Quoted 16 lines · Jesse Barnes
Quoted 12 lines · On Wed, 21 Jul 2010 08:54:30 +1000 Dave Airlie <[email protected]> wrote:
Quoted 5 lines · 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.
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.
Quoted 8 lines
Quoted 3 lines
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.

Quoted 4 lines

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

Signature · 1 line

Jesse Barnes, Intel Open Source Technology Center

Alex Deucher

2010-07-21 00:08 UTC #8 lore raw

Quoted 37 lines · Jesse Barnes

Quoted 29 lines · On Tue, 20 Jul 2010 16:34:39 -0700 Jesse Barnes <[email protected]>…
Quoted 17 lines · On Wed, 21 Jul 2010 09:27:54 +1000 Dave Airlie <[email protected]> wrote:
Quoted 16 lines · Jesse Barnes
Quoted 12 lines · On Wed, 21 Jul 2010 08:54:30 +1000 Dave Airlie <[email protected]> wrote:
Quoted 5 lines · 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.
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.
Quoted 8 lines
Quoted 3 lines
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.
Quoted 4 lines
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.

Quoted 6 lines

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