[Intel-gfx] [PATCH 3/4] drm/i915: Sanitize watermarks after hardware state readout (v2)
Matt Roper
matthew.d.roper at intel.com
Thu Nov 19 20:01:41 PST 2015
On Thu, Nov 05, 2015 at 02:55:20PM +0200, Jani Nikula wrote:
> On Thu, 05 Nov 2015, Matt Roper <matthew.d.roper at 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 at 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 at intel.com>
> >> > Cc: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> >> > Signed-off-by: Matt Roper <matthew.d.roper at 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.
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
More information about the Intel-gfx
mailing list