[Intel-gfx] [RFC PATCH] drm/vblanks: Deal with HW vblank counter resets.

[Ville Syrjälä]

On Tue, Nov 07, 2017 at 11:55:44AM +0100, Michel Dänzer wrote:
> On 07/11/17 11:50 AM, Ville Syrjälä wrote:
> > On Tue, Nov 07, 2017 at 10:47:00AM +0100, Michel Dänzer wrote:
> >> On 07/11/17 07:26 AM, Dhinakaran Pandiyan wrote:
> >>> Some HW vblank counters reset due to power management events, which messes
> >>> up the vblank counting logic. This leads to screen freezes with user space
> >>> waiting on vblank events that may not occur if the counter keeps resetting.
> >>>
> >>> For e.g., After the HW vblank counter resets
> >>> [    9.007359] [drm:drm_update_vblank_count [drm]] updating vblank count
> >>> on crtc 0: current=297, diff=4294965389, hw=5 hw_last=1912
> >>>
> >>> So, fall back to the SW counter, computed using  vblank timestamps
> >>> and frame duration, when the HW counter value deviates by 50% of the SW
> >>> computed value.
> >>>
> >>> I have tested this patch on my SKL laptop with i915.enable_psr=1 and it
> >>> *seems* to solve the screen freeze issue seen with PSR when DMC is loaded.
> >>>
> >>> Known issues:
> >>> 1) The 50% deviation margin is arbitrary.
> >>> 2) "Redundant vblirq ignored" messages are more frequent.
> >>>
> >>> I am sending this as an RFC to get feedback on whether the fall back
> >>> approach is sane and if it should be implemented in the core.
> >>
> >> Is there no way for the driver to know under which circumstances the
> >> reset to 0 might happen? If there is, maybe it could be solved by
> >> calling drm_crtc_vblank_off() before it might happen and
> >> drm_crtc_vblank_on() after it might have happened.
> >>
> >> Otherwise, might it be better not to use the HW counter at all when it's
> >> known not to be reliable?
> > 
> > Yeah, I think we could just avoid using the HW counter whenever there's
> > a possibility of PSR being used on the crtc.
> > 
> > Assuming we still want to use the HW counter on crtcs that can't do PSR,
> > we're going to need some kind of per-crtc mechanism to tell drm_vblank.c
> > which method to use. hwmode.private_flags is one option. We already use
> > it for choosing between the scanline counter and hardware timestamps when
> > calculating the scanout position. But in this case the flag wouldn't be
> > exactly private since drm_vblank.c would have to know about it as well.
> > If that is deemed to be a problem, then we might just need to add a bool
> > to struct drm_vblank_crtc to indicate that the hw counter is good, or
> > maybe even move the dev->max_vblank_count to live inside
> > struct drm_vblank_crtc.
> 
> Or just allow setting struct drm_vblank_crtc's get_vblank_counter member
> to NULL?

Looks like that's actually under drm_crtc_funcs. I'm thinking we want
to keep that as const. Not sure we want to add a third way for drivers
to provide a .get_vblank_counter() hook (the second one being the
old drm_driver.get_vblank_counter() hook, which i915 is still using
actually).

-- 
Ville Syrjälä
Intel OTC