[AMD Official Use Only - AMD Internal Distribution Only]
Thanks for the feedback! I'll adjust it and give v2 soon.
Thanks,
Wayne
> -----Original Message-----
> From: Limonciello, Mario <mario.limoncie...@amd.com>
> Sent: Tuesday, May 6, 2025 4:22 AM
> To: Lin, Wayne <wayne....@amd.com>; dri-devel@lists.freedesktop.org
> Cc: ville.syrj...@linux.intel.com; jani.nik...@intel.com; Wentland, Harry
> <harry.wentl...@amd.com>; sta...@vger.kernel.org
> Subject: Re: [PATCH] drm/dp: Fix Write_Status_Update_Request AUX request
> format
>
> 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,
