On Thu, Apr 21, 2016 at 02:19:33PM +0200, Maarten Lankhorst wrote: > Op 20-04-16 om 04:26 schreef Matt Roper: > > Once we move watermark calculation to the atomic check phase, we'll want > > to start rejecting display configurations that exceed out watermark > > limits. At the moment we just assume that there's always a valid set of > > watermarks, even though this may not actually be true. Let's prepare by > > passing return codes up through the call stack in preparation. > > > > Signed-off-by: Matt Roper <matthew.d.roper@xxxxxxxxx> > > Reviewed-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx> > > --- > > drivers/gpu/drm/i915/intel_display.c | 10 ++-- > > drivers/gpu/drm/i915/intel_pm.c | 90 ++++++++++++++++++++++-------------- > > 2 files changed, 61 insertions(+), 39 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > index 80d8f77..7e8fa88 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -13312,7 +13312,7 @@ static int intel_modeset_checks(struct drm_atomic_state *state) > > * phase. The code here should be run after the per-crtc and per-plane 'check' > > * handlers to ensure that all derived state has been updated. > > */ > > -static void calc_watermark_data(struct drm_atomic_state *state) > > +static int calc_watermark_data(struct drm_atomic_state *state) > > { > > struct drm_device *dev = state->dev; > > struct drm_i915_private *dev_priv = to_i915(dev); > > @@ -13348,7 +13348,9 @@ static void calc_watermark_data(struct drm_atomic_state *state) > > > > /* Is there platform-specific watermark information to calculate? */ > > if (dev_priv->display.compute_global_watermarks) > > - dev_priv->display.compute_global_watermarks(state); > > + return dev_priv->display.compute_global_watermarks(state); > > + > > + return 0; > > } > > > > /** > > @@ -13432,9 +13434,7 @@ static int intel_atomic_check(struct drm_device *dev, > > return ret; > > > > intel_fbc_choose_crtc(dev_priv, state); > > - calc_watermark_data(state); > > - > > - return 0; > > + return calc_watermark_data(state); > > } > > > > static int intel_atomic_prepare_commit(struct drm_device *dev, > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > > index 21fde05..f07054a 100644 > > --- a/drivers/gpu/drm/i915/intel_pm.c > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > @@ -3241,13 +3241,14 @@ static bool skl_ddb_allocation_changed(const struct skl_ddb_allocation *new_ddb, > > return false; > > } > > > > -static bool skl_compute_plane_wm(const struct drm_i915_private *dev_priv, > > - struct intel_crtc_state *cstate, > > - struct intel_plane_state *intel_pstate, > > - uint16_t ddb_allocation, > > - int level, > > - uint16_t *out_blocks, /* out */ > > - uint8_t *out_lines /* out */) > > +static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv, > > + struct intel_crtc_state *cstate, > > + struct intel_plane_state *intel_pstate, > > + uint16_t ddb_allocation, > > + int level, > > + uint16_t *out_blocks, /* out */ > > + uint8_t *out_lines, /* out */ > > + bool *enabled /* out */) > > { > > struct drm_plane_state *pstate = &intel_pstate->base; > > struct drm_framebuffer *fb = pstate->fb; > > @@ -3259,8 +3260,10 @@ static bool skl_compute_plane_wm(const struct drm_i915_private *dev_priv, > > uint8_t cpp; > > uint32_t width = 0, height = 0; > > > > - if (latency == 0 || !cstate->base.active || !intel_pstate->visible) > > - return false; > > + if (latency == 0 || !cstate->base.active || !intel_pstate->visible) { > > + *enabled = false; > > + return 0; > > + } > > > > width = drm_rect_width(&intel_pstate->src) >> 16; > > height = drm_rect_height(&intel_pstate->src) >> 16; > > @@ -3321,13 +3324,16 @@ static bool skl_compute_plane_wm(const struct drm_i915_private *dev_priv, > > res_blocks++; > > } > > > > - if (res_blocks >= ddb_allocation || res_lines > 31) > > - return false; > > + if (res_blocks >= ddb_allocation || res_lines > 31) { > > + *enabled = false; > > + return 0; > > + } > > > > *out_blocks = res_blocks; > > *out_lines = res_lines; > > + *enabled = true; > > > > - return true; > > + return 0; > > } > > > > static int > > @@ -3345,6 +3351,7 @@ skl_compute_wm_level(const struct drm_i915_private *dev_priv, > > struct intel_plane_state *intel_pstate; > > uint16_t ddb_blocks; > > enum pipe pipe = intel_crtc->pipe; > > + int ret; > > > > for_each_intel_plane_on_crtc(dev, intel_crtc, intel_plane) { > > int i = skl_wm_plane_id(intel_plane); > > @@ -3374,13 +3381,16 @@ skl_compute_wm_level(const struct drm_i915_private *dev_priv, > > > > ddb_blocks = skl_ddb_entry_size(&ddb->plane[pipe][i]); > > > > - result->plane_en[i] = skl_compute_plane_wm(dev_priv, > > - cstate, > > - intel_pstate, > > - ddb_blocks, > > - level, > > - &result->plane_res_b[i], > > - &result->plane_res_l[i]); > > + ret = skl_compute_plane_wm(dev_priv, > > + cstate, > > + intel_pstate, > > + ddb_blocks, > > + level, > > + &result->plane_res_b[i], > > + &result->plane_res_l[i], > > + &result->plane_en[i]); > > + if (ret) > > + return ret; > > } > > > > return 0; > > @@ -3417,21 +3427,26 @@ static void skl_compute_transition_wm(struct intel_crtc_state *cstate, > > } > > } > > > > -static void skl_build_pipe_wm(struct intel_crtc_state *cstate, > > - struct skl_ddb_allocation *ddb, > > - struct skl_pipe_wm *pipe_wm) > > +static int skl_build_pipe_wm(struct intel_crtc_state *cstate, > > + struct skl_ddb_allocation *ddb, > > + struct skl_pipe_wm *pipe_wm) > > { > > struct drm_device *dev = cstate->base.crtc->dev; > > const struct drm_i915_private *dev_priv = dev->dev_private; > > int level, max_level = ilk_wm_max_level(dev); > > + int ret; > > > > for (level = 0; level <= max_level; level++) { > > - skl_compute_wm_level(dev_priv, ddb, cstate, > > - level, &pipe_wm->wm[level]); > > + ret = skl_compute_wm_level(dev_priv, ddb, cstate, > > + level, &pipe_wm->wm[level]); > > + if (ret) > > + return ret; > > } > > pipe_wm->linetime = skl_compute_linetime_wm(cstate); > > > > skl_compute_transition_wm(cstate, &pipe_wm->trans_wm); > > + > > + return 0; > > } > > > > static void skl_compute_wm_results(struct drm_device *dev, > > @@ -3678,21 +3693,27 @@ static void skl_flush_wm_values(struct drm_i915_private *dev_priv, > > } > > } > > > > -static bool skl_update_pipe_wm(struct drm_crtc_state *cstate, > > - struct skl_ddb_allocation *ddb, /* out */ > > - struct skl_pipe_wm *pipe_wm /* out */) > > +static int skl_update_pipe_wm(struct drm_crtc_state *cstate, > > + struct skl_ddb_allocation *ddb, /* out */ > > + struct skl_pipe_wm *pipe_wm, /* out */ > > + bool *changed /* out */) > > { > > struct intel_crtc *intel_crtc = to_intel_crtc(cstate->crtc); > > struct intel_crtc_state *intel_cstate = to_intel_crtc_state(cstate); > > + int ret; > > > > - skl_build_pipe_wm(intel_cstate, ddb, pipe_wm); > > + ret = skl_build_pipe_wm(intel_cstate, ddb, pipe_wm); > > + if (ret) > > + return ret; > > > > if (!memcmp(&intel_crtc->wm.active.skl, pipe_wm, sizeof(*pipe_wm))) > > - return false; > > + *changed = false; > > + else > > + *changed = true; > > > > intel_crtc->wm.active.skl = *pipe_wm; > > > > - return true; > > + return 0; > > } > > > > static void skl_update_other_pipe_wm(struct drm_device *dev, > > @@ -3725,8 +3746,8 @@ static void skl_update_other_pipe_wm(struct drm_device *dev, > > if (!intel_crtc->active) > > continue; > > > > - wm_changed = skl_update_pipe_wm(intel_crtc->base.state, > > - &r->ddb, &pipe_wm); > > + skl_update_pipe_wm(intel_crtc->base.state, > > + &r->ddb, &pipe_wm, &wm_changed); > > > > /* > > * If we end up re-computing the other pipe WM values, it's > > @@ -3827,14 +3848,15 @@ static void skl_update_wm(struct drm_crtc *crtc) > > struct skl_wm_values *results = &dev_priv->wm.skl_results; > > struct intel_crtc_state *cstate = to_intel_crtc_state(crtc->state); > > struct skl_pipe_wm *pipe_wm = &cstate->wm.skl.optimal; > > - > > + bool wm_changed; > > > > /* Clear all dirty flags */ > > results->dirty_pipes = 0; > > > > skl_clear_wm(results, intel_crtc->pipe); > > > > - if (!skl_update_pipe_wm(crtc->state, &results->ddb, pipe_wm)) > > + skl_update_pipe_wm(crtc->state, &results->ddb, pipe_wm, &wm_changed); > > + if (!wm_changed) > > return; > > > > skl_compute_wm_results(dev, pipe_wm, results, intel_crtc); > With the nitpicks fixed, and with CI happy. > > Reviewed-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx> I'm interpreting this to refer to the entire series (and not just this one patch that already had your r-b and no nitpicks); if that's not what you meant, please let me know. :-) Matt -- Matt Roper Graphics Software Engineer IoTG Platform Enabling & Development Intel Corporation (916) 356-2795 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx