Hi Robert, Daniel,
On Thu, 2019-06-20 at 12:12 +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.
Thank you for chiming in, I can confirm that just moving the
drm_crtc_vblank_off around does not change anything.
I'll drop this patch and wait for v3.
> 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 would be great to have this in the commit message for context.
> 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.
Which means this patch isn't "Fixes: a474478642d5" after all.
The offending code has been there since commit 5f2f911578fb ("drm/imx:
atomic phase 3 step 1: Use atomic configuration").
> 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.
This is what had me confused as well. It doesn't really say, but it
seems to imply that drm_crtc_send_vblank_event must only be sent after
drm_crtc_vblank_off.
If this is not a hard requirement, maybe this could be mentioned here?
> > > > 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?
Yes, please. The !state->active fix is stable material, moving
drm_crtc_vblank_off around apparently not so much.
regards
Philipp
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
