| Message ID | 1377885465-23851-2-git-send-email-chon.ming.lee@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
[Okay, I missed Daniel's review, and noticed I hadn't actually hit send on this one either... but here goes anyway...] On Fri, 30 Aug 2013, Chon Ming Lee <chon.ming.lee@intel.com> wrote: > For DP pll settings, there is only two golden configs. Instead of running > through the algorithm to determine it, hardcode the value and get it > determine in intel_dp_set_clock. > > Signed-off-by: Chon Ming Lee <chon.ming.lee@intel.com> > --- > drivers/gpu/drm/i915/intel_display.c | 22 ++++------------------ > drivers/gpu/drm/i915/intel_dp.c | 12 +++++++++++- > 2 files changed, 15 insertions(+), 19 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index f526ea9..453fa16 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -339,19 +339,6 @@ static const intel_limit_t intel_limits_vlv_hdmi = { > .p2_slow = 2, .p2_fast = 20 }, > }; > > -static const intel_limit_t intel_limits_vlv_dp = { > - .dot = { .min = 25000, .max = 270000 }, > - .vco = { .min = 4000000, .max = 6000000 }, > - .n = { .min = 1, .max = 7 }, > - .m = { .min = 22, .max = 450 }, > - .m1 = { .min = 2, .max = 3 }, > - .m2 = { .min = 11, .max = 156 }, > - .p = { .min = 10, .max = 30 }, > - .p1 = { .min = 1, .max = 3 }, > - .p2 = { .dot_limit = 270000, > - .p2_slow = 2, .p2_fast = 20 }, > -}; > - > static const intel_limit_t *intel_ironlake_limit(struct drm_crtc *crtc, > int refclk) > { > @@ -414,10 +401,8 @@ static const intel_limit_t *intel_limit(struct drm_crtc *crtc, int refclk) > } else if (IS_VALLEYVIEW(dev)) { > if (intel_pipe_has_type(crtc, INTEL_OUTPUT_ANALOG)) > limit = &intel_limits_vlv_dac; > - else if (intel_pipe_has_type(crtc, INTEL_OUTPUT_HDMI)) > + else > limit = &intel_limits_vlv_hdmi; > - else > - limit = &intel_limits_vlv_dp; > } else if (!IS_GEN2(dev)) { > if (intel_pipe_has_type(crtc, INTEL_OUTPUT_LVDS)) > limit = &intel_limits_i9xx_lvds; > @@ -4889,15 +4874,16 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc, > } > > refclk = i9xx_get_refclk(crtc, num_connectors); > + > + limit = intel_limit(crtc, refclk); Did you move this here just to avoid the warning about uninitialized limit? It's a bit ugly... but then again the the whole is_dsi vs. not is rather ugly already. *shrug*. > > - if (!is_dsi) { > + if (!is_dsi && !intel_crtc->config.clock_set) { > /* > * Returns a set of divisors for the desired target clock with > * the given refclk, or FALSE. The returned values represent > * the clock equation: reflck * (5 * (m1 + 2) + (m2 + 2)) / (n + > * 2) / p1 / p2. > */ > - limit = intel_limit(crtc, refclk); > ok = dev_priv->display.find_dpll(limit, crtc, > intel_crtc->config.port_clock, > refclk, NULL, &clock); > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index ab8a5ff..89a2606 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -51,6 +51,10 @@ static const struct dp_link_dpll pch_dpll[] = > {{ DP_LINK_BW_1_62, {1,12,9,2,10,0,0,0,0}}, > { DP_LINK_BW_2_7, {2,14,8,1,10,0,0,0,0}}}; > > +static const struct dp_link_dpll vlv_dpll[] = > + {{ DP_LINK_BW_1_62, {5,3,81,3,2,0,0,0,0}}, > + { DP_LINK_BW_2_7, {1,2,27,2,2,0,0,0,0}}}; > + > /** > * is_edp - is the given port attached to an eDP panel (either CPU or PCH) > * @intel_dp: DP struct > @@ -683,7 +687,13 @@ intel_dp_set_clock(struct intel_encoder *encoder, > } > pipe_config->clock_set = true; > } else if (IS_VALLEYVIEW(dev)) { > - /* FIXME: Need to figure out optimized DP clocks for vlv. */ > + for(i = 0; i < sizeof(vlv_dpll) / sizeof(struct dp_link_dpll); i++) { > + if (link_bw == vlv_dpll[i].link_bw){ > + pipe_config->dpll = vlv_dpll[i].dpll; > + break; > + } > + } > + pipe_config->clock_set = true; You now have three similar loops in the function. A follow-up patch could pick the table to use in the if branches, and have a single loop at the end. You could handle the array size by having .link_bw = 0 in the last entry as a stop condition, and using that as the fallback entry too (see my review of patch 1 about unknown link_bw values). BR, Jani. > } > } > > -- > 1.7.7.6 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On 08/30 11:00, Jani Nikula wrote: > > [Okay, I missed Daniel's review, and noticed I hadn't actually hit send > on this one either... but here goes anyway...] > > On Fri, 30 Aug 2013, Chon Ming Lee <chon.ming.lee@intel.com> wrote: > > For DP pll settings, there is only two golden configs. Instead of running > > through the algorithm to determine it, hardcode the value and get it > > determine in intel_dp_set_clock. > > > > Signed-off-by: Chon Ming Lee <chon.ming.lee@intel.com> > > --- > > drivers/gpu/drm/i915/intel_display.c | 22 ++++------------------ > > drivers/gpu/drm/i915/intel_dp.c | 12 +++++++++++- > > 2 files changed, 15 insertions(+), 19 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > index f526ea9..453fa16 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -339,19 +339,6 @@ static const intel_limit_t intel_limits_vlv_hdmi = { > > .p2_slow = 2, .p2_fast = 20 }, > > }; > > > > -static const intel_limit_t intel_limits_vlv_dp = { > > - .dot = { .min = 25000, .max = 270000 }, > > - .vco = { .min = 4000000, .max = 6000000 }, > > - .n = { .min = 1, .max = 7 }, > > - .m = { .min = 22, .max = 450 }, > > - .m1 = { .min = 2, .max = 3 }, > > - .m2 = { .min = 11, .max = 156 }, > > - .p = { .min = 10, .max = 30 }, > > - .p1 = { .min = 1, .max = 3 }, > > - .p2 = { .dot_limit = 270000, > > - .p2_slow = 2, .p2_fast = 20 }, > > -}; > > - > > static const intel_limit_t *intel_ironlake_limit(struct drm_crtc *crtc, > > int refclk) > > { > > @@ -414,10 +401,8 @@ static const intel_limit_t *intel_limit(struct drm_crtc *crtc, int refclk) > > } else if (IS_VALLEYVIEW(dev)) { > > if (intel_pipe_has_type(crtc, INTEL_OUTPUT_ANALOG)) > > limit = &intel_limits_vlv_dac; > > - else if (intel_pipe_has_type(crtc, INTEL_OUTPUT_HDMI)) > > + else > > limit = &intel_limits_vlv_hdmi; > > - else > > - limit = &intel_limits_vlv_dp; > > } else if (!IS_GEN2(dev)) { > > if (intel_pipe_has_type(crtc, INTEL_OUTPUT_LVDS)) > > limit = &intel_limits_i9xx_lvds; > > @@ -4889,15 +4874,16 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc, > > } > > > > refclk = i9xx_get_refclk(crtc, num_connectors); > > + > > + limit = intel_limit(crtc, refclk); > > Did you move this here just to avoid the warning about uninitialized > limit? It's a bit ugly... but then again the the whole is_dsi vs. not is > rather ugly already. *shrug*. > Yes, correct. I can reverse it but, just have to add another limit = intel_limit(crtc, refclk); in this if statement. if (!is_dsi && is_lvds && dev_priv->lvds_downclock_avail) { Will it more make sense? > > > > - if (!is_dsi) { > > + if (!is_dsi && !intel_crtc->config.clock_set) { > > /* > > * Returns a set of divisors for the desired target clock with > > * the given refclk, or FALSE. The returned values represent > > * the clock equation: reflck * (5 * (m1 + 2) + (m2 + 2)) / (n + > > * 2) / p1 / p2. > > */ > > - limit = intel_limit(crtc, refclk); > > ok = dev_priv->display.find_dpll(limit, crtc, > > intel_crtc->config.port_clock, > > refclk, NULL, &clock); > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > > index ab8a5ff..89a2606 100644 > > --- a/drivers/gpu/drm/i915/intel_dp.c > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > @@ -51,6 +51,10 @@ static const struct dp_link_dpll pch_dpll[] = > > {{ DP_LINK_BW_1_62, {1,12,9,2,10,0,0,0,0}}, > > { DP_LINK_BW_2_7, {2,14,8,1,10,0,0,0,0}}}; > > > > +static const struct dp_link_dpll vlv_dpll[] = > > + {{ DP_LINK_BW_1_62, {5,3,81,3,2,0,0,0,0}}, > > + { DP_LINK_BW_2_7, {1,2,27,2,2,0,0,0,0}}}; > > + > > /** > > * is_edp - is the given port attached to an eDP panel (either CPU or PCH) > > * @intel_dp: DP struct > > @@ -683,7 +687,13 @@ intel_dp_set_clock(struct intel_encoder *encoder, > > } > > pipe_config->clock_set = true; > > } else if (IS_VALLEYVIEW(dev)) { > > - /* FIXME: Need to figure out optimized DP clocks for vlv. */ > > + for(i = 0; i < sizeof(vlv_dpll) / sizeof(struct dp_link_dpll); i++) { > > + if (link_bw == vlv_dpll[i].link_bw){ > > + pipe_config->dpll = vlv_dpll[i].dpll; > > + break; > > + } > > + } > > + pipe_config->clock_set = true; > > You now have three similar loops in the function. A follow-up patch > could pick the table to use in the if branches, and have a single loop > at the end. You could handle the array size by having .link_bw = 0 in > the last entry as a stop condition, and using that as the fallback entry > too (see my review of patch 1 about unknown link_bw values). > Will make this change. > BR, > Jani. > > > > } > > } > > > > -- > > 1.7.7.6 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Jani Nikula, Intel Open Source Technology Center
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index f526ea9..453fa16 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -339,19 +339,6 @@ static const intel_limit_t intel_limits_vlv_hdmi = { .p2_slow = 2, .p2_fast = 20 }, }; -static const intel_limit_t intel_limits_vlv_dp = { - .dot = { .min = 25000, .max = 270000 }, - .vco = { .min = 4000000, .max = 6000000 }, - .n = { .min = 1, .max = 7 }, - .m = { .min = 22, .max = 450 }, - .m1 = { .min = 2, .max = 3 }, - .m2 = { .min = 11, .max = 156 }, - .p = { .min = 10, .max = 30 }, - .p1 = { .min = 1, .max = 3 }, - .p2 = { .dot_limit = 270000, - .p2_slow = 2, .p2_fast = 20 }, -}; - static const intel_limit_t *intel_ironlake_limit(struct drm_crtc *crtc, int refclk) { @@ -414,10 +401,8 @@ static const intel_limit_t *intel_limit(struct drm_crtc *crtc, int refclk) } else if (IS_VALLEYVIEW(dev)) { if (intel_pipe_has_type(crtc, INTEL_OUTPUT_ANALOG)) limit = &intel_limits_vlv_dac; - else if (intel_pipe_has_type(crtc, INTEL_OUTPUT_HDMI)) + else limit = &intel_limits_vlv_hdmi; - else - limit = &intel_limits_vlv_dp; } else if (!IS_GEN2(dev)) { if (intel_pipe_has_type(crtc, INTEL_OUTPUT_LVDS)) limit = &intel_limits_i9xx_lvds; @@ -4889,15 +4874,16 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc, } refclk = i9xx_get_refclk(crtc, num_connectors); + + limit = intel_limit(crtc, refclk); - if (!is_dsi) { + if (!is_dsi && !intel_crtc->config.clock_set) { /* * Returns a set of divisors for the desired target clock with * the given refclk, or FALSE. The returned values represent * the clock equation: reflck * (5 * (m1 + 2) + (m2 + 2)) / (n + * 2) / p1 / p2. */ - limit = intel_limit(crtc, refclk); ok = dev_priv->display.find_dpll(limit, crtc, intel_crtc->config.port_clock, refclk, NULL, &clock); diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index ab8a5ff..89a2606 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -51,6 +51,10 @@ static const struct dp_link_dpll pch_dpll[] = {{ DP_LINK_BW_1_62, {1,12,9,2,10,0,0,0,0}}, { DP_LINK_BW_2_7, {2,14,8,1,10,0,0,0,0}}}; +static const struct dp_link_dpll vlv_dpll[] = + {{ DP_LINK_BW_1_62, {5,3,81,3,2,0,0,0,0}}, + { DP_LINK_BW_2_7, {1,2,27,2,2,0,0,0,0}}}; + /** * is_edp - is the given port attached to an eDP panel (either CPU or PCH) * @intel_dp: DP struct @@ -683,7 +687,13 @@ intel_dp_set_clock(struct intel_encoder *encoder, } pipe_config->clock_set = true; } else if (IS_VALLEYVIEW(dev)) { - /* FIXME: Need to figure out optimized DP clocks for vlv. */ + for(i = 0; i < sizeof(vlv_dpll) / sizeof(struct dp_link_dpll); i++) { + if (link_bw == vlv_dpll[i].link_bw){ + pipe_config->dpll = vlv_dpll[i].dpll; + break; + } + } + pipe_config->clock_set = true; } }
For DP pll settings, there is only two golden configs. Instead of running through the algorithm to determine it, hardcode the value and get it determine in intel_dp_set_clock. Signed-off-by: Chon Ming Lee <chon.ming.lee@intel.com> --- drivers/gpu/drm/i915/intel_display.c | 22 ++++------------------ drivers/gpu/drm/i915/intel_dp.c | 12 +++++++++++- 2 files changed, 15 insertions(+), 19 deletions(-)