Re: [RFC 12/15] drm/i915: Actually use pre-computer watermark values (!!SQUASHME)

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

On Wed, May 20, 2015 at 07:12:24PM -0700, Matt Roper wrote:
> Rather that just comparing and then throwing away the pipe watermark
> values we precomputed at atomic check time, actually use those values
> when we program watermarks.
> 
> FIXME:  This should be squashed into the previous patch eventually, once
> we're convinced that pre-computed watermarks get the proper values in
> all cases.
> 
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h     |  3 +-
>  drivers/gpu/drm/i915/intel_atomic.c |  4 +-
>  drivers/gpu/drm/i915/intel_drv.h    |  3 --
>  drivers/gpu/drm/i915/intel_pm.c     | 77 +++++++++++--------------------------
>  4 files changed, 24 insertions(+), 63 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 214ce76..d577eba 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -563,8 +563,7 @@ struct drm_i915_display_funcs {
>  			  struct dpll *match_clock,
>  			  struct dpll *best_clock);
>  	int (*compute_pipe_wm)(struct drm_crtc *crtc,
> -			       struct drm_atomic_state *state,
> -			       void *target);
> +			       struct drm_atomic_state *state);
>  	void (*update_wm)(struct drm_crtc *crtc);
>  	void (*update_sprite_wm)(struct drm_plane *plane,
>  				 struct drm_crtc *crtc,
> diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
> index db349dd..5294840 100644
> --- a/drivers/gpu/drm/i915/intel_atomic.c
> +++ b/drivers/gpu/drm/i915/intel_atomic.c
> @@ -435,14 +435,12 @@ intel_check_crtc(struct drm_crtc *crtc,
>  		 struct drm_crtc_state *state)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
> -	struct intel_crtc_state *intel_state = to_intel_crtc_state(state);
>  	int ret;
>  
>  	if (!dev_priv->display.compute_pipe_wm)
>  		return 0;
>  
> -	ret = dev_priv->display.compute_pipe_wm(crtc, state->state,
> -						&intel_state->wm.tmp);
> +	ret = dev_priv->display.compute_pipe_wm(crtc, state->state);
>  
>  	return ret;
>  }
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 6ca3c06..627741a 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -464,9 +464,6 @@ struct intel_crtc_state {
>  	struct intel_crtc_scaler_state scaler_state;
>  
>  	struct {
> -		/* tmp wm */
> -		struct intel_pipe_wm tmp;
> -
>  		/* final, target watermarks for state */
>  		union {
>  			struct intel_pipe_wm ilk;
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 017b184..27337fe 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -2073,10 +2073,9 @@ static void ilk_compute_wm_config(struct drm_device *dev,
>  
>  /* Compute new watermarks for the pipe */
>  static int ilk_compute_pipe_wm(struct drm_crtc *crtc,
> -			       struct drm_atomic_state *state,
> -			       void *target)
> +			       struct drm_atomic_state *state)
>  {
> -	struct intel_pipe_wm *pipe_wm = target;
> +	struct intel_pipe_wm *pipe_wm;
>  	struct drm_device *dev = crtc->dev;
>  	const struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> @@ -2094,47 +2093,26 @@ static int ilk_compute_pipe_wm(struct drm_crtc *crtc,
>  	};
>  	struct ilk_wm_maximums max;
>  
> -	/*
> -	 * FIXME: In an upcoming patch, we'll *only* be calling this from the
> -	 * atomic 'check' codepath and thus will always have a top-level atomic
> -	 * transaction.  However at the moment we still call this in the legacy
> -	 * 'ilk_update_wm' function, which means we need to be able to also
> -	 * operate on already-committed state which has been detached from any
> -	 * top-level transaction.
> -	 *
> -	 * Drop the 'else' block here once we only do atomic-style watermarks.
> -	 */
> -	if (state) {
> -		cs = drm_atomic_get_crtc_state(state, crtc);
> -		if (IS_ERR(cs))
> -			return PTR_ERR(cs);
> -		else
> -			cstate = to_intel_crtc_state(cs);
> -
> -		for_each_intel_plane_of_crtc(intel_crtc, intel_plane) {
> -			ps = drm_atomic_get_plane_state(state,
> -							&intel_plane->base);
> -			if (IS_ERR(ps))
> -				return PTR_ERR(ps);
> -
> -			if (intel_plane->base.type == DRM_PLANE_TYPE_PRIMARY)
> -				pristate = to_intel_plane_state(ps);
> -			else if (intel_plane->base.type == DRM_PLANE_TYPE_OVERLAY)
> -				sprstate = to_intel_plane_state(ps);
> -			else if (intel_plane->base.type == DRM_PLANE_TYPE_CURSOR)
> -				curstate = to_intel_plane_state(ps);
> -		}
> -	} else {
> -		/* Use already-committed state */
> -		cstate = to_intel_crtc_state(crtc->state);
> -		for_each_intel_plane_of_crtc(intel_crtc, intel_plane) {
> -			if (intel_plane->base.type == DRM_PLANE_TYPE_OVERLAY) {
> -				sprstate = to_intel_plane_state(intel_plane->base.state);
> -				break;
> -			}
> -		}
> -		pristate = to_intel_plane_state(crtc->primary->state);
> -		curstate = to_intel_plane_state(crtc->cursor->state);
> +	cs = drm_atomic_get_crtc_state(state, crtc);
> +	if (IS_ERR(cs))
> +		return PTR_ERR(cs);
> +	else
> +		cstate = to_intel_crtc_state(cs);
> +
> +	pipe_wm = &cstate->wm.target.ilk;
> +
> +	for_each_intel_plane_of_crtc(intel_crtc, intel_plane) {
> +		ps = drm_atomic_get_plane_state(state,
> +						&intel_plane->base);
> +		if (IS_ERR(ps))
> +			return PTR_ERR(ps);
> +
> +		if (intel_plane->base.type == DRM_PLANE_TYPE_PRIMARY)
> +			pristate = to_intel_plane_state(ps);
> +		else if (intel_plane->base.type == DRM_PLANE_TYPE_OVERLAY)
> +			sprstate = to_intel_plane_state(ps);
> +		else if (intel_plane->base.type == DRM_PLANE_TYPE_CURSOR)
> +			curstate = to_intel_plane_state(ps);
>  	}
>  
>  	config.sprites_enabled = sprstate->visible;
> @@ -3511,17 +3489,6 @@ static void ilk_program_watermarks(struct drm_i915_private *dev_priv)
>  static void ilk_update_wm(struct drm_crtc *crtc)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
> -	struct intel_crtc_state *cstate = to_intel_crtc_state(crtc->state);
> -	struct intel_pipe_wm pipe_wm = {};
> -
> -	ilk_compute_pipe_wm(crtc, NULL, &pipe_wm);
> -	WARN(memcmp(&cstate->wm.tmp, &pipe_wm, sizeof(pipe_wm)) != 0,
> -	     "Pre-computed atomic watermark values do not match final values!");
> -
> -	if (!memcmp(&cstate->wm.target.ilk, &pipe_wm, sizeof(pipe_wm)))
> -		return;

The memcmp() is there to avoid reprogromaming needlessly. It should
stay.

> -
> -	cstate->wm.target.ilk = pipe_wm;
>  
>  	ilk_program_watermarks(dev_priv);
>  }
> -- 
> 1.8.5.1
> 
> _______________________________________________
> 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