Re: [v1] drm/msm/dpu: Fix reservation failures in modeset

[Rob Clark]

On Wed, Aug 5, 2020 at 6:34 AM Kalyan Thota <kalyan_t@xxxxxxxxxxxxxx> wrote:
>
> In TEST_ONLY commit, rm global_state will duplicate the
> object and request for new reservations, once they pass
> then the new state will be swapped with the old and will
> be available for the Atomic Commit.
>
> This patch fixes some of missing links in the resource
> reservation sequence mentioned above.
>
> 1) Creation of a duplicate state in test_only commit (Rob)
> 2) Allow resource release only during crtc_active false.
>
> For #2
> In a modeset operation, swap state happens well before disable.
> Hence clearing reservations in disable will cause failures
> in modeset enable.
>
> Sequence:
>     Swap state --> old, new
>     modeset disables --> virt disable
>     modeset enable --> virt modeset
>
> Allow reservations to be cleared only when crtc active is false
> as in that case there wont be any modeset enable after disable.
>
> Signed-off-by: Kalyan Thota <kalyan_t@xxxxxxxxxxxxxx>
> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index 63976dc..b85a576 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -582,7 +582,7 @@ static int dpu_encoder_virt_atomic_check(
>         dpu_kms = to_dpu_kms(priv->kms);
>         mode = &crtc_state->mode;
>         adj_mode = &crtc_state->adjusted_mode;
> -       global_state = dpu_kms_get_existing_global_state(dpu_kms);
> +       global_state = dpu_kms_get_global_state(crtc_state->state);
>         trace_dpu_enc_atomic_check(DRMID(drm_enc));
>
>         /*
> @@ -1172,6 +1172,7 @@ static void dpu_encoder_virt_disable(struct drm_encoder *drm_enc)
>         struct msm_drm_private *priv;
>         struct dpu_kms *dpu_kms;
>         struct dpu_global_state *global_state;
> +       struct drm_crtc_state *crtc_state;
>         int i = 0;
>
>         if (!drm_enc) {
> @@ -1191,6 +1192,7 @@ static void dpu_encoder_virt_disable(struct drm_encoder *drm_enc)
>         priv = drm_enc->dev->dev_private;
>         dpu_kms = to_dpu_kms(priv->kms);
>         global_state = dpu_kms_get_existing_global_state(dpu_kms);
> +       crtc_state = drm_enc->crtc->state;
>
>         trace_dpu_enc_disable(DRMID(drm_enc));
>
> @@ -1220,7 +1222,8 @@ static void dpu_encoder_virt_disable(struct drm_encoder *drm_enc)
>
>         DPU_DEBUG_ENC(dpu_enc, "encoder disabled\n");
>
> -       dpu_rm_release(global_state, drm_enc);
> +       if (crtc_state->active_changed && !crtc_state->active)
> +               dpu_rm_release(global_state, drm_enc);

I still think releasing the state in the atomic_commit() path is the
wrong thing to do.  In the commit path, the various state objects
should be immutable.. ie. in the atomic_test() path you derive the new
hw state (including assignment/release of resources), and
atomic_commit() is simply pushing the state down to the hw.

Otherwise, this looks better than v1.

BR,
-R

>
>         mutex_unlock(&dpu_enc->enc_lock);
>  }
> --
> 1.9.1
>