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

[Chris Wilson]

Quoting Maarten Lankhorst (2018-02-12 15:39:16)
> Op 12-02-18 om 16:19 schreef Chris Wilson:
> > 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 at 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.)
> 
> Is the serialization only needed for writing?

No, sadly not. Concurrent access of any type to the same cacheline is
the trigger. (Supposed to be ivb-only.)
 
> The only thing that can race with nonblocking atomic commits are legacy
> cursor updates, but those can only happen if the cursor plane are not part
> of the previous atomic commit. Those are also protected by plane->mutex,
> so in theory same cache lines on the same pipes probably can't race..

At worst, we could just use a vblank->spinlock?
-Chris