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.ro...@intel.com>
> > Reviewed-by: Maarten Lankhorst <maarten.lankho...@linux.intel.com>
> > ---
> > 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.lankho...@linux.intel.com>
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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
