Re: [PATCH 3/5] drm/i915: Call i915_pipe_update_start with uncore.lock held.

[Chris Wilson <chris@chris-wilson.co.uk>]

Quoting Maarten Lankhorst (2018-02-09 09:54:02)
> This requires being able to read the vblank counter with the
> uncore.lock already held. This is also a preparation for
> being able to run the entire vblank update sequence with
> the uncore lock held.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_irq.c     | 66 ++++++++++++++++++++++++++++++-------
>  drivers/gpu/drm/i915/i915_trace.h   |  5 ++-
>  drivers/gpu/drm/i915/intel_drv.h    |  1 +
>  drivers/gpu/drm/i915/intel_sprite.c |  3 +-
>  4 files changed, 60 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index eda9543a0199..6c491e63e07c 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -736,13 +736,12 @@ static void i915_enable_asle_pipestat(struct drm_i915_private *dev_priv)
>  /* Called from drm generic code, passed a 'crtc', which
>   * we use as a pipe index
>   */
> -static u32 i915_get_vblank_counter(struct drm_device *dev, unsigned int pipe)
> +static u32 __i915_get_vblank_counter(struct intel_crtc *crtc)
>  {
> -       struct drm_i915_private *dev_priv = to_i915(dev);
> +       struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>         i915_reg_t high_frame, low_frame;
>         u32 high1, high2, low, pixel, vbl_start, hsync_start, htotal;
> -       const struct drm_display_mode *mode = &dev->vblank[pipe].hwmode;
> -       unsigned long irqflags;
> +       const struct drm_display_mode *mode = &crtc->base.dev->vblank[crtc->pipe].hwmode;

lockdep_assert_held(&dev_priv->uncore.lock);

>  
>         htotal = mode->crtc_htotal;
>         hsync_start = mode->crtc_hsync_start;
> @@ -756,10 +755,8 @@ static u32 i915_get_vblank_counter(struct drm_device *dev, unsigned int pipe)
>         /* Start of vblank event occurs at start of hsync */
>         vbl_start -= htotal - hsync_start;
>  
> -       high_frame = PIPEFRAME(pipe);
> -       low_frame = PIPEFRAMEPIXEL(pipe);
> -
> -       spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
> +       high_frame = PIPEFRAME(crtc->pipe);
> +       low_frame = PIPEFRAMEPIXEL(crtc->pipe);
>  
>         /*
>          * High & low register fields aren't synchronized, so make sure
> @@ -772,8 +769,6 @@ static u32 i915_get_vblank_counter(struct drm_device *dev, unsigned int pipe)
>                 high2 = I915_READ_FW(high_frame) & PIPE_FRAME_HIGH_MASK;
>         } while (high1 != high2);
>  
> -       spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
> -
>         high1 >>= PIPE_FRAME_HIGH_SHIFT;
>         pixel = low & PIPE_PIXEL_MASK;
>         low >>= PIPE_FRAME_LOW_SHIFT;
> @@ -786,11 +781,60 @@ static u32 i915_get_vblank_counter(struct drm_device *dev, unsigned int pipe)
>         return (((high1 << 8) | low) + (pixel >= vbl_start)) & 0xffffff;
>  }
>  
> +static u32 i915_get_vblank_counter(struct drm_device *dev, unsigned int pipe)
> +{
> +       struct drm_i915_private *dev_priv = to_i915(dev);
> +       unsigned long irqflags;
> +       u32 ret;
> +
> +       spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
> +       ret = i915_get_vblank_counter(dev, pipe);
> +       spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
> +
> +       return ret;
> +}
> +
> +static u32 __g4x_get_vblank_counter(struct intel_crtc *crtc)
> +{
> +       struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);

lockdep_assert_held(&dev_priv->uncore.lock); ?

Ok, why do we need uncore.lock held here at all? Serialisation of mmio
access to the same cacheline is the age old reason, can we guarantee
that doesn't happen anyway? (Probably not, but really most callers do
not need the mmio w/a.)
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx