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

James Ausmus
2018-02-09T23:05:59Z

On Fri, Feb 09, 2018 at 10:54:02AM +0100, Maarten Lankhorst wrote:
> 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;
>
> 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);

Shouldn't this be __i915_get_vblank_counter ?

> + 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);
> +
> + return I915_READ_FW(PIPE_FRMCOUNT_G4X(crtc->pipe));
> +}
> +
> static u32 g4x_get_vblank_counter(struct drm_device *dev, unsigned int pipe)
> {
> struct drm_i915_private *dev_priv = to_i915(dev);
> + struct intel_crtc *crtc = intel_get_crtc_for_pipe(dev_priv, pipe);
> + unsigned long irqflags;
> + u32 ret;
> +
> + spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
> + ret = __g4x_get_vblank_counter(crtc);
> + spin_unlock_irqrestore(&dev_priv->uncore.lock, irqflags);
> +
> + return ret;
> +}
> +
> +u32 __intel_crtc_get_vblank_counter(struct intel_crtc *crtc)
> +{
> + struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> +
> + if (IS_G4X(dev_priv) || INTEL_GEN(dev_priv) >= 5)
> + return __g4x_get_vblank_counter(crtc);
> + else if (IS_GEN2(dev_priv))
> + return 0;
> + else
> + return __i915_get_vblank_counter(crtc);
> +}
> +
> +u32 intel_crtc_get_vblank_counter(struct intel_crtc *crtc)
> +{
> + struct drm_device *dev = crtc->base.dev;
> +
> + if (!dev->max_vblank_count)
> + return drm_crtc_accurate_vblank_count(&crtc->base);
>
> - return I915_READ(PIPE_FRMCOUNT_G4X(pipe));
> + return dev->driver->get_vblank_counter(dev, crtc->pipe);
> }
>
> /*
> diff --git a/drivers/gpu/drm/i915/i915_trace.h b/drivers/gpu/drm/i915/i915_trace.h
> index e1169c02eb2b..d4a5776282ff 100644
> --- a/drivers/gpu/drm/i915/i915_trace.h
> +++ b/drivers/gpu/drm/i915/i915_trace.h
> @@ -280,9 +280,8 @@ TRACE_EVENT(i915_pipe_update_start,
>
> TP_fast_assign(
> __entry->pipe = crtc->pipe;
> - __entry->frame = crtc->base.dev->driver->get_vblank_counter(crtc->base.dev,
> - crtc->pipe);
> - __entry->scanline = intel_get_crtc_scanline(crtc);
> + __entry->frame = __intel_crtc_get_vblank_counter(crtc);
> + __entry->scanline = __intel_get_crtc_scanline(crtc);
> __entry->min = crtc->debug.min_vbl;
> __entry->max = crtc->debug.max_vbl;
> ),
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index fbdbbe741b2f..048fcd3c960e 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1489,6 +1489,7 @@ intel_wait_for_vblank_if_active(struct drm_i915_private *dev_priv, int pipe)
> }
>
> u32 intel_crtc_get_vblank_counter(struct intel_crtc *crtc);
> +u32 __intel_crtc_get_vblank_counter(struct intel_crtc *crtc);
>
> int ironlake_get_lanes_required(int target_clock, int link_bw, int bpp);
> void vlv_wait_port_ready(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 3a34be4fd956..95f0999ea18a 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -117,9 +117,10 @@ void intel_pipe_update_start(const struct intel_crtc_state *new_crtc_state)
>
> crtc->debug.min_vbl = min;
> crtc->debug.max_vbl = max;
> - trace_i915_pipe_update_start(crtc);
>
> spin_lock(&dev_priv->uncore.lock);
> + trace_i915_pipe_update_start(crtc);
> +
> for (;;) {
> /*
> * prepare_to_wait() has a memory barrier, which guarantees
> --
> 2.16.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx