RE: [PATCH Resend v11 05/15] drm/msm/dp: disable self_refresh_aware after entering psr

[Vinod Polimera]

> -----Original Message-----
> From: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
> Sent: Tuesday, January 24, 2023 5:52 AM
> To: Vinod Polimera (QUIC) <quic_vpolimer@xxxxxxxxxxx>; dri-
> devel@xxxxxxxxxxxxxxxxxxxxx; linux-arm-msm@xxxxxxxxxxxxxxx;
> freedreno@xxxxxxxxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx
> Cc: Sankeerth Billakanti (QUIC) <quic_sbillaka@xxxxxxxxxxx>; linux-
> kernel@xxxxxxxxxxxxxxx; robdclark@xxxxxxxxx; dianders@xxxxxxxxxxxx;
> swboyd@xxxxxxxxxxxx; Kalyan Thota (QUIC) <quic_kalyant@xxxxxxxxxxx>;
> Kuogee Hsieh (QUIC) <quic_khsieh@xxxxxxxxxxx>; Vishnuvardhan
> Prodduturi (QUIC) <quic_vproddut@xxxxxxxxxxx>; Bjorn Andersson (QUIC)
> <quic_bjorande@xxxxxxxxxxx>; Abhinav Kumar (QUIC)
> <quic_abhinavk@xxxxxxxxxxx>
> Subject: Re: [PATCH Resend v11 05/15] drm/msm/dp: disable
> self_refresh_aware after entering psr
> 
> WARNING: This email originated from outside of Qualcomm. Please be wary
> of any links or attachments, and do not enable macros.
> 
> On 19/01/2023 16:26, Vinod Polimera wrote:
> > From: Sankeerth Billakanti <quic_sbillaka@xxxxxxxxxxx>
> >
> > Updated frames get queued if self_refresh_aware is set when the
> > sink is in psr. To support bridge enable and avoid queuing of update
> > frames, reset the self_refresh_aware state after entering psr.
> 
> I'm not convinced by this change. E.g. analogix code doesn't do this.
> Could you please clarify, why do you need to toggle the
> self_refresh_aware flag?
> 
This was done to fix a bug reported by google. The use case is as follows:
	CPU was running in a low frequency with debug build.
	When self refresh was triggered by the library, due to system latency, the queued work was not scheduled.
	There in another commit came and reinitialized the timer for the next PSR trigger.
	This sequence happened multiple times  and we found there were multiple works which are stuck and yet to be run.
	As PSR trigger is guarded by self_refresh_aware, we initialized the variable such that, if we are in PSR then until PSR exit, there cannot be any further PSR entry again.
		https://chromium.googlesource.com/chromiumos/third_party/kernel/+/refs/tags/v5.15.90/drivers/gpu/drm/drm_self_refresh_helper.c#105
	This has solved few flicker issues during the stress testing. 
> >
> > Signed-off-by: Sankeerth Billakanti <quic_sbillaka@xxxxxxxxxxx>
> > Signed-off-by: Vinod Polimera <quic_vpolimer@xxxxxxxxxxx>
> > ---
> >   drivers/gpu/drm/msm/dp/dp_drm.c | 27
> ++++++++++++++++++++++++++-
> >   1 file changed, 26 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/msm/dp/dp_drm.c
> b/drivers/gpu/drm/msm/dp/dp_drm.c
> > index 029e08c..92d1a1b 100644
> > --- a/drivers/gpu/drm/msm/dp/dp_drm.c
> > +++ b/drivers/gpu/drm/msm/dp/dp_drm.c
> > @@ -134,6 +134,8 @@ static void edp_bridge_atomic_enable(struct
> drm_bridge *drm_bridge,
> >       struct drm_crtc_state *old_crtc_state;
> >       struct msm_dp_bridge *dp_bridge = to_dp_bridge(drm_bridge);
> >       struct msm_dp *dp = dp_bridge->dp_display;
> > +     struct drm_connector *connector;
> > +     struct drm_connector_state *conn_state = NULL;
> >
> >       /*
> >        * Check the old state of the crtc to determine if the panel
> > @@ -150,10 +152,22 @@ static void edp_bridge_atomic_enable(struct
> drm_bridge *drm_bridge,
> >
> >       if (old_crtc_state && old_crtc_state->self_refresh_active) {
> >               dp_display_set_psr(dp, false);
> > -             return;
> > +             goto psr_aware;
> >       }
> >
> >       dp_bridge_atomic_enable(drm_bridge, old_bridge_state);
> > +
> > +psr_aware:
> > +     connector =
> drm_atomic_get_new_connector_for_encoder(atomic_state,
> > +                                                     drm_bridge->encoder);
> > +     if (connector)
> > +             conn_state =
> drm_atomic_get_new_connector_state(atomic_state,
> > +                                                             connector);
> > +
> > +     if (conn_state) {
> > +             conn_state->self_refresh_aware = dp->psr_supported;
> > +     }
> 
> No need to wrap a single line statement in brackets.
> 
> > +
> >   }
> >
> >   static void edp_bridge_atomic_disable(struct drm_bridge *drm_bridge,
> > @@ -164,6 +178,14 @@ static void edp_bridge_atomic_disable(struct
> drm_bridge *drm_bridge,
> >       struct drm_crtc_state *new_crtc_state = NULL, *old_crtc_state = NULL;
> >       struct msm_dp_bridge *dp_bridge = to_dp_bridge(drm_bridge);
> >       struct msm_dp *dp = dp_bridge->dp_display;
> > +     struct drm_connector *connector;
> > +     struct drm_connector_state *conn_state = NULL;
> > +
> > +     connector =
> drm_atomic_get_old_connector_for_encoder(atomic_state,
> > +                                                     drm_bridge->encoder);
> > +     if (connector)
> > +             conn_state =
> drm_atomic_get_new_connector_state(atomic_state,
> > +                                                             connector);
> >
> >       crtc = drm_atomic_get_old_crtc_for_encoder(atomic_state,
> >                                                  drm_bridge->encoder);
> > @@ -190,6 +212,9 @@ static void edp_bridge_atomic_disable(struct
> drm_bridge *drm_bridge,
> >        * when display disable occurs while the sink is in psr state.
> >        */
> >       if (new_crtc_state->self_refresh_active) {
> > +             if (conn_state)
> > +                     conn_state->self_refresh_aware = false;
> > +
> >               dp_display_set_psr(dp, true);
> >               return;
> >       } else if (old_crtc_state->self_refresh_active) {
> 
> --
> With best wishes
> Dmitry

Thanks,
Vinod P.