[PATCHv5] optimize wait for vblank

[PATCHv5] optimize wait for vblank

[Arun R Murthy]

Implemented the comments based on
www.spinics.net/lists/intel-gfx/msg42439.html

Arun R Murthy (1):
  drm/i915: use waitqueue in wait for vblank

 drivers/gpu/drm/i915/i915_dma.c      |    2 ++
 drivers/gpu/drm/i915/i915_drv.h      |    1 +
 drivers/gpu/drm/i915/i915_irq.c      |   16 ++++++++++++----
 drivers/gpu/drm/i915/intel_display.c |   10 ++++++----
 4 files changed, 21 insertions(+), 8 deletions(-)

-- 
1.7.9.5

[PATCHv5] drm/i915: use waitqueue in wait for vblank

[Arun R Murthy]

Instead of using sleep functions in wait for vblank, which wakes up on
sleep timeout aften, thereby scheduler provoking scheduler. Hence use
waitqueue in wait for vblank.

Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
---
 drivers/gpu/drm/i915/i915_dma.c      |    2 ++
 drivers/gpu/drm/i915/i915_drv.h      |    1 +
 drivers/gpu/drm/i915/i915_irq.c      |   17 +++++++++++++----
 drivers/gpu/drm/i915/intel_display.c |   10 ++++++----
 4 files changed, 22 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 258b1be..896620c 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1506,6 +1506,8 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
 	mutex_init(&dev_priv->dpio_lock);
 	mutex_init(&dev_priv->modeset_restore_lock);
 
+	init_waitqueue_head(&dev_priv->wait_vblank);
+
 	intel_pm_setup(dev);
 
 	intel_display_crc_init(dev);
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 728b9c3..449bfef 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1590,6 +1590,7 @@ typedef struct drm_i915_private {
 	struct i915_dri1_state dri1;
 	/* Old ums support infrastructure, same warning applies. */
 	struct i915_ums_state ums;
+	wait_queue_head_t wait_vblank;
 } drm_i915_private_t;
 
 static inline struct drm_i915_private *to_i915(const struct drm_device *dev)
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 56edff3..f97f0fe 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1513,8 +1513,11 @@ static irqreturn_t valleyview_irq_handler(int irq, void *arg)
 		spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
 
 		for_each_pipe(pipe) {
-			if (pipe_stats[pipe] & PIPE_START_VBLANK_INTERRUPT_STATUS)
+			if (pipe_stats[pipe] &
+					PIPE_START_VBLANK_INTERRUPT_STATUS) {
 				drm_handle_vblank(dev, pipe);
+				wake_up_interruptible(&dev_priv->wait_vblank);
+			}
 
 			if (pipe_stats[pipe] & PLANE_FLIPDONE_INT_STATUS_VLV) {
 				intel_prepare_page_flip(dev, pipe);
@@ -3269,8 +3272,10 @@ static irqreturn_t i8xx_irq_handler(int irq, void *arg)
 				plane = !plane;
 
 			if (pipe_stats[pipe] & PIPE_VBLANK_INTERRUPT_STATUS &&
-			    i8xx_handle_vblank(dev, plane, pipe, iir))
+			    i8xx_handle_vblank(dev, plane, pipe, iir)) {
 				flip_mask &= ~DISPLAY_PLANE_FLIP_PENDING(plane);
+				wake_up_interruptible(&dev_priv->wait_vblank);
+			}
 
 			if (pipe_stats[pipe] & PIPE_CRC_DONE_INTERRUPT_STATUS)
 				i9xx_pipe_crc_irq_handler(dev, pipe);
@@ -3464,8 +3469,10 @@ static irqreturn_t i915_irq_handler(int irq, void *arg)
 				plane = !plane;
 
 			if (pipe_stats[pipe] & PIPE_VBLANK_INTERRUPT_STATUS &&
-			    i915_handle_vblank(dev, plane, pipe, iir))
+			    i915_handle_vblank(dev, plane, pipe, iir)) {
 				flip_mask &= ~DISPLAY_PLANE_FLIP_PENDING(plane);
+				wake_up_interruptible(&dev_priv->wait_vblank);
+			}
 
 			if (pipe_stats[pipe] & PIPE_LEGACY_BLC_EVENT_STATUS)
 				blc_event = true;
@@ -3709,8 +3716,10 @@ static irqreturn_t i965_irq_handler(int irq, void *arg)
 
 		for_each_pipe(pipe) {
 			if (pipe_stats[pipe] & PIPE_START_VBLANK_INTERRUPT_STATUS &&
-			    i915_handle_vblank(dev, pipe, pipe, iir))
+			    i915_handle_vblank(dev, pipe, pipe, iir)) {
 				flip_mask &= ~DISPLAY_PLANE_FLIP_PENDING(pipe);
+				wake_up_interruptible(&dev_priv->wait_vblank);
+			}
 
 			if (pipe_stats[pipe] & PIPE_LEGACY_BLC_EVENT_STATUS)
 				blc_event = true;
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 4d4a0d9..e1eb564 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -757,12 +757,14 @@ enum transcoder intel_pipe_to_cpu_transcoder(struct drm_i915_private *dev_priv,
 static void g4x_wait_for_vblank(struct drm_device *dev, int pipe)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	u32 frame, frame_reg = PIPE_FRMCOUNT_GM45(pipe);
+	u32 vblank_cnt;
 
-	frame = I915_READ(frame_reg);
+	vblank_cnt = drm_vblank_count(dev, pipe);
 
-	if (wait_for(I915_READ_NOTRACE(frame_reg) != frame, 50))
-		DRM_DEBUG_KMS("vblank wait timed out\n");
+	/* TODO: get the vblank time dynamically or from platform data */
+	wait_event_interruptible_timeout(dev_priv->wait_vblank,
+			(vblank_cnt != drm_vblank_count(dev, pipe)),
+			msecs_to_jiffies(16));
 }
 
 /**
-- 
1.7.9.5

Re: [PATCHv5] drm/i915: use waitqueue in wait for vblank

[Chris Wilson]

On Wed, May 21, 2014 at 01:09:58PM +0530, Arun R Murthy wrote:
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 56edff3..f97f0fe 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -1513,8 +1513,11 @@ static irqreturn_t valleyview_irq_handler(int irq, void *arg)
>  		spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
>  
>  		for_each_pipe(pipe) {
> -			if (pipe_stats[pipe] & PIPE_START_VBLANK_INTERRUPT_STATUS)
> +			if (pipe_stats[pipe] &
> +					PIPE_START_VBLANK_INTERRUPT_STATUS) {
>  				drm_handle_vblank(dev, pipe);
> +				wake_up_interruptible(&dev_priv->wait_vblank);

We now have intel_handle_vblank() so these chunks can be simplified.

> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 4d4a0d9..e1eb564 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -757,12 +757,14 @@ enum transcoder intel_pipe_to_cpu_transcoder(struct drm_i915_private *dev_priv,
>  static void g4x_wait_for_vblank(struct drm_device *dev, int pipe)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> -	u32 frame, frame_reg = PIPE_FRMCOUNT_GM45(pipe);
> +	u32 vblank_cnt;
>  
> -	frame = I915_READ(frame_reg);
> +	vblank_cnt = drm_vblank_count(dev, pipe);
>  
> -	if (wait_for(I915_READ_NOTRACE(frame_reg) != frame, 50))
> -		DRM_DEBUG_KMS("vblank wait timed out\n");
> +	/* TODO: get the vblank time dynamically or from platform data */
> +	wait_event_interruptible_timeout(dev_priv->wait_vblank,
> +			(vblank_cnt != drm_vblank_count(dev, pipe)),
> +			msecs_to_jiffies(16));

Keep the ultimate timeout at 50 until you have evidence you can reduce
it. And then it should be 2x vrefresh interval to be safe. However, you
are likely still hitting the timeout as the vblank irq is not guaranteed
to be enabled here. How safe calling drm_vblank_get() is during modeset
I defer to Ville since he has just taken a pass over the whole mess.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCHv5] drm/i915: use waitqueue in wait for vblank

[Daniel Vetter]

On Wed, May 21, 2014 at 01:09:58PM +0530, Arun R Murthy wrote:
> Instead of using sleep functions in wait for vblank, which wakes up on
> sleep timeout aften, thereby scheduler provoking scheduler. Hence use
> waitqueue in wait for vblank.
> 
> Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>

You're rebuilding infrastructure that drm_irq.c already has. In latest
nightly all you really need should be

	drm_crtc_vblank_get();
	wait_event_timeout(dev->vblank[drm_crtc_index()].queue, ...);
	drm_crtc_vblank_put();

If you fancy it you could wrap this little sequence up into a new helper
in drm_irq.c called drm_crtc_wait_one_vblank(). Please add kerneldoc for
that, too.

Also latest upstream WARNs if the vblank wait times out, we want that
here, too. And there's no need to optimize the timeout since it's an
exceptional condition which should never happen. If it does it's a driver
bug and needs to be addressed.

Note that you might need to shuffle around the drm_crtc_vblank_on/off
calls a bit to make sure the vblank support is enabled again.
-Daniel
> ---
>  drivers/gpu/drm/i915/i915_dma.c      |    2 ++
>  drivers/gpu/drm/i915/i915_drv.h      |    1 +
>  drivers/gpu/drm/i915/i915_irq.c      |   17 +++++++++++++----
>  drivers/gpu/drm/i915/intel_display.c |   10 ++++++----
>  4 files changed, 22 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index 258b1be..896620c 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -1506,6 +1506,8 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
>  	mutex_init(&dev_priv->dpio_lock);
>  	mutex_init(&dev_priv->modeset_restore_lock);
>  
> +	init_waitqueue_head(&dev_priv->wait_vblank);
> +
>  	intel_pm_setup(dev);
>  
>  	intel_display_crc_init(dev);
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 728b9c3..449bfef 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1590,6 +1590,7 @@ typedef struct drm_i915_private {
>  	struct i915_dri1_state dri1;
>  	/* Old ums support infrastructure, same warning applies. */
>  	struct i915_ums_state ums;
> +	wait_queue_head_t wait_vblank;
>  } drm_i915_private_t;
>  
>  static inline struct drm_i915_private *to_i915(const struct drm_device *dev)
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 56edff3..f97f0fe 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -1513,8 +1513,11 @@ static irqreturn_t valleyview_irq_handler(int irq, void *arg)
>  		spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
>  
>  		for_each_pipe(pipe) {
> -			if (pipe_stats[pipe] & PIPE_START_VBLANK_INTERRUPT_STATUS)
> +			if (pipe_stats[pipe] &
> +					PIPE_START_VBLANK_INTERRUPT_STATUS) {
>  				drm_handle_vblank(dev, pipe);
> +				wake_up_interruptible(&dev_priv->wait_vblank);
> +			}
>  
>  			if (pipe_stats[pipe] & PLANE_FLIPDONE_INT_STATUS_VLV) {
>  				intel_prepare_page_flip(dev, pipe);
> @@ -3269,8 +3272,10 @@ static irqreturn_t i8xx_irq_handler(int irq, void *arg)
>  				plane = !plane;
>  
>  			if (pipe_stats[pipe] & PIPE_VBLANK_INTERRUPT_STATUS &&
> -			    i8xx_handle_vblank(dev, plane, pipe, iir))
> +			    i8xx_handle_vblank(dev, plane, pipe, iir)) {
>  				flip_mask &= ~DISPLAY_PLANE_FLIP_PENDING(plane);
> +				wake_up_interruptible(&dev_priv->wait_vblank);
> +			}
>  
>  			if (pipe_stats[pipe] & PIPE_CRC_DONE_INTERRUPT_STATUS)
>  				i9xx_pipe_crc_irq_handler(dev, pipe);
> @@ -3464,8 +3469,10 @@ static irqreturn_t i915_irq_handler(int irq, void *arg)
>  				plane = !plane;
>  
>  			if (pipe_stats[pipe] & PIPE_VBLANK_INTERRUPT_STATUS &&
> -			    i915_handle_vblank(dev, plane, pipe, iir))
> +			    i915_handle_vblank(dev, plane, pipe, iir)) {
>  				flip_mask &= ~DISPLAY_PLANE_FLIP_PENDING(plane);
> +				wake_up_interruptible(&dev_priv->wait_vblank);
> +			}
>  
>  			if (pipe_stats[pipe] & PIPE_LEGACY_BLC_EVENT_STATUS)
>  				blc_event = true;
> @@ -3709,8 +3716,10 @@ static irqreturn_t i965_irq_handler(int irq, void *arg)
>  
>  		for_each_pipe(pipe) {
>  			if (pipe_stats[pipe] & PIPE_START_VBLANK_INTERRUPT_STATUS &&
> -			    i915_handle_vblank(dev, pipe, pipe, iir))
> +			    i915_handle_vblank(dev, pipe, pipe, iir)) {
>  				flip_mask &= ~DISPLAY_PLANE_FLIP_PENDING(pipe);
> +				wake_up_interruptible(&dev_priv->wait_vblank);
> +			}
>  
>  			if (pipe_stats[pipe] & PIPE_LEGACY_BLC_EVENT_STATUS)
>  				blc_event = true;
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 4d4a0d9..e1eb564 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -757,12 +757,14 @@ enum transcoder intel_pipe_to_cpu_transcoder(struct drm_i915_private *dev_priv,
>  static void g4x_wait_for_vblank(struct drm_device *dev, int pipe)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> -	u32 frame, frame_reg = PIPE_FRMCOUNT_GM45(pipe);
> +	u32 vblank_cnt;
>  
> -	frame = I915_READ(frame_reg);
> +	vblank_cnt = drm_vblank_count(dev, pipe);
>  
> -	if (wait_for(I915_READ_NOTRACE(frame_reg) != frame, 50))
> -		DRM_DEBUG_KMS("vblank wait timed out\n");
> +	/* TODO: get the vblank time dynamically or from platform data */
> +	wait_event_interruptible_timeout(dev_priv->wait_vblank,
> +			(vblank_cnt != drm_vblank_count(dev, pipe)),
> +			msecs_to_jiffies(16));
>  }
>  
>  /**
> -- 
> 1.7.9.5
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCHv5] drm/i915: use waitqueue in wait for vblank

[Ville Syrjälä]

On Wed, May 21, 2014 at 08:54:04AM +0100, Chris Wilson wrote:
> On Wed, May 21, 2014 at 01:09:58PM +0530, Arun R Murthy wrote:
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > index 56edff3..f97f0fe 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -1513,8 +1513,11 @@ static irqreturn_t valleyview_irq_handler(int irq, void *arg)
> >  		spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
> >  
> >  		for_each_pipe(pipe) {
> > -			if (pipe_stats[pipe] & PIPE_START_VBLANK_INTERRUPT_STATUS)
> > +			if (pipe_stats[pipe] &
> > +					PIPE_START_VBLANK_INTERRUPT_STATUS) {
> >  				drm_handle_vblank(dev, pipe);
> > +				wake_up_interruptible(&dev_priv->wait_vblank);
> 
> We now have intel_handle_vblank() so these chunks can be simplified.
> 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 4d4a0d9..e1eb564 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -757,12 +757,14 @@ enum transcoder intel_pipe_to_cpu_transcoder(struct drm_i915_private *dev_priv,
> >  static void g4x_wait_for_vblank(struct drm_device *dev, int pipe)
> >  {
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > -	u32 frame, frame_reg = PIPE_FRMCOUNT_GM45(pipe);
> > +	u32 vblank_cnt;
> >  
> > -	frame = I915_READ(frame_reg);
> > +	vblank_cnt = drm_vblank_count(dev, pipe);
> >  
> > -	if (wait_for(I915_READ_NOTRACE(frame_reg) != frame, 50))
> > -		DRM_DEBUG_KMS("vblank wait timed out\n");
> > +	/* TODO: get the vblank time dynamically or from platform data */
> > +	wait_event_interruptible_timeout(dev_priv->wait_vblank,
> > +			(vblank_cnt != drm_vblank_count(dev, pipe)),
> > +			msecs_to_jiffies(16));
> 
> Keep the ultimate timeout at 50 until you have evidence you can reduce
> it. And then it should be 2x vrefresh interval to be safe. However, you
> are likely still hitting the timeout as the vblank irq is not guaranteed
> to be enabled here. How safe calling drm_vblank_get() is during modeset
> I defer to Ville since he has just taken a pass over the whole mess.

The plan is to make drm_vblank_get() work until drm_vblank_off() has
been called. And when enabling, drm_vblank_get() will succeed only after
drm_vblank_on() has been called. The place where those should end up is
at the start/end of intel_crtc_{disable,enable}_planes(). So you have
access to vblank irqs while planes are getting enabled/disabled, but no
further since we can't guarantee their function once we start shutting
off pipes/ports/etc.

-- 
Ville Syrjälä
Intel OTC