| Message ID | 1400657998-10086-2-git-send-email-arun.r.murthy@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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 >
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.
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)); } /**
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(-)