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@xxxxxxxxx> wrote: > > > On Wed, Nov 04, 2015 at 04:12:33PM +0200, Jani Nikula wrote: > > >> On Tue, 03 Nov 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. > > >> > > > >> > 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@xxxxxxxxx> > > >> > Cc: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx> > > >> > Signed-off-by: Matt Roper <matthew.d.roper@xxxxxxxxx> > > >> > --- > > >> > 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@xxxxxxxxxxxxxxxxxxxxx > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
