Re: [PATCH 1/2] drm/i915: Modify DP set clock to accomodate more eDP timings.

Re: [PATCH 1/2] drm/i915: Modify DP set clock to accomodate more eDP timings.

[Daniel Vetter]

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

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

Re: [PATCH 1/2] drm/i915: Modify DP set clock to accomodate more eDP timings.

[Jani Nikula]

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

-- 
Jani Nikula, Intel Open Source Technology Center

Re: [PATCH 2/2] drm/i915: Move Valleyview DP DPLL divisor calc to intel_dp_set_clock

[Jani Nikula]

[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

-- 
Jani Nikula, Intel Open Source Technology Center

[PATCH 1/2] drm/i915: Modify DP set clock to accomodate more eDP timings.

[Chon Ming Lee]

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(-)

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)) {
-- 
1.7.7.6

[PATCH 2/2] drm/i915: Move Valleyview DP DPLL divisor calc to intel_dp_set_clock

[Chon Ming Lee]

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);
 
-	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;
 	}
 }
 
-- 
1.7.7.6

Re: [PATCH 1/2] drm/i915: Modify DP set clock to accomodate more eDP timings.

[Lee, Chon Ming]

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

Re: [PATCH 2/2] drm/i915: Move Valleyview DP DPLL divisor calc to intel_dp_set_clock

[Lee, Chon Ming]

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