[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.syrj...@linux.intel.com>
> Sent: Friday, April 25, 2025 10:48 PM
> To: Lin, Wayne <wayne....@amd.com>
> Cc: dri-devel@lists.freedesktop.org; jani.nik...@intel.com; Limonciello, Mario
> <mario.limoncie...@amd.com>; Wentland, Harry <harry.wentl...@amd.com>;
> sta...@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.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
