| Message ID | 1377885465-23851-1-git-send-email-chon.ming.lee@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sat, Aug 31, 2013 at 01:57:44AM +0800, Chon Ming Lee wrote: > eDP 1.4 supports 4-5 extra link rates in additional to current 2 link > rate. Create a structure to store the DPLL divisor data to improve > readability. > > Signed-off-by: Chon Ming Lee <chon.ming.lee@intel.com> This is a neat idea and I agree it'll be much better when we add edp link rates. Small comments below: > --- > drivers/gpu/drm/i915/intel_dp.c | 48 +++++++++++++++++++------------------- > 1 files changed, 24 insertions(+), 24 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index 2151d13..ab8a5ff 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -38,6 +38,19 @@ > > #define DP_LINK_CHECK_TIMEOUT (10 * 1000) > > +struct dp_link_dpll{ > + int link_bw; > + struct dpll dpll; > +}; > + > +static const struct dp_link_dpll gen4_dpll[] = > + {{ DP_LINK_BW_1_62, {2,23,8,2,10,0,0,0,0}}, > + { DP_LINK_BW_2_7, {1,14,2,1,10,0,0,0,0}}}; Usually we only indent by one tab here. Also please use c99 initializers and you don't need to explicitly initialize to 0. And a bit more space helps readbility further imo: static const struct dp_link_dpll gen4_dpll[] = { { DP_LINK_BW_1_62, { .p1 = 2, .p2 = 10, .n = 2, .m1 = 23, .m2 = 8 }}, /* more ... */ }; > + > +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}}}; > + > /** > * is_edp - is the given port attached to an eDP panel (either CPU or PCH) > * @intel_dp: DP struct > @@ -649,37 +662,24 @@ intel_dp_set_clock(struct intel_encoder *encoder, > struct intel_crtc_config *pipe_config, int link_bw) > { > struct drm_device *dev = encoder->base.dev; > + int i; > > if (IS_G4X(dev)) { > - if (link_bw == DP_LINK_BW_1_62) { > - pipe_config->dpll.p1 = 2; > - pipe_config->dpll.p2 = 10; > - pipe_config->dpll.n = 2; > - pipe_config->dpll.m1 = 23; > - pipe_config->dpll.m2 = 8; > - } else { > - pipe_config->dpll.p1 = 1; > - pipe_config->dpll.p2 = 10; > - pipe_config->dpll.n = 1; > - pipe_config->dpll.m1 = 14; > - pipe_config->dpll.m2 = 2; > + for(i = 0; i < sizeof(gen4_dpll) / sizeof(struct dp_link_dpll); i++) { > + if (link_bw == gen4_dpll[i].link_bw){ > + pipe_config->dpll = gen4_dpll[i].dpll; > + break; > + } > } > pipe_config->clock_set = true; > } else if (IS_HASWELL(dev)) { > /* Haswell has special-purpose DP DDI clocks. */ > } else if (HAS_PCH_SPLIT(dev)) { > - if (link_bw == DP_LINK_BW_1_62) { > - pipe_config->dpll.n = 1; > - pipe_config->dpll.p1 = 2; > - pipe_config->dpll.p2 = 10; > - pipe_config->dpll.m1 = 12; > - pipe_config->dpll.m2 = 9; > - } else { > - pipe_config->dpll.n = 2; > - pipe_config->dpll.p1 = 1; > - pipe_config->dpll.p2 = 10; > - pipe_config->dpll.m1 = 14; > - pipe_config->dpll.m2 = 8; > + for(i = 0; i < sizeof(pch_dpll) / sizeof(struct dp_link_dpll); i++) { > + if (link_bw == pch_dpll[i].link_bw){ > + pipe_config->dpll = pch_dpll[i].dpll; > + break; > + } I'd add temporary variables here to first select the right platform (and size of the array) and then only have one for loop to fish out the right value. Cheers, Daniel > } > pipe_config->clock_set = true; > } else if (IS_VALLEYVIEW(dev)) { > -- > 1.7.7.6 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Fri, 30 Aug 2013, Chon Ming Lee <chon.ming.lee@intel.com> wrote: > eDP 1.4 supports 4-5 extra link rates in additional to current 2 link > rate. Create a structure to store the DPLL divisor data to improve > readability. DP 1.2 already supports 3 link rates, right? > Signed-off-by: Chon Ming Lee <chon.ming.lee@intel.com> > --- > drivers/gpu/drm/i915/intel_dp.c | 48 +++++++++++++++++++------------------- > 1 files changed, 24 insertions(+), 24 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index 2151d13..ab8a5ff 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -38,6 +38,19 @@ > > #define DP_LINK_CHECK_TIMEOUT (10 * 1000) > > +struct dp_link_dpll{ Missing space before {. > + int link_bw; > + struct dpll dpll; > +}; > + > +static const struct dp_link_dpll gen4_dpll[] = > + {{ DP_LINK_BW_1_62, {2,23,8,2,10,0,0,0,0}}, > + { DP_LINK_BW_2_7, {1,14,2,1,10,0,0,0,0}}}; > + > +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}}}; > + Please switch these to use C99 designated initializers for clarity, something like this: static const struct dp_link_dpll gen4_dpll[] = { { .link_bw = DP_LINK_BW_1_62, .dpll = { .n = 2, .m1 = 23, m2 = 8, p1 = 2, p2 = 10, }, }, { .link_bw = DP_LINK_BW_2_7, .dpll = { .n = 1, .m1 = 14, m2 = 2, p1 = 1, p2 = 10, }, } }; > /** > * is_edp - is the given port attached to an eDP panel (either CPU or PCH) > * @intel_dp: DP struct > @@ -649,37 +662,24 @@ intel_dp_set_clock(struct intel_encoder *encoder, > struct intel_crtc_config *pipe_config, int link_bw) > { > struct drm_device *dev = encoder->base.dev; > + int i; > > if (IS_G4X(dev)) { > - if (link_bw == DP_LINK_BW_1_62) { > - pipe_config->dpll.p1 = 2; > - pipe_config->dpll.p2 = 10; > - pipe_config->dpll.n = 2; > - pipe_config->dpll.m1 = 23; > - pipe_config->dpll.m2 = 8; > - } else { > - pipe_config->dpll.p1 = 1; > - pipe_config->dpll.p2 = 10; > - pipe_config->dpll.n = 1; > - pipe_config->dpll.m1 = 14; > - pipe_config->dpll.m2 = 2; > + for(i = 0; i < sizeof(gen4_dpll) / sizeof(struct dp_link_dpll); i++) { Please use i < ARRAY_SIZE(gen4_dpll). > + if (link_bw == gen4_dpll[i].link_bw){ > + pipe_config->dpll = gen4_dpll[i].dpll; > + break; > + } > } The old if-else used some values even if link_bw was bogus. I'm not sure how likely that is. But if the link_bw is not found in the table for some obscure reason (e.g. the hw doesn't support the link rate), this fails. Perhaps we should have a test for i == ARRAY_SIZE(gen4_dpll) here and complain loudly if we hit that, and perhaps use a fallback value. > pipe_config->clock_set = true; > } else if (IS_HASWELL(dev)) { > /* Haswell has special-purpose DP DDI clocks. */ > } else if (HAS_PCH_SPLIT(dev)) { > - if (link_bw == DP_LINK_BW_1_62) { > - pipe_config->dpll.n = 1; > - pipe_config->dpll.p1 = 2; > - pipe_config->dpll.p2 = 10; > - pipe_config->dpll.m1 = 12; > - pipe_config->dpll.m2 = 9; > - } else { > - pipe_config->dpll.n = 2; > - pipe_config->dpll.p1 = 1; > - pipe_config->dpll.p2 = 10; > - pipe_config->dpll.m1 = 14; > - pipe_config->dpll.m2 = 8; > + for(i = 0; i < sizeof(pch_dpll) / sizeof(struct dp_link_dpll); i++) { > + if (link_bw == pch_dpll[i].link_bw){ > + pipe_config->dpll = pch_dpll[i].dpll; > + break; > + } > } Same here. BR, Jani. > pipe_config->clock_set = true; > } else if (IS_VALLEYVIEW(dev)) { > -- > 1.7.7.6 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On 08/30 10:28, Jani Nikula wrote: > On Fri, 30 Aug 2013, Chon Ming Lee <chon.ming.lee@intel.com> wrote: > > eDP 1.4 supports 4-5 extra link rates in additional to current 2 link > > rate. Create a structure to store the DPLL divisor data to improve > > readability. > > DP 1.2 already supports 3 link rates, right? Yes, 3 link rate for DP 1.2, but most of the older intel gfx doesn't support the highest 5.4Gbps link rate yet. > > > Signed-off-by: Chon Ming Lee <chon.ming.lee@intel.com> > > --- > > drivers/gpu/drm/i915/intel_dp.c | 48 +++++++++++++++++++------------------- > > 1 files changed, 24 insertions(+), 24 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > > index 2151d13..ab8a5ff 100644 > > --- a/drivers/gpu/drm/i915/intel_dp.c > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > @@ -38,6 +38,19 @@ > > > > #define DP_LINK_CHECK_TIMEOUT (10 * 1000) > > > > +struct dp_link_dpll{ > > Missing space before {. > > > + int link_bw; > > + struct dpll dpll; > > +}; > > + > > +static const struct dp_link_dpll gen4_dpll[] = > > + {{ DP_LINK_BW_1_62, {2,23,8,2,10,0,0,0,0}}, > > + { DP_LINK_BW_2_7, {1,14,2,1,10,0,0,0,0}}}; > > + > > +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}}}; > > + > > Please switch these to use C99 designated initializers for clarity, > something like this: > > static const struct dp_link_dpll gen4_dpll[] = { > { > .link_bw = DP_LINK_BW_1_62, > .dpll = { > .n = 2, > .m1 = 23, m2 = 8, > p1 = 2, p2 = 10, > }, > }, { > .link_bw = DP_LINK_BW_2_7, > .dpll = { > .n = 1, > .m1 = 14, m2 = 2, > p1 = 1, p2 = 10, > }, > } > }; > Thanks, will make the correction. > > /** > > * is_edp - is the given port attached to an eDP panel (either CPU or PCH) > > * @intel_dp: DP struct > > @@ -649,37 +662,24 @@ intel_dp_set_clock(struct intel_encoder *encoder, > > struct intel_crtc_config *pipe_config, int link_bw) > > { > > struct drm_device *dev = encoder->base.dev; > > + int i; > > > > if (IS_G4X(dev)) { > > - if (link_bw == DP_LINK_BW_1_62) { > > - pipe_config->dpll.p1 = 2; > > - pipe_config->dpll.p2 = 10; > > - pipe_config->dpll.n = 2; > > - pipe_config->dpll.m1 = 23; > > - pipe_config->dpll.m2 = 8; > > - } else { > > - pipe_config->dpll.p1 = 1; > > - pipe_config->dpll.p2 = 10; > > - pipe_config->dpll.n = 1; > > - pipe_config->dpll.m1 = 14; > > - pipe_config->dpll.m2 = 2; > > + for(i = 0; i < sizeof(gen4_dpll) / sizeof(struct dp_link_dpll); i++) { > > Please use i < ARRAY_SIZE(gen4_dpll). Already make this change. After sent out the patch, realize I should this ARRAY_SIZE. Thanks to point this out. > > > + if (link_bw == gen4_dpll[i].link_bw){ > > + pipe_config->dpll = gen4_dpll[i].dpll; > > + break; > > + } > > } > > The old if-else used some values even if link_bw was bogus. I'm not sure > how likely that is. But if the link_bw is not found in the table for > some obscure reason (e.g. the hw doesn't support the link rate), this > fails. Perhaps we should have a test for i == ARRAY_SIZE(gen4_dpll) here > and complain loudly if we hit that, and perhaps use a fallback value. > In intel_dp_compute_config, it will only assign either one of two link rates into link_bw before calling this function. The link_bw won't be out of range. I would think the checking should be done prior to calling this function. For example, in eDP 1.4, instead of supporting more link rates, there is possible to use single lane, but choose the largest link rate, or use 4 lanes, with lower link rate. If fail out here, might be too late to recover. > > pipe_config->clock_set = true; > > } else if (IS_HASWELL(dev)) { > > /* Haswell has special-purpose DP DDI clocks. */ > > } else if (HAS_PCH_SPLIT(dev)) { > > - if (link_bw == DP_LINK_BW_1_62) { > > - pipe_config->dpll.n = 1; > > - pipe_config->dpll.p1 = 2; > > - pipe_config->dpll.p2 = 10; > > - pipe_config->dpll.m1 = 12; > > - pipe_config->dpll.m2 = 9; > > - } else { > > - pipe_config->dpll.n = 2; > > - pipe_config->dpll.p1 = 1; > > - pipe_config->dpll.p2 = 10; > > - pipe_config->dpll.m1 = 14; > > - pipe_config->dpll.m2 = 8; > > + for(i = 0; i < sizeof(pch_dpll) / sizeof(struct dp_link_dpll); i++) { > > + if (link_bw == pch_dpll[i].link_bw){ > > + pipe_config->dpll = pch_dpll[i].dpll; > > + break; > > + } > > } > > Same here. > > BR, > Jani. > > > > pipe_config->clock_set = true; > > } else if (IS_VALLEYVIEW(dev)) { > > -- > > 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_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 2151d13..ab8a5ff 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -38,6 +38,19 @@ #define DP_LINK_CHECK_TIMEOUT (10 * 1000) +struct dp_link_dpll{ + int link_bw; + struct dpll dpll; +}; + +static const struct dp_link_dpll gen4_dpll[] = + {{ DP_LINK_BW_1_62, {2,23,8,2,10,0,0,0,0}}, + { DP_LINK_BW_2_7, {1,14,2,1,10,0,0,0,0}}}; + +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}}}; + /** * is_edp - is the given port attached to an eDP panel (either CPU or PCH) * @intel_dp: DP struct @@ -649,37 +662,24 @@ intel_dp_set_clock(struct intel_encoder *encoder, struct intel_crtc_config *pipe_config, int link_bw) { struct drm_device *dev = encoder->base.dev; + int i; if (IS_G4X(dev)) { - if (link_bw == DP_LINK_BW_1_62) { - pipe_config->dpll.p1 = 2; - pipe_config->dpll.p2 = 10; - pipe_config->dpll.n = 2; - pipe_config->dpll.m1 = 23; - pipe_config->dpll.m2 = 8; - } else { - pipe_config->dpll.p1 = 1; - pipe_config->dpll.p2 = 10; - pipe_config->dpll.n = 1; - pipe_config->dpll.m1 = 14; - pipe_config->dpll.m2 = 2; + for(i = 0; i < sizeof(gen4_dpll) / sizeof(struct dp_link_dpll); i++) { + if (link_bw == gen4_dpll[i].link_bw){ + pipe_config->dpll = gen4_dpll[i].dpll; + break; + } } pipe_config->clock_set = true; } else if (IS_HASWELL(dev)) { /* Haswell has special-purpose DP DDI clocks. */ } else if (HAS_PCH_SPLIT(dev)) { - if (link_bw == DP_LINK_BW_1_62) { - pipe_config->dpll.n = 1; - pipe_config->dpll.p1 = 2; - pipe_config->dpll.p2 = 10; - pipe_config->dpll.m1 = 12; - pipe_config->dpll.m2 = 9; - } else { - pipe_config->dpll.n = 2; - pipe_config->dpll.p1 = 1; - pipe_config->dpll.p2 = 10; - pipe_config->dpll.m1 = 14; - pipe_config->dpll.m2 = 8; + for(i = 0; i < sizeof(pch_dpll) / sizeof(struct dp_link_dpll); i++) { + if (link_bw == pch_dpll[i].link_bw){ + pipe_config->dpll = pch_dpll[i].dpll; + break; + } } pipe_config->clock_set = true; } else if (IS_VALLEYVIEW(dev)) {
eDP 1.4 supports 4-5 extra link rates in additional to current 2 link rate. Create a structure to store the DPLL divisor data to improve readability. Signed-off-by: Chon Ming Lee <chon.ming.lee@intel.com> --- drivers/gpu/drm/i915/intel_dp.c | 48 +++++++++++++++++++------------------- 1 files changed, 24 insertions(+), 24 deletions(-)