[PATCH 0/4] drm/i915/dp: native aux read return value fixes and cleanup

[PATCH 0/4] drm/i915/dp: native aux read return value fixes and cleanup

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

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

-- 
1.7.10.4

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

[Jani Nikula]

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 <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c |   16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 0ef2690..7e3d56e 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -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);
-- 
1.7.10.4

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

[Chris Wilson]

On Thu, Jan 30, 2014 at 11:37:01AM +0200, Jani Nikula wrote:
> +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;

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

-- 
Chris Wilson, Intel Open Source Technology Centre

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

[Jani Nikula]

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 <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c |   19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 0ef2690..8c61fce 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -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);
 	}
-- 
1.7.10.4

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

[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 <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c |   50 +++++++++++++++++++--------------------
 1 file changed, 25 insertions(+), 25 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 7e3d56e..9f305f7 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -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
  */
-- 
1.7.10.4

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

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

v2: rebase.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c |   48 +++++++++++++++++++--------------------
 1 file changed, 24 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 8c61fce..4d04507 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -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
  */
-- 
1.7.10.4

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

[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 <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c |   32 ++++++++++++--------------------
 1 file changed, 12 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 9f305f7..3b2c4f0 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -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;
-- 
1.7.10.4

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

[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 <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 3b2c4f0..27ba24d 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -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),
-- 
1.7.10.4

Re: [PATCH 0/4] drm/i915/dp: native aux read return value fixes and cleanup

[Daniel Vetter]

On Thu, Jan 30, 2014 at 11:37:00AM +0200, Jani Nikula wrote:
> 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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 0/4] drm/i915/dp: native aux read return value fixes and cleanup

[Jani Nikula]

On Thu, 30 Jan 2014, Daniel Vetter <daniel@ffwll.ch> wrote:
> 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.


-- 
Jani Nikula, Intel Open Source Technology Center

Re: [PATCH 0/4] drm/i915/dp: native aux read return value fixes and cleanup

[Jani Nikula]

On Thu, 30 Jan 2014, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Thu, Jan 30, 2014 at 11:37:00AM +0200, Jani Nikula wrote:
>> 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 <paulo.r.zanoni@intel.com>
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.


-- 
Jani Nikula, Intel Open Source Technology Center

Re: [PATCH 0/4] drm/i915/dp: native aux read return value fixes and cleanup

[Daniel Vetter]

On Thu, Jan 30, 2014 at 4:05 PM, Jani Nikula <jani.nikula@intel.com> wrote:
> On Thu, 30 Jan 2014, Daniel Vetter <daniel@ffwll.ch> wrote:
>> On Thu, Jan 30, 2014 at 11:37:00AM +0200, Jani Nikula wrote:
>>> 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 <paulo.r.zanoni@intel.com>
> 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.

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch