diff mbox

[3/4] drm/i915: Compute dsi_clk from pixel clock

Message ID 1382358067-5578-4-git-send-email-shobhit.kumar@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Kumar, Shobhit Oct. 21, 2013, 12:21 p.m. UTC 
Minor modification to m_n_p calculations as well

Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
---
 drivers/gpu/drm/i915/intel_dsi_pll.c |   75 ++++++++++++++++++++++++++++------
 1 file changed, 63 insertions(+), 12 deletions(-)

Comments

Ville Syrjala Oct. 21, 2013, 1:28 p.m. UTC | #1 
On Mon, Oct 21, 2013 at 05:51:06PM +0530, Shobhit Kumar wrote:
> Minor modification to m_n_p calculations as well
> 
> Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dsi_pll.c |   75 ++++++++++++++++++++++++++++------
>  1 file changed, 63 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dsi_pll.c b/drivers/gpu/drm/i915/intel_dsi_pll.c
> index 44279b2..bf12335 100644
> --- a/drivers/gpu/drm/i915/intel_dsi_pll.c
> +++ b/drivers/gpu/drm/i915/intel_dsi_pll.c
> @@ -50,6 +50,8 @@ static const u32 lfsr_converts[] = {
>  	71, 35							/* 91 - 92 */
>  };
>  
> +#ifdef DSI_CLK_FROM_RR
> +
>  static u32 dsi_rr_formula(const struct drm_display_mode *mode,
>  			  int pixel_format, int video_mode_format,
>  			  int lane_count, bool eotp)
> @@ -129,6 +131,40 @@ static u32 dsi_rr_formula(const struct drm_display_mode *mode,
>  	return dsi_clk;
>  }
>  
> +#else
> +
> +/* Get DSI clock from pixel clock */
> +static u32 dsi_clk_from_pclk(const struct drm_display_mode *mode,
> +			  int pixel_format, int lane_count)
> +{
> +	u32 dsi_bit_clock_hz, dsi_clk;
> +	u32 bpp;
> +
> +	switch (pixel_format) {
> +	default:
> +	case VID_MODE_FORMAT_RGB888:
> +	case VID_MODE_FORMAT_RGB666_LOOSE:
> +		bpp = 24;
> +		break;
> +	case VID_MODE_FORMAT_RGB666:
> +		bpp = 18;
> +		break;
> +	case VID_MODE_FORMAT_RGB565:
> +		bpp = 16;
> +		break;
> +	}
> +
> +	/* DSI data rate = pixel clock * bits per pixel / lane count
> +	   pixel clock is converted from KHz to Hz */
> +	dsi_bit_clock_hz = (((mode->clock * 1000) * bpp) / lane_count);
> +
> +	/* DSI clock rate */
> +	dsi_clk = dsi_bit_clock_hz / (1000 * 1000);
> +	return dsi_clk;

And why is this better than the rr_formula thing that tries to
account for the packetization overhead?

Also I don't understand why you go from kHz to Hz and then to MHz.
I'd just do something like:
return DIV_ROUND_CLOSEST(mode->clock * bpp, lane_count);
and then change the rest of the code to work in kHz as well.

> +}
> +
> +#endif
> +
>  #ifdef MNP_FROM_TABLE
>  
>  struct dsi_clock_table {
> @@ -208,29 +244,42 @@ static int dsi_calc_mnp(u32 dsi_clk, struct dsi_mnp *dsi_mnp)
>  	ref_clk = 25000;
>  	target_dsi_clk = dsi_clk * 1000;
>  	error = 0xFFFFFFFF;
> +	tmp_error = 0xFFFFFFFF;
>  	calc_m = 0;
>  	calc_p = 0;
>  
>  	for (m = 62; m <= 92; m++) {
>  		for (p = 2; p <= 6; p++) {
> -
> +			/* Find the optimal m and p divisors
> +			with minimal error +/- the required clock */
>  			calc_dsi_clk = (m * ref_clk) / p;
> -			if (calc_dsi_clk >= target_dsi_clk) {
> +			if (calc_dsi_clk == target_dsi_clk) {
> +				calc_m = m;
> +				calc_p = p;
> +				error = 0;
> +				break;
> +			} else if (calc_dsi_clk > target_dsi_clk)
>  				tmp_error = calc_dsi_clk - target_dsi_clk;
> -				if (tmp_error < error) {
> -					error = tmp_error;
> -					calc_m = m;
> -					calc_p = p;
> -				}
> +			else
> +				tmp_error = target_dsi_clk - calc_dsi_clk;
> +
> +			if (tmp_error < error) {
> +				error = tmp_error;
> +				calc_m = m;
> +				calc_p = p;
>  			}
>  		}
> +
> +		if (error == 0)
> +			break;
>  	}
>  
>  	m_seed = lfsr_converts[calc_m - 62];
>  	n = 1;
> +
>  	dsi_mnp->dsi_pll_ctrl = 1 << (DSI_PLL_P1_POST_DIV_SHIFT + calc_p - 2);
>  	dsi_mnp->dsi_pll_div = (n - 1) << DSI_PLL_N1_DIV_SHIFT |
> -		m_seed << DSI_PLL_M1_DIV_SHIFT;
> +					m_seed << DSI_PLL_M1_DIV_SHIFT;
>  
>  	return 0;
>  }
> @@ -249,11 +298,13 @@ static void vlv_configure_dsi_pll(struct intel_encoder *encoder)
>  	struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base);
>  	int ret;
>  	struct dsi_mnp dsi_mnp;
> -	u32 dsi_clk;
> +	u32 dsi_clk = 0;
>  
> -	dsi_clk = dsi_rr_formula(mode, intel_dsi->pixel_format,
> -				 intel_dsi->video_mode_format,
> -				 intel_dsi->lane_count, !intel_dsi->eot_disable);
> +	if (intel_dsi->dsi_clock_freq)
> +		dsi_clk = intel_dsi->dsi_clock_freq;
> +	else
> +		dsi_clk = dsi_clk_from_pclk(mode, intel_dsi->pixel_format,
> +							intel_dsi->lane_count);
>  
>  	ret = dsi_calc_mnp(dsi_clk, &dsi_mnp);
>  	if (ret) {
> -- 
> 1.7.9.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Jani Nikula Oct. 21, 2013, 1:44 p.m. UTC | #2 
On Mon, 21 Oct 2013, Shobhit Kumar <shobhit.kumar@intel.com> wrote:
> Minor modification to m_n_p calculations as well

That should probably be a separate patch, unless it's a requirement for
what the main subject of this patch is. The commit message does not say.

> Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dsi_pll.c |   75 ++++++++++++++++++++++++++++------
>  1 file changed, 63 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dsi_pll.c b/drivers/gpu/drm/i915/intel_dsi_pll.c
> index 44279b2..bf12335 100644
> --- a/drivers/gpu/drm/i915/intel_dsi_pll.c
> +++ b/drivers/gpu/drm/i915/intel_dsi_pll.c
> @@ -50,6 +50,8 @@ static const u32 lfsr_converts[] = {
>  	71, 35							/* 91 - 92 */
>  };
>  
> +#ifdef DSI_CLK_FROM_RR
> +
>  static u32 dsi_rr_formula(const struct drm_display_mode *mode,
>  			  int pixel_format, int video_mode_format,
>  			  int lane_count, bool eotp)
> @@ -129,6 +131,40 @@ static u32 dsi_rr_formula(const struct drm_display_mode *mode,
>  	return dsi_clk;
>  }
>  
> +#else
> +
> +/* Get DSI clock from pixel clock */
> +static u32 dsi_clk_from_pclk(const struct drm_display_mode *mode,
> +			  int pixel_format, int lane_count)
> +{
> +	u32 dsi_bit_clock_hz, dsi_clk;
> +	u32 bpp;
> +
> +	switch (pixel_format) {
> +	default:
> +	case VID_MODE_FORMAT_RGB888:
> +	case VID_MODE_FORMAT_RGB666_LOOSE:
> +		bpp = 24;
> +		break;
> +	case VID_MODE_FORMAT_RGB666:
> +		bpp = 18;
> +		break;
> +	case VID_MODE_FORMAT_RGB565:
> +		bpp = 16;
> +		break;
> +	}
> +
> +	/* DSI data rate = pixel clock * bits per pixel / lane count
> +	   pixel clock is converted from KHz to Hz */
> +	dsi_bit_clock_hz = (((mode->clock * 1000) * bpp) / lane_count);
> +
> +	/* DSI clock rate */
> +	dsi_clk = dsi_bit_clock_hz / (1000 * 1000);
> +	return dsi_clk;
> +}
> +
> +#endif
> +
>  #ifdef MNP_FROM_TABLE
>  
>  struct dsi_clock_table {
> @@ -208,29 +244,42 @@ static int dsi_calc_mnp(u32 dsi_clk, struct dsi_mnp *dsi_mnp)
>  	ref_clk = 25000;
>  	target_dsi_clk = dsi_clk * 1000;
>  	error = 0xFFFFFFFF;
> +	tmp_error = 0xFFFFFFFF;
>  	calc_m = 0;
>  	calc_p = 0;
>  
>  	for (m = 62; m <= 92; m++) {
>  		for (p = 2; p <= 6; p++) {
> -
> +			/* Find the optimal m and p divisors
> +			with minimal error +/- the required clock */
>  			calc_dsi_clk = (m * ref_clk) / p;
> -			if (calc_dsi_clk >= target_dsi_clk) {
> +			if (calc_dsi_clk == target_dsi_clk) {
> +				calc_m = m;
> +				calc_p = p;
> +				error = 0;
> +				break;
> +			} else if (calc_dsi_clk > target_dsi_clk)
>  				tmp_error = calc_dsi_clk - target_dsi_clk;
> -				if (tmp_error < error) {
> -					error = tmp_error;
> -					calc_m = m;
> -					calc_p = p;
> -				}
> +			else
> +				tmp_error = target_dsi_clk - calc_dsi_clk;
> +

Using abs() might clean this up a bit.

> +			if (tmp_error < error) {
> +				error = tmp_error;
> +				calc_m = m;
> +				calc_p = p;
>  			}
>  		}
> +
> +		if (error == 0)
> +			break;

The above changes should be a separate patch, with rationale in the
commit message.

>  	}
>  
>  	m_seed = lfsr_converts[calc_m - 62];
>  	n = 1;
> +
>  	dsi_mnp->dsi_pll_ctrl = 1 << (DSI_PLL_P1_POST_DIV_SHIFT + calc_p - 2);
>  	dsi_mnp->dsi_pll_div = (n - 1) << DSI_PLL_N1_DIV_SHIFT |
> -		m_seed << DSI_PLL_M1_DIV_SHIFT;
> +					m_seed << DSI_PLL_M1_DIV_SHIFT;

If really needed, style/whitespace changes should also be separated.

>  
>  	return 0;
>  }
> @@ -249,11 +298,13 @@ static void vlv_configure_dsi_pll(struct intel_encoder *encoder)
>  	struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base);
>  	int ret;
>  	struct dsi_mnp dsi_mnp;
> -	u32 dsi_clk;
> +	u32 dsi_clk = 0;
>  
> -	dsi_clk = dsi_rr_formula(mode, intel_dsi->pixel_format,
> -				 intel_dsi->video_mode_format,
> -				 intel_dsi->lane_count, !intel_dsi->eot_disable);
> +	if (intel_dsi->dsi_clock_freq)
> +		dsi_clk = intel_dsi->dsi_clock_freq;

This is the third major change in the patch. Should be separate, with
rationale, or possibly bundled with a different change which starts
using it. Who is responsible for setting and clearing this?

> +	else
> +		dsi_clk = dsi_clk_from_pclk(mode, intel_dsi->pixel_format,
> +							intel_dsi->lane_count);
>  
>  	ret = dsi_calc_mnp(dsi_clk, &dsi_mnp);
>  	if (ret) {
> -- 
> 1.7.9.5
>
Kumar, Shobhit Oct. 22, 2013, 9:15 a.m. UTC | #3 
On 10/21/2013 6:58 PM, Ville Syrjälä wrote:
> On Mon, Oct 21, 2013 at 05:51:06PM +0530, Shobhit Kumar wrote:
>> Minor modification to m_n_p calculations as well
>>
>> Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_dsi_pll.c |   75 ++++++++++++++++++++++++++++------
>>   1 file changed, 63 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dsi_pll.c b/drivers/gpu/drm/i915/intel_dsi_pll.c
>> index 44279b2..bf12335 100644
>> --- a/drivers/gpu/drm/i915/intel_dsi_pll.c
>> +++ b/drivers/gpu/drm/i915/intel_dsi_pll.c
>> @@ -50,6 +50,8 @@ static const u32 lfsr_converts[] = {
>>   	71, 35							/* 91 - 92 */
>>   };
>>
>> +#ifdef DSI_CLK_FROM_RR
>> +
>>   static u32 dsi_rr_formula(const struct drm_display_mode *mode,
>>   			  int pixel_format, int video_mode_format,
>>   			  int lane_count, bool eotp)
>> @@ -129,6 +131,40 @@ static u32 dsi_rr_formula(const struct drm_display_mode *mode,
>>   	return dsi_clk;
>>   }
>>
>> +#else
>> +
>> +/* Get DSI clock from pixel clock */
>> +static u32 dsi_clk_from_pclk(const struct drm_display_mode *mode,
>> +			  int pixel_format, int lane_count)
>> +{
>> +	u32 dsi_bit_clock_hz, dsi_clk;
>> +	u32 bpp;
>> +
>> +	switch (pixel_format) {
>> +	default:
>> +	case VID_MODE_FORMAT_RGB888:
>> +	case VID_MODE_FORMAT_RGB666_LOOSE:
>> +		bpp = 24;
>> +		break;
>> +	case VID_MODE_FORMAT_RGB666:
>> +		bpp = 18;
>> +		break;
>> +	case VID_MODE_FORMAT_RGB565:
>> +		bpp = 16;
>> +		break;
>> +	}
>> +
>> +	/* DSI data rate = pixel clock * bits per pixel / lane count
>> +	   pixel clock is converted from KHz to Hz */
>> +	dsi_bit_clock_hz = (((mode->clock * 1000) * bpp) / lane_count);
>> +
>> +	/* DSI clock rate */
>> +	dsi_clk = dsi_bit_clock_hz / (1000 * 1000);
>> +	return dsi_clk;
>
> And why is this better than the rr_formula thing that tries to
> account for the packetization overhead?
>

Its been changed to pixel_clock based formula as MIPI host controller 
specification in our platform recommends this formula

> Also I don't understand why you go from kHz to Hz and then to MHz.
> I'd just do something like:
> return DIV_ROUND_CLOSEST(mode->clock * bpp, lane_count);
> and then change the rest of the code to work in kHz as well.

We wanted more accuracy, but on after thought anyway EDID spec specifies 
pixel_clock in 10 KHz and we already lost the precision, so it does not 
make sense to convert in to Hz and then revert to MHz. Will make the 
change accordingly.

>
>> +}
>> +
>> +#endif
>> +
>>   #ifdef MNP_FROM_TABLE
>>
>>   struct dsi_clock_table {
>> @@ -208,29 +244,42 @@ static int dsi_calc_mnp(u32 dsi_clk, struct dsi_mnp *dsi_mnp)
>>   	ref_clk = 25000;
>>   	target_dsi_clk = dsi_clk * 1000;
>>   	error = 0xFFFFFFFF;
>> +	tmp_error = 0xFFFFFFFF;
>>   	calc_m = 0;
>>   	calc_p = 0;
>>
>>   	for (m = 62; m <= 92; m++) {
>>   		for (p = 2; p <= 6; p++) {
>> -
>> +			/* Find the optimal m and p divisors
>> +			with minimal error +/- the required clock */
>>   			calc_dsi_clk = (m * ref_clk) / p;
>> -			if (calc_dsi_clk >= target_dsi_clk) {
>> +			if (calc_dsi_clk == target_dsi_clk) {
>> +				calc_m = m;
>> +				calc_p = p;
>> +				error = 0;
>> +				break;
>> +			} else if (calc_dsi_clk > target_dsi_clk)
>>   				tmp_error = calc_dsi_clk - target_dsi_clk;
>> -				if (tmp_error < error) {
>> -					error = tmp_error;
>> -					calc_m = m;
>> -					calc_p = p;
>> -				}
>> +			else
>> +				tmp_error = target_dsi_clk - calc_dsi_clk;
>> +
>> +			if (tmp_error < error) {
>> +				error = tmp_error;
>> +				calc_m = m;
>> +				calc_p = p;
>>   			}
>>   		}
>> +
>> +		if (error == 0)
>> +			break;
>>   	}
>>
>>   	m_seed = lfsr_converts[calc_m - 62];
>>   	n = 1;
>> +
>>   	dsi_mnp->dsi_pll_ctrl = 1 << (DSI_PLL_P1_POST_DIV_SHIFT + calc_p - 2);
>>   	dsi_mnp->dsi_pll_div = (n - 1) << DSI_PLL_N1_DIV_SHIFT |
>> -		m_seed << DSI_PLL_M1_DIV_SHIFT;
>> +					m_seed << DSI_PLL_M1_DIV_SHIFT;
>>
>>   	return 0;
>>   }
>> @@ -249,11 +298,13 @@ static void vlv_configure_dsi_pll(struct intel_encoder *encoder)
>>   	struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base);
>>   	int ret;
>>   	struct dsi_mnp dsi_mnp;
>> -	u32 dsi_clk;
>> +	u32 dsi_clk = 0;
>>
>> -	dsi_clk = dsi_rr_formula(mode, intel_dsi->pixel_format,
>> -				 intel_dsi->video_mode_format,
>> -				 intel_dsi->lane_count, !intel_dsi->eot_disable);
>> +	if (intel_dsi->dsi_clock_freq)
>> +		dsi_clk = intel_dsi->dsi_clock_freq;
>> +	else
>> +		dsi_clk = dsi_clk_from_pclk(mode, intel_dsi->pixel_format,
>> +							intel_dsi->lane_count);
>>
>>   	ret = dsi_calc_mnp(dsi_clk, &dsi_mnp);
>>   	if (ret) {
>> --
>> 1.7.9.5
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
Kumar, Shobhit Oct. 22, 2013, 9:25 a.m. UTC | #4 
On 10/21/2013 7:14 PM, Jani Nikula wrote:
> On Mon, 21 Oct 2013, Shobhit Kumar <shobhit.kumar@intel.com> wrote:
>> Minor modification to m_n_p calculations as well
>
> That should probably be a separate patch, unless it's a requirement for
> what the main subject of this patch is. The commit message does not say.

Will do the needful

>
>> Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_dsi_pll.c |   75 ++++++++++++++++++++++++++++------
>>   1 file changed, 63 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dsi_pll.c b/drivers/gpu/drm/i915/intel_dsi_pll.c
>> index 44279b2..bf12335 100644
>> --- a/drivers/gpu/drm/i915/intel_dsi_pll.c
>> +++ b/drivers/gpu/drm/i915/intel_dsi_pll.c
>> @@ -50,6 +50,8 @@ static const u32 lfsr_converts[] = {
>>   	71, 35							/* 91 - 92 */
>>   };
>>
>> +#ifdef DSI_CLK_FROM_RR
>> +
>>   static u32 dsi_rr_formula(const struct drm_display_mode *mode,
>>   			  int pixel_format, int video_mode_format,
>>   			  int lane_count, bool eotp)
>> @@ -129,6 +131,40 @@ static u32 dsi_rr_formula(const struct drm_display_mode *mode,
>>   	return dsi_clk;
>>   }
>>
>> +#else
>> +
>> +/* Get DSI clock from pixel clock */
>> +static u32 dsi_clk_from_pclk(const struct drm_display_mode *mode,
>> +			  int pixel_format, int lane_count)
>> +{
>> +	u32 dsi_bit_clock_hz, dsi_clk;
>> +	u32 bpp;
>> +
>> +	switch (pixel_format) {
>> +	default:
>> +	case VID_MODE_FORMAT_RGB888:
>> +	case VID_MODE_FORMAT_RGB666_LOOSE:
>> +		bpp = 24;
>> +		break;
>> +	case VID_MODE_FORMAT_RGB666:
>> +		bpp = 18;
>> +		break;
>> +	case VID_MODE_FORMAT_RGB565:
>> +		bpp = 16;
>> +		break;
>> +	}
>> +
>> +	/* DSI data rate = pixel clock * bits per pixel / lane count
>> +	   pixel clock is converted from KHz to Hz */
>> +	dsi_bit_clock_hz = (((mode->clock * 1000) * bpp) / lane_count);
>> +
>> +	/* DSI clock rate */
>> +	dsi_clk = dsi_bit_clock_hz / (1000 * 1000);
>> +	return dsi_clk;
>> +}
>> +
>> +#endif
>> +
>>   #ifdef MNP_FROM_TABLE
>>
>>   struct dsi_clock_table {
>> @@ -208,29 +244,42 @@ static int dsi_calc_mnp(u32 dsi_clk, struct dsi_mnp *dsi_mnp)
>>   	ref_clk = 25000;
>>   	target_dsi_clk = dsi_clk * 1000;
>>   	error = 0xFFFFFFFF;
>> +	tmp_error = 0xFFFFFFFF;
>>   	calc_m = 0;
>>   	calc_p = 0;
>>
>>   	for (m = 62; m <= 92; m++) {
>>   		for (p = 2; p <= 6; p++) {
>> -
>> +			/* Find the optimal m and p divisors
>> +			with minimal error +/- the required clock */
>>   			calc_dsi_clk = (m * ref_clk) / p;
>> -			if (calc_dsi_clk >= target_dsi_clk) {
>> +			if (calc_dsi_clk == target_dsi_clk) {
>> +				calc_m = m;
>> +				calc_p = p;
>> +				error = 0;
>> +				break;
>> +			} else if (calc_dsi_clk > target_dsi_clk)
>>   				tmp_error = calc_dsi_clk - target_dsi_clk;
>> -				if (tmp_error < error) {
>> -					error = tmp_error;
>> -					calc_m = m;
>> -					calc_p = p;
>> -				}
>> +			else
>> +				tmp_error = target_dsi_clk - calc_dsi_clk;
>> +
>
> Using abs() might clean this up a bit.
>
>> +			if (tmp_error < error) {
>> +				error = tmp_error;
>> +				calc_m = m;
>> +				calc_p = p;
>>   			}
>>   		}
>> +
>> +		if (error == 0)
>> +			break;
>
> The above changes should be a separate patch, with rationale in the
> commit message.

Will do the needful

>
>>   	}
>>
>>   	m_seed = lfsr_converts[calc_m - 62];
>>   	n = 1;
>> +
>>   	dsi_mnp->dsi_pll_ctrl = 1 << (DSI_PLL_P1_POST_DIV_SHIFT + calc_p - 2);
>>   	dsi_mnp->dsi_pll_div = (n - 1) << DSI_PLL_N1_DIV_SHIFT |
>> -		m_seed << DSI_PLL_M1_DIV_SHIFT;
>> +					m_seed << DSI_PLL_M1_DIV_SHIFT;
>
> If really needed, style/whitespace changes should also be separated.
>
>>
>>   	return 0;
>>   }
>> @@ -249,11 +298,13 @@ static void vlv_configure_dsi_pll(struct intel_encoder *encoder)
>>   	struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base);
>>   	int ret;
>>   	struct dsi_mnp dsi_mnp;
>> -	u32 dsi_clk;
>> +	u32 dsi_clk = 0;
>>
>> -	dsi_clk = dsi_rr_formula(mode, intel_dsi->pixel_format,
>> -				 intel_dsi->video_mode_format,
>> -				 intel_dsi->lane_count, !intel_dsi->eot_disable);
>> +	if (intel_dsi->dsi_clock_freq)
>> +		dsi_clk = intel_dsi->dsi_clock_freq;
>
> This is the third major change in the patch. Should be separate, with
> rationale, or possibly bundled with a different change which starts
> using it. Who is responsible for setting and clearing this?
>

Panel vendor recommended frequency value should override the formula 
based value and this parameter provides support for the same. We have 
come across panel where the vendor specified specific data rates.

>> +	else
>> +		dsi_clk = dsi_clk_from_pclk(mode, intel_dsi->pixel_format,
>> +							intel_dsi->lane_count);
>>
>>   	ret = dsi_calc_mnp(dsi_clk, &dsi_mnp);
>>   	if (ret) {
>> --
>> 1.7.9.5
>>
>

Regards
Shobhit
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/intel_dsi_pll.c b/drivers/gpu/drm/i915/intel_dsi_pll.c
index 44279b2..bf12335 100644
--- a/drivers/gpu/drm/i915/intel_dsi_pll.c
+++ b/drivers/gpu/drm/i915/intel_dsi_pll.c
@@ -50,6 +50,8 @@  static const u32 lfsr_converts[] = {
 	71, 35							/* 91 - 92 */
 };
 
+#ifdef DSI_CLK_FROM_RR
+
 static u32 dsi_rr_formula(const struct drm_display_mode *mode,
 			  int pixel_format, int video_mode_format,
 			  int lane_count, bool eotp)
@@ -129,6 +131,40 @@  static u32 dsi_rr_formula(const struct drm_display_mode *mode,
 	return dsi_clk;
 }
 
+#else
+
+/* Get DSI clock from pixel clock */
+static u32 dsi_clk_from_pclk(const struct drm_display_mode *mode,
+			  int pixel_format, int lane_count)
+{
+	u32 dsi_bit_clock_hz, dsi_clk;
+	u32 bpp;
+
+	switch (pixel_format) {
+	default:
+	case VID_MODE_FORMAT_RGB888:
+	case VID_MODE_FORMAT_RGB666_LOOSE:
+		bpp = 24;
+		break;
+	case VID_MODE_FORMAT_RGB666:
+		bpp = 18;
+		break;
+	case VID_MODE_FORMAT_RGB565:
+		bpp = 16;
+		break;
+	}
+
+	/* DSI data rate = pixel clock * bits per pixel / lane count
+	   pixel clock is converted from KHz to Hz */
+	dsi_bit_clock_hz = (((mode->clock * 1000) * bpp) / lane_count);
+
+	/* DSI clock rate */
+	dsi_clk = dsi_bit_clock_hz / (1000 * 1000);
+	return dsi_clk;
+}
+
+#endif
+
 #ifdef MNP_FROM_TABLE
 
 struct dsi_clock_table {
@@ -208,29 +244,42 @@  static int dsi_calc_mnp(u32 dsi_clk, struct dsi_mnp *dsi_mnp)
 	ref_clk = 25000;
 	target_dsi_clk = dsi_clk * 1000;
 	error = 0xFFFFFFFF;
+	tmp_error = 0xFFFFFFFF;
 	calc_m = 0;
 	calc_p = 0;
 
 	for (m = 62; m <= 92; m++) {
 		for (p = 2; p <= 6; p++) {
-
+			/* Find the optimal m and p divisors
+			with minimal error +/- the required clock */
 			calc_dsi_clk = (m * ref_clk) / p;
-			if (calc_dsi_clk >= target_dsi_clk) {
+			if (calc_dsi_clk == target_dsi_clk) {
+				calc_m = m;
+				calc_p = p;
+				error = 0;
+				break;
+			} else if (calc_dsi_clk > target_dsi_clk)
 				tmp_error = calc_dsi_clk - target_dsi_clk;
-				if (tmp_error < error) {
-					error = tmp_error;
-					calc_m = m;
-					calc_p = p;
-				}
+			else
+				tmp_error = target_dsi_clk - calc_dsi_clk;
+
+			if (tmp_error < error) {
+				error = tmp_error;
+				calc_m = m;
+				calc_p = p;
 			}
 		}
+
+		if (error == 0)
+			break;
 	}
 
 	m_seed = lfsr_converts[calc_m - 62];
 	n = 1;
+
 	dsi_mnp->dsi_pll_ctrl = 1 << (DSI_PLL_P1_POST_DIV_SHIFT + calc_p - 2);
 	dsi_mnp->dsi_pll_div = (n - 1) << DSI_PLL_N1_DIV_SHIFT |
-		m_seed << DSI_PLL_M1_DIV_SHIFT;
+					m_seed << DSI_PLL_M1_DIV_SHIFT;
 
 	return 0;
 }
@@ -249,11 +298,13 @@  static void vlv_configure_dsi_pll(struct intel_encoder *encoder)
 	struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base);
 	int ret;
 	struct dsi_mnp dsi_mnp;
-	u32 dsi_clk;
+	u32 dsi_clk = 0;
 
-	dsi_clk = dsi_rr_formula(mode, intel_dsi->pixel_format,
-				 intel_dsi->video_mode_format,
-				 intel_dsi->lane_count, !intel_dsi->eot_disable);
+	if (intel_dsi->dsi_clock_freq)
+		dsi_clk = intel_dsi->dsi_clock_freq;
+	else
+		dsi_clk = dsi_clk_from_pclk(mode, intel_dsi->pixel_format,
+							intel_dsi->lane_count);
 
 	ret = dsi_calc_mnp(dsi_clk, &dsi_mnp);
 	if (ret) {