RE: [PATCH v2 4/4] drm/msm/disp/dpu1: add PSR support for eDP interface in dpu driver

[Vinod Polimera]

> -----Original Message-----
> From: Stephen Boyd <swboyd@xxxxxxxxxxxx>
> Sent: Wednesday, February 23, 2022 2:59 AM
> To: quic_vpolimer <quic_vpolimer@xxxxxxxxxxx>; agross@xxxxxxxxxx;
> airlied@xxxxxxxx; bjorn.andersson@xxxxxxxxxx; daniel@xxxxxxxx;
> devicetree@xxxxxxxxxxxxxxx; dianders@xxxxxxxxxxxx; dri-
> devel@xxxxxxxxxxxxxxxxxxxxx; freedreno@xxxxxxxxxxxxxxxxxxxxx;
> krzysztof.kozlowski@xxxxxxxxxxxxx; linux-arm-msm@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; robdclark@xxxxxxxxx; robh+dt@xxxxxxxxxx;
> sam@xxxxxxxxxxxx; seanpaul@xxxxxxxxxxxx; thierry.reding@xxxxxxxxx
> Cc: quic_kalyant <quic_kalyant@xxxxxxxxxxx>; Sankeerth Billakanti (QUIC)
> <quic_sbillaka@xxxxxxxxxxx>; quic_vproddut <quic_vproddut@xxxxxxxxxxx>
> Subject: Re: [PATCH v2 4/4] drm/msm/disp/dpu1: add PSR support for eDP
> interface in dpu driver
> 
> WARNING: This email originated from outside of Qualcomm. Please be wary
> of any links or attachments, and do not enable macros.
> 
> Quoting Vinod Polimera (2022-02-21 06:51:26)
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > index e7c9fe1..ba3240c 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > @@ -951,6 +952,14 @@ static void dpu_crtc_disable(struct drm_crtc *crtc,
> >
> >         DRM_DEBUG_KMS("crtc%d\n", crtc->base.id);
> >
> > +       if (old_crtc_state->self_refresh_active) {
> > +               drm_for_each_encoder_mask(encoder, crtc->dev,
> > +                                old_crtc_state->encoder_mask) {
> > +                       dpu_encoder_assign_crtc(encoder, NULL);
> > +               }
> > +               return;
> > +       }
> > +
> >         /* Disable/save vblank irq handling */
> >         drm_crtc_vblank_off(crtc);
> >
> > @@ -962,7 +971,12 @@ static void dpu_crtc_disable(struct drm_crtc *crtc,
> >                  */
> >                 if (dpu_encoder_get_intf_mode(encoder) == INTF_MODE_VIDEO)
> >                         release_bandwidth = true;
> > -               dpu_encoder_assign_crtc(encoder, NULL);
> > +               /* If disable is triggered during psr active(e.g: screen dim in PSR),
> 
> Multiline comments start with /* on a line by itself
> 
>                 /*
>                  * If disable is triggered ...
> 
> > +                * we will need encoder->crtc connection to process the device
> sleep &
> > +                * preserve it during psr sequence.
> > +                */
> > +               if (!crtc->state->self_refresh_active)
> > +                       dpu_encoder_assign_crtc(encoder, NULL);
> >         }
> >
> >         /* wait for frame_event_done completion */
> > @@ -1010,6 +1024,8 @@ static void dpu_crtc_enable(struct drm_crtc *crtc,
> >         struct dpu_crtc *dpu_crtc = to_dpu_crtc(crtc);
> >         struct drm_encoder *encoder;
> >         bool request_bandwidth = false;
> > +       struct drm_crtc_state *old_crtc_state =
> > +               drm_atomic_get_old_crtc_state(state, crtc);
> 
> Use two lines
> 
>         struct drm_crtc_state *old_crtc_state;
> 
>         old_crtc_state = drm_atomic_get_old_crtc_state(state, crtc);
> 
> >
> >         pm_runtime_get_sync(crtc->dev->dev);
> >
> > @@ -1032,8 +1048,10 @@ static void dpu_crtc_enable(struct drm_crtc
> *crtc,
> >         trace_dpu_crtc_enable(DRMID(crtc), true, dpu_crtc);
> >         dpu_crtc->enabled = true;
> >
> > -       drm_for_each_encoder_mask(encoder, crtc->dev, crtc->state-
> >encoder_mask)
> > -               dpu_encoder_assign_crtc(encoder, crtc);
> > +       if (!old_crtc_state->self_refresh_active) {
> > +               drm_for_each_encoder_mask(encoder, crtc->dev, crtc->state-
> >encoder_mask)
> > +                       dpu_encoder_assign_crtc(encoder, crtc);
> 
> Make these two lines above into a function with a meaningful name?
> dpu_encoder_assign_crtcs()? And then push the encoder mask iteration
> into the loop by passing the mask as a function argument. I see
> dpu_encoder_assign_crtc() takes a spinlock, so we could probably take
> that lock out too and push it into this new function to avoid grabbing
> and dropping the spinlock multiple times.
 enc spinlock is specific to each dpu_enc which will grab the lock for each crtc assign. 
> 
> > +       }
> >
> >         /* Enable/restore vblank irq handling */
> >         drm_crtc_vblank_on(crtc);
> > @@ -1497,7 +1515,7 @@ struct drm_crtc *dpu_crtc_init(struct drm_device
> *dev, struct drm_plane *plane,
> >  {
> >         struct drm_crtc *crtc = NULL;
> >         struct dpu_crtc *dpu_crtc = NULL;
> > -       int i;
> > +       int i, ret;
> >
> >         dpu_crtc = kzalloc(sizeof(*dpu_crtc), GFP_KERNEL);
> >         if (!dpu_crtc)
> > @@ -1534,6 +1552,11 @@ struct drm_crtc *dpu_crtc_init(struct
> drm_device *dev, struct drm_plane *plane,
> >         /* initialize event handling */
> >         spin_lock_init(&dpu_crtc->event_lock);
> >
> > +       ret = drm_self_refresh_helper_init(crtc);
> > +       if (ret)
> > +               DPU_ERROR("Failed to initialize %s with SR helpers %d\n",
> 
> What is SR? Write self-refresh?
> 
> > +                       crtc->name, ret);
> > +
> >         DRM_DEBUG_KMS("%s: successfully initialized crtc\n", dpu_crtc-
> >name);
> >         return crtc;
> >  }
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > index 6eac417..ba9d8ea 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > @@ -217,6 +217,14 @@ static u32 dither_matrix[DITHER_MATRIX_SZ] = {
> >         15, 7, 13, 5, 3, 11, 1, 9, 12, 4, 14, 6, 0, 8, 2, 10
> >  };
> >
> > +static inline bool is_self_refresh_active(struct drm_crtc_state *state)
> 
> const drm_crtc_state?
> 
> > +{
> > +       if (state && state->self_refresh_active)
> > +               return true;
> > +
> > +       return false;
> 
>         return state && state->self_refresh_active;
> 
> > +}
> > +
> >  static void _dpu_encoder_setup_dither(struct dpu_hw_pingpong
> *hw_pp, unsigned bpc)
> >  {
> >         struct dpu_hw_dither_cfg dither_cfg = { 0 };
> > @@ -629,7 +637,8 @@ static int dpu_encoder_virt_atomic_check(
> >                 if (drm_atomic_crtc_needs_modeset(crtc_state)) {
> >                         dpu_rm_release(global_state, drm_enc);
> >
> > -                       if (!crtc_state->active_changed || crtc_state->active)
> > +                       if (!crtc_state->active_changed || crtc_state->active ||
> > +                                       crtc_state->self_refresh_active)
> >                                 ret = dpu_rm_reserve(&dpu_kms->rm, global_state,
> >                                                 drm_enc, crtc_state, topology);
> >                 }
> > @@ -1182,11 +1191,30 @@ static void dpu_encoder_virt_disable(struct
> drm_encoder *drm_enc,
> >  {
> >         struct dpu_encoder_virt *dpu_enc = NULL;
> >         struct msm_drm_private *priv;
> > +       struct drm_crtc *crtc;
> > +       struct drm_crtc_state *old_state;
> >         int i = 0;
> >
> >         dpu_enc = to_dpu_encoder_virt(drm_enc);
> >         DPU_DEBUG_ENC(dpu_enc, "\n");
> 
> Presumably this print wants a valid 'dpu_enc' pointer.
> 
> >
> > +       if (!drm_enc) {
> 
> So this check for !drm_enc is impossible? Please remove it.
> 
> > +               DPU_ERROR("invalid encoder\n");
> > +               return;
> > +       }
> > +       dpu_enc = to_dpu_encoder_virt(drm_enc);
> 
> We got it again?
> 
> > +
> > +       crtc = dpu_enc->crtc;
> > +
> > +       old_state = drm_atomic_get_old_crtc_state(state, crtc);
> > +
> > +       /*
> > +        * The encoder turn off already occurred when self refresh mode
> 
> s/turn off/disable/
> 
> > +        * was set earlier, in the old_state for the corresponding crtc.
> > +        */
> > +       if (drm_enc->encoder_type == DRM_MODE_ENCODER_TMDS &&
> is_self_refresh_active(old_state))
> > +               return;
> > +
> >         mutex_lock(&dpu_enc->enc_lock);
> >         dpu_enc->enabled = false;
> >
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> > index 47fe11a..d550f90 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> > @@ -495,7 +495,7 @@ static void dpu_kms_wait_for_commit_done(struct
> msm_kms *kms,
> >                 return;
> >         }
> >
> > -       if (!crtc->state->active) {
> > +       if (!drm_atomic_crtc_effectively_active(crtc->state)) {
> >                 DPU_DEBUG("[crtc:%d] not active\n", crtc->base.id);
> >                 return;
> >         }
> > --
> > 2.7.4
> >