Re: [PATCH v3 04/11] drm/i915/gen9+: Kill off hw_ddb from intel_crtc.

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

On Tue, Nov 08, 2016 at 01:55:35PM +0100, Maarten Lankhorst wrote:
> This member is only used in skl_update_crtcs now. It's easy to remove it
> by keeping track of which ddb entries in an array, and update them after

I'm having trouble parsing this line...not sure if you have an extra
word or are missing a word.  But I think what you meant is that you're
snapshotting the DDB values at the beginning of this function so that
you'll have a copy of what the 'old' values that were already in the
hardware , then you update that snapshot as you write the DDB for pipe
to the hardware?

> we update the crtc. This removes the last bits of SKL-style watermarks
> kept outside of crtc_state.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 15 ++++++++++++---
>  drivers/gpu/drm/i915/intel_drv.h     | 11 +++--------
>  drivers/gpu/drm/i915/intel_pm.c      | 25 +++++++------------------
>  3 files changed, 22 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 69f9addb29b3..e59adb03933e 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -14280,6 +14280,14 @@ static void skl_update_crtcs(struct drm_atomic_state *state,
>  	unsigned int updated = 0;
>  	bool progress;
>  	enum pipe pipe;
> +	int i;
> +
> +	const struct skl_ddb_entry *entries[I915_MAX_PIPES] = {};

Do we want this to be const?

> +
> +	for_each_crtc_in_state(state, crtc, old_crtc_state, i)
> +		/* ignore allocations for crtc's that have been turned off. */
> +		if (crtc->state->active)
> +			entries[i] = &to_intel_crtc_state(old_crtc_state)->wm.skl.ddb;
>  
>  	/*
>  	 * Whenever the number of active pipes changes, we need to make sure we
> @@ -14288,7 +14296,6 @@ static void skl_update_crtcs(struct drm_atomic_state *state,
>  	 * cause pipe underruns and other bad stuff.
>  	 */
>  	do {
> -		int i;
>  		progress = false;
>  
>  		for_each_crtc_in_state(state, crtc, old_crtc_state, i) {
> @@ -14299,12 +14306,14 @@ static void skl_update_crtcs(struct drm_atomic_state *state,
>  			cstate = to_intel_crtc_state(crtc->state);
>  			pipe = intel_crtc->pipe;
>  
> -			if (updated & cmask || !crtc->state->active)
> +			if (updated & cmask || !cstate->base.active)

This change seems unrelated/unnecessary?  cstate was just set a couple
lines above, so this is effectively just replacing crtc->state with
to_intel_crtc_state(crtc->state)->base.


Matt

>  				continue;
> -			if (skl_ddb_allocation_overlaps(state, intel_crtc))
> +
> +			if (skl_ddb_allocation_overlaps(entries, &cstate->wm.skl.ddb, i))
>  				continue;
>  
>  			updated |= cmask;
> +			entries[i] = &cstate->wm.skl.ddb;
>  
>  			/*
>  			 * If this is an already active pipe, it's DDB changed,
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index d18444097e1c..815e8dae3904 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -728,9 +728,6 @@ struct intel_crtc {
>  		bool cxsr_allowed;
>  	} wm;
>  
> -	/* gen9+: ddb allocation currently being used */
> -	struct skl_ddb_entry hw_ddb;
> -
>  	int scanline_offset;
>  
>  	struct {
> @@ -1738,11 +1735,9 @@ int intel_enable_sagv(struct drm_i915_private *dev_priv);
>  int intel_disable_sagv(struct drm_i915_private *dev_priv);
>  bool skl_wm_level_equals(const struct skl_wm_level *l1,
>  			 const struct skl_wm_level *l2);
> -bool skl_ddb_allocation_equals(const struct skl_ddb_allocation *old,
> -			       const struct skl_ddb_allocation *new,
> -			       enum pipe pipe);
> -bool skl_ddb_allocation_overlaps(struct drm_atomic_state *state,
> -				 struct intel_crtc *intel_crtc);
> +bool skl_ddb_allocation_overlaps(const struct skl_ddb_entry **entries,
> +				 const struct skl_ddb_entry *ddb,
> +				 int ignore);
>  uint32_t ilk_pipe_pixel_rate(const struct intel_crtc_state *pipe_config);
>  bool ilk_disable_lp_wm(struct drm_device *dev);
>  int sanitize_rc6_option(struct drm_i915_private *dev_priv, int enable_rc6);
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index bde6c68eb0db..02f52b52a03d 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3914,25 +3914,16 @@ static inline bool skl_ddb_entries_overlap(const struct skl_ddb_entry *a,
>  	return a->start < b->end && b->start < a->end;
>  }
>  
> -bool skl_ddb_allocation_overlaps(struct drm_atomic_state *state,
> -				 struct intel_crtc *intel_crtc)
> -{
> -	struct drm_crtc *other_crtc;
> -	struct drm_crtc_state *other_cstate;
> -	struct intel_crtc *other_intel_crtc;
> -	const struct skl_ddb_entry *ddb =
> -		&to_intel_crtc_state(intel_crtc->base.state)->wm.skl.ddb;
> +bool skl_ddb_allocation_overlaps(const struct skl_ddb_entry **entries,
> +				 const struct skl_ddb_entry *ddb,
> +				 int ignore)
> +{
>  	int i;
>  
> -	for_each_crtc_in_state(state, other_crtc, other_cstate, i) {
> -		other_intel_crtc = to_intel_crtc(other_crtc);
> -
> -		if (other_intel_crtc == intel_crtc)
> -			continue;
> -
> -		if (skl_ddb_entries_overlap(ddb, &other_intel_crtc->hw_ddb))
> +	for (i = 0; i < I915_MAX_PIPES; i++)
> +		if (i != ignore && entries[i] &&
> +		    skl_ddb_entries_overlap(ddb, entries[i]))
>  			return true;
> -	}
>  
>  	return false;
>  }
> @@ -4242,8 +4233,6 @@ static void skl_initial_wm(struct intel_atomic_state *state,
>  
>  	skl_copy_wm_for_pipe(hw_vals, results, pipe);
>  
> -	intel_crtc->hw_ddb = cstate->wm.skl.ddb;
> -
>  	mutex_unlock(&dev_priv->wm.wm_mutex);
>  }
>  
> -- 
> 2.7.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
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