On Fri, 25 Sep 2015, Matt Roper <matthew.d.roper@xxxxxxxxx> 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.
>
> Cc: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
> Signed-off-by: Matt Roper <matthew.d.roper@xxxxxxxxx>
Bisect tells me this patch, i.e.
commit c103f2b83dfa3b6fb6b5819ae0362fee6cf4b242
Author: Matt Roper <matthew.d.roper@xxxxxxxxx>
Date: Thu Sep 24 15:53:19 2015 -0700
drm/i915: Sanitize watermarks after hardware state readout
gives me black screen on a BDW NUC pciid=0x1616.
Daniel, please drop it.
BR,
Jani.
> ---
> Maarten, does this solve the problem you were seeing on Ironlake? You
> indicated that your firmware had sprite watermarks programmed even though
> sprites themselves were off and I don't have any kind of system that can
> reproduce that setup. I'm hoping this will patch will do the trick.
>
> drivers/gpu/drm/i915/i915_drv.h | 1 +
> drivers/gpu/drm/i915/intel_display.c | 51 ++++++++++++++++++++++++++++++++++++
> drivers/gpu/drm/i915/intel_pm.c | 14 +++++-----
> 3 files changed, 60 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 8b7c8f9..a9bac1e 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 e2a0777..0c3783c 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -15310,6 +15310,54 @@ static void intel_modeset_readout_hw_state(struct drm_device *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);
> +}
> +
> /* Scan out the current hw modeset state,
> * and sanitizes it to the current state
> */
> @@ -15365,6 +15413,9 @@ intel_modeset_setup_hw_state(struct drm_device *dev)
> modeset_put_power_domains(dev_priv, put_domains);
> }
> intel_display_set_init_power(dev_priv, false);
> +
> + /* Make sure hardware watermarks really match the state we read out */
> + sanitize_watermarks(dev);
> }
>
> void intel_display_resume(struct drm_device *dev)
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index f3652bb..988893e 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3664,15 +3664,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);
>
> @@ -3697,7 +3701,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);
>
> @@ -3715,9 +3718,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,
> @@ -7051,6 +7052,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");
> --
> 2.1.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
