[PATCH 1/3] drm: Send pending vblank events before disabling vblank.

[PATCH 1/3] drm: Send pending vblank events before disabling vblank.

[christopher.halse.rogers]

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);
-- 
1.7.4.1

[PATCH 2/3] drm: Warn if vblank state has become inconsistent.

[christopher.halse.rogers]

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);
-- 
1.7.4.1

[PATCH 3/3] drm: Factor-out drm_emit_vblank_event code.

[christopher.halse.rogers]

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);
-- 
1.7.4.1

Re: [PATCH 1/3] drm: Send pending vblank events before disabling vblank.

[Michel Dänzer]

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.


-- 
Earthling Michel Dänzer           |                http://www.vmware.com
Libre software enthusiast         |          Debian, X and DRI developer
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/3] drm: Factor-out drm_emit_vblank_event code.

[Michel Dänzer]

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;
>  		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);
>  	}

Makes sense, but shouldn't the drm_vblank_put() calls move into
drm_emit_vblank_event() as well?


-- 
Earthling Michel Dänzer           |                http://www.vmware.com
Libre software enthusiast         |          Debian, X and DRI developer
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/3] drm: Warn if vblank state has become inconsistent.

[Michel Dänzer]

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>


-- 
Earthling Michel Dänzer           |                http://www.vmware.com
Libre software enthusiast         |          Debian, X and DRI developer
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/3] drm: Factor-out drm_emit_vblank_event code.

[Christopher James Halse Rogers]

[-- Attachment #1.1: Type: text/plain, Size: 2324 bytes --]

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;
> >  		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);
> >  	}
> 
> 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.



[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/3] drm: Send pending vblank events before disabling vblank.

[Christopher James Halse Rogers]

[-- Attachment #1.1: Type: text/plain, Size: 1766 bytes --]

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. :/

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/3] drm: Factor-out drm_emit_vblank_event code.

[Michel Dänzer]

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;
> > >  		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);
> > >  	}
> > 
> > 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.


-- 
Earthling Michel Dänzer           |                http://www.vmware.com
Libre software enthusiast         |          Debian, X and DRI developer
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/3] drm: Send pending vblank events before disabling vblank.

[Michel Dänzer]

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.


-- 
Earthling Michel Dänzer           |                http://www.vmware.com
Libre software enthusiast         |          Debian, X and DRI developer
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/3] drm: Send pending vblank events before disabling vblank.

[Christopher James Halse Rogers]

[-- Attachment #1.1: Type: text/plain, Size: 1796 bytes --]

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.


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/3] drm: Send pending vblank events before disabling vblank.

[Christopher James Halse Rogers]

[-- Attachment #1.1: Type: text/plain, Size: 2548 bytes --]

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.


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/3] drm: Send pending vblank events before disabling vblank.

[Jesse Barnes]

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.

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 3/3] drm: Factor-out drm_emit_vblank_event code.

[Jesse Barnes]

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.


-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 1/3] drm: Send pending vblank events before disabling vblank.

[Jesse Barnes]

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.

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 1/3] drm: Send pending vblank events before disabling vblank.

[Jesse Barnes]

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,
-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 1/3] drm: Send pending vblank events before disabling vblank.

[Jesse Barnes]

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).

-- 
Jesse Barnes, Intel Open Source Technology Center

[PATCH] drm: Factor-out drm_emit_vblank_event code. (v2)

[christopher.halse.rogers]

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);
-- 
1.7.4.1

Re: [PATCH] drm: Factor-out drm_emit_vblank_event code. (v2)

[Michel Dänzer]

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>


-- 
Earthling Michel Dänzer           |                http://www.vmware.com
Libre software enthusiast         |          Debian, X and DRI developer
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: Factor-out drm_emit_vblank_event code. (v2)

[Marcin Slusarz]

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);
>  	} 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);
> -- 

Re: [PATCH] drm: Factor-out drm_emit_vblank_event code. (v2)

[Christopher James Halse Rogers]

[-- Attachment #1.1: Type: text/plain, Size: 4469 bytes --]

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);
> >  	} 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);
> > -- 



[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] drm: Factor-out drm_emit_vblank_event code. (v3)

[christopher.halse.rogers]

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);
-- 
1.7.4.1