From: Christopher James Halse Rogers christopher.halse.rogers@canonical.com
This is the least-bad behaviour. It means that we signal the vblank event before it actually happens, but since we're disabling vblanks there's no guarantee that it will *ever* happen otherwise.
This prevents GL applications which use WaitMSC from hanging indefinitely.
Signed-off-by: Christopher James Halse Rogers christopher.halse.rogers@canonical.com --- drivers/gpu/drm/drm_irq.c | 23 +++++++++++++++++++++++ 1 files changed, 23 insertions(+), 0 deletions(-)
diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c index 741457b..a1f12cb 100644 --- a/drivers/gpu/drm/drm_irq.c +++ b/drivers/gpu/drm/drm_irq.c @@ -932,11 +932,34 @@ EXPORT_SYMBOL(drm_vblank_put);
void drm_vblank_off(struct drm_device *dev, int crtc) { + struct drm_pending_vblank_event *e, *t; + struct timeval now; unsigned long irqflags; + unsigned int seq;
spin_lock_irqsave(&dev->vbl_lock, irqflags); vblank_disable_and_save(dev, crtc); DRM_WAKEUP(&dev->vbl_queue[crtc]); + + /* Send any queued vblank events, lest the natives grow disquiet */ + seq = drm_vblank_count_and_time(dev, crtc, &now); + list_for_each_entry_safe(e, t, &dev->vblank_event_list, base.link) { + if (e->pipe != crtc) + continue; + DRM_DEBUG("Sending premature vblank event on disable: \ + wanted %d, current %d\n", + e->event.sequence, seq); + + e->event.sequence = seq; + e->event.tv_sec = now.tv_sec; + e->event.tv_usec = now.tv_usec; + drm_vblank_put(dev, e->pipe); + list_move_tail(&e->base.link, &e->base.file_priv->event_list); + wake_up_interruptible(&e->base.file_priv->event_wait); + trace_drm_vblank_event_delivered(e->base.pid, e->pipe, + e->event.sequence); + } + spin_unlock_irqrestore(&dev->vbl_lock, irqflags); } EXPORT_SYMBOL(drm_vblank_off);
From: Christopher James Halse Rogers christopher.halse.rogers@canonical.com
After emitting all the waiting vblank events no-one should hold a vblank reference. Emit a warning if this is not the case.
Signed-off-by: Christopher James Halse Rogers christopher.halse.rogers@canonical.com --- drivers/gpu/drm/drm_irq.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c index a1f12cb..72407fa 100644 --- a/drivers/gpu/drm/drm_irq.c +++ b/drivers/gpu/drm/drm_irq.c @@ -960,6 +960,7 @@ void drm_vblank_off(struct drm_device *dev, int crtc) e->event.sequence); }
+ WARN_ON(atomic_read(&dev->vblank_refcount[crtc]) != 0); spin_unlock_irqrestore(&dev->vbl_lock, irqflags); } EXPORT_SYMBOL(drm_vblank_off);
On Mit, 2011-04-27 at 16:10 +1000, christopher.halse.rogers@canonical.com wrote:
From: Christopher James Halse Rogers christopher.halse.rogers@canonical.com
After emitting all the waiting vblank events no-one should hold a vblank reference. Emit a warning if this is not the case.
Signed-off-by: Christopher James Halse Rogers christopher.halse.rogers@canonical.com
drivers/gpu/drm/drm_irq.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c index a1f12cb..72407fa 100644 --- a/drivers/gpu/drm/drm_irq.c +++ b/drivers/gpu/drm/drm_irq.c @@ -960,6 +960,7 @@ void drm_vblank_off(struct drm_device *dev, int crtc) e->event.sequence); }
- WARN_ON(atomic_read(&dev->vblank_refcount[crtc]) != 0); spin_unlock_irqrestore(&dev->vbl_lock, irqflags);
} EXPORT_SYMBOL(drm_vblank_off);
Reviewed-by: Michel Dänzer michel@daenzer.net
From: Christopher James Halse Rogers christopher.halse.rogers@canonical.com
Signed-off-by: Christopher James Halse Rogers christopher.halse.rogers@canonical.com --- drivers/gpu/drm/drm_irq.c | 39 ++++++++++++++++----------------------- 1 files changed, 16 insertions(+), 23 deletions(-)
diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c index 72407fa..485714b 100644 --- a/drivers/gpu/drm/drm_irq.c +++ b/drivers/gpu/drm/drm_irq.c @@ -930,6 +930,18 @@ void drm_vblank_put(struct drm_device *dev, int crtc) } EXPORT_SYMBOL(drm_vblank_put);
+static void drm_emit_vblank_event (struct drm_pending_vblank_event *e, + unsigned int seq, struct timeval *now) +{ + e->event.sequence = seq; + e->event.tv_sec = now->tv_sec; + e->event.tv_usec = now->tv_usec; + list_move_tail(&e->base.link, &e->base.file_priv->event_list); + wake_up_interruptible(&e->base.file_priv->event_wait); + trace_drm_vblank_event_delivered(e->base.pid, e->pipe, + e->event.sequence); +} + void drm_vblank_off(struct drm_device *dev, int crtc) { struct drm_pending_vblank_event *e, *t; @@ -950,14 +962,8 @@ void drm_vblank_off(struct drm_device *dev, int crtc) wanted %d, current %d\n", e->event.sequence, seq);
- e->event.sequence = seq; - e->event.tv_sec = now.tv_sec; - e->event.tv_usec = now.tv_usec; drm_vblank_put(dev, e->pipe); - list_move_tail(&e->base.link, &e->base.file_priv->event_list); - wake_up_interruptible(&e->base.file_priv->event_wait); - trace_drm_vblank_event_delivered(e->base.pid, e->pipe, - e->event.sequence); + drm_emit_vblank_event(e, seq, &now); }
WARN_ON(atomic_read(&dev->vblank_refcount[crtc]) != 0); @@ -1103,18 +1109,11 @@ static int drm_queue_vblank_event(struct drm_device *dev, int pipe, vblwait->request.sequence);
e->event.sequence = vblwait->request.sequence; + list_add_tail(&e->base.link, &dev->vblank_event_list); if ((seq - vblwait->request.sequence) <= (1 << 23)) { - e->event.sequence = seq; - e->event.tv_sec = now.tv_sec; - e->event.tv_usec = now.tv_usec; drm_vblank_put(dev, pipe); - list_add_tail(&e->base.link, &e->base.file_priv->event_list); - wake_up_interruptible(&e->base.file_priv->event_wait); - vblwait->reply.sequence = seq; - trace_drm_vblank_event_delivered(current->pid, pipe, - vblwait->request.sequence); + drm_emit_vblank_event(e, seq, &now); } else { - list_add_tail(&e->base.link, &dev->vblank_event_list); vblwait->reply.sequence = vblwait->request.sequence; }
@@ -1248,14 +1247,8 @@ void drm_handle_vblank_events(struct drm_device *dev, int crtc) DRM_DEBUG("vblank event on %d, current %d\n", e->event.sequence, seq);
- e->event.sequence = seq; - e->event.tv_sec = now.tv_sec; - e->event.tv_usec = now.tv_usec; drm_vblank_put(dev, e->pipe); - list_move_tail(&e->base.link, &e->base.file_priv->event_list); - wake_up_interruptible(&e->base.file_priv->event_wait); - trace_drm_vblank_event_delivered(e->base.pid, e->pipe, - e->event.sequence); + drm_emit_vblank_event(e, seq, &now); }
spin_unlock_irqrestore(&dev->event_lock, flags);
On Mit, 2011-04-27 at 16:10 +1000, christopher.halse.rogers@canonical.com wrote:
From: Christopher James Halse Rogers christopher.halse.rogers@canonical.com
Signed-off-by: Christopher James Halse Rogers christopher.halse.rogers@canonical.com
drivers/gpu/drm/drm_irq.c | 39 ++++++++++++++++----------------------- 1 files changed, 16 insertions(+), 23 deletions(-)
diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c index 72407fa..485714b 100644 --- a/drivers/gpu/drm/drm_irq.c +++ b/drivers/gpu/drm/drm_irq.c @@ -930,6 +930,18 @@ void drm_vblank_put(struct drm_device *dev, int crtc) } EXPORT_SYMBOL(drm_vblank_put);
+static void drm_emit_vblank_event (struct drm_pending_vblank_event *e,
unsigned int seq, struct timeval *now)+{
- e->event.sequence = seq;
- e->event.tv_sec = now->tv_sec;
- e->event.tv_usec = now->tv_usec;
- list_move_tail(&e->base.link, &e->base.file_priv->event_list);
- wake_up_interruptible(&e->base.file_priv->event_wait);
- trace_drm_vblank_event_delivered(e->base.pid, e->pipe,
e->event.sequence);+}
void drm_vblank_off(struct drm_device *dev, int crtc) { struct drm_pending_vblank_event *e, *t; @@ -950,14 +962,8 @@ void drm_vblank_off(struct drm_device *dev, int crtc) wanted %d, current %d\n", e->event.sequence, seq);
e->event.sequence = seq;e->event.tv_sec = now.tv_sec;e->event.tv_usec = now.tv_usec;list_move_tail(&e->base.link, &e->base.file_priv->event_list);wake_up_interruptible(&e->base.file_priv->event_wait);trace_drm_vblank_event_delivered(e->base.pid, e->pipe,e->event.sequence);
drm_emit_vblank_event(e, seq, &now);
Makes sense, but shouldn't the drm_vblank_put() calls move into drm_emit_vblank_event() as well?
On Wed, 2011-04-27 at 10:36 +0200, Michel Dänzer wrote:
On Mit, 2011-04-27 at 16:10 +1000, christopher.halse.rogers@canonical.com wrote:
From: Christopher James Halse Rogers christopher.halse.rogers@canonical.com
Signed-off-by: Christopher James Halse Rogers christopher.halse.rogers@canonical.com
drivers/gpu/drm/drm_irq.c | 39 ++++++++++++++++----------------------- 1 files changed, 16 insertions(+), 23 deletions(-)
diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c index 72407fa..485714b 100644 --- a/drivers/gpu/drm/drm_irq.c +++ b/drivers/gpu/drm/drm_irq.c @@ -930,6 +930,18 @@ void drm_vblank_put(struct drm_device *dev, int crtc) } EXPORT_SYMBOL(drm_vblank_put);
+static void drm_emit_vblank_event (struct drm_pending_vblank_event *e,
unsigned int seq, struct timeval *now)+{
- e->event.sequence = seq;
- e->event.tv_sec = now->tv_sec;
- e->event.tv_usec = now->tv_usec;
- list_move_tail(&e->base.link, &e->base.file_priv->event_list);
- wake_up_interruptible(&e->base.file_priv->event_wait);
- trace_drm_vblank_event_delivered(e->base.pid, e->pipe,
e->event.sequence);+}
void drm_vblank_off(struct drm_device *dev, int crtc) { struct drm_pending_vblank_event *e, *t; @@ -950,14 +962,8 @@ void drm_vblank_off(struct drm_device *dev, int crtc) wanted %d, current %d\n", e->event.sequence, seq);
e->event.sequence = seq;e->event.tv_sec = now.tv_sec;e->event.tv_usec = now.tv_usec;list_move_tail(&e->base.link, &e->base.file_priv->event_list);wake_up_interruptible(&e->base.file_priv->event_wait);trace_drm_vblank_event_delivered(e->base.pid, e->pipe,e->event.sequence);
drm_emit_vblank_event(e, seq, &now);Makes sense, but shouldn't the drm_vblank_put() calls move into drm_emit_vblank_event() as well?
Possibly. I felt that dropping the vblank reference was a conceptually distinct action from emitting the vblank event, which is why I left it out. I'd be happy enough to have it as a part of drm_emit_vblank_event, though.
On Mit, 2011-04-27 at 18:48 +1000, Christopher James Halse Rogers wrote:
On Wed, 2011-04-27 at 10:36 +0200, Michel Dänzer wrote:
On Mit, 2011-04-27 at 16:10 +1000, christopher.halse.rogers@canonical.com wrote:
From: Christopher James Halse Rogers christopher.halse.rogers@canonical.com
Signed-off-by: Christopher James Halse Rogers christopher.halse.rogers@canonical.com
drivers/gpu/drm/drm_irq.c | 39 ++++++++++++++++----------------------- 1 files changed, 16 insertions(+), 23 deletions(-)
diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c index 72407fa..485714b 100644 --- a/drivers/gpu/drm/drm_irq.c +++ b/drivers/gpu/drm/drm_irq.c @@ -930,6 +930,18 @@ void drm_vblank_put(struct drm_device *dev, int crtc) } EXPORT_SYMBOL(drm_vblank_put);
+static void drm_emit_vblank_event (struct drm_pending_vblank_event *e,
unsigned int seq, struct timeval *now)+{
- e->event.sequence = seq;
- e->event.tv_sec = now->tv_sec;
- e->event.tv_usec = now->tv_usec;
- list_move_tail(&e->base.link, &e->base.file_priv->event_list);
- wake_up_interruptible(&e->base.file_priv->event_wait);
- trace_drm_vblank_event_delivered(e->base.pid, e->pipe,
e->event.sequence);+}
void drm_vblank_off(struct drm_device *dev, int crtc) { struct drm_pending_vblank_event *e, *t; @@ -950,14 +962,8 @@ void drm_vblank_off(struct drm_device *dev, int crtc) wanted %d, current %d\n", e->event.sequence, seq);
e->event.sequence = seq;e->event.tv_sec = now.tv_sec;e->event.tv_usec = now.tv_usec;list_move_tail(&e->base.link, &e->base.file_priv->event_list);wake_up_interruptible(&e->base.file_priv->event_wait);trace_drm_vblank_event_delivered(e->base.pid, e->pipe,e->event.sequence);
drm_emit_vblank_event(e, seq, &now);Makes sense, but shouldn't the drm_vblank_put() calls move into drm_emit_vblank_event() as well?
Possibly. I felt that dropping the vblank reference was a conceptually distinct action from emitting the vblank event, which is why I left it out. I'd be happy enough to have it as a part of drm_emit_vblank_event, though.
I think they belong together, as the reference was taken for the pending event.
On Wed, 27 Apr 2011 16:10:59 +1000 christopher.halse.rogers@canonical.com wrote:
From: Christopher James Halse Rogers christopher.halse.rogers@canonical.com
Signed-off-by: Christopher James Halse Rogers christopher.halse.rogers@canonical.com
drivers/gpu/drm/drm_irq.c | 39 ++++++++++++++++----------------------- 1 files changed, 16 insertions(+), 23 deletions(-)
Oh I see you already addressed my last comment. :)
I think Michel is right about the put; the event queue holds the ref and the emit should drop it, so keeping them together seems good to me.
Other than that, Reviewed-by: Jesse Barnes jbarnes@virtuousgeek.org
same for the first patch now that you've addressed my cleanup issue.
From: Christopher James Halse Rogers christopher.halse.rogers@canonical.com
v2: Also pull out the drm_vblank_put call. Signed-off-by: Christopher James Halse Rogers christopher.halse.rogers@canonical.com --- drivers/gpu/drm/drm_irq.c | 44 ++++++++++++++++++-------------------------- 1 files changed, 18 insertions(+), 26 deletions(-)
diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c index 982ca8c..da56685 100644 --- a/drivers/gpu/drm/drm_irq.c +++ b/drivers/gpu/drm/drm_irq.c @@ -931,6 +931,20 @@ void drm_vblank_put(struct drm_device *dev, int crtc) } EXPORT_SYMBOL(drm_vblank_put);
+static void drm_emit_vblank_event (struct drm_device *dev, + struct drm_pending_vblank_event *e, + unsigned int seq, struct timeval *now) +{ + e->event.sequence = seq; + e->event.tv_sec = now->tv_sec; + e->event.tv_usec = now->tv_usec; + drm_vblank_put(dev, e->pipe); + list_move_tail(&e->base.link, &e->base.file_priv->event_list); + wake_up_interruptible(&e->base.file_priv->event_wait); + trace_drm_vblank_event_delivered(e->base.pid, e->pipe, + e->event.sequence); +} + void drm_vblank_off(struct drm_device *dev, int crtc) { struct drm_pending_vblank_event *e, *t; @@ -951,14 +965,7 @@ void drm_vblank_off(struct drm_device *dev, int crtc) wanted %d, current %d\n", e->event.sequence, seq);
- e->event.sequence = seq; - e->event.tv_sec = now.tv_sec; - e->event.tv_usec = now.tv_usec; - drm_vblank_put(dev, e->pipe); - list_move_tail(&e->base.link, &e->base.file_priv->event_list); - wake_up_interruptible(&e->base.file_priv->event_wait); - trace_drm_vblank_event_delivered(e->base.pid, e->pipe, - e->event.sequence); + drm_emit_vblank_event(dev, e, seq, &now); }
WARN_ON(atomic_read(&dev->vblank_refcount[crtc]) != 0); @@ -1104,18 +1111,10 @@ static int drm_queue_vblank_event(struct drm_device *dev, int pipe, vblwait->request.sequence);
e->event.sequence = vblwait->request.sequence; + list_add_tail(&e->base.link, &dev->vblank_event_list); if ((seq - vblwait->request.sequence) <= (1 << 23)) { - e->event.sequence = seq; - e->event.tv_sec = now.tv_sec; - e->event.tv_usec = now.tv_usec; - drm_vblank_put(dev, pipe); - list_add_tail(&e->base.link, &e->base.file_priv->event_list); - wake_up_interruptible(&e->base.file_priv->event_wait); - vblwait->reply.sequence = seq; - trace_drm_vblank_event_delivered(current->pid, pipe, - vblwait->request.sequence); + drm_emit_vblank_event(dev, e, seq, &now); } else { - list_add_tail(&e->base.link, &dev->vblank_event_list); vblwait->reply.sequence = vblwait->request.sequence; }
@@ -1249,14 +1248,7 @@ void drm_handle_vblank_events(struct drm_device *dev, int crtc) DRM_DEBUG("vblank event on %d, current %d\n", e->event.sequence, seq);
- e->event.sequence = seq; - e->event.tv_sec = now.tv_sec; - e->event.tv_usec = now.tv_usec; - drm_vblank_put(dev, e->pipe); - list_move_tail(&e->base.link, &e->base.file_priv->event_list); - wake_up_interruptible(&e->base.file_priv->event_wait); - trace_drm_vblank_event_delivered(e->base.pid, e->pipe, - e->event.sequence); + drm_emit_vblank_event(dev, e, seq, &now); }
spin_unlock_irqrestore(&dev->event_lock, flags);
On Fre, 2011-04-29 at 13:57 +1000, christopher.halse.rogers@canonical.com wrote:
From: Christopher James Halse Rogers christopher.halse.rogers@canonical.com
v2: Also pull out the drm_vblank_put call. Signed-off-by: Christopher James Halse Rogers christopher.halse.rogers@canonical.com
Reviewed-by: Michel Dänzer michel@daenzer.net
On Fri, Apr 29, 2011 at 01:57:10PM +1000, christopher.halse.rogers@canonical.com wrote:
From: Christopher James Halse Rogers christopher.halse.rogers@canonical.com
v2: Also pull out the drm_vblank_put call. Signed-off-by: Christopher James Halse Rogers christopher.halse.rogers@canonical.com
drivers/gpu/drm/drm_irq.c | 44 ++++++++++++++++++-------------------------- 1 files changed, 18 insertions(+), 26 deletions(-)
diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c index 982ca8c..da56685 100644 --- a/drivers/gpu/drm/drm_irq.c +++ b/drivers/gpu/drm/drm_irq.c @@ -931,6 +931,20 @@ void drm_vblank_put(struct drm_device *dev, int crtc) } EXPORT_SYMBOL(drm_vblank_put);
+static void drm_emit_vblank_event (struct drm_device *dev,
struct drm_pending_vblank_event *e,unsigned int seq, struct timeval *now)+{
- e->event.sequence = seq;
- e->event.tv_sec = now->tv_sec;
- e->event.tv_usec = now->tv_usec;
- drm_vblank_put(dev, e->pipe);
- list_move_tail(&e->base.link, &e->base.file_priv->event_list);
- wake_up_interruptible(&e->base.file_priv->event_wait);
- trace_drm_vblank_event_delivered(e->base.pid, e->pipe,
e->event.sequence);+}
void drm_vblank_off(struct drm_device *dev, int crtc) { struct drm_pending_vblank_event *e, *t; @@ -951,14 +965,7 @@ void drm_vblank_off(struct drm_device *dev, int crtc) wanted %d, current %d\n", e->event.sequence, seq);
e->event.sequence = seq;e->event.tv_sec = now.tv_sec;e->event.tv_usec = now.tv_usec;drm_vblank_put(dev, e->pipe);list_move_tail(&e->base.link, &e->base.file_priv->event_list);wake_up_interruptible(&e->base.file_priv->event_wait);trace_drm_vblank_event_delivered(e->base.pid, e->pipe,e->event.sequence);
drm_emit_vblank_event(dev, e, seq, &now);}
WARN_ON(atomic_read(&dev->vblank_refcount[crtc]) != 0);
@@ -1104,18 +1111,10 @@ static int drm_queue_vblank_event(struct drm_device *dev, int pipe, vblwait->request.sequence);
e->event.sequence = vblwait->request.sequence;
- list_add_tail(&e->base.link, &dev->vblank_event_list);
Is &dev->vblank_event_list == &e->base.file_priv->event_list? If they are not equal this is changing the behavior...
if ((seq - vblwait->request.sequence) <= (1 << 23)) {
e->event.sequence = seq;e->event.tv_sec = now.tv_sec;e->event.tv_usec = now.tv_usec;drm_vblank_put(dev, pipe);list_add_tail(&e->base.link, &e->base.file_priv->event_list);wake_up_interruptible(&e->base.file_priv->event_wait);vblwait->reply.sequence = seq;
Is it OK to drop this substitution?
trace_drm_vblank_event_delivered(current->pid, pipe,vblwait->request.sequence);
drm_emit_vblank_event(dev, e, seq, &now);
list_add_tail(&e->base.link, &dev->vblank_event_list);@@ -1249,14 +1248,7 @@ void drm_handle_vblank_events(struct drm_device *dev, int crtc) DRM_DEBUG("vblank event on %d, current %d\n", e->event.sequence, seq);
e->event.sequence = seq;e->event.tv_sec = now.tv_sec;e->event.tv_usec = now.tv_usec;drm_vblank_put(dev, e->pipe);list_move_tail(&e->base.link, &e->base.file_priv->event_list);wake_up_interruptible(&e->base.file_priv->event_wait);trace_drm_vblank_event_delivered(e->base.pid, e->pipe,e->event.sequence);
drm_emit_vblank_event(dev, e, seq, &now);}
spin_unlock_irqrestore(&dev->event_lock, flags);
--
On Fri, 2011-04-29 at 17:55 +0200, Marcin Slusarz wrote:
On Fri, Apr 29, 2011 at 01:57:10PM +1000, christopher.halse.rogers@canonical.com wrote:
From: Christopher James Halse Rogers christopher.halse.rogers@canonical.com
v2: Also pull out the drm_vblank_put call. Signed-off-by: Christopher James Halse Rogers christopher.halse.rogers@canonical.com
drivers/gpu/drm/drm_irq.c | 44 ++++++++++++++++++-------------------------- 1 files changed, 18 insertions(+), 26 deletions(-)
diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c index 982ca8c..da56685 100644 --- a/drivers/gpu/drm/drm_irq.c +++ b/drivers/gpu/drm/drm_irq.c @@ -931,6 +931,20 @@ void drm_vblank_put(struct drm_device *dev, int crtc) } EXPORT_SYMBOL(drm_vblank_put);
+static void drm_emit_vblank_event (struct drm_device *dev,
struct drm_pending_vblank_event *e,unsigned int seq, struct timeval *now)+{
- e->event.sequence = seq;
- e->event.tv_sec = now->tv_sec;
- e->event.tv_usec = now->tv_usec;
- drm_vblank_put(dev, e->pipe);
- list_move_tail(&e->base.link, &e->base.file_priv->event_list);
- wake_up_interruptible(&e->base.file_priv->event_wait);
- trace_drm_vblank_event_delivered(e->base.pid, e->pipe,
e->event.sequence);+}
void drm_vblank_off(struct drm_device *dev, int crtc) { struct drm_pending_vblank_event *e, *t; @@ -951,14 +965,7 @@ void drm_vblank_off(struct drm_device *dev, int crtc) wanted %d, current %d\n", e->event.sequence, seq);
e->event.sequence = seq;e->event.tv_sec = now.tv_sec;e->event.tv_usec = now.tv_usec;drm_vblank_put(dev, e->pipe);list_move_tail(&e->base.link, &e->base.file_priv->event_list);wake_up_interruptible(&e->base.file_priv->event_wait);trace_drm_vblank_event_delivered(e->base.pid, e->pipe,e->event.sequence);
drm_emit_vblank_event(dev, e, seq, &now);}
WARN_ON(atomic_read(&dev->vblank_refcount[crtc]) != 0);
@@ -1104,18 +1111,10 @@ static int drm_queue_vblank_event(struct drm_device *dev, int pipe, vblwait->request.sequence);
e->event.sequence = vblwait->request.sequence;
- list_add_tail(&e->base.link, &dev->vblank_event_list);
Is &dev->vblank_event_list == &e->base.file_priv->event_list? If they are not equal this is changing the behavior...
This gets moved to e->base.file_priv->event_list in drm_emit_vblank_event (). So it's doing a bit of useless work setting a couple of pointers, but this doesn't change the behaviour.
if ((seq - vblwait->request.sequence) <= (1 << 23)) {
e->event.sequence = seq;e->event.tv_sec = now.tv_sec;e->event.tv_usec = now.tv_usec;drm_vblank_put(dev, pipe);list_add_tail(&e->base.link, &e->base.file_priv->event_list);wake_up_interruptible(&e->base.file_priv->event_wait);vblwait->reply.sequence = seq;Is it OK to drop this substitution?
Ahem. No, it's not. That's an overzealous delete key going off. Hm. Either the clients I was testing don't care about the reply, or the clients I was testing always set _DRM_BLANK_NEXTONMISS. I think it's the latter.
Updated patch coming.
trace_drm_vblank_event_delivered(current->pid, pipe,vblwait->request.sequence);
drm_emit_vblank_event(dev, e, seq, &now);
list_add_tail(&e->base.link, &dev->vblank_event_list);@@ -1249,14 +1248,7 @@ void drm_handle_vblank_events(struct drm_device *dev, int crtc) DRM_DEBUG("vblank event on %d, current %d\n", e->event.sequence, seq);
e->event.sequence = seq;e->event.tv_sec = now.tv_sec;e->event.tv_usec = now.tv_usec;drm_vblank_put(dev, e->pipe);list_move_tail(&e->base.link, &e->base.file_priv->event_list);wake_up_interruptible(&e->base.file_priv->event_wait);trace_drm_vblank_event_delivered(e->base.pid, e->pipe,e->event.sequence);
drm_emit_vblank_event(dev, e, seq, &now);}
spin_unlock_irqrestore(&dev->event_lock, flags);
--
From: Christopher James Halse Rogers christopher.halse.rogers@canonical.com
v2: Also pull out the drm_vblank_put call. v3: Always set the reply.sequence value properly.
Signed-off-by: Christopher James Halse Rogers christopher.halse.rogers@canonical.com --- drivers/gpu/drm/drm_irq.c | 43 ++++++++++++++++++------------------------- 1 files changed, 18 insertions(+), 25 deletions(-)
diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c index 982ca8c..c05fe41 100644 --- a/drivers/gpu/drm/drm_irq.c +++ b/drivers/gpu/drm/drm_irq.c @@ -931,6 +931,20 @@ void drm_vblank_put(struct drm_device *dev, int crtc) } EXPORT_SYMBOL(drm_vblank_put);
+static void drm_emit_vblank_event (struct drm_device *dev, + struct drm_pending_vblank_event *e, + unsigned int seq, struct timeval *now) +{ + e->event.sequence = seq; + e->event.tv_sec = now->tv_sec; + e->event.tv_usec = now->tv_usec; + drm_vblank_put(dev, e->pipe); + list_move_tail(&e->base.link, &e->base.file_priv->event_list); + wake_up_interruptible(&e->base.file_priv->event_wait); + trace_drm_vblank_event_delivered(e->base.pid, e->pipe, + e->event.sequence); +} + void drm_vblank_off(struct drm_device *dev, int crtc) { struct drm_pending_vblank_event *e, *t; @@ -951,14 +965,7 @@ void drm_vblank_off(struct drm_device *dev, int crtc) wanted %d, current %d\n", e->event.sequence, seq);
- e->event.sequence = seq; - e->event.tv_sec = now.tv_sec; - e->event.tv_usec = now.tv_usec; - drm_vblank_put(dev, e->pipe); - list_move_tail(&e->base.link, &e->base.file_priv->event_list); - wake_up_interruptible(&e->base.file_priv->event_wait); - trace_drm_vblank_event_delivered(e->base.pid, e->pipe, - e->event.sequence); + drm_emit_vblank_event(dev, e, seq, &now); }
WARN_ON(atomic_read(&dev->vblank_refcount[crtc]) != 0); @@ -1104,18 +1111,11 @@ static int drm_queue_vblank_event(struct drm_device *dev, int pipe, vblwait->request.sequence);
e->event.sequence = vblwait->request.sequence; + list_add_tail(&e->base.link, &dev->vblank_event_list); if ((seq - vblwait->request.sequence) <= (1 << 23)) { - e->event.sequence = seq; - e->event.tv_sec = now.tv_sec; - e->event.tv_usec = now.tv_usec; - drm_vblank_put(dev, pipe); - list_add_tail(&e->base.link, &e->base.file_priv->event_list); - wake_up_interruptible(&e->base.file_priv->event_wait); + drm_emit_vblank_event(dev, e, seq, &now); vblwait->reply.sequence = seq; - trace_drm_vblank_event_delivered(current->pid, pipe, - vblwait->request.sequence); } else { - list_add_tail(&e->base.link, &dev->vblank_event_list); vblwait->reply.sequence = vblwait->request.sequence; }
@@ -1249,14 +1249,7 @@ void drm_handle_vblank_events(struct drm_device *dev, int crtc) DRM_DEBUG("vblank event on %d, current %d\n", e->event.sequence, seq);
- e->event.sequence = seq; - e->event.tv_sec = now.tv_sec; - e->event.tv_usec = now.tv_usec; - drm_vblank_put(dev, e->pipe); - list_move_tail(&e->base.link, &e->base.file_priv->event_list); - wake_up_interruptible(&e->base.file_priv->event_wait); - trace_drm_vblank_event_delivered(e->base.pid, e->pipe, - e->event.sequence); + drm_emit_vblank_event(dev, e, seq, &now); }
spin_unlock_irqrestore(&dev->event_lock, flags);
On Mit, 2011-04-27 at 16:10 +1000, christopher.halse.rogers@canonical.com wrote:
From: Christopher James Halse Rogers christopher.halse.rogers@canonical.com
This is the least-bad behaviour. It means that we signal the vblank event before it actually happens, but since we're disabling vblanks there's no guarantee that it will *ever* happen otherwise.
This may indeed be the best we can do for events that are pending when the CRTC is disabled[0], but I can't see anything that would prevent new events from getting scheduled (or synchronous vblank waits from timing out) while the CRTC is disabled?
[0] Though it might unnecessarily send events prematurely when the CRTC is just disabled temporarily, e.g. as part of a modeset.
Also, this patch won't seem to help at all for other drivers which don't call drm_vblank_off() directly when disabling a CRTC.
Maybe it would be possible to move those calls to core code, and/or only force sending out events when the CRTC isn't just getting disabled temporarily.
On Wed, 2011-04-27 at 10:32 +0200, Michel Dänzer wrote:
On Mit, 2011-04-27 at 16:10 +1000, christopher.halse.rogers@canonical.com wrote:
From: Christopher James Halse Rogers christopher.halse.rogers@canonical.com
This is the least-bad behaviour. It means that we signal the vblank event before it actually happens, but since we're disabling vblanks there's no guarantee that it will *ever* happen otherwise.
This may indeed be the best we can do for events that are pending when the CRTC is disabled[0], but I can't see anything that would prevent new events from getting scheduled (or synchronous vblank waits from timing out) while the CRTC is disabled?
[0] Though it might unnecessarily send events prematurely when the CRTC is just disabled temporarily, e.g. as part of a modeset.
Also, this patch won't seem to help at all for other drivers which don't call drm_vblank_off() directly when disabling a CRTC.
This is true. On the other hand, the other drivers don't wedge the vblank code into a state where vblanks cannot be re-enabled. So it's only a problem when disabling one of 2+ monitors on those drivers, whereas it's easily triggerable on single monitor systems on intel.
Maybe it would be possible to move those calls to core code, and/or only force sending out events when the CRTC isn't just getting disabled temporarily.
As in: have the core modesetting code call drm_vblank_off before making the driver-specific calls when disabling a crtc? I'll look into it - that would appear to be a more general solution.
It would be nice if the OML_sync_control had been written with a less than laser-like focus on the single monitor case, too. :/
On Mit, 2011-04-27 at 18:58 +1000, Christopher James Halse Rogers wrote:
On Wed, 2011-04-27 at 10:32 +0200, Michel Dänzer wrote:
On Mit, 2011-04-27 at 16:10 +1000, christopher.halse.rogers@canonical.com wrote:
From: Christopher James Halse Rogers christopher.halse.rogers@canonical.com
This is the least-bad behaviour. It means that we signal the vblank event before it actually happens, but since we're disabling vblanks there's no guarantee that it will *ever* happen otherwise.
This may indeed be the best we can do for events that are pending when the CRTC is disabled[0], but I can't see anything that would prevent new events from getting scheduled (or synchronous vblank waits from timing out) while the CRTC is disabled?
[0] Though it might unnecessarily send events prematurely when the CRTC is just disabled temporarily, e.g. as part of a modeset.
Also, this patch won't seem to help at all for other drivers which don't call drm_vblank_off() directly when disabling a CRTC.
This is true. On the other hand, the other drivers don't wedge the vblank code into a state where vblanks cannot be re-enabled. So it's only a problem when disabling one of 2+ monitors on those drivers,
And with DPMS?
whereas it's easily triggerable on single monitor systems on intel.
Anyway, this patch is probably good at least as a preliminary fix for the inconsistent vblank state with the intel driver.
Maybe it would be possible to move those calls to core code, and/or only force sending out events when the CRTC isn't just getting disabled temporarily.
As in: have the core modesetting code call drm_vblank_off before making the driver-specific calls when disabling a crtc? I'll look into it - that would appear to be a more general solution.
Yeah, something like that, thanks.
On Wed, 2011-04-27 at 11:08 +0200, Michel Dänzer wrote:
On Mit, 2011-04-27 at 18:58 +1000, Christopher James Halse Rogers wrote:
On Wed, 2011-04-27 at 10:32 +0200, Michel Dänzer wrote:
On Mit, 2011-04-27 at 16:10 +1000, christopher.halse.rogers@canonical.com wrote:
From: Christopher James Halse Rogers christopher.halse.rogers@canonical.com
This is the least-bad behaviour. It means that we signal the vblank event before it actually happens, but since we're disabling vblanks there's no guarantee that it will *ever* happen otherwise.
This may indeed be the best we can do for events that are pending when the CRTC is disabled[0], but I can't see anything that would prevent new events from getting scheduled (or synchronous vblank waits from timing out) while the CRTC is disabled?
[0] Though it might unnecessarily send events prematurely when the CRTC is just disabled temporarily, e.g. as part of a modeset.
Also, this patch won't seem to help at all for other drivers which don't call drm_vblank_off() directly when disabling a CRTC.
This is true. On the other hand, the other drivers don't wedge the vblank code into a state where vblanks cannot be re-enabled. So it's only a problem when disabling one of 2+ monitors on those drivers,
And with DPMS?
Possibly. Since vblanks aren't wedged off in this case it's more likely that the user turning the monitors back on will result in a vblank irq, which will kick everything back into correct operation.
I've not managed to trigger this on my radeon system in the same way as my intel systems, but I haven't stressed it as hard either.
On Wed, 2011-04-27 at 11:08 +0200, Michel Dänzer wrote:
On Mit, 2011-04-27 at 18:58 +1000, Christopher James Halse Rogers wrote:
On Wed, 2011-04-27 at 10:32 +0200, Michel Dänzer wrote:
On Mit, 2011-04-27 at 16:10 +1000, christopher.halse.rogers@canonical.com wrote:
From: Christopher James Halse Rogers christopher.halse.rogers@canonical.com
This is the least-bad behaviour. It means that we signal the vblank event before it actually happens, but since we're disabling vblanks there's no guarantee that it will *ever* happen otherwise.
This may indeed be the best we can do for events that are pending when the CRTC is disabled[0], but I can't see anything that would prevent new events from getting scheduled (or synchronous vblank waits from timing out) while the CRTC is disabled?
[0] Though it might unnecessarily send events prematurely when the CRTC is just disabled temporarily, e.g. as part of a modeset.
Also, this patch won't seem to help at all for other drivers which don't call drm_vblank_off() directly when disabling a CRTC.
This is true. On the other hand, the other drivers don't wedge the vblank code into a state where vblanks cannot be re-enabled. So it's only a problem when disabling one of 2+ monitors on those drivers,
And with DPMS?
whereas it's easily triggerable on single monitor systems on intel.
Anyway, this patch is probably good at least as a preliminary fix for the inconsistent vblank state with the intel driver.
Maybe it would be possible to move those calls to core code, and/or only force sending out events when the CRTC isn't just getting disabled temporarily.
As in: have the core modesetting code call drm_vblank_off before making the driver-specific calls when disabling a crtc? I'll look into it - that would appear to be a more general solution.
Yeah, something like that, thanks.
Ok. This appears to be surprisingly difficult. Particularly, the vblank code indexes crtcs based on the driver-private numbering, and there doesn't appear to be a generic interface to grab this number; Intel uses the DRM_IOCTL_I915_GET_PIPE_FROM_CRTC_ID ioctl, radeon uses something different.
I'll see what I can come up with, but I don't think I'm sufficiently familiar with the kms code to quickly come up with a nice API.
On Thu, 28 Apr 2011 18:09:58 +1000 Christopher James Halse Rogers christopher.halse.rogers@canonical.com wrote:
Ok. This appears to be surprisingly difficult. Particularly, the vblank code indexes crtcs based on the driver-private numbering, and there doesn't appear to be a generic interface to grab this number; Intel uses the DRM_IOCTL_I915_GET_PIPE_FROM_CRTC_ID ioctl, radeon uses something different.
I'll see what I can come up with, but I don't think I'm sufficiently familiar with the kms code to quickly come up with a nice API.
Yeah, unfortunately the vblank code pre-dates the KMS code by quite a bit, so the IDs don't match.
But with the events emitted as in your previous patch, this should be a harder case to hit, but it sounds like we need to make sure the flip waits are dealt with too to avoid this from triggering at all on Intel. Or did you see other cases where the refcount was nonzero at this point?
Thanks,
On Thu, 28 Apr 2011 13:46:30 -0700 Jesse Barnes jbarnes@virtuousgeek.org wrote:
On Thu, 28 Apr 2011 18:09:58 +1000 Christopher James Halse Rogers christopher.halse.rogers@canonical.com wrote:
Ok. This appears to be surprisingly difficult. Particularly, the vblank code indexes crtcs based on the driver-private numbering, and there doesn't appear to be a generic interface to grab this number; Intel uses the DRM_IOCTL_I915_GET_PIPE_FROM_CRTC_ID ioctl, radeon uses something different.
I'll see what I can come up with, but I don't think I'm sufficiently familiar with the kms code to quickly come up with a nice API.
Yeah, unfortunately the vblank code pre-dates the KMS code by quite a bit, so the IDs don't match.
But with the events emitted as in your previous patch, this should be a harder case to hit, but it sounds like we need to make sure the flip waits are dealt with too to avoid this from triggering at all on Intel. Or did you see other cases where the refcount was nonzero at this point?
Actually it looks like we wait for any outstanding flips before disabling so that seems correct (though I didn't check the locking, it's possible we could race a completion and a new flip if the flip ioctl and the dpms code don't exclude each other).
On Wed, 27 Apr 2011 10:32:36 +0200 Michel Dänzer michel@daenzer.net wrote:
On Mit, 2011-04-27 at 16:10 +1000, christopher.halse.rogers@canonical.com wrote:
From: Christopher James Halse Rogers christopher.halse.rogers@canonical.com
This is the least-bad behaviour. It means that we signal the vblank event before it actually happens, but since we're disabling vblanks there's no guarantee that it will *ever* happen otherwise.
This may indeed be the best we can do for events that are pending when the CRTC is disabled[0], but I can't see anything that would prevent new events from getting scheduled (or synchronous vblank waits from timing out) while the CRTC is disabled?
[0] Though it might unnecessarily send events prematurely when the CRTC is just disabled temporarily, e.g. as part of a modeset.
We should return -EINVAL in that case from drm_wait_vblank due to drm_vblank_get failing (i.e. the driver enable_vblank hook should fail if the corresponding crtc is off). At least that's how it's supposed to work.
On Wed, 27 Apr 2011 16:10:57 +1000 christopher.halse.rogers@canonical.com wrote:
From: Christopher James Halse Rogers christopher.halse.rogers@canonical.com
This is the least-bad behaviour. It means that we signal the vblank event before it actually happens, but since we're disabling vblanks there's no guarantee that it will *ever* happen otherwise.
This prevents GL applications which use WaitMSC from hanging indefinitely.
Signed-off-by: Christopher James Halse Rogers christopher.halse.rogers@canonical.com
drivers/gpu/drm/drm_irq.c | 23 +++++++++++++++++++++++ 1 files changed, 23 insertions(+), 0 deletions(-)
diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c index 741457b..a1f12cb 100644 --- a/drivers/gpu/drm/drm_irq.c +++ b/drivers/gpu/drm/drm_irq.c @@ -932,11 +932,34 @@ EXPORT_SYMBOL(drm_vblank_put);
void drm_vblank_off(struct drm_device *dev, int crtc) {
struct drm_pending_vblank_event *e, *t;
struct timeval now; unsigned long irqflags;
unsigned int seq;
spin_lock_irqsave(&dev->vbl_lock, irqflags); vblank_disable_and_save(dev, crtc); DRM_WAKEUP(&dev->vbl_queue[crtc]);
/* Send any queued vblank events, lest the natives grow disquiet */
seq = drm_vblank_count_and_time(dev, crtc, &now);
list_for_each_entry_safe(e, t, &dev->vblank_event_list, base.link) {
if (e->pipe != crtc)continue;DRM_DEBUG("Sending premature vblank event on disable: \wanted %d, current %d\n",e->event.sequence, seq);e->event.sequence = seq;e->event.tv_sec = now.tv_sec;e->event.tv_usec = now.tv_usec;drm_vblank_put(dev, e->pipe);list_move_tail(&e->base.link, &e->base.file_priv->event_list);wake_up_interruptible(&e->base.file_priv->event_wait);trace_drm_vblank_event_delivered(e->base.pid, e->pipe,e->event.sequence);}
spin_unlock_irqrestore(&dev->vbl_lock, irqflags);
} EXPORT_SYMBOL(drm_vblank_off);
Yeah, this matches what we do for the blocking waits, and it's probably a good idea. Userspace can decide what to do with apps running against a disabled CRTC.
Would be nice to share the code with drm_handle_vblank_events though somehow. Looks like
e->event.sequence = seq; e->event.tv_sec = now.tv_sec; e->event.tv_usec = now.tv_usec; drm_vblank_put(dev, e->pipe); list_move_tail(&e->base.link, &e->base.file_priv->event_list); wake_up_interruptible(&e->base.file_priv->event_wait); trace_drm_vblank_event_delivered(e->base.pid, e->pipe, e->event.sequence);
could be pulled out into a separate function for both to use.
dri-devel@lists.freedesktop.org
-
Christopher James Halse Rogers -
christopher.halse.rogers＠canonical.com
-
Jesse Barnes -
Marcin Slusarz -
Michel Dänzer