On 02/09/2016 10:54 AM, Daniel Vetter wrote:
> On Mon, Feb 08, 2016 at 02:13:24AM +0100, Mario Kleiner wrote:
>> Otherwise if a kms driver calls into drm_vblank_off() more than once
>> before calling drm_vblank_on() again, the redundant calls to
>> vblank_disable_and_save() will call drm_update_vblank_count()
>> while hw vblank counters and vblank timestamping are in a undefined
>> state during modesets, dpms off etc.
>>
>> At least with the legacy drm helpers it is not unusual to
>> get multiple calls to drm_vblank_off and drm_vblank_on, e.g.,
>> half a dozen calls to drm_vblank_off and two calls to drm_vblank_on
>> were observed on radeon-kms during dpms-off -> dpms-on transition.
>>
>> Fixes large jumps of the software maintained vblank counter due to
>> the hardware vblank counter resetting to zero during dpms off or
>> modeset, e.g., if radeon-kms is modified to use drm_vblank_off/on
>> instead of drm_vblank_pre/post_modeset().
>>
>> This fixes a regression caused by the changes made to
>> drm_update_vblank_count() in Linux 4.4.
>>
>> Signed-off-by: Mario Kleiner <mario.kleiner.de at gmail.com>
>> Cc: <stable at vger.kernel.org> # 4.4+
>> Cc: michel at daenzer.net
>> Cc: vbabka at suse.cz
>> Cc: ville.syrjala at linux.intel.com
>> Cc: daniel.vetter at ffwll.ch
>> Cc: dri-devel at lists.freedesktop.org
>> Cc: alexander.deucher at amd.com
>> Cc: christian.koenig at amd.com
>> ---
>> drivers/gpu/drm/drm_irq.c | 11 ++++++++++-
>> 1 file changed, 10 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
>> index 607f493..bcb8528 100644
>> --- a/drivers/gpu/drm/drm_irq.c
>> +++ b/drivers/gpu/drm/drm_irq.c
>> @@ -1313,7 +1313,13 @@ void drm_vblank_off(struct drm_device *dev, unsigned
>> int pipe)
>> spin_lock_irqsave(&dev->event_lock, irqflags);
>>
>> spin_lock(&dev->vbl_lock);
>> - vblank_disable_and_save(dev, pipe);
>> + DRM_DEBUG_VBL("crtc %d, vblank enabled %d, inmodeset %d\n",
>> + pipe, vblank->enabled, vblank->inmodeset);
>> +
>> + /* Avoid redundant vblank disables without previous drm_vblank_on(). */
>> + if (!vblank->inmodeset)
>
> Since atomic drivers shouldn't suck so badly at this, maybe
>
> if (DRIVER_ATOMIC || !vblank->inmodeset)
>
> instead? That way the flexibility would be constraint to old drivers that
> need it because legacy crtc helpers suck. With that change:
>
> Reviewed-by: Daniel Vetter <daniel.vetter at ffwll.ch>
>
Ok, thanks, will change that.
-mario
>> + vblank_disable_and_save(dev, pipe);
>> +
>> wake_up(&vblank->queue);
>>
>> /*
>> @@ -1415,6 +1421,9 @@ void drm_vblank_on(struct drm_device *dev, unsigned
>> int pipe)
>> return;
>>
>> spin_lock_irqsave(&dev->vbl_lock, irqflags);
>> + DRM_DEBUG_VBL("crtc %d, vblank enabled %d, inmodeset %d\n",
>> + pipe, vblank->enabled, vblank->inmodeset);
>> +
>> /* Drop our private "prevent drm_vblank_get" refcount */
>> if (vblank->inmodeset) {
>> atomic_dec(&vblank->refcount);
>> --
>> 1.9.1
>>
>
