| Message ID | 20200505102247.32452-3-stanislav.lisovskiy@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | SAGV support for Gen12+ | expand |
On Tue, May 05, 2020 at 01:22:43PM +0300, Stanislav Lisovskiy wrote: > Introduce platform dependent SAGV checking in > combination with bandwidth state pipe SAGV mask. > > v2, v3, v4, v5, v6: Fix rebase conflict > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > --- > drivers/gpu/drm/i915/intel_pm.c | 30 ++++++++++++++++++++++++++++-- > 1 file changed, 28 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index da567fac7c93..c7d726a656b2 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -3853,6 +3853,24 @@ static bool intel_crtc_can_enable_sagv(const struct intel_crtc_state *crtc_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); > + /* > + * SKL+ workaround: bspec recommends we disable SAGV when we have > + * more then one pipe enabled > + */ > + if (hweight8(state->active_pipes) > 1) > + return false; That stuff should no longer be here since we now have it done properly in intel_can_eanble_sagv(). > + > + return intel_crtc_can_enable_sagv(crtc_state); > +} > + > +static bool icl_crtc_can_enable_sagv(const struct intel_crtc_state *crtc_state) > +{ > + return intel_crtc_can_enable_sagv(crtc_state); > +} This looks the wrong way around. IMO intel_crtc_can_enable_sagv() should rather call the skl vs. icl variants as needed. Although we don't yet have the icl variant so the oerdering of the patches is a bit weird. > + > bool intel_can_enable_sagv(const struct intel_bw_state *bw_state) > { > if (bw_state->active_pipes && !is_power_of_2(bw_state->active_pipes)) > @@ -3863,22 +3881,30 @@ bool intel_can_enable_sagv(const struct intel_bw_state *bw_state) > > static int intel_compute_sagv_mask(struct intel_atomic_state *state) > { > + struct drm_i915_private *dev_priv = to_i915(state->base.dev); > int ret; > struct intel_crtc *crtc; > - struct intel_crtc_state *new_crtc_state; > + const struct intel_crtc_state *new_crtc_state; > struct intel_bw_state *new_bw_state = NULL; > const struct intel_bw_state *old_bw_state = NULL; > int i; > > for_each_new_intel_crtc_in_state(state, crtc, > new_crtc_state, i) { > + bool can_sagv; > + > new_bw_state = intel_atomic_get_bw_state(state); > if (IS_ERR(new_bw_state)) > return PTR_ERR(new_bw_state); > > old_bw_state = intel_atomic_get_old_bw_state(state); > > - if (intel_crtc_can_enable_sagv(new_crtc_state)) > + if (INTEL_GEN(dev_priv) >= 11) > + can_sagv = icl_crtc_can_enable_sagv(new_crtc_state); > + else > + can_sagv = skl_crtc_can_enable_sagv(new_crtc_state); > + > + if (can_sagv) > new_bw_state->pipe_sagv_reject &= ~BIT(crtc->pipe); > else > new_bw_state->pipe_sagv_reject |= BIT(crtc->pipe); > -- > 2.24.1.485.gad05a3d8e5
On Tue, May 05, 2020 at 01:42:46PM +0300, Ville Syrjälä wrote: > On Tue, May 05, 2020 at 01:22:43PM +0300, Stanislav Lisovskiy wrote: > > Introduce platform dependent SAGV checking in > > combination with bandwidth state pipe SAGV mask. > > > > v2, v3, v4, v5, v6: Fix rebase conflict > > > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > > --- > > drivers/gpu/drm/i915/intel_pm.c | 30 ++++++++++++++++++++++++++++-- > > 1 file changed, 28 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > > index da567fac7c93..c7d726a656b2 100644 > > --- a/drivers/gpu/drm/i915/intel_pm.c > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > @@ -3853,6 +3853,24 @@ static bool intel_crtc_can_enable_sagv(const struct intel_crtc_state *crtc_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); > > + /* > > + * SKL+ workaround: bspec recommends we disable SAGV when we have > > + * more then one pipe enabled > > + */ > > + if (hweight8(state->active_pipes) > 1) > > + return false; > > That stuff should no longer be here since we now have it done properly > in intel_can_eanble_sagv(). > > > + > > + return intel_crtc_can_enable_sagv(crtc_state); > > +} > > + > > +static bool icl_crtc_can_enable_sagv(const struct intel_crtc_state *crtc_state) > > +{ > > + return intel_crtc_can_enable_sagv(crtc_state); > > +} > > This looks the wrong way around. IMO intel_crtc_can_enable_sagv() > should rather call the skl vs. icl variants as needed. Although we > don't yet have the icl variant so the oerdering of the patches is > a bit weird. Do we even need an icl variant actually? Does it use the skl or tgl way of checking for sagv yes vs. no? > > > + > > bool intel_can_enable_sagv(const struct intel_bw_state *bw_state) > > { > > if (bw_state->active_pipes && !is_power_of_2(bw_state->active_pipes)) > > @@ -3863,22 +3881,30 @@ bool intel_can_enable_sagv(const struct intel_bw_state *bw_state) > > > > static int intel_compute_sagv_mask(struct intel_atomic_state *state) > > { > > + struct drm_i915_private *dev_priv = to_i915(state->base.dev); > > int ret; > > struct intel_crtc *crtc; > > - struct intel_crtc_state *new_crtc_state; > > + const struct intel_crtc_state *new_crtc_state; > > struct intel_bw_state *new_bw_state = NULL; > > const struct intel_bw_state *old_bw_state = NULL; > > int i; > > > > for_each_new_intel_crtc_in_state(state, crtc, > > new_crtc_state, i) { > > + bool can_sagv; > > + > > new_bw_state = intel_atomic_get_bw_state(state); > > if (IS_ERR(new_bw_state)) > > return PTR_ERR(new_bw_state); > > > > old_bw_state = intel_atomic_get_old_bw_state(state); > > > > - if (intel_crtc_can_enable_sagv(new_crtc_state)) > > + if (INTEL_GEN(dev_priv) >= 11) > > + can_sagv = icl_crtc_can_enable_sagv(new_crtc_state); > > + else > > + can_sagv = skl_crtc_can_enable_sagv(new_crtc_state); > > + > > + if (can_sagv) > > new_bw_state->pipe_sagv_reject &= ~BIT(crtc->pipe); > > else > > new_bw_state->pipe_sagv_reject |= BIT(crtc->pipe); > > -- > > 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
On Tue, May 05, 2020 at 02:01:16PM +0300, Ville Syrjälä wrote: > On Tue, May 05, 2020 at 01:42:46PM +0300, Ville Syrjälä wrote: > > On Tue, May 05, 2020 at 01:22:43PM +0300, Stanislav Lisovskiy wrote: > > > Introduce platform dependent SAGV checking in > > > combination with bandwidth state pipe SAGV mask. > > > > > > v2, v3, v4, v5, v6: Fix rebase conflict > > > > > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > > > --- > > > drivers/gpu/drm/i915/intel_pm.c | 30 ++++++++++++++++++++++++++++-- > > > 1 file changed, 28 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > > > index da567fac7c93..c7d726a656b2 100644 > > > --- a/drivers/gpu/drm/i915/intel_pm.c > > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > > @@ -3853,6 +3853,24 @@ static bool intel_crtc_can_enable_sagv(const struct intel_crtc_state *crtc_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); > > > + /* > > > + * SKL+ workaround: bspec recommends we disable SAGV when we have > > > + * more then one pipe enabled > > > + */ > > > + if (hweight8(state->active_pipes) > 1) > > > + return false; > > > > That stuff should no longer be here since we now have it done properly > > in intel_can_eanble_sagv(). > > > > > + > > > + return intel_crtc_can_enable_sagv(crtc_state); > > > +} > > > + > > > +static bool icl_crtc_can_enable_sagv(const struct intel_crtc_state *crtc_state) > > > +{ > > > + return intel_crtc_can_enable_sagv(crtc_state); > > > +} > > > > This looks the wrong way around. IMO intel_crtc_can_enable_sagv() > > should rather call the skl vs. icl variants as needed. Although we > > don't yet have the icl variant so the oerdering of the patches is > > a bit weird. > > Do we even need an icl variant actually? Does it use the skl or tgl > way of checking for sagv yes vs. no? As I undestand icl implementation should be pretty much the same as skl, except that icl doesn't have this one active pipe limitation thing. Stan > > > > > > + > > > bool intel_can_enable_sagv(const struct intel_bw_state *bw_state) > > > { > > > if (bw_state->active_pipes && !is_power_of_2(bw_state->active_pipes)) > > > @@ -3863,22 +3881,30 @@ bool intel_can_enable_sagv(const struct intel_bw_state *bw_state) > > > > > > static int intel_compute_sagv_mask(struct intel_atomic_state *state) > > > { > > > + struct drm_i915_private *dev_priv = to_i915(state->base.dev); > > > int ret; > > > struct intel_crtc *crtc; > > > - struct intel_crtc_state *new_crtc_state; > > > + const struct intel_crtc_state *new_crtc_state; > > > struct intel_bw_state *new_bw_state = NULL; > > > const struct intel_bw_state *old_bw_state = NULL; > > > int i; > > > > > > for_each_new_intel_crtc_in_state(state, crtc, > > > new_crtc_state, i) { > > > + bool can_sagv; > > > + > > > new_bw_state = intel_atomic_get_bw_state(state); > > > if (IS_ERR(new_bw_state)) > > > return PTR_ERR(new_bw_state); > > > > > > old_bw_state = intel_atomic_get_old_bw_state(state); > > > > > > - if (intel_crtc_can_enable_sagv(new_crtc_state)) > > > + if (INTEL_GEN(dev_priv) >= 11) > > > + can_sagv = icl_crtc_can_enable_sagv(new_crtc_state); > > > + else > > > + can_sagv = skl_crtc_can_enable_sagv(new_crtc_state); > > > + > > > + if (can_sagv) > > > new_bw_state->pipe_sagv_reject &= ~BIT(crtc->pipe); > > > else > > > new_bw_state->pipe_sagv_reject |= BIT(crtc->pipe); > > > -- > > > 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 > > -- > Ville Syrjälä > Intel
On Tue, May 05, 2020 at 11:37:23PM +0300, Lisovskiy, Stanislav wrote: > On Tue, May 05, 2020 at 02:01:16PM +0300, Ville Syrjälä wrote: > > On Tue, May 05, 2020 at 01:42:46PM +0300, Ville Syrjälä wrote: > > > On Tue, May 05, 2020 at 01:22:43PM +0300, Stanislav Lisovskiy wrote: > > > > Introduce platform dependent SAGV checking in > > > > combination with bandwidth state pipe SAGV mask. > > > > > > > > v2, v3, v4, v5, v6: Fix rebase conflict > > > > > > > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > > > > --- > > > > drivers/gpu/drm/i915/intel_pm.c | 30 ++++++++++++++++++++++++++++-- > > > > 1 file changed, 28 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > > > > index da567fac7c93..c7d726a656b2 100644 > > > > --- a/drivers/gpu/drm/i915/intel_pm.c > > > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > > > @@ -3853,6 +3853,24 @@ static bool intel_crtc_can_enable_sagv(const struct intel_crtc_state *crtc_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); > > > > + /* > > > > + * SKL+ workaround: bspec recommends we disable SAGV when we have > > > > + * more then one pipe enabled > > > > + */ > > > > + if (hweight8(state->active_pipes) > 1) > > > > + return false; > > > > > > That stuff should no longer be here since we now have it done properly > > > in intel_can_eanble_sagv(). > > > > > > > + > > > > + return intel_crtc_can_enable_sagv(crtc_state); > > > > +} > > > > + > > > > +static bool icl_crtc_can_enable_sagv(const struct intel_crtc_state *crtc_state) > > > > +{ > > > > + return intel_crtc_can_enable_sagv(crtc_state); > > > > +} > > > > > > This looks the wrong way around. IMO intel_crtc_can_enable_sagv() > > > should rather call the skl vs. icl variants as needed. Although we > > > don't yet have the icl variant so the oerdering of the patches is > > > a bit weird. > > > > Do we even need an icl variant actually? Does it use the skl or tgl > > way of checking for sagv yes vs. no? > > As I undestand icl implementation should be pretty much the same as > skl, except that icl doesn't have this one active pipe limitation > thing. Right, and since that's now moved into intel_can_enable_sagv() we can use skl_crtc_can_enable_sagv() for all pre-tgl. > > > Stan > > > > > > > > > + > > > > bool intel_can_enable_sagv(const struct intel_bw_state *bw_state) > > > > { > > > > if (bw_state->active_pipes && !is_power_of_2(bw_state->active_pipes)) > > > > @@ -3863,22 +3881,30 @@ bool intel_can_enable_sagv(const struct intel_bw_state *bw_state) > > > > > > > > static int intel_compute_sagv_mask(struct intel_atomic_state *state) > > > > { > > > > + struct drm_i915_private *dev_priv = to_i915(state->base.dev); > > > > int ret; > > > > struct intel_crtc *crtc; > > > > - struct intel_crtc_state *new_crtc_state; > > > > + const struct intel_crtc_state *new_crtc_state; > > > > struct intel_bw_state *new_bw_state = NULL; > > > > const struct intel_bw_state *old_bw_state = NULL; > > > > int i; > > > > > > > > for_each_new_intel_crtc_in_state(state, crtc, > > > > new_crtc_state, i) { > > > > + bool can_sagv; > > > > + > > > > new_bw_state = intel_atomic_get_bw_state(state); > > > > if (IS_ERR(new_bw_state)) > > > > return PTR_ERR(new_bw_state); > > > > > > > > old_bw_state = intel_atomic_get_old_bw_state(state); > > > > > > > > - if (intel_crtc_can_enable_sagv(new_crtc_state)) > > > > + if (INTEL_GEN(dev_priv) >= 11) > > > > + can_sagv = icl_crtc_can_enable_sagv(new_crtc_state); > > > > + else > > > > + can_sagv = skl_crtc_can_enable_sagv(new_crtc_state); > > > > + > > > > + if (can_sagv) > > > > new_bw_state->pipe_sagv_reject &= ~BIT(crtc->pipe); > > > > else > > > > new_bw_state->pipe_sagv_reject |= BIT(crtc->pipe); > > > > -- > > > > 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 > > > > -- > > Ville Syrjälä > > Intel
On Tue, May 05, 2020 at 01:42:46PM +0300, Ville Syrjälä wrote: > On Tue, May 05, 2020 at 01:22:43PM +0300, Stanislav Lisovskiy wrote: > > Introduce platform dependent SAGV checking in > > combination with bandwidth state pipe SAGV mask. > > > > v2, v3, v4, v5, v6: Fix rebase conflict > > > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > > --- > > drivers/gpu/drm/i915/intel_pm.c | 30 ++++++++++++++++++++++++++++-- > > 1 file changed, 28 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > > index da567fac7c93..c7d726a656b2 100644 > > --- a/drivers/gpu/drm/i915/intel_pm.c > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > @@ -3853,6 +3853,24 @@ static bool intel_crtc_can_enable_sagv(const struct intel_crtc_state *crtc_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); > > + /* > > + * SKL+ workaround: bspec recommends we disable SAGV when we have > > + * more then one pipe enabled > > + */ > > + if (hweight8(state->active_pipes) > 1) > > + return false; > > That stuff should no longer be here since we now have it done properly > in intel_can_eanble_sagv(). > > > + > > + return intel_crtc_can_enable_sagv(crtc_state); > > +} > > + > > +static bool icl_crtc_can_enable_sagv(const struct intel_crtc_state *crtc_state) > > +{ > > + return intel_crtc_can_enable_sagv(crtc_state); > > +} > > This looks the wrong way around. IMO intel_crtc_can_enable_sagv() > should rather call the skl vs. icl variants as needed. Although we > don't yet have the icl variant so the oerdering of the patches is > a bit weird. This is done so, because icl and skl checking share the same code to check if SAGV can be enabled, except active_pipes > 1 thing. So that icl and skl can share the same code avoiding duplicating, i.e if I put code from intel_crtc_can_enable_sagv to skl_crtc_can_enable_sagv, I will have to 1) either duplicate this code to icl_crtc_can_enable_sagv(if I add remaining active_pipes check to skl) 2) use skl_crtc_can_enable_sagv from icl_crtc_can_enable_sagv, but this active_pipes check will be still outside of this skl function, which I don't find nice - to me the best way is to keep all skl specific checks in a correspondent function. So that is why I preferred to extract some common code to some separate universal function which can be then used from both icl and skl functions: from icl it is used "as is" and from skl it is intel_crtc_can_enable_sagv + this active_pipes check. Currently anyway we of course have that active_pipes check in intel_can_enable_sagv i.e already outside of skl_crtc_can_enable_sagv(where it should be imo), so was your intention to leave it outside anyway? Stan > > > + > > bool intel_can_enable_sagv(const struct intel_bw_state *bw_state) > > { > > if (bw_state->active_pipes && !is_power_of_2(bw_state->active_pipes)) > > @@ -3863,22 +3881,30 @@ bool intel_can_enable_sagv(const struct intel_bw_state *bw_state) > > > > static int intel_compute_sagv_mask(struct intel_atomic_state *state) > > { > > + struct drm_i915_private *dev_priv = to_i915(state->base.dev); > > int ret; > > struct intel_crtc *crtc; > > - struct intel_crtc_state *new_crtc_state; > > + const struct intel_crtc_state *new_crtc_state; > > struct intel_bw_state *new_bw_state = NULL; > > const struct intel_bw_state *old_bw_state = NULL; > > int i; > > > > for_each_new_intel_crtc_in_state(state, crtc, > > new_crtc_state, i) { > > + bool can_sagv; > > + > > new_bw_state = intel_atomic_get_bw_state(state); > > if (IS_ERR(new_bw_state)) > > return PTR_ERR(new_bw_state); > > > > old_bw_state = intel_atomic_get_old_bw_state(state); > > > > - if (intel_crtc_can_enable_sagv(new_crtc_state)) > > + if (INTEL_GEN(dev_priv) >= 11) > > + can_sagv = icl_crtc_can_enable_sagv(new_crtc_state); > > + else > > + can_sagv = skl_crtc_can_enable_sagv(new_crtc_state); > > + > > + if (can_sagv) > > new_bw_state->pipe_sagv_reject &= ~BIT(crtc->pipe); > > else > > new_bw_state->pipe_sagv_reject |= BIT(crtc->pipe); > > -- > > 2.24.1.485.gad05a3d8e5 > > -- > Ville Syrjälä > Intel
On Wed, May 06, 2020 at 10:55:44AM +0300, Lisovskiy, Stanislav wrote: > On Tue, May 05, 2020 at 01:42:46PM +0300, Ville Syrjälä wrote: > > On Tue, May 05, 2020 at 01:22:43PM +0300, Stanislav Lisovskiy wrote: > > > Introduce platform dependent SAGV checking in > > > combination with bandwidth state pipe SAGV mask. > > > > > > v2, v3, v4, v5, v6: Fix rebase conflict > > > > > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > > > --- > > > drivers/gpu/drm/i915/intel_pm.c | 30 ++++++++++++++++++++++++++++-- > > > 1 file changed, 28 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > > > index da567fac7c93..c7d726a656b2 100644 > > > --- a/drivers/gpu/drm/i915/intel_pm.c > > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > > @@ -3853,6 +3853,24 @@ static bool intel_crtc_can_enable_sagv(const struct intel_crtc_state *crtc_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); > > > + /* > > > + * SKL+ workaround: bspec recommends we disable SAGV when we have > > > + * more then one pipe enabled > > > + */ > > > + if (hweight8(state->active_pipes) > 1) > > > + return false; > > > > That stuff should no longer be here since we now have it done properly > > in intel_can_eanble_sagv(). > > > > > + > > > + return intel_crtc_can_enable_sagv(crtc_state); > > > +} > > > + > > > +static bool icl_crtc_can_enable_sagv(const struct intel_crtc_state *crtc_state) > > > +{ > > > + return intel_crtc_can_enable_sagv(crtc_state); > > > +} > > > > This looks the wrong way around. IMO intel_crtc_can_enable_sagv() > > should rather call the skl vs. icl variants as needed. Although we > > don't yet have the icl variant so the oerdering of the patches is > > a bit weird. > > This is done so, because icl and skl checking share the same code > to check if SAGV can be enabled, except active_pipes > 1 thing. > > So that icl and skl can share the same code avoiding duplicating, > i.e if I put code from intel_crtc_can_enable_sagv to > skl_crtc_can_enable_sagv, I will have to > 1) either duplicate this code to icl_crtc_can_enable_sagv(if I add remaining active_pipes check to > skl) > 2) use skl_crtc_can_enable_sagv from icl_crtc_can_enable_sagv, > but this active_pipes check will be still outside of this skl function, > which I don't find nice - to me the best way is to keep all skl > specific checks in a correspondent function. > > So that is why I preferred to extract some common code to some separate > universal function which can be then used from both icl and skl functions: > from icl it is used "as is" and from skl it is intel_crtc_can_enable_sagv > + this active_pipes check. > > Currently anyway we of course have that active_pipes check in intel_can_enable_sagv > i.e already outside of skl_crtc_can_enable_sagv(where it should be imo), > so was your intention to leave it outside anyway? My intention is that we eventually remove it altogeher. In the meantime intel_can_enable_sagv() looks like the right place for it.
On Wed, May 06, 2020 at 11:08:34AM +0300, Ville Syrjälä wrote: > On Wed, May 06, 2020 at 10:55:44AM +0300, Lisovskiy, Stanislav wrote: > > On Tue, May 05, 2020 at 01:42:46PM +0300, Ville Syrjälä wrote: > > > On Tue, May 05, 2020 at 01:22:43PM +0300, Stanislav Lisovskiy wrote: > > > > Introduce platform dependent SAGV checking in > > > > combination with bandwidth state pipe SAGV mask. > > > > > > > > v2, v3, v4, v5, v6: Fix rebase conflict > > > > > > > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > > > > --- > > > > drivers/gpu/drm/i915/intel_pm.c | 30 ++++++++++++++++++++++++++++-- > > > > 1 file changed, 28 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > > > > index da567fac7c93..c7d726a656b2 100644 > > > > --- a/drivers/gpu/drm/i915/intel_pm.c > > > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > > > @@ -3853,6 +3853,24 @@ static bool intel_crtc_can_enable_sagv(const struct intel_crtc_state *crtc_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); > > > > + /* > > > > + * SKL+ workaround: bspec recommends we disable SAGV when we have > > > > + * more then one pipe enabled > > > > + */ > > > > + if (hweight8(state->active_pipes) > 1) > > > > + return false; > > > > > > That stuff should no longer be here since we now have it done properly > > > in intel_can_eanble_sagv(). > > > > > > > + > > > > + return intel_crtc_can_enable_sagv(crtc_state); > > > > +} > > > > + > > > > +static bool icl_crtc_can_enable_sagv(const struct intel_crtc_state *crtc_state) > > > > +{ > > > > + return intel_crtc_can_enable_sagv(crtc_state); > > > > +} > > > > > > This looks the wrong way around. IMO intel_crtc_can_enable_sagv() > > > should rather call the skl vs. icl variants as needed. Although we > > > don't yet have the icl variant so the oerdering of the patches is > > > a bit weird. > > > > This is done so, because icl and skl checking share the same code > > to check if SAGV can be enabled, except active_pipes > 1 thing. > > > > So that icl and skl can share the same code avoiding duplicating, > > i.e if I put code from intel_crtc_can_enable_sagv to > > skl_crtc_can_enable_sagv, I will have to > > 1) either duplicate this code to icl_crtc_can_enable_sagv(if I add remaining active_pipes check to > > skl) > > 2) use skl_crtc_can_enable_sagv from icl_crtc_can_enable_sagv, > > but this active_pipes check will be still outside of this skl function, > > which I don't find nice - to me the best way is to keep all skl > > specific checks in a correspondent function. > > > > So that is why I preferred to extract some common code to some separate > > universal function which can be then used from both icl and skl functions: > > from icl it is used "as is" and from skl it is intel_crtc_can_enable_sagv > > + this active_pipes check. > > > > Currently anyway we of course have that active_pipes check in intel_can_enable_sagv > > i.e already outside of skl_crtc_can_enable_sagv(where it should be imo), > > so was your intention to leave it outside anyway? > > My intention is that we eventually remove it altogeher. In the > meantime intel_can_enable_sagv() looks like the right place > for it. Wow, you were really fast replying :) Ok, then I just leave skl_crtc_can_enable_sagv, use it for icl and active_pipes check stays in intel_can_enable_sagv, however probably it will now need somekind of INTEL_GEN < 11 check, I guess. Stan > > -- > Ville Syrjälä > Intel
On Wed, May 06, 2020 at 11:43:30AM +0300, Lisovskiy, Stanislav wrote: > On Wed, May 06, 2020 at 11:08:34AM +0300, Ville Syrjälä wrote: > > On Wed, May 06, 2020 at 10:55:44AM +0300, Lisovskiy, Stanislav wrote: > > > On Tue, May 05, 2020 at 01:42:46PM +0300, Ville Syrjälä wrote: > > > > On Tue, May 05, 2020 at 01:22:43PM +0300, Stanislav Lisovskiy wrote: > > > > > Introduce platform dependent SAGV checking in > > > > > combination with bandwidth state pipe SAGV mask. > > > > > > > > > > v2, v3, v4, v5, v6: Fix rebase conflict > > > > > > > > > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > > > > > --- > > > > > drivers/gpu/drm/i915/intel_pm.c | 30 ++++++++++++++++++++++++++++-- > > > > > 1 file changed, 28 insertions(+), 2 deletions(-) > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > > > > > index da567fac7c93..c7d726a656b2 100644 > > > > > --- a/drivers/gpu/drm/i915/intel_pm.c > > > > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > > > > @@ -3853,6 +3853,24 @@ static bool intel_crtc_can_enable_sagv(const struct intel_crtc_state *crtc_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); > > > > > + /* > > > > > + * SKL+ workaround: bspec recommends we disable SAGV when we have > > > > > + * more then one pipe enabled > > > > > + */ > > > > > + if (hweight8(state->active_pipes) > 1) > > > > > + return false; > > > > > > > > That stuff should no longer be here since we now have it done properly > > > > in intel_can_eanble_sagv(). > > > > > > > > > + > > > > > + return intel_crtc_can_enable_sagv(crtc_state); > > > > > +} > > > > > + > > > > > +static bool icl_crtc_can_enable_sagv(const struct intel_crtc_state *crtc_state) > > > > > +{ > > > > > + return intel_crtc_can_enable_sagv(crtc_state); > > > > > +} > > > > > > > > This looks the wrong way around. IMO intel_crtc_can_enable_sagv() > > > > should rather call the skl vs. icl variants as needed. Although we > > > > don't yet have the icl variant so the oerdering of the patches is > > > > a bit weird. > > > > > > This is done so, because icl and skl checking share the same code > > > to check if SAGV can be enabled, except active_pipes > 1 thing. > > > > > > So that icl and skl can share the same code avoiding duplicating, > > > i.e if I put code from intel_crtc_can_enable_sagv to > > > skl_crtc_can_enable_sagv, I will have to > > > 1) either duplicate this code to icl_crtc_can_enable_sagv(if I add remaining active_pipes check to > > > skl) > > > 2) use skl_crtc_can_enable_sagv from icl_crtc_can_enable_sagv, > > > but this active_pipes check will be still outside of this skl function, > > > which I don't find nice - to me the best way is to keep all skl > > > specific checks in a correspondent function. > > > > > > So that is why I preferred to extract some common code to some separate > > > universal function which can be then used from both icl and skl functions: > > > from icl it is used "as is" and from skl it is intel_crtc_can_enable_sagv > > > + this active_pipes check. > > > > > > Currently anyway we of course have that active_pipes check in intel_can_enable_sagv > > > i.e already outside of skl_crtc_can_enable_sagv(where it should be imo), > > > so was your intention to leave it outside anyway? > > > > My intention is that we eventually remove it altogeher. In the > > meantime intel_can_enable_sagv() looks like the right place > > for it. > > Wow, you were really fast replying :) Ok, then I just leave skl_crtc_can_enable_sagv, > use it for icl and active_pipes check stays in intel_can_enable_sagv, however probably > it will now need somekind of INTEL_GEN < 11 check, I guess. If we want to enable sagv for multi-pipe on just icl+ first, yes. I guess we can do that. I suggest making that a standalone patch so we can easily revert it if things blow up. We can then let that bake for a while and follow up later with nuking the single pipe restriction for all platforms.
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index da567fac7c93..c7d726a656b2 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -3853,6 +3853,24 @@ static bool intel_crtc_can_enable_sagv(const struct intel_crtc_state *crtc_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); + /* + * SKL+ workaround: bspec recommends we disable SAGV when we have + * more then one pipe enabled + */ + if (hweight8(state->active_pipes) > 1) + return false; + + return intel_crtc_can_enable_sagv(crtc_state); +} + +static bool icl_crtc_can_enable_sagv(const struct intel_crtc_state *crtc_state) +{ + return intel_crtc_can_enable_sagv(crtc_state); +} + bool intel_can_enable_sagv(const struct intel_bw_state *bw_state) { if (bw_state->active_pipes && !is_power_of_2(bw_state->active_pipes)) @@ -3863,22 +3881,30 @@ bool intel_can_enable_sagv(const struct intel_bw_state *bw_state) static int intel_compute_sagv_mask(struct intel_atomic_state *state) { + struct drm_i915_private *dev_priv = to_i915(state->base.dev); int ret; struct intel_crtc *crtc; - struct intel_crtc_state *new_crtc_state; + const struct intel_crtc_state *new_crtc_state; struct intel_bw_state *new_bw_state = NULL; const struct intel_bw_state *old_bw_state = NULL; int i; for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state, i) { + bool can_sagv; + new_bw_state = intel_atomic_get_bw_state(state); if (IS_ERR(new_bw_state)) return PTR_ERR(new_bw_state); old_bw_state = intel_atomic_get_old_bw_state(state); - if (intel_crtc_can_enable_sagv(new_crtc_state)) + if (INTEL_GEN(dev_priv) >= 11) + can_sagv = icl_crtc_can_enable_sagv(new_crtc_state); + else + can_sagv = skl_crtc_can_enable_sagv(new_crtc_state); + + if (can_sagv) new_bw_state->pipe_sagv_reject &= ~BIT(crtc->pipe); else new_bw_state->pipe_sagv_reject |= BIT(crtc->pipe);
Introduce platform dependent SAGV checking in combination with bandwidth state pipe SAGV mask. v2, v3, v4, v5, v6: Fix rebase conflict Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> --- drivers/gpu/drm/i915/intel_pm.c | 30 ++++++++++++++++++++++++++++-- 1 file changed, 28 insertions(+), 2 deletions(-)