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.syrj...@linux.intel.com>
Cc: Jani Nikula <jani.nik...@intel.com>
Cc: Mario Limonciello <mario.limoncie...@amd.com>
Cc: Harry Wentland <harry.wentl...@amd.com>
Cc: sta...@vger.kernel.org
Signed-off-by: Wayne Lin <wayne....@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 
== 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*/

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,

Reply via email to