Re: [PATCH 2/2] drm/i915: Use the active wm config for merging on ILK-BDW

[Matt Roper <matthew.d.roper@intel.com>]

On Thu, Jan 14, 2016 at 02:53:35PM +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> ilk_program_watermarks() is supposed to merge the active watermarks from
> all pipes. Thus we need to use the active config too instead of some
> precomputed stuff.

So to clarify, the bug you're fixing here would be if we have racing
commits that operate on disjoint sets of CRTC's; in that case the second
one that actually gets into wm_mutex will fail to see the config changes
made by the first commit, right?

Seems like we could go ahead and remove dev_priv->wm.config (and
calc_watermark_data() that builds it) since it's not actually doing us
any good.  Although it's probably fine to hold that off to a subsequent
patch.

Both of your patches are

Reviewed-by: Matt Roper <matthew.d.roper@intel.com>

CI results do report SKL failures, but those are clearly bogus since SKL
doesn't even run any of the functions that you're changing in these two
patches; as you noted, it looks more like the machine had some kind of
bizarre hardware failure that was unrelated to the patchset.  The other
results look clean.  Given that, I've gone ahead and pushed your patches
to dinq.  Thanks!


Matt


> 
> Fixes: aa363136866c ("drm/i915: Calculate watermark configuration during atomic check (v2)")
> Cc: Matt Roper <matthew.d.roper@intel.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 32 ++++++++++++++++++++++++++------
>  1 file changed, 26 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index e9f4e6e7b736..f44a961183d7 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3680,23 +3680,43 @@ static void skl_update_wm(struct drm_crtc *crtc)
>  	dev_priv->wm.skl_hw = *results;
>  }
>  
> +static void ilk_compute_wm_config(struct drm_device *dev,
> +				  struct intel_wm_config *config)
> +{
> +	struct intel_crtc *crtc;
> +
> +	/* Compute the currently _active_ config */
> +	for_each_intel_crtc(dev, crtc) {
> +		const struct intel_pipe_wm *wm = &crtc->wm.active.ilk;
> +
> +		if (!wm->pipe_enabled)
> +			continue;
> +
> +		config->sprites_enabled |= wm->sprites_enabled;
> +		config->sprites_scaled |= wm->sprites_scaled;
> +		config->num_pipes_active++;
> +	}
> +}
> +
>  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 = &dev_priv->wm.config;
> +	struct intel_wm_config config = {};
>  	struct ilk_wm_values results = {};
>  	enum intel_ddb_partitioning partitioning;
>  
> -	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_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);
>  
>  	/* 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 {
> -- 
> 2.4.10
> 

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx