Re: [PATCH 4/4] drm/i915: Add support for Generic MIPI panel driver

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

On Monday 19 May 2014 07:53 PM, Damien Lespiau wrote:
> On Mon, Apr 14, 2014 at 11:18:27AM +0530, Shobhit Kumar wrote:
>> +#define NS_MHZ_RATIO 1000000
>
> [...]
>
>> +static bool generic_init(struct intel_dsi_device *dsi)
>> +{
>
> [...]
>
>> +	/*
>> +	 * ui(s) = 1/f [f in hz]
>> +	 * ui(ns) = 10^9/f*10^6 [f in Mhz] -> 10^3/f(Mhz)
>
> ui(ns) = 10^9/(f*10^6)
>
>> +	 *
>> +	 * LP byte clock = TLPX/8ui
>
> Mind putting that comment just above the appropriate computation?
> Also, LP byte clock = Tlpx / (8UI)
>
>> +	 *
>> +	 * Since txddrclkhs_i is 2xUI, the count values programmed in
>> +	 * DPHY param registers are divided by 2
>
> That looks like a general comment that apply to a bunch of calculations
> below, probably worth separating it from the UI comment.
>

Will update as suggested.

>> +	 *
>> +	 */
>> +
>> +	/* in Kbps */
>> +	ui_num = bitrate;
>> +	ui_den = NS_MHZ_RATIO;
>
> I'm a bit confused here, most likely missing something, care to clarify?
>
> - IIUC, you want the computations to happen in ns. I'm a bit puzzled by
>    that NS_MHZ_RATIO constant name when we're dealing with Kbps.
>
>    That constant is 10^6 which seems to be OK for KHz. So maybe just a
>    naming problem?

Yeah, I now realize that it should be something like NS_KHZ_RATIO to 
avoid confusion

>
> - UI is a period, so is homogeneous to time (s), but ui_num being in
>    s^-1 and ui_den a constant, ui_num/ui_den looks like a frequency. Or
>    could it be that UI = ui_den / ui_num? would be confusing, but the
>    code below would make more sense. In which case could we have UI =
>    ui_num / ui_den?

I just kept ui_num and ui_den separately to take care of precision loss, 
but I see how it is adding to confusion. Actually it is ui_den / ui_num 
and we have all computations as 1/UI so it works. I think I will compute 
UI directly as UI = (NS_KHZ_RATIO * 1000) /bitrate and divide by 1000 
wherever we use to maintain precision. Sounds ok ?

>
>> +
>> +	tclk_prepare_clkzero = mipi_config->tclk_prepare_clkzero;
>> +	ths_prepare_hszero = mipi_config->ths_prepare_hszero;
>> +
>> +	/* B060 */
>> +	intel_dsi->lp_byte_clk = CEIL_DIV(tlpx_ns * ui_num, 8 * ui_den);
>> +
>> +	/* count values in UI = (ns value) * (bitrate / (2 * 10^6)) */
>> +	/* prepare count */
>> +	ths_prepare_ns =
>> +		(mipi_config->ths_prepare >  mipi_config->tclk_prepare) ?
>> +				mipi_config->ths_prepare :
>> +				mipi_config->tclk_prepare;
>
> That looks like max()
>
>> +
>> +	prepare_cnt = CEIL_DIV(ths_prepare_ns * ui_num,	ui_den * 2);
>
> The formula above has a 10^6, why is that OK not to have it there? (in
> which unit is bitrate in the formula? MHz?) Is this something like:
>
>    Count in UI = count(ns) / UI(ns)
>
> and then as UI = ui_den/ui_num (?!) we end up with:
>
>    Count in UI = count(ns) * ui_num / ui_den
>
> And then the / 2 comment applies.

Yeah actually its like this. I will correct as suggested above.

>
>> +
>> +	/* exit zero count */
>> +	exit_zero_cnt = CEIL_DIV(
>> +				(ths_prepare_hszero - ths_prepare_ns) * ui_num,
>> +				ui_den * 2
>> +				);
>> +
>> +	/*
>> +	 * Exit zero  is unified val ths_zero and ths_exit
>> +	 * minimum value for ths_exit = 110ns
>> +	 * min (exit_zero_cnt * 2) = 110/UI
>> +	 * exit_zero_cnt = 55/UI
>> +	 */
>> +	 if (exit_zero_cnt < (55 * ui_num / ui_den))
>> +		if ((55 * ui_num) % ui_den)
>> +			exit_zero_cnt += 1;
>
> I'm not sure what we're achieving with the +=1 here, mind explaining?

This is as per MIPI host controller spec to ceil the value

>
>> +
>> +	/* clk zero count */
>> +	clk_zero_cnt = CEIL_DIV(
>> +			(tclk_prepare_clkzero -	ths_prepare_ns)
>> +			* ui_num, 2 * ui_den);
>> +
>> +	/* trail count */
>> +	tclk_trail_ns = (mipi_config->tclk_trail > mipi_config->ths_trail) ?
>> +			mipi_config->tclk_trail : mipi_config->ths_trail;
>
> max()
>
>> +	trail_cnt = CEIL_DIV(tclk_trail_ns * ui_num, 2 * ui_den);
>> +
>> +	if (prepare_cnt > PREPARE_CNT_MAX ||
>> +		exit_zero_cnt > EXIT_ZERO_CNT_MAX ||
>> +		clk_zero_cnt > CLK_ZERO_CNT_MAX ||
>> +		trail_cnt > TRAIL_CNT_MAX)
>> +		DRM_DEBUG_DRIVER("Values crossing maximum limits\n");
>
> Is that a situation that may happen in a normal case? or should we go
> with a DRM_ERROR() here and potentially abort the modeset?
>

Generally it should not happen. We should not abort but clip to max 
values though there is no guarantee it will work, but there is high 
chance that it will work.

>> +
>> +	if (prepare_cnt > PREPARE_CNT_MAX)
>> +		prepare_cnt = PREPARE_CNT_MAX;
>> +
>> +	if (exit_zero_cnt > EXIT_ZERO_CNT_MAX)
>> +		exit_zero_cnt = EXIT_ZERO_CNT_MAX;
>> +
>> +	if (clk_zero_cnt > CLK_ZERO_CNT_MAX)
>> +		clk_zero_cnt = CLK_ZERO_CNT_MAX;
>> +
>> +	if (trail_cnt > TRAIL_CNT_MAX)
>> +		trail_cnt = TRAIL_CNT_MAX;
>> +
>> +	/* B080 */
>> +	intel_dsi->dphy_reg = exit_zero_cnt << 24 | trail_cnt << 16 |
>> +						clk_zero_cnt << 8 | prepare_cnt;
>> +
>> +	/*
>> +	 * LP to HS switch count = 4TLPX + PREP_COUNT * 2 + EXIT_ZERO_COUNT * 2
>> +	 *					+ 10UI + Extra Byte Count
>> +	 *
>> +	 * HS to LP switch count = THS-TRAIL + 2TLPX + Extra Byte Count
>> +	 * Extra Byte Count is calculated according to number of lanes.
>> +	 * High Low Switch Count is the Max of LP to HS and
>> +	 * HS to LP switch count
>> +	 *
>> +	 */
>> +	tlpx_ui = CEIL_DIV(tlpx_ns * ui_num, ui_den);
>> +
>> +	/* B044 */
>> +	intel_dsi->hs_to_lp_count =
>> +		CEIL_DIV(
>> +			4 * tlpx_ui + prepare_cnt * 2 +
>> +			exit_zero_cnt * 2 + 10,
>> +			8);
>
> The previous was before I tried to look at the spec too closely. Mind
> explaining why we don't look at the HS to LP switch count? ie why HS to
> LP switch cound is always smaller than the LP to HS one?

Because LP to HS uses exit_zero_count which is generally higher than 
clk_zero_count. So just directly used LP to HS which amounts to saying 
that switching from HS to LP takes lesser time than switching from LP to 
HS. I can/should add code to compute max of the two.

>
>> +
>> +	intel_dsi->hs_to_lp_count += extra_byte_count;
>> +
>> +	/* B088 */
>> +	/* LP -> HS for clock lanes
>> +	 * LP clk sync + LP11 + LP01 + tclk_prepare + tclk_zero +
>> +	 *						extra byte count
>> +	 * 2TPLX + 1TLPX + 1 TPLX(in ns) + prepare_cnt * 2 + clk_zero_cnt *
>> +	 *					2(in UI) + extra byte count
>> +	 * In byteclks = (4TLPX + prepare_cnt * 2 + clk_zero_cnt *2 (in UI)) /
>> +	 *					8 + extra byte count
>> +	 */
>> +	intel_dsi->clk_lp_to_hs_count =
>> +		CEIL_DIV(
>> +			4 * tlpx_ui + prepare_cnt * 2 +
>> +			clk_zero_cnt * 2,
>> +			8);
>> +
>> +	intel_dsi->clk_lp_to_hs_count += extra_byte_count;
>> +
>> +	/* HS->LP for Clock Lanes
>> +	 * Low Power clock synchronisations + 1Tx byteclk + tclk_trail +
>> +	 *						Extra byte count
>> +	 * 2TLPX + 8UI + (trail_count*2)(in UI) + Extra byte count
>> +	 * In byteclks = (2*TLpx(in UI) + trail_count*2 +8)(in UI)/8 +
>> +	 *						Extra byte count
>> +	 */
>> +	intel_dsi->clk_hs_to_lp_count =
>> +		CEIL_DIV(2 * tlpx_ui + trail_cnt * 2 + 8,
>> +			8);
>> +	intel_dsi->clk_hs_to_lp_count += extra_byte_count;
>> +
>> +	DRM_DEBUG_KMS("EOT %s\n", intel_dsi->eotp_pkt ? "ENABLED" : "DISABLED");
>> +	DRM_DEBUG_KMS("CLOCKSTOP %s\n", intel_dsi->clock_stop ?
>> +						"ENABLED" : "DISABLED");
>> +	DRM_DEBUG_KMS("Mode %s\n", intel_dsi->operation_mode ? "COMMAND" : "VIDEO");
>> +	DRM_DEBUG_KMS("Pixel Format %d\n", intel_dsi->pixel_format);
>> +	DRM_DEBUG_KMS("TLPX %d\n", intel_dsi->escape_clk_div);
>> +	DRM_DEBUG_KMS("LP RX Timeout 0x%x\n", intel_dsi->lp_rx_timeout);
>> +	DRM_DEBUG_KMS("Turnaround Timeout 0x%x\n", intel_dsi->turn_arnd_val);
>> +	DRM_DEBUG_KMS("Init Count 0x%x\n", intel_dsi->init_count);
>> +	DRM_DEBUG_KMS("HS to LP Count 0x%x\n", intel_dsi->hs_to_lp_count);
>> +	DRM_DEBUG_KMS("LP Byte Clock %d\n", intel_dsi->lp_byte_clk);
>> +	DRM_DEBUG_KMS("DBI BW Timer 0x%x\n", intel_dsi->bw_timer);
>> +	DRM_DEBUG_KMS("LP to HS Clock Count 0x%x\n", intel_dsi->clk_lp_to_hs_count);
>> +	DRM_DEBUG_KMS("HS to LP Clock Count 0x%x\n", intel_dsi->clk_hs_to_lp_count);
>> +	DRM_DEBUG_KMS("BTA %s\n",
>> +			intel_dsi->video_frmt_cfg_bits & DISABLE_VIDEO_BTA ?
>> +			"DISABLED" : "ENABLED");
>> +	DRM_DEBUG_KMS("B060 = 0x%Xx, B080 = 0x%x, B044 = 0x%x, B088 = 0x%x\n",
>> +			intel_dsi->lp_byte_clk, intel_dsi->dphy_reg, intel_dsi->hs_to_lp_count,
>> +			(intel_dsi->clk_lp_to_hs_count << LP_HS_SSW_CNT_SHIFT) |
>> +			(intel_dsi->clk_hs_to_lp_count << HS_LP_PWR_SW_CNT_SHIFT));
>> +
>> +	/* delays in VBT are in unit of 100us, so need to convert
>> +	 * here in ms
>> +	 * Delay (100us) * 100 /1000 = Delay / 10 (ms) */
>> +	intel_dsi->backlight_off_delay = pps->bl_disable_delay / 10;
>> +	intel_dsi->backlight_on_delay = pps->bl_enable_delay / 10;
>> +	intel_dsi->panel_on_delay = pps->panel_on_delay / 10;
>> +	intel_dsi->panel_off_delay = pps->panel_off_delay / 10;
>> +	intel_dsi->panel_pwr_cycle_delay = pps->panel_power_cycle_delay / 10;
>> +
>> +	return true;
>> +}