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

[Vinod Polimera]

> -----Original Message-----
> From: Dmitry Baryshkov <dmitry.baryshkov at linaro.org>
> Sent: Tuesday, January 24, 2023 5:52 AM
> To: Vinod Polimera (QUIC) <quic_vpolimer at quicinc.com>; dri-
> devel at lists.freedesktop.org; linux-arm-msm at vger.kernel.org;
> freedreno at lists.freedesktop.org; devicetree at vger.kernel.org
> Cc: Sankeerth Billakanti (QUIC) <quic_sbillaka at quicinc.com>; linux-
> kernel at vger.kernel.org; robdclark at gmail.com; dianders at chromium.org;
> swboyd at chromium.org; Kalyan Thota (QUIC) <quic_kalyant at quicinc.com>;
> Kuogee Hsieh (QUIC) <quic_khsieh at quicinc.com>; Vishnuvardhan
> Prodduturi (QUIC) <quic_vproddut at quicinc.com>; Bjorn Andersson (QUIC)
> <quic_bjorande at quicinc.com>; Abhinav Kumar (QUIC)
> <quic_abhinavk at quicinc.com>
> 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 at quicinc.com>
> >
> > 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 at quicinc.com>
> > Signed-off-by: Vinod Polimera <quic_vpolimer at quicinc.com>
> > ---
> >   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.