[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 at linux.intel.com>
> Sent: Friday, April 25, 2025 10:48 PM
> To: Lin, Wayne <Wayne.Lin at amd.com>
> Cc: dri-devel at lists.freedesktop.org; jani.nikula at intel.com; Limonciello, Mario
> <Mario.Limonciello at amd.com>; Wentland, Harry <Harry.Wentland at amd.com>;
> stable at 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 at linux.intel.com>
> > Cc: Jani Nikula <jani.nikula at intel.com>
> > Cc: Mario Limonciello <mario.limonciello at amd.com>
> > Cc: Harry Wentland <harry.wentland at amd.com>
> > Cc: stable at vger.kernel.org
> > Signed-off-by: Wayne Lin <Wayne.Lin at 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