[PATCH v4] drm/i915/dsi: let HW maintain the HS-TRAIL timing

[Tseng, William]

> -----Original Message-----
> From: Jani Nikula <jani.nikula at linux.intel.com>
> Sent: Thursday, March 13, 2025 5:55 PM
> To: Tseng, William <william.tseng at intel.com>; intel-
> gfx at lists.freedesktop.org; intel-xe at lists.freedesktop.org
> Cc: Tseng, William <william.tseng at intel.com>; Ville Syrjala
> <ville.syrjala at linux.intel.com>; Vandita Kulkarni
> <vandita.kulkarni at intel.com>; Lee, Shawn C <shawn.c.lee at intel.com>;
> Cooper Chiou <cooper.chiou at intel.com>
> Subject: Re: [PATCH v4] drm/i915/dsi: let HW maintain the HS-TRAIL timing
> 
> On Tue, 11 Mar 2025, William Tseng <william.tseng at intel.com> wrote:
> > This change is to avoid over-specification of the TEOT timing
> > parameter, which is derived from software in current design.
> >
> > Supposed that THS-TRAIL and THS-EXIT have the minimum values, i.e., 60
> > and 100 in ns. If SW is overriding the HW default, the TEOT value
> > becomes 150 ns, approximately calculated by the following formula.
> >
> >   DIV_ROUND_UP(60/50)*50 + DIV_ROUND_UP(100/50))*50/2, where 50
> >   is LP Escape Clock time in ns.
> >
> > The TEOT value 150 ns is larger than the maximum value, around 136 ns
> > if UI is 1.8ns, (105 ns + 12*UI, defined by MIPI DPHY specification).
> >
> > However, the TEOT value will meet the specification if THS-TRAIL is
> > set to the HW default, instead of software overriding.
> >
> > The timing change is made for both data lane and clock lane.
> 
> What does the VBT contain?
> 
In VBT, the value of THSTRAIL is set to 60 and TCLKTRAIL is set to 70 for the
device with DSI panel.

> Granted, the spec only describes the relevant fields as "TClkTrail:
> Clock Trail value" and "THSTrail: HS Trail value".
> 
> But we might have panels and setups where we depend on VBT providing the
> values. So why do we pick and choose some VBT values for overrides and not
> others?
> 
Maybe timing conformance is the consideration which may be subject to the change of 
data rate.  We need to keep it adjustable.

> I also don't get why we do this:
> 
> 	tclk_trail_ns = max(mipi_config->tclk_trail, mipi_config->ths_trail);
> 
The reason might be linked to the minimum values for both TClkTrail and THSTrail,
defined in Table 18, MIPI D-PHY version 2.5.  We need to get a value above the required
minimum value for both timing parameters, via the formula.


Regards
William


> and then use that for both. Again, would be interesting to see what VBT has
> for them.
> 
> 
> BR,
> Jani.
> 
> 
> >
> > v1: initial version.
> > v2: change clock lane dphy timings.
> > v3: remove calculation of trail cnt.
> > v4: rebase.
> >
> > Closes: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/13891
> > Cc: Ville Syrjala <ville.syrjala at linux.intel.com>
> > Cc: Jani Nikula <jani.nikula at linux.intel.com>
> > Cc: Vandita Kulkarni <vandita.kulkarni at intel.com>
> > Cc: Lee Shawn C <shawn.c.lee at intel.com>
> > Cc: Cooper Chiou <cooper.chiou at intel.com>
> > Signed-off-by: William Tseng <william.tseng at intel.com>
> > Acked-by: Vandita Kulkarni <vandita.kulkarni at intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/icl_dsi.c | 19 +++----------------
> >  1 file changed, 3 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/icl_dsi.c
> > b/drivers/gpu/drm/i915/display/icl_dsi.c
> > index 82bf6c654de2..6cf9c9275031 100644
> > --- a/drivers/gpu/drm/i915/display/icl_dsi.c
> > +++ b/drivers/gpu/drm/i915/display/icl_dsi.c
> > @@ -1846,14 +1846,13 @@ static void icl_dphy_param_init(struct intel_dsi
> *intel_dsi)
> >  	struct intel_connector *connector = intel_dsi->attached_connector;
> >  	struct mipi_config *mipi_config = connector->panel.vbt.dsi.config;
> >  	u32 tlpx_ns;
> > -	u32 prepare_cnt, exit_zero_cnt, clk_zero_cnt, trail_cnt;
> > -	u32 ths_prepare_ns, tclk_trail_ns;
> > +	u32 prepare_cnt, exit_zero_cnt, clk_zero_cnt;
> > +	u32 ths_prepare_ns;
> >  	u32 hs_zero_cnt;
> >  	u32 tclk_pre_cnt;
> >
> >  	tlpx_ns = intel_dsi_tlpx_ns(intel_dsi);
> >
> > -	tclk_trail_ns = max(mipi_config->tclk_trail, mipi_config->ths_trail);
> >  	ths_prepare_ns = max(mipi_config->ths_prepare,
> >  			     mipi_config->tclk_prepare);
> >
> > @@ -1880,14 +1879,6 @@ static void icl_dphy_param_init(struct intel_dsi
> *intel_dsi)
> >  		clk_zero_cnt = ICL_CLK_ZERO_CNT_MAX;
> >  	}
> >
> > -	/* trail cnt in escape clocks*/
> > -	trail_cnt = DIV_ROUND_UP(tclk_trail_ns, tlpx_ns);
> > -	if (trail_cnt > ICL_TRAIL_CNT_MAX) {
> > -		drm_dbg_kms(display->drm, "trail_cnt out of range (%d)\n",
> > -			    trail_cnt);
> > -		trail_cnt = ICL_TRAIL_CNT_MAX;
> > -	}
> > -
> >  	/* tclk pre count in escape clocks */
> >  	tclk_pre_cnt = DIV_ROUND_UP(mipi_config->tclk_pre, tlpx_ns);
> >  	if (tclk_pre_cnt > ICL_TCLK_PRE_CNT_MAX) { @@ -1920,17 +1911,13
> @@
> > static void icl_dphy_param_init(struct intel_dsi *intel_dsi)
> >  			       CLK_ZERO_OVERRIDE |
> >  			       CLK_ZERO(clk_zero_cnt) |
> >  			       CLK_PRE_OVERRIDE |
> > -			       CLK_PRE(tclk_pre_cnt) |
> > -			       CLK_TRAIL_OVERRIDE |
> > -			       CLK_TRAIL(trail_cnt));
> > +			       CLK_PRE(tclk_pre_cnt));
> >
> >  	/* data lanes dphy timings */
> >  	intel_dsi->dphy_data_lane_reg = (HS_PREPARE_OVERRIDE |
> >  					 HS_PREPARE(prepare_cnt) |
> >  					 HS_ZERO_OVERRIDE |
> >  					 HS_ZERO(hs_zero_cnt) |
> > -					 HS_TRAIL_OVERRIDE |
> > -					 HS_TRAIL(trail_cnt) |
> >  					 HS_EXIT_OVERRIDE |
> >  					 HS_EXIT(exit_zero_cnt));
> 
> --
> Jani Nikula, Intel