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

[kalyan_t]

On 2020-08-05 21:18, Rob Clark wrote:
> 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
okay. Should we avoid reservation all together if active=0 on that crtc 
and trigger rm_release on the enc during atomic_check ?
how do you see the approach ?

-Kalyan
> >         mutex_unlock(&dpu_enc->enc_lock);
> >  }
> > --
> > 1.9.1
> _______________________________________________
> Freedreno mailing list
> Freedreno@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/freedreno