[PATCH] drm/imx: correct order of crtc disable

[Daniel Vetter]

On Wed, Jun 19, 2019 at 11:40 AM Philipp Zabel <p.zabel at pengutronix.de> wrote:
>
> Hi Robert,
>
> thank you for the patch.
>
> On Tue, 2019-06-18 at 16:50 +0100, Robert Beckett wrote:
> > Notify drm core before sending pending events during crtc disable.
> > This fixes the first event after disable having an old stale timestamp
> > by having drm_crtc_vblank_off update the timestamp to now.
> >
> > This was seen while debugging weston log message:
> > Warning: computed repaint delay is insane: -8212 msec
> >
>
> Would you say this
> Fixes: a474478642d5 ("drm/imx: fix crtc vblank state regression")
> ?
>
> > Signed-off-by: Robert Beckett <bob.beckett at collabora.com>
> > ---
> >  drivers/gpu/drm/imx/ipuv3-crtc.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/imx/ipuv3-crtc.c b/drivers/gpu/drm/imx/ipuv3-crtc.c
> > index 9cc1d678674f..c436a28d50e4 100644
> > --- a/drivers/gpu/drm/imx/ipuv3-crtc.c
> > +++ b/drivers/gpu/drm/imx/ipuv3-crtc.c
> > @@ -91,14 +91,14 @@ static void ipu_crtc_atomic_disable(struct drm_crtc *crtc,
> >       ipu_dc_disable(ipu);
> >       ipu_prg_disable(ipu);
> >
> > +     drm_crtc_vblank_off(crtc);
> > +
>
> This is explained in the commit message and aligns with the
> drm_crtc_state @event documentation.

This part here looks fishy. The drm_vblank.c code is supposed to do
the right thing, no matter where or when you ask it to generate an
event. It definitely shouldn't generate a timestamp that's a few
seconds too early. Bunch of options:
- there's a bug in drm_vblank.c and it's mixing up something and
generating a totally bogus value.
- there's a lie in your imx vblank code, which trips the drm_vblank.c
counter interpolation and results in a totally bogus value.

drm_vblank.c assumes that if you do claim to have a hw counter and
generate timestamps, that those are perfectly accurate. It only falls
back to guestimating using the system timer if that's not present.

Either way, this very much smells like papering over a bug if this
change indeed fixes your wrong vblank timestamps.

> >       spin_lock_irq(&crtc->dev->event_lock);
> > -     if (crtc->state->event) {
> > +     if (crtc->state->event && !crtc->state->active) {
>
> This is not mentioned though.
>
> If the pending event is not sent here, I assume it will be picked up by
> .atomic_flush and will then be sent after the first EOF interrupt after
> the modeset is complete. Can you explain this in the commit message?

Yeah looks correct (you only want to generate the event here when the
crtc stays off), if it gets re-enabled the event should only be
generated later on once that's all finished. But separate bugfix.
-Daniel

>
> With that,
> Reviewed-by: Philipp Zabel <p.zabel at pengutronix.de>
>
> >               drm_crtc_send_vblank_event(crtc, crtc->state->event);
> >               crtc->state->event = NULL;
> >       }
> >       spin_unlock_irq(&crtc->dev->event_lock);
> > -
> > -     drm_crtc_vblank_off(crtc);
> >  }
> >
> >  static void imx_drm_crtc_reset(struct drm_crtc *crtc)
>
> regards
> Philipp
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch