On Mon, Mar 23, 2020 at 04:50:08PM +0200, Ville Syrjälä wrote: > On Mon, Mar 23, 2020 at 04:36:16PM +0200, Lisovskiy, Stanislav wrote: > > On Mon, Mar 23, 2020 at 04:18:36PM +0200, Ville Syrjälä wrote: > > > On Fri, Mar 20, 2020 at 02:51:41PM +0200, Lisovskiy, Stanislav wrote: > > > > On Wed, Mar 11, 2020 at 06:31:30PM +0200, Ville Syrjälä wrote: > > > > > On Mon, Mar 09, 2020 at 06:12:00PM +0200, Stanislav Lisovskiy wrote: > > > > > > Currently intel_can_enable_sagv function contains > > > > > > a mix of workarounds for different platforms > > > > > > some of them are not valid for gens >= 11 already, > > > > > > so lets split it into separate functions. > > > > > > > > > > > > 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. > > > > > > > > > > > > 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 | 18 + > > > > > > drivers/gpu/drm/i915/display/intel_display.c | 23 +- > > > > > > .../drm/i915/display/intel_display_types.h | 3 + > > > > > > drivers/gpu/drm/i915/intel_pm.c | 314 > > > > > > ++++++++++++++++-- > > > > > > drivers/gpu/drm/i915/intel_pm.h | 1 + > > > > > > 5 files changed, 318 insertions(+), 41 deletions(-) > > > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_bw.h > > > > > > b/drivers/gpu/drm/i915/display/intel_bw.h > > > > > > index b5f61463922f..4083adf4b432 100644 > > > > > > --- a/drivers/gpu/drm/i915/display/intel_bw.h > > > > > > +++ b/drivers/gpu/drm/i915/display/intel_bw.h > > > > > > @@ -18,6 +18,24 @@ 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; > > > > > > + > > > > > > + /* > > > > > > + * Used to determine if we already had calculated > > > > > > + * SAGV mask for this state once. > > > > > > + */ > > > > > > + bool sagv_calculated; > > > > > > > > > > Why would we even attempt to calculate it many times? > > > > > > > > > > > + > > > > > > + /* > > > > > > + * Contains final SAGV decision based on current mask, > > > > > > + * to prevent doing the same job over and over again. > > > > > > + */ > > > > > > + bool can_sagv; > > > > > > > > > > This is redundant since it's just sagv_reject==0. > > > > > > > > > > > + > > > > > > 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 8f23c4d51c33..9e0058a78ea6 100644 > > > > > > --- a/drivers/gpu/drm/i915/display/intel_display.c > > > > > > +++ b/drivers/gpu/drm/i915/display/intel_display.c > > > > > > @@ -14010,7 +14010,10 @@ 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]) || > > > > > > + (skl_wm_level_equals(&hw_plane_wm->wm[level], > > > > > > + &sw_plane_wm->sagv_wm0) > > > > > > && > > > > > > + (level == 0))) > > > > > > > > > > Pointless parens. Also we should do the check as > > > > > 'level == 0 && wm_equals(sagv)' to skip the pointless comparison when > > > > > level != 0. > > > > > > > > > > I guess we can't read out sagv state due to the silly pcode interface? > > > > > > > > > > > continue; > > > > > > > > > > > > drm_err(&dev_priv->drm, > > > > > > @@ -14065,7 +14068,10 @@ 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]) || > > > > > > + (skl_wm_level_equals(&hw_plane_wm->wm[level], > > > > > > + &sw_plane_wm->sagv_wm0) > > > > > > && > > > > > > + (level == 0))) > > > > > > continue; > > > > > > > > > > > > drm_err(&dev_priv->drm, > > > > > > @@ -15544,8 +15550,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 (!intel_can_enable_sagv(state)) > > > > > > + intel_disable_sagv(dev_priv); > > > > > > + } > > > > > > > > > > > > intel_modeset_verify_disabled(dev_priv, state); > > > > > > } > > > > > > @@ -15645,8 +15653,10 @@ 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) { > > > > > > + if (state->modeset && intel_can_enable_sagv(state)) > > > > > > + intel_enable_sagv(dev_priv); > > > > > > + } > > > > > > > > > > > > drm_atomic_helper_commit_hw_done(&state->base); > > > > > > > > > > > > @@ -15798,7 +15808,6 @@ static int intel_atomic_commit(struct > > > > > > drm_device *dev, > > > > > > > > > > > > if (state->global_state_changed) { > > > > > > assert_global_state_locked(dev_priv); > > > > > > - > > > > > > 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 5e00e611f077..da0308b87dad 100644 > > > > > > --- a/drivers/gpu/drm/i915/display/intel_display_types.h > > > > > > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h > > > > > > @@ -669,11 +669,14 @@ 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; > > > > > > }; > > > > > > > > > > > > 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 c72fa59a8302..f598b55f4abc 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! */ > > > > > > @@ -3757,39 +3758,25 @@ intel_disable_sagv(struct drm_i915_private > > > > > > *dev_priv) > > > > > > return 0; > > > > > > } > > > > > > > > > > > > -bool intel_can_enable_sagv(struct intel_atomic_state *state) > > > > > > +static bool skl_can_enable_sagv_on_pipe(struct intel_crtc_state > > > > > > *crtc_state) > > > > > > > > > > This extraction looks to be trivially done as a separate patch. > > > > > > > > > > > { > > > > > > - struct drm_device *dev = state->base.dev; > > > > > > + struct drm_device *dev = crtc_state->uapi.crtc->dev; > > > > > > struct drm_i915_private *dev_priv = to_i915(dev); > > > > > > + struct intel_atomic_state *state = > > > > > > to_intel_atomic_state(crtc_state->uapi.state); > > > > > > struct intel_crtc *crtc; > > > > > > struct intel_plane *plane; > > > > > > - struct intel_crtc_state *crtc_state; > > > > > > - enum pipe pipe; > > > > > > int level, latency; > > > > > > > > > > > > - if (!intel_has_sagv(dev_priv)) > > > > > > - return false; > > > > > > - > > > > > > - /* > > > > > > - * If there are no active CRTCs, no additional checks need be > > > > > > performed > > > > > > - */ > > > > > > - if (hweight8(state->active_pipes) == 0) > > > > > > - return true; > > > > > > + crtc = to_intel_crtc(crtc_state->uapi.crtc); > > > > > > > > > > > > - /* > > > > > > - * SKL+ workaround: bspec recommends we disable SAGV when we > > > > > > have > > > > > > - * more then one pipe enabled > > > > > > - */ > > > > > > - if (hweight8(state->active_pipes) > 1) > > > > > > + if ((INTEL_GEN(dev_priv) <= 9) && > > > > > > (hweight8(state->active_pipes) > 1)) > > > > > > return false; > > > > > > > > > > > > - /* Since we're now guaranteed to only have one active CRTC... */ > > > > > > - pipe = ffs(state->active_pipes) - 1; > > > > > > - crtc = intel_get_crtc_for_pipe(dev_priv, pipe); > > > > > > - crtc_state = to_intel_crtc_state(crtc->base.state); > > > > > > - > > > > > > - if (crtc_state->hw.adjusted_mode.flags & > > > > > > DRM_MODE_FLAG_INTERLACE) > > > > > > + if (crtc_state->hw.adjusted_mode.flags & > > > > > > DRM_MODE_FLAG_INTERLACE) { > > > > > > + DRM_DEBUG_KMS("No SAGV for interlaced mode on pipe > > > > > > %c\n", > > > > > > + pipe_name(crtc->pipe)); > > > > > > return false; > > > > > > + } > > > > > > > > > > > > for_each_intel_plane_on_crtc(dev, crtc, plane) { > > > > > > struct skl_plane_wm *wm = > > > > > > @@ -3816,13 +3803,145 @@ bool intel_can_enable_sagv(struct > > > > > > intel_atomic_state *state) > > > > > > * incur memory latencies higher than > > > > > > sagv_block_time_us we > > > > > > * can't enable SAGV. > > > > > > */ > > > > > > - if (latency < dev_priv->sagv_block_time_us) > > > > > > + if (latency < dev_priv->sagv_block_time_us) { > > > > > > + DRM_DEBUG_KMS("Latency %d < sagv block time %d, > > > > > > no SAGV for pipe %c\n", > > > > > > + latency, > > > > > > dev_priv->sagv_block_time_us, pipe_name(crtc->pipe)); > > > > > > return false; > > > > > > + } > > > > > > } > > > > > > > > > > > > return true; > > > > > > } > > > > > > > > > > > > +static bool > > > > > > +tgl_can_enable_sagv_on_pipe(struct intel_crtc_state *crtc_state); > > > > > > + > > > > > > +static bool intel_calculate_sagv_result(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 *new_crtc_state; > > > > > > + struct intel_bw_state *new_bw_state = NULL; > > > > > > + struct intel_bw_state *old_bw_state = NULL; > > > > > > + int i; > > > > > > + > > > > > > + /* > > > > > > + * 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. > > > > > > + */ > > > > > > + if (!intel_has_sagv(dev_priv)) > > > > > > + return 0; > > > > > > + > > > > > > + for_each_new_intel_crtc_in_state(state, crtc, > > > > > > + new_crtc_state, i) { > > > > > > + bool pipe_sagv_enable; > > > > > > + > > > > > > + new_bw_state = intel_bw_get_state(state); > > > > > > + old_bw_state = intel_bw_get_old_state(state); > > > > > > + > > > > > > + if (IS_ERR_OR_NULL(new_bw_state) || > > > > > > IS_ERR_OR_NULL(old_bw_state)) {a > > > > > > + WARN(1, "Could not get bw_state\n"); > > > > > > + return -EINVAL; > > > > > > > > > > What is this? > > > > > > > > > > > + } > > > > > > + > > > > > > + new_bw_state->sagv_calculated = false; > > > > > > + > > > > > > + if (INTEL_GEN(dev_priv) >= 12) > > > > > > + pipe_sagv_enable = > > > > > > tgl_can_enable_sagv_on_pipe(new_crtc_state); > > > > > > + else > > > > > > + pipe_sagv_enable = > > > > > > skl_can_enable_sagv_on_pipe(new_crtc_state); > > > > > > + > > > > > > + 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; > > > > > > + > > > > > > + new_bw_state->can_sagv = > > > > > > intel_calculate_sagv_result(new_bw_state); > > > > > > + new_bw_state->sagv_calculated = true; > > > > > > + > > > > > > + 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 = new_bw_state->can_sagv; > > > > > > > > > > I would probably name that wm->can_sagv as wm->use_sagv_wm so it's > > > > > clear > > > > > what it does. > > > > > > > > > > > + } > > > > > > + > > > > > > + /* > > > > > > + * 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. > > > > > > + */ > > > > > > + 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); > > > > > > + > > > > > > + ret = > > > > > > intel_atomic_serialize_global_state(&new_bw_state->base); > > > > > > + if (ret) { > > > > > > + DRM_DEBUG_KMS("Could not serialize global > > > > > > state\n"); > > > > > > + return ret; > > > > > > + } > > > > > > + } > > > > > > + > > > > > > + return 0; > > > > > > +} > > > > > > + > > > > > > +/* > > > > > > + * This function to be used before swap state > > > > > > + */ > > > > > > +bool intel_can_enable_sagv(struct intel_atomic_state *state) > > > > > > +{ > > > > > > + struct drm_device *dev = state->base.dev; > > > > > > + struct drm_i915_private *dev_priv = to_i915(dev); > > > > > > + struct intel_bw_state *bw_state; > > > > > > + > > > > > > + if (!intel_has_sagv(dev_priv)) { > > > > > > + DRM_DEBUG_KMS("No SAGV support detected\n"); > > > > > > + return false; > > > > > > + } > > > > > > + > > > > > > + bw_state = intel_bw_get_state(state); > > > > > > + > > > > > > + if (IS_ERR_OR_NULL(bw_state)) { > > > > > > > > > > It can't be NULL. And if you get an error you must propagate it > > > > > upwards. > > > > > > > > Can you please elaborate what I should do here? > > > > Just want to save some time without wasting time guessing. > > > > The options are: > > > > > > > > 1) If I propagate an error upwards, I obviously need to change a > > > > signature > > > > to int intel_can_enable_sagv, also if I do that usage at legacy > > > > callsites > > > > for this function will change, i.e you won't be able to call it like > > > > if (intel_can_enable_sagv()) anymore. Is it that what you want? > > > > Should note also that most of the legacy call sites are from > > > > commit_tail > > > > which wouldn't even propagate it further, because as you know it is > > > > already > > > > point of no return. > > > > > > Looks like intel_can_enable_sagv() should not exist anymore. We should > > > just precompute the sagv mask in the bw atomic check, and then the commit > > > time checks will simply become checks of the sagv mask. > > > > Was thinking about that, but then the question is how to deal with legacy > > stuff - for instance, skl doesn't have QGV points at all, as I understand > > some platforms just have it as a switch so we'll have to have somekind > > of code like this anyway, i.e just checking if we can and then switch. > > We should just have two different ways to calculate whether a pipe can > do sagv or not, one for skl+ another for icl+. Then we use the > appropriate method when computing the sagv mask.
Yes, I have two ways already, but I would like to have sagv mask computed in some centralized place i.e intel_compute_sagv_mask, so that each part of code has its own responsiblity and it is not spread across different functions/areas(SOLID). As I understand you want wm sagv checks to be done in allocate_pipe_ddb - ok I can transfer it there, but still I would like pipe_reject_mask to be set in a single place for obvious reasons. Also still the question is how we evaluate pipe_reject_mask - as I understand from you mean that I just nuke intel_can_enable_sagv and start just checking pipe_reject_mask == 0 instead everywhere. I'm still more in favour of having helpers for that - however in order to avoid continuing wasting time discussing err propagation and other secondary stuff - I'm fine with this :) > > > > > Or basically we would be just inlining the intel_can_enable_sagv to > > intel_atomic_commit_tail as I understand. Because yep for gen >=11 > > it just a matter of pre/post updating qgv points, but for skl we > > still need some condition to check when we are calling intel_enable/disable > > sagv > > > > > > Stan > > > > > > > > > > -- > > > Ville Syrjälä > > > Intel > > -- > Ville Syrjälä > Intel _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx