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

[Shobhit Kumar <shobhit.kumar@intel.com>]

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
>