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

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

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