| Message ID | 1446569697-17425-1-git-send-email-matthew.d.roper@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 03 Nov 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. > > 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@intel.com> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Signed-off-by: Matt Roper <matthew.d.roper@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 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");
On Wed, Nov 04, 2015 at 04:12:33PM +0200, Jani Nikula wrote: > On Tue, 03 Nov 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. > > > > 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@intel.com> > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > Signed-off-by: Matt Roper <matthew.d.roper@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. 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
On Thu, 05 Nov 2015, Matt Roper <matthew.d.roper@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@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@intel.com> >> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >> > Signed-off-by: Matt Roper <matthew.d.roper@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
On Thu, Nov 05, 2015 at 02:55:20PM +0200, Jani Nikula wrote: > On Thu, 05 Nov 2015, Matt Roper <matthew.d.roper@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@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@intel.com> > >> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > >> > Signed-off-by: Matt Roper <matthew.d.roper@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. Sorry this is taking so long to zero in on. When you have some free time would you mind running tag 'forjani/watermark_debug-v1' from https://github.com/mattrope/kernel.git with debug messages turned on? I was a bit confused by your earlier logs because apparently some of our startup-time state dumping was giving misleading/inaccurate information; hopefully with the changes here, I'll get more useful debug information and better understand what's leading to the failures on your system. Thanks again for your help. 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
On Thu, Nov 05, 2015 at 02:55:20PM +0200, Jani Nikula wrote: > On Thu, 05 Nov 2015, Matt Roper <matthew.d.roper@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@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@intel.com> > >> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > >> > Signed-off-by: Matt Roper <matthew.d.roper@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
On Thu, Nov 19, 2015 at 08:01:41PM -0800, Matt Roper wrote: > On Thu, Nov 05, 2015 at 02:55:20PM +0200, Jani Nikula wrote: > > On Thu, 05 Nov 2015, Matt Roper <matthew.d.roper@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@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@intel.com> > > >> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > >> > Signed-off-by: Matt Roper <matthew.d.roper@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. This one. We do clear crtc_state->plane_mask already, but we fail to disable the hw plane and we also don't reset plane_state->visble. For added chaos the plane readout/sanitize code is splattered over at least intel_find_initial_plane_obj, readout_plane_state and intel_modeset_gem_init. It would be really great if we can at least somewhat consolidate this. -Daniel > > 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 > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Thu, Nov 19, 2015 at 08:01:41PM -0800, Matt Roper wrote: > On Thu, Nov 05, 2015 at 02:55:20PM +0200, Jani Nikula wrote: > > On Thu, 05 Nov 2015, Matt Roper <matthew.d.roper@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@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@intel.com> > > >> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > >> > Signed-off-by: Matt Roper <matthew.d.roper@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), I would just revert the patch that added that restriction. IMO it makes no sense as long as FBC remains disabled by default. And even worse it doesn't even check that FBC is even supported by the platform. We're just wasting memory now. > 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. And yes, option 3 is what I also think we should do.
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");
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@intel.com> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Signed-off-by: Matt Roper <matthew.d.roper@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. 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(-)