[Intel-gfx] [PATCH 4/4] drm/i915: reorganize the get_cdclk assignment
Ville Syrjälä
ville.syrjala at linux.intel.com
Fri Feb 17 15:28:58 UTC 2017
On Fri, Feb 17, 2017 at 01:17:22PM -0200, Paulo Zanoni wrote:
> Em Sex, 2017-02-17 às 16:05 +0200, Ville Syrjälä escreveu:
> > On Fri, Feb 17, 2017 at 11:22:07AM -0200, Paulo Zanoni wrote:
> > >
> > > Possible problems of the current if-ladder:
> > > - It's a huge if ladder with almost a different check for each of
> > > our platforms.
> > > - It mixes 3 different types of checks: IS_GENX, IS_PLATFORM and
> > > IS_GROUP_OF_PLATFORMS.
> > > - As demonstrated by the recent IS_G4X commit, it's not easy to
> > > be
> > > sure if a platform down on the list isn't also checked earlier.
> > > - As demonstrated by the WARN at the end, it's not easy to be
> > > sure
> > > if we're actually checking for every single platform.
> > >
> > > Possible advantages of the new switch statement:
> > > - It may be easier for the compiler to optimize stuff (I didn't
> > > check this), especially since the values are labels of an enum.
> > > - The compiler will tell us in case we miss some platform.
> > > - All platforms are explicitly there instead of maybe hidden in
> > > some
> > > check for a certain group of platforms such as IS_GEN9_BC.
> >
> > Performance is a bit of a moot point since this is run exaclty once,
> > but
> > the IS_GEN9_BC() stuff I tend to agree with. I don't really like
> > those
> > macros at all since they don't actully mean anything as far as the
> > hardware features go.
>
> I think they make some sense when they're a single check. But when we
> have tons of checks for tons of platforms, I don't know.
The problem problem is that they basically mean different things in
different parts of the driver. So you anyway have to mentally expand
the list out to figure out what's really going on.
>
> >
> > >
> > >
> > > Possible disadvantages with the new code:
> > > - A few lines bigger.
> > >
> > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni at intel.com>
> > > ---
> > > drivers/gpu/drm/i915/intel_cdclk.c | 103 ++++++++++++++++++++++---
> > > ------------
> > > 1 file changed, 62 insertions(+), 41 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/intel_cdclk.c
> > > b/drivers/gpu/drm/i915/intel_cdclk.c
> > > index 7c92dc7..58a2f5c 100644
> > > --- a/drivers/gpu/drm/i915/intel_cdclk.c
> > > +++ b/drivers/gpu/drm/i915/intel_cdclk.c
> > > @@ -1789,49 +1789,70 @@ void intel_init_cdclk_hooks(struct
> > > drm_i915_private *dev_priv)
> > > dev_priv->display.modeset_calc_cdclk =
> > > skl_modeset_calc_cdclk;
> > > }
> > >
> > > - if (IS_GEN9_BC(dev_priv))
> > > - dev_priv->display.get_cdclk = skl_get_cdclk;
> > > - else if (IS_GEN9_LP(dev_priv))
> > > - dev_priv->display.get_cdclk = bxt_get_cdclk;
> > > - else if (IS_BROADWELL(dev_priv))
> > > - dev_priv->display.get_cdclk = bdw_get_cdclk;
> > > - else if (IS_HASWELL(dev_priv))
> > > - dev_priv->display.get_cdclk = hsw_get_cdclk;
> > > - else if (IS_VALLEYVIEW(dev_priv) ||
> > > IS_CHERRYVIEW(dev_priv))
> > > - dev_priv->display.get_cdclk = vlv_get_cdclk;
> > > - else if (IS_GEN6(dev_priv) || IS_IVYBRIDGE(dev_priv))
> > > + switch (INTEL_INFO(dev_priv)->platform) {
> > > + case INTEL_PLATFORM_UNINITIALIZED:
> >
> > Just default: ?
>
> If we add a default case the compiler will stop complaining in case we
> don't explicitly list every platform. It's a trade-off, I really think
> the current way is slightly better, but I won't oppose in case you
> still think it's better adding the default case.
Nah. Getting the compiler to do the work for us seems like
a decent idea.
>
>
> >
> > >
> > > + MISSING_CASE(INTEL_INFO(dev_priv)->platform);
> > > + /* Fall through. */
> > > + case INTEL_I830:
> > > + dev_priv->display.get_cdclk =
> > > fixed_133mhz_get_cdclk;
> > > + break;
> > > + case INTEL_I845G:
> > > + dev_priv->display.get_cdclk =
> > > fixed_200mhz_get_cdclk;
> > > + break;
> > > + case INTEL_I85X:
> > > + dev_priv->display.get_cdclk = i85x_get_cdclk;
> > > + break;
> > > + case INTEL_I865G:
> > > + dev_priv->display.get_cdclk =
> > > fixed_266mhz_get_cdclk;
> > > + break;
> > > + case INTEL_I915G:
> > > + dev_priv->display.get_cdclk =
> > > fixed_333mhz_get_cdclk;
> > > + break;
> > > + case INTEL_I915GM:
> > > + dev_priv->display.get_cdclk = i915gm_get_cdclk;
> > > + break;
> > > + case INTEL_I945G:
> > > + case INTEL_I965G:
> > > + case INTEL_SANDYBRIDGE:
> > > + case INTEL_IVYBRIDGE:
> >
> > I don't particularly like this disorder. I just managed to get the
> > list into some sort of sane order recently.
>
> My original thought here was that since the compiler will actually
> complain in case we miss some platform, keeping a strict order is not
> as meaningful as it was before. But I was also wondering if this was
> actually better or not, so I can change this.
And unsorted list is quite hard to verify visually for correctness. We
might have a function pointer assigned for each platform, but if you
actually want to verify that it's the correct one in each case then
going through the list is IMO much easier when it's in a decent order.
>
> But I did notice you sorted the list. In fact, I originally wrote this
> commit against a tree without your improvements, so one of the reasons
> I cited in the commit message was the mess of an ordering we had at
> that time :).
>
> >
> > >
> > > dev_priv->display.get_cdclk =
> > > fixed_400mhz_get_cdclk;
> > > - else if (IS_GEN5(dev_priv))
> > > - dev_priv->display.get_cdclk =
> > > fixed_450mhz_get_cdclk;
> > > - else if (IS_GM45(dev_priv))
> > > - dev_priv->display.get_cdclk = gm45_get_cdclk;
> > > - else if (IS_G45(dev_priv))
> > > + break;
> > > + case INTEL_I945GM:
> > > + dev_priv->display.get_cdclk = i945gm_get_cdclk;
> > > + break;
> > > + case INTEL_G33:
> > > + case INTEL_G45:
> >
> > More disorder.
> >
> > >
> > > dev_priv->display.get_cdclk = g33_get_cdclk;
> > > - else if (IS_I965GM(dev_priv))
> > > - dev_priv->display.get_cdclk = i965gm_get_cdclk;
> > > - else if (IS_I965G(dev_priv))
> > > - dev_priv->display.get_cdclk =
> > > fixed_400mhz_get_cdclk;
> > > - else if (IS_PINEVIEW(dev_priv))
> > > + break;
> > > + case INTEL_PINEVIEW:
> > > dev_priv->display.get_cdclk = pnv_get_cdclk;
> > > - else if (IS_G33(dev_priv))
> > > - dev_priv->display.get_cdclk = g33_get_cdclk;
> > > - else if (IS_I945GM(dev_priv))
> > > - dev_priv->display.get_cdclk = i945gm_get_cdclk;
> > > - else if (IS_I945G(dev_priv))
> > > - dev_priv->display.get_cdclk =
> > > fixed_400mhz_get_cdclk;
> > > - else if (IS_I915GM(dev_priv))
> > > - dev_priv->display.get_cdclk = i915gm_get_cdclk;
> > > - else if (IS_I915G(dev_priv))
> > > - dev_priv->display.get_cdclk =
> > > fixed_333mhz_get_cdclk;
> > > - else if (IS_I865G(dev_priv))
> > > - dev_priv->display.get_cdclk =
> > > fixed_266mhz_get_cdclk;
> > > - else if (IS_I85X(dev_priv))
> > > - dev_priv->display.get_cdclk = i85x_get_cdclk;
> > > - else if (IS_I845G(dev_priv))
> > > - dev_priv->display.get_cdclk =
> > > fixed_200mhz_get_cdclk;
> > > - else { /* 830 */
> > > - WARN(!IS_I830(dev_priv),
> > > - "Unknown platform. Assuming 133 MHz
> > > CDCLK\n");
> > > - dev_priv->display.get_cdclk =
> > > fixed_133mhz_get_cdclk;
> > > + break;
> > > + case INTEL_I965GM:
> > > + dev_priv->display.get_cdclk = i965gm_get_cdclk;
> > > + break;
> > > + case INTEL_GM45:
> > > + dev_priv->display.get_cdclk = gm45_get_cdclk;
> > > + break;
> > > + case INTEL_IRONLAKE:
> > > + dev_priv->display.get_cdclk =
> > > fixed_450mhz_get_cdclk;
> > > + break;
> > > + case INTEL_VALLEYVIEW:
> > > + case INTEL_CHERRYVIEW:
> > > + dev_priv->display.get_cdclk = vlv_get_cdclk;
> > > + break;
> > > + case INTEL_HASWELL:
> > > + dev_priv->display.get_cdclk = hsw_get_cdclk;
> > > + break;
> > > + case INTEL_BROADWELL:
> > > + dev_priv->display.get_cdclk = bdw_get_cdclk;
> > > + break;
> > > + case INTEL_SKYLAKE:
> > > + case INTEL_KABYLAKE:
> > > + dev_priv->display.get_cdclk = skl_get_cdclk;
> > > + break;
> > > + case INTEL_BROXTON:
> > > + case INTEL_GEMINILAKE:
> > > + dev_priv->display.get_cdclk = bxt_get_cdclk;
> > > + break;
> > > }
> > > }
> > > --
> > > 2.7.4
> > >
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx at lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >
--
Ville Syrjälä
Intel OTC
More information about the Intel-gfx
mailing list