[Intel-gfx] [PATCH 1/2] drm/i915/display: Group DC9 mask set

[Souza, Jose]

On Wed, 2020-11-11 at 13:12 -0800, Lucas De Marchi wrote:
> On Wed, Nov 11, 2020 at 08:24:07AM -0800, Jose Souza wrote:
> > DC9 has a separate HW flow from the rest of the DC states and it is
> > available in GEN9 LP platforms and on GEN11 and newer, so here
> > moving the assignment of the mask to a single conditional block to
> > simplifly code.
> > 
> > Cc: Lucas De Marchi <lucas.demarchi at intel.com>
> > Cc: Anusha Srivatsa <anusha.srivatsa at intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza at intel.com>
> > ---
> > .../gpu/drm/i915/display/intel_display_power.c | 18 ++++++++----------
> > 1 file changed, 8 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c b/drivers/gpu/drm/i915/display/intel_display_power.c
> > index 689922480661..48d41a43fbb2 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display_power.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display_power.c
> > @@ -4497,26 +4497,24 @@ static u32 get_allowed_dc_mask(const struct drm_i915_private *dev_priv,
> > 			max_dc = 3;
> > 		else
> > 			max_dc = 4;
> > -		/*
> > -		 * DC9 has a separate HW flow from the rest of the DC states,
> > -		 * not depending on the DMC firmware. It's needed by system
> > -		 * suspend/resume, so allow it unconditionally.
> > -		 */
> > -		mask = DC_STATE_EN_DC9;
> > 	} else if (IS_GEN(dev_priv, 11)) {
> > 		max_dc = 2;
> > -		mask = DC_STATE_EN_DC9;
> > 	} else if (IS_GEN(dev_priv, 10) || IS_GEN9_BC(dev_priv)) {
> > 		max_dc = 2;
> > -		mask = 0;
> > 	} else if (IS_GEN9_LP(dev_priv)) {
> > 		max_dc = 1;
> > -		mask = DC_STATE_EN_DC9;
> > 	} else {
> > 		max_dc = 0;
> > -		mask = 0;
> > 	}
> > 
> > +	/*
> > +	 * DC9 has a separate HW flow from the rest of the DC states,
> > +	 * not depending on the DMC firmware. It's needed by system
> > +	 * suspend/resume, so allow it unconditionally.
> > +	 */
> > +	mask = IS_GEN9_LP(dev_priv) || INTEL_GEN(dev_priv) >= 11 ?
> > +	       DC_STATE_EN_DC9 : 0;
> 
> humn... these 2 conditions here in a ternary operator is something that
> will probably get even harder to read if we have to add more conditions.
> Maybe just move the default value to the declaration (mask = 0) and here
> you do:
> 
> if (IS_GEN9_LP(dev_priv) || INTEL_GEN(dev_priv) >= 11)
> 	mask = DC_STATE_EN_DC9;
> 
> ?
> 
> Up to you. Change looks correct

Will keep the way this patch is as I believe that all new platforms will support DC9 so this block will not change but if it happens we should do like
you suggested.

> 
> 
> Reviewed-by: Lucas De Marchi <lucas.demarchi at intel.com>

thanks

> 
> Lucas De Marchi
> 
> > +
> > 	if (!dev_priv->params.disable_power_well)
> > 		max_dc = 0;
> > 
> > -- 
> > 2.29.2
> >