* drm vblank fixes
@ 2014-08-06 1:22 Mario Kleiner
2014-08-06 1:22 ` [PATCH 1/3] drm: Remove drm_vblank_cleanup from drm_vblank_init error path Mario Kleiner
` (3 more replies)
0 siblings, 4 replies; 13+ messages in thread
From: Mario Kleiner @ 2014-08-06 1:22 UTC (permalink / raw)
To: dri-devel; +Cc: airlied
Hi all,
some small bug fixes for some small bugs i saw
when looking at the current drm vblank handling code.
All patches are rather simple, all compile-tested against
drm-next, but only the drm_vblank_off() one (patch 2)
tested in "real life" so far.
thanks,
-mario
^ permalink raw reply [flat|nested] 13+ messages in thread* [PATCH 1/3] drm: Remove drm_vblank_cleanup from drm_vblank_init error path. 2014-08-06 1:22 drm vblank fixes Mario Kleiner @ 2014-08-06 1:22 ` Mario Kleiner 2014-08-06 10:51 ` Ville Syrjälä 2014-08-06 1:22 ` [PATCH 2/3] drm: Fix emitted vblank timestamps in drm_vblank_off() Mario Kleiner ` (2 subsequent siblings) 3 siblings, 1 reply; 13+ messages in thread From: Mario Kleiner @ 2014-08-06 1:22 UTC (permalink / raw) To: dri-devel; +Cc: airlied, mario.kleiner.de, stable drm_vblank_cleanup() would operate on non-existent dev->vblank data structure, as failure to allocate that data structure is what triggers the error path in the first place. Signed-off-by: Mario Kleiner <mario.kleiner.de@gmail.com> Cc: stable@vger.kernel.org --- drivers/gpu/drm/drm_irq.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c index 0de123a..553a58c 100644 --- a/drivers/gpu/drm/drm_irq.c +++ b/drivers/gpu/drm/drm_irq.c @@ -224,7 +224,7 @@ int drm_vblank_init(struct drm_device *dev, int num_crtcs) return 0; err: - drm_vblank_cleanup(dev); + dev->num_crtcs = 0; return ret; } EXPORT_SYMBOL(drm_vblank_init); -- 1.9.1 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 1/3] drm: Remove drm_vblank_cleanup from drm_vblank_init error path. 2014-08-06 1:22 ` [PATCH 1/3] drm: Remove drm_vblank_cleanup from drm_vblank_init error path Mario Kleiner @ 2014-08-06 10:51 ` Ville Syrjälä 0 siblings, 0 replies; 13+ messages in thread From: Ville Syrjälä @ 2014-08-06 10:51 UTC (permalink / raw) To: Mario Kleiner; +Cc: airlied, stable, dri-devel On Wed, Aug 06, 2014 at 03:22:44AM +0200, Mario Kleiner wrote: > drm_vblank_cleanup() would operate on non-existent dev->vblank > data structure, as failure to allocate that data structure is > what triggers the error path in the first place. > > Signed-off-by: Mario Kleiner <mario.kleiner.de@gmail.com> > Cc: stable@vger.kernel.org Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/drm_irq.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c > index 0de123a..553a58c 100644 > --- a/drivers/gpu/drm/drm_irq.c > +++ b/drivers/gpu/drm/drm_irq.c > @@ -224,7 +224,7 @@ int drm_vblank_init(struct drm_device *dev, int num_crtcs) > return 0; > > err: > - drm_vblank_cleanup(dev); > + dev->num_crtcs = 0; > return ret; > } > EXPORT_SYMBOL(drm_vblank_init); > -- > 1.9.1 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel -- Ville Syrjälä Intel OTC ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 2/3] drm: Fix emitted vblank timestamps in drm_vblank_off() 2014-08-06 1:22 drm vblank fixes Mario Kleiner 2014-08-06 1:22 ` [PATCH 1/3] drm: Remove drm_vblank_cleanup from drm_vblank_init error path Mario Kleiner @ 2014-08-06 1:22 ` Mario Kleiner 2014-08-06 10:53 ` Ville Syrjälä 2014-08-06 1:22 ` [PATCH 3/3] drm: Use vblank_disable_and_save in drm_vblank_cleanup() Mario Kleiner 2014-08-06 6:37 ` drm vblank fixes Daniel Vetter 3 siblings, 1 reply; 13+ messages in thread From: Mario Kleiner @ 2014-08-06 1:22 UTC (permalink / raw) To: dri-devel; +Cc: airlied, stable Move the query for vblank count and time before the vblank_disable_and_save(), because the disable fn will invalidate the vblank timestamps, so all emitted events would carry an invalid zero timestamp instead of the timestamp of the vblank of vblank disable. This could confuse clients. Signed-off-by: Mario Kleiner <mario.kleiner.de@gmail.com> Cc: stable@vger.kernel.org --- drivers/gpu/drm/drm_irq.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c index 553a58c..89e91e3 100644 --- a/drivers/gpu/drm/drm_irq.c +++ b/drivers/gpu/drm/drm_irq.c @@ -1019,13 +1019,14 @@ void drm_vblank_off(struct drm_device *dev, int crtc) unsigned long irqflags; unsigned int seq; + /* Get 'now' vblank ts before it gets cleared by vblank disable */ + seq = drm_vblank_count_and_time(dev, crtc, &now); + spin_lock_irqsave(&dev->vbl_lock, irqflags); vblank_disable_and_save(dev, crtc); wake_up(&dev->vblank[crtc].queue); /* Send any queued vblank events, lest the natives grow disquiet */ - seq = drm_vblank_count_and_time(dev, crtc, &now); - spin_lock(&dev->event_lock); list_for_each_entry_safe(e, t, &dev->vblank_event_list, base.link) { if (e->pipe != crtc) -- 1.9.1 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 2/3] drm: Fix emitted vblank timestamps in drm_vblank_off() 2014-08-06 1:22 ` [PATCH 2/3] drm: Fix emitted vblank timestamps in drm_vblank_off() Mario Kleiner @ 2014-08-06 10:53 ` Ville Syrjälä 0 siblings, 0 replies; 13+ messages in thread From: Ville Syrjälä @ 2014-08-06 10:53 UTC (permalink / raw) To: Mario Kleiner; +Cc: airlied, stable, dri-devel On Wed, Aug 06, 2014 at 03:22:45AM +0200, Mario Kleiner wrote: > Move the query for vblank count and time before the > vblank_disable_and_save(), because the disable fn > will invalidate the vblank timestamps, so all emitted > events would carry an invalid zero timestamp instead of > the timestamp of the vblank of vblank disable. This could > confuse clients. > > Signed-off-by: Mario Kleiner <mario.kleiner.de@gmail.com> > Cc: stable@vger.kernel.org > --- > drivers/gpu/drm/drm_irq.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c > index 553a58c..89e91e3 100644 > --- a/drivers/gpu/drm/drm_irq.c > +++ b/drivers/gpu/drm/drm_irq.c > @@ -1019,13 +1019,14 @@ void drm_vblank_off(struct drm_device *dev, int crtc) > unsigned long irqflags; > unsigned int seq; > > + /* Get 'now' vblank ts before it gets cleared by vblank disable */ > + seq = drm_vblank_count_and_time(dev, crtc, &now); > + This will cause us to send out a potentially stale vblank seq/ts. I had a different solution to this (not clearing the timestamps) in my pending vblank series. I have a few more patches on top of that series lined up. Let me send out the full series so we can discuss it if needed... > spin_lock_irqsave(&dev->vbl_lock, irqflags); > vblank_disable_and_save(dev, crtc); > wake_up(&dev->vblank[crtc].queue); > > /* Send any queued vblank events, lest the natives grow disquiet */ > - seq = drm_vblank_count_and_time(dev, crtc, &now); > - > spin_lock(&dev->event_lock); > list_for_each_entry_safe(e, t, &dev->vblank_event_list, base.link) { > if (e->pipe != crtc) > -- > 1.9.1 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel -- Ville Syrjälä Intel OTC ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 3/3] drm: Use vblank_disable_and_save in drm_vblank_cleanup() 2014-08-06 1:22 drm vblank fixes Mario Kleiner 2014-08-06 1:22 ` [PATCH 1/3] drm: Remove drm_vblank_cleanup from drm_vblank_init error path Mario Kleiner 2014-08-06 1:22 ` [PATCH 2/3] drm: Fix emitted vblank timestamps in drm_vblank_off() Mario Kleiner @ 2014-08-06 1:22 ` Mario Kleiner 2014-08-06 10:51 ` Ville Syrjälä 2014-08-06 6:37 ` drm vblank fixes Daniel Vetter 3 siblings, 1 reply; 13+ messages in thread From: Mario Kleiner @ 2014-08-06 1:22 UTC (permalink / raw) To: dri-devel; +Cc: airlied, mario.kleiner.de, stable Calling vblank_disable_fn() will cause that function to no-op if !dev->vblank_disable_allowed for some kms drivers, e.g., on nouveau-kms. This can cause the gpu vblank irq's to not get disabled before freeing the dev->vblank array, so if a vblank irq fires and calls into drm_handle_vblank() after drm_vblank_cleanup() completes, it will cause use-after-free access to dev->vblank array. Call vblank_disable_and_save unconditionally, so vblank irqs are guaranteed to be off, before we delete the data structures on which they operate. Signed-off-by: Mario Kleiner <mario.kleiner.de@gmail.com> Cc: stable@vger.kernel.org --- drivers/gpu/drm/drm_irq.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c index 89e91e3..22e2bba9 100644 --- a/drivers/gpu/drm/drm_irq.c +++ b/drivers/gpu/drm/drm_irq.c @@ -164,6 +164,7 @@ static void vblank_disable_fn(unsigned long arg) void drm_vblank_cleanup(struct drm_device *dev) { int crtc; + unsigned long irqflags; /* Bail if the driver didn't call drm_vblank_init() */ if (dev->num_crtcs == 0) @@ -171,7 +172,9 @@ void drm_vblank_cleanup(struct drm_device *dev) for (crtc = 0; crtc < dev->num_crtcs; crtc++) { del_timer_sync(&dev->vblank[crtc].disable_timer); - vblank_disable_fn((unsigned long)&dev->vblank[crtc]); + spin_lock_irqsave(&dev->vbl_lock, irqflags); + vblank_disable_and_save(dev, crtc); + spin_unlock_irqrestore(&dev->vbl_lock, irqflags); } kfree(dev->vblank); -- 1.9.1 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 3/3] drm: Use vblank_disable_and_save in drm_vblank_cleanup() 2014-08-06 1:22 ` [PATCH 3/3] drm: Use vblank_disable_and_save in drm_vblank_cleanup() Mario Kleiner @ 2014-08-06 10:51 ` Ville Syrjälä 2014-08-06 13:57 ` Daniel Vetter 0 siblings, 1 reply; 13+ messages in thread From: Ville Syrjälä @ 2014-08-06 10:51 UTC (permalink / raw) To: Mario Kleiner; +Cc: dri-devel, airlied, stable On Wed, Aug 06, 2014 at 03:22:46AM +0200, Mario Kleiner wrote: > Calling vblank_disable_fn() will cause that function to no-op > if !dev->vblank_disable_allowed for some kms drivers, e.g., > on nouveau-kms. This can cause the gpu vblank irq's to not get > disabled before freeing the dev->vblank array, so if a > vblank irq fires and calls into drm_handle_vblank() after > drm_vblank_cleanup() completes, it will cause use-after-free > access to dev->vblank array. > > Call vblank_disable_and_save unconditionally, so vblank irqs > are guaranteed to be off, before we delete the data structures > on which they operate. > > Signed-off-by: Mario Kleiner <mario.kleiner.de@gmail.com> > Cc: stable@vger.kernel.org No idea what games nouveau is playign with that flag, but this patch should be fine at least for drivers that don't do such things. Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/drm_irq.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c > index 89e91e3..22e2bba9 100644 > --- a/drivers/gpu/drm/drm_irq.c > +++ b/drivers/gpu/drm/drm_irq.c > @@ -164,6 +164,7 @@ static void vblank_disable_fn(unsigned long arg) > void drm_vblank_cleanup(struct drm_device *dev) > { > int crtc; > + unsigned long irqflags; > > /* Bail if the driver didn't call drm_vblank_init() */ > if (dev->num_crtcs == 0) > @@ -171,7 +172,9 @@ void drm_vblank_cleanup(struct drm_device *dev) > > for (crtc = 0; crtc < dev->num_crtcs; crtc++) { > del_timer_sync(&dev->vblank[crtc].disable_timer); > - vblank_disable_fn((unsigned long)&dev->vblank[crtc]); > + spin_lock_irqsave(&dev->vbl_lock, irqflags); > + vblank_disable_and_save(dev, crtc); > + spin_unlock_irqrestore(&dev->vbl_lock, irqflags); > } > > kfree(dev->vblank); > -- > 1.9.1 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel -- Ville Syrjälä Intel OTC ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 3/3] drm: Use vblank_disable_and_save in drm_vblank_cleanup() 2014-08-06 10:51 ` Ville Syrjälä @ 2014-08-06 13:57 ` Daniel Vetter 2014-08-07 0:50 ` Mario Kleiner 0 siblings, 1 reply; 13+ messages in thread From: Daniel Vetter @ 2014-08-06 13:57 UTC (permalink / raw) To: Ville Syrjälä; +Cc: airlied, dri-devel, stable On Wed, Aug 06, 2014 at 01:51:41PM +0300, Ville Syrjälä wrote: > On Wed, Aug 06, 2014 at 03:22:46AM +0200, Mario Kleiner wrote: > > Calling vblank_disable_fn() will cause that function to no-op > > if !dev->vblank_disable_allowed for some kms drivers, e.g., > > on nouveau-kms. This can cause the gpu vblank irq's to not get > > disabled before freeing the dev->vblank array, so if a > > vblank irq fires and calls into drm_handle_vblank() after > > drm_vblank_cleanup() completes, it will cause use-after-free > > access to dev->vblank array. > > > > Call vblank_disable_and_save unconditionally, so vblank irqs > > are guaranteed to be off, before we delete the data structures > > on which they operate. > > > > Signed-off-by: Mario Kleiner <mario.kleiner.de@gmail.com> > > Cc: stable@vger.kernel.org Imo cc: stable isn't justified for these patches which fix stuff that normal users don't really see (driver load failure and module reload for kms drivers never tends to happen for normal users). So I've dropped that and pulled the 2 patches Ville reviewd into my topic/vblank-rework branch for 3.18. Thanks, Daniel > > No idea what games nouveau is playign with that flag, but this patch > should be fine at least for drivers that don't do such things. > > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > --- > > drivers/gpu/drm/drm_irq.c | 5 ++++- > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c > > index 89e91e3..22e2bba9 100644 > > --- a/drivers/gpu/drm/drm_irq.c > > +++ b/drivers/gpu/drm/drm_irq.c > > @@ -164,6 +164,7 @@ static void vblank_disable_fn(unsigned long arg) > > void drm_vblank_cleanup(struct drm_device *dev) > > { > > int crtc; > > + unsigned long irqflags; > > > > /* Bail if the driver didn't call drm_vblank_init() */ > > if (dev->num_crtcs == 0) > > @@ -171,7 +172,9 @@ void drm_vblank_cleanup(struct drm_device *dev) > > > > for (crtc = 0; crtc < dev->num_crtcs; crtc++) { > > del_timer_sync(&dev->vblank[crtc].disable_timer); > > - vblank_disable_fn((unsigned long)&dev->vblank[crtc]); > > + spin_lock_irqsave(&dev->vbl_lock, irqflags); > > + vblank_disable_and_save(dev, crtc); > > + spin_unlock_irqrestore(&dev->vbl_lock, irqflags); > > } > > > > kfree(dev->vblank); > > -- > > 1.9.1 > > > > _______________________________________________ > > dri-devel mailing list > > dri-devel@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/dri-devel > > -- > Ville Syrjälä > Intel OTC > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 3/3] drm: Use vblank_disable_and_save in drm_vblank_cleanup() 2014-08-06 13:57 ` Daniel Vetter @ 2014-08-07 0:50 ` Mario Kleiner 2014-08-07 6:50 ` Daniel Vetter 2014-08-07 6:57 ` Daniel Vetter 0 siblings, 2 replies; 13+ messages in thread From: Mario Kleiner @ 2014-08-07 0:50 UTC (permalink / raw) To: Daniel Vetter, Ville Syrjälä; +Cc: airlied, dri-devel, stable On 08/06/2014 03:57 PM, Daniel Vetter wrote: > On Wed, Aug 06, 2014 at 01:51:41PM +0300, Ville Syrjälä wrote: >> On Wed, Aug 06, 2014 at 03:22:46AM +0200, Mario Kleiner wrote: >>> Calling vblank_disable_fn() will cause that function to no-op >>> if !dev->vblank_disable_allowed for some kms drivers, e.g., >>> on nouveau-kms. This can cause the gpu vblank irq's to not get >>> disabled before freeing the dev->vblank array, so if a >>> vblank irq fires and calls into drm_handle_vblank() after >>> drm_vblank_cleanup() completes, it will cause use-after-free >>> access to dev->vblank array. >>> >>> Call vblank_disable_and_save unconditionally, so vblank irqs >>> are guaranteed to be off, before we delete the data structures >>> on which they operate. >>> >>> Signed-off-by: Mario Kleiner <mario.kleiner.de@gmail.com> >>> Cc: stable@vger.kernel.org > Imo cc: stable isn't justified for these patches which fix stuff that > normal users don't really see (driver load failure and module reload for > kms drivers never tends to happen for normal users). > > So I've dropped that and pulled the 2 patches Ville reviewd into my > topic/vblank-rework branch for 3.18. > > Thanks, Daniel Ok, good with me, thanks. Ville, thanks for the review. I'll review and test your vblank series next week when i have access to suitable machines and enough time. I need to go through this in single-step mode, vblank on/off changes always make me nervous, given how dependent my main application is on this for its timing, so i want to move through it in slow motion. Btw. wrt to nouveau "No idea what games nouveau is playign with that flag, but this patch should be fine at least for drivers that don't do such things." (Villes comment). Nouveau currently doesn't support hw vblank counter queries at all. The dev->driver->get_vblank_count() is just hooked up to drm_vblank_count(), so it's a no-op. Therefore nouveau can't allow disabling of vblank irq during "normal" operation as it would lose all vblank counts during the off period. That's why it leaves dev->vblank_disable_allowed = false; Pre NV-50 apparently doesn't have any hw vblank counter register, but NV50+ seems to have one. I'll probably give implementing this a try for 3.18 if nobody else does. I'm not sure about all the new embedded drivers, if they have hw vblank counters? thanks, -mario > >> No idea what games nouveau is playign with that flag, but this patch >> should be fine at least for drivers that don't do such things. >> >> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> >> >>> --- >>> drivers/gpu/drm/drm_irq.c | 5 ++++- >>> 1 file changed, 4 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c >>> index 89e91e3..22e2bba9 100644 >>> --- a/drivers/gpu/drm/drm_irq.c >>> +++ b/drivers/gpu/drm/drm_irq.c >>> @@ -164,6 +164,7 @@ static void vblank_disable_fn(unsigned long arg) >>> void drm_vblank_cleanup(struct drm_device *dev) >>> { >>> int crtc; >>> + unsigned long irqflags; >>> >>> /* Bail if the driver didn't call drm_vblank_init() */ >>> if (dev->num_crtcs == 0) >>> @@ -171,7 +172,9 @@ void drm_vblank_cleanup(struct drm_device *dev) >>> >>> for (crtc = 0; crtc < dev->num_crtcs; crtc++) { >>> del_timer_sync(&dev->vblank[crtc].disable_timer); >>> - vblank_disable_fn((unsigned long)&dev->vblank[crtc]); >>> + spin_lock_irqsave(&dev->vbl_lock, irqflags); >>> + vblank_disable_and_save(dev, crtc); >>> + spin_unlock_irqrestore(&dev->vbl_lock, irqflags); >>> } >>> >>> kfree(dev->vblank); >>> -- >>> 1.9.1 >>> >>> _______________________________________________ >>> dri-devel mailing list >>> dri-devel@lists.freedesktop.org >>> http://lists.freedesktop.org/mailman/listinfo/dri-devel >> -- >> Ville Syrjälä >> Intel OTC >> _______________________________________________ >> dri-devel mailing list >> dri-devel@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 3/3] drm: Use vblank_disable_and_save in drm_vblank_cleanup() 2014-08-07 0:50 ` Mario Kleiner @ 2014-08-07 6:50 ` Daniel Vetter 2014-08-07 15:13 ` Mario Kleiner 2014-08-07 6:57 ` Daniel Vetter 1 sibling, 1 reply; 13+ messages in thread From: Daniel Vetter @ 2014-08-07 6:50 UTC (permalink / raw) To: Mario Kleiner; +Cc: Ville Syrjälä, Dave Airlie, stable, dri-devel On Thu, Aug 7, 2014 at 2:50 AM, Mario Kleiner <mario.kleiner.de@gmail.com> wrote: > I'm not sure about all the new embedded drivers, if they have hw vblank > counters? Quick grep says a lot don't have it or at least not implemented - they use drm_vblank_count. Thinking about this, should we use that as a signal to also set dev->vblnka_disable_allowed = false in drm_vblank_init? -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 3/3] drm: Use vblank_disable_and_save in drm_vblank_cleanup() 2014-08-07 6:50 ` Daniel Vetter @ 2014-08-07 15:13 ` Mario Kleiner 0 siblings, 0 replies; 13+ messages in thread From: Mario Kleiner @ 2014-08-07 15:13 UTC (permalink / raw) To: Daniel Vetter; +Cc: Dave Airlie, dri-devel, stable On 08/07/2014 08:50 AM, Daniel Vetter wrote: > On Thu, Aug 7, 2014 at 2:50 AM, Mario Kleiner > <mario.kleiner.de@gmail.com> wrote: >> I'm not sure about all the new embedded drivers, if they have hw vblank >> counters? > Quick grep says a lot don't have it or at least not implemented - they > use drm_vblank_count. Thinking about this, should we use that as a > signal to also set dev->vblnka_disable_allowed = false in > drm_vblank_init? > -Daniel dev->vblank_disable_allowed = false; is already the default set in drm_vblank_init(). I think it was the idea of that flag that drivers which support a somewhat correct vblank dis/enable (= have useable hw vblank counters) opt-in to the vblank disable after some idle time by setting it to true. The strange thing is that unconditional dev->vblank_disable_allowed = true in drm_vblank_post_modeset()? It's there since the first introduction of the flag. The i915, gma500, armada and exynos drivers explicitely set the flag to true to opt-in to the auto vblank disable. radeon gets it implicitely set by calling the vblank_post_modeset function. Tegra also gets it via the post_modeset, and nouveau on old cards with nv04 display engine. armada, exynos and tegra don't have proper hw vblank counter queries, but opt-in, so those will lose vblank counts whenever vblank irqs get turned off. It's a bit all over the place. -mario ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 3/3] drm: Use vblank_disable_and_save in drm_vblank_cleanup() 2014-08-07 0:50 ` Mario Kleiner 2014-08-07 6:50 ` Daniel Vetter @ 2014-08-07 6:57 ` Daniel Vetter 1 sibling, 0 replies; 13+ messages in thread From: Daniel Vetter @ 2014-08-07 6:57 UTC (permalink / raw) To: Mario Kleiner; +Cc: Ville Syrjälä, Dave Airlie, stable, dri-devel On Thu, Aug 7, 2014 at 2:50 AM, Mario Kleiner <mario.kleiner.de@gmail.com> wrote: > Ok, good with me, thanks. Ville, thanks for the review. I'll review and test > your vblank series next week when i have access to suitable machines and > enough time. I need to go through this in single-step mode, vblank on/off > changes always make me nervous, given how dependent my main application is > on this for its timing, so i want to move through it in slow motion. btw I've pulled in your patches and the one's from Ville that I think are sane into a topic branch, in case that helps: http://cgit.freedesktop.org/drm-intel/log/?h=topic/vblank-rework Cheers, Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: drm vblank fixes 2014-08-06 1:22 drm vblank fixes Mario Kleiner ` (2 preceding siblings ...) 2014-08-06 1:22 ` [PATCH 3/3] drm: Use vblank_disable_and_save in drm_vblank_cleanup() Mario Kleiner @ 2014-08-06 6:37 ` Daniel Vetter 3 siblings, 0 replies; 13+ messages in thread From: Daniel Vetter @ 2014-08-06 6:37 UTC (permalink / raw) To: Mario Kleiner, Syrjala, Ville; +Cc: Dave Airlie, dri-devel On Wed, Aug 6, 2014 at 3:22 AM, Mario Kleiner <mario.kleiner.de@gmail.com> wrote: > some small bug fixes for some small bugs i saw > when looking at the current drm vblank handling code. > > All patches are rather simple, all compile-tested against > drm-next, but only the drm_vblank_off() one (patch 2) > tested in "real life" so far. Ville has an older patch series touching the same code and afaict fixing some of the same bugs. Unfortunately it's not in yet. Have you looked at that one? -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2014-08-07 15:13 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2014-08-06 1:22 drm vblank fixes Mario Kleiner 2014-08-06 1:22 ` [PATCH 1/3] drm: Remove drm_vblank_cleanup from drm_vblank_init error path Mario Kleiner 2014-08-06 10:51 ` Ville Syrjälä 2014-08-06 1:22 ` [PATCH 2/3] drm: Fix emitted vblank timestamps in drm_vblank_off() Mario Kleiner 2014-08-06 10:53 ` Ville Syrjälä 2014-08-06 1:22 ` [PATCH 3/3] drm: Use vblank_disable_and_save in drm_vblank_cleanup() Mario Kleiner 2014-08-06 10:51 ` Ville Syrjälä 2014-08-06 13:57 ` Daniel Vetter 2014-08-07 0:50 ` Mario Kleiner 2014-08-07 6:50 ` Daniel Vetter 2014-08-07 15:13 ` Mario Kleiner 2014-08-07 6:57 ` Daniel Vetter 2014-08-06 6:37 ` drm vblank fixes Daniel Vetter
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.