> -----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
> >
