On Fri, 03 Jan 2025, "Kandpal, Suraj" <suraj.kandpal@xxxxxxxxx> wrote: >> -----Original Message----- >> From: Intel-xe <intel-xe-bounces@xxxxxxxxxxxxxxxxxxxxx> On Behalf Of Arun R >> Murthy >> Sent: Friday, January 3, 2025 10:43 AM >> To: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; intel-xe@xxxxxxxxxxxxxxxxxxxxx >> Cc: Murthy, Arun R <arun.r.murthy@xxxxxxxxx> >> Subject: [PATCH v5] drm/i915/dp: Guarantee a minimum HBlank time >> >> Mandate a minimum Hblank symbol cycle count between BS and BE in 8b/10b >> MST and 128b/132b mode. >> Spec: DP2.1a > > The spec is mentioned usually right before the Signed-off-by > Spec: > Signed-off-by They're called commit message trailers [1]. We just don't reference DP specs like this, there really isn't much point because it's all free form. Please put it in the commit message prose as proper sentences. And what good does a "DP2.1a" spec reference bring us? It's 1500+ pages. Please cite a section. (Bspec references we do use, and use "Bspec:" for them.) [1] https://git-scm.com/docs/git-interpret-trailers > >> >> v2: Affine calculation/updation of min HBlank to dp_mst (Jani) >> v3: moved min_hblank from struct intel_dp to intel_crtc_state (Jani) >> v4: use max/min functions, change intel_xx *intel_xx to intel_xx *xx >> (Jani) >> Limit hblank to 511 and accommodate BS/BE in calculated value >> (Srikanth) >> v5: Some spelling corrections (Suraj) >> >> Signed-off-by: Arun R Murthy <arun.r.murthy@xxxxxxxxx> >> --- >> .../gpu/drm/i915/display/intel_crtc_state_dump.c | 1 + >> drivers/gpu/drm/i915/display/intel_display_types.h | 1 + >> drivers/gpu/drm/i915/display/intel_dp_mst.c | 35 >> ++++++++++++++++++++++ >> drivers/gpu/drm/i915/i915_reg.h | 4 +++ >> 4 files changed, 41 insertions(+) >> >> diff --git a/drivers/gpu/drm/i915/display/intel_crtc_state_dump.c >> b/drivers/gpu/drm/i915/display/intel_crtc_state_dump.c >> index >> 1faef60be4728cd80a0a6b0151797ceda5c443ce..0e7e0b7803d9865177d6f >> 68e8afdef94a91d9697 100644 >> --- a/drivers/gpu/drm/i915/display/intel_crtc_state_dump.c >> +++ b/drivers/gpu/drm/i915/display/intel_crtc_state_dump.c >> @@ -248,6 +248,7 @@ void intel_crtc_state_dump(const struct >> intel_crtc_state *pipe_config, >> str_enabled_disabled(pipe_config- >> >has_sel_update), >> str_enabled_disabled(pipe_config- >> >has_panel_replay), >> str_enabled_disabled(pipe_config- >> >enable_psr2_sel_fetch)); >> + drm_printf(&p, "minimum HBlank: %d\n", pipe_config- >> >min_hblank); >> } >> >> drm_printf(&p, "framestart delay: %d, MSA timing delay: %d\n", diff - >> -git a/drivers/gpu/drm/i915/display/intel_display_types.h >> b/drivers/gpu/drm/i915/display/intel_display_types.h >> index >> eb9dd1125a4a09511936b81219e7f38fae106dfd..7d3a7700c44ef200d811 >> d9e90ea465c06104287e 100644 >> --- a/drivers/gpu/drm/i915/display/intel_display_types.h >> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h >> @@ -1095,6 +1095,7 @@ struct intel_crtc_state { >> >> int max_link_bpp_x16; /* in 1/16 bpp units */ >> int pipe_bpp; /* in 1 bpp units */ >> + int min_hblank; /* min HBlank for DP2.1 */ > > I think this comment should state min HBlank for MST and not DP2.1 since we use it > For non uhbr cases too > With the above fixed > Reviewed-by: Suraj Kandpal <suraj.kandpal@xxxxxxxxx> > >> struct intel_link_m_n dp_m_n; >> >> /* m2_n2 for eDP downclock */ >> diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c >> b/drivers/gpu/drm/i915/display/intel_dp_mst.c >> index >> fffd199999e02eb66ea478ff872f72b277bd3970..bd561c08d945fcafa65af92 >> 54a71cd66f17923d2 100644 >> --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c >> +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c >> @@ -211,6 +211,35 @@ static int intel_dp_mst_dsc_get_slice_count(const >> struct intel_connector *connec >> num_joined_pipes); >> } >> >> +static void intel_dp_mst_compute_min_hblank(struct intel_crtc_state >> *crtc_state, >> + struct intel_connector *connector, >> + int bpp_x16) >> +{ >> + struct intel_encoder *encoder = connector->encoder; >> + struct intel_display *display = to_intel_display(encoder); >> + const struct drm_display_mode *adjusted_mode = >> + &crtc_state->hw.adjusted_mode; >> + int symbol_size = intel_dp_is_uhbr(crtc_state) ? 32 : 8; >> + int hblank; >> + >> + if (DISPLAY_VER(display) < 20) >> + return; >> + >> + /* Calculate min Hblank Link Layer Symbol Cycle Count for 8b/10b >> MST & 128b/132b */ >> + hblank = DIV_ROUND_UP((DIV_ROUND_UP(adjusted_mode->htotal - >> +adjusted_mode->hdisplay, 4) * bpp_x16), symbol_size); >> + >> + /* bit 8:0 minimum hblank symbol cylce count, i.e maximum value >> would be 511 */ >> + hblank = min(511, hblank); >> + >> + /* Software needs to adjust the BS/BE framing control from the >> calculated value */ >> + hblank = hblank - 2; Please reconsider your comments above. How does "Software needs to" in driver code help? But then "BS/BE" goes unexplained. Please try to actually help the reader instead of copy-pasting from bspec. Also, spell check. >> + >> + if (intel_dp_is_uhbr(crtc_state)) >> + crtc_state->min_hblank = max(hblank, 5); >> + else >> + crtc_state->min_hblank = max(hblank, 3); I'm not a huge fan of storing direct register values in crtc state. The member name is "min_hblank", the comment and logging for it say "min hblank", but there's zero indication what the value actually is. I think it's surprising it's some direct register value instead of the logical value. } >> + >> static int mst_stream_find_vcpi_slots_for_bpp(struct intel_dp *intel_dp, >> struct intel_crtc_state *crtc_state, >> int max_bpp, int min_bpp, >> @@ -284,6 +313,8 @@ static int mst_stream_find_vcpi_slots_for_bpp(struct >> intel_dp *intel_dp, >> remote_bw_overhead = >> intel_dp_mst_bw_overhead(crtc_state, connector, >> true, >> dsc_slice_count, link_bpp_x16); >> >> + intel_dp_mst_compute_min_hblank(crtc_state, connector, >> link_bpp_x16); >> + >> intel_dp_mst_compute_m_n(crtc_state, connector, >> local_bw_overhead, >> link_bpp_x16, >> @@ -1267,6 +1298,10 @@ static void mst_stream_enable(struct >> intel_atomic_state *state, >> TRANS_DP2_VFREQ_PIXEL_CLOCK(crtc_clock_hz >> & 0xffffff)); >> } >> >> + if (DISPLAY_VER(display) >= 20) >> + intel_de_write(display, DP_MIN_HBLANK_CTL(trans), >> + pipe_config->min_hblank); >> + >> enable_bs_jitter_was(pipe_config); >> >> intel_ddi_enable_transcoder_func(encoder, pipe_config); diff --git >> a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h >> index >> 765e6c0528fb0b5a894395b77a5edbf0b0c80009..7bd783931199e2e5c7e1 >> 5358bb4d2c904f28176a 100644 >> --- a/drivers/gpu/drm/i915/i915_reg.h >> +++ b/drivers/gpu/drm/i915/i915_reg.h >> @@ -3197,6 +3197,10 @@ >> #define _TRANS_DP2_VFREQLOW_D 0x630a8 >> #define TRANS_DP2_VFREQLOW(trans) >> _MMIO_TRANS(trans, _TRANS_DP2_VFREQLOW_A, >> _TRANS_DP2_VFREQLOW_B) >> >> +#define _DP_MIN_HBLANK_CTL_A 0x600ac >> +#define _DP_MIN_HBLANK_CTL_B 0x610ac >> +#define DP_MIN_HBLANK_CTL(trans) _MMIO_TRANS(trans, >> _DP_MIN_HBLANK_CTL_A, _DP_MIN_HBLANK_CTL_B) >> + >> /* SNB eDP training params */ >> /* SNB A-stepping */ >> #define EDP_LINK_TRAIN_400MV_0DB_SNB_A (0x38 << 22) >> >> --- >> base-commit: 048d83e7f9dae81c058d31c371634c1c317b3013 >> change-id: 20250103-hblank-cd7e78cbe178 >> >> Best regards, >> -- >> Arun R Murthy <arun.r.murthy@xxxxxxxxx> > -- Jani Nikula, Intel
