> -----Original Message----- > From: Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx> > Sent: Thursday, March 13, 2025 5:55 PM > To: Tseng, William <william.tseng@xxxxxxxxx>; intel- > gfx@xxxxxxxxxxxxxxxxxxxxx; intel-xe@xxxxxxxxxxxxxxxxxxxxx > Cc: Tseng, William <william.tseng@xxxxxxxxx>; Ville Syrjala > <ville.syrjala@xxxxxxxxxxxxxxx>; Vandita Kulkarni > <vandita.kulkarni@xxxxxxxxx>; Lee, Shawn C <shawn.c.lee@xxxxxxxxx>; > Cooper Chiou <cooper.chiou@xxxxxxxxx> > Subject: Re: [PATCH v4] drm/i915/dsi: let HW maintain the HS-TRAIL timing > > On Tue, 11 Mar 2025, William Tseng <william.tseng@xxxxxxxxx> 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@xxxxxxxxxxxxxxx> > > Cc: Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx> > > Cc: Vandita Kulkarni <vandita.kulkarni@xxxxxxxxx> > > Cc: Lee Shawn C <shawn.c.lee@xxxxxxxxx> > > Cc: Cooper Chiou <cooper.chiou@xxxxxxxxx> > > Signed-off-by: William Tseng <william.tseng@xxxxxxxxx> > > Acked-by: Vandita Kulkarni <vandita.kulkarni@xxxxxxxxx> > > --- > > 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
