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@xxxxxxxxxxxxxxx>
> >> ---
> >> 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
