| Message ID | 20200403062003.11539-1-stanislav.lisovskiy@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | None | expand |
On Fri, Apr 03, 2020 at 09:20:03AM +0300, Stanislav Lisovskiy wrote: > Addressing one of the comments, recommending to extract platform > specific code from intel_can_enable_sagv as a preparation, before > we are going to add support for tgl+. > > Current code in intel_can_enable_sagv is valid only for skl, > so this patch adds also proper support for icl, subsequent > patches will add support for tgl+, combined with other required > changes. > > v2: - Renamed icl_can_enable_sagv into icl_crtc_can_enable_sagv(Ville) > - Removed dev variables(Ville) > - Constified crtc/plane_state in icl_crtc_can_enable_sagv > function(Ville) > - Added hw.active check(Ville) > - Refactored if ladder(Ville) > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > --- > drivers/gpu/drm/i915/intel_pm.c | 84 +++++++++++++++++++++------------ > 1 file changed, 55 insertions(+), 29 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index f8d62d1977ac..27d4d626cb34 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -3757,42 +3757,25 @@ intel_disable_sagv(struct drm_i915_private *dev_priv) > return 0; > } > > -bool intel_can_enable_sagv(struct intel_atomic_state *state) > +static bool icl_crtc_can_enable_sagv(const struct intel_crtc_state *crtc_state) > { > - struct drm_device *dev = state->base.dev; > - struct drm_i915_private *dev_priv = to_i915(dev); > - struct intel_crtc *crtc; > + struct drm_i915_private *dev_priv = to_i915(crtc_state->uapi.crtc->dev); > + struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc); > struct intel_plane *plane; > - struct intel_crtc_state *crtc_state; > - enum pipe pipe; > + const struct intel_plane_state *plane_state; > int level, latency; > > - if (!intel_has_sagv(dev_priv)) > + 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; > + } > > - /* > - * If there are no active CRTCs, no additional checks need be performed > - */ > - if (hweight8(state->active_pipes) == 0) > + if (!crtc_state->hw.active) Should really be checked before anything else. Doesn't matter too much anymore since I made us clear the crtc state always, but still a bit inconsistent to look at other stuff in the state before we even know if the crtc is even enabled. > return true; > > - /* > - * SKL+ workaround: bspec recommends we disable SAGV when we have > - * more then one pipe enabled > - */ > - if (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) > - return false; > - > - for_each_intel_plane_on_crtc(dev, crtc, plane) { > - struct skl_plane_wm *wm = > + intel_atomic_crtc_state_for_each_plane_state(plane, plane_state, crtc_state) { > + const struct skl_plane_wm *wm = > &crtc_state->wm.skl.optimal.planes[plane->id]; > > /* Skip this plane if it's not enabled */ > @@ -3807,7 +3790,7 @@ bool intel_can_enable_sagv(struct intel_atomic_state *state) > latency = dev_priv->wm.skl_latency[level]; > > if (skl_needs_memory_bw_wa(dev_priv) && > - plane->base.state->fb->modifier == > + plane_state->uapi.fb->modifier == hw.fb With those this is basically good, but still need to think how to avoid the regression due to only checking the crtcs in the state. > I915_FORMAT_MOD_X_TILED) > latency += 15; > > @@ -3823,6 +3806,49 @@ bool intel_can_enable_sagv(struct intel_atomic_state *state) > return true; > } > > +static bool skl_crtc_can_enable_sagv(const struct intel_crtc_state *crtc_state) > +{ > + struct intel_atomic_state *state = to_intel_atomic_state(crtc_state->uapi.state); > + > + /* > + * It has been recommended that for Gen 9 we switch SAGV off when > + * multiple pipes are used. > + */ > + if (hweight8(state->active_pipes) > 1) > + return false; > + > + /* > + * Besides active pipe limitation, rest of checks pretty much match ICL > + * so no need to duplicate code > + */ > + return icl_crtc_can_enable_sagv(crtc_state); > +} > + > +bool intel_can_enable_sagv(struct intel_atomic_state *state) > +{ > + struct drm_i915_private *dev_priv = to_i915(state->base.dev); > + struct intel_crtc *crtc; > + const struct intel_crtc_state *crtc_state; > + int i; > + > + if (!intel_has_sagv(dev_priv)) > + return false; > + > + for_each_new_intel_crtc_in_state(state, crtc, crtc_state, i) { > + bool can_sagv; > + > + if (INTEL_GEN(dev_priv) >= 11) > + can_sagv = icl_crtc_can_enable_sagv(crtc_state); > + else > + can_sagv = skl_crtc_can_enable_sagv(crtc_state); > + > + if (!can_sagv) > + return false; > + } > + > + return true; > +} > + > /* > * Calculate initial DBuf slice offset, based on slice size > * and mask(i.e if slice size is 1024 and second slice is enabled > -- > 2.24.1.485.gad05a3d8e5
On Tue, Apr 07, 2020 at 10:01:28PM +0300, Ville Syrjälä wrote: > On Fri, Apr 03, 2020 at 09:20:03AM +0300, Stanislav Lisovskiy wrote: > > Addressing one of the comments, recommending to extract platform > > specific code from intel_can_enable_sagv as a preparation, before > > we are going to add support for tgl+. > > > > Current code in intel_can_enable_sagv is valid only for skl, > > so this patch adds also proper support for icl, subsequent > > patches will add support for tgl+, combined with other required > > changes. > > > > v2: - Renamed icl_can_enable_sagv into icl_crtc_can_enable_sagv(Ville) > > - Removed dev variables(Ville) > > - Constified crtc/plane_state in icl_crtc_can_enable_sagv > > function(Ville) > > - Added hw.active check(Ville) > > - Refactored if ladder(Ville) > > > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > > --- > > drivers/gpu/drm/i915/intel_pm.c | 84 +++++++++++++++++++++------------ > > 1 file changed, 55 insertions(+), 29 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > > index f8d62d1977ac..27d4d626cb34 100644 > > --- a/drivers/gpu/drm/i915/intel_pm.c > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > @@ -3757,42 +3757,25 @@ intel_disable_sagv(struct drm_i915_private *dev_priv) > > return 0; > > } > > > > -bool intel_can_enable_sagv(struct intel_atomic_state *state) > > +static bool icl_crtc_can_enable_sagv(const struct intel_crtc_state *crtc_state) > > { > > - struct drm_device *dev = state->base.dev; > > - struct drm_i915_private *dev_priv = to_i915(dev); > > - struct intel_crtc *crtc; > > + struct drm_i915_private *dev_priv = to_i915(crtc_state->uapi.crtc->dev); > > + struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc); > > struct intel_plane *plane; > > - struct intel_crtc_state *crtc_state; > > - enum pipe pipe; > > + const struct intel_plane_state *plane_state; > > int level, latency; > > > > - if (!intel_has_sagv(dev_priv)) > > + 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; > > + } > > > > - /* > > - * If there are no active CRTCs, no additional checks need be performed > > - */ > > - if (hweight8(state->active_pipes) == 0) > > + if (!crtc_state->hw.active) > > Should really be checked before anything else. Doesn't matter too much > anymore since I made us clear the crtc state always, but still a bit > inconsistent to look at other stuff in the state before we even know if > the crtc is even enabled. > > > return true; > > > > - /* > > - * SKL+ workaround: bspec recommends we disable SAGV when we have > > - * more then one pipe enabled > > - */ > > - if (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) > > - return false; > > - > > - for_each_intel_plane_on_crtc(dev, crtc, plane) { > > - struct skl_plane_wm *wm = > > + intel_atomic_crtc_state_for_each_plane_state(plane, plane_state, crtc_state) { > > + const struct skl_plane_wm *wm = > > &crtc_state->wm.skl.optimal.planes[plane->id]; > > > > /* Skip this plane if it's not enabled */ > > @@ -3807,7 +3790,7 @@ bool intel_can_enable_sagv(struct intel_atomic_state *state) > > latency = dev_priv->wm.skl_latency[level]; > > > > if (skl_needs_memory_bw_wa(dev_priv) && > > - plane->base.state->fb->modifier == > > + plane_state->uapi.fb->modifier == > > hw.fb > > With those this is basically good, but still need to think how to avoid > the regression due to only checking the crtcs in the state. Well tbh, initially you told me that this *_crtc_can_enable_sagv function extraction can be "trivially" done as a separate patch :)) So I followed your instruction, and then I got a comment saying that "this is now temporary busted because we are checking only crtcs in a state". This kind of contraversial requirements - in order not to have it "temporary busted", we should have introduced it at the same time with SAGV mask, at the same time you wanted it to be extracted as a separate patch. In my opinion we are chasing own tail here. 1) Anyway this patch is transient and moreover it is quite likely that if wm/ddb allocations had changed we anyway now have all of those added into the state. 2) We could have just introduced it at the same time with a mask in bw_state thus avoiding that problem, what I actually had done in my _initial_ patch. Stan > > > I915_FORMAT_MOD_X_TILED) > > latency += 15; > > > > @@ -3823,6 +3806,49 @@ bool intel_can_enable_sagv(struct intel_atomic_state *state) > > return true; > > } > > > > +static bool skl_crtc_can_enable_sagv(const struct intel_crtc_state *crtc_state) > > +{ > > + struct intel_atomic_state *state = to_intel_atomic_state(crtc_state->uapi.state); > > + > > + /* > > + * It has been recommended that for Gen 9 we switch SAGV off when > > + * multiple pipes are used. > > + */ > > + if (hweight8(state->active_pipes) > 1) > > + return false; > > + > > + /* > > + * Besides active pipe limitation, rest of checks pretty much match ICL > > + * so no need to duplicate code > > + */ > > + return icl_crtc_can_enable_sagv(crtc_state); > > +} > > + > > +bool intel_can_enable_sagv(struct intel_atomic_state *state) > > +{ > > + struct drm_i915_private *dev_priv = to_i915(state->base.dev); > > + struct intel_crtc *crtc; > > + const struct intel_crtc_state *crtc_state; > > + int i; > > + > > + if (!intel_has_sagv(dev_priv)) > > + return false; > > + > > + for_each_new_intel_crtc_in_state(state, crtc, crtc_state, i) { > > + bool can_sagv; > > + > > + if (INTEL_GEN(dev_priv) >= 11) > > + can_sagv = icl_crtc_can_enable_sagv(crtc_state); > > + else > > + can_sagv = skl_crtc_can_enable_sagv(crtc_state); > > + > > + if (!can_sagv) > > + return false; > > + } > > + > > + return true; > > +} > > + > > /* > > * Calculate initial DBuf slice offset, based on slice size > > * and mask(i.e if slice size is 1024 and second slice is enabled > > -- > > 2.24.1.485.gad05a3d8e5 > > -- > Ville Syrjälä > Intel
On Wed, Apr 08, 2020 at 10:58:04AM +0300, Lisovskiy, Stanislav wrote: > On Tue, Apr 07, 2020 at 10:01:28PM +0300, Ville Syrjälä wrote: > > On Fri, Apr 03, 2020 at 09:20:03AM +0300, Stanislav Lisovskiy wrote: > > > Addressing one of the comments, recommending to extract platform > > > specific code from intel_can_enable_sagv as a preparation, before > > > we are going to add support for tgl+. > > > > > > Current code in intel_can_enable_sagv is valid only for skl, > > > so this patch adds also proper support for icl, subsequent > > > patches will add support for tgl+, combined with other required > > > changes. > > > > > > v2: - Renamed icl_can_enable_sagv into icl_crtc_can_enable_sagv(Ville) > > > - Removed dev variables(Ville) > > > - Constified crtc/plane_state in icl_crtc_can_enable_sagv > > > function(Ville) > > > - Added hw.active check(Ville) > > > - Refactored if ladder(Ville) > > > > > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > > > --- > > > drivers/gpu/drm/i915/intel_pm.c | 84 +++++++++++++++++++++------------ > > > 1 file changed, 55 insertions(+), 29 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > > > index f8d62d1977ac..27d4d626cb34 100644 > > > --- a/drivers/gpu/drm/i915/intel_pm.c > > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > > @@ -3757,42 +3757,25 @@ intel_disable_sagv(struct drm_i915_private *dev_priv) > > > return 0; > > > } > > > > > > -bool intel_can_enable_sagv(struct intel_atomic_state *state) > > > +static bool icl_crtc_can_enable_sagv(const struct intel_crtc_state *crtc_state) > > > { > > > - struct drm_device *dev = state->base.dev; > > > - struct drm_i915_private *dev_priv = to_i915(dev); > > > - struct intel_crtc *crtc; > > > + struct drm_i915_private *dev_priv = to_i915(crtc_state->uapi.crtc->dev); > > > + struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc); > > > struct intel_plane *plane; > > > - struct intel_crtc_state *crtc_state; > > > - enum pipe pipe; > > > + const struct intel_plane_state *plane_state; > > > int level, latency; > > > > > > - if (!intel_has_sagv(dev_priv)) > > > + 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; > > > + } > > > > > > - /* > > > - * If there are no active CRTCs, no additional checks need be performed > > > - */ > > > - if (hweight8(state->active_pipes) == 0) > > > + if (!crtc_state->hw.active) > > > > Should really be checked before anything else. Doesn't matter too much > > anymore since I made us clear the crtc state always, but still a bit > > inconsistent to look at other stuff in the state before we even know if > > the crtc is even enabled. > > > > > return true; > > > > > > - /* > > > - * SKL+ workaround: bspec recommends we disable SAGV when we have > > > - * more then one pipe enabled > > > - */ > > > - if (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) > > > - return false; > > > - > > > - for_each_intel_plane_on_crtc(dev, crtc, plane) { > > > - struct skl_plane_wm *wm = > > > + intel_atomic_crtc_state_for_each_plane_state(plane, plane_state, crtc_state) { > > > + const struct skl_plane_wm *wm = > > > &crtc_state->wm.skl.optimal.planes[plane->id]; > > > > > > /* Skip this plane if it's not enabled */ > > > @@ -3807,7 +3790,7 @@ bool intel_can_enable_sagv(struct intel_atomic_state *state) > > > latency = dev_priv->wm.skl_latency[level]; > > > > > > if (skl_needs_memory_bw_wa(dev_priv) && > > > - plane->base.state->fb->modifier == > > > + plane_state->uapi.fb->modifier == > > > > hw.fb > > > > With those this is basically good, but still need to think how to avoid > > the regression due to only checking the crtcs in the state. > > Well tbh, initially you told me that this *_crtc_can_enable_sagv function extraction > can be "trivially" done as a separate patch :)) So I followed your instruction, and > then I got a comment saying that "this is now temporary busted because we are checking > only crtcs in a state". This kind of contraversial requirements - in order not to > have it "temporary busted", we should have introduced it at the same time with SAGV mask, > at the same time you wanted it to be extracted as a separate patch. TBF this patch does quite a bit more than extract the current code. What I think would work as a series is something like: patch 1: +intel_crtc_can_enable_sagv(crtc_state) { + stuff } intel_can_enable_sagv(state) { ... crtc_state = to_intel_crtc_state(crtc->base.state); - stuff + return intel_crtc_can_eanble_sagv(crtc_state); } patch 2: +sagv_pre_plane_update(state) +{ + if (!intel_can_enable_sagv(state)) + intel_disable_sagv(dev_priv); +} intel_atomic_commit_tail() { ... - if (!intel_can_enable_sagv(state)) - intel_disable_sagv(dev_priv); + sagv_pre_plane_update(state); ... } (+ identical changes for post_plane_update()) So far everything has been pure refactoring. patch 3: Introduce the sagv mask in bw state and precompute it using intel_crtc_can_enable_sagv() (while fixing the iterator issue therein), and update the pre/post hooks to consult said mask. Not quite pure refactoring anymore but seems like a bit more straightforward change now. At this point we should have a nicely precomputed sagv mask without intentional changes to current behaviour. After which it should be easier to extend this for new platforms.
On Wed, Apr 08, 2020 at 05:55:02PM +0300, Ville Syrjälä wrote: > On Wed, Apr 08, 2020 at 10:58:04AM +0300, Lisovskiy, Stanislav wrote: > > On Tue, Apr 07, 2020 at 10:01:28PM +0300, Ville Syrjälä wrote: > > > On Fri, Apr 03, 2020 at 09:20:03AM +0300, Stanislav Lisovskiy wrote: > > > > Addressing one of the comments, recommending to extract platform > > > > specific code from intel_can_enable_sagv as a preparation, before > > > > we are going to add support for tgl+. > > > > > > > > Current code in intel_can_enable_sagv is valid only for skl, > > > > so this patch adds also proper support for icl, subsequent > > > > patches will add support for tgl+, combined with other required > > > > changes. > > > > > > > > v2: - Renamed icl_can_enable_sagv into icl_crtc_can_enable_sagv(Ville) > > > > - Removed dev variables(Ville) > > > > - Constified crtc/plane_state in icl_crtc_can_enable_sagv > > > > function(Ville) > > > > - Added hw.active check(Ville) > > > > - Refactored if ladder(Ville) > > > > > > > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > > > > --- > > > > drivers/gpu/drm/i915/intel_pm.c | 84 +++++++++++++++++++++------------ > > > > 1 file changed, 55 insertions(+), 29 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > > > > index f8d62d1977ac..27d4d626cb34 100644 > > > > --- a/drivers/gpu/drm/i915/intel_pm.c > > > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > > > @@ -3757,42 +3757,25 @@ intel_disable_sagv(struct drm_i915_private *dev_priv) > > > > return 0; > > > > } > > > > > > > > -bool intel_can_enable_sagv(struct intel_atomic_state *state) > > > > +static bool icl_crtc_can_enable_sagv(const struct intel_crtc_state *crtc_state) > > > > { > > > > - struct drm_device *dev = state->base.dev; > > > > - struct drm_i915_private *dev_priv = to_i915(dev); > > > > - struct intel_crtc *crtc; > > > > + struct drm_i915_private *dev_priv = to_i915(crtc_state->uapi.crtc->dev); > > > > + struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc); > > > > struct intel_plane *plane; > > > > - struct intel_crtc_state *crtc_state; > > > > - enum pipe pipe; > > > > + const struct intel_plane_state *plane_state; > > > > int level, latency; > > > > > > > > - if (!intel_has_sagv(dev_priv)) > > > > + 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; > > > > + } > > > > > > > > - /* > > > > - * If there are no active CRTCs, no additional checks need be performed > > > > - */ > > > > - if (hweight8(state->active_pipes) == 0) > > > > + if (!crtc_state->hw.active) > > > > > > Should really be checked before anything else. Doesn't matter too much > > > anymore since I made us clear the crtc state always, but still a bit > > > inconsistent to look at other stuff in the state before we even know if > > > the crtc is even enabled. > > > > > > > return true; > > > > > > > > - /* > > > > - * SKL+ workaround: bspec recommends we disable SAGV when we have > > > > - * more then one pipe enabled > > > > - */ > > > > - if (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) > > > > - return false; > > > > - > > > > - for_each_intel_plane_on_crtc(dev, crtc, plane) { > > > > - struct skl_plane_wm *wm = > > > > + intel_atomic_crtc_state_for_each_plane_state(plane, plane_state, crtc_state) { > > > > + const struct skl_plane_wm *wm = > > > > &crtc_state->wm.skl.optimal.planes[plane->id]; > > > > > > > > /* Skip this plane if it's not enabled */ > > > > @@ -3807,7 +3790,7 @@ bool intel_can_enable_sagv(struct intel_atomic_state *state) > > > > latency = dev_priv->wm.skl_latency[level]; > > > > > > > > if (skl_needs_memory_bw_wa(dev_priv) && > > > > - plane->base.state->fb->modifier == > > > > + plane_state->uapi.fb->modifier == > > > > > > hw.fb > > > > > > With those this is basically good, but still need to think how to avoid > > > the regression due to only checking the crtcs in the state. > > > > Well tbh, initially you told me that this *_crtc_can_enable_sagv function extraction > > can be "trivially" done as a separate patch :)) So I followed your instruction, and > > then I got a comment saying that "this is now temporary busted because we are checking > > only crtcs in a state". This kind of contraversial requirements - in order not to > > have it "temporary busted", we should have introduced it at the same time with SAGV mask, > > at the same time you wanted it to be extracted as a separate patch. > > TBF this patch does quite a bit more than extract the current code. > > What I think would work as a series is something like: > patch 1: > +intel_crtc_can_enable_sagv(crtc_state) > { > + stuff > } > > intel_can_enable_sagv(state) > { > ... > crtc_state = to_intel_crtc_state(crtc->base.state); > > - stuff > + return intel_crtc_can_eanble_sagv(crtc_state); > } > > patch 2: > +sagv_pre_plane_update(state) > +{ > + if (!intel_can_enable_sagv(state)) > + intel_disable_sagv(dev_priv); > +} > > intel_atomic_commit_tail() > { > ... > - if (!intel_can_enable_sagv(state)) > - intel_disable_sagv(dev_priv); > + sagv_pre_plane_update(state); > ... > } > > (+ identical changes for post_plane_update()) > > So far everything has been pure refactoring. > > patch 3: > Introduce the sagv mask in bw state and precompute it using > intel_crtc_can_enable_sagv() (while fixing the iterator issue therein), > and update the pre/post hooks to consult said mask. Not quite pure > refactoring anymore but seems like a bit more straightforward change > now. > > At this point we should have a nicely precomputed sagv mask without > intentional changes to current behaviour. After which it should be > easier to extend this for new platforms. This all makes sense, however in the end we'll have the same result as now, however this would require to reshuffle the whole series...again. Will try do it, the least painful way :) > > -- > Ville Syrjälä > Intel
On Wed, Apr 08, 2020 at 06:54:09PM +0300, Lisovskiy, Stanislav wrote: > On Wed, Apr 08, 2020 at 05:55:02PM +0300, Ville Syrjälä wrote: > > On Wed, Apr 08, 2020 at 10:58:04AM +0300, Lisovskiy, Stanislav wrote: > > > On Tue, Apr 07, 2020 at 10:01:28PM +0300, Ville Syrjälä wrote: > > > > On Fri, Apr 03, 2020 at 09:20:03AM +0300, Stanislav Lisovskiy wrote: > > > > > Addressing one of the comments, recommending to extract platform > > > > > specific code from intel_can_enable_sagv as a preparation, before > > > > > we are going to add support for tgl+. > > > > > > > > > > Current code in intel_can_enable_sagv is valid only for skl, > > > > > so this patch adds also proper support for icl, subsequent > > > > > patches will add support for tgl+, combined with other required > > > > > changes. > > > > > > > > > > v2: - Renamed icl_can_enable_sagv into icl_crtc_can_enable_sagv(Ville) > > > > > - Removed dev variables(Ville) > > > > > - Constified crtc/plane_state in icl_crtc_can_enable_sagv > > > > > function(Ville) > > > > > - Added hw.active check(Ville) > > > > > - Refactored if ladder(Ville) > > > > > > > > > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > > > > > --- > > > > > drivers/gpu/drm/i915/intel_pm.c | 84 +++++++++++++++++++++------------ > > > > > 1 file changed, 55 insertions(+), 29 deletions(-) > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > > > > > index f8d62d1977ac..27d4d626cb34 100644 > > > > > --- a/drivers/gpu/drm/i915/intel_pm.c > > > > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > > > > @@ -3757,42 +3757,25 @@ intel_disable_sagv(struct drm_i915_private *dev_priv) > > > > > return 0; > > > > > } > > > > > > > > > > -bool intel_can_enable_sagv(struct intel_atomic_state *state) > > > > > +static bool icl_crtc_can_enable_sagv(const struct intel_crtc_state *crtc_state) > > > > > { > > > > > - struct drm_device *dev = state->base.dev; > > > > > - struct drm_i915_private *dev_priv = to_i915(dev); > > > > > - struct intel_crtc *crtc; > > > > > + struct drm_i915_private *dev_priv = to_i915(crtc_state->uapi.crtc->dev); > > > > > + struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc); > > > > > struct intel_plane *plane; > > > > > - struct intel_crtc_state *crtc_state; > > > > > - enum pipe pipe; > > > > > + const struct intel_plane_state *plane_state; > > > > > int level, latency; > > > > > > > > > > - if (!intel_has_sagv(dev_priv)) > > > > > + 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; > > > > > + } > > > > > > > > > > - /* > > > > > - * If there are no active CRTCs, no additional checks need be performed > > > > > - */ > > > > > - if (hweight8(state->active_pipes) == 0) > > > > > + if (!crtc_state->hw.active) > > > > > > > > Should really be checked before anything else. Doesn't matter too much > > > > anymore since I made us clear the crtc state always, but still a bit > > > > inconsistent to look at other stuff in the state before we even know if > > > > the crtc is even enabled. > > > > > > > > > return true; > > > > > > > > > > - /* > > > > > - * SKL+ workaround: bspec recommends we disable SAGV when we have > > > > > - * more then one pipe enabled > > > > > - */ > > > > > - if (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) > > > > > - return false; > > > > > - > > > > > - for_each_intel_plane_on_crtc(dev, crtc, plane) { > > > > > - struct skl_plane_wm *wm = > > > > > + intel_atomic_crtc_state_for_each_plane_state(plane, plane_state, crtc_state) { > > > > > + const struct skl_plane_wm *wm = > > > > > &crtc_state->wm.skl.optimal.planes[plane->id]; > > > > > > > > > > /* Skip this plane if it's not enabled */ > > > > > @@ -3807,7 +3790,7 @@ bool intel_can_enable_sagv(struct intel_atomic_state *state) > > > > > latency = dev_priv->wm.skl_latency[level]; > > > > > > > > > > if (skl_needs_memory_bw_wa(dev_priv) && > > > > > - plane->base.state->fb->modifier == > > > > > + plane_state->uapi.fb->modifier == > > > > > > > > hw.fb > > > > > > > > With those this is basically good, but still need to think how to avoid > > > > the regression due to only checking the crtcs in the state. > > > > > > Well tbh, initially you told me that this *_crtc_can_enable_sagv function extraction > > > can be "trivially" done as a separate patch :)) So I followed your instruction, and > > > then I got a comment saying that "this is now temporary busted because we are checking > > > only crtcs in a state". This kind of contraversial requirements - in order not to > > > have it "temporary busted", we should have introduced it at the same time with SAGV mask, > > > at the same time you wanted it to be extracted as a separate patch. > > > > TBF this patch does quite a bit more than extract the current code. > > > > What I think would work as a series is something like: > > patch 1: > > +intel_crtc_can_enable_sagv(crtc_state) > > { > > + stuff > > } > > > > intel_can_enable_sagv(state) > > { > > ... > > crtc_state = to_intel_crtc_state(crtc->base.state); > > > > - stuff > > + return intel_crtc_can_eanble_sagv(crtc_state); > > } > > > > patch 2: > > +sagv_pre_plane_update(state) > > +{ > > + if (!intel_can_enable_sagv(state)) > > + intel_disable_sagv(dev_priv); > > +} > > > > intel_atomic_commit_tail() > > { > > ... > > - if (!intel_can_enable_sagv(state)) > > - intel_disable_sagv(dev_priv); > > + sagv_pre_plane_update(state); > > ... > > } > > > > (+ identical changes for post_plane_update()) > > > > So far everything has been pure refactoring. > > > > patch 3: > > Introduce the sagv mask in bw state and precompute it using > > intel_crtc_can_enable_sagv() (while fixing the iterator issue therein), > > and update the pre/post hooks to consult said mask. Not quite pure > > refactoring anymore but seems like a bit more straightforward change > > now. > > > > At this point we should have a nicely precomputed sagv mask without > > intentional changes to current behaviour. After which it should be > > easier to extend this for new platforms. > > This all makes sense, however in the end we'll have the same result as now, however this would > require to reshuffle the whole series...again. > Will try do it, the least painful way :) Also the only weird thing with this approach is that it is going to stay this ugly _one_ crtc way, until sagv mask and bw state is introduced: bool intel_can_enable_sagv(struct intel_atomic_state *state) { struct drm_i915_private *dev_priv = to_i915(state->base.dev); struct intel_crtc *crtc; const struct intel_crtc_state *crtc_state; int i; if (!intel_has_sagv(dev_priv)) return false; /* * SKL+ workaround: bspec recommends we disable SAGV when we have * more then one pipe enabled */ if (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); return intel_crtc_can_enable_sagv(crtc_state); } because you can't iterate crtcs anyway so that patch would be just a name change basically. The I can add pre/post plane update and only once bw state->sagv_mask is in place - the real SAGV changes can come. So SAGV logic would be anyway wrong in the middle of that series. Stan > > > > > -- > > Ville Syrjälä > > Intel
On Wed, Apr 08, 2020 at 07:18:11PM +0300, Lisovskiy, Stanislav wrote: > On Wed, Apr 08, 2020 at 06:54:09PM +0300, Lisovskiy, Stanislav wrote: > > On Wed, Apr 08, 2020 at 05:55:02PM +0300, Ville Syrjälä wrote: > > > On Wed, Apr 08, 2020 at 10:58:04AM +0300, Lisovskiy, Stanislav wrote: > > > > On Tue, Apr 07, 2020 at 10:01:28PM +0300, Ville Syrjälä wrote: > > > > > On Fri, Apr 03, 2020 at 09:20:03AM +0300, Stanislav Lisovskiy wrote: > > > > > > Addressing one of the comments, recommending to extract platform > > > > > > specific code from intel_can_enable_sagv as a preparation, before > > > > > > we are going to add support for tgl+. > > > > > > > > > > > > Current code in intel_can_enable_sagv is valid only for skl, > > > > > > so this patch adds also proper support for icl, subsequent > > > > > > patches will add support for tgl+, combined with other required > > > > > > changes. > > > > > > > > > > > > v2: - Renamed icl_can_enable_sagv into icl_crtc_can_enable_sagv(Ville) > > > > > > - Removed dev variables(Ville) > > > > > > - Constified crtc/plane_state in icl_crtc_can_enable_sagv > > > > > > function(Ville) > > > > > > - Added hw.active check(Ville) > > > > > > - Refactored if ladder(Ville) > > > > > > > > > > > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > > > > > > --- > > > > > > drivers/gpu/drm/i915/intel_pm.c | 84 +++++++++++++++++++++------------ > > > > > > 1 file changed, 55 insertions(+), 29 deletions(-) > > > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > > > > > > index f8d62d1977ac..27d4d626cb34 100644 > > > > > > --- a/drivers/gpu/drm/i915/intel_pm.c > > > > > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > > > > > @@ -3757,42 +3757,25 @@ intel_disable_sagv(struct drm_i915_private *dev_priv) > > > > > > return 0; > > > > > > } > > > > > > > > > > > > -bool intel_can_enable_sagv(struct intel_atomic_state *state) > > > > > > +static bool icl_crtc_can_enable_sagv(const struct intel_crtc_state *crtc_state) > > > > > > { > > > > > > - struct drm_device *dev = state->base.dev; > > > > > > - struct drm_i915_private *dev_priv = to_i915(dev); > > > > > > - struct intel_crtc *crtc; > > > > > > + struct drm_i915_private *dev_priv = to_i915(crtc_state->uapi.crtc->dev); > > > > > > + struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc); > > > > > > struct intel_plane *plane; > > > > > > - struct intel_crtc_state *crtc_state; > > > > > > - enum pipe pipe; > > > > > > + const struct intel_plane_state *plane_state; > > > > > > int level, latency; > > > > > > > > > > > > - if (!intel_has_sagv(dev_priv)) > > > > > > + 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; > > > > > > + } > > > > > > > > > > > > - /* > > > > > > - * If there are no active CRTCs, no additional checks need be performed > > > > > > - */ > > > > > > - if (hweight8(state->active_pipes) == 0) > > > > > > + if (!crtc_state->hw.active) > > > > > > > > > > Should really be checked before anything else. Doesn't matter too much > > > > > anymore since I made us clear the crtc state always, but still a bit > > > > > inconsistent to look at other stuff in the state before we even know if > > > > > the crtc is even enabled. > > > > > > > > > > > return true; > > > > > > > > > > > > - /* > > > > > > - * SKL+ workaround: bspec recommends we disable SAGV when we have > > > > > > - * more then one pipe enabled > > > > > > - */ > > > > > > - if (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) > > > > > > - return false; > > > > > > - > > > > > > - for_each_intel_plane_on_crtc(dev, crtc, plane) { > > > > > > - struct skl_plane_wm *wm = > > > > > > + intel_atomic_crtc_state_for_each_plane_state(plane, plane_state, crtc_state) { > > > > > > + const struct skl_plane_wm *wm = > > > > > > &crtc_state->wm.skl.optimal.planes[plane->id]; > > > > > > > > > > > > /* Skip this plane if it's not enabled */ > > > > > > @@ -3807,7 +3790,7 @@ bool intel_can_enable_sagv(struct intel_atomic_state *state) > > > > > > latency = dev_priv->wm.skl_latency[level]; > > > > > > > > > > > > if (skl_needs_memory_bw_wa(dev_priv) && > > > > > > - plane->base.state->fb->modifier == > > > > > > + plane_state->uapi.fb->modifier == > > > > > > > > > > hw.fb > > > > > > > > > > With those this is basically good, but still need to think how to avoid > > > > > the regression due to only checking the crtcs in the state. > > > > > > > > Well tbh, initially you told me that this *_crtc_can_enable_sagv function extraction > > > > can be "trivially" done as a separate patch :)) So I followed your instruction, and > > > > then I got a comment saying that "this is now temporary busted because we are checking > > > > only crtcs in a state". This kind of contraversial requirements - in order not to > > > > have it "temporary busted", we should have introduced it at the same time with SAGV mask, > > > > at the same time you wanted it to be extracted as a separate patch. > > > > > > TBF this patch does quite a bit more than extract the current code. > > > > > > What I think would work as a series is something like: > > > patch 1: > > > +intel_crtc_can_enable_sagv(crtc_state) > > > { > > > + stuff > > > } > > > > > > intel_can_enable_sagv(state) > > > { > > > ... > > > crtc_state = to_intel_crtc_state(crtc->base.state); > > > > > > - stuff > > > + return intel_crtc_can_eanble_sagv(crtc_state); > > > } > > > > > > patch 2: > > > +sagv_pre_plane_update(state) > > > +{ > > > + if (!intel_can_enable_sagv(state)) > > > + intel_disable_sagv(dev_priv); > > > +} > > > > > > intel_atomic_commit_tail() > > > { > > > ... > > > - if (!intel_can_enable_sagv(state)) > > > - intel_disable_sagv(dev_priv); > > > + sagv_pre_plane_update(state); > > > ... > > > } > > > > > > (+ identical changes for post_plane_update()) > > > > > > So far everything has been pure refactoring. > > > > > > patch 3: > > > Introduce the sagv mask in bw state and precompute it using > > > intel_crtc_can_enable_sagv() (while fixing the iterator issue therein), > > > and update the pre/post hooks to consult said mask. Not quite pure > > > refactoring anymore but seems like a bit more straightforward change > > > now. > > > > > > At this point we should have a nicely precomputed sagv mask without > > > intentional changes to current behaviour. After which it should be > > > easier to extend this for new platforms. > > > > This all makes sense, however in the end we'll have the same result as now, however this would > > require to reshuffle the whole series...again. > > Will try do it, the least painful way :) > > Also the only weird thing with this approach is that it is going to stay > this ugly _one_ crtc way, until sagv mask and bw state is introduced: > > bool intel_can_enable_sagv(struct intel_atomic_state *state) > { > struct drm_i915_private *dev_priv = to_i915(state->base.dev); > struct intel_crtc *crtc; > const struct intel_crtc_state *crtc_state; > int i; > > if (!intel_has_sagv(dev_priv)) > return false; > > /* > * SKL+ workaround: bspec recommends we disable SAGV when we have > * more then one pipe enabled > */ > if (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); > > return intel_crtc_can_enable_sagv(crtc_state); > } > > because you can't iterate crtcs anyway so that patch would be > just a name change basically. > The I can add pre/post plane update and only once bw state->sagv_mask > is in place - the real SAGV changes can come. So SAGV logic would be > anyway wrong in the middle of that series. No more wrong than it is now.
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index f8d62d1977ac..27d4d626cb34 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -3757,42 +3757,25 @@ intel_disable_sagv(struct drm_i915_private *dev_priv) return 0; } -bool intel_can_enable_sagv(struct intel_atomic_state *state) +static bool icl_crtc_can_enable_sagv(const struct intel_crtc_state *crtc_state) { - struct drm_device *dev = state->base.dev; - struct drm_i915_private *dev_priv = to_i915(dev); - struct intel_crtc *crtc; + struct drm_i915_private *dev_priv = to_i915(crtc_state->uapi.crtc->dev); + struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc); struct intel_plane *plane; - struct intel_crtc_state *crtc_state; - enum pipe pipe; + const struct intel_plane_state *plane_state; int level, latency; - if (!intel_has_sagv(dev_priv)) + 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; + } - /* - * If there are no active CRTCs, no additional checks need be performed - */ - if (hweight8(state->active_pipes) == 0) + if (!crtc_state->hw.active) return true; - /* - * SKL+ workaround: bspec recommends we disable SAGV when we have - * more then one pipe enabled - */ - if (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) - return false; - - for_each_intel_plane_on_crtc(dev, crtc, plane) { - struct skl_plane_wm *wm = + intel_atomic_crtc_state_for_each_plane_state(plane, plane_state, crtc_state) { + const struct skl_plane_wm *wm = &crtc_state->wm.skl.optimal.planes[plane->id]; /* Skip this plane if it's not enabled */ @@ -3807,7 +3790,7 @@ bool intel_can_enable_sagv(struct intel_atomic_state *state) latency = dev_priv->wm.skl_latency[level]; if (skl_needs_memory_bw_wa(dev_priv) && - plane->base.state->fb->modifier == + plane_state->uapi.fb->modifier == I915_FORMAT_MOD_X_TILED) latency += 15; @@ -3823,6 +3806,49 @@ bool intel_can_enable_sagv(struct intel_atomic_state *state) return true; } +static bool skl_crtc_can_enable_sagv(const struct intel_crtc_state *crtc_state) +{ + struct intel_atomic_state *state = to_intel_atomic_state(crtc_state->uapi.state); + + /* + * It has been recommended that for Gen 9 we switch SAGV off when + * multiple pipes are used. + */ + if (hweight8(state->active_pipes) > 1) + return false; + + /* + * Besides active pipe limitation, rest of checks pretty much match ICL + * so no need to duplicate code + */ + return icl_crtc_can_enable_sagv(crtc_state); +} + +bool intel_can_enable_sagv(struct intel_atomic_state *state) +{ + struct drm_i915_private *dev_priv = to_i915(state->base.dev); + struct intel_crtc *crtc; + const struct intel_crtc_state *crtc_state; + int i; + + if (!intel_has_sagv(dev_priv)) + return false; + + for_each_new_intel_crtc_in_state(state, crtc, crtc_state, i) { + bool can_sagv; + + if (INTEL_GEN(dev_priv) >= 11) + can_sagv = icl_crtc_can_enable_sagv(crtc_state); + else + can_sagv = skl_crtc_can_enable_sagv(crtc_state); + + if (!can_sagv) + return false; + } + + return true; +} + /* * Calculate initial DBuf slice offset, based on slice size * and mask(i.e if slice size is 1024 and second slice is enabled
Addressing one of the comments, recommending to extract platform specific code from intel_can_enable_sagv as a preparation, before we are going to add support for tgl+. Current code in intel_can_enable_sagv is valid only for skl, so this patch adds also proper support for icl, subsequent patches will add support for tgl+, combined with other required changes. v2: - Renamed icl_can_enable_sagv into icl_crtc_can_enable_sagv(Ville) - Removed dev variables(Ville) - Constified crtc/plane_state in icl_crtc_can_enable_sagv function(Ville) - Added hw.active check(Ville) - Refactored if ladder(Ville) Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> --- drivers/gpu/drm/i915/intel_pm.c | 84 +++++++++++++++++++++------------ 1 file changed, 55 insertions(+), 29 deletions(-)