On Thu, Apr 24, 2025 at 11:07:33AM +0800, Wayne Lin wrote:
> [Why]
> Notice few problems under I2C-write-over-Aux with
> Write_Status_Update_Request flag set cases:
>
> - I2C-write-over-Aux request with
> Write_Status_Update_Request flag set won't get sent
> upon the reply of I2C_ACK|AUX_ACK followed by “M”
> Value. Now just set the flag but won't send out
drm_dp_i2c_drain_msg() should keep going until an error
or the full message got transferred.
>
> - The I2C-over-Aux request command with
> Write_Status_Update_Request flag set is incorrect.
> Should be "SYNC->COM3:0 (= 0110)|0000-> 0000|0000->
> 0|7-bit I2C address (the same as the last)-> STOP->".
> Address only transaction without length and data.
This looks like a real issue.
>
> - Upon I2C_DEFER|AUX_ACK Reply for I2C-read-over-Aux,
> soure should repeat the identical I2C-read-over-AUX
> transaction with the same LEN value. Not with
> Write_Status_Update_Request set.
drm_dp_i2c_msg_write_status_update() already
checks the request type.
>
> [How]
> Refer to DP v2.1: 2.11.7.1.5.3 & 2.11.7.1.5.4
> - Clean aux msg buffer and size when constructing
> write status update request.
>
> - Send out Write_Status_Update_Request upon reply of
> I2C_ACK|AUX_ACK followed by “M”
>
> - Send Write_Status_Update_Request upon I2C_DEFER|AUX_ACK
> reply only when the request is I2C-write-over-Aux.
>
> 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 | 27 +++++++++++++++++++++----
> 1 file changed, 23 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/display/drm_dp_helper.c
> b/drivers/gpu/drm/display/drm_dp_helper.c
> index 6ee51003de3c..28f0708c3b27 100644
> --- a/drivers/gpu/drm/display/drm_dp_helper.c
> +++ b/drivers/gpu/drm/display/drm_dp_helper.c
> @@ -1631,6 +1631,12 @@ static void drm_dp_i2c_msg_write_status_update(struct
> drm_dp_aux_msg *msg)
> msg->request &= DP_AUX_I2C_MOT;
> msg->request |= DP_AUX_I2C_WRITE_STATUS_UPDATE;
> }
> +
> + /*
> + * Address only transaction
> + */
> + msg->buffer = NULL;
> + msg->size = 0;
> }
>
> #define AUX_PRECHARGE_LEN 10 /* 10 to 16 */
> @@ -1797,10 +1803,22 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux,
> struct drm_dp_aux_msg *msg)
> case DP_AUX_I2C_REPLY_ACK:
> /*
> * Both native ACK and I2C ACK replies received. We
> - * can assume the transfer was successful.
> + * can't assume the transfer was completed. Both I2C
> + * WRITE/READ request may get I2C ack reply with
> partially
> + * completion. We have to continue to poll for the
> + * completion of the request.
> */
> - if (ret != msg->size)
> - drm_dp_i2c_msg_write_status_update(msg);
> + if (ret != msg->size) {
> + drm_dbg_kms(aux->drm_dev,
> + "%s: I2C partially ack (result=%d,
> size=%zu)\n",
> + aux->name, ret, msg->size);
> + if (!(msg->request & DP_AUX_I2C_READ)) {
> + usleep_range(AUX_RETRY_INTERVAL,
> AUX_RETRY_INTERVAL + 100);
> + drm_dp_i2c_msg_write_status_update(msg);
> + }
> +
> + continue;
> + }
> return ret;
>
> case DP_AUX_I2C_REPLY_NACK:
> @@ -1819,7 +1837,8 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux,
> struct drm_dp_aux_msg *msg)
> if (defer_i2c < 7)
> defer_i2c++;
> usleep_range(AUX_RETRY_INTERVAL, AUX_RETRY_INTERVAL +
> 100);
> - drm_dp_i2c_msg_write_status_update(msg);
> + if (!(msg->request & DP_AUX_I2C_READ))
> + drm_dp_i2c_msg_write_status_update(msg);
>
> continue;
>
> --
> 2.43.0
--
Ville Syrjälä
Intel
