[Intel-gfx] [PATCH 2/5] drm/i915: Grab uncore.lock around enabling vblank evasion

[Chris Wilson]

Quoting Maarten Lankhorst (2018-02-09 09:54:01)
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_irq.c     |  2 +-
>  drivers/gpu/drm/i915/intel_drv.h    |  1 +
>  drivers/gpu/drm/i915/intel_sprite.c | 10 ++++++----
>  3 files changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index b886bd459acc..eda9543a0199 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -845,7 +845,7 @@ static u32 __intel_get_crtc_scanline_from_timestamp(struct intel_crtc *crtc)
>  }
>  
>  /* I915_READ_FW, only for fast reads of display block, no need for forcewake etc. */
> -static int __intel_get_crtc_scanline(struct intel_crtc *crtc)
> +int __intel_get_crtc_scanline(struct intel_crtc *crtc)
>  {
>         struct drm_device *dev = crtc->base.dev;
>         struct drm_i915_private *dev_priv = to_i915(dev);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 468ec1e90e16..fbdbbe741b2f 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1340,6 +1340,7 @@ static inline bool intel_irqs_enabled(struct drm_i915_private *dev_priv)
>  }
>  
>  int intel_get_crtc_scanline(struct intel_crtc *crtc);
> +int __intel_get_crtc_scanline(struct intel_crtc *crtc);
>  void gen8_irq_power_well_post_enable(struct drm_i915_private *dev_priv,
>                                      u8 pipe_mask);
>  void gen8_irq_power_well_pre_disable(struct drm_i915_private *dev_priv,
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index 971a1ea0db45..3a34be4fd956 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -119,6 +119,7 @@ void intel_pipe_update_start(const struct intel_crtc_state *new_crtc_state)
>         crtc->debug.max_vbl = max;
>         trace_i915_pipe_update_start(crtc);
>  
> +       spin_lock(&dev_priv->uncore.lock);
>         for (;;) {
>                 /*
>                  * prepare_to_wait() has a memory barrier, which guarantees
> @@ -127,7 +128,7 @@ void intel_pipe_update_start(const struct intel_crtc_state *new_crtc_state)
>                  */
>                 prepare_to_wait(wq, &wait, TASK_UNINTERRUPTIBLE);
>  
> -               scanline = intel_get_crtc_scanline(crtc);
> +               scanline = __intel_get_crtc_scanline(crtc);
>                 if (scanline < min || scanline > max)
>                         break;
>  
> @@ -137,11 +138,11 @@ void intel_pipe_update_start(const struct intel_crtc_state *new_crtc_state)
>                         break;
>                 }
>  
> -               local_irq_enable();
> +               spin_unlock_irq(&dev_priv->uncore.lock);
>  
>                 timeout = schedule_timeout(timeout);
>  
> -               local_irq_disable();
> +               spin_lock_irq(&dev_priv->uncore.lock);
>         }
>  
>         finish_wait(wq, &wait);

There's no danger that drm_crtc_vblank_put() does something crazy here.
(Feels like a layering violation to call into DRM with the low level
uncore.lock held at least.) It looks like the driver can be tricked into
called ->disable_vblank()?

Overall though, I think it is just this need_vlv_dsi_wa chunk that has
any benefit here (although trading lock_irq for lock_irqsave is enough
to justify a change if frequently hit).
-Chris