[PATCH 1/2] drm/i915: Merge duplicate gen4 and vlv/chv enable vblank callbacks

[PATCH 1/2] drm/i915: Merge duplicate gen4 and vlv/chv enable vblank callbacks

[Chris Wilson]

gen4/vlv/chv all use the same bits in pipestat to enable the vblank
interrupt, so they can share the same callbacks to enable/disable.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 59 +++++++++++++++++++----------------------
 1 file changed, 28 insertions(+), 31 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index bd6c8b0eeaef..1e43fe30da11 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -2718,45 +2718,40 @@ void i915_handle_error(struct drm_i915_private *dev_priv,
 /* Called from drm generic code, passed 'crtc' which
  * we use as a pipe index
  */
-static int i915_enable_vblank(struct drm_device *dev, unsigned int pipe)
+static int i8xx_enable_vblank(struct drm_device *dev, unsigned int pipe)
 {
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	unsigned long irqflags;
 
 	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
-	if (INTEL_INFO(dev)->gen >= 4)
-		i915_enable_pipestat(dev_priv, pipe,
-				     PIPE_START_VBLANK_INTERRUPT_STATUS);
-	else
-		i915_enable_pipestat(dev_priv, pipe,
-				     PIPE_VBLANK_INTERRUPT_STATUS);
+	i915_enable_pipestat(dev_priv, pipe, PIPE_VBLANK_INTERRUPT_STATUS);
 	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
 
 	return 0;
 }
 
-static int ironlake_enable_vblank(struct drm_device *dev, unsigned int pipe)
+static int i965_enable_vblank(struct drm_device *dev, unsigned int pipe)
 {
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	unsigned long irqflags;
-	uint32_t bit = (INTEL_INFO(dev)->gen >= 7) ? DE_PIPE_VBLANK_IVB(pipe) :
-						     DE_PIPE_VBLANK(pipe);
 
 	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
-	ilk_enable_display_irq(dev_priv, bit);
+	i915_enable_pipestat(dev_priv, pipe,
+			     PIPE_START_VBLANK_INTERRUPT_STATUS);
 	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
 
 	return 0;
 }
 
-static int valleyview_enable_vblank(struct drm_device *dev, unsigned int pipe)
+static int ironlake_enable_vblank(struct drm_device *dev, unsigned int pipe)
 {
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	unsigned long irqflags;
+	uint32_t bit = INTEL_GEN(dev) >= 7 ?
+		DE_PIPE_VBLANK_IVB(pipe) : DE_PIPE_VBLANK(pipe);
 
 	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
-	i915_enable_pipestat(dev_priv, pipe,
-			     PIPE_START_VBLANK_INTERRUPT_STATUS);
+	ilk_enable_display_irq(dev_priv, bit);
 	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
 
 	return 0;
@@ -2777,38 +2772,36 @@ static int gen8_enable_vblank(struct drm_device *dev, unsigned int pipe)
 /* Called from drm generic code, passed 'crtc' which
  * we use as a pipe index
  */
-static void i915_disable_vblank(struct drm_device *dev, unsigned int pipe)
+static void i8xx_disable_vblank(struct drm_device *dev, unsigned int pipe)
 {
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	unsigned long irqflags;
 
 	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
-	i915_disable_pipestat(dev_priv, pipe,
-			      PIPE_VBLANK_INTERRUPT_STATUS |
-			      PIPE_START_VBLANK_INTERRUPT_STATUS);
+	i915_disable_pipestat(dev_priv, pipe, PIPE_VBLANK_INTERRUPT_STATUS);
 	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
 }
 
-static void ironlake_disable_vblank(struct drm_device *dev, unsigned int pipe)
+static void i965_disable_vblank(struct drm_device *dev, unsigned int pipe)
 {
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	unsigned long irqflags;
-	uint32_t bit = (INTEL_INFO(dev)->gen >= 7) ? DE_PIPE_VBLANK_IVB(pipe) :
-						     DE_PIPE_VBLANK(pipe);
 
 	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
-	ilk_disable_display_irq(dev_priv, bit);
+	i915_disable_pipestat(dev_priv, pipe,
+			      PIPE_START_VBLANK_INTERRUPT_STATUS);
 	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
 }
 
-static void valleyview_disable_vblank(struct drm_device *dev, unsigned int pipe)
+static void ironlake_disable_vblank(struct drm_device *dev, unsigned int pipe)
 {
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	unsigned long irqflags;
+	uint32_t bit = INTEL_GEN(dev) >= 7 ?
+		DE_PIPE_VBLANK_IVB(pipe) : DE_PIPE_VBLANK(pipe);
 
 	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
-	i915_disable_pipestat(dev_priv, pipe,
-			      PIPE_START_VBLANK_INTERRUPT_STATUS);
+	ilk_disable_display_irq(dev_priv, bit);
 	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
 }
 
@@ -4579,16 +4572,16 @@ void intel_irq_init(struct drm_i915_private *dev_priv)
 		dev->driver->irq_preinstall = cherryview_irq_preinstall;
 		dev->driver->irq_postinstall = cherryview_irq_postinstall;
 		dev->driver->irq_uninstall = cherryview_irq_uninstall;
-		dev->driver->enable_vblank = valleyview_enable_vblank;
-		dev->driver->disable_vblank = valleyview_disable_vblank;
+		dev->driver->enable_vblank = i965_enable_vblank;
+		dev->driver->disable_vblank = i965_disable_vblank;
 		dev_priv->display.hpd_irq_setup = i915_hpd_irq_setup;
 	} else if (IS_VALLEYVIEW(dev_priv)) {
 		dev->driver->irq_handler = valleyview_irq_handler;
 		dev->driver->irq_preinstall = valleyview_irq_preinstall;
 		dev->driver->irq_postinstall = valleyview_irq_postinstall;
 		dev->driver->irq_uninstall = valleyview_irq_uninstall;
-		dev->driver->enable_vblank = valleyview_enable_vblank;
-		dev->driver->disable_vblank = valleyview_disable_vblank;
+		dev->driver->enable_vblank = i965_enable_vblank;
+		dev->driver->disable_vblank = i965_disable_vblank;
 		dev_priv->display.hpd_irq_setup = i915_hpd_irq_setup;
 	} else if (INTEL_INFO(dev_priv)->gen >= 8) {
 		dev->driver->irq_handler = gen8_irq_handler;
@@ -4617,21 +4610,25 @@ void intel_irq_init(struct drm_i915_private *dev_priv)
 			dev->driver->irq_postinstall = i8xx_irq_postinstall;
 			dev->driver->irq_handler = i8xx_irq_handler;
 			dev->driver->irq_uninstall = i8xx_irq_uninstall;
+			dev->driver->enable_vblank = i8xx_enable_vblank;
+			dev->driver->disable_vblank = i8xx_disable_vblank;
 		} else if (IS_GEN3(dev_priv)) {
 			dev->driver->irq_preinstall = i915_irq_preinstall;
 			dev->driver->irq_postinstall = i915_irq_postinstall;
 			dev->driver->irq_uninstall = i915_irq_uninstall;
 			dev->driver->irq_handler = i915_irq_handler;
+			dev->driver->enable_vblank = i8xx_enable_vblank;
+			dev->driver->disable_vblank = i8xx_disable_vblank;
 		} else {
 			dev->driver->irq_preinstall = i965_irq_preinstall;
 			dev->driver->irq_postinstall = i965_irq_postinstall;
 			dev->driver->irq_uninstall = i965_irq_uninstall;
 			dev->driver->irq_handler = i965_irq_handler;
+			dev->driver->enable_vblank = i965_enable_vblank;
+			dev->driver->disable_vblank = i965_disable_vblank;
 		}
 		if (I915_HAS_HOTPLUG(dev_priv))
 			dev_priv->display.hpd_irq_setup = i915_hpd_irq_setup;
-		dev->driver->enable_vblank = i915_enable_vblank;
-		dev->driver->disable_vblank = i915_disable_vblank;
 	}
 }
 
-- 
2.9.3

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

[PATCH 2/2] drm/i915: Assert we hold the CRTC powerwell for generating vblank interrupts

[Chris Wilson]

To enable the vblank itself, we need to have an RPM wakeref for the mmio
access, and whilst generating the vblank interrupts we continue to
require the rpm wakeref. The assumption is that the RPM wakeref is held
by the display powerwell held by the active pipe. As this chain was not
obvious to me chasing the drm_wait_vblank ioctl, document it with a WARN
during *_vblank_enable().

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 1e43fe30da11..4491573ea329 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -2715,6 +2715,15 @@ void i915_handle_error(struct drm_i915_private *dev_priv,
 	i915_reset_and_wakeup(dev_priv);
 }
 
+static void assert_pipe_is_active(struct drm_i915_private *dev_priv,
+				  enum pipe pipe)
+{
+	struct intel_crtc *crtc =
+		to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]);
+
+	I915_STATE_WARN_ON(!crtc->base.state->active);
+}
+
 /* Called from drm generic code, passed 'crtc' which
  * we use as a pipe index
  */
@@ -2723,6 +2732,9 @@ static int i8xx_enable_vblank(struct drm_device *dev, unsigned int pipe)
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	unsigned long irqflags;
 
+	/* vblank IRQ requires the powerwell, held awake by the CRTC */
+	assert_pipe_is_active(dev_priv, pipe);
+
 	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
 	i915_enable_pipestat(dev_priv, pipe, PIPE_VBLANK_INTERRUPT_STATUS);
 	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
@@ -2735,6 +2747,9 @@ static int i965_enable_vblank(struct drm_device *dev, unsigned int pipe)
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	unsigned long irqflags;
 
+	/* vblank IRQ requires the powerwell, held awake by the CRTC */
+	assert_pipe_is_active(dev_priv, pipe);
+
 	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
 	i915_enable_pipestat(dev_priv, pipe,
 			     PIPE_START_VBLANK_INTERRUPT_STATUS);
@@ -2750,6 +2765,9 @@ static int ironlake_enable_vblank(struct drm_device *dev, unsigned int pipe)
 	uint32_t bit = INTEL_GEN(dev) >= 7 ?
 		DE_PIPE_VBLANK_IVB(pipe) : DE_PIPE_VBLANK(pipe);
 
+	/* vblank IRQ requires the powerwell, held awake by the CRTC */
+	assert_pipe_is_active(dev_priv, pipe);
+
 	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
 	ilk_enable_display_irq(dev_priv, bit);
 	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
@@ -2762,6 +2780,9 @@ static int gen8_enable_vblank(struct drm_device *dev, unsigned int pipe)
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	unsigned long irqflags;
 
+	/* vblank IRQ requires the powerwell, held awake by the CRTC */
+	assert_pipe_is_active(dev_priv, pipe);
+
 	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
 	bdw_enable_pipe_irq(dev_priv, pipe, GEN8_PIPE_VBLANK);
 	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
-- 
2.9.3

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

Re: [PATCH 2/2] drm/i915: Assert we hold the CRTC powerwell for generating vblank interrupts

[Maarten Lankhorst]

Op 07-10-16 om 21:49 schreef Chris Wilson:
> To enable the vblank itself, we need to have an RPM wakeref for the mmio
> access, and whilst generating the vblank interrupts we continue to
> require the rpm wakeref. The assumption is that the RPM wakeref is held
> by the display powerwell held by the active pipe. As this chain was not
> obvious to me chasing the drm_wait_vblank ioctl, document it with a WARN
> during *_vblank_enable().
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Don't we prevent enabling the vblank irq through drm_crtc_vblank_on/off?

I'd rather not have things look at crtc->state if possible, locking might not help you.

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

Re: [PATCH 2/2] drm/i915: Assert we hold the CRTC powerwell for generating vblank interrupts

[Chris Wilson]

On Mon, Oct 10, 2016 at 11:35:07AM +0200, Maarten Lankhorst wrote:
> Op 07-10-16 om 21:49 schreef Chris Wilson:
> > To enable the vblank itself, we need to have an RPM wakeref for the mmio
> > access, and whilst generating the vblank interrupts we continue to
> > require the rpm wakeref. The assumption is that the RPM wakeref is held
> > by the display powerwell held by the active pipe. As this chain was not
> > obvious to me chasing the drm_wait_vblank ioctl, document it with a WARN
> > during *_vblank_enable().
> >
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Don't we prevent enabling the vblank irq through drm_crtc_vblank_on/off?

Should, but this is the boundary point from the midlayer, so
sanitychecks ahoy.
 
> I'd rather not have things look at crtc->state if possible, locking might not help you.

Ok, anything better?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915: Assert we hold the CRTC powerwell for generating vblank interrupts

[Maarten Lankhorst]

Op 10-10-16 om 11:57 schreef Chris Wilson:
> On Mon, Oct 10, 2016 at 11:35:07AM +0200, Maarten Lankhorst wrote:
>> Op 07-10-16 om 21:49 schreef Chris Wilson:
>>> To enable the vblank itself, we need to have an RPM wakeref for the mmio
>>> access, and whilst generating the vblank interrupts we continue to
>>> require the rpm wakeref. The assumption is that the RPM wakeref is held
>>> by the display powerwell held by the active pipe. As this chain was not
>>> obvious to me chasing the drm_wait_vblank ioctl, document it with a WARN
>>> during *_vblank_enable().
>>>
>>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> Don't we prevent enabling the vblank irq through drm_crtc_vblank_on/off?
> Should, but this is the boundary point from the midlayer, so
> sanitychecks ahoy.
>  
>> I'd rather not have things look at crtc->state if possible, locking might not help you.
> Ok, anything better?
> -Chris

I would say either intel_display_is_enabled(POWER_DOMAIN_PIPE(pipe)) or assert_rpm_wakelock_held.

~Maarten

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

Re: [PATCH 2/2] drm/i915: Assert we hold the CRTC powerwell for generating vblank interrupts

[Chris Wilson]

On Mon, Oct 10, 2016 at 12:38:13PM +0200, Maarten Lankhorst wrote:
> Op 10-10-16 om 11:57 schreef Chris Wilson:
> > On Mon, Oct 10, 2016 at 11:35:07AM +0200, Maarten Lankhorst wrote:
> >> Op 07-10-16 om 21:49 schreef Chris Wilson:
> >>> To enable the vblank itself, we need to have an RPM wakeref for the mmio
> >>> access, and whilst generating the vblank interrupts we continue to
> >>> require the rpm wakeref. The assumption is that the RPM wakeref is held
> >>> by the display powerwell held by the active pipe. As this chain was not
> >>> obvious to me chasing the drm_wait_vblank ioctl, document it with a WARN
> >>> during *_vblank_enable().
> >>>
> >>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> >>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >> Don't we prevent enabling the vblank irq through drm_crtc_vblank_on/off?
> > Should, but this is the boundary point from the midlayer, so
> > sanitychecks ahoy.
> >  
> >> I'd rather not have things look at crtc->state if possible, locking might not help you.
> > Ok, anything better?
> > -Chris
> 
> I would say either intel_display_is_enabled(POWER_DOMAIN_PIPE(pipe)) or assert_rpm_wakelock_held.

I was aiming for a higher level assert than assert rpm wakelock, so
let's try intel_display_is_enabled...
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH] drm/i915: Assert we hold the CRTC powerwell for generating vblank interrupts

[Chris Wilson]

To enable the vblank itself, we need to have an RPM wakeref for the mmio
access, and whilst generating the vblank interrupts we continue to
require the rpm wakeref. The assumption is that the RPM wakeref is held
by the display powerwell held by the active pipe. As this chain was not
obvious to me chasing the drm_wait_vblank ioctl, document it with a WARN
during *_vblank_enable().

v2: Check the display power well rather than digging inside the atomic
CRTC state.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 1e43fe30da11..f0f17055dbb9 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -2715,6 +2715,14 @@ void i915_handle_error(struct drm_i915_private *dev_priv,
 	i915_reset_and_wakeup(dev_priv);
 }
 
+static void assert_pipe_is_awake(struct drm_i915_private *dev_priv,
+				 enum pipe pipe)
+{
+	WARN_ON(IS_ENABLED(CONFIG_DRM_I915_DEBUG) &&
+		!intel_display_power_is_enabled(dev_priv,
+						POWER_DOMAIN_PIPE(pipe)));
+}
+
 /* Called from drm generic code, passed 'crtc' which
  * we use as a pipe index
  */
@@ -2723,6 +2731,9 @@ static int i8xx_enable_vblank(struct drm_device *dev, unsigned int pipe)
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	unsigned long irqflags;
 
+	/* vblank IRQ requires the powerwell, held awake by the CRTC */
+	assert_pipe_is_awake(dev_priv, pipe);
+
 	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
 	i915_enable_pipestat(dev_priv, pipe, PIPE_VBLANK_INTERRUPT_STATUS);
 	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
@@ -2735,6 +2746,9 @@ static int i965_enable_vblank(struct drm_device *dev, unsigned int pipe)
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	unsigned long irqflags;
 
+	/* vblank IRQ requires the powerwell, held awake by the CRTC */
+	assert_pipe_is_awake(dev_priv, pipe);
+
 	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
 	i915_enable_pipestat(dev_priv, pipe,
 			     PIPE_START_VBLANK_INTERRUPT_STATUS);
@@ -2750,6 +2764,9 @@ static int ironlake_enable_vblank(struct drm_device *dev, unsigned int pipe)
 	uint32_t bit = INTEL_GEN(dev) >= 7 ?
 		DE_PIPE_VBLANK_IVB(pipe) : DE_PIPE_VBLANK(pipe);
 
+	/* vblank IRQ requires the powerwell, held awake by the CRTC */
+	assert_pipe_is_awake(dev_priv, pipe);
+
 	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
 	ilk_enable_display_irq(dev_priv, bit);
 	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
@@ -2762,6 +2779,9 @@ static int gen8_enable_vblank(struct drm_device *dev, unsigned int pipe)
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	unsigned long irqflags;
 
+	/* vblank IRQ requires the powerwell, held awake by the CRTC */
+	assert_pipe_is_awake(dev_priv, pipe);
+
 	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
 	bdw_enable_pipe_irq(dev_priv, pipe, GEN8_PIPE_VBLANK);
 	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
-- 
2.9.3

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

[PATCH] drm/i915: Assert we hold the CRTC powerwell for generating vblank interrupts

[Chris Wilson]

To enable the vblank itself, we need to have an RPM wakeref for the mmio
access, and whilst generating the vblank interrupts we continue to
require the rpm wakeref. The assumption is that the RPM wakeref is held
by the display powerwell held by the active pipe. As this chain was not
obvious to me chasing the drm_wait_vblank ioctl, document it with a WARN
during *_vblank_enable().

v2: Check the display power well rather than digging inside the atomic
CRTC state.
v3: Keep the WARN_ON() tidy (avoid macro expansions that get recorded
in their completeness in the user string).

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 1e43fe30da11..8e2eb5adde33 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -2715,6 +2715,18 @@ void i915_handle_error(struct drm_i915_private *dev_priv,
 	i915_reset_and_wakeup(dev_priv);
 }
 
+static void assert_pipe_is_awake(struct drm_i915_private *dev_priv,
+				 enum pipe pipe)
+{
+	enum intel_display_power_domain pipe_domain;
+
+	if (!IS_ENABLED(CONFIG_DRM_I915_DEBUG))
+		return;
+
+	pipe_domain = POWER_DOMAIN_PIPE(pipe);
+	WARN_ON(!intel_display_power_is_enabled(dev_priv, pipe_domain));
+}
+
 /* Called from drm generic code, passed 'crtc' which
  * we use as a pipe index
  */
@@ -2723,6 +2735,9 @@ static int i8xx_enable_vblank(struct drm_device *dev, unsigned int pipe)
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	unsigned long irqflags;
 
+	/* vblank IRQ requires the powerwell, held awake by the CRTC */
+	assert_pipe_is_awake(dev_priv, pipe);
+
 	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
 	i915_enable_pipestat(dev_priv, pipe, PIPE_VBLANK_INTERRUPT_STATUS);
 	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
@@ -2735,6 +2750,9 @@ static int i965_enable_vblank(struct drm_device *dev, unsigned int pipe)
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	unsigned long irqflags;
 
+	/* vblank IRQ requires the powerwell, held awake by the CRTC */
+	assert_pipe_is_awake(dev_priv, pipe);
+
 	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
 	i915_enable_pipestat(dev_priv, pipe,
 			     PIPE_START_VBLANK_INTERRUPT_STATUS);
@@ -2750,6 +2768,9 @@ static int ironlake_enable_vblank(struct drm_device *dev, unsigned int pipe)
 	uint32_t bit = INTEL_GEN(dev) >= 7 ?
 		DE_PIPE_VBLANK_IVB(pipe) : DE_PIPE_VBLANK(pipe);
 
+	/* vblank IRQ requires the powerwell, held awake by the CRTC */
+	assert_pipe_is_awake(dev_priv, pipe);
+
 	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
 	ilk_enable_display_irq(dev_priv, bit);
 	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
@@ -2762,6 +2783,9 @@ static int gen8_enable_vblank(struct drm_device *dev, unsigned int pipe)
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	unsigned long irqflags;
 
+	/* vblank IRQ requires the powerwell, held awake by the CRTC */
+	assert_pipe_is_awake(dev_priv, pipe);
+
 	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
 	bdw_enable_pipe_irq(dev_priv, pipe, GEN8_PIPE_VBLANK);
 	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
-- 
2.9.3

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

Re: [PATCH] drm/i915: Assert we hold the CRTC powerwell for generating vblank interrupts

[Ville Syrjälä]

On Mon, Oct 10, 2016 at 12:34:54PM +0100, Chris Wilson wrote:
> To enable the vblank itself, we need to have an RPM wakeref for the mmio
> access, and whilst generating the vblank interrupts we continue to
> require the rpm wakeref. The assumption is that the RPM wakeref is held
> by the display powerwell held by the active pipe. As this chain was not
> obvious to me chasing the drm_wait_vblank ioctl, document it with a WARN
> during *_vblank_enable().
> 
> v2: Check the display power well rather than digging inside the atomic
> CRTC state.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_irq.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 1e43fe30da11..f0f17055dbb9 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -2715,6 +2715,14 @@ void i915_handle_error(struct drm_i915_private *dev_priv,
>  	i915_reset_and_wakeup(dev_priv);
>  }
>  
> +static void assert_pipe_is_awake(struct drm_i915_private *dev_priv,
> +				 enum pipe pipe)
> +{
> +	WARN_ON(IS_ENABLED(CONFIG_DRM_I915_DEBUG) &&
> +		!intel_display_power_is_enabled(dev_priv,
> +						POWER_DOMAIN_PIPE(pipe)));

Uses a mutex. And having a power well enabled doesn't mean much. It
doesn't guarantee that vblanks work.

> +}
> +
>  /* Called from drm generic code, passed 'crtc' which
>   * we use as a pipe index
>   */
> @@ -2723,6 +2731,9 @@ static int i8xx_enable_vblank(struct drm_device *dev, unsigned int pipe)
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  	unsigned long irqflags;
>  
> +	/* vblank IRQ requires the powerwell, held awake by the CRTC */
> +	assert_pipe_is_awake(dev_priv, pipe);
> +
>  	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
>  	i915_enable_pipestat(dev_priv, pipe, PIPE_VBLANK_INTERRUPT_STATUS);
>  	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
> @@ -2735,6 +2746,9 @@ static int i965_enable_vblank(struct drm_device *dev, unsigned int pipe)
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  	unsigned long irqflags;
>  
> +	/* vblank IRQ requires the powerwell, held awake by the CRTC */
> +	assert_pipe_is_awake(dev_priv, pipe);
> +
>  	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
>  	i915_enable_pipestat(dev_priv, pipe,
>  			     PIPE_START_VBLANK_INTERRUPT_STATUS);
> @@ -2750,6 +2764,9 @@ static int ironlake_enable_vblank(struct drm_device *dev, unsigned int pipe)
>  	uint32_t bit = INTEL_GEN(dev) >= 7 ?
>  		DE_PIPE_VBLANK_IVB(pipe) : DE_PIPE_VBLANK(pipe);
>  
> +	/* vblank IRQ requires the powerwell, held awake by the CRTC */
> +	assert_pipe_is_awake(dev_priv, pipe);
> +
>  	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
>  	ilk_enable_display_irq(dev_priv, bit);
>  	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
> @@ -2762,6 +2779,9 @@ static int gen8_enable_vblank(struct drm_device *dev, unsigned int pipe)
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  	unsigned long irqflags;
>  
> +	/* vblank IRQ requires the powerwell, held awake by the CRTC */
> +	assert_pipe_is_awake(dev_priv, pipe);
> +
>  	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
>  	bdw_enable_pipe_irq(dev_priv, pipe, GEN8_PIPE_VBLANK);
>  	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
> -- 
> 2.9.3

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Assert we hold the CRTC powerwell for generating vblank interrupts

[Chris Wilson]

On Mon, Oct 10, 2016 at 02:42:01PM +0300, Ville Syrjälä wrote:
> On Mon, Oct 10, 2016 at 12:34:54PM +0100, Chris Wilson wrote:
> > To enable the vblank itself, we need to have an RPM wakeref for the mmio
> > access, and whilst generating the vblank interrupts we continue to
> > require the rpm wakeref. The assumption is that the RPM wakeref is held
> > by the display powerwell held by the active pipe. As this chain was not
> > obvious to me chasing the drm_wait_vblank ioctl, document it with a WARN
> > during *_vblank_enable().
> > 
> > v2: Check the display power well rather than digging inside the atomic
> > CRTC state.
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_irq.c | 20 ++++++++++++++++++++
> >  1 file changed, 20 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > index 1e43fe30da11..f0f17055dbb9 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -2715,6 +2715,14 @@ void i915_handle_error(struct drm_i915_private *dev_priv,
> >  	i915_reset_and_wakeup(dev_priv);
> >  }
> >  
> > +static void assert_pipe_is_awake(struct drm_i915_private *dev_priv,
> > +				 enum pipe pipe)
> > +{
> > +	WARN_ON(IS_ENABLED(CONFIG_DRM_I915_DEBUG) &&
> > +		!intel_display_power_is_enabled(dev_priv,
> > +						POWER_DOMAIN_PIPE(pipe)));
> 
> Uses a mutex. And having a power well enabled doesn't mean much. It
> doesn't guarantee that vblanks work.

Impasse. :|

There should be no point in an explicit assert_rpm_wakeref here as the
register access should catch an error there. Is there no safe way we can
assert the current state of the CRTC is correct for enabling vblanks?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Assert we hold the CRTC powerwell for generating vblank interrupts

[Ville Syrjälä]

On Mon, Oct 10, 2016 at 12:46:32PM +0100, Chris Wilson wrote:
> On Mon, Oct 10, 2016 at 02:42:01PM +0300, Ville Syrjälä wrote:
> > On Mon, Oct 10, 2016 at 12:34:54PM +0100, Chris Wilson wrote:
> > > To enable the vblank itself, we need to have an RPM wakeref for the mmio
> > > access, and whilst generating the vblank interrupts we continue to
> > > require the rpm wakeref. The assumption is that the RPM wakeref is held
> > > by the display powerwell held by the active pipe. As this chain was not
> > > obvious to me chasing the drm_wait_vblank ioctl, document it with a WARN
> > > during *_vblank_enable().
> > > 
> > > v2: Check the display power well rather than digging inside the atomic
> > > CRTC state.
> > > 
> > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_irq.c | 20 ++++++++++++++++++++
> > >  1 file changed, 20 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > > index 1e43fe30da11..f0f17055dbb9 100644
> > > --- a/drivers/gpu/drm/i915/i915_irq.c
> > > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > > @@ -2715,6 +2715,14 @@ void i915_handle_error(struct drm_i915_private *dev_priv,
> > >  	i915_reset_and_wakeup(dev_priv);
> > >  }
> > >  
> > > +static void assert_pipe_is_awake(struct drm_i915_private *dev_priv,
> > > +				 enum pipe pipe)
> > > +{
> > > +	WARN_ON(IS_ENABLED(CONFIG_DRM_I915_DEBUG) &&
> > > +		!intel_display_power_is_enabled(dev_priv,
> > > +						POWER_DOMAIN_PIPE(pipe)));
> > 
> > Uses a mutex. And having a power well enabled doesn't mean much. It
> > doesn't guarantee that vblanks work.
> 
> Impasse. :|
> 
> There should be no point in an explicit assert_rpm_wakeref here as the
> register access should catch an error there. Is there no safe way we can
> assert the current state of the CRTC is correct for enabling vblanks?

crtc->active might be the closest thing, if we just ignore any locking.
Though it looks like that has gone a bit mad these days, what with being
set from the .crtc_enable() hooks but getting cleared outside the
.crtc_disable() hooks.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: warning for series starting with [1/2] drm/i915: Merge duplicate gen4 and vlv/chv enable vblank callbacks (rev3)

[Patchwork]

== Series Details ==

Series: series starting with [1/2] drm/i915: Merge duplicate gen4 and vlv/chv enable vblank callbacks (rev3)
URL   : https://patchwork.freedesktop.org/series/13459/
State : warning

== Summary ==

Series 13459v3 Series without cover letter
https://patchwork.freedesktop.org/api/1.0/series/13459/revisions/3/mbox/

Test vgem_basic:
        Subgroup unload:
                pass       -> SKIP       (fi-ilk-650)
                pass       -> SKIP       (fi-byt-n2820)
                skip       -> PASS       (fi-skl-6700k)
                skip       -> PASS       (fi-hsw-4770)

fi-bdw-5557u     total:248  pass:231  dwarn:0   dfail:0   fail:0   skip:17 
fi-bsw-n3050     total:248  pass:204  dwarn:0   dfail:0   fail:0   skip:44 
fi-bxt-t5700     total:248  pass:217  dwarn:0   dfail:0   fail:0   skip:31 
fi-byt-j1900     total:248  pass:214  dwarn:1   dfail:0   fail:1   skip:32 
fi-byt-n2820     total:248  pass:210  dwarn:0   dfail:0   fail:1   skip:37 
fi-hsw-4770      total:248  pass:225  dwarn:0   dfail:0   fail:0   skip:23 
fi-hsw-4770r     total:248  pass:224  dwarn:0   dfail:0   fail:0   skip:24 
fi-ilk-650       total:248  pass:184  dwarn:0   dfail:0   fail:2   skip:62 
fi-ivb-3520m     total:248  pass:221  dwarn:0   dfail:0   fail:0   skip:27 
fi-ivb-3770      total:248  pass:207  dwarn:0   dfail:0   fail:0   skip:41 
fi-kbl-7200u     total:248  pass:222  dwarn:0   dfail:0   fail:0   skip:26 
fi-skl-6260u     total:248  pass:232  dwarn:0   dfail:0   fail:0   skip:16 
fi-skl-6700hq    total:248  pass:224  dwarn:0   dfail:0   fail:0   skip:24 
fi-skl-6700k     total:248  pass:222  dwarn:1   dfail:0   fail:0   skip:25 
fi-skl-6770hq    total:248  pass:231  dwarn:1   dfail:0   fail:1   skip:15 
fi-snb-2520m     total:248  pass:211  dwarn:0   dfail:0   fail:0   skip:37 
fi-snb-2600      total:248  pass:209  dwarn:0   dfail:0   fail:0   skip:39 

Results at /archive/results/CI_IGT_test/Patchwork_2660/

e37a15c8d775e79dddc8345a0f6afdcfe1f607d9 drm-intel-nightly: 2016y-10m-10d-14h-33m-29s UTC integration manifest
5dfc3dd drm/i915: Assert we hold the CRTC powerwell for generating vblank interrupts
2c51949 drm/i915: Merge duplicate gen4 and vlv/chv enable vblank callbacks

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

Re: [PATCH] drm/i915: Assert we hold the CRTC powerwell for generating vblank interrupts

[Maarten Lankhorst]

Op 10-10-16 om 13:56 schreef Ville Syrjälä:
> On Mon, Oct 10, 2016 at 12:46:32PM +0100, Chris Wilson wrote:
>> On Mon, Oct 10, 2016 at 02:42:01PM +0300, Ville Syrjälä wrote:
>>> On Mon, Oct 10, 2016 at 12:34:54PM +0100, Chris Wilson wrote:
>>>> To enable the vblank itself, we need to have an RPM wakeref for the mmio
>>>> access, and whilst generating the vblank interrupts we continue to
>>>> require the rpm wakeref. The assumption is that the RPM wakeref is held
>>>> by the display powerwell held by the active pipe. As this chain was not
>>>> obvious to me chasing the drm_wait_vblank ioctl, document it with a WARN
>>>> during *_vblank_enable().
>>>>
>>>> v2: Check the display power well rather than digging inside the atomic
>>>> CRTC state.
>>>>
>>>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>>>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>>> ---
>>>>  drivers/gpu/drm/i915/i915_irq.c | 20 ++++++++++++++++++++
>>>>  1 file changed, 20 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
>>>> index 1e43fe30da11..f0f17055dbb9 100644
>>>> --- a/drivers/gpu/drm/i915/i915_irq.c
>>>> +++ b/drivers/gpu/drm/i915/i915_irq.c
>>>> @@ -2715,6 +2715,14 @@ void i915_handle_error(struct drm_i915_private *dev_priv,
>>>>  	i915_reset_and_wakeup(dev_priv);
>>>>  }
>>>>  
>>>> +static void assert_pipe_is_awake(struct drm_i915_private *dev_priv,
>>>> +				 enum pipe pipe)
>>>> +{
>>>> +	WARN_ON(IS_ENABLED(CONFIG_DRM_I915_DEBUG) &&
>>>> +		!intel_display_power_is_enabled(dev_priv,
>>>> +						POWER_DOMAIN_PIPE(pipe)));
>>> Uses a mutex. And having a power well enabled doesn't mean much. It
>>> doesn't guarantee that vblanks work.
>> Impasse. :|
>>
>> There should be no point in an explicit assert_rpm_wakeref here as the
>> register access should catch an error there. Is there no safe way we can
>> assert the current state of the CRTC is correct for enabling vblanks?
> crtc->active might be the closest thing, if we just ignore any locking.
> Though it looks like that has gone a bit mad these days, what with being
> set from the .crtc_enable() hooks but getting cleared outside the
> .crtc_disable() hooks.
>
I'm trying to kill crtc->active. Maybe you'd want to use dev_priv->active_crtcs, but that won't save you if you enable interrupts in between atomic commit and .crtc_disable

Safest bet is to look at the power state I think.

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

Re: [PATCH] drm/i915: Assert we hold the CRTC powerwell for generating vblank interrupts

[Ville Syrjälä]

On Tue, Oct 11, 2016 at 08:17:22AM +0200, Maarten Lankhorst wrote:
> Op 10-10-16 om 13:56 schreef Ville Syrjälä:
> > On Mon, Oct 10, 2016 at 12:46:32PM +0100, Chris Wilson wrote:
> >> On Mon, Oct 10, 2016 at 02:42:01PM +0300, Ville Syrjälä wrote:
> >>> On Mon, Oct 10, 2016 at 12:34:54PM +0100, Chris Wilson wrote:
> >>>> To enable the vblank itself, we need to have an RPM wakeref for the mmio
> >>>> access, and whilst generating the vblank interrupts we continue to
> >>>> require the rpm wakeref. The assumption is that the RPM wakeref is held
> >>>> by the display powerwell held by the active pipe. As this chain was not
> >>>> obvious to me chasing the drm_wait_vblank ioctl, document it with a WARN
> >>>> during *_vblank_enable().
> >>>>
> >>>> v2: Check the display power well rather than digging inside the atomic
> >>>> CRTC state.
> >>>>
> >>>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> >>>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >>>> ---
> >>>>  drivers/gpu/drm/i915/i915_irq.c | 20 ++++++++++++++++++++
> >>>>  1 file changed, 20 insertions(+)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> >>>> index 1e43fe30da11..f0f17055dbb9 100644
> >>>> --- a/drivers/gpu/drm/i915/i915_irq.c
> >>>> +++ b/drivers/gpu/drm/i915/i915_irq.c
> >>>> @@ -2715,6 +2715,14 @@ void i915_handle_error(struct drm_i915_private *dev_priv,
> >>>>  	i915_reset_and_wakeup(dev_priv);
> >>>>  }
> >>>>  
> >>>> +static void assert_pipe_is_awake(struct drm_i915_private *dev_priv,
> >>>> +				 enum pipe pipe)
> >>>> +{
> >>>> +	WARN_ON(IS_ENABLED(CONFIG_DRM_I915_DEBUG) &&
> >>>> +		!intel_display_power_is_enabled(dev_priv,
> >>>> +						POWER_DOMAIN_PIPE(pipe)));
> >>> Uses a mutex. And having a power well enabled doesn't mean much. It
> >>> doesn't guarantee that vblanks work.
> >> Impasse. :|
> >>
> >> There should be no point in an explicit assert_rpm_wakeref here as the
> >> register access should catch an error there. Is there no safe way we can
> >> assert the current state of the CRTC is correct for enabling vblanks?
> > crtc->active might be the closest thing, if we just ignore any locking.
> > Though it looks like that has gone a bit mad these days, what with being
> > set from the .crtc_enable() hooks but getting cleared outside the
> > .crtc_disable() hooks.
> >
> I'm trying to kill crtc->active.

Because it's evil? I still don't see much problem in having a thing to
track the state of each pipe fairly accurately.

> Maybe you'd want to use dev_priv->active_crtcs, but that won't save you if you enable interrupts in between atomic commit and .crtc_disable

Nothing atomic based will work well because the state is not flipped at
the same time as the actual hardware state changes.

> 
> Safest bet is to look at the power state I think.

I don't know which power state you mean, but I already shot down the
power domain thing.


-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Assert we hold the CRTC powerwell for generating vblank interrupts

[Maarten Lankhorst]

Op 11-10-16 om 08:55 schreef Ville Syrjälä:
> On Tue, Oct 11, 2016 at 08:17:22AM +0200, Maarten Lankhorst wrote:
>> Op 10-10-16 om 13:56 schreef Ville Syrjälä:
>>> On Mon, Oct 10, 2016 at 12:46:32PM +0100, Chris Wilson wrote:
>>>> On Mon, Oct 10, 2016 at 02:42:01PM +0300, Ville Syrjälä wrote:
>>>>> On Mon, Oct 10, 2016 at 12:34:54PM +0100, Chris Wilson wrote:
>>>>>> To enable the vblank itself, we need to have an RPM wakeref for the mmio
>>>>>> access, and whilst generating the vblank interrupts we continue to
>>>>>> require the rpm wakeref. The assumption is that the RPM wakeref is held
>>>>>> by the display powerwell held by the active pipe. As this chain was not
>>>>>> obvious to me chasing the drm_wait_vblank ioctl, document it with a WARN
>>>>>> during *_vblank_enable().
>>>>>>
>>>>>> v2: Check the display power well rather than digging inside the atomic
>>>>>> CRTC state.
>>>>>>
>>>>>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>>>>>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>>>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>>>>> ---
>>>>>>  drivers/gpu/drm/i915/i915_irq.c | 20 ++++++++++++++++++++
>>>>>>  1 file changed, 20 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
>>>>>> index 1e43fe30da11..f0f17055dbb9 100644
>>>>>> --- a/drivers/gpu/drm/i915/i915_irq.c
>>>>>> +++ b/drivers/gpu/drm/i915/i915_irq.c
>>>>>> @@ -2715,6 +2715,14 @@ void i915_handle_error(struct drm_i915_private *dev_priv,
>>>>>>  	i915_reset_and_wakeup(dev_priv);
>>>>>>  }
>>>>>>  
>>>>>> +static void assert_pipe_is_awake(struct drm_i915_private *dev_priv,
>>>>>> +				 enum pipe pipe)
>>>>>> +{
>>>>>> +	WARN_ON(IS_ENABLED(CONFIG_DRM_I915_DEBUG) &&
>>>>>> +		!intel_display_power_is_enabled(dev_priv,
>>>>>> +						POWER_DOMAIN_PIPE(pipe)));
>>>>> Uses a mutex. And having a power well enabled doesn't mean much. It
>>>>> doesn't guarantee that vblanks work.
>>>> Impasse. :|
>>>>
>>>> There should be no point in an explicit assert_rpm_wakeref here as the
>>>> register access should catch an error there. Is there no safe way we can
>>>> assert the current state of the CRTC is correct for enabling vblanks?
>>> crtc->active might be the closest thing, if we just ignore any locking.
>>> Though it looks like that has gone a bit mad these days, what with being
>>> set from the .crtc_enable() hooks but getting cleared outside the
>>> .crtc_disable() hooks.
>>>
>> I'm trying to kill crtc->active.
> Because it's evil? I still don't see much problem in having a thing to
> track the state of each pipe fairly accurately.
>
>> Maybe you'd want to use dev_priv->active_crtcs, but that won't save you if you enable interrupts in between atomic commit and .crtc_disable
> Nothing atomic based will work well because the state is not flipped at
> the same time as the actual hardware state changes.
>
>> Safest bet is to look at the power state I think.
> I don't know which power state you mean, but I already shot down the
> power domain thing.
>
>
I would say use assert_pipe_enabled then.

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

Re: [PATCH] drm/i915: Assert we hold the CRTC powerwell for generating vblank interrupts

[Ville Syrjälä]

On Tue, Oct 11, 2016 at 09:45:45AM +0200, Maarten Lankhorst wrote:
> Op 11-10-16 om 08:55 schreef Ville Syrjälä:
> > On Tue, Oct 11, 2016 at 08:17:22AM +0200, Maarten Lankhorst wrote:
> >> Op 10-10-16 om 13:56 schreef Ville Syrjälä:
> >>> On Mon, Oct 10, 2016 at 12:46:32PM +0100, Chris Wilson wrote:
> >>>> On Mon, Oct 10, 2016 at 02:42:01PM +0300, Ville Syrjälä wrote:
> >>>>> On Mon, Oct 10, 2016 at 12:34:54PM +0100, Chris Wilson wrote:
> >>>>>> To enable the vblank itself, we need to have an RPM wakeref for the mmio
> >>>>>> access, and whilst generating the vblank interrupts we continue to
> >>>>>> require the rpm wakeref. The assumption is that the RPM wakeref is held
> >>>>>> by the display powerwell held by the active pipe. As this chain was not
> >>>>>> obvious to me chasing the drm_wait_vblank ioctl, document it with a WARN
> >>>>>> during *_vblank_enable().
> >>>>>>
> >>>>>> v2: Check the display power well rather than digging inside the atomic
> >>>>>> CRTC state.
> >>>>>>
> >>>>>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> >>>>>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>>>>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >>>>>> ---
> >>>>>>  drivers/gpu/drm/i915/i915_irq.c | 20 ++++++++++++++++++++
> >>>>>>  1 file changed, 20 insertions(+)
> >>>>>>
> >>>>>> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> >>>>>> index 1e43fe30da11..f0f17055dbb9 100644
> >>>>>> --- a/drivers/gpu/drm/i915/i915_irq.c
> >>>>>> +++ b/drivers/gpu/drm/i915/i915_irq.c
> >>>>>> @@ -2715,6 +2715,14 @@ void i915_handle_error(struct drm_i915_private *dev_priv,
> >>>>>>  	i915_reset_and_wakeup(dev_priv);
> >>>>>>  }
> >>>>>>  
> >>>>>> +static void assert_pipe_is_awake(struct drm_i915_private *dev_priv,
> >>>>>> +				 enum pipe pipe)
> >>>>>> +{
> >>>>>> +	WARN_ON(IS_ENABLED(CONFIG_DRM_I915_DEBUG) &&
> >>>>>> +		!intel_display_power_is_enabled(dev_priv,
> >>>>>> +						POWER_DOMAIN_PIPE(pipe)));
> >>>>> Uses a mutex. And having a power well enabled doesn't mean much. It
> >>>>> doesn't guarantee that vblanks work.
> >>>> Impasse. :|
> >>>>
> >>>> There should be no point in an explicit assert_rpm_wakeref here as the
> >>>> register access should catch an error there. Is there no safe way we can
> >>>> assert the current state of the CRTC is correct for enabling vblanks?
> >>> crtc->active might be the closest thing, if we just ignore any locking.
> >>> Though it looks like that has gone a bit mad these days, what with being
> >>> set from the .crtc_enable() hooks but getting cleared outside the
> >>> .crtc_disable() hooks.
> >>>
> >> I'm trying to kill crtc->active.
> > Because it's evil? I still don't see much problem in having a thing to
> > track the state of each pipe fairly accurately.
> >
> >> Maybe you'd want to use dev_priv->active_crtcs, but that won't save you if you enable interrupts in between atomic commit and .crtc_disable
> > Nothing atomic based will work well because the state is not flipped at
> > the same time as the actual hardware state changes.
> >
> >> Safest bet is to look at the power state I think.
> > I don't know which power state you mean, but I already shot down the
> > power domain thing.
> >
> >
> I would say use assert_pipe_enabled then.

Nope. That one frobs with power domains too these days.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Assert we hold the CRTC powerwell for generating vblank interrupts

[Maarten Lankhorst]

Op 11-10-16 om 10:32 schreef Ville Syrjälä:
> On Tue, Oct 11, 2016 at 09:45:45AM +0200, Maarten Lankhorst wrote:
>> Op 11-10-16 om 08:55 schreef Ville Syrjälä:
>>> On Tue, Oct 11, 2016 at 08:17:22AM +0200, Maarten Lankhorst wrote:
>>>> Op 10-10-16 om 13:56 schreef Ville Syrjälä:
>>>>> On Mon, Oct 10, 2016 at 12:46:32PM +0100, Chris Wilson wrote:
>>>>>> On Mon, Oct 10, 2016 at 02:42:01PM +0300, Ville Syrjälä wrote:
>>>>>>> On Mon, Oct 10, 2016 at 12:34:54PM +0100, Chris Wilson wrote:
>>>>>>>> To enable the vblank itself, we need to have an RPM wakeref for the mmio
>>>>>>>> access, and whilst generating the vblank interrupts we continue to
>>>>>>>> require the rpm wakeref. The assumption is that the RPM wakeref is held
>>>>>>>> by the display powerwell held by the active pipe. As this chain was not
>>>>>>>> obvious to me chasing the drm_wait_vblank ioctl, document it with a WARN
>>>>>>>> during *_vblank_enable().
>>>>>>>>
>>>>>>>> v2: Check the display power well rather than digging inside the atomic
>>>>>>>> CRTC state.
>>>>>>>>
>>>>>>>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>>>>>>>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>>>>>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>>>>>>> ---
>>>>>>>>  drivers/gpu/drm/i915/i915_irq.c | 20 ++++++++++++++++++++
>>>>>>>>  1 file changed, 20 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
>>>>>>>> index 1e43fe30da11..f0f17055dbb9 100644
>>>>>>>> --- a/drivers/gpu/drm/i915/i915_irq.c
>>>>>>>> +++ b/drivers/gpu/drm/i915/i915_irq.c
>>>>>>>> @@ -2715,6 +2715,14 @@ void i915_handle_error(struct drm_i915_private *dev_priv,
>>>>>>>>  	i915_reset_and_wakeup(dev_priv);
>>>>>>>>  }
>>>>>>>>  
>>>>>>>> +static void assert_pipe_is_awake(struct drm_i915_private *dev_priv,
>>>>>>>> +				 enum pipe pipe)
>>>>>>>> +{
>>>>>>>> +	WARN_ON(IS_ENABLED(CONFIG_DRM_I915_DEBUG) &&
>>>>>>>> +		!intel_display_power_is_enabled(dev_priv,
>>>>>>>> +						POWER_DOMAIN_PIPE(pipe)));
>>>>>>> Uses a mutex. And having a power well enabled doesn't mean much. It
>>>>>>> doesn't guarantee that vblanks work.
>>>>>> Impasse. :|
>>>>>>
>>>>>> There should be no point in an explicit assert_rpm_wakeref here as the
>>>>>> register access should catch an error there. Is there no safe way we can
>>>>>> assert the current state of the CRTC is correct for enabling vblanks?
>>>>> crtc->active might be the closest thing, if we just ignore any locking.
>>>>> Though it looks like that has gone a bit mad these days, what with being
>>>>> set from the .crtc_enable() hooks but getting cleared outside the
>>>>> .crtc_disable() hooks.
>>>>>
>>>> I'm trying to kill crtc->active.
>>> Because it's evil? I still don't see much problem in having a thing to
>>> track the state of each pipe fairly accurately.
>>>
>>>> Maybe you'd want to use dev_priv->active_crtcs, but that won't save you if you enable interrupts in between atomic commit and .crtc_disable
>>> Nothing atomic based will work well because the state is not flipped at
>>> the same time as the actual hardware state changes.
>>>
>>>> Safest bet is to look at the power state I think.
>>> I don't know which power state you mean, but I already shot down the
>>> power domain thing.
>>>
>>>
>> I would say use assert_pipe_enabled then.
> Nope. That one frobs with power domains too these days.
>
I don't see a nice way to do this, it probably means we shouldn't do this at all..
Maybe have a function look at dev_priv->power_domains.domain_use_count[[POWER_DOMAIN_PIPE(pipe)] >= 1?
It's potentially racy but I don't see a nice way to check, apart from hoping we handle the drm vblank on/off
calls correctly.

The only other way I see is demidlayering vblank.

~Maarten

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

Re: [PATCH] drm/i915: Assert we hold the CRTC powerwell for generating vblank interrupts

[Daniel Vetter]

On Wed, Oct 12, 2016 at 07:21:44AM +0200, Maarten Lankhorst wrote:
> Op 11-10-16 om 10:32 schreef Ville Syrjälä:
> > On Tue, Oct 11, 2016 at 09:45:45AM +0200, Maarten Lankhorst wrote:
> >> Op 11-10-16 om 08:55 schreef Ville Syrjälä:
> >>> On Tue, Oct 11, 2016 at 08:17:22AM +0200, Maarten Lankhorst wrote:
> >>>> Op 10-10-16 om 13:56 schreef Ville Syrjälä:
> >>>>> On Mon, Oct 10, 2016 at 12:46:32PM +0100, Chris Wilson wrote:
> >>>>>> On Mon, Oct 10, 2016 at 02:42:01PM +0300, Ville Syrjälä wrote:
> >>>>>>> On Mon, Oct 10, 2016 at 12:34:54PM +0100, Chris Wilson wrote:
> >>>>>>>> To enable the vblank itself, we need to have an RPM wakeref for the mmio
> >>>>>>>> access, and whilst generating the vblank interrupts we continue to
> >>>>>>>> require the rpm wakeref. The assumption is that the RPM wakeref is held
> >>>>>>>> by the display powerwell held by the active pipe. As this chain was not
> >>>>>>>> obvious to me chasing the drm_wait_vblank ioctl, document it with a WARN
> >>>>>>>> during *_vblank_enable().
> >>>>>>>>
> >>>>>>>> v2: Check the display power well rather than digging inside the atomic
> >>>>>>>> CRTC state.
> >>>>>>>>
> >>>>>>>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> >>>>>>>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>>>>>>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >>>>>>>> ---
> >>>>>>>>  drivers/gpu/drm/i915/i915_irq.c | 20 ++++++++++++++++++++
> >>>>>>>>  1 file changed, 20 insertions(+)
> >>>>>>>>
> >>>>>>>> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> >>>>>>>> index 1e43fe30da11..f0f17055dbb9 100644
> >>>>>>>> --- a/drivers/gpu/drm/i915/i915_irq.c
> >>>>>>>> +++ b/drivers/gpu/drm/i915/i915_irq.c
> >>>>>>>> @@ -2715,6 +2715,14 @@ void i915_handle_error(struct drm_i915_private *dev_priv,
> >>>>>>>>  	i915_reset_and_wakeup(dev_priv);
> >>>>>>>>  }
> >>>>>>>>  
> >>>>>>>> +static void assert_pipe_is_awake(struct drm_i915_private *dev_priv,
> >>>>>>>> +				 enum pipe pipe)
> >>>>>>>> +{
> >>>>>>>> +	WARN_ON(IS_ENABLED(CONFIG_DRM_I915_DEBUG) &&
> >>>>>>>> +		!intel_display_power_is_enabled(dev_priv,
> >>>>>>>> +						POWER_DOMAIN_PIPE(pipe)));
> >>>>>>> Uses a mutex. And having a power well enabled doesn't mean much. It
> >>>>>>> doesn't guarantee that vblanks work.
> >>>>>> Impasse. :|
> >>>>>>
> >>>>>> There should be no point in an explicit assert_rpm_wakeref here as the
> >>>>>> register access should catch an error there. Is there no safe way we can
> >>>>>> assert the current state of the CRTC is correct for enabling vblanks?
> >>>>> crtc->active might be the closest thing, if we just ignore any locking.
> >>>>> Though it looks like that has gone a bit mad these days, what with being
> >>>>> set from the .crtc_enable() hooks but getting cleared outside the
> >>>>> .crtc_disable() hooks.
> >>>>>
> >>>> I'm trying to kill crtc->active.
> >>> Because it's evil? I still don't see much problem in having a thing to
> >>> track the state of each pipe fairly accurately.
> >>>
> >>>> Maybe you'd want to use dev_priv->active_crtcs, but that won't save you if you enable interrupts in between atomic commit and .crtc_disable
> >>> Nothing atomic based will work well because the state is not flipped at
> >>> the same time as the actual hardware state changes.
> >>>
> >>>> Safest bet is to look at the power state I think.
> >>> I don't know which power state you mean, but I already shot down the
> >>> power domain thing.
> >>>
> >>>
> >> I would say use assert_pipe_enabled then.
> > Nope. That one frobs with power domains too these days.
> >
> I don't see a nice way to do this, it probably means we shouldn't do this at all..
> Maybe have a function look at dev_priv->power_domains.domain_use_count[[POWER_DOMAIN_PIPE(pipe)] >= 1?
> It's potentially racy but I don't see a nice way to check, apart from hoping we handle the drm vblank on/off
> calls correctly.
> 
> The only other way I see is demidlayering vblank.

Or switch over power domains over to the core power domain stuff. There's
a reason those are protected with spinlocks, so that you can do these
kinds of checks ;-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/i915: Merge duplicate gen4 and vlv/chv enable vblank callbacks

[Daniel Vetter]

On Fri, Oct 07, 2016 at 08:49:52PM +0100, Chris Wilson wrote:
> gen4/vlv/chv all use the same bits in pipestat to enable the vblank
> interrupt, so they can share the same callbacks to enable/disable.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>

Irrespective of patch 2 this seems nice. git made a mess of the diff, but
I think it all checks out.

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

> ---
>  drivers/gpu/drm/i915/i915_irq.c | 59 +++++++++++++++++++----------------------
>  1 file changed, 28 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index bd6c8b0eeaef..1e43fe30da11 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -2718,45 +2718,40 @@ void i915_handle_error(struct drm_i915_private *dev_priv,
>  /* Called from drm generic code, passed 'crtc' which
>   * we use as a pipe index
>   */
> -static int i915_enable_vblank(struct drm_device *dev, unsigned int pipe)
> +static int i8xx_enable_vblank(struct drm_device *dev, unsigned int pipe)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  	unsigned long irqflags;
>  
>  	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
> -	if (INTEL_INFO(dev)->gen >= 4)
> -		i915_enable_pipestat(dev_priv, pipe,
> -				     PIPE_START_VBLANK_INTERRUPT_STATUS);
> -	else
> -		i915_enable_pipestat(dev_priv, pipe,
> -				     PIPE_VBLANK_INTERRUPT_STATUS);
> +	i915_enable_pipestat(dev_priv, pipe, PIPE_VBLANK_INTERRUPT_STATUS);
>  	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
>  
>  	return 0;
>  }
>  
> -static int ironlake_enable_vblank(struct drm_device *dev, unsigned int pipe)
> +static int i965_enable_vblank(struct drm_device *dev, unsigned int pipe)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  	unsigned long irqflags;
> -	uint32_t bit = (INTEL_INFO(dev)->gen >= 7) ? DE_PIPE_VBLANK_IVB(pipe) :
> -						     DE_PIPE_VBLANK(pipe);
>  
>  	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
> -	ilk_enable_display_irq(dev_priv, bit);
> +	i915_enable_pipestat(dev_priv, pipe,
> +			     PIPE_START_VBLANK_INTERRUPT_STATUS);
>  	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
>  
>  	return 0;
>  }
>  
> -static int valleyview_enable_vblank(struct drm_device *dev, unsigned int pipe)
> +static int ironlake_enable_vblank(struct drm_device *dev, unsigned int pipe)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  	unsigned long irqflags;
> +	uint32_t bit = INTEL_GEN(dev) >= 7 ?
> +		DE_PIPE_VBLANK_IVB(pipe) : DE_PIPE_VBLANK(pipe);
>  
>  	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
> -	i915_enable_pipestat(dev_priv, pipe,
> -			     PIPE_START_VBLANK_INTERRUPT_STATUS);
> +	ilk_enable_display_irq(dev_priv, bit);
>  	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
>  
>  	return 0;
> @@ -2777,38 +2772,36 @@ static int gen8_enable_vblank(struct drm_device *dev, unsigned int pipe)
>  /* Called from drm generic code, passed 'crtc' which
>   * we use as a pipe index
>   */
> -static void i915_disable_vblank(struct drm_device *dev, unsigned int pipe)
> +static void i8xx_disable_vblank(struct drm_device *dev, unsigned int pipe)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  	unsigned long irqflags;
>  
>  	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
> -	i915_disable_pipestat(dev_priv, pipe,
> -			      PIPE_VBLANK_INTERRUPT_STATUS |
> -			      PIPE_START_VBLANK_INTERRUPT_STATUS);
> +	i915_disable_pipestat(dev_priv, pipe, PIPE_VBLANK_INTERRUPT_STATUS);
>  	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
>  }
>  
> -static void ironlake_disable_vblank(struct drm_device *dev, unsigned int pipe)
> +static void i965_disable_vblank(struct drm_device *dev, unsigned int pipe)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  	unsigned long irqflags;
> -	uint32_t bit = (INTEL_INFO(dev)->gen >= 7) ? DE_PIPE_VBLANK_IVB(pipe) :
> -						     DE_PIPE_VBLANK(pipe);
>  
>  	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
> -	ilk_disable_display_irq(dev_priv, bit);
> +	i915_disable_pipestat(dev_priv, pipe,
> +			      PIPE_START_VBLANK_INTERRUPT_STATUS);
>  	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
>  }
>  
> -static void valleyview_disable_vblank(struct drm_device *dev, unsigned int pipe)
> +static void ironlake_disable_vblank(struct drm_device *dev, unsigned int pipe)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  	unsigned long irqflags;
> +	uint32_t bit = INTEL_GEN(dev) >= 7 ?
> +		DE_PIPE_VBLANK_IVB(pipe) : DE_PIPE_VBLANK(pipe);
>  
>  	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
> -	i915_disable_pipestat(dev_priv, pipe,
> -			      PIPE_START_VBLANK_INTERRUPT_STATUS);
> +	ilk_disable_display_irq(dev_priv, bit);
>  	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
>  }
>  
> @@ -4579,16 +4572,16 @@ void intel_irq_init(struct drm_i915_private *dev_priv)
>  		dev->driver->irq_preinstall = cherryview_irq_preinstall;
>  		dev->driver->irq_postinstall = cherryview_irq_postinstall;
>  		dev->driver->irq_uninstall = cherryview_irq_uninstall;
> -		dev->driver->enable_vblank = valleyview_enable_vblank;
> -		dev->driver->disable_vblank = valleyview_disable_vblank;
> +		dev->driver->enable_vblank = i965_enable_vblank;
> +		dev->driver->disable_vblank = i965_disable_vblank;
>  		dev_priv->display.hpd_irq_setup = i915_hpd_irq_setup;
>  	} else if (IS_VALLEYVIEW(dev_priv)) {
>  		dev->driver->irq_handler = valleyview_irq_handler;
>  		dev->driver->irq_preinstall = valleyview_irq_preinstall;
>  		dev->driver->irq_postinstall = valleyview_irq_postinstall;
>  		dev->driver->irq_uninstall = valleyview_irq_uninstall;
> -		dev->driver->enable_vblank = valleyview_enable_vblank;
> -		dev->driver->disable_vblank = valleyview_disable_vblank;
> +		dev->driver->enable_vblank = i965_enable_vblank;
> +		dev->driver->disable_vblank = i965_disable_vblank;
>  		dev_priv->display.hpd_irq_setup = i915_hpd_irq_setup;
>  	} else if (INTEL_INFO(dev_priv)->gen >= 8) {
>  		dev->driver->irq_handler = gen8_irq_handler;
> @@ -4617,21 +4610,25 @@ void intel_irq_init(struct drm_i915_private *dev_priv)
>  			dev->driver->irq_postinstall = i8xx_irq_postinstall;
>  			dev->driver->irq_handler = i8xx_irq_handler;
>  			dev->driver->irq_uninstall = i8xx_irq_uninstall;
> +			dev->driver->enable_vblank = i8xx_enable_vblank;
> +			dev->driver->disable_vblank = i8xx_disable_vblank;
>  		} else if (IS_GEN3(dev_priv)) {
>  			dev->driver->irq_preinstall = i915_irq_preinstall;
>  			dev->driver->irq_postinstall = i915_irq_postinstall;
>  			dev->driver->irq_uninstall = i915_irq_uninstall;
>  			dev->driver->irq_handler = i915_irq_handler;
> +			dev->driver->enable_vblank = i8xx_enable_vblank;
> +			dev->driver->disable_vblank = i8xx_disable_vblank;
>  		} else {
>  			dev->driver->irq_preinstall = i965_irq_preinstall;
>  			dev->driver->irq_postinstall = i965_irq_postinstall;
>  			dev->driver->irq_uninstall = i965_irq_uninstall;
>  			dev->driver->irq_handler = i965_irq_handler;
> +			dev->driver->enable_vblank = i965_enable_vblank;
> +			dev->driver->disable_vblank = i965_disable_vblank;
>  		}
>  		if (I915_HAS_HOTPLUG(dev_priv))
>  			dev_priv->display.hpd_irq_setup = i915_hpd_irq_setup;
> -		dev->driver->enable_vblank = i915_enable_vblank;
> -		dev->driver->disable_vblank = i915_disable_vblank;
>  	}
>  }
>  
> -- 
> 2.9.3
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Assert we hold the CRTC powerwell for generating vblank interrupts

[Chris Wilson]

On Thu, Oct 13, 2016 at 04:17:07PM +0200, Daniel Vetter wrote:
> On Wed, Oct 12, 2016 at 07:21:44AM +0200, Maarten Lankhorst wrote:
> > I don't see a nice way to do this, it probably means we shouldn't do this at all..
> > Maybe have a function look at dev_priv->power_domains.domain_use_count[[POWER_DOMAIN_PIPE(pipe)] >= 1?
> > It's potentially racy but I don't see a nice way to check, apart from hoping we handle the drm vblank on/off
> > calls correctly.
> > 
> > The only other way I see is demidlayering vblank.
> 
> Or switch over power domains over to the core power domain stuff. There's
> a reason those are protected with spinlocks, so that you can do these
> kinds of checks ;-)

Why not both! They're on the wishlist somewhere.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx