Re: [PATCH v4 08/17] drm/i915: Add distrust_bios_wm flag to dev_priv

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

On Wed, May 11, 2016 at 12:38:36PM +0200, Maarten Lankhorst wrote:
> Op 10-05-16 om 03:21 schreef Matt Roper:
> > SKL-style platforms can't fully trust the watermark/DDB settings
> > programmed by the BIOS and need to do extra sanitization on their first
> > atomic update.  Add a flag to dev_priv that is set during hardware
> > readout and cleared at the end of the first commit.
> >
> > Note that for the somewhat common case where everything is turned off
> > when the driver starts up, we don't need to bother with a recompute...we
> > know exactly what the DDB should be (all zero's) so just setup the DDB
> > directly in that case.
> >
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h      |  7 +++++++
> >  drivers/gpu/drm/i915/intel_display.c |  7 +++++++
> >  drivers/gpu/drm/i915/intel_pm.c      | 15 ++++++++++++++-
> >  3 files changed, 28 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index a67462a..27aaaca 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -1993,6 +1993,13 @@ struct drm_i915_private {
> >  		 * cstate->wm.need_postvbl_update.
> >  		 */
> >  		struct mutex wm_mutex;
> > +
> > +		/*
> > +		 * Set during HW readout of watermarks/DDB.  Some platforms
> > +		 * need to know when we're still using BIOS-provided values
> > +		 * (which we don't fully trust).
> > +		 */
> > +		bool distrust_bios_wm;
> >  	} wm;
> >  
> >  	struct i915_runtime_pm pm;
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 7acdfa3..3028bf4 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -13703,6 +13703,13 @@ static int intel_atomic_commit(struct drm_device *dev,
> >  			dev_priv->display.optimize_watermarks(intel_cstate);
> >  	}
> >  
> > +	/*
> > +	 * We've now finished at least one atomic commit following hw readout,
> > +	 * so any platforms that needed to do extra sanitization of watermarks
> > +	 * should have done it by now.
> > +	 */
> > +	dev_priv->wm.distrust_bios_wm = false;
> I guess at this point atomic wm commit doesn't work yet?
> 
> When it does it should be put after swap_state, or kill it and rely on skip_intermediate_wm. :)

Yeah, moving it to swap_state might be a more natural location.  I'll
update the patch to do that.

Since we don't need two-step watermark programming on gen9, I haven't
touched the commit side of watermarks here, just the check/compute side.
I think there's some improvements that could be done around the whole
DDB write dance that gets done on gen9, but I haven't had time to really
think carefully about that yet.  If I make some changes there in the
future, then I'll probably switch the commit of watermarks from the old
update_wm entrypoint to the atomic entrypoints and that will let the
same sanitization that runs on ILK-style platforms to just do a dummy
commit to setup the DDB properly.

> >  	for_each_crtc_in_state(state, crtc, old_crtc_state, i) {
> >  		intel_post_plane_update(to_intel_crtc_state(old_crtc_state));
> >  
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index 90ba05c..e3877dc 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -4021,11 +4021,24 @@ void skl_wm_get_hw_state(struct drm_device *dev)
> >  	struct skl_ddb_allocation *ddb = &dev_priv->wm.skl_hw.ddb;
> >  	struct drm_crtc *crtc;
> >  	struct intel_crtc *intel_crtc;
> > +	bool any_on = false;
> >  
> >  	skl_ddb_get_hw_state(dev_priv, ddb);
> > -	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head)
> > +	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
> >  		skl_pipe_wm_get_hw_state(crtc);
> >  
> > +		if (crtc->state->active)
> > +			any_on = true;
> > +	}
> > +
> dev_priv->active_crtcs?

Hmm, you're right.  For some reason I had convinced myself that it
wasn't initialized yet at this point, but it looks like it should be
(intel_modeset_readout_hw_state has already run).  I'll switch over to
just use that.  Thanks.


Matt

> 
> > +	if (any_on) {
> > +		/* Fully recompute DDB on first atomic commit */
> > +		dev_priv->wm.distrust_bios_wm = true;
> > +	} else {
> > +		/* Easy/common case; just sanitize DDB now if everything off */
> > +		memset(ddb, 0, sizeof(*ddb));
> > +	}
> > +
> >  	/* Calculate plane data rates */
> >  	for_each_intel_crtc(dev, intel_crtc) {
> >  		struct intel_crtc_state *cstate = intel_crtc->config;
> 
> 

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