Funky new vblank counter regressions in Linux 4.4-rc1

[Ville Syrjälä]

On Wed, Nov 25, 2015 at 06:24:13PM +0100, Mario Kleiner wrote:
> On 11/23/2015 09:24 PM, Ville Syrjälä wrote:
> > On Mon, Nov 23, 2015 at 06:58:34PM +0100, Mario Kleiner wrote:
> >>
> >>
> >> On 11/23/2015 04:51 PM, Ville Syrjälä wrote:
> >>> On Mon, Nov 23, 2015 at 04:23:21PM +0100, Mario Kleiner wrote:
> >>>> On 11/20/2015 04:34 PM, Ville Syrjälä wrote:
> >>>>> On Fri, Nov 20, 2015 at 04:24:50PM +0100, Mario Kleiner wrote:
> >>>>
> >>>> ...
> >>>> Ok, but why would that be a bad thing? I think we want it to think it is
> >>>> in the previous frame if it is called outside the vblank irq context.
> >>>> The only reason we fudge it to the next frames vblank if i vblank irq is
> >>>> because we know the vblank irq handler we are executing atm. was meant
> >>>> to execute within the upcoming vblank for the next frame, so we fudge
> >>>> the scanout positions and thereby timestamp to correspond to that new
> >>>> frame. But if something called outside irq context it should get a
> >>>> scanout position/timestamp that corresponds to "reality".
> >>>
> >>> It would be a bad thing since it would cause the timestamp to jump
> >>> backwards, and that would also cause the frame count guesstimate to go
> >>> backwards.
> >>>
> >>
> >> But only if we don't use the dev->driver->get_vblank_counter() method,
> >> which we try to use on AMD.
> >
> > Well, if you do it that way then you have the problem of the hw counter
> > seeming to jump forward by one after crossing the start of vblank (when
> > compared to the value you sampled when you processed the early vblank
> > interrupt).
> >
> 
> Ok, finally i see the bad scenario that wouldn't get prevented by our 
> current locking with the new vblank counting in the core. The vblank 
> enable path is safe due to locking and discounting of redundant 
> timestamps etc. But the disable path could go wrong:
> 
> 1. Vblank irq fires, drm_handle_vblank() -> drm_update_vblank_count(),
> updates timestamps and counts "as if" in vblank -> incremented vblank 
> count and timestamp now set in the future.
> 
> 2. After vblank irq finishes, but just before leading edge of vblank, 
> vblank_disable_and_save() executes, doesn't get bumped timestamp or 
> count because before vblank and not in vblank irq. Now 
> drm_update_vblank_count() would process a
> "new" timestamp and count from the past and we'd have time and counts 
> going backwards, and bad things would happen.
> 
> I haven't observed such a thing happening during testing so far, 
> probably because the time window in which it could happen is tiny, but 
> given how awfully bad it would be, it needs to be prevented.
> 
> I had a look at the description of the Vblank irq in the "M76 Register 
> Reference Guide" for older asics and the description suggests that the 
> vblank irq fires when the crtc's line buffer is finished reading pixel 
> data from the scanout buffer in memory for a frame, ie., when the line 
> buffer read "enters" vblank.

Hmm. Does that mean there's always at least one fullscreen plane enabled
in the hw? As in you can't turn off the primary plane or make it smaller
than the active video area? Othwewise it sounds like you'd could either
not get it at all, or get it somewhere in the middle of the screen.

-- 
Ville Syrjälä
Intel OTC