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

[Daniel Vetter]

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.

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch