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

[Michel Dänzer]

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?


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer