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

[kalyan_t]

On 2020-08-06 22:15, Rob Clark wrote:
> On Thu, Aug 6, 2020 at 7:46 AM <kalyan_t@xxxxxxxxxxxxxx> wrote:
> > 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 ?
>
> Yeah, I suppose something like:
>
>    if (drm_atomic_crtc_needs_modeset()) {
>       reserve()
>    } else if (active_changed && !active) {
>       release()
>    }
>
> I think it could happen (at least with atomic api) that you get a
> modeset without active_changed, so we might need to release() and then
> reserve() in that case?  (This is probably where starting to run more
> IGT tests would be useful)
>
> BR,
> -R
> Thanks Rob, please review the v2 version.
> > -Kalyan
> > >>
> > >>         mutex_unlock(&dpu_enc->enc_lock);
> > >>  }
> > >> --
> > >> 1.9.1
> > >>
> > > _______________________________________________
> > > Freedreno mailing list
> > > Freedreno@xxxxxxxxxxxxxxxxxxxxx
> > > https://lists.freedesktop.org/mailman/listinfo/freedreno