[PATCH 0/9] drm: More vblank on/off work

[PATCH 0/9] drm: More vblank on/off work

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Another vblank series with the following features:
- Plug a race between drm_vblank_off() and marking the crtc inactive
- Don't send zeroed vblank evens to userspace at drm_vblank_off()
- Have the user visible vblank counter account the entire time
  when the crtc was active, regardless of how long vblank interrupts
  were enabled
- Avoid random jumps in the user visible vblank counter if the hardware
  counter gets reset
- Allow disabling vblank interrupts immediately at drm_vblank_put()
- Some polish via coccinelle

While setting drm_vblank_offdelay to 0 is now possible, I'm not sure
if we should set it 0 automatically in the i915 driver. If there are
multiple GPUs in the system that setting will affect them all, which
might have bad consequences if the other GPU doesn't have a hardware
frame counter, or if it's just buggy. So perhaps we should move that
option to be per-driver?

Ville Syrjälä (9):
  drm: Always reject drm_vblank_get() after drm_vblank_off()
  drm/i915: Warn if drm_vblank_get() still works after drm_vblank_off()
  drm: Don't clear vblank timestamps when vblank interrupt is disabled
  drm: Move drm_update_vblank_count()
  drm: Have the vblank counter account for the time between vblank irq
    disable and drm_vblank_off()
  drm: Avoid random vblank counter jumps if the hardware counter has
    been reset
  drm: Disable vblank interrupt immediately when drm_vblank_offdelay==0
  drm: Reduce the amount of dev->vblank[crtc] in the code
  drm/i915: Leave interrupts enabled while disabling crtcs during
    suspend

 Documentation/DocBook/drm.tmpl          |   1 +
 drivers/gpu/drm/drm_irq.c               | 285 +++++++++++++++++++-------------
 drivers/gpu/drm/drm_stub.c              |   4 +-
 drivers/gpu/drm/exynos/exynos_drm_drv.h |   2 +-
 drivers/gpu/drm/i915/i915_drv.c         |   3 +-
 drivers/gpu/drm/i915/intel_display.c    |  15 ++
 include/drm/drmP.h                      |   2 +-
 7 files changed, 192 insertions(+), 120 deletions(-)

-- 
1.8.5.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 1/9] drm: Always reject drm_vblank_get() after drm_vblank_off()

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Make sure drm_vblank_get() never succeeds when called between
drm_vblank_off() and drm_vblank_on(). Borrow a trick from the
old drm_vblank_{pre,post}_modeset() functions and just bump
the refcount in drm_vblank_off() and drop it in drm_vblank_on().

Hopefully the use of inmodeset won't conflict badly with
drm_vblank_{pre,post}_modeset().

For i915 there's a window between drm_vblank_off() and marking the
crtc as inactive where the current code still allows drm_vblank_get().

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_irq.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
index b565372..7b1e08c 100644
--- a/drivers/gpu/drm/drm_irq.c
+++ b/drivers/gpu/drm/drm_irq.c
@@ -1039,6 +1039,15 @@ void drm_vblank_off(struct drm_device *dev, int crtc)
 	}
 	spin_unlock(&dev->event_lock);
 
+	/*
+	 * Prevent subsequent drm_vblank_get() from re-enabling
+	 * the vblank interrupt by bumping the refcount.
+	 */
+	if (!dev->vblank[crtc].inmodeset) {
+		atomic_inc(&dev->vblank[crtc].refcount);
+		dev->vblank[crtc].inmodeset = 1;
+	}
+
 	spin_unlock_irqrestore(&dev->vbl_lock, irqflags);
 }
 EXPORT_SYMBOL(drm_vblank_off);
@@ -1079,6 +1088,11 @@ void drm_vblank_on(struct drm_device *dev, int crtc)
 	unsigned long irqflags;
 
 	spin_lock_irqsave(&dev->vbl_lock, irqflags);
+	/* Drop our private "prevent drm_vblank_get" refcount */
+	if (dev->vblank[crtc].inmodeset) {
+		atomic_dec(&dev->vblank[crtc].refcount);
+		dev->vblank[crtc].inmodeset = 0;
+	}
 	/* re-enable interrupts if there's are users left */
 	if (atomic_read(&dev->vblank[crtc].refcount) != 0)
 		WARN_ON(drm_vblank_enable(dev, crtc));
-- 
1.8.5.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/9] drm: Always reject drm_vblank_get() after drm_vblank_off()

[Daniel Vetter]

On Mon, May 26, 2014 at 02:46:24PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Make sure drm_vblank_get() never succeeds when called between
> drm_vblank_off() and drm_vblank_on(). Borrow a trick from the
> old drm_vblank_{pre,post}_modeset() functions and just bump
> the refcount in drm_vblank_off() and drop it in drm_vblank_on().
> 
> Hopefully the use of inmodeset won't conflict badly with
> drm_vblank_{pre,post}_modeset().
> 
> For i915 there's a window between drm_vblank_off() and marking the
> crtc as inactive where the current code still allows drm_vblank_get().
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Afaics we never check anywhere for inmodeset in drm_vblank_get, so how
does this work?
-Daniel

> ---
>  drivers/gpu/drm/drm_irq.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> index b565372..7b1e08c 100644
> --- a/drivers/gpu/drm/drm_irq.c
> +++ b/drivers/gpu/drm/drm_irq.c
> @@ -1039,6 +1039,15 @@ void drm_vblank_off(struct drm_device *dev, int crtc)
>  	}
>  	spin_unlock(&dev->event_lock);
>  
> +	/*
> +	 * Prevent subsequent drm_vblank_get() from re-enabling
> +	 * the vblank interrupt by bumping the refcount.
> +	 */
> +	if (!dev->vblank[crtc].inmodeset) {
> +		atomic_inc(&dev->vblank[crtc].refcount);
> +		dev->vblank[crtc].inmodeset = 1;
> +	}
> +
>  	spin_unlock_irqrestore(&dev->vbl_lock, irqflags);
>  }
>  EXPORT_SYMBOL(drm_vblank_off);
> @@ -1079,6 +1088,11 @@ void drm_vblank_on(struct drm_device *dev, int crtc)
>  	unsigned long irqflags;
>  
>  	spin_lock_irqsave(&dev->vbl_lock, irqflags);
> +	/* Drop our private "prevent drm_vblank_get" refcount */
> +	if (dev->vblank[crtc].inmodeset) {
> +		atomic_dec(&dev->vblank[crtc].refcount);
> +		dev->vblank[crtc].inmodeset = 0;
> +	}
>  	/* re-enable interrupts if there's are users left */
>  	if (atomic_read(&dev->vblank[crtc].refcount) != 0)
>  		WARN_ON(drm_vblank_enable(dev, crtc));
> -- 
> 1.8.5.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 1/9] drm: Always reject drm_vblank_get() after drm_vblank_off()

[Ville Syrjälä]

On Mon, May 26, 2014 at 03:21:41PM +0200, Daniel Vetter wrote:
> On Mon, May 26, 2014 at 02:46:24PM +0300, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Make sure drm_vblank_get() never succeeds when called between
> > drm_vblank_off() and drm_vblank_on(). Borrow a trick from the
> > old drm_vblank_{pre,post}_modeset() functions and just bump
> > the refcount in drm_vblank_off() and drop it in drm_vblank_on().
> > 
> > Hopefully the use of inmodeset won't conflict badly with
> > drm_vblank_{pre,post}_modeset().
> > 
> > For i915 there's a window between drm_vblank_off() and marking the
> > crtc as inactive where the current code still allows drm_vblank_get().
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Afaics we never check anywhere for inmodeset in drm_vblank_get, so how
> does this work?

If refcount is already >0 and vblank irq is disabled, drm_vblank_get() will
return -EINVAL.

> -Daniel
> 
> > ---
> >  drivers/gpu/drm/drm_irq.c | 14 ++++++++++++++
> >  1 file changed, 14 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> > index b565372..7b1e08c 100644
> > --- a/drivers/gpu/drm/drm_irq.c
> > +++ b/drivers/gpu/drm/drm_irq.c
> > @@ -1039,6 +1039,15 @@ void drm_vblank_off(struct drm_device *dev, int crtc)
> >  	}
> >  	spin_unlock(&dev->event_lock);
> >  
> > +	/*
> > +	 * Prevent subsequent drm_vblank_get() from re-enabling
> > +	 * the vblank interrupt by bumping the refcount.
> > +	 */
> > +	if (!dev->vblank[crtc].inmodeset) {
> > +		atomic_inc(&dev->vblank[crtc].refcount);
> > +		dev->vblank[crtc].inmodeset = 1;
> > +	}
> > +
> >  	spin_unlock_irqrestore(&dev->vbl_lock, irqflags);
> >  }
> >  EXPORT_SYMBOL(drm_vblank_off);
> > @@ -1079,6 +1088,11 @@ void drm_vblank_on(struct drm_device *dev, int crtc)
> >  	unsigned long irqflags;
> >  
> >  	spin_lock_irqsave(&dev->vbl_lock, irqflags);
> > +	/* Drop our private "prevent drm_vblank_get" refcount */
> > +	if (dev->vblank[crtc].inmodeset) {
> > +		atomic_dec(&dev->vblank[crtc].refcount);
> > +		dev->vblank[crtc].inmodeset = 0;
> > +	}
> >  	/* re-enable interrupts if there's are users left */
> >  	if (atomic_read(&dev->vblank[crtc].refcount) != 0)
> >  		WARN_ON(drm_vblank_enable(dev, crtc));
> > -- 
> > 1.8.5.5
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 1/9] drm: Always reject drm_vblank_get() after drm_vblank_off()

[Daniel Vetter]

On Mon, May 26, 2014 at 04:32:02PM +0300, Ville Syrjälä wrote:
> On Mon, May 26, 2014 at 03:21:41PM +0200, Daniel Vetter wrote:
> > On Mon, May 26, 2014 at 02:46:24PM +0300, ville.syrjala@linux.intel.com wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > Make sure drm_vblank_get() never succeeds when called between
> > > drm_vblank_off() and drm_vblank_on(). Borrow a trick from the
> > > old drm_vblank_{pre,post}_modeset() functions and just bump
> > > the refcount in drm_vblank_off() and drop it in drm_vblank_on().
> > > 
> > > Hopefully the use of inmodeset won't conflict badly with
> > > drm_vblank_{pre,post}_modeset().
> > > 
> > > For i915 there's a window between drm_vblank_off() and marking the
> > > crtc as inactive where the current code still allows drm_vblank_get().
> > > 
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Afaics we never check anywhere for inmodeset in drm_vblank_get, so how
> > does this work?
> 
> If refcount is already >0 and vblank irq is disabled, drm_vblank_get() will
> return -EINVAL.

Oh right, I've missed that. drm_irq.c is never short of suprises. With
that enlightenment (and maybe a pimped commit message for blind people
like me) this is Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> > -Daniel
> > 
> > > ---
> > >  drivers/gpu/drm/drm_irq.c | 14 ++++++++++++++
> > >  1 file changed, 14 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> > > index b565372..7b1e08c 100644
> > > --- a/drivers/gpu/drm/drm_irq.c
> > > +++ b/drivers/gpu/drm/drm_irq.c
> > > @@ -1039,6 +1039,15 @@ void drm_vblank_off(struct drm_device *dev, int crtc)
> > >  	}
> > >  	spin_unlock(&dev->event_lock);
> > >  
> > > +	/*
> > > +	 * Prevent subsequent drm_vblank_get() from re-enabling
> > > +	 * the vblank interrupt by bumping the refcount.
> > > +	 */
> > > +	if (!dev->vblank[crtc].inmodeset) {
> > > +		atomic_inc(&dev->vblank[crtc].refcount);
> > > +		dev->vblank[crtc].inmodeset = 1;
> > > +	}
> > > +
> > >  	spin_unlock_irqrestore(&dev->vbl_lock, irqflags);
> > >  }
> > >  EXPORT_SYMBOL(drm_vblank_off);
> > > @@ -1079,6 +1088,11 @@ void drm_vblank_on(struct drm_device *dev, int crtc)
> > >  	unsigned long irqflags;
> > >  
> > >  	spin_lock_irqsave(&dev->vbl_lock, irqflags);
> > > +	/* Drop our private "prevent drm_vblank_get" refcount */
> > > +	if (dev->vblank[crtc].inmodeset) {
> > > +		atomic_dec(&dev->vblank[crtc].refcount);
> > > +		dev->vblank[crtc].inmodeset = 0;
> > > +	}
> > >  	/* re-enable interrupts if there's are users left */
> > >  	if (atomic_read(&dev->vblank[crtc].refcount) != 0)
> > >  		WARN_ON(drm_vblank_enable(dev, crtc));
> > > -- 
> > > 1.8.5.5
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> > -- 
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> 
> -- 
> Ville Syrjälä
> Intel OTC

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

[PATCH 2/9] drm/i915: Warn if drm_vblank_get() still works after drm_vblank_off()

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 3da73ef..da318a7 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1305,6 +1305,17 @@ static void assert_sprites_disabled(struct drm_i915_private *dev_priv,
 	}
 }
 
+static void assert_vblank_disabled(struct intel_crtc *crtc)
+{
+	struct drm_device *dev = crtc->base.dev;
+	enum pipe pipe = crtc->pipe;
+
+	if (WARN_ON(drm_vblank_get(dev, pipe) == 0)) {
+		drm_vblank_put(dev, pipe);
+		drm_vblank_off(dev, pipe);
+	}
+}
+
 static void ibx_assert_pch_refclk_enabled(struct drm_i915_private *dev_priv)
 {
 	u32 val;
@@ -3885,6 +3896,8 @@ static void intel_crtc_enable_planes(struct drm_crtc *crtc)
 	int pipe = intel_crtc->pipe;
 	int plane = intel_crtc->plane;
 
+	assert_vblank_disabled(intel_crtc);
+
 	intel_enable_primary_hw_plane(dev_priv, plane, pipe);
 	intel_enable_planes(crtc);
 	/* The fixup needs to happen before cursor is enabled */
@@ -3921,6 +3934,8 @@ static void intel_crtc_disable_planes(struct drm_crtc *crtc)
 	intel_crtc_update_cursor(crtc, false);
 	intel_disable_planes(crtc);
 	intel_disable_primary_hw_plane(dev_priv, plane, pipe);
+
+	assert_vblank_disabled(intel_crtc);
 }
 
 static void ironlake_crtc_enable(struct drm_crtc *crtc)
-- 
1.8.5.5

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

Re: [PATCH 2/9] drm/i915: Warn if drm_vblank_get() still works after drm_vblank_off()

[Daniel Vetter]

On Mon, May 26, 2014 at 02:46:25PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 3da73ef..da318a7 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -1305,6 +1305,17 @@ static void assert_sprites_disabled(struct drm_i915_private *dev_priv,
>  	}
>  }
>  
> +static void assert_vblank_disabled(struct intel_crtc *crtc)
> +{
> +	struct drm_device *dev = crtc->base.dev;
> +	enum pipe pipe = crtc->pipe;
> +
> +	if (WARN_ON(drm_vblank_get(dev, pipe) == 0)) {
> +		drm_vblank_put(dev, pipe);
> +		drm_vblank_off(dev, pipe);

Imo the _off is too much, since with that it's not just an assert but a
"... and please make it so if not". Imo better to drop that.
-Daniel

> +	}
> +}
> +
>  static void ibx_assert_pch_refclk_enabled(struct drm_i915_private *dev_priv)
>  {
>  	u32 val;
> @@ -3885,6 +3896,8 @@ static void intel_crtc_enable_planes(struct drm_crtc *crtc)
>  	int pipe = intel_crtc->pipe;
>  	int plane = intel_crtc->plane;
>  
> +	assert_vblank_disabled(intel_crtc);
> +
>  	intel_enable_primary_hw_plane(dev_priv, plane, pipe);
>  	intel_enable_planes(crtc);
>  	/* The fixup needs to happen before cursor is enabled */
> @@ -3921,6 +3934,8 @@ static void intel_crtc_disable_planes(struct drm_crtc *crtc)
>  	intel_crtc_update_cursor(crtc, false);
>  	intel_disable_planes(crtc);
>  	intel_disable_primary_hw_plane(dev_priv, plane, pipe);
> +
> +	assert_vblank_disabled(intel_crtc);
>  }
>  
>  static void ironlake_crtc_enable(struct drm_crtc *crtc)
> -- 
> 1.8.5.5
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

Re: [PATCH 2/9] drm/i915: Warn if drm_vblank_get() still works after drm_vblank_off()

[Ville Syrjälä]

On Mon, May 26, 2014 at 03:22:45PM +0200, Daniel Vetter wrote:
> On Mon, May 26, 2014 at 02:46:25PM +0300, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 15 +++++++++++++++
> >  1 file changed, 15 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 3da73ef..da318a7 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -1305,6 +1305,17 @@ static void assert_sprites_disabled(struct drm_i915_private *dev_priv,
> >  	}
> >  }
> >  
> > +static void assert_vblank_disabled(struct intel_crtc *crtc)
> > +{
> > +	struct drm_device *dev = crtc->base.dev;
> > +	enum pipe pipe = crtc->pipe;
> > +
> > +	if (WARN_ON(drm_vblank_get(dev, pipe) == 0)) {
> > +		drm_vblank_put(dev, pipe);
> > +		drm_vblank_off(dev, pipe);
> 
> Imo the _off is too much, since with that it's not just an assert but a
> "... and please make it so if not". Imo better to drop that.

The idea was that if drm_vblank_get() managed to re-enable vblank
interrupts when it wasn't supposed to, we should turn them off again.
But the whole thing is a bug, and another drm_vblank_get() might
happen just after the drm_vblank_off() anyway, so I guess it's a bit
pointless.

> -Daniel
> 
> > +	}
> > +}
> > +
> >  static void ibx_assert_pch_refclk_enabled(struct drm_i915_private *dev_priv)
> >  {
> >  	u32 val;
> > @@ -3885,6 +3896,8 @@ static void intel_crtc_enable_planes(struct drm_crtc *crtc)
> >  	int pipe = intel_crtc->pipe;
> >  	int plane = intel_crtc->plane;
> >  
> > +	assert_vblank_disabled(intel_crtc);
> > +
> >  	intel_enable_primary_hw_plane(dev_priv, plane, pipe);
> >  	intel_enable_planes(crtc);
> >  	/* The fixup needs to happen before cursor is enabled */
> > @@ -3921,6 +3934,8 @@ static void intel_crtc_disable_planes(struct drm_crtc *crtc)
> >  	intel_crtc_update_cursor(crtc, false);
> >  	intel_disable_planes(crtc);
> >  	intel_disable_primary_hw_plane(dev_priv, plane, pipe);
> > +
> > +	assert_vblank_disabled(intel_crtc);
> >  }
> >  
> >  static void ironlake_crtc_enable(struct drm_crtc *crtc)
> > -- 
> > 1.8.5.5
> > 
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 2/9] drm/i915: Warn if drm_vblank_get() still works after drm_vblank_off()

[Daniel Vetter]

On Mon, May 26, 2014 at 04:36:26PM +0300, Ville Syrjälä wrote:
> On Mon, May 26, 2014 at 03:22:45PM +0200, Daniel Vetter wrote:
> > On Mon, May 26, 2014 at 02:46:25PM +0300, ville.syrjala@linux.intel.com wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_display.c | 15 +++++++++++++++
> > >  1 file changed, 15 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > index 3da73ef..da318a7 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -1305,6 +1305,17 @@ static void assert_sprites_disabled(struct drm_i915_private *dev_priv,
> > >  	}
> > >  }
> > >  
> > > +static void assert_vblank_disabled(struct intel_crtc *crtc)
> > > +{
> > > +	struct drm_device *dev = crtc->base.dev;
> > > +	enum pipe pipe = crtc->pipe;
> > > +
> > > +	if (WARN_ON(drm_vblank_get(dev, pipe) == 0)) {
> > > +		drm_vblank_put(dev, pipe);
> > > +		drm_vblank_off(dev, pipe);
> > 
> > Imo the _off is too much, since with that it's not just an assert but a
> > "... and please make it so if not". Imo better to drop that.
> 
> The idea was that if drm_vblank_get() managed to re-enable vblank
> interrupts when it wasn't supposed to, we should turn them off again.
> But the whole thing is a bug, and another drm_vblank_get() might
> happen just after the drm_vblank_off() anyway, so I guess it's a bit
> pointless.

Yeah, I guess we could also drop the _put, since something is clearly
amiss already and will likely go down in flames soonish.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH v2 2/9] drm/i915: Warn if drm_vblank_get() still works after drm_vblank_off()

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

v2: Drop the drm_vblank_off() (Daniel)
    Use drm_crtc_vblank_{get,put}()

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 3da73ef..94ac51f 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1305,6 +1305,12 @@ static void assert_sprites_disabled(struct drm_i915_private *dev_priv,
 	}
 }
 
+static void assert_vblank_disabled(struct drm_crtc *crtc)
+{
+	if (WARN_ON(drm_crtc_vblank_get(crtc) == 0))
+		drm_crtc_vblank_put(crtc);
+}
+
 static void ibx_assert_pch_refclk_enabled(struct drm_i915_private *dev_priv)
 {
 	u32 val;
@@ -3885,6 +3891,8 @@ static void intel_crtc_enable_planes(struct drm_crtc *crtc)
 	int pipe = intel_crtc->pipe;
 	int plane = intel_crtc->plane;
 
+	assert_vblank_disabled(crtc);
+
 	intel_enable_primary_hw_plane(dev_priv, plane, pipe);
 	intel_enable_planes(crtc);
 	/* The fixup needs to happen before cursor is enabled */
@@ -3921,6 +3929,8 @@ static void intel_crtc_disable_planes(struct drm_crtc *crtc)
 	intel_crtc_update_cursor(crtc, false);
 	intel_disable_planes(crtc);
 	intel_disable_primary_hw_plane(dev_priv, plane, pipe);
+
+	assert_vblank_disabled(crtc);
 }
 
 static void ironlake_crtc_enable(struct drm_crtc *crtc)
-- 
1.8.5.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 2/9] drm/i915: Warn if drm_vblank_get() still works after drm_vblank_off()

[Daniel Vetter]

On Mon, May 26, 2014 at 04:49:28PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> v2: Drop the drm_vblank_off() (Daniel)
>     Use drm_crtc_vblank_{get,put}()
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> ---
>  drivers/gpu/drm/i915/intel_display.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 3da73ef..94ac51f 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -1305,6 +1305,12 @@ static void assert_sprites_disabled(struct drm_i915_private *dev_priv,
>  	}
>  }
>  
> +static void assert_vblank_disabled(struct drm_crtc *crtc)
> +{
> +	if (WARN_ON(drm_crtc_vblank_get(crtc) == 0))
> +		drm_crtc_vblank_put(crtc);
> +}
> +
>  static void ibx_assert_pch_refclk_enabled(struct drm_i915_private *dev_priv)
>  {
>  	u32 val;
> @@ -3885,6 +3891,8 @@ static void intel_crtc_enable_planes(struct drm_crtc *crtc)
>  	int pipe = intel_crtc->pipe;
>  	int plane = intel_crtc->plane;
>  
> +	assert_vblank_disabled(crtc);
> +
>  	intel_enable_primary_hw_plane(dev_priv, plane, pipe);
>  	intel_enable_planes(crtc);
>  	/* The fixup needs to happen before cursor is enabled */
> @@ -3921,6 +3929,8 @@ static void intel_crtc_disable_planes(struct drm_crtc *crtc)
>  	intel_crtc_update_cursor(crtc, false);
>  	intel_disable_planes(crtc);
>  	intel_disable_primary_hw_plane(dev_priv, plane, pipe);
> +
> +	assert_vblank_disabled(crtc);
>  }
>  
>  static void ironlake_crtc_enable(struct drm_crtc *crtc)
> -- 
> 1.8.5.5
> 

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

[PATCH 3/9] drm: Don't clear vblank timestamps when vblank interrupt is disabled

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Clearing the timestamps causes us to send zeroed timestamps to userspace
if they get sent out in response to the drm_vblank_off(). It's better
to send the very latest timestamp and count instead.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_irq.c | 11 -----------
 1 file changed, 11 deletions(-)

diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
index 7b1e08c..6fa7474 100644
--- a/drivers/gpu/drm/drm_irq.c
+++ b/drivers/gpu/drm/drm_irq.c
@@ -56,14 +56,6 @@
 #define DRM_REDUNDANT_VBLIRQ_THRESH_NS 1000000
 
 /*
- * Clear vblank timestamp buffer for a crtc.
- */
-static void clear_vblank_timestamps(struct drm_device *dev, int crtc)
-{
-	memset(dev->vblank[crtc].time, 0, sizeof(dev->vblank[crtc].time));
-}
-
-/*
  * Disable vblank irq's on crtc, make sure that last vblank count
  * of hardware and corresponding consistent software vblank counter
  * are preserved, even if there are any spurious vblank irq's after
@@ -131,9 +123,6 @@ static void vblank_disable_and_save(struct drm_device *dev, int crtc)
 		smp_mb__after_atomic_inc();
 	}
 
-	/* Invalidate all timestamps while vblank irq's are off. */
-	clear_vblank_timestamps(dev, crtc);
-
 	spin_unlock_irqrestore(&dev->vblank_time_lock, irqflags);
 }
 
-- 
1.8.5.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/9] drm: Don't clear vblank timestamps when vblank interrupt is disabled

[Daniel Vetter]

On Mon, May 26, 2014 at 02:46:26PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Clearing the timestamps causes us to send zeroed timestamps to userspace
> if they get sent out in response to the drm_vblank_off(). It's better
> to send the very latest timestamp and count instead.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Do we have a kms_flip testcase which hunts for this? I guess a vblank
event with a vblank sufficiently far into the future would be useful.
Maybe call it vblank_cancel-vs-* or so. Besides the lack of testcase the
patch is Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/drm_irq.c | 11 -----------
>  1 file changed, 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> index 7b1e08c..6fa7474 100644
> --- a/drivers/gpu/drm/drm_irq.c
> +++ b/drivers/gpu/drm/drm_irq.c
> @@ -56,14 +56,6 @@
>  #define DRM_REDUNDANT_VBLIRQ_THRESH_NS 1000000
>  
>  /*
> - * Clear vblank timestamp buffer for a crtc.
> - */
> -static void clear_vblank_timestamps(struct drm_device *dev, int crtc)
> -{
> -	memset(dev->vblank[crtc].time, 0, sizeof(dev->vblank[crtc].time));
> -}
> -
> -/*
>   * Disable vblank irq's on crtc, make sure that last vblank count
>   * of hardware and corresponding consistent software vblank counter
>   * are preserved, even if there are any spurious vblank irq's after
> @@ -131,9 +123,6 @@ static void vblank_disable_and_save(struct drm_device *dev, int crtc)
>  		smp_mb__after_atomic_inc();
>  	}
>  
> -	/* Invalidate all timestamps while vblank irq's are off. */
> -	clear_vblank_timestamps(dev, crtc);
> -
>  	spin_unlock_irqrestore(&dev->vblank_time_lock, irqflags);
>  }
>  
> -- 
> 1.8.5.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[PATCH 4/9] drm: Move drm_update_vblank_count()

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Move drm_update_vblank_count() to avoid forward a declaration.
No functional change.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_irq.c | 128 +++++++++++++++++++++++-----------------------
 1 file changed, 64 insertions(+), 64 deletions(-)

diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
index 6fa7474..e12cf69 100644
--- a/drivers/gpu/drm/drm_irq.c
+++ b/drivers/gpu/drm/drm_irq.c
@@ -55,6 +55,70 @@
  */
 #define DRM_REDUNDANT_VBLIRQ_THRESH_NS 1000000
 
+/**
+ * drm_update_vblank_count - update the master vblank counter
+ * @dev: DRM device
+ * @crtc: counter to update
+ *
+ * Call back into the driver to update the appropriate vblank counter
+ * (specified by @crtc).  Deal with wraparound, if it occurred, and
+ * update the last read value so we can deal with wraparound on the next
+ * call if necessary.
+ *
+ * Only necessary when going from off->on, to account for frames we
+ * didn't get an interrupt for.
+ *
+ * Note: caller must hold dev->vbl_lock since this reads & writes
+ * device vblank fields.
+ */
+static void drm_update_vblank_count(struct drm_device *dev, int crtc)
+{
+	u32 cur_vblank, diff, tslot, rc;
+	struct timeval t_vblank;
+
+	/*
+	 * Interrupts were disabled prior to this call, so deal with counter
+	 * wrap if needed.
+	 * NOTE!  It's possible we lost a full dev->max_vblank_count events
+	 * here if the register is small or we had vblank interrupts off for
+	 * a long time.
+	 *
+	 * We repeat the hardware vblank counter & timestamp query until
+	 * we get consistent results. This to prevent races between gpu
+	 * updating its hardware counter while we are retrieving the
+	 * corresponding vblank timestamp.
+	 */
+	do {
+		cur_vblank = dev->driver->get_vblank_counter(dev, crtc);
+		rc = drm_get_last_vbltimestamp(dev, crtc, &t_vblank, 0);
+	} while (cur_vblank != dev->driver->get_vblank_counter(dev, crtc));
+
+	/* Deal with counter wrap */
+	diff = cur_vblank - dev->vblank[crtc].last;
+	if (cur_vblank < dev->vblank[crtc].last) {
+		diff += dev->max_vblank_count;
+
+		DRM_DEBUG("last_vblank[%d]=0x%x, cur_vblank=0x%x => diff=0x%x\n",
+			  crtc, dev->vblank[crtc].last, cur_vblank, diff);
+	}
+
+	DRM_DEBUG("enabling vblank interrupts on crtc %d, missed %d\n",
+		  crtc, diff);
+
+	/* Reinitialize corresponding vblank timestamp if high-precision query
+	 * available. Skip this step if query unsupported or failed. Will
+	 * reinitialize delayed at next vblank interrupt in that case.
+	 */
+	if (rc) {
+		tslot = atomic_read(&dev->vblank[crtc].count) + diff;
+		vblanktimestamp(dev, crtc, tslot) = t_vblank;
+	}
+
+	smp_mb__before_atomic_inc();
+	atomic_add(diff, &dev->vblank[crtc].count);
+	smp_mb__after_atomic_inc();
+}
+
 /*
  * Disable vblank irq's on crtc, make sure that last vblank count
  * of hardware and corresponding consistent software vblank counter
@@ -799,70 +863,6 @@ void drm_send_vblank_event(struct drm_device *dev, int crtc,
 EXPORT_SYMBOL(drm_send_vblank_event);
 
 /**
- * drm_update_vblank_count - update the master vblank counter
- * @dev: DRM device
- * @crtc: counter to update
- *
- * Call back into the driver to update the appropriate vblank counter
- * (specified by @crtc).  Deal with wraparound, if it occurred, and
- * update the last read value so we can deal with wraparound on the next
- * call if necessary.
- *
- * Only necessary when going from off->on, to account for frames we
- * didn't get an interrupt for.
- *
- * Note: caller must hold dev->vbl_lock since this reads & writes
- * device vblank fields.
- */
-static void drm_update_vblank_count(struct drm_device *dev, int crtc)
-{
-	u32 cur_vblank, diff, tslot, rc;
-	struct timeval t_vblank;
-
-	/*
-	 * Interrupts were disabled prior to this call, so deal with counter
-	 * wrap if needed.
-	 * NOTE!  It's possible we lost a full dev->max_vblank_count events
-	 * here if the register is small or we had vblank interrupts off for
-	 * a long time.
-	 *
-	 * We repeat the hardware vblank counter & timestamp query until
-	 * we get consistent results. This to prevent races between gpu
-	 * updating its hardware counter while we are retrieving the
-	 * corresponding vblank timestamp.
-	 */
-	do {
-		cur_vblank = dev->driver->get_vblank_counter(dev, crtc);
-		rc = drm_get_last_vbltimestamp(dev, crtc, &t_vblank, 0);
-	} while (cur_vblank != dev->driver->get_vblank_counter(dev, crtc));
-
-	/* Deal with counter wrap */
-	diff = cur_vblank - dev->vblank[crtc].last;
-	if (cur_vblank < dev->vblank[crtc].last) {
-		diff += dev->max_vblank_count;
-
-		DRM_DEBUG("last_vblank[%d]=0x%x, cur_vblank=0x%x => diff=0x%x\n",
-			  crtc, dev->vblank[crtc].last, cur_vblank, diff);
-	}
-
-	DRM_DEBUG("enabling vblank interrupts on crtc %d, missed %d\n",
-		  crtc, diff);
-
-	/* Reinitialize corresponding vblank timestamp if high-precision query
-	 * available. Skip this step if query unsupported or failed. Will
-	 * reinitialize delayed at next vblank interrupt in that case.
-	 */
-	if (rc) {
-		tslot = atomic_read(&dev->vblank[crtc].count) + diff;
-		vblanktimestamp(dev, crtc, tslot) = t_vblank;
-	}
-
-	smp_mb__before_atomic_inc();
-	atomic_add(diff, &dev->vblank[crtc].count);
-	smp_mb__after_atomic_inc();
-}
-
-/**
  * drm_vblank_enable - enable the vblank interrupt on a CRTC
  * @dev: DRM device
  * @crtc: CRTC in question
-- 
1.8.5.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 5/9] drm: Have the vblank counter account for the time between vblank irq disable and drm_vblank_off()

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

If the vblank irq has already been disabled (via the disable timer) when
we call drm_vblank_off() sample the counter and timestamp one last time.
This will make the sure that the user space visible counter will account
for time between vblank irq disable and drm_vblank_off().

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_irq.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
index e12cf69..bb64f0f 100644
--- a/drivers/gpu/drm/drm_irq.c
+++ b/drivers/gpu/drm/drm_irq.c
@@ -140,6 +140,19 @@ static void vblank_disable_and_save(struct drm_device *dev, int crtc)
 	 */
 	spin_lock_irqsave(&dev->vblank_time_lock, irqflags);
 
+	/*
+	 * If the vblank interrupt was already disbled update the count
+	 * and timestamp to maintain the appearance that the counter
+	 * has been ticking all along until this time. This makes the
+	 * count account for the entire time between drm_vblank_on() and
+	 * drm_vblank_off().
+	 */
+	if (!dev->vblank[crtc].enabled) {
+		drm_update_vblank_count(dev, crtc);
+		spin_unlock_irqrestore(&dev->vblank_time_lock, irqflags);
+		return;
+	}
+
 	dev->driver->disable_vblank(dev, crtc);
 	dev->vblank[crtc].enabled = false;
 
-- 
1.8.5.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 5/9] drm: Have the vblank counter account for the time between vblank irq disable and drm_vblank_off()

[Daniel Vetter]

On Mon, May 26, 2014 at 02:46:28PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> If the vblank irq has already been disabled (via the disable timer) when
> we call drm_vblank_off() sample the counter and timestamp one last time.
> This will make the sure that the user space visible counter will account
> for time between vblank irq disable and drm_vblank_off().
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/drm_irq.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> index e12cf69..bb64f0f 100644
> --- a/drivers/gpu/drm/drm_irq.c
> +++ b/drivers/gpu/drm/drm_irq.c
> @@ -140,6 +140,19 @@ static void vblank_disable_and_save(struct drm_device *dev, int crtc)
>  	 */
>  	spin_lock_irqsave(&dev->vblank_time_lock, irqflags);
>  
> +	/*
> +	 * If the vblank interrupt was already disbled update the count
> +	 * and timestamp to maintain the appearance that the counter
> +	 * has been ticking all along until this time. This makes the
> +	 * count account for the entire time between drm_vblank_on() and
> +	 * drm_vblank_off().
> +	 */
> +	if (!dev->vblank[crtc].enabled) {
> +		drm_update_vblank_count(dev, crtc);
> +		spin_unlock_irqrestore(&dev->vblank_time_lock, irqflags);
> +		return;
> +	}
> +
>  	dev->driver->disable_vblank(dev, crtc);
>  	dev->vblank[crtc].enabled = false;
>  
> -- 
> 1.8.5.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[PATCH 6/9] drm: Avoid random vblank counter jumps if the hardware counter has been reset

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

When drm_vblank_on() is called the hardware vblank counter may have
been reset, so we can't trust that the old values sampled prior to
drm_vblank_off() have anything to do with the new values.

So update the .last count in drm_vblank_on() to make the first
drm_vblank_enable() consider that as the reference point. This
will correct the user space visible counter to account for the
time between drm_vblank_on() and the first drm_vblank_enable()
calls.

For extra safety subtract one from the .last count in drm_vblank_on()
to make sure that user space will never see the same counter value
before and after modeset.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_irq.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
index bb64f0f..54cb85d 100644
--- a/drivers/gpu/drm/drm_irq.c
+++ b/drivers/gpu/drm/drm_irq.c
@@ -1095,6 +1095,18 @@ void drm_vblank_on(struct drm_device *dev, int crtc)
 		atomic_dec(&dev->vblank[crtc].refcount);
 		dev->vblank[crtc].inmodeset = 0;
 	}
+
+	/*
+	 * sample the current counter to avoid random jumps
+	 * when drm_vblank_enable() applies the diff
+	 *
+	 * -1 to make sure user will never see the same
+	 * vblank counter value before and after a modeset
+	 */
+	dev->vblank[crtc].last =
+		(dev->driver->get_vblank_counter(dev, crtc) - 1) &
+		dev->max_vblank_count;
+
 	/* re-enable interrupts if there's are users left */
 	if (atomic_read(&dev->vblank[crtc].refcount) != 0)
 		WARN_ON(drm_vblank_enable(dev, crtc));
-- 
1.8.5.5

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

Re: [PATCH 6/9] drm: Avoid random vblank counter jumps if the hardware counter has been reset

[Daniel Vetter]

On Mon, May 26, 2014 at 02:46:29PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> When drm_vblank_on() is called the hardware vblank counter may have
> been reset, so we can't trust that the old values sampled prior to
> drm_vblank_off() have anything to do with the new values.
> 
> So update the .last count in drm_vblank_on() to make the first
> drm_vblank_enable() consider that as the reference point. This
> will correct the user space visible counter to account for the
> time between drm_vblank_on() and the first drm_vblank_enable()
> calls.
> 
> For extra safety subtract one from the .last count in drm_vblank_on()
> to make sure that user space will never see the same counter value
> before and after modeset.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/drm_irq.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> index bb64f0f..54cb85d 100644
> --- a/drivers/gpu/drm/drm_irq.c
> +++ b/drivers/gpu/drm/drm_irq.c
> @@ -1095,6 +1095,18 @@ void drm_vblank_on(struct drm_device *dev, int crtc)
>  		atomic_dec(&dev->vblank[crtc].refcount);
>  		dev->vblank[crtc].inmodeset = 0;
>  	}
> +
> +	/*
> +	 * sample the current counter to avoid random jumps
> +	 * when drm_vblank_enable() applies the diff
> +	 *
> +	 * -1 to make sure user will never see the same
> +	 * vblank counter value before and after a modeset
> +	 */
> +	dev->vblank[crtc].last =
> +		(dev->driver->get_vblank_counter(dev, crtc) - 1) &
> +		dev->max_vblank_count;
> +
>  	/* re-enable interrupts if there's are users left */
>  	if (atomic_read(&dev->vblank[crtc].refcount) != 0)
>  		WARN_ON(drm_vblank_enable(dev, crtc));
> -- 
> 1.8.5.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[PATCH 7/9] drm: Disable vblank interrupt immediately when drm_vblank_offdelay==0

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Make drm_vblank_put() disable the vblank interrupt immediately when the
refcount drops to zero and drm_vblank_offdelay==0.

Currently drm_vblank_put() would just leave vblank interrupts enabled all
the time if drm_vblank_offdelay==0. In case someone might still want that
behaviour drm_vblank_offdelay is now signed and a negative value will
allow the user to keep vblank interrupts on all the time. Well, since
the first drm_vblank_get() anyway.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 Documentation/DocBook/drm.tmpl          |  1 +
 drivers/gpu/drm/drm_irq.c               | 11 +++++++----
 drivers/gpu/drm/drm_stub.c              |  4 ++--
 drivers/gpu/drm/exynos/exynos_drm_drv.h |  2 +-
 include/drm/drmP.h                      |  2 +-
 5 files changed, 12 insertions(+), 8 deletions(-)

diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl
index 9574bf2..25632b0 100644
--- a/Documentation/DocBook/drm.tmpl
+++ b/Documentation/DocBook/drm.tmpl
@@ -2508,6 +2508,7 @@ void (*disable_vblank) (struct drm_device *dev, int crtc);</synopsis>
       by scheduling a timer. The delay is accessible through the vblankoffdelay
       module parameter or the <varname>drm_vblank_offdelay</varname> global
       variable and expressed in milliseconds. Its default value is 5000 ms.
+      Zero means disable immediately, and a negative value means never disable.
     </para>
     <para>
       When a vertical blanking interrupt occurs drivers only need to call the
diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
index 54cb85d..54a56b2 100644
--- a/drivers/gpu/drm/drm_irq.c
+++ b/drivers/gpu/drm/drm_irq.c
@@ -978,10 +978,13 @@ void drm_vblank_put(struct drm_device *dev, int crtc)
 	BUG_ON(atomic_read(&dev->vblank[crtc].refcount) == 0);
 
 	/* Last user schedules interrupt disable */
-	if (atomic_dec_and_test(&dev->vblank[crtc].refcount) &&
-	    (drm_vblank_offdelay > 0))
-		mod_timer(&dev->vblank[crtc].disable_timer,
-			  jiffies + ((drm_vblank_offdelay * HZ)/1000));
+	if (atomic_dec_and_test(&dev->vblank[crtc].refcount)) {
+		if (drm_vblank_offdelay > 0)
+			mod_timer(&dev->vblank[crtc].disable_timer,
+				  jiffies + ((drm_vblank_offdelay * HZ)/1000));
+		else if (drm_vblank_offdelay == 0)
+			vblank_disable_fn((unsigned long)&dev->vblank[crtc]);
+	}
 }
 EXPORT_SYMBOL(drm_vblank_put);
 
diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c
index 3727ac8..8758f81 100644
--- a/drivers/gpu/drm/drm_stub.c
+++ b/drivers/gpu/drm/drm_stub.c
@@ -49,7 +49,7 @@ EXPORT_SYMBOL(drm_rnodes);
 unsigned int drm_universal_planes = 0;
 EXPORT_SYMBOL(drm_universal_planes);
 
-unsigned int drm_vblank_offdelay = 5000;    /* Default to 5000 msecs. */
+int drm_vblank_offdelay = 5000;    /* Default to 5000 msecs. */
 EXPORT_SYMBOL(drm_vblank_offdelay);
 
 unsigned int drm_timestamp_precision = 20;  /* Default to 20 usecs. */
@@ -66,7 +66,7 @@ MODULE_DESCRIPTION(CORE_DESC);
 MODULE_LICENSE("GPL and additional rights");
 MODULE_PARM_DESC(debug, "Enable debug output");
 MODULE_PARM_DESC(rnodes, "Enable experimental render nodes API");
-MODULE_PARM_DESC(vblankoffdelay, "Delay until vblank irq auto-disable [msecs]");
+MODULE_PARM_DESC(vblankoffdelay, "Delay until vblank irq auto-disable [msecs] (< 0 means never disable)");
 MODULE_PARM_DESC(timestamp_precision_usec, "Max. error on timestamps [usecs]");
 MODULE_PARM_DESC(timestamp_monotonic, "Use monotonic timestamps");
 
diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.h b/drivers/gpu/drm/exynos/exynos_drm_drv.h
index ce3e6a3..9734bec 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_drv.h
+++ b/drivers/gpu/drm/exynos/exynos_drm_drv.h
@@ -40,7 +40,7 @@ struct drm_device;
 struct exynos_drm_overlay;
 struct drm_connector;
 
-extern unsigned int drm_vblank_offdelay;
+extern int drm_vblank_offdelay;
 
 /* this enumerates display type. */
 enum exynos_drm_output_type {
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 76ccaab..979a498 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -1404,7 +1404,7 @@ extern unsigned int drm_debug;
 extern unsigned int drm_rnodes;
 extern unsigned int drm_universal_planes;
 
-extern unsigned int drm_vblank_offdelay;
+extern int drm_vblank_offdelay;
 extern unsigned int drm_timestamp_precision;
 extern unsigned int drm_timestamp_monotonic;
 
-- 
1.8.5.5

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

Re: [Intel-gfx] [PATCH 7/9] drm: Disable vblank interrupt immediately when drm_vblank_offdelay==0

[Daniel Vetter]

On Mon, May 26, 2014 at 02:46:30PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Make drm_vblank_put() disable the vblank interrupt immediately when the
> refcount drops to zero and drm_vblank_offdelay==0.
> 
> Currently drm_vblank_put() would just leave vblank interrupts enabled all
> the time if drm_vblank_offdelay==0. In case someone might still want that
> behaviour drm_vblank_offdelay is now signed and a negative value will
> allow the user to keep vblank interrupts on all the time. Well, since
> the first drm_vblank_get() anyway.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

I think what we actually want is a new dev->vblank_is_race_free or
something which is a boolean. Then we can keep the drm_vblank_offdelay
as-is and seletively kill the entire logic for drivers where this works
correctly.
-Daniel

> ---
>  Documentation/DocBook/drm.tmpl          |  1 +
>  drivers/gpu/drm/drm_irq.c               | 11 +++++++----
>  drivers/gpu/drm/drm_stub.c              |  4 ++--
>  drivers/gpu/drm/exynos/exynos_drm_drv.h |  2 +-
>  include/drm/drmP.h                      |  2 +-
>  5 files changed, 12 insertions(+), 8 deletions(-)
> 
> diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl
> index 9574bf2..25632b0 100644
> --- a/Documentation/DocBook/drm.tmpl
> +++ b/Documentation/DocBook/drm.tmpl
> @@ -2508,6 +2508,7 @@ void (*disable_vblank) (struct drm_device *dev, int crtc);</synopsis>
>        by scheduling a timer. The delay is accessible through the vblankoffdelay
>        module parameter or the <varname>drm_vblank_offdelay</varname> global
>        variable and expressed in milliseconds. Its default value is 5000 ms.
> +      Zero means disable immediately, and a negative value means never disable.
>      </para>
>      <para>
>        When a vertical blanking interrupt occurs drivers only need to call the
> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> index 54cb85d..54a56b2 100644
> --- a/drivers/gpu/drm/drm_irq.c
> +++ b/drivers/gpu/drm/drm_irq.c
> @@ -978,10 +978,13 @@ void drm_vblank_put(struct drm_device *dev, int crtc)
>  	BUG_ON(atomic_read(&dev->vblank[crtc].refcount) == 0);
>  
>  	/* Last user schedules interrupt disable */
> -	if (atomic_dec_and_test(&dev->vblank[crtc].refcount) &&
> -	    (drm_vblank_offdelay > 0))
> -		mod_timer(&dev->vblank[crtc].disable_timer,
> -			  jiffies + ((drm_vblank_offdelay * HZ)/1000));
> +	if (atomic_dec_and_test(&dev->vblank[crtc].refcount)) {
> +		if (drm_vblank_offdelay > 0)
> +			mod_timer(&dev->vblank[crtc].disable_timer,
> +				  jiffies + ((drm_vblank_offdelay * HZ)/1000));
> +		else if (drm_vblank_offdelay == 0)
> +			vblank_disable_fn((unsigned long)&dev->vblank[crtc]);
> +	}
>  }
>  EXPORT_SYMBOL(drm_vblank_put);
>  
> diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c
> index 3727ac8..8758f81 100644
> --- a/drivers/gpu/drm/drm_stub.c
> +++ b/drivers/gpu/drm/drm_stub.c
> @@ -49,7 +49,7 @@ EXPORT_SYMBOL(drm_rnodes);
>  unsigned int drm_universal_planes = 0;
>  EXPORT_SYMBOL(drm_universal_planes);
>  
> -unsigned int drm_vblank_offdelay = 5000;    /* Default to 5000 msecs. */
> +int drm_vblank_offdelay = 5000;    /* Default to 5000 msecs. */
>  EXPORT_SYMBOL(drm_vblank_offdelay);
>  
>  unsigned int drm_timestamp_precision = 20;  /* Default to 20 usecs. */
> @@ -66,7 +66,7 @@ MODULE_DESCRIPTION(CORE_DESC);
>  MODULE_LICENSE("GPL and additional rights");
>  MODULE_PARM_DESC(debug, "Enable debug output");
>  MODULE_PARM_DESC(rnodes, "Enable experimental render nodes API");
> -MODULE_PARM_DESC(vblankoffdelay, "Delay until vblank irq auto-disable [msecs]");
> +MODULE_PARM_DESC(vblankoffdelay, "Delay until vblank irq auto-disable [msecs] (< 0 means never disable)");
>  MODULE_PARM_DESC(timestamp_precision_usec, "Max. error on timestamps [usecs]");
>  MODULE_PARM_DESC(timestamp_monotonic, "Use monotonic timestamps");
>  
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.h b/drivers/gpu/drm/exynos/exynos_drm_drv.h
> index ce3e6a3..9734bec 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_drv.h
> +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.h
> @@ -40,7 +40,7 @@ struct drm_device;
>  struct exynos_drm_overlay;
>  struct drm_connector;
>  
> -extern unsigned int drm_vblank_offdelay;
> +extern int drm_vblank_offdelay;
>  
>  /* this enumerates display type. */
>  enum exynos_drm_output_type {
> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> index 76ccaab..979a498 100644
> --- a/include/drm/drmP.h
> +++ b/include/drm/drmP.h
> @@ -1404,7 +1404,7 @@ extern unsigned int drm_debug;
>  extern unsigned int drm_rnodes;
>  extern unsigned int drm_universal_planes;
>  
> -extern unsigned int drm_vblank_offdelay;
> +extern int drm_vblank_offdelay;
>  extern unsigned int drm_timestamp_precision;
>  extern unsigned int drm_timestamp_monotonic;
>  
> -- 
> 1.8.5.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[PATCH 10/9] drm: Add dev->vblank_disable_immediate flag

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Add a flag to drm_device which will cause the vblank code to bypass the
disable timer and always disable the vblank interrupt immediately when
the last reference is dropped.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_irq.c |  6 +++---
 include/drm/drmP.h        | 10 ++++++++++
 2 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
index 20a4855..b008803 100644
--- a/drivers/gpu/drm/drm_irq.c
+++ b/drivers/gpu/drm/drm_irq.c
@@ -994,11 +994,11 @@ void drm_vblank_put(struct drm_device *dev, int crtc)
 
 	/* Last user schedules interrupt disable */
 	if (atomic_dec_and_test(&vblank->refcount)) {
-		if (drm_vblank_offdelay > 0)
+		if (dev->vblank_disable_immediate || drm_vblank_offdelay == 0)
+			vblank_disable_fn((unsigned long)vblank);
+		else if (drm_vblank_offdelay > 0)
 			mod_timer(&vblank->disable_timer,
 				  jiffies + ((drm_vblank_offdelay * HZ)/1000));
-		else if (drm_vblank_offdelay == 0)
-			vblank_disable_fn((unsigned long)vblank);
 	}
 }
 EXPORT_SYMBOL(drm_vblank_put);
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 979a498..0944544 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -1117,6 +1117,16 @@ struct drm_device {
 	 */
 	bool vblank_disable_allowed;
 
+	/*
+	 * If true, vblank interrupt will be disabled immediately when the
+	 * refcount drops to zero, as opposed to via the vblank disable
+	 * timer.
+	 * This can be set to true it the hardware has a working vblank
+	 * counter and the driver uses drm_vblank_on() and drm_vblank_off()
+	 * appropriately.
+	 */
+	bool vblank_disable_immediate;
+
 	/* array of size num_crtcs */
 	struct drm_vblank_crtc *vblank;
 
-- 
1.8.5.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 11/9] drm/i915: Opt out of vblank disable timer on >gen2

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Now that the vblank races are plugged, we can opt out of using
the vblank disable timer and just let vblank interrupts get
disabled immediately when the last reference is dropped.

Gen2 is the exception since it has no hardware frame counter.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 28bae6e..4b2e7af 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -4364,6 +4364,14 @@ void intel_irq_init(struct drm_device *dev)
 		dev->max_vblank_count = 0xffffff; /* only 24 bits of frame count */
 	}
 
+	/*
+	 * Opt out of the vblank disable timer on everything except gen2.
+	 * Gen2 doesn't have a hardware frame counter and so depends on
+	 * vblank interrupts to produce sane vblank seuquence numbers.
+	 */
+	if (!IS_GEN2(dev))
+		dev->vblank_disable_immediate = true;
+
 	if (drm_core_check_feature(dev, DRIVER_MODESET)) {
 		dev->driver->get_vblank_timestamp = i915_get_vblank_timestamp;
 		dev->driver->get_scanout_position = i915_get_crtc_scanoutpos;
-- 
1.8.5.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 11/9] drm/i915: Opt out of vblank disable timer on >gen2

[Daniel Vetter]

On Mon, May 26, 2014 at 05:26:48PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Now that the vblank races are plugged, we can opt out of using
> the vblank disable timer and just let vblank interrupts get
> disabled immediately when the last reference is dropped.
> 
> Gen2 is the exception since it has no hardware frame counter.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

For both patches: Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/i915_irq.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 28bae6e..4b2e7af 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -4364,6 +4364,14 @@ void intel_irq_init(struct drm_device *dev)
>  		dev->max_vblank_count = 0xffffff; /* only 24 bits of frame count */
>  	}
>  
> +	/*
> +	 * Opt out of the vblank disable timer on everything except gen2.
> +	 * Gen2 doesn't have a hardware frame counter and so depends on
> +	 * vblank interrupts to produce sane vblank seuquence numbers.
> +	 */
> +	if (!IS_GEN2(dev))
> +		dev->vblank_disable_immediate = true;
> +
>  	if (drm_core_check_feature(dev, DRIVER_MODESET)) {
>  		dev->driver->get_vblank_timestamp = i915_get_vblank_timestamp;
>  		dev->driver->get_scanout_position = i915_get_crtc_scanoutpos;
> -- 
> 1.8.5.5
> 

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

Re: [PATCH 11/9] drm/i915: Opt out of vblank disable timer on >gen2

[Daniel Vetter]

On Mon, May 26, 2014 at 05:26:48PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Now that the vblank races are plugged, we can opt out of using
> the vblank disable timer and just let vblank interrupts get
> disabled immediately when the last reference is dropped.
> 
> Gen2 is the exception since it has no hardware frame counter.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

I've forgotten to mention (I think so at least) that I'd like to have a
new kms_flip subtest which alternates vblank events with longer hrtimer
sleeps and still enables all the precise vblank counter/ts checks we have.
That should give us tons of flip-flopping of the vblank counter.

After all videos run at 25fps, so are about the worst case for this (since
the enable vblank for the 1 frame vblank wait and the for the pageflip,
disabling it each time in between) and we very much don't want to fail
this.

Bonus points if you add a 2nd thread which races against the first one for
added fun (in a 2nd subtest).
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_irq.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 28bae6e..4b2e7af 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -4364,6 +4364,14 @@ void intel_irq_init(struct drm_device *dev)
>  		dev->max_vblank_count = 0xffffff; /* only 24 bits of frame count */
>  	}
>  
> +	/*
> +	 * Opt out of the vblank disable timer on everything except gen2.
> +	 * Gen2 doesn't have a hardware frame counter and so depends on
> +	 * vblank interrupts to produce sane vblank seuquence numbers.
> +	 */
> +	if (!IS_GEN2(dev))
> +		dev->vblank_disable_immediate = true;
> +
>  	if (drm_core_check_feature(dev, DRIVER_MODESET)) {
>  		dev->driver->get_vblank_timestamp = i915_get_vblank_timestamp;
>  		dev->driver->get_scanout_position = i915_get_crtc_scanoutpos;
> -- 
> 1.8.5.5
> 

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

Re: [Intel-gfx] [PATCH 11/9] drm/i915: Opt out of vblank disable timer on >gen2

[Paulo Zanoni]

2014-05-26 11:26 GMT-03:00  <ville.syrjala@linux.intel.com>:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Now that the vblank races are plugged, we can opt out of using
> the vblank disable timer and just let vblank interrupts get
> disabled immediately when the last reference is dropped.
>
> Gen2 is the exception since it has no hardware frame counter.

Hi

Remember last week's FBC vblank optimization patch that had an
erroneous drm_crtc_vblank_get() instead of drm_crtc_vblank_count()?
After I fixed the bug in the patch I realized that it was the
unbalanced vblank_get() call that moved PC state residency up.

I did some experiments, and on my specific BDW machine, after running
"powertop --auto-tune", I get about 15-25% PC7 residency without FBC.
If I revert this patch, the number jumps to 40-45%. With FBC, the PC7
residency goes from 60-70% to 85-90% when I revert this patch. I'm
running just an idle Cinnamon with an open terminal.

So, since the commit message lacks more details, what are the
downsides of reverting this patch? What are the advantages of opting
out of the vblank timer? I see my desktop does tons and tons of vblank
get/put calls per second, so the disable timer makes a lot of sense.

I also wish there was some easy way to check how this patch (or its
revert) affect a bunch of different workloads...

(Also CCing Chris for insightful comments on performance)

Thanks,
Paulo

>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_irq.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 28bae6e..4b2e7af 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -4364,6 +4364,14 @@ void intel_irq_init(struct drm_device *dev)
>                 dev->max_vblank_count = 0xffffff; /* only 24 bits of frame count */
>         }
>
> +       /*
> +        * Opt out of the vblank disable timer on everything except gen2.
> +        * Gen2 doesn't have a hardware frame counter and so depends on
> +        * vblank interrupts to produce sane vblank seuquence numbers.
> +        */
> +       if (!IS_GEN2(dev))
> +               dev->vblank_disable_immediate = true;
> +
>         if (drm_core_check_feature(dev, DRIVER_MODESET)) {
>                 dev->driver->get_vblank_timestamp = i915_get_vblank_timestamp;
>                 dev->driver->get_scanout_position = i915_get_crtc_scanoutpos;
> --
> 1.8.5.5
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



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

Re: [Intel-gfx] [PATCH 11/9] drm/i915: Opt out of vblank disable timer on >gen2

[Ville Syrjälä]

On Thu, Nov 19, 2015 at 05:44:51PM -0200, Paulo Zanoni wrote:
> 2014-05-26 11:26 GMT-03:00  <ville.syrjala@linux.intel.com>:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > Now that the vblank races are plugged, we can opt out of using
> > the vblank disable timer and just let vblank interrupts get
> > disabled immediately when the last reference is dropped.
> >
> > Gen2 is the exception since it has no hardware frame counter.
> 
> Hi
> 
> Remember last week's FBC vblank optimization patch that had an
> erroneous drm_crtc_vblank_get() instead of drm_crtc_vblank_count()?
> After I fixed the bug in the patch I realized that it was the
> unbalanced vblank_get() call that moved PC state residency up.
> 
> I did some experiments, and on my specific BDW machine, after running
> "powertop --auto-tune", I get about 15-25% PC7 residency without FBC.
> If I revert this patch, the number jumps to 40-45%. With FBC, the PC7
> residency goes from 60-70% to 85-90% when I revert this patch. I'm
> running just an idle Cinnamon with an open terminal.
> 
> So, since the commit message lacks more details, what are the
> downsides of reverting this patch? What are the advantages of opting
> out of the vblank timer? I see my desktop does tons and tons of vblank
> get/put calls per second, so the disable timer makes a lot of sense.

"Idle" desktop :(

Really the immediate disable should save power. Where are these tons of
vblank get/puts coming from actually? I would assume you'd get a handful
per frame at most, and that when you're actually doing something. On an
idle system I would expect nothing at all happens during most frames.

Not sure, but I guess it's possible the extra register accesses in the
get/puts actually cause the display to exit low power states all the time,
or something.

There's also this note in Bspec (for HSW at least):
 "Workaround : Do not enable and unmask this interrupt if the associated
  pipe is disabled.  Do not leave this interrupt enabled and unmasked
  after the associated pipe is disabled."
which we took to mean that having the interrupt masked but enabled is
fine. But maybe we'd actually have to frob IER too to avoid wasting
power somehow?

> I also wish there was some easy way to check how this patch (or its
> revert) affect a bunch of different workloads...
> 
> (Also CCing Chris for insightful comments on performance)

IIRC Chris had a patch to not disable the interrupt immediately when
the refcount drops to 0, but instead delay the disable until the next
interrupt actually happens. But I guess it didn't go in? Probably I
should have reviewed it but didn't. It sounds like a decent idea to
me in any case for the active use case.

> 
> Thanks,
> Paulo
> 
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_irq.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > index 28bae6e..4b2e7af 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -4364,6 +4364,14 @@ void intel_irq_init(struct drm_device *dev)
> >                 dev->max_vblank_count = 0xffffff; /* only 24 bits of frame count */
> >         }
> >
> > +       /*
> > +        * Opt out of the vblank disable timer on everything except gen2.
> > +        * Gen2 doesn't have a hardware frame counter and so depends on
> > +        * vblank interrupts to produce sane vblank seuquence numbers.
> > +        */
> > +       if (!IS_GEN2(dev))
> > +               dev->vblank_disable_immediate = true;
> > +
> >         if (drm_core_check_feature(dev, DRIVER_MODESET)) {
> >                 dev->driver->get_vblank_timestamp = i915_get_vblank_timestamp;
> >                 dev->driver->get_scanout_position = i915_get_crtc_scanoutpos;
> > --
> > 1.8.5.5
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 
> -- 
> Paulo Zanoni

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH 11/9] drm/i915: Opt out of vblank disable timer on >gen2

[Paulo Zanoni]

2015-11-19 18:06 GMT-02:00 Ville Syrjälä <ville.syrjala@linux.intel.com>:
> On Thu, Nov 19, 2015 at 05:44:51PM -0200, Paulo Zanoni wrote:
>> 2014-05-26 11:26 GMT-03:00  <ville.syrjala@linux.intel.com>:
>> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> >
>> > Now that the vblank races are plugged, we can opt out of using
>> > the vblank disable timer and just let vblank interrupts get
>> > disabled immediately when the last reference is dropped.
>> >
>> > Gen2 is the exception since it has no hardware frame counter.
>>
>> Hi
>>
>> Remember last week's FBC vblank optimization patch that had an
>> erroneous drm_crtc_vblank_get() instead of drm_crtc_vblank_count()?
>> After I fixed the bug in the patch I realized that it was the
>> unbalanced vblank_get() call that moved PC state residency up.
>>
>> I did some experiments, and on my specific BDW machine, after running
>> "powertop --auto-tune", I get about 15-25% PC7 residency without FBC.
>> If I revert this patch, the number jumps to 40-45%. With FBC, the PC7
>> residency goes from 60-70% to 85-90% when I revert this patch. I'm
>> running just an idle Cinnamon with an open terminal.
>>
>> So, since the commit message lacks more details, what are the
>> downsides of reverting this patch? What are the advantages of opting
>> out of the vblank timer? I see my desktop does tons and tons of vblank
>> get/put calls per second, so the disable timer makes a lot of sense.
>
> "Idle" desktop :(

My first realization of this little problem was when I was
implementing runtime PM :)


>
> Really the immediate disable should save power. Where are these tons of
> vblank get/puts coming from actually?

I'll take a finer look tomorrow, but I assume it's probably some
application redrawing. I see it does calm down sometimes, but that's
not enough to get better PC7 residency.


> I would assume you'd get a handful
> per frame at most, and that when you're actually doing something. On an
> idle system I would expect nothing at all happens during most frames.
>
> Not sure, but I guess it's possible the extra register accesses in the
> get/puts actually cause the display to exit low power states all the time,
> or something.

I tried replacing the register macros with the _FW version and that didn't help.


>
> There's also this note in Bspec (for HSW at least):

I think this not is present on most (all?) gens.


>  "Workaround : Do not enable and unmask this interrupt if the associated
>   pipe is disabled.  Do not leave this interrupt enabled and unmasked
>   after the associated pipe is disabled."
> which we took to mean that having the interrupt masked but enabled is
> fine.

I'm aware of this, but I think the problem is that the resources
drained by the constant enable+disable+enable+disable outweigh the
resources saved by turning off vblanks. Not sure if there's an extra
reason why BSpec asks us to immediately disable vblanks though...

So, to summarize, the main (only?) reason is the BSpec comment?


> But maybe we'd actually have to frob IER too to avoid wasting
> power somehow?

With the interrupt masked on IMR, I don't think IER matters.

>
>> I also wish there was some easy way to check how this patch (or its
>> revert) affect a bunch of different workloads...
>>
>> (Also CCing Chris for insightful comments on performance)
>
> IIRC Chris had a patch to not disable the interrupt immediately when
> the refcount drops to 0, but instead delay the disable until the next
> interrupt actually happens. But I guess it didn't go in? Probably I
> should have reviewed it but didn't. It sounds like a decent idea to
> me in any case for the active use case.
>
>>
>> Thanks,
>> Paulo
>>
>> >
>> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> > ---
>> >  drivers/gpu/drm/i915/i915_irq.c | 8 ++++++++
>> >  1 file changed, 8 insertions(+)
>> >
>> > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
>> > index 28bae6e..4b2e7af 100644
>> > --- a/drivers/gpu/drm/i915/i915_irq.c
>> > +++ b/drivers/gpu/drm/i915/i915_irq.c
>> > @@ -4364,6 +4364,14 @@ void intel_irq_init(struct drm_device *dev)
>> >                 dev->max_vblank_count = 0xffffff; /* only 24 bits of frame count */
>> >         }
>> >
>> > +       /*
>> > +        * Opt out of the vblank disable timer on everything except gen2.
>> > +        * Gen2 doesn't have a hardware frame counter and so depends on
>> > +        * vblank interrupts to produce sane vblank seuquence numbers.
>> > +        */
>> > +       if (!IS_GEN2(dev))
>> > +               dev->vblank_disable_immediate = true;
>> > +
>> >         if (drm_core_check_feature(dev, DRIVER_MODESET)) {
>> >                 dev->driver->get_vblank_timestamp = i915_get_vblank_timestamp;
>> >                 dev->driver->get_scanout_position = i915_get_crtc_scanoutpos;
>> > --
>> > 1.8.5.5
>> >
>> > _______________________________________________
>> > Intel-gfx mailing list
>> > Intel-gfx@lists.freedesktop.org
>> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>
>>
>>
>> --
>> Paulo Zanoni
>
> --
> Ville Syrjälä
> Intel OTC



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

Re: [Intel-gfx] [PATCH 11/9] drm/i915: Opt out of vblank disable timer on >gen2

[Ville Syrjälä]

On Thu, Nov 19, 2015 at 06:35:04PM -0200, Paulo Zanoni wrote:
> 2015-11-19 18:06 GMT-02:00 Ville Syrjälä <ville.syrjala@linux.intel.com>:
> > On Thu, Nov 19, 2015 at 05:44:51PM -0200, Paulo Zanoni wrote:
> >> 2014-05-26 11:26 GMT-03:00  <ville.syrjala@linux.intel.com>:
> >> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >> >
> >> > Now that the vblank races are plugged, we can opt out of using
> >> > the vblank disable timer and just let vblank interrupts get
> >> > disabled immediately when the last reference is dropped.
> >> >
> >> > Gen2 is the exception since it has no hardware frame counter.
> >>
> >> Hi
> >>
> >> Remember last week's FBC vblank optimization patch that had an
> >> erroneous drm_crtc_vblank_get() instead of drm_crtc_vblank_count()?
> >> After I fixed the bug in the patch I realized that it was the
> >> unbalanced vblank_get() call that moved PC state residency up.
> >>
> >> I did some experiments, and on my specific BDW machine, after running
> >> "powertop --auto-tune", I get about 15-25% PC7 residency without FBC.
> >> If I revert this patch, the number jumps to 40-45%. With FBC, the PC7
> >> residency goes from 60-70% to 85-90% when I revert this patch. I'm
> >> running just an idle Cinnamon with an open terminal.
> >>
> >> So, since the commit message lacks more details, what are the
> >> downsides of reverting this patch? What are the advantages of opting
> >> out of the vblank timer? I see my desktop does tons and tons of vblank
> >> get/put calls per second, so the disable timer makes a lot of sense.
> >
> > "Idle" desktop :(
> 
> My first realization of this little problem was when I was
> implementing runtime PM :)
> 
> 
> >
> > Really the immediate disable should save power. Where are these tons of
> > vblank get/puts coming from actually?
> 
> I'll take a finer look tomorrow, but I assume it's probably some
> application redrawing. I see it does calm down sometimes, but that's
> not enough to get better PC7 residency.
> 
> 
> > I would assume you'd get a handful
> > per frame at most, and that when you're actually doing something. On an
> > idle system I would expect nothing at all happens during most frames.
> >
> > Not sure, but I guess it's possible the extra register accesses in the
> > get/puts actually cause the display to exit low power states all the time,
> > or something.
> 
> I tried replacing the register macros with the _FW version and that didn't help.

Well, that would just get rid of the unclaimed reg checks. Nothing more
I think.

> 
> 
> >
> > There's also this note in Bspec (for HSW at least):
> 
> I think this not is present on most (all?) gens.

Doesn't really prove anything.

> >  "Workaround : Do not enable and unmask this interrupt if the associated
> >   pipe is disabled.  Do not leave this interrupt enabled and unmasked
> >   after the associated pipe is disabled."
> > which we took to mean that having the interrupt masked but enabled is
> > fine.
> 
> I'm aware of this, but I think the problem is that the resources
> drained by the constant enable+disable+enable+disable outweigh the
> resources saved by turning off vblanks.

Well the CPU is awake anyway doing the get/put, so not sure why a a few
extra register accesses there would have such a huge impact.

> Not sure if there's an extra
> reason why BSpec asks us to immediately disable vblanks though...
> 
> So, to summarize, the main (only?) reason is the BSpec comment?

The point is not to wake up due to interrupts when we don't need them.

> 
> 
> > But maybe we'd actually have to frob IER too to avoid wasting
> > power somehow?
> 
> With the interrupt masked on IMR, I don't think IER matters.

I'm not sure anyone actually verified that.

> 
> >
> >> I also wish there was some easy way to check how this patch (or its
> >> revert) affect a bunch of different workloads...
> >>
> >> (Also CCing Chris for insightful comments on performance)
> >
> > IIRC Chris had a patch to not disable the interrupt immediately when
> > the refcount drops to 0, but instead delay the disable until the next
> > interrupt actually happens. But I guess it didn't go in? Probably I
> > should have reviewed it but didn't. It sounds like a decent idea to
> > me in any case for the active use case.
> >
> >>
> >> Thanks,
> >> Paulo
> >>
> >> >
> >> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >> > ---
> >> >  drivers/gpu/drm/i915/i915_irq.c | 8 ++++++++
> >> >  1 file changed, 8 insertions(+)
> >> >
> >> > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> >> > index 28bae6e..4b2e7af 100644
> >> > --- a/drivers/gpu/drm/i915/i915_irq.c
> >> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> >> > @@ -4364,6 +4364,14 @@ void intel_irq_init(struct drm_device *dev)
> >> >                 dev->max_vblank_count = 0xffffff; /* only 24 bits of frame count */
> >> >         }
> >> >
> >> > +       /*
> >> > +        * Opt out of the vblank disable timer on everything except gen2.
> >> > +        * Gen2 doesn't have a hardware frame counter and so depends on
> >> > +        * vblank interrupts to produce sane vblank seuquence numbers.
> >> > +        */
> >> > +       if (!IS_GEN2(dev))
> >> > +               dev->vblank_disable_immediate = true;
> >> > +
> >> >         if (drm_core_check_feature(dev, DRIVER_MODESET)) {
> >> >                 dev->driver->get_vblank_timestamp = i915_get_vblank_timestamp;
> >> >                 dev->driver->get_scanout_position = i915_get_crtc_scanoutpos;
> >> > --
> >> > 1.8.5.5
> >> >
> >> > _______________________________________________
> >> > Intel-gfx mailing list
> >> > Intel-gfx@lists.freedesktop.org
> >> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >>
> >>
> >>
> >> --
> >> Paulo Zanoni
> >
> > --
> > Ville Syrjälä
> > Intel OTC
> 
> 
> 
> -- 
> Paulo Zanoni

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 11/9] drm/i915: Opt out of vblank disable timer on >gen2

[Ville Syrjälä]

On Thu, Nov 19, 2015 at 10:53:30PM +0200, Ville Syrjälä wrote:
> On Thu, Nov 19, 2015 at 06:35:04PM -0200, Paulo Zanoni wrote:
> > 2015-11-19 18:06 GMT-02:00 Ville Syrjälä <ville.syrjala@linux.intel.com>:
> > > On Thu, Nov 19, 2015 at 05:44:51PM -0200, Paulo Zanoni wrote:
> > >> 2014-05-26 11:26 GMT-03:00  <ville.syrjala@linux.intel.com>:
> > >> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > >> >
> > >> > Now that the vblank races are plugged, we can opt out of using
> > >> > the vblank disable timer and just let vblank interrupts get
> > >> > disabled immediately when the last reference is dropped.
> > >> >
> > >> > Gen2 is the exception since it has no hardware frame counter.
> > >>
> > >> Hi
> > >>
> > >> Remember last week's FBC vblank optimization patch that had an
> > >> erroneous drm_crtc_vblank_get() instead of drm_crtc_vblank_count()?
> > >> After I fixed the bug in the patch I realized that it was the
> > >> unbalanced vblank_get() call that moved PC state residency up.
> > >>
> > >> I did some experiments, and on my specific BDW machine, after running
> > >> "powertop --auto-tune", I get about 15-25% PC7 residency without FBC.
> > >> If I revert this patch, the number jumps to 40-45%. With FBC, the PC7
> > >> residency goes from 60-70% to 85-90% when I revert this patch. I'm
> > >> running just an idle Cinnamon with an open terminal.
> > >>
> > >> So, since the commit message lacks more details, what are the
> > >> downsides of reverting this patch? What are the advantages of opting
> > >> out of the vblank timer? I see my desktop does tons and tons of vblank
> > >> get/put calls per second, so the disable timer makes a lot of sense.
> > >
> > > "Idle" desktop :(
> > 
> > My first realization of this little problem was when I was
> > implementing runtime PM :)
> > 
> > 
> > >
> > > Really the immediate disable should save power. Where are these tons of
> > > vblank get/puts coming from actually?
> > 
> > I'll take a finer look tomorrow, but I assume it's probably some
> > application redrawing. I see it does calm down sometimes, but that's
> > not enough to get better PC7 residency.
> > 
> > 
> > > I would assume you'd get a handful
> > > per frame at most, and that when you're actually doing something. On an
> > > idle system I would expect nothing at all happens during most frames.
> > >
> > > Not sure, but I guess it's possible the extra register accesses in the
> > > get/puts actually cause the display to exit low power states all the time,
> > > or something.
> > 
> > I tried replacing the register macros with the _FW version and that didn't help.
> 
> Well, that would just get rid of the unclaimed reg checks. Nothing more
> I think.
> 
> > 
> > 
> > >
> > > There's also this note in Bspec (for HSW at least):
> > 
> > I think this not is present on most (all?) gens.
> 
> Doesn't really prove anything.
> 
> > >  "Workaround : Do not enable and unmask this interrupt if the associated
> > >   pipe is disabled.  Do not leave this interrupt enabled and unmasked
> > >   after the associated pipe is disabled."
> > > which we took to mean that having the interrupt masked but enabled is
> > > fine.
> > 
> > I'm aware of this, but I think the problem is that the resources
> > drained by the constant enable+disable+enable+disable outweigh the
> > resources saved by turning off vblanks.
> 
> Well the CPU is awake anyway doing the get/put, so not sure why a a few
> extra register accesses there would have such a huge impact.
> 
> > Not sure if there's an extra
> > reason why BSpec asks us to immediately disable vblanks though...
> > 
> > So, to summarize, the main (only?) reason is the BSpec comment?
> 
> The point is not to wake up due to interrupts when we don't need them.
> 
> > 
> > 
> > > But maybe we'd actually have to frob IER too to avoid wasting
> > > power somehow?
> > 
> > With the interrupt masked on IMR, I don't think IER matters.
> 
> I'm not sure anyone actually verified that.

Well, I just tried this on HSW. And on that one IER didn't make a
difference to pc7 residency (~70%) at least. This was on an actually
idle system ;)

> 
> > 
> > >
> > >> I also wish there was some easy way to check how this patch (or its
> > >> revert) affect a bunch of different workloads...
> > >>
> > >> (Also CCing Chris for insightful comments on performance)
> > >
> > > IIRC Chris had a patch to not disable the interrupt immediately when
> > > the refcount drops to 0, but instead delay the disable until the next
> > > interrupt actually happens. But I guess it didn't go in? Probably I
> > > should have reviewed it but didn't. It sounds like a decent idea to
> > > me in any case for the active use case.
> > >
> > >>
> > >> Thanks,
> > >> Paulo
> > >>
> > >> >
> > >> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > >> > ---
> > >> >  drivers/gpu/drm/i915/i915_irq.c | 8 ++++++++
> > >> >  1 file changed, 8 insertions(+)
> > >> >
> > >> > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > >> > index 28bae6e..4b2e7af 100644
> > >> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > >> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > >> > @@ -4364,6 +4364,14 @@ void intel_irq_init(struct drm_device *dev)
> > >> >                 dev->max_vblank_count = 0xffffff; /* only 24 bits of frame count */
> > >> >         }
> > >> >
> > >> > +       /*
> > >> > +        * Opt out of the vblank disable timer on everything except gen2.
> > >> > +        * Gen2 doesn't have a hardware frame counter and so depends on
> > >> > +        * vblank interrupts to produce sane vblank seuquence numbers.
> > >> > +        */
> > >> > +       if (!IS_GEN2(dev))
> > >> > +               dev->vblank_disable_immediate = true;
> > >> > +
> > >> >         if (drm_core_check_feature(dev, DRIVER_MODESET)) {
> > >> >                 dev->driver->get_vblank_timestamp = i915_get_vblank_timestamp;
> > >> >                 dev->driver->get_scanout_position = i915_get_crtc_scanoutpos;
> > >> > --
> > >> > 1.8.5.5
> > >> >
> > >> > _______________________________________________
> > >> > Intel-gfx mailing list
> > >> > Intel-gfx@lists.freedesktop.org
> > >> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > >>
> > >>
> > >>
> > >> --
> > >> Paulo Zanoni
> > >
> > > --
> > > Ville Syrjälä
> > > Intel OTC
> > 
> > 
> > 
> > -- 
> > Paulo Zanoni
> 
> -- 
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
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] [PATCH 11/9] drm/i915: Opt out of vblank disable timer on >gen2

[Chris Wilson]

On Thu, Nov 19, 2015 at 10:06:01PM +0200, Ville Syrjälä wrote:
> On Thu, Nov 19, 2015 at 05:44:51PM -0200, Paulo Zanoni wrote:
> > 2014-05-26 11:26 GMT-03:00  <ville.syrjala@linux.intel.com>:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > >
> > > Now that the vblank races are plugged, we can opt out of using
> > > the vblank disable timer and just let vblank interrupts get
> > > disabled immediately when the last reference is dropped.
> > >
> > > Gen2 is the exception since it has no hardware frame counter.
> > 
> > Hi
> > 
> > Remember last week's FBC vblank optimization patch that had an
> > erroneous drm_crtc_vblank_get() instead of drm_crtc_vblank_count()?
> > After I fixed the bug in the patch I realized that it was the
> > unbalanced vblank_get() call that moved PC state residency up.
> > 
> > I did some experiments, and on my specific BDW machine, after running
> > "powertop --auto-tune", I get about 15-25% PC7 residency without FBC.
> > If I revert this patch, the number jumps to 40-45%. With FBC, the PC7
> > residency goes from 60-70% to 85-90% when I revert this patch. I'm
> > running just an idle Cinnamon with an open terminal.
> > 
> > So, since the commit message lacks more details, what are the
> > downsides of reverting this patch? What are the advantages of opting
> > out of the vblank timer? I see my desktop does tons and tons of vblank
> > get/put calls per second, so the disable timer makes a lot of sense.
> 
> "Idle" desktop :(
> 
> Really the immediate disable should save power. Where are these tons of
> vblank get/puts coming from actually? I would assume you'd get a handful
> per frame at most, and that when you're actually doing something. On an
> idle system I would expect nothing at all happens during most frames.

Yes, with the immediate disable we end up with a few get/put per frame
of rendering, as userspace queries and queues the next flip/swap. With
more clients, there are more opportunities for queries prior to queuing
the swap. It really shouldn't be more than a handful per frame if all
the clients are vrefresh limited. (The oddity was the vblank_mode=0
rendering where we still maintained the handful of get/put per frame...)
 
> > I also wish there was some easy way to check how this patch (or its
> > revert) affect a bunch of different workloads...

Maybe add the PC residencies to the power meter in intel-gpu-overlay,
alongside the RC residencies?

> > (Also CCing Chris for insightful comments on performance)
> 
> IIRC Chris had a patch to not disable the interrupt immediately when
> the refcount drops to 0, but instead delay the disable until the next
> interrupt actually happens. But I guess it didn't go in? Probably I
> should have reviewed it but didn't. It sounds like a decent idea to
> me in any case for the active use case.

The discussion went off into the barriers around the vblank counter, and
I forgot to bring it up again. I think even Mario was happy enough about
it.

Paulo, you may want to try

http://cgit.freedesktop.org/~ickle/linux-2.6/commit/?h=nightly&id=ef97f4680d316cc8f9656dded1e1e41544c7225e

or you can change the KEEPALIVES value in src/sna/sna_dri2.c for a
similar effect.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 10/9] drm: Add dev->vblank_disable_immediate flag

[Matt Roper]

On Mon, May 26, 2014 at 05:26:47PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Add a flag to drm_device which will cause the vblank code to bypass the
> disable timer and always disable the vblank interrupt immediately when
> the last reference is dropped.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_irq.c |  6 +++---
>  include/drm/drmP.h        | 10 ++++++++++
>  2 files changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> index 20a4855..b008803 100644
> --- a/drivers/gpu/drm/drm_irq.c
> +++ b/drivers/gpu/drm/drm_irq.c
> @@ -994,11 +994,11 @@ void drm_vblank_put(struct drm_device *dev, int crtc)
>  
>  	/* Last user schedules interrupt disable */
>  	if (atomic_dec_and_test(&vblank->refcount)) {
> -		if (drm_vblank_offdelay > 0)
> +		if (dev->vblank_disable_immediate || drm_vblank_offdelay == 0)
> +			vblank_disable_fn((unsigned long)vblank);
> +		else if (drm_vblank_offdelay > 0)
>  			mod_timer(&vblank->disable_timer,
>  				  jiffies + ((drm_vblank_offdelay * HZ)/1000));
> -		else if (drm_vblank_offdelay == 0)
> -			vblank_disable_fn((unsigned long)vblank);
>  	}
>  }
>  EXPORT_SYMBOL(drm_vblank_put);

Would it be better if we just squashed this device flag logic back into
patch 7, but kept the drm_vblank_offdelay == 0 meaning of "never
disable?"  I can see there being people who might already use this when
debugging new and potentially flaky hardware platforms who would be
surprised by the change in behavior.  So basically something like:

        if (drm_vblank_offdelay == 0)
                return
        else if (dev->vblank_disable_immediate)
                vblank_disable_fn((unsigned long)vblank);
        else
                mod_timer(...);


I'd also suggest adding a comment in drm_stub.c to indicate that
drm_vblank_offdelay gets ignored for drivers that set
vblank_disable_immediate.

Other than that, patches 1-8, 10, and 11 are
Reviewed-by: Matt Roper <matthew.d.roper@intel.com>

I'll finish up reviewing #9 and 12-14 tomorrow when I have some more
time.



Matt


> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> index 979a498..0944544 100644
> --- a/include/drm/drmP.h
> +++ b/include/drm/drmP.h
> @@ -1117,6 +1117,16 @@ struct drm_device {
>  	 */
>  	bool vblank_disable_allowed;
>  
> +	/*
> +	 * If true, vblank interrupt will be disabled immediately when the
> +	 * refcount drops to zero, as opposed to via the vblank disable
> +	 * timer.
> +	 * This can be set to true it the hardware has a working vblank
> +	 * counter and the driver uses drm_vblank_on() and drm_vblank_off()
> +	 * appropriately.
> +	 */
> +	bool vblank_disable_immediate;
> +
>  	/* array of size num_crtcs */
>  	struct drm_vblank_crtc *vblank;
>  
> -- 
> 1.8.5.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795

Re: [Intel-gfx] [PATCH 10/9] drm: Add dev->vblank_disable_immediate flag

[Ville Syrjälä]

On Thu, Jun 19, 2014 at 05:41:24PM -0700, Matt Roper wrote:
> On Mon, May 26, 2014 at 05:26:47PM +0300, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Add a flag to drm_device which will cause the vblank code to bypass the
> > disable timer and always disable the vblank interrupt immediately when
> > the last reference is dropped.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/drm_irq.c |  6 +++---
> >  include/drm/drmP.h        | 10 ++++++++++
> >  2 files changed, 13 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> > index 20a4855..b008803 100644
> > --- a/drivers/gpu/drm/drm_irq.c
> > +++ b/drivers/gpu/drm/drm_irq.c
> > @@ -994,11 +994,11 @@ void drm_vblank_put(struct drm_device *dev, int crtc)
> >  
> >  	/* Last user schedules interrupt disable */
> >  	if (atomic_dec_and_test(&vblank->refcount)) {
> > -		if (drm_vblank_offdelay > 0)
> > +		if (dev->vblank_disable_immediate || drm_vblank_offdelay == 0)
> > +			vblank_disable_fn((unsigned long)vblank);
> > +		else if (drm_vblank_offdelay > 0)
> >  			mod_timer(&vblank->disable_timer,
> >  				  jiffies + ((drm_vblank_offdelay * HZ)/1000));
> > -		else if (drm_vblank_offdelay == 0)
> > -			vblank_disable_fn((unsigned long)vblank);
> >  	}
> >  }
> >  EXPORT_SYMBOL(drm_vblank_put);
> 
> Would it be better if we just squashed this device flag logic back into
> patch 7, but kept the drm_vblank_offdelay == 0 meaning of "never
> disable?"  I can see there being people who might already use this when
> debugging new and potentially flaky hardware platforms who would be
> surprised by the change in behavior. So basically something like:
> 
>         if (drm_vblank_offdelay == 0)
>                 return
>         else if (dev->vblank_disable_immediate)
>                 vblank_disable_fn((unsigned long)vblank);
>         else
>                 mod_timer(...);

I'm not sure I want drm_vblank_offdelay affecting drivers that have
vblank_disable_immediate set since it's a global variable. If there
are two devices on the system where one has
vblank_disable_immediate==true and the other doesn't, the user
might want to keep vblank interrupts enabled on the crappy device
all time by frobbing drm_vblank_offdelay.

I agree that changing the meaning of drm_vblank_offdelay=0 is a bit
questionable, but reversing the meaning so that '==0' meas 'never disable'
and '<0' means 'disable immediately' seemed a bit weird for my taste. But
I suppose I could make that change if people think it's better. Or maybe
just forget about the 'disable immediately' behaviour when
vblank_disable_immediate==false?

> I'd also suggest adding a comment in drm_stub.c to indicate that
> drm_vblank_offdelay gets ignored for drivers that set
> vblank_disable_immediate.

Good idea.

> 
> Other than that, patches 1-8, 10, and 11 are
> Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
> 
> I'll finish up reviewing #9 and 12-14 tomorrow when I have some more
> time.
> 
> 
> 
> Matt
> 
> 
> > diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> > index 979a498..0944544 100644
> > --- a/include/drm/drmP.h
> > +++ b/include/drm/drmP.h
> > @@ -1117,6 +1117,16 @@ struct drm_device {
> >  	 */
> >  	bool vblank_disable_allowed;
> >  
> > +	/*
> > +	 * If true, vblank interrupt will be disabled immediately when the
> > +	 * refcount drops to zero, as opposed to via the vblank disable
> > +	 * timer.
> > +	 * This can be set to true it the hardware has a working vblank
> > +	 * counter and the driver uses drm_vblank_on() and drm_vblank_off()
> > +	 * appropriately.
> > +	 */
> > +	bool vblank_disable_immediate;
> > +
> >  	/* array of size num_crtcs */
> >  	struct drm_vblank_crtc *vblank;
> >  
> > -- 
> > 1.8.5.5
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Matt Roper
> Graphics Software Engineer
> IoTG Platform Enabling & Development
> Intel Corporation
> (916) 356-2795

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 10/9] drm: Add dev->vblank_disable_immediate flag

[Daniel Vetter]

On Tue, Jul 29, 2014 at 08:31:55PM +0300, Ville Syrjälä wrote:
> On Thu, Jun 19, 2014 at 05:41:24PM -0700, Matt Roper wrote:
> > On Mon, May 26, 2014 at 05:26:47PM +0300, ville.syrjala@linux.intel.com wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > Add a flag to drm_device which will cause the vblank code to bypass the
> > > disable timer and always disable the vblank interrupt immediately when
> > > the last reference is dropped.
> > > 
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/drm_irq.c |  6 +++---
> > >  include/drm/drmP.h        | 10 ++++++++++
> > >  2 files changed, 13 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> > > index 20a4855..b008803 100644
> > > --- a/drivers/gpu/drm/drm_irq.c
> > > +++ b/drivers/gpu/drm/drm_irq.c
> > > @@ -994,11 +994,11 @@ void drm_vblank_put(struct drm_device *dev, int crtc)
> > >  
> > >  	/* Last user schedules interrupt disable */
> > >  	if (atomic_dec_and_test(&vblank->refcount)) {
> > > -		if (drm_vblank_offdelay > 0)
> > > +		if (dev->vblank_disable_immediate || drm_vblank_offdelay == 0)
> > > +			vblank_disable_fn((unsigned long)vblank);
> > > +		else if (drm_vblank_offdelay > 0)
> > >  			mod_timer(&vblank->disable_timer,
> > >  				  jiffies + ((drm_vblank_offdelay * HZ)/1000));
> > > -		else if (drm_vblank_offdelay == 0)
> > > -			vblank_disable_fn((unsigned long)vblank);
> > >  	}
> > >  }
> > >  EXPORT_SYMBOL(drm_vblank_put);
> > 
> > Would it be better if we just squashed this device flag logic back into
> > patch 7, but kept the drm_vblank_offdelay == 0 meaning of "never
> > disable?"  I can see there being people who might already use this when
> > debugging new and potentially flaky hardware platforms who would be
> > surprised by the change in behavior. So basically something like:
> > 
> >         if (drm_vblank_offdelay == 0)
> >                 return
> >         else if (dev->vblank_disable_immediate)
> >                 vblank_disable_fn((unsigned long)vblank);
> >         else
> >                 mod_timer(...);
> 
> I'm not sure I want drm_vblank_offdelay affecting drivers that have
> vblank_disable_immediate set since it's a global variable. If there
> are two devices on the system where one has
> vblank_disable_immediate==true and the other doesn't, the user
> might want to keep vblank interrupts enabled on the crappy device
> all time by frobbing drm_vblank_offdelay.
> 
> I agree that changing the meaning of drm_vblank_offdelay=0 is a bit
> questionable, but reversing the meaning so that '==0' meas 'never disable'
> and '<0' means 'disable immediately' seemed a bit weird for my taste. But
> I suppose I could make that change if people think it's better. Or maybe
> just forget about the 'disable immediately' behaviour when
> vblank_disable_immediate==false?
> 
> > I'd also suggest adding a comment in drm_stub.c to indicate that
> > drm_vblank_offdelay gets ignored for drivers that set
> > vblank_disable_immediate.
> 
> Good idea.

Yeah, I think that's good enough. There really shouldn't be any need for
drivers which support immediate vblank disabling to ever keep it on for a
while. Presuming the immediate disable actually works ofc.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH v2 1/9] drm: Always reject drm_vblank_get() after drm_vblank_off()

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Make sure drm_vblank_get() never succeeds when called between
drm_vblank_off() and drm_vblank_on(). Borrow a trick from the
old drm_vblank_{pre,post}_modeset() functions and just bump
the refcount in drm_vblank_off() and drop it in drm_vblank_on().

When drm_vblank_get() encounters a >0 refcount and the vblank
interrupt is already disabled it will simply return -EINVAL.

Hopefully the use of inmodeset won't conflict badly with
drm_vblank_{pre,post}_modeset().

For i915 there's a window between drm_vblank_off() and marking the
crtc as inactive where the current code still allows drm_vblank_get().

v2: Describe what drm_vblank_get() does to explain how
    a simple refcount bump manages to fix things (Daniel)

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_irq.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
index b565372..7b1e08c 100644
--- a/drivers/gpu/drm/drm_irq.c
+++ b/drivers/gpu/drm/drm_irq.c
@@ -1039,6 +1039,15 @@ void drm_vblank_off(struct drm_device *dev, int crtc)
 	}
 	spin_unlock(&dev->event_lock);
 
+	/*
+	 * Prevent subsequent drm_vblank_get() from re-enabling
+	 * the vblank interrupt by bumping the refcount.
+	 */
+	if (!dev->vblank[crtc].inmodeset) {
+		atomic_inc(&dev->vblank[crtc].refcount);
+		dev->vblank[crtc].inmodeset = 1;
+	}
+
 	spin_unlock_irqrestore(&dev->vbl_lock, irqflags);
 }
 EXPORT_SYMBOL(drm_vblank_off);
@@ -1079,6 +1088,11 @@ void drm_vblank_on(struct drm_device *dev, int crtc)
 	unsigned long irqflags;
 
 	spin_lock_irqsave(&dev->vbl_lock, irqflags);
+	/* Drop our private "prevent drm_vblank_get" refcount */
+	if (dev->vblank[crtc].inmodeset) {
+		atomic_dec(&dev->vblank[crtc].refcount);
+		dev->vblank[crtc].inmodeset = 0;
+	}
 	/* re-enable interrupts if there's are users left */
 	if (atomic_read(&dev->vblank[crtc].refcount) != 0)
 		WARN_ON(drm_vblank_enable(dev, crtc));
-- 
1.8.5.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 8/9] drm: Reduce the amount of dev->vblank[crtc] in the code

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Declare a local struct drm_vblank_crtc * and use that
instead of having to do dig it out via 'dev->vblank[crtc]'
everywhere.

Performed with the following coccinelle incantation,
and a few manual whitespace cleanups:

@@
identifier func,member;
expression num_crtcs;
struct drm_device *dev;
unsigned int crtc;
@@
func (...) {
+ struct drm_vblank_crtc *vblank;
...
if (crtc >= num_crtcs)
   return ...;
+ vblank = &dev->vblank[crtc];
<+...
(
- dev->vblank[crtc].member
+ vblank->member
|
- &(dev->vblank[crtc])
+ vblank
)
...+>
}

@@
struct drm_device *dev;
int crtc;
identifier member;
expression num_crtcs;
@@
for (crtc = 0; crtc < num_crtcs; crtc++) {
+ struct drm_vblank_crtc *vblank = &dev->vblank[crtc];
+
<+...
(
- dev->vblank[crtc].member
+ vblank->member
|
- &(dev->vblank[crtc])
+ vblank
)
...+>
}

@@
identifier func,member;
@@
func (struct drm_device *dev, int crtc, ...) {
+ struct drm_vblank_crtc *vblank = &dev->vblank[crtc];
<+...
(
- dev->vblank[crtc].member
+ vblank->member
|
- &(dev->vblank[crtc])
+ vblank
)
...+>
}

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_irq.c | 136 +++++++++++++++++++++++++++-------------------
 1 file changed, 80 insertions(+), 56 deletions(-)

diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
index 54a56b2..20a4855 100644
--- a/drivers/gpu/drm/drm_irq.c
+++ b/drivers/gpu/drm/drm_irq.c
@@ -73,6 +73,7 @@
  */
 static void drm_update_vblank_count(struct drm_device *dev, int crtc)
 {
+	struct drm_vblank_crtc *vblank = &dev->vblank[crtc];
 	u32 cur_vblank, diff, tslot, rc;
 	struct timeval t_vblank;
 
@@ -94,12 +95,12 @@ static void drm_update_vblank_count(struct drm_device *dev, int crtc)
 	} while (cur_vblank != dev->driver->get_vblank_counter(dev, crtc));
 
 	/* Deal with counter wrap */
-	diff = cur_vblank - dev->vblank[crtc].last;
-	if (cur_vblank < dev->vblank[crtc].last) {
+	diff = cur_vblank - vblank->last;
+	if (cur_vblank < vblank->last) {
 		diff += dev->max_vblank_count;
 
 		DRM_DEBUG("last_vblank[%d]=0x%x, cur_vblank=0x%x => diff=0x%x\n",
-			  crtc, dev->vblank[crtc].last, cur_vblank, diff);
+			  crtc, vblank->last, cur_vblank, diff);
 	}
 
 	DRM_DEBUG("enabling vblank interrupts on crtc %d, missed %d\n",
@@ -110,12 +111,12 @@ static void drm_update_vblank_count(struct drm_device *dev, int crtc)
 	 * reinitialize delayed at next vblank interrupt in that case.
 	 */
 	if (rc) {
-		tslot = atomic_read(&dev->vblank[crtc].count) + diff;
+		tslot = atomic_read(&vblank->count) + diff;
 		vblanktimestamp(dev, crtc, tslot) = t_vblank;
 	}
 
 	smp_mb__before_atomic_inc();
-	atomic_add(diff, &dev->vblank[crtc].count);
+	atomic_add(diff, &vblank->count);
 	smp_mb__after_atomic_inc();
 }
 
@@ -127,6 +128,7 @@ static void drm_update_vblank_count(struct drm_device *dev, int crtc)
  */
 static void vblank_disable_and_save(struct drm_device *dev, int crtc)
 {
+	struct drm_vblank_crtc *vblank = &dev->vblank[crtc];
 	unsigned long irqflags;
 	u32 vblcount;
 	s64 diff_ns;
@@ -147,14 +149,14 @@ static void vblank_disable_and_save(struct drm_device *dev, int crtc)
 	 * count account for the entire time between drm_vblank_on() and
 	 * drm_vblank_off().
 	 */
-	if (!dev->vblank[crtc].enabled) {
+	if (!vblank->enabled) {
 		drm_update_vblank_count(dev, crtc);
 		spin_unlock_irqrestore(&dev->vblank_time_lock, irqflags);
 		return;
 	}
 
 	dev->driver->disable_vblank(dev, crtc);
-	dev->vblank[crtc].enabled = false;
+	vblank->enabled = false;
 
 	/* No further vblank irq's will be processed after
 	 * this point. Get current hardware vblank count and
@@ -169,9 +171,9 @@ static void vblank_disable_and_save(struct drm_device *dev, int crtc)
 	 * delayed gpu counter increment.
 	 */
 	do {
-		dev->vblank[crtc].last = dev->driver->get_vblank_counter(dev, crtc);
+		vblank->last = dev->driver->get_vblank_counter(dev, crtc);
 		vblrc = drm_get_last_vbltimestamp(dev, crtc, &tvblank, 0);
-	} while (dev->vblank[crtc].last != dev->driver->get_vblank_counter(dev, crtc) && (--count) && vblrc);
+	} while (vblank->last != dev->driver->get_vblank_counter(dev, crtc) && (--count) && vblrc);
 
 	if (!count)
 		vblrc = 0;
@@ -179,7 +181,7 @@ static void vblank_disable_and_save(struct drm_device *dev, int crtc)
 	/* Compute time difference to stored timestamp of last vblank
 	 * as updated by last invocation of drm_handle_vblank() in vblank irq.
 	 */
-	vblcount = atomic_read(&dev->vblank[crtc].count);
+	vblcount = atomic_read(&vblank->count);
 	diff_ns = timeval_to_ns(&tvblank) -
 		  timeval_to_ns(&vblanktimestamp(dev, crtc, vblcount));
 
@@ -196,7 +198,7 @@ static void vblank_disable_and_save(struct drm_device *dev, int crtc)
 	 * hope for the best.
 	 */
 	if ((vblrc > 0) && (abs64(diff_ns) > 1000000)) {
-		atomic_inc(&dev->vblank[crtc].count);
+		atomic_inc(&vblank->count);
 		smp_mb__after_atomic_inc();
 	}
 
@@ -236,8 +238,10 @@ void drm_vblank_cleanup(struct drm_device *dev)
 		return;
 
 	for (crtc = 0; crtc < dev->num_crtcs; crtc++) {
-		del_timer_sync(&dev->vblank[crtc].disable_timer);
-		vblank_disable_fn((unsigned long)&dev->vblank[crtc]);
+		struct drm_vblank_crtc *vblank = &dev->vblank[crtc];
+
+		del_timer_sync(&vblank->disable_timer);
+		vblank_disable_fn((unsigned long)vblank);
 	}
 
 	kfree(dev->vblank);
@@ -270,11 +274,13 @@ int drm_vblank_init(struct drm_device *dev, int num_crtcs)
 		goto err;
 
 	for (i = 0; i < num_crtcs; i++) {
-		dev->vblank[i].dev = dev;
-		dev->vblank[i].crtc = i;
-		init_waitqueue_head(&dev->vblank[i].queue);
-		setup_timer(&dev->vblank[i].disable_timer, vblank_disable_fn,
-			    (unsigned long)&dev->vblank[i]);
+		struct drm_vblank_crtc *vblank = &dev->vblank[i];
+
+		vblank->dev = dev;
+		vblank->crtc = i;
+		init_waitqueue_head(&vblank->queue);
+		setup_timer(&vblank->disable_timer, vblank_disable_fn,
+			    (unsigned long)vblank);
 	}
 
 	DRM_INFO("Supports vblank timestamp caching Rev 2 (21.10.2013).\n");
@@ -426,9 +432,11 @@ int drm_irq_uninstall(struct drm_device *dev)
 	if (dev->num_crtcs) {
 		spin_lock_irqsave(&dev->vbl_lock, irqflags);
 		for (i = 0; i < dev->num_crtcs; i++) {
-			wake_up(&dev->vblank[i].queue);
-			dev->vblank[i].enabled = false;
-			dev->vblank[i].last =
+			struct drm_vblank_crtc *vblank = &dev->vblank[i];
+
+			wake_up(&vblank->queue);
+			vblank->enabled = false;
+			vblank->last =
 				dev->driver->get_vblank_counter(dev, i);
 		}
 		spin_unlock_irqrestore(&dev->vbl_lock, irqflags);
@@ -796,7 +804,9 @@ EXPORT_SYMBOL(drm_get_last_vbltimestamp);
  */
 u32 drm_vblank_count(struct drm_device *dev, int crtc)
 {
-	return atomic_read(&dev->vblank[crtc].count);
+	struct drm_vblank_crtc *vblank = &dev->vblank[crtc];
+
+	return atomic_read(&vblank->count);
 }
 EXPORT_SYMBOL(drm_vblank_count);
 
@@ -816,6 +826,7 @@ EXPORT_SYMBOL(drm_vblank_count);
 u32 drm_vblank_count_and_time(struct drm_device *dev, int crtc,
 			      struct timeval *vblanktime)
 {
+	struct drm_vblank_crtc *vblank = &dev->vblank[crtc];
 	u32 cur_vblank;
 
 	/* Read timestamp from slot of _vblank_time ringbuffer
@@ -824,10 +835,10 @@ u32 drm_vblank_count_and_time(struct drm_device *dev, int crtc,
 	 * a seqlock.
 	 */
 	do {
-		cur_vblank = atomic_read(&dev->vblank[crtc].count);
+		cur_vblank = atomic_read(&vblank->count);
 		*vblanktime = vblanktimestamp(dev, crtc, cur_vblank);
 		smp_rmb();
-	} while (cur_vblank != atomic_read(&dev->vblank[crtc].count));
+	} while (cur_vblank != atomic_read(&vblank->count));
 
 	return cur_vblank;
 }
@@ -882,13 +893,14 @@ EXPORT_SYMBOL(drm_send_vblank_event);
  */
 static int drm_vblank_enable(struct drm_device *dev, int crtc)
 {
+	struct drm_vblank_crtc *vblank = &dev->vblank[crtc];
 	int ret = 0;
 
 	assert_spin_locked(&dev->vbl_lock);
 
 	spin_lock(&dev->vblank_time_lock);
 
-	if (!dev->vblank[crtc].enabled) {
+	if (!vblank->enabled) {
 		/*
 		 * Enable vblank irqs under vblank_time_lock protection.
 		 * All vblank count & timestamp updates are held off
@@ -899,9 +911,9 @@ static int drm_vblank_enable(struct drm_device *dev, int crtc)
 		ret = dev->driver->enable_vblank(dev, crtc);
 		DRM_DEBUG("enabling vblank on crtc %d, ret: %d\n", crtc, ret);
 		if (ret)
-			atomic_dec(&dev->vblank[crtc].refcount);
+			atomic_dec(&vblank->refcount);
 		else {
-			dev->vblank[crtc].enabled = true;
+			vblank->enabled = true;
 			drm_update_vblank_count(dev, crtc);
 		}
 	}
@@ -926,16 +938,17 @@ static int drm_vblank_enable(struct drm_device *dev, int crtc)
  */
 int drm_vblank_get(struct drm_device *dev, int crtc)
 {
+	struct drm_vblank_crtc *vblank = &dev->vblank[crtc];
 	unsigned long irqflags;
 	int ret = 0;
 
 	spin_lock_irqsave(&dev->vbl_lock, irqflags);
 	/* Going from 0->1 means we have to enable interrupts again */
-	if (atomic_add_return(1, &dev->vblank[crtc].refcount) == 1) {
+	if (atomic_add_return(1, &vblank->refcount) == 1) {
 		ret = drm_vblank_enable(dev, crtc);
 	} else {
-		if (!dev->vblank[crtc].enabled) {
-			atomic_dec(&dev->vblank[crtc].refcount);
+		if (!vblank->enabled) {
+			atomic_dec(&vblank->refcount);
 			ret = -EINVAL;
 		}
 	}
@@ -975,15 +988,17 @@ EXPORT_SYMBOL(drm_crtc_vblank_get);
  */
 void drm_vblank_put(struct drm_device *dev, int crtc)
 {
-	BUG_ON(atomic_read(&dev->vblank[crtc].refcount) == 0);
+	struct drm_vblank_crtc *vblank = &dev->vblank[crtc];
+
+	BUG_ON(atomic_read(&vblank->refcount) == 0);
 
 	/* Last user schedules interrupt disable */
-	if (atomic_dec_and_test(&dev->vblank[crtc].refcount)) {
+	if (atomic_dec_and_test(&vblank->refcount)) {
 		if (drm_vblank_offdelay > 0)
-			mod_timer(&dev->vblank[crtc].disable_timer,
+			mod_timer(&vblank->disable_timer,
 				  jiffies + ((drm_vblank_offdelay * HZ)/1000));
 		else if (drm_vblank_offdelay == 0)
-			vblank_disable_fn((unsigned long)&dev->vblank[crtc]);
+			vblank_disable_fn((unsigned long)vblank);
 	}
 }
 EXPORT_SYMBOL(drm_vblank_put);
@@ -1019,6 +1034,7 @@ EXPORT_SYMBOL(drm_crtc_vblank_put);
  */
 void drm_vblank_off(struct drm_device *dev, int crtc)
 {
+	struct drm_vblank_crtc *vblank = &dev->vblank[crtc];
 	struct drm_pending_vblank_event *e, *t;
 	struct timeval now;
 	unsigned long irqflags;
@@ -1026,7 +1042,7 @@ void drm_vblank_off(struct drm_device *dev, int crtc)
 
 	spin_lock_irqsave(&dev->vbl_lock, irqflags);
 	vblank_disable_and_save(dev, crtc);
-	wake_up(&dev->vblank[crtc].queue);
+	wake_up(&vblank->queue);
 
 	/* Send any queued vblank events, lest the natives grow disquiet */
 	seq = drm_vblank_count_and_time(dev, crtc, &now);
@@ -1048,9 +1064,9 @@ void drm_vblank_off(struct drm_device *dev, int crtc)
 	 * Prevent subsequent drm_vblank_get() from re-enabling
 	 * the vblank interrupt by bumping the refcount.
 	 */
-	if (!dev->vblank[crtc].inmodeset) {
-		atomic_inc(&dev->vblank[crtc].refcount);
-		dev->vblank[crtc].inmodeset = 1;
+	if (!vblank->inmodeset) {
+		atomic_inc(&vblank->refcount);
+		vblank->inmodeset = 1;
 	}
 
 	spin_unlock_irqrestore(&dev->vbl_lock, irqflags);
@@ -1090,13 +1106,14 @@ EXPORT_SYMBOL(drm_crtc_vblank_off);
  */
 void drm_vblank_on(struct drm_device *dev, int crtc)
 {
+	struct drm_vblank_crtc *vblank = &dev->vblank[crtc];
 	unsigned long irqflags;
 
 	spin_lock_irqsave(&dev->vbl_lock, irqflags);
 	/* Drop our private "prevent drm_vblank_get" refcount */
-	if (dev->vblank[crtc].inmodeset) {
-		atomic_dec(&dev->vblank[crtc].refcount);
-		dev->vblank[crtc].inmodeset = 0;
+	if (vblank->inmodeset) {
+		atomic_dec(&vblank->refcount);
+		vblank->inmodeset = 0;
 	}
 
 	/*
@@ -1106,12 +1123,12 @@ void drm_vblank_on(struct drm_device *dev, int crtc)
 	 * -1 to make sure user will never see the same
 	 * vblank counter value before and after a modeset
 	 */
-	dev->vblank[crtc].last =
+	vblank->last =
 		(dev->driver->get_vblank_counter(dev, crtc) - 1) &
 		dev->max_vblank_count;
 
 	/* re-enable interrupts if there's are users left */
-	if (atomic_read(&dev->vblank[crtc].refcount) != 0)
+	if (atomic_read(&vblank->refcount) != 0)
 		WARN_ON(drm_vblank_enable(dev, crtc));
 	spin_unlock_irqrestore(&dev->vbl_lock, irqflags);
 }
@@ -1159,6 +1176,8 @@ EXPORT_SYMBOL(drm_crtc_vblank_on);
  */
 void drm_vblank_pre_modeset(struct drm_device *dev, int crtc)
 {
+	struct drm_vblank_crtc *vblank = &dev->vblank[crtc];
+
 	/* vblank is not initialized (IRQ not installed ?), or has been freed */
 	if (!dev->num_crtcs)
 		return;
@@ -1169,10 +1188,10 @@ void drm_vblank_pre_modeset(struct drm_device *dev, int crtc)
 	 * to avoid corrupting the count if multiple, mismatch calls occur),
 	 * so that interrupts remain enabled in the interim.
 	 */
-	if (!dev->vblank[crtc].inmodeset) {
-		dev->vblank[crtc].inmodeset = 0x1;
+	if (!vblank->inmodeset) {
+		vblank->inmodeset = 0x1;
 		if (drm_vblank_get(dev, crtc) == 0)
-			dev->vblank[crtc].inmodeset |= 0x2;
+			vblank->inmodeset |= 0x2;
 	}
 }
 EXPORT_SYMBOL(drm_vblank_pre_modeset);
@@ -1187,21 +1206,22 @@ EXPORT_SYMBOL(drm_vblank_pre_modeset);
  */
 void drm_vblank_post_modeset(struct drm_device *dev, int crtc)
 {
+	struct drm_vblank_crtc *vblank = &dev->vblank[crtc];
 	unsigned long irqflags;
 
 	/* vblank is not initialized (IRQ not installed ?), or has been freed */
 	if (!dev->num_crtcs)
 		return;
 
-	if (dev->vblank[crtc].inmodeset) {
+	if (vblank->inmodeset) {
 		spin_lock_irqsave(&dev->vbl_lock, irqflags);
 		dev->vblank_disable_allowed = true;
 		spin_unlock_irqrestore(&dev->vbl_lock, irqflags);
 
-		if (dev->vblank[crtc].inmodeset & 0x2)
+		if (vblank->inmodeset & 0x2)
 			drm_vblank_put(dev, crtc);
 
-		dev->vblank[crtc].inmodeset = 0;
+		vblank->inmodeset = 0;
 	}
 }
 EXPORT_SYMBOL(drm_vblank_post_modeset);
@@ -1336,6 +1356,7 @@ err_put:
 int drm_wait_vblank(struct drm_device *dev, void *data,
 		    struct drm_file *file_priv)
 {
+	struct drm_vblank_crtc *vblank;
 	union drm_wait_vblank *vblwait = data;
 	int ret;
 	unsigned int flags, seq, crtc, high_crtc;
@@ -1365,6 +1386,8 @@ int drm_wait_vblank(struct drm_device *dev, void *data,
 	if (crtc >= dev->num_crtcs)
 		return -EINVAL;
 
+	vblank = &dev->vblank[crtc];
+
 	ret = drm_vblank_get(dev, crtc);
 	if (ret) {
 		DRM_DEBUG("failed to acquire vblank counter, %d\n", ret);
@@ -1397,11 +1420,11 @@ int drm_wait_vblank(struct drm_device *dev, void *data,
 
 	DRM_DEBUG("waiting on vblank count %d, crtc %d\n",
 		  vblwait->request.sequence, crtc);
-	dev->vblank[crtc].last_wait = vblwait->request.sequence;
-	DRM_WAIT_ON(ret, dev->vblank[crtc].queue, 3 * HZ,
+	vblank->last_wait = vblwait->request.sequence;
+	DRM_WAIT_ON(ret, vblank->queue, 3 * HZ,
 		    (((drm_vblank_count(dev, crtc) -
 		       vblwait->request.sequence) <= (1 << 23)) ||
-		     !dev->vblank[crtc].enabled ||
+		     !vblank->enabled ||
 		     !dev->irq_enabled));
 
 	if (ret != -EINTR) {
@@ -1462,6 +1485,7 @@ static void drm_handle_vblank_events(struct drm_device *dev, int crtc)
  */
 bool drm_handle_vblank(struct drm_device *dev, int crtc)
 {
+	struct drm_vblank_crtc *vblank = &dev->vblank[crtc];
 	u32 vblcount;
 	s64 diff_ns;
 	struct timeval tvblank;
@@ -1477,7 +1501,7 @@ bool drm_handle_vblank(struct drm_device *dev, int crtc)
 	spin_lock_irqsave(&dev->vblank_time_lock, irqflags);
 
 	/* Vblank irq handling disabled. Nothing to do. */
-	if (!dev->vblank[crtc].enabled) {
+	if (!vblank->enabled) {
 		spin_unlock_irqrestore(&dev->vblank_time_lock, irqflags);
 		return false;
 	}
@@ -1487,7 +1511,7 @@ bool drm_handle_vblank(struct drm_device *dev, int crtc)
 	 */
 
 	/* Get current timestamp and count. */
-	vblcount = atomic_read(&dev->vblank[crtc].count);
+	vblcount = atomic_read(&vblank->count);
 	drm_get_last_vbltimestamp(dev, crtc, &tvblank, DRM_CALLED_FROM_VBLIRQ);
 
 	/* Compute time difference to timestamp of last vblank */
@@ -1511,14 +1535,14 @@ bool drm_handle_vblank(struct drm_device *dev, int crtc)
 		 * the timestamp computed above.
 		 */
 		smp_mb__before_atomic_inc();
-		atomic_inc(&dev->vblank[crtc].count);
+		atomic_inc(&vblank->count);
 		smp_mb__after_atomic_inc();
 	} else {
 		DRM_DEBUG("crtc %d: Redundant vblirq ignored. diff_ns = %d\n",
 			  crtc, (int) diff_ns);
 	}
 
-	wake_up(&dev->vblank[crtc].queue);
+	wake_up(&vblank->queue);
 	drm_handle_vblank_events(dev, crtc);
 
 	spin_unlock_irqrestore(&dev->vblank_time_lock, irqflags);
-- 
1.8.5.5

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

Re: [PATCH 8/9] drm: Reduce the amount of dev->vblank[crtc] in the code

[Daniel Vetter]

On Mon, May 26, 2014 at 02:46:31PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Declare a local struct drm_vblank_crtc * and use that
> instead of having to do dig it out via 'dev->vblank[crtc]'
> everywhere.
> 
> Performed with the following coccinelle incantation,
> and a few manual whitespace cleanups:
> 
> @@
> identifier func,member;
> expression num_crtcs;
> struct drm_device *dev;
> unsigned int crtc;
> @@
> func (...) {
> + struct drm_vblank_crtc *vblank;
> ...
> if (crtc >= num_crtcs)
>    return ...;
> + vblank = &dev->vblank[crtc];
> <+...
> (
> - dev->vblank[crtc].member
> + vblank->member
> |
> - &(dev->vblank[crtc])
> + vblank
> )
> ...+>
> }
> 
> @@
> struct drm_device *dev;
> int crtc;
> identifier member;
> expression num_crtcs;
> @@
> for (crtc = 0; crtc < num_crtcs; crtc++) {
> + struct drm_vblank_crtc *vblank = &dev->vblank[crtc];
> +
> <+...
> (
> - dev->vblank[crtc].member
> + vblank->member
> |
> - &(dev->vblank[crtc])
> + vblank
> )
> ...+>
> }
> 
> @@
> identifier func,member;
> @@
> func (struct drm_device *dev, int crtc, ...) {
> + struct drm_vblank_crtc *vblank = &dev->vblank[crtc];
> <+...
> (
> - dev->vblank[crtc].member
> + vblank->member
> |
> - &(dev->vblank[crtc])
> + vblank
> )
> ...+>
> }
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Reviewing coccinelle scripts is a bit easier than reviewing the actual
patch ;-)

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Btw one cleanup on top I've onticed is that drm_update_vblank_count and
vblank_disable_and_save duplicate the same do {} while (vblank_not_stable)
loop. But not quite since one has a timeout. I think one more patch on top
to extract that would be pretty.
-Daniel

> ---
>  drivers/gpu/drm/drm_irq.c | 136 +++++++++++++++++++++++++++-------------------
>  1 file changed, 80 insertions(+), 56 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> index 54a56b2..20a4855 100644
> --- a/drivers/gpu/drm/drm_irq.c
> +++ b/drivers/gpu/drm/drm_irq.c
> @@ -73,6 +73,7 @@
>   */
>  static void drm_update_vblank_count(struct drm_device *dev, int crtc)
>  {
> +	struct drm_vblank_crtc *vblank = &dev->vblank[crtc];
>  	u32 cur_vblank, diff, tslot, rc;
>  	struct timeval t_vblank;
>  
> @@ -94,12 +95,12 @@ static void drm_update_vblank_count(struct drm_device *dev, int crtc)
>  	} while (cur_vblank != dev->driver->get_vblank_counter(dev, crtc));
>  
>  	/* Deal with counter wrap */
> -	diff = cur_vblank - dev->vblank[crtc].last;
> -	if (cur_vblank < dev->vblank[crtc].last) {
> +	diff = cur_vblank - vblank->last;
> +	if (cur_vblank < vblank->last) {
>  		diff += dev->max_vblank_count;
>  
>  		DRM_DEBUG("last_vblank[%d]=0x%x, cur_vblank=0x%x => diff=0x%x\n",
> -			  crtc, dev->vblank[crtc].last, cur_vblank, diff);
> +			  crtc, vblank->last, cur_vblank, diff);
>  	}
>  
>  	DRM_DEBUG("enabling vblank interrupts on crtc %d, missed %d\n",
> @@ -110,12 +111,12 @@ static void drm_update_vblank_count(struct drm_device *dev, int crtc)
>  	 * reinitialize delayed at next vblank interrupt in that case.
>  	 */
>  	if (rc) {
> -		tslot = atomic_read(&dev->vblank[crtc].count) + diff;
> +		tslot = atomic_read(&vblank->count) + diff;
>  		vblanktimestamp(dev, crtc, tslot) = t_vblank;
>  	}
>  
>  	smp_mb__before_atomic_inc();
> -	atomic_add(diff, &dev->vblank[crtc].count);
> +	atomic_add(diff, &vblank->count);
>  	smp_mb__after_atomic_inc();
>  }
>  
> @@ -127,6 +128,7 @@ static void drm_update_vblank_count(struct drm_device *dev, int crtc)
>   */
>  static void vblank_disable_and_save(struct drm_device *dev, int crtc)
>  {
> +	struct drm_vblank_crtc *vblank = &dev->vblank[crtc];
>  	unsigned long irqflags;
>  	u32 vblcount;
>  	s64 diff_ns;
> @@ -147,14 +149,14 @@ static void vblank_disable_and_save(struct drm_device *dev, int crtc)
>  	 * count account for the entire time between drm_vblank_on() and
>  	 * drm_vblank_off().
>  	 */
> -	if (!dev->vblank[crtc].enabled) {
> +	if (!vblank->enabled) {
>  		drm_update_vblank_count(dev, crtc);
>  		spin_unlock_irqrestore(&dev->vblank_time_lock, irqflags);
>  		return;
>  	}
>  
>  	dev->driver->disable_vblank(dev, crtc);
> -	dev->vblank[crtc].enabled = false;
> +	vblank->enabled = false;
>  
>  	/* No further vblank irq's will be processed after
>  	 * this point. Get current hardware vblank count and
> @@ -169,9 +171,9 @@ static void vblank_disable_and_save(struct drm_device *dev, int crtc)
>  	 * delayed gpu counter increment.
>  	 */
>  	do {
> -		dev->vblank[crtc].last = dev->driver->get_vblank_counter(dev, crtc);
> +		vblank->last = dev->driver->get_vblank_counter(dev, crtc);
>  		vblrc = drm_get_last_vbltimestamp(dev, crtc, &tvblank, 0);
> -	} while (dev->vblank[crtc].last != dev->driver->get_vblank_counter(dev, crtc) && (--count) && vblrc);
> +	} while (vblank->last != dev->driver->get_vblank_counter(dev, crtc) && (--count) && vblrc);
>  
>  	if (!count)
>  		vblrc = 0;
> @@ -179,7 +181,7 @@ static void vblank_disable_and_save(struct drm_device *dev, int crtc)
>  	/* Compute time difference to stored timestamp of last vblank
>  	 * as updated by last invocation of drm_handle_vblank() in vblank irq.
>  	 */
> -	vblcount = atomic_read(&dev->vblank[crtc].count);
> +	vblcount = atomic_read(&vblank->count);
>  	diff_ns = timeval_to_ns(&tvblank) -
>  		  timeval_to_ns(&vblanktimestamp(dev, crtc, vblcount));
>  
> @@ -196,7 +198,7 @@ static void vblank_disable_and_save(struct drm_device *dev, int crtc)
>  	 * hope for the best.
>  	 */
>  	if ((vblrc > 0) && (abs64(diff_ns) > 1000000)) {
> -		atomic_inc(&dev->vblank[crtc].count);
> +		atomic_inc(&vblank->count);
>  		smp_mb__after_atomic_inc();
>  	}
>  
> @@ -236,8 +238,10 @@ void drm_vblank_cleanup(struct drm_device *dev)
>  		return;
>  
>  	for (crtc = 0; crtc < dev->num_crtcs; crtc++) {
> -		del_timer_sync(&dev->vblank[crtc].disable_timer);
> -		vblank_disable_fn((unsigned long)&dev->vblank[crtc]);
> +		struct drm_vblank_crtc *vblank = &dev->vblank[crtc];
> +
> +		del_timer_sync(&vblank->disable_timer);
> +		vblank_disable_fn((unsigned long)vblank);
>  	}
>  
>  	kfree(dev->vblank);
> @@ -270,11 +274,13 @@ int drm_vblank_init(struct drm_device *dev, int num_crtcs)
>  		goto err;
>  
>  	for (i = 0; i < num_crtcs; i++) {
> -		dev->vblank[i].dev = dev;
> -		dev->vblank[i].crtc = i;
> -		init_waitqueue_head(&dev->vblank[i].queue);
> -		setup_timer(&dev->vblank[i].disable_timer, vblank_disable_fn,
> -			    (unsigned long)&dev->vblank[i]);
> +		struct drm_vblank_crtc *vblank = &dev->vblank[i];
> +
> +		vblank->dev = dev;
> +		vblank->crtc = i;
> +		init_waitqueue_head(&vblank->queue);
> +		setup_timer(&vblank->disable_timer, vblank_disable_fn,
> +			    (unsigned long)vblank);
>  	}
>  
>  	DRM_INFO("Supports vblank timestamp caching Rev 2 (21.10.2013).\n");
> @@ -426,9 +432,11 @@ int drm_irq_uninstall(struct drm_device *dev)
>  	if (dev->num_crtcs) {
>  		spin_lock_irqsave(&dev->vbl_lock, irqflags);
>  		for (i = 0; i < dev->num_crtcs; i++) {
> -			wake_up(&dev->vblank[i].queue);
> -			dev->vblank[i].enabled = false;
> -			dev->vblank[i].last =
> +			struct drm_vblank_crtc *vblank = &dev->vblank[i];
> +
> +			wake_up(&vblank->queue);
> +			vblank->enabled = false;
> +			vblank->last =
>  				dev->driver->get_vblank_counter(dev, i);
>  		}
>  		spin_unlock_irqrestore(&dev->vbl_lock, irqflags);
> @@ -796,7 +804,9 @@ EXPORT_SYMBOL(drm_get_last_vbltimestamp);
>   */
>  u32 drm_vblank_count(struct drm_device *dev, int crtc)
>  {
> -	return atomic_read(&dev->vblank[crtc].count);
> +	struct drm_vblank_crtc *vblank = &dev->vblank[crtc];
> +
> +	return atomic_read(&vblank->count);
>  }
>  EXPORT_SYMBOL(drm_vblank_count);
>  
> @@ -816,6 +826,7 @@ EXPORT_SYMBOL(drm_vblank_count);
>  u32 drm_vblank_count_and_time(struct drm_device *dev, int crtc,
>  			      struct timeval *vblanktime)
>  {
> +	struct drm_vblank_crtc *vblank = &dev->vblank[crtc];
>  	u32 cur_vblank;
>  
>  	/* Read timestamp from slot of _vblank_time ringbuffer
> @@ -824,10 +835,10 @@ u32 drm_vblank_count_and_time(struct drm_device *dev, int crtc,
>  	 * a seqlock.
>  	 */
>  	do {
> -		cur_vblank = atomic_read(&dev->vblank[crtc].count);
> +		cur_vblank = atomic_read(&vblank->count);
>  		*vblanktime = vblanktimestamp(dev, crtc, cur_vblank);
>  		smp_rmb();
> -	} while (cur_vblank != atomic_read(&dev->vblank[crtc].count));
> +	} while (cur_vblank != atomic_read(&vblank->count));
>  
>  	return cur_vblank;
>  }
> @@ -882,13 +893,14 @@ EXPORT_SYMBOL(drm_send_vblank_event);
>   */
>  static int drm_vblank_enable(struct drm_device *dev, int crtc)
>  {
> +	struct drm_vblank_crtc *vblank = &dev->vblank[crtc];
>  	int ret = 0;
>  
>  	assert_spin_locked(&dev->vbl_lock);
>  
>  	spin_lock(&dev->vblank_time_lock);
>  
> -	if (!dev->vblank[crtc].enabled) {
> +	if (!vblank->enabled) {
>  		/*
>  		 * Enable vblank irqs under vblank_time_lock protection.
>  		 * All vblank count & timestamp updates are held off
> @@ -899,9 +911,9 @@ static int drm_vblank_enable(struct drm_device *dev, int crtc)
>  		ret = dev->driver->enable_vblank(dev, crtc);
>  		DRM_DEBUG("enabling vblank on crtc %d, ret: %d\n", crtc, ret);
>  		if (ret)
> -			atomic_dec(&dev->vblank[crtc].refcount);
> +			atomic_dec(&vblank->refcount);
>  		else {
> -			dev->vblank[crtc].enabled = true;
> +			vblank->enabled = true;
>  			drm_update_vblank_count(dev, crtc);
>  		}
>  	}
> @@ -926,16 +938,17 @@ static int drm_vblank_enable(struct drm_device *dev, int crtc)
>   */
>  int drm_vblank_get(struct drm_device *dev, int crtc)
>  {
> +	struct drm_vblank_crtc *vblank = &dev->vblank[crtc];
>  	unsigned long irqflags;
>  	int ret = 0;
>  
>  	spin_lock_irqsave(&dev->vbl_lock, irqflags);
>  	/* Going from 0->1 means we have to enable interrupts again */
> -	if (atomic_add_return(1, &dev->vblank[crtc].refcount) == 1) {
> +	if (atomic_add_return(1, &vblank->refcount) == 1) {
>  		ret = drm_vblank_enable(dev, crtc);
>  	} else {
> -		if (!dev->vblank[crtc].enabled) {
> -			atomic_dec(&dev->vblank[crtc].refcount);
> +		if (!vblank->enabled) {
> +			atomic_dec(&vblank->refcount);
>  			ret = -EINVAL;
>  		}
>  	}
> @@ -975,15 +988,17 @@ EXPORT_SYMBOL(drm_crtc_vblank_get);
>   */
>  void drm_vblank_put(struct drm_device *dev, int crtc)
>  {
> -	BUG_ON(atomic_read(&dev->vblank[crtc].refcount) == 0);
> +	struct drm_vblank_crtc *vblank = &dev->vblank[crtc];
> +
> +	BUG_ON(atomic_read(&vblank->refcount) == 0);
>  
>  	/* Last user schedules interrupt disable */
> -	if (atomic_dec_and_test(&dev->vblank[crtc].refcount)) {
> +	if (atomic_dec_and_test(&vblank->refcount)) {
>  		if (drm_vblank_offdelay > 0)
> -			mod_timer(&dev->vblank[crtc].disable_timer,
> +			mod_timer(&vblank->disable_timer,
>  				  jiffies + ((drm_vblank_offdelay * HZ)/1000));
>  		else if (drm_vblank_offdelay == 0)
> -			vblank_disable_fn((unsigned long)&dev->vblank[crtc]);
> +			vblank_disable_fn((unsigned long)vblank);
>  	}
>  }
>  EXPORT_SYMBOL(drm_vblank_put);
> @@ -1019,6 +1034,7 @@ EXPORT_SYMBOL(drm_crtc_vblank_put);
>   */
>  void drm_vblank_off(struct drm_device *dev, int crtc)
>  {
> +	struct drm_vblank_crtc *vblank = &dev->vblank[crtc];
>  	struct drm_pending_vblank_event *e, *t;
>  	struct timeval now;
>  	unsigned long irqflags;
> @@ -1026,7 +1042,7 @@ void drm_vblank_off(struct drm_device *dev, int crtc)
>  
>  	spin_lock_irqsave(&dev->vbl_lock, irqflags);
>  	vblank_disable_and_save(dev, crtc);
> -	wake_up(&dev->vblank[crtc].queue);
> +	wake_up(&vblank->queue);
>  
>  	/* Send any queued vblank events, lest the natives grow disquiet */
>  	seq = drm_vblank_count_and_time(dev, crtc, &now);
> @@ -1048,9 +1064,9 @@ void drm_vblank_off(struct drm_device *dev, int crtc)
>  	 * Prevent subsequent drm_vblank_get() from re-enabling
>  	 * the vblank interrupt by bumping the refcount.
>  	 */
> -	if (!dev->vblank[crtc].inmodeset) {
> -		atomic_inc(&dev->vblank[crtc].refcount);
> -		dev->vblank[crtc].inmodeset = 1;
> +	if (!vblank->inmodeset) {
> +		atomic_inc(&vblank->refcount);
> +		vblank->inmodeset = 1;
>  	}
>  
>  	spin_unlock_irqrestore(&dev->vbl_lock, irqflags);
> @@ -1090,13 +1106,14 @@ EXPORT_SYMBOL(drm_crtc_vblank_off);
>   */
>  void drm_vblank_on(struct drm_device *dev, int crtc)
>  {
> +	struct drm_vblank_crtc *vblank = &dev->vblank[crtc];
>  	unsigned long irqflags;
>  
>  	spin_lock_irqsave(&dev->vbl_lock, irqflags);
>  	/* Drop our private "prevent drm_vblank_get" refcount */
> -	if (dev->vblank[crtc].inmodeset) {
> -		atomic_dec(&dev->vblank[crtc].refcount);
> -		dev->vblank[crtc].inmodeset = 0;
> +	if (vblank->inmodeset) {
> +		atomic_dec(&vblank->refcount);
> +		vblank->inmodeset = 0;
>  	}
>  
>  	/*
> @@ -1106,12 +1123,12 @@ void drm_vblank_on(struct drm_device *dev, int crtc)
>  	 * -1 to make sure user will never see the same
>  	 * vblank counter value before and after a modeset
>  	 */
> -	dev->vblank[crtc].last =
> +	vblank->last =
>  		(dev->driver->get_vblank_counter(dev, crtc) - 1) &
>  		dev->max_vblank_count;
>  
>  	/* re-enable interrupts if there's are users left */
> -	if (atomic_read(&dev->vblank[crtc].refcount) != 0)
> +	if (atomic_read(&vblank->refcount) != 0)
>  		WARN_ON(drm_vblank_enable(dev, crtc));
>  	spin_unlock_irqrestore(&dev->vbl_lock, irqflags);
>  }
> @@ -1159,6 +1176,8 @@ EXPORT_SYMBOL(drm_crtc_vblank_on);
>   */
>  void drm_vblank_pre_modeset(struct drm_device *dev, int crtc)
>  {
> +	struct drm_vblank_crtc *vblank = &dev->vblank[crtc];
> +
>  	/* vblank is not initialized (IRQ not installed ?), or has been freed */
>  	if (!dev->num_crtcs)
>  		return;
> @@ -1169,10 +1188,10 @@ void drm_vblank_pre_modeset(struct drm_device *dev, int crtc)
>  	 * to avoid corrupting the count if multiple, mismatch calls occur),
>  	 * so that interrupts remain enabled in the interim.
>  	 */
> -	if (!dev->vblank[crtc].inmodeset) {
> -		dev->vblank[crtc].inmodeset = 0x1;
> +	if (!vblank->inmodeset) {
> +		vblank->inmodeset = 0x1;
>  		if (drm_vblank_get(dev, crtc) == 0)
> -			dev->vblank[crtc].inmodeset |= 0x2;
> +			vblank->inmodeset |= 0x2;
>  	}
>  }
>  EXPORT_SYMBOL(drm_vblank_pre_modeset);
> @@ -1187,21 +1206,22 @@ EXPORT_SYMBOL(drm_vblank_pre_modeset);
>   */
>  void drm_vblank_post_modeset(struct drm_device *dev, int crtc)
>  {
> +	struct drm_vblank_crtc *vblank = &dev->vblank[crtc];
>  	unsigned long irqflags;
>  
>  	/* vblank is not initialized (IRQ not installed ?), or has been freed */
>  	if (!dev->num_crtcs)
>  		return;
>  
> -	if (dev->vblank[crtc].inmodeset) {
> +	if (vblank->inmodeset) {
>  		spin_lock_irqsave(&dev->vbl_lock, irqflags);
>  		dev->vblank_disable_allowed = true;
>  		spin_unlock_irqrestore(&dev->vbl_lock, irqflags);
>  
> -		if (dev->vblank[crtc].inmodeset & 0x2)
> +		if (vblank->inmodeset & 0x2)
>  			drm_vblank_put(dev, crtc);
>  
> -		dev->vblank[crtc].inmodeset = 0;
> +		vblank->inmodeset = 0;
>  	}
>  }
>  EXPORT_SYMBOL(drm_vblank_post_modeset);
> @@ -1336,6 +1356,7 @@ err_put:
>  int drm_wait_vblank(struct drm_device *dev, void *data,
>  		    struct drm_file *file_priv)
>  {
> +	struct drm_vblank_crtc *vblank;
>  	union drm_wait_vblank *vblwait = data;
>  	int ret;
>  	unsigned int flags, seq, crtc, high_crtc;
> @@ -1365,6 +1386,8 @@ int drm_wait_vblank(struct drm_device *dev, void *data,
>  	if (crtc >= dev->num_crtcs)
>  		return -EINVAL;
>  
> +	vblank = &dev->vblank[crtc];
> +
>  	ret = drm_vblank_get(dev, crtc);
>  	if (ret) {
>  		DRM_DEBUG("failed to acquire vblank counter, %d\n", ret);
> @@ -1397,11 +1420,11 @@ int drm_wait_vblank(struct drm_device *dev, void *data,
>  
>  	DRM_DEBUG("waiting on vblank count %d, crtc %d\n",
>  		  vblwait->request.sequence, crtc);
> -	dev->vblank[crtc].last_wait = vblwait->request.sequence;
> -	DRM_WAIT_ON(ret, dev->vblank[crtc].queue, 3 * HZ,
> +	vblank->last_wait = vblwait->request.sequence;
> +	DRM_WAIT_ON(ret, vblank->queue, 3 * HZ,
>  		    (((drm_vblank_count(dev, crtc) -
>  		       vblwait->request.sequence) <= (1 << 23)) ||
> -		     !dev->vblank[crtc].enabled ||
> +		     !vblank->enabled ||
>  		     !dev->irq_enabled));
>  
>  	if (ret != -EINTR) {
> @@ -1462,6 +1485,7 @@ static void drm_handle_vblank_events(struct drm_device *dev, int crtc)
>   */
>  bool drm_handle_vblank(struct drm_device *dev, int crtc)
>  {
> +	struct drm_vblank_crtc *vblank = &dev->vblank[crtc];
>  	u32 vblcount;
>  	s64 diff_ns;
>  	struct timeval tvblank;
> @@ -1477,7 +1501,7 @@ bool drm_handle_vblank(struct drm_device *dev, int crtc)
>  	spin_lock_irqsave(&dev->vblank_time_lock, irqflags);
>  
>  	/* Vblank irq handling disabled. Nothing to do. */
> -	if (!dev->vblank[crtc].enabled) {
> +	if (!vblank->enabled) {
>  		spin_unlock_irqrestore(&dev->vblank_time_lock, irqflags);
>  		return false;
>  	}
> @@ -1487,7 +1511,7 @@ bool drm_handle_vblank(struct drm_device *dev, int crtc)
>  	 */
>  
>  	/* Get current timestamp and count. */
> -	vblcount = atomic_read(&dev->vblank[crtc].count);
> +	vblcount = atomic_read(&vblank->count);
>  	drm_get_last_vbltimestamp(dev, crtc, &tvblank, DRM_CALLED_FROM_VBLIRQ);
>  
>  	/* Compute time difference to timestamp of last vblank */
> @@ -1511,14 +1535,14 @@ bool drm_handle_vblank(struct drm_device *dev, int crtc)
>  		 * the timestamp computed above.
>  		 */
>  		smp_mb__before_atomic_inc();
> -		atomic_inc(&dev->vblank[crtc].count);
> +		atomic_inc(&vblank->count);
>  		smp_mb__after_atomic_inc();
>  	} else {
>  		DRM_DEBUG("crtc %d: Redundant vblirq ignored. diff_ns = %d\n",
>  			  crtc, (int) diff_ns);
>  	}
>  
> -	wake_up(&dev->vblank[crtc].queue);
> +	wake_up(&vblank->queue);
>  	drm_handle_vblank_events(dev, crtc);
>  
>  	spin_unlock_irqrestore(&dev->vblank_time_lock, irqflags);
> -- 
> 1.8.5.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[PATCH v2 9/9] drm/i915: Leave interrupts enabled while disabling crtcs during suspend

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

The new watermaek update mechanism requires interrupts to work
correctly. Because of this we need interrupts while disabling crtcs
during suspend. So move the irq disable to happen a bit later.

This also avoid clobbering the vblank.last count in case the
vblank interrupt was already disabled earlier by the timer.
In that case drm_vblank_off() will need .last to be correct so
that it can update the user visible vblank counter value
approapriately.

v2: Note vblank counter in commit message

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 4619c9e..21554a0 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -524,7 +524,6 @@ static int i915_drm_freeze(struct drm_device *dev)
 			return error;
 		}
 
-		drm_irq_uninstall(dev);
 		dev_priv->enable_hotplug_processing = false;
 
 		intel_disable_gt_powersave(dev);
@@ -541,6 +540,8 @@ static int i915_drm_freeze(struct drm_device *dev)
 		}
 		mutex_unlock(&dev->mode_config.mutex);
 
+		drm_irq_uninstall(dev);
+
 		intel_modeset_suspend_hw(dev);
 	}
 
-- 
1.8.5.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 9/9] drm/i915: Leave interrupts enabled while disabling crtcs during suspend

[Daniel Vetter]

On Mon, May 26, 2014 at 02:46:32PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> The new watermaek update mechanism requires interrupts to work
> correctly. Because of this we need interrupts while disabling crtcs
> during suspend. So move the irq disable to happen a bit later.
> 
> This also avoid clobbering the vblank.last count in case the
> vblank interrupt was already disabled earlier by the timer.
> In that case drm_vblank_off() will need .last to be correct so
> that it can update the user visible vblank counter value
> approapriately.
> 
> v2: Note vblank counter in commit message
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

First round of this patch had a question from me whether we have
sufficient amounts of WARN_ON(! or !!dev->irqs_enabled) sprinkled all over
the place ... Do we have that?

I'm specifically thinking of places where we cancel some work/time
lauchned by interrupts and must have the guarantee in place that it can't
rearm.

Otoh we have places where we absolutely want interrupts to still work,
e.g. in gpu_idle.

Sprinkling some modeset amounts of WARNS in a follow-up would make me
really happy.
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_drv.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 4619c9e..21554a0 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -524,7 +524,6 @@ static int i915_drm_freeze(struct drm_device *dev)
>  			return error;
>  		}
>  
> -		drm_irq_uninstall(dev);
>  		dev_priv->enable_hotplug_processing = false;
>  
>  		intel_disable_gt_powersave(dev);
> @@ -541,6 +540,8 @@ static int i915_drm_freeze(struct drm_device *dev)
>  		}
>  		mutex_unlock(&dev->mode_config.mutex);
>  
> +		drm_irq_uninstall(dev);
> +
>  		intel_modeset_suspend_hw(dev);
>  	}
>  
> -- 
> 1.8.5.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH v2 9/9] drm/i915: Leave interrupts enabled while disabling crtcs during suspend

[Matt Roper]

On Mon, May 26, 2014 at 02:46:32PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> The new watermaek update mechanism requires interrupts to work

s/watermaek/watermark/

> correctly. Because of this we need interrupts while disabling crtcs
> during suspend. So move the irq disable to happen a bit later.

I'm not super familiar with this code, so I might be missing something,
but won't this cause us to call drm_irq_uninstall() and then potentially
dev->driver->get_vblank_counter() while the pipe is disabled (which will
result in get_vblank_counter() returning 0)?

Also, does it cause any problems if if our interrupt handler races with
intel_disable_pipe()?


Matt

> 
> This also avoid clobbering the vblank.last count in case the
> vblank interrupt was already disabled earlier by the timer.
> In that case drm_vblank_off() will need .last to be correct so
> that it can update the user visible vblank counter value
> approapriately.
> 
> v2: Note vblank counter in commit message
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 4619c9e..21554a0 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -524,7 +524,6 @@ static int i915_drm_freeze(struct drm_device *dev)
>  			return error;
>  		}
>  
> -		drm_irq_uninstall(dev);
>  		dev_priv->enable_hotplug_processing = false;
>  
>  		intel_disable_gt_powersave(dev);
> @@ -541,6 +540,8 @@ static int i915_drm_freeze(struct drm_device *dev)
>  		}
>  		mutex_unlock(&dev->mode_config.mutex);
>  
> +		drm_irq_uninstall(dev);
> +
>  		intel_modeset_suspend_hw(dev);
>  	}
>  
> -- 
> 1.8.5.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795

[PATCH 12/9] drm: Fix deadlock between event_lock and vbl_lock/vblank_time_lock

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Currently both drm_irq.c and several drivers call drm_vblank_put()
while holding event_lock. Now that drm_vblank_put() can disable the
vblank interrupt directly it may need to grab vbl_lock and
vblank_time_lock. That causes deadlocks since we take the locks
in the opposite order in two places in drm_irq.c. So let's make
sure the locking order is always event_lock->vbl_lock->vblank_time_lock.

In drm_vblank_off() pull up event_lock from underneath vbl_lock. Hold
the event_lock across the whole operation to make sure we only send
out the events that were on the queue when we disabled the interrupt,
and not ones that got added just after (assuming drm_vblank_on() already
managed to get called somewhere between).

To sort the other deadlock pull the event_lock out from
drm_handle_vblank_events() into drm_handle_vblank() to be taken outside
vblank_time_lock. Add the appropriate assert_spin_locked() to
drm_handle_vblank_events().

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_irq.c | 47 +++++++++++++++++++++++++----------------------
 1 file changed, 25 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
index b008803..2b97059 100644
--- a/drivers/gpu/drm/drm_irq.c
+++ b/drivers/gpu/drm/drm_irq.c
@@ -1040,14 +1040,25 @@ void drm_vblank_off(struct drm_device *dev, int crtc)
 	unsigned long irqflags;
 	unsigned int seq;
 
-	spin_lock_irqsave(&dev->vbl_lock, irqflags);
+	spin_lock_irqsave(&dev->event_lock, irqflags);
+
+	spin_lock(&dev->vbl_lock);
 	vblank_disable_and_save(dev, crtc);
 	wake_up(&vblank->queue);
 
+	/*
+	 * Prevent subsequent drm_vblank_get() from re-enabling
+	 * the vblank interrupt by bumping the refcount.
+	 */
+	if (!vblank->inmodeset) {
+		atomic_inc(&vblank->refcount);
+		vblank->inmodeset = 1;
+	}
+	spin_unlock(&dev->vbl_lock);
+
 	/* Send any queued vblank events, lest the natives grow disquiet */
 	seq = drm_vblank_count_and_time(dev, crtc, &now);
 
-	spin_lock(&dev->event_lock);
 	list_for_each_entry_safe(e, t, &dev->vblank_event_list, base.link) {
 		if (e->pipe != crtc)
 			continue;
@@ -1058,18 +1069,7 @@ void drm_vblank_off(struct drm_device *dev, int crtc)
 		drm_vblank_put(dev, e->pipe);
 		send_vblank_event(dev, e, seq, &now);
 	}
-	spin_unlock(&dev->event_lock);
-
-	/*
-	 * Prevent subsequent drm_vblank_get() from re-enabling
-	 * the vblank interrupt by bumping the refcount.
-	 */
-	if (!vblank->inmodeset) {
-		atomic_inc(&vblank->refcount);
-		vblank->inmodeset = 1;
-	}
-
-	spin_unlock_irqrestore(&dev->vbl_lock, irqflags);
+	spin_unlock_irqrestore(&dev->event_lock, irqflags);
 }
 EXPORT_SYMBOL(drm_vblank_off);
 
@@ -1449,12 +1449,11 @@ static void drm_handle_vblank_events(struct drm_device *dev, int crtc)
 {
 	struct drm_pending_vblank_event *e, *t;
 	struct timeval now;
-	unsigned long flags;
 	unsigned int seq;
 
-	seq = drm_vblank_count_and_time(dev, crtc, &now);
+	assert_spin_locked(&dev->event_lock);
 
-	spin_lock_irqsave(&dev->event_lock, flags);
+	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)
@@ -1470,8 +1469,6 @@ static void drm_handle_vblank_events(struct drm_device *dev, int crtc)
 		send_vblank_event(dev, e, seq, &now);
 	}
 
-	spin_unlock_irqrestore(&dev->event_lock, flags);
-
 	trace_drm_vblank_event(crtc, seq);
 }
 
@@ -1494,15 +1491,18 @@ bool drm_handle_vblank(struct drm_device *dev, int crtc)
 	if (!dev->num_crtcs)
 		return false;
 
+	spin_lock_irqsave(&dev->event_lock, irqflags);
+
 	/* Need timestamp lock to prevent concurrent execution with
 	 * vblank enable/disable, as this would cause inconsistent
 	 * or corrupted timestamps and vblank counts.
 	 */
-	spin_lock_irqsave(&dev->vblank_time_lock, irqflags);
+	spin_lock(&dev->vblank_time_lock);
 
 	/* Vblank irq handling disabled. Nothing to do. */
 	if (!vblank->enabled) {
-		spin_unlock_irqrestore(&dev->vblank_time_lock, irqflags);
+		spin_unlock(&dev->vblank_time_lock);
+		spin_unlock_irqrestore(&dev->event_lock, irqflags);
 		return false;
 	}
 
@@ -1542,10 +1542,13 @@ bool drm_handle_vblank(struct drm_device *dev, int crtc)
 			  crtc, (int) diff_ns);
 	}
 
+	spin_unlock(&dev->vblank_time_lock);
+
 	wake_up(&vblank->queue);
 	drm_handle_vblank_events(dev, crtc);
 
-	spin_unlock_irqrestore(&dev->vblank_time_lock, irqflags);
+	spin_unlock_irqrestore(&dev->event_lock, irqflags);
+
 	return true;
 }
 EXPORT_SYMBOL(drm_handle_vblank);
-- 
1.8.5.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 13/9] drm: Fix race between drm_vblank_off() and drm_queue_vblank_event()

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Currently it's possible that the following will happen:
1. drm_wait_vblank() calls drm_vblank_get()
2. drm_vblank_off() gets called
3. drm_wait_vblank() calls drm_queue_vblank_event() which
   adds the event to the queue event though vblank interrupts
   are currently disabled (and may not be re-enabled ever again).

To fix the problem, add another vblank->enabled check into
drm_queue_vblank_event().

drm_vblank_off() holds event_lock around the vblank disable,
so no further locking needs to be added to drm_queue_vblank_event().
vblank disable from another source is not possible since
drm_wait_vblank() already holds a vblank reference.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_irq.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
index 2b97059..82a039a 100644
--- a/drivers/gpu/drm/drm_irq.c
+++ b/drivers/gpu/drm/drm_irq.c
@@ -1273,6 +1273,7 @@ static int drm_queue_vblank_event(struct drm_device *dev, int pipe,
 				  union drm_wait_vblank *vblwait,
 				  struct drm_file *file_priv)
 {
+	struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
 	struct drm_pending_vblank_event *e;
 	struct timeval now;
 	unsigned long flags;
@@ -1296,6 +1297,18 @@ static int drm_queue_vblank_event(struct drm_device *dev, int pipe,
 
 	spin_lock_irqsave(&dev->event_lock, flags);
 
+	/*
+	 * drm_vblank_off() might have been called after we called
+	 * drm_vblank_get(). drm_vblank_off() holds event_lock
+	 * around the vblank disable, so no need for further locking.
+	 * The reference from drm_vblank_get() protects against
+	 * vblank disable from another source.
+	 */
+	if (!vblank->enabled) {
+		ret = -EINVAL;
+		goto err_unlock;
+	}
+
 	if (file_priv->event_space < sizeof e->event) {
 		ret = -EBUSY;
 		goto err_unlock;
-- 
1.8.5.5

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

[PATCH 14/9] drm: Kick start vblank interrupts at drm_vblank_on()

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

If the user is interested in getting accurate vblank sequence
numbers all the time they may disable the vblank disable timer
entirely. In that case it seems appropriate to kick start the
vblank interrupts already from drm_vblank_on().

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/drm_irq.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
index 82a039a..6376d96 100644
--- a/drivers/gpu/drm/drm_irq.c
+++ b/drivers/gpu/drm/drm_irq.c
@@ -1126,9 +1126,12 @@ void drm_vblank_on(struct drm_device *dev, int crtc)
 	vblank->last =
 		(dev->driver->get_vblank_counter(dev, crtc) - 1) &
 		dev->max_vblank_count;
-
-	/* re-enable interrupts if there's are users left */
-	if (atomic_read(&vblank->refcount) != 0)
+	/*
+	 * re-enable interrupts if there are users left, or the
+	 * user wishes vblank interrupts to be enabled all the time.
+	 */
+	if (atomic_read(&vblank->refcount) != 0 ||
+	    (!dev->vblank_disable_immediate && drm_vblank_offdelay < 0))
 		WARN_ON(drm_vblank_enable(dev, crtc));
 	spin_unlock_irqrestore(&dev->vbl_lock, irqflags);
 }
-- 
1.8.5.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 14/9] drm: Kick start vblank interrupts at drm_vblank_on()

[Matt Roper]

On Mon, Jun 02, 2014 at 11:15:51AM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> If the user is interested in getting accurate vblank sequence
> numbers all the time they may disable the vblank disable timer
> entirely. In that case it seems appropriate to kick start the
> vblank interrupts already from drm_vblank_on().
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_irq.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> index 82a039a..6376d96 100644
> --- a/drivers/gpu/drm/drm_irq.c
> +++ b/drivers/gpu/drm/drm_irq.c
> @@ -1126,9 +1126,12 @@ void drm_vblank_on(struct drm_device *dev, int crtc)
>  	vblank->last =
>  		(dev->driver->get_vblank_counter(dev, crtc) - 1) &
>  		dev->max_vblank_count;
> -
> -	/* re-enable interrupts if there's are users left */
> -	if (atomic_read(&vblank->refcount) != 0)
> +	/*
> +	 * re-enable interrupts if there are users left, or the
> +	 * user wishes vblank interrupts to be enabled all the time.
> +	 */
> +	if (atomic_read(&vblank->refcount) != 0 ||
> +	    (!dev->vblank_disable_immediate && drm_vblank_offdelay < 0))

As noted on patch 10, wouldn't it make sense for the user-provided
module parameter override the driver ability to disable immediately in
this case where they've specifically asked for "never disable?"

Otherwise, patches 12-14 are 

Reviewed-by: Matt Roper <matthew.d.roper@intel.com>

>  		WARN_ON(drm_vblank_enable(dev, crtc));
>  	spin_unlock_irqrestore(&dev->vbl_lock, irqflags);
>  }
> -- 
> 1.8.5.5
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795

Re: [PATCH 0/9] drm: More vblank on/off work

[Jesse Barnes]

On Mon, 26 May 2014 14:46:23 +0300
ville.syrjala@linux.intel.com wrote:

> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Another vblank series with the following features:
> - Plug a race between drm_vblank_off() and marking the crtc inactive
> - Don't send zeroed vblank evens to userspace at drm_vblank_off()
> - Have the user visible vblank counter account the entire time
>   when the crtc was active, regardless of how long vblank interrupts
>   were enabled
> - Avoid random jumps in the user visible vblank counter if the hardware
>   counter gets reset
> - Allow disabling vblank interrupts immediately at drm_vblank_put()
> - Some polish via coccinelle
> 
> While setting drm_vblank_offdelay to 0 is now possible, I'm not sure
> if we should set it 0 automatically in the i915 driver. If there are
> multiple GPUs in the system that setting will affect them all, which
> might have bad consequences if the other GPU doesn't have a hardware
> frame counter, or if it's just buggy. So perhaps we should move that
> option to be per-driver?
> 
> Ville Syrjälä (9):
>   drm: Always reject drm_vblank_get() after drm_vblank_off()
>   drm/i915: Warn if drm_vblank_get() still works after drm_vblank_off()
>   drm: Don't clear vblank timestamps when vblank interrupt is disabled
>   drm: Move drm_update_vblank_count()
>   drm: Have the vblank counter account for the time between vblank irq
>     disable and drm_vblank_off()
>   drm: Avoid random vblank counter jumps if the hardware counter has
>     been reset
>   drm: Disable vblank interrupt immediately when drm_vblank_offdelay==0
>   drm: Reduce the amount of dev->vblank[crtc] in the code
>   drm/i915: Leave interrupts enabled while disabling crtcs during
>     suspend

Here's one that may be fixed by this series, needs testing though:
https://bugs.freedesktop.org/show_bug.cgi?id=79054

-- 
Jesse Barnes, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx