[PATCH 0/4] Reset i2c connection between xfers (v3)

[PATCH 0/4] Reset i2c connection between xfers (v3)

[Alex Deucher]

V3 just drops a left over debug statement.

Alex Deucher (4):
  drm/radeon/dp: handle zero sized i2c over aux transactions
  drm/dp/i2c: send bare addresses to properly reset i2c connections (v3)
  drm/dp/i2c: Update comments about common i2c over dp assumptions
  drm/radeon/dp: switch to the common i2c over aux code

 drivers/gpu/drm/drm_dp_helper.c            |  56 ++++++------
 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, 87 insertions(+), 228 deletions(-)

-- 
1.8.3.1

[PATCH 1/4] drm/radeon/dp: handle zero sized i2c over aux transactions

[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

[PATCH 2/4] drm/dp/i2c: send bare addresses to properly reset i2c connections (v3)

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

v2: update as per Ville's comments
    - Set buffer to NULL for zero sized packets
    - abort the entre transaction if one of the messages fails
v3: drop leftover debugging code

Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Jani Nikula <jani.nikula@intel.com>
Cc: Thierry Reding <treding@nvidia.com>
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_dp_helper.c | 52 +++++++++++++++++++++++------------------
 1 file changed, 29 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index 74724aa..dfe4cf4 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -664,12 +664,23 @@ 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;
 
-	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 = NULL;
+		msg.size = 0;
+		err = drm_dp_i2c_do_msg(aux, &msg);
+		if (err < 0)
+			break;
 		/*
 		 * Many hardware implementations support FIFOs larger than a
 		 * single byte, but it has been empirically determined that
@@ -677,31 +688,26 @@ 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;
 		}
+		if (err < 0)
+			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 = NULL;
+	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

[PATCH 3/4] drm/dp/i2c: Update comments about common i2c over dp assumptions

[Alex Deucher]

If you are using the common dp over i2c functionality, it is
asumed that the aux transfer function does not modify the any
of the msg structure other than the reply field.  Doing so
breaks the logic in the common code.

Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Jani Nikula <jani.nikula@intel.com>
Cc: Thierry Reding <treding@nvidia.com>
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_dp_helper.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index dfe4cf4..948de4f 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -577,7 +577,9 @@ static u32 drm_dp_i2c_functionality(struct i2c_adapter *adapter)
 
 /*
  * Transfer a single I2C-over-AUX message and handle various error conditions,
- * retrying the transaction as appropriate.
+ * retrying the transaction as appropriate.  It is assumed that the
+ * aux->transfer function does not modify anything in the msg other than the
+ * reply field.
  */
 static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
 {
-- 
1.8.3.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 4/4] drm/radeon/dp: switch to the common i2c over aux code

[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 ec958e86..e1388cd 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 fbd8b93..04bf503 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

Re: [PATCH 1/4] drm/radeon/dp: handle zero sized i2c over aux transactions

[Christian König]

Am 04.04.2014 21:58, schrieb 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>

My planning was that yesterdays pull request is the last one with new 
features. I can't judge how invasive this series is, so should I add it 
to my 3.15 branch and send Dave another pull request or should we wait 
for 3.16?

Christian.

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

Re: [PATCH 1/4] drm/radeon/dp: handle zero sized i2c over aux transactions

[Daniel Vetter]

On Sat, Apr 05, 2014 at 11:25:01AM +0200, Christian König wrote:
> Am 04.04.2014 21:58, schrieb 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>
> 
> My planning was that yesterdays pull request is the last one with
> new features. I can't judge how invasive this series is, so should I
> add it to my 3.15 branch and send Dave another pull request or
> should we wait for 3.16?

At least the two patches for the dp aux helper code should go into 3.15
since they fix a behavioral regression in i915, too.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 1/4] drm/radeon/dp: handle zero sized i2c over aux transactions

[Alex Deucher]

On Sat, Apr 5, 2014 at 5:25 AM, Christian König <deathsimple@vodafone.de> wrote:
> Am 04.04.2014 21:58, schrieb 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>
>
>
> My planning was that yesterdays pull request is the last one with new
> features. I can't judge how invasive this series is, so should I add it to
> my 3.15 branch and send Dave another pull request or should we wait for
> 3.16?

I'd like to get them into 3.15.  They provide a really nice cleanup
for the radeon dp i2c handling.

Alex

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

Re: [PATCH 2/4] drm/dp/i2c: send bare addresses to properly reset i2c connections (v3)

[Thierry Reding]

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

On Fri, Apr 04, 2014 at 03:58:37PM -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.
> 
> v2: update as per Ville's comments
>     - Set buffer to NULL for zero sized packets
>     - abort the entre transaction if one of the messages fails
> v3: drop leftover debugging code
> 
> Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Cc: Thierry Reding <treding@nvidia.com>
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_dp_helper.c | 52 +++++++++++++++++++++++------------------
>  1 file changed, 29 insertions(+), 23 deletions(-)

Can we please document that zero-sized messages specify address-only
transactions? Perhaps it would also be useful to mention that these can
only happen for I2C-over-AUX messages.

> diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
> index 74724aa..dfe4cf4 100644
> --- a/drivers/gpu/drm/drm_dp_helper.c
> +++ b/drivers/gpu/drm/drm_dp_helper.c
> @@ -664,12 +664,23 @@ 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;

I don't see why these would need to be changed. i and j are perfectly
fine loop variable names.

> -	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 = NULL;
> +		msg.size = 0;
> +		err = drm_dp_i2c_do_msg(aux, &msg);
> +		if (err < 0)
> +			break;

This seems somewhat brittle to me. Even though I notice that patch 3/4
updates a comment that documents these assumptions, I don't see a reason
for these assumptions in the first place.

I'd prefer if we simply provided the complete message rather than rely
on drivers not to touch anything but the reply field.

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --]

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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/4] drm/radeon/dp: handle zero sized i2c over aux transactions

[Jani Nikula]

On Fri, 04 Apr 2014, Alex Deucher <alexdeucher@gmail.com> wrote:
> 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);

I think your tz_size and tx_buf[3] are confused. Shouldn't you only send
3 bytes of tx_buf when msg->size == 0?

Disclaimer, I don't know anything about your hw and all that... :)

BR,
Jani.


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

-- 
Jani Nikula, Intel Open Source Technology Center

Re: [PATCH 2/4] drm/dp/i2c: send bare addresses to properly reset i2c connections (v3)

[Thierry Reding]

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

On Fri, Apr 04, 2014 at 03:58:37PM -0400, Alex Deucher wrote:
[...]
> diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
[...]
> +	/* send a bare address packet to close out the connection */

Missed this one earlier. Perhaps s/connection/transaction/?

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --]

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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/4] drm/radeon/dp: handle zero sized i2c over aux transactions

[Alex Deucher]

On Mon, Apr 7, 2014 at 3:57 AM, Jani Nikula <jani.nikula@linux.intel.com> wrote:
> On Fri, 04 Apr 2014, Alex Deucher <alexdeucher@gmail.com> wrote:
>> 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);
>
> I think your tz_size and tx_buf[3] are confused. Shouldn't you only send
> 3 bytes of tx_buf when msg->size == 0?
>
> Disclaimer, I don't know anything about your hw and all that... :)

It doesn't really matter.  The hw only cares about the size specified
in tx_buf[3]. tx_size is only used to determine how much data to the
buffer used by the atom table.  We end up copying an extra byte that
never gets used.  I suppose I should fix it up for clarify.

Alex

>
> BR,
> Jani.
>
>
>> @@ -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
>>
>
> --
> Jani Nikula, Intel Open Source Technology Center

Re: [PATCH 1/4] drm/radeon/dp: handle zero sized i2c over aux transactions

[Alex Deucher]

On Mon, Apr 7, 2014 at 9:24 AM, Alex Deucher <alexdeucher@gmail.com> wrote:
> On Mon, Apr 7, 2014 at 3:57 AM, Jani Nikula <jani.nikula@linux.intel.com> wrote:
>> On Fri, 04 Apr 2014, Alex Deucher <alexdeucher@gmail.com> wrote:
>>> 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);
>>
>> I think your tz_size and tx_buf[3] are confused. Shouldn't you only send
>> 3 bytes of tx_buf when msg->size == 0?
>>
>> Disclaimer, I don't know anything about your hw and all that... :)
>
> It doesn't really matter.  The hw only cares about the size specified
> in tx_buf[3]. tx_size is only used to determine how much data to the
> buffer used by the atom table.  We end up copying an extra byte that
> never gets used.  I suppose I should fix it up for clarify.

Actually, I was wrong.  We need all 4 bytes of the tx_buf, but the hw
only sends 3 based on the size specified in tx_buf[3]. so the code is
correct as is.

Alex

>
> Alex
>
>>
>> BR,
>> Jani.
>>
>>
>>> @@ -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
>>>
>>
>> --
>> Jani Nikula, Intel Open Source Technology Center

Re: [PATCH 2/4] drm/dp/i2c: send bare addresses to properly reset i2c connections (v3)

[Alex Deucher]

On Mon, Apr 7, 2014 at 3:49 AM, Thierry Reding <thierry.reding@gmail.com> wrote:
> On Fri, Apr 04, 2014 at 03:58:37PM -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.
>>
>> v2: update as per Ville's comments
>>     - Set buffer to NULL for zero sized packets
>>     - abort the entre transaction if one of the messages fails
>> v3: drop leftover debugging code
>>
>> Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> Cc: Jani Nikula <jani.nikula@intel.com>
>> Cc: Thierry Reding <treding@nvidia.com>
>> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> ---
>>  drivers/gpu/drm/drm_dp_helper.c | 52 +++++++++++++++++++++++------------------
>>  1 file changed, 29 insertions(+), 23 deletions(-)
>
> Can we please document that zero-sized messages specify address-only
> transactions? Perhaps it would also be useful to mention that these can
> only happen for I2C-over-AUX messages.

Can do.  I don't know of any uses for bare address packets with
regular aux off hand, but there may be cases I'm not familiar with.
Does anyone know of any use for a bare address packet with regular
aux?

>
>> diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
>> index 74724aa..dfe4cf4 100644
>> --- a/drivers/gpu/drm/drm_dp_helper.c
>> +++ b/drivers/gpu/drm/drm_dp_helper.c
>> @@ -664,12 +664,23 @@ 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;
>
> I don't see why these would need to be changed. i and j are perfectly
> fine loop variable names.

It was easier for me to follow the code since the variables matched
the objects they were iterating, but I can change them back if you'd
prefer.

>
>> -     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 = NULL;
>> +             msg.size = 0;
>> +             err = drm_dp_i2c_do_msg(aux, &msg);
>> +             if (err < 0)
>> +                     break;
>
> This seems somewhat brittle to me. Even though I notice that patch 3/4
> updates a comment that documents these assumptions, I don't see a reason
> for these assumptions in the first place.

We already assume that in drm_dp_i2c_do_msg() for the retry loop.

>
> I'd prefer if we simply provided the complete message rather than rely
> on drivers not to touch anything but the reply field.

Are you suggesting we re-write drm_dp_i2c_do_msg() or move the retry
logic into drm_dp_i2c_xfer()?  Do you mind if we do that in a follow
up patch so we can keep it separate from the addition of the bare
address packets?


Alex

Re: [PATCH 2/4] drm/dp/i2c: send bare addresses to properly reset i2c connections (v3)

[Thierry Reding]

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

On Mon, Apr 07, 2014 at 09:44:06AM -0400, Alex Deucher wrote:
> On Mon, Apr 7, 2014 at 3:49 AM, Thierry Reding <thierry.reding@gmail.com> wrote:
> > On Fri, Apr 04, 2014 at 03:58:37PM -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.
> >>
> >> v2: update as per Ville's comments
> >>     - Set buffer to NULL for zero sized packets
> >>     - abort the entre transaction if one of the messages fails
> >> v3: drop leftover debugging code
> >>
> >> Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
> >> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >> Cc: Jani Nikula <jani.nikula@intel.com>
> >> Cc: Thierry Reding <treding@nvidia.com>
> >> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >> ---
> >>  drivers/gpu/drm/drm_dp_helper.c | 52 +++++++++++++++++++++++------------------
> >>  1 file changed, 29 insertions(+), 23 deletions(-)
> >
> > Can we please document that zero-sized messages specify address-only
> > transactions? Perhaps it would also be useful to mention that these can
> > only happen for I2C-over-AUX messages.
> 
> Can do.  I don't know of any uses for bare address packets with
> regular aux off hand, but there may be cases I'm not familiar with.
> Does anyone know of any use for a bare address packet with regular
> aux?
> 
> >
> >> diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
> >> index 74724aa..dfe4cf4 100644
> >> --- a/drivers/gpu/drm/drm_dp_helper.c
> >> +++ b/drivers/gpu/drm/drm_dp_helper.c
> >> @@ -664,12 +664,23 @@ 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;
> >
> > I don't see why these would need to be changed. i and j are perfectly
> > fine loop variable names.
> 
> It was easier for me to follow the code since the variables matched
> the objects they were iterating, but I can change them back if you'd
> prefer.

I think there's enough context in the surrounding code to make it
obvious what they are used for. I also think that keeping the names
makes the diff easier to follow.

But if you feel really strongly about keeping m and b I can live with
it.

> >> -     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 = NULL;
> >> +             msg.size = 0;
> >> +             err = drm_dp_i2c_do_msg(aux, &msg);
> >> +             if (err < 0)
> >> +                     break;
> >
> > This seems somewhat brittle to me. Even though I notice that patch 3/4
> > updates a comment that documents these assumptions, I don't see a reason
> > for these assumptions in the first place.
> 
> We already assume that in drm_dp_i2c_do_msg() for the retry loop.

Ugh... you're right.

> > I'd prefer if we simply provided the complete message rather than rely
> > on drivers not to touch anything but the reply field.
> 
> Are you suggesting we re-write drm_dp_i2c_do_msg() or move the retry
> logic into drm_dp_i2c_xfer()?  Do you mind if we do that in a follow
> up patch so we can keep it separate from the addition of the bare
> address packets?

No, looking at the code again, I don't see how we can do much better
without sacrificing much of the clarity of the code.

But perhaps this could be documented somewhere else than the I2C
helpers. Perhaps the struct drm_dp_aux comment should be updated, which
would cause the DRM docbook to automatically pick up that change as
well.

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --]

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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel