All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/3] Reset i2c connection between xfers
@ 2014-04-04 14:40 Alex Deucher
  2014-04-04 14:40 ` [PATCH 1/3] drm/radeon/dp: handle zero sized i2c over aux transactions Alex Deucher
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Alex Deucher @ 2014-04-04 14:40 UTC (permalink / raw)
  To: dri-devel, jani.nikula; +Cc: Alex Deucher

When switching to the new common i2c over aux code,
I ran into problems fetching the EDID on certain DP monitors.
I tracked this down to the lack of bare address packets being
sent between i2c transfers.  This patch set adds that functionality
which brings it back inline with the old drm dpm i2c algo.

Originally, I added flags to note a bare address packet, but after
review switched to sending a zero sized packet instead. Note that I'm
not sure the other drivers that use these helpers currently handle
zero sized packets properly.  I had to fix up radeon to do so.

Alex Deucher (3):
  drm/radeon/dp: handle zero sized i2c over aux transactions
  drm/dp/i2c: send bare addresses to properly reset i2c connections
  drm/radeon/dp: switch to the common i2c over aux code

 drivers/gpu/drm/drm_dp_helper.c            |  53 +++++++-----
 drivers/gpu/drm/radeon/atombios_dp.c       | 132 ++++++-----------------------
 drivers/gpu/drm/radeon/radeon_connectors.c |  44 ++--------
 drivers/gpu/drm/radeon/radeon_display.c    |  11 ++-
 drivers/gpu/drm/radeon/radeon_i2c.c        |  60 +++----------
 drivers/gpu/drm/radeon/radeon_mode.h       |  12 +--
 6 files changed, 85 insertions(+), 227 deletions(-)

-- 
1.8.3.1

^ permalink raw reply	[flat|nested] 6+ messages in thread
* [PATCH 1/3] drm/radeon/dp: handle zero sized i2c over aux transactions
  2014-04-04 14:40 [PATCH 0/3] Reset i2c connection between xfers Alex Deucher
@ 2014-04-04 14:40 ` Alex Deucher
  2014-04-04 14:40 ` [PATCH 2/3] drm/dp/i2c: send bare addresses to properly reset i2c connections Alex Deucher
  2014-04-04 14:40 ` [PATCH 3/3] drm/radeon/dp: switch to the common i2c over aux code Alex Deucher
  2 siblings, 0 replies; 6+ messages in thread
From: Alex Deucher @ 2014-04-04 14:40 UTC (permalink / raw)
  To: dri-devel, jani.nikula; +Cc: Alex Deucher

Needed for proper i2c over aux handling for certain
monitors and configurations (e.g., dp bridges or
adapters).

Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
---
 drivers/gpu/drm/radeon/atombios_dp.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/radeon/atombios_dp.c b/drivers/gpu/drm/radeon/atombios_dp.c
index 8b0ab17..e914008 100644
--- a/drivers/gpu/drm/radeon/atombios_dp.c
+++ b/drivers/gpu/drm/radeon/atombios_dp.c
@@ -143,6 +143,7 @@ static int radeon_process_aux_ch(struct radeon_i2c_chan *chan,
 }
 
 #define HEADER_SIZE 4
+#define BARE_ADDRESS_SIZE 3
 
 static ssize_t
 radeon_dp_aux_transfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
@@ -160,13 +161,16 @@ radeon_dp_aux_transfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
 	tx_buf[0] = msg->address & 0xff;
 	tx_buf[1] = msg->address >> 8;
 	tx_buf[2] = msg->request << 4;
-	tx_buf[3] = msg->size - 1;
+	tx_buf[3] = msg->size ? (msg->size - 1) : 0;
 
 	switch (msg->request & ~DP_AUX_I2C_MOT) {
 	case DP_AUX_NATIVE_WRITE:
 	case DP_AUX_I2C_WRITE:
 		tx_size = HEADER_SIZE + msg->size;
-		tx_buf[3] |= tx_size << 4;
+		if (msg->size == 0)
+			tx_buf[3] |= BARE_ADDRESS_SIZE << 4;
+		else
+			tx_buf[3] |= tx_size << 4;
 		memcpy(tx_buf + HEADER_SIZE, msg->buffer, msg->size);
 		ret = radeon_process_aux_ch(chan,
 					    tx_buf, tx_size, NULL, 0, delay, &ack);
@@ -177,7 +181,10 @@ radeon_dp_aux_transfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
 	case DP_AUX_NATIVE_READ:
 	case DP_AUX_I2C_READ:
 		tx_size = HEADER_SIZE;
-		tx_buf[3] |= tx_size << 4;
+		if (msg->size == 0)
+			tx_buf[3] |= BARE_ADDRESS_SIZE << 4;
+		else
+			tx_buf[3] |= tx_size << 4;
 		ret = radeon_process_aux_ch(chan,
 					    tx_buf, tx_size, msg->buffer, msg->size, delay, &ack);
 		break;
@@ -186,7 +193,7 @@ radeon_dp_aux_transfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
 		break;
 	}
 
-	if (ret > 0)
+	if (ret >= 0)
 		msg->reply = ack >> 4;
 
 	return ret;
-- 
1.8.3.1

^ permalink raw reply related	[flat|nested] 6+ messages in thread
* [PATCH 2/3] drm/dp/i2c: send bare addresses to properly reset i2c connections
  2014-04-04 14:40 [PATCH 0/3] Reset i2c connection between xfers Alex Deucher
  2014-04-04 14:40 ` [PATCH 1/3] drm/radeon/dp: handle zero sized i2c over aux transactions Alex Deucher
@ 2014-04-04 14:40 ` Alex Deucher
  2014-04-04 15:31   ` Ville Syrjälä
  2014-04-04 15:32   ` Daniel Vetter
  2014-04-04 14:40 ` [PATCH 3/3] drm/radeon/dp: switch to the common i2c over aux code Alex Deucher
  2 siblings, 2 replies; 6+ messages in thread
From: Alex Deucher @ 2014-04-04 14:40 UTC (permalink / raw)
  To: dri-devel, jani.nikula; +Cc: Alex Deucher

We need bare address packets at the start and end of
each i2c over aux transaction to properly reset the connection
between transactions.  This mirrors what the existing dp i2c
over aux algo currently does.

This fixes EDID fetches on certain monitors especially with
dp bridges.

Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
---
 drivers/gpu/drm/drm_dp_helper.c | 53 +++++++++++++++++++++++------------------
 1 file changed, 30 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index f4babed..f0c2850 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -664,12 +664,26 @@ static int drm_dp_i2c_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs,
 			   int num)
 {
 	struct drm_dp_aux *aux = adapter->algo_data;
-	unsigned int i, j;
+	unsigned int m, b;
+	struct drm_dp_aux_msg msg;
+	int err = 0;
+	u8 buf = 0;
 
-	for (i = 0; i < num; i++) {
-		struct drm_dp_aux_msg msg;
-		int err;
+	memset(&msg, 0, sizeof(msg));
 
+	for (m = 0; m < num; m++) {
+		msg.address = msgs[m].addr;
+		msg.request = (msgs[m].flags & I2C_M_RD) ?
+			DP_AUX_I2C_READ :
+			DP_AUX_I2C_WRITE;
+		msg.request |= DP_AUX_I2C_MOT;
+		msg.buffer = &buf;
+		msg.size = 0;
+		err = drm_dp_i2c_do_msg(aux, &msg);
+		if (err < 0) {
+			printk("error %d in bare address write\n", err);
+			break;
+		}
 		/*
 		 * Many hardware implementations support FIFOs larger than a
 		 * single byte, but it has been empirically determined that
@@ -677,31 +691,24 @@ static int drm_dp_i2c_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs,
 		 * decreased performance. Therefore each message is simply
 		 * transferred byte-by-byte.
 		 */
-		for (j = 0; j < msgs[i].len; j++) {
-			memset(&msg, 0, sizeof(msg));
-			msg.address = msgs[i].addr;
-
-			msg.request = (msgs[i].flags & I2C_M_RD) ?
-					DP_AUX_I2C_READ :
-					DP_AUX_I2C_WRITE;
-
-			/*
-			 * All messages except the last one are middle-of-
-			 * transfer messages.
-			 */
-			if ((i < num - 1) || (j < msgs[i].len - 1))
-				msg.request |= DP_AUX_I2C_MOT;
-
-			msg.buffer = msgs[i].buf + j;
+		for (b = 0; b < msgs[m].len; b++) {
+			msg.buffer = msgs[m].buf + b;
 			msg.size = 1;
 
 			err = drm_dp_i2c_do_msg(aux, &msg);
 			if (err < 0)
-				return err;
+				break;
 		}
 	}
-
-	return num;
+	if (err >= 0)
+		err = num;
+	/* send a bare address packet to close out the connection */
+	msg.request &= ~DP_AUX_I2C_MOT;
+	msg.buffer = &buf;
+	msg.size = 0;
+	(void)drm_dp_i2c_do_msg(aux, &msg);
+
+	return err;
 }
 
 static const struct i2c_algorithm drm_dp_i2c_algo = {
-- 
1.8.3.1

^ permalink raw reply related	[flat|nested] 6+ messages in thread
* [PATCH 3/3] drm/radeon/dp: switch to the common i2c over aux code
  2014-04-04 14:40 [PATCH 0/3] Reset i2c connection between xfers Alex Deucher
  2014-04-04 14:40 ` [PATCH 1/3] drm/radeon/dp: handle zero sized i2c over aux transactions Alex Deucher
  2014-04-04 14:40 ` [PATCH 2/3] drm/dp/i2c: send bare addresses to properly reset i2c connections Alex Deucher
@ 2014-04-04 14:40 ` Alex Deucher
  2 siblings, 0 replies; 6+ messages in thread
From: Alex Deucher @ 2014-04-04 14:40 UTC (permalink / raw)
  To: dri-devel, jani.nikula; +Cc: Alex Deucher

Provides a nice cleanup in radeon.

Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
---
 drivers/gpu/drm/radeon/atombios_dp.c       | 117 +++++------------------------
 drivers/gpu/drm/radeon/radeon_connectors.c |  44 ++---------
 drivers/gpu/drm/radeon/radeon_display.c    |  11 ++-
 drivers/gpu/drm/radeon/radeon_i2c.c        |  60 ++++-----------
 drivers/gpu/drm/radeon/radeon_mode.h       |  12 +--
 5 files changed, 44 insertions(+), 200 deletions(-)

diff --git a/drivers/gpu/drm/radeon/atombios_dp.c b/drivers/gpu/drm/radeon/atombios_dp.c
index e914008..d545769 100644
--- a/drivers/gpu/drm/radeon/atombios_dp.c
+++ b/drivers/gpu/drm/radeon/atombios_dp.c
@@ -201,98 +201,15 @@ radeon_dp_aux_transfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
 
 void radeon_dp_aux_init(struct radeon_connector *radeon_connector)
 {
-	struct radeon_connector_atom_dig *dig_connector = radeon_connector->con_priv;
-
-	dig_connector->dp_i2c_bus->aux.dev = radeon_connector->base.kdev;
-	dig_connector->dp_i2c_bus->aux.transfer = radeon_dp_aux_transfer;
-}
-
-int radeon_dp_i2c_aux_ch(struct i2c_adapter *adapter, int mode,
-			 u8 write_byte, u8 *read_byte)
-{
-	struct i2c_algo_dp_aux_data *algo_data = adapter->algo_data;
-	struct radeon_i2c_chan *auxch = i2c_get_adapdata(adapter);
-	u16 address = algo_data->address;
-	u8 msg[5];
-	u8 reply[2];
-	unsigned retry;
-	int msg_bytes;
-	int reply_bytes = 1;
 	int ret;
-	u8 ack;
-
-	/* Set up the address */
-	msg[0] = address;
-	msg[1] = address >> 8;
-
-	/* Set up the command byte */
-	if (mode & MODE_I2C_READ) {
-		msg[2] = DP_AUX_I2C_READ << 4;
-		msg_bytes = 4;
-		msg[3] = msg_bytes << 4;
-	} else {
-		msg[2] = DP_AUX_I2C_WRITE << 4;
-		msg_bytes = 5;
-		msg[3] = msg_bytes << 4;
-		msg[4] = write_byte;
-	}
 
-	/* special handling for start/stop */
-	if (mode & (MODE_I2C_START | MODE_I2C_STOP))
-		msg[3] = 3 << 4;
-
-	/* Set MOT bit for all but stop */
-	if ((mode & MODE_I2C_STOP) == 0)
-		msg[2] |= DP_AUX_I2C_MOT << 4;
-
-	for (retry = 0; retry < 7; retry++) {
-		ret = radeon_process_aux_ch(auxch,
-					    msg, msg_bytes, reply, reply_bytes, 0, &ack);
-		if (ret == -EBUSY)
-			continue;
-		else if (ret < 0) {
-			DRM_DEBUG_KMS("aux_ch failed %d\n", ret);
-			return ret;
-		}
+	radeon_connector->ddc_bus->aux.dev = radeon_connector->base.kdev;
+	radeon_connector->ddc_bus->aux.transfer = radeon_dp_aux_transfer;
+	ret = drm_dp_aux_register_i2c_bus(&radeon_connector->ddc_bus->aux);
+	if (!ret)
+		radeon_connector->ddc_bus->has_aux = true;
 
-		switch ((ack >> 4) & DP_AUX_NATIVE_REPLY_MASK) {
-		case DP_AUX_NATIVE_REPLY_ACK:
-			/* I2C-over-AUX Reply field is only valid
-			 * when paired with AUX ACK.
-			 */
-			break;
-		case DP_AUX_NATIVE_REPLY_NACK:
-			DRM_DEBUG_KMS("aux_ch native nack\n");
-			return -EREMOTEIO;
-		case DP_AUX_NATIVE_REPLY_DEFER:
-			DRM_DEBUG_KMS("aux_ch native defer\n");
-			usleep_range(500, 600);
-			continue;
-		default:
-			DRM_ERROR("aux_ch invalid native reply 0x%02x\n", ack);
-			return -EREMOTEIO;
-		}
-
-		switch ((ack >> 4) & DP_AUX_I2C_REPLY_MASK) {
-		case DP_AUX_I2C_REPLY_ACK:
-			if (mode == MODE_I2C_READ)
-				*read_byte = reply[0];
-			return ret;
-		case DP_AUX_I2C_REPLY_NACK:
-			DRM_DEBUG_KMS("aux_i2c nack\n");
-			return -EREMOTEIO;
-		case DP_AUX_I2C_REPLY_DEFER:
-			DRM_DEBUG_KMS("aux_i2c defer\n");
-			usleep_range(400, 500);
-			break;
-		default:
-			DRM_ERROR("aux_i2c invalid reply 0x%02x\n", ack);
-			return -EREMOTEIO;
-		}
-	}
-
-	DRM_DEBUG_KMS("aux i2c too many retries, giving up\n");
-	return -EREMOTEIO;
+	WARN(ret, "drm_dp_aux_register_i2c_bus() failed with error %d\n", ret);
 }
 
 /***** general DP utility functions *****/
@@ -427,12 +344,11 @@ static u8 radeon_dp_encoder_service(struct radeon_device *rdev,
 
 u8 radeon_dp_getsinktype(struct radeon_connector *radeon_connector)
 {
-	struct radeon_connector_atom_dig *dig_connector = radeon_connector->con_priv;
 	struct drm_device *dev = radeon_connector->base.dev;
 	struct radeon_device *rdev = dev->dev_private;
 
 	return radeon_dp_encoder_service(rdev, ATOM_DP_ACTION_GET_SINK_TYPE, 0,
-					 dig_connector->dp_i2c_bus->rec.i2c_id, 0);
+					 radeon_connector->ddc_bus->rec.i2c_id, 0);
 }
 
 static void radeon_dp_probe_oui(struct radeon_connector *radeon_connector)
@@ -443,11 +359,11 @@ static void radeon_dp_probe_oui(struct radeon_connector *radeon_connector)
 	if (!(dig_connector->dpcd[DP_DOWN_STREAM_PORT_COUNT] & DP_OUI_SUPPORT))
 		return;
 
-	if (drm_dp_dpcd_read(&dig_connector->dp_i2c_bus->aux, DP_SINK_OUI, buf, 3))
+	if (drm_dp_dpcd_read(&radeon_connector->ddc_bus->aux, DP_SINK_OUI, buf, 3))
 		DRM_DEBUG_KMS("Sink OUI: %02hx%02hx%02hx\n",
 			      buf[0], buf[1], buf[2]);
 
-	if (drm_dp_dpcd_read(&dig_connector->dp_i2c_bus->aux, DP_BRANCH_OUI, buf, 3))
+	if (drm_dp_dpcd_read(&radeon_connector->ddc_bus->aux, DP_BRANCH_OUI, buf, 3))
 		DRM_DEBUG_KMS("Branch OUI: %02hx%02hx%02hx\n",
 			      buf[0], buf[1], buf[2]);
 }
@@ -458,7 +374,7 @@ bool radeon_dp_getdpcd(struct radeon_connector *radeon_connector)
 	u8 msg[DP_DPCD_SIZE];
 	int ret, i;
 
-	ret = drm_dp_dpcd_read(&dig_connector->dp_i2c_bus->aux, DP_DPCD_REV, msg,
+	ret = drm_dp_dpcd_read(&radeon_connector->ddc_bus->aux, DP_DPCD_REV, msg,
 			       DP_DPCD_SIZE);
 	if (ret > 0) {
 		memcpy(dig_connector->dpcd, msg, DP_DPCD_SIZE);
@@ -496,7 +412,7 @@ int radeon_dp_get_panel_mode(struct drm_encoder *encoder,
 
 	if (dp_bridge != ENCODER_OBJECT_ID_NONE) {
 		/* DP bridge chips */
-		drm_dp_dpcd_readb(&dig_connector->dp_i2c_bus->aux,
+		drm_dp_dpcd_readb(&radeon_connector->ddc_bus->aux,
 				  DP_EDP_CONFIGURATION_CAP, &tmp);
 		if (tmp & 1)
 			panel_mode = DP_PANEL_MODE_INTERNAL_DP2_MODE;
@@ -507,7 +423,7 @@ int radeon_dp_get_panel_mode(struct drm_encoder *encoder,
 			panel_mode = DP_PANEL_MODE_EXTERNAL_DP_MODE;
 	} else if (connector->connector_type == DRM_MODE_CONNECTOR_eDP) {
 		/* eDP */
-		drm_dp_dpcd_readb(&dig_connector->dp_i2c_bus->aux,
+		drm_dp_dpcd_readb(&radeon_connector->ddc_bus->aux,
 				  DP_EDP_CONFIGURATION_CAP, &tmp);
 		if (tmp & 1)
 			panel_mode = DP_PANEL_MODE_INTERNAL_DP2_MODE;
@@ -561,7 +477,8 @@ bool radeon_dp_needs_link_train(struct radeon_connector *radeon_connector)
 	u8 link_status[DP_LINK_STATUS_SIZE];
 	struct radeon_connector_atom_dig *dig = radeon_connector->con_priv;
 
-	if (drm_dp_dpcd_read_link_status(&dig->dp_i2c_bus->aux, link_status) <= 0)
+	if (drm_dp_dpcd_read_link_status(&radeon_connector->ddc_bus->aux, link_status)
+	    <= 0)
 		return false;
 	if (drm_dp_channel_eq_ok(link_status, dig->dp_lane_count))
 		return false;
@@ -581,7 +498,7 @@ void radeon_dp_set_rx_power_state(struct drm_connector *connector,
 
 	/* power up/down the sink */
 	if (dig_connector->dpcd[0] >= 0x11) {
-		drm_dp_dpcd_writeb(&dig_connector->dp_i2c_bus->aux,
+		drm_dp_dpcd_writeb(&radeon_connector->ddc_bus->aux,
 				   DP_SET_POWER, power_state);
 		usleep_range(1000, 2000);
 	}
@@ -885,7 +802,7 @@ void radeon_dp_link_train(struct drm_encoder *encoder,
 	else
 		dp_info.enc_id |= ATOM_DP_CONFIG_LINK_A;
 
-	drm_dp_dpcd_readb(&dig_connector->dp_i2c_bus->aux, DP_MAX_LANE_COUNT, &tmp);
+	drm_dp_dpcd_readb(&radeon_connector->ddc_bus->aux, DP_MAX_LANE_COUNT, &tmp);
 	if (ASIC_IS_DCE5(rdev) && (tmp & DP_TPS3_SUPPORTED))
 		dp_info.tp3_supported = true;
 	else
@@ -897,7 +814,7 @@ void radeon_dp_link_train(struct drm_encoder *encoder,
 	dp_info.connector = connector;
 	dp_info.dp_lane_count = dig_connector->dp_lane_count;
 	dp_info.dp_clock = dig_connector->dp_clock;
-	dp_info.aux = &dig_connector->dp_i2c_bus->aux;
+	dp_info.aux = &radeon_connector->ddc_bus->aux;
 
 	if (radeon_dp_link_train_init(&dp_info))
 		goto done;
diff --git a/drivers/gpu/drm/radeon/radeon_connectors.c b/drivers/gpu/drm/radeon/radeon_connectors.c
index c566b48..ea50e0a 100644
--- a/drivers/gpu/drm/radeon/radeon_connectors.c
+++ b/drivers/gpu/drm/radeon/radeon_connectors.c
@@ -1261,21 +1261,6 @@ static const struct drm_connector_funcs radeon_dvi_connector_funcs = {
 	.force = radeon_dvi_force,
 };
 
-static void radeon_dp_connector_destroy(struct drm_connector *connector)
-{
-	struct radeon_connector *radeon_connector = to_radeon_connector(connector);
-	struct radeon_connector_atom_dig *radeon_dig_connector = radeon_connector->con_priv;
-
-	if (radeon_connector->edid)
-		kfree(radeon_connector->edid);
-	if (radeon_dig_connector->dp_i2c_bus)
-		radeon_i2c_destroy(radeon_dig_connector->dp_i2c_bus);
-	kfree(radeon_connector->con_priv);
-	drm_sysfs_connector_remove(connector);
-	drm_connector_cleanup(connector);
-	kfree(connector);
-}
-
 static int radeon_dp_get_modes(struct drm_connector *connector)
 {
 	struct radeon_connector *radeon_connector = to_radeon_connector(connector);
@@ -1553,7 +1538,7 @@ static const struct drm_connector_funcs radeon_dp_connector_funcs = {
 	.detect = radeon_dp_detect,
 	.fill_modes = drm_helper_probe_single_connector_modes,
 	.set_property = radeon_connector_set_property,
-	.destroy = radeon_dp_connector_destroy,
+	.destroy = radeon_connector_destroy,
 	.force = radeon_dvi_force,
 };
 
@@ -1562,7 +1547,7 @@ static const struct drm_connector_funcs radeon_edp_connector_funcs = {
 	.detect = radeon_dp_detect,
 	.fill_modes = drm_helper_probe_single_connector_modes,
 	.set_property = radeon_lvds_set_property,
-	.destroy = radeon_dp_connector_destroy,
+	.destroy = radeon_connector_destroy,
 	.force = radeon_dvi_force,
 };
 
@@ -1571,7 +1556,7 @@ static const struct drm_connector_funcs radeon_lvds_bridge_connector_funcs = {
 	.detect = radeon_dp_detect,
 	.fill_modes = drm_helper_probe_single_connector_modes,
 	.set_property = radeon_lvds_set_property,
-	.destroy = radeon_dp_connector_destroy,
+	.destroy = radeon_connector_destroy,
 	.force = radeon_dvi_force,
 };
 
@@ -1668,17 +1653,10 @@ radeon_add_atom_connector(struct drm_device *dev,
 		radeon_dig_connector->igp_lane_info = igp_lane_info;
 		radeon_connector->con_priv = radeon_dig_connector;
 		if (i2c_bus->valid) {
-			/* add DP i2c bus */
-			if (connector_type == DRM_MODE_CONNECTOR_eDP)
-				radeon_dig_connector->dp_i2c_bus = radeon_i2c_create_dp(dev, i2c_bus, "eDP-auxch");
-			else
-				radeon_dig_connector->dp_i2c_bus = radeon_i2c_create_dp(dev, i2c_bus, "DP-auxch");
-			if (radeon_dig_connector->dp_i2c_bus)
+			radeon_connector->ddc_bus = radeon_i2c_lookup(rdev, i2c_bus);
+			if (radeon_connector->ddc_bus)
 				has_aux = true;
 			else
-				DRM_ERROR("DP: Failed to assign dp ddc bus! Check dmesg for i2c errors.\n");
-			radeon_connector->ddc_bus = radeon_i2c_lookup(rdev, i2c_bus);
-			if (!radeon_connector->ddc_bus)
 				DRM_ERROR("DP: Failed to assign ddc bus! Check dmesg for i2c errors.\n");
 		}
 		switch (connector_type) {
@@ -1893,10 +1871,6 @@ radeon_add_atom_connector(struct drm_device *dev,
 			drm_connector_init(dev, &radeon_connector->base, &radeon_dp_connector_funcs, connector_type);
 			drm_connector_helper_add(&radeon_connector->base, &radeon_dp_connector_helper_funcs);
 			if (i2c_bus->valid) {
-				/* add DP i2c bus */
-				radeon_dig_connector->dp_i2c_bus = radeon_i2c_create_dp(dev, i2c_bus, "DP-auxch");
-				if (!radeon_dig_connector->dp_i2c_bus)
-					DRM_ERROR("DP: Failed to assign dp ddc bus! Check dmesg for i2c errors.\n");
 				radeon_connector->ddc_bus = radeon_i2c_lookup(rdev, i2c_bus);
 				if (radeon_connector->ddc_bus)
 					has_aux = true;
@@ -1942,14 +1916,10 @@ radeon_add_atom_connector(struct drm_device *dev,
 			drm_connector_init(dev, &radeon_connector->base, &radeon_edp_connector_funcs, connector_type);
 			drm_connector_helper_add(&radeon_connector->base, &radeon_dp_connector_helper_funcs);
 			if (i2c_bus->valid) {
-				/* add DP i2c bus */
-				radeon_dig_connector->dp_i2c_bus = radeon_i2c_create_dp(dev, i2c_bus, "eDP-auxch");
-				if (radeon_dig_connector->dp_i2c_bus)
+				radeon_connector->ddc_bus = radeon_i2c_lookup(rdev, i2c_bus);
+				if (radeon_connector->ddc_bus)
 					has_aux = true;
 				else
-					DRM_ERROR("DP: Failed to assign dp ddc bus! Check dmesg for i2c errors.\n");
-				radeon_connector->ddc_bus = radeon_i2c_lookup(rdev, i2c_bus);
-				if (!radeon_connector->ddc_bus)
 					DRM_ERROR("DP: Failed to assign ddc bus! Check dmesg for i2c errors.\n");
 			}
 			drm_object_attach_property(&radeon_connector->base.base,
diff --git a/drivers/gpu/drm/radeon/radeon_display.c b/drivers/gpu/drm/radeon/radeon_display.c
index 5701fbb..d1af60a 100644
--- a/drivers/gpu/drm/radeon/radeon_display.c
+++ b/drivers/gpu/drm/radeon/radeon_display.c
@@ -757,19 +757,18 @@ int radeon_ddc_get_modes(struct radeon_connector *radeon_connector)
 
 	if (radeon_connector_encoder_get_dp_bridge_encoder_id(&radeon_connector->base) !=
 	    ENCODER_OBJECT_ID_NONE) {
-		struct radeon_connector_atom_dig *dig = radeon_connector->con_priv;
-
-		if (dig->dp_i2c_bus)
+		if (radeon_connector->ddc_bus->has_aux)
 			radeon_connector->edid = drm_get_edid(&radeon_connector->base,
-							      &dig->dp_i2c_bus->adapter);
+							      &radeon_connector->ddc_bus->aux.ddc);
 	} else if ((radeon_connector->base.connector_type == DRM_MODE_CONNECTOR_DisplayPort) ||
 		   (radeon_connector->base.connector_type == DRM_MODE_CONNECTOR_eDP)) {
 		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)
+		     dig->dp_sink_type == CONNECTOR_OBJECT_ID_eDP) &&
+		    radeon_connector->ddc_bus->has_aux)
 			radeon_connector->edid = drm_get_edid(&radeon_connector->base,
-							      &dig->dp_i2c_bus->adapter);
+							      &radeon_connector->ddc_bus->aux.ddc);
 		else if (radeon_connector->ddc_bus && !radeon_connector->edid)
 			radeon_connector->edid = drm_get_edid(&radeon_connector->base,
 							      &radeon_connector->ddc_bus->adapter);
diff --git a/drivers/gpu/drm/radeon/radeon_i2c.c b/drivers/gpu/drm/radeon/radeon_i2c.c
index e24ca6a..7b94414 100644
--- a/drivers/gpu/drm/radeon/radeon_i2c.c
+++ b/drivers/gpu/drm/radeon/radeon_i2c.c
@@ -64,8 +64,7 @@ bool radeon_ddc_probe(struct radeon_connector *radeon_connector, bool use_aux)
 		radeon_router_select_ddc_port(radeon_connector);
 
 	if (use_aux) {
-		struct radeon_connector_atom_dig *dig = radeon_connector->con_priv;
-		ret = i2c_transfer(&dig->dp_i2c_bus->adapter, msgs, 2);
+		ret = i2c_transfer(&radeon_connector->ddc_bus->aux.ddc, msgs, 2);
 	} else {
 		ret = i2c_transfer(&radeon_connector->ddc_bus->adapter, msgs, 2);
 	}
@@ -950,16 +949,16 @@ struct radeon_i2c_chan *radeon_i2c_create(struct drm_device *dev,
 		/* set the radeon bit adapter */
 		snprintf(i2c->adapter.name, sizeof(i2c->adapter.name),
 			 "Radeon i2c bit bus %s", name);
-		i2c->adapter.algo_data = &i2c->algo.bit;
-		i2c->algo.bit.pre_xfer = pre_xfer;
-		i2c->algo.bit.post_xfer = post_xfer;
-		i2c->algo.bit.setsda = set_data;
-		i2c->algo.bit.setscl = set_clock;
-		i2c->algo.bit.getsda = get_data;
-		i2c->algo.bit.getscl = get_clock;
-		i2c->algo.bit.udelay = 10;
-		i2c->algo.bit.timeout = usecs_to_jiffies(2200);	/* from VESA */
-		i2c->algo.bit.data = i2c;
+		i2c->adapter.algo_data = &i2c->bit;
+		i2c->bit.pre_xfer = pre_xfer;
+		i2c->bit.post_xfer = post_xfer;
+		i2c->bit.setsda = set_data;
+		i2c->bit.setscl = set_clock;
+		i2c->bit.getsda = get_data;
+		i2c->bit.getscl = get_clock;
+		i2c->bit.udelay = 10;
+		i2c->bit.timeout = usecs_to_jiffies(2200);	/* from VESA */
+		i2c->bit.data = i2c;
 		ret = i2c_bit_add_bus(&i2c->adapter);
 		if (ret) {
 			DRM_ERROR("Failed to register bit i2c %s\n", name);
@@ -974,46 +973,13 @@ out_free:
 
 }
 
-struct radeon_i2c_chan *radeon_i2c_create_dp(struct drm_device *dev,
-					     struct radeon_i2c_bus_rec *rec,
-					     const char *name)
-{
-	struct radeon_i2c_chan *i2c;
-	int ret;
-
-	i2c = kzalloc(sizeof(struct radeon_i2c_chan), GFP_KERNEL);
-	if (i2c == NULL)
-		return NULL;
-
-	i2c->rec = *rec;
-	i2c->adapter.owner = THIS_MODULE;
-	i2c->adapter.class = I2C_CLASS_DDC;
-	i2c->adapter.dev.parent = &dev->pdev->dev;
-	i2c->dev = dev;
-	snprintf(i2c->adapter.name, sizeof(i2c->adapter.name),
-		 "Radeon aux bus %s", name);
-	i2c_set_adapdata(&i2c->adapter, i2c);
-	i2c->adapter.algo_data = &i2c->algo.dp;
-	i2c->algo.dp.aux_ch = radeon_dp_i2c_aux_ch;
-	i2c->algo.dp.address = 0;
-	ret = i2c_dp_aux_add_bus(&i2c->adapter);
-	if (ret) {
-		DRM_INFO("Failed to register i2c %s\n", name);
-		goto out_free;
-	}
-
-	return i2c;
-out_free:
-	kfree(i2c);
-	return NULL;
-
-}
-
 void radeon_i2c_destroy(struct radeon_i2c_chan *i2c)
 {
 	if (!i2c)
 		return;
 	i2c_del_adapter(&i2c->adapter);
+	if (i2c->has_aux)
+		drm_dp_aux_unregister_i2c_bus(&i2c->aux);
 	kfree(i2c);
 }
 
diff --git a/drivers/gpu/drm/radeon/radeon_mode.h b/drivers/gpu/drm/radeon/radeon_mode.h
index 832d9fa..6ddf31a 100644
--- a/drivers/gpu/drm/radeon/radeon_mode.h
+++ b/drivers/gpu/drm/radeon/radeon_mode.h
@@ -187,12 +187,10 @@ struct radeon_pll {
 struct radeon_i2c_chan {
 	struct i2c_adapter adapter;
 	struct drm_device *dev;
-	union {
-		struct i2c_algo_bit_data bit;
-		struct i2c_algo_dp_aux_data dp;
-	} algo;
+	struct i2c_algo_bit_data bit;
 	struct radeon_i2c_bus_rec rec;
 	struct drm_dp_aux aux;
+	bool has_aux;
 };
 
 /* mostly for macs, but really any system without connector tables */
@@ -440,7 +438,6 @@ struct radeon_encoder {
 struct radeon_connector_atom_dig {
 	uint32_t igp_lane_info;
 	/* displayport */
-	struct radeon_i2c_chan *dp_i2c_bus;
 	u8 dpcd[DP_RECEIVER_CAP_SIZE];
 	u8 dp_sink_type;
 	int dp_clock;
@@ -702,8 +699,6 @@ extern void atombios_dig_transmitter_setup(struct drm_encoder *encoder,
 					   uint8_t lane_set);
 extern void radeon_atom_ext_encoder_setup_ddc(struct drm_encoder *encoder);
 extern struct drm_encoder *radeon_get_external_encoder(struct drm_encoder *encoder);
-extern int radeon_dp_i2c_aux_ch(struct i2c_adapter *adapter, int mode,
-				u8 write_byte, u8 *read_byte);
 void radeon_atom_copy_swap(u8 *dst, u8 *src, u8 num_bytes, bool to_le);
 
 extern void radeon_i2c_init(struct radeon_device *rdev);
@@ -715,9 +710,6 @@ extern void radeon_i2c_add(struct radeon_device *rdev,
 			   const char *name);
 extern struct radeon_i2c_chan *radeon_i2c_lookup(struct radeon_device *rdev,
 						 struct radeon_i2c_bus_rec *i2c_bus);
-extern struct radeon_i2c_chan *radeon_i2c_create_dp(struct drm_device *dev,
-						    struct radeon_i2c_bus_rec *rec,
-						    const char *name);
 extern struct radeon_i2c_chan *radeon_i2c_create(struct drm_device *dev,
 						 struct radeon_i2c_bus_rec *rec,
 						 const char *name);
-- 
1.8.3.1

^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH 2/3] drm/dp/i2c: send bare addresses to properly reset i2c connections
  2014-04-04 14:40 ` [PATCH 2/3] drm/dp/i2c: send bare addresses to properly reset i2c connections Alex Deucher
@ 2014-04-04 15:31   ` Ville Syrjälä
  2014-04-04 15:32   ` Daniel Vetter
  1 sibling, 0 replies; 6+ messages in thread
From: Ville Syrjälä @ 2014-04-04 15:31 UTC (permalink / raw)
  To: Alex Deucher; +Cc: Alex Deucher, dri-devel

On Fri, Apr 04, 2014 at 10:40:57AM -0400, Alex Deucher wrote:
> We need bare address packets at the start and end of
> each i2c over aux transaction to properly reset the connection
> between transactions.  This mirrors what the existing dp i2c
> over aux algo currently does.
> 
> This fixes EDID fetches on certain monitors especially with
> dp bridges.
> 
> Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
> ---
>  drivers/gpu/drm/drm_dp_helper.c | 53 +++++++++++++++++++++++------------------
>  1 file changed, 30 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
> index f4babed..f0c2850 100644
> --- a/drivers/gpu/drm/drm_dp_helper.c
> +++ b/drivers/gpu/drm/drm_dp_helper.c
> @@ -664,12 +664,26 @@ static int drm_dp_i2c_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs,
>  			   int num)
>  {
>  	struct drm_dp_aux *aux = adapter->algo_data;
> -	unsigned int i, j;
> +	unsigned int m, b;
> +	struct drm_dp_aux_msg msg;
> +	int err = 0;
> +	u8 buf = 0;
>  
> -	for (i = 0; i < num; i++) {
> -		struct drm_dp_aux_msg msg;
> -		int err;
> +	memset(&msg, 0, sizeof(msg));
>  
> +	for (m = 0; m < num; m++) {
> +		msg.address = msgs[m].addr;
> +		msg.request = (msgs[m].flags & I2C_M_RD) ?
> +			DP_AUX_I2C_READ :
> +			DP_AUX_I2C_WRITE;
> +		msg.request |= DP_AUX_I2C_MOT;
> +		msg.buffer = &buf;

Maybe just pass NULL and let buggy implementations explode?

> +		msg.size = 0;
> +		err = drm_dp_i2c_do_msg(aux, &msg);
> +		if (err < 0) {
> +			printk("error %d in bare address write\n", err);
> +			break;
> +		}
>  		/*
>  		 * Many hardware implementations support FIFOs larger than a
>  		 * single byte, but it has been empirically determined that
> @@ -677,31 +691,24 @@ static int drm_dp_i2c_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs,
>  		 * decreased performance. Therefore each message is simply
>  		 * transferred byte-by-byte.
>  		 */
> -		for (j = 0; j < msgs[i].len; j++) {
> -			memset(&msg, 0, sizeof(msg));
> -			msg.address = msgs[i].addr;
> -
> -			msg.request = (msgs[i].flags & I2C_M_RD) ?
> -					DP_AUX_I2C_READ :
> -					DP_AUX_I2C_WRITE;
> -
> -			/*
> -			 * All messages except the last one are middle-of-
> -			 * transfer messages.
> -			 */
> -			if ((i < num - 1) || (j < msgs[i].len - 1))
> -				msg.request |= DP_AUX_I2C_MOT;
> -
> -			msg.buffer = msgs[i].buf + j;
> +		for (b = 0; b < msgs[m].len; b++) {
> +			msg.buffer = msgs[m].buf + b;
>  			msg.size = 1;
>  
>  			err = drm_dp_i2c_do_msg(aux, &msg);
>  			if (err < 0)
> -				return err;
> +				break;

This will abort the current message, but it'll keep going and try to
tranfer the next message. We need to abort the entire thing and proceed
to generate the i2c STOP.

Also you're now reusing the msg and expecting .transfer() to not clobber
any of the fields (apart from msg.reply). Hmm, actually the retry loop
in drm_dp_i2c_do_msg() already requires such a contract between the
caller and the callee. As we can't make the msg const due to msg.reply,
it would be nice if the documentation mentioned this somewhat important
detail.

>  		}
>  	}
> -
> -	return num;
> +	if (err >= 0)
> +		err = num;
> +	/* send a bare address packet to close out the connection */
> +	msg.request &= ~DP_AUX_I2C_MOT;
> +	msg.buffer = &buf;

Another chance to make buggy drivers explode ;)

> +	msg.size = 0;
> +	(void)drm_dp_i2c_do_msg(aux, &msg);
> +
> +	return err;
>  }
>  
>  static const struct i2c_algorithm drm_dp_i2c_algo = {
> -- 
> 1.8.3.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel OTC

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH 2/3] drm/dp/i2c: send bare addresses to properly reset i2c connections
  2014-04-04 14:40 ` [PATCH 2/3] drm/dp/i2c: send bare addresses to properly reset i2c connections Alex Deucher
  2014-04-04 15:31   ` Ville Syrjälä
@ 2014-04-04 15:32   ` Daniel Vetter
  1 sibling, 0 replies; 6+ messages in thread
From: Daniel Vetter @ 2014-04-04 15:32 UTC (permalink / raw)
  To: Alex Deucher; +Cc: Alex Deucher, dri-devel

On Fri, Apr 04, 2014 at 10:40:57AM -0400, Alex Deucher wrote:
> We need bare address packets at the start and end of
> each i2c over aux transaction to properly reset the connection
> between transactions.  This mirrors what the existing dp i2c
> over aux algo currently does.
> 
> This fixes EDID fetches on certain monitors especially with
> dp bridges.
> 
> Signed-off-by: Alex Deucher <alexander.deucher@amd.com>

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

The i915 dp aux code will work the same way still, but curiously the hw
spec doesn't mention anything for this really. I didn't check tegra.
-Daniel

> ---
>  drivers/gpu/drm/drm_dp_helper.c | 53 +++++++++++++++++++++++------------------
>  1 file changed, 30 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
> index f4babed..f0c2850 100644
> --- a/drivers/gpu/drm/drm_dp_helper.c
> +++ b/drivers/gpu/drm/drm_dp_helper.c
> @@ -664,12 +664,26 @@ static int drm_dp_i2c_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs,
>  			   int num)
>  {
>  	struct drm_dp_aux *aux = adapter->algo_data;
> -	unsigned int i, j;
> +	unsigned int m, b;
> +	struct drm_dp_aux_msg msg;
> +	int err = 0;
> +	u8 buf = 0;
>  
> -	for (i = 0; i < num; i++) {
> -		struct drm_dp_aux_msg msg;
> -		int err;
> +	memset(&msg, 0, sizeof(msg));
>  
> +	for (m = 0; m < num; m++) {
> +		msg.address = msgs[m].addr;
> +		msg.request = (msgs[m].flags & I2C_M_RD) ?
> +			DP_AUX_I2C_READ :
> +			DP_AUX_I2C_WRITE;
> +		msg.request |= DP_AUX_I2C_MOT;
> +		msg.buffer = &buf;
> +		msg.size = 0;
> +		err = drm_dp_i2c_do_msg(aux, &msg);
> +		if (err < 0) {
> +			printk("error %d in bare address write\n", err);
> +			break;
> +		}
>  		/*
>  		 * Many hardware implementations support FIFOs larger than a
>  		 * single byte, but it has been empirically determined that
> @@ -677,31 +691,24 @@ static int drm_dp_i2c_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs,
>  		 * decreased performance. Therefore each message is simply
>  		 * transferred byte-by-byte.
>  		 */
> -		for (j = 0; j < msgs[i].len; j++) {
> -			memset(&msg, 0, sizeof(msg));
> -			msg.address = msgs[i].addr;
> -
> -			msg.request = (msgs[i].flags & I2C_M_RD) ?
> -					DP_AUX_I2C_READ :
> -					DP_AUX_I2C_WRITE;
> -
> -			/*
> -			 * All messages except the last one are middle-of-
> -			 * transfer messages.
> -			 */
> -			if ((i < num - 1) || (j < msgs[i].len - 1))
> -				msg.request |= DP_AUX_I2C_MOT;
> -
> -			msg.buffer = msgs[i].buf + j;
> +		for (b = 0; b < msgs[m].len; b++) {
> +			msg.buffer = msgs[m].buf + b;
>  			msg.size = 1;
>  
>  			err = drm_dp_i2c_do_msg(aux, &msg);
>  			if (err < 0)
> -				return err;
> +				break;
>  		}
>  	}
> -
> -	return num;
> +	if (err >= 0)
> +		err = num;
> +	/* send a bare address packet to close out the connection */
> +	msg.request &= ~DP_AUX_I2C_MOT;
> +	msg.buffer = &buf;
> +	msg.size = 0;
> +	(void)drm_dp_i2c_do_msg(aux, &msg);
> +
> +	return err;
>  }
>  
>  static const struct i2c_algorithm drm_dp_i2c_algo = {
> -- 
> 1.8.3.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-04-04 15:32 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-04-04 14:40 [PATCH 0/3] Reset i2c connection between xfers Alex Deucher
2014-04-04 14:40 ` [PATCH 1/3] drm/radeon/dp: handle zero sized i2c over aux transactions Alex Deucher
2014-04-04 14:40 ` [PATCH 2/3] drm/dp/i2c: send bare addresses to properly reset i2c connections Alex Deucher
2014-04-04 15:31   ` Ville Syrjälä
2014-04-04 15:32   ` Daniel Vetter
2014-04-04 14:40 ` [PATCH 3/3] drm/radeon/dp: switch to the common i2c over aux code Alex Deucher

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.