From: Daniel Vetter <daniel@ffwll.ch> To: Mario Kleiner <mario.kleiner.de@gmail.com> Cc: dri-devel@lists.freedesktop.org, linux@bernd-steinhauser.de, stable@vger.kernel.org, michel@daenzer.net, vbabka@suse.cz, ville.syrjala@linux.intel.com, daniel.vetter@ffwll.ch, alexander.deucher@amd.com, christian.koenig@amd.com Subject: Re: [PATCH 1/6] drm: No-Op redundant calls to drm_vblank_off() Date: Tue, 9 Feb 2016 10:54:55 +0100 [thread overview] Message-ID: <20160209095455.GL11240@phenom.ffwll.local> (raw) In-Reply-To: <1454894009-15466-2-git-send-email-mario.kleiner.de@gmail.com> 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 > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
WARNING: multiple messages have this Message-ID (diff)
From: Daniel Vetter <daniel@ffwll.ch> To: Mario Kleiner <mario.kleiner.de@gmail.com> Cc: dri-devel@lists.freedesktop.org, daniel.vetter@ffwll.ch, michel@daenzer.net, linux@bernd-steinhauser.de, stable@vger.kernel.org, alexander.deucher@amd.com, christian.koenig@amd.com, vbabka@suse.cz Subject: Re: [PATCH 1/6] drm: No-Op redundant calls to drm_vblank_off() Date: Tue, 9 Feb 2016 10:54:55 +0100 [thread overview] Message-ID: <20160209095455.GL11240@phenom.ffwll.local> (raw) In-Reply-To: <1454894009-15466-2-git-send-email-mario.kleiner.de@gmail.com> 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 > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
next prev parent reply other threads:[~2016-02-09 9:54 UTC|newest] Thread overview: 52+ messages / expand[flat|nested] mbox.gz Atom feed top 2016-02-08 1:13 drm vblank regression fixes for Linux 4.4+ Mario Kleiner 2016-02-08 1:13 ` [PATCH 1/6] drm: No-Op redundant calls to drm_vblank_off() Mario Kleiner 2016-02-08 1:13 ` Mario Kleiner 2016-02-09 9:54 ` Daniel Vetter [this message] 2016-02-09 9:54 ` Daniel Vetter 2016-02-09 13:27 ` Mario Kleiner 2016-02-08 1:13 ` [PATCH 2/6] drm: Prevent vblank counter bumps > 1 with active vblank clients Mario Kleiner 2016-02-08 1:13 ` Mario Kleiner 2016-02-09 9:56 ` Daniel Vetter 2016-02-09 9:56 ` Daniel Vetter 2016-02-09 10:07 ` Ville Syrjälä 2016-02-09 10:07 ` Ville Syrjälä 2016-02-09 10:23 ` Daniel Vetter 2016-02-09 10:23 ` Daniel Vetter 2016-02-09 13:39 ` Mario Kleiner 2016-02-09 13:39 ` Mario Kleiner 2016-02-09 14:29 ` Daniel Vetter 2016-02-09 14:29 ` Daniel Vetter 2016-02-09 16:18 ` Mario Kleiner 2016-02-09 16:18 ` Mario Kleiner 2016-02-08 1:13 ` [PATCH 3/6] drm: Fix drm_vblank_pre/post_modeset regression from Linux 4.4 Mario Kleiner 2016-02-08 1:13 ` Mario Kleiner 2016-02-09 10:00 ` Daniel Vetter 2016-02-11 13:03 ` Vlastimil Babka 2016-02-08 1:13 ` [PATCH 4/6] drm: Fix treatment of drm_vblank_offdelay in drm_vblank_on() Mario Kleiner 2016-02-08 1:13 ` Mario Kleiner 2016-02-09 10:06 ` Daniel Vetter 2016-02-09 10:06 ` Daniel Vetter 2016-02-09 11:10 ` Ville Syrjälä 2016-02-09 11:10 ` Ville Syrjälä 2016-02-09 13:29 ` Mario Kleiner 2016-02-09 13:29 ` Mario Kleiner 2016-02-09 13:41 ` Ville Syrjälä 2016-02-09 13:41 ` Ville Syrjälä 2016-02-09 14:31 ` Daniel Vetter 2016-02-09 14:31 ` Daniel Vetter 2016-02-08 1:13 ` [PATCH 5/6] drm: Prevent vblank counter jumps with timestamp based update method Mario Kleiner 2016-02-08 1:13 ` Mario Kleiner 2016-02-09 10:09 ` Daniel Vetter 2016-02-09 13:53 ` Mario Kleiner 2016-02-09 14:11 ` Ville Syrjälä 2016-02-09 14:11 ` Ville Syrjälä 2016-02-09 15:03 ` Daniel Vetter 2016-02-09 15:03 ` Daniel Vetter 2016-02-10 16:28 ` Mario Kleiner 2016-02-10 16:28 ` Mario Kleiner 2016-02-10 17:17 ` Daniel Vetter 2016-02-10 18:36 ` Mario Kleiner 2016-02-10 19:34 ` Daniel Vetter 2016-02-10 19:34 ` Daniel Vetter 2016-02-08 1:13 ` [PATCH 6/6] drm/radeon/pm: Handle failure of drm_vblank_get Mario Kleiner 2016-02-09 10:10 ` Daniel Vetter
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20160209095455.GL11240@phenom.ffwll.local \ --to=daniel@ffwll.ch \ --cc=alexander.deucher@amd.com \ --cc=christian.koenig@amd.com \ --cc=daniel.vetter@ffwll.ch \ --cc=dri-devel@lists.freedesktop.org \ --cc=linux@bernd-steinhauser.de \ --cc=mario.kleiner.de@gmail.com \ --cc=michel@daenzer.net \ --cc=stable@vger.kernel.org \ --cc=vbabka@suse.cz \ --cc=ville.syrjala@linux.intel.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.