[Intel-gfx] [PATCH v28 3/6] drm/i915: Make active_pipes check skl specific

[Lisovskiy, Stanislav]

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 at 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