Re: [PATCH 8/9] drm/i915: Sanitize watermarks after hardware state readout (v2)

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

On Mon, Nov 30, 2015 at 10:50:26AM +0100, Maarten Lankhorst wrote:
> Op 25-11-15 om 17:48 schreef Matt Roper:
> > Although we can do a good job of reading out hardware state, the
> > graphics firmware may have programmed the watermarks in a creative way
> > that doesn't match how i915 would have chosen to program them.  We
> > shouldn't trust the firmware's watermark programming, but should rather
> > re-calculate how we think WM's should be programmed and then shove those
> > values into the hardware.
> >
> > We can do this pretty easily by creating a dummy top-level state,
> > running it through the check process to calculate all the values, and
> > then just programming the watermarks for each CRTC.
> >
> > v2:  Move watermark sanitization after our BIOS fb reconstruction; the
> >      watermark calculations that we do here need to look at pstate->fb,
> >      which isn't setup yet in intel_modeset_setup_hw_state(), even
> >      though we have an enabled & visible plane.
> >
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> > ---
> >  drivers/gpu/drm/drm_atomic_helper.c  |  1 +
> >  drivers/gpu/drm/i915/i915_drv.h      |  1 +
> >  drivers/gpu/drm/i915/intel_display.c | 58 ++++++++++++++++++++++++++++++++++++
> >  drivers/gpu/drm/i915/intel_pm.c      | 14 +++++----
> >  4 files changed, 68 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> > index 3731a26..8a98e0c 100644
> > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > @@ -2478,6 +2478,7 @@ drm_atomic_helper_duplicate_state(struct drm_device *dev,
> >  		}
> >  	}
> >  
> > +	drm_modeset_lock(&dev->mode_config.connection_mutex, ctx);
> >  	drm_for_each_connector(conn, dev) {
> >  		struct drm_connector_state *conn_state;
> Seems this hunk doesn't belong to this patch?
> Also that locking is already taken care of by drm_atomic_get_connector_state,
> and it doesn't check for drm_modeset_lock returning an error code..

This should be a separate patch, but it is indeed necessary.  We hit the
locking error before we call drm_atomic_get_connector_state...the loop
iterator itself (drm_for_each_connector) will throw a warning if we're
not already holding connection_mutex as of

        commit 7a3f3d6667f5f9ffd1517f6b21d64bbf5312042c
        Author: Daniel Vetter <daniel.vetter@ffwll.ch>
        Date:   Thu Jul 9 23:44:28 2015 +0200

            drm: Check locking in drm_for_each_connector

I'll pull this out and send it separately.


Matt

> 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 11ae5a5..5172604 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -630,6 +630,7 @@ struct drm_i915_display_funcs {
> >  			  struct dpll *best_clock);
> >  	int (*compute_pipe_wm)(struct intel_crtc *crtc,
> >  			       struct drm_atomic_state *state);
> > +	void (*program_watermarks)(struct intel_crtc_state *cstate);
> >  	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 00e4c37..eb52afa 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -15120,6 +15120,57 @@ void intel_modeset_init_hw(struct drm_device *dev)
> >  	intel_enable_gt_powersave(dev);
> >  }
> >  
> > +/*
> > + * Calculate what we think the watermarks should be for the state we've read
> > + * out of the hardware and then immediately program those watermarks so that
> > + * we ensure the hardware settings match our internal state.
> > + */
> > +static void sanitize_watermarks(struct drm_device *dev)
> > +{
> > +	struct drm_i915_private *dev_priv = to_i915(dev);
> > +	struct drm_atomic_state *state;
> > +	struct drm_crtc *crtc;
> > +	struct drm_crtc_state *cstate;
> > +	struct drm_modeset_acquire_ctx ctx;
> > +	int ret;
> > +	int i;
> > +
> > +	/* Only supported on platforms that use atomic watermark design */
> > +	if (!dev_priv->display.program_watermarks)
> > +		return;
> > +
> > +	/*
> > +	 * Calculate what we think WM's should be by creating a dummy state and
> > +	 * running it through the atomic check code.
> > +	 */
> > +	drm_modeset_acquire_init(&ctx, 0);
> > +	state = drm_atomic_helper_duplicate_state(dev, &ctx);
> > +	if (WARN_ON(IS_ERR(state)))
> > +		return;
> > +
> > +	ret = intel_atomic_check(dev, state);
> > +	if (ret) {
> > +		/*
> > +		 * Just give up and leave watermarks untouched if we get an
> > +		 * error back from 'check'
> > +		 */
> > +		DRM_DEBUG_KMS("Could not determine valid watermarks for inherited state\n");
> > +		return;
> > +	}
> > +
> > +	/* Write calculated watermark values back */
> > +	to_i915(dev)->wm.config = to_intel_atomic_state(state)->wm_config;
> > +	for_each_crtc_in_state(state, crtc, cstate, i) {
> > +		struct intel_crtc_state *cs = to_intel_crtc_state(cstate);
> > +
> > +		dev_priv->display.program_watermarks(cs);
> > +	}
> > +
> > +	drm_atomic_state_free(state);
> > +	drm_modeset_drop_locks(&ctx);
> > +	drm_modeset_acquire_fini(&ctx);
> > +}
> > +
> >  void intel_modeset_init(struct drm_device *dev)
> >  {
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > @@ -15243,6 +15294,13 @@ void intel_modeset_init(struct drm_device *dev)
> >  		intel_dump_pipe_config(crtc, crtc->config,
> >  				       "[state after init fb reconstruction]");
> >  	}
> > +
> > +	/*
> > +	 * Make sure hardware watermarks really match the state we read out.
> > +	 * Note that we need to do this after reconstructing the BIOS fb's
> > +	 * since the watermark calculation done here will use pstate->fb.
> > +	 */
> > +	sanitize_watermarks(dev);
> >  }
> >  
> >  static void intel_enable_pipe_a(struct drm_device *dev)
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index d4cd5d5..c209a69 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -3610,15 +3610,19 @@ static void skl_update_wm(struct drm_crtc *crtc)
> >  	dev_priv->wm.skl_hw = *results;
> >  }
> >  
> > -static void ilk_program_watermarks(struct drm_i915_private *dev_priv)
> > +static void ilk_program_watermarks(struct intel_crtc_state *cstate)
> >  {
> > -	struct drm_device *dev = dev_priv->dev;
> > +	struct drm_crtc *crtc = cstate->base.crtc;
> > +	struct drm_device *dev = crtc->dev;
> > +	struct drm_i915_private *dev_priv = to_i915(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 ilk_wm_values results = {};
> >  	enum intel_ddb_partitioning partitioning;
> >  
> > +	to_intel_crtc(crtc)->wm.active.ilk = cstate->wm.optimal.ilk;
> > +
> >  	ilk_compute_wm_maximums(dev, 1, config, INTEL_DDB_PART_1_2, &max);
> >  	ilk_wm_merge(dev, config, &max, &lp_wm_1_2);
> >  
> > @@ -3643,7 +3647,6 @@ static void ilk_program_watermarks(struct drm_i915_private *dev_priv)
> >  
> >  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);
> >  
> > @@ -3661,9 +3664,7 @@ static void ilk_update_wm(struct drm_crtc *crtc)
> >  		intel_wait_for_vblank(crtc->dev, intel_crtc->pipe);
> >  	}
> >  
> > -	intel_crtc->wm.active.ilk = cstate->wm.optimal.ilk;
> > -
> > -	ilk_program_watermarks(dev_priv);
> > +	ilk_program_watermarks(cstate);
> >  }
> >  
> >  static void skl_pipe_wm_active_state(uint32_t val,
> > @@ -6971,6 +6972,7 @@ void intel_init_pm(struct drm_device *dev)
> >  		     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;
> > +			dev_priv->display.program_watermarks = ilk_program_watermarks;
> >  		} else {
> >  			DRM_DEBUG_KMS("Failed to read display plane latency. "
> >  				      "Disable CxSR\n");
> 

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