Re: [PATCH 1/7] drm/i915: split out intel_pnv_find_best_PLL

[Daniel Vetter <daniel@ffwll.ch>]

On Mon, Jun 03, 2013 at 11:16:37AM -0300, Paulo Zanoni wrote:
> 2013/6/1 Daniel Vetter <daniel.vetter@ffwll.ch>:
> > Pineview is just different.
> >
> > Also split out i9xx_clock from intel_clock and drop the now redundant
> > struct device * parameter.
> >
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 92 ++++++++++++++++++++++++++++++------
> >  1 file changed, 77 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index ffdf511..7b0b16b 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -97,10 +97,13 @@ intel_find_best_PLL(const intel_limit_t *limit, struct drm_crtc *crtc,
> >                     int target, int refclk, intel_clock_t *match_clock,
> >                     intel_clock_t *best_clock);
> >  static bool
> > +intel_pnv_find_best_PLL(const intel_limit_t *limit, struct drm_crtc *crtc,
> > +                       int target, int refclk, intel_clock_t *match_clock,
> > +                       intel_clock_t *best_clock);
> > +static bool
> >  intel_g4x_find_best_PLL(const intel_limit_t *limit, struct drm_crtc *crtc,
> >                         int target, int refclk, intel_clock_t *match_clock,
> >                         intel_clock_t *best_clock);
> > -
> >  static bool
> >  intel_vlv_find_best_pll(const intel_limit_t *limit, struct drm_crtc *crtc,
> >                         int target, int refclk, intel_clock_t *match_clock,
> > @@ -246,7 +249,7 @@ static const intel_limit_t intel_limits_pineview_sdvo = {
> >         .p1 = { .min = 1, .max = 8 },
> >         .p2 = { .dot_limit = 200000,
> >                 .p2_slow = 10, .p2_fast = 5 },
> > -       .find_pll = intel_find_best_PLL,
> > +       .find_pll = intel_pnv_find_best_PLL,
> >  };
> >
> >  static const intel_limit_t intel_limits_pineview_lvds = {
> > @@ -260,7 +263,7 @@ static const intel_limit_t intel_limits_pineview_lvds = {
> >         .p1 = { .min = 1, .max = 8 },
> >         .p2 = { .dot_limit = 112000,
> >                 .p2_slow = 14, .p2_fast = 14 },
> > -       .find_pll = intel_find_best_PLL,
> > +       .find_pll = intel_pnv_find_best_PLL,
> >  };
> >
> >  /* Ironlake / Sandybridge
> > @@ -475,12 +478,8 @@ static uint32_t i9xx_dpll_compute_m(struct dpll *dpll)
> >         return 5 * (dpll->m1 + 2) + (dpll->m2 + 2);
> >  }
> >
> > -static void intel_clock(struct drm_device *dev, int refclk, intel_clock_t *clock)
> > +static void i9xx_clock(int refclk, intel_clock_t *clock)
> >  {
> > -       if (IS_PINEVIEW(dev)) {
> > -               pineview_clock(refclk, clock);
> > -               return;
> > -       }
> >         clock->m = i9xx_dpll_compute_m(clock);
> >         clock->p = clock->p1 * clock->p2;
> >         clock->vco = refclk * clock->m / (clock->n + 2);
> > @@ -541,7 +540,68 @@ static bool
> >  intel_find_best_PLL(const intel_limit_t *limit, struct drm_crtc *crtc,
> >                     int target, int refclk, intel_clock_t *match_clock,
> >                     intel_clock_t *best_clock)
> > +{
> > +       struct drm_device *dev = crtc->dev;
> > +       intel_clock_t clock;
> > +       int err = target;
> > +
> > +       if (intel_pipe_has_type(crtc, INTEL_OUTPUT_LVDS)) {
> > +               /*
> > +                * For LVDS just rely on its current settings for dual-channel.
> > +                * We haven't figured out how to reliably set up different
> > +                * single/dual channel state, if we even can.
> > +                */
> > +               if (intel_is_dual_link_lvds(dev))
> > +                       clock.p2 = limit->p2.p2_fast;
> > +               else
> > +                       clock.p2 = limit->p2.p2_slow;
> > +       } else {
> > +               if (target < limit->p2.dot_limit)
> > +                       clock.p2 = limit->p2.p2_slow;
> > +               else
> > +                       clock.p2 = limit->p2.p2_fast;
> > +       }
> > +
> > +       memset(best_clock, 0, sizeof(*best_clock));
> > +
> > +       for (clock.m1 = limit->m1.min; clock.m1 <= limit->m1.max;
> > +            clock.m1++) {
> > +               for (clock.m2 = limit->m2.min;
> > +                    clock.m2 <= limit->m2.max; clock.m2++) {
> > +                       /* m1 is always 0 in Pineview */
> > +                       if (clock.m2 >= clock.m1 && !IS_PINEVIEW(dev))
> > +                               break;
> 
> You're checking for IS_PINEVIEW on a function that doesn't run on
> pineview, so I guess you can remove the "&& !IS_PINEVIEW(dev)" part
> and the comment. You also have the same check inside
> intel_pnv_find_best_PLL, so you could update it too.

When I split a function into two, the first patch will _only_ ever
copy&paste the function, unchanged. I guess I could smash a cleanup patch
on top if you want.

> 
> 
> > +                       for (clock.n = limit->n.min;
> > +                            clock.n <= limit->n.max; clock.n++) {
> > +                               for (clock.p1 = limit->p1.min;
> > +                                       clock.p1 <= limit->p1.max; clock.p1++) {
> > +                                       int this_err;
> >
> > +                                       i9xx_clock(refclk, &clock);
> > +                                       if (!intel_PLL_is_valid(dev, limit,
> > +                                                               &clock))
> > +                                               continue;
> > +                                       if (match_clock &&
> > +                                           clock.p != match_clock->p)
> > +                                               continue;
> > +
> > +                                       this_err = abs(clock.dot - target);
> > +                                       if (this_err < err) {
> > +                                               *best_clock = clock;
> > +                                               err = this_err;
> > +                                       }
> > +                               }
> > +                       }
> > +               }
> > +       }
> > +
> > +       return (err != target);
> > +}
> > +
> > +static bool
> > +intel_pnv_find_best_PLL(const intel_limit_t *limit, struct drm_crtc *crtc,
> > +                       int target, int refclk, intel_clock_t *match_clock,
> > +                       intel_clock_t *best_clock)
> >  {
> >         struct drm_device *dev = crtc->dev;
> >         intel_clock_t clock;
> > @@ -579,7 +639,7 @@ intel_find_best_PLL(const intel_limit_t *limit, struct drm_crtc *crtc,
> >                                         clock.p1 <= limit->p1.max; clock.p1++) {
> >                                         int this_err;
> >
> > -                                       intel_clock(dev, refclk, &clock);
> > +                                       pineview_clock(refclk, &clock);
> >                                         if (!intel_PLL_is_valid(dev, limit,
> >                                                                 &clock))
> >                                                 continue;
> > @@ -638,7 +698,7 @@ intel_g4x_find_best_PLL(const intel_limit_t *limit, struct drm_crtc *crtc,
> >                                      clock.p1 >= limit->p1.min; clock.p1--) {
> >                                         int this_err;
> >
> > -                                       intel_clock(dev, refclk, &clock);
> > +                                       i9xx_clock(refclk, &clock);
> >                                         if (!intel_PLL_is_valid(dev, limit,
> >                                                                 &clock))
> >                                                 continue;
> > @@ -6909,8 +6969,10 @@ static int intel_crtc_clock_get(struct drm_device *dev, struct drm_crtc *crtc)
> >                         return 0;
> >                 }
> >
> > -               /* XXX: Handle the 100Mhz refclk */
> 
> Why did we remove this line?
> 
> Besides these 2 details, everything looks correct.

I couldn't figure out what it means. Since no one complained yet and I
couldn't find any references two a 100MHz clock I've killed it. Might be a
remnant from when the ilk+ code was smashed together with i9xx code, since
ilk _does_ have a 100MHz clock (and a 120MHz refclock, too).

> 
> 
> > -               intel_clock(dev, 96000, &clock);
> > +               if (IS_PINEVIEW(dev))
> > +                       pineview_clock(96000, &clock);
> > +               else
> > +                       i9xx_clock(96000, &clock);
> >         } else {
> >                 bool is_lvds = (pipe == 1) && (I915_READ(LVDS) & LVDS_PORT_EN);
> >
> > @@ -6922,9 +6984,9 @@ static int intel_crtc_clock_get(struct drm_device *dev, struct drm_crtc *crtc)
> >                         if ((dpll & PLL_REF_INPUT_MASK) ==
> >                             PLLB_REF_INPUT_SPREADSPECTRUMIN) {
> >                                 /* XXX: might not be 66MHz */
> > -                               intel_clock(dev, 66000, &clock);
> > +                               i9xx_clock(66000, &clock);
> >                         } else
> > -                               intel_clock(dev, 48000, &clock);
> > +                               i9xx_clock(48000, &clock);
> >                 } else {
> >                         if (dpll & PLL_P1_DIVIDE_BY_TWO)
> >                                 clock.p1 = 2;
> > @@ -6937,7 +6999,7 @@ static int intel_crtc_clock_get(struct drm_device *dev, struct drm_crtc *crtc)
> >                         else
> >                                 clock.p2 = 2;
> >
> > -                       intel_clock(dev, 48000, &clock);
> > +                       i9xx_clock(48000, &clock);
> >                 }
> >         }
> >
> > --
> > 1.7.11.7
> >
> 
> 
> 
> -- 
> Paulo Zanoni

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch