On Thu, Mar 30, 2017 at 2:03 PM, Ville Syrjälä ville.syrjala@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.
But what we could do (at least with atomic) is WARN in the vblank helper if it's called outside of drm_vblank_on/off ... Not sure how useful that is (it won't catch when a driver outright forgets to call these) or whether we have enough checks already. Would be a separate patch (can do ofc if we agree on what exactly). -Daniel