Re: [Intel-gfx] [PATCHv5] drm/i915: use waitqueue in wait for vblank
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
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCHv5] drm/i915: use waitqueue in wait for vblank
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
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))
> +
Re: [Intel-gfx] [PATCHv5] drm/i915: use waitqueue in wait for vblank
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
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCHv5] drm/i915: use waitqueue in wait for vblank
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
---
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 = dr
