On Tue, 2016-12-20 at 12:08 +0200, Jani Nikula wrote:
> On Tue, 20 Dec 2016, Ander Conselvan De Oliveira <conselvan2@xxxxxxxxx> wrote:
> >
> > On Mon, 2016-12-19 at 19:35 +0200, Jani Nikula wrote:
> > >
> > > On Mon, 19 Dec 2016, ville.syrjala@xxxxxxxxxxxxxxx wrote:
> > > >
> > > >
> > > > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > > >
> > > > Let's clean up the mess we have in the if ladder that assigns the
> > > > .get_cdclk() hooks. The grouping of the platforms by the function
> > > > results in a thing that's not really legible, so let's do it the
> > > > other way around and order the if ladder by platform and duplicate
> > > > whatever assignments we need.
> > > >
> > > > To further avoid confusion with the function names let's rename
> > > > them to just fixed_<freq>_get_cdclk(). The other option would
> > > > be to duplicate the functions entirely but it seems quite
> > > > pointless to do that since each one just returns a fixed value.
> > > >
> > > > Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > > > ---
> > > > drivers/gpu/drm/i915/intel_display.c | 41 +++++++++++++++++++++------
> > > > ----
> > > > -----
> > > > 1 file changed, 24 insertions(+), 17 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > > > b/drivers/gpu/drm/i915/intel_display.c
> > > > index bbfef348783b..29f91e799272 100644
> > > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > > @@ -7379,22 +7379,22 @@ static int valleyview_get_cdclk(struct
> > > > drm_i915_private *dev_priv)
> > > > CCK_DISPLAY_CLOCK_CONTROL);
> > > > }
> > > >
> > > > -static int ilk_get_cdclk(struct drm_i915_private *dev_priv)
> > > > +static int fixed_450mhz_get_cdclk(struct drm_i915_private *dev_priv)
> > > > {
> > > > return 450000;
> > > > }
> > > >
> > > > -static int i945_get_cdclk(struct drm_i915_private *dev_priv)
> > > > +static int fixed_400mhz_get_cdclk(struct drm_i915_private *dev_priv)
> > > > {
> > > > return 400000;
> > > > }
> > > >
> > > > -static int i915_get_cdclk(struct drm_i915_private *dev_priv)
> > > > +static int fixed_333mhz_get_cdclk(struct drm_i915_private *dev_priv)
> > > > {
> > > > return 333333;
> > > > }
> > > >
> > > > -static int i9xx_misc_get_cdclk(struct drm_i915_private *dev_priv)
> > > > +static int fixed_200mhz_get_cdclk(struct drm_i915_private *dev_priv)
> > > > {
> > > > return 200000;
> > > > }
> > > > @@ -7444,7 +7444,7 @@ static int i915gm_get_cdclk(struct
> > > > drm_i915_private
> > > > *dev_priv)
> > > > }
> > > > }
> > > >
> > > > -static int i865_get_cdclk(struct drm_i915_private *dev_priv)
> > > > +static int fixed_266mhz_get_cdclk(struct drm_i915_private *dev_priv)
> > > > {
> > > > return 266667;
> > > > }
> > > > @@ -7487,7 +7487,7 @@ static int i85x_get_cdclk(struct drm_i915_private
> > > > *dev_priv)
> > > > return 0;
> > > > }
> > > >
> > > > -static int i830_get_cdclk(struct drm_i915_private *dev_priv)
> > > > +static int fixed_133mhz_get_cdclk(struct drm_i915_private *dev_priv)
> > > > {
> > > > return 133333;
> > > > }
> > > > @@ -16098,32 +16098,39 @@ void intel_init_display_hooks(struct
> > > > drm_i915_private *dev_priv)
> > > > dev_priv->display.get_cdclk = haswell_get_cdclk;
> > > > else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> > > > dev_priv->display.get_cdclk = valleyview_get_cdclk;
> > > > + else if (IS_GEN6(dev_priv) || IS_IVYBRIDGE(dev_priv))
> > > > + dev_priv->display.get_cdclk = fixed_400mhz_get_cdclk;
> > > > else if (IS_GEN5(dev_priv))
> > > > - dev_priv->display.get_cdclk = ilk_get_cdclk;
> > > > - else if (IS_I945G(dev_priv) || IS_I965G(dev_priv) ||
> > > > - IS_GEN6(dev_priv) || IS_IVYBRIDGE(dev_priv))
> > > > - dev_priv->display.get_cdclk = i945_get_cdclk;
> > > > + 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_G4X(dev_priv))
> > > > + 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))
> > > > dev_priv->display.get_cdclk = pnv_get_cdclk;
> > > > - else if (IS_G33(dev_priv) || IS_G4X(dev_priv))
> > > > + else if (IS_G33(dev_priv))
> > > > dev_priv->display.get_cdclk = g33_get_cdclk;
> > > > - else if (IS_I915G(dev_priv))
> > > > - dev_priv->display.get_cdclk = i915_get_cdclk;
> > > > - else if (IS_I945GM(dev_priv) || IS_I845G(dev_priv))
> > > > - dev_priv->display.get_cdclk = i9xx_misc_get_cdclk;
> > > > + else if (IS_I945GM(dev_priv))
> > > > + dev_priv->display.get_cdclk = fixed_200mhz_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 = i865_get_cdclk;
> > > > + 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 = i830_get_cdclk;
> > > > + dev_priv->display.get_cdclk = fixed_133mhz_get_cdclk;
> > > > }
> > > I wonder if "switch (dev_priv->info.platform)" is a viable alternative
> > > to some of the worst if ladders we have, like this one.
> > How about something like this?
> I like this, but probably too intrusive to ask Ville to put this in
> mid-series. A useful cleanup afterwards?
I agree. I can even volunteer to do it after Ville's patches land.
Ander
>
> BR,
> Jani.
>
>
> >
> >
> > struct cdclk_iface {
> > int (*get)(...);
> > void (*set)(...);
> > int (*calc)(...);
> >
> > int fixed;
> > };
> >
> > static int fixed_cdclk_get(struct drm_i915_private *dev_priv)
> > {
> > return INTEL_INFO(dev_priv)->cdclk.fixed;
> > }
> >
> > #define FIXED_CDCLK(value) \
> > { .get = fixed_cdclk_get, .fixed = value }
> >
> > struct cdclk_iface skl_cdclk = {
> > .get = skl_get_cdclk,
> > .set = skl_set_cdclk,
> > .calc = skl_get_cdclk,
> > };
> >
> > ...
> >
> > And then in device info we just do either
> >
> > .cdclk = FIXED_CDCLK(value),
> >
> > or
> >
> > .cdclk = skl_cdclk,
> >
> > Ander
> >
> > >
> > >
> > > BR,
> > > Jani.
> > >
> > >
> > > >
> > > >
> > > >
> > > > if (IS_GEN5(dev_priv)) {
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
