[Intel-gfx] [PATCH 3/4] drm/i915: Sanitize watermarks after hardware state readout (v2)
Jani Nikula
jani.nikula at intel.com
Thu Nov 5 04:55:20 PST 2015
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.
>
>
> 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
More information about the Intel-gfx
mailing list