| Message ID | 20200507144503.15506-4-stanislav.lisovskiy@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | SAGV support for Gen12+ | expand |
On Thu, May 07, 2020 at 05:45:00PM +0300, Stanislav Lisovskiy wrote: > Seems that only skl needs to have SAGV turned off > for multipipe scenarios, so lets do it this way. It doesn't afaics. It's just someone added the check for some random reason. So this should be reworded a bit. Also this isn't just about skl/derivatives but all pre-icl so the <subject> is a bit misleading too. > > If anything blows up - we can always revert this patch. > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > --- > drivers/gpu/drm/i915/intel_pm.c | 15 +++++++++------ > drivers/gpu/drm/i915/intel_pm.h | 3 ++- > 2 files changed, 11 insertions(+), 7 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index 3dc1ad66beb3..db188efee21e 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -3777,7 +3777,7 @@ void intel_sagv_pre_plane_update(struct intel_atomic_state *state) > if (!new_bw_state) > return; > > - if (!intel_can_enable_sagv(new_bw_state)) > + if (!intel_can_enable_sagv(dev_priv, new_bw_state)) > intel_disable_sagv(dev_priv); > } > > @@ -3800,7 +3800,7 @@ void intel_sagv_post_plane_update(struct intel_atomic_state *state) > if (!new_bw_state) > return; > > - if (intel_can_enable_sagv(new_bw_state)) > + if (intel_can_enable_sagv(dev_priv, new_bw_state)) > intel_enable_sagv(dev_priv); > } > > @@ -3853,16 +3853,19 @@ static bool skl_crtc_can_enable_sagv(const struct intel_crtc_state *crtc_state) > return true; > } > > -bool intel_can_enable_sagv(const struct intel_bw_state *bw_state) > +bool intel_can_enable_sagv(struct drm_i915_private *dev_priv, > + const struct intel_bw_state *bw_state) > { > - if (bw_state->active_pipes && !is_power_of_2(bw_state->active_pipes)) > - return false; > + if (INTEL_GEN(dev_priv) < 11) > + if (bw_state->active_pipes && !is_power_of_2(bw_state->active_pipes)) If (a && b && c) return false; > + return false; > > return bw_state->pipe_sagv_reject == 0; > } > > static int intel_compute_sagv_mask(struct intel_atomic_state *state) > { > + struct drm_i915_private *dev_priv = to_i915(state->base.dev); > int ret; > struct intel_crtc *crtc; > const struct intel_crtc_state *new_crtc_state; > @@ -3896,7 +3899,7 @@ static int intel_compute_sagv_mask(struct intel_atomic_state *state) > return ret; > } > > - if (intel_can_enable_sagv(new_bw_state) != intel_can_enable_sagv(old_bw_state)) { > + if (intel_can_enable_sagv(dev_priv, new_bw_state) != intel_can_enable_sagv(dev_priv, old_bw_state)) { > ret = intel_atomic_serialize_global_state(&new_bw_state->base); > if (ret) > return ret; > diff --git a/drivers/gpu/drm/i915/intel_pm.h b/drivers/gpu/drm/i915/intel_pm.h > index fd1dc422e6c5..614ac7f8d4cc 100644 > --- a/drivers/gpu/drm/i915/intel_pm.h > +++ b/drivers/gpu/drm/i915/intel_pm.h > @@ -42,7 +42,8 @@ void skl_pipe_wm_get_hw_state(struct intel_crtc *crtc, > struct skl_pipe_wm *out); > void g4x_wm_sanitize(struct drm_i915_private *dev_priv); > void vlv_wm_sanitize(struct drm_i915_private *dev_priv); > -bool intel_can_enable_sagv(const struct intel_bw_state *bw_state); > +bool intel_can_enable_sagv(struct drm_i915_private *dev_priv, > + const struct intel_bw_state *bw_state); > int intel_enable_sagv(struct drm_i915_private *dev_priv); > int intel_disable_sagv(struct drm_i915_private *dev_priv); > void intel_sagv_pre_plane_update(struct intel_atomic_state *state); > -- > 2.24.1.485.gad05a3d8e5
On Tue, May 12, 2020 at 02:39:25PM +0300, Ville Syrjälä wrote: > On Thu, May 07, 2020 at 05:45:00PM +0300, Stanislav Lisovskiy wrote: > > Seems that only skl needs to have SAGV turned off > > for multipipe scenarios, so lets do it this way. > > It doesn't afaics. It's just someone added the check for some random > reason. So this should be reworded a bit. Also this isn't just about > skl/derivatives but all pre-icl so the <subject> is a bit misleading too. This is in BSpec anyway. And it was in the code before, so I really don't get what do you mean here. > > > > > If anything blows up - we can always revert this patch. > > > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > > --- > > drivers/gpu/drm/i915/intel_pm.c | 15 +++++++++------ > > drivers/gpu/drm/i915/intel_pm.h | 3 ++- > > 2 files changed, 11 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > > index 3dc1ad66beb3..db188efee21e 100644 > > --- a/drivers/gpu/drm/i915/intel_pm.c > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > @@ -3777,7 +3777,7 @@ void intel_sagv_pre_plane_update(struct intel_atomic_state *state) > > if (!new_bw_state) > > return; > > > > - if (!intel_can_enable_sagv(new_bw_state)) > > + if (!intel_can_enable_sagv(dev_priv, new_bw_state)) > > intel_disable_sagv(dev_priv); > > } > > > > @@ -3800,7 +3800,7 @@ void intel_sagv_post_plane_update(struct intel_atomic_state *state) > > if (!new_bw_state) > > return; > > > > - if (intel_can_enable_sagv(new_bw_state)) > > + if (intel_can_enable_sagv(dev_priv, new_bw_state)) > > intel_enable_sagv(dev_priv); > > } > > > > @@ -3853,16 +3853,19 @@ static bool skl_crtc_can_enable_sagv(const struct intel_crtc_state *crtc_state) > > return true; > > } > > > > -bool intel_can_enable_sagv(const struct intel_bw_state *bw_state) > > +bool intel_can_enable_sagv(struct drm_i915_private *dev_priv, > > + const struct intel_bw_state *bw_state) > > { > > - if (bw_state->active_pipes && !is_power_of_2(bw_state->active_pipes)) > > - return false; > > + if (INTEL_GEN(dev_priv) < 11) > > + if (bw_state->active_pipes && !is_power_of_2(bw_state->active_pipes)) > > If (a && b && c) > return false; Then the line would get too long, and it does exactly same thing. I really don't understand such comments. Stan > > > > + return false; > > > > return bw_state->pipe_sagv_reject == 0; > > } > > > > static int intel_compute_sagv_mask(struct intel_atomic_state *state) > > { > > + struct drm_i915_private *dev_priv = to_i915(state->base.dev); > > int ret; > > struct intel_crtc *crtc; > > const struct intel_crtc_state *new_crtc_state; > > @@ -3896,7 +3899,7 @@ static int intel_compute_sagv_mask(struct intel_atomic_state *state) > > return ret; > > } > > > > - if (intel_can_enable_sagv(new_bw_state) != intel_can_enable_sagv(old_bw_state)) { > > + if (intel_can_enable_sagv(dev_priv, new_bw_state) != intel_can_enable_sagv(dev_priv, old_bw_state)) { > > > ret = intel_atomic_serialize_global_state(&new_bw_state->base); > > if (ret) > > return ret; > > diff --git a/drivers/gpu/drm/i915/intel_pm.h b/drivers/gpu/drm/i915/intel_pm.h > > index fd1dc422e6c5..614ac7f8d4cc 100644 > > --- a/drivers/gpu/drm/i915/intel_pm.h > > +++ b/drivers/gpu/drm/i915/intel_pm.h > > @@ -42,7 +42,8 @@ void skl_pipe_wm_get_hw_state(struct intel_crtc *crtc, > > struct skl_pipe_wm *out); > > void g4x_wm_sanitize(struct drm_i915_private *dev_priv); > > void vlv_wm_sanitize(struct drm_i915_private *dev_priv); > > -bool intel_can_enable_sagv(const struct intel_bw_state *bw_state); > > +bool intel_can_enable_sagv(struct drm_i915_private *dev_priv, > > + const struct intel_bw_state *bw_state); > > int intel_enable_sagv(struct drm_i915_private *dev_priv); > > int intel_disable_sagv(struct drm_i915_private *dev_priv); > > void intel_sagv_pre_plane_update(struct intel_atomic_state *state); > > -- > > 2.24.1.485.gad05a3d8e5 > > -- > Ville Syrjälä > Intel
On Tue, May 12, 2020 at 03:44:06PM +0300, Lisovskiy, Stanislav wrote: > On Tue, May 12, 2020 at 02:39:25PM +0300, Ville Syrjälä wrote: > > On Thu, May 07, 2020 at 05:45:00PM +0300, Stanislav Lisovskiy wrote: > > > Seems that only skl needs to have SAGV turned off > > > for multipipe scenarios, so lets do it this way. > > > > It doesn't afaics. It's just someone added the check for some random > > reason. So this should be reworded a bit. Also this isn't just about > > skl/derivatives but all pre-icl so the <subject> is a bit misleading too. > > This is in BSpec anyway. And it was in the code before, so I really > don't get what do you mean here. That's not what it says. It just suggests that if you guarantee that you always have enough ddb for sagv in single pipe configuration then you can just toggle sagv when transitioning between single vs. multi pipe configurations. The implication being that you don't guarantee enough ddb for sagv in multi pipe configurations, hence you simply assume sagv can't be used with multiple pipes. We don't even guarantee that out single pipe configuration has enough ddb for sagv, hence we have to actually check the block time constraint even in single pipe configurations. That makes the whole bspec paragraph moot and we might as well just do the obvious thing and check the sagv block time for all pipes (which we're already doing anyway). > > > > > > > > > If anything blows up - we can always revert this patch. > > > > > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > > > --- > > > drivers/gpu/drm/i915/intel_pm.c | 15 +++++++++------ > > > drivers/gpu/drm/i915/intel_pm.h | 3 ++- > > > 2 files changed, 11 insertions(+), 7 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > > > index 3dc1ad66beb3..db188efee21e 100644 > > > --- a/drivers/gpu/drm/i915/intel_pm.c > > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > > @@ -3777,7 +3777,7 @@ void intel_sagv_pre_plane_update(struct intel_atomic_state *state) > > > if (!new_bw_state) > > > return; > > > > > > - if (!intel_can_enable_sagv(new_bw_state)) > > > + if (!intel_can_enable_sagv(dev_priv, new_bw_state)) > > > intel_disable_sagv(dev_priv); > > > } > > > > > > @@ -3800,7 +3800,7 @@ void intel_sagv_post_plane_update(struct intel_atomic_state *state) > > > if (!new_bw_state) > > > return; > > > > > > - if (intel_can_enable_sagv(new_bw_state)) > > > + if (intel_can_enable_sagv(dev_priv, new_bw_state)) > > > intel_enable_sagv(dev_priv); > > > } > > > > > > @@ -3853,16 +3853,19 @@ static bool skl_crtc_can_enable_sagv(const struct intel_crtc_state *crtc_state) > > > return true; > > > } > > > > > > -bool intel_can_enable_sagv(const struct intel_bw_state *bw_state) > > > +bool intel_can_enable_sagv(struct drm_i915_private *dev_priv, > > > + const struct intel_bw_state *bw_state) > > > { > > > - if (bw_state->active_pipes && !is_power_of_2(bw_state->active_pipes)) > > > - return false; > > > + if (INTEL_GEN(dev_priv) < 11) > > > + if (bw_state->active_pipes && !is_power_of_2(bw_state->active_pipes)) > > > > If (a && b && c) > > return false; > > Then the line would get too long, and it does exactly same thing. > I really don't understand such comments. > > Stan > > > > > > > > + return false; > > > > > > return bw_state->pipe_sagv_reject == 0; > > > } > > > > > > static int intel_compute_sagv_mask(struct intel_atomic_state *state) > > > { > > > + struct drm_i915_private *dev_priv = to_i915(state->base.dev); > > > int ret; > > > struct intel_crtc *crtc; > > > const struct intel_crtc_state *new_crtc_state; > > > @@ -3896,7 +3899,7 @@ static int intel_compute_sagv_mask(struct intel_atomic_state *state) > > > return ret; > > > } > > > > > > - if (intel_can_enable_sagv(new_bw_state) != intel_can_enable_sagv(old_bw_state)) { > > > + if (intel_can_enable_sagv(dev_priv, new_bw_state) != intel_can_enable_sagv(dev_priv, old_bw_state)) { > > > > > ret = intel_atomic_serialize_global_state(&new_bw_state->base); > > > if (ret) > > > return ret; > > > diff --git a/drivers/gpu/drm/i915/intel_pm.h b/drivers/gpu/drm/i915/intel_pm.h > > > index fd1dc422e6c5..614ac7f8d4cc 100644 > > > --- a/drivers/gpu/drm/i915/intel_pm.h > > > +++ b/drivers/gpu/drm/i915/intel_pm.h > > > @@ -42,7 +42,8 @@ void skl_pipe_wm_get_hw_state(struct intel_crtc *crtc, > > > struct skl_pipe_wm *out); > > > void g4x_wm_sanitize(struct drm_i915_private *dev_priv); > > > void vlv_wm_sanitize(struct drm_i915_private *dev_priv); > > > -bool intel_can_enable_sagv(const struct intel_bw_state *bw_state); > > > +bool intel_can_enable_sagv(struct drm_i915_private *dev_priv, > > > + const struct intel_bw_state *bw_state); > > > int intel_enable_sagv(struct drm_i915_private *dev_priv); > > > int intel_disable_sagv(struct drm_i915_private *dev_priv); > > > void intel_sagv_pre_plane_update(struct intel_atomic_state *state); > > > -- > > > 2.24.1.485.gad05a3d8e5 > > > > -- > > Ville Syrjälä > > Intel
On Tue, May 12, 2020 at 03:44:06PM +0300, Lisovskiy, Stanislav wrote: > On Tue, May 12, 2020 at 02:39:25PM +0300, Ville Syrjälä wrote: > > On Thu, May 07, 2020 at 05:45:00PM +0300, Stanislav Lisovskiy wrote: > > > Seems that only skl needs to have SAGV turned off > > > for multipipe scenarios, so lets do it this way. > > > > It doesn't afaics. It's just someone added the check for some random > > reason. So this should be reworded a bit. Also this isn't just about > > skl/derivatives but all pre-icl so the <subject> is a bit misleading too. > > This is in BSpec anyway. And it was in the code before, so I really > don't get what do you mean here. > > > > > > > > > If anything blows up - we can always revert this patch. > > > > > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > > > --- > > > drivers/gpu/drm/i915/intel_pm.c | 15 +++++++++------ > > > drivers/gpu/drm/i915/intel_pm.h | 3 ++- > > > 2 files changed, 11 insertions(+), 7 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > > > index 3dc1ad66beb3..db188efee21e 100644 > > > --- a/drivers/gpu/drm/i915/intel_pm.c > > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > > @@ -3777,7 +3777,7 @@ void intel_sagv_pre_plane_update(struct intel_atomic_state *state) > > > if (!new_bw_state) > > > return; > > > > > > - if (!intel_can_enable_sagv(new_bw_state)) > > > + if (!intel_can_enable_sagv(dev_priv, new_bw_state)) > > > intel_disable_sagv(dev_priv); > > > } > > > > > > @@ -3800,7 +3800,7 @@ void intel_sagv_post_plane_update(struct intel_atomic_state *state) > > > if (!new_bw_state) > > > return; > > > > > > - if (intel_can_enable_sagv(new_bw_state)) > > > + if (intel_can_enable_sagv(dev_priv, new_bw_state)) > > > intel_enable_sagv(dev_priv); > > > } > > > > > > @@ -3853,16 +3853,19 @@ static bool skl_crtc_can_enable_sagv(const struct intel_crtc_state *crtc_state) > > > return true; > > > } > > > > > > -bool intel_can_enable_sagv(const struct intel_bw_state *bw_state) > > > +bool intel_can_enable_sagv(struct drm_i915_private *dev_priv, > > > + const struct intel_bw_state *bw_state) > > > { > > > - if (bw_state->active_pipes && !is_power_of_2(bw_state->active_pipes)) > > > - return false; > > > + if (INTEL_GEN(dev_priv) < 11) > > > + if (bw_state->active_pipes && !is_power_of_2(bw_state->active_pipes)) > > > > If (a && b && c) > > return false; > > Then the line would get too long, and it does exactly same thing. > I really don't understand such comments. if (a && b && c) if (a && b && c) if (a && b && c) there are plenty of options. The point is nested ifs like this only serve to indent code needlessly deep. > > Stan > > > > > > > > + return false; > > > > > > return bw_state->pipe_sagv_reject == 0; > > > } > > > > > > static int intel_compute_sagv_mask(struct intel_atomic_state *state) > > > { > > > + struct drm_i915_private *dev_priv = to_i915(state->base.dev); > > > int ret; > > > struct intel_crtc *crtc; > > > const struct intel_crtc_state *new_crtc_state; > > > @@ -3896,7 +3899,7 @@ static int intel_compute_sagv_mask(struct intel_atomic_state *state) > > > return ret; > > > } > > > > > > - if (intel_can_enable_sagv(new_bw_state) != intel_can_enable_sagv(old_bw_state)) { > > > + if (intel_can_enable_sagv(dev_priv, new_bw_state) != intel_can_enable_sagv(dev_priv, old_bw_state)) { > > > > > ret = intel_atomic_serialize_global_state(&new_bw_state->base); > > > if (ret) > > > return ret; > > > diff --git a/drivers/gpu/drm/i915/intel_pm.h b/drivers/gpu/drm/i915/intel_pm.h > > > index fd1dc422e6c5..614ac7f8d4cc 100644 > > > --- a/drivers/gpu/drm/i915/intel_pm.h > > > +++ b/drivers/gpu/drm/i915/intel_pm.h > > > @@ -42,7 +42,8 @@ void skl_pipe_wm_get_hw_state(struct intel_crtc *crtc, > > > struct skl_pipe_wm *out); > > > void g4x_wm_sanitize(struct drm_i915_private *dev_priv); > > > void vlv_wm_sanitize(struct drm_i915_private *dev_priv); > > > -bool intel_can_enable_sagv(const struct intel_bw_state *bw_state); > > > +bool intel_can_enable_sagv(struct drm_i915_private *dev_priv, > > > + const struct intel_bw_state *bw_state); > > > int intel_enable_sagv(struct drm_i915_private *dev_priv); > > > int intel_disable_sagv(struct drm_i915_private *dev_priv); > > > void intel_sagv_pre_plane_update(struct intel_atomic_state *state); > > > -- > > > 2.24.1.485.gad05a3d8e5 > > > > -- > > Ville Syrjälä > > Intel
On Tue, May 12, 2020 at 04:14:33PM +0300, Ville Syrjälä wrote: > On Tue, May 12, 2020 at 03:44:06PM +0300, Lisovskiy, Stanislav wrote: > > On Tue, May 12, 2020 at 02:39:25PM +0300, Ville Syrjälä wrote: > > > On Thu, May 07, 2020 at 05:45:00PM +0300, Stanislav Lisovskiy wrote: > > > > Seems that only skl needs to have SAGV turned off > > > > for multipipe scenarios, so lets do it this way. > > > > > > It doesn't afaics. It's just someone added the check for some random > > > reason. So this should be reworded a bit. Also this isn't just about > > > skl/derivatives but all pre-icl so the <subject> is a bit misleading too. > > > > This is in BSpec anyway. And it was in the code before, so I really > > don't get what do you mean here. > > > > > > > > > > > > > If anything blows up - we can always revert this patch. > > > > > > > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > > > > --- > > > > drivers/gpu/drm/i915/intel_pm.c | 15 +++++++++------ > > > > drivers/gpu/drm/i915/intel_pm.h | 3 ++- > > > > 2 files changed, 11 insertions(+), 7 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > > > > index 3dc1ad66beb3..db188efee21e 100644 > > > > --- a/drivers/gpu/drm/i915/intel_pm.c > > > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > > > @@ -3777,7 +3777,7 @@ void intel_sagv_pre_plane_update(struct intel_atomic_state *state) > > > > if (!new_bw_state) > > > > return; > > > > > > > > - if (!intel_can_enable_sagv(new_bw_state)) > > > > + if (!intel_can_enable_sagv(dev_priv, new_bw_state)) > > > > intel_disable_sagv(dev_priv); > > > > } > > > > > > > > @@ -3800,7 +3800,7 @@ void intel_sagv_post_plane_update(struct intel_atomic_state *state) > > > > if (!new_bw_state) > > > > return; > > > > > > > > - if (intel_can_enable_sagv(new_bw_state)) > > > > + if (intel_can_enable_sagv(dev_priv, new_bw_state)) > > > > intel_enable_sagv(dev_priv); > > > > } > > > > > > > > @@ -3853,16 +3853,19 @@ static bool skl_crtc_can_enable_sagv(const struct intel_crtc_state *crtc_state) > > > > return true; > > > > } > > > > > > > > -bool intel_can_enable_sagv(const struct intel_bw_state *bw_state) > > > > +bool intel_can_enable_sagv(struct drm_i915_private *dev_priv, > > > > + const struct intel_bw_state *bw_state) > > > > { > > > > - if (bw_state->active_pipes && !is_power_of_2(bw_state->active_pipes)) > > > > - return false; > > > > + if (INTEL_GEN(dev_priv) < 11) > > > > + if (bw_state->active_pipes && !is_power_of_2(bw_state->active_pipes)) > > > > > > If (a && b && c) > > > return false; > > > > Then the line would get too long, and it does exactly same thing. > > I really don't understand such comments. > > if (a && b && > c) > > if (a && > b && c) > > if (a && > b && > c) > > there are plenty of options. The point is nested ifs like this > only serve to indent code needlessly deep. and ifs like if (long condition1 && long condition2 && ...) make unnecessary "wide". I would understand of course if I would do something like 3-4 nested ifs sure, however that one seems to be completely similar. I don't even get why if (a && b && c) reads better than if (a) if(b && c) Stan > > > > > > Stan > > > > > > > > > > > > + return false; > > > > > > > > return bw_state->pipe_sagv_reject == 0; > > > > } > > > > > > > > static int intel_compute_sagv_mask(struct intel_atomic_state *state) > > > > { > > > > + struct drm_i915_private *dev_priv = to_i915(state->base.dev); > > > > int ret; > > > > struct intel_crtc *crtc; > > > > const struct intel_crtc_state *new_crtc_state; > > > > @@ -3896,7 +3899,7 @@ static int intel_compute_sagv_mask(struct intel_atomic_state *state) > > > > return ret; > > > > } > > > > > > > > - if (intel_can_enable_sagv(new_bw_state) != intel_can_enable_sagv(old_bw_state)) { > > > > + if (intel_can_enable_sagv(dev_priv, new_bw_state) != intel_can_enable_sagv(dev_priv, old_bw_state)) { > > > > > > > ret = intel_atomic_serialize_global_state(&new_bw_state->base); > > > > if (ret) > > > > return ret; > > > > diff --git a/drivers/gpu/drm/i915/intel_pm.h b/drivers/gpu/drm/i915/intel_pm.h > > > > index fd1dc422e6c5..614ac7f8d4cc 100644 > > > > --- a/drivers/gpu/drm/i915/intel_pm.h > > > > +++ b/drivers/gpu/drm/i915/intel_pm.h > > > > @@ -42,7 +42,8 @@ void skl_pipe_wm_get_hw_state(struct intel_crtc *crtc, > > > > struct skl_pipe_wm *out); > > > > void g4x_wm_sanitize(struct drm_i915_private *dev_priv); > > > > void vlv_wm_sanitize(struct drm_i915_private *dev_priv); > > > > -bool intel_can_enable_sagv(const struct intel_bw_state *bw_state); > > > > +bool intel_can_enable_sagv(struct drm_i915_private *dev_priv, > > > > + const struct intel_bw_state *bw_state); > > > > int intel_enable_sagv(struct drm_i915_private *dev_priv); > > > > int intel_disable_sagv(struct drm_i915_private *dev_priv); > > > > void intel_sagv_pre_plane_update(struct intel_atomic_state *state); > > > > -- > > > > 2.24.1.485.gad05a3d8e5 > > > > > > -- > > > Ville Syrjälä > > > Intel > > -- > Ville Syrjälä > Intel
On Tue, May 12, 2020 at 04:26:53PM +0300, Lisovskiy, Stanislav wrote: > On Tue, May 12, 2020 at 04:14:33PM +0300, Ville Syrjälä wrote: > > On Tue, May 12, 2020 at 03:44:06PM +0300, Lisovskiy, Stanislav wrote: > > > On Tue, May 12, 2020 at 02:39:25PM +0300, Ville Syrjälä wrote: > > > > On Thu, May 07, 2020 at 05:45:00PM +0300, Stanislav Lisovskiy wrote: > > > > > Seems that only skl needs to have SAGV turned off > > > > > for multipipe scenarios, so lets do it this way. > > > > > > > > It doesn't afaics. It's just someone added the check for some random > > > > reason. So this should be reworded a bit. Also this isn't just about > > > > skl/derivatives but all pre-icl so the <subject> is a bit misleading too. > > > > > > This is in BSpec anyway. And it was in the code before, so I really > > > don't get what do you mean here. > > > > > > > > > > > > > > > > > If anything blows up - we can always revert this patch. > > > > > > > > > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > > > > > --- > > > > > drivers/gpu/drm/i915/intel_pm.c | 15 +++++++++------ > > > > > drivers/gpu/drm/i915/intel_pm.h | 3 ++- > > > > > 2 files changed, 11 insertions(+), 7 deletions(-) > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > > > > > index 3dc1ad66beb3..db188efee21e 100644 > > > > > --- a/drivers/gpu/drm/i915/intel_pm.c > > > > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > > > > @@ -3777,7 +3777,7 @@ void intel_sagv_pre_plane_update(struct intel_atomic_state *state) > > > > > if (!new_bw_state) > > > > > return; > > > > > > > > > > - if (!intel_can_enable_sagv(new_bw_state)) > > > > > + if (!intel_can_enable_sagv(dev_priv, new_bw_state)) > > > > > intel_disable_sagv(dev_priv); > > > > > } > > > > > > > > > > @@ -3800,7 +3800,7 @@ void intel_sagv_post_plane_update(struct intel_atomic_state *state) > > > > > if (!new_bw_state) > > > > > return; > > > > > > > > > > - if (intel_can_enable_sagv(new_bw_state)) > > > > > + if (intel_can_enable_sagv(dev_priv, new_bw_state)) > > > > > intel_enable_sagv(dev_priv); > > > > > } > > > > > > > > > > @@ -3853,16 +3853,19 @@ static bool skl_crtc_can_enable_sagv(const struct intel_crtc_state *crtc_state) > > > > > return true; > > > > > } > > > > > > > > > > -bool intel_can_enable_sagv(const struct intel_bw_state *bw_state) > > > > > +bool intel_can_enable_sagv(struct drm_i915_private *dev_priv, > > > > > + const struct intel_bw_state *bw_state) > > > > > { > > > > > - if (bw_state->active_pipes && !is_power_of_2(bw_state->active_pipes)) > > > > > - return false; > > > > > + if (INTEL_GEN(dev_priv) < 11) > > > > > + if (bw_state->active_pipes && !is_power_of_2(bw_state->active_pipes)) > > > > > > > > If (a && b && c) > > > > return false; > > > > > > Then the line would get too long, and it does exactly same thing. > > > I really don't understand such comments. > > > > if (a && b && > > c) > > > > if (a && > > b && c) > > > > if (a && > > b && > > c) > > > > there are plenty of options. The point is nested ifs like this > > only serve to indent code needlessly deep. > > and ifs like if (long condition1 && long condition2 && ...) make > unnecessary "wide". > > I would understand of course if I would do something like > 3-4 nested ifs sure, however that one seems to be completely similar. > > I don't even get why > > if (a && > b && c) "if a and b and c then do stuff" > > reads better than > > if (a) > if(b && c) "if a then if b and c then do stuff" The first one definitely sounds better to my ears. Not sure the second one can even be called English.
On Tue, May 12, 2020 at 06:32:38PM +0300, Ville Syrjälä wrote: > On Tue, May 12, 2020 at 04:26:53PM +0300, Lisovskiy, Stanislav wrote: > > On Tue, May 12, 2020 at 04:14:33PM +0300, Ville Syrjälä wrote: > > > On Tue, May 12, 2020 at 03:44:06PM +0300, Lisovskiy, Stanislav wrote: > > > > On Tue, May 12, 2020 at 02:39:25PM +0300, Ville Syrjälä wrote: > > > > > On Thu, May 07, 2020 at 05:45:00PM +0300, Stanislav Lisovskiy wrote: > > > > > > Seems that only skl needs to have SAGV turned off > > > > > > for multipipe scenarios, so lets do it this way. > > > > > > > > > > It doesn't afaics. It's just someone added the check for some random > > > > > reason. So this should be reworded a bit. Also this isn't just about > > > > > skl/derivatives but all pre-icl so the <subject> is a bit misleading too. > > > > > > > > This is in BSpec anyway. And it was in the code before, so I really > > > > don't get what do you mean here. > > > > > > > > > > > > > > > > > > > > > If anything blows up - we can always revert this patch. > > > > > > > > > > > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > > > > > > --- > > > > > > drivers/gpu/drm/i915/intel_pm.c | 15 +++++++++------ > > > > > > drivers/gpu/drm/i915/intel_pm.h | 3 ++- > > > > > > 2 files changed, 11 insertions(+), 7 deletions(-) > > > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > > > > > > index 3dc1ad66beb3..db188efee21e 100644 > > > > > > --- a/drivers/gpu/drm/i915/intel_pm.c > > > > > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > > > > > @@ -3777,7 +3777,7 @@ void intel_sagv_pre_plane_update(struct intel_atomic_state *state) > > > > > > if (!new_bw_state) > > > > > > return; > > > > > > > > > > > > - if (!intel_can_enable_sagv(new_bw_state)) > > > > > > + if (!intel_can_enable_sagv(dev_priv, new_bw_state)) > > > > > > intel_disable_sagv(dev_priv); > > > > > > } > > > > > > > > > > > > @@ -3800,7 +3800,7 @@ void intel_sagv_post_plane_update(struct intel_atomic_state *state) > > > > > > if (!new_bw_state) > > > > > > return; > > > > > > > > > > > > - if (intel_can_enable_sagv(new_bw_state)) > > > > > > + if (intel_can_enable_sagv(dev_priv, new_bw_state)) > > > > > > intel_enable_sagv(dev_priv); > > > > > > } > > > > > > > > > > > > @@ -3853,16 +3853,19 @@ static bool skl_crtc_can_enable_sagv(const struct intel_crtc_state *crtc_state) > > > > > > return true; > > > > > > } > > > > > > > > > > > > -bool intel_can_enable_sagv(const struct intel_bw_state *bw_state) > > > > > > +bool intel_can_enable_sagv(struct drm_i915_private *dev_priv, > > > > > > + const struct intel_bw_state *bw_state) > > > > > > { > > > > > > - if (bw_state->active_pipes && !is_power_of_2(bw_state->active_pipes)) > > > > > > - return false; > > > > > > + if (INTEL_GEN(dev_priv) < 11) > > > > > > + if (bw_state->active_pipes && !is_power_of_2(bw_state->active_pipes)) > > > > > > > > > > If (a && b && c) > > > > > return false; > > > > > > > > Then the line would get too long, and it does exactly same thing. > > > > I really don't understand such comments. > > > > > > if (a && b && > > > c) > > > > > > if (a && > > > b && c) > > > > > > if (a && > > > b && > > > c) > > > > > > there are plenty of options. The point is nested ifs like this > > > only serve to indent code needlessly deep. > > > > and ifs like if (long condition1 && long condition2 && ...) make > > unnecessary "wide". > > > > I would understand of course if I would do something like > > 3-4 nested ifs sure, however that one seems to be completely similar. > > > > I don't even get why > > > > if (a && > > b && c) > > "if a and b and c then do stuff" > > > > > reads better than > > > > if (a) > > if(b && c) > > "if a then if b and c then do stuff" > > The first one definitely sounds better to my ears. Not sure > the second one can even be called English. It is just a game of words here. You can also spell logical expression like a && b && c, as "if a evaluates to True then if b evaluates to true then if c evaluates to true, then do stuff". Those expressions most likely produce same assembly even, so basically you can spell both same way either. So this arguing is honestly all about your personal matter of taste. Which is fine I mean, everyone can write code according to own preference unless it violates some _well known_ and _formal_ conventions. Like of course I do realize that doing something like: if (a) if(b) if(c) ... is stupid, however that would be exaggeration to say that I'm doing something like this here. For example I really don't like long lines in conditions like (if verylongstuff && verylongstuff2 && ..) neither those hanging && like if (somestuff && somestuff2 || somestuff3) to me the latter is way more horrible than if (somestuff) if (somestuff2 || somestuff3) which looks much more clear to me. Again imho.. Stan > > -- > Ville Syrjälä > Intel
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 3dc1ad66beb3..db188efee21e 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -3777,7 +3777,7 @@ void intel_sagv_pre_plane_update(struct intel_atomic_state *state) if (!new_bw_state) return; - if (!intel_can_enable_sagv(new_bw_state)) + if (!intel_can_enable_sagv(dev_priv, new_bw_state)) intel_disable_sagv(dev_priv); } @@ -3800,7 +3800,7 @@ void intel_sagv_post_plane_update(struct intel_atomic_state *state) if (!new_bw_state) return; - if (intel_can_enable_sagv(new_bw_state)) + if (intel_can_enable_sagv(dev_priv, new_bw_state)) intel_enable_sagv(dev_priv); } @@ -3853,16 +3853,19 @@ static bool skl_crtc_can_enable_sagv(const struct intel_crtc_state *crtc_state) return true; } -bool intel_can_enable_sagv(const struct intel_bw_state *bw_state) +bool intel_can_enable_sagv(struct drm_i915_private *dev_priv, + const struct intel_bw_state *bw_state) { - if (bw_state->active_pipes && !is_power_of_2(bw_state->active_pipes)) - return false; + if (INTEL_GEN(dev_priv) < 11) + if (bw_state->active_pipes && !is_power_of_2(bw_state->active_pipes)) + return false; return bw_state->pipe_sagv_reject == 0; } static int intel_compute_sagv_mask(struct intel_atomic_state *state) { + struct drm_i915_private *dev_priv = to_i915(state->base.dev); int ret; struct intel_crtc *crtc; const struct intel_crtc_state *new_crtc_state; @@ -3896,7 +3899,7 @@ static int intel_compute_sagv_mask(struct intel_atomic_state *state) return ret; } - if (intel_can_enable_sagv(new_bw_state) != intel_can_enable_sagv(old_bw_state)) { + if (intel_can_enable_sagv(dev_priv, new_bw_state) != intel_can_enable_sagv(dev_priv, old_bw_state)) { ret = intel_atomic_serialize_global_state(&new_bw_state->base); if (ret) return ret; diff --git a/drivers/gpu/drm/i915/intel_pm.h b/drivers/gpu/drm/i915/intel_pm.h index fd1dc422e6c5..614ac7f8d4cc 100644 --- a/drivers/gpu/drm/i915/intel_pm.h +++ b/drivers/gpu/drm/i915/intel_pm.h @@ -42,7 +42,8 @@ void skl_pipe_wm_get_hw_state(struct intel_crtc *crtc, struct skl_pipe_wm *out); void g4x_wm_sanitize(struct drm_i915_private *dev_priv); void vlv_wm_sanitize(struct drm_i915_private *dev_priv); -bool intel_can_enable_sagv(const struct intel_bw_state *bw_state); +bool intel_can_enable_sagv(struct drm_i915_private *dev_priv, + const struct intel_bw_state *bw_state); int intel_enable_sagv(struct drm_i915_private *dev_priv); int intel_disable_sagv(struct drm_i915_private *dev_priv); void intel_sagv_pre_plane_update(struct intel_atomic_state *state);
Seems that only skl needs to have SAGV turned off for multipipe scenarios, so lets do it this way. If anything blows up - we can always revert this patch. Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> --- drivers/gpu/drm/i915/intel_pm.c | 15 +++++++++------ drivers/gpu/drm/i915/intel_pm.h | 3 ++- 2 files changed, 11 insertions(+), 7 deletions(-)