drm/dp: Fix Write_Status_Update_Request AUX request format

PATCH Superseded Wayne Lin 3 participants 7 messages Apr 27, 2025 – May 8, 2025

Subsystems (3)

Stable branch DRM drivers DRM misc

lore

Share & tools

lore

Wayne Lin

author

2025-04-27 09:51 UTC #0 lore raw

PATCH 1/1 drm/dp: Fix Write_Status_Update_Request AUX request format

[Why]
Notice AUX request format of I2C-over-AUX with Write_Status_Update_Request flag set is incorrect. It should be address only request without length and data like:
"SYNC->COM3:0 (= 0110)|0000-> 0000|0000->
0|7-bit I2C address (the same as the last)-> STOP->".

[How]
Refer to DP v2.1 Table 2-178, correct the Write_Status_Update_Request to be address only request.

Note that we might receive 0 returned by aux->transfer() when receive reply I2C_ACK|AUX_ACK of Write_Status_Update_Request transaction. Which indicating all data bytes get written. We should avoid to return 0 bytes get transferred under this case.

Fixes 68ec2a2a2481 ("drm/dp: Use I2C_WRITE_STATUS_UPDATE to drain partial I2C_WRITE requests")

Cc Ville Syrjälä <[email protected]>

Cc Jani Nikula <[email protected]>

Cc Mario Limonciello <[email protected]>

Cc Harry Wentland <[email protected]>

Cc [email protected]

Signed-off-by Wayne Lin <[email protected]>

---
 drivers/gpu/drm/display/drm_dp_helper.c | 45 +++++++++++++++++++++----
 1 file changed, 38 insertions(+), 7 deletions(-)

drivers/gpu/drm/display/drm_dp_helper.c +38 -7

index 57828f2b7b5a..0c8cba7ed875 100644
@@ -1857,6 +1857,12 @@ static u32 drm_dp_i2c_functionality(struct i2c_adapter *adapter)
 	       I2C_FUNC_10BIT_ADDR;
 }
 
+static inline bool
+drm_dp_i2c_msg_is_write_status_update(struct drm_dp_aux_msg *msg)
+{
+	return ((msg->request & ~DP_AUX_I2C_MOT) == DP_AUX_I2C_WRITE_STATUS_UPDATE);
+}
+
 static void drm_dp_i2c_msg_write_status_update(struct drm_dp_aux_msg *msg)
 {
 	/*

@@ -1965,6 +1971,7 @@ MODULE_PARM_DESC(dp_aux_i2c_speed_khz,
 static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
 {
 	unsigned int retry, defer_i2c;
+	struct drm_dp_aux_msg orig_msg = *msg;
 	int ret;
 	/*
 	 * DP1.2 sections 2.7.7.1.5.6.1 and 2.7.7.1.6.6.1: A DP Source device

@@ -1976,6 +1983,12 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
 	int max_retries = max(7, drm_dp_i2c_retry_count(msg, dp_aux_i2c_speed_khz));
 
 	for (retry = 0, defer_i2c = 0; retry < (max_retries + defer_i2c); retry++) {
+		if (drm_dp_i2c_msg_is_write_status_update(msg)) {
+			/* Address only transaction */
+			msg->buffer = NULL;
+			msg->size = 0;
+		}
+
 		ret = aux->transfer(aux, msg);
 		if (ret < 0) {
 			if (ret == -EBUSY)

@@ -1993,7 +2006,7 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
 			else
 				drm_dbg_kms(aux->drm_dev, "%s: transaction failed: %d\n",
 					    aux->name, ret);
-			return ret;
+			goto out;
 		}

@@ -2008,7 +2021,8 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
 		case DP_AUX_NATIVE_REPLY_NACK:
 			drm_dbg_kms(aux->drm_dev, "%s: native nack (result=%d, size=%zu)\n",
 				    aux->name, ret, msg->size);
-			return -EREMOTEIO;
+			ret = -EREMOTEIO;
+			goto out;
 
 		case DP_AUX_NATIVE_REPLY_DEFER:
 			drm_dbg_kms(aux->drm_dev, "%s: native defer\n", aux->name);

@@ -2027,24 +2041,35 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
 		default:
 			drm_err(aux->drm_dev, "%s: invalid native reply %#04x\n",
 				aux->name, msg->reply);
-			return -EREMOTEIO;
+			ret = -EREMOTEIO;
+			goto out;
 		}
 
 		switch (msg->reply & DP_AUX_I2C_REPLY_MASK) {
 		case DP_AUX_I2C_REPLY_ACK:
+			/*
+			 * When I2C write firstly get defer and get ack after
+			 * retries by wirte_status_update, we have to return
+			 * all data bytes get transferred instead of 0.
+			 */
+			if (drm_dp_i2c_msg_is_write_status_update(msg) && ret == 0)
+				ret = orig_msg.size;
+
 			/*
 			 * Both native ACK and I2C ACK replies received. We
 			 * can assume the transfer was successful.
 			 */
 			if (ret != msg->size)
 				drm_dp_i2c_msg_write_status_update(msg);
-			return ret;
+
+			goto out;
 
 		case DP_AUX_I2C_REPLY_NACK:
 			drm_dbg_kms(aux->drm_dev, "%s: I2C nack (result=%d, size=%zu)\n",
 				    aux->name, ret, msg->size);
 			aux->i2c_nack_count++;
-			return -EREMOTEIO;
+			ret = -EREMOTEIO;
+			goto out;
 
 		case DP_AUX_I2C_REPLY_DEFER:
 			drm_dbg_kms(aux->drm_dev, "%s: I2C defer\n", aux->name);

@@ -2063,12 +2088,18 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
 		default:
 			drm_err(aux->drm_dev, "%s: invalid I2C reply %#04x\n",
 				aux->name, msg->reply);
-			return -EREMOTEIO;
+			ret = -EREMOTEIO;
+			goto out;
 		}
 	}
 
 	drm_dbg_kms(aux->drm_dev, "%s: Too many retries, giving up\n", aux->name);
-	return -EREMOTEIO;
+	ret = -EREMOTEIO;
+out:
+	/* In case we change original msg by Write_Status_Update*/
+	msg->buffer = orig_msg.buffer;
+	msg->size = orig_msg.size;
+	return ret;
 }
 
 static void drm_dp_i2c_msg_set_request(struct drm_dp_aux_msg *msg,

Signature · 1 line

2.43.0

Mario Limonciello

2025-05-05 20:22 UTC #1 lore raw

Quoted 129 lines · Wayne Lin

[How]
Refer to DP v2.1 Table 2-178, correct the Write_Status_Update_Request to be address only request.

Fixes 68ec2a2a2481 ("drm/dp: Use I2C_WRITE_STATUS_UPDATE to drain partial I2C_WRITE requests")

Cc Ville Syrjälä <[email protected]>

Cc Jani Nikula <[email protected]>

Cc Mario Limonciello <[email protected]>

Cc Harry Wentland <[email protected]>

Cc [email protected]

Signed-off-by Wayne Lin <[email protected]>

---
  drivers/gpu/drm/display/drm_dp_helper.c | 45 +++++++++++++++++++++----
  1 file changed, 38 insertions(+), 7 deletions(-)

drivers/gpu/drm/display/drm_dp_helper.c +30 -6

index 57828f2b7b5a..0c8cba7ed875 100644
@@ -1857,6 +1857,12 @@ static u32 drm_dp_i2c_functionality(struct i2c_adapter *adapter)
  	       I2C_FUNC_10BIT_ADDR;
  }
 
+static inline bool
+drm_dp_i2c_msg_is_write_status_update(struct drm_dp_aux_msg *msg)
+{
+	return ((msg->request & ~DP_AUX_I2C_MOT) == DP_AUX_I2C_WRITE_STATUS_UPDATE);
+}
+
  static void drm_dp_i2c_msg_write_status_update(struct drm_dp_aux_msg *msg)
  {
  	/*

@@ -1965,6 +1971,7 @@ MODULE_PARM_DESC(dp_aux_i2c_speed_khz,
  static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
  {
  	unsigned int retry, defer_i2c;
+	struct drm_dp_aux_msg orig_msg = *msg;
  	int ret;
  	/*
  	 * DP1.2 sections 2.7.7.1.5.6.1 and 2.7.7.1.6.6.1: A DP Source device

@@ -1976,6 +1983,12 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
  	int max_retries = max(7, drm_dp_i2c_retry_count(msg, dp_aux_i2c_speed_khz));
 
  	for (retry = 0, defer_i2c = 0; retry < (max_retries + defer_i2c); retry++) {
+		if (drm_dp_i2c_msg_is_write_status_update(msg)) {
+			/* Address only transaction */
+			msg->buffer = NULL;
+			msg->size = 0;
+		}
+
  		ret = aux->transfer(aux, msg);
  		if (ret < 0) {
  			if (ret == -EBUSY)

@@ -1993,7 +2006,7 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
  			else
  				drm_dbg_kms(aux->drm_dev, "%s: transaction failed: %d\n",
  					    aux->name, ret);
-			return ret;
+			goto out;
  		}

@@ -2008,7 +2021,8 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
  		case DP_AUX_NATIVE_REPLY_NACK:
  			drm_dbg_kms(aux->drm_dev, "%s: native nack (result=%d, size=%zu)\n",
  				    aux->name, ret, msg->size);
-			return -EREMOTEIO;
+			ret = -EREMOTEIO;
+			goto out;
 
  		case DP_AUX_NATIVE_REPLY_DEFER:
  			drm_dbg_kms(aux->drm_dev, "%s: native defer\n", aux->name);

@@ -2027,24 +2041,35 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
  		default:
  			drm_err(aux->drm_dev, "%s: invalid native reply %#04x\n",
  				aux->name, msg->reply);
-			return -EREMOTEIO;
+			ret = -EREMOTEIO;
+			goto out;
  		}
 
  		switch (msg->reply & DP_AUX_I2C_REPLY_MASK) {
  		case DP_AUX_I2C_REPLY_ACK:
+			/*
+			 * When I2C write firstly get defer and get ack after
+			 * retries by wirte_status_update, we have to return
+			 * all data bytes get transferred instead of 0.
+			 */
+			if (drm_dp_i2c_msg_is_write_status_update(msg) && ret == 0)
+				ret = orig_msg.size;
+
  			/*
  			 * Both native ACK and I2C ACK replies received. We
  			 * can assume the transfer was successful.
  			 */
  			if (ret != msg->size)
  				drm_dp_i2c_msg_write_status_update(msg);
-			return ret;
+
+			goto out;
 
  		case DP_AUX_I2C_REPLY_NACK:
  			drm_dbg_kms(aux->drm_dev, "%s: I2C nack (result=%d, size=%zu)\n",
  				    aux->name, ret, msg->size);
  			aux->i2c_nack_count++;
-			return -EREMOTEIO;
+			ret = -EREMOTEIO;
+			goto out;
 
  		case DP_AUX_I2C_REPLY_DEFER:
  			drm_dbg_kms(aux->drm_dev, "%s: I2C defer\n", aux->name);

@@ -2063,12 +2088,18 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
  		default:
  			drm_err(aux->drm_dev, "%s: invalid I2C reply %#04x\n",
  				aux->name, msg->reply);
-			return -EREMOTEIO;

drivers/gpu/drm/display/drm_dp_helper.c static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg) · line 2099

Quoted 10 lines

drivers/gpu/drm/display/drm_dp_helper.c +5 -1

+			ret = -EREMOTEIO;
+			goto out;
  		}
  	}
 
  	drm_dbg_kms(aux->drm_dev, "%s: Too many retries, giving up\n", aux->name);
-	return -EREMOTEIO;
+	ret = -EREMOTEIO;
+out:
+	/* In case we change original msg by Write_Status_Update*/

As there are multiple cases that jump to the "out" label, would it be clearer to use:

if (drm_dp_i2c_msg_is_write_status_update(msg)) {
msg->buffer = orig_msg.buffer;
msg->size = orig_msg.size;
}

return ret;

Quoted 6 lines

drivers/gpu/drm/display/drm_dp_helper.c +3 -0

+	msg->buffer = orig_msg.buffer;
+	msg->size = orig_msg.size;
+	return ret;
  }
 
  static void drm_dp_i2c_msg_set_request(struct drm_dp_aux_msg *msg,

Lin, Wayne

author

2025-05-08 01:49 UTC #2 lore raw

[AMD Official Use Only - AMD Internal Distribution Only]

Thanks for the feedback! I'll adjust it and give v2 soon.

Thanks,
Wayne

Quoted 181 lines

-----Original Message-----
From: Limonciello, Mario <[email protected]>
Sent: Tuesday, May 6, 2025 4:22 AM
To: Lin, Wayne <[email protected]>; [email protected]
Cc: [email protected]; [email protected]; Wentland, Harry <[email protected]>; [email protected]
Subject: Re: [PATCH] drm/dp: Fix Write_Status_Update_Request AUX request format
drivers/gpu/drm/display/drm_dp_helper.c static u32 drm_dp_i2c_functionality(struct i2c_adapter
Quoted 34 lines · Wayne Lin
[Why]
Notice AUX request format of I2C-over-AUX with Write_Status_Update_Request flag set is incorrect. It should be address only request without length and data like:
"SYNC->COM3:0 (= 0110)|0000-> 0000|0000->
0|7-bit I2C address (the same as the last)-> STOP->".
[How]
Refer to DP v2.1 Table 2-178, correct the Write_Status_Update_Request to be address only request.
Note that we might receive 0 returned by aux->transfer() when receive reply I2C_ACK|AUX_ACK of Write_Status_Update_Request transaction. Which indicating all data bytes get written.
We should avoid to return 0 bytes get transferred under this case.
Fixes 68ec2a2a2481 ("drm/dp: Use I2C_WRITE_STATUS_UPDATE to drain
partial I2C_WRITE requests")
Cc Ville Syrjälä <[email protected]>
Cc Jani Nikula <[email protected]>
Cc Mario Limonciello <[email protected]>
Cc Harry Wentland <[email protected]>
Cc [email protected]
Signed-off-by Wayne Lin <[email protected]>
---
  drivers/gpu/drm/display/drm_dp_helper.c | 45 +++++++++++++++++++++----
  1 file changed, 38 insertions(+), 7 deletions(-)
drivers/gpu/drm/display/drm_dp_helper.c +0 -0
index 57828f2b7b5a..0c8cba7ed875 100644
@@ -1857,6 +1857,12 @@ static u32 drm_dp_i2c_functionality(struct i2c_adapter
*adapter)
drivers/gpu/drm/display/drm_dp_helper.c MODULE_PARM_DESC(dp_aux_i2c_speed_khz, · line 1971
Quoted 13 lines
drivers/gpu/drm/display/drm_dp_helper.c +5 -0
           I2C_FUNC_10BIT_ADDR;
  }
 
+static inline bool
+drm_dp_i2c_msg_is_write_status_update(struct drm_dp_aux_msg *msg) {
+   return ((msg->request & ~DP_AUX_I2C_MOT) ==
+DP_AUX_I2C_WRITE_STATUS_UPDATE); }
+
  static void drm_dp_i2c_msg_write_status_update(struct drm_dp_aux_msg *msg)
  {
    /*
@@ -1965,6 +1971,7 @@ MODULE_PARM_DESC(dp_aux_i2c_speed_khz,
  static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg
*msg)
Quoted 7 lines
drivers/gpu/drm/display/drm_dp_helper.c +1 -0
  {
    unsigned int retry, defer_i2c;
+   struct drm_dp_aux_msg orig_msg = *msg;
    int ret;
    /*
     * DP1.2 sections 2.7.7.1.5.6.1 and 2.7.7.1.6.6.1: A DP Source
device @@ -1976,6 +1983,12 @@ static int drm_dp_i2c_do_msg(struct
drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
drivers/gpu/drm/display/drm_dp_helper.c static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux,
Quoted 15 lines
int max_retries = max(7, drm_dp_i2c_retry_count(msg,
dp_aux_i2c_speed_khz));
    for (retry = 0, defer_i2c = 0; retry < (max_retries + defer_i2c);
retry++) {
drivers/gpu/drm/display/drm_dp_helper.c +6 -0
+           if (drm_dp_i2c_msg_is_write_status_update(msg)) {
+                   /* Address only transaction */
+                   msg->buffer = NULL;
+                   msg->size = 0;
+           }
+
            ret = aux->transfer(aux, msg);
            if (ret < 0) {
                    if (ret == -EBUSY)
@@ -1993,7 +2006,7 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux,
struct drm_dp_aux_msg *msg)
drivers/gpu/drm/display/drm_dp_helper.c static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, · line 2007
Quoted 2 lines
drivers/gpu/drm/display/drm_dp_helper.c +0 -0
                    else
                            drm_dbg_kms(aux->drm_dev, "%s: transaction failed:
%d\n",
drivers/gpu/drm/display/drm_dp_helper.c static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux,
Quoted 7 lines
drivers/gpu/drm/display/drm_dp_helper.c +1 -1
                                        aux->name, ret);
-                   return ret;
+                   goto out;
            }
 
 
@@ -2008,7 +2021,8 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux,
struct drm_dp_aux_msg *msg)
drivers/gpu/drm/display/drm_dp_helper.c static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, · line 2022
Quoted 2 lines
drivers/gpu/drm/display/drm_dp_helper.c +0 -0
            case DP_AUX_NATIVE_REPLY_NACK:
                    drm_dbg_kms(aux->drm_dev, "%s: native nack (result=%d,
size=%zu)\n",
Quoted 9 lines
drivers/gpu/drm/display/drm_dp_helper.c +2 -1
                                aux->name, ret, msg->size);
-                   return -EREMOTEIO;
+                   ret = -EREMOTEIO;
+                   goto out;
 
            case DP_AUX_NATIVE_REPLY_DEFER:
                    drm_dbg_kms(aux->drm_dev, "%s: native defer\n", aux-
name); @@
-2027,24 +2041,35 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux,
struct drm_dp_aux_msg *msg)
Quoted 6 lines
            default:
                    drm_err(aux->drm_dev, "%s: invalid native reply %#04x\n",
                            aux->name, msg->reply);
drivers/gpu/drm/display/drm_dp_helper.c +2 -1
-                   return -EREMOTEIO;
+                   ret = -EREMOTEIO;
+                   goto out;
drivers/gpu/drm/display/drm_dp_helper.c static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
Quoted 10 lines
drivers/gpu/drm/display/drm_dp_helper.c +6 -0
            }
 
            switch (msg->reply & DP_AUX_I2C_REPLY_MASK) {
            case DP_AUX_I2C_REPLY_ACK:
+                   /*
+                    * When I2C write firstly get defer and get ack after
+                    * retries by wirte_status_update, we have to return
+                    * all data bytes get transferred instead of 0.
+                    */
+                   if (drm_dp_i2c_msg_is_write_status_update(msg) && ret ==
0)
Quoted 4 lines
drivers/gpu/drm/display/drm_dp_helper.c +2 -0
+                           ret = orig_msg.size;
+
                    /*
                     * Both native ACK and I2C ACK replies received. We
drivers/gpu/drm/display/drm_dp_helper.c static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
Quoted 10 lines
drivers/gpu/drm/display/drm_dp_helper.c +2 -1
                     * can assume the transfer was successful.
                     */
                    if (ret != msg->size)
                            drm_dp_i2c_msg_write_status_update(msg);
-                   return ret;
+
+                   goto out;
 
            case DP_AUX_I2C_REPLY_NACK:
                    drm_dbg_kms(aux->drm_dev, "%s: I2C nack (result=%d,
size=%zu)\n",
drivers/gpu/drm/display/drm_dp_helper.c static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg) · line 2075
Quoted 8 lines
                                aux->name, ret, msg->size);
                    aux->i2c_nack_count++;
drivers/gpu/drm/display/drm_dp_helper.c +2 -1
-                   return -EREMOTEIO;
+                   ret = -EREMOTEIO;
+                   goto out;
 
            case DP_AUX_I2C_REPLY_DEFER:
                    drm_dbg_kms(aux->drm_dev, "%s: I2C defer\n", aux->name);
@@
Quoted 1 line
-2063,12 +2088,18 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux,
struct drm_dp_aux_msg *msg)
Quoted 11 lines
            default:
                    drm_err(aux->drm_dev, "%s: invalid I2C reply %#04x\n",
                            aux->name, msg->reply);
drivers/gpu/drm/display/drm_dp_helper.c +2 -1
-                   return -EREMOTEIO;
+                   ret = -EREMOTEIO;
+                   goto out;
            }
    }
 
    drm_dbg_kms(aux->drm_dev, "%s: Too many retries, giving up\n", aux-
name);
drivers/gpu/drm/display/drm_dp_helper.c static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg) · line 2099
Quoted 4 lines
drivers/gpu/drm/display/drm_dp_helper.c +3 -1
-   return -EREMOTEIO;
+   ret = -EREMOTEIO;
+out:
+   /* In case we change original msg by Write_Status_Update*/
As there are multiple cases that jump to the "out" label, would it be clearer to use:
if (drm_dp_i2c_msg_is_write_status_update(msg)) {
      msg->buffer = orig_msg.buffer;
      msg->size = orig_msg.size;
}
return ret;
Quoted 6 lines
drivers/gpu/drm/display/drm_dp_helper.c +3 -0
+   msg->buffer = orig_msg.buffer;
+   msg->size = orig_msg.size;
+   return ret;
  }
 
  static void drm_dp_i2c_msg_set_request(struct drm_dp_aux_msg *msg,

Jani Nikula

2025-05-08 08:19 UTC #3 lore raw

In reply to Wayne Lin

drivers/gpu/drm/display/drm_dp_helper.c static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg) · line 2054

Quoted 5 lines · Wayne Lin

drivers/gpu/drm/display/drm_dp_helper.c +5 -0

+			/*
+			 * When I2C write firstly get defer and get ack after
+			 * retries by wirte_status_update, we have to return
+			 * all data bytes get transferred instead of 0.
+			 */

My brain gives me syntax and parse error here. ;)

BR,
Jani.

Signature · 1 line

Jani Nikula, Intel

Lin, Wayne

author

2025-05-08 12:02 UTC #4 lore raw

[Public]

Quoted 10 lines

-----Original Message-----
From: Jani Nikula <[email protected]>
Sent: Thursday, May 8, 2025 4:19 PM
To: Lin, Wayne <[email protected]>; [email protected]
Cc: [email protected]; Limonciello, Mario <[email protected]>; Wentland, Harry <[email protected]>; Lin, Wayne <[email protected]>; [email protected]
Subject: Re: [PATCH] drm/dp: Fix Write_Status_Update_Request AUX request format

Quoted 8 lines

drivers/gpu/drm/display/drm_dp_helper.c static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg) · line 2054
Quoted 5 lines · Wayne Lin
drivers/gpu/drm/display/drm_dp_helper.c +5 -0
+                   /*
+                    * When I2C write firstly get defer and get ack after
+                    * retries by wirte_status_update, we have to return
+                    * all data bytes get transferred instead of 0.
+                    */
My brain gives me syntax and parse error here. ;)

Appreciate for the feedback, Jani.
Could you elaborate more on your concerns please?

Since Write_Status_Update_Request is address only request. Data length is 0. When I2C write request completes, reply for Write_Status_Update_Request from DPRx will be ACK only (i.e. data length is 0).

Is your concern about returning 0 from aux->transfer?
My thoughts is drm_dp_i2c_do_msg() is designed to handle I2C-Over-Aux reply data, and aux->transfer() is handling hw specific manipulation and return transferred bytes. For Write_Status_Update_Request request itself, nothing new to be transferred. I think drm_dp_i2c_do_msg() should be responsible for determining the correct transferred data bytes under this case. Or do you expect aux->transfer to memorize the data length of write request?

Thanks!

Quoted 6 lines

BR,
Jani.
--
Jani Nikula, Intel

--
Wayne Lin

Jani Nikula

2025-05-08 12:16 UTC #5 lore raw

Quoted 29 lines · Wayne

[Public]
Quoted 10 lines
-----Original Message-----
From: Jani Nikula <[email protected]>
Sent: Thursday, May 8, 2025 4:19 PM
To: Lin, Wayne <[email protected]>; [email protected]
Cc: [email protected]; Limonciello, Mario <[email protected]>; Wentland, Harry <[email protected]>; Lin, Wayne <[email protected]>; [email protected]
Subject: Re: [PATCH] drm/dp: Fix Write_Status_Update_Request AUX request format
Quoted 8 lines
drivers/gpu/drm/display/drm_dp_helper.c static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg) · line 2054
Quoted 5 lines · Wayne Lin
drivers/gpu/drm/display/drm_dp_helper.c +5 -0
+                   /*
+                    * When I2C write firstly get defer and get ack after
+                    * retries by wirte_status_update, we have to return
+                    * all data bytes get transferred instead of 0.
+                    */
My brain gives me syntax and parse error here. ;)
Appreciate for the feedback, Jani.
Could you elaborate more on your concerns please?
Since Write_Status_Update_Request is address only request. Data length is 0. When I2C write request completes, reply for Write_Status_Update_Request from DPRx will be ACK only (i.e. data length is 0).

Quoted 8 lines

Is your concern about returning 0 from aux->transfer?
My thoughts is drm_dp_i2c_do_msg() is designed to handle I2C-Over-Aux reply data, and aux->transfer() is handling hw specific manipulation and return transferred bytes. For Write_Status_Update_Request request itself, nothing new to be transferred. I think drm_dp_i2c_do_msg() should be responsible for determining the correct transferred data bytes under this case. Or do you expect aux->transfer to memorize the data length of write request?

My concern is that I don't understand what the comment is trying to say.

"when i2c write firstly get defer" - what does it mean?

"wirte_status_update" - typo

"we have to" - why?

"return all data bytes get transferred" - what does it mean?

Quoted 10 lines

Thanks!
Quoted 6 lines
BR,
Jani.
--
Jani Nikula, Intel
--
Wayne Lin

Signature · 1 line

Jani Nikula, Intel

Lin, Wayne

author

2025-05-08 12:54 UTC #6 lore raw

[Public]

Quoted 51 lines

-----Original Message-----
From: Jani Nikula <[email protected]>
Sent: Thursday, May 8, 2025 8:16 PM
To: Lin, Wayne <[email protected]>; [email protected]
Cc: [email protected]; Limonciello, Mario <[email protected]>; Wentland, Harry <[email protected]>; [email protected]
Subject: RE: [PATCH] drm/dp: Fix Write_Status_Update_Request AUX request format
Quoted 30 lines · Wayne
[Public]
Quoted 11 lines
-----Original Message-----
From: Jani Nikula <[email protected]>
Sent: Thursday, May 8, 2025 4:19 PM
To: Lin, Wayne <[email protected]>; [email protected]
Cc: [email protected]; Limonciello, Mario <[email protected]>; Wentland, Harry <[email protected]>; Lin, Wayne <[email protected]>; [email protected]
Subject: Re: [PATCH] drm/dp: Fix Write_Status_Update_Request AUX request format
Quoted 8 lines
drivers/gpu/drm/display/drm_dp_helper.c static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg) · line 2054
Quoted 5 lines · Wayne Lin
drivers/gpu/drm/display/drm_dp_helper.c +5 -0
+                   /*
+                    * When I2C write firstly get defer and get ack after
+                    * retries by wirte_status_update, we have to return
+                    * all data bytes get transferred instead of 0.
+                    */
My brain gives me syntax and parse error here. ;)
Appreciate for the feedback, Jani.
Could you elaborate more on your concerns please?
Since Write_Status_Update_Request is address only request. Data length is 0. When I2C write request completes, reply for Write_Status_Update_Request from DPRx will be ACK only (i.e. data length is 0).
Quoted 8 lines
Is your concern about returning 0 from aux->transfer?
My thoughts is drm_dp_i2c_do_msg() is designed to handle I2C-Over-Aux reply data, and aux->transfer() is handling hw specific manipulation and return transferred bytes. For Write_Status_Update_Request request itself, nothing new to be transferred. I think drm_dp_i2c_do_msg() should be responsible for determining the correct transferred data bytes under this case. Or do you expect aux->transfer to memorize the data length of write request?
My concern is that I don't understand what the comment is trying to say.

Quoted 7 lines

"when i2c write firstly get defer" - what does it mean?
"wirte_status_update" - typo
"we have to" - why?
"return all data bytes get transferred" - what does it mean?

I see. Thanks!
We can't reply 0 since drm_dp_i2c_drain_msg() take 0 returned from drm_dp_i2c_do_msg() as error. And it actually completes transferring all data bytes.

I'll refine the comment. Thanks again.

Quoted 7 lines

Quoted 7 lines
Thanks!
Quoted 4 lines
BR,
Jani.

Quoted 7 lines

Quoted 4 lines
Quoted 2 lines
--
Jani Nikula, Intel
--
Wayne Lin
--
Jani Nikula, Intel

--
Wayne Lin