On Thu, Jun 20, 2019 at 3:30 PM Robert Beckett
<bob.beckett@xxxxxxxxxxxxx> wrote:
> On Thu, 2019-06-20 at 14:32 +0200, Daniel Vetter wrote:
> > On Thu, Jun 20, 2019 at 12:12:13PM +0100, Robert Beckett wrote:
> > > On Thu, 2019-06-20 at 10:50 +0200, Daniel Vetter wrote:
> > > > On Wed, Jun 19, 2019 at 11:40 AM Philipp Zabel <
> > > > p.zabel@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxx>
> > > > > > ---
> > > > > > 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.
> > > >
> > >
> > > A quick explaination of where the dodgy timestamp came from:
> > > 1. driver starts up
> > > 2. fbcon comes along and restores fbdev, enabling vblank
> > > 3. vblank_disable_fn fires via timer disabling vblank, keeping
> > > vblank
> > > seq number and time set at current value
> > > (some time later)
> > > 4. weston starts and does a modeset
> > > 5. atomic commit disables crtc while it does the modeset
> > > 6. ipu_crtc_atomic_disable sends vblank with old seq number and
> > > time
> > >
> > > It turns out the actual fix for the old vblank is the next change,
> > > which stops it being sent at all during the crtc disable as it is
> > > is
> > > still active, it would then go through drm_crtc_vblank_off,
> > > reseting
> > > the timestamp, and get delivered during the vblank enable as part
> > > of
> > > the atomic commit.
> >
> > This shouldn't fix your vblank timestamp troubles either. It might
> > mean
> > that the timestamp is slightly too early (because you take it while
> > shutting down the crtc, not while re-enabling), but not by seconds.
> >
> > Quick experiment: Disable vblank disabling with drm.vblankoffdelay =
> > 0. If
> > that also fixes the timestamps, then I'm pretty sure you have a
> > driver bug
> > somewhere and lie to the vblank core code about something.
> > -Daniel
> >
>
> Experiment done. The timestamp is fine, it is the timestamp of the
> previous vblank update. But, this would fix it because the vblank
> interrupt was never disabled.
>
> The original issue was that the event got sent after vblank was
> disabled and before it got re-enabled during the modeset, so nothing
> had happened to update the timestamp and seq number.
>
> What are you expecting to update the timestamp and seq number while the
> interrupt is disabled after vblank_disable_fn?
Hm maybe this indeed needs to be shuffled around a bit, since
currently it's indeed not not allowed to call
drm_crtc_send_vblank_event if:
- your driver has vblank support (i.e. dev->num_crtcs > 0)
- the vblank irq is off (i.e. no one called drm_crtc_vblank_get)
- from the vblank code's pov the pipe is still running (i.e. not
in-between a drm_crtc_vblank_off()/on() pair)
If all of these conditions hold then drm_crtc_send_vblank_event is
going to give you a garbage timestamp and and sequence number. If you
call drm_crtc_vblank_get/put around it, or after vblank_off, then
either of those will have rolled things forward for you.
The other option I was chasing is a get_vblank_counter driver callback
which lies, but I think that's only relevant for immediate disabling
support.
Anyway, thanks for sticking with me on this. Can I volunteer you to
write a patch to improve the documentation of
drm_crtc_send_vblank_event, plus add a WARN_ON in there that fires if
the above conditions are all true? I expect imx isn't the only one
that gets this wrong, would be good to close this trap for real.
> Im struggling to see what this experiment was meant to test/prove.
Worked all fine in confirming what I wanted to confirm.
Thanks, Daniel
>
> > >
> > > So, in theory, we could just have the following change to fix the
> > > specific issue of a stale timestamp.
> > >
> > > However, given the documentation for the event in
> > > include/drm/drm_crtc.h:
> > >
> > > * - The event is for a CRTC which is being disabled
> > > through
> > > this
> > > * atomic commit. In that case the event can be send out
> > > any
> > > time
> > > * after the hardware has stopped scanning out the
> > > current
> > > * framebuffers. It should contain the timestamp and
> > > counter
> > > for the
> > > * last vblank before the display pipeline was shut off.
> > > The
> > > simplest
> > > * way to achieve that is calling
> > > drm_crtc_send_vblank_event()
> > > * somewhen after drm_crtc_vblank_off() has been called.
> > >
> > > This still seems like a sensible change for when the crtc is being
> > > disabled.
> > >
> > >
> > >
> > > > > > 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
> > > >
> > >
> > > It looks like this is actually the fix needed to avoid the bogus
> > > timestamp.
> > >
> > > I can split this patch up in to 2 commits if desired?
> > >
> > > > >
> > > > > With that,
> > > > > Reviewed-by: Philipp Zabel <p.zabel@xxxxxxxxxxxxxx>
> > > > >
> > > > > > 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@xxxxxxxxxxxxxxxxxxxxx
> > > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > > >
> > > >
> > > >
> >
> >
>
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
