Re: [PATCH v2] RFC drm/i915: Only acquire vblank-irq when waiting for vblank evade

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Mon, Feb 12, 2018 at 10:21:11PM +0000, Chris Wilson wrote:
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_sprite.c | 65 ++++++++++++++++++++++---------------
>  1 file changed, 38 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index e098e4b2c85c..ad707c97a9d9 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -74,6 +74,14 @@ int intel_usecs_to_scanlines(const struct drm_display_mode *adjusted_mode,
>  #define VBLANK_EVASION_TIME_US 100
>  #endif
>  
> +static inline bool scanline_in_vblank(struct intel_crtc *crtc,
> +				      int min, int max,
> +				      int *scanline)

The function name is wrong. Not sure what to call it really.
scanline_ok(), or reverse the condition and call it
scanline_bad() or scanline_in_danger_zone() (cue Kenny Loggins)?

> +{
> +	*scanline = intel_get_crtc_scanline(crtc);
> +	return *scanline < min || *scanline > max;
> +}
> +
>  /**
>   * intel_pipe_update_start() - start update of a set of display registers
>   * @new_crtc_state: the new crtc state
> @@ -91,12 +99,9 @@ void intel_pipe_update_start(const struct intel_crtc_state *new_crtc_state)
>  	struct intel_crtc *crtc = to_intel_crtc(new_crtc_state->base.crtc);
>  	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>  	const struct drm_display_mode *adjusted_mode = &new_crtc_state->base.adjusted_mode;
> -	long timeout = msecs_to_jiffies_timeout(1);
>  	int scanline, min, max, vblank_start;
> -	wait_queue_head_t *wq = drm_crtc_vblank_waitqueue(&crtc->base);
>  	bool need_vlv_dsi_wa = (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) &&
>  		intel_crtc_has_type(new_crtc_state, INTEL_OUTPUT_DSI);
> -	DEFINE_WAIT(wait);
>  
>  	vblank_start = adjusted_mode->crtc_vblank_start;
>  	if (adjusted_mode->flags & DRM_MODE_FLAG_INTERLACE)
> @@ -112,41 +117,46 @@ void intel_pipe_update_start(const struct intel_crtc_state *new_crtc_state)
>  	if (min <= 0 || max <= 0)
>  		return;
>  
> -	if (WARN_ON(drm_crtc_vblank_get(&crtc->base)))
> -		return;
> -
>  	crtc->debug.min_vbl = min;
>  	crtc->debug.max_vbl = max;
>  	trace_i915_pipe_update_start(crtc);
>  
> -	for (;;) {
> -		/*
> -		 * prepare_to_wait() has a memory barrier, which guarantees
> -		 * other CPUs can see the task state update by the time we
> -		 * read the scanline.
> -		 */
> -		prepare_to_wait(wq, &wait, TASK_UNINTERRUPTIBLE);
> +	if (!scanline_in_vblank(crtc, min, max, &scanline)) {

'goto skip_evade' to avoid messing with the loop indentation?

> +		wait_queue_head_t *wq = drm_crtc_vblank_waitqueue(&crtc->base);
> +		long timeout = msecs_to_jiffies_timeout(1);
> +		DEFINE_WAIT(wait);
>  
> -		scanline = intel_get_crtc_scanline(crtc);
> -		if (scanline < min || scanline > max)
> -			break;
> +		if (WARN_ON(drm_crtc_vblank_get(&crtc->base)))
> +			goto skip_evade;
>  
> -		if (timeout <= 0) {
> -			DRM_ERROR("Potential atomic update failure on pipe %c\n",
> -				  pipe_name(crtc->pipe));
> -			break;
> -		}
> +		for (;;) {
> +			/*
> +			 * prepare_to_wait() has a memory barrier, which
> +			 * guarantees other CPUs can see the task state update
> +			 * by the time we read the scanline.
> +			 */
> +			prepare_to_wait(wq, &wait, TASK_UNINTERRUPTIBLE);
>  
> -		local_irq_enable();
> +			if (scanline_in_vblank(crtc, min, max, &scanline))
> +				break;
>  
> -		timeout = schedule_timeout(timeout);
> +			if (timeout <= 0) {
> +				DRM_ERROR("Potential atomic update failure on pipe %c\n",
> +					  pipe_name(crtc->pipe));
> +				break;
> +			}
>  
> -		local_irq_disable();
> -	}
> +			local_irq_enable();
>  
> -	finish_wait(wq, &wait);
> +			timeout = schedule_timeout(timeout);
>  
> -	drm_crtc_vblank_put(&crtc->base);
> +			local_irq_disable();
> +		}
> +
> +		finish_wait(wq, &wait);
> +
> +		drm_crtc_vblank_put(&crtc->base);
> +	}
>  
>  	/*
>  	 * On VLV/CHV DSI the scanline counter would appear to
> @@ -166,6 +176,7 @@ void intel_pipe_update_start(const struct intel_crtc_state *new_crtc_state)
>  	while (need_vlv_dsi_wa && scanline == vblank_start)
>  		scanline = intel_get_crtc_scanline(crtc);

I think we'll need to do the dsi w/a regardless, so the label should be
before it.

>  
> +skip_evade:
>  	crtc->debug.scanline_start = scanline;
>  	crtc->debug.start_vbl_time = ktime_get();
>  	crtc->debug.start_vbl_count = intel_crtc_get_vblank_counter(crtc);
> -- 
> 2.16.1

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx