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

[Daniel Vetter <daniel@ffwll.ch>]

On Thu, Nov 19, 2015 at 08:01:41PM -0800, Matt Roper wrote:
> On Thu, Nov 05, 2015 at 02:55:20PM +0200, Jani Nikula wrote:
> > On Thu, 05 Nov 2015, Matt Roper <matthew.d.roper@intel.com> wrote:
> > > On Wed, Nov 04, 2015 at 04:12:33PM +0200, Jani Nikula wrote:
> > >> On Tue, 03 Nov 2015, Matt Roper <matthew.d.roper@intel.com> wrote:
> > >> > 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: Jani Nikula <jani.nikula@intel.com>
> > >> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > >> > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> > >> > ---
> > >> > Jani, based on your logs it looks like the culprit is that we're calculating
> > >> > watermarks at startup time after we've read out preliminary hardware state (so
> > >> > we know the primary plane is enabled & visible), but before we reconstruct the
> > >> > BIOS fb (so pstate->fb is NULL).  I think moving the watermark sanitization
> > >> > later in the process so that we'll have a proper fb setup should hopefully
> > >> > solve the issue.  Can you test this version when you get a chance?  I'll also
> > >> > send a rebased patch #4 since the code movement here means that the previous
> > >> > version won't apply cleanly.
> > >> 
> > >> Sorry Matt, black screen with new versions of patches 3-4. Dmesg at
> > >> http://pastebin.com/3LXZwvWu
> > >
> > > Hmm, okay, looks like we're getting closer (successfully avoided the
> > > divide by zero problem), but I neglected to grab the necessary locks so
> > > the sanitization doesn't actually happen.  Does applying
> > > http://paste.debian.net/322932 on top of the series work any better for
> > > you?  I haven't had time to pull back out an ILK-style system, so that's
> > > only compile-tested at the moment.
> > 
> > Still warns http://pastebin.com/yGtde5X2
> > 
> > BR,
> > Jani.
> 
> Okay, thanks to Jani setting up his machine so that I can debug
> remotely, I think I understand where we're going wrong now.  The missing
> piece of the puzzle is that Jani's display is 3840x2160.  That means the
> BIOS FB we need to inherit is deemed too large by our initial hardware
> readout code (i.e., greater than half of
> dev_priv->gtt.stolen_usable_size), so we don't even bother inheriting
> the FB and pstate->fb stays NULL, even though pstate->visible = true and
> the plane is part of the CRTC's plane_mask.
> 
> Adding Maarten, Ville, and Ander to Cc since I think they may have some
> insights on how best to handle this.  Seems like our options are:
>  * If our watermark code comes up with a NULL FB while pstate->visible,
>    just pretend we're working on a 32-bit framebuffer that matches the
>    CRTC mode dimensions.  We'll then calculate appropriate watermark
>    values, even though we don't have any real information for the FB
>    that the hardware is scanning out of.
>  * Set pstate->visible = false and clear the bit from plane_mask.  I
>    think we'll run into state verification warnings then ("plane A
>    should be disabled but is not") since our HW state and SW state don't
>    match.
>  * Actually turn off the primary plane (hardware-wise) if we fail to
>    inherit the BIOS FB.

This one. We do clear crtc_state->plane_mask already, but we fail to
disable the hw plane and we also don't reset plane_state->visble.

For added chaos the plane readout/sanitize code is splattered over at
least intel_find_initial_plane_obj, readout_plane_state and
intel_modeset_gem_init. It would be really great if we can at least
somewhat consolidate this.
-Daniel

> 
> Seems like the first option may be the simplest.   Thoughts?  Anything
> I'm overlooking?
> 
> 
> Matt
> 
> > 
> > 
> > >
> > >
> > > Matt
> > >
> > >> 
> > >> BR,
> > >> Jani.
> > >> 
> > >> 
> > >> 
> > >> >
> > >> >  drivers/gpu/drm/i915/i915_drv.h      |  1 +
> > >> >  drivers/gpu/drm/i915/intel_display.c | 55 ++++++++++++++++++++++++++++++++++++
> > >> >  drivers/gpu/drm/i915/intel_pm.c      | 14 +++++----
> > >> >  3 files changed, 64 insertions(+), 6 deletions(-)
> > >> >
> > >> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > >> > index 20cd6d8..09807c8 100644
> > >> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > >> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > >> > @@ -629,6 +629,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 7b3cfb6..e289311 100644
> > >> > --- a/drivers/gpu/drm/i915/intel_display.c
> > >> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > >> > @@ -14936,6 +14936,54 @@ 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;
> > >> > +	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.
> > >> > +	 */
> > >> > +	state = drm_atomic_helper_duplicate_state(dev,
> > >> > +						  dev->mode_config.acquire_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);
> > >> > +}
> > >> > +
> > >> >  void intel_modeset_init(struct drm_device *dev)
> > >> >  {
> > >> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > >> > @@ -15059,6 +15107,13 @@ void intel_modeset_init(struct drm_device *dev)
> > >> >  		 */
> > >> >  		intel_find_initial_plane_obj(crtc, &plane_config);
> > >> >  	}
> > >> > +
> > >> > +	/*
> > >> > +	 * 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 180348b..fbcb072 100644
> > >> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > >> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > >> > @@ -3611,15 +3611,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);
> > >> >  
> > >> > @@ -3644,7 +3648,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);
> > >> >  
> > >> > @@ -3662,9 +3665,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,
> > >> > @@ -6972,6 +6973,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");
> > >> 
> > >> -- 
> > >> Jani Nikula, Intel Open Source Technology Center
> > 
> > -- 
> > Jani Nikula, Intel Open Source Technology Center
> 
> -- 
> 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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx