On Fri, Dec 13, 2019 at 03:02:25PM +0200, Stanislav Lisovskiy wrote:
> Current consensus that it is redundant as
> we already have skl_ddb_values struct out there,
> also this struct contains only single member
> which makes it unnecessary.
>
> v2: As dirty_pipes soon going to be nuked away
> from skl_ddb_values, evacuating enabled_slices
> to safer in dev_priv.
>
> v3: Changed "enabled_slices" to be "enabled_dbuf_slices_num"
> (Matt Roper)
>
> Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@xxxxxxxxx>
> ---
> drivers/gpu/drm/i915/display/intel_display.c | 16 +++----
> .../drm/i915/display/intel_display_power.c | 8 ++--
> .../drm/i915/display/intel_display_types.h | 3 ++
> drivers/gpu/drm/i915/i915_drv.h | 7 +--
> drivers/gpu/drm/i915/intel_pm.c | 44 +++++++++----------
> drivers/gpu/drm/i915/intel_pm.h | 6 +--
> 6 files changed, 39 insertions(+), 45 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 0f37f1d2026d..62e33bca7014 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -13339,14 +13339,13 @@ static void verify_wm_state(struct intel_crtc *crtc,
> struct skl_hw_state {
> struct skl_ddb_entry ddb_y[I915_MAX_PLANES];
> struct skl_ddb_entry ddb_uv[I915_MAX_PLANES];
> - struct skl_ddb_allocation ddb;
> struct skl_pipe_wm wm;
> } *hw;
> - struct skl_ddb_allocation *sw_ddb;
> struct skl_pipe_wm *sw_wm;
> struct skl_ddb_entry *hw_ddb_entry, *sw_ddb_entry;
> const enum pipe pipe = crtc->pipe;
> int plane, level, max_level = ilk_wm_max_level(dev_priv);
> + u8 hw_enabled_slices;
>
> if (INTEL_GEN(dev_priv) < 9 || !new_crtc_state->hw.active)
> return;
> @@ -13360,14 +13359,13 @@ static void verify_wm_state(struct intel_crtc *crtc,
>
> skl_pipe_ddb_get_hw_state(crtc, hw->ddb_y, hw->ddb_uv);
>
> - skl_ddb_get_hw_state(dev_priv, &hw->ddb);
> - sw_ddb = &dev_priv->wm.skl_hw.ddb;
> + hw_enabled_slices = intel_enabled_dbuf_slices_num(dev_priv);
>
> if (INTEL_GEN(dev_priv) >= 11 &&
> - hw->ddb.enabled_slices != sw_ddb->enabled_slices)
> + hw_enabled_slices != dev_priv->enabled_dbuf_slices_num)
> DRM_ERROR("mismatch in DBUF Slices (expected %u, got %u)\n",
> - sw_ddb->enabled_slices,
> - hw->ddb.enabled_slices);
> + dev_priv->enabled_dbuf_slices_num,
> + hw_enabled_slices);
>
> /* planes */
> for_each_universal_plane(dev_priv, pipe, plane) {
> @@ -14553,8 +14551,8 @@ static void skl_commit_modeset_enables(struct intel_atomic_state *state)
> struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> struct intel_crtc *crtc;
> struct intel_crtc_state *old_crtc_state, *new_crtc_state;
> - u8 hw_enabled_slices = dev_priv->wm.skl_hw.ddb.enabled_slices;
> - u8 required_slices = state->wm_results.ddb.enabled_slices;
> + u8 hw_enabled_slices = dev_priv->enabled_dbuf_slices_num;
> + u8 required_slices = state->enabled_dbuf_slices_num;
> struct skl_ddb_entry entries[I915_MAX_PIPES] = {};
> u8 dirty_pipes = 0;
> int i;
> diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c b/drivers/gpu/drm/i915/display/intel_display_power.c
> index 0b3dd2a3b94d..b8983422a882 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_power.c
> +++ b/drivers/gpu/drm/i915/display/intel_display_power.c
> @@ -4264,7 +4264,7 @@ static u8 intel_dbuf_max_slices(struct drm_i915_private *dev_priv)
> void icl_dbuf_slices_update(struct drm_i915_private *dev_priv,
> u8 req_slices)
> {
> - const u8 hw_enabled_slices = dev_priv->wm.skl_hw.ddb.enabled_slices;
> + const u8 hw_enabled_slices = dev_priv->enabled_dbuf_slices_num;
> bool ret;
>
> if (req_slices > intel_dbuf_max_slices(dev_priv)) {
> @@ -4281,7 +4281,7 @@ void icl_dbuf_slices_update(struct drm_i915_private *dev_priv,
> ret = intel_dbuf_slice_set(dev_priv, DBUF_CTL_S2, false);
>
> if (ret)
> - dev_priv->wm.skl_hw.ddb.enabled_slices = req_slices;
> + dev_priv->enabled_dbuf_slices_num = req_slices;
> }
>
> static void icl_dbuf_enable(struct drm_i915_private *dev_priv)
> @@ -4300,7 +4300,7 @@ static void icl_dbuf_enable(struct drm_i915_private *dev_priv)
> * FIXME: for now pretend that we only have 1 slice, see
> * intel_enabled_dbuf_slices_num().
> */
> - dev_priv->wm.skl_hw.ddb.enabled_slices = 1;
> + dev_priv->enabled_dbuf_slices_num = 1;
> }
>
> static void icl_dbuf_disable(struct drm_i915_private *dev_priv)
> @@ -4319,7 +4319,7 @@ static void icl_dbuf_disable(struct drm_i915_private *dev_priv)
> * FIXME: for now pretend that the first slice is always
> * enabled, see intel_enabled_dbuf_slices_num().
> */
> - dev_priv->wm.skl_hw.ddb.enabled_slices = 1;
> + dev_priv->enabled_dbuf_slices_num = 1;
> }
>
> static void icl_mbus_init(struct drm_i915_private *dev_priv)
> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
> index 83ea04149b77..70e65c2d525d 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> @@ -517,6 +517,9 @@ struct intel_atomic_state {
> /* Gen9+ only */
> struct skl_ddb_values wm_results;
>
> + /* Number of enabled DBuf slices */
> + u8 enabled_dbuf_slices_num;
> +
> struct i915_sw_fence commit_ready;
>
> struct llist_node freed;
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 0781b6326b8c..7a2d9fa5a9a6 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -798,13 +798,8 @@ static inline bool skl_ddb_entry_equal(const struct skl_ddb_entry *e1,
> return false;
> }
>
> -struct skl_ddb_allocation {
> - u8 enabled_slices; /* GEN11 has configurable 2 slices */
> -};
> -
> struct skl_ddb_values {
> unsigned dirty_pipes;
> - struct skl_ddb_allocation ddb;
> };
>
> struct skl_wm_level {
> @@ -1215,6 +1210,8 @@ struct drm_i915_private {
> bool distrust_bios_wm;
> } wm;
>
> + u8 enabled_dbuf_slices_num; /* GEN11 has configurable 2 slices */
> +
> struct dram_info {
> bool valid;
> bool is_16gb_dimm;
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index ccbbdf4a6aab..c2510978ccdf 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3616,16 +3616,16 @@ bool ilk_disable_lp_wm(struct drm_i915_private *dev_priv)
> return _ilk_disable_lp_wm(dev_priv, WM_DIRTY_LP_ALL);
> }
>
> -static u8 intel_enabled_dbuf_slices_num(struct drm_i915_private *dev_priv)
> +u8 intel_enabled_dbuf_slices_num(struct drm_i915_private *dev_priv)
> {
> - u8 enabled_slices;
> + u8 enabled_dbuf_slices_num;
>
> /* Slice 1 will always be enabled */
> - enabled_slices = 1;
> + enabled_dbuf_slices_num = 1;
>
> /* Gen prior to GEN11 have only one DBuf slice */
> if (INTEL_GEN(dev_priv) < 11)
> - return enabled_slices;
> + return enabled_dbuf_slices_num;
>
> /*
> * FIXME: for now we'll only ever use 1 slice; pretend that we have
> @@ -3633,9 +3633,9 @@ static u8 intel_enabled_dbuf_slices_num(struct drm_i915_private *dev_priv)
> * toggling of the second slice.
> */
> if (0 && I915_READ(DBUF_CTL_S2) & DBUF_POWER_STATE)
> - enabled_slices++;
> + enabled_dbuf_slices_num++;
>
> - return enabled_slices;
> + return enabled_dbuf_slices_num;
> }
>
> /*
> @@ -3839,9 +3839,10 @@ bool intel_can_enable_sagv(struct intel_atomic_state *state)
> static u16 intel_get_ddb_size(struct drm_i915_private *dev_priv,
> const struct intel_crtc_state *crtc_state,
> const u64 total_data_rate,
> - const int num_active,
> - struct skl_ddb_allocation *ddb)
> + const int num_active)
> {
> + struct drm_atomic_state *state = crtc_state->uapi.state;
> + struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
> const struct drm_display_mode *adjusted_mode;
> u64 total_data_bw;
> u16 ddb_size = INTEL_INFO(dev_priv)->ddb_size;
> @@ -3863,9 +3864,9 @@ static u16 intel_get_ddb_size(struct drm_i915_private *dev_priv,
> * - should validate we stay within the hw bandwidth limits
> */
> if (0 && (num_active > 1 || total_data_bw >= GBps(12))) {
> - ddb->enabled_slices = 2;
> + intel_state->enabled_dbuf_slices_num = 2;
> } else {
> - ddb->enabled_slices = 1;
> + intel_state->enabled_dbuf_slices_num = 1;
> ddb_size /= 2;
> }
>
> @@ -3876,7 +3877,6 @@ static void
> skl_ddb_get_pipe_allocation_limits(struct drm_i915_private *dev_priv,
> const struct intel_crtc_state *crtc_state,
> const u64 total_data_rate,
> - struct skl_ddb_allocation *ddb,
> struct skl_ddb_entry *alloc, /* out */
> int *num_active /* out */)
> {
> @@ -3902,7 +3902,7 @@ skl_ddb_get_pipe_allocation_limits(struct drm_i915_private *dev_priv,
> *num_active = hweight8(dev_priv->active_pipes);
>
> ddb_size = intel_get_ddb_size(dev_priv, crtc_state, total_data_rate,
> - *num_active, ddb);
> + *num_active);
>
> /*
> * If the state doesn't change the active CRTC's or there is no
> @@ -4063,10 +4063,9 @@ void skl_pipe_ddb_get_hw_state(struct intel_crtc *crtc,
> intel_display_power_put(dev_priv, power_domain, wakeref);
> }
>
> -void skl_ddb_get_hw_state(struct drm_i915_private *dev_priv,
> - struct skl_ddb_allocation *ddb /* out */)
> +void skl_ddb_get_hw_state(struct drm_i915_private *dev_priv)
> {
> - ddb->enabled_slices = intel_enabled_dbuf_slices_num(dev_priv);
> + dev_priv->enabled_dbuf_slices_num = intel_enabled_dbuf_slices_num(dev_priv);
I think you may want to wrap this line to keep it under 80 chars.
> }
>
> /*
> @@ -4243,8 +4242,7 @@ icl_get_total_relative_data_rate(struct intel_crtc_state *crtc_state,
> }
>
> static int
> -skl_allocate_pipe_ddb(struct intel_crtc_state *crtc_state,
> - struct skl_ddb_allocation *ddb /* out */)
> +skl_allocate_pipe_ddb(struct intel_crtc_state *crtc_state)
> {
> struct drm_atomic_state *state = crtc_state->uapi.state;
> struct drm_crtc *crtc = crtc_state->uapi.crtc;
> @@ -4286,7 +4284,7 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *crtc_state,
>
>
> skl_ddb_get_pipe_allocation_limits(dev_priv, crtc_state, total_data_rate,
> - ddb, alloc, &num_active);
> + alloc, &num_active);
> alloc_size = skl_ddb_entry_size(alloc);
> if (alloc_size == 0)
> return 0;
> @@ -5200,18 +5198,17 @@ skl_ddb_add_affected_planes(const struct intel_crtc_state *old_crtc_state,
> static int
> skl_compute_ddb(struct intel_atomic_state *state)
> {
> - const struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> - struct skl_ddb_allocation *ddb = &state->wm_results.ddb;
> + struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> struct intel_crtc_state *old_crtc_state;
> struct intel_crtc_state *new_crtc_state;
> struct intel_crtc *crtc;
> int ret, i;
>
> - memcpy(ddb, &dev_priv->wm.skl_hw.ddb, sizeof(*ddb));
> + state->enabled_dbuf_slices_num = dev_priv->enabled_dbuf_slices_num;
>
> for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
> new_crtc_state, i) {
> - ret = skl_allocate_pipe_ddb(new_crtc_state, ddb);
> + ret = skl_allocate_pipe_ddb(new_crtc_state);
> if (ret)
> return ret;
>
> @@ -5683,11 +5680,10 @@ void skl_pipe_wm_get_hw_state(struct intel_crtc *crtc,
> void skl_wm_get_hw_state(struct drm_i915_private *dev_priv)
> {
> struct skl_ddb_values *hw = &dev_priv->wm.skl_hw;
> - struct skl_ddb_allocation *ddb = &dev_priv->wm.skl_hw.ddb;
> struct intel_crtc *crtc;
> struct intel_crtc_state *crtc_state;
>
> - skl_ddb_get_hw_state(dev_priv, ddb);
> + skl_ddb_get_hw_state(dev_priv);
> for_each_intel_crtc(&dev_priv->drm, crtc) {
> crtc_state = to_intel_crtc_state(crtc->base.state);
>
> diff --git a/drivers/gpu/drm/i915/intel_pm.h b/drivers/gpu/drm/i915/intel_pm.h
> index c06c6a846d9a..a476f6c730e9 100644
> --- a/drivers/gpu/drm/i915/intel_pm.h
> +++ b/drivers/gpu/drm/i915/intel_pm.h
> @@ -17,8 +17,8 @@ struct intel_atomic_state;
> struct intel_crtc;
> struct intel_crtc_state;
> struct intel_plane;
> -struct skl_ddb_allocation;
> struct skl_ddb_entry;
> +struct skl_ddb_values;
Do we actually need to add this here? It doesn't seem like this patch
has started using this type anywhere that it wasn't already available.
Aside from that and the linewrap mentioned farther up,
Reviewed-by: Matt Roper <matthew.d.roper@xxxxxxxxx>
> struct skl_pipe_wm;
> struct skl_wm_level;
>
> @@ -33,11 +33,11 @@ void g4x_wm_get_hw_state(struct drm_i915_private *dev_priv);
> void vlv_wm_get_hw_state(struct drm_i915_private *dev_priv);
> void ilk_wm_get_hw_state(struct drm_i915_private *dev_priv);
> void skl_wm_get_hw_state(struct drm_i915_private *dev_priv);
> +u8 intel_enabled_dbuf_slices_num(struct drm_i915_private *dev_priv);
> void skl_pipe_ddb_get_hw_state(struct intel_crtc *crtc,
> struct skl_ddb_entry *ddb_y,
> struct skl_ddb_entry *ddb_uv);
> -void skl_ddb_get_hw_state(struct drm_i915_private *dev_priv,
> - struct skl_ddb_allocation *ddb /* out */);
> +void skl_ddb_get_hw_state(struct drm_i915_private *dev_priv);
> void skl_pipe_wm_get_hw_state(struct intel_crtc *crtc,
> struct skl_pipe_wm *out);
> void g4x_wm_sanitize(struct drm_i915_private *dev_priv);
> --
> 2.17.1
>
--
Matt Roper
Graphics Software Engineer
VTT-OSGC Platform Enablement
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
