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@xxxxxxxxx>
> > Cc: Anusha Srivatsa <anusha.srivatsa@xxxxxxxxx>
> > Signed-off-by: José Roberto de Souza <jose.souza@xxxxxxxxx>
> > ---
> > .../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@xxxxxxxxx>
thanks
>
> Lucas De Marchi
>
> > +
> > if (!dev_priv->params.disable_power_well)
> > max_dc = 0;
> >
> > --
> > 2.29.2
> >
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
