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

Commit Message

Ville Syrjala Jan. 14, 2016, 12:53 p.m. UTC

From: Ville Syrjälä <[e-mail]>

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.

Fixes: aa363136866c ("drm/i915: Calculate watermark configuration during atomic check (v2)")
Cc: Matt Roper <[e-mail]>
Signed-off-by: Ville Syrjälä <[e-mail]>
---
 drivers/gpu/drm/i915/intel_pm.c | 32 ++++++++++++++++++++++++++------
 1 file changed, 26 insertions(+), 6 deletions(-)

Comments

Matt Roper Jan. 15, 2016, 2:37 a.m. UTC | #1

On Thu, Jan 14, 2016 at 02:53:35PM +0200, [e-mail] wrote:
> From: Ville Syrjälä <[e-mail]>
> 
> 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 <[e-mail]>

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 <[e-mail]>
> Signed-off-by: Ville Syrjälä <[e-mail]>
> ---
>  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
>

Ville Syrjala Jan. 15, 2016, 7:03 p.m. UTC | #2

On Thu, Jan 14, 2016 at 06:37:32PM -0800, Matt Roper wrote:
> On Thu, Jan 14, 2016 at 02:53:35PM +0200, [e-mail] wrote:
> > From: Ville Syrjälä <[e-mail]>
> > 
> > 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 <[e-mail]>
> 
> 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 <[e-mail]>
> > Signed-off-by: Ville Syrjälä <[e-mail]>
> > ---
> >  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
> [phone]

Patch

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 {