Re: Reboot crash at msm_atomic_commit_tail

[Rob Clark <robdclark@gmail.com>]

forgot to CC Krishna

On Tue, Jan 19, 2021 at 8:34 AM Rob Clark <robdclark@gmail.com> wrote:
>
> On Mon, Jan 18, 2021 at 11:00 PM Daniel Vetter <daniel@ffwll.ch> wrote:
> >
> > On Mon, Jan 18, 2021 at 11:00 PM Fabio Estevam <festevam@gmail.com> wrote:
> > >
> > > On Mon, Jan 18, 2021 at 6:44 PM Fabio Estevam <festevam@gmail.com> wrote:
> > > >
> > > > Adding some more folks in case anyone has any suggestions to fix this
> > > > reboot hang.
> > >
> > > Not sure if this is a valid fix, but the change below makes reboot
> > > works correctly.
> > >
> > > kmscube still works.
> > >
> > > --- a/drivers/gpu/drm/msm/msm_atomic.c
> > > +++ b/drivers/gpu/drm/msm/msm_atomic.c
> > > @@ -207,8 +207,12 @@ void msm_atomic_commit_tail(struct drm_atomic_state *state)
> > >         struct msm_kms *kms = priv->kms;
> > >         struct drm_crtc *async_crtc = NULL;
> > >         unsigned crtc_mask = get_crtc_mask(state);
> > > -       bool async = kms->funcs->vsync_time &&
> > > -                       can_do_async(state, &async_crtc);
> > > +       bool async;
> > > +
> > > +       if (!kms)
> > > +               return;
> >
> > That looks a bit like a hack papering over the real issue.
> >
> > From your report it sounds like earlier kernels worked, did you
> > attempt bisecting? Also for regressions put regressions into the
> > subject, it's the magic work that gets much more attention.
>
> the root issue is how are we doing KMS stuff on imx (where drm/msm is
> only used for gpu).. which I think is this commit:
>
> ----------
> commit 9d5cbf5fe46e350715389d89d0c350d83289a102
> Author:     Krishna Manikandan <mkrishn@codeaurora.org>
> AuthorDate: Mon Jun 1 16:33:22 2020 +0530
> Commit:     Rob Clark <robdclark@chromium.org>
> CommitDate: Tue Aug 18 08:09:01 2020 -0700
>
>     drm/msm: add shutdown support for display platform_driver
>
>     Define shutdown callback for display drm driver,
>     so as to disable all the CRTCS when shutdown
>     notification is received by the driver.
>
>     This change will turn off the timing engine so
>     that no display transactions are requested
>     while mmu translations are getting disabled
>     during reboot sequence.
>
>     Signed-off-by: Krishna Manikandan <mkrishn@codeaurora.org>
>
>     Changes in v2:
>             - Remove NULL check from msm_pdev_shutdown (Stephen Boyd)
>             - Change commit text to reflect when this issue
>               was uncovered (Sai Prakash Ranjan)
>
>     Signed-off-by: Rob Clark <robdclark@chromium.org>
> ----------
>
> I suppose we should do the drm_atomic_helper_shutdown() conditionally?
>  Or the helper should bail if there is no kms?
>
> BR,
> -R
>
> > -Daniel
> >
> > > +
> > > +       async = kms->funcs->vsync_time && can_do_async(state, &async_crtc);
> > >
> > >         trace_msm_atomic_commit_tail_start(async, crtc_mask);
> > >
> > > Any comments?
> > >
> > > Thanks
> > > _______________________________________________
> > > dri-devel mailing list
> > > dri-devel@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> >
> >
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel