Re: [Intel-gfx] [PATCH v28 3/6] drm/i915: Make active_pipes check skl specific
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 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
> > > > > > ---
> > > > > > 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
Re: [Intel-gfx] [PATCH v28 3/6] drm/i915: Make active_pipes check skl specific
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 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
> > > > > ---
> > > > > 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.
--
Ville Syrjälä
Intel
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v28 3/6] drm/i915: Make active_pipes check skl specific
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 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
> > > > ---
> > > > 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(_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
Re: [Intel-gfx] [PATCH v28 3/6] drm/i915: Make active_pipes check skl specific
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 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
> > > ---
> > > 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(_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
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v28 3/6] drm/i915: Make active_pipes check skl specific
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 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
> > > ---
> > > 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(_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
Re: [Intel-gfx] [PATCH v28 3/6] drm/i915: Make active_pipes check skl specific
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 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
> > ---
> > 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(_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
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v28 3/6] drm/i915: Make active_pipes check skl specific
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 is a bit misleading too.
>
> If anything blows up - we can always revert this patch.
>
> Signed-off-by: Stanislav Lisovskiy
> ---
> 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(_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
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH v28 3/6] drm/i915: Make active_pipes check skl specific
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
---
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))
+ 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(_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
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
