[PATCH 1/2] drm/dp: Correct Write_Status_Update_Request handling

[PATCH 1/2] drm/dp: Correct Write_Status_Update_Request handling

[Wayne Lin]

[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

- 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.

- 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.

[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.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 | 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

[PATCH 2/2] drm/dp: Add handling for partially read under I2-readC-over-AUX

[Wayne Lin]

[Why]
There is no handling for I2C-read-over-AUX when receive reply of
I2C_ACK|AUX_ACK followed by the total number of data bytes Fewer
than LEN + 1

[How]
Refer to DP v2.1: 2.11.7.1.6.3 & 2.11.7.1.6.4, repeat the identical
I2C-read-over-AUX transaction with the updated LEN value equal to
the original LEN value minus the total number of data bytes received
so far.

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 | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/display/drm_dp_helper.c b/drivers/gpu/drm/display/drm_dp_helper.c
index 28f0708c3b27..938214a980a9 100644
--- a/drivers/gpu/drm/display/drm_dp_helper.c
+++ b/drivers/gpu/drm/display/drm_dp_helper.c
@@ -1812,10 +1812,11 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
 				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);
+				usleep_range(AUX_RETRY_INTERVAL, AUX_RETRY_INTERVAL + 100);
+				if (msg->request & DP_AUX_I2C_READ)
+					msg->size -= ret;
+				else
 					drm_dp_i2c_msg_write_status_update(msg);
-				}
 
 				continue;
 			}
-- 
2.43.0

Re: [PATCH 1/2] drm/dp: Correct Write_Status_Update_Request handling

[Ville Syrjälä]

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.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 | 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

RE: [PATCH 1/2] drm/dp: Correct Write_Status_Update_Request handling

[Lin, Wayne]

[Public]

Thanks for you time Ville Syrjälä !
And like what you pointed out here, sorry I realized that I misread the code
flow just after I sent it out. Will have another patch for fixing the aux request format
only. Thanks!

Regards,
Wayne

> -----Original Message-----
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Sent: Friday, April 25, 2025 10:48 PM
> To: Lin, Wayne <Wayne.Lin@amd.com>
> Cc: dri-devel@lists.freedesktop.org; jani.nikula@intel.com; Limonciello, Mario
> <Mario.Limonciello@amd.com>; Wentland, Harry <Harry.Wentland@amd.com>;
> stable@vger.kernel.org
> Subject: Re: [PATCH 1/2] drm/dp: Correct Write_Status_Update_Request handling
>
> 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.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 | 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