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

Ville Syrjälä
2016-10-10T11:42:05Z

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