Re: [PATCH 10/13] drm/i915: Calculate watermark configuration during atomic check

[Ander Conselvan De Oliveira <conselvan2@gmail.com>]

On Thu, 2015-08-20 at 18:12 -0700, Matt Roper wrote:
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h      | 10 ++++++
>  drivers/gpu/drm/i915/intel_display.c | 51 ++++++++++++++++++++++++++--
>  drivers/gpu/drm/i915/intel_drv.h     |  1 +
>  drivers/gpu/drm/i915/intel_pm.c      | 66 +++++++-----------------------------
>  4 files changed, 71 insertions(+), 57 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index c91bab9..ac13cd7 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1686,6 +1686,13 @@ struct i915_execbuffer_params {
>  	struct drm_i915_gem_request     *request;
>  };
>  
> +/* used in computing the new watermarks state */
> +struct intel_wm_config {
> +	unsigned int num_pipes_active;
> +	bool sprites_enabled;
> +	bool sprites_scaled;
> +};
> +
>  struct drm_i915_private {
>  	struct drm_device *dev;
>  	struct kmem_cache *objects;
> @@ -1903,6 +1910,9 @@ struct drm_i915_private {
>  		 */
>  		uint16_t skl_latency[8];
>  
> +		/* Committed wm config */
> +		struct intel_wm_config config;
> +
>  		/*
>  		 * The skl_wm_values structure is a bit too big for stack
>  		 * allocation, so we keep the staging struct where we store
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index c40f025..8e9d87a 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13005,6 +13005,44 @@ static int intel_modeset_checks(struct drm_atomic_state *state)
>  	return 0;
>  }
>  
> +/*
> + * Handle calculation of various watermark data at the end of the atomic check
> + * phase.  The code here should be run after the per-crtc and per-plane 'check'
> + * handlers to ensure that all derived state has been updated.
> + */
> +static void calc_watermark_data(struct drm_atomic_state *state)
> +{
> +	struct drm_device *dev = state->dev;
> +	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
> +	struct drm_crtc *crtc;
> +	struct drm_crtc_state *cstate;
> +	struct drm_plane *plane;
> +	struct drm_plane_state *pstate;
> +
> +	/*
> +	 * Calculate watermark configuration details now that derived
> +	 * plane/crtc state is all properly updated.
> +	 */
> +	drm_for_each_crtc(crtc, dev) {
> +		cstate = drm_atomic_get_existing_crtc_state(state, crtc) ?:
> +			crtc->state;
> +

Did you intend to check crtc->active here?

> +		intel_state->wm_config.num_pipes_active++;
> +	}
> +	drm_for_each_legacy_plane(plane, dev) {
> +		pstate = drm_atomic_get_existing_plane_state(state, plane) ?:
> +			plane->state;
> +
> +		if (!to_intel_plane_state(pstate)->visible)
> +			continue;

If I understand correctly, it is possible for a plane on an inactive crtc to have visible = true. In
that case, the result here would be different than the function this replaces, which counts only
planes on active crtcs.


Ander

> +
> +		intel_state->wm_config.sprites_enabled = true;
> +		if (pstate->crtc_w != pstate->src_w >> 16 ||
> +		    pstate->crtc_h != pstate->src_h >> 16)
> +			intel_state->wm_config.sprites_scaled = true;
> +	}
> +}
> +
>  /**
>   * intel_atomic_check - validate state object
>   * @dev: drm device
> @@ -13013,6 +13051,7 @@ static int intel_modeset_checks(struct drm_atomic_state *state)
>  static int intel_atomic_check(struct drm_device *dev,
>  			      struct drm_atomic_state *state)
>  {
> +	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
>  	struct drm_crtc *crtc;
>  	struct drm_crtc_state *crtc_state;
>  	int ret, i;
> @@ -13076,10 +13115,15 @@ static int intel_atomic_check(struct drm_device *dev,
>  		if (ret)
>  			return ret;
>  	} else
> -		to_intel_atomic_state(state)->cdclk =
> -			to_i915(state->dev)->cdclk_freq;
> +		intel_state->cdclk = to_i915(state->dev)->cdclk_freq;
>  
> -	return drm_atomic_helper_check_planes(state->dev, state);
> +	ret = drm_atomic_helper_check_planes(state->dev, state);
> +	if (ret)
> +		return ret;
> +
> +	calc_watermark_data(state);
> +
> +	return 0;
>  }
>  
>  /**
> @@ -13119,6 +13163,7 @@ static int intel_atomic_commit(struct drm_device *dev,
>  		return ret;
>  
>  	drm_atomic_helper_swap_state(dev, state);
> +	dev_priv->wm.config = to_intel_atomic_state(state)->wm_config;
>  
>  	for_each_crtc_in_state(state, crtc, crtc_state, i) {
>  		struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index f9cac4b..6ac1010c 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -239,6 +239,7 @@ struct intel_atomic_state {
>  	unsigned int cdclk;
>  	bool dpll_set;
>  	struct intel_shared_dpll_config shared_dpll[I915_NUM_PLLS];
> +	struct intel_wm_config wm_config;
>  };
>  
>  struct intel_plane_state {
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index bc29260..b32d6b0 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -1786,13 +1786,6 @@ struct ilk_wm_maximums {
>  	uint16_t fbc;
>  };
>  
> -/* used in computing the new watermarks state */
> -struct intel_wm_config {
> -	unsigned int num_pipes_active;
> -	bool sprites_enabled;
> -	bool sprites_scaled;
> -};
> -
>  /*
>   * For both WM_PIPE and WM_LP.
>   * mem_value must be in 0.1us units.
> @@ -2322,26 +2315,6 @@ static void skl_setup_wm_latency(struct drm_device *dev)
>  	intel_print_wm_latency(dev, "Gen9 Plane", dev_priv->wm.skl_latency);
>  }
>  
> -static void ilk_compute_wm_config(struct drm_device *dev,
> -				  struct intel_wm_config *config)
> -{
> -	struct intel_crtc *intel_crtc;
> -
> -	/* Compute the currently _active_ config */
> -	for_each_intel_crtc(dev, intel_crtc) {
> -		const struct intel_crtc_state *cstate =
> -			to_intel_crtc_state(intel_crtc->base.state);
> -		const struct intel_pipe_wm *wm = &cstate->wm.active.ilk;
> -
> -		if (!wm->pipe_enabled)
> -			continue;
> -
> -		config->sprites_enabled |= wm->sprites_enabled;
> -		config->sprites_scaled |= wm->sprites_scaled;
> -		config->num_pipes_active++;
> -	}
> -}
> -
>  /* Compute new watermarks for the pipe */
>  static int ilk_compute_pipe_wm(struct drm_crtc *crtc,
>  			       struct drm_atomic_state *state)
> @@ -2983,11 +2956,12 @@ skl_get_total_relative_data_rate(const struct intel_crtc *intel_crtc)
>  
>  static void
>  skl_allocate_pipe_ddb(struct drm_crtc *crtc,
> -		      const struct intel_wm_config *config,
>  		      const struct intel_crtc_state *cstate,
>  		      struct skl_ddb_allocation *ddb /* out */)
>  {
>  	struct drm_device *dev = crtc->dev;
> +	struct drm_i915_private *dev_priv = to_i915(dev);
> +	struct intel_wm_config *config = &dev_priv->wm.config;
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  	struct intel_plane *intel_plane;
>  	enum pipe pipe = intel_crtc->pipe;
> @@ -3157,15 +3131,6 @@ static bool skl_ddb_allocation_changed(const struct skl_ddb_allocation 
> *new_ddb,
>  	return false;
>  }
>  
> -static void skl_compute_wm_global_parameters(struct drm_device *dev,
> -					     struct intel_wm_config *config)
> -{
> -	struct drm_crtc *crtc;
> -
> -	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head)
> -		config->num_pipes_active += to_intel_crtc(crtc)->active;
> -}
> -
>  static bool skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
>  				 struct intel_crtc *intel_crtc,
>  				 struct intel_plane *intel_plane,
> @@ -3571,13 +3536,12 @@ static void skl_flush_wm_values(struct drm_i915_private *dev_priv,
>  }
>  
>  static bool skl_update_pipe_wm(struct drm_crtc *crtc,
> -			       struct intel_wm_config *config,
>  			       struct skl_ddb_allocation *ddb, /* out */
>  			       struct skl_pipe_wm *pipe_wm /* out */)
>  {
>  	struct intel_crtc_state *cstate = to_intel_crtc_state(crtc->state);
>  
> -	skl_allocate_pipe_ddb(crtc, config, cstate, ddb);
> +	skl_allocate_pipe_ddb(crtc, cstate, ddb);
>  	skl_compute_pipe_wm(crtc, ddb, cstate, pipe_wm);
>  
>  	if (!memcmp(&cstate->wm.active.skl, pipe_wm, sizeof(*pipe_wm)))
> @@ -3590,7 +3554,6 @@ static bool skl_update_pipe_wm(struct drm_crtc *crtc,
>  
>  static void skl_update_other_pipe_wm(struct drm_device *dev,
>  				     struct drm_crtc *crtc,
> -				     struct intel_wm_config *config,
>  				     struct skl_wm_values *r)
>  {
>  	struct intel_crtc *intel_crtc;
> @@ -3620,7 +3583,7 @@ static void skl_update_other_pipe_wm(struct drm_device *dev,
>  		if (!intel_crtc->active)
>  			continue;
>  
> -		wm_changed = skl_update_pipe_wm(&intel_crtc->base, config,
> +		wm_changed = skl_update_pipe_wm(&intel_crtc->base,
>  						&r->ddb, &pipe_wm);
>  
>  		/*
> @@ -3642,19 +3605,16 @@ static void skl_update_wm(struct drm_crtc *crtc)
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct skl_wm_values *results = &dev_priv->wm.skl_results;
>  	struct skl_pipe_wm pipe_wm = {};
> -	struct intel_wm_config config = {};
>  
>  	memset(results, 0, sizeof(*results));
>  
> -	skl_compute_wm_global_parameters(dev, &config);
> -
> -	if (!skl_update_pipe_wm(crtc, &config, &results->ddb, &pipe_wm))
> +	if (!skl_update_pipe_wm(crtc, &results->ddb, &pipe_wm))
>  		return;
>  
>  	skl_compute_wm_results(dev, &pipe_wm, results, intel_crtc);
>  	results->dirty[intel_crtc->pipe] = true;
>  
> -	skl_update_other_pipe_wm(dev, crtc, &config, results);
> +	skl_update_other_pipe_wm(dev, crtc, results);
>  	skl_write_wm_values(dev_priv, results);
>  	skl_flush_wm_values(dev_priv, results);
>  
> @@ -3667,20 +3627,18 @@ static void ilk_program_watermarks(struct drm_i915_private *dev_priv)
>  	struct drm_device *dev = dev_priv->dev;
>  	struct intel_pipe_wm lp_wm_1_2 = {}, lp_wm_5_6 = {}, *best_lp_wm;
>  	struct ilk_wm_maximums max;
> -	struct intel_wm_config config = {};
> +	struct intel_wm_config *config = &dev_priv->wm.config;
>  	struct ilk_wm_values results = {};
>  	enum intel_ddb_partitioning partitioning;
>  
> -	ilk_compute_wm_config(dev, &config);
> -
> -	ilk_compute_wm_maximums(dev, 1, &config, INTEL_DDB_PART_1_2, &max);
> -	ilk_wm_merge(dev, &config, &max, &lp_wm_1_2);
> +	ilk_compute_wm_maximums(dev, 1, config, INTEL_DDB_PART_1_2, &max);
> +	ilk_wm_merge(dev, config, &max, &lp_wm_1_2);
>  
>  	/* 5/6 split only in single pipe config on IVB+ */
>  	if (INTEL_INFO(dev)->gen >= 7 &&
> -	    config.num_pipes_active == 1 && config.sprites_enabled) {
> -		ilk_compute_wm_maximums(dev, 1, &config, INTEL_DDB_PART_5_6, &max);
> -		ilk_wm_merge(dev, &config, &max, &lp_wm_5_6);
> +	    config->num_pipes_active == 1 && config->sprites_enabled) {
> +		ilk_compute_wm_maximums(dev, 1, config, INTEL_DDB_PART_5_6, &max);
> +		ilk_wm_merge(dev, config, &max, &lp_wm_5_6);
>  
>  		best_lp_wm = ilk_find_best_result(dev, &lp_wm_1_2, &lp_wm_5_6);
>  	} else {
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx