[AMD Official Use Only - AMD Internal Distribution Only]
Thanks for the feedback! I'll adjust it and give v2 soon.
Thanks, Wayne
-----Original Message----- From: Limonciello, Mario Mario.Limonciello@amd.com Sent: Tuesday, May 6, 2025 4:22 AM To: Lin, Wayne Wayne.Lin@amd.com; dri-devel@lists.freedesktop.org Cc: ville.syrjala@linux.intel.com; jani.nikula@intel.com; Wentland, Harry Harry.Wentland@amd.com; stable@vger.kernel.org Subject: Re: [PATCH] drm/dp: Fix Write_Status_Update_Request AUX request format
On 4/27/2025 4:50 AM, Wayne Lin wrote:
[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ä ville.syrjala@linux.intel.com Cc: Jani Nikula jani.nikula@intel.com Cc: Mario Limonciello mario.limonciello@amd.com Cc: Harry Wentland harry.wentland@amd.com Cc: stable@vger.kernel.org Signed-off-by: Wayne Lin Wayne.Lin@amd.com
drivers/gpu/drm/display/drm_dp_helper.c | 45 +++++++++++++++++++++---- 1 file changed, 38 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/display/drm_dp_helper.c b/drivers/gpu/drm/display/drm_dp_helper.c index 57828f2b7b5a..0c8cba7ed875 100644 --- a/drivers/gpu/drm/display/drm_dp_helper.c +++ b/drivers/gpu/drm/display/drm_dp_helper.c @@ -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 ==
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*/
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;
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,