Re: [PATCH v3] drm/msm/dp: update is_connected status base on sink count at dp_pm_resume()

[khsieh@xxxxxxxxxxxxxx]

On 2021-08-03 12:05, Stephen Boyd wrote:
> Quoting Kuogee Hsieh (2021-08-03 09:25:13)
> > Currently at dp_pm_resume() is_connected state is decided base on hpd 
> > connection
> > status only. This will put is_connected in wrongly "true" state at the 
> > scenario
> > that dongle attached to DUT but without hmdi cable connecting to it. 
> > Fix this
> > problem by adding read sink count from dongle and decided is_connected 
> > state base
> > on both sink count and hpd connection status.
> >
> > Changes in v2:
> > -- remove dp_get_sink_count() cand call drm_dp_read_sink_count()
> >
> > Changes in v3:
> > -- delete status local variable from dp_pm_resume()
> >
> > Fixes: d9aa6571b28ba ("drm/msm/dp: check sink_count before update 
> > is_connected status")
> > Signed-off-by: Kuogee Hsieh <khsieh@xxxxxxxxxxxxxx>
> > ---
> >  drivers/gpu/drm/msm/dp/dp_display.c | 18 +++++++++++++++---
> >  1 file changed, 15 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c 
> > b/drivers/gpu/drm/msm/dp/dp_display.c
> > index 78c5301..0f39256 100644
> > --- a/drivers/gpu/drm/msm/dp/dp_display.c
> > +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> > @@ -1313,7 +1313,7 @@ static int dp_pm_resume(struct device *dev)
> >         struct platform_device *pdev = to_platform_device(dev);
> >         struct msm_dp *dp_display = platform_get_drvdata(pdev);
> >         struct dp_display_private *dp;
> > -       u32 status;
> > +       int sink_count = 0;
> >
> >         dp = container_of(dp_display, struct dp_display_private, 
> > dp_display);
> >
> > @@ -1327,14 +1327,26 @@ static int dp_pm_resume(struct device *dev)
> >
> >         dp_catalog_ctrl_hpd_config(dp->catalog);
> >
> > -       status = dp_catalog_link_is_connected(dp->catalog);
> > +       /*
> > +        * set sink to normal operation mode -- D0
> > +        * before dpcd read
> > +        */
> > +       dp_link_psm_config(dp->link, &dp->panel->link_info, false);
> > +
> > +       /* if sink conencted, do dpcd read sink count */
>
> s/conencted/connected/
>
> This also just says what the code is doing. Why do we only read the 
> sink
> count if the link is connected? Can we read the sink count even if the
> link isn't connected and then consider sink count as 0 if trying to 
> read
> fails?
yes, we can do that.
But it will suffer aux time out and retry.
i think it is better to avoid this overhead by check connection first.

> > +       if (dp_catalog_link_is_connected(dp->catalog)) {
> > +               sink_count = drm_dp_read_sink_count(dp->aux);
> > +               if (sink_count < 0)
> > +                       sink_count = 0;
> > +       }
> >
> > +       dp->link->sink_count = sink_count;
> >         /*
> >          * can not declared display is connected unless
> >          * HDMI cable is plugged in and sink_count of
> >          * dongle become 1
> >          */
> > -       if (status && dp->link->sink_count)
> > +       if (dp->link->sink_count)
> >                 dp->dp_display.is_connected = true;
> >         else
> >                 dp->dp_display.is_connected = false;