* [PATCH 1/2] drm/dp: Correct Write_Status_Update_Request handling [not found] <20250424030734.873693-1-Wayne.Lin@amd.com> @ 2025-04-24 3:07 ` Wayne Lin 2025-04-25 14:47 ` Ville Syrjälä 2025-04-24 3:07 ` [PATCH 2/2] drm/dp: Add handling for partially read under I2-readC-over-AUX Wayne Lin 1 sibling, 1 reply; 4+ messages in thread From: Wayne Lin @ 2025-04-24 3:07 UTC (permalink / raw) To: dri-devel Cc: ville.syrjala, jani.nikula, mario.limonciello, harry.wentland, Wayne Lin, stable [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 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH 1/2] drm/dp: Correct Write_Status_Update_Request handling 2025-04-24 3:07 ` [PATCH 1/2] drm/dp: Correct Write_Status_Update_Request handling Wayne Lin @ 2025-04-25 14:47 ` Ville Syrjälä 2025-04-27 9:47 ` Lin, Wayne 0 siblings, 1 reply; 4+ messages in thread From: Ville Syrjälä @ 2025-04-25 14:47 UTC (permalink / raw) To: Wayne Lin Cc: dri-devel, jani.nikula, mario.limonciello, harry.wentland, stable 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* RE: [PATCH 1/2] drm/dp: Correct Write_Status_Update_Request handling 2025-04-25 14:47 ` Ville Syrjälä @ 2025-04-27 9:47 ` Lin, Wayne 0 siblings, 0 replies; 4+ messages in thread From: Lin, Wayne @ 2025-04-27 9:47 UTC (permalink / raw) To: Ville Syrjälä Cc: dri-devel@lists.freedesktop.org, jani.nikula@intel.com, Limonciello, Mario, Wentland, Harry, stable@vger.kernel.org [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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH 2/2] drm/dp: Add handling for partially read under I2-readC-over-AUX [not found] <20250424030734.873693-1-Wayne.Lin@amd.com> 2025-04-24 3:07 ` [PATCH 1/2] drm/dp: Correct Write_Status_Update_Request handling Wayne Lin @ 2025-04-24 3:07 ` Wayne Lin 1 sibling, 0 replies; 4+ messages in thread From: Wayne Lin @ 2025-04-24 3:07 UTC (permalink / raw) To: dri-devel Cc: ville.syrjala, jani.nikula, mario.limonciello, harry.wentland, Wayne Lin, stable [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 ^ permalink raw reply related [flat|nested] 4+ messages in thread
end of thread, other threads:[~2025-04-27 9:48 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20250424030734.873693-1-Wayne.Lin@amd.com>
2025-04-24 3:07 ` [PATCH 1/2] drm/dp: Correct Write_Status_Update_Request handling Wayne Lin
2025-04-25 14:47 ` Ville Syrjälä
2025-04-27 9:47 ` Lin, Wayne
2025-04-24 3:07 ` [PATCH 2/2] drm/dp: Add handling for partially read under I2-readC-over-AUX Wayne Lin
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox