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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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