[Intel-gfx] [PATCH] drm/vblank: drop the mode argument from drm_calc_vbltimestamp_from_scanoutpos

[Daniel Vetter]

On Thu, Mar 30, 2017 at 08:27:40PM +0200, Daniel Vetter wrote:
> On Thu, Mar 30, 2017 at 04:41:57PM +0300, Ville Syrjälä wrote:
> > On Thu, Mar 30, 2017 at 03:27:57PM +0200, Daniel Vetter wrote:
> > > On Thu, Mar 30, 2017 at 2:03 PM, Ville Syrjälä
> > > <ville.syrjala at linux.intel.com> wrote:
> > > > On Wed, Mar 22, 2017 at 09:56:12PM +0100, Daniel Vetter wrote:
> > > >> If we restrict this helper to only kms drivers (which is the case) we
> > > >> can look up the correct mode easily ourselves. But it's a bit tricky:
> > > >>
> > > >> - All legacy drivers look at crtc->hwmode. But that is update already
> > > >>   at the beginning of the modeset helper, which means when we disable
> > > >>   a pipe. Hence the final timestamps might be a bit off. But since
> > > >>   this is an existing bug I'm not going to change it, but just try to
> > > >>   be bug-for-bug compatible with the current code. This only applies
> > > >>   to radeon&amdgpu.
> > > >>
> > > >> - i915 tries to get it perfect by updating crtc->hwmode when the pipe
> > > >>   is off (i.e. vblank->enabled = false).
> > > >>
> > > >> - All other atomic drivers look at crtc->state->adjusted_mode. Those
> > > >>   that look at state->requested_mode simply don't adjust their mode,
> > > >>   so it's the same. That has two problems: Accessing crtc->state from
> > > >>   interrupt handling code is unsafe, and it's updated before we shut
> > > >>   down the pipe. For nonblocking modesets it's even worse.
> > > >>
> > > >> For atomic drivers try to implement what i915 does. To do that we add
> > > >> a new hwmode field to the vblank structure, and update it from
> > > >> drm_calc_timestamping_constants().
> > > >
> > > > i915 clear crtc->hwmode.crtc_clock when turning the crtc off, which
> > > > this does not do for the new vblank->hwmode. I guess no one should
> > > > really end up in these codepaths with a disabled crtc, but parts of
> > > > drm_irq.c sort of look like they're expecting it to happen.
> > > >
> > > > So should we have some way to clear out the vblank->hwmode.crtc_clock
> > > > for disabled crtcs? And then maybe make some of these crtc_clock checks
> > > > WARN and eventually just nuke it all if it looks like nothing is hitting
> > > > those?
> > > 
> > > So the trouble is that with a pile of dpms on/off/on/off you could run
> > > drm_crtc_vblank_on/off a lot, without ever calling the
> > > drm_calc_vbltimestamps helper again to re-upload the mode. So I don't
> > > think we can clear vblank->hwmode.crtc_clock unfortunately in
> > > drm_crtc_vblank_off.
> > 
> > I was thinking that we'd just try to avoid making pontetially functional
> > changes here. Ie. reset where we currently reset, which I think is somewhere
> > in the atomic commit for i915, but definitely not in drm_crtc_vblank_off().
> 
> Hm, sounds like a good idea. I'll try to shuffle things around. Upon
> re-reading all that stuff I also noticed that atm i915 only calls the
> helper that would update vblank->hwmode when we do a full modeset, but not
> when we just fastset. Shouldn't matter, because the hwmode will match, but
> better safe than sorry.
> 
> I'll re-read all that stuff again and will try to lock it down fully, like
> we have now, and remove the i915 crtc->hwmode access maybe entirely while
> at it.

Ok, I stitched something together which I think should work well. Done as
a follow-up patch though since this refactoring patch here was already
pretty big. Resend the entire pile for CI to double-check, review from you
on the vblank stuff would be much appreciated.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch