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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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

-- 
Ville Syrjälä
Intel OTC