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

[Stephen Boyd <swboyd@chromium.org>]

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.

> +       }
>
>         /* 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
>