Re: [PATCH 03/11] drm/i915: Kill off intel_crtc->atomic.wait_vblank.

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

On Thu, Oct 22, 2015 at 01:56:28PM +0200, Maarten Lankhorst wrote:
> By handling this after the atomic helper waits for vblanks there will
> be one less wait for vblank in the atomic path.
> 
> Also get rid of the double wait_for_vblank on broadwell, looks like
> it's a bug doing the vblank wait twice.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 28 +++-------------------------
>  drivers/gpu/drm/i915/intel_drv.h     |  1 -
>  2 files changed, 3 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 051a1e2b1c55..b8e1a5471bed 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4672,14 +4672,6 @@ intel_post_enable_primary(struct drm_crtc *crtc)
>  	int pipe = intel_crtc->pipe;
>  
>  	/*
> -	 * BDW signals flip done immediately if the plane
> -	 * is disabled, even if the plane enable is already
> -	 * armed to occur at the next vblank :(
> -	 */
> -	if (IS_BROADWELL(dev))
> -		intel_wait_for_vblank(dev, pipe);

The right fix for this crap is to not use the flip done interrupt. But
apparently no one wants to fix that.

So now I'm worried that we now depend on the atomic helper doing
synchornous vblank waits here. Are we expecting those vblank waits to
remain there? I would have expected to get rid of all synchronois vblank
waits in plane codepaths (apart from ones for hw workarounds).

Also does the helper actually wait for vblanks when the plane gets
enabled w/o the framebuffer actually changing?

> -
> -	/*
>  	 * FIXME IPS should be fine as long as one plane is
>  	 * enabled, but in practice it seems to have problems
>  	 * when going from primary only to sprite only and vice
> @@ -4760,9 +4752,6 @@ static void intel_post_plane_update(struct intel_crtc *crtc)
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct drm_plane *plane;
>  
> -	if (atomic->wait_vblank)
> -		intel_wait_for_vblank(dev, crtc->pipe);
> -
>  	intel_frontbuffer_flip(dev, atomic->fb_bits);
>  
>  	if (atomic->disable_cxsr)
> @@ -11661,15 +11650,12 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
>  		if (plane->type != DRM_PLANE_TYPE_CURSOR) {
>  			intel_crtc->atomic.disable_cxsr = true;
>  			/* to potentially re-enable cxsr */
> -			intel_crtc->atomic.wait_vblank = true;

What's there to make sure cxsr doesn't get enabled/disabled at the wrong
time now? I guess this is the same question as for the BDW w/a, ie. does
the plane getting enabled/disabled (or rather becoming
visible/invisible) always imply a vblank wait?

>  			intel_crtc->atomic.update_wm_post = true;
>  		}
>  	} else if (turn_off) {
>  		intel_crtc->atomic.update_wm_post = true;
>  		/* must disable cxsr around plane enable/disable */
>  		if (plane->type != DRM_PLANE_TYPE_CURSOR) {
> -			if (is_crtc_enabled)
> -				intel_crtc->atomic.wait_vblank = true;
>  			intel_crtc->atomic.disable_cxsr = true;
>  		}
>  	} else if (intel_wm_need_update(plane, plane_state)) {
> @@ -11716,21 +11702,12 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
>  		    plane_state->rotation != BIT(DRM_ROTATE_0))
>  			intel_crtc->atomic.disable_fbc = true;
>  
> -		/*
> -		 * BDW signals flip done immediately if the plane
> -		 * is disabled, even if the plane enable is already
> -		 * armed to occur at the next vblank :(
> -		 */
> -		if (turn_on && IS_BROADWELL(dev))
> -			intel_crtc->atomic.wait_vblank = true;
> -
>  		intel_crtc->atomic.update_fbc |= visible || mode_changed;
>  		break;
>  	case DRM_PLANE_TYPE_CURSOR:
>  		break;
>  	case DRM_PLANE_TYPE_OVERLAY:
>  		if (turn_off && !mode_changed) {
> -			intel_crtc->atomic.wait_vblank = true;
>  			intel_crtc->atomic.update_sprite_watermarks |=
>  				1 << i;
>  		}
> @@ -13271,14 +13248,15 @@ static int intel_atomic_commit(struct drm_device *dev,
>  
>  		if (put_domains)
>  			modeset_put_power_domains(dev_priv, put_domains);
> -
> -		intel_post_plane_update(intel_crtc);
>  	}
>  
>  	/* FIXME: add subpixel order */
>  
>  	drm_atomic_helper_wait_for_vblanks(dev, state);
>  
> +	for_each_crtc_in_state(state, crtc, crtc_state, i)
> +		intel_post_plane_update(to_intel_crtc(crtc));
> +
>  	mutex_lock(&dev->struct_mutex);
>  	drm_atomic_helper_cleanup_planes(dev, state);
>  	mutex_unlock(&dev->struct_mutex);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 54a2c0da2ece..4b6bb1adfddd 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -522,7 +522,6 @@ struct intel_crtc_atomic_commit {
>  
>  	/* Sleepable operations to perform after commit */
>  	unsigned fb_bits;
> -	bool wait_vblank;
>  	bool update_fbc;
>  	bool post_enable_primary;
>  	unsigned update_sprite_watermarks;
> -- 
> 2.1.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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