Re: [PATCH 09/13] drm/i915: Calculate ILK-style watermarks during atomic check (v2)

From: Ander Conselvan De Oliveira <conselvan2@gmail.com>
To: Matt Roper <matthew.d.roper@intel.com>, intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 09/13] drm/i915: Calculate ILK-style watermarks during atomic check (v2)
Date: Fri, 28 Aug 2015 15:53:18 +0300	[thread overview]
Message-ID: <1440766398.5933.4.camel@gmail.com> (raw)
In-Reply-To: <1440119524-13867-10-git-send-email-matthew.d.roper@intel.com>

On Thu, 2015-08-20 at 18:12 -0700, Matt Roper wrote:
> Calculate pipe watermarks during atomic calculation phase, based on the
> contents of the atomic transaction's state structure.  We still program
> the watermarks at the same time we did before, but the computation now
> happens much earlier.
> 
> While this patch isn't too exciting by itself, it paves the way for
> future patches.  The eventual goal (which will be realized in future
> patches in this series) is to calculate multiple sets up watermark
> values up front, and then program them at different times (pre- vs
> post-vblank) on the platforms that need a two-step watermark update.
> 
> While we're at it, s/intel_compute_pipe_wm/ilk_compute_pipe_wm/ since
> this function only applies to ILK-style watermarks and we have a
> completely different function for SKL-style watermarks.
> 
> Note that the original code had a memcmp() in ilk_update_wm() to avoid
> calling ilk_program_watermarks() if the watermarks hadn't changed.  This
> memcmp vanishes here, which means we may do some unnecessary result
> generation and merging in cases where watermarks didn't change, but the
> lower-level function ilk_write_wm_values already makes sure that we
> don't actually try to program the watermark registers again.
> 
> v2: Squash a few commits from the original series together; no longer
>     leave pre-calculated wm's in a separate temporary structure since
>     it's easier to follow the logic if we just cut over to using the
>     pre-calculated values directly.
> 
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h      |  2 +
>  drivers/gpu/drm/i915/intel_display.c |  6 +++
>  drivers/gpu/drm/i915/intel_drv.h     |  2 +
>  drivers/gpu/drm/i915/intel_pm.c      | 87 ++++++++++++++++++------------------
>  4 files changed, 53 insertions(+), 44 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 7c58f38..c91bab9 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -620,6 +620,8 @@ struct drm_i915_display_funcs {
>  			  int target, int refclk,
>  			  struct dpll *match_clock,
>  			  struct dpll *best_clock);
> +	int (*compute_pipe_wm)(struct drm_crtc *crtc,
> +			       struct drm_atomic_state *state);
>  	void (*update_wm)(struct drm_crtc *crtc);
>  	int (*modeset_calc_cdclk)(struct drm_atomic_state *state);
>  	void (*modeset_commit_cdclk)(struct drm_atomic_state *state);
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 6bcc3da..c40f025 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11791,6 +11791,12 @@ static int intel_crtc_atomic_check(struct drm_crtc *crtc,
>  	}
>  
>  	ret = 0;
> +	if (dev_priv->display.compute_pipe_wm) {
> +		ret = dev_priv->display.compute_pipe_wm(crtc, state);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	if (INTEL_INFO(dev)->gen >= 9) {
>  		if (mode_changed)
>  			ret = skl_update_scaler_crtc(pipe_config);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index dfbfba9..f9cac4b 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1427,6 +1427,8 @@ intel_atomic_get_crtc_state(struct drm_atomic_state *state,
>  int intel_atomic_setup_scalers(struct drm_device *dev,
>  	struct intel_crtc *intel_crtc,
>  	struct intel_crtc_state *crtc_state);
> +int intel_check_crtc(struct drm_crtc *crtc,
> +		     struct drm_crtc_state *state);
>  
>  /* intel_atomic_plane.c */
>  struct intel_plane_state *intel_create_plane_state(struct drm_plane *plane);
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 04fc092..bc29260 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -2043,9 +2043,11 @@ static void ilk_compute_wm_level(const struct drm_i915_private *dev_priv,
>  				 const struct intel_crtc *intel_crtc,
>  				 int level,
>  				 struct intel_crtc_state *cstate,
> +				 struct intel_plane_state *pristate,
> +				 struct intel_plane_state *sprstate,
> +				 struct intel_plane_state *curstate,
>  				 struct intel_wm_level *result)
>  {
> -	struct intel_plane *intel_plane;
>  	uint16_t pri_latency = dev_priv->wm.pri_latency[level];
>  	uint16_t spr_latency = dev_priv->wm.spr_latency[level];
>  	uint16_t cur_latency = dev_priv->wm.cur_latency[level];
> @@ -2057,29 +2059,11 @@ static void ilk_compute_wm_level(const struct drm_i915_private *dev_priv,
>  		cur_latency *= 5;
>  	}
>  
> -	for_each_intel_plane_on_crtc(dev_priv->dev, intel_crtc, intel_plane) {
> -		struct intel_plane_state *pstate =
> -			to_intel_plane_state(intel_plane->base.state);
> -
> -		switch (intel_plane->base.type) {
> -		case DRM_PLANE_TYPE_PRIMARY:
> -			result->pri_val = ilk_compute_pri_wm(cstate, pstate,
> -							     pri_latency,
> -							     level);
> -			result->fbc_val = ilk_compute_fbc_wm(cstate, pstate,
> -							     result->pri_val);
> -			break;
> -		case DRM_PLANE_TYPE_OVERLAY:
> -			result->spr_val = ilk_compute_spr_wm(cstate, pstate,
> -							     spr_latency);
> -			break;
> -		case DRM_PLANE_TYPE_CURSOR:
> -			result->cur_val = ilk_compute_cur_wm(cstate, pstate,
> -							     cur_latency);
> -			break;
> -		}
> -	}
> -
> +	result->pri_val = ilk_compute_pri_wm(cstate, pristate,
> +					     pri_latency, level);
> +	result->spr_val = ilk_compute_spr_wm(cstate, sprstate, spr_latency);
> +	result->cur_val = ilk_compute_cur_wm(cstate, curstate, cur_latency);
> +	result->fbc_val = ilk_compute_fbc_wm(cstate, pristate, result->pri_val);
>  	result->enable = true;
>  }
>  
> @@ -2359,15 +2343,20 @@ static void ilk_compute_wm_config(struct drm_device *dev,
>  }
>  
>  /* Compute new watermarks for the pipe */
> -static bool intel_compute_pipe_wm(struct intel_crtc_state *cstate,
> -				  struct intel_pipe_wm *pipe_wm)
> +static int ilk_compute_pipe_wm(struct drm_crtc *crtc,
> +			       struct drm_atomic_state *state)
>  {
> -	struct drm_crtc *crtc = cstate->base.crtc;
> +	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);
> +	struct drm_crtc_state *cs;
> +	struct intel_crtc_state *cstate = NULL;
>  	struct intel_plane *intel_plane;
> +	struct drm_plane_state *ps;
> +	struct intel_plane_state *pristate = NULL;
>  	struct intel_plane_state *sprstate = NULL;
> +	struct intel_plane_state *curstate = NULL;
>  	int level, max_level = ilk_wm_max_level(dev);
>  	/* LP0 watermark maximums depend on this pipe alone */
>  	struct intel_wm_config config = {
> @@ -2375,11 +2364,26 @@ static bool intel_compute_pipe_wm(struct intel_crtc_state *cstate,
>  	};
>  	struct ilk_wm_maximums max;
>  
> +	cs = drm_atomic_get_crtc_state(state, crtc);
> +	if (IS_ERR(cs))
> +		return PTR_ERR(cs);
> +	else
> +		cstate = to_intel_crtc_state(cs);

You could replace this with intel_atomic_get_crtc_state(). Maybe even pass an intel_crtc to this
function instead of a drm one, since there is only one other place besides this where the drm one is
used.

Ander

> +
> +	pipe_wm = &cstate->wm.active.ilk;
> +
>  	for_each_intel_plane_on_crtc(dev, intel_crtc, intel_plane) {
> -		if (intel_plane->base.type == DRM_PLANE_TYPE_OVERLAY) {
> -			sprstate = to_intel_plane_state(intel_plane->base.state);
> -			break;
> -		}
> +		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;
> @@ -2388,7 +2392,7 @@ static bool intel_compute_pipe_wm(struct intel_crtc_state *cstate,
>  		drm_rect_height(&sprstate->dst) != drm_rect_height(&sprstate->src) >> 16;
>  
>  	pipe_wm->pipe_enabled = cstate->base.active;
> -	pipe_wm->sprites_enabled = sprstate->visible;
> +	pipe_wm->sprites_enabled = config.sprites_enabled;
>  	pipe_wm->sprites_scaled = config.sprites_scaled;
>  
>  	/* ILK/SNB: LP2+ watermarks only w/o sprites */
> @@ -2399,7 +2403,8 @@ static bool intel_compute_pipe_wm(struct intel_crtc_state *cstate,
>  	if (config.sprites_scaled)
>  		max_level = 0;
>  
> -	ilk_compute_wm_level(dev_priv, intel_crtc, 0, cstate, &pipe_wm->wm[0]);
> +	ilk_compute_wm_level(dev_priv, intel_crtc, 0, cstate,
> +			     pristate, sprstate, curstate, &pipe_wm->wm[0]);
>  
>  	if (IS_HASWELL(dev) || IS_BROADWELL(dev))
>  		pipe_wm->linetime = hsw_compute_linetime_wm(dev, crtc);
> @@ -2409,14 +2414,15 @@ static bool intel_compute_pipe_wm(struct intel_crtc_state *cstate,
>  
>  	/* At least LP0 must be valid */
>  	if (!ilk_validate_wm_level(0, &max, &pipe_wm->wm[0]))
> -		return false;
> +		return -EINVAL;
>  
>  	ilk_compute_wm_reg_maximums(dev, 1, &max);
>  
>  	for (level = 1; level <= max_level; level++) {
>  		struct intel_wm_level wm = {};
>  
> -		ilk_compute_wm_level(dev_priv, intel_crtc, level, cstate, &wm);
> +		ilk_compute_wm_level(dev_priv, intel_crtc, level, cstate,
> +				     pristate, sprstate, curstate, &wm);
>  
>  		/*
>  		 * Disable any watermark level that exceeds the
> @@ -2429,7 +2435,7 @@ static bool intel_compute_pipe_wm(struct intel_crtc_state *cstate,
>  		pipe_wm->wm[level] = wm;
>  	}
>  
> -	return true;
> +	return 0;
>  }
>  
>  /*
> @@ -3694,7 +3700,6 @@ static void ilk_update_wm(struct drm_crtc *crtc)
>  	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  	struct intel_crtc_state *cstate = to_intel_crtc_state(crtc->state);
> -	struct intel_pipe_wm pipe_wm = {};
>  
>  	/*
>  	 * IVB workaround: must disable low power watermarks for at least
> @@ -3708,13 +3713,6 @@ static void ilk_update_wm(struct drm_crtc *crtc)
>  		intel_wait_for_vblank(crtc->dev, intel_crtc->pipe);
>  	}
>  
> -	intel_compute_pipe_wm(cstate, &pipe_wm);
> -
> -	if (!memcmp(&cstate->wm.active.ilk, &pipe_wm, sizeof(pipe_wm)))
> -		return;
> -
> -	cstate->wm.active.ilk = pipe_wm;
> -
>  	ilk_program_watermarks(dev_priv);
>  }
>  
> @@ -6994,6 +6992,7 @@ void intel_init_pm(struct drm_device *dev)
>  		    (!IS_GEN5(dev) && dev_priv->wm.pri_latency[0] &&
>  		     dev_priv->wm.spr_latency[0] && dev_priv->wm.cur_latency[0])) {
>  			dev_priv->display.update_wm = ilk_update_wm;
> +			dev_priv->display.compute_pipe_wm = ilk_compute_pipe_wm;
>  		} else {
>  			DRM_DEBUG_KMS("Failed to read display plane latency. "
>  				      "Disable CxSR\n");
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx