* [PATCH v2] drm/dp: Fix Write_Status_Update_Request AUX request format
@ 2025-05-09 9:22 Wayne Lin
2025-05-09 16:27 ` Mario Limonciello
0 siblings, 1 reply; 2+ messages in thread
From: Wayne Lin @ 2025-05-09 9:22 UTC (permalink / raw)
To: dri-devel
Cc: ville.syrjala, jani.nikula, mario.limonciello, harry.wentland,
Wayne Lin, stable
[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.
V2:
- Add checking condition before restoring msg->buffer and
msg->size. (Limonciello Mario)
- Revise unclear comment to appropriately describe the idea.
(Jani Nikula)
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 | 54 +++++++++++++++++++++----
1 file changed, 47 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..c71a1395a2d6 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,41 @@ 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 DPTx sets Write_Status_Update_Request flag to
+ * ask DPRx for the write status, the AUX reply from
+ * DPRx will be I2C_ACK|AUX_ACK if I2C write request
+ * completes successfully. Such AUX transaction is for
+ * status checking only, so no new data is written by
+ * aux->transfer(). In this case, here we have to
+ * report all original data get written. Otherwise,
+ * drm_dp_i2c_drain_msg() takes returned value 0 as
+ * an error.
+ */
+ 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 +2094,21 @@ 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 case*/
+ if (drm_dp_i2c_msg_is_write_status_update(msg)) {
+ 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,
--
2.43.0
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH v2] drm/dp: Fix Write_Status_Update_Request AUX request format
2025-05-09 9:22 [PATCH v2] drm/dp: Fix Write_Status_Update_Request AUX request format Wayne Lin
@ 2025-05-09 16:27 ` Mario Limonciello
0 siblings, 0 replies; 2+ messages in thread
From: Mario Limonciello @ 2025-05-09 16:27 UTC (permalink / raw)
To: Wayne Lin, dri-devel; +Cc: ville.syrjala, jani.nikula, harry.wentland, stable
On 5/9/2025 4:22 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.
>
> V2:
> - Add checking condition before restoring msg->buffer and
> msg->size. (Limonciello Mario)
> - Revise unclear comment to appropriately describe the idea.
> (Jani Nikula)
>
> 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>
> ---
Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
One nit below.
> drivers/gpu/drm/display/drm_dp_helper.c | 54 +++++++++++++++++++++----
> 1 file changed, 47 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..c71a1395a2d6 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,41 @@ 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 DPTx sets Write_Status_Update_Request flag to
> + * ask DPRx for the write status, the AUX reply from
> + * DPRx will be I2C_ACK|AUX_ACK if I2C write request
> + * completes successfully. Such AUX transaction is for
> + * status checking only, so no new data is written by
> + * aux->transfer(). In this case, here we have to
> + * report all original data get written. Otherwise,
> + * drm_dp_i2c_drain_msg() takes returned value 0 as
> + * an error.
> + */
> + 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 +2094,21 @@ 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 case*/
This comment feels unnecessary given the name of the function..
> + if (drm_dp_i2c_msg_is_write_status_update(msg)) {
> + 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,
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2025-05-09 16:27 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-05-09 9:22 [PATCH v2] drm/dp: Fix Write_Status_Update_Request AUX request format Wayne Lin
2025-05-09 16:27 ` Mario Limonciello
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox