| Message ID | 1443135200-9870-15-git-send-email-matthew.d.roper@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, 25 Sep 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. > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Signed-off-by: Matt Roper <matthew.d.roper@intel.com> Bisect tells me this patch, i.e. commit c103f2b83dfa3b6fb6b5819ae0362fee6cf4b242 Author: Matt Roper <matthew.d.roper@intel.com> 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@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Thu, Oct 01, 2015 at 04:58:38PM +0300, Jani Nikula wrote: > On Fri, 25 Sep 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. > > > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > Signed-off-by: Matt Roper <matthew.d.roper@intel.com> > > Bisect tells me this patch, i.e. > > commit c103f2b83dfa3b6fb6b5819ae0362fee6cf4b242 > Author: Matt Roper <matthew.d.roper@intel.com> > 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. Done, plus the next patch due to conflicts. Matt, I'll reapply them as soon as we have a fix for this boot issue. -Daniel > > 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@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Jani Nikula, Intel Open Source Technology Center
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");
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@linux.intel.com> Signed-off-by: Matt Roper <matthew.d.roper@intel.com> --- 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(-)