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

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

On Thu, Jan 14, 2016 at 06:37:32PM -0800, Matt Roper wrote:
> 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?

That, or I suppose cases where the intermediate and optimal watermarks
have a different idea about the sprite enabled/scaled flags. I assume
what it was doing is picking those flags always based on the optimal wms
even when programming the intermedidate wms.

> 
> 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.

IIRC I was a reference to that stuff in the SKL code too, so I didn't bother
trying to kill it without knowing what it was doing. And I was too lazy to
take a deeper look at the code right now.

> 
> 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!

Cheers.

> 
> 
> 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

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