Re: [PATCH] drm/mediatek: dp: change the aux retries times when receiving AUX_DEFER

[Xinlei Lee (李昕磊)]

On Mon, 2023-04-03 at 11:49 +0800, Chun-Kuang Hu wrote:
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> Hi, Xinlei:
> 
> <xinlei.lee@xxxxxxxxxxxx> 於 2023年3月29日 週三 下午2:43寫道：
> > 
> > From: Xinlei Lee <xinlei.lee@xxxxxxxxxxxx>
> > 
> > DP 1.4a Section 2.8.7.1.5.6.1:
> > A DP Source device shall retry at least seven times upon receiving
> > AUX_DEFER before giving up the AUX transaction.
> > 
> > The drm_dp_i2c_do_msg() function in the drm_dp_helper.c file will
> > judge the status of the msg->reply parameter passed to aux_transfer
> > ange-the-aux-retries-times-when-re.patchfor different processing.
> > 
> > Fixes: f70ac097a2cf ("drm/mediatek: Add MT8195 Embedded DisplayPort
> > driver")
> > Signed-off-by: Xinlei Lee <xinlei.lee@xxxxxxxxxxxx>
> > ---
> >  drivers/gpu/drm/mediatek/mtk_dp.c | 12 +++++-------
> >  1 file changed, 5 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/mediatek/mtk_dp.c
> > b/drivers/gpu/drm/mediatek/mtk_dp.c
> > index 1f94fcc144d3..767b71da31a4 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_dp.c
> > +++ b/drivers/gpu/drm/mediatek/mtk_dp.c
> > @@ -806,10 +806,9 @@ static int
> > mtk_dp_aux_wait_for_completion(struct mtk_dp *mtk_dp, bool is_read)
> >  }
> > 
> >  static int mtk_dp_aux_do_transfer(struct mtk_dp *mtk_dp, bool
> > is_read, u8 cmd,
> > -                                 u32 addr, u8 *buf, size_t length)
> > +                                 u32 addr, u8 *buf, size_t length,
> > u8 *reply_cmd)
> >  {
> >         int ret;
> > -       u32 reply_cmd;
> > 
> >         if (is_read && (length > DP_AUX_MAX_PAYLOAD_BYTES ||
> >                         (cmd == DP_AUX_NATIVE_READ && !length)))
> > @@ -841,10 +840,10 @@ static int mtk_dp_aux_do_transfer(struct
> > mtk_dp *mtk_dp, bool is_read, u8 cmd,
> >         /* Wait for feedback from sink device. */
> >         ret = mtk_dp_aux_wait_for_completion(mtk_dp, is_read);
> > 
> > -       reply_cmd = mtk_dp_read(mtk_dp, MTK_DP_AUX_P0_3624) &
> > -                   AUX_RX_REPLY_COMMAND_AUX_TX_P0_MASK;
> > +       *reply_cmd = mtk_dp_read(mtk_dp, MTK_DP_AUX_P0_3624) &
> > +                    AUX_RX_REPLY_COMMAND_AUX_TX_P0_MASK;
> > 
> > -       if (ret || reply_cmd) {
> > +       if (ret) {
> >                 u32 phy_status = mtk_dp_read(mtk_dp,
> > MTK_DP_AUX_P0_3628) &
> >                                  AUX_RX_PHY_STATE_AUX_TX_P0_MASK;
> >                 if (phy_status !=
> > AUX_RX_PHY_STATE_AUX_TX_P0_RX_IDLE) {
> > @@ -2070,7 +2069,7 @@ static ssize_t mtk_dp_aux_transfer(struct
> > drm_dp_aux *mtk_aux,
> >                 ret = mtk_dp_aux_do_transfer(mtk_dp, is_read,
> > request,
> >                                              msg->address +
> > accessed_bytes,
> >                                              msg->buffer +
> > accessed_bytes,
> > -                                            to_access);
> > +                                            to_access, &msg-
> > >reply);
> > 
> >                 if (ret) {
> >                         drm_info(mtk_dp->drm_dev,
> > @@ -2080,7 +2079,6 @@ static ssize_t mtk_dp_aux_transfer(struct
> > drm_dp_aux *mtk_aux,
> >                 accessed_bytes += to_access;
> >         } while (accessed_bytes < msg->size);
> > 
> > -       msg->reply = DP_AUX_NATIVE_REPLY_ACK |
> > DP_AUX_I2C_REPLY_ACK;
> 
> In your description, you just mention the retry count is 7 times, but
> you does not mention you should change the reply. Why you modify
> this?
> And where is the 7 times retry?
> 
> Regards,
> Chun-Kuang.
> 
> >         return msg->size;
> >  err:
> >         msg->reply = DP_AUX_NATIVE_REPLY_NACK |
> > DP_AUX_I2C_REPLY_NACK;
> > --
> > 2.18.0
> > 

Hi CK:

Thanks for your review!

This patch is to fix some DP sinks that return AUX_DEFER, and the dp 
driver does not handle it according to the specification. DP_v1.4a
spec 2.8.1.2 describes that if the sink returns AUX_DEFER, DPTX may 
retry later:

The logic before the modification is that reply_cmd returns ETIMEDOUT 
if it is not AUX_ACK after the read operation, without considering the 
retry operation when returning AUX_DEFER;

The modified logic is to add parameters to mtk_dp_aux_do_transfer() to 
store the return value of the sink. In the dmr_dp_helper.c file, 
drm_dp_i2c_do_msg calls aux->transfer and then performs retry
operation according to msg->reply. The 7 times specified in the spec 
are also in this function defined in (max_retries).

Best Regards!
xinlei