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 _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
