| Message ID | 1459475198-30094-9-git-send-email-matthew.d.roper@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Op 01-04-16 om 03:46 schreef Matt Roper: > Calculate the DDB blocks needed to satisfy the current atomic > transaction at atomic check time. This is a prerequisite to calculating > SKL watermarks during the 'check' phase and rejecting any configurations > that we can't find valid watermarks for. > > Due to the nature of DDB allocation, it's possible for the addition of a > new CRTC to make the watermark configuration already in use on another, > unchanged CRTC become invalid. A change in which CRTC's are active > triggers a recompute of the entire DDB, which unfortunately means we > need to disallow any other atomic commits from racing with such an > update. If the active CRTC's change, we need to grab the lock on all > CRTC's and run all CRTC's through their 'check' handler to recompute and > re-check their per-CRTC DDB allocations. > > Note that with this patch we only compute the DDB allocation but we > don't actually use the computed values during watermark programming yet. > For ease of review/testing/bisecting, we still recompute the DDB at > watermark programming time and just WARN() if it doesn't match the > precomputed values. A future patch will switch over to using the > precomputed values once we're sure they're being properly computed. > > Another clarifying note: DDB allocation itself shouldn't ever fail with > the algorithm we use today (i.e., we have enough DDB blocks on BXT to > support the minimum needs of the worst-case scenario of every pipe/plane > enabled at full size). However the watermarks calculations based on the > DDB may fail and we'll be moving those to the atomic check as well in > future patches. > > Signed-off-by: Matt Roper <matthew.d.roper@intel.com> > --- > drivers/gpu/drm/i915/i915_drv.h | 5 ++++ > drivers/gpu/drm/i915/intel_display.c | 18 ++++++++++++ > drivers/gpu/drm/i915/intel_drv.h | 3 ++ > drivers/gpu/drm/i915/intel_pm.c | 53 ++++++++++++++++++++++++++++++++++++ > 4 files changed, 79 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 79f1974..35cebd4 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -333,6 +333,10 @@ struct i915_hotplug { > #define for_each_intel_crtc(dev, intel_crtc) \ > list_for_each_entry(intel_crtc, &dev->mode_config.crtc_list, base.head) > > +#define for_each_intel_crtc_mask(dev, intel_crtc, crtc_mask) \ > + list_for_each_entry(intel_crtc, &dev->mode_config.crtc_list, base.head) \ > + for_each_if ((crtc_mask) & (1 << drm_crtc_index(&intel_crtc->base))) > + > #define for_each_intel_encoder(dev, intel_encoder) \ > list_for_each_entry(intel_encoder, \ > &(dev)->mode_config.encoder_list, \ > @@ -587,6 +591,7 @@ struct drm_i915_display_funcs { > struct intel_crtc_state *newstate); > void (*initial_watermarks)(struct intel_crtc_state *cstate); > void (*optimize_watermarks)(struct intel_crtc_state *cstate); > + int (*compute_global_watermarks)(struct drm_atomic_state *state); > 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 ab1fc3d..6bf2ede 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -13230,6 +13230,7 @@ static int intel_modeset_checks(struct drm_atomic_state *state) > static void calc_watermark_data(struct drm_atomic_state *state) > { > struct drm_device *dev = state->dev; > + struct drm_i915_private *dev_priv = to_i915(dev); > struct intel_atomic_state *intel_state = to_intel_atomic_state(state); > struct drm_crtc *crtc; > struct drm_crtc_state *cstate; > @@ -13259,6 +13260,10 @@ static void calc_watermark_data(struct drm_atomic_state *state) > pstate->crtc_h != pstate->src_h >> 16) > intel_state->wm_config.sprites_scaled = true; > } > + > + /* Is there platform-specific watermark information to calculate? */ > + if (dev_priv->display.compute_global_watermarks) > + dev_priv->display.compute_global_watermarks(state); > } > > /** > @@ -13631,6 +13636,19 @@ static int intel_atomic_commit(struct drm_device *dev, > modeset_put_power_domains(dev_priv, put_domains[i]); > } > > + /* > + * Temporary sanity check: make sure our pre-computed DDB matches the > + * one we actually wind up programming. > + * > + * Not a great place to put this, but the easiest place we have access > + * to both the pre-computed and final DDB's; we'll be removing this > + * check in the next patch anyway. > + */ > + WARN(IS_GEN9(dev) && > + memcmp(&intel_state->ddb, &dev_priv->wm.skl_results.ddb, > + sizeof(intel_state->ddb)), > + "Pre-computed DDB does not match final DDB!\n"); > + > if (intel_state->modeset) > intel_display_power_put(dev_priv, POWER_DOMAIN_MODESET); > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index 3b9c084..483261c 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -301,6 +301,9 @@ struct intel_atomic_state { > * don't bother calculating intermediate watermarks. > */ > bool skip_intermediate_wm; > + > + /* Gen9+ only */ > + struct skl_ddb_allocation ddb; > }; > > struct intel_plane_state { > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index e0ca2b9..8e283cf 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -3736,6 +3736,58 @@ static void skl_clear_wm(struct skl_wm_values *watermarks, enum pipe pipe) > > } > > +static int > +skl_compute_ddb(struct drm_atomic_state *state) > +{ > + struct drm_device *dev = state->dev; > + struct intel_atomic_state *intel_state = to_intel_atomic_state(state); > + struct intel_crtc *intel_crtc; > + unsigned realloc_pipes = intel_state->active_crtcs; > + int ret; > + > + /* > + * If the modeset changes which CRTC's are active, we need to > + * recompute the DDB allocation for *all* active pipes, even > + * those that weren't otherwise being modified in any way by this > + * atomic commit. Due to the shrinking of the per-pipe allocations > + * when new active CRTC's are added, it's possible for a pipe that > + * we were already using and aren't changing at all here to suddenly > + * become invalid if its DDB needs exceeds its new allocation. > + * > + * Note that if we wind up doing a full DDB recompute, we can't let > + * any other display updates race with this transaction, so we need > + * to grab the lock on *all* CRTC's. > + */ > + if (intel_state->active_crtcs != to_i915(dev)->active_crtcs) > + realloc_pipes = ~0; > + As I noted in the previous patch you're not allowed to look at active_crtcs like this. I would just go with if (intel_state->modeset here), and look only at crtcs part of the state. Not at intel_state->active_crtcs. If the ddb allocation for any crtc part of the state changes then you would add all acive crtcs. Don't be afraid to just lock all, including the disabled crtcs. :) ~Maarten
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 79f1974..35cebd4 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -333,6 +333,10 @@ struct i915_hotplug { #define for_each_intel_crtc(dev, intel_crtc) \ list_for_each_entry(intel_crtc, &dev->mode_config.crtc_list, base.head) +#define for_each_intel_crtc_mask(dev, intel_crtc, crtc_mask) \ + list_for_each_entry(intel_crtc, &dev->mode_config.crtc_list, base.head) \ + for_each_if ((crtc_mask) & (1 << drm_crtc_index(&intel_crtc->base))) + #define for_each_intel_encoder(dev, intel_encoder) \ list_for_each_entry(intel_encoder, \ &(dev)->mode_config.encoder_list, \ @@ -587,6 +591,7 @@ struct drm_i915_display_funcs { struct intel_crtc_state *newstate); void (*initial_watermarks)(struct intel_crtc_state *cstate); void (*optimize_watermarks)(struct intel_crtc_state *cstate); + int (*compute_global_watermarks)(struct drm_atomic_state *state); 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 ab1fc3d..6bf2ede 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -13230,6 +13230,7 @@ static int intel_modeset_checks(struct drm_atomic_state *state) static void calc_watermark_data(struct drm_atomic_state *state) { struct drm_device *dev = state->dev; + struct drm_i915_private *dev_priv = to_i915(dev); struct intel_atomic_state *intel_state = to_intel_atomic_state(state); struct drm_crtc *crtc; struct drm_crtc_state *cstate; @@ -13259,6 +13260,10 @@ static void calc_watermark_data(struct drm_atomic_state *state) pstate->crtc_h != pstate->src_h >> 16) intel_state->wm_config.sprites_scaled = true; } + + /* Is there platform-specific watermark information to calculate? */ + if (dev_priv->display.compute_global_watermarks) + dev_priv->display.compute_global_watermarks(state); } /** @@ -13631,6 +13636,19 @@ static int intel_atomic_commit(struct drm_device *dev, modeset_put_power_domains(dev_priv, put_domains[i]); } + /* + * Temporary sanity check: make sure our pre-computed DDB matches the + * one we actually wind up programming. + * + * Not a great place to put this, but the easiest place we have access + * to both the pre-computed and final DDB's; we'll be removing this + * check in the next patch anyway. + */ + WARN(IS_GEN9(dev) && + memcmp(&intel_state->ddb, &dev_priv->wm.skl_results.ddb, + sizeof(intel_state->ddb)), + "Pre-computed DDB does not match final DDB!\n"); + if (intel_state->modeset) intel_display_power_put(dev_priv, POWER_DOMAIN_MODESET); diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 3b9c084..483261c 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -301,6 +301,9 @@ struct intel_atomic_state { * don't bother calculating intermediate watermarks. */ bool skip_intermediate_wm; + + /* Gen9+ only */ + struct skl_ddb_allocation ddb; }; struct intel_plane_state { diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index e0ca2b9..8e283cf 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -3736,6 +3736,58 @@ static void skl_clear_wm(struct skl_wm_values *watermarks, enum pipe pipe) } +static int +skl_compute_ddb(struct drm_atomic_state *state) +{ + struct drm_device *dev = state->dev; + struct intel_atomic_state *intel_state = to_intel_atomic_state(state); + struct intel_crtc *intel_crtc; + unsigned realloc_pipes = intel_state->active_crtcs; + int ret; + + /* + * If the modeset changes which CRTC's are active, we need to + * recompute the DDB allocation for *all* active pipes, even + * those that weren't otherwise being modified in any way by this + * atomic commit. Due to the shrinking of the per-pipe allocations + * when new active CRTC's are added, it's possible for a pipe that + * we were already using and aren't changing at all here to suddenly + * become invalid if its DDB needs exceeds its new allocation. + * + * Note that if we wind up doing a full DDB recompute, we can't let + * any other display updates race with this transaction, so we need + * to grab the lock on *all* CRTC's. + */ + if (intel_state->active_crtcs != to_i915(dev)->active_crtcs) + realloc_pipes = ~0; + + for_each_intel_crtc_mask(dev, intel_crtc, realloc_pipes) { + struct intel_crtc_state *cstate; + + cstate = intel_atomic_get_crtc_state(state, intel_crtc); + if (IS_ERR(cstate)) + return PTR_ERR(cstate); + + ret = skl_allocate_pipe_ddb(cstate, &intel_state->ddb); + if (ret) + return ret; + } + + return 0; +} + +static int +skl_compute_wm(struct drm_atomic_state *state) +{ + int ret; + + ret = skl_compute_ddb(state); + if (ret) + return ret; + + return 0; +} + static void skl_update_wm(struct drm_crtc *crtc) { struct intel_crtc *intel_crtc = to_intel_crtc(crtc); @@ -7252,6 +7304,7 @@ void intel_init_pm(struct drm_device *dev) if (INTEL_INFO(dev)->gen >= 9) { skl_setup_wm_latency(dev); dev_priv->display.update_wm = skl_update_wm; + dev_priv->display.compute_global_watermarks = skl_compute_wm; } else if (HAS_PCH_SPLIT(dev)) { ilk_setup_wm_latency(dev);
Calculate the DDB blocks needed to satisfy the current atomic transaction at atomic check time. This is a prerequisite to calculating SKL watermarks during the 'check' phase and rejecting any configurations that we can't find valid watermarks for. Due to the nature of DDB allocation, it's possible for the addition of a new CRTC to make the watermark configuration already in use on another, unchanged CRTC become invalid. A change in which CRTC's are active triggers a recompute of the entire DDB, which unfortunately means we need to disallow any other atomic commits from racing with such an update. If the active CRTC's change, we need to grab the lock on all CRTC's and run all CRTC's through their 'check' handler to recompute and re-check their per-CRTC DDB allocations. Note that with this patch we only compute the DDB allocation but we don't actually use the computed values during watermark programming yet. For ease of review/testing/bisecting, we still recompute the DDB at watermark programming time and just WARN() if it doesn't match the precomputed values. A future patch will switch over to using the precomputed values once we're sure they're being properly computed. Another clarifying note: DDB allocation itself shouldn't ever fail with the algorithm we use today (i.e., we have enough DDB blocks on BXT to support the minimum needs of the worst-case scenario of every pipe/plane enabled at full size). However the watermarks calculations based on the DDB may fail and we'll be moving those to the atomic check as well in future patches. Signed-off-by: Matt Roper <matthew.d.roper@intel.com> --- drivers/gpu/drm/i915/i915_drv.h | 5 ++++ drivers/gpu/drm/i915/intel_display.c | 18 ++++++++++++ drivers/gpu/drm/i915/intel_drv.h | 3 ++ drivers/gpu/drm/i915/intel_pm.c | 53 ++++++++++++++++++++++++++++++++++++ 4 files changed, 79 insertions(+)