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

Maarten Lankhorst
2018-02-10T08:46:16Z

Op 10-02-18 om 00:08 schreef James Ausmus:
> 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 ?
Good catch, it shouldn't have passed BAT. I guess we don't have a g4 or gen3 system there.

~Maarten
>
>> + 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