drm/i915/dp: native aux read return value fixes and cleanup

PATCH Superseded Jani Nikula 3 participants 12 messages Jan 30, 2014

Subsystems (2)

DRM drivers Intel i915

lore

Share & tools

lore

Jani Nikula

author

2014-01-30 09:37 UTC #0 lore raw

cover drm/i915/dp: native aux read return value fixes and cleanup

Fix and cleanup the rather confused native aux read return values and return value checks. Maybe we'll get them right from now on. Just maybe.

Did not have a chance to test this, I'm afraid, but it's pretty straightforward.

BR,
Jani.

Jani Nikula (4):
drm/i915/dp: make intel_dp_aux_native_read() return bool drm/i915/dp: move intel_dp_aux_native_read_retry() near other reads drm/i915/dp: clean up cargo culted intel_dp_aux_native_read_retry()
usage
drm/i915/dp: fix intel_dp_aux_native_read_retry() return value check

 drivers/gpu/drm/i915/intel_dp.c |   98 +++++++++++++++++++--------------------
 1 file changed, 49 insertions(+), 49 deletions(-)

Signature · 1 line

1.7.10.4

Jani Nikula

author

2014-01-30 09:37 UTC #1 lore raw

PATCH 1/4 drm/i915/dp: make intel_dp_aux_native_read() return bool

In most cases we only care about success or fail, so make the function easier to use, and consistent with intel_dp_aux_native_read_retry().

This fixes the intel_dp_aux_native_read() usage in intel_dp_sink_crc() which has already assumed bool, and would only have caught very specific failure modes.

Signed-off-by Jani Nikula <[email protected]>

---
 drivers/gpu/drm/i915/intel_dp.c |   16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

drivers/gpu/drm/i915/intel_dp.c +12 -4

index 0ef2690..7e3d56e 100644
@@ -613,8 +613,8 @@ intel_dp_aux_native_write_1(struct intel_dp *intel_dp,
 
 /* read bytes from a native aux channel */
 static int
-intel_dp_aux_native_read(struct intel_dp *intel_dp,
-			 uint16_t address, uint8_t *recv, int recv_bytes)
+_intel_dp_aux_native_read(struct intel_dp *intel_dp,
+			  uint16_t address, uint8_t *recv, int recv_bytes)
 {
 	uint8_t msg[4];
 	int msg_bytes;

@@ -654,6 +654,14 @@ intel_dp_aux_native_read(struct intel_dp *intel_dp,
 	}
 }
 
+static inline bool
+intel_dp_aux_native_read(struct intel_dp *intel_dp,
+			 uint16_t address, uint8_t *recv, int recv_bytes)
+{
+	return _intel_dp_aux_native_read(intel_dp, address,
+					 recv, recv_bytes) == recv_bytes;
+}
+
 static int
 intel_dp_i2c_aux_ch(struct i2c_adapter *adapter, int mode,
 		    uint8_t write_byte, uint8_t *read_byte)

@@ -1986,8 +1994,8 @@ intel_dp_aux_native_read_retry(struct intel_dp *intel_dp, uint16_t address,
 	 * but we're also supposed to retry 3 times per the spec.
 	 */
 	for (i = 0; i < 3; i++) {
-		ret = intel_dp_aux_native_read(intel_dp, address, recv,
-					       recv_bytes);
+		ret = _intel_dp_aux_native_read(intel_dp, address, recv,
+						recv_bytes);
 		if (ret == recv_bytes)
 			return true;
 		msleep(1);

Signature · 1 line

1.7.10.4

Chris Wilson

2014-01-30 10:07 UTC #2 lore raw

Re: [PATCH 1/4] drm/i915/dp: make intel_dp_aux_native_read() return bool

Quoted 12 lines · Jani Nikula

drivers/gpu/drm/i915/intel_dp.c +8 -0

+static inline bool
+intel_dp_aux_native_read(struct intel_dp *intel_dp,
+			 uint16_t address, uint8_t *recv, int recv_bytes)
+{
+	return _intel_dp_aux_native_read(intel_dp, address,
+					 recv, recv_bytes) == recv_bytes;
+}
+
 static int
 intel_dp_i2c_aux_ch(struct i2c_adapter *adapter, int mode,
 		    uint8_t write_byte, uint8_t *read_byte)

drivers/gpu/drm/i915/intel_dp.c intel_dp_aux_native_read_retry(struct intel_dp *intel_dp, uint16_t address, · line 2000

Quoted 9 lines

drivers/gpu/drm/i915/intel_dp.c +2 -2

@@ -1986,8 +1994,8 @@ intel_dp_aux_native_read_retry(struct intel_dp *intel_dp, uint16_t address,
 	 * but we're also supposed to retry 3 times per the spec.
 	 */
 	for (i = 0; i < 3; i++) {
-		ret = intel_dp_aux_native_read(intel_dp, address, recv,
-					       recv_bytes);
+		ret = _intel_dp_aux_native_read(intel_dp, address, recv,
+						recv_bytes);
 		if (ret == recv_bytes)
 			return true;

ret is not being used other than to evaulate the boolean exactly like the new intel_dp_aux_native_read().
-Chris

Signature · 1 line

Chris Wilson, Intel Open Source Technology Centre

Jani Nikula

author

2014-01-30 12:00 UTC #3 lore raw

PATCH v2 1/1 drm/i915/dp: make intel_dp_aux_native_read() return bool

In most cases we only care about success or fail, so make the function easier to use, and consistent with intel_dp_aux_native_read_retry().

This fixes the intel_dp_aux_native_read() usage in intel_dp_sink_crc() which has already assumed bool, and would only have caught very specific failure modes.

v2: simplify intel_dp_aux_native_read_retry() while at it. (Chris)

Signed-off-by Jani Nikula <[email protected]>

---
 drivers/gpu/drm/i915/intel_dp.c |   19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

drivers/gpu/drm/i915/intel_dp.c +13 -6

index 0ef2690..8c61fce 100644
@@ -613,8 +613,8 @@ intel_dp_aux_native_write_1(struct intel_dp *intel_dp,
 
 /* read bytes from a native aux channel */
 static int
-intel_dp_aux_native_read(struct intel_dp *intel_dp,
-			 uint16_t address, uint8_t *recv, int recv_bytes)
+_intel_dp_aux_native_read(struct intel_dp *intel_dp,
+			  uint16_t address, uint8_t *recv, int recv_bytes)
 {
 	uint8_t msg[4];
 	int msg_bytes;

@@ -654,6 +654,14 @@ intel_dp_aux_native_read(struct intel_dp *intel_dp,
 	}
 }
 
+static inline bool
+intel_dp_aux_native_read(struct intel_dp *intel_dp,
+			 uint16_t address, uint8_t *recv, int recv_bytes)
+{
+	return _intel_dp_aux_native_read(intel_dp, address,
+					 recv, recv_bytes) == recv_bytes;
+}
+
 static int
 intel_dp_i2c_aux_ch(struct i2c_adapter *adapter, int mode,
 		    uint8_t write_byte, uint8_t *read_byte)

@@ -1979,16 +1987,15 @@ static bool
 intel_dp_aux_native_read_retry(struct intel_dp *intel_dp, uint16_t address,
 			       uint8_t *recv, int recv_bytes)
 {
-	int ret, i;
+	int i;
 
 	/*
 	 * Sinks are *supposed* to come up within 1ms from an off state,
 	 * but we're also supposed to retry 3 times per the spec.
 	 */
 	for (i = 0; i < 3; i++) {
-		ret = intel_dp_aux_native_read(intel_dp, address, recv,
-					       recv_bytes);
-		if (ret == recv_bytes)
+		if (intel_dp_aux_native_read(intel_dp, address, recv,
+					     recv_bytes))
 			return true;
 		msleep(1);
 	}

Signature · 1 line

1.7.10.4

Jani Nikula

author

2014-01-30 09:37 UTC #4 lore raw

PATCH 2/4 drm/i915/dp: move intel_dp_aux_native_read_retry() near other reads

In reply to Jani Nikula

Just code shuffling to have intel_dp_aux_native_read_retry() next to its sibling intel_dp_aux_native_read(). No functional changes.

Signed-off-by Jani Nikula <[email protected]>

---
 drivers/gpu/drm/i915/intel_dp.c |   50 +++++++++++++++++++--------------------
 1 file changed, 25 insertions(+), 25 deletions(-)

drivers/gpu/drm/i915/intel_dp.c +25 ~~-25~~

index 7e3d56e..9f305f7 100644
@@ -662,6 +662,31 @@ intel_dp_aux_native_read(struct intel_dp *intel_dp,
 					 recv, recv_bytes) == recv_bytes;
 }
 
+/*
+ * Native read with retry for link status and receiver capability reads for
+ * cases where the sink may still be asleep.
+ */
+static bool
+intel_dp_aux_native_read_retry(struct intel_dp *intel_dp, uint16_t address,
+			       uint8_t *recv, int recv_bytes)
+{
+	int ret, i;
+
+	/*
+	 * Sinks are *supposed* to come up within 1ms from an off state,
+	 * but we're also supposed to retry 3 times per the spec.
+	 */
+	for (i = 0; i < 3; i++) {
+		ret = _intel_dp_aux_native_read(intel_dp, address, recv,
+						recv_bytes);
+		if (ret == recv_bytes)
+			return true;
+		msleep(1);
+	}
+
+	return false;
+}
+
 static int
 intel_dp_i2c_aux_ch(struct i2c_adapter *adapter, int mode,
 		    uint8_t write_byte, uint8_t *read_byte)

@@ -1980,31 +2005,6 @@ static void vlv_dp_pre_pll_enable(struct intel_encoder *encoder)
 }
 
 /*
- * Native read with retry for link status and receiver capability reads for
- * cases where the sink may still be asleep.
- */
-static bool
-intel_dp_aux_native_read_retry(struct intel_dp *intel_dp, uint16_t address,
-			       uint8_t *recv, int recv_bytes)
-{
-	int ret, i;
-
-	/*
-	 * Sinks are *supposed* to come up within 1ms from an off state,
-	 * but we're also supposed to retry 3 times per the spec.
-	 */
-	for (i = 0; i < 3; i++) {
-		ret = _intel_dp_aux_native_read(intel_dp, address, recv,
-						recv_bytes);
-		if (ret == recv_bytes)
-			return true;
-		msleep(1);
-	}
-
-	return false;
-}
-
-/*
  * Fetch AUX CH registers 0x202 - 0x207 which contain
  * link status information
  */

Signature · 1 line

1.7.10.4

Jani Nikula

author

2014-01-30 12:02 UTC #5 lore raw

PATCH v2 1/1 drm/i915/dp: move intel_dp_aux_native_read_retry() near other reads

Just code shuffling to have intel_dp_aux_native_read_retry() next to its sibling intel_dp_aux_native_read(). No functional changes.

v2: rebase.

Signed-off-by Jani Nikula <[email protected]>

---
 drivers/gpu/drm/i915/intel_dp.c |   48 +++++++++++++++++++--------------------
 1 file changed, 24 insertions(+), 24 deletions(-)

drivers/gpu/drm/i915/intel_dp.c +24 ~~-24~~

index 8c61fce..4d04507 100644
@@ -662,6 +662,30 @@ intel_dp_aux_native_read(struct intel_dp *intel_dp,
 					 recv, recv_bytes) == recv_bytes;
 }
 
+/*
+ * Native read with retry for link status and receiver capability reads for
+ * cases where the sink may still be asleep.
+ */
+static bool
+intel_dp_aux_native_read_retry(struct intel_dp *intel_dp, uint16_t address,
+			       uint8_t *recv, int recv_bytes)
+{
+	int i;
+
+	/*
+	 * Sinks are *supposed* to come up within 1ms from an off state,
+	 * but we're also supposed to retry 3 times per the spec.
+	 */
+	for (i = 0; i < 3; i++) {
+		if (intel_dp_aux_native_read(intel_dp, address, recv,
+					     recv_bytes))
+			return true;
+		msleep(1);
+	}
+
+	return false;
+}
+
 static int
 intel_dp_i2c_aux_ch(struct i2c_adapter *adapter, int mode,
 		    uint8_t write_byte, uint8_t *read_byte)

@@ -1980,30 +2004,6 @@ static void vlv_dp_pre_pll_enable(struct intel_encoder *encoder)
 }
 
 /*
- * Native read with retry for link status and receiver capability reads for
- * cases where the sink may still be asleep.
- */
-static bool
-intel_dp_aux_native_read_retry(struct intel_dp *intel_dp, uint16_t address,
-			       uint8_t *recv, int recv_bytes)
-{
-	int i;
-
-	/*
-	 * Sinks are *supposed* to come up within 1ms from an off state,
-	 * but we're also supposed to retry 3 times per the spec.
-	 */
-	for (i = 0; i < 3; i++) {
-		if (intel_dp_aux_native_read(intel_dp, address, recv,
-					     recv_bytes))
-			return true;
-		msleep(1);
-	}
-
-	return false;
-}
-
-/*
  * Fetch AUX CH registers 0x202 - 0x207 which contain
  * link status information
  */

Signature · 1 line

1.7.10.4

Jani Nikula

author

2014-01-30 09:37 UTC #6 lore raw

PATCH 3/4 drm/i915/dp: clean up cargo culted intel_dp_aux_native_read_retry() usage

In reply to Jani Nikula

intel_dp_aux_native_read_retry() is only needed when the sink might be asleep. Use the regular read without retries otherwise.

Signed-off-by Jani Nikula <[email protected]>

---
 drivers/gpu/drm/i915/intel_dp.c |   32 ++++++++++++--------------------
 1 file changed, 12 insertions(+), 20 deletions(-)

drivers/gpu/drm/i915/intel_dp.c +12 ~~-20~~

index 9f305f7..3b2c4f0 100644
@@ -2877,9 +2877,9 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
 	/* Check if the panel supports PSR */
 	memset(intel_dp->psr_dpcd, 0, sizeof(intel_dp->psr_dpcd));
 	if (is_edp(intel_dp)) {
-		intel_dp_aux_native_read_retry(intel_dp, DP_PSR_SUPPORT,
-					       intel_dp->psr_dpcd,
-					       sizeof(intel_dp->psr_dpcd));
+		intel_dp_aux_native_read(intel_dp, DP_PSR_SUPPORT,
+					 intel_dp->psr_dpcd,
+					 sizeof(intel_dp->psr_dpcd));
 		if (intel_dp->psr_dpcd[0] & DP_PSR_IS_SUPPORTED) {
 			dev_priv->psr.sink_support = true;
 			DRM_DEBUG_KMS("Detected EDP PSR Panel.\n");

@@ -2901,9 +2901,9 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
 	if (intel_dp->dpcd[DP_DPCD_REV] == 0x10)
 		return true; /* no per-port downstream info */
 
-	if (intel_dp_aux_native_read_retry(intel_dp, DP_DOWNSTREAM_PORT_0,
-					   intel_dp->downstream_ports,
-					   DP_MAX_DOWNSTREAM_PORTS) == 0)
+	if (!intel_dp_aux_native_read(intel_dp, DP_DOWNSTREAM_PORT_0,
+				      intel_dp->downstream_ports,
+				      DP_MAX_DOWNSTREAM_PORTS))
 		return false; /* downstream port status fetch failed */
 
 	return true;

@@ -2919,11 +2919,11 @@ intel_dp_probe_oui(struct intel_dp *intel_dp)
 
 	edp_panel_vdd_on(intel_dp);
 
-	if (intel_dp_aux_native_read_retry(intel_dp, DP_SINK_OUI, buf, 3))
+	if (intel_dp_aux_native_read(intel_dp, DP_SINK_OUI, buf, 3))
 		DRM_DEBUG_KMS("Sink OUI: %02hx%02hx%02hx\n",
 			      buf[0], buf[1], buf[2]);
 
-	if (intel_dp_aux_native_read_retry(intel_dp, DP_BRANCH_OUI, buf, 3))
+	if (intel_dp_aux_native_read(intel_dp, DP_BRANCH_OUI, buf, 3))
 		DRM_DEBUG_KMS("Branch OUI: %02hx%02hx%02hx\n",
 			      buf[0], buf[1], buf[2]);

@@ -2959,18 +2959,11 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc)
 	return 0;
 }
 
-static bool
+static inline bool
 intel_dp_get_sink_irq(struct intel_dp *intel_dp, u8 *sink_irq_vector)
 {
-	int ret;
-
-	ret = intel_dp_aux_native_read_retry(intel_dp,
-					     DP_DEVICE_SERVICE_IRQ_VECTOR,
-					     sink_irq_vector, 1);
-	if (!ret)
-		return false;
-
-	return true;
+	return intel_dp_aux_native_read(intel_dp, DP_DEVICE_SERVICE_IRQ_VECTOR,
+					sink_irq_vector, 1);
 }
 
 static void

@@ -3053,8 +3046,7 @@ intel_dp_detect_dpcd(struct intel_dp *intel_dp)
 	if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 &&
 	    intel_dp->downstream_ports[0] & DP_DS_PORT_HPD) {
 		uint8_t reg;
-		if (!intel_dp_aux_native_read_retry(intel_dp, DP_SINK_COUNT,
-						    &reg, 1))
+		if (!intel_dp_aux_native_read(intel_dp, DP_SINK_COUNT, &reg, 1))
 			return connector_status_unknown;
 		return DP_GET_SINK_COUNT(reg) ? connector_status_connected
 					      : connector_status_disconnected;

Signature · 1 line

1.7.10.4

Jani Nikula

author

2014-01-30 09:37 UTC #7 lore raw

PATCH 4/4 drm/i915/dp: fix intel_dp_aux_native_read_retry() return value check

In reply to Jani Nikula

intel_dp_aux_native_read_retry() returns a bool, so check for a bool for consistency. No functional changes.

Signed-off-by Jani Nikula <[email protected]>

---
 drivers/gpu/drm/i915/intel_dp.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/gpu/drm/i915/intel_dp.c +2 -2

index 3b2c4f0..27ba24d 100644
@@ -2863,8 +2863,8 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
 
 	char dpcd_hex_dump[sizeof(intel_dp->dpcd) * 3];
 
-	if (intel_dp_aux_native_read_retry(intel_dp, 0x000, intel_dp->dpcd,
-					   sizeof(intel_dp->dpcd)) == 0)
+	if (!intel_dp_aux_native_read_retry(intel_dp, 0x000, intel_dp->dpcd,
+					    sizeof(intel_dp->dpcd)))
 		return false; /* aux transfer failed */
 
 	hex_dump_to_buffer(intel_dp->dpcd, sizeof(intel_dp->dpcd),

Signature · 1 line

1.7.10.4

Daniel Vetter

2014-01-30 09:50 UTC #8 lore raw

In reply to Jani Nikula

Quoted 5 lines · Jani Nikula

Fix and cleanup the rather confused native aux read return values and return value checks. Maybe we'll get them right from now on. Just maybe.
Did not have a chance to test this, I'm afraid, but it's pretty straightforward.

How does this fit in with Thierry's dp aux helper improvements? I prefer if we start to converge on that sooner than later ... -Daniel

Signature · 3 lines

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

Jani Nikula

author

2014-01-30 13:38 UTC #9 lore raw

Quoted 2 lines · Daniel Vetter

How does this fit in with Thierry's dp aux helper improvements? I prefer if we start to converge on that sooner than later ...

Patch 1/4 actually fixes a potential bug in intel_dp_sink_crc(), which now detects failure only if the read returns 0, but plunges on with any negative error values. So we may want to queue that for fixes.

Patch 2/4 could be dropped.

I'd like to do 3/4 as a prep patch anyway, as it potentially changes behaviour, and makes conversion to the helpers more straightforward.

Patch 4/4 could be dropped.

To see how this would pan out, I'm almost done with the native aux conversion (did not look at the i2c-over-aux part yet).

If we use those interfaces directly all over the place, the only annoyance is the potential for the same confusion that I've tried to avoid in this series.

The only correct check for success is comparing the transfer function return value to the number of bytes that was to be transferred:

	if (drm_dp_dpcd_read(&intel_dp->dp_aux, offset, buffer, len) != len)

but len may in fact be a fairly long #define like
DP_DEVICE_SERVICE_IRQ_VECTOR.

I do notice Thierry checks for ret < 0 in his code, although, IIUC, it's possible the transfer ends with fewer bytes transferred than requested.

I'm beginning to feel like the functions should return and guarantee an error code < 0 if not all bytes were transferred, just for the ease of use. I mean, it's a nice feature to be able to make the distinction, but I can't fathom a practical situation where that would be necessary.

BR,
Jani.

Signature · 1 line

Jani Nikula, Intel Open Source Technology Center

Jani Nikula

author

2014-01-30 15:06 UTC #10 lore raw

In reply to Daniel Vetter

Quoted 9 lines · Daniel Vetter

Quoted 5 lines · Jani Nikula
Fix and cleanup the rather confused native aux read return values and return value checks. Maybe we'll get them right from now on. Just maybe.
Did not have a chance to test this, I'm afraid, but it's pretty straightforward.
How does this fit in with Thierry's dp aux helper improvements? I prefer if we start to converge on that sooner than later ...

For the i2c-over-aux conversion

commit 8a5e6aeb30ecaf8f11a99c0d008c8935cd6fba9f

Author: Paulo Zanoni <[email protected]>
Date:   Wed Oct 30 19:50:26 2013 -0200

drm/i915: turn the eDP VDD on for any i2c transactions

will be a PITA, as we lose control of that level in the stack.

So we'd need to move the vdd enable/disable higher or lower. Per that commit, higher was problematic. If we move it lower, we may need to add refcounting to the vdd, but then we'll lose the warn for "eDP VDD already requested on\n".

Ideas welcome.

Jani.

Signature · 1 line

Jani Nikula, Intel Open Source Technology Center

Daniel Vetter

2014-01-30 15:12 UTC #11 lore raw

Quoted 19 lines · Jani Nikula

Quoted 9 lines · Daniel Vetter
Quoted 5 lines · Jani Nikula
Fix and cleanup the rather confused native aux read return values and return value checks. Maybe we'll get them right from now on. Just maybe.
Did not have a chance to test this, I'm afraid, but it's pretty straightforward.
How does this fit in with Thierry's dp aux helper improvements? I prefer if we start to converge on that sooner than later ...
For the i2c-over-aux conversion
commit 8a5e6aeb30ecaf8f11a99c0d008c8935cd6fba9f
Author: Paulo Zanoni <[email protected]>
Date:   Wed Oct 30 19:50:26 2013 -0200
drm/i915: turn the eDP VDD on for any i2c transactions

Quoted 8 lines

will be a PITA, as we lose control of that level in the stack.
So we'd need to move the vdd enable/disable higher or lower. Per that commit, higher was problematic. If we move it lower, we may need to add refcounting to the vdd, but then we'll lose the warn for "eDP VDD already requested on\n".
Ideas welcome.

Nasty. Although this was just to make the /dev i2c access work from userspace, so I don't expect more places to pop up. Hence I think a panel_vdd_force_on which returns the current state + panel_vdd_restore to undo that in the low-level dp aux to be sufficient. But not sure. -Daniel

Signature · 3 lines

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