Re: [PATCH v2 13/16] drm/i915/gen9: Propagate watermark calculation failures up the call chain

[Matt Roper <matthew.d.roper@intel.com>]

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@intel.com>
> > Reviewed-by: Maarten Lankhorst <maarten.lankhorst@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.lankhorst@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