On Thu, Mar 26, 2020 at 08:39:59PM +0200, Stanislav Lisovskiy wrote:
> Let's refactor the whole SAGV logic, moving
> the main calculations from intel_can_enable_sagv
> to intel_compute_sagv_mask, which also handles
> this in a unified way calling gen specific
> functions to evaluate if SAGV is allowed for
> each crtc. If crtc sagv mask have been changed
> we serialize access and modify global state.
>
> intel_can_enable_sagv now uses bw_state which
> stores all information related to SAGV and
> is now a trivial helper.
>
> v2:
> - Rework watermark calculation algorithm to
> attempt to calculate Level 0 watermark
> with added sagv block time latency and
> check if it fits in DBuf in order to
> determine if SAGV can be enabled already
> at this stage, just as BSpec 49325 states.
> if that fails rollback to usual Level 0
> latency and disable SAGV.
> - Remove unneeded tabs(James Ausmus)
>
> v3: Rebased the patch
>
> v4: - Added back interlaced check for Gen12 and
> added separate function for TGL SAGV check
> (thanks to James Ausmus for spotting)
> - Removed unneeded gen check
> - Extracted Gen12 SAGV decision making code
> to a separate function from skl_compute_wm
>
> v5: - Added SAGV global state to dev_priv, because
> we need to track all pipes, not only those
> in atomic state. Each pipe has now correspondent
> bit mask reflecting, whether it can tolerate
> SAGV or not(thanks to Ville Syrjala for suggestions).
> - Now using active flag instead of enable in crc
> usage check.
>
> v6: - Fixed rebase conflicts
>
> v7: - kms_cursor_legacy seems to get broken because of multiple memcpy
> calls when copying level 0 water marks for enabled SAGV, to
> fix this now simply using that field right away, without copying,
> for that introduced a new wm_level accessor which decides which
> wm_level to return based on SAGV state.
>
> v8: - Protect crtc_sagv_mask same way as we do for other global state
> changes: i.e check if changes are needed, then grab all crtc locks
> to serialize the changes(Ville Syrjälä)
> - Add crtc_sagv_mask caching in order to avoid needless recalculations
> (Matthew Roper)
> - Put back Gen12 SAGV switch in order to get it enabled in separate
> patch(Matthew Roper)
> - Rename *_set_sagv_mask to *_compute_sagv_mask(Matthew Roper)
> - Check if there are no active pipes in intel_can_enable_sagv
> instead of platform specific functions(Matthew Roper), same
> for intel_has_sagv check.
>
> v9 - Switched to u8 for crtc_sagv_mask(Ville Syrjälä)
> - crtc_sagv_mask now is pipe_sagv_mask(Ville Syrjälä)
> - Extracted sagv checking logic from skl/icl/tgl_compute_sagv_mask
> - Extracted skl_plane_wm_level function and passing latency to
> separate patches(Ville Syrjälä)
> - Removed part of unneeded copy-paste from tgl_check_pipe_fits_sagv_wm
> (Ville Syrjälä)
> - Now using simple assignment for sagv_wm0 as it contains only
> pod types and no pointers(Ville Syrjälä)
> - Fixed intel_can_enable_sagv not to do double duty, now it only
> check SAGV bits by ANDing those between local and global state.
> The SAGV masks are now computed after watermarks are available,
> in order to be able to figure out if ddb ranges are fitting nicely.
> (Ville Syrjälä)
> - Now having uv_sagv_wm0 and sagv_wm0, otherwise we have wrong logic
> when using skl_plane_wm_level accessor, as we had previously for
> Gen11+ color plane and regular wm levels, so probably both
> has to be recalculated with additional SAGV block time for Level 0.
>
> v10: - Starting to use new global state for storing pipe_sagv_mask
>
> v11: - Fixed rebase conflict with recent drm-tip
> - Check if we really need to recalculate SAGV mask, otherwise
> bail out without making any changes.
> - Use cached SAGV result, instead of recalculating it everytime,
> if bw_state hasn't changed.
>
> v12: - Removed WARN from intel_can_enable_sagv, in some of the commits
> if we don't recalculated watermarks, bw_state is not recalculated,
> thus leading to SAGV state not recalculated by the commit state,
> which is still calling intel_can_enable_sagv function. Fix that
> by just analyzing the current global bw_state object - because
> we simply have no other objects related to that.
>
> v13: - Rebased, fixed warnings regarding long lines
> - Changed function call sites from intel_atomic_bw* to
> intel_wb_* as was suggested.(Jani Nikula)
> - Taken ddb_state_changed and bw_state_changed into use.
>
> v14: - total_affected_planes is no longer needed to check for ddb changes,
> just as active_pipe_changes.
>
> v15: - Fixed stupid mistake with uninitialized crtc in
> skl_compute_sagv_mask.
>
> v16: - Convert pipe_sagv_mask to pipe_sagv_reject and now using inverted
> flag to indicate SAGV readiness for the pipe(Ville Syrjälä)
> - Added return value to intel_compute_sagv_mask which call
> intel_atomic_serialize_global_state in order to properly
> propagate EDEADLCK to drm.
> - Based on the discussion with Ville, removed active_pipe_changes
> check and also there seems to be no need for checking
> ddb_state_changes as well.
> Instead we just iterate through crtcs in state - having
> crtc in a state already guarantees that it is at least read-locked
> Having additional flag to check if there actually were some plane
> wm/ddb changes would be probably added later as an optimization.
> - We can't get parent atomic state from crtc_state at commit stage
> (nice drm feature), also propagating state through function call
> chain seems to be overkill and not possible(cursor legacy updates)
> Querying for bw_state object from global state is not possible as
> it might get swapped with other global state.
> So... just sticked can_sagv boolean into wm crtc state.
>
> v17: - Skip inactive crtcs, when checking for SAGV-readiness.
>
> v18: - Switch to use intel_atomic_crtc_state_for_each_plane_state
> instead of for_each_intel_plane_on_crtc and fixed previous
> code, which was using old plane state, which caused NULL ptr
> dereference, bacause that code is now called before we swap the
> state.
>
> v19: - Use intel_atomic_bw_* pattern again
> - Optimized sagv checks in verify_wm_state(Ville Syrjälä)
> - Do intel_compute_sagv_mask after ddb is allocated(Ville Syrjälä),
> using it's results
> - Use bw_state in intel_can_enable_sagv
> - Use COLOR_PLANE enum instead of boolean yuv or "0", "1" magic
> - Extracted sagv wm0 calculation into separate skl_compute_sagv_wm
> function(Ville Syrjälä)
>
> v20: - Added check for NULL for bw_state in commit_tail
>
> Signed-off-by: Stanislav Lisovskiy <stanislav.lisovs...@intel.com>
> Cc: Ville Syrjälä <ville.syrj...@intel.com>
> Cc: James Ausmus <james.aus...@intel.com>
> ---
> drivers/gpu/drm/i915/display/intel_bw.h | 6 +
> drivers/gpu/drm/i915/display/intel_display.c | 30 ++-
> .../drm/i915/display/intel_display_types.h | 3 +
> drivers/gpu/drm/i915/intel_pm.c | 244 ++++++++++++++++--
> drivers/gpu/drm/i915/intel_pm.h | 4 +-
> 5 files changed, 256 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_bw.h
> b/drivers/gpu/drm/i915/display/intel_bw.h
> index fe6579c952f5..6ad3ea7cedfe 100644
> --- a/drivers/gpu/drm/i915/display/intel_bw.h
> +++ b/drivers/gpu/drm/i915/display/intel_bw.h
> @@ -18,6 +18,12 @@ struct intel_crtc_state;
> struct intel_bw_state {
> struct intel_global_state base;
>
> + /*
> + * Contains a bit mask, used to determine, whether correspondent
> + * pipe allows SAGV or not.
> + */
> + u8 pipe_sagv_reject;
> +
> unsigned int data_rate[I915_MAX_PIPES];
> u8 num_active_planes[I915_MAX_PIPES];
> };
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> b/drivers/gpu/drm/i915/display/intel_display.c
> index e630429af2c0..8a0d952c67bf 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -14009,7 +14009,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,
> @@ -14064,7 +14066,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,
> @@ -15535,6 +15539,10 @@ static void intel_atomic_commit_tail(struct
> intel_atomic_state *state)
> crtc->config = new_crtc_state;
>
> if (state->modeset) {
> + struct intel_bw_state *bw_state;
> + bw_state = intel_atomic_get_bw_new_state(state);
Needlessly wide scope. Can declare+init at the same time. Also const.
> +
> drm_atomic_helper_update_legacy_modeset_state(dev,
> &state->base);
>
> intel_set_cdclk_pre_plane_update(state);
> @@ -15543,8 +15551,10 @@ static void intel_atomic_commit_tail(struct
> intel_atomic_state *state)
> * SKL workaround: bspec recommends we disable the SAGV when we
> * have more then one pipe enabled
> */
> - if (!intel_can_enable_sagv(state))
> - intel_disable_sagv(dev_priv);
> + if (INTEL_GEN(dev_priv) < 11) {
> + if (bw_state && !intel_can_enable_sagv(bw_state))
> + intel_disable_sagv(dev_priv);
> + }
>
> intel_modeset_verify_disabled(dev_priv, state);
> }
> @@ -15644,8 +15654,15 @@ static void intel_atomic_commit_tail(struct
> intel_atomic_state *state)
> if (state->modeset)
> intel_verify_planes(state);
>
> - if (state->modeset && intel_can_enable_sagv(state))
> - intel_enable_sagv(dev_priv);
> + if (INTEL_GEN(dev_priv) < 11) {
> + struct intel_bw_state *bw_state;
> +
> + bw_state = intel_atomic_get_bw_new_state(state);
> +
> + if (bw_state && state->modeset &&
> intel_can_enable_sagv(bw_state)) {
> + intel_enable_sagv(dev_priv);
> + }
Pointless {}
The different strcuture of the nested ifs also irks me a bit. But meh
for now I guess.
> + }
>
> drm_atomic_helper_commit_hw_done(&state->base);
>
> @@ -15797,7 +15814,6 @@ static int intel_atomic_commit(struct drm_device *dev,
>
> if (state->global_state_changed) {
> assert_global_state_locked(dev_priv);
> -
Spurious change
> dev_priv->active_pipes = state->active_pipes;
> }
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h
> b/drivers/gpu/drm/i915/display/intel_display_types.h
> index 523e0444b373..51fa5a746a5f 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> @@ -679,6 +679,8 @@ 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;
> + struct skl_wm_level uv_sagv_wm0;
> bool is_planar;
> };
>
> @@ -689,6 +691,7 @@ enum color_plane {
>
> struct skl_pipe_wm {
> struct skl_plane_wm planes[I915_MAX_PLANES];
> + bool can_sagv;
> };
>
> enum vlv_wm_level {
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 64193b098175..63c46918d1ed 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -43,6 +43,7 @@
> #include "i915_fixed.h"
> #include "i915_irq.h"
> #include "i915_trace.h"
> +#include "display/intel_bw.h"
> #include "intel_pm.h"
> #include "intel_sideband.h"
> #include "../../../platform/x86/intel_ips.h"
> @@ -3634,7 +3635,7 @@ static bool skl_needs_memory_bw_wa(struct
> drm_i915_private *dev_priv)
> return IS_GEN9_BC(dev_priv) || IS_BROXTON(dev_priv);
> }
>
> -static bool
> +bool
> intel_has_sagv(struct drm_i915_private *dev_priv)
> {
> /* HACK! */
> @@ -3827,33 +3828,106 @@ static bool skl_can_enable_sagv_on_pipe(struct
> intel_crtc_state *crtc_state)
> return icl_can_enable_sagv_on_pipe(crtc_state);
> }
>
> -bool intel_can_enable_sagv(struct intel_atomic_state *state)
> +static bool
> +tgl_can_enable_sagv_on_pipe(struct intel_crtc_state *crtc_state);
> +
> +bool intel_can_enable_sagv(struct intel_bw_state *bw_state)
> +{
> + return bw_state->pipe_sagv_reject == 0;
> +}
> +
> +static int intel_compute_sagv_mask(struct intel_atomic_state *state)
> {
> + int ret;
> struct drm_device *dev = state->base.dev;
> struct drm_i915_private *dev_priv = to_i915(dev);
> struct intel_crtc *crtc;
> - struct intel_crtc_state *crtc_state;
> + struct intel_crtc_state *new_crtc_state;
> + struct intel_bw_state *new_bw_state = NULL;
> + struct intel_bw_state *old_bw_state = NULL;
> int i;
> + bool can_sagv;
>
> + /*
> + * If SAGV is not supported we just can't do anything
> + * not even set or reject SAGV points - just bail out.
> + * Thus avoid needless calculations.
> + */
Seems overly verbose. I'd just drop the entire comment.
Rule of thumb: never add comments, unless absolutely necessary.
> if (!intel_has_sagv(dev_priv))
> - return false;
> + return 0;
> +
> + for_each_new_intel_crtc_in_state(state, crtc,
> + new_crtc_state, i) {
> + bool pipe_sagv_enable;
> +
> + new_bw_state = intel_atomic_get_bw_state(state);
> + if (IS_ERR(new_bw_state)) {
> + WARN(1, "Could not get bw_state\n");
What's with the warn?
> + return PTR_ERR(new_bw_state);
> + }
> +
> + old_bw_state = intel_atomic_get_bw_old_state(state);
> +
> + if (!new_crtc_state->hw.active)
> + continue;
There's the missing active check!. Just move into the functions(s)
below.
> +
> + if (INTEL_GEN(dev_priv) >= 12) {
> + pipe_sagv_enable =
> tgl_can_enable_sagv_on_pipe(new_crtc_state);
> + } else if (INTEL_GEN(dev_priv) >= 11) {
> + pipe_sagv_enable =
> icl_can_enable_sagv_on_pipe(new_crtc_state);
> + } else {
> + pipe_sagv_enable =
> skl_can_enable_sagv_on_pipe(new_crtc_state);
> + }
I would put that if ladder into a separate function to reduce the noise
here. At which point we don't even need this 'pipe_sagv_enable' bool
anymore and the code will read better.
> +
> + if (pipe_sagv_enable)
> + new_bw_state->pipe_sagv_reject &= ~BIT(crtc->pipe);
> + else
> + new_bw_state->pipe_sagv_reject |= BIT(crtc->pipe);
> + }
> +
> + if (!new_bw_state || !old_bw_state)
> + return 0;
> +
> + can_sagv = new_bw_state->pipe_sagv_reject == 0;
> +
> + 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->can_sagv = can_sagv;
> + }
>
> /*
> - * If there are no active CRTCs, no additional checks need be performed
> + * For SAGV we need to account all the pipes,
> + * not only the ones which are in state currently.
> + * Grab all locks if we detect that we are actually
> + * going to do something.
> */
More overly verbose comments. IMO just nuke.
> - if (hweight8(state->active_pipes) == 0)
> - return true;
> + if (new_bw_state->pipe_sagv_reject != old_bw_state->pipe_sagv_reject) {
> + DRM_DEBUG_KMS("State %p: old sagv mask 0x%x, new sagv mask
> 0x%x\n",
> + state,
> + old_bw_state->pipe_sagv_reject,
> + new_bw_state->pipe_sagv_reject);
Very temporary looking debug.
>
> - for_each_new_intel_crtc_in_state(state, crtc, crtc_state, i) {
> - if (INTEL_GEN(dev_priv) <= 9) {
> - if (!skl_can_enable_sagv_on_pipe(crtc_state))
> - return false;
> - } else if (!icl_can_enable_sagv_on_pipe(crtc_state)) {
> - return false;
> + ret = intel_atomic_serialize_global_state(&new_bw_state->base);
> + if (ret) {
> + DRM_DEBUG_KMS("Could not serialize global state\n");
No debugs for these pls.
> + return ret;
> }
> }
>
> - return true;
> + return 0;
> }
>
> /*
> @@ -4075,6 +4149,7 @@ skl_cursor_allocation(const struct intel_crtc_state
> *crtc_state,
> unsigned int latency = dev_priv->wm.skl_latency[level];
>
> skl_compute_plane_wm(crtc_state, level, latency, &wp, &wm, &wm);
> +
Spurious change.
> if (wm.min_ddb_alloc == U16_MAX)
> break;
>
> @@ -4589,9 +4664,66 @@ skl_plane_wm_level(const struct intel_crtc_state
> *crtc_state,
> const struct skl_plane_wm *wm =
> &crtc_state->wm.skl.optimal.planes[plane_id];
>
> + if (!level) {
> + const struct skl_pipe_wm *pipe_wm = &crtc_state->wm.skl.optimal;
Why is this here instead of higher up?
> +
> + if (pipe_wm->can_sagv)
> + return color_plane == COLOR_PLANE_Y ? &wm->sagv_wm0 :
> &wm->uv_sagv_wm0;
> + }
> +
> return color_plane == COLOR_PLANE_Y ? &wm->wm[level] :
> &wm->uv_wm[level];
> }
>
> +static bool
> +tgl_can_enable_sagv_on_pipe(struct intel_crtc_state *crtc_state)
const
> +{
> + struct intel_crtc *intel_crtc = to_intel_crtc(crtc_state->uapi.crtc);
'crtc'
> + enum plane_id plane_id;
> +
> + /*
> + * If pipe is not active it can't affect SAGV rejection
> + * Checking it here is needed to leave only cases when
> + * alloc_size is 0 for any other reasons, except inactive
> + * pipe. As inactive pipe is fine, however having no ddb
> + * space available is already problematic - so need to
> + * to separate those.
> + */
> + if (!crtc_state->hw.active)
> + return true;
> +
> + /*
> + * skl_allocate_pipe_ddb already done it's job to allocate
> + * as much blocks as possible for each plane - let's now
> + * evaluate if those were enough to enable SAGV.
> + * Criteria is that we need to have wm0 + sagv_block_time blocks
> + * for level 0 for each plane.
> + */
The comment-to-code ratio seems way too high to me.
> + for_each_plane_id_on_crtc(intel_crtc, plane_id) {
> + struct skl_ddb_entry *plane_alloc =
> + &crtc_state->wm.skl.plane_ddb_y[plane_id];
> + struct skl_ddb_entry *uv_plane_alloc =
> + &crtc_state->wm.skl.plane_ddb_uv[plane_id];
consts missing
> + 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) {
> + DRM_DEBUG_KMS("Not enough ddb blocks(%d-%d) for SAGV
> plane %d pipe %c\n",
> + plane_alloc->start, plane_alloc->end,
> plane_id,
> + pipe_name(intel_crtc->pipe));
Are we going to drown in debug noise?
> + return false;
> + }
> +
> + if (skl_ddb_entry_size(uv_plane_alloc) <
> wm->uv_sagv_wm0.min_ddb_alloc) {
> + DRM_DEBUG_KMS("Not enough ddb blocks(%d-%d) for SAGV uv
> plane %d pipe %c\n",
> + plane_alloc->start, plane_alloc->end,
> plane_id,
> + pipe_name(intel_crtc->pipe));
> + return false;
> + }
uv_wm isn't a thing on icl+.
> + }
> +
> + return true;
> +}
> +
> static int
> skl_allocate_pipe_ddb(struct intel_crtc_state *crtc_state)
> {
> @@ -5173,10 +5305,17 @@ static void skl_compute_plane_wm(const struct
> intel_crtc_state *crtc_state,
> static void
> skl_compute_wm_levels(const struct intel_crtc_state *crtc_state,
> const struct skl_wm_params *wm_params,
> - struct skl_wm_level *levels)
> + struct skl_plane_wm *plane_wm,
> + enum color_plane color_plane)
> {
> struct drm_i915_private *dev_priv = to_i915(crtc_state->uapi.crtc->dev);
> int level, max_level = ilk_wm_max_level(dev_priv);
> + /*
> + * Check which kind of plane is it and based on that calculate
> + * correspondent WM levels.
> + */
> + struct skl_wm_level *levels = color_plane == COLOR_PLANE_UV ?
> + plane_wm->uv_wm : plane_wm->wm;
> struct skl_wm_level *result_prev = &levels[0];
Why are we chancing this function? I see no reason for it.
>
> for (level = 0; level <= max_level; level++) {
> @@ -5190,6 +5329,40 @@ skl_compute_wm_levels(const struct intel_crtc_state
> *crtc_state,
> }
> }
>
> +static void skl_compute_sagv_wm(const struct intel_crtc_state *crtc_state,
> + const struct skl_wm_params *wm_params,
> + struct skl_plane_wm *plane_wm,
> + enum color_plane color_plane)
> +{
> + struct drm_i915_private *dev_priv = to_i915(crtc_state->uapi.crtc->dev);
> + struct skl_wm_level *sagv_wm = color_plane == COLOR_PLANE_UV ?
> + &plane_wm->uv_sagv_wm0 : &plane_wm->sagv_wm0;
> + struct skl_wm_level *levels = color_plane == COLOR_PLANE_UV ?
> + plane_wm->uv_wm : plane_wm->wm;
Seems wrong here too. uv_wm isn't a thing on icl+.
> +
> + /*
> + * 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)
> @@ -5270,7 +5443,8 @@ static int skl_build_plane_wm_single(struct
> intel_crtc_state *crtc_state,
> if (ret)
> return ret;
>
> - skl_compute_wm_levels(crtc_state, &wm_params, wm->wm);
> + skl_compute_wm_levels(crtc_state, &wm_params, wm, COLOR_PLANE_Y);
> + skl_compute_sagv_wm(crtc_state, &wm_params, wm, COLOR_PLANE_Y);
> skl_compute_transition_wm(crtc_state, &wm_params, wm);
>
> return 0;
> @@ -5292,7 +5466,8 @@ static int skl_build_plane_wm_uv(struct
> intel_crtc_state *crtc_state,
> if (ret)
> return ret;
>
> - skl_compute_wm_levels(crtc_state, &wm_params, wm->uv_wm);
> + skl_compute_wm_levels(crtc_state, &wm_params, wm, COLOR_PLANE_UV);
> + skl_compute_sagv_wm(crtc_state, &wm_params, wm, COLOR_PLANE_UV);
>
> return 0;
> }
> @@ -5631,9 +5806,25 @@ skl_print_wm_changes(struct intel_atomic_state *state)
> for_each_intel_plane_on_crtc(&dev_priv->drm, crtc, plane) {
> enum plane_id plane_id = plane->id;
> const struct skl_plane_wm *old_wm, *new_wm;
> + const struct skl_wm_level *old_wm_level, *new_wm_level;
> + u16 old_plane_res_l, new_plane_res_l;
> + u8 old_plane_res_b, new_plane_res_b;
> + u16 old_min_ddb_alloc, new_min_ddb_alloc;
> + int color_plane = 0;
>
> old_wm = &old_pipe_wm->planes[plane_id];
> new_wm = &new_pipe_wm->planes[plane_id];
> + old_wm_level = skl_plane_wm_level(old_crtc_state,
> plane_id, 0, color_plane);
> + new_wm_level = skl_plane_wm_level(new_crtc_state,
> plane_id, 0, color_plane);
> +
> + old_plane_res_l = old_wm_level->plane_res_l;
> + old_plane_res_b = old_wm_level->plane_res_b;
> +
> + new_plane_res_l = new_wm_level->plane_res_l;
> + new_plane_res_b = new_wm_level->plane_res_b;
> +
> + old_min_ddb_alloc = old_wm_level->min_ddb_alloc;
> + new_min_ddb_alloc = new_wm_level->min_ddb_alloc;
>
> if (skl_plane_wm_equals(dev_priv, old_wm, new_wm))
> continue;
> @@ -5657,7 +5848,7 @@ skl_print_wm_changes(struct intel_atomic_state *state)
> "[PLANE:%d:%s] lines
> %c%3d,%c%3d,%c%3d,%c%3d,%c%3d,%c%3d,%c%3d,%c%3d,%c%3d"
> " ->
> %c%3d,%c%3d,%c%3d,%c%3d,%c%3d,%c%3d,%c%3d,%c%3d,%c%3d\n",
> plane->base.base.id, plane->base.name,
> - enast(old_wm->wm[0].ignore_lines),
> old_wm->wm[0].plane_res_l,
> + enast(old_wm->wm[0].ignore_lines),
> old_plane_res_l,
> enast(old_wm->wm[1].ignore_lines),
> old_wm->wm[1].plane_res_l,
> enast(old_wm->wm[2].ignore_lines),
> old_wm->wm[2].plane_res_l,
> enast(old_wm->wm[3].ignore_lines),
> old_wm->wm[3].plane_res_l,
> @@ -5667,7 +5858,7 @@ skl_print_wm_changes(struct intel_atomic_state *state)
> enast(old_wm->wm[7].ignore_lines),
> old_wm->wm[7].plane_res_l,
> enast(old_wm->trans_wm.ignore_lines),
> old_wm->trans_wm.plane_res_l,
>
> - enast(new_wm->wm[0].ignore_lines),
> new_wm->wm[0].plane_res_l,
> + enast(new_wm->wm[0].ignore_lines),
> new_plane_res_l,
> enast(new_wm->wm[1].ignore_lines),
> new_wm->wm[1].plane_res_l,
> enast(new_wm->wm[2].ignore_lines),
> new_wm->wm[2].plane_res_l,
> enast(new_wm->wm[3].ignore_lines),
> new_wm->wm[3].plane_res_l,
> @@ -5681,12 +5872,12 @@ skl_print_wm_changes(struct intel_atomic_state *state)
> "[PLANE:%d:%s] blocks
> %4d,%4d,%4d,%4d,%4d,%4d,%4d,%4d,%4d"
> " -> %4d,%4d,%4d,%4d,%4d,%4d,%4d,%4d,%4d\n",
> plane->base.base.id, plane->base.name,
> - old_wm->wm[0].plane_res_b,
> old_wm->wm[1].plane_res_b,
> + old_plane_res_b, old_wm->wm[1].plane_res_b,
> old_wm->wm[2].plane_res_b,
> old_wm->wm[3].plane_res_b,
> old_wm->wm[4].plane_res_b,
> old_wm->wm[5].plane_res_b,
> old_wm->wm[6].plane_res_b,
> old_wm->wm[7].plane_res_b,
> old_wm->trans_wm.plane_res_b,
> - new_wm->wm[0].plane_res_b,
> new_wm->wm[1].plane_res_b,
> + new_plane_res_b, new_wm->wm[1].plane_res_b,
> new_wm->wm[2].plane_res_b,
> new_wm->wm[3].plane_res_b,
> new_wm->wm[4].plane_res_b,
> new_wm->wm[5].plane_res_b,
> new_wm->wm[6].plane_res_b,
> new_wm->wm[7].plane_res_b,
> @@ -5696,12 +5887,12 @@ skl_print_wm_changes(struct intel_atomic_state *state)
> "[PLANE:%d:%s] min_ddb
> %4d,%4d,%4d,%4d,%4d,%4d,%4d,%4d,%4d"
> " -> %4d,%4d,%4d,%4d,%4d,%4d,%4d,%4d,%4d\n",
> plane->base.base.id, plane->base.name,
> - old_wm->wm[0].min_ddb_alloc,
> old_wm->wm[1].min_ddb_alloc,
> + old_min_ddb_alloc,
> old_wm->wm[1].min_ddb_alloc,
> old_wm->wm[2].min_ddb_alloc,
> old_wm->wm[3].min_ddb_alloc,
> old_wm->wm[4].min_ddb_alloc,
> old_wm->wm[5].min_ddb_alloc,
> old_wm->wm[6].min_ddb_alloc,
> old_wm->wm[7].min_ddb_alloc,
> old_wm->trans_wm.min_ddb_alloc,
> - new_wm->wm[0].min_ddb_alloc,
> new_wm->wm[1].min_ddb_alloc,
> + new_min_ddb_alloc,
> new_wm->wm[1].min_ddb_alloc,
> new_wm->wm[2].min_ddb_alloc,
> new_wm->wm[3].min_ddb_alloc,
> 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,
I think I mentioned it before already, but I think we may want to just
dump the sagv wm0 as its own thing here.
> @@ -5866,6 +6057,10 @@ skl_compute_wm(struct intel_atomic_state *state)
> if (ret)
> return ret;
>
> + ret = intel_compute_sagv_mask(state);
> + if (ret)
> + return ret;
> +
> /*
> * skl_compute_ddb() will have adjusted the final watermarks
> * based on how much ddb is available. Now we can actually
> @@ -5993,6 +6188,9 @@ void skl_pipe_wm_get_hw_state(struct intel_crtc *crtc,
> val = I915_READ(CUR_WM(pipe, level));
>
> skl_wm_level_from_reg_val(val, &wm->wm[level]);
> + if (level == 0)
> + memcpy(&wm->sagv_wm0, &wm->wm[level],
> + sizeof(struct skl_wm_level));
A bit pointless to do that inside the loop.
> }
>
> if (plane_id != PLANE_CURSOR)
> diff --git a/drivers/gpu/drm/i915/intel_pm.h b/drivers/gpu/drm/i915/intel_pm.h
> index d60a85421c5a..5fb32db4abc5 100644
> --- a/drivers/gpu/drm/i915/intel_pm.h
> +++ b/drivers/gpu/drm/i915/intel_pm.h
> @@ -9,6 +9,7 @@
> #include <linux/types.h>
>
> #include "i915_reg.h"
> +#include "display/intel_bw.h"
>
> struct drm_device;
> struct drm_i915_private;
> @@ -41,7 +42,8 @@ void skl_pipe_wm_get_hw_state(struct intel_crtc *crtc,
> struct skl_pipe_wm *out);
> void g4x_wm_sanitize(struct drm_i915_private *dev_priv);
> void vlv_wm_sanitize(struct drm_i915_private *dev_priv);
> -bool intel_can_enable_sagv(struct intel_atomic_state *state);
> +bool intel_has_sagv(struct drm_i915_private *dev_priv);
> +bool intel_can_enable_sagv(struct intel_bw_state *bw_state);
> int intel_enable_sagv(struct drm_i915_private *dev_priv);
> int intel_disable_sagv(struct drm_i915_private *dev_priv);
> bool skl_wm_level_equals(const struct skl_wm_level *l1,
> --
> 2.24.1.485.gad05a3d8e5
--
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
