| Message ID | 1454894009-15466-2-git-send-email-mario.kleiner.de@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@gmail.com> > Cc: <stable@vger.kernel.org> # 4.4+ > Cc: michel@daenzer.net > Cc: vbabka@suse.cz > Cc: ville.syrjala@linux.intel.com > Cc: daniel.vetter@ffwll.ch > Cc: dri-devel@lists.freedesktop.org > Cc: alexander.deucher@amd.com > Cc: christian.koenig@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@ffwll.ch> > + 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 >
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@gmail.com> >> Cc: <stable@vger.kernel.org> # 4.4+ >> Cc: michel@daenzer.net >> Cc: vbabka@suse.cz >> Cc: ville.syrjala@linux.intel.com >> Cc: daniel.vetter@ffwll.ch >> Cc: dri-devel@lists.freedesktop.org >> Cc: alexander.deucher@amd.com >> Cc: christian.koenig@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@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 >> >
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) + 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);
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@gmail.com> Cc: <stable@vger.kernel.org> # 4.4+ Cc: michel@daenzer.net Cc: vbabka@suse.cz Cc: ville.syrjala@linux.intel.com Cc: daniel.vetter@ffwll.ch Cc: dri-devel@lists.freedesktop.org Cc: alexander.deucher@amd.com Cc: christian.koenig@amd.com --- drivers/gpu/drm/drm_irq.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)