* [PATCH 1/7] drm/i915: split out intel_pnv_find_best_PLL
2013-06-01 15:16 [PATCH 0/7] adjusted mode rework and assorted patches Daniel Vetter
@ 2013-06-01 15:16 ` Daniel Vetter
2013-06-03 14:16 ` Paulo Zanoni
2013-06-01 15:16 ` [PATCH 2/7] drm/i915: move find_pll callback to dev_priv->display Daniel Vetter
` (5 subsequent siblings)
6 siblings, 1 reply; 28+ messages in thread
From: Daniel Vetter @ 2013-06-01 15:16 UTC (permalink / raw)
To: Intel Graphics Development; +Cc: Daniel Vetter
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;
+ 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 */
- 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
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [PATCH 1/7] drm/i915: split out intel_pnv_find_best_PLL
2013-06-01 15:16 ` [PATCH 1/7] drm/i915: split out intel_pnv_find_best_PLL Daniel Vetter
@ 2013-06-03 14:16 ` Paulo Zanoni
2013-06-03 18:26 ` Daniel Vetter
0 siblings, 1 reply; 28+ messages in thread
From: Paulo Zanoni @ 2013-06-03 14:16 UTC (permalink / raw)
To: Daniel Vetter; +Cc: Intel Graphics Development
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.
> + 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.
> - 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
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH 1/7] drm/i915: split out intel_pnv_find_best_PLL
2013-06-03 14:16 ` Paulo Zanoni
@ 2013-06-03 18:26 ` Daniel Vetter
0 siblings, 0 replies; 28+ messages in thread
From: Daniel Vetter @ 2013-06-03 18:26 UTC (permalink / raw)
To: Paulo Zanoni; +Cc: Daniel Vetter, Intel Graphics Development
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
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 2/7] drm/i915: move find_pll callback to dev_priv->display
2013-06-01 15:16 [PATCH 0/7] adjusted mode rework and assorted patches Daniel Vetter
2013-06-01 15:16 ` [PATCH 1/7] drm/i915: split out intel_pnv_find_best_PLL Daniel Vetter
@ 2013-06-01 15:16 ` Daniel Vetter
2013-06-03 14:35 ` Paulo Zanoni
2013-06-01 15:16 ` [PATCH 3/7] drm/i915: clear up the fdi dotclock semantics for M/N computation Daniel Vetter
` (4 subsequent siblings)
6 siblings, 1 reply; 28+ messages in thread
From: Daniel Vetter @ 2013-06-01 15:16 UTC (permalink / raw)
To: Intel Graphics Development; +Cc: Daniel Vetter
Now that the DP madness is cleared out, this is all only per-platform.
So move it out from the intel clock limits structure.
While at it drop the intel prefix on the static functions, call the
vtable entry find_dpll (since it's for the display pll) and rip out
the now unnecessary forward declarations.
Note that the parameters of ->find_dpll are still unchanged, but they
eventually need to be moved over to just take in a pipe configuration.
But currently a lot of things are still missing from the pipe
configuration (reflock, output-specific dpll limits and preferences,
downclocked dotclock). So this will happen in a later step.
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
drivers/gpu/drm/i915/i915_drv.h | 7 +++
drivers/gpu/drm/i915/intel_display.c | 106 ++++++++++-------------------------
2 files changed, 38 insertions(+), 75 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 359a200..1472a50 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -306,6 +306,8 @@ struct drm_i915_error_state {
struct intel_crtc_config;
struct intel_crtc;
+struct intel_limit;
+struct dpll;
struct drm_i915_display_funcs {
bool (*fbc_enabled)(struct drm_device *dev);
@@ -313,6 +315,11 @@ struct drm_i915_display_funcs {
void (*disable_fbc)(struct drm_device *dev);
int (*get_display_clock_speed)(struct drm_device *dev);
int (*get_fifo_size)(struct drm_device *dev, int plane);
+ bool (*find_dpll)(const struct intel_limit *limit,
+ struct drm_crtc *crtc,
+ int target, int refclk,
+ struct dpll *match_clock,
+ struct dpll *best_clock);
void (*update_wm)(struct drm_device *dev);
void (*update_sprite_wm)(struct drm_device *dev, int pipe,
uint32_t sprite_width, int pixel_size,
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 7b0b16b..a29295e 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -59,24 +59,6 @@ typedef struct intel_limit intel_limit_t;
struct intel_limit {
intel_range_t dot, vco, n, m, m1, m2, p, p1;
intel_p2_t p2;
- /**
- * find_pll() - Find the best values for the PLL
- * @limit: limits for the PLL
- * @crtc: current CRTC
- * @target: target frequency in kHz
- * @refclk: reference clock frequency in kHz
- * @match_clock: if provided, @best_clock P divider must
- * match the P divider from @match_clock
- * used for LVDS downclocking
- * @best_clock: best PLL values found
- *
- * Returns true on success, false on failure.
- */
- bool (*find_pll)(const intel_limit_t *limit,
- struct drm_crtc *crtc,
- int target, int refclk,
- intel_clock_t *match_clock,
- intel_clock_t *best_clock);
};
/* FDI */
@@ -92,23 +74,6 @@ intel_pch_rawclk(struct drm_device *dev)
return I915_READ(PCH_RAWCLK_FREQ) & RAWCLK_FREQ_MASK;
}
-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);
-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,
- intel_clock_t *best_clock);
-
static inline u32 /* units of 100MHz */
intel_fdi_link_freq(struct drm_device *dev)
{
@@ -130,7 +95,6 @@ static const intel_limit_t intel_limits_i8xx_dvo = {
.p1 = { .min = 2, .max = 33 },
.p2 = { .dot_limit = 165000,
.p2_slow = 4, .p2_fast = 2 },
- .find_pll = intel_find_best_PLL,
};
static const intel_limit_t intel_limits_i8xx_lvds = {
@@ -144,7 +108,6 @@ static const intel_limit_t intel_limits_i8xx_lvds = {
.p1 = { .min = 1, .max = 6 },
.p2 = { .dot_limit = 165000,
.p2_slow = 14, .p2_fast = 7 },
- .find_pll = intel_find_best_PLL,
};
static const intel_limit_t intel_limits_i9xx_sdvo = {
@@ -158,7 +121,6 @@ static const intel_limit_t intel_limits_i9xx_sdvo = {
.p1 = { .min = 1, .max = 8 },
.p2 = { .dot_limit = 200000,
.p2_slow = 10, .p2_fast = 5 },
- .find_pll = intel_find_best_PLL,
};
static const intel_limit_t intel_limits_i9xx_lvds = {
@@ -172,7 +134,6 @@ static const intel_limit_t intel_limits_i9xx_lvds = {
.p1 = { .min = 1, .max = 8 },
.p2 = { .dot_limit = 112000,
.p2_slow = 14, .p2_fast = 7 },
- .find_pll = intel_find_best_PLL,
};
@@ -189,7 +150,6 @@ static const intel_limit_t intel_limits_g4x_sdvo = {
.p2_slow = 10,
.p2_fast = 10
},
- .find_pll = intel_g4x_find_best_PLL,
};
static const intel_limit_t intel_limits_g4x_hdmi = {
@@ -203,7 +163,6 @@ static const intel_limit_t intel_limits_g4x_hdmi = {
.p1 = { .min = 1, .max = 8},
.p2 = { .dot_limit = 165000,
.p2_slow = 10, .p2_fast = 5 },
- .find_pll = intel_g4x_find_best_PLL,
};
static const intel_limit_t intel_limits_g4x_single_channel_lvds = {
@@ -218,7 +177,6 @@ static const intel_limit_t intel_limits_g4x_single_channel_lvds = {
.p2 = { .dot_limit = 0,
.p2_slow = 14, .p2_fast = 14
},
- .find_pll = intel_g4x_find_best_PLL,
};
static const intel_limit_t intel_limits_g4x_dual_channel_lvds = {
@@ -233,7 +191,6 @@ static const intel_limit_t intel_limits_g4x_dual_channel_lvds = {
.p2 = { .dot_limit = 0,
.p2_slow = 7, .p2_fast = 7
},
- .find_pll = intel_g4x_find_best_PLL,
};
static const intel_limit_t intel_limits_pineview_sdvo = {
@@ -249,7 +206,6 @@ 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_pnv_find_best_PLL,
};
static const intel_limit_t intel_limits_pineview_lvds = {
@@ -263,7 +219,6 @@ 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_pnv_find_best_PLL,
};
/* Ironlake / Sandybridge
@@ -282,7 +237,6 @@ static const intel_limit_t intel_limits_ironlake_dac = {
.p1 = { .min = 1, .max = 8 },
.p2 = { .dot_limit = 225000,
.p2_slow = 10, .p2_fast = 5 },
- .find_pll = intel_g4x_find_best_PLL,
};
static const intel_limit_t intel_limits_ironlake_single_lvds = {
@@ -296,7 +250,6 @@ static const intel_limit_t intel_limits_ironlake_single_lvds = {
.p1 = { .min = 2, .max = 8 },
.p2 = { .dot_limit = 225000,
.p2_slow = 14, .p2_fast = 14 },
- .find_pll = intel_g4x_find_best_PLL,
};
static const intel_limit_t intel_limits_ironlake_dual_lvds = {
@@ -310,7 +263,6 @@ static const intel_limit_t intel_limits_ironlake_dual_lvds = {
.p1 = { .min = 2, .max = 8 },
.p2 = { .dot_limit = 225000,
.p2_slow = 7, .p2_fast = 7 },
- .find_pll = intel_g4x_find_best_PLL,
};
/* LVDS 100mhz refclk limits. */
@@ -325,7 +277,6 @@ static const intel_limit_t intel_limits_ironlake_single_lvds_100m = {
.p1 = { .min = 2, .max = 8 },
.p2 = { .dot_limit = 225000,
.p2_slow = 14, .p2_fast = 14 },
- .find_pll = intel_g4x_find_best_PLL,
};
static const intel_limit_t intel_limits_ironlake_dual_lvds_100m = {
@@ -339,7 +290,6 @@ static const intel_limit_t intel_limits_ironlake_dual_lvds_100m = {
.p1 = { .min = 2, .max = 6 },
.p2 = { .dot_limit = 225000,
.p2_slow = 7, .p2_fast = 7 },
- .find_pll = intel_g4x_find_best_PLL,
};
static const intel_limit_t intel_limits_vlv_dac = {
@@ -353,7 +303,6 @@ static const intel_limit_t intel_limits_vlv_dac = {
.p1 = { .min = 1, .max = 3 },
.p2 = { .dot_limit = 270000,
.p2_slow = 2, .p2_fast = 20 },
- .find_pll = intel_vlv_find_best_pll,
};
static const intel_limit_t intel_limits_vlv_hdmi = {
@@ -367,7 +316,6 @@ static const intel_limit_t intel_limits_vlv_hdmi = {
.p1 = { .min = 2, .max = 3 },
.p2 = { .dot_limit = 270000,
.p2_slow = 2, .p2_fast = 20 },
- .find_pll = intel_vlv_find_best_pll,
};
static const intel_limit_t intel_limits_vlv_dp = {
@@ -381,7 +329,6 @@ static const intel_limit_t intel_limits_vlv_dp = {
.p1 = { .min = 1, .max = 3 },
.p2 = { .dot_limit = 270000,
.p2_slow = 2, .p2_fast = 20 },
- .find_pll = intel_vlv_find_best_pll,
};
static const intel_limit_t *intel_ironlake_limit(struct drm_crtc *crtc,
@@ -537,7 +484,7 @@ static bool intel_PLL_is_valid(struct drm_device *dev,
}
static bool
-intel_find_best_PLL(const intel_limit_t *limit, struct drm_crtc *crtc,
+i9xx_find_best_dpll(const intel_limit_t *limit, struct drm_crtc *crtc,
int target, int refclk, intel_clock_t *match_clock,
intel_clock_t *best_clock)
{
@@ -599,9 +546,9 @@ intel_find_best_PLL(const intel_limit_t *limit, struct drm_crtc *crtc,
}
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)
+pnv_find_best_dpll(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;
@@ -661,9 +608,9 @@ intel_pnv_find_best_PLL(const intel_limit_t *limit, struct drm_crtc *crtc,
}
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)
+g4x_find_best_dpll(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;
@@ -718,9 +665,9 @@ intel_g4x_find_best_PLL(const intel_limit_t *limit, struct drm_crtc *crtc,
}
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,
- intel_clock_t *best_clock)
+vlv_find_best_dpll(const intel_limit_t *limit, struct drm_crtc *crtc,
+ int target, int refclk, intel_clock_t *match_clock,
+ intel_clock_t *best_clock)
{
u32 p1, p2, m1, m2, vco, bestn, bestm1, bestm2, bestp1, bestp2;
u32 m, n, fastclk;
@@ -4910,8 +4857,8 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
* reflck * (5 * (m1 + 2) + (m2 + 2)) / (n + 2) / p1 / p2.
*/
limit = intel_limit(crtc, refclk);
- ok = limit->find_pll(limit, crtc, adjusted_mode->clock, refclk, NULL,
- &clock);
+ ok = dev_priv->display.find_dpll(limit, crtc, adjusted_mode->clock,
+ refclk, NULL, &clock);
if (!ok) {
DRM_ERROR("Couldn't find PLL settings for mode!\n");
return -EINVAL;
@@ -4927,10 +4874,10 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
* by using the FP0/FP1. In such case we will disable the LVDS
* downclock feature.
*/
- has_reduced_clock = limit->find_pll(limit, crtc,
+ has_reduced_clock =
+ dev_priv->display.find_dpll(limit, crtc,
dev_priv->lvds_downclock,
- refclk,
- &clock,
+ refclk, &clock,
&reduced_clock);
}
/* Compat-code for transition, will disappear. */
@@ -5546,8 +5493,8 @@ static bool ironlake_compute_clocks(struct drm_crtc *crtc,
* reflck * (5 * (m1 + 2) + (m2 + 2)) / (n + 2) / p1 / p2.
*/
limit = intel_limit(crtc, refclk);
- ret = limit->find_pll(limit, crtc, adjusted_mode->clock, refclk, NULL,
- clock);
+ ret = dev_priv->display.find_dpll(limit, crtc, adjusted_mode->clock,
+ refclk, NULL, clock);
if (!ret)
return false;
@@ -5558,11 +5505,11 @@ static bool ironlake_compute_clocks(struct drm_crtc *crtc,
* by using the FP0/FP1. In such case we will disable the LVDS
* downclock feature.
*/
- *has_reduced_clock = limit->find_pll(limit, crtc,
- dev_priv->lvds_downclock,
- refclk,
- clock,
- reduced_clock);
+ *has_reduced_clock =
+ dev_priv->display.find_dpll(limit, crtc,
+ dev_priv->lvds_downclock,
+ refclk, clock,
+ reduced_clock);
}
return true;
@@ -9073,6 +9020,15 @@ static void intel_init_display(struct drm_device *dev)
{
struct drm_i915_private *dev_priv = dev->dev_private;
+ if (HAS_PCH_SPLIT(dev) || IS_G4X(dev))
+ dev_priv->display.find_dpll = g4x_find_best_dpll;
+ else if (IS_VALLEYVIEW(dev))
+ dev_priv->display.find_dpll = vlv_find_best_dpll;
+ else if (IS_PINEVIEW(dev))
+ dev_priv->display.find_dpll = pnv_find_best_dpll;
+ else
+ dev_priv->display.find_dpll = i9xx_find_best_dpll;
+
if (HAS_DDI(dev)) {
dev_priv->display.get_pipe_config = haswell_get_pipe_config;
dev_priv->display.crtc_mode_set = haswell_crtc_mode_set;
--
1.7.11.7
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [PATCH 2/7] drm/i915: move find_pll callback to dev_priv->display
2013-06-01 15:16 ` [PATCH 2/7] drm/i915: move find_pll callback to dev_priv->display Daniel Vetter
@ 2013-06-03 14:35 ` Paulo Zanoni
2013-06-03 18:36 ` Daniel Vetter
2013-06-03 18:56 ` [PATCH 1/2] " Daniel Vetter
0 siblings, 2 replies; 28+ messages in thread
From: Paulo Zanoni @ 2013-06-03 14:35 UTC (permalink / raw)
To: Daniel Vetter; +Cc: Intel Graphics Development
2013/6/1 Daniel Vetter <daniel.vetter@ffwll.ch>:
> Now that the DP madness is cleared out, this is all only per-platform.
> So move it out from the intel clock limits structure.
>
> While at it drop the intel prefix on the static functions, call the
> vtable entry find_dpll (since it's for the display pll) and rip out
> the now unnecessary forward declarations.
>
> Note that the parameters of ->find_dpll are still unchanged, but they
> eventually need to be moved over to just take in a pipe configuration.
> But currently a lot of things are still missing from the pipe
> configuration (reflock, output-specific dpll limits and preferences,
> downclocked dotclock). So this will happen in a later step.
>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
> drivers/gpu/drm/i915/i915_drv.h | 7 +++
> drivers/gpu/drm/i915/intel_display.c | 106 ++++++++++-------------------------
> 2 files changed, 38 insertions(+), 75 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 359a200..1472a50 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -306,6 +306,8 @@ struct drm_i915_error_state {
>
> struct intel_crtc_config;
> struct intel_crtc;
> +struct intel_limit;
> +struct dpll;
>
> struct drm_i915_display_funcs {
> bool (*fbc_enabled)(struct drm_device *dev);
> @@ -313,6 +315,11 @@ struct drm_i915_display_funcs {
> void (*disable_fbc)(struct drm_device *dev);
> int (*get_display_clock_speed)(struct drm_device *dev);
> int (*get_fifo_size)(struct drm_device *dev, int plane);
> + bool (*find_dpll)(const struct intel_limit *limit,
> + struct drm_crtc *crtc,
> + int target, int refclk,
> + struct dpll *match_clock,
> + struct dpll *best_clock);
And now the nice comments explaining how the function works are gone.
Please re-add them.
> void (*update_wm)(struct drm_device *dev);
> void (*update_sprite_wm)(struct drm_device *dev, int pipe,
> uint32_t sprite_width, int pixel_size,
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 7b0b16b..a29295e 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -59,24 +59,6 @@ typedef struct intel_limit intel_limit_t;
> struct intel_limit {
> intel_range_t dot, vco, n, m, m1, m2, p, p1;
> intel_p2_t p2;
> - /**
> - * find_pll() - Find the best values for the PLL
> - * @limit: limits for the PLL
> - * @crtc: current CRTC
> - * @target: target frequency in kHz
> - * @refclk: reference clock frequency in kHz
> - * @match_clock: if provided, @best_clock P divider must
> - * match the P divider from @match_clock
> - * used for LVDS downclocking
> - * @best_clock: best PLL values found
> - *
> - * Returns true on success, false on failure.
> - */
> - bool (*find_pll)(const intel_limit_t *limit,
> - struct drm_crtc *crtc,
> - int target, int refclk,
> - intel_clock_t *match_clock,
> - intel_clock_t *best_clock);
> };
>
> /* FDI */
> @@ -92,23 +74,6 @@ intel_pch_rawclk(struct drm_device *dev)
> return I915_READ(PCH_RAWCLK_FREQ) & RAWCLK_FREQ_MASK;
> }
>
> -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);
> -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,
> - intel_clock_t *best_clock);
> -
> static inline u32 /* units of 100MHz */
> intel_fdi_link_freq(struct drm_device *dev)
> {
> @@ -130,7 +95,6 @@ static const intel_limit_t intel_limits_i8xx_dvo = {
> .p1 = { .min = 2, .max = 33 },
> .p2 = { .dot_limit = 165000,
> .p2_slow = 4, .p2_fast = 2 },
> - .find_pll = intel_find_best_PLL,
> };
>
> static const intel_limit_t intel_limits_i8xx_lvds = {
> @@ -144,7 +108,6 @@ static const intel_limit_t intel_limits_i8xx_lvds = {
> .p1 = { .min = 1, .max = 6 },
> .p2 = { .dot_limit = 165000,
> .p2_slow = 14, .p2_fast = 7 },
> - .find_pll = intel_find_best_PLL,
> };
>
> static const intel_limit_t intel_limits_i9xx_sdvo = {
> @@ -158,7 +121,6 @@ static const intel_limit_t intel_limits_i9xx_sdvo = {
> .p1 = { .min = 1, .max = 8 },
> .p2 = { .dot_limit = 200000,
> .p2_slow = 10, .p2_fast = 5 },
> - .find_pll = intel_find_best_PLL,
> };
>
> static const intel_limit_t intel_limits_i9xx_lvds = {
> @@ -172,7 +134,6 @@ static const intel_limit_t intel_limits_i9xx_lvds = {
> .p1 = { .min = 1, .max = 8 },
> .p2 = { .dot_limit = 112000,
> .p2_slow = 14, .p2_fast = 7 },
> - .find_pll = intel_find_best_PLL,
> };
>
>
> @@ -189,7 +150,6 @@ static const intel_limit_t intel_limits_g4x_sdvo = {
> .p2_slow = 10,
> .p2_fast = 10
> },
> - .find_pll = intel_g4x_find_best_PLL,
> };
>
> static const intel_limit_t intel_limits_g4x_hdmi = {
> @@ -203,7 +163,6 @@ static const intel_limit_t intel_limits_g4x_hdmi = {
> .p1 = { .min = 1, .max = 8},
> .p2 = { .dot_limit = 165000,
> .p2_slow = 10, .p2_fast = 5 },
> - .find_pll = intel_g4x_find_best_PLL,
> };
>
> static const intel_limit_t intel_limits_g4x_single_channel_lvds = {
> @@ -218,7 +177,6 @@ static const intel_limit_t intel_limits_g4x_single_channel_lvds = {
> .p2 = { .dot_limit = 0,
> .p2_slow = 14, .p2_fast = 14
> },
> - .find_pll = intel_g4x_find_best_PLL,
> };
>
> static const intel_limit_t intel_limits_g4x_dual_channel_lvds = {
> @@ -233,7 +191,6 @@ static const intel_limit_t intel_limits_g4x_dual_channel_lvds = {
> .p2 = { .dot_limit = 0,
> .p2_slow = 7, .p2_fast = 7
> },
> - .find_pll = intel_g4x_find_best_PLL,
> };
>
> static const intel_limit_t intel_limits_pineview_sdvo = {
> @@ -249,7 +206,6 @@ 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_pnv_find_best_PLL,
> };
>
> static const intel_limit_t intel_limits_pineview_lvds = {
> @@ -263,7 +219,6 @@ 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_pnv_find_best_PLL,
> };
>
> /* Ironlake / Sandybridge
> @@ -282,7 +237,6 @@ static const intel_limit_t intel_limits_ironlake_dac = {
> .p1 = { .min = 1, .max = 8 },
> .p2 = { .dot_limit = 225000,
> .p2_slow = 10, .p2_fast = 5 },
> - .find_pll = intel_g4x_find_best_PLL,
> };
>
> static const intel_limit_t intel_limits_ironlake_single_lvds = {
> @@ -296,7 +250,6 @@ static const intel_limit_t intel_limits_ironlake_single_lvds = {
> .p1 = { .min = 2, .max = 8 },
> .p2 = { .dot_limit = 225000,
> .p2_slow = 14, .p2_fast = 14 },
> - .find_pll = intel_g4x_find_best_PLL,
> };
>
> static const intel_limit_t intel_limits_ironlake_dual_lvds = {
> @@ -310,7 +263,6 @@ static const intel_limit_t intel_limits_ironlake_dual_lvds = {
> .p1 = { .min = 2, .max = 8 },
> .p2 = { .dot_limit = 225000,
> .p2_slow = 7, .p2_fast = 7 },
> - .find_pll = intel_g4x_find_best_PLL,
> };
>
> /* LVDS 100mhz refclk limits. */
> @@ -325,7 +277,6 @@ static const intel_limit_t intel_limits_ironlake_single_lvds_100m = {
> .p1 = { .min = 2, .max = 8 },
> .p2 = { .dot_limit = 225000,
> .p2_slow = 14, .p2_fast = 14 },
> - .find_pll = intel_g4x_find_best_PLL,
> };
>
> static const intel_limit_t intel_limits_ironlake_dual_lvds_100m = {
> @@ -339,7 +290,6 @@ static const intel_limit_t intel_limits_ironlake_dual_lvds_100m = {
> .p1 = { .min = 2, .max = 6 },
> .p2 = { .dot_limit = 225000,
> .p2_slow = 7, .p2_fast = 7 },
> - .find_pll = intel_g4x_find_best_PLL,
> };
>
> static const intel_limit_t intel_limits_vlv_dac = {
> @@ -353,7 +303,6 @@ static const intel_limit_t intel_limits_vlv_dac = {
> .p1 = { .min = 1, .max = 3 },
> .p2 = { .dot_limit = 270000,
> .p2_slow = 2, .p2_fast = 20 },
> - .find_pll = intel_vlv_find_best_pll,
> };
>
> static const intel_limit_t intel_limits_vlv_hdmi = {
> @@ -367,7 +316,6 @@ static const intel_limit_t intel_limits_vlv_hdmi = {
> .p1 = { .min = 2, .max = 3 },
> .p2 = { .dot_limit = 270000,
> .p2_slow = 2, .p2_fast = 20 },
> - .find_pll = intel_vlv_find_best_pll,
> };
>
> static const intel_limit_t intel_limits_vlv_dp = {
> @@ -381,7 +329,6 @@ static const intel_limit_t intel_limits_vlv_dp = {
> .p1 = { .min = 1, .max = 3 },
> .p2 = { .dot_limit = 270000,
> .p2_slow = 2, .p2_fast = 20 },
> - .find_pll = intel_vlv_find_best_pll,
> };
>
> static const intel_limit_t *intel_ironlake_limit(struct drm_crtc *crtc,
> @@ -537,7 +484,7 @@ static bool intel_PLL_is_valid(struct drm_device *dev,
> }
>
> static bool
> -intel_find_best_PLL(const intel_limit_t *limit, struct drm_crtc *crtc,
> +i9xx_find_best_dpll(const intel_limit_t *limit, struct drm_crtc *crtc,
> int target, int refclk, intel_clock_t *match_clock,
> intel_clock_t *best_clock)
> {
> @@ -599,9 +546,9 @@ intel_find_best_PLL(const intel_limit_t *limit, struct drm_crtc *crtc,
> }
>
> 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)
> +pnv_find_best_dpll(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;
> @@ -661,9 +608,9 @@ intel_pnv_find_best_PLL(const intel_limit_t *limit, struct drm_crtc *crtc,
> }
>
> 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)
> +g4x_find_best_dpll(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;
> @@ -718,9 +665,9 @@ intel_g4x_find_best_PLL(const intel_limit_t *limit, struct drm_crtc *crtc,
> }
>
> 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,
> - intel_clock_t *best_clock)
> +vlv_find_best_dpll(const intel_limit_t *limit, struct drm_crtc *crtc,
> + int target, int refclk, intel_clock_t *match_clock,
> + intel_clock_t *best_clock)
> {
> u32 p1, p2, m1, m2, vco, bestn, bestm1, bestm2, bestp1, bestp2;
> u32 m, n, fastclk;
> @@ -4910,8 +4857,8 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
> * reflck * (5 * (m1 + 2) + (m2 + 2)) / (n + 2) / p1 / p2.
> */
> limit = intel_limit(crtc, refclk);
> - ok = limit->find_pll(limit, crtc, adjusted_mode->clock, refclk, NULL,
> - &clock);
> + ok = dev_priv->display.find_dpll(limit, crtc, adjusted_mode->clock,
> + refclk, NULL, &clock);
> if (!ok) {
> DRM_ERROR("Couldn't find PLL settings for mode!\n");
> return -EINVAL;
> @@ -4927,10 +4874,10 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
> * by using the FP0/FP1. In such case we will disable the LVDS
> * downclock feature.
> */
> - has_reduced_clock = limit->find_pll(limit, crtc,
> + has_reduced_clock =
> + dev_priv->display.find_dpll(limit, crtc,
> dev_priv->lvds_downclock,
> - refclk,
> - &clock,
> + refclk, &clock,
> &reduced_clock);
> }
> /* Compat-code for transition, will disappear. */
> @@ -5546,8 +5493,8 @@ static bool ironlake_compute_clocks(struct drm_crtc *crtc,
> * reflck * (5 * (m1 + 2) + (m2 + 2)) / (n + 2) / p1 / p2.
> */
> limit = intel_limit(crtc, refclk);
> - ret = limit->find_pll(limit, crtc, adjusted_mode->clock, refclk, NULL,
> - clock);
> + ret = dev_priv->display.find_dpll(limit, crtc, adjusted_mode->clock,
> + refclk, NULL, clock);
> if (!ret)
> return false;
>
> @@ -5558,11 +5505,11 @@ static bool ironlake_compute_clocks(struct drm_crtc *crtc,
> * by using the FP0/FP1. In such case we will disable the LVDS
> * downclock feature.
> */
> - *has_reduced_clock = limit->find_pll(limit, crtc,
> - dev_priv->lvds_downclock,
> - refclk,
> - clock,
> - reduced_clock);
> + *has_reduced_clock =
> + dev_priv->display.find_dpll(limit, crtc,
> + dev_priv->lvds_downclock,
> + refclk, clock,
> + reduced_clock);
> }
>
> return true;
> @@ -9073,6 +9020,15 @@ static void intel_init_display(struct drm_device *dev)
> {
> struct drm_i915_private *dev_priv = dev->dev_private;
>
> + if (HAS_PCH_SPLIT(dev) || IS_G4X(dev))
> + dev_priv->display.find_dpll = g4x_find_best_dpll;
Doesn't look correct. If you look at intel_g4x_limit, you'll see that
it assigns intel_limits_i9xx_sdvo for the "!lvds && !hdmi && !analog
&& !sdvo" case. And intel_limits_i9xx_sdvo uses intel_find_best_PLL,
not intel_g4x_find_best_PLL.
> + else if (IS_VALLEYVIEW(dev))
> + dev_priv->display.find_dpll = vlv_find_best_dpll;
> + else if (IS_PINEVIEW(dev))
> + dev_priv->display.find_dpll = pnv_find_best_dpll;
> + else
> + dev_priv->display.find_dpll = i9xx_find_best_dpll;
> +
> if (HAS_DDI(dev)) {
> dev_priv->display.get_pipe_config = haswell_get_pipe_config;
> dev_priv->display.crtc_mode_set = haswell_crtc_mode_set;
> --
> 1.7.11.7
>
--
Paulo Zanoni
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH 2/7] drm/i915: move find_pll callback to dev_priv->display
2013-06-03 14:35 ` Paulo Zanoni
@ 2013-06-03 18:36 ` Daniel Vetter
2013-06-03 18:56 ` [PATCH 1/2] " Daniel Vetter
1 sibling, 0 replies; 28+ messages in thread
From: Daniel Vetter @ 2013-06-03 18:36 UTC (permalink / raw)
To: Paulo Zanoni; +Cc: Daniel Vetter, Intel Graphics Development
On Mon, Jun 03, 2013 at 11:35:02AM -0300, Paulo Zanoni wrote:
> 2013/6/1 Daniel Vetter <daniel.vetter@ffwll.ch>:
> > Now that the DP madness is cleared out, this is all only per-platform.
> > So move it out from the intel clock limits structure.
> >
> > While at it drop the intel prefix on the static functions, call the
> > vtable entry find_dpll (since it's for the display pll) and rip out
> > the now unnecessary forward declarations.
> >
> > Note that the parameters of ->find_dpll are still unchanged, but they
> > eventually need to be moved over to just take in a pipe configuration.
> > But currently a lot of things are still missing from the pipe
> > configuration (reflock, output-specific dpll limits and preferences,
> > downclocked dotclock). So this will happen in a later step.
> >
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > ---
> > drivers/gpu/drm/i915/i915_drv.h | 7 +++
> > drivers/gpu/drm/i915/intel_display.c | 106 ++++++++++-------------------------
> > 2 files changed, 38 insertions(+), 75 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 359a200..1472a50 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -306,6 +306,8 @@ struct drm_i915_error_state {
> >
> > struct intel_crtc_config;
> > struct intel_crtc;
> > +struct intel_limit;
> > +struct dpll;
> >
> > struct drm_i915_display_funcs {
> > bool (*fbc_enabled)(struct drm_device *dev);
> > @@ -313,6 +315,11 @@ struct drm_i915_display_funcs {
> > void (*disable_fbc)(struct drm_device *dev);
> > int (*get_display_clock_speed)(struct drm_device *dev);
> > int (*get_fifo_size)(struct drm_device *dev, int plane);
> > + bool (*find_dpll)(const struct intel_limit *limit,
> > + struct drm_crtc *crtc,
> > + int target, int refclk,
> > + struct dpll *match_clock,
> > + struct dpll *best_clock);
>
> And now the nice comments explaining how the function works are gone.
> Please re-add them.
I find comments which explain the obvious not too valuable, so I've killed
it. Will re-add though.
>
>
> > void (*update_wm)(struct drm_device *dev);
> > void (*update_sprite_wm)(struct drm_device *dev, int pipe,
> > uint32_t sprite_width, int pixel_size,
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 7b0b16b..a29295e 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -59,24 +59,6 @@ typedef struct intel_limit intel_limit_t;
> > struct intel_limit {
> > intel_range_t dot, vco, n, m, m1, m2, p, p1;
> > intel_p2_t p2;
> > - /**
> > - * find_pll() - Find the best values for the PLL
> > - * @limit: limits for the PLL
> > - * @crtc: current CRTC
> > - * @target: target frequency in kHz
> > - * @refclk: reference clock frequency in kHz
> > - * @match_clock: if provided, @best_clock P divider must
> > - * match the P divider from @match_clock
> > - * used for LVDS downclocking
> > - * @best_clock: best PLL values found
> > - *
> > - * Returns true on success, false on failure.
> > - */
> > - bool (*find_pll)(const intel_limit_t *limit,
> > - struct drm_crtc *crtc,
> > - int target, int refclk,
> > - intel_clock_t *match_clock,
> > - intel_clock_t *best_clock);
> > };
> >
> > /* FDI */
> > @@ -92,23 +74,6 @@ intel_pch_rawclk(struct drm_device *dev)
> > return I915_READ(PCH_RAWCLK_FREQ) & RAWCLK_FREQ_MASK;
> > }
> >
> > -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);
> > -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,
> > - intel_clock_t *best_clock);
> > -
> > static inline u32 /* units of 100MHz */
> > intel_fdi_link_freq(struct drm_device *dev)
> > {
> > @@ -130,7 +95,6 @@ static const intel_limit_t intel_limits_i8xx_dvo = {
> > .p1 = { .min = 2, .max = 33 },
> > .p2 = { .dot_limit = 165000,
> > .p2_slow = 4, .p2_fast = 2 },
> > - .find_pll = intel_find_best_PLL,
> > };
> >
> > static const intel_limit_t intel_limits_i8xx_lvds = {
> > @@ -144,7 +108,6 @@ static const intel_limit_t intel_limits_i8xx_lvds = {
> > .p1 = { .min = 1, .max = 6 },
> > .p2 = { .dot_limit = 165000,
> > .p2_slow = 14, .p2_fast = 7 },
> > - .find_pll = intel_find_best_PLL,
> > };
> >
> > static const intel_limit_t intel_limits_i9xx_sdvo = {
> > @@ -158,7 +121,6 @@ static const intel_limit_t intel_limits_i9xx_sdvo = {
> > .p1 = { .min = 1, .max = 8 },
> > .p2 = { .dot_limit = 200000,
> > .p2_slow = 10, .p2_fast = 5 },
> > - .find_pll = intel_find_best_PLL,
> > };
> >
> > static const intel_limit_t intel_limits_i9xx_lvds = {
> > @@ -172,7 +134,6 @@ static const intel_limit_t intel_limits_i9xx_lvds = {
> > .p1 = { .min = 1, .max = 8 },
> > .p2 = { .dot_limit = 112000,
> > .p2_slow = 14, .p2_fast = 7 },
> > - .find_pll = intel_find_best_PLL,
> > };
> >
> >
> > @@ -189,7 +150,6 @@ static const intel_limit_t intel_limits_g4x_sdvo = {
> > .p2_slow = 10,
> > .p2_fast = 10
> > },
> > - .find_pll = intel_g4x_find_best_PLL,
> > };
> >
> > static const intel_limit_t intel_limits_g4x_hdmi = {
> > @@ -203,7 +163,6 @@ static const intel_limit_t intel_limits_g4x_hdmi = {
> > .p1 = { .min = 1, .max = 8},
> > .p2 = { .dot_limit = 165000,
> > .p2_slow = 10, .p2_fast = 5 },
> > - .find_pll = intel_g4x_find_best_PLL,
> > };
> >
> > static const intel_limit_t intel_limits_g4x_single_channel_lvds = {
> > @@ -218,7 +177,6 @@ static const intel_limit_t intel_limits_g4x_single_channel_lvds = {
> > .p2 = { .dot_limit = 0,
> > .p2_slow = 14, .p2_fast = 14
> > },
> > - .find_pll = intel_g4x_find_best_PLL,
> > };
> >
> > static const intel_limit_t intel_limits_g4x_dual_channel_lvds = {
> > @@ -233,7 +191,6 @@ static const intel_limit_t intel_limits_g4x_dual_channel_lvds = {
> > .p2 = { .dot_limit = 0,
> > .p2_slow = 7, .p2_fast = 7
> > },
> > - .find_pll = intel_g4x_find_best_PLL,
> > };
> >
> > static const intel_limit_t intel_limits_pineview_sdvo = {
> > @@ -249,7 +206,6 @@ 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_pnv_find_best_PLL,
> > };
> >
> > static const intel_limit_t intel_limits_pineview_lvds = {
> > @@ -263,7 +219,6 @@ 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_pnv_find_best_PLL,
> > };
> >
> > /* Ironlake / Sandybridge
> > @@ -282,7 +237,6 @@ static const intel_limit_t intel_limits_ironlake_dac = {
> > .p1 = { .min = 1, .max = 8 },
> > .p2 = { .dot_limit = 225000,
> > .p2_slow = 10, .p2_fast = 5 },
> > - .find_pll = intel_g4x_find_best_PLL,
> > };
> >
> > static const intel_limit_t intel_limits_ironlake_single_lvds = {
> > @@ -296,7 +250,6 @@ static const intel_limit_t intel_limits_ironlake_single_lvds = {
> > .p1 = { .min = 2, .max = 8 },
> > .p2 = { .dot_limit = 225000,
> > .p2_slow = 14, .p2_fast = 14 },
> > - .find_pll = intel_g4x_find_best_PLL,
> > };
> >
> > static const intel_limit_t intel_limits_ironlake_dual_lvds = {
> > @@ -310,7 +263,6 @@ static const intel_limit_t intel_limits_ironlake_dual_lvds = {
> > .p1 = { .min = 2, .max = 8 },
> > .p2 = { .dot_limit = 225000,
> > .p2_slow = 7, .p2_fast = 7 },
> > - .find_pll = intel_g4x_find_best_PLL,
> > };
> >
> > /* LVDS 100mhz refclk limits. */
> > @@ -325,7 +277,6 @@ static const intel_limit_t intel_limits_ironlake_single_lvds_100m = {
> > .p1 = { .min = 2, .max = 8 },
> > .p2 = { .dot_limit = 225000,
> > .p2_slow = 14, .p2_fast = 14 },
> > - .find_pll = intel_g4x_find_best_PLL,
> > };
> >
> > static const intel_limit_t intel_limits_ironlake_dual_lvds_100m = {
> > @@ -339,7 +290,6 @@ static const intel_limit_t intel_limits_ironlake_dual_lvds_100m = {
> > .p1 = { .min = 2, .max = 6 },
> > .p2 = { .dot_limit = 225000,
> > .p2_slow = 7, .p2_fast = 7 },
> > - .find_pll = intel_g4x_find_best_PLL,
> > };
> >
> > static const intel_limit_t intel_limits_vlv_dac = {
> > @@ -353,7 +303,6 @@ static const intel_limit_t intel_limits_vlv_dac = {
> > .p1 = { .min = 1, .max = 3 },
> > .p2 = { .dot_limit = 270000,
> > .p2_slow = 2, .p2_fast = 20 },
> > - .find_pll = intel_vlv_find_best_pll,
> > };
> >
> > static const intel_limit_t intel_limits_vlv_hdmi = {
> > @@ -367,7 +316,6 @@ static const intel_limit_t intel_limits_vlv_hdmi = {
> > .p1 = { .min = 2, .max = 3 },
> > .p2 = { .dot_limit = 270000,
> > .p2_slow = 2, .p2_fast = 20 },
> > - .find_pll = intel_vlv_find_best_pll,
> > };
> >
> > static const intel_limit_t intel_limits_vlv_dp = {
> > @@ -381,7 +329,6 @@ static const intel_limit_t intel_limits_vlv_dp = {
> > .p1 = { .min = 1, .max = 3 },
> > .p2 = { .dot_limit = 270000,
> > .p2_slow = 2, .p2_fast = 20 },
> > - .find_pll = intel_vlv_find_best_pll,
> > };
> >
> > static const intel_limit_t *intel_ironlake_limit(struct drm_crtc *crtc,
> > @@ -537,7 +484,7 @@ static bool intel_PLL_is_valid(struct drm_device *dev,
> > }
> >
> > static bool
> > -intel_find_best_PLL(const intel_limit_t *limit, struct drm_crtc *crtc,
> > +i9xx_find_best_dpll(const intel_limit_t *limit, struct drm_crtc *crtc,
> > int target, int refclk, intel_clock_t *match_clock,
> > intel_clock_t *best_clock)
> > {
> > @@ -599,9 +546,9 @@ intel_find_best_PLL(const intel_limit_t *limit, struct drm_crtc *crtc,
> > }
> >
> > 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)
> > +pnv_find_best_dpll(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;
> > @@ -661,9 +608,9 @@ intel_pnv_find_best_PLL(const intel_limit_t *limit, struct drm_crtc *crtc,
> > }
> >
> > 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)
> > +g4x_find_best_dpll(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;
> > @@ -718,9 +665,9 @@ intel_g4x_find_best_PLL(const intel_limit_t *limit, struct drm_crtc *crtc,
> > }
> >
> > 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,
> > - intel_clock_t *best_clock)
> > +vlv_find_best_dpll(const intel_limit_t *limit, struct drm_crtc *crtc,
> > + int target, int refclk, intel_clock_t *match_clock,
> > + intel_clock_t *best_clock)
> > {
> > u32 p1, p2, m1, m2, vco, bestn, bestm1, bestm2, bestp1, bestp2;
> > u32 m, n, fastclk;
> > @@ -4910,8 +4857,8 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
> > * reflck * (5 * (m1 + 2) + (m2 + 2)) / (n + 2) / p1 / p2.
> > */
> > limit = intel_limit(crtc, refclk);
> > - ok = limit->find_pll(limit, crtc, adjusted_mode->clock, refclk, NULL,
> > - &clock);
> > + ok = dev_priv->display.find_dpll(limit, crtc, adjusted_mode->clock,
> > + refclk, NULL, &clock);
> > if (!ok) {
> > DRM_ERROR("Couldn't find PLL settings for mode!\n");
> > return -EINVAL;
> > @@ -4927,10 +4874,10 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
> > * by using the FP0/FP1. In such case we will disable the LVDS
> > * downclock feature.
> > */
> > - has_reduced_clock = limit->find_pll(limit, crtc,
> > + has_reduced_clock =
> > + dev_priv->display.find_dpll(limit, crtc,
> > dev_priv->lvds_downclock,
> > - refclk,
> > - &clock,
> > + refclk, &clock,
> > &reduced_clock);
> > }
> > /* Compat-code for transition, will disappear. */
> > @@ -5546,8 +5493,8 @@ static bool ironlake_compute_clocks(struct drm_crtc *crtc,
> > * reflck * (5 * (m1 + 2) + (m2 + 2)) / (n + 2) / p1 / p2.
> > */
> > limit = intel_limit(crtc, refclk);
> > - ret = limit->find_pll(limit, crtc, adjusted_mode->clock, refclk, NULL,
> > - clock);
> > + ret = dev_priv->display.find_dpll(limit, crtc, adjusted_mode->clock,
> > + refclk, NULL, clock);
> > if (!ret)
> > return false;
> >
> > @@ -5558,11 +5505,11 @@ static bool ironlake_compute_clocks(struct drm_crtc *crtc,
> > * by using the FP0/FP1. In such case we will disable the LVDS
> > * downclock feature.
> > */
> > - *has_reduced_clock = limit->find_pll(limit, crtc,
> > - dev_priv->lvds_downclock,
> > - refclk,
> > - clock,
> > - reduced_clock);
> > + *has_reduced_clock =
> > + dev_priv->display.find_dpll(limit, crtc,
> > + dev_priv->lvds_downclock,
> > + refclk, clock,
> > + reduced_clock);
> > }
> >
> > return true;
> > @@ -9073,6 +9020,15 @@ static void intel_init_display(struct drm_device *dev)
> > {
> > struct drm_i915_private *dev_priv = dev->dev_private;
> >
> > + if (HAS_PCH_SPLIT(dev) || IS_G4X(dev))
> > + dev_priv->display.find_dpll = g4x_find_best_dpll;
>
> Doesn't look correct. If you look at intel_g4x_limit, you'll see that
> it assigns intel_limits_i9xx_sdvo for the "!lvds && !hdmi && !analog
> && !sdvo" case. And intel_limits_i9xx_sdvo uses intel_find_best_PLL,
> not intel_g4x_find_best_PLL.
Luckily that case doesn't exist, since native TV out and DP have their
fixed set of pre-tuned dpll values. And there are no other output encoder
types. I'll amend the commit message a bit that this is just dead code.
>
>
> > + else if (IS_VALLEYVIEW(dev))
> > + dev_priv->display.find_dpll = vlv_find_best_dpll;
> > + else if (IS_PINEVIEW(dev))
> > + dev_priv->display.find_dpll = pnv_find_best_dpll;
> > + else
> > + dev_priv->display.find_dpll = i9xx_find_best_dpll;
> > +
> > if (HAS_DDI(dev)) {
> > dev_priv->display.get_pipe_config = haswell_get_pipe_config;
> > dev_priv->display.crtc_mode_set = haswell_crtc_mode_set;
> > --
> > 1.7.11.7
> >
>
>
>
> --
> Paulo Zanoni
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 28+ messages in thread* [PATCH 1/2] drm/i915: move find_pll callback to dev_priv->display
2013-06-03 14:35 ` Paulo Zanoni
2013-06-03 18:36 ` Daniel Vetter
@ 2013-06-03 18:56 ` Daniel Vetter
2013-06-03 18:56 ` [PATCH 2/2] drm/i915: fold in IS_PNV checks from the split up find_dpll functions Daniel Vetter
2013-06-03 19:22 ` [PATCH] drm/i915: move find_pll callback to dev_priv->display Daniel Vetter
1 sibling, 2 replies; 28+ messages in thread
From: Daniel Vetter @ 2013-06-03 18:56 UTC (permalink / raw)
To: Intel Graphics Development; +Cc: Daniel Vetter
Now that the DP madness is cleared out, this is all only per-platform.
So move it out from the intel clock limits structure.
While at it drop the intel prefix on the static functions, call the
vtable entry find_dpll (since it's for the display pll) and rip out
the now unnecessary forward declarations.
Note that the parameters of ->find_dpll are still unchanged, but they
eventually need to be moved over to just take in a pipe configuration.
But currently a lot of things are still missing from the pipe
configuration (reflock, output-specific dpll limits and preferences,
downclocked dotclock). So this will happen in a later step.
Note that intel_g4x_limit has a peculiar case where it selects
intel_limits_i9xx_sdvo as the limit. This is pretty bogus and also not
used since the only output types left are DP and native TV-out which
both use special pre-tuned dpll values.
v2: Re-add comment for the find_pll callback (requested by Paulo) and
elaborate on why the transformation is correct for g4x platforms (to
clarify a review question from Paulo). Double up on that by adding a
WARN as suggested by Paulo Zanoni on irc.
v3: Initialize limits to NULL since gcc is now unhappy.
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
drivers/gpu/drm/i915/i915_drv.h | 20 +++++++
drivers/gpu/drm/i915/intel_display.c | 112 +++++++++++------------------------
2 files changed, 54 insertions(+), 78 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 359a200..6e104ef 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -306,6 +306,8 @@ struct drm_i915_error_state {
struct intel_crtc_config;
struct intel_crtc;
+struct intel_limit;
+struct dpll;
struct drm_i915_display_funcs {
bool (*fbc_enabled)(struct drm_device *dev);
@@ -313,6 +315,24 @@ struct drm_i915_display_funcs {
void (*disable_fbc)(struct drm_device *dev);
int (*get_display_clock_speed)(struct drm_device *dev);
int (*get_fifo_size)(struct drm_device *dev, int plane);
+ /**
+ * find_dpll() - Find the best values for the PLL
+ * @limit: limits for the PLL
+ * @crtc: current CRTC
+ * @target: target frequency in kHz
+ * @refclk: reference clock frequency in kHz
+ * @match_clock: if provided, @best_clock P divider must
+ * match the P divider from @match_clock
+ * used for LVDS downclocking
+ * @best_clock: best PLL values found
+ *
+ * Returns true on success, false on failure.
+ */
+ bool (*find_dpll)(const struct intel_limit *limit,
+ struct drm_crtc *crtc,
+ int target, int refclk,
+ struct dpll *match_clock,
+ struct dpll *best_clock);
void (*update_wm)(struct drm_device *dev);
void (*update_sprite_wm)(struct drm_device *dev, int pipe,
uint32_t sprite_width, int pixel_size,
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 7b0b16b..874441e 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -59,24 +59,6 @@ typedef struct intel_limit intel_limit_t;
struct intel_limit {
intel_range_t dot, vco, n, m, m1, m2, p, p1;
intel_p2_t p2;
- /**
- * find_pll() - Find the best values for the PLL
- * @limit: limits for the PLL
- * @crtc: current CRTC
- * @target: target frequency in kHz
- * @refclk: reference clock frequency in kHz
- * @match_clock: if provided, @best_clock P divider must
- * match the P divider from @match_clock
- * used for LVDS downclocking
- * @best_clock: best PLL values found
- *
- * Returns true on success, false on failure.
- */
- bool (*find_pll)(const intel_limit_t *limit,
- struct drm_crtc *crtc,
- int target, int refclk,
- intel_clock_t *match_clock,
- intel_clock_t *best_clock);
};
/* FDI */
@@ -92,23 +74,6 @@ intel_pch_rawclk(struct drm_device *dev)
return I915_READ(PCH_RAWCLK_FREQ) & RAWCLK_FREQ_MASK;
}
-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);
-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,
- intel_clock_t *best_clock);
-
static inline u32 /* units of 100MHz */
intel_fdi_link_freq(struct drm_device *dev)
{
@@ -130,7 +95,6 @@ static const intel_limit_t intel_limits_i8xx_dvo = {
.p1 = { .min = 2, .max = 33 },
.p2 = { .dot_limit = 165000,
.p2_slow = 4, .p2_fast = 2 },
- .find_pll = intel_find_best_PLL,
};
static const intel_limit_t intel_limits_i8xx_lvds = {
@@ -144,7 +108,6 @@ static const intel_limit_t intel_limits_i8xx_lvds = {
.p1 = { .min = 1, .max = 6 },
.p2 = { .dot_limit = 165000,
.p2_slow = 14, .p2_fast = 7 },
- .find_pll = intel_find_best_PLL,
};
static const intel_limit_t intel_limits_i9xx_sdvo = {
@@ -158,7 +121,6 @@ static const intel_limit_t intel_limits_i9xx_sdvo = {
.p1 = { .min = 1, .max = 8 },
.p2 = { .dot_limit = 200000,
.p2_slow = 10, .p2_fast = 5 },
- .find_pll = intel_find_best_PLL,
};
static const intel_limit_t intel_limits_i9xx_lvds = {
@@ -172,7 +134,6 @@ static const intel_limit_t intel_limits_i9xx_lvds = {
.p1 = { .min = 1, .max = 8 },
.p2 = { .dot_limit = 112000,
.p2_slow = 14, .p2_fast = 7 },
- .find_pll = intel_find_best_PLL,
};
@@ -189,7 +150,6 @@ static const intel_limit_t intel_limits_g4x_sdvo = {
.p2_slow = 10,
.p2_fast = 10
},
- .find_pll = intel_g4x_find_best_PLL,
};
static const intel_limit_t intel_limits_g4x_hdmi = {
@@ -203,7 +163,6 @@ static const intel_limit_t intel_limits_g4x_hdmi = {
.p1 = { .min = 1, .max = 8},
.p2 = { .dot_limit = 165000,
.p2_slow = 10, .p2_fast = 5 },
- .find_pll = intel_g4x_find_best_PLL,
};
static const intel_limit_t intel_limits_g4x_single_channel_lvds = {
@@ -218,7 +177,6 @@ static const intel_limit_t intel_limits_g4x_single_channel_lvds = {
.p2 = { .dot_limit = 0,
.p2_slow = 14, .p2_fast = 14
},
- .find_pll = intel_g4x_find_best_PLL,
};
static const intel_limit_t intel_limits_g4x_dual_channel_lvds = {
@@ -233,7 +191,6 @@ static const intel_limit_t intel_limits_g4x_dual_channel_lvds = {
.p2 = { .dot_limit = 0,
.p2_slow = 7, .p2_fast = 7
},
- .find_pll = intel_g4x_find_best_PLL,
};
static const intel_limit_t intel_limits_pineview_sdvo = {
@@ -249,7 +206,6 @@ 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_pnv_find_best_PLL,
};
static const intel_limit_t intel_limits_pineview_lvds = {
@@ -263,7 +219,6 @@ 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_pnv_find_best_PLL,
};
/* Ironlake / Sandybridge
@@ -282,7 +237,6 @@ static const intel_limit_t intel_limits_ironlake_dac = {
.p1 = { .min = 1, .max = 8 },
.p2 = { .dot_limit = 225000,
.p2_slow = 10, .p2_fast = 5 },
- .find_pll = intel_g4x_find_best_PLL,
};
static const intel_limit_t intel_limits_ironlake_single_lvds = {
@@ -296,7 +250,6 @@ static const intel_limit_t intel_limits_ironlake_single_lvds = {
.p1 = { .min = 2, .max = 8 },
.p2 = { .dot_limit = 225000,
.p2_slow = 14, .p2_fast = 14 },
- .find_pll = intel_g4x_find_best_PLL,
};
static const intel_limit_t intel_limits_ironlake_dual_lvds = {
@@ -310,7 +263,6 @@ static const intel_limit_t intel_limits_ironlake_dual_lvds = {
.p1 = { .min = 2, .max = 8 },
.p2 = { .dot_limit = 225000,
.p2_slow = 7, .p2_fast = 7 },
- .find_pll = intel_g4x_find_best_PLL,
};
/* LVDS 100mhz refclk limits. */
@@ -325,7 +277,6 @@ static const intel_limit_t intel_limits_ironlake_single_lvds_100m = {
.p1 = { .min = 2, .max = 8 },
.p2 = { .dot_limit = 225000,
.p2_slow = 14, .p2_fast = 14 },
- .find_pll = intel_g4x_find_best_PLL,
};
static const intel_limit_t intel_limits_ironlake_dual_lvds_100m = {
@@ -339,7 +290,6 @@ static const intel_limit_t intel_limits_ironlake_dual_lvds_100m = {
.p1 = { .min = 2, .max = 6 },
.p2 = { .dot_limit = 225000,
.p2_slow = 7, .p2_fast = 7 },
- .find_pll = intel_g4x_find_best_PLL,
};
static const intel_limit_t intel_limits_vlv_dac = {
@@ -353,7 +303,6 @@ static const intel_limit_t intel_limits_vlv_dac = {
.p1 = { .min = 1, .max = 3 },
.p2 = { .dot_limit = 270000,
.p2_slow = 2, .p2_fast = 20 },
- .find_pll = intel_vlv_find_best_pll,
};
static const intel_limit_t intel_limits_vlv_hdmi = {
@@ -367,7 +316,6 @@ static const intel_limit_t intel_limits_vlv_hdmi = {
.p1 = { .min = 2, .max = 3 },
.p2 = { .dot_limit = 270000,
.p2_slow = 2, .p2_fast = 20 },
- .find_pll = intel_vlv_find_best_pll,
};
static const intel_limit_t intel_limits_vlv_dp = {
@@ -381,7 +329,6 @@ static const intel_limit_t intel_limits_vlv_dp = {
.p1 = { .min = 1, .max = 3 },
.p2 = { .dot_limit = 270000,
.p2_slow = 2, .p2_fast = 20 },
- .find_pll = intel_vlv_find_best_pll,
};
static const intel_limit_t *intel_ironlake_limit(struct drm_crtc *crtc,
@@ -411,7 +358,7 @@ static const intel_limit_t *intel_ironlake_limit(struct drm_crtc *crtc,
static const intel_limit_t *intel_g4x_limit(struct drm_crtc *crtc)
{
struct drm_device *dev = crtc->dev;
- const intel_limit_t *limit;
+ const intel_limit_t *limit = NULL;
if (intel_pipe_has_type(crtc, INTEL_OUTPUT_LVDS)) {
if (intel_is_dual_link_lvds(dev))
@@ -423,8 +370,8 @@ static const intel_limit_t *intel_g4x_limit(struct drm_crtc *crtc)
limit = &intel_limits_g4x_hdmi;
} else if (intel_pipe_has_type(crtc, INTEL_OUTPUT_SDVO)) {
limit = &intel_limits_g4x_sdvo;
- } else /* The option is for other outputs */
- limit = &intel_limits_i9xx_sdvo;
+ } else
+ WARN_ON(1);
return limit;
}
@@ -537,7 +484,7 @@ static bool intel_PLL_is_valid(struct drm_device *dev,
}
static bool
-intel_find_best_PLL(const intel_limit_t *limit, struct drm_crtc *crtc,
+i9xx_find_best_dpll(const intel_limit_t *limit, struct drm_crtc *crtc,
int target, int refclk, intel_clock_t *match_clock,
intel_clock_t *best_clock)
{
@@ -599,9 +546,9 @@ intel_find_best_PLL(const intel_limit_t *limit, struct drm_crtc *crtc,
}
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)
+pnv_find_best_dpll(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;
@@ -661,9 +608,9 @@ intel_pnv_find_best_PLL(const intel_limit_t *limit, struct drm_crtc *crtc,
}
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)
+g4x_find_best_dpll(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;
@@ -718,9 +665,9 @@ intel_g4x_find_best_PLL(const intel_limit_t *limit, struct drm_crtc *crtc,
}
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,
- intel_clock_t *best_clock)
+vlv_find_best_dpll(const intel_limit_t *limit, struct drm_crtc *crtc,
+ int target, int refclk, intel_clock_t *match_clock,
+ intel_clock_t *best_clock)
{
u32 p1, p2, m1, m2, vco, bestn, bestm1, bestm2, bestp1, bestp2;
u32 m, n, fastclk;
@@ -4910,8 +4857,8 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
* reflck * (5 * (m1 + 2) + (m2 + 2)) / (n + 2) / p1 / p2.
*/
limit = intel_limit(crtc, refclk);
- ok = limit->find_pll(limit, crtc, adjusted_mode->clock, refclk, NULL,
- &clock);
+ ok = dev_priv->display.find_dpll(limit, crtc, adjusted_mode->clock,
+ refclk, NULL, &clock);
if (!ok) {
DRM_ERROR("Couldn't find PLL settings for mode!\n");
return -EINVAL;
@@ -4927,10 +4874,10 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
* by using the FP0/FP1. In such case we will disable the LVDS
* downclock feature.
*/
- has_reduced_clock = limit->find_pll(limit, crtc,
+ has_reduced_clock =
+ dev_priv->display.find_dpll(limit, crtc,
dev_priv->lvds_downclock,
- refclk,
- &clock,
+ refclk, &clock,
&reduced_clock);
}
/* Compat-code for transition, will disappear. */
@@ -5546,8 +5493,8 @@ static bool ironlake_compute_clocks(struct drm_crtc *crtc,
* reflck * (5 * (m1 + 2) + (m2 + 2)) / (n + 2) / p1 / p2.
*/
limit = intel_limit(crtc, refclk);
- ret = limit->find_pll(limit, crtc, adjusted_mode->clock, refclk, NULL,
- clock);
+ ret = dev_priv->display.find_dpll(limit, crtc, adjusted_mode->clock,
+ refclk, NULL, clock);
if (!ret)
return false;
@@ -5558,11 +5505,11 @@ static bool ironlake_compute_clocks(struct drm_crtc *crtc,
* by using the FP0/FP1. In such case we will disable the LVDS
* downclock feature.
*/
- *has_reduced_clock = limit->find_pll(limit, crtc,
- dev_priv->lvds_downclock,
- refclk,
- clock,
- reduced_clock);
+ *has_reduced_clock =
+ dev_priv->display.find_dpll(limit, crtc,
+ dev_priv->lvds_downclock,
+ refclk, clock,
+ reduced_clock);
}
return true;
@@ -9073,6 +9020,15 @@ static void intel_init_display(struct drm_device *dev)
{
struct drm_i915_private *dev_priv = dev->dev_private;
+ if (HAS_PCH_SPLIT(dev) || IS_G4X(dev))
+ dev_priv->display.find_dpll = g4x_find_best_dpll;
+ else if (IS_VALLEYVIEW(dev))
+ dev_priv->display.find_dpll = vlv_find_best_dpll;
+ else if (IS_PINEVIEW(dev))
+ dev_priv->display.find_dpll = pnv_find_best_dpll;
+ else
+ dev_priv->display.find_dpll = i9xx_find_best_dpll;
+
if (HAS_DDI(dev)) {
dev_priv->display.get_pipe_config = haswell_get_pipe_config;
dev_priv->display.crtc_mode_set = haswell_crtc_mode_set;
--
1.7.11.7
^ permalink raw reply related [flat|nested] 28+ messages in thread* [PATCH 2/2] drm/i915: fold in IS_PNV checks from the split up find_dpll functions
2013-06-03 18:56 ` [PATCH 1/2] " Daniel Vetter
@ 2013-06-03 18:56 ` Daniel Vetter
2013-06-03 21:28 ` Paulo Zanoni
2013-06-03 19:22 ` [PATCH] drm/i915: move find_pll callback to dev_priv->display Daniel Vetter
1 sibling, 1 reply; 28+ messages in thread
From: Daniel Vetter @ 2013-06-03 18:56 UTC (permalink / raw)
To: Intel Graphics Development; +Cc: Daniel Vetter
Since I stand by my rule that splitting functions should only do an
exact copy, this is a follow-up patch.
Suggested-by: Paulo Zanoni <przanoni@gmail.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
drivers/gpu/drm/i915/intel_display.c | 6 +-----
1 file changed, 1 insertion(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 874441e..a1f17d7 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -515,8 +515,7 @@ i9xx_find_best_dpll(const intel_limit_t *limit, struct drm_crtc *crtc,
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))
+ if (clock.m2 >= clock.m1)
break;
for (clock.n = limit->n.min;
clock.n <= limit->n.max; clock.n++) {
@@ -577,9 +576,6 @@ pnv_find_best_dpll(const intel_limit_t *limit, struct drm_crtc *crtc,
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;
for (clock.n = limit->n.min;
clock.n <= limit->n.max; clock.n++) {
for (clock.p1 = limit->p1.min;
--
1.7.11.7
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [PATCH 2/2] drm/i915: fold in IS_PNV checks from the split up find_dpll functions
2013-06-03 18:56 ` [PATCH 2/2] drm/i915: fold in IS_PNV checks from the split up find_dpll functions Daniel Vetter
@ 2013-06-03 21:28 ` Paulo Zanoni
0 siblings, 0 replies; 28+ messages in thread
From: Paulo Zanoni @ 2013-06-03 21:28 UTC (permalink / raw)
To: Daniel Vetter; +Cc: Intel Graphics Development
2013/6/3 Daniel Vetter <daniel.vetter@ffwll.ch>:
> Since I stand by my rule that splitting functions should only do an
> exact copy, this is a follow-up patch.
>
> Suggested-by: Paulo Zanoni <przanoni@gmail.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
> drivers/gpu/drm/i915/intel_display.c | 6 +-----
> 1 file changed, 1 insertion(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 874441e..a1f17d7 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -515,8 +515,7 @@ i9xx_find_best_dpll(const intel_limit_t *limit, struct drm_crtc *crtc,
> 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))
> + if (clock.m2 >= clock.m1)
> break;
> for (clock.n = limit->n.min;
> clock.n <= limit->n.max; clock.n++) {
> @@ -577,9 +576,6 @@ pnv_find_best_dpll(const intel_limit_t *limit, struct drm_crtc *crtc,
> 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;
> for (clock.n = limit->n.min;
> clock.n <= limit->n.max; clock.n++) {
> for (clock.p1 = limit->p1.min;
> --
> 1.7.11.7
>
--
Paulo Zanoni
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH] drm/i915: move find_pll callback to dev_priv->display
2013-06-03 18:56 ` [PATCH 1/2] " Daniel Vetter
2013-06-03 18:56 ` [PATCH 2/2] drm/i915: fold in IS_PNV checks from the split up find_dpll functions Daniel Vetter
@ 2013-06-03 19:22 ` Daniel Vetter
2013-06-03 20:40 ` Daniel Vetter
1 sibling, 1 reply; 28+ messages in thread
From: Daniel Vetter @ 2013-06-03 19:22 UTC (permalink / raw)
To: Intel Graphics Development; +Cc: Daniel Vetter
Now that the DP madness is cleared out, this is all only per-platform.
So move it out from the intel clock limits structure.
While at it drop the intel prefix on the static functions, call the
vtable entry find_dpll (since it's for the display pll) and rip out
the now unnecessary forward declarations.
Note that the parameters of ->find_dpll are still unchanged, but they
eventually need to be moved over to just take in a pipe configuration.
But currently a lot of things are still missing from the pipe
configuration (reflock, output-specific dpll limits and preferences,
downclocked dotclock). So this will happen in a later step.
Note that intel_g4x_limit has a peculiar case where it selects
intel_limits_i9xx_sdvo as the limit. This is pretty bogus and also not
used since the only output types left are DP and native TV-out which
both use special pre-tuned dpll values.
v2: Re-add comment for the find_pll callback (requested by Paulo) and
elaborate on why the transformation is correct for g4x platforms (to
clarify a review question from Paulo). Double up on that by adding a
WARN as suggested by Paulo Zanoni on irc.
v3: Initialize limits to NULL since gcc is now unhappy.
v4: v2/3 will blow up with a NULL dereference in ->find_dpll for dp and
TV-out ports, spotted by Paulo on irc. So just give up on this madness for
now, and leave this to be fixed in a later patch.
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
drivers/gpu/drm/i915/i915_drv.h | 20 +++++++
drivers/gpu/drm/i915/intel_display.c | 106 ++++++++++-------------------------
2 files changed, 51 insertions(+), 75 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 359a200..6e104ef 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -306,6 +306,8 @@ struct drm_i915_error_state {
struct intel_crtc_config;
struct intel_crtc;
+struct intel_limit;
+struct dpll;
struct drm_i915_display_funcs {
bool (*fbc_enabled)(struct drm_device *dev);
@@ -313,6 +315,24 @@ struct drm_i915_display_funcs {
void (*disable_fbc)(struct drm_device *dev);
int (*get_display_clock_speed)(struct drm_device *dev);
int (*get_fifo_size)(struct drm_device *dev, int plane);
+ /**
+ * find_dpll() - Find the best values for the PLL
+ * @limit: limits for the PLL
+ * @crtc: current CRTC
+ * @target: target frequency in kHz
+ * @refclk: reference clock frequency in kHz
+ * @match_clock: if provided, @best_clock P divider must
+ * match the P divider from @match_clock
+ * used for LVDS downclocking
+ * @best_clock: best PLL values found
+ *
+ * Returns true on success, false on failure.
+ */
+ bool (*find_dpll)(const struct intel_limit *limit,
+ struct drm_crtc *crtc,
+ int target, int refclk,
+ struct dpll *match_clock,
+ struct dpll *best_clock);
void (*update_wm)(struct drm_device *dev);
void (*update_sprite_wm)(struct drm_device *dev, int pipe,
uint32_t sprite_width, int pixel_size,
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 7b0b16b..a29295e 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -59,24 +59,6 @@ typedef struct intel_limit intel_limit_t;
struct intel_limit {
intel_range_t dot, vco, n, m, m1, m2, p, p1;
intel_p2_t p2;
- /**
- * find_pll() - Find the best values for the PLL
- * @limit: limits for the PLL
- * @crtc: current CRTC
- * @target: target frequency in kHz
- * @refclk: reference clock frequency in kHz
- * @match_clock: if provided, @best_clock P divider must
- * match the P divider from @match_clock
- * used for LVDS downclocking
- * @best_clock: best PLL values found
- *
- * Returns true on success, false on failure.
- */
- bool (*find_pll)(const intel_limit_t *limit,
- struct drm_crtc *crtc,
- int target, int refclk,
- intel_clock_t *match_clock,
- intel_clock_t *best_clock);
};
/* FDI */
@@ -92,23 +74,6 @@ intel_pch_rawclk(struct drm_device *dev)
return I915_READ(PCH_RAWCLK_FREQ) & RAWCLK_FREQ_MASK;
}
-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);
-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,
- intel_clock_t *best_clock);
-
static inline u32 /* units of 100MHz */
intel_fdi_link_freq(struct drm_device *dev)
{
@@ -130,7 +95,6 @@ static const intel_limit_t intel_limits_i8xx_dvo = {
.p1 = { .min = 2, .max = 33 },
.p2 = { .dot_limit = 165000,
.p2_slow = 4, .p2_fast = 2 },
- .find_pll = intel_find_best_PLL,
};
static const intel_limit_t intel_limits_i8xx_lvds = {
@@ -144,7 +108,6 @@ static const intel_limit_t intel_limits_i8xx_lvds = {
.p1 = { .min = 1, .max = 6 },
.p2 = { .dot_limit = 165000,
.p2_slow = 14, .p2_fast = 7 },
- .find_pll = intel_find_best_PLL,
};
static const intel_limit_t intel_limits_i9xx_sdvo = {
@@ -158,7 +121,6 @@ static const intel_limit_t intel_limits_i9xx_sdvo = {
.p1 = { .min = 1, .max = 8 },
.p2 = { .dot_limit = 200000,
.p2_slow = 10, .p2_fast = 5 },
- .find_pll = intel_find_best_PLL,
};
static const intel_limit_t intel_limits_i9xx_lvds = {
@@ -172,7 +134,6 @@ static const intel_limit_t intel_limits_i9xx_lvds = {
.p1 = { .min = 1, .max = 8 },
.p2 = { .dot_limit = 112000,
.p2_slow = 14, .p2_fast = 7 },
- .find_pll = intel_find_best_PLL,
};
@@ -189,7 +150,6 @@ static const intel_limit_t intel_limits_g4x_sdvo = {
.p2_slow = 10,
.p2_fast = 10
},
- .find_pll = intel_g4x_find_best_PLL,
};
static const intel_limit_t intel_limits_g4x_hdmi = {
@@ -203,7 +163,6 @@ static const intel_limit_t intel_limits_g4x_hdmi = {
.p1 = { .min = 1, .max = 8},
.p2 = { .dot_limit = 165000,
.p2_slow = 10, .p2_fast = 5 },
- .find_pll = intel_g4x_find_best_PLL,
};
static const intel_limit_t intel_limits_g4x_single_channel_lvds = {
@@ -218,7 +177,6 @@ static const intel_limit_t intel_limits_g4x_single_channel_lvds = {
.p2 = { .dot_limit = 0,
.p2_slow = 14, .p2_fast = 14
},
- .find_pll = intel_g4x_find_best_PLL,
};
static const intel_limit_t intel_limits_g4x_dual_channel_lvds = {
@@ -233,7 +191,6 @@ static const intel_limit_t intel_limits_g4x_dual_channel_lvds = {
.p2 = { .dot_limit = 0,
.p2_slow = 7, .p2_fast = 7
},
- .find_pll = intel_g4x_find_best_PLL,
};
static const intel_limit_t intel_limits_pineview_sdvo = {
@@ -249,7 +206,6 @@ 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_pnv_find_best_PLL,
};
static const intel_limit_t intel_limits_pineview_lvds = {
@@ -263,7 +219,6 @@ 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_pnv_find_best_PLL,
};
/* Ironlake / Sandybridge
@@ -282,7 +237,6 @@ static const intel_limit_t intel_limits_ironlake_dac = {
.p1 = { .min = 1, .max = 8 },
.p2 = { .dot_limit = 225000,
.p2_slow = 10, .p2_fast = 5 },
- .find_pll = intel_g4x_find_best_PLL,
};
static const intel_limit_t intel_limits_ironlake_single_lvds = {
@@ -296,7 +250,6 @@ static const intel_limit_t intel_limits_ironlake_single_lvds = {
.p1 = { .min = 2, .max = 8 },
.p2 = { .dot_limit = 225000,
.p2_slow = 14, .p2_fast = 14 },
- .find_pll = intel_g4x_find_best_PLL,
};
static const intel_limit_t intel_limits_ironlake_dual_lvds = {
@@ -310,7 +263,6 @@ static const intel_limit_t intel_limits_ironlake_dual_lvds = {
.p1 = { .min = 2, .max = 8 },
.p2 = { .dot_limit = 225000,
.p2_slow = 7, .p2_fast = 7 },
- .find_pll = intel_g4x_find_best_PLL,
};
/* LVDS 100mhz refclk limits. */
@@ -325,7 +277,6 @@ static const intel_limit_t intel_limits_ironlake_single_lvds_100m = {
.p1 = { .min = 2, .max = 8 },
.p2 = { .dot_limit = 225000,
.p2_slow = 14, .p2_fast = 14 },
- .find_pll = intel_g4x_find_best_PLL,
};
static const intel_limit_t intel_limits_ironlake_dual_lvds_100m = {
@@ -339,7 +290,6 @@ static const intel_limit_t intel_limits_ironlake_dual_lvds_100m = {
.p1 = { .min = 2, .max = 6 },
.p2 = { .dot_limit = 225000,
.p2_slow = 7, .p2_fast = 7 },
- .find_pll = intel_g4x_find_best_PLL,
};
static const intel_limit_t intel_limits_vlv_dac = {
@@ -353,7 +303,6 @@ static const intel_limit_t intel_limits_vlv_dac = {
.p1 = { .min = 1, .max = 3 },
.p2 = { .dot_limit = 270000,
.p2_slow = 2, .p2_fast = 20 },
- .find_pll = intel_vlv_find_best_pll,
};
static const intel_limit_t intel_limits_vlv_hdmi = {
@@ -367,7 +316,6 @@ static const intel_limit_t intel_limits_vlv_hdmi = {
.p1 = { .min = 2, .max = 3 },
.p2 = { .dot_limit = 270000,
.p2_slow = 2, .p2_fast = 20 },
- .find_pll = intel_vlv_find_best_pll,
};
static const intel_limit_t intel_limits_vlv_dp = {
@@ -381,7 +329,6 @@ static const intel_limit_t intel_limits_vlv_dp = {
.p1 = { .min = 1, .max = 3 },
.p2 = { .dot_limit = 270000,
.p2_slow = 2, .p2_fast = 20 },
- .find_pll = intel_vlv_find_best_pll,
};
static const intel_limit_t *intel_ironlake_limit(struct drm_crtc *crtc,
@@ -537,7 +484,7 @@ static bool intel_PLL_is_valid(struct drm_device *dev,
}
static bool
-intel_find_best_PLL(const intel_limit_t *limit, struct drm_crtc *crtc,
+i9xx_find_best_dpll(const intel_limit_t *limit, struct drm_crtc *crtc,
int target, int refclk, intel_clock_t *match_clock,
intel_clock_t *best_clock)
{
@@ -599,9 +546,9 @@ intel_find_best_PLL(const intel_limit_t *limit, struct drm_crtc *crtc,
}
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)
+pnv_find_best_dpll(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;
@@ -661,9 +608,9 @@ intel_pnv_find_best_PLL(const intel_limit_t *limit, struct drm_crtc *crtc,
}
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)
+g4x_find_best_dpll(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;
@@ -718,9 +665,9 @@ intel_g4x_find_best_PLL(const intel_limit_t *limit, struct drm_crtc *crtc,
}
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,
- intel_clock_t *best_clock)
+vlv_find_best_dpll(const intel_limit_t *limit, struct drm_crtc *crtc,
+ int target, int refclk, intel_clock_t *match_clock,
+ intel_clock_t *best_clock)
{
u32 p1, p2, m1, m2, vco, bestn, bestm1, bestm2, bestp1, bestp2;
u32 m, n, fastclk;
@@ -4910,8 +4857,8 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
* reflck * (5 * (m1 + 2) + (m2 + 2)) / (n + 2) / p1 / p2.
*/
limit = intel_limit(crtc, refclk);
- ok = limit->find_pll(limit, crtc, adjusted_mode->clock, refclk, NULL,
- &clock);
+ ok = dev_priv->display.find_dpll(limit, crtc, adjusted_mode->clock,
+ refclk, NULL, &clock);
if (!ok) {
DRM_ERROR("Couldn't find PLL settings for mode!\n");
return -EINVAL;
@@ -4927,10 +4874,10 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
* by using the FP0/FP1. In such case we will disable the LVDS
* downclock feature.
*/
- has_reduced_clock = limit->find_pll(limit, crtc,
+ has_reduced_clock =
+ dev_priv->display.find_dpll(limit, crtc,
dev_priv->lvds_downclock,
- refclk,
- &clock,
+ refclk, &clock,
&reduced_clock);
}
/* Compat-code for transition, will disappear. */
@@ -5546,8 +5493,8 @@ static bool ironlake_compute_clocks(struct drm_crtc *crtc,
* reflck * (5 * (m1 + 2) + (m2 + 2)) / (n + 2) / p1 / p2.
*/
limit = intel_limit(crtc, refclk);
- ret = limit->find_pll(limit, crtc, adjusted_mode->clock, refclk, NULL,
- clock);
+ ret = dev_priv->display.find_dpll(limit, crtc, adjusted_mode->clock,
+ refclk, NULL, clock);
if (!ret)
return false;
@@ -5558,11 +5505,11 @@ static bool ironlake_compute_clocks(struct drm_crtc *crtc,
* by using the FP0/FP1. In such case we will disable the LVDS
* downclock feature.
*/
- *has_reduced_clock = limit->find_pll(limit, crtc,
- dev_priv->lvds_downclock,
- refclk,
- clock,
- reduced_clock);
+ *has_reduced_clock =
+ dev_priv->display.find_dpll(limit, crtc,
+ dev_priv->lvds_downclock,
+ refclk, clock,
+ reduced_clock);
}
return true;
@@ -9073,6 +9020,15 @@ static void intel_init_display(struct drm_device *dev)
{
struct drm_i915_private *dev_priv = dev->dev_private;
+ if (HAS_PCH_SPLIT(dev) || IS_G4X(dev))
+ dev_priv->display.find_dpll = g4x_find_best_dpll;
+ else if (IS_VALLEYVIEW(dev))
+ dev_priv->display.find_dpll = vlv_find_best_dpll;
+ else if (IS_PINEVIEW(dev))
+ dev_priv->display.find_dpll = pnv_find_best_dpll;
+ else
+ dev_priv->display.find_dpll = i9xx_find_best_dpll;
+
if (HAS_DDI(dev)) {
dev_priv->display.get_pipe_config = haswell_get_pipe_config;
dev_priv->display.crtc_mode_set = haswell_crtc_mode_set;
--
1.7.11.7
^ permalink raw reply related [flat|nested] 28+ messages in thread* [PATCH] drm/i915: move find_pll callback to dev_priv->display
2013-06-03 19:22 ` [PATCH] drm/i915: move find_pll callback to dev_priv->display Daniel Vetter
@ 2013-06-03 20:40 ` Daniel Vetter
2013-06-03 21:26 ` Paulo Zanoni
0 siblings, 1 reply; 28+ messages in thread
From: Daniel Vetter @ 2013-06-03 20:40 UTC (permalink / raw)
To: Intel Graphics Development; +Cc: Daniel Vetter
Now that the DP madness is cleared out, this is all only per-platform.
So move it out from the intel clock limits structure.
While at it drop the intel prefix on the static functions, call the
vtable entry find_dpll (since it's for the display pll) and rip out
the now unnecessary forward declarations.
Note that the parameters of ->find_dpll are still unchanged, but they
eventually need to be moved over to just take in a pipe configuration.
But currently a lot of things are still missing from the pipe
configuration (reflock, output-specific dpll limits and preferences,
downclocked dotclock). So this will happen in a later step.
Note that intel_g4x_limit has a peculiar case where it selects
intel_limits_i9xx_sdvo as the limit. This is pretty bogus and also not
used since the only output types left are DP and native TV-out which
both use special pre-tuned dpll values.
v2: Re-add comment for the find_pll callback (requested by Paulo) and
elaborate on why the transformation is correct for g4x platforms (to
clarify a review question from Paulo). Double up on that by adding a
WARN as suggested by Paulo Zanoni on irc.
v3: Initialize limits to NULL since gcc is now unhappy.
v4: v2/3 will blow up with a NULL dereference in ->find_dpll for dp and
TV-out ports, spotted by Paulo on irc. So just give up on this madness for
now, and leave this to be fixed in a later patch.
v5: Since the ever-so-slight change for g4x might result in some dpll
parameter computation failing spuriously where before it didn't for
ports with preset dpll settings (DP & TV-out) override this. For
paranoia also do it in the ilk+ code.
Cc: Paulo Zanoni <przanoni@gmail.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
drivers/gpu/drm/i915/i915_drv.h | 20 +++++++
drivers/gpu/drm/i915/intel_display.c | 110 +++++++++++------------------------
2 files changed, 53 insertions(+), 77 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 359a200..6e104ef 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -306,6 +306,8 @@ struct drm_i915_error_state {
struct intel_crtc_config;
struct intel_crtc;
+struct intel_limit;
+struct dpll;
struct drm_i915_display_funcs {
bool (*fbc_enabled)(struct drm_device *dev);
@@ -313,6 +315,24 @@ struct drm_i915_display_funcs {
void (*disable_fbc)(struct drm_device *dev);
int (*get_display_clock_speed)(struct drm_device *dev);
int (*get_fifo_size)(struct drm_device *dev, int plane);
+ /**
+ * find_dpll() - Find the best values for the PLL
+ * @limit: limits for the PLL
+ * @crtc: current CRTC
+ * @target: target frequency in kHz
+ * @refclk: reference clock frequency in kHz
+ * @match_clock: if provided, @best_clock P divider must
+ * match the P divider from @match_clock
+ * used for LVDS downclocking
+ * @best_clock: best PLL values found
+ *
+ * Returns true on success, false on failure.
+ */
+ bool (*find_dpll)(const struct intel_limit *limit,
+ struct drm_crtc *crtc,
+ int target, int refclk,
+ struct dpll *match_clock,
+ struct dpll *best_clock);
void (*update_wm)(struct drm_device *dev);
void (*update_sprite_wm)(struct drm_device *dev, int pipe,
uint32_t sprite_width, int pixel_size,
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 7b0b16b..5d0f16d 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -59,24 +59,6 @@ typedef struct intel_limit intel_limit_t;
struct intel_limit {
intel_range_t dot, vco, n, m, m1, m2, p, p1;
intel_p2_t p2;
- /**
- * find_pll() - Find the best values for the PLL
- * @limit: limits for the PLL
- * @crtc: current CRTC
- * @target: target frequency in kHz
- * @refclk: reference clock frequency in kHz
- * @match_clock: if provided, @best_clock P divider must
- * match the P divider from @match_clock
- * used for LVDS downclocking
- * @best_clock: best PLL values found
- *
- * Returns true on success, false on failure.
- */
- bool (*find_pll)(const intel_limit_t *limit,
- struct drm_crtc *crtc,
- int target, int refclk,
- intel_clock_t *match_clock,
- intel_clock_t *best_clock);
};
/* FDI */
@@ -92,23 +74,6 @@ intel_pch_rawclk(struct drm_device *dev)
return I915_READ(PCH_RAWCLK_FREQ) & RAWCLK_FREQ_MASK;
}
-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);
-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,
- intel_clock_t *best_clock);
-
static inline u32 /* units of 100MHz */
intel_fdi_link_freq(struct drm_device *dev)
{
@@ -130,7 +95,6 @@ static const intel_limit_t intel_limits_i8xx_dvo = {
.p1 = { .min = 2, .max = 33 },
.p2 = { .dot_limit = 165000,
.p2_slow = 4, .p2_fast = 2 },
- .find_pll = intel_find_best_PLL,
};
static const intel_limit_t intel_limits_i8xx_lvds = {
@@ -144,7 +108,6 @@ static const intel_limit_t intel_limits_i8xx_lvds = {
.p1 = { .min = 1, .max = 6 },
.p2 = { .dot_limit = 165000,
.p2_slow = 14, .p2_fast = 7 },
- .find_pll = intel_find_best_PLL,
};
static const intel_limit_t intel_limits_i9xx_sdvo = {
@@ -158,7 +121,6 @@ static const intel_limit_t intel_limits_i9xx_sdvo = {
.p1 = { .min = 1, .max = 8 },
.p2 = { .dot_limit = 200000,
.p2_slow = 10, .p2_fast = 5 },
- .find_pll = intel_find_best_PLL,
};
static const intel_limit_t intel_limits_i9xx_lvds = {
@@ -172,7 +134,6 @@ static const intel_limit_t intel_limits_i9xx_lvds = {
.p1 = { .min = 1, .max = 8 },
.p2 = { .dot_limit = 112000,
.p2_slow = 14, .p2_fast = 7 },
- .find_pll = intel_find_best_PLL,
};
@@ -189,7 +150,6 @@ static const intel_limit_t intel_limits_g4x_sdvo = {
.p2_slow = 10,
.p2_fast = 10
},
- .find_pll = intel_g4x_find_best_PLL,
};
static const intel_limit_t intel_limits_g4x_hdmi = {
@@ -203,7 +163,6 @@ static const intel_limit_t intel_limits_g4x_hdmi = {
.p1 = { .min = 1, .max = 8},
.p2 = { .dot_limit = 165000,
.p2_slow = 10, .p2_fast = 5 },
- .find_pll = intel_g4x_find_best_PLL,
};
static const intel_limit_t intel_limits_g4x_single_channel_lvds = {
@@ -218,7 +177,6 @@ static const intel_limit_t intel_limits_g4x_single_channel_lvds = {
.p2 = { .dot_limit = 0,
.p2_slow = 14, .p2_fast = 14
},
- .find_pll = intel_g4x_find_best_PLL,
};
static const intel_limit_t intel_limits_g4x_dual_channel_lvds = {
@@ -233,7 +191,6 @@ static const intel_limit_t intel_limits_g4x_dual_channel_lvds = {
.p2 = { .dot_limit = 0,
.p2_slow = 7, .p2_fast = 7
},
- .find_pll = intel_g4x_find_best_PLL,
};
static const intel_limit_t intel_limits_pineview_sdvo = {
@@ -249,7 +206,6 @@ 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_pnv_find_best_PLL,
};
static const intel_limit_t intel_limits_pineview_lvds = {
@@ -263,7 +219,6 @@ 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_pnv_find_best_PLL,
};
/* Ironlake / Sandybridge
@@ -282,7 +237,6 @@ static const intel_limit_t intel_limits_ironlake_dac = {
.p1 = { .min = 1, .max = 8 },
.p2 = { .dot_limit = 225000,
.p2_slow = 10, .p2_fast = 5 },
- .find_pll = intel_g4x_find_best_PLL,
};
static const intel_limit_t intel_limits_ironlake_single_lvds = {
@@ -296,7 +250,6 @@ static const intel_limit_t intel_limits_ironlake_single_lvds = {
.p1 = { .min = 2, .max = 8 },
.p2 = { .dot_limit = 225000,
.p2_slow = 14, .p2_fast = 14 },
- .find_pll = intel_g4x_find_best_PLL,
};
static const intel_limit_t intel_limits_ironlake_dual_lvds = {
@@ -310,7 +263,6 @@ static const intel_limit_t intel_limits_ironlake_dual_lvds = {
.p1 = { .min = 2, .max = 8 },
.p2 = { .dot_limit = 225000,
.p2_slow = 7, .p2_fast = 7 },
- .find_pll = intel_g4x_find_best_PLL,
};
/* LVDS 100mhz refclk limits. */
@@ -325,7 +277,6 @@ static const intel_limit_t intel_limits_ironlake_single_lvds_100m = {
.p1 = { .min = 2, .max = 8 },
.p2 = { .dot_limit = 225000,
.p2_slow = 14, .p2_fast = 14 },
- .find_pll = intel_g4x_find_best_PLL,
};
static const intel_limit_t intel_limits_ironlake_dual_lvds_100m = {
@@ -339,7 +290,6 @@ static const intel_limit_t intel_limits_ironlake_dual_lvds_100m = {
.p1 = { .min = 2, .max = 6 },
.p2 = { .dot_limit = 225000,
.p2_slow = 7, .p2_fast = 7 },
- .find_pll = intel_g4x_find_best_PLL,
};
static const intel_limit_t intel_limits_vlv_dac = {
@@ -353,7 +303,6 @@ static const intel_limit_t intel_limits_vlv_dac = {
.p1 = { .min = 1, .max = 3 },
.p2 = { .dot_limit = 270000,
.p2_slow = 2, .p2_fast = 20 },
- .find_pll = intel_vlv_find_best_pll,
};
static const intel_limit_t intel_limits_vlv_hdmi = {
@@ -367,7 +316,6 @@ static const intel_limit_t intel_limits_vlv_hdmi = {
.p1 = { .min = 2, .max = 3 },
.p2 = { .dot_limit = 270000,
.p2_slow = 2, .p2_fast = 20 },
- .find_pll = intel_vlv_find_best_pll,
};
static const intel_limit_t intel_limits_vlv_dp = {
@@ -381,7 +329,6 @@ static const intel_limit_t intel_limits_vlv_dp = {
.p1 = { .min = 1, .max = 3 },
.p2 = { .dot_limit = 270000,
.p2_slow = 2, .p2_fast = 20 },
- .find_pll = intel_vlv_find_best_pll,
};
static const intel_limit_t *intel_ironlake_limit(struct drm_crtc *crtc,
@@ -537,7 +484,7 @@ static bool intel_PLL_is_valid(struct drm_device *dev,
}
static bool
-intel_find_best_PLL(const intel_limit_t *limit, struct drm_crtc *crtc,
+i9xx_find_best_dpll(const intel_limit_t *limit, struct drm_crtc *crtc,
int target, int refclk, intel_clock_t *match_clock,
intel_clock_t *best_clock)
{
@@ -599,9 +546,9 @@ intel_find_best_PLL(const intel_limit_t *limit, struct drm_crtc *crtc,
}
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)
+pnv_find_best_dpll(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;
@@ -661,9 +608,9 @@ intel_pnv_find_best_PLL(const intel_limit_t *limit, struct drm_crtc *crtc,
}
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)
+g4x_find_best_dpll(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;
@@ -718,9 +665,9 @@ intel_g4x_find_best_PLL(const intel_limit_t *limit, struct drm_crtc *crtc,
}
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,
- intel_clock_t *best_clock)
+vlv_find_best_dpll(const intel_limit_t *limit, struct drm_crtc *crtc,
+ int target, int refclk, intel_clock_t *match_clock,
+ intel_clock_t *best_clock)
{
u32 p1, p2, m1, m2, vco, bestn, bestm1, bestm2, bestp1, bestp2;
u32 m, n, fastclk;
@@ -4910,9 +4857,9 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
* reflck * (5 * (m1 + 2) + (m2 + 2)) / (n + 2) / p1 / p2.
*/
limit = intel_limit(crtc, refclk);
- ok = limit->find_pll(limit, crtc, adjusted_mode->clock, refclk, NULL,
- &clock);
- if (!ok) {
+ ok = dev_priv->display.find_dpll(limit, crtc, adjusted_mode->clock,
+ refclk, NULL, &clock);
+ if (!ok && !intel_crtc->config.clock_set) {
DRM_ERROR("Couldn't find PLL settings for mode!\n");
return -EINVAL;
}
@@ -4927,10 +4874,10 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
* by using the FP0/FP1. In such case we will disable the LVDS
* downclock feature.
*/
- has_reduced_clock = limit->find_pll(limit, crtc,
+ has_reduced_clock =
+ dev_priv->display.find_dpll(limit, crtc,
dev_priv->lvds_downclock,
- refclk,
- &clock,
+ refclk, &clock,
&reduced_clock);
}
/* Compat-code for transition, will disappear. */
@@ -5546,8 +5493,8 @@ static bool ironlake_compute_clocks(struct drm_crtc *crtc,
* reflck * (5 * (m1 + 2) + (m2 + 2)) / (n + 2) / p1 / p2.
*/
limit = intel_limit(crtc, refclk);
- ret = limit->find_pll(limit, crtc, adjusted_mode->clock, refclk, NULL,
- clock);
+ ret = dev_priv->display.find_dpll(limit, crtc, adjusted_mode->clock,
+ refclk, NULL, clock);
if (!ret)
return false;
@@ -5558,11 +5505,11 @@ static bool ironlake_compute_clocks(struct drm_crtc *crtc,
* by using the FP0/FP1. In such case we will disable the LVDS
* downclock feature.
*/
- *has_reduced_clock = limit->find_pll(limit, crtc,
- dev_priv->lvds_downclock,
- refclk,
- clock,
- reduced_clock);
+ *has_reduced_clock =
+ dev_priv->display.find_dpll(limit, crtc,
+ dev_priv->lvds_downclock,
+ refclk, clock,
+ reduced_clock);
}
return true;
@@ -5748,7 +5695,7 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
ok = ironlake_compute_clocks(crtc, adjusted_mode, &clock,
&has_reduced_clock, &reduced_clock);
- if (!ok) {
+ if (!ok && !intel_crtc->config.clock_set) {
DRM_ERROR("Couldn't find PLL settings for mode!\n");
return -EINVAL;
}
@@ -9073,6 +9020,15 @@ static void intel_init_display(struct drm_device *dev)
{
struct drm_i915_private *dev_priv = dev->dev_private;
+ if (HAS_PCH_SPLIT(dev) || IS_G4X(dev))
+ dev_priv->display.find_dpll = g4x_find_best_dpll;
+ else if (IS_VALLEYVIEW(dev))
+ dev_priv->display.find_dpll = vlv_find_best_dpll;
+ else if (IS_PINEVIEW(dev))
+ dev_priv->display.find_dpll = pnv_find_best_dpll;
+ else
+ dev_priv->display.find_dpll = i9xx_find_best_dpll;
+
if (HAS_DDI(dev)) {
dev_priv->display.get_pipe_config = haswell_get_pipe_config;
dev_priv->display.crtc_mode_set = haswell_crtc_mode_set;
--
1.7.11.7
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [PATCH] drm/i915: move find_pll callback to dev_priv->display
2013-06-03 20:40 ` Daniel Vetter
@ 2013-06-03 21:26 ` Paulo Zanoni
0 siblings, 0 replies; 28+ messages in thread
From: Paulo Zanoni @ 2013-06-03 21:26 UTC (permalink / raw)
To: Daniel Vetter; +Cc: Intel Graphics Development
2013/6/3 Daniel Vetter <daniel.vetter@ffwll.ch>:
> Now that the DP madness is cleared out, this is all only per-platform.
> So move it out from the intel clock limits structure.
>
> While at it drop the intel prefix on the static functions, call the
> vtable entry find_dpll (since it's for the display pll) and rip out
> the now unnecessary forward declarations.
>
> Note that the parameters of ->find_dpll are still unchanged, but they
> eventually need to be moved over to just take in a pipe configuration.
> But currently a lot of things are still missing from the pipe
> configuration (reflock, output-specific dpll limits and preferences,
> downclocked dotclock). So this will happen in a later step.
>
> Note that intel_g4x_limit has a peculiar case where it selects
> intel_limits_i9xx_sdvo as the limit. This is pretty bogus and also not
> used since the only output types left are DP and native TV-out which
> both use special pre-tuned dpll values.
>
> v2: Re-add comment for the find_pll callback (requested by Paulo) and
> elaborate on why the transformation is correct for g4x platforms (to
> clarify a review question from Paulo). Double up on that by adding a
> WARN as suggested by Paulo Zanoni on irc.
>
> v3: Initialize limits to NULL since gcc is now unhappy.
>
> v4: v2/3 will blow up with a NULL dereference in ->find_dpll for dp and
> TV-out ports, spotted by Paulo on irc. So just give up on this madness for
> now, and leave this to be fixed in a later patch.
>
> v5: Since the ever-so-slight change for g4x might result in some dpll
> parameter computation failing spuriously where before it didn't for
> ports with preset dpll settings (DP & TV-out) override this. For
> paranoia also do it in the ilk+ code.
>
> Cc: Paulo Zanoni <przanoni@gmail.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
> drivers/gpu/drm/i915/i915_drv.h | 20 +++++++
> drivers/gpu/drm/i915/intel_display.c | 110 +++++++++++------------------------
> 2 files changed, 53 insertions(+), 77 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 359a200..6e104ef 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -306,6 +306,8 @@ struct drm_i915_error_state {
>
> struct intel_crtc_config;
> struct intel_crtc;
> +struct intel_limit;
> +struct dpll;
>
> struct drm_i915_display_funcs {
> bool (*fbc_enabled)(struct drm_device *dev);
> @@ -313,6 +315,24 @@ struct drm_i915_display_funcs {
> void (*disable_fbc)(struct drm_device *dev);
> int (*get_display_clock_speed)(struct drm_device *dev);
> int (*get_fifo_size)(struct drm_device *dev, int plane);
> + /**
> + * find_dpll() - Find the best values for the PLL
> + * @limit: limits for the PLL
> + * @crtc: current CRTC
> + * @target: target frequency in kHz
> + * @refclk: reference clock frequency in kHz
> + * @match_clock: if provided, @best_clock P divider must
> + * match the P divider from @match_clock
> + * used for LVDS downclocking
> + * @best_clock: best PLL values found
> + *
> + * Returns true on success, false on failure.
> + */
> + bool (*find_dpll)(const struct intel_limit *limit,
> + struct drm_crtc *crtc,
> + int target, int refclk,
> + struct dpll *match_clock,
> + struct dpll *best_clock);
> void (*update_wm)(struct drm_device *dev);
> void (*update_sprite_wm)(struct drm_device *dev, int pipe,
> uint32_t sprite_width, int pixel_size,
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 7b0b16b..5d0f16d 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -59,24 +59,6 @@ typedef struct intel_limit intel_limit_t;
> struct intel_limit {
> intel_range_t dot, vco, n, m, m1, m2, p, p1;
> intel_p2_t p2;
> - /**
> - * find_pll() - Find the best values for the PLL
> - * @limit: limits for the PLL
> - * @crtc: current CRTC
> - * @target: target frequency in kHz
> - * @refclk: reference clock frequency in kHz
> - * @match_clock: if provided, @best_clock P divider must
> - * match the P divider from @match_clock
> - * used for LVDS downclocking
> - * @best_clock: best PLL values found
> - *
> - * Returns true on success, false on failure.
> - */
> - bool (*find_pll)(const intel_limit_t *limit,
> - struct drm_crtc *crtc,
> - int target, int refclk,
> - intel_clock_t *match_clock,
> - intel_clock_t *best_clock);
> };
>
> /* FDI */
> @@ -92,23 +74,6 @@ intel_pch_rawclk(struct drm_device *dev)
> return I915_READ(PCH_RAWCLK_FREQ) & RAWCLK_FREQ_MASK;
> }
>
> -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);
> -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,
> - intel_clock_t *best_clock);
> -
> static inline u32 /* units of 100MHz */
> intel_fdi_link_freq(struct drm_device *dev)
> {
> @@ -130,7 +95,6 @@ static const intel_limit_t intel_limits_i8xx_dvo = {
> .p1 = { .min = 2, .max = 33 },
> .p2 = { .dot_limit = 165000,
> .p2_slow = 4, .p2_fast = 2 },
> - .find_pll = intel_find_best_PLL,
> };
>
> static const intel_limit_t intel_limits_i8xx_lvds = {
> @@ -144,7 +108,6 @@ static const intel_limit_t intel_limits_i8xx_lvds = {
> .p1 = { .min = 1, .max = 6 },
> .p2 = { .dot_limit = 165000,
> .p2_slow = 14, .p2_fast = 7 },
> - .find_pll = intel_find_best_PLL,
> };
>
> static const intel_limit_t intel_limits_i9xx_sdvo = {
> @@ -158,7 +121,6 @@ static const intel_limit_t intel_limits_i9xx_sdvo = {
> .p1 = { .min = 1, .max = 8 },
> .p2 = { .dot_limit = 200000,
> .p2_slow = 10, .p2_fast = 5 },
> - .find_pll = intel_find_best_PLL,
> };
>
> static const intel_limit_t intel_limits_i9xx_lvds = {
> @@ -172,7 +134,6 @@ static const intel_limit_t intel_limits_i9xx_lvds = {
> .p1 = { .min = 1, .max = 8 },
> .p2 = { .dot_limit = 112000,
> .p2_slow = 14, .p2_fast = 7 },
> - .find_pll = intel_find_best_PLL,
> };
>
>
> @@ -189,7 +150,6 @@ static const intel_limit_t intel_limits_g4x_sdvo = {
> .p2_slow = 10,
> .p2_fast = 10
> },
> - .find_pll = intel_g4x_find_best_PLL,
> };
>
> static const intel_limit_t intel_limits_g4x_hdmi = {
> @@ -203,7 +163,6 @@ static const intel_limit_t intel_limits_g4x_hdmi = {
> .p1 = { .min = 1, .max = 8},
> .p2 = { .dot_limit = 165000,
> .p2_slow = 10, .p2_fast = 5 },
> - .find_pll = intel_g4x_find_best_PLL,
> };
>
> static const intel_limit_t intel_limits_g4x_single_channel_lvds = {
> @@ -218,7 +177,6 @@ static const intel_limit_t intel_limits_g4x_single_channel_lvds = {
> .p2 = { .dot_limit = 0,
> .p2_slow = 14, .p2_fast = 14
> },
> - .find_pll = intel_g4x_find_best_PLL,
> };
>
> static const intel_limit_t intel_limits_g4x_dual_channel_lvds = {
> @@ -233,7 +191,6 @@ static const intel_limit_t intel_limits_g4x_dual_channel_lvds = {
> .p2 = { .dot_limit = 0,
> .p2_slow = 7, .p2_fast = 7
> },
> - .find_pll = intel_g4x_find_best_PLL,
> };
>
> static const intel_limit_t intel_limits_pineview_sdvo = {
> @@ -249,7 +206,6 @@ 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_pnv_find_best_PLL,
> };
>
> static const intel_limit_t intel_limits_pineview_lvds = {
> @@ -263,7 +219,6 @@ 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_pnv_find_best_PLL,
> };
>
> /* Ironlake / Sandybridge
> @@ -282,7 +237,6 @@ static const intel_limit_t intel_limits_ironlake_dac = {
> .p1 = { .min = 1, .max = 8 },
> .p2 = { .dot_limit = 225000,
> .p2_slow = 10, .p2_fast = 5 },
> - .find_pll = intel_g4x_find_best_PLL,
> };
>
> static const intel_limit_t intel_limits_ironlake_single_lvds = {
> @@ -296,7 +250,6 @@ static const intel_limit_t intel_limits_ironlake_single_lvds = {
> .p1 = { .min = 2, .max = 8 },
> .p2 = { .dot_limit = 225000,
> .p2_slow = 14, .p2_fast = 14 },
> - .find_pll = intel_g4x_find_best_PLL,
> };
>
> static const intel_limit_t intel_limits_ironlake_dual_lvds = {
> @@ -310,7 +263,6 @@ static const intel_limit_t intel_limits_ironlake_dual_lvds = {
> .p1 = { .min = 2, .max = 8 },
> .p2 = { .dot_limit = 225000,
> .p2_slow = 7, .p2_fast = 7 },
> - .find_pll = intel_g4x_find_best_PLL,
> };
>
> /* LVDS 100mhz refclk limits. */
> @@ -325,7 +277,6 @@ static const intel_limit_t intel_limits_ironlake_single_lvds_100m = {
> .p1 = { .min = 2, .max = 8 },
> .p2 = { .dot_limit = 225000,
> .p2_slow = 14, .p2_fast = 14 },
> - .find_pll = intel_g4x_find_best_PLL,
> };
>
> static const intel_limit_t intel_limits_ironlake_dual_lvds_100m = {
> @@ -339,7 +290,6 @@ static const intel_limit_t intel_limits_ironlake_dual_lvds_100m = {
> .p1 = { .min = 2, .max = 6 },
> .p2 = { .dot_limit = 225000,
> .p2_slow = 7, .p2_fast = 7 },
> - .find_pll = intel_g4x_find_best_PLL,
> };
>
> static const intel_limit_t intel_limits_vlv_dac = {
> @@ -353,7 +303,6 @@ static const intel_limit_t intel_limits_vlv_dac = {
> .p1 = { .min = 1, .max = 3 },
> .p2 = { .dot_limit = 270000,
> .p2_slow = 2, .p2_fast = 20 },
> - .find_pll = intel_vlv_find_best_pll,
> };
>
> static const intel_limit_t intel_limits_vlv_hdmi = {
> @@ -367,7 +316,6 @@ static const intel_limit_t intel_limits_vlv_hdmi = {
> .p1 = { .min = 2, .max = 3 },
> .p2 = { .dot_limit = 270000,
> .p2_slow = 2, .p2_fast = 20 },
> - .find_pll = intel_vlv_find_best_pll,
> };
>
> static const intel_limit_t intel_limits_vlv_dp = {
> @@ -381,7 +329,6 @@ static const intel_limit_t intel_limits_vlv_dp = {
> .p1 = { .min = 1, .max = 3 },
> .p2 = { .dot_limit = 270000,
> .p2_slow = 2, .p2_fast = 20 },
> - .find_pll = intel_vlv_find_best_pll,
> };
>
> static const intel_limit_t *intel_ironlake_limit(struct drm_crtc *crtc,
> @@ -537,7 +484,7 @@ static bool intel_PLL_is_valid(struct drm_device *dev,
> }
>
> static bool
> -intel_find_best_PLL(const intel_limit_t *limit, struct drm_crtc *crtc,
> +i9xx_find_best_dpll(const intel_limit_t *limit, struct drm_crtc *crtc,
> int target, int refclk, intel_clock_t *match_clock,
> intel_clock_t *best_clock)
> {
> @@ -599,9 +546,9 @@ intel_find_best_PLL(const intel_limit_t *limit, struct drm_crtc *crtc,
> }
>
> 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)
> +pnv_find_best_dpll(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;
> @@ -661,9 +608,9 @@ intel_pnv_find_best_PLL(const intel_limit_t *limit, struct drm_crtc *crtc,
> }
>
> 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)
> +g4x_find_best_dpll(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;
> @@ -718,9 +665,9 @@ intel_g4x_find_best_PLL(const intel_limit_t *limit, struct drm_crtc *crtc,
> }
>
> 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,
> - intel_clock_t *best_clock)
> +vlv_find_best_dpll(const intel_limit_t *limit, struct drm_crtc *crtc,
> + int target, int refclk, intel_clock_t *match_clock,
> + intel_clock_t *best_clock)
> {
> u32 p1, p2, m1, m2, vco, bestn, bestm1, bestm2, bestp1, bestp2;
> u32 m, n, fastclk;
> @@ -4910,9 +4857,9 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
> * reflck * (5 * (m1 + 2) + (m2 + 2)) / (n + 2) / p1 / p2.
> */
> limit = intel_limit(crtc, refclk);
> - ok = limit->find_pll(limit, crtc, adjusted_mode->clock, refclk, NULL,
> - &clock);
> - if (!ok) {
> + ok = dev_priv->display.find_dpll(limit, crtc, adjusted_mode->clock,
> + refclk, NULL, &clock);
> + if (!ok && !intel_crtc->config.clock_set) {
> DRM_ERROR("Couldn't find PLL settings for mode!\n");
> return -EINVAL;
> }
> @@ -4927,10 +4874,10 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
> * by using the FP0/FP1. In such case we will disable the LVDS
> * downclock feature.
> */
> - has_reduced_clock = limit->find_pll(limit, crtc,
> + has_reduced_clock =
> + dev_priv->display.find_dpll(limit, crtc,
> dev_priv->lvds_downclock,
> - refclk,
> - &clock,
> + refclk, &clock,
> &reduced_clock);
> }
> /* Compat-code for transition, will disappear. */
> @@ -5546,8 +5493,8 @@ static bool ironlake_compute_clocks(struct drm_crtc *crtc,
> * reflck * (5 * (m1 + 2) + (m2 + 2)) / (n + 2) / p1 / p2.
> */
> limit = intel_limit(crtc, refclk);
> - ret = limit->find_pll(limit, crtc, adjusted_mode->clock, refclk, NULL,
> - clock);
> + ret = dev_priv->display.find_dpll(limit, crtc, adjusted_mode->clock,
> + refclk, NULL, clock);
> if (!ret)
> return false;
>
> @@ -5558,11 +5505,11 @@ static bool ironlake_compute_clocks(struct drm_crtc *crtc,
> * by using the FP0/FP1. In such case we will disable the LVDS
> * downclock feature.
> */
> - *has_reduced_clock = limit->find_pll(limit, crtc,
> - dev_priv->lvds_downclock,
> - refclk,
> - clock,
> - reduced_clock);
> + *has_reduced_clock =
> + dev_priv->display.find_dpll(limit, crtc,
> + dev_priv->lvds_downclock,
> + refclk, clock,
> + reduced_clock);
> }
>
> return true;
> @@ -5748,7 +5695,7 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
>
> ok = ironlake_compute_clocks(crtc, adjusted_mode, &clock,
> &has_reduced_clock, &reduced_clock);
> - if (!ok) {
> + if (!ok && !intel_crtc->config.clock_set) {
> DRM_ERROR("Couldn't find PLL settings for mode!\n");
> return -EINVAL;
> }
> @@ -9073,6 +9020,15 @@ static void intel_init_display(struct drm_device *dev)
> {
> struct drm_i915_private *dev_priv = dev->dev_private;
>
> + if (HAS_PCH_SPLIT(dev) || IS_G4X(dev))
> + dev_priv->display.find_dpll = g4x_find_best_dpll;
> + else if (IS_VALLEYVIEW(dev))
> + dev_priv->display.find_dpll = vlv_find_best_dpll;
> + else if (IS_PINEVIEW(dev))
> + dev_priv->display.find_dpll = pnv_find_best_dpll;
> + else
> + dev_priv->display.find_dpll = i9xx_find_best_dpll;
> +
> if (HAS_DDI(dev)) {
> dev_priv->display.get_pipe_config = haswell_get_pipe_config;
> dev_priv->display.crtc_mode_set = haswell_crtc_mode_set;
> --
> 1.7.11.7
>
--
Paulo Zanoni
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 3/7] drm/i915: clear up the fdi dotclock semantics for M/N computation
2013-06-01 15:16 [PATCH 0/7] adjusted mode rework and assorted patches Daniel Vetter
2013-06-01 15:16 ` [PATCH 1/7] drm/i915: split out intel_pnv_find_best_PLL Daniel Vetter
2013-06-01 15:16 ` [PATCH 2/7] drm/i915: move find_pll callback to dev_priv->display Daniel Vetter
@ 2013-06-01 15:16 ` Daniel Vetter
2013-06-03 15:59 ` Paulo Zanoni
2013-06-01 15:16 ` [PATCH 4/7] drm/i915: refactor cpu eDP PLL handling a bit Daniel Vetter
` (3 subsequent siblings)
6 siblings, 1 reply; 28+ messages in thread
From: Daniel Vetter @ 2013-06-01 15:16 UTC (permalink / raw)
To: Intel Graphics Development; +Cc: Daniel Vetter
We currently mutliply the link_bw of the fdi link with the pixel
multiplier, which is wrong: The FDI link doesn't suddenly grow more
bandwidth. In reality the pixel mutliplication only happens in the PCH,
before the pixels are fed into the port.
But since we our code treats the uses the target clock after pixels
are doubled (tripled, ...) already, we need to correct this.
Semantically it's clearer to divide the target clock to get the fdi
dotclock instead of multiplying the bw, so do that instead.
Note that the target clock is already multiplied by the same factor,
so the division will never loose accuracy for the M/N computation.
The lane computation otoh used the wrong value, we also need to feed
the fdi dotclock to that.
Split out on a request from Paulo Zanoni.
v2: Also fix the lane computation.
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
drivers/gpu/drm/i915/intel_display.c | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index a29295e..761254d 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3992,7 +3992,7 @@ static int ironlake_fdi_compute_config(struct intel_crtc *intel_crtc,
{
struct drm_device *dev = intel_crtc->base.dev;
struct drm_display_mode *adjusted_mode = &pipe_config->adjusted_mode;
- int target_clock, lane, link_bw;
+ int target_clock, lane, link_bw, fdi_dotclock;
bool setup_ok, needs_recompute = false;
retry:
@@ -4010,14 +4010,16 @@ retry:
else
target_clock = adjusted_mode->clock;
- lane = ironlake_get_lanes_required(target_clock, link_bw,
+ fdi_dotclock = target_clock;
+ if (pipe_config->pixel_multiplier > 1)
+ fdi_dotclock /= pipe_config->pixel_multiplier;
+
+ lane = ironlake_get_lanes_required(fdi_dotclock, link_bw,
pipe_config->pipe_bpp);
pipe_config->fdi_lanes = lane;
- if (pipe_config->pixel_multiplier > 1)
- link_bw *= pipe_config->pixel_multiplier;
- intel_link_compute_m_n(pipe_config->pipe_bpp, lane, target_clock,
+ intel_link_compute_m_n(pipe_config->pipe_bpp, lane, fdi_dotclock,
link_bw, &pipe_config->fdi_m_n);
setup_ok = ironlake_check_fdi_lanes(intel_crtc->base.dev,
--
1.7.11.7
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [PATCH 3/7] drm/i915: clear up the fdi dotclock semantics for M/N computation
2013-06-01 15:16 ` [PATCH 3/7] drm/i915: clear up the fdi dotclock semantics for M/N computation Daniel Vetter
@ 2013-06-03 15:59 ` Paulo Zanoni
2013-06-03 16:39 ` Paulo Zanoni
0 siblings, 1 reply; 28+ messages in thread
From: Paulo Zanoni @ 2013-06-03 15:59 UTC (permalink / raw)
To: Daniel Vetter; +Cc: Intel Graphics Development
2013/6/1 Daniel Vetter <daniel.vetter@ffwll.ch>:
> We currently mutliply the link_bw of the fdi link with the pixel
> multiplier, which is wrong: The FDI link doesn't suddenly grow more
> bandwidth. In reality the pixel mutliplication only happens in the PCH,
> before the pixels are fed into the port.
>
> But since we our code treats the uses the target clock after pixels
> are doubled (tripled, ...) already, we need to correct this.
>
> Semantically it's clearer to divide the target clock to get the fdi
> dotclock instead of multiplying the bw, so do that instead.
>
> Note that the target clock is already multiplied by the same factor,
> so the division will never loose accuracy for the M/N computation.
>
> The lane computation otoh used the wrong value, we also need to feed
> the fdi dotclock to that.
>
> Split out on a request from Paulo Zanoni.
>
> v2: Also fix the lane computation.
>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Do we ever test the pixel_multiplier case? Does it work at all?
Why is intel_ddi_get_config the only get_config function that has
"pipe_config->pixel_multiplier = 1"? If we assigned "1" to everybody
that don't need other values we would be able to remove many of those
"if (pixel_multiplier > 1)" checks.
Anyway, the patch seems to do what it says, so: Reviewed-by: Paulo
Zanoni <paulo.r.zanoni@intel.com>.
> ---
> drivers/gpu/drm/i915/intel_display.c | 12 +++++++-----
> 1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index a29295e..761254d 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3992,7 +3992,7 @@ static int ironlake_fdi_compute_config(struct intel_crtc *intel_crtc,
> {
> struct drm_device *dev = intel_crtc->base.dev;
> struct drm_display_mode *adjusted_mode = &pipe_config->adjusted_mode;
> - int target_clock, lane, link_bw;
> + int target_clock, lane, link_bw, fdi_dotclock;
> bool setup_ok, needs_recompute = false;
>
> retry:
> @@ -4010,14 +4010,16 @@ retry:
> else
> target_clock = adjusted_mode->clock;
>
> - lane = ironlake_get_lanes_required(target_clock, link_bw,
> + fdi_dotclock = target_clock;
> + if (pipe_config->pixel_multiplier > 1)
> + fdi_dotclock /= pipe_config->pixel_multiplier;
> +
> + lane = ironlake_get_lanes_required(fdi_dotclock, link_bw,
> pipe_config->pipe_bpp);
>
> pipe_config->fdi_lanes = lane;
>
> - if (pipe_config->pixel_multiplier > 1)
> - link_bw *= pipe_config->pixel_multiplier;
> - intel_link_compute_m_n(pipe_config->pipe_bpp, lane, target_clock,
> + intel_link_compute_m_n(pipe_config->pipe_bpp, lane, fdi_dotclock,
> link_bw, &pipe_config->fdi_m_n);
>
> setup_ok = ironlake_check_fdi_lanes(intel_crtc->base.dev,
> --
> 1.7.11.7
>
--
Paulo Zanoni
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH 3/7] drm/i915: clear up the fdi dotclock semantics for M/N computation
2013-06-03 15:59 ` Paulo Zanoni
@ 2013-06-03 16:39 ` Paulo Zanoni
2013-06-03 18:28 ` Daniel Vetter
0 siblings, 1 reply; 28+ messages in thread
From: Paulo Zanoni @ 2013-06-03 16:39 UTC (permalink / raw)
To: Daniel Vetter; +Cc: Intel Graphics Development
2013/6/3 Paulo Zanoni <przanoni@gmail.com>:
> 2013/6/1 Daniel Vetter <daniel.vetter@ffwll.ch>:
>> We currently mutliply the link_bw of the fdi link with the pixel
>> multiplier, which is wrong: The FDI link doesn't suddenly grow more
>> bandwidth. In reality the pixel mutliplication only happens in the PCH,
>> before the pixels are fed into the port.
>>
>> But since we our code treats the uses the target clock after pixels
>> are doubled (tripled, ...) already, we need to correct this.
>>
>> Semantically it's clearer to divide the target clock to get the fdi
>> dotclock instead of multiplying the bw, so do that instead.
>>
>> Note that the target clock is already multiplied by the same factor,
>> so the division will never loose accuracy for the M/N computation.
>>
>> The lane computation otoh used the wrong value, we also need to feed
>> the fdi dotclock to that.
>>
>> Split out on a request from Paulo Zanoni.
>>
>> v2: Also fix the lane computation.
>>
>> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>
> Do we ever test the pixel_multiplier case? Does it work at all?
>
> Why is intel_ddi_get_config the only get_config function that has
> "pipe_config->pixel_multiplier = 1"? If we assigned "1" to everybody
> that don't need other values we would be able to remove many of those
> "if (pixel_multiplier > 1)" checks.
>
> Anyway, the patch seems to do what it says, so: Reviewed-by: Paulo
> Zanoni <paulo.r.zanoni@intel.com>.
Actually I noticed this patch is different from the one I reviewed on Friday.
Last Friday's patch:
1) ironlake_get_lanes_required(target_clock, etc)
2) fdi_dotclock = target_clock
3) if (pixel_multiplier > 1) etc
4) intel_link_compute_m_n(fdi_dotclock)
Today's patch:
1) fdi_dotclock = target_clock
2) if (pixel_multiplier > 1) etc
3) ironlake_get_lanes_required(fdi_dotclock)
4) intel_link_compute_m_n(fdi_dotclock)
The difference is the argument used in ironlake_get_lanes_required. Is
this intentional?
>
>> ---
>> drivers/gpu/drm/i915/intel_display.c | 12 +++++++-----
>> 1 file changed, 7 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index a29295e..761254d 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -3992,7 +3992,7 @@ static int ironlake_fdi_compute_config(struct intel_crtc *intel_crtc,
>> {
>> struct drm_device *dev = intel_crtc->base.dev;
>> struct drm_display_mode *adjusted_mode = &pipe_config->adjusted_mode;
>> - int target_clock, lane, link_bw;
>> + int target_clock, lane, link_bw, fdi_dotclock;
>> bool setup_ok, needs_recompute = false;
>>
>> retry:
>> @@ -4010,14 +4010,16 @@ retry:
>> else
>> target_clock = adjusted_mode->clock;
>>
>> - lane = ironlake_get_lanes_required(target_clock, link_bw,
>> + fdi_dotclock = target_clock;
>> + if (pipe_config->pixel_multiplier > 1)
>> + fdi_dotclock /= pipe_config->pixel_multiplier;
>> +
>> + lane = ironlake_get_lanes_required(fdi_dotclock, link_bw,
>> pipe_config->pipe_bpp);
>>
>> pipe_config->fdi_lanes = lane;
>>
>> - if (pipe_config->pixel_multiplier > 1)
>> - link_bw *= pipe_config->pixel_multiplier;
>> - intel_link_compute_m_n(pipe_config->pipe_bpp, lane, target_clock,
>> + intel_link_compute_m_n(pipe_config->pipe_bpp, lane, fdi_dotclock,
>> link_bw, &pipe_config->fdi_m_n);
>>
>> setup_ok = ironlake_check_fdi_lanes(intel_crtc->base.dev,
>> --
>> 1.7.11.7
>>
>
>
>
> --
> Paulo Zanoni
--
Paulo Zanoni
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH 3/7] drm/i915: clear up the fdi dotclock semantics for M/N computation
2013-06-03 16:39 ` Paulo Zanoni
@ 2013-06-03 18:28 ` Daniel Vetter
0 siblings, 0 replies; 28+ messages in thread
From: Daniel Vetter @ 2013-06-03 18:28 UTC (permalink / raw)
To: Paulo Zanoni; +Cc: Daniel Vetter, Intel Graphics Development
On Mon, Jun 03, 2013 at 01:39:27PM -0300, Paulo Zanoni wrote:
> 2013/6/3 Paulo Zanoni <przanoni@gmail.com>:
> > 2013/6/1 Daniel Vetter <daniel.vetter@ffwll.ch>:
> >> We currently mutliply the link_bw of the fdi link with the pixel
> >> multiplier, which is wrong: The FDI link doesn't suddenly grow more
> >> bandwidth. In reality the pixel mutliplication only happens in the PCH,
> >> before the pixels are fed into the port.
> >>
> >> But since we our code treats the uses the target clock after pixels
> >> are doubled (tripled, ...) already, we need to correct this.
> >>
> >> Semantically it's clearer to divide the target clock to get the fdi
> >> dotclock instead of multiplying the bw, so do that instead.
> >>
> >> Note that the target clock is already multiplied by the same factor,
> >> so the division will never loose accuracy for the M/N computation.
> >>
> >> The lane computation otoh used the wrong value, we also need to feed
> >> the fdi dotclock to that.
> >>
> >> Split out on a request from Paulo Zanoni.
> >>
> >> v2: Also fix the lane computation.
> >>
> >> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> >
> > Do we ever test the pixel_multiplier case? Does it work at all?
> >
> > Why is intel_ddi_get_config the only get_config function that has
> > "pipe_config->pixel_multiplier = 1"? If we assigned "1" to everybody
> > that don't need other values we would be able to remove many of those
> > "if (pixel_multiplier > 1)" checks.
> >
> > Anyway, the patch seems to do what it says, so: Reviewed-by: Paulo
> > Zanoni <paulo.r.zanoni@intel.com>.
>
> Actually I noticed this patch is different from the one I reviewed on Friday.
>
> Last Friday's patch:
>
> 1) ironlake_get_lanes_required(target_clock, etc)
> 2) fdi_dotclock = target_clock
> 3) if (pixel_multiplier > 1) etc
> 4) intel_link_compute_m_n(fdi_dotclock)
>
> Today's patch:
> 1) fdi_dotclock = target_clock
> 2) if (pixel_multiplier > 1) etc
> 3) ironlake_get_lanes_required(fdi_dotclock)
> 4) intel_link_compute_m_n(fdi_dotclock)
>
> The difference is the argument used in ironlake_get_lanes_required. Is
> this intentional?
Yep, see the commit message:
"v2: Also fix the lane computation."
I might need to be a notch more verbose here ...
>
> >
> >> ---
> >> drivers/gpu/drm/i915/intel_display.c | 12 +++++++-----
> >> 1 file changed, 7 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >> index a29295e..761254d 100644
> >> --- a/drivers/gpu/drm/i915/intel_display.c
> >> +++ b/drivers/gpu/drm/i915/intel_display.c
> >> @@ -3992,7 +3992,7 @@ static int ironlake_fdi_compute_config(struct intel_crtc *intel_crtc,
> >> {
> >> struct drm_device *dev = intel_crtc->base.dev;
> >> struct drm_display_mode *adjusted_mode = &pipe_config->adjusted_mode;
> >> - int target_clock, lane, link_bw;
> >> + int target_clock, lane, link_bw, fdi_dotclock;
> >> bool setup_ok, needs_recompute = false;
> >>
> >> retry:
> >> @@ -4010,14 +4010,16 @@ retry:
> >> else
> >> target_clock = adjusted_mode->clock;
> >>
> >> - lane = ironlake_get_lanes_required(target_clock, link_bw,
> >> + fdi_dotclock = target_clock;
> >> + if (pipe_config->pixel_multiplier > 1)
> >> + fdi_dotclock /= pipe_config->pixel_multiplier;
> >> +
> >> + lane = ironlake_get_lanes_required(fdi_dotclock, link_bw,
> >> pipe_config->pipe_bpp);
> >>
> >> pipe_config->fdi_lanes = lane;
> >>
> >> - if (pipe_config->pixel_multiplier > 1)
> >> - link_bw *= pipe_config->pixel_multiplier;
> >> - intel_link_compute_m_n(pipe_config->pipe_bpp, lane, target_clock,
> >> + intel_link_compute_m_n(pipe_config->pipe_bpp, lane, fdi_dotclock,
> >> link_bw, &pipe_config->fdi_m_n);
> >>
> >> setup_ok = ironlake_check_fdi_lanes(intel_crtc->base.dev,
> >> --
> >> 1.7.11.7
> >>
> >
> >
> >
> > --
> > Paulo Zanoni
>
>
>
> --
> Paulo Zanoni
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 4/7] drm/i915: refactor cpu eDP PLL handling a bit
2013-06-01 15:16 [PATCH 0/7] adjusted mode rework and assorted patches Daniel Vetter
` (2 preceding siblings ...)
2013-06-01 15:16 ` [PATCH 3/7] drm/i915: clear up the fdi dotclock semantics for M/N computation Daniel Vetter
@ 2013-06-01 15:16 ` Daniel Vetter
2013-06-03 16:14 ` Paulo Zanoni
2013-06-01 15:16 ` [PATCH 5/7] drm/i915: store adjusted dotclock in adjusted_mode->clock Daniel Vetter
` (2 subsequent siblings)
6 siblings, 1 reply; 28+ messages in thread
From: Daniel Vetter @ 2013-06-01 15:16 UTC (permalink / raw)
To: Intel Graphics Development; +Cc: Daniel Vetter
This prepares a bit for the next big patch, where we switch the
semantics of the different clocks in the pipe config around.
Since I've broken cpu eDP PLL handling in the first version I've
figured some refactoring is in order.
Split out on request from Paulo Zanoni.
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
drivers/gpu/drm/i915/intel_dp.c | 38 ++++++++++++++------------------------
1 file changed, 14 insertions(+), 24 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 91a31b3..647cc2b 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -780,24 +780,29 @@ void intel_dp_init_link_config(struct intel_dp *intel_dp)
}
}
-static void ironlake_set_pll_edp(struct drm_crtc *crtc, int clock)
+static void ironlake_set_pll_cpu_edp(struct intel_dp *intel_dp)
{
- struct drm_device *dev = crtc->dev;
+ struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
+ struct intel_crtc *crtc = to_intel_crtc(dig_port->base.base.crtc);
+ struct drm_device *dev = crtc->base.dev;
struct drm_i915_private *dev_priv = dev->dev_private;
u32 dpa_ctl;
- DRM_DEBUG_KMS("eDP PLL enable for clock %d\n", clock);
+ DRM_DEBUG_KMS("eDP PLL enable for clock %d\n",
+ crtc->config.adjusted_mode.clock);
dpa_ctl = I915_READ(DP_A);
dpa_ctl &= ~DP_PLL_FREQ_MASK;
- if (clock < 200000) {
+ if (crtc->config.adjusted_mode.clock == 162000) {
/* For a long time we've carried around a ILK-DevA w/a for the
* 160MHz clock. If we're really unlucky, it's still required.
*/
DRM_DEBUG_KMS("160MHz cpu eDP clock, might need ilk devA w/a\n");
dpa_ctl |= DP_PLL_FREQ_160MHZ;
+ intel_dp->DP |= DP_PLL_FREQ_160MHZ;
} else {
dpa_ctl |= DP_PLL_FREQ_270MHZ;
+ intel_dp->DP |= DP_PLL_FREQ_270MHZ;
}
I915_WRITE(DP_A, dpa_ctl);
@@ -814,8 +819,7 @@ intel_dp_mode_set(struct drm_encoder *encoder, struct drm_display_mode *mode,
struct drm_i915_private *dev_priv = dev->dev_private;
struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
enum port port = dp_to_dig_port(intel_dp)->port;
- struct drm_crtc *crtc = encoder->crtc;
- struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+ struct intel_crtc *crtc = to_intel_crtc(encoder->crtc);
/*
* There are four kinds of DP registers:
@@ -845,7 +849,7 @@ intel_dp_mode_set(struct drm_encoder *encoder, struct drm_display_mode *mode,
if (intel_dp->has_audio) {
DRM_DEBUG_DRIVER("Enabling DP audio on pipe %c\n",
- pipe_name(intel_crtc->pipe));
+ pipe_name(crtc->pipe));
intel_dp->DP |= DP_AUDIO_OUTPUT_ENABLE;
intel_write_eld(encoder, adjusted_mode);
}
@@ -864,13 +868,7 @@ intel_dp_mode_set(struct drm_encoder *encoder, struct drm_display_mode *mode,
if (intel_dp->link_configuration[1] & DP_LANE_COUNT_ENHANCED_FRAME_EN)
intel_dp->DP |= DP_ENHANCED_FRAMING;
- intel_dp->DP |= intel_crtc->pipe << 29;
-
- /* don't miss out required setting for eDP */
- if (adjusted_mode->clock < 200000)
- intel_dp->DP |= DP_PLL_FREQ_160MHZ;
- else
- intel_dp->DP |= DP_PLL_FREQ_270MHZ;
+ intel_dp->DP |= crtc->pipe << 29;
} else if (!HAS_PCH_CPT(dev) || port == PORT_A) {
if (!HAS_PCH_SPLIT(dev) && !IS_VALLEYVIEW(dev))
intel_dp->DP |= intel_dp->color_range;
@@ -884,22 +882,14 @@ intel_dp_mode_set(struct drm_encoder *encoder, struct drm_display_mode *mode,
if (intel_dp->link_configuration[1] & DP_LANE_COUNT_ENHANCED_FRAME_EN)
intel_dp->DP |= DP_ENHANCED_FRAMING;
- if (intel_crtc->pipe == 1)
+ if (crtc->pipe == 1)
intel_dp->DP |= DP_PIPEB_SELECT;
-
- if (port == PORT_A && !IS_VALLEYVIEW(dev)) {
- /* don't miss out required setting for eDP */
- if (adjusted_mode->clock < 200000)
- intel_dp->DP |= DP_PLL_FREQ_160MHZ;
- else
- intel_dp->DP |= DP_PLL_FREQ_270MHZ;
- }
} else {
intel_dp->DP |= DP_LINK_TRAIN_OFF_CPT;
}
if (port == PORT_A && !IS_VALLEYVIEW(dev))
- ironlake_set_pll_edp(crtc, adjusted_mode->clock);
+ ironlake_set_pll_cpu_edp(intel_dp);
}
#define IDLE_ON_MASK (PP_ON | 0 | PP_SEQUENCE_MASK | 0 | PP_SEQUENCE_STATE_MASK)
--
1.7.11.7
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [PATCH 4/7] drm/i915: refactor cpu eDP PLL handling a bit
2013-06-01 15:16 ` [PATCH 4/7] drm/i915: refactor cpu eDP PLL handling a bit Daniel Vetter
@ 2013-06-03 16:14 ` Paulo Zanoni
0 siblings, 0 replies; 28+ messages in thread
From: Paulo Zanoni @ 2013-06-03 16:14 UTC (permalink / raw)
To: Daniel Vetter; +Cc: Intel Graphics Development
2013/6/1 Daniel Vetter <daniel.vetter@ffwll.ch>:
> This prepares a bit for the next big patch, where we switch the
> semantics of the different clocks in the pipe config around.
>
> Since I've broken cpu eDP PLL handling in the first version I've
> figured some refactoring is in order.
>
> Split out on request from Paulo Zanoni.
>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Possible next steps:
- Uncomplicate those port+gen+pch checks inside intel_dp_mode_set (and
everywhere else inside intel_dp.c).
- Define an enum for 162000, 270000 and 540000 and all the other
possible DP frequencies that might arrive in the future.
> ---
> drivers/gpu/drm/i915/intel_dp.c | 38 ++++++++++++++------------------------
> 1 file changed, 14 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 91a31b3..647cc2b 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -780,24 +780,29 @@ void intel_dp_init_link_config(struct intel_dp *intel_dp)
> }
> }
>
> -static void ironlake_set_pll_edp(struct drm_crtc *crtc, int clock)
> +static void ironlake_set_pll_cpu_edp(struct intel_dp *intel_dp)
> {
> - struct drm_device *dev = crtc->dev;
> + struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
> + struct intel_crtc *crtc = to_intel_crtc(dig_port->base.base.crtc);
> + struct drm_device *dev = crtc->base.dev;
> struct drm_i915_private *dev_priv = dev->dev_private;
> u32 dpa_ctl;
>
> - DRM_DEBUG_KMS("eDP PLL enable for clock %d\n", clock);
> + DRM_DEBUG_KMS("eDP PLL enable for clock %d\n",
> + crtc->config.adjusted_mode.clock);
> dpa_ctl = I915_READ(DP_A);
> dpa_ctl &= ~DP_PLL_FREQ_MASK;
>
> - if (clock < 200000) {
> + if (crtc->config.adjusted_mode.clock == 162000) {
> /* For a long time we've carried around a ILK-DevA w/a for the
> * 160MHz clock. If we're really unlucky, it's still required.
> */
> DRM_DEBUG_KMS("160MHz cpu eDP clock, might need ilk devA w/a\n");
> dpa_ctl |= DP_PLL_FREQ_160MHZ;
> + intel_dp->DP |= DP_PLL_FREQ_160MHZ;
> } else {
> dpa_ctl |= DP_PLL_FREQ_270MHZ;
> + intel_dp->DP |= DP_PLL_FREQ_270MHZ;
> }
>
> I915_WRITE(DP_A, dpa_ctl);
> @@ -814,8 +819,7 @@ intel_dp_mode_set(struct drm_encoder *encoder, struct drm_display_mode *mode,
> struct drm_i915_private *dev_priv = dev->dev_private;
> struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
> enum port port = dp_to_dig_port(intel_dp)->port;
> - struct drm_crtc *crtc = encoder->crtc;
> - struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> + struct intel_crtc *crtc = to_intel_crtc(encoder->crtc);
>
> /*
> * There are four kinds of DP registers:
> @@ -845,7 +849,7 @@ intel_dp_mode_set(struct drm_encoder *encoder, struct drm_display_mode *mode,
>
> if (intel_dp->has_audio) {
> DRM_DEBUG_DRIVER("Enabling DP audio on pipe %c\n",
> - pipe_name(intel_crtc->pipe));
> + pipe_name(crtc->pipe));
> intel_dp->DP |= DP_AUDIO_OUTPUT_ENABLE;
> intel_write_eld(encoder, adjusted_mode);
> }
> @@ -864,13 +868,7 @@ intel_dp_mode_set(struct drm_encoder *encoder, struct drm_display_mode *mode,
> if (intel_dp->link_configuration[1] & DP_LANE_COUNT_ENHANCED_FRAME_EN)
> intel_dp->DP |= DP_ENHANCED_FRAMING;
>
> - intel_dp->DP |= intel_crtc->pipe << 29;
> -
> - /* don't miss out required setting for eDP */
> - if (adjusted_mode->clock < 200000)
> - intel_dp->DP |= DP_PLL_FREQ_160MHZ;
> - else
> - intel_dp->DP |= DP_PLL_FREQ_270MHZ;
> + intel_dp->DP |= crtc->pipe << 29;
> } else if (!HAS_PCH_CPT(dev) || port == PORT_A) {
> if (!HAS_PCH_SPLIT(dev) && !IS_VALLEYVIEW(dev))
> intel_dp->DP |= intel_dp->color_range;
> @@ -884,22 +882,14 @@ intel_dp_mode_set(struct drm_encoder *encoder, struct drm_display_mode *mode,
> if (intel_dp->link_configuration[1] & DP_LANE_COUNT_ENHANCED_FRAME_EN)
> intel_dp->DP |= DP_ENHANCED_FRAMING;
>
> - if (intel_crtc->pipe == 1)
> + if (crtc->pipe == 1)
> intel_dp->DP |= DP_PIPEB_SELECT;
> -
> - if (port == PORT_A && !IS_VALLEYVIEW(dev)) {
> - /* don't miss out required setting for eDP */
> - if (adjusted_mode->clock < 200000)
> - intel_dp->DP |= DP_PLL_FREQ_160MHZ;
> - else
> - intel_dp->DP |= DP_PLL_FREQ_270MHZ;
> - }
> } else {
> intel_dp->DP |= DP_LINK_TRAIN_OFF_CPT;
> }
>
> if (port == PORT_A && !IS_VALLEYVIEW(dev))
> - ironlake_set_pll_edp(crtc, adjusted_mode->clock);
> + ironlake_set_pll_cpu_edp(intel_dp);
> }
>
> #define IDLE_ON_MASK (PP_ON | 0 | PP_SEQUENCE_MASK | 0 | PP_SEQUENCE_STATE_MASK)
> --
> 1.7.11.7
>
--
Paulo Zanoni
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 5/7] drm/i915: store adjusted dotclock in adjusted_mode->clock
2013-06-01 15:16 [PATCH 0/7] adjusted mode rework and assorted patches Daniel Vetter
` (3 preceding siblings ...)
2013-06-01 15:16 ` [PATCH 4/7] drm/i915: refactor cpu eDP PLL handling a bit Daniel Vetter
@ 2013-06-01 15:16 ` Daniel Vetter
2013-06-03 16:54 ` Paulo Zanoni
2013-06-01 15:16 ` [PATCH 6/7] drm/i915: Drop some no longer required mode/adjusted_mode parameters Daniel Vetter
2013-06-01 15:16 ` [PATCH 7/7] drm/i915: check for strange pfit pipe assignemnt on ivb/hsw Daniel Vetter
6 siblings, 1 reply; 28+ messages in thread
From: Daniel Vetter @ 2013-06-01 15:16 UTC (permalink / raw)
To: Intel Graphics Development; +Cc: Daniel Vetter
... not the port clock. This allows us to kill the funny semantics
around pixel_target_clock.
Since the dpll code still needs the real port clock, add a new
port_clock field to the pipe configuration. Handling the default case
for that one is a bit tricky, since encoders might not consistently
overwrite it when retrying the crtc/encoder bw arbitrage step in the
compute config stage. Hence we need to always clear port_clock and
update it again if the encoder hasn't put in something more specific.
This can't be done in one step since the encoder might want to adjust
the mode first.
I was a bit on the fence whether I should subsume the pixel multiplier
handling into the port_clock, too. But then I decided against this
since it's on an abstract level still the dotclock of the adjusted
mode, and only our hw makes it a bit special due to the separate pixel
mulitplier setting (which requires that the dpll runs at the
non-multiplied dotclock).
So after this patch the adjusted_mode accurately describes the mode we
feed into the port, after the panel fitter and pixel multiplier (or
line doubling, if we ever bother with that) have done their job.
Since the fdi link is between the pfit and the pixel multiplier steps
we need to be careful with calculating the fdi link config.
v2: Fix up ilk cpu pll handling.
v3: Introduce an fdi_dotclock variable in ironlake_fdi_compute_config
to make it clearer that we transmit the adjusted_mode without the
pixel multiplier taken into account. The old code multiplied the the
available link bw with the pixel multiplier, which results in the same
fdi configuration, but is much more confusing.
v4: Rebase on top of Imre's is_cpu_edp removal.
v5: Rebase on top of Paulo's haswell watermark fixes, which introduce
a new place which looked at the pixel_clock and so needed conversion.
v6: Split out prep patches as requested by Paulo Zanoni. Also rebase
on top of the fdi dotclock handling fix in the fdi lanes/bw
computation code.
Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org> (v3)
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
drivers/gpu/drm/i915/intel_ddi.c | 3 ++-
drivers/gpu/drm/i915/intel_display.c | 32 +++++++++++++++++---------------
drivers/gpu/drm/i915/intel_dp.c | 18 +++++++-----------
drivers/gpu/drm/i915/intel_drv.h | 13 +++++++------
drivers/gpu/drm/i915/intel_hdmi.c | 4 +---
drivers/gpu/drm/i915/intel_pm.c | 5 +----
6 files changed, 35 insertions(+), 40 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 9649df8..486c46b 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -624,7 +624,7 @@ intel_ddi_calculate_wrpll(int clock /* in Hz */,
clock, *p_out, *n2_out, *r2_out);
}
-bool intel_ddi_pll_mode_set(struct drm_crtc *crtc, int clock)
+bool intel_ddi_pll_mode_set(struct drm_crtc *crtc)
{
struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
struct intel_encoder *intel_encoder = intel_ddi_get_crtc_encoder(crtc);
@@ -634,6 +634,7 @@ bool intel_ddi_pll_mode_set(struct drm_crtc *crtc, int clock)
int type = intel_encoder->type;
enum pipe pipe = intel_crtc->pipe;
uint32_t reg, val;
+ int clock = intel_crtc->config.port_clock;
/* TODO: reuse PLLs when possible (compare values) */
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 761254d..103f4e9 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3992,7 +3992,7 @@ static int ironlake_fdi_compute_config(struct intel_crtc *intel_crtc,
{
struct drm_device *dev = intel_crtc->base.dev;
struct drm_display_mode *adjusted_mode = &pipe_config->adjusted_mode;
- int target_clock, lane, link_bw, fdi_dotclock;
+ int lane, link_bw, fdi_dotclock;
bool setup_ok, needs_recompute = false;
retry:
@@ -4005,12 +4005,7 @@ retry:
*/
link_bw = intel_fdi_link_freq(dev) * MHz(100)/KHz(1)/10;
- if (pipe_config->pixel_target_clock)
- target_clock = pipe_config->pixel_target_clock;
- else
- target_clock = adjusted_mode->clock;
-
- fdi_dotclock = target_clock;
+ fdi_dotclock = adjusted_mode->clock;
if (pipe_config->pixel_multiplier > 1)
fdi_dotclock /= pipe_config->pixel_multiplier;
@@ -4360,8 +4355,6 @@ static void vlv_update_pll(struct intel_crtc *crtc)
{
struct drm_device *dev = crtc->base.dev;
struct drm_i915_private *dev_priv = dev->dev_private;
- struct drm_display_mode *adjusted_mode =
- &crtc->config.adjusted_mode;
struct intel_encoder *encoder;
int pipe = crtc->pipe;
u32 dpll, mdiv;
@@ -4414,7 +4407,7 @@ static void vlv_update_pll(struct intel_crtc *crtc)
vlv_dpio_write(dev_priv, DPIO_DIV(pipe), mdiv);
/* Set HBR and RBR LPF coefficients */
- if (adjusted_mode->clock == 162000 ||
+ if (crtc->config.port_clock == 162000 ||
intel_pipe_has_type(&crtc->base, INTEL_OUTPUT_HDMI))
vlv_dpio_write(dev_priv, DPIO_LFP_COEFF(pipe),
0x005f0021);
@@ -4859,7 +4852,8 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
* reflck * (5 * (m1 + 2) + (m2 + 2)) / (n + 2) / p1 / p2.
*/
limit = intel_limit(crtc, refclk);
- ok = dev_priv->display.find_dpll(limit, crtc, adjusted_mode->clock,
+ ok = dev_priv->display.find_dpll(limit, crtc,
+ intel_crtc->config.port_clock,
refclk, NULL, &clock);
if (!ok) {
DRM_ERROR("Couldn't find PLL settings for mode!\n");
@@ -5467,7 +5461,6 @@ static void haswell_set_pipeconf(struct drm_crtc *crtc)
}
static bool ironlake_compute_clocks(struct drm_crtc *crtc,
- struct drm_display_mode *adjusted_mode,
intel_clock_t *clock,
bool *has_reduced_clock,
intel_clock_t *reduced_clock)
@@ -5495,7 +5488,8 @@ static bool ironlake_compute_clocks(struct drm_crtc *crtc,
* reflck * (5 * (m1 + 2) + (m2 + 2)) / (n + 2) / p1 / p2.
*/
limit = intel_limit(crtc, refclk);
- ret = dev_priv->display.find_dpll(limit, crtc, adjusted_mode->clock,
+ ret = dev_priv->display.find_dpll(limit, crtc,
+ to_intel_crtc(crtc)->config.port_clock,
refclk, NULL, clock);
if (!ret)
return false;
@@ -5695,7 +5689,7 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
WARN(!(HAS_PCH_IBX(dev) || HAS_PCH_CPT(dev)),
"Unexpected PCH type %d\n", INTEL_PCH_TYPE(dev));
- ok = ironlake_compute_clocks(crtc, adjusted_mode, &clock,
+ ok = ironlake_compute_clocks(crtc, &clock,
&has_reduced_clock, &reduced_clock);
if (!ok) {
DRM_ERROR("Couldn't find PLL settings for mode!\n");
@@ -5898,7 +5892,7 @@ static int haswell_crtc_mode_set(struct drm_crtc *crtc,
WARN(num_connectors != 1, "%d connectors attached to pipe %c\n",
num_connectors, pipe_name(pipe));
- if (!intel_ddi_pll_mode_set(crtc, adjusted_mode->clock))
+ if (!intel_ddi_pll_mode_set(crtc))
return -EINVAL;
/* Ensure that the cursor is valid for the new mode before changing... */
@@ -7789,6 +7783,9 @@ intel_modeset_pipe_config(struct drm_crtc *crtc,
goto fail;
encoder_retry:
+ /* Ensure the port clock default is reset when retrying. */
+ pipe_config->port_clock = 0;
+
/* Pass our mode to the connectors and the CRTC to give them a chance to
* adjust it according to limitations or connector properties, and also
* a chance to reject the mode entirely.
@@ -7817,6 +7814,11 @@ encoder_retry:
}
}
+ /* Set default port clock if not overwritten by the encoder. Needs to be
+ * done afterwards in case the encoder adjusts the mode. */
+ if (!pipe_config->port_clock)
+ pipe_config->port_clock = pipe_config->adjusted_mode.clock;
+
ret = intel_crtc_compute_config(crtc, pipe_config);
if (ret < 0) {
DRM_DEBUG_KMS("CRTC fixup failed\n");
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 647cc2b..c92eeeb 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -677,7 +677,7 @@ intel_dp_compute_config(struct intel_encoder *encoder,
int max_clock = intel_dp_max_link_bw(intel_dp) == DP_LINK_BW_2_7 ? 1 : 0;
int bpp, mode_rate;
static int bws[2] = { DP_LINK_BW_1_62, DP_LINK_BW_2_7 };
- int target_clock, link_avail, link_clock;
+ int link_avail, link_clock;
if (HAS_PCH_SPLIT(dev) && !HAS_DDI(dev) && port != PORT_A)
pipe_config->has_pch_encoder = true;
@@ -694,8 +694,6 @@ intel_dp_compute_config(struct intel_encoder *encoder,
intel_pch_panel_fitting(intel_crtc, pipe_config,
intel_connector->panel.fitting_mode);
}
- /* We need to take the panel's fixed mode into account. */
- target_clock = adjusted_mode->clock;
if (adjusted_mode->flags & DRM_MODE_FLAG_DBLCLK)
return false;
@@ -711,7 +709,7 @@ intel_dp_compute_config(struct intel_encoder *encoder,
bpp = min_t(int, bpp, dev_priv->vbt.edp_bpp);
for (; bpp >= 6*3; bpp -= 2*3) {
- mode_rate = intel_dp_link_required(target_clock, bpp);
+ mode_rate = intel_dp_link_required(adjusted_mode->clock, bpp);
for (clock = 0; clock <= max_clock; clock++) {
for (lane_count = 1; lane_count <= max_lane_count; lane_count <<= 1) {
@@ -746,18 +744,17 @@ found:
intel_dp->link_bw = bws[clock];
intel_dp->lane_count = lane_count;
- adjusted_mode->clock = drm_dp_bw_code_to_link_rate(intel_dp->link_bw);
pipe_config->pipe_bpp = bpp;
- pipe_config->pixel_target_clock = target_clock;
+ pipe_config->port_clock = drm_dp_bw_code_to_link_rate(intel_dp->link_bw);
DRM_DEBUG_KMS("DP link bw %02x lane count %d clock %d bpp %d\n",
intel_dp->link_bw, intel_dp->lane_count,
- adjusted_mode->clock, bpp);
+ pipe_config->port_clock, bpp);
DRM_DEBUG_KMS("DP link bw required %i available %i\n",
mode_rate, link_avail);
intel_link_compute_m_n(bpp, lane_count,
- target_clock, adjusted_mode->clock,
+ adjusted_mode->clock, pipe_config->port_clock,
&pipe_config->dp_m_n);
intel_dp_set_clock(encoder, pipe_config, intel_dp->link_bw);
@@ -788,12 +785,11 @@ static void ironlake_set_pll_cpu_edp(struct intel_dp *intel_dp)
struct drm_i915_private *dev_priv = dev->dev_private;
u32 dpa_ctl;
- DRM_DEBUG_KMS("eDP PLL enable for clock %d\n",
- crtc->config.adjusted_mode.clock);
+ DRM_DEBUG_KMS("eDP PLL enable for clock %d\n", crtc->config.port_clock);
dpa_ctl = I915_READ(DP_A);
dpa_ctl &= ~DP_PLL_FREQ_MASK;
- if (crtc->config.adjusted_mode.clock == 162000) {
+ if (crtc->config.port_clock == 162000) {
/* For a long time we've carried around a ILK-DevA w/a for the
* 160MHz clock. If we're really unlucky, it's still required.
*/
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index fdf6303..afda71f 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -243,12 +243,13 @@ struct intel_crtc_config {
int pipe_bpp;
struct intel_link_m_n dp_m_n;
- /**
- * This is currently used by DP and HDMI encoders since those can have a
- * target pixel clock != the port link clock (which is currently stored
- * in adjusted_mode->clock).
+
+ /*
+ * Frequence the dpll for the port should run at. Differs from the
+ * adjusted dotclock e.g. for DP or 12bpc hdmi mode.
*/
- int pixel_target_clock;
+ int port_clock;
+
/* Used by SDVO (and if we ever fix it, HDMI). */
unsigned pixel_multiplier;
@@ -786,7 +787,7 @@ extern void intel_ddi_disable_transcoder_func(struct drm_i915_private *dev_priv,
extern void intel_ddi_enable_pipe_clock(struct intel_crtc *intel_crtc);
extern void intel_ddi_disable_pipe_clock(struct intel_crtc *intel_crtc);
extern void intel_ddi_setup_hw_pll_state(struct drm_device *dev);
-extern bool intel_ddi_pll_mode_set(struct drm_crtc *crtc, int clock);
+extern bool intel_ddi_pll_mode_set(struct drm_crtc *crtc);
extern void intel_ddi_put_crtc_pll(struct drm_crtc *crtc);
extern void intel_ddi_set_pipe_settings(struct drm_crtc *crtc);
extern void intel_ddi_prepare_link_retrain(struct drm_encoder *encoder);
diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index 8062a92..bc12518 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -835,9 +835,7 @@ bool intel_hdmi_compute_config(struct intel_encoder *encoder,
desired_bpp = 12*3;
/* Need to adjust the port link by 1.5x for 12bpc. */
- adjusted_mode->clock = clock_12bpc;
- pipe_config->pixel_target_clock =
- pipe_config->requested_mode.clock;
+ pipe_config->port_clock = clock_12bpc;
} else {
DRM_DEBUG_KMS("picking bpc to 8 for HDMI output\n");
desired_bpp = 8*3;
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 49a1887..4126fb1 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -2078,10 +2078,7 @@ static uint32_t hsw_wm_get_pixel_rate(struct drm_device *dev,
struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
uint32_t pixel_rate, pfit_size;
- if (intel_crtc->config.pixel_target_clock)
- pixel_rate = intel_crtc->config.pixel_target_clock;
- else
- pixel_rate = intel_crtc->config.adjusted_mode.clock;
+ pixel_rate = intel_crtc->config.adjusted_mode.clock;
/* We only use IF-ID interlacing. If we ever use PF-ID we'll need to
* adjust the pixel_rate here. */
--
1.7.11.7
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [PATCH 5/7] drm/i915: store adjusted dotclock in adjusted_mode->clock
2013-06-01 15:16 ` [PATCH 5/7] drm/i915: store adjusted dotclock in adjusted_mode->clock Daniel Vetter
@ 2013-06-03 16:54 ` Paulo Zanoni
2013-06-03 16:56 ` Paulo Zanoni
2013-06-04 12:01 ` Daniel Vetter
0 siblings, 2 replies; 28+ messages in thread
From: Paulo Zanoni @ 2013-06-03 16:54 UTC (permalink / raw)
To: Daniel Vetter; +Cc: Intel Graphics Development
2013/6/1 Daniel Vetter <daniel.vetter@ffwll.ch>:
> ... not the port clock. This allows us to kill the funny semantics
> around pixel_target_clock.
>
> Since the dpll code still needs the real port clock, add a new
> port_clock field to the pipe configuration. Handling the default case
> for that one is a bit tricky, since encoders might not consistently
> overwrite it when retrying the crtc/encoder bw arbitrage step in the
> compute config stage. Hence we need to always clear port_clock and
> update it again if the encoder hasn't put in something more specific.
> This can't be done in one step since the encoder might want to adjust
> the mode first.
>
> I was a bit on the fence whether I should subsume the pixel multiplier
> handling into the port_clock, too. But then I decided against this
> since it's on an abstract level still the dotclock of the adjusted
> mode, and only our hw makes it a bit special due to the separate pixel
> mulitplier setting (which requires that the dpll runs at the
> non-multiplied dotclock).
>
> So after this patch the adjusted_mode accurately describes the mode we
> feed into the port, after the panel fitter and pixel multiplier (or
> line doubling, if we ever bother with that) have done their job.
> Since the fdi link is between the pfit and the pixel multiplier steps
> we need to be careful with calculating the fdi link config.
>
> v2: Fix up ilk cpu pll handling.
>
> v3: Introduce an fdi_dotclock variable in ironlake_fdi_compute_config
> to make it clearer that we transmit the adjusted_mode without the
> pixel multiplier taken into account. The old code multiplied the the
> available link bw with the pixel multiplier, which results in the same
> fdi configuration, but is much more confusing.
>
> v4: Rebase on top of Imre's is_cpu_edp removal.
>
> v5: Rebase on top of Paulo's haswell watermark fixes, which introduce
> a new place which looked at the pixel_clock and so needed conversion.
>
> v6: Split out prep patches as requested by Paulo Zanoni. Also rebase
> on top of the fdi dotclock handling fix in the fdi lanes/bw
> computation code.
>
> Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org> (v3)
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Due to my comments on patch 3, there's a possibility you may need to
rebase this patch.
My only optional bikeshed is to print port_clock inside
intel_dump_pipe_config. If you rebase, you may consider doing it :)
Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com> (v6)
> ---
> drivers/gpu/drm/i915/intel_ddi.c | 3 ++-
> drivers/gpu/drm/i915/intel_display.c | 32 +++++++++++++++++---------------
> drivers/gpu/drm/i915/intel_dp.c | 18 +++++++-----------
> drivers/gpu/drm/i915/intel_drv.h | 13 +++++++------
> drivers/gpu/drm/i915/intel_hdmi.c | 4 +---
> drivers/gpu/drm/i915/intel_pm.c | 5 +----
> 6 files changed, 35 insertions(+), 40 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index 9649df8..486c46b 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -624,7 +624,7 @@ intel_ddi_calculate_wrpll(int clock /* in Hz */,
> clock, *p_out, *n2_out, *r2_out);
> }
>
> -bool intel_ddi_pll_mode_set(struct drm_crtc *crtc, int clock)
> +bool intel_ddi_pll_mode_set(struct drm_crtc *crtc)
> {
> struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> struct intel_encoder *intel_encoder = intel_ddi_get_crtc_encoder(crtc);
> @@ -634,6 +634,7 @@ bool intel_ddi_pll_mode_set(struct drm_crtc *crtc, int clock)
> int type = intel_encoder->type;
> enum pipe pipe = intel_crtc->pipe;
> uint32_t reg, val;
> + int clock = intel_crtc->config.port_clock;
>
> /* TODO: reuse PLLs when possible (compare values) */
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 761254d..103f4e9 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3992,7 +3992,7 @@ static int ironlake_fdi_compute_config(struct intel_crtc *intel_crtc,
> {
> struct drm_device *dev = intel_crtc->base.dev;
> struct drm_display_mode *adjusted_mode = &pipe_config->adjusted_mode;
> - int target_clock, lane, link_bw, fdi_dotclock;
> + int lane, link_bw, fdi_dotclock;
> bool setup_ok, needs_recompute = false;
>
> retry:
> @@ -4005,12 +4005,7 @@ retry:
> */
> link_bw = intel_fdi_link_freq(dev) * MHz(100)/KHz(1)/10;
>
> - if (pipe_config->pixel_target_clock)
> - target_clock = pipe_config->pixel_target_clock;
> - else
> - target_clock = adjusted_mode->clock;
> -
> - fdi_dotclock = target_clock;
> + fdi_dotclock = adjusted_mode->clock;
In the patch you sent Friday, you used adjusted_mode->clock instead of
fdi_dotclock as the parameter to ironlake_get_lanes_required, you only
initialized fdi_dotclock before calling intel_link_compute_m_n. This
doesn't change the code behavior, I'm just pointing in case you
changed without noticing and prefer the older version.
> if (pipe_config->pixel_multiplier > 1)
> fdi_dotclock /= pipe_config->pixel_multiplier;
>
> @@ -4360,8 +4355,6 @@ static void vlv_update_pll(struct intel_crtc *crtc)
> {
> struct drm_device *dev = crtc->base.dev;
> struct drm_i915_private *dev_priv = dev->dev_private;
> - struct drm_display_mode *adjusted_mode =
> - &crtc->config.adjusted_mode;
> struct intel_encoder *encoder;
> int pipe = crtc->pipe;
> u32 dpll, mdiv;
> @@ -4414,7 +4407,7 @@ static void vlv_update_pll(struct intel_crtc *crtc)
> vlv_dpio_write(dev_priv, DPIO_DIV(pipe), mdiv);
>
> /* Set HBR and RBR LPF coefficients */
> - if (adjusted_mode->clock == 162000 ||
> + if (crtc->config.port_clock == 162000 ||
> intel_pipe_has_type(&crtc->base, INTEL_OUTPUT_HDMI))
> vlv_dpio_write(dev_priv, DPIO_LFP_COEFF(pipe),
> 0x005f0021);
> @@ -4859,7 +4852,8 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
> * reflck * (5 * (m1 + 2) + (m2 + 2)) / (n + 2) / p1 / p2.
> */
> limit = intel_limit(crtc, refclk);
> - ok = dev_priv->display.find_dpll(limit, crtc, adjusted_mode->clock,
> + ok = dev_priv->display.find_dpll(limit, crtc,
> + intel_crtc->config.port_clock,
> refclk, NULL, &clock);
> if (!ok) {
> DRM_ERROR("Couldn't find PLL settings for mode!\n");
> @@ -5467,7 +5461,6 @@ static void haswell_set_pipeconf(struct drm_crtc *crtc)
> }
>
> static bool ironlake_compute_clocks(struct drm_crtc *crtc,
> - struct drm_display_mode *adjusted_mode,
> intel_clock_t *clock,
> bool *has_reduced_clock,
> intel_clock_t *reduced_clock)
> @@ -5495,7 +5488,8 @@ static bool ironlake_compute_clocks(struct drm_crtc *crtc,
> * reflck * (5 * (m1 + 2) + (m2 + 2)) / (n + 2) / p1 / p2.
> */
> limit = intel_limit(crtc, refclk);
> - ret = dev_priv->display.find_dpll(limit, crtc, adjusted_mode->clock,
> + ret = dev_priv->display.find_dpll(limit, crtc,
> + to_intel_crtc(crtc)->config.port_clock,
> refclk, NULL, clock);
> if (!ret)
> return false;
> @@ -5695,7 +5689,7 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
> WARN(!(HAS_PCH_IBX(dev) || HAS_PCH_CPT(dev)),
> "Unexpected PCH type %d\n", INTEL_PCH_TYPE(dev));
>
> - ok = ironlake_compute_clocks(crtc, adjusted_mode, &clock,
> + ok = ironlake_compute_clocks(crtc, &clock,
> &has_reduced_clock, &reduced_clock);
> if (!ok) {
> DRM_ERROR("Couldn't find PLL settings for mode!\n");
> @@ -5898,7 +5892,7 @@ static int haswell_crtc_mode_set(struct drm_crtc *crtc,
> WARN(num_connectors != 1, "%d connectors attached to pipe %c\n",
> num_connectors, pipe_name(pipe));
>
> - if (!intel_ddi_pll_mode_set(crtc, adjusted_mode->clock))
> + if (!intel_ddi_pll_mode_set(crtc))
> return -EINVAL;
>
> /* Ensure that the cursor is valid for the new mode before changing... */
> @@ -7789,6 +7783,9 @@ intel_modeset_pipe_config(struct drm_crtc *crtc,
> goto fail;
>
> encoder_retry:
> + /* Ensure the port clock default is reset when retrying. */
> + pipe_config->port_clock = 0;
> +
> /* Pass our mode to the connectors and the CRTC to give them a chance to
> * adjust it according to limitations or connector properties, and also
> * a chance to reject the mode entirely.
> @@ -7817,6 +7814,11 @@ encoder_retry:
> }
> }
>
> + /* Set default port clock if not overwritten by the encoder. Needs to be
> + * done afterwards in case the encoder adjusts the mode. */
> + if (!pipe_config->port_clock)
> + pipe_config->port_clock = pipe_config->adjusted_mode.clock;
> +
> ret = intel_crtc_compute_config(crtc, pipe_config);
> if (ret < 0) {
> DRM_DEBUG_KMS("CRTC fixup failed\n");
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 647cc2b..c92eeeb 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -677,7 +677,7 @@ intel_dp_compute_config(struct intel_encoder *encoder,
> int max_clock = intel_dp_max_link_bw(intel_dp) == DP_LINK_BW_2_7 ? 1 : 0;
> int bpp, mode_rate;
> static int bws[2] = { DP_LINK_BW_1_62, DP_LINK_BW_2_7 };
> - int target_clock, link_avail, link_clock;
> + int link_avail, link_clock;
>
> if (HAS_PCH_SPLIT(dev) && !HAS_DDI(dev) && port != PORT_A)
> pipe_config->has_pch_encoder = true;
> @@ -694,8 +694,6 @@ intel_dp_compute_config(struct intel_encoder *encoder,
> intel_pch_panel_fitting(intel_crtc, pipe_config,
> intel_connector->panel.fitting_mode);
> }
> - /* We need to take the panel's fixed mode into account. */
> - target_clock = adjusted_mode->clock;
>
> if (adjusted_mode->flags & DRM_MODE_FLAG_DBLCLK)
> return false;
> @@ -711,7 +709,7 @@ intel_dp_compute_config(struct intel_encoder *encoder,
> bpp = min_t(int, bpp, dev_priv->vbt.edp_bpp);
>
> for (; bpp >= 6*3; bpp -= 2*3) {
> - mode_rate = intel_dp_link_required(target_clock, bpp);
> + mode_rate = intel_dp_link_required(adjusted_mode->clock, bpp);
>
> for (clock = 0; clock <= max_clock; clock++) {
> for (lane_count = 1; lane_count <= max_lane_count; lane_count <<= 1) {
> @@ -746,18 +744,17 @@ found:
>
> intel_dp->link_bw = bws[clock];
> intel_dp->lane_count = lane_count;
> - adjusted_mode->clock = drm_dp_bw_code_to_link_rate(intel_dp->link_bw);
> pipe_config->pipe_bpp = bpp;
> - pipe_config->pixel_target_clock = target_clock;
> + pipe_config->port_clock = drm_dp_bw_code_to_link_rate(intel_dp->link_bw);
>
> DRM_DEBUG_KMS("DP link bw %02x lane count %d clock %d bpp %d\n",
> intel_dp->link_bw, intel_dp->lane_count,
> - adjusted_mode->clock, bpp);
> + pipe_config->port_clock, bpp);
> DRM_DEBUG_KMS("DP link bw required %i available %i\n",
> mode_rate, link_avail);
>
> intel_link_compute_m_n(bpp, lane_count,
> - target_clock, adjusted_mode->clock,
> + adjusted_mode->clock, pipe_config->port_clock,
> &pipe_config->dp_m_n);
>
> intel_dp_set_clock(encoder, pipe_config, intel_dp->link_bw);
> @@ -788,12 +785,11 @@ static void ironlake_set_pll_cpu_edp(struct intel_dp *intel_dp)
> struct drm_i915_private *dev_priv = dev->dev_private;
> u32 dpa_ctl;
>
> - DRM_DEBUG_KMS("eDP PLL enable for clock %d\n",
> - crtc->config.adjusted_mode.clock);
> + DRM_DEBUG_KMS("eDP PLL enable for clock %d\n", crtc->config.port_clock);
> dpa_ctl = I915_READ(DP_A);
> dpa_ctl &= ~DP_PLL_FREQ_MASK;
>
> - if (crtc->config.adjusted_mode.clock == 162000) {
> + if (crtc->config.port_clock == 162000) {
> /* For a long time we've carried around a ILK-DevA w/a for the
> * 160MHz clock. If we're really unlucky, it's still required.
> */
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index fdf6303..afda71f 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -243,12 +243,13 @@ struct intel_crtc_config {
>
> int pipe_bpp;
> struct intel_link_m_n dp_m_n;
> - /**
> - * This is currently used by DP and HDMI encoders since those can have a
> - * target pixel clock != the port link clock (which is currently stored
> - * in adjusted_mode->clock).
> +
> + /*
> + * Frequence the dpll for the port should run at. Differs from the
> + * adjusted dotclock e.g. for DP or 12bpc hdmi mode.
> */
> - int pixel_target_clock;
> + int port_clock;
> +
> /* Used by SDVO (and if we ever fix it, HDMI). */
> unsigned pixel_multiplier;
>
> @@ -786,7 +787,7 @@ extern void intel_ddi_disable_transcoder_func(struct drm_i915_private *dev_priv,
> extern void intel_ddi_enable_pipe_clock(struct intel_crtc *intel_crtc);
> extern void intel_ddi_disable_pipe_clock(struct intel_crtc *intel_crtc);
> extern void intel_ddi_setup_hw_pll_state(struct drm_device *dev);
> -extern bool intel_ddi_pll_mode_set(struct drm_crtc *crtc, int clock);
> +extern bool intel_ddi_pll_mode_set(struct drm_crtc *crtc);
> extern void intel_ddi_put_crtc_pll(struct drm_crtc *crtc);
> extern void intel_ddi_set_pipe_settings(struct drm_crtc *crtc);
> extern void intel_ddi_prepare_link_retrain(struct drm_encoder *encoder);
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> index 8062a92..bc12518 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -835,9 +835,7 @@ bool intel_hdmi_compute_config(struct intel_encoder *encoder,
> desired_bpp = 12*3;
>
> /* Need to adjust the port link by 1.5x for 12bpc. */
> - adjusted_mode->clock = clock_12bpc;
> - pipe_config->pixel_target_clock =
> - pipe_config->requested_mode.clock;
> + pipe_config->port_clock = clock_12bpc;
> } else {
> DRM_DEBUG_KMS("picking bpc to 8 for HDMI output\n");
> desired_bpp = 8*3;
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 49a1887..4126fb1 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -2078,10 +2078,7 @@ static uint32_t hsw_wm_get_pixel_rate(struct drm_device *dev,
> struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> uint32_t pixel_rate, pfit_size;
>
> - if (intel_crtc->config.pixel_target_clock)
> - pixel_rate = intel_crtc->config.pixel_target_clock;
> - else
> - pixel_rate = intel_crtc->config.adjusted_mode.clock;
> + pixel_rate = intel_crtc->config.adjusted_mode.clock;
>
> /* We only use IF-ID interlacing. If we ever use PF-ID we'll need to
> * adjust the pixel_rate here. */
> --
> 1.7.11.7
>
--
Paulo Zanoni
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH 5/7] drm/i915: store adjusted dotclock in adjusted_mode->clock
2013-06-03 16:54 ` Paulo Zanoni
@ 2013-06-03 16:56 ` Paulo Zanoni
2013-06-04 12:01 ` Daniel Vetter
1 sibling, 0 replies; 28+ messages in thread
From: Paulo Zanoni @ 2013-06-03 16:56 UTC (permalink / raw)
To: Daniel Vetter; +Cc: Intel Graphics Development
2013/6/3 Paulo Zanoni <przanoni@gmail.com>:
> 2013/6/1 Daniel Vetter <daniel.vetter@ffwll.ch>:
>> ... not the port clock. This allows us to kill the funny semantics
>> around pixel_target_clock.
>>
>> Since the dpll code still needs the real port clock, add a new
>> port_clock field to the pipe configuration. Handling the default case
>> for that one is a bit tricky, since encoders might not consistently
>> overwrite it when retrying the crtc/encoder bw arbitrage step in the
>> compute config stage. Hence we need to always clear port_clock and
>> update it again if the encoder hasn't put in something more specific.
>> This can't be done in one step since the encoder might want to adjust
>> the mode first.
>>
>> I was a bit on the fence whether I should subsume the pixel multiplier
>> handling into the port_clock, too. But then I decided against this
>> since it's on an abstract level still the dotclock of the adjusted
>> mode, and only our hw makes it a bit special due to the separate pixel
>> mulitplier setting (which requires that the dpll runs at the
>> non-multiplied dotclock).
>>
>> So after this patch the adjusted_mode accurately describes the mode we
>> feed into the port, after the panel fitter and pixel multiplier (or
>> line doubling, if we ever bother with that) have done their job.
>> Since the fdi link is between the pfit and the pixel multiplier steps
>> we need to be careful with calculating the fdi link config.
>>
>> v2: Fix up ilk cpu pll handling.
>>
>> v3: Introduce an fdi_dotclock variable in ironlake_fdi_compute_config
>> to make it clearer that we transmit the adjusted_mode without the
>> pixel multiplier taken into account. The old code multiplied the the
>> available link bw with the pixel multiplier, which results in the same
>> fdi configuration, but is much more confusing.
>>
>> v4: Rebase on top of Imre's is_cpu_edp removal.
>>
>> v5: Rebase on top of Paulo's haswell watermark fixes, which introduce
>> a new place which looked at the pixel_clock and so needed conversion.
>>
>> v6: Split out prep patches as requested by Paulo Zanoni. Also rebase
>> on top of the fdi dotclock handling fix in the fdi lanes/bw
>> computation code.
>>
>> Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org> (v3)
>> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>
> Due to my comments on patch 3, there's a possibility you may need to
> rebase this patch.
>
> My only optional bikeshed is to print port_clock inside
> intel_dump_pipe_config. If you rebase, you may consider doing it :)
>
> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com> (v6)
>
>> ---
>> drivers/gpu/drm/i915/intel_ddi.c | 3 ++-
>> drivers/gpu/drm/i915/intel_display.c | 32 +++++++++++++++++---------------
>> drivers/gpu/drm/i915/intel_dp.c | 18 +++++++-----------
>> drivers/gpu/drm/i915/intel_drv.h | 13 +++++++------
>> drivers/gpu/drm/i915/intel_hdmi.c | 4 +---
>> drivers/gpu/drm/i915/intel_pm.c | 5 +----
>> 6 files changed, 35 insertions(+), 40 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
>> index 9649df8..486c46b 100644
>> --- a/drivers/gpu/drm/i915/intel_ddi.c
>> +++ b/drivers/gpu/drm/i915/intel_ddi.c
>> @@ -624,7 +624,7 @@ intel_ddi_calculate_wrpll(int clock /* in Hz */,
>> clock, *p_out, *n2_out, *r2_out);
>> }
>>
>> -bool intel_ddi_pll_mode_set(struct drm_crtc *crtc, int clock)
>> +bool intel_ddi_pll_mode_set(struct drm_crtc *crtc)
>> {
>> struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>> struct intel_encoder *intel_encoder = intel_ddi_get_crtc_encoder(crtc);
>> @@ -634,6 +634,7 @@ bool intel_ddi_pll_mode_set(struct drm_crtc *crtc, int clock)
>> int type = intel_encoder->type;
>> enum pipe pipe = intel_crtc->pipe;
>> uint32_t reg, val;
>> + int clock = intel_crtc->config.port_clock;
>>
>> /* TODO: reuse PLLs when possible (compare values) */
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 761254d..103f4e9 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -3992,7 +3992,7 @@ static int ironlake_fdi_compute_config(struct intel_crtc *intel_crtc,
>> {
>> struct drm_device *dev = intel_crtc->base.dev;
>> struct drm_display_mode *adjusted_mode = &pipe_config->adjusted_mode;
>> - int target_clock, lane, link_bw, fdi_dotclock;
>> + int lane, link_bw, fdi_dotclock;
>> bool setup_ok, needs_recompute = false;
>>
>> retry:
>> @@ -4005,12 +4005,7 @@ retry:
>> */
>> link_bw = intel_fdi_link_freq(dev) * MHz(100)/KHz(1)/10;
>>
>> - if (pipe_config->pixel_target_clock)
>> - target_clock = pipe_config->pixel_target_clock;
>> - else
>> - target_clock = adjusted_mode->clock;
>> -
>> - fdi_dotclock = target_clock;
>> + fdi_dotclock = adjusted_mode->clock;
>
> In the patch you sent Friday, you used adjusted_mode->clock instead of
> fdi_dotclock as the parameter to ironlake_get_lanes_required, you only
> initialized fdi_dotclock before calling intel_link_compute_m_n. This
> doesn't change the code behavior, I'm just pointing in case you
> changed without noticing and prefer the older version.
Completely ignore this paragraph and read the comment inside patch 3.
I forgot to remove the paragraph before sending the email. Sorry.
>
>
>> if (pipe_config->pixel_multiplier > 1)
>> fdi_dotclock /= pipe_config->pixel_multiplier;
>>
>> @@ -4360,8 +4355,6 @@ static void vlv_update_pll(struct intel_crtc *crtc)
>> {
>> struct drm_device *dev = crtc->base.dev;
>> struct drm_i915_private *dev_priv = dev->dev_private;
>> - struct drm_display_mode *adjusted_mode =
>> - &crtc->config.adjusted_mode;
>> struct intel_encoder *encoder;
>> int pipe = crtc->pipe;
>> u32 dpll, mdiv;
>> @@ -4414,7 +4407,7 @@ static void vlv_update_pll(struct intel_crtc *crtc)
>> vlv_dpio_write(dev_priv, DPIO_DIV(pipe), mdiv);
>>
>> /* Set HBR and RBR LPF coefficients */
>> - if (adjusted_mode->clock == 162000 ||
>> + if (crtc->config.port_clock == 162000 ||
>> intel_pipe_has_type(&crtc->base, INTEL_OUTPUT_HDMI))
>> vlv_dpio_write(dev_priv, DPIO_LFP_COEFF(pipe),
>> 0x005f0021);
>> @@ -4859,7 +4852,8 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
>> * reflck * (5 * (m1 + 2) + (m2 + 2)) / (n + 2) / p1 / p2.
>> */
>> limit = intel_limit(crtc, refclk);
>> - ok = dev_priv->display.find_dpll(limit, crtc, adjusted_mode->clock,
>> + ok = dev_priv->display.find_dpll(limit, crtc,
>> + intel_crtc->config.port_clock,
>> refclk, NULL, &clock);
>> if (!ok) {
>> DRM_ERROR("Couldn't find PLL settings for mode!\n");
>> @@ -5467,7 +5461,6 @@ static void haswell_set_pipeconf(struct drm_crtc *crtc)
>> }
>>
>> static bool ironlake_compute_clocks(struct drm_crtc *crtc,
>> - struct drm_display_mode *adjusted_mode,
>> intel_clock_t *clock,
>> bool *has_reduced_clock,
>> intel_clock_t *reduced_clock)
>> @@ -5495,7 +5488,8 @@ static bool ironlake_compute_clocks(struct drm_crtc *crtc,
>> * reflck * (5 * (m1 + 2) + (m2 + 2)) / (n + 2) / p1 / p2.
>> */
>> limit = intel_limit(crtc, refclk);
>> - ret = dev_priv->display.find_dpll(limit, crtc, adjusted_mode->clock,
>> + ret = dev_priv->display.find_dpll(limit, crtc,
>> + to_intel_crtc(crtc)->config.port_clock,
>> refclk, NULL, clock);
>> if (!ret)
>> return false;
>> @@ -5695,7 +5689,7 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
>> WARN(!(HAS_PCH_IBX(dev) || HAS_PCH_CPT(dev)),
>> "Unexpected PCH type %d\n", INTEL_PCH_TYPE(dev));
>>
>> - ok = ironlake_compute_clocks(crtc, adjusted_mode, &clock,
>> + ok = ironlake_compute_clocks(crtc, &clock,
>> &has_reduced_clock, &reduced_clock);
>> if (!ok) {
>> DRM_ERROR("Couldn't find PLL settings for mode!\n");
>> @@ -5898,7 +5892,7 @@ static int haswell_crtc_mode_set(struct drm_crtc *crtc,
>> WARN(num_connectors != 1, "%d connectors attached to pipe %c\n",
>> num_connectors, pipe_name(pipe));
>>
>> - if (!intel_ddi_pll_mode_set(crtc, adjusted_mode->clock))
>> + if (!intel_ddi_pll_mode_set(crtc))
>> return -EINVAL;
>>
>> /* Ensure that the cursor is valid for the new mode before changing... */
>> @@ -7789,6 +7783,9 @@ intel_modeset_pipe_config(struct drm_crtc *crtc,
>> goto fail;
>>
>> encoder_retry:
>> + /* Ensure the port clock default is reset when retrying. */
>> + pipe_config->port_clock = 0;
>> +
>> /* Pass our mode to the connectors and the CRTC to give them a chance to
>> * adjust it according to limitations or connector properties, and also
>> * a chance to reject the mode entirely.
>> @@ -7817,6 +7814,11 @@ encoder_retry:
>> }
>> }
>>
>> + /* Set default port clock if not overwritten by the encoder. Needs to be
>> + * done afterwards in case the encoder adjusts the mode. */
>> + if (!pipe_config->port_clock)
>> + pipe_config->port_clock = pipe_config->adjusted_mode.clock;
>> +
>> ret = intel_crtc_compute_config(crtc, pipe_config);
>> if (ret < 0) {
>> DRM_DEBUG_KMS("CRTC fixup failed\n");
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> index 647cc2b..c92eeeb 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -677,7 +677,7 @@ intel_dp_compute_config(struct intel_encoder *encoder,
>> int max_clock = intel_dp_max_link_bw(intel_dp) == DP_LINK_BW_2_7 ? 1 : 0;
>> int bpp, mode_rate;
>> static int bws[2] = { DP_LINK_BW_1_62, DP_LINK_BW_2_7 };
>> - int target_clock, link_avail, link_clock;
>> + int link_avail, link_clock;
>>
>> if (HAS_PCH_SPLIT(dev) && !HAS_DDI(dev) && port != PORT_A)
>> pipe_config->has_pch_encoder = true;
>> @@ -694,8 +694,6 @@ intel_dp_compute_config(struct intel_encoder *encoder,
>> intel_pch_panel_fitting(intel_crtc, pipe_config,
>> intel_connector->panel.fitting_mode);
>> }
>> - /* We need to take the panel's fixed mode into account. */
>> - target_clock = adjusted_mode->clock;
>>
>> if (adjusted_mode->flags & DRM_MODE_FLAG_DBLCLK)
>> return false;
>> @@ -711,7 +709,7 @@ intel_dp_compute_config(struct intel_encoder *encoder,
>> bpp = min_t(int, bpp, dev_priv->vbt.edp_bpp);
>>
>> for (; bpp >= 6*3; bpp -= 2*3) {
>> - mode_rate = intel_dp_link_required(target_clock, bpp);
>> + mode_rate = intel_dp_link_required(adjusted_mode->clock, bpp);
>>
>> for (clock = 0; clock <= max_clock; clock++) {
>> for (lane_count = 1; lane_count <= max_lane_count; lane_count <<= 1) {
>> @@ -746,18 +744,17 @@ found:
>>
>> intel_dp->link_bw = bws[clock];
>> intel_dp->lane_count = lane_count;
>> - adjusted_mode->clock = drm_dp_bw_code_to_link_rate(intel_dp->link_bw);
>> pipe_config->pipe_bpp = bpp;
>> - pipe_config->pixel_target_clock = target_clock;
>> + pipe_config->port_clock = drm_dp_bw_code_to_link_rate(intel_dp->link_bw);
>>
>> DRM_DEBUG_KMS("DP link bw %02x lane count %d clock %d bpp %d\n",
>> intel_dp->link_bw, intel_dp->lane_count,
>> - adjusted_mode->clock, bpp);
>> + pipe_config->port_clock, bpp);
>> DRM_DEBUG_KMS("DP link bw required %i available %i\n",
>> mode_rate, link_avail);
>>
>> intel_link_compute_m_n(bpp, lane_count,
>> - target_clock, adjusted_mode->clock,
>> + adjusted_mode->clock, pipe_config->port_clock,
>> &pipe_config->dp_m_n);
>>
>> intel_dp_set_clock(encoder, pipe_config, intel_dp->link_bw);
>> @@ -788,12 +785,11 @@ static void ironlake_set_pll_cpu_edp(struct intel_dp *intel_dp)
>> struct drm_i915_private *dev_priv = dev->dev_private;
>> u32 dpa_ctl;
>>
>> - DRM_DEBUG_KMS("eDP PLL enable for clock %d\n",
>> - crtc->config.adjusted_mode.clock);
>> + DRM_DEBUG_KMS("eDP PLL enable for clock %d\n", crtc->config.port_clock);
>> dpa_ctl = I915_READ(DP_A);
>> dpa_ctl &= ~DP_PLL_FREQ_MASK;
>>
>> - if (crtc->config.adjusted_mode.clock == 162000) {
>> + if (crtc->config.port_clock == 162000) {
>> /* For a long time we've carried around a ILK-DevA w/a for the
>> * 160MHz clock. If we're really unlucky, it's still required.
>> */
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index fdf6303..afda71f 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -243,12 +243,13 @@ struct intel_crtc_config {
>>
>> int pipe_bpp;
>> struct intel_link_m_n dp_m_n;
>> - /**
>> - * This is currently used by DP and HDMI encoders since those can have a
>> - * target pixel clock != the port link clock (which is currently stored
>> - * in adjusted_mode->clock).
>> +
>> + /*
>> + * Frequence the dpll for the port should run at. Differs from the
>> + * adjusted dotclock e.g. for DP or 12bpc hdmi mode.
>> */
>> - int pixel_target_clock;
>> + int port_clock;
>> +
>> /* Used by SDVO (and if we ever fix it, HDMI). */
>> unsigned pixel_multiplier;
>>
>> @@ -786,7 +787,7 @@ extern void intel_ddi_disable_transcoder_func(struct drm_i915_private *dev_priv,
>> extern void intel_ddi_enable_pipe_clock(struct intel_crtc *intel_crtc);
>> extern void intel_ddi_disable_pipe_clock(struct intel_crtc *intel_crtc);
>> extern void intel_ddi_setup_hw_pll_state(struct drm_device *dev);
>> -extern bool intel_ddi_pll_mode_set(struct drm_crtc *crtc, int clock);
>> +extern bool intel_ddi_pll_mode_set(struct drm_crtc *crtc);
>> extern void intel_ddi_put_crtc_pll(struct drm_crtc *crtc);
>> extern void intel_ddi_set_pipe_settings(struct drm_crtc *crtc);
>> extern void intel_ddi_prepare_link_retrain(struct drm_encoder *encoder);
>> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
>> index 8062a92..bc12518 100644
>> --- a/drivers/gpu/drm/i915/intel_hdmi.c
>> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
>> @@ -835,9 +835,7 @@ bool intel_hdmi_compute_config(struct intel_encoder *encoder,
>> desired_bpp = 12*3;
>>
>> /* Need to adjust the port link by 1.5x for 12bpc. */
>> - adjusted_mode->clock = clock_12bpc;
>> - pipe_config->pixel_target_clock =
>> - pipe_config->requested_mode.clock;
>> + pipe_config->port_clock = clock_12bpc;
>> } else {
>> DRM_DEBUG_KMS("picking bpc to 8 for HDMI output\n");
>> desired_bpp = 8*3;
>> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
>> index 49a1887..4126fb1 100644
>> --- a/drivers/gpu/drm/i915/intel_pm.c
>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>> @@ -2078,10 +2078,7 @@ static uint32_t hsw_wm_get_pixel_rate(struct drm_device *dev,
>> struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>> uint32_t pixel_rate, pfit_size;
>>
>> - if (intel_crtc->config.pixel_target_clock)
>> - pixel_rate = intel_crtc->config.pixel_target_clock;
>> - else
>> - pixel_rate = intel_crtc->config.adjusted_mode.clock;
>> + pixel_rate = intel_crtc->config.adjusted_mode.clock;
>>
>> /* We only use IF-ID interlacing. If we ever use PF-ID we'll need to
>> * adjust the pixel_rate here. */
>> --
>> 1.7.11.7
>>
>
>
>
> --
> Paulo Zanoni
--
Paulo Zanoni
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH 5/7] drm/i915: store adjusted dotclock in adjusted_mode->clock
2013-06-03 16:54 ` Paulo Zanoni
2013-06-03 16:56 ` Paulo Zanoni
@ 2013-06-04 12:01 ` Daniel Vetter
1 sibling, 0 replies; 28+ messages in thread
From: Daniel Vetter @ 2013-06-04 12:01 UTC (permalink / raw)
To: Paulo Zanoni; +Cc: Daniel Vetter, Intel Graphics Development
On Mon, Jun 03, 2013 at 01:54:40PM -0300, Paulo Zanoni wrote:
> 2013/6/1 Daniel Vetter <daniel.vetter@ffwll.ch>:
> > ... not the port clock. This allows us to kill the funny semantics
> > around pixel_target_clock.
> >
> > Since the dpll code still needs the real port clock, add a new
> > port_clock field to the pipe configuration. Handling the default case
> > for that one is a bit tricky, since encoders might not consistently
> > overwrite it when retrying the crtc/encoder bw arbitrage step in the
> > compute config stage. Hence we need to always clear port_clock and
> > update it again if the encoder hasn't put in something more specific.
> > This can't be done in one step since the encoder might want to adjust
> > the mode first.
> >
> > I was a bit on the fence whether I should subsume the pixel multiplier
> > handling into the port_clock, too. But then I decided against this
> > since it's on an abstract level still the dotclock of the adjusted
> > mode, and only our hw makes it a bit special due to the separate pixel
> > mulitplier setting (which requires that the dpll runs at the
> > non-multiplied dotclock).
> >
> > So after this patch the adjusted_mode accurately describes the mode we
> > feed into the port, after the panel fitter and pixel multiplier (or
> > line doubling, if we ever bother with that) have done their job.
> > Since the fdi link is between the pfit and the pixel multiplier steps
> > we need to be careful with calculating the fdi link config.
> >
> > v2: Fix up ilk cpu pll handling.
> >
> > v3: Introduce an fdi_dotclock variable in ironlake_fdi_compute_config
> > to make it clearer that we transmit the adjusted_mode without the
> > pixel multiplier taken into account. The old code multiplied the the
> > available link bw with the pixel multiplier, which results in the same
> > fdi configuration, but is much more confusing.
> >
> > v4: Rebase on top of Imre's is_cpu_edp removal.
> >
> > v5: Rebase on top of Paulo's haswell watermark fixes, which introduce
> > a new place which looked at the pixel_clock and so needed conversion.
> >
> > v6: Split out prep patches as requested by Paulo Zanoni. Also rebase
> > on top of the fdi dotclock handling fix in the fdi lanes/bw
> > computation code.
> >
> > Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org> (v3)
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>
> Due to my comments on patch 3, there's a possibility you may need to
> rebase this patch.
>
> My only optional bikeshed is to print port_clock inside
> intel_dump_pipe_config. If you rebase, you may consider doing it :)
Yeah, I think I'll follow up with another pipe config dump patch to add
lots more stuff. Already screamed around because it wasn't as complete as
I've wished for ;-)
-Daniel
>
> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com> (v6)
>
> > ---
> > drivers/gpu/drm/i915/intel_ddi.c | 3 ++-
> > drivers/gpu/drm/i915/intel_display.c | 32 +++++++++++++++++---------------
> > drivers/gpu/drm/i915/intel_dp.c | 18 +++++++-----------
> > drivers/gpu/drm/i915/intel_drv.h | 13 +++++++------
> > drivers/gpu/drm/i915/intel_hdmi.c | 4 +---
> > drivers/gpu/drm/i915/intel_pm.c | 5 +----
> > 6 files changed, 35 insertions(+), 40 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> > index 9649df8..486c46b 100644
> > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > @@ -624,7 +624,7 @@ intel_ddi_calculate_wrpll(int clock /* in Hz */,
> > clock, *p_out, *n2_out, *r2_out);
> > }
> >
> > -bool intel_ddi_pll_mode_set(struct drm_crtc *crtc, int clock)
> > +bool intel_ddi_pll_mode_set(struct drm_crtc *crtc)
> > {
> > struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > struct intel_encoder *intel_encoder = intel_ddi_get_crtc_encoder(crtc);
> > @@ -634,6 +634,7 @@ bool intel_ddi_pll_mode_set(struct drm_crtc *crtc, int clock)
> > int type = intel_encoder->type;
> > enum pipe pipe = intel_crtc->pipe;
> > uint32_t reg, val;
> > + int clock = intel_crtc->config.port_clock;
> >
> > /* TODO: reuse PLLs when possible (compare values) */
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 761254d..103f4e9 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -3992,7 +3992,7 @@ static int ironlake_fdi_compute_config(struct intel_crtc *intel_crtc,
> > {
> > struct drm_device *dev = intel_crtc->base.dev;
> > struct drm_display_mode *adjusted_mode = &pipe_config->adjusted_mode;
> > - int target_clock, lane, link_bw, fdi_dotclock;
> > + int lane, link_bw, fdi_dotclock;
> > bool setup_ok, needs_recompute = false;
> >
> > retry:
> > @@ -4005,12 +4005,7 @@ retry:
> > */
> > link_bw = intel_fdi_link_freq(dev) * MHz(100)/KHz(1)/10;
> >
> > - if (pipe_config->pixel_target_clock)
> > - target_clock = pipe_config->pixel_target_clock;
> > - else
> > - target_clock = adjusted_mode->clock;
> > -
> > - fdi_dotclock = target_clock;
> > + fdi_dotclock = adjusted_mode->clock;
>
> In the patch you sent Friday, you used adjusted_mode->clock instead of
> fdi_dotclock as the parameter to ironlake_get_lanes_required, you only
> initialized fdi_dotclock before calling intel_link_compute_m_n. This
> doesn't change the code behavior, I'm just pointing in case you
> changed without noticing and prefer the older version.
>
>
> > if (pipe_config->pixel_multiplier > 1)
> > fdi_dotclock /= pipe_config->pixel_multiplier;
> >
> > @@ -4360,8 +4355,6 @@ static void vlv_update_pll(struct intel_crtc *crtc)
> > {
> > struct drm_device *dev = crtc->base.dev;
> > struct drm_i915_private *dev_priv = dev->dev_private;
> > - struct drm_display_mode *adjusted_mode =
> > - &crtc->config.adjusted_mode;
> > struct intel_encoder *encoder;
> > int pipe = crtc->pipe;
> > u32 dpll, mdiv;
> > @@ -4414,7 +4407,7 @@ static void vlv_update_pll(struct intel_crtc *crtc)
> > vlv_dpio_write(dev_priv, DPIO_DIV(pipe), mdiv);
> >
> > /* Set HBR and RBR LPF coefficients */
> > - if (adjusted_mode->clock == 162000 ||
> > + if (crtc->config.port_clock == 162000 ||
> > intel_pipe_has_type(&crtc->base, INTEL_OUTPUT_HDMI))
> > vlv_dpio_write(dev_priv, DPIO_LFP_COEFF(pipe),
> > 0x005f0021);
> > @@ -4859,7 +4852,8 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
> > * reflck * (5 * (m1 + 2) + (m2 + 2)) / (n + 2) / p1 / p2.
> > */
> > limit = intel_limit(crtc, refclk);
> > - ok = dev_priv->display.find_dpll(limit, crtc, adjusted_mode->clock,
> > + ok = dev_priv->display.find_dpll(limit, crtc,
> > + intel_crtc->config.port_clock,
> > refclk, NULL, &clock);
> > if (!ok) {
> > DRM_ERROR("Couldn't find PLL settings for mode!\n");
> > @@ -5467,7 +5461,6 @@ static void haswell_set_pipeconf(struct drm_crtc *crtc)
> > }
> >
> > static bool ironlake_compute_clocks(struct drm_crtc *crtc,
> > - struct drm_display_mode *adjusted_mode,
> > intel_clock_t *clock,
> > bool *has_reduced_clock,
> > intel_clock_t *reduced_clock)
> > @@ -5495,7 +5488,8 @@ static bool ironlake_compute_clocks(struct drm_crtc *crtc,
> > * reflck * (5 * (m1 + 2) + (m2 + 2)) / (n + 2) / p1 / p2.
> > */
> > limit = intel_limit(crtc, refclk);
> > - ret = dev_priv->display.find_dpll(limit, crtc, adjusted_mode->clock,
> > + ret = dev_priv->display.find_dpll(limit, crtc,
> > + to_intel_crtc(crtc)->config.port_clock,
> > refclk, NULL, clock);
> > if (!ret)
> > return false;
> > @@ -5695,7 +5689,7 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
> > WARN(!(HAS_PCH_IBX(dev) || HAS_PCH_CPT(dev)),
> > "Unexpected PCH type %d\n", INTEL_PCH_TYPE(dev));
> >
> > - ok = ironlake_compute_clocks(crtc, adjusted_mode, &clock,
> > + ok = ironlake_compute_clocks(crtc, &clock,
> > &has_reduced_clock, &reduced_clock);
> > if (!ok) {
> > DRM_ERROR("Couldn't find PLL settings for mode!\n");
> > @@ -5898,7 +5892,7 @@ static int haswell_crtc_mode_set(struct drm_crtc *crtc,
> > WARN(num_connectors != 1, "%d connectors attached to pipe %c\n",
> > num_connectors, pipe_name(pipe));
> >
> > - if (!intel_ddi_pll_mode_set(crtc, adjusted_mode->clock))
> > + if (!intel_ddi_pll_mode_set(crtc))
> > return -EINVAL;
> >
> > /* Ensure that the cursor is valid for the new mode before changing... */
> > @@ -7789,6 +7783,9 @@ intel_modeset_pipe_config(struct drm_crtc *crtc,
> > goto fail;
> >
> > encoder_retry:
> > + /* Ensure the port clock default is reset when retrying. */
> > + pipe_config->port_clock = 0;
> > +
> > /* Pass our mode to the connectors and the CRTC to give them a chance to
> > * adjust it according to limitations or connector properties, and also
> > * a chance to reject the mode entirely.
> > @@ -7817,6 +7814,11 @@ encoder_retry:
> > }
> > }
> >
> > + /* Set default port clock if not overwritten by the encoder. Needs to be
> > + * done afterwards in case the encoder adjusts the mode. */
> > + if (!pipe_config->port_clock)
> > + pipe_config->port_clock = pipe_config->adjusted_mode.clock;
> > +
> > ret = intel_crtc_compute_config(crtc, pipe_config);
> > if (ret < 0) {
> > DRM_DEBUG_KMS("CRTC fixup failed\n");
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index 647cc2b..c92eeeb 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -677,7 +677,7 @@ intel_dp_compute_config(struct intel_encoder *encoder,
> > int max_clock = intel_dp_max_link_bw(intel_dp) == DP_LINK_BW_2_7 ? 1 : 0;
> > int bpp, mode_rate;
> > static int bws[2] = { DP_LINK_BW_1_62, DP_LINK_BW_2_7 };
> > - int target_clock, link_avail, link_clock;
> > + int link_avail, link_clock;
> >
> > if (HAS_PCH_SPLIT(dev) && !HAS_DDI(dev) && port != PORT_A)
> > pipe_config->has_pch_encoder = true;
> > @@ -694,8 +694,6 @@ intel_dp_compute_config(struct intel_encoder *encoder,
> > intel_pch_panel_fitting(intel_crtc, pipe_config,
> > intel_connector->panel.fitting_mode);
> > }
> > - /* We need to take the panel's fixed mode into account. */
> > - target_clock = adjusted_mode->clock;
> >
> > if (adjusted_mode->flags & DRM_MODE_FLAG_DBLCLK)
> > return false;
> > @@ -711,7 +709,7 @@ intel_dp_compute_config(struct intel_encoder *encoder,
> > bpp = min_t(int, bpp, dev_priv->vbt.edp_bpp);
> >
> > for (; bpp >= 6*3; bpp -= 2*3) {
> > - mode_rate = intel_dp_link_required(target_clock, bpp);
> > + mode_rate = intel_dp_link_required(adjusted_mode->clock, bpp);
> >
> > for (clock = 0; clock <= max_clock; clock++) {
> > for (lane_count = 1; lane_count <= max_lane_count; lane_count <<= 1) {
> > @@ -746,18 +744,17 @@ found:
> >
> > intel_dp->link_bw = bws[clock];
> > intel_dp->lane_count = lane_count;
> > - adjusted_mode->clock = drm_dp_bw_code_to_link_rate(intel_dp->link_bw);
> > pipe_config->pipe_bpp = bpp;
> > - pipe_config->pixel_target_clock = target_clock;
> > + pipe_config->port_clock = drm_dp_bw_code_to_link_rate(intel_dp->link_bw);
> >
> > DRM_DEBUG_KMS("DP link bw %02x lane count %d clock %d bpp %d\n",
> > intel_dp->link_bw, intel_dp->lane_count,
> > - adjusted_mode->clock, bpp);
> > + pipe_config->port_clock, bpp);
> > DRM_DEBUG_KMS("DP link bw required %i available %i\n",
> > mode_rate, link_avail);
> >
> > intel_link_compute_m_n(bpp, lane_count,
> > - target_clock, adjusted_mode->clock,
> > + adjusted_mode->clock, pipe_config->port_clock,
> > &pipe_config->dp_m_n);
> >
> > intel_dp_set_clock(encoder, pipe_config, intel_dp->link_bw);
> > @@ -788,12 +785,11 @@ static void ironlake_set_pll_cpu_edp(struct intel_dp *intel_dp)
> > struct drm_i915_private *dev_priv = dev->dev_private;
> > u32 dpa_ctl;
> >
> > - DRM_DEBUG_KMS("eDP PLL enable for clock %d\n",
> > - crtc->config.adjusted_mode.clock);
> > + DRM_DEBUG_KMS("eDP PLL enable for clock %d\n", crtc->config.port_clock);
> > dpa_ctl = I915_READ(DP_A);
> > dpa_ctl &= ~DP_PLL_FREQ_MASK;
> >
> > - if (crtc->config.adjusted_mode.clock == 162000) {
> > + if (crtc->config.port_clock == 162000) {
> > /* For a long time we've carried around a ILK-DevA w/a for the
> > * 160MHz clock. If we're really unlucky, it's still required.
> > */
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index fdf6303..afda71f 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -243,12 +243,13 @@ struct intel_crtc_config {
> >
> > int pipe_bpp;
> > struct intel_link_m_n dp_m_n;
> > - /**
> > - * This is currently used by DP and HDMI encoders since those can have a
> > - * target pixel clock != the port link clock (which is currently stored
> > - * in adjusted_mode->clock).
> > +
> > + /*
> > + * Frequence the dpll for the port should run at. Differs from the
> > + * adjusted dotclock e.g. for DP or 12bpc hdmi mode.
> > */
> > - int pixel_target_clock;
> > + int port_clock;
> > +
> > /* Used by SDVO (and if we ever fix it, HDMI). */
> > unsigned pixel_multiplier;
> >
> > @@ -786,7 +787,7 @@ extern void intel_ddi_disable_transcoder_func(struct drm_i915_private *dev_priv,
> > extern void intel_ddi_enable_pipe_clock(struct intel_crtc *intel_crtc);
> > extern void intel_ddi_disable_pipe_clock(struct intel_crtc *intel_crtc);
> > extern void intel_ddi_setup_hw_pll_state(struct drm_device *dev);
> > -extern bool intel_ddi_pll_mode_set(struct drm_crtc *crtc, int clock);
> > +extern bool intel_ddi_pll_mode_set(struct drm_crtc *crtc);
> > extern void intel_ddi_put_crtc_pll(struct drm_crtc *crtc);
> > extern void intel_ddi_set_pipe_settings(struct drm_crtc *crtc);
> > extern void intel_ddi_prepare_link_retrain(struct drm_encoder *encoder);
> > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> > index 8062a92..bc12518 100644
> > --- a/drivers/gpu/drm/i915/intel_hdmi.c
> > +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> > @@ -835,9 +835,7 @@ bool intel_hdmi_compute_config(struct intel_encoder *encoder,
> > desired_bpp = 12*3;
> >
> > /* Need to adjust the port link by 1.5x for 12bpc. */
> > - adjusted_mode->clock = clock_12bpc;
> > - pipe_config->pixel_target_clock =
> > - pipe_config->requested_mode.clock;
> > + pipe_config->port_clock = clock_12bpc;
> > } else {
> > DRM_DEBUG_KMS("picking bpc to 8 for HDMI output\n");
> > desired_bpp = 8*3;
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index 49a1887..4126fb1 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -2078,10 +2078,7 @@ static uint32_t hsw_wm_get_pixel_rate(struct drm_device *dev,
> > struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > uint32_t pixel_rate, pfit_size;
> >
> > - if (intel_crtc->config.pixel_target_clock)
> > - pixel_rate = intel_crtc->config.pixel_target_clock;
> > - else
> > - pixel_rate = intel_crtc->config.adjusted_mode.clock;
> > + pixel_rate = intel_crtc->config.adjusted_mode.clock;
> >
> > /* We only use IF-ID interlacing. If we ever use PF-ID we'll need to
> > * adjust the pixel_rate here. */
> > --
> > 1.7.11.7
> >
>
>
>
> --
> Paulo Zanoni
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 6/7] drm/i915: Drop some no longer required mode/adjusted_mode parameters
2013-06-01 15:16 [PATCH 0/7] adjusted mode rework and assorted patches Daniel Vetter
` (4 preceding siblings ...)
2013-06-01 15:16 ` [PATCH 5/7] drm/i915: store adjusted dotclock in adjusted_mode->clock Daniel Vetter
@ 2013-06-01 15:16 ` Daniel Vetter
2013-06-03 17:00 ` Paulo Zanoni
2013-06-01 15:16 ` [PATCH 7/7] drm/i915: check for strange pfit pipe assignemnt on ivb/hsw Daniel Vetter
6 siblings, 1 reply; 28+ messages in thread
From: Daniel Vetter @ 2013-06-01 15:16 UTC (permalink / raw)
To: Intel Graphics Development; +Cc: Daniel Vetter
We can get at this easily through intel_crtc->config now.
v2: Drop more stuff gcc spotted.
v3: Drop even more stuff gcc spotted.
v4: Yet more ...
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
drivers/gpu/drm/i915/intel_display.c | 24 ++++++++----------------
1 file changed, 8 insertions(+), 16 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 103f4e9..432e699 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4580,7 +4580,6 @@ static void i9xx_update_pll(struct intel_crtc *crtc,
}
static void i8xx_update_pll(struct intel_crtc *crtc,
- struct drm_display_mode *adjusted_mode,
intel_clock_t *reduced_clock,
int num_connectors)
{
@@ -4635,14 +4634,15 @@ static void i8xx_update_pll(struct intel_crtc *crtc,
I915_WRITE(DPLL(pipe), dpll);
}
-static void intel_set_pipe_timings(struct intel_crtc *intel_crtc,
- struct drm_display_mode *mode,
- struct drm_display_mode *adjusted_mode)
+static void intel_set_pipe_timings(struct intel_crtc *intel_crtc)
{
struct drm_device *dev = intel_crtc->base.dev;
struct drm_i915_private *dev_priv = dev->dev_private;
enum pipe pipe = intel_crtc->pipe;
enum transcoder cpu_transcoder = intel_crtc->config.cpu_transcoder;
+ struct drm_display_mode *adjusted_mode =
+ &intel_crtc->config.adjusted_mode;
+ struct drm_display_mode *mode = &intel_crtc->config.requested_mode;
uint32_t vsyncshift, crtc_vtotal, crtc_vblank_end;
/* We need to be careful not to changed the adjusted mode, for otherwise
@@ -4820,8 +4820,6 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
struct drm_device *dev = crtc->dev;
struct drm_i915_private *dev_priv = dev->dev_private;
struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
- struct drm_display_mode *adjusted_mode =
- &intel_crtc->config.adjusted_mode;
struct drm_display_mode *mode = &intel_crtc->config.requested_mode;
int pipe = intel_crtc->pipe;
int plane = intel_crtc->plane;
@@ -4886,7 +4884,7 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
}
if (IS_GEN2(dev))
- i8xx_update_pll(intel_crtc, adjusted_mode,
+ i8xx_update_pll(intel_crtc,
has_reduced_clock ? &reduced_clock : NULL,
num_connectors);
else if (IS_VALLEYVIEW(dev))
@@ -4906,7 +4904,7 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
dspcntr |= DISPPLANE_SEL_PIPE_B;
}
- intel_set_pipe_timings(intel_crtc, mode, adjusted_mode);
+ intel_set_pipe_timings(intel_crtc);
/* pipesrc and dspsize control the size that is scaled from,
* which should always be the user's requested size.
@@ -5663,9 +5661,6 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
struct drm_device *dev = crtc->dev;
struct drm_i915_private *dev_priv = dev->dev_private;
struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
- struct drm_display_mode *adjusted_mode =
- &intel_crtc->config.adjusted_mode;
- struct drm_display_mode *mode = &intel_crtc->config.requested_mode;
int pipe = intel_crtc->pipe;
int plane = intel_crtc->plane;
int num_connectors = 0;
@@ -5760,7 +5755,7 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
}
}
- intel_set_pipe_timings(intel_crtc, mode, adjusted_mode);
+ intel_set_pipe_timings(intel_crtc);
if (intel_crtc->config.has_pch_encoder) {
intel_cpu_transcoder_set_m_n(intel_crtc,
@@ -5868,9 +5863,6 @@ static int haswell_crtc_mode_set(struct drm_crtc *crtc,
struct drm_device *dev = crtc->dev;
struct drm_i915_private *dev_priv = dev->dev_private;
struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
- struct drm_display_mode *adjusted_mode =
- &intel_crtc->config.adjusted_mode;
- struct drm_display_mode *mode = &intel_crtc->config.requested_mode;
int pipe = intel_crtc->pipe;
int plane = intel_crtc->plane;
int num_connectors = 0;
@@ -5903,7 +5895,7 @@ static int haswell_crtc_mode_set(struct drm_crtc *crtc,
intel_crtc->lowfreq_avail = false;
- intel_set_pipe_timings(intel_crtc, mode, adjusted_mode);
+ intel_set_pipe_timings(intel_crtc);
if (intel_crtc->config.has_pch_encoder) {
intel_cpu_transcoder_set_m_n(intel_crtc,
--
1.7.11.7
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [PATCH 6/7] drm/i915: Drop some no longer required mode/adjusted_mode parameters
2013-06-01 15:16 ` [PATCH 6/7] drm/i915: Drop some no longer required mode/adjusted_mode parameters Daniel Vetter
@ 2013-06-03 17:00 ` Paulo Zanoni
0 siblings, 0 replies; 28+ messages in thread
From: Paulo Zanoni @ 2013-06-03 17:00 UTC (permalink / raw)
To: Daniel Vetter; +Cc: Intel Graphics Development
2013/6/1 Daniel Vetter <daniel.vetter@ffwll.ch>:
> We can get at this easily through intel_crtc->config now.
>
> v2: Drop more stuff gcc spotted.
>
> v3: Drop even more stuff gcc spotted.
>
> v4: Yet more ...
>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
> drivers/gpu/drm/i915/intel_display.c | 24 ++++++++----------------
> 1 file changed, 8 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 103f4e9..432e699 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4580,7 +4580,6 @@ static void i9xx_update_pll(struct intel_crtc *crtc,
> }
>
> static void i8xx_update_pll(struct intel_crtc *crtc,
> - struct drm_display_mode *adjusted_mode,
> intel_clock_t *reduced_clock,
> int num_connectors)
> {
> @@ -4635,14 +4634,15 @@ static void i8xx_update_pll(struct intel_crtc *crtc,
> I915_WRITE(DPLL(pipe), dpll);
> }
>
> -static void intel_set_pipe_timings(struct intel_crtc *intel_crtc,
> - struct drm_display_mode *mode,
> - struct drm_display_mode *adjusted_mode)
> +static void intel_set_pipe_timings(struct intel_crtc *intel_crtc)
> {
> struct drm_device *dev = intel_crtc->base.dev;
> struct drm_i915_private *dev_priv = dev->dev_private;
> enum pipe pipe = intel_crtc->pipe;
> enum transcoder cpu_transcoder = intel_crtc->config.cpu_transcoder;
> + struct drm_display_mode *adjusted_mode =
> + &intel_crtc->config.adjusted_mode;
> + struct drm_display_mode *mode = &intel_crtc->config.requested_mode;
> uint32_t vsyncshift, crtc_vtotal, crtc_vblank_end;
>
> /* We need to be careful not to changed the adjusted mode, for otherwise
> @@ -4820,8 +4820,6 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
> struct drm_device *dev = crtc->dev;
> struct drm_i915_private *dev_priv = dev->dev_private;
> struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> - struct drm_display_mode *adjusted_mode =
> - &intel_crtc->config.adjusted_mode;
> struct drm_display_mode *mode = &intel_crtc->config.requested_mode;
> int pipe = intel_crtc->pipe;
> int plane = intel_crtc->plane;
> @@ -4886,7 +4884,7 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
> }
>
> if (IS_GEN2(dev))
> - i8xx_update_pll(intel_crtc, adjusted_mode,
> + i8xx_update_pll(intel_crtc,
> has_reduced_clock ? &reduced_clock : NULL,
> num_connectors);
> else if (IS_VALLEYVIEW(dev))
> @@ -4906,7 +4904,7 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
> dspcntr |= DISPPLANE_SEL_PIPE_B;
> }
>
> - intel_set_pipe_timings(intel_crtc, mode, adjusted_mode);
> + intel_set_pipe_timings(intel_crtc);
>
> /* pipesrc and dspsize control the size that is scaled from,
> * which should always be the user's requested size.
> @@ -5663,9 +5661,6 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
> struct drm_device *dev = crtc->dev;
> struct drm_i915_private *dev_priv = dev->dev_private;
> struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> - struct drm_display_mode *adjusted_mode =
> - &intel_crtc->config.adjusted_mode;
> - struct drm_display_mode *mode = &intel_crtc->config.requested_mode;
> int pipe = intel_crtc->pipe;
> int plane = intel_crtc->plane;
> int num_connectors = 0;
> @@ -5760,7 +5755,7 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
> }
> }
>
> - intel_set_pipe_timings(intel_crtc, mode, adjusted_mode);
> + intel_set_pipe_timings(intel_crtc);
>
> if (intel_crtc->config.has_pch_encoder) {
> intel_cpu_transcoder_set_m_n(intel_crtc,
> @@ -5868,9 +5863,6 @@ static int haswell_crtc_mode_set(struct drm_crtc *crtc,
> struct drm_device *dev = crtc->dev;
> struct drm_i915_private *dev_priv = dev->dev_private;
> struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> - struct drm_display_mode *adjusted_mode =
> - &intel_crtc->config.adjusted_mode;
> - struct drm_display_mode *mode = &intel_crtc->config.requested_mode;
> int pipe = intel_crtc->pipe;
> int plane = intel_crtc->plane;
> int num_connectors = 0;
> @@ -5903,7 +5895,7 @@ static int haswell_crtc_mode_set(struct drm_crtc *crtc,
>
> intel_crtc->lowfreq_avail = false;
>
> - intel_set_pipe_timings(intel_crtc, mode, adjusted_mode);
> + intel_set_pipe_timings(intel_crtc);
>
> if (intel_crtc->config.has_pch_encoder) {
> intel_cpu_transcoder_set_m_n(intel_crtc,
> --
> 1.7.11.7
>
--
Paulo Zanoni
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 7/7] drm/i915: check for strange pfit pipe assignemnt on ivb/hsw
2013-06-01 15:16 [PATCH 0/7] adjusted mode rework and assorted patches Daniel Vetter
` (5 preceding siblings ...)
2013-06-01 15:16 ` [PATCH 6/7] drm/i915: Drop some no longer required mode/adjusted_mode parameters Daniel Vetter
@ 2013-06-01 15:16 ` Daniel Vetter
2013-06-03 17:08 ` Paulo Zanoni
6 siblings, 1 reply; 28+ messages in thread
From: Daniel Vetter @ 2013-06-01 15:16 UTC (permalink / raw)
To: Intel Graphics Development; +Cc: Daniel Vetter, Mika Kuoppala
Panel fitters on ivb/hsw are not created equal since not all of them
support the new high-quality upscaling mode. To offset this the hw
allows us to freely assign the pfits to pipes.
Since our code currently doesn't support this we might fall over when
taking over firmware state. So check for this case and WARN about it.
We can then improve the code once we've hit this in the wild. Or once
we decide to support the improved upscale modes, though that requires
global arbitrage of modeset resources across crtcs.
Suggested-by: Mika Kuoppala <mika.kuoppala@intel.com>
Cc: Mika Kuoppala <mika.kuoppala@intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
drivers/gpu/drm/i915/intel_display.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 432e699..2b6e141 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5806,6 +5806,14 @@ static void ironlake_get_pfit_config(struct intel_crtc *crtc,
if (tmp & PF_ENABLE) {
pipe_config->pch_pfit.pos = I915_READ(PF_WIN_POS(crtc->pipe));
pipe_config->pch_pfit.size = I915_READ(PF_WIN_SZ(crtc->pipe));
+
+ /* We currently do not free assignements of panel fitters on
+ * ivb/hsw (since we don't use the higher upscaling modes which
+ * differentiates them) so just WARN about this case for now. */
+ if (IS_IVYBRIDGE(dev) || IS_HASWELL(dev)) {
+ WARN_ON((tmp & PF_PIPE_SEL_MASK_IVB) !=
+ PF_PIPE_SEL_IVB(crtc->pipe));
+ }
}
}
--
1.7.11.7
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [PATCH 7/7] drm/i915: check for strange pfit pipe assignemnt on ivb/hsw
2013-06-01 15:16 ` [PATCH 7/7] drm/i915: check for strange pfit pipe assignemnt on ivb/hsw Daniel Vetter
@ 2013-06-03 17:08 ` Paulo Zanoni
2013-06-04 12:08 ` Daniel Vetter
0 siblings, 1 reply; 28+ messages in thread
From: Paulo Zanoni @ 2013-06-03 17:08 UTC (permalink / raw)
To: Daniel Vetter; +Cc: Intel Graphics Development, Mika Kuoppala
2013/6/1 Daniel Vetter <daniel.vetter@ffwll.ch>:
> Panel fitters on ivb/hsw are not created equal since not all of them
> support the new high-quality upscaling mode. To offset this the hw
> allows us to freely assign the pfits to pipes.
>
> Since our code currently doesn't support this we might fall over when
> taking over firmware state. So check for this case and WARN about it.
> We can then improve the code once we've hit this in the wild. Or once
> we decide to support the improved upscale modes, though that requires
> global arbitrage of modeset resources across crtcs.
>
> Suggested-by: Mika Kuoppala <mika.kuoppala@intel.com>
> Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
> drivers/gpu/drm/i915/intel_display.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 432e699..2b6e141 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5806,6 +5806,14 @@ static void ironlake_get_pfit_config(struct intel_crtc *crtc,
> if (tmp & PF_ENABLE) {
> pipe_config->pch_pfit.pos = I915_READ(PF_WIN_POS(crtc->pipe));
> pipe_config->pch_pfit.size = I915_READ(PF_WIN_SZ(crtc->pipe));
> +
> + /* We currently do not free assignements of panel fitters on
> + * ivb/hsw (since we don't use the higher upscaling modes which
> + * differentiates them) so just WARN about this case for now. */
> + if (IS_IVYBRIDGE(dev) || IS_HASWELL(dev)) {
Or just check for IS_GEN7 or, to be future-proof, check for "gen >= 7"
since there's a higher chance that newer gens will be similar to gen 7
instead of the previous ones.
Anyway: Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> + WARN_ON((tmp & PF_PIPE_SEL_MASK_IVB) !=
> + PF_PIPE_SEL_IVB(crtc->pipe));
> + }
> }
> }
>
> --
> 1.7.11.7
>
--
Paulo Zanoni
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH 7/7] drm/i915: check for strange pfit pipe assignemnt on ivb/hsw
2013-06-03 17:08 ` Paulo Zanoni
@ 2013-06-04 12:08 ` Daniel Vetter
0 siblings, 0 replies; 28+ messages in thread
From: Daniel Vetter @ 2013-06-04 12:08 UTC (permalink / raw)
To: Paulo Zanoni; +Cc: Daniel Vetter, Intel Graphics Development, Mika Kuoppala
On Mon, Jun 03, 2013 at 02:08:33PM -0300, Paulo Zanoni wrote:
> 2013/6/1 Daniel Vetter <daniel.vetter@ffwll.ch>:
> > Panel fitters on ivb/hsw are not created equal since not all of them
> > support the new high-quality upscaling mode. To offset this the hw
> > allows us to freely assign the pfits to pipes.
> >
> > Since our code currently doesn't support this we might fall over when
> > taking over firmware state. So check for this case and WARN about it.
> > We can then improve the code once we've hit this in the wild. Or once
> > we decide to support the improved upscale modes, though that requires
> > global arbitrage of modeset resources across crtcs.
> >
> > Suggested-by: Mika Kuoppala <mika.kuoppala@intel.com>
> > Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > ---
> > drivers/gpu/drm/i915/intel_display.c | 8 ++++++++
> > 1 file changed, 8 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 432e699..2b6e141 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -5806,6 +5806,14 @@ static void ironlake_get_pfit_config(struct intel_crtc *crtc,
> > if (tmp & PF_ENABLE) {
> > pipe_config->pch_pfit.pos = I915_READ(PF_WIN_POS(crtc->pipe));
> > pipe_config->pch_pfit.size = I915_READ(PF_WIN_SZ(crtc->pipe));
> > +
> > + /* We currently do not free assignements of panel fitters on
> > + * ivb/hsw (since we don't use the higher upscaling modes which
> > + * differentiates them) so just WARN about this case for now. */
> > + if (IS_IVYBRIDGE(dev) || IS_HASWELL(dev)) {
>
> Or just check for IS_GEN7 or, to be future-proof, check for "gen >= 7"
> since there's a higher chance that newer gens will be similar to gen 7
> instead of the previous ones.
As discussed in private, I think the explicit list here is better. But
checking for IS_GEN7 is a good idea - I didn't realize that vlv is already
excluded here. So applied that little changed.
>
> Anyway: Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Merged all patches from this series, thanks a lot for your critical
review. It's not always fun, but it _does_ improve the patches by a lot
;-)
Cheers, Daniel
>
> > + WARN_ON((tmp & PF_PIPE_SEL_MASK_IVB) !=
> > + PF_PIPE_SEL_IVB(crtc->pipe));
> > + }
> > }
> > }
> >
> > --
> > 1.7.11.7
> >
>
>
>
> --
> Paulo Zanoni
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 28+ messages in thread