| Message ID | 20200507144503.15506-5-stanislav.lisovskiy@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | SAGV support for Gen12+ | expand |
On Thu, May 07, 2020 at 05:45:01PM +0300, Stanislav Lisovskiy wrote: > Starting from TGL we need to have a separate wm0 > values for SAGV and non-SAGV which affects > how calculations are done. > > v2: Remove long lines > v3: Removed COLOR_PLANE enum references > v4, v5, v6: Fixed rebase conflict > v7: - Removed skl_plane_wm_level accessor from skl_allocate_pipe_ddb(Ville) > - Removed sagv_uv_wm0(Ville) > - can_sagv->use_sagv_wm(Ville) > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > --- > drivers/gpu/drm/i915/display/intel_display.c | 8 +- > .../drm/i915/display/intel_display_types.h | 2 + > drivers/gpu/drm/i915/intel_pm.c | 118 +++++++++++++++++- > 3 files changed, 121 insertions(+), 7 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c > index fd6d63b03489..be5741cb7595 100644 > --- a/drivers/gpu/drm/i915/display/intel_display.c > +++ b/drivers/gpu/drm/i915/display/intel_display.c > @@ -13961,7 +13961,9 @@ static void verify_wm_state(struct intel_crtc *crtc, > /* Watermarks */ > for (level = 0; level <= max_level; level++) { > if (skl_wm_level_equals(&hw_plane_wm->wm[level], > - &sw_plane_wm->wm[level])) > + &sw_plane_wm->wm[level]) || > + (level == 0 && skl_wm_level_equals(&hw_plane_wm->wm[level], > + &sw_plane_wm->sagv_wm0))) > continue; > > drm_err(&dev_priv->drm, > @@ -14016,7 +14018,9 @@ static void verify_wm_state(struct intel_crtc *crtc, > /* Watermarks */ > for (level = 0; level <= max_level; level++) { > if (skl_wm_level_equals(&hw_plane_wm->wm[level], > - &sw_plane_wm->wm[level])) > + &sw_plane_wm->wm[level]) || > + (level == 0 && skl_wm_level_equals(&hw_plane_wm->wm[level], > + &sw_plane_wm->sagv_wm0))) > continue; > > drm_err(&dev_priv->drm, > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h > index 9488449e4b94..8cede29c9562 100644 > --- a/drivers/gpu/drm/i915/display/intel_display_types.h > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h > @@ -688,11 +688,13 @@ struct skl_plane_wm { > struct skl_wm_level wm[8]; > struct skl_wm_level uv_wm[8]; > struct skl_wm_level trans_wm; > + struct skl_wm_level sagv_wm0; > bool is_planar; > }; > > struct skl_pipe_wm { > struct skl_plane_wm planes[I915_MAX_PLANES]; > + bool use_sagv_wm; > }; > > enum vlv_wm_level { > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index db188efee21e..934a686342ad 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -3863,25 +3863,35 @@ bool intel_can_enable_sagv(struct drm_i915_private *dev_priv, > return bw_state->pipe_sagv_reject == 0; > } > > +static bool > +tgl_crtc_can_enable_sagv(const struct intel_crtc_state *crtc_state); Just put the function here instead of adding fwd decalrations. > + > static int intel_compute_sagv_mask(struct intel_atomic_state *state) > { > struct drm_i915_private *dev_priv = to_i915(state->base.dev); > int ret; > struct intel_crtc *crtc; > - const struct intel_crtc_state *new_crtc_state; > + struct intel_crtc_state *new_crtc_state; > struct intel_bw_state *new_bw_state = NULL; > const struct intel_bw_state *old_bw_state = NULL; > int i; > > for_each_new_intel_crtc_in_state(state, crtc, > new_crtc_state, i) { > + bool can_sagv; > + > new_bw_state = intel_atomic_get_bw_state(state); > if (IS_ERR(new_bw_state)) > return PTR_ERR(new_bw_state); > > old_bw_state = intel_atomic_get_old_bw_state(state); > > - if (skl_crtc_can_enable_sagv(new_crtc_state)) > + if (INTEL_GEN(dev_priv) >= 12) > + can_sagv = tgl_crtc_can_enable_sagv(new_crtc_state); > + else > + can_sagv = skl_crtc_can_enable_sagv(new_crtc_state); > + > + if (can_sagv) > new_bw_state->pipe_sagv_reject &= ~BIT(crtc->pipe); > else > new_bw_state->pipe_sagv_reject |= BIT(crtc->pipe); > @@ -3899,6 +3909,24 @@ static int intel_compute_sagv_mask(struct intel_atomic_state *state) > return ret; > } > > + for_each_new_intel_crtc_in_state(state, crtc, > + new_crtc_state, i) { > + struct skl_pipe_wm *pipe_wm = &new_crtc_state->wm.skl.optimal; > + > + /* > + * Due to drm limitation at commit state, when > + * changes are written the whole atomic state is > + * zeroed away => which prevents from using it, > + * so just sticking it into pipe wm state for > + * keeping it simple - anyway this is related to wm. > + * Proper way in ideal universe would be of course not > + * to lose parent atomic state object from child crtc_state, > + * and stick to OOP programming principles, which had been > + * scientifically proven to work. > + */ More ramblings. Just drop this comment too imo. > + pipe_wm->use_sagv_wm = intel_can_enable_sagv(dev_priv, new_bw_state); I think this should be gen >= 12 && intel_can_enable_sagv(); > + } > + > if (intel_can_enable_sagv(dev_priv, new_bw_state) != intel_can_enable_sagv(dev_priv, old_bw_state)) { > ret = intel_atomic_serialize_global_state(&new_bw_state->base); > if (ret) > @@ -4642,12 +4670,39 @@ skl_plane_wm_level(const struct intel_crtc_state *crtc_state, > int level, > int color_plane) > { > - const struct skl_plane_wm *wm = > - &crtc_state->wm.skl.optimal.planes[plane_id]; > + const struct skl_pipe_wm *pipe_wm = &crtc_state->wm.skl.optimal; > + const struct skl_plane_wm *wm = &pipe_wm->planes[plane_id]; > + > + if (!level) { > + if (pipe_wm->use_sagv_wm) > + return &wm->sagv_wm0; > + } if (level == 0 && use_sagv_wm) return sagv_wm0; > > return color_plane == 0 ? &wm->wm[level] : &wm->uv_wm[level]; > } > > +static bool > +tgl_crtc_can_enable_sagv(const struct intel_crtc_state *crtc_state) > +{ > + struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc); > + enum plane_id plane_id; > + > + if (!crtc_state->hw.active) > + return true; > + > + for_each_plane_id_on_crtc(crtc, plane_id) { > + const struct skl_ddb_entry *plane_alloc = > + &crtc_state->wm.skl.plane_ddb_y[plane_id]; > + const struct skl_plane_wm *wm = > + &crtc_state->wm.skl.optimal.planes[plane_id]; > + > + if (skl_ddb_entry_size(plane_alloc) < wm->sagv_wm0.min_ddb_alloc) > + return false; > + } > + > + return true; > +} > + > static int > skl_allocate_pipe_ddb(struct intel_crtc_state *crtc_state) > { > @@ -4684,7 +4739,6 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *crtc_state) > plane_data_rate, > uv_plane_data_rate); > > - > skl_ddb_get_pipe_allocation_limits(dev_priv, crtc_state, total_data_rate, > alloc, &num_active); > alloc_size = skl_ddb_entry_size(alloc); > @@ -5219,6 +5273,37 @@ skl_compute_wm_levels(const struct intel_crtc_state *crtc_state, > } > } > > +static void tgl_compute_sagv_wm(const struct intel_crtc_state *crtc_state, > + const struct skl_wm_params *wm_params, > + struct skl_plane_wm *plane_wm) > +{ > + struct drm_i915_private *dev_priv = to_i915(crtc_state->uapi.crtc->dev); > + struct skl_wm_level *sagv_wm = &plane_wm->sagv_wm0; > + struct skl_wm_level *levels = plane_wm->wm; > + > + /* > + * For Gen12 if it is an L0 we need to also > + * consider sagv_block_time when calculating > + * L0 watermark - we will need that when making > + * a decision whether enable SAGV or not. > + * For older gens we agreed to copy L0 value for > + * compatibility. > + */ > + if ((INTEL_GEN(dev_priv) >= 12)) { Drop this if-else and only call tgl_compute_sagv_wm() on tgl+. The comment can then go as well I think. > + u32 latency = dev_priv->wm.skl_latency[0]; > + > + latency += dev_priv->sagv_block_time_us; > + skl_compute_plane_wm(crtc_state, 0, latency, > + wm_params, &levels[0], > + sagv_wm); > + DRM_DEBUG_KMS("%d L0 blocks required for SAGV vs %d for non-SAGV\n", > + sagv_wm->min_ddb_alloc, levels[0].min_ddb_alloc); Leftover debugs. Pls remove. > + } else { > + /* Since all members are POD */ > + *sagv_wm = levels[0]; > + } > +} > + > static void skl_compute_transition_wm(const struct intel_crtc_state *crtc_state, > const struct skl_wm_params *wp, > struct skl_plane_wm *wm) > @@ -5296,6 +5381,8 @@ static int skl_build_plane_wm_single(struct intel_crtc_state *crtc_state, > return ret; > > skl_compute_wm_levels(crtc_state, &wm_params, wm->wm); > + if (color_plane == 0) We do want this for both the Y plane and UV plane. > + tgl_compute_sagv_wm(crtc_state, &wm_params, wm); > skl_compute_transition_wm(crtc_state, &wm_params, wm); > > return 0; > @@ -5702,6 +5789,12 @@ skl_print_wm_changes(struct intel_atomic_state *state) > enast(new_wm->wm[7].ignore_lines), new_wm->wm[7].plane_res_l, > enast(new_wm->trans_wm.ignore_lines), new_wm->trans_wm.plane_res_l); > > + drm_dbg_kms(&dev_priv->drm, > + "[PLANE:%d:%s] sagv wm0 lines %4d -> %4d\n", > + plane->base.base.id, plane->base.name, > + old_wm->sagv_wm0.plane_res_l, > + new_wm->sagv_wm0.plane_res_l); > + IIRC I already suggested just slapping these onto the existing debug prints instead of bloating the debug output with extra lines. Ie instead of wms ... -> ... sagv wm ... -> ... we just get wms ...,old_sagv_wm0 -> ...,new_sagv_wm0 just like we already do for eg. the transition wm. > drm_dbg_kms(&dev_priv->drm, > "[PLANE:%d:%s] blocks %4d,%4d,%4d,%4d,%4d,%4d,%4d,%4d,%4d" > " -> %4d,%4d,%4d,%4d,%4d,%4d,%4d,%4d,%4d\n", > @@ -5717,6 +5810,12 @@ skl_print_wm_changes(struct intel_atomic_state *state) > new_wm->wm[6].plane_res_b, new_wm->wm[7].plane_res_b, > new_wm->trans_wm.plane_res_b); > > + drm_dbg_kms(&dev_priv->drm, > + "[PLANE:%d:%s] sagv wm0 blocks %4d -> %4d\n", > + plane->base.base.id, plane->base.name, > + old_wm->sagv_wm0.plane_res_b, > + new_wm->sagv_wm0.plane_res_b); > + > drm_dbg_kms(&dev_priv->drm, > "[PLANE:%d:%s] min_ddb %4d,%4d,%4d,%4d,%4d,%4d,%4d,%4d,%4d" > " -> %4d,%4d,%4d,%4d,%4d,%4d,%4d,%4d,%4d\n", > @@ -5731,6 +5830,12 @@ skl_print_wm_changes(struct intel_atomic_state *state) > new_wm->wm[4].min_ddb_alloc, new_wm->wm[5].min_ddb_alloc, > new_wm->wm[6].min_ddb_alloc, new_wm->wm[7].min_ddb_alloc, > new_wm->trans_wm.min_ddb_alloc); > + > + drm_dbg_kms(&dev_priv->drm, > + "[PLANE:%d:%s] sagv wm0 min ddb %4d -> %4d\n", > + plane->base.base.id, plane->base.name, > + old_wm->sagv_wm0.min_ddb_alloc, > + new_wm->sagv_wm0.min_ddb_alloc); > } > } > } > @@ -6023,6 +6128,9 @@ void skl_pipe_wm_get_hw_state(struct intel_crtc *crtc, > skl_wm_level_from_reg_val(val, &wm->wm[level]); > } > > + memcpy(&wm->sagv_wm0, &wm->wm[0], > + sizeof(struct skl_wm_level)); A simple assignment should suffice? > + > if (plane_id != PLANE_CURSOR) > val = I915_READ(PLANE_WM_TRANS(pipe, plane_id)); > else > -- > 2.24.1.485.gad05a3d8e5
On Tue, May 12, 2020 at 03:03:26PM +0300, Ville Syrjälä wrote: > On Thu, May 07, 2020 at 05:45:01PM +0300, Stanislav Lisovskiy wrote: > > Starting from TGL we need to have a separate wm0 > > values for SAGV and non-SAGV which affects > > how calculations are done. > > > > v2: Remove long lines > > v3: Removed COLOR_PLANE enum references > > v4, v5, v6: Fixed rebase conflict > > v7: - Removed skl_plane_wm_level accessor from skl_allocate_pipe_ddb(Ville) > > - Removed sagv_uv_wm0(Ville) > > - can_sagv->use_sagv_wm(Ville) > > > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > > --- > > drivers/gpu/drm/i915/display/intel_display.c | 8 +- > > .../drm/i915/display/intel_display_types.h | 2 + > > drivers/gpu/drm/i915/intel_pm.c | 118 +++++++++++++++++- > > 3 files changed, 121 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c > > index fd6d63b03489..be5741cb7595 100644 > > --- a/drivers/gpu/drm/i915/display/intel_display.c > > +++ b/drivers/gpu/drm/i915/display/intel_display.c > > @@ -13961,7 +13961,9 @@ static void verify_wm_state(struct intel_crtc *crtc, > > /* Watermarks */ > > for (level = 0; level <= max_level; level++) { > > if (skl_wm_level_equals(&hw_plane_wm->wm[level], > > - &sw_plane_wm->wm[level])) > > + &sw_plane_wm->wm[level]) || > > + (level == 0 && skl_wm_level_equals(&hw_plane_wm->wm[level], > > + &sw_plane_wm->sagv_wm0))) > > continue; > > > > drm_err(&dev_priv->drm, > > @@ -14016,7 +14018,9 @@ static void verify_wm_state(struct intel_crtc *crtc, > > /* Watermarks */ > > for (level = 0; level <= max_level; level++) { > > if (skl_wm_level_equals(&hw_plane_wm->wm[level], > > - &sw_plane_wm->wm[level])) > > + &sw_plane_wm->wm[level]) || > > + (level == 0 && skl_wm_level_equals(&hw_plane_wm->wm[level], > > + &sw_plane_wm->sagv_wm0))) > > continue; > > > > drm_err(&dev_priv->drm, > > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h > > index 9488449e4b94..8cede29c9562 100644 > > --- a/drivers/gpu/drm/i915/display/intel_display_types.h > > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h > > @@ -688,11 +688,13 @@ struct skl_plane_wm { > > struct skl_wm_level wm[8]; > > struct skl_wm_level uv_wm[8]; > > struct skl_wm_level trans_wm; > > + struct skl_wm_level sagv_wm0; > > bool is_planar; > > }; > > > > struct skl_pipe_wm { > > struct skl_plane_wm planes[I915_MAX_PLANES]; > > + bool use_sagv_wm; > > }; > > > > enum vlv_wm_level { > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > > index db188efee21e..934a686342ad 100644 > > --- a/drivers/gpu/drm/i915/intel_pm.c > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > @@ -3863,25 +3863,35 @@ bool intel_can_enable_sagv(struct drm_i915_private *dev_priv, > > return bw_state->pipe_sagv_reject == 0; > > } > > > > +static bool > > +tgl_crtc_can_enable_sagv(const struct intel_crtc_state *crtc_state); > > Just put the function here instead of adding fwd decalrations. > > > + > > static int intel_compute_sagv_mask(struct intel_atomic_state *state) > > { > > struct drm_i915_private *dev_priv = to_i915(state->base.dev); > > int ret; > > struct intel_crtc *crtc; > > - const struct intel_crtc_state *new_crtc_state; > > + struct intel_crtc_state *new_crtc_state; > > struct intel_bw_state *new_bw_state = NULL; > > const struct intel_bw_state *old_bw_state = NULL; > > int i; > > > > for_each_new_intel_crtc_in_state(state, crtc, > > new_crtc_state, i) { > > + bool can_sagv; > > + > > new_bw_state = intel_atomic_get_bw_state(state); > > if (IS_ERR(new_bw_state)) > > return PTR_ERR(new_bw_state); > > > > old_bw_state = intel_atomic_get_old_bw_state(state); > > > > - if (skl_crtc_can_enable_sagv(new_crtc_state)) > > + if (INTEL_GEN(dev_priv) >= 12) > > + can_sagv = tgl_crtc_can_enable_sagv(new_crtc_state); > > + else > > + can_sagv = skl_crtc_can_enable_sagv(new_crtc_state); > > + > > + if (can_sagv) > > new_bw_state->pipe_sagv_reject &= ~BIT(crtc->pipe); > > else > > new_bw_state->pipe_sagv_reject |= BIT(crtc->pipe); > > @@ -3899,6 +3909,24 @@ static int intel_compute_sagv_mask(struct intel_atomic_state *state) > > return ret; > > } > > > > + for_each_new_intel_crtc_in_state(state, crtc, > > + new_crtc_state, i) { > > + struct skl_pipe_wm *pipe_wm = &new_crtc_state->wm.skl.optimal; > > + > > + /* > > + * Due to drm limitation at commit state, when > > + * changes are written the whole atomic state is > > + * zeroed away => which prevents from using it, > > + * so just sticking it into pipe wm state for > > + * keeping it simple - anyway this is related to wm. > > + * Proper way in ideal universe would be of course not > > + * to lose parent atomic state object from child crtc_state, > > + * and stick to OOP programming principles, which had been > > + * scientifically proven to work. > > + */ > > More ramblings. Just drop this comment too imo. As I understand what is happening here is rather weird, so I thought commenting is good idea. > > > + pipe_wm->use_sagv_wm = intel_can_enable_sagv(dev_priv, new_bw_state); > > I think this should be > gen >= 12 && intel_can_enable_sagv(); > > > + } > > + > > if (intel_can_enable_sagv(dev_priv, new_bw_state) != intel_can_enable_sagv(dev_priv, old_bw_state)) { > > ret = intel_atomic_serialize_global_state(&new_bw_state->base); > > if (ret) > > @@ -4642,12 +4670,39 @@ skl_plane_wm_level(const struct intel_crtc_state *crtc_state, > > int level, > > int color_plane) > > { > > - const struct skl_plane_wm *wm = > > - &crtc_state->wm.skl.optimal.planes[plane_id]; > > + const struct skl_pipe_wm *pipe_wm = &crtc_state->wm.skl.optimal; > > + const struct skl_plane_wm *wm = &pipe_wm->planes[plane_id]; > > + > > + if (!level) { > > + if (pipe_wm->use_sagv_wm) > > + return &wm->sagv_wm0; > > + } > > if (level == 0 && use_sagv_wm) > return sagv_wm0; > > > > > return color_plane == 0 ? &wm->wm[level] : &wm->uv_wm[level]; > > } > > > > +static bool > > +tgl_crtc_can_enable_sagv(const struct intel_crtc_state *crtc_state) > > +{ > > + struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc); > > + enum plane_id plane_id; > > + > > + if (!crtc_state->hw.active) > > + return true; > > + > > + for_each_plane_id_on_crtc(crtc, plane_id) { > > + const struct skl_ddb_entry *plane_alloc = > > + &crtc_state->wm.skl.plane_ddb_y[plane_id]; > > + const struct skl_plane_wm *wm = > > + &crtc_state->wm.skl.optimal.planes[plane_id]; > > + > > + if (skl_ddb_entry_size(plane_alloc) < wm->sagv_wm0.min_ddb_alloc) > > + return false; > > + } > > + > > + return true; > > +} > > + > > static int > > skl_allocate_pipe_ddb(struct intel_crtc_state *crtc_state) > > { > > @@ -4684,7 +4739,6 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *crtc_state) > > plane_data_rate, > > uv_plane_data_rate); > > > > - > > skl_ddb_get_pipe_allocation_limits(dev_priv, crtc_state, total_data_rate, > > alloc, &num_active); > > alloc_size = skl_ddb_entry_size(alloc); > > @@ -5219,6 +5273,37 @@ skl_compute_wm_levels(const struct intel_crtc_state *crtc_state, > > } > > } > > > > +static void tgl_compute_sagv_wm(const struct intel_crtc_state *crtc_state, > > + const struct skl_wm_params *wm_params, > > + struct skl_plane_wm *plane_wm) > > +{ > > + struct drm_i915_private *dev_priv = to_i915(crtc_state->uapi.crtc->dev); > > + struct skl_wm_level *sagv_wm = &plane_wm->sagv_wm0; > > + struct skl_wm_level *levels = plane_wm->wm; > > + > > + /* > > + * For Gen12 if it is an L0 we need to also > > + * consider sagv_block_time when calculating > > + * L0 watermark - we will need that when making > > + * a decision whether enable SAGV or not. > > + * For older gens we agreed to copy L0 value for > > + * compatibility. > > + */ > > + if ((INTEL_GEN(dev_priv) >= 12)) { > > Drop this if-else and only call tgl_compute_sagv_wm() on tgl+. > The comment can then go as well I think. We actually had _agreed_ with you personally that I do copy wm0 values for other platforms for compatibility reasons. Now after one month you say that we are going to call this only for gen12. > > > + u32 latency = dev_priv->wm.skl_latency[0]; > > + > > + latency += dev_priv->sagv_block_time_us; > > + skl_compute_plane_wm(crtc_state, 0, latency, > > + wm_params, &levels[0], > > + sagv_wm); > > + DRM_DEBUG_KMS("%d L0 blocks required for SAGV vs %d for non-SAGV\n", > > + sagv_wm->min_ddb_alloc, levels[0].min_ddb_alloc); > > Leftover debugs. Pls remove. I thought this might be useful, we should have some understanding what is happening, i.e what was the reasoning and why SAGV was switched on/off. Not crucial, but really don't understand why this is _that_ harmful. We have plenty of wm/ddb debugs anyway, but no way to figure out how SAGV logic is working. > > > + } else { > > + /* Since all members are POD */ > > + *sagv_wm = levels[0]; > > + } > > +} > > + > > static void skl_compute_transition_wm(const struct intel_crtc_state *crtc_state, > > const struct skl_wm_params *wp, > > struct skl_plane_wm *wm) > > @@ -5296,6 +5381,8 @@ static int skl_build_plane_wm_single(struct intel_crtc_state *crtc_state, > > return ret; > > > > skl_compute_wm_levels(crtc_state, &wm_params, wm->wm); > > + if (color_plane == 0) > > We do want this for both the Y plane and UV plane. > > > + tgl_compute_sagv_wm(crtc_state, &wm_params, wm); > > skl_compute_transition_wm(crtc_state, &wm_params, wm); > > > > return 0; > > @@ -5702,6 +5789,12 @@ skl_print_wm_changes(struct intel_atomic_state *state) > > enast(new_wm->wm[7].ignore_lines), new_wm->wm[7].plane_res_l, > > enast(new_wm->trans_wm.ignore_lines), new_wm->trans_wm.plane_res_l); > > > > + drm_dbg_kms(&dev_priv->drm, > > + "[PLANE:%d:%s] sagv wm0 lines %4d -> %4d\n", > > + plane->base.base.id, plane->base.name, > > + old_wm->sagv_wm0.plane_res_l, > > + new_wm->sagv_wm0.plane_res_l); > > + > > IIRC I already suggested just slapping these onto the existing debug > prints instead of bloating the debug output with extra lines. > > Ie instead of > wms ... -> ... > sagv wm ... -> ... > > we just get > wms ...,old_sagv_wm0 -> ...,new_sagv_wm0 > > just like we already do for eg. the transition wm. Not sure about this one. I will check, I thought you suggested the opposite, i.e do those debugs separately. Stan > > > drm_dbg_kms(&dev_priv->drm, > > "[PLANE:%d:%s] blocks %4d,%4d,%4d,%4d,%4d,%4d,%4d,%4d,%4d" > > " -> %4d,%4d,%4d,%4d,%4d,%4d,%4d,%4d,%4d\n", > > @@ -5717,6 +5810,12 @@ skl_print_wm_changes(struct intel_atomic_state *state) > > new_wm->wm[6].plane_res_b, new_wm->wm[7].plane_res_b, > > new_wm->trans_wm.plane_res_b); > > > > + drm_dbg_kms(&dev_priv->drm, > > + "[PLANE:%d:%s] sagv wm0 blocks %4d -> %4d\n", > > + plane->base.base.id, plane->base.name, > > + old_wm->sagv_wm0.plane_res_b, > > + new_wm->sagv_wm0.plane_res_b); > > + > > drm_dbg_kms(&dev_priv->drm, > > "[PLANE:%d:%s] min_ddb %4d,%4d,%4d,%4d,%4d,%4d,%4d,%4d,%4d" > > " -> %4d,%4d,%4d,%4d,%4d,%4d,%4d,%4d,%4d\n", > > @@ -5731,6 +5830,12 @@ skl_print_wm_changes(struct intel_atomic_state *state) > > new_wm->wm[4].min_ddb_alloc, new_wm->wm[5].min_ddb_alloc, > > new_wm->wm[6].min_ddb_alloc, new_wm->wm[7].min_ddb_alloc, > > new_wm->trans_wm.min_ddb_alloc); > > + > > + drm_dbg_kms(&dev_priv->drm, > > + "[PLANE:%d:%s] sagv wm0 min ddb %4d -> %4d\n", > > + plane->base.base.id, plane->base.name, > > + old_wm->sagv_wm0.min_ddb_alloc, > > + new_wm->sagv_wm0.min_ddb_alloc); > > } > > } > > } > > @@ -6023,6 +6128,9 @@ void skl_pipe_wm_get_hw_state(struct intel_crtc *crtc, > > skl_wm_level_from_reg_val(val, &wm->wm[level]); > > } > > > > + memcpy(&wm->sagv_wm0, &wm->wm[0], > > + sizeof(struct skl_wm_level)); > > A simple assignment should suffice? Agree. > > > + > > if (plane_id != PLANE_CURSOR) > > val = I915_READ(PLANE_WM_TRANS(pipe, plane_id)); > > else > > -- > > 2.24.1.485.gad05a3d8e5 > > -- > Ville Syrjälä > Intel
On Tue, May 12, 2020 at 03:52:27PM +0300, Lisovskiy, Stanislav wrote: > On Tue, May 12, 2020 at 03:03:26PM +0300, Ville Syrjälä wrote: > > On Thu, May 07, 2020 at 05:45:01PM +0300, Stanislav Lisovskiy wrote: > > > Starting from TGL we need to have a separate wm0 > > > values for SAGV and non-SAGV which affects > > > how calculations are done. > > > > > > v2: Remove long lines > > > v3: Removed COLOR_PLANE enum references > > > v4, v5, v6: Fixed rebase conflict > > > v7: - Removed skl_plane_wm_level accessor from skl_allocate_pipe_ddb(Ville) > > > - Removed sagv_uv_wm0(Ville) > > > - can_sagv->use_sagv_wm(Ville) > > > > > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > > > --- > > > drivers/gpu/drm/i915/display/intel_display.c | 8 +- > > > .../drm/i915/display/intel_display_types.h | 2 + > > > drivers/gpu/drm/i915/intel_pm.c | 118 +++++++++++++++++- > > > 3 files changed, 121 insertions(+), 7 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c > > > index fd6d63b03489..be5741cb7595 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_display.c > > > +++ b/drivers/gpu/drm/i915/display/intel_display.c > > > @@ -13961,7 +13961,9 @@ static void verify_wm_state(struct intel_crtc *crtc, > > > /* Watermarks */ > > > for (level = 0; level <= max_level; level++) { > > > if (skl_wm_level_equals(&hw_plane_wm->wm[level], > > > - &sw_plane_wm->wm[level])) > > > + &sw_plane_wm->wm[level]) || > > > + (level == 0 && skl_wm_level_equals(&hw_plane_wm->wm[level], > > > + &sw_plane_wm->sagv_wm0))) > > > continue; > > > > > > drm_err(&dev_priv->drm, > > > @@ -14016,7 +14018,9 @@ static void verify_wm_state(struct intel_crtc *crtc, > > > /* Watermarks */ > > > for (level = 0; level <= max_level; level++) { > > > if (skl_wm_level_equals(&hw_plane_wm->wm[level], > > > - &sw_plane_wm->wm[level])) > > > + &sw_plane_wm->wm[level]) || > > > + (level == 0 && skl_wm_level_equals(&hw_plane_wm->wm[level], > > > + &sw_plane_wm->sagv_wm0))) > > > continue; > > > > > > drm_err(&dev_priv->drm, > > > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h > > > index 9488449e4b94..8cede29c9562 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_display_types.h > > > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h > > > @@ -688,11 +688,13 @@ struct skl_plane_wm { > > > struct skl_wm_level wm[8]; > > > struct skl_wm_level uv_wm[8]; > > > struct skl_wm_level trans_wm; > > > + struct skl_wm_level sagv_wm0; > > > bool is_planar; > > > }; > > > > > > struct skl_pipe_wm { > > > struct skl_plane_wm planes[I915_MAX_PLANES]; > > > + bool use_sagv_wm; > > > }; > > > > > > enum vlv_wm_level { > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > > > index db188efee21e..934a686342ad 100644 > > > --- a/drivers/gpu/drm/i915/intel_pm.c > > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > > @@ -3863,25 +3863,35 @@ bool intel_can_enable_sagv(struct drm_i915_private *dev_priv, > > > return bw_state->pipe_sagv_reject == 0; > > > } > > > > > > +static bool > > > +tgl_crtc_can_enable_sagv(const struct intel_crtc_state *crtc_state); > > > > Just put the function here instead of adding fwd decalrations. > > > > > + > > > static int intel_compute_sagv_mask(struct intel_atomic_state *state) > > > { > > > struct drm_i915_private *dev_priv = to_i915(state->base.dev); > > > int ret; > > > struct intel_crtc *crtc; > > > - const struct intel_crtc_state *new_crtc_state; > > > + struct intel_crtc_state *new_crtc_state; > > > struct intel_bw_state *new_bw_state = NULL; > > > const struct intel_bw_state *old_bw_state = NULL; > > > int i; > > > > > > for_each_new_intel_crtc_in_state(state, crtc, > > > new_crtc_state, i) { > > > + bool can_sagv; > > > + > > > new_bw_state = intel_atomic_get_bw_state(state); > > > if (IS_ERR(new_bw_state)) > > > return PTR_ERR(new_bw_state); > > > > > > old_bw_state = intel_atomic_get_old_bw_state(state); > > > > > > - if (skl_crtc_can_enable_sagv(new_crtc_state)) > > > + if (INTEL_GEN(dev_priv) >= 12) > > > + can_sagv = tgl_crtc_can_enable_sagv(new_crtc_state); > > > + else > > > + can_sagv = skl_crtc_can_enable_sagv(new_crtc_state); > > > + > > > + if (can_sagv) > > > new_bw_state->pipe_sagv_reject &= ~BIT(crtc->pipe); > > > else > > > new_bw_state->pipe_sagv_reject |= BIT(crtc->pipe); > > > @@ -3899,6 +3909,24 @@ static int intel_compute_sagv_mask(struct intel_atomic_state *state) > > > return ret; > > > } > > > > > > + for_each_new_intel_crtc_in_state(state, crtc, > > > + new_crtc_state, i) { > > > + struct skl_pipe_wm *pipe_wm = &new_crtc_state->wm.skl.optimal; > > > + > > > + /* > > > + * Due to drm limitation at commit state, when > > > + * changes are written the whole atomic state is > > > + * zeroed away => which prevents from using it, > > > + * so just sticking it into pipe wm state for > > > + * keeping it simple - anyway this is related to wm. > > > + * Proper way in ideal universe would be of course not > > > + * to lose parent atomic state object from child crtc_state, > > > + * and stick to OOP programming principles, which had been > > > + * scientifically proven to work. > > > + */ > > > > More ramblings. Just drop this comment too imo. > > As I understand what is happening here is rather weird, so I thought > commenting is good idea. At least I have no idea what the comment is trying to say. I see nothing weird hapening here, we're just computing the state which is totally standard stuff. > > > > > > + pipe_wm->use_sagv_wm = intel_can_enable_sagv(dev_priv, new_bw_state); > > > > I think this should be > > gen >= 12 && intel_can_enable_sagv(); > > > > > + } > > > + > > > if (intel_can_enable_sagv(dev_priv, new_bw_state) != intel_can_enable_sagv(dev_priv, old_bw_state)) { > > > ret = intel_atomic_serialize_global_state(&new_bw_state->base); > > > if (ret) > > > @@ -4642,12 +4670,39 @@ skl_plane_wm_level(const struct intel_crtc_state *crtc_state, > > > int level, > > > int color_plane) > > > { > > > - const struct skl_plane_wm *wm = > > > - &crtc_state->wm.skl.optimal.planes[plane_id]; > > > + const struct skl_pipe_wm *pipe_wm = &crtc_state->wm.skl.optimal; > > > + const struct skl_plane_wm *wm = &pipe_wm->planes[plane_id]; > > > + > > > + if (!level) { > > > + if (pipe_wm->use_sagv_wm) > > > + return &wm->sagv_wm0; > > > + } > > > > if (level == 0 && use_sagv_wm) > > return sagv_wm0; > > > > > > > > return color_plane == 0 ? &wm->wm[level] : &wm->uv_wm[level]; > > > } > > > > > > +static bool > > > +tgl_crtc_can_enable_sagv(const struct intel_crtc_state *crtc_state) > > > +{ > > > + struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc); > > > + enum plane_id plane_id; > > > + > > > + if (!crtc_state->hw.active) > > > + return true; > > > + > > > + for_each_plane_id_on_crtc(crtc, plane_id) { > > > + const struct skl_ddb_entry *plane_alloc = > > > + &crtc_state->wm.skl.plane_ddb_y[plane_id]; > > > + const struct skl_plane_wm *wm = > > > + &crtc_state->wm.skl.optimal.planes[plane_id]; > > > + > > > + if (skl_ddb_entry_size(plane_alloc) < wm->sagv_wm0.min_ddb_alloc) > > > + return false; > > > + } > > > + > > > + return true; > > > +} > > > + > > > static int > > > skl_allocate_pipe_ddb(struct intel_crtc_state *crtc_state) > > > { > > > @@ -4684,7 +4739,6 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *crtc_state) > > > plane_data_rate, > > > uv_plane_data_rate); > > > > > > - > > > skl_ddb_get_pipe_allocation_limits(dev_priv, crtc_state, total_data_rate, > > > alloc, &num_active); > > > alloc_size = skl_ddb_entry_size(alloc); > > > @@ -5219,6 +5273,37 @@ skl_compute_wm_levels(const struct intel_crtc_state *crtc_state, > > > } > > > } > > > > > > +static void tgl_compute_sagv_wm(const struct intel_crtc_state *crtc_state, > > > + const struct skl_wm_params *wm_params, > > > + struct skl_plane_wm *plane_wm) > > > +{ > > > + struct drm_i915_private *dev_priv = to_i915(crtc_state->uapi.crtc->dev); > > > + struct skl_wm_level *sagv_wm = &plane_wm->sagv_wm0; > > > + struct skl_wm_level *levels = plane_wm->wm; > > > + > > > + /* > > > + * For Gen12 if it is an L0 we need to also > > > + * consider sagv_block_time when calculating > > > + * L0 watermark - we will need that when making > > > + * a decision whether enable SAGV or not. > > > + * For older gens we agreed to copy L0 value for > > > + * compatibility. > > > + */ > > > + if ((INTEL_GEN(dev_priv) >= 12)) { > > > > Drop this if-else and only call tgl_compute_sagv_wm() on tgl+. > > The comment can then go as well I think. > > We actually had _agreed_ with you personally that I do copy > wm0 values for other platforms for compatibility reasons. > Now after one month you say that we are going to call this > only for gen12. I see no point in having this populated for pre-tgl. > > > > > > + u32 latency = dev_priv->wm.skl_latency[0]; > > > + > > > + latency += dev_priv->sagv_block_time_us; > > > + skl_compute_plane_wm(crtc_state, 0, latency, > > > + wm_params, &levels[0], > > > + sagv_wm); > > > + DRM_DEBUG_KMS("%d L0 blocks required for SAGV vs %d for non-SAGV\n", > > > + sagv_wm->min_ddb_alloc, levels[0].min_ddb_alloc); > > > > Leftover debugs. Pls remove. > > I thought this might be useful, we should have some understanding what > is happening, i.e what was the reasoning and why SAGV was switched on/off. > Not crucial, but really don't understand why this is _that_ harmful. > We have plenty of wm/ddb debugs anyway, but no way to figure out how SAGV logic is working. If we need more debugs they need to added carefully so as to not totally blow up the logs. I think this here would spam the logs heavily. > > > > > > + } else { > > > + /* Since all members are POD */ > > > + *sagv_wm = levels[0]; > > > + } > > > +} > > > + > > > static void skl_compute_transition_wm(const struct intel_crtc_state *crtc_state, > > > const struct skl_wm_params *wp, > > > struct skl_plane_wm *wm) > > > @@ -5296,6 +5381,8 @@ static int skl_build_plane_wm_single(struct intel_crtc_state *crtc_state, > > > return ret; > > > > > > skl_compute_wm_levels(crtc_state, &wm_params, wm->wm); > > > + if (color_plane == 0) > > > > We do want this for both the Y plane and UV plane. > > > > > + tgl_compute_sagv_wm(crtc_state, &wm_params, wm); > > > skl_compute_transition_wm(crtc_state, &wm_params, wm); > > > > > > return 0; > > > @@ -5702,6 +5789,12 @@ skl_print_wm_changes(struct intel_atomic_state *state) > > > enast(new_wm->wm[7].ignore_lines), new_wm->wm[7].plane_res_l, > > > enast(new_wm->trans_wm.ignore_lines), new_wm->trans_wm.plane_res_l); > > > > > > + drm_dbg_kms(&dev_priv->drm, > > > + "[PLANE:%d:%s] sagv wm0 lines %4d -> %4d\n", > > > + plane->base.base.id, plane->base.name, > > > + old_wm->sagv_wm0.plane_res_l, > > > + new_wm->sagv_wm0.plane_res_l); > > > + > > > > IIRC I already suggested just slapping these onto the existing debug > > prints instead of bloating the debug output with extra lines. > > > > Ie instead of > > wms ... -> ... > > sagv wm ... -> ... > > > > we just get > > wms ...,old_sagv_wm0 -> ...,new_sagv_wm0 > > > > just like we already do for eg. the transition wm. > > Not sure about this one. I will check, I thought you suggested the opposite, > i.e do those debugs separately. > > Stan > > > > > drm_dbg_kms(&dev_priv->drm, > > > "[PLANE:%d:%s] blocks %4d,%4d,%4d,%4d,%4d,%4d,%4d,%4d,%4d" > > > " -> %4d,%4d,%4d,%4d,%4d,%4d,%4d,%4d,%4d\n", > > > @@ -5717,6 +5810,12 @@ skl_print_wm_changes(struct intel_atomic_state *state) > > > new_wm->wm[6].plane_res_b, new_wm->wm[7].plane_res_b, > > > new_wm->trans_wm.plane_res_b); > > > > > > + drm_dbg_kms(&dev_priv->drm, > > > + "[PLANE:%d:%s] sagv wm0 blocks %4d -> %4d\n", > > > + plane->base.base.id, plane->base.name, > > > + old_wm->sagv_wm0.plane_res_b, > > > + new_wm->sagv_wm0.plane_res_b); > > > + > > > drm_dbg_kms(&dev_priv->drm, > > > "[PLANE:%d:%s] min_ddb %4d,%4d,%4d,%4d,%4d,%4d,%4d,%4d,%4d" > > > " -> %4d,%4d,%4d,%4d,%4d,%4d,%4d,%4d,%4d\n", > > > @@ -5731,6 +5830,12 @@ skl_print_wm_changes(struct intel_atomic_state *state) > > > new_wm->wm[4].min_ddb_alloc, new_wm->wm[5].min_ddb_alloc, > > > new_wm->wm[6].min_ddb_alloc, new_wm->wm[7].min_ddb_alloc, > > > new_wm->trans_wm.min_ddb_alloc); > > > + > > > + drm_dbg_kms(&dev_priv->drm, > > > + "[PLANE:%d:%s] sagv wm0 min ddb %4d -> %4d\n", > > > + plane->base.base.id, plane->base.name, > > > + old_wm->sagv_wm0.min_ddb_alloc, > > > + new_wm->sagv_wm0.min_ddb_alloc); > > > } > > > } > > > } > > > @@ -6023,6 +6128,9 @@ void skl_pipe_wm_get_hw_state(struct intel_crtc *crtc, > > > skl_wm_level_from_reg_val(val, &wm->wm[level]); > > > } > > > > > > + memcpy(&wm->sagv_wm0, &wm->wm[0], > > > + sizeof(struct skl_wm_level)); > > > > A simple assignment should suffice? > > Agree. > > > > > > + > > > if (plane_id != PLANE_CURSOR) > > > val = I915_READ(PLANE_WM_TRANS(pipe, plane_id)); > > > else > > > -- > > > 2.24.1.485.gad05a3d8e5 > > > > -- > > Ville Syrjälä > > Intel
On Tue, May 12, 2020 at 04:10:46PM +0300, Ville Syrjälä wrote: > On Tue, May 12, 2020 at 03:52:27PM +0300, Lisovskiy, Stanislav wrote: > > On Tue, May 12, 2020 at 03:03:26PM +0300, Ville Syrjälä wrote: > > > On Thu, May 07, 2020 at 05:45:01PM +0300, Stanislav Lisovskiy wrote: > > > > Starting from TGL we need to have a separate wm0 > > > > values for SAGV and non-SAGV which affects > > > > how calculations are done. > > > > > > > > v2: Remove long lines > > > > v3: Removed COLOR_PLANE enum references > > > > v4, v5, v6: Fixed rebase conflict > > > > v7: - Removed skl_plane_wm_level accessor from skl_allocate_pipe_ddb(Ville) > > > > - Removed sagv_uv_wm0(Ville) > > > > - can_sagv->use_sagv_wm(Ville) > > > > > > > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > > > > --- > > > > drivers/gpu/drm/i915/display/intel_display.c | 8 +- > > > > .../drm/i915/display/intel_display_types.h | 2 + > > > > drivers/gpu/drm/i915/intel_pm.c | 118 +++++++++++++++++- > > > > 3 files changed, 121 insertions(+), 7 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c > > > > index fd6d63b03489..be5741cb7595 100644 > > > > --- a/drivers/gpu/drm/i915/display/intel_display.c > > > > +++ b/drivers/gpu/drm/i915/display/intel_display.c > > > > @@ -13961,7 +13961,9 @@ static void verify_wm_state(struct intel_crtc *crtc, > > > > /* Watermarks */ > > > > for (level = 0; level <= max_level; level++) { > > > > if (skl_wm_level_equals(&hw_plane_wm->wm[level], > > > > - &sw_plane_wm->wm[level])) > > > > + &sw_plane_wm->wm[level]) || > > > > + (level == 0 && skl_wm_level_equals(&hw_plane_wm->wm[level], > > > > + &sw_plane_wm->sagv_wm0))) > > > > continue; > > > > > > > > drm_err(&dev_priv->drm, > > > > @@ -14016,7 +14018,9 @@ static void verify_wm_state(struct intel_crtc *crtc, > > > > /* Watermarks */ > > > > for (level = 0; level <= max_level; level++) { > > > > if (skl_wm_level_equals(&hw_plane_wm->wm[level], > > > > - &sw_plane_wm->wm[level])) > > > > + &sw_plane_wm->wm[level]) || > > > > + (level == 0 && skl_wm_level_equals(&hw_plane_wm->wm[level], > > > > + &sw_plane_wm->sagv_wm0))) > > > > continue; > > > > > > > > drm_err(&dev_priv->drm, > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h > > > > index 9488449e4b94..8cede29c9562 100644 > > > > --- a/drivers/gpu/drm/i915/display/intel_display_types.h > > > > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h > > > > @@ -688,11 +688,13 @@ struct skl_plane_wm { > > > > struct skl_wm_level wm[8]; > > > > struct skl_wm_level uv_wm[8]; > > > > struct skl_wm_level trans_wm; > > > > + struct skl_wm_level sagv_wm0; > > > > bool is_planar; > > > > }; > > > > > > > > struct skl_pipe_wm { > > > > struct skl_plane_wm planes[I915_MAX_PLANES]; > > > > + bool use_sagv_wm; > > > > }; > > > > > > > > enum vlv_wm_level { > > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > > > > index db188efee21e..934a686342ad 100644 > > > > --- a/drivers/gpu/drm/i915/intel_pm.c > > > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > > > @@ -3863,25 +3863,35 @@ bool intel_can_enable_sagv(struct drm_i915_private *dev_priv, > > > > return bw_state->pipe_sagv_reject == 0; > > > > } > > > > > > > > +static bool > > > > +tgl_crtc_can_enable_sagv(const struct intel_crtc_state *crtc_state); > > > > > > Just put the function here instead of adding fwd decalrations. > > > > > > > + > > > > static int intel_compute_sagv_mask(struct intel_atomic_state *state) > > > > { > > > > struct drm_i915_private *dev_priv = to_i915(state->base.dev); > > > > int ret; > > > > struct intel_crtc *crtc; > > > > - const struct intel_crtc_state *new_crtc_state; > > > > + struct intel_crtc_state *new_crtc_state; > > > > struct intel_bw_state *new_bw_state = NULL; > > > > const struct intel_bw_state *old_bw_state = NULL; > > > > int i; > > > > > > > > for_each_new_intel_crtc_in_state(state, crtc, > > > > new_crtc_state, i) { > > > > + bool can_sagv; > > > > + > > > > new_bw_state = intel_atomic_get_bw_state(state); > > > > if (IS_ERR(new_bw_state)) > > > > return PTR_ERR(new_bw_state); > > > > > > > > old_bw_state = intel_atomic_get_old_bw_state(state); > > > > > > > > - if (skl_crtc_can_enable_sagv(new_crtc_state)) > > > > + if (INTEL_GEN(dev_priv) >= 12) > > > > + can_sagv = tgl_crtc_can_enable_sagv(new_crtc_state); > > > > + else > > > > + can_sagv = skl_crtc_can_enable_sagv(new_crtc_state); > > > > + > > > > + if (can_sagv) > > > > new_bw_state->pipe_sagv_reject &= ~BIT(crtc->pipe); > > > > else > > > > new_bw_state->pipe_sagv_reject |= BIT(crtc->pipe); > > > > @@ -3899,6 +3909,24 @@ static int intel_compute_sagv_mask(struct intel_atomic_state *state) > > > > return ret; > > > > } > > > > > > > > + for_each_new_intel_crtc_in_state(state, crtc, > > > > + new_crtc_state, i) { > > > > + struct skl_pipe_wm *pipe_wm = &new_crtc_state->wm.skl.optimal; > > > > + > > > > + /* > > > > + * Due to drm limitation at commit state, when > > > > + * changes are written the whole atomic state is > > > > + * zeroed away => which prevents from using it, > > > > + * so just sticking it into pipe wm state for > > > > + * keeping it simple - anyway this is related to wm. > > > > + * Proper way in ideal universe would be of course not > > > > + * to lose parent atomic state object from child crtc_state, > > > > + * and stick to OOP programming principles, which had been > > > > + * scientifically proven to work. > > > > + */ > > > > > > More ramblings. Just drop this comment too imo. > > > > As I understand what is happening here is rather weird, so I thought > > commenting is good idea. > > At least I have no idea what the comment is trying to say. > I see nothing weird hapening here, we're just computing the > state which is totally standard stuff. Well I can remind, this is because there is no way to get parent state from crtc_state, because of weird drm atomic behaviour which leaves us with NULL parent state. So that we had to stick this boolean to some place. In normal code universe this wouldn't even be needed if we could just get atomic state from crtc_state when we write wm in skl_write_plane_wm. However probably OOP principles like parent-child hieararchy is not a thing here. That what this comment was trying to say. In normal OOP you can't destroy parent object _before_ destroying child one. Stan > > > > > > > > > > + pipe_wm->use_sagv_wm = intel_can_enable_sagv(dev_priv, new_bw_state); > > > > > > I think this should be > > > gen >= 12 && intel_can_enable_sagv(); > > > > > > > + } > > > > + > > > > if (intel_can_enable_sagv(dev_priv, new_bw_state) != intel_can_enable_sagv(dev_priv, old_bw_state)) { > > > > ret = intel_atomic_serialize_global_state(&new_bw_state->base); > > > > if (ret) > > > > @@ -4642,12 +4670,39 @@ skl_plane_wm_level(const struct intel_crtc_state *crtc_state, > > > > int level, > > > > int color_plane) > > > > { > > > > - const struct skl_plane_wm *wm = > > > > - &crtc_state->wm.skl.optimal.planes[plane_id]; > > > > + const struct skl_pipe_wm *pipe_wm = &crtc_state->wm.skl.optimal; > > > > + const struct skl_plane_wm *wm = &pipe_wm->planes[plane_id]; > > > > + > > > > + if (!level) { > > > > + if (pipe_wm->use_sagv_wm) > > > > + return &wm->sagv_wm0; > > > > + } > > > > > > if (level == 0 && use_sagv_wm) > > > return sagv_wm0; > > > > > > > > > > > return color_plane == 0 ? &wm->wm[level] : &wm->uv_wm[level]; > > > > } > > > > > > > > +static bool > > > > +tgl_crtc_can_enable_sagv(const struct intel_crtc_state *crtc_state) > > > > +{ > > > > + struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc); > > > > + enum plane_id plane_id; > > > > + > > > > + if (!crtc_state->hw.active) > > > > + return true; > > > > + > > > > + for_each_plane_id_on_crtc(crtc, plane_id) { > > > > + const struct skl_ddb_entry *plane_alloc = > > > > + &crtc_state->wm.skl.plane_ddb_y[plane_id]; > > > > + const struct skl_plane_wm *wm = > > > > + &crtc_state->wm.skl.optimal.planes[plane_id]; > > > > + > > > > + if (skl_ddb_entry_size(plane_alloc) < wm->sagv_wm0.min_ddb_alloc) > > > > + return false; > > > > + } > > > > + > > > > + return true; > > > > +} > > > > + > > > > static int > > > > skl_allocate_pipe_ddb(struct intel_crtc_state *crtc_state) > > > > { > > > > @@ -4684,7 +4739,6 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *crtc_state) > > > > plane_data_rate, > > > > uv_plane_data_rate); > > > > > > > > - > > > > skl_ddb_get_pipe_allocation_limits(dev_priv, crtc_state, total_data_rate, > > > > alloc, &num_active); > > > > alloc_size = skl_ddb_entry_size(alloc); > > > > @@ -5219,6 +5273,37 @@ skl_compute_wm_levels(const struct intel_crtc_state *crtc_state, > > > > } > > > > } > > > > > > > > +static void tgl_compute_sagv_wm(const struct intel_crtc_state *crtc_state, > > > > + const struct skl_wm_params *wm_params, > > > > + struct skl_plane_wm *plane_wm) > > > > +{ > > > > + struct drm_i915_private *dev_priv = to_i915(crtc_state->uapi.crtc->dev); > > > > + struct skl_wm_level *sagv_wm = &plane_wm->sagv_wm0; > > > > + struct skl_wm_level *levels = plane_wm->wm; > > > > + > > > > + /* > > > > + * For Gen12 if it is an L0 we need to also > > > > + * consider sagv_block_time when calculating > > > > + * L0 watermark - we will need that when making > > > > + * a decision whether enable SAGV or not. > > > > + * For older gens we agreed to copy L0 value for > > > > + * compatibility. > > > > + */ > > > > + if ((INTEL_GEN(dev_priv) >= 12)) { > > > > > > Drop this if-else and only call tgl_compute_sagv_wm() on tgl+. > > > The comment can then go as well I think. > > > > We actually had _agreed_ with you personally that I do copy > > wm0 values for other platforms for compatibility reasons. > > Now after one month you say that we are going to call this > > only for gen12. > > I see no point in having this populated for pre-tgl. > > > > > > > > > > + u32 latency = dev_priv->wm.skl_latency[0]; > > > > + > > > > + latency += dev_priv->sagv_block_time_us; > > > > + skl_compute_plane_wm(crtc_state, 0, latency, > > > > + wm_params, &levels[0], > > > > + sagv_wm); > > > > + DRM_DEBUG_KMS("%d L0 blocks required for SAGV vs %d for non-SAGV\n", > > > > + sagv_wm->min_ddb_alloc, levels[0].min_ddb_alloc); > > > > > > Leftover debugs. Pls remove. > > > > I thought this might be useful, we should have some understanding what > > is happening, i.e what was the reasoning and why SAGV was switched on/off. > > Not crucial, but really don't understand why this is _that_ harmful. > > We have plenty of wm/ddb debugs anyway, but no way to figure out how SAGV logic is working. > > If we need more debugs they need to added carefully so as to not totally > blow up the logs. I think this here would spam the logs heavily. > > > > > > > > > > + } else { > > > > + /* Since all members are POD */ > > > > + *sagv_wm = levels[0]; > > > > + } > > > > +} > > > > + > > > > static void skl_compute_transition_wm(const struct intel_crtc_state *crtc_state, > > > > const struct skl_wm_params *wp, > > > > struct skl_plane_wm *wm) > > > > @@ -5296,6 +5381,8 @@ static int skl_build_plane_wm_single(struct intel_crtc_state *crtc_state, > > > > return ret; > > > > > > > > skl_compute_wm_levels(crtc_state, &wm_params, wm->wm); > > > > + if (color_plane == 0) > > > > > > We do want this for both the Y plane and UV plane. > > > > > > > + tgl_compute_sagv_wm(crtc_state, &wm_params, wm); > > > > skl_compute_transition_wm(crtc_state, &wm_params, wm); > > > > > > > > return 0; > > > > @@ -5702,6 +5789,12 @@ skl_print_wm_changes(struct intel_atomic_state *state) > > > > enast(new_wm->wm[7].ignore_lines), new_wm->wm[7].plane_res_l, > > > > enast(new_wm->trans_wm.ignore_lines), new_wm->trans_wm.plane_res_l); > > > > > > > > + drm_dbg_kms(&dev_priv->drm, > > > > + "[PLANE:%d:%s] sagv wm0 lines %4d -> %4d\n", > > > > + plane->base.base.id, plane->base.name, > > > > + old_wm->sagv_wm0.plane_res_l, > > > > + new_wm->sagv_wm0.plane_res_l); > > > > + > > > > > > IIRC I already suggested just slapping these onto the existing debug > > > prints instead of bloating the debug output with extra lines. > > > > > > Ie instead of > > > wms ... -> ... > > > sagv wm ... -> ... > > > > > > we just get > > > wms ...,old_sagv_wm0 -> ...,new_sagv_wm0 > > > > > > just like we already do for eg. the transition wm. > > > > Not sure about this one. I will check, I thought you suggested the opposite, > > i.e do those debugs separately. > > > > Stan > > > > > > > drm_dbg_kms(&dev_priv->drm, > > > > "[PLANE:%d:%s] blocks %4d,%4d,%4d,%4d,%4d,%4d,%4d,%4d,%4d" > > > > " -> %4d,%4d,%4d,%4d,%4d,%4d,%4d,%4d,%4d\n", > > > > @@ -5717,6 +5810,12 @@ skl_print_wm_changes(struct intel_atomic_state *state) > > > > new_wm->wm[6].plane_res_b, new_wm->wm[7].plane_res_b, > > > > new_wm->trans_wm.plane_res_b); > > > > > > > > + drm_dbg_kms(&dev_priv->drm, > > > > + "[PLANE:%d:%s] sagv wm0 blocks %4d -> %4d\n", > > > > + plane->base.base.id, plane->base.name, > > > > + old_wm->sagv_wm0.plane_res_b, > > > > + new_wm->sagv_wm0.plane_res_b); > > > > + > > > > drm_dbg_kms(&dev_priv->drm, > > > > "[PLANE:%d:%s] min_ddb %4d,%4d,%4d,%4d,%4d,%4d,%4d,%4d,%4d" > > > > " -> %4d,%4d,%4d,%4d,%4d,%4d,%4d,%4d,%4d\n", > > > > @@ -5731,6 +5830,12 @@ skl_print_wm_changes(struct intel_atomic_state *state) > > > > new_wm->wm[4].min_ddb_alloc, new_wm->wm[5].min_ddb_alloc, > > > > new_wm->wm[6].min_ddb_alloc, new_wm->wm[7].min_ddb_alloc, > > > > new_wm->trans_wm.min_ddb_alloc); > > > > + > > > > + drm_dbg_kms(&dev_priv->drm, > > > > + "[PLANE:%d:%s] sagv wm0 min ddb %4d -> %4d\n", > > > > + plane->base.base.id, plane->base.name, > > > > + old_wm->sagv_wm0.min_ddb_alloc, > > > > + new_wm->sagv_wm0.min_ddb_alloc); > > > > } > > > > } > > > > } > > > > @@ -6023,6 +6128,9 @@ void skl_pipe_wm_get_hw_state(struct intel_crtc *crtc, > > > > skl_wm_level_from_reg_val(val, &wm->wm[level]); > > > > } > > > > > > > > + memcpy(&wm->sagv_wm0, &wm->wm[0], > > > > + sizeof(struct skl_wm_level)); > > > > > > A simple assignment should suffice? > > > > Agree. > > > > > > > > > + > > > > if (plane_id != PLANE_CURSOR) > > > > val = I915_READ(PLANE_WM_TRANS(pipe, plane_id)); > > > > else > > > > -- > > > > 2.24.1.485.gad05a3d8e5 > > > > > > -- > > > Ville Syrjälä > > > Intel > > -- > Ville Syrjälä > Intel
On Tue, May 12, 2020 at 04:17:34PM +0300, Lisovskiy, Stanislav wrote: > On Tue, May 12, 2020 at 04:10:46PM +0300, Ville Syrjälä wrote: > > On Tue, May 12, 2020 at 03:52:27PM +0300, Lisovskiy, Stanislav wrote: > > > On Tue, May 12, 2020 at 03:03:26PM +0300, Ville Syrjälä wrote: > > > > On Thu, May 07, 2020 at 05:45:01PM +0300, Stanislav Lisovskiy wrote: > > > > > Starting from TGL we need to have a separate wm0 > > > > > values for SAGV and non-SAGV which affects > > > > > how calculations are done. > > > > > > > > > > v2: Remove long lines > > > > > v3: Removed COLOR_PLANE enum references > > > > > v4, v5, v6: Fixed rebase conflict > > > > > v7: - Removed skl_plane_wm_level accessor from skl_allocate_pipe_ddb(Ville) > > > > > - Removed sagv_uv_wm0(Ville) > > > > > - can_sagv->use_sagv_wm(Ville) > > > > > > > > > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > > > > > --- > > > > > drivers/gpu/drm/i915/display/intel_display.c | 8 +- > > > > > .../drm/i915/display/intel_display_types.h | 2 + > > > > > drivers/gpu/drm/i915/intel_pm.c | 118 +++++++++++++++++- > > > > > 3 files changed, 121 insertions(+), 7 deletions(-) > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c > > > > > index fd6d63b03489..be5741cb7595 100644 > > > > > --- a/drivers/gpu/drm/i915/display/intel_display.c > > > > > +++ b/drivers/gpu/drm/i915/display/intel_display.c > > > > > @@ -13961,7 +13961,9 @@ static void verify_wm_state(struct intel_crtc *crtc, > > > > > /* Watermarks */ > > > > > for (level = 0; level <= max_level; level++) { > > > > > if (skl_wm_level_equals(&hw_plane_wm->wm[level], > > > > > - &sw_plane_wm->wm[level])) > > > > > + &sw_plane_wm->wm[level]) || > > > > > + (level == 0 && skl_wm_level_equals(&hw_plane_wm->wm[level], > > > > > + &sw_plane_wm->sagv_wm0))) > > > > > continue; > > > > > > > > > > drm_err(&dev_priv->drm, > > > > > @@ -14016,7 +14018,9 @@ static void verify_wm_state(struct intel_crtc *crtc, > > > > > /* Watermarks */ > > > > > for (level = 0; level <= max_level; level++) { > > > > > if (skl_wm_level_equals(&hw_plane_wm->wm[level], > > > > > - &sw_plane_wm->wm[level])) > > > > > + &sw_plane_wm->wm[level]) || > > > > > + (level == 0 && skl_wm_level_equals(&hw_plane_wm->wm[level], > > > > > + &sw_plane_wm->sagv_wm0))) > > > > > continue; > > > > > > > > > > drm_err(&dev_priv->drm, > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h > > > > > index 9488449e4b94..8cede29c9562 100644 > > > > > --- a/drivers/gpu/drm/i915/display/intel_display_types.h > > > > > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h > > > > > @@ -688,11 +688,13 @@ struct skl_plane_wm { > > > > > struct skl_wm_level wm[8]; > > > > > struct skl_wm_level uv_wm[8]; > > > > > struct skl_wm_level trans_wm; > > > > > + struct skl_wm_level sagv_wm0; > > > > > bool is_planar; > > > > > }; > > > > > > > > > > struct skl_pipe_wm { > > > > > struct skl_plane_wm planes[I915_MAX_PLANES]; > > > > > + bool use_sagv_wm; > > > > > }; > > > > > > > > > > enum vlv_wm_level { > > > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > > > > > index db188efee21e..934a686342ad 100644 > > > > > --- a/drivers/gpu/drm/i915/intel_pm.c > > > > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > > > > @@ -3863,25 +3863,35 @@ bool intel_can_enable_sagv(struct drm_i915_private *dev_priv, > > > > > return bw_state->pipe_sagv_reject == 0; > > > > > } > > > > > > > > > > +static bool > > > > > +tgl_crtc_can_enable_sagv(const struct intel_crtc_state *crtc_state); > > > > > > > > Just put the function here instead of adding fwd decalrations. > > > > > > > > > + > > > > > static int intel_compute_sagv_mask(struct intel_atomic_state *state) > > > > > { > > > > > struct drm_i915_private *dev_priv = to_i915(state->base.dev); > > > > > int ret; > > > > > struct intel_crtc *crtc; > > > > > - const struct intel_crtc_state *new_crtc_state; > > > > > + struct intel_crtc_state *new_crtc_state; > > > > > struct intel_bw_state *new_bw_state = NULL; > > > > > const struct intel_bw_state *old_bw_state = NULL; > > > > > int i; > > > > > > > > > > for_each_new_intel_crtc_in_state(state, crtc, > > > > > new_crtc_state, i) { > > > > > + bool can_sagv; > > > > > + > > > > > new_bw_state = intel_atomic_get_bw_state(state); > > > > > if (IS_ERR(new_bw_state)) > > > > > return PTR_ERR(new_bw_state); > > > > > > > > > > old_bw_state = intel_atomic_get_old_bw_state(state); > > > > > > > > > > - if (skl_crtc_can_enable_sagv(new_crtc_state)) > > > > > + if (INTEL_GEN(dev_priv) >= 12) > > > > > + can_sagv = tgl_crtc_can_enable_sagv(new_crtc_state); > > > > > + else > > > > > + can_sagv = skl_crtc_can_enable_sagv(new_crtc_state); > > > > > + > > > > > + if (can_sagv) > > > > > new_bw_state->pipe_sagv_reject &= ~BIT(crtc->pipe); > > > > > else > > > > > new_bw_state->pipe_sagv_reject |= BIT(crtc->pipe); > > > > > @@ -3899,6 +3909,24 @@ static int intel_compute_sagv_mask(struct intel_atomic_state *state) > > > > > return ret; > > > > > } > > > > > > > > > > + for_each_new_intel_crtc_in_state(state, crtc, > > > > > + new_crtc_state, i) { > > > > > + struct skl_pipe_wm *pipe_wm = &new_crtc_state->wm.skl.optimal; > > > > > + > > > > > + /* > > > > > + * Due to drm limitation at commit state, when > > > > > + * changes are written the whole atomic state is > > > > > + * zeroed away => which prevents from using it, > > > > > + * so just sticking it into pipe wm state for > > > > > + * keeping it simple - anyway this is related to wm. > > > > > + * Proper way in ideal universe would be of course not > > > > > + * to lose parent atomic state object from child crtc_state, > > > > > + * and stick to OOP programming principles, which had been > > > > > + * scientifically proven to work. > > > > > + */ > > > > > > > > More ramblings. Just drop this comment too imo. > > > > > > As I understand what is happening here is rather weird, so I thought > > > commenting is good idea. > > > > At least I have no idea what the comment is trying to say. > > I see nothing weird hapening here, we're just computing the > > state which is totally standard stuff. > > Well I can remind, this is because there is no way to get parent state > from crtc_state, because of weird drm atomic behaviour which leaves us > with NULL parent state. So that we had to stick this boolean to some > place. > In normal code universe this wouldn't even be needed if we could > just get atomic state from crtc_state when we write wm in skl_write_plane_wm. Didn't get that at all from the comment. It talked about zeroing some whole state which I took as 'memset(state, 0, sizeof(*state))'. As that is not what's going on I just got confused by the comment. Now that I understand what this is about I think the comment (if we want to have one) should probably say something more like: "we store use_sagv_wm in the crtc state rather than relying on the bw state since we have no convenient way to get at the latter from the plane commit hooks (especially in the legacy cursor case)". > > However probably OOP principles like parent-child hieararchy is not a thing > here. That what this comment was trying to say. > > In normal OOP you can't destroy parent object _before_ destroying > child one. There's no parent-child relationship between the crtc state and atomic state (which really shouldn't be called a state to begin with, rather it should be "transaction" or something along those lines).
On Tue, May 12, 2020 at 04:38:21PM +0300, Ville Syrjälä wrote: > On Tue, May 12, 2020 at 04:17:34PM +0300, Lisovskiy, Stanislav wrote: > > On Tue, May 12, 2020 at 04:10:46PM +0300, Ville Syrjälä wrote: > > > On Tue, May 12, 2020 at 03:52:27PM +0300, Lisovskiy, Stanislav wrote: > > > > On Tue, May 12, 2020 at 03:03:26PM +0300, Ville Syrjälä wrote: > > > > > On Thu, May 07, 2020 at 05:45:01PM +0300, Stanislav Lisovskiy wrote: > > > > > > Starting from TGL we need to have a separate wm0 > > > > > > values for SAGV and non-SAGV which affects > > > > > > how calculations are done. > > > > > > > > > > > > v2: Remove long lines > > > > > > v3: Removed COLOR_PLANE enum references > > > > > > v4, v5, v6: Fixed rebase conflict > > > > > > v7: - Removed skl_plane_wm_level accessor from skl_allocate_pipe_ddb(Ville) > > > > > > - Removed sagv_uv_wm0(Ville) > > > > > > - can_sagv->use_sagv_wm(Ville) > > > > > > > > > > > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > > > > > > --- > > > > > > drivers/gpu/drm/i915/display/intel_display.c | 8 +- > > > > > > .../drm/i915/display/intel_display_types.h | 2 + > > > > > > drivers/gpu/drm/i915/intel_pm.c | 118 +++++++++++++++++- > > > > > > 3 files changed, 121 insertions(+), 7 deletions(-) > > > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c > > > > > > index fd6d63b03489..be5741cb7595 100644 > > > > > > --- a/drivers/gpu/drm/i915/display/intel_display.c > > > > > > +++ b/drivers/gpu/drm/i915/display/intel_display.c > > > > > > @@ -13961,7 +13961,9 @@ static void verify_wm_state(struct intel_crtc *crtc, > > > > > > /* Watermarks */ > > > > > > for (level = 0; level <= max_level; level++) { > > > > > > if (skl_wm_level_equals(&hw_plane_wm->wm[level], > > > > > > - &sw_plane_wm->wm[level])) > > > > > > + &sw_plane_wm->wm[level]) || > > > > > > + (level == 0 && skl_wm_level_equals(&hw_plane_wm->wm[level], > > > > > > + &sw_plane_wm->sagv_wm0))) > > > > > > continue; > > > > > > > > > > > > drm_err(&dev_priv->drm, > > > > > > @@ -14016,7 +14018,9 @@ static void verify_wm_state(struct intel_crtc *crtc, > > > > > > /* Watermarks */ > > > > > > for (level = 0; level <= max_level; level++) { > > > > > > if (skl_wm_level_equals(&hw_plane_wm->wm[level], > > > > > > - &sw_plane_wm->wm[level])) > > > > > > + &sw_plane_wm->wm[level]) || > > > > > > + (level == 0 && skl_wm_level_equals(&hw_plane_wm->wm[level], > > > > > > + &sw_plane_wm->sagv_wm0))) > > > > > > continue; > > > > > > > > > > > > drm_err(&dev_priv->drm, > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h > > > > > > index 9488449e4b94..8cede29c9562 100644 > > > > > > --- a/drivers/gpu/drm/i915/display/intel_display_types.h > > > > > > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h > > > > > > @@ -688,11 +688,13 @@ struct skl_plane_wm { > > > > > > struct skl_wm_level wm[8]; > > > > > > struct skl_wm_level uv_wm[8]; > > > > > > struct skl_wm_level trans_wm; > > > > > > + struct skl_wm_level sagv_wm0; > > > > > > bool is_planar; > > > > > > }; > > > > > > > > > > > > struct skl_pipe_wm { > > > > > > struct skl_plane_wm planes[I915_MAX_PLANES]; > > > > > > + bool use_sagv_wm; > > > > > > }; > > > > > > > > > > > > enum vlv_wm_level { > > > > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > > > > > > index db188efee21e..934a686342ad 100644 > > > > > > --- a/drivers/gpu/drm/i915/intel_pm.c > > > > > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > > > > > @@ -3863,25 +3863,35 @@ bool intel_can_enable_sagv(struct drm_i915_private *dev_priv, > > > > > > return bw_state->pipe_sagv_reject == 0; > > > > > > } > > > > > > > > > > > > +static bool > > > > > > +tgl_crtc_can_enable_sagv(const struct intel_crtc_state *crtc_state); > > > > > > > > > > Just put the function here instead of adding fwd decalrations. > > > > > > > > > > > + > > > > > > static int intel_compute_sagv_mask(struct intel_atomic_state *state) > > > > > > { > > > > > > struct drm_i915_private *dev_priv = to_i915(state->base.dev); > > > > > > int ret; > > > > > > struct intel_crtc *crtc; > > > > > > - const struct intel_crtc_state *new_crtc_state; > > > > > > + struct intel_crtc_state *new_crtc_state; > > > > > > struct intel_bw_state *new_bw_state = NULL; > > > > > > const struct intel_bw_state *old_bw_state = NULL; > > > > > > int i; > > > > > > > > > > > > for_each_new_intel_crtc_in_state(state, crtc, > > > > > > new_crtc_state, i) { > > > > > > + bool can_sagv; > > > > > > + > > > > > > new_bw_state = intel_atomic_get_bw_state(state); > > > > > > if (IS_ERR(new_bw_state)) > > > > > > return PTR_ERR(new_bw_state); > > > > > > > > > > > > old_bw_state = intel_atomic_get_old_bw_state(state); > > > > > > > > > > > > - if (skl_crtc_can_enable_sagv(new_crtc_state)) > > > > > > + if (INTEL_GEN(dev_priv) >= 12) > > > > > > + can_sagv = tgl_crtc_can_enable_sagv(new_crtc_state); > > > > > > + else > > > > > > + can_sagv = skl_crtc_can_enable_sagv(new_crtc_state); > > > > > > + > > > > > > + if (can_sagv) > > > > > > new_bw_state->pipe_sagv_reject &= ~BIT(crtc->pipe); > > > > > > else > > > > > > new_bw_state->pipe_sagv_reject |= BIT(crtc->pipe); > > > > > > @@ -3899,6 +3909,24 @@ static int intel_compute_sagv_mask(struct intel_atomic_state *state) > > > > > > return ret; > > > > > > } > > > > > > > > > > > > + for_each_new_intel_crtc_in_state(state, crtc, > > > > > > + new_crtc_state, i) { > > > > > > + struct skl_pipe_wm *pipe_wm = &new_crtc_state->wm.skl.optimal; > > > > > > + > > > > > > + /* > > > > > > + * Due to drm limitation at commit state, when > > > > > > + * changes are written the whole atomic state is > > > > > > + * zeroed away => which prevents from using it, > > > > > > + * so just sticking it into pipe wm state for > > > > > > + * keeping it simple - anyway this is related to wm. > > > > > > + * Proper way in ideal universe would be of course not > > > > > > + * to lose parent atomic state object from child crtc_state, > > > > > > + * and stick to OOP programming principles, which had been > > > > > > + * scientifically proven to work. > > > > > > + */ > > > > > > > > > > More ramblings. Just drop this comment too imo. > > > > > > > > As I understand what is happening here is rather weird, so I thought > > > > commenting is good idea. > > > > > > At least I have no idea what the comment is trying to say. > > > I see nothing weird hapening here, we're just computing the > > > state which is totally standard stuff. > > > > Well I can remind, this is because there is no way to get parent state > > from crtc_state, because of weird drm atomic behaviour which leaves us > > with NULL parent state. So that we had to stick this boolean to some > > place. > > In normal code universe this wouldn't even be needed if we could > > just get atomic state from crtc_state when we write wm in skl_write_plane_wm. > > Didn't get that at all from the comment. It talked about zeroing some > whole state which I took as 'memset(state, 0, sizeof(*state))'. > As that is not what's going on I just got confused by the comment. > > Now that I understand what this is about I think the comment > (if we want to have one) should probably say something more like: > "we store use_sagv_wm in the crtc state rather than relying on > the bw state since we have no convenient way to get at the > latter from the plane commit hooks (especially in the legacy > cursor case)". > > > > > However probably OOP principles like parent-child hieararchy is not a thing > > here. That what this comment was trying to say. > > > > In normal OOP you can't destroy parent object _before_ destroying > > child one. > > There's no parent-child relationship between the crtc state and atomic > state (which really shouldn't be called a state to begin with, rather > it should be "transaction" or something along those lines). In practice there is. crtc_state is being aggregated and contained as part of more general atomic state. That is exactly what parent-child object relation is. If you want to decouple those, this needs to be not aggregation but a reference, i.e atomic state would not contain those state objects, but have a pointer instead, but then you would not be using that container_of magic. Stan > > -- > Ville Syrjälä > Intel
On Tue, May 12, 2020 at 04:41:26PM +0300, Lisovskiy, Stanislav wrote: > On Tue, May 12, 2020 at 04:38:21PM +0300, Ville Syrjälä wrote: > > On Tue, May 12, 2020 at 04:17:34PM +0300, Lisovskiy, Stanislav wrote: > > > On Tue, May 12, 2020 at 04:10:46PM +0300, Ville Syrjälä wrote: > > > > On Tue, May 12, 2020 at 03:52:27PM +0300, Lisovskiy, Stanislav wrote: > > > > > On Tue, May 12, 2020 at 03:03:26PM +0300, Ville Syrjälä wrote: > > > > > > On Thu, May 07, 2020 at 05:45:01PM +0300, Stanislav Lisovskiy wrote: > > > > > > > Starting from TGL we need to have a separate wm0 > > > > > > > values for SAGV and non-SAGV which affects > > > > > > > how calculations are done. > > > > > > > > > > > > > > v2: Remove long lines > > > > > > > v3: Removed COLOR_PLANE enum references > > > > > > > v4, v5, v6: Fixed rebase conflict > > > > > > > v7: - Removed skl_plane_wm_level accessor from skl_allocate_pipe_ddb(Ville) > > > > > > > - Removed sagv_uv_wm0(Ville) > > > > > > > - can_sagv->use_sagv_wm(Ville) > > > > > > > > > > > > > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > > > > > > > --- > > > > > > > drivers/gpu/drm/i915/display/intel_display.c | 8 +- > > > > > > > .../drm/i915/display/intel_display_types.h | 2 + > > > > > > > drivers/gpu/drm/i915/intel_pm.c | 118 +++++++++++++++++- > > > > > > > 3 files changed, 121 insertions(+), 7 deletions(-) > > > > > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c > > > > > > > index fd6d63b03489..be5741cb7595 100644 > > > > > > > --- a/drivers/gpu/drm/i915/display/intel_display.c > > > > > > > +++ b/drivers/gpu/drm/i915/display/intel_display.c > > > > > > > @@ -13961,7 +13961,9 @@ static void verify_wm_state(struct intel_crtc *crtc, > > > > > > > /* Watermarks */ > > > > > > > for (level = 0; level <= max_level; level++) { > > > > > > > if (skl_wm_level_equals(&hw_plane_wm->wm[level], > > > > > > > - &sw_plane_wm->wm[level])) > > > > > > > + &sw_plane_wm->wm[level]) || > > > > > > > + (level == 0 && skl_wm_level_equals(&hw_plane_wm->wm[level], > > > > > > > + &sw_plane_wm->sagv_wm0))) > > > > > > > continue; > > > > > > > > > > > > > > drm_err(&dev_priv->drm, > > > > > > > @@ -14016,7 +14018,9 @@ static void verify_wm_state(struct intel_crtc *crtc, > > > > > > > /* Watermarks */ > > > > > > > for (level = 0; level <= max_level; level++) { > > > > > > > if (skl_wm_level_equals(&hw_plane_wm->wm[level], > > > > > > > - &sw_plane_wm->wm[level])) > > > > > > > + &sw_plane_wm->wm[level]) || > > > > > > > + (level == 0 && skl_wm_level_equals(&hw_plane_wm->wm[level], > > > > > > > + &sw_plane_wm->sagv_wm0))) > > > > > > > continue; > > > > > > > > > > > > > > drm_err(&dev_priv->drm, > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h > > > > > > > index 9488449e4b94..8cede29c9562 100644 > > > > > > > --- a/drivers/gpu/drm/i915/display/intel_display_types.h > > > > > > > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h > > > > > > > @@ -688,11 +688,13 @@ struct skl_plane_wm { > > > > > > > struct skl_wm_level wm[8]; > > > > > > > struct skl_wm_level uv_wm[8]; > > > > > > > struct skl_wm_level trans_wm; > > > > > > > + struct skl_wm_level sagv_wm0; > > > > > > > bool is_planar; > > > > > > > }; > > > > > > > > > > > > > > struct skl_pipe_wm { > > > > > > > struct skl_plane_wm planes[I915_MAX_PLANES]; > > > > > > > + bool use_sagv_wm; > > > > > > > }; > > > > > > > > > > > > > > enum vlv_wm_level { > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > > > > > > > index db188efee21e..934a686342ad 100644 > > > > > > > --- a/drivers/gpu/drm/i915/intel_pm.c > > > > > > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > > > > > > @@ -3863,25 +3863,35 @@ bool intel_can_enable_sagv(struct drm_i915_private *dev_priv, > > > > > > > return bw_state->pipe_sagv_reject == 0; > > > > > > > } > > > > > > > > > > > > > > +static bool > > > > > > > +tgl_crtc_can_enable_sagv(const struct intel_crtc_state *crtc_state); > > > > > > > > > > > > Just put the function here instead of adding fwd decalrations. > > > > > > > > > > > > > + > > > > > > > static int intel_compute_sagv_mask(struct intel_atomic_state *state) > > > > > > > { > > > > > > > struct drm_i915_private *dev_priv = to_i915(state->base.dev); > > > > > > > int ret; > > > > > > > struct intel_crtc *crtc; > > > > > > > - const struct intel_crtc_state *new_crtc_state; > > > > > > > + struct intel_crtc_state *new_crtc_state; > > > > > > > struct intel_bw_state *new_bw_state = NULL; > > > > > > > const struct intel_bw_state *old_bw_state = NULL; > > > > > > > int i; > > > > > > > > > > > > > > for_each_new_intel_crtc_in_state(state, crtc, > > > > > > > new_crtc_state, i) { > > > > > > > + bool can_sagv; > > > > > > > + > > > > > > > new_bw_state = intel_atomic_get_bw_state(state); > > > > > > > if (IS_ERR(new_bw_state)) > > > > > > > return PTR_ERR(new_bw_state); > > > > > > > > > > > > > > old_bw_state = intel_atomic_get_old_bw_state(state); > > > > > > > > > > > > > > - if (skl_crtc_can_enable_sagv(new_crtc_state)) > > > > > > > + if (INTEL_GEN(dev_priv) >= 12) > > > > > > > + can_sagv = tgl_crtc_can_enable_sagv(new_crtc_state); > > > > > > > + else > > > > > > > + can_sagv = skl_crtc_can_enable_sagv(new_crtc_state); > > > > > > > + > > > > > > > + if (can_sagv) > > > > > > > new_bw_state->pipe_sagv_reject &= ~BIT(crtc->pipe); > > > > > > > else > > > > > > > new_bw_state->pipe_sagv_reject |= BIT(crtc->pipe); > > > > > > > @@ -3899,6 +3909,24 @@ static int intel_compute_sagv_mask(struct intel_atomic_state *state) > > > > > > > return ret; > > > > > > > } > > > > > > > > > > > > > > + for_each_new_intel_crtc_in_state(state, crtc, > > > > > > > + new_crtc_state, i) { > > > > > > > + struct skl_pipe_wm *pipe_wm = &new_crtc_state->wm.skl.optimal; > > > > > > > + > > > > > > > + /* > > > > > > > + * Due to drm limitation at commit state, when > > > > > > > + * changes are written the whole atomic state is > > > > > > > + * zeroed away => which prevents from using it, > > > > > > > + * so just sticking it into pipe wm state for > > > > > > > + * keeping it simple - anyway this is related to wm. > > > > > > > + * Proper way in ideal universe would be of course not > > > > > > > + * to lose parent atomic state object from child crtc_state, > > > > > > > + * and stick to OOP programming principles, which had been > > > > > > > + * scientifically proven to work. > > > > > > > + */ > > > > > > > > > > > > More ramblings. Just drop this comment too imo. > > > > > > > > > > As I understand what is happening here is rather weird, so I thought > > > > > commenting is good idea. > > > > > > > > At least I have no idea what the comment is trying to say. > > > > I see nothing weird hapening here, we're just computing the > > > > state which is totally standard stuff. > > > > > > Well I can remind, this is because there is no way to get parent state > > > from crtc_state, because of weird drm atomic behaviour which leaves us > > > with NULL parent state. So that we had to stick this boolean to some > > > place. > > > In normal code universe this wouldn't even be needed if we could > > > just get atomic state from crtc_state when we write wm in skl_write_plane_wm. > > > > Didn't get that at all from the comment. It talked about zeroing some > > whole state which I took as 'memset(state, 0, sizeof(*state))'. > > As that is not what's going on I just got confused by the comment. > > > > Now that I understand what this is about I think the comment > > (if we want to have one) should probably say something more like: > > "we store use_sagv_wm in the crtc state rather than relying on > > the bw state since we have no convenient way to get at the > > latter from the plane commit hooks (especially in the legacy > > cursor case)". > > > > > > > > However probably OOP principles like parent-child hieararchy is not a thing > > > here. That what this comment was trying to say. > > > > > > In normal OOP you can't destroy parent object _before_ destroying > > > child one. > > > > There's no parent-child relationship between the crtc state and atomic > > state (which really shouldn't be called a state to begin with, rather > > it should be "transaction" or something along those lines). > > In practice there is. crtc_state is being aggregated and contained as > part of more general atomic state. That is exactly what parent-child > object relation is. > If you want to decouple those, this needs to be not aggregation but a reference, > i.e atomic state would not contain those state objects, but have a pointer > instead, but then you would not be using that container_of magic. Pointers is what it has. And once the atomic commit is done the atomic_state (ie. the object used to track the single transaction) goes away while the crtc/plane/etc. states remain behind.
On Tue, May 12, 2020 at 04:50:46PM +0300, Ville Syrjälä wrote: > On Tue, May 12, 2020 at 04:41:26PM +0300, Lisovskiy, Stanislav wrote: > > On Tue, May 12, 2020 at 04:38:21PM +0300, Ville Syrjälä wrote: > > > On Tue, May 12, 2020 at 04:17:34PM +0300, Lisovskiy, Stanislav wrote: > > > > On Tue, May 12, 2020 at 04:10:46PM +0300, Ville Syrjälä wrote: > > > > > On Tue, May 12, 2020 at 03:52:27PM +0300, Lisovskiy, Stanislav wrote: > > > > > > On Tue, May 12, 2020 at 03:03:26PM +0300, Ville Syrjälä wrote: > > > > > > > On Thu, May 07, 2020 at 05:45:01PM +0300, Stanislav Lisovskiy wrote: > > > > > > > > Starting from TGL we need to have a separate wm0 > > > > > > > > values for SAGV and non-SAGV which affects > > > > > > > > how calculations are done. > > > > > > > > > > > > > > > > v2: Remove long lines > > > > > > > > v3: Removed COLOR_PLANE enum references > > > > > > > > v4, v5, v6: Fixed rebase conflict > > > > > > > > v7: - Removed skl_plane_wm_level accessor from skl_allocate_pipe_ddb(Ville) > > > > > > > > - Removed sagv_uv_wm0(Ville) > > > > > > > > - can_sagv->use_sagv_wm(Ville) > > > > > > > > > > > > > > > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > > > > > > > > --- > > > > > > > > drivers/gpu/drm/i915/display/intel_display.c | 8 +- > > > > > > > > .../drm/i915/display/intel_display_types.h | 2 + > > > > > > > > drivers/gpu/drm/i915/intel_pm.c | 118 +++++++++++++++++- > > > > > > > > 3 files changed, 121 insertions(+), 7 deletions(-) > > > > > > > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c > > > > > > > > index fd6d63b03489..be5741cb7595 100644 > > > > > > > > --- a/drivers/gpu/drm/i915/display/intel_display.c > > > > > > > > +++ b/drivers/gpu/drm/i915/display/intel_display.c > > > > > > > > @@ -13961,7 +13961,9 @@ static void verify_wm_state(struct intel_crtc *crtc, > > > > > > > > /* Watermarks */ > > > > > > > > for (level = 0; level <= max_level; level++) { > > > > > > > > if (skl_wm_level_equals(&hw_plane_wm->wm[level], > > > > > > > > - &sw_plane_wm->wm[level])) > > > > > > > > + &sw_plane_wm->wm[level]) || > > > > > > > > + (level == 0 && skl_wm_level_equals(&hw_plane_wm->wm[level], > > > > > > > > + &sw_plane_wm->sagv_wm0))) > > > > > > > > continue; > > > > > > > > > > > > > > > > drm_err(&dev_priv->drm, > > > > > > > > @@ -14016,7 +14018,9 @@ static void verify_wm_state(struct intel_crtc *crtc, > > > > > > > > /* Watermarks */ > > > > > > > > for (level = 0; level <= max_level; level++) { > > > > > > > > if (skl_wm_level_equals(&hw_plane_wm->wm[level], > > > > > > > > - &sw_plane_wm->wm[level])) > > > > > > > > + &sw_plane_wm->wm[level]) || > > > > > > > > + (level == 0 && skl_wm_level_equals(&hw_plane_wm->wm[level], > > > > > > > > + &sw_plane_wm->sagv_wm0))) > > > > > > > > continue; > > > > > > > > > > > > > > > > drm_err(&dev_priv->drm, > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h > > > > > > > > index 9488449e4b94..8cede29c9562 100644 > > > > > > > > --- a/drivers/gpu/drm/i915/display/intel_display_types.h > > > > > > > > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h > > > > > > > > @@ -688,11 +688,13 @@ struct skl_plane_wm { > > > > > > > > struct skl_wm_level wm[8]; > > > > > > > > struct skl_wm_level uv_wm[8]; > > > > > > > > struct skl_wm_level trans_wm; > > > > > > > > + struct skl_wm_level sagv_wm0; > > > > > > > > bool is_planar; > > > > > > > > }; > > > > > > > > > > > > > > > > struct skl_pipe_wm { > > > > > > > > struct skl_plane_wm planes[I915_MAX_PLANES]; > > > > > > > > + bool use_sagv_wm; > > > > > > > > }; > > > > > > > > > > > > > > > > enum vlv_wm_level { > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > > > > > > > > index db188efee21e..934a686342ad 100644 > > > > > > > > --- a/drivers/gpu/drm/i915/intel_pm.c > > > > > > > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > > > > > > > @@ -3863,25 +3863,35 @@ bool intel_can_enable_sagv(struct drm_i915_private *dev_priv, > > > > > > > > return bw_state->pipe_sagv_reject == 0; > > > > > > > > } > > > > > > > > > > > > > > > > +static bool > > > > > > > > +tgl_crtc_can_enable_sagv(const struct intel_crtc_state *crtc_state); > > > > > > > > > > > > > > Just put the function here instead of adding fwd decalrations. > > > > > > > > > > > > > > > + > > > > > > > > static int intel_compute_sagv_mask(struct intel_atomic_state *state) > > > > > > > > { > > > > > > > > struct drm_i915_private *dev_priv = to_i915(state->base.dev); > > > > > > > > int ret; > > > > > > > > struct intel_crtc *crtc; > > > > > > > > - const struct intel_crtc_state *new_crtc_state; > > > > > > > > + struct intel_crtc_state *new_crtc_state; > > > > > > > > struct intel_bw_state *new_bw_state = NULL; > > > > > > > > const struct intel_bw_state *old_bw_state = NULL; > > > > > > > > int i; > > > > > > > > > > > > > > > > for_each_new_intel_crtc_in_state(state, crtc, > > > > > > > > new_crtc_state, i) { > > > > > > > > + bool can_sagv; > > > > > > > > + > > > > > > > > new_bw_state = intel_atomic_get_bw_state(state); > > > > > > > > if (IS_ERR(new_bw_state)) > > > > > > > > return PTR_ERR(new_bw_state); > > > > > > > > > > > > > > > > old_bw_state = intel_atomic_get_old_bw_state(state); > > > > > > > > > > > > > > > > - if (skl_crtc_can_enable_sagv(new_crtc_state)) > > > > > > > > + if (INTEL_GEN(dev_priv) >= 12) > > > > > > > > + can_sagv = tgl_crtc_can_enable_sagv(new_crtc_state); > > > > > > > > + else > > > > > > > > + can_sagv = skl_crtc_can_enable_sagv(new_crtc_state); > > > > > > > > + > > > > > > > > + if (can_sagv) > > > > > > > > new_bw_state->pipe_sagv_reject &= ~BIT(crtc->pipe); > > > > > > > > else > > > > > > > > new_bw_state->pipe_sagv_reject |= BIT(crtc->pipe); > > > > > > > > @@ -3899,6 +3909,24 @@ static int intel_compute_sagv_mask(struct intel_atomic_state *state) > > > > > > > > return ret; > > > > > > > > } > > > > > > > > > > > > > > > > + for_each_new_intel_crtc_in_state(state, crtc, > > > > > > > > + new_crtc_state, i) { > > > > > > > > + struct skl_pipe_wm *pipe_wm = &new_crtc_state->wm.skl.optimal; > > > > > > > > + > > > > > > > > + /* > > > > > > > > + * Due to drm limitation at commit state, when > > > > > > > > + * changes are written the whole atomic state is > > > > > > > > + * zeroed away => which prevents from using it, > > > > > > > > + * so just sticking it into pipe wm state for > > > > > > > > + * keeping it simple - anyway this is related to wm. > > > > > > > > + * Proper way in ideal universe would be of course not > > > > > > > > + * to lose parent atomic state object from child crtc_state, > > > > > > > > + * and stick to OOP programming principles, which had been > > > > > > > > + * scientifically proven to work. > > > > > > > > + */ > > > > > > > > > > > > > > More ramblings. Just drop this comment too imo. > > > > > > > > > > > > As I understand what is happening here is rather weird, so I thought > > > > > > commenting is good idea. > > > > > > > > > > At least I have no idea what the comment is trying to say. > > > > > I see nothing weird hapening here, we're just computing the > > > > > state which is totally standard stuff. > > > > > > > > Well I can remind, this is because there is no way to get parent state > > > > from crtc_state, because of weird drm atomic behaviour which leaves us > > > > with NULL parent state. So that we had to stick this boolean to some > > > > place. > > > > In normal code universe this wouldn't even be needed if we could > > > > just get atomic state from crtc_state when we write wm in skl_write_plane_wm. > > > > > > Didn't get that at all from the comment. It talked about zeroing some > > > whole state which I took as 'memset(state, 0, sizeof(*state))'. > > > As that is not what's going on I just got confused by the comment. > > > > > > Now that I understand what this is about I think the comment > > > (if we want to have one) should probably say something more like: > > > "we store use_sagv_wm in the crtc state rather than relying on > > > the bw state since we have no convenient way to get at the > > > latter from the plane commit hooks (especially in the legacy > > > cursor case)". > > > > > > > > > > > However probably OOP principles like parent-child hieararchy is not a thing > > > > here. That what this comment was trying to say. > > > > > > > > In normal OOP you can't destroy parent object _before_ destroying > > > > child one. > > > > > > There's no parent-child relationship between the crtc state and atomic > > > state (which really shouldn't be called a state to begin with, rather > > > it should be "transaction" or something along those lines). > > > > In practice there is. crtc_state is being aggregated and contained as > > part of more general atomic state. That is exactly what parent-child > > object relation is. > > If you want to decouple those, this needs to be not aggregation but a reference, > > i.e atomic state would not contain those state objects, but have a pointer > > instead, but then you would not be using that container_of magic. > > Pointers is what it has. And once the atomic commit is done the > atomic_state (ie. the object used to track the single transaction) > goes away while the crtc/plane/etc. states remain behind. If the rest of states are independent there should be sane way to get those without the atomic state. Currently bw_state, cdclk_state and co - all can be retrieved only using atomic state, which is at some point "gone". Also it is actually not even gone, we just zero out a pointer to it in drm_crtc_state. I know why this done as we discussed, however I would emphasize that the proper way would be then to completely decouple from it, so that all required states can be retrieved without atomic state. Because currently we are kind of in some "fuzzy" state in between. Stan > > -- > Ville Syrjälä > Intel
On Tue, May 12, 2020 at 04:59:19PM +0300, Lisovskiy, Stanislav wrote: > On Tue, May 12, 2020 at 04:50:46PM +0300, Ville Syrjälä wrote: > > On Tue, May 12, 2020 at 04:41:26PM +0300, Lisovskiy, Stanislav wrote: > > > On Tue, May 12, 2020 at 04:38:21PM +0300, Ville Syrjälä wrote: > > > > On Tue, May 12, 2020 at 04:17:34PM +0300, Lisovskiy, Stanislav wrote: > > > > > On Tue, May 12, 2020 at 04:10:46PM +0300, Ville Syrjälä wrote: > > > > > > On Tue, May 12, 2020 at 03:52:27PM +0300, Lisovskiy, Stanislav wrote: > > > > > > > On Tue, May 12, 2020 at 03:03:26PM +0300, Ville Syrjälä wrote: > > > > > > > > On Thu, May 07, 2020 at 05:45:01PM +0300, Stanislav Lisovskiy wrote: > > > > > > > > > Starting from TGL we need to have a separate wm0 > > > > > > > > > values for SAGV and non-SAGV which affects > > > > > > > > > how calculations are done. > > > > > > > > > > > > > > > > > > v2: Remove long lines > > > > > > > > > v3: Removed COLOR_PLANE enum references > > > > > > > > > v4, v5, v6: Fixed rebase conflict > > > > > > > > > v7: - Removed skl_plane_wm_level accessor from skl_allocate_pipe_ddb(Ville) > > > > > > > > > - Removed sagv_uv_wm0(Ville) > > > > > > > > > - can_sagv->use_sagv_wm(Ville) > > > > > > > > > > > > > > > > > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > > > > > > > > > --- > > > > > > > > > drivers/gpu/drm/i915/display/intel_display.c | 8 +- > > > > > > > > > .../drm/i915/display/intel_display_types.h | 2 + > > > > > > > > > drivers/gpu/drm/i915/intel_pm.c | 118 +++++++++++++++++- > > > > > > > > > 3 files changed, 121 insertions(+), 7 deletions(-) > > > > > > > > > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c > > > > > > > > > index fd6d63b03489..be5741cb7595 100644 > > > > > > > > > --- a/drivers/gpu/drm/i915/display/intel_display.c > > > > > > > > > +++ b/drivers/gpu/drm/i915/display/intel_display.c > > > > > > > > > @@ -13961,7 +13961,9 @@ static void verify_wm_state(struct intel_crtc *crtc, > > > > > > > > > /* Watermarks */ > > > > > > > > > for (level = 0; level <= max_level; level++) { > > > > > > > > > if (skl_wm_level_equals(&hw_plane_wm->wm[level], > > > > > > > > > - &sw_plane_wm->wm[level])) > > > > > > > > > + &sw_plane_wm->wm[level]) || > > > > > > > > > + (level == 0 && skl_wm_level_equals(&hw_plane_wm->wm[level], > > > > > > > > > + &sw_plane_wm->sagv_wm0))) > > > > > > > > > continue; > > > > > > > > > > > > > > > > > > drm_err(&dev_priv->drm, > > > > > > > > > @@ -14016,7 +14018,9 @@ static void verify_wm_state(struct intel_crtc *crtc, > > > > > > > > > /* Watermarks */ > > > > > > > > > for (level = 0; level <= max_level; level++) { > > > > > > > > > if (skl_wm_level_equals(&hw_plane_wm->wm[level], > > > > > > > > > - &sw_plane_wm->wm[level])) > > > > > > > > > + &sw_plane_wm->wm[level]) || > > > > > > > > > + (level == 0 && skl_wm_level_equals(&hw_plane_wm->wm[level], > > > > > > > > > + &sw_plane_wm->sagv_wm0))) > > > > > > > > > continue; > > > > > > > > > > > > > > > > > > drm_err(&dev_priv->drm, > > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h > > > > > > > > > index 9488449e4b94..8cede29c9562 100644 > > > > > > > > > --- a/drivers/gpu/drm/i915/display/intel_display_types.h > > > > > > > > > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h > > > > > > > > > @@ -688,11 +688,13 @@ struct skl_plane_wm { > > > > > > > > > struct skl_wm_level wm[8]; > > > > > > > > > struct skl_wm_level uv_wm[8]; > > > > > > > > > struct skl_wm_level trans_wm; > > > > > > > > > + struct skl_wm_level sagv_wm0; > > > > > > > > > bool is_planar; > > > > > > > > > }; > > > > > > > > > > > > > > > > > > struct skl_pipe_wm { > > > > > > > > > struct skl_plane_wm planes[I915_MAX_PLANES]; > > > > > > > > > + bool use_sagv_wm; > > > > > > > > > }; > > > > > > > > > > > > > > > > > > enum vlv_wm_level { > > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > > > > > > > > > index db188efee21e..934a686342ad 100644 > > > > > > > > > --- a/drivers/gpu/drm/i915/intel_pm.c > > > > > > > > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > > > > > > > > @@ -3863,25 +3863,35 @@ bool intel_can_enable_sagv(struct drm_i915_private *dev_priv, > > > > > > > > > return bw_state->pipe_sagv_reject == 0; > > > > > > > > > } > > > > > > > > > > > > > > > > > > +static bool > > > > > > > > > +tgl_crtc_can_enable_sagv(const struct intel_crtc_state *crtc_state); > > > > > > > > > > > > > > > > Just put the function here instead of adding fwd decalrations. > > > > > > > > > > > > > > > > > + > > > > > > > > > static int intel_compute_sagv_mask(struct intel_atomic_state *state) > > > > > > > > > { > > > > > > > > > struct drm_i915_private *dev_priv = to_i915(state->base.dev); > > > > > > > > > int ret; > > > > > > > > > struct intel_crtc *crtc; > > > > > > > > > - const struct intel_crtc_state *new_crtc_state; > > > > > > > > > + struct intel_crtc_state *new_crtc_state; > > > > > > > > > struct intel_bw_state *new_bw_state = NULL; > > > > > > > > > const struct intel_bw_state *old_bw_state = NULL; > > > > > > > > > int i; > > > > > > > > > > > > > > > > > > for_each_new_intel_crtc_in_state(state, crtc, > > > > > > > > > new_crtc_state, i) { > > > > > > > > > + bool can_sagv; > > > > > > > > > + > > > > > > > > > new_bw_state = intel_atomic_get_bw_state(state); > > > > > > > > > if (IS_ERR(new_bw_state)) > > > > > > > > > return PTR_ERR(new_bw_state); > > > > > > > > > > > > > > > > > > old_bw_state = intel_atomic_get_old_bw_state(state); > > > > > > > > > > > > > > > > > > - if (skl_crtc_can_enable_sagv(new_crtc_state)) > > > > > > > > > + if (INTEL_GEN(dev_priv) >= 12) > > > > > > > > > + can_sagv = tgl_crtc_can_enable_sagv(new_crtc_state); > > > > > > > > > + else > > > > > > > > > + can_sagv = skl_crtc_can_enable_sagv(new_crtc_state); > > > > > > > > > + > > > > > > > > > + if (can_sagv) > > > > > > > > > new_bw_state->pipe_sagv_reject &= ~BIT(crtc->pipe); > > > > > > > > > else > > > > > > > > > new_bw_state->pipe_sagv_reject |= BIT(crtc->pipe); > > > > > > > > > @@ -3899,6 +3909,24 @@ static int intel_compute_sagv_mask(struct intel_atomic_state *state) > > > > > > > > > return ret; > > > > > > > > > } > > > > > > > > > > > > > > > > > > + for_each_new_intel_crtc_in_state(state, crtc, > > > > > > > > > + new_crtc_state, i) { > > > > > > > > > + struct skl_pipe_wm *pipe_wm = &new_crtc_state->wm.skl.optimal; > > > > > > > > > + > > > > > > > > > + /* > > > > > > > > > + * Due to drm limitation at commit state, when > > > > > > > > > + * changes are written the whole atomic state is > > > > > > > > > + * zeroed away => which prevents from using it, > > > > > > > > > + * so just sticking it into pipe wm state for > > > > > > > > > + * keeping it simple - anyway this is related to wm. > > > > > > > > > + * Proper way in ideal universe would be of course not > > > > > > > > > + * to lose parent atomic state object from child crtc_state, > > > > > > > > > + * and stick to OOP programming principles, which had been > > > > > > > > > + * scientifically proven to work. > > > > > > > > > + */ > > > > > > > > > > > > > > > > More ramblings. Just drop this comment too imo. > > > > > > > > > > > > > > As I understand what is happening here is rather weird, so I thought > > > > > > > commenting is good idea. > > > > > > > > > > > > At least I have no idea what the comment is trying to say. > > > > > > I see nothing weird hapening here, we're just computing the > > > > > > state which is totally standard stuff. > > > > > > > > > > Well I can remind, this is because there is no way to get parent state > > > > > from crtc_state, because of weird drm atomic behaviour which leaves us > > > > > with NULL parent state. So that we had to stick this boolean to some > > > > > place. > > > > > In normal code universe this wouldn't even be needed if we could > > > > > just get atomic state from crtc_state when we write wm in skl_write_plane_wm. > > > > > > > > Didn't get that at all from the comment. It talked about zeroing some > > > > whole state which I took as 'memset(state, 0, sizeof(*state))'. > > > > As that is not what's going on I just got confused by the comment. > > > > > > > > Now that I understand what this is about I think the comment > > > > (if we want to have one) should probably say something more like: > > > > "we store use_sagv_wm in the crtc state rather than relying on > > > > the bw state since we have no convenient way to get at the > > > > latter from the plane commit hooks (especially in the legacy > > > > cursor case)". > > > > > > > > > > > > > > However probably OOP principles like parent-child hieararchy is not a thing > > > > > here. That what this comment was trying to say. > > > > > > > > > > In normal OOP you can't destroy parent object _before_ destroying > > > > > child one. > > > > > > > > There's no parent-child relationship between the crtc state and atomic > > > > state (which really shouldn't be called a state to begin with, rather > > > > it should be "transaction" or something along those lines). > > > > > > In practice there is. crtc_state is being aggregated and contained as > > > part of more general atomic state. That is exactly what parent-child > > > object relation is. > > > If you want to decouple those, this needs to be not aggregation but a reference, > > > i.e atomic state would not contain those state objects, but have a pointer > > > instead, but then you would not be using that container_of magic. > > > > Pointers is what it has. And once the atomic commit is done the > > atomic_state (ie. the object used to track the single transaction) > > goes away while the crtc/plane/etc. states remain behind. > > If the rest of states are independent there should be sane way > to get those without the atomic state. How could you possibly get the right one without specifying which transaction you want them for? > > Currently bw_state, cdclk_state and co - all can be retrieved only > using atomic state, which is at some point "gone". > Also it is actually not even gone, we just zero out a pointer > to it in drm_crtc_state. > > I know why this done as we discussed, however I would > emphasize that the proper way would be then > to completely decouple from it, so that all required states can > be retrieved without atomic state. Because currently we are > kind of in some "fuzzy" state in between. > > Stan > > > > > -- > > Ville Syrjälä > > Intel
On Tue, May 12, 2020 at 05:32:21PM +0300, Ville Syrjälä wrote: > On Tue, May 12, 2020 at 04:59:19PM +0300, Lisovskiy, Stanislav wrote: > > On Tue, May 12, 2020 at 04:50:46PM +0300, Ville Syrjälä wrote: > > > On Tue, May 12, 2020 at 04:41:26PM +0300, Lisovskiy, Stanislav wrote: > > > > On Tue, May 12, 2020 at 04:38:21PM +0300, Ville Syrjälä wrote: > > > > > On Tue, May 12, 2020 at 04:17:34PM +0300, Lisovskiy, Stanislav wrote: > > > > > > On Tue, May 12, 2020 at 04:10:46PM +0300, Ville Syrjälä wrote: > > > > > > > On Tue, May 12, 2020 at 03:52:27PM +0300, Lisovskiy, Stanislav wrote: > > > > > > > > On Tue, May 12, 2020 at 03:03:26PM +0300, Ville Syrjälä wrote: > > > > > > > > > On Thu, May 07, 2020 at 05:45:01PM +0300, Stanislav Lisovskiy wrote: > > > > > > > > > > Starting from TGL we need to have a separate wm0 > > > > > > > > > > values for SAGV and non-SAGV which affects > > > > > > > > > > how calculations are done. > > > > > > > > > > > > > > > > > > > > v2: Remove long lines > > > > > > > > > > v3: Removed COLOR_PLANE enum references > > > > > > > > > > v4, v5, v6: Fixed rebase conflict > > > > > > > > > > v7: - Removed skl_plane_wm_level accessor from skl_allocate_pipe_ddb(Ville) > > > > > > > > > > - Removed sagv_uv_wm0(Ville) > > > > > > > > > > - can_sagv->use_sagv_wm(Ville) > > > > > > > > > > > > > > > > > > > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > > > > > > > > > > --- > > > > > > > > > > drivers/gpu/drm/i915/display/intel_display.c | 8 +- > > > > > > > > > > .../drm/i915/display/intel_display_types.h | 2 + > > > > > > > > > > drivers/gpu/drm/i915/intel_pm.c | 118 +++++++++++++++++- > > > > > > > > > > 3 files changed, 121 insertions(+), 7 deletions(-) > > > > > > > > > > > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c > > > > > > > > > > index fd6d63b03489..be5741cb7595 100644 > > > > > > > > > > --- a/drivers/gpu/drm/i915/display/intel_display.c > > > > > > > > > > +++ b/drivers/gpu/drm/i915/display/intel_display.c > > > > > > > > > > @@ -13961,7 +13961,9 @@ static void verify_wm_state(struct intel_crtc *crtc, > > > > > > > > > > /* Watermarks */ > > > > > > > > > > for (level = 0; level <= max_level; level++) { > > > > > > > > > > if (skl_wm_level_equals(&hw_plane_wm->wm[level], > > > > > > > > > > - &sw_plane_wm->wm[level])) > > > > > > > > > > + &sw_plane_wm->wm[level]) || > > > > > > > > > > + (level == 0 && skl_wm_level_equals(&hw_plane_wm->wm[level], > > > > > > > > > > + &sw_plane_wm->sagv_wm0))) > > > > > > > > > > continue; > > > > > > > > > > > > > > > > > > > > drm_err(&dev_priv->drm, > > > > > > > > > > @@ -14016,7 +14018,9 @@ static void verify_wm_state(struct intel_crtc *crtc, > > > > > > > > > > /* Watermarks */ > > > > > > > > > > for (level = 0; level <= max_level; level++) { > > > > > > > > > > if (skl_wm_level_equals(&hw_plane_wm->wm[level], > > > > > > > > > > - &sw_plane_wm->wm[level])) > > > > > > > > > > + &sw_plane_wm->wm[level]) || > > > > > > > > > > + (level == 0 && skl_wm_level_equals(&hw_plane_wm->wm[level], > > > > > > > > > > + &sw_plane_wm->sagv_wm0))) > > > > > > > > > > continue; > > > > > > > > > > > > > > > > > > > > drm_err(&dev_priv->drm, > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h > > > > > > > > > > index 9488449e4b94..8cede29c9562 100644 > > > > > > > > > > --- a/drivers/gpu/drm/i915/display/intel_display_types.h > > > > > > > > > > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h > > > > > > > > > > @@ -688,11 +688,13 @@ struct skl_plane_wm { > > > > > > > > > > struct skl_wm_level wm[8]; > > > > > > > > > > struct skl_wm_level uv_wm[8]; > > > > > > > > > > struct skl_wm_level trans_wm; > > > > > > > > > > + struct skl_wm_level sagv_wm0; > > > > > > > > > > bool is_planar; > > > > > > > > > > }; > > > > > > > > > > > > > > > > > > > > struct skl_pipe_wm { > > > > > > > > > > struct skl_plane_wm planes[I915_MAX_PLANES]; > > > > > > > > > > + bool use_sagv_wm; > > > > > > > > > > }; > > > > > > > > > > > > > > > > > > > > enum vlv_wm_level { > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > > > > > > > > > > index db188efee21e..934a686342ad 100644 > > > > > > > > > > --- a/drivers/gpu/drm/i915/intel_pm.c > > > > > > > > > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > > > > > > > > > @@ -3863,25 +3863,35 @@ bool intel_can_enable_sagv(struct drm_i915_private *dev_priv, > > > > > > > > > > return bw_state->pipe_sagv_reject == 0; > > > > > > > > > > } > > > > > > > > > > > > > > > > > > > > +static bool > > > > > > > > > > +tgl_crtc_can_enable_sagv(const struct intel_crtc_state *crtc_state); > > > > > > > > > > > > > > > > > > Just put the function here instead of adding fwd decalrations. > > > > > > > > > > > > > > > > > > > + > > > > > > > > > > static int intel_compute_sagv_mask(struct intel_atomic_state *state) > > > > > > > > > > { > > > > > > > > > > struct drm_i915_private *dev_priv = to_i915(state->base.dev); > > > > > > > > > > int ret; > > > > > > > > > > struct intel_crtc *crtc; > > > > > > > > > > - const struct intel_crtc_state *new_crtc_state; > > > > > > > > > > + struct intel_crtc_state *new_crtc_state; > > > > > > > > > > struct intel_bw_state *new_bw_state = NULL; > > > > > > > > > > const struct intel_bw_state *old_bw_state = NULL; > > > > > > > > > > int i; > > > > > > > > > > > > > > > > > > > > for_each_new_intel_crtc_in_state(state, crtc, > > > > > > > > > > new_crtc_state, i) { > > > > > > > > > > + bool can_sagv; > > > > > > > > > > + > > > > > > > > > > new_bw_state = intel_atomic_get_bw_state(state); > > > > > > > > > > if (IS_ERR(new_bw_state)) > > > > > > > > > > return PTR_ERR(new_bw_state); > > > > > > > > > > > > > > > > > > > > old_bw_state = intel_atomic_get_old_bw_state(state); > > > > > > > > > > > > > > > > > > > > - if (skl_crtc_can_enable_sagv(new_crtc_state)) > > > > > > > > > > + if (INTEL_GEN(dev_priv) >= 12) > > > > > > > > > > + can_sagv = tgl_crtc_can_enable_sagv(new_crtc_state); > > > > > > > > > > + else > > > > > > > > > > + can_sagv = skl_crtc_can_enable_sagv(new_crtc_state); > > > > > > > > > > + > > > > > > > > > > + if (can_sagv) > > > > > > > > > > new_bw_state->pipe_sagv_reject &= ~BIT(crtc->pipe); > > > > > > > > > > else > > > > > > > > > > new_bw_state->pipe_sagv_reject |= BIT(crtc->pipe); > > > > > > > > > > @@ -3899,6 +3909,24 @@ static int intel_compute_sagv_mask(struct intel_atomic_state *state) > > > > > > > > > > return ret; > > > > > > > > > > } > > > > > > > > > > > > > > > > > > > > + for_each_new_intel_crtc_in_state(state, crtc, > > > > > > > > > > + new_crtc_state, i) { > > > > > > > > > > + struct skl_pipe_wm *pipe_wm = &new_crtc_state->wm.skl.optimal; > > > > > > > > > > + > > > > > > > > > > + /* > > > > > > > > > > + * Due to drm limitation at commit state, when > > > > > > > > > > + * changes are written the whole atomic state is > > > > > > > > > > + * zeroed away => which prevents from using it, > > > > > > > > > > + * so just sticking it into pipe wm state for > > > > > > > > > > + * keeping it simple - anyway this is related to wm. > > > > > > > > > > + * Proper way in ideal universe would be of course not > > > > > > > > > > + * to lose parent atomic state object from child crtc_state, > > > > > > > > > > + * and stick to OOP programming principles, which had been > > > > > > > > > > + * scientifically proven to work. > > > > > > > > > > + */ > > > > > > > > > > > > > > > > > > More ramblings. Just drop this comment too imo. > > > > > > > > > > > > > > > > As I understand what is happening here is rather weird, so I thought > > > > > > > > commenting is good idea. > > > > > > > > > > > > > > At least I have no idea what the comment is trying to say. > > > > > > > I see nothing weird hapening here, we're just computing the > > > > > > > state which is totally standard stuff. > > > > > > > > > > > > Well I can remind, this is because there is no way to get parent state > > > > > > from crtc_state, because of weird drm atomic behaviour which leaves us > > > > > > with NULL parent state. So that we had to stick this boolean to some > > > > > > place. > > > > > > In normal code universe this wouldn't even be needed if we could > > > > > > just get atomic state from crtc_state when we write wm in skl_write_plane_wm. > > > > > > > > > > Didn't get that at all from the comment. It talked about zeroing some > > > > > whole state which I took as 'memset(state, 0, sizeof(*state))'. > > > > > As that is not what's going on I just got confused by the comment. > > > > > > > > > > Now that I understand what this is about I think the comment > > > > > (if we want to have one) should probably say something more like: > > > > > "we store use_sagv_wm in the crtc state rather than relying on > > > > > the bw state since we have no convenient way to get at the > > > > > latter from the plane commit hooks (especially in the legacy > > > > > cursor case)". > > > > > > > > > > > > > > > > > However probably OOP principles like parent-child hieararchy is not a thing > > > > > > here. That what this comment was trying to say. > > > > > > > > > > > > In normal OOP you can't destroy parent object _before_ destroying > > > > > > child one. > > > > > > > > > > There's no parent-child relationship between the crtc state and atomic > > > > > state (which really shouldn't be called a state to begin with, rather > > > > > it should be "transaction" or something along those lines). > > > > > > > > In practice there is. crtc_state is being aggregated and contained as > > > > part of more general atomic state. That is exactly what parent-child > > > > object relation is. > > > > If you want to decouple those, this needs to be not aggregation but a reference, > > > > i.e atomic state would not contain those state objects, but have a pointer > > > > instead, but then you would not be using that container_of magic. > > > > > > Pointers is what it has. And once the atomic commit is done the > > > atomic_state (ie. the object used to track the single transaction) > > > goes away while the crtc/plane/etc. states remain behind. > > > > If the rest of states are independent there should be sane way > > to get those without the atomic state. > > How could you possibly get the right one without specifying > which transaction you want them for? Then if we still need those in skl_write_plane_wm or somewhere else during commit, transaction should not go away yet. Otherwise there is a logical contraversy: we need those objects, but can't get them without transaction. But transaction still goes away, because it's kinda "separate". Stan > > > > > Currently bw_state, cdclk_state and co - all can be retrieved only > > using atomic state, which is at some point "gone". > > Also it is actually not even gone, we just zero out a pointer > > to it in drm_crtc_state. > > > > I know why this done as we discussed, however I would > > emphasize that the proper way would be then > > to completely decouple from it, so that all required states can > > be retrieved without atomic state. Because currently we are > > kind of in some "fuzzy" state in between. > > > > Stan > > > > > > > > -- > > > Ville Syrjälä > > > Intel > > -- > Ville Syrjälä > Intel
On Tue, May 12, 2020 at 06:04:45PM +0300, Lisovskiy, Stanislav wrote: > On Tue, May 12, 2020 at 05:32:21PM +0300, Ville Syrjälä wrote: > > On Tue, May 12, 2020 at 04:59:19PM +0300, Lisovskiy, Stanislav wrote: > > > On Tue, May 12, 2020 at 04:50:46PM +0300, Ville Syrjälä wrote: > > > > On Tue, May 12, 2020 at 04:41:26PM +0300, Lisovskiy, Stanislav wrote: > > > > > On Tue, May 12, 2020 at 04:38:21PM +0300, Ville Syrjälä wrote: > > > > > > On Tue, May 12, 2020 at 04:17:34PM +0300, Lisovskiy, Stanislav wrote: > > > > > > > On Tue, May 12, 2020 at 04:10:46PM +0300, Ville Syrjälä wrote: > > > > > > > > On Tue, May 12, 2020 at 03:52:27PM +0300, Lisovskiy, Stanislav wrote: > > > > > > > > > On Tue, May 12, 2020 at 03:03:26PM +0300, Ville Syrjälä wrote: > > > > > > > > > > On Thu, May 07, 2020 at 05:45:01PM +0300, Stanislav Lisovskiy wrote: > > > > > > > > > > > Starting from TGL we need to have a separate wm0 > > > > > > > > > > > values for SAGV and non-SAGV which affects > > > > > > > > > > > how calculations are done. > > > > > > > > > > > > > > > > > > > > > > v2: Remove long lines > > > > > > > > > > > v3: Removed COLOR_PLANE enum references > > > > > > > > > > > v4, v5, v6: Fixed rebase conflict > > > > > > > > > > > v7: - Removed skl_plane_wm_level accessor from skl_allocate_pipe_ddb(Ville) > > > > > > > > > > > - Removed sagv_uv_wm0(Ville) > > > > > > > > > > > - can_sagv->use_sagv_wm(Ville) > > > > > > > > > > > > > > > > > > > > > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > > > > > > > > > > > --- > > > > > > > > > > > drivers/gpu/drm/i915/display/intel_display.c | 8 +- > > > > > > > > > > > .../drm/i915/display/intel_display_types.h | 2 + > > > > > > > > > > > drivers/gpu/drm/i915/intel_pm.c | 118 +++++++++++++++++- > > > > > > > > > > > 3 files changed, 121 insertions(+), 7 deletions(-) > > > > > > > > > > > > > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c > > > > > > > > > > > index fd6d63b03489..be5741cb7595 100644 > > > > > > > > > > > --- a/drivers/gpu/drm/i915/display/intel_display.c > > > > > > > > > > > +++ b/drivers/gpu/drm/i915/display/intel_display.c > > > > > > > > > > > @@ -13961,7 +13961,9 @@ static void verify_wm_state(struct intel_crtc *crtc, > > > > > > > > > > > /* Watermarks */ > > > > > > > > > > > for (level = 0; level <= max_level; level++) { > > > > > > > > > > > if (skl_wm_level_equals(&hw_plane_wm->wm[level], > > > > > > > > > > > - &sw_plane_wm->wm[level])) > > > > > > > > > > > + &sw_plane_wm->wm[level]) || > > > > > > > > > > > + (level == 0 && skl_wm_level_equals(&hw_plane_wm->wm[level], > > > > > > > > > > > + &sw_plane_wm->sagv_wm0))) > > > > > > > > > > > continue; > > > > > > > > > > > > > > > > > > > > > > drm_err(&dev_priv->drm, > > > > > > > > > > > @@ -14016,7 +14018,9 @@ static void verify_wm_state(struct intel_crtc *crtc, > > > > > > > > > > > /* Watermarks */ > > > > > > > > > > > for (level = 0; level <= max_level; level++) { > > > > > > > > > > > if (skl_wm_level_equals(&hw_plane_wm->wm[level], > > > > > > > > > > > - &sw_plane_wm->wm[level])) > > > > > > > > > > > + &sw_plane_wm->wm[level]) || > > > > > > > > > > > + (level == 0 && skl_wm_level_equals(&hw_plane_wm->wm[level], > > > > > > > > > > > + &sw_plane_wm->sagv_wm0))) > > > > > > > > > > > continue; > > > > > > > > > > > > > > > > > > > > > > drm_err(&dev_priv->drm, > > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h > > > > > > > > > > > index 9488449e4b94..8cede29c9562 100644 > > > > > > > > > > > --- a/drivers/gpu/drm/i915/display/intel_display_types.h > > > > > > > > > > > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h > > > > > > > > > > > @@ -688,11 +688,13 @@ struct skl_plane_wm { > > > > > > > > > > > struct skl_wm_level wm[8]; > > > > > > > > > > > struct skl_wm_level uv_wm[8]; > > > > > > > > > > > struct skl_wm_level trans_wm; > > > > > > > > > > > + struct skl_wm_level sagv_wm0; > > > > > > > > > > > bool is_planar; > > > > > > > > > > > }; > > > > > > > > > > > > > > > > > > > > > > struct skl_pipe_wm { > > > > > > > > > > > struct skl_plane_wm planes[I915_MAX_PLANES]; > > > > > > > > > > > + bool use_sagv_wm; > > > > > > > > > > > }; > > > > > > > > > > > > > > > > > > > > > > enum vlv_wm_level { > > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > > > > > > > > > > > index db188efee21e..934a686342ad 100644 > > > > > > > > > > > --- a/drivers/gpu/drm/i915/intel_pm.c > > > > > > > > > > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > > > > > > > > > > @@ -3863,25 +3863,35 @@ bool intel_can_enable_sagv(struct drm_i915_private *dev_priv, > > > > > > > > > > > return bw_state->pipe_sagv_reject == 0; > > > > > > > > > > > } > > > > > > > > > > > > > > > > > > > > > > +static bool > > > > > > > > > > > +tgl_crtc_can_enable_sagv(const struct intel_crtc_state *crtc_state); > > > > > > > > > > > > > > > > > > > > Just put the function here instead of adding fwd decalrations. > > > > > > > > > > > > > > > > > > > > > + > > > > > > > > > > > static int intel_compute_sagv_mask(struct intel_atomic_state *state) > > > > > > > > > > > { > > > > > > > > > > > struct drm_i915_private *dev_priv = to_i915(state->base.dev); > > > > > > > > > > > int ret; > > > > > > > > > > > struct intel_crtc *crtc; > > > > > > > > > > > - const struct intel_crtc_state *new_crtc_state; > > > > > > > > > > > + struct intel_crtc_state *new_crtc_state; > > > > > > > > > > > struct intel_bw_state *new_bw_state = NULL; > > > > > > > > > > > const struct intel_bw_state *old_bw_state = NULL; > > > > > > > > > > > int i; > > > > > > > > > > > > > > > > > > > > > > for_each_new_intel_crtc_in_state(state, crtc, > > > > > > > > > > > new_crtc_state, i) { > > > > > > > > > > > + bool can_sagv; > > > > > > > > > > > + > > > > > > > > > > > new_bw_state = intel_atomic_get_bw_state(state); > > > > > > > > > > > if (IS_ERR(new_bw_state)) > > > > > > > > > > > return PTR_ERR(new_bw_state); > > > > > > > > > > > > > > > > > > > > > > old_bw_state = intel_atomic_get_old_bw_state(state); > > > > > > > > > > > > > > > > > > > > > > - if (skl_crtc_can_enable_sagv(new_crtc_state)) > > > > > > > > > > > + if (INTEL_GEN(dev_priv) >= 12) > > > > > > > > > > > + can_sagv = tgl_crtc_can_enable_sagv(new_crtc_state); > > > > > > > > > > > + else > > > > > > > > > > > + can_sagv = skl_crtc_can_enable_sagv(new_crtc_state); > > > > > > > > > > > + > > > > > > > > > > > + if (can_sagv) > > > > > > > > > > > new_bw_state->pipe_sagv_reject &= ~BIT(crtc->pipe); > > > > > > > > > > > else > > > > > > > > > > > new_bw_state->pipe_sagv_reject |= BIT(crtc->pipe); > > > > > > > > > > > @@ -3899,6 +3909,24 @@ static int intel_compute_sagv_mask(struct intel_atomic_state *state) > > > > > > > > > > > return ret; > > > > > > > > > > > } > > > > > > > > > > > > > > > > > > > > > > + for_each_new_intel_crtc_in_state(state, crtc, > > > > > > > > > > > + new_crtc_state, i) { > > > > > > > > > > > + struct skl_pipe_wm *pipe_wm = &new_crtc_state->wm.skl.optimal; > > > > > > > > > > > + > > > > > > > > > > > + /* > > > > > > > > > > > + * Due to drm limitation at commit state, when > > > > > > > > > > > + * changes are written the whole atomic state is > > > > > > > > > > > + * zeroed away => which prevents from using it, > > > > > > > > > > > + * so just sticking it into pipe wm state for > > > > > > > > > > > + * keeping it simple - anyway this is related to wm. > > > > > > > > > > > + * Proper way in ideal universe would be of course not > > > > > > > > > > > + * to lose parent atomic state object from child crtc_state, > > > > > > > > > > > + * and stick to OOP programming principles, which had been > > > > > > > > > > > + * scientifically proven to work. > > > > > > > > > > > + */ > > > > > > > > > > > > > > > > > > > > More ramblings. Just drop this comment too imo. > > > > > > > > > > > > > > > > > > As I understand what is happening here is rather weird, so I thought > > > > > > > > > commenting is good idea. > > > > > > > > > > > > > > > > At least I have no idea what the comment is trying to say. > > > > > > > > I see nothing weird hapening here, we're just computing the > > > > > > > > state which is totally standard stuff. > > > > > > > > > > > > > > Well I can remind, this is because there is no way to get parent state > > > > > > > from crtc_state, because of weird drm atomic behaviour which leaves us > > > > > > > with NULL parent state. So that we had to stick this boolean to some > > > > > > > place. > > > > > > > In normal code universe this wouldn't even be needed if we could > > > > > > > just get atomic state from crtc_state when we write wm in skl_write_plane_wm. > > > > > > > > > > > > Didn't get that at all from the comment. It talked about zeroing some > > > > > > whole state which I took as 'memset(state, 0, sizeof(*state))'. > > > > > > As that is not what's going on I just got confused by the comment. > > > > > > > > > > > > Now that I understand what this is about I think the comment > > > > > > (if we want to have one) should probably say something more like: > > > > > > "we store use_sagv_wm in the crtc state rather than relying on > > > > > > the bw state since we have no convenient way to get at the > > > > > > latter from the plane commit hooks (especially in the legacy > > > > > > cursor case)". > > > > > > > > > > > > > > > > > > > > However probably OOP principles like parent-child hieararchy is not a thing > > > > > > > here. That what this comment was trying to say. > > > > > > > > > > > > > > In normal OOP you can't destroy parent object _before_ destroying > > > > > > > child one. > > > > > > > > > > > > There's no parent-child relationship between the crtc state and atomic > > > > > > state (which really shouldn't be called a state to begin with, rather > > > > > > it should be "transaction" or something along those lines). > > > > > > > > > > In practice there is. crtc_state is being aggregated and contained as > > > > > part of more general atomic state. That is exactly what parent-child > > > > > object relation is. > > > > > If you want to decouple those, this needs to be not aggregation but a reference, > > > > > i.e atomic state would not contain those state objects, but have a pointer > > > > > instead, but then you would not be using that container_of magic. > > > > > > > > Pointers is what it has. And once the atomic commit is done the > > > > atomic_state (ie. the object used to track the single transaction) > > > > goes away while the crtc/plane/etc. states remain behind. > > > > > > If the rest of states are independent there should be sane way > > > to get those without the atomic state. > > > > How could you possibly get the right one without specifying > > which transaction you want them for? > > Then if we still need those in skl_write_plane_wm or somewhere else during commit, > transaction should not go away yet. IMO those backpointers shouldn't perhaps exist at all and instead we should just plumb the atomic state through everywhere. But you tried that and it kinda failed thanks to the legacy cursor hacks, so here we are. > > Otherwise there is a logical contraversy: we need those objects, but can't get > them without transaction. But transaction still goes away, > because it's kinda "separate". It's a pre-existing issue with the atomic core/helpers. Changing that is a lot of work, as we discused before a few times. > > Stan > > > > > > > > > Currently bw_state, cdclk_state and co - all can be retrieved only > > > using atomic state, which is at some point "gone". > > > Also it is actually not even gone, we just zero out a pointer > > > to it in drm_crtc_state. > > > > > > I know why this done as we discussed, however I would > > > emphasize that the proper way would be then > > > to completely decouple from it, so that all required states can > > > be retrieved without atomic state. Because currently we are > > > kind of in some "fuzzy" state in between. > > > > > > Stan > > > > > > > > > > > -- > > > > Ville Syrjälä > > > > Intel > > > > -- > > Ville Syrjälä > > Intel
diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c index fd6d63b03489..be5741cb7595 100644 --- a/drivers/gpu/drm/i915/display/intel_display.c +++ b/drivers/gpu/drm/i915/display/intel_display.c @@ -13961,7 +13961,9 @@ static void verify_wm_state(struct intel_crtc *crtc, /* Watermarks */ for (level = 0; level <= max_level; level++) { if (skl_wm_level_equals(&hw_plane_wm->wm[level], - &sw_plane_wm->wm[level])) + &sw_plane_wm->wm[level]) || + (level == 0 && skl_wm_level_equals(&hw_plane_wm->wm[level], + &sw_plane_wm->sagv_wm0))) continue; drm_err(&dev_priv->drm, @@ -14016,7 +14018,9 @@ static void verify_wm_state(struct intel_crtc *crtc, /* Watermarks */ for (level = 0; level <= max_level; level++) { if (skl_wm_level_equals(&hw_plane_wm->wm[level], - &sw_plane_wm->wm[level])) + &sw_plane_wm->wm[level]) || + (level == 0 && skl_wm_level_equals(&hw_plane_wm->wm[level], + &sw_plane_wm->sagv_wm0))) continue; drm_err(&dev_priv->drm, diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h index 9488449e4b94..8cede29c9562 100644 --- a/drivers/gpu/drm/i915/display/intel_display_types.h +++ b/drivers/gpu/drm/i915/display/intel_display_types.h @@ -688,11 +688,13 @@ struct skl_plane_wm { struct skl_wm_level wm[8]; struct skl_wm_level uv_wm[8]; struct skl_wm_level trans_wm; + struct skl_wm_level sagv_wm0; bool is_planar; }; struct skl_pipe_wm { struct skl_plane_wm planes[I915_MAX_PLANES]; + bool use_sagv_wm; }; enum vlv_wm_level { diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index db188efee21e..934a686342ad 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -3863,25 +3863,35 @@ bool intel_can_enable_sagv(struct drm_i915_private *dev_priv, return bw_state->pipe_sagv_reject == 0; } +static bool +tgl_crtc_can_enable_sagv(const struct intel_crtc_state *crtc_state); + static int intel_compute_sagv_mask(struct intel_atomic_state *state) { struct drm_i915_private *dev_priv = to_i915(state->base.dev); int ret; struct intel_crtc *crtc; - const struct intel_crtc_state *new_crtc_state; + struct intel_crtc_state *new_crtc_state; struct intel_bw_state *new_bw_state = NULL; const struct intel_bw_state *old_bw_state = NULL; int i; for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state, i) { + bool can_sagv; + new_bw_state = intel_atomic_get_bw_state(state); if (IS_ERR(new_bw_state)) return PTR_ERR(new_bw_state); old_bw_state = intel_atomic_get_old_bw_state(state); - if (skl_crtc_can_enable_sagv(new_crtc_state)) + if (INTEL_GEN(dev_priv) >= 12) + can_sagv = tgl_crtc_can_enable_sagv(new_crtc_state); + else + can_sagv = skl_crtc_can_enable_sagv(new_crtc_state); + + if (can_sagv) new_bw_state->pipe_sagv_reject &= ~BIT(crtc->pipe); else new_bw_state->pipe_sagv_reject |= BIT(crtc->pipe); @@ -3899,6 +3909,24 @@ static int intel_compute_sagv_mask(struct intel_atomic_state *state) return ret; } + for_each_new_intel_crtc_in_state(state, crtc, + new_crtc_state, i) { + struct skl_pipe_wm *pipe_wm = &new_crtc_state->wm.skl.optimal; + + /* + * Due to drm limitation at commit state, when + * changes are written the whole atomic state is + * zeroed away => which prevents from using it, + * so just sticking it into pipe wm state for + * keeping it simple - anyway this is related to wm. + * Proper way in ideal universe would be of course not + * to lose parent atomic state object from child crtc_state, + * and stick to OOP programming principles, which had been + * scientifically proven to work. + */ + pipe_wm->use_sagv_wm = intel_can_enable_sagv(dev_priv, new_bw_state); + } + if (intel_can_enable_sagv(dev_priv, new_bw_state) != intel_can_enable_sagv(dev_priv, old_bw_state)) { ret = intel_atomic_serialize_global_state(&new_bw_state->base); if (ret) @@ -4642,12 +4670,39 @@ skl_plane_wm_level(const struct intel_crtc_state *crtc_state, int level, int color_plane) { - const struct skl_plane_wm *wm = - &crtc_state->wm.skl.optimal.planes[plane_id]; + const struct skl_pipe_wm *pipe_wm = &crtc_state->wm.skl.optimal; + const struct skl_plane_wm *wm = &pipe_wm->planes[plane_id]; + + if (!level) { + if (pipe_wm->use_sagv_wm) + return &wm->sagv_wm0; + } return color_plane == 0 ? &wm->wm[level] : &wm->uv_wm[level]; } +static bool +tgl_crtc_can_enable_sagv(const struct intel_crtc_state *crtc_state) +{ + struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc); + enum plane_id plane_id; + + if (!crtc_state->hw.active) + return true; + + for_each_plane_id_on_crtc(crtc, plane_id) { + const struct skl_ddb_entry *plane_alloc = + &crtc_state->wm.skl.plane_ddb_y[plane_id]; + const struct skl_plane_wm *wm = + &crtc_state->wm.skl.optimal.planes[plane_id]; + + if (skl_ddb_entry_size(plane_alloc) < wm->sagv_wm0.min_ddb_alloc) + return false; + } + + return true; +} + static int skl_allocate_pipe_ddb(struct intel_crtc_state *crtc_state) { @@ -4684,7 +4739,6 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *crtc_state) plane_data_rate, uv_plane_data_rate); - skl_ddb_get_pipe_allocation_limits(dev_priv, crtc_state, total_data_rate, alloc, &num_active); alloc_size = skl_ddb_entry_size(alloc); @@ -5219,6 +5273,37 @@ skl_compute_wm_levels(const struct intel_crtc_state *crtc_state, } } +static void tgl_compute_sagv_wm(const struct intel_crtc_state *crtc_state, + const struct skl_wm_params *wm_params, + struct skl_plane_wm *plane_wm) +{ + struct drm_i915_private *dev_priv = to_i915(crtc_state->uapi.crtc->dev); + struct skl_wm_level *sagv_wm = &plane_wm->sagv_wm0; + struct skl_wm_level *levels = plane_wm->wm; + + /* + * For Gen12 if it is an L0 we need to also + * consider sagv_block_time when calculating + * L0 watermark - we will need that when making + * a decision whether enable SAGV or not. + * For older gens we agreed to copy L0 value for + * compatibility. + */ + if ((INTEL_GEN(dev_priv) >= 12)) { + u32 latency = dev_priv->wm.skl_latency[0]; + + latency += dev_priv->sagv_block_time_us; + skl_compute_plane_wm(crtc_state, 0, latency, + wm_params, &levels[0], + sagv_wm); + DRM_DEBUG_KMS("%d L0 blocks required for SAGV vs %d for non-SAGV\n", + sagv_wm->min_ddb_alloc, levels[0].min_ddb_alloc); + } else { + /* Since all members are POD */ + *sagv_wm = levels[0]; + } +} + static void skl_compute_transition_wm(const struct intel_crtc_state *crtc_state, const struct skl_wm_params *wp, struct skl_plane_wm *wm) @@ -5296,6 +5381,8 @@ static int skl_build_plane_wm_single(struct intel_crtc_state *crtc_state, return ret; skl_compute_wm_levels(crtc_state, &wm_params, wm->wm); + if (color_plane == 0) + tgl_compute_sagv_wm(crtc_state, &wm_params, wm); skl_compute_transition_wm(crtc_state, &wm_params, wm); return 0; @@ -5702,6 +5789,12 @@ skl_print_wm_changes(struct intel_atomic_state *state) enast(new_wm->wm[7].ignore_lines), new_wm->wm[7].plane_res_l, enast(new_wm->trans_wm.ignore_lines), new_wm->trans_wm.plane_res_l); + drm_dbg_kms(&dev_priv->drm, + "[PLANE:%d:%s] sagv wm0 lines %4d -> %4d\n", + plane->base.base.id, plane->base.name, + old_wm->sagv_wm0.plane_res_l, + new_wm->sagv_wm0.plane_res_l); + drm_dbg_kms(&dev_priv->drm, "[PLANE:%d:%s] blocks %4d,%4d,%4d,%4d,%4d,%4d,%4d,%4d,%4d" " -> %4d,%4d,%4d,%4d,%4d,%4d,%4d,%4d,%4d\n", @@ -5717,6 +5810,12 @@ skl_print_wm_changes(struct intel_atomic_state *state) new_wm->wm[6].plane_res_b, new_wm->wm[7].plane_res_b, new_wm->trans_wm.plane_res_b); + drm_dbg_kms(&dev_priv->drm, + "[PLANE:%d:%s] sagv wm0 blocks %4d -> %4d\n", + plane->base.base.id, plane->base.name, + old_wm->sagv_wm0.plane_res_b, + new_wm->sagv_wm0.plane_res_b); + drm_dbg_kms(&dev_priv->drm, "[PLANE:%d:%s] min_ddb %4d,%4d,%4d,%4d,%4d,%4d,%4d,%4d,%4d" " -> %4d,%4d,%4d,%4d,%4d,%4d,%4d,%4d,%4d\n", @@ -5731,6 +5830,12 @@ skl_print_wm_changes(struct intel_atomic_state *state) new_wm->wm[4].min_ddb_alloc, new_wm->wm[5].min_ddb_alloc, new_wm->wm[6].min_ddb_alloc, new_wm->wm[7].min_ddb_alloc, new_wm->trans_wm.min_ddb_alloc); + + drm_dbg_kms(&dev_priv->drm, + "[PLANE:%d:%s] sagv wm0 min ddb %4d -> %4d\n", + plane->base.base.id, plane->base.name, + old_wm->sagv_wm0.min_ddb_alloc, + new_wm->sagv_wm0.min_ddb_alloc); } } } @@ -6023,6 +6128,9 @@ void skl_pipe_wm_get_hw_state(struct intel_crtc *crtc, skl_wm_level_from_reg_val(val, &wm->wm[level]); } + memcpy(&wm->sagv_wm0, &wm->wm[0], + sizeof(struct skl_wm_level)); + if (plane_id != PLANE_CURSOR) val = I915_READ(PLANE_WM_TRANS(pipe, plane_id)); else
Starting from TGL we need to have a separate wm0 values for SAGV and non-SAGV which affects how calculations are done. v2: Remove long lines v3: Removed COLOR_PLANE enum references v4, v5, v6: Fixed rebase conflict v7: - Removed skl_plane_wm_level accessor from skl_allocate_pipe_ddb(Ville) - Removed sagv_uv_wm0(Ville) - can_sagv->use_sagv_wm(Ville) Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> --- drivers/gpu/drm/i915/display/intel_display.c | 8 +- .../drm/i915/display/intel_display_types.h | 2 + drivers/gpu/drm/i915/intel_pm.c | 118 +++++++++++++++++- 3 files changed, 121 insertions(+), 7 deletions(-)