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@xxxxxxxxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxxxxxxxx
> 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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
