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

[Maarten Lankhorst <maarten.lankhorst@linux.intel.com>]

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


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx