On Tue, 21 Jan 2025, Arun R Murthy <arun.r.mur...@intel.com> wrote:
> Mandate a minimum Hblank symbol cycle count between BlankingStart and
> BlankingEnd in 8b/10b MST and 128b/132b mode.
>
> 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)
> v6: Removed DP2.1 in comment as this is applicable for both DP2.1 and
> DP1.4 (Suraj)
> v7: crtc_state holds the logical values and the register value
> computation is moved to mst_enable() (Jani)
>
> Bspec: 74379
> Signed-off-by: Arun R Murthy <arun.r.mur...@intel.com>
> Reviewed-by: Suraj Kandpal <suraj.kand...@intel.com>
> ---
> .../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 | 53
> +++++++++++++++++++++-
> drivers/gpu/drm/i915/i915_reg.h | 4 ++
> 4 files changed, 58 insertions(+), 1 deletion(-)
>
> 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
> 1fbaa67e2fea77279f120bfb9755a2642550046c..07c671741513f7f263b7b233ffec71998745fd0f
> 100644
> --- a/drivers/gpu/drm/i915/display/intel_crtc_state_dump.c
> +++ b/drivers/gpu/drm/i915/display/intel_crtc_state_dump.c
> @@ -249,6 +249,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, "audio: %i, infoframes: %i, infoframes enabled: 0x%x\n",
> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h
> b/drivers/gpu/drm/i915/display/intel_display_types.h
> index
> 8271e50e36447a6c97a93ca0d0b83327ff6ee461..f525e266c0232e8c29ba3f84d2c81612f78e894b
> 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;
> 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
> 227bd2783e64105dc8dd521b99e7d04ce2e577cc..833167dc0e147dc5e793c0aeda1280453b38f385
> 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> @@ -209,6 +209,28 @@ 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);
> +
> + crtc_state->min_hblank = hblank;
> +}
> +
> int intel_dp_mtp_tu_compute_config(struct intel_dp *intel_dp,
> struct intel_crtc_state *crtc_state,
> int max_bpp, int min_bpp,
> @@ -266,6 +288,9 @@ int intel_dp_mtp_tu_compute_config(struct intel_dp
> *intel_dp,
>
> local_bw_overhead = intel_dp_mst_bw_overhead(crtc_state,
> false,
> 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,
> local_bw_overhead,
> link_bpp_x16,
> @@ -1265,7 +1290,7 @@ static void mst_stream_enable(struct intel_atomic_state
> *state,
> enum transcoder trans = pipe_config->cpu_transcoder;
> bool first_mst_stream = intel_dp->active_mst_links == 1;
> struct intel_crtc *pipe_crtc;
> - int ret, i;
> + int ret, i, min_hblank;
>
> drm_WARN_ON(display->drm, pipe_config->has_pch_encoder);
>
> @@ -1280,6 +1305,32 @@ static void mst_stream_enable(struct
> intel_atomic_state *state,
> TRANS_DP2_VFREQ_PIXEL_CLOCK(crtc_clock_hz &
> 0xffffff));
> }
>
> + if (DISPLAY_VER(display) >= 20) {
> + /*
> + * bit 8:0 minimum hblank symbol cylce count, i.e maximum value
> + * would be 511
> + */
If you defined the register field with REG_GENMASK(), you'd get the max
from there, the code would document itself, and the comment would be
unnecessary.
> + min_hblank = min(511, pipe_config->min_hblank);
> +
> + /*
> + * adjust the BlankingStart/BlankingEnd framing control from
> + * the calculated value
> + */
> + min_hblank = min_hblank - 2;
This means you'll never write values 511 or 510 to the register. Should
the order be changed?
But then... bspec says, "The maximum value programmed in this field
(MIN_HBLNK_LL_SYM_CYC_CNT_MAX) should be a value of 10."
Please double check the discrepancy.
> +
> + /*
> + * Minimum hblank accepted for 128b/132b would be 5 and for
> + * 8b/10b would be 3 symbol count
> + */
> + if (intel_dp_is_uhbr(pipe_config))
> + min_hblank = max(min_hblank, 5);
> + else
> + min_hblank = max(min_hblank, 3);
> +
> + intel_de_write(display, DP_MIN_HBLANK_CTL(trans),
> + min_hblank);
This is left in place, never cleared, and who knows what'll happen if
you unplug an MST hub and plug in an SST display. Bspec says "This
register should only be programmed to a non-zero value when in 8b/10b
MST or 128b/132b operation."
That's bound to be hard to debug, and never going to happen in CI,
because we won't be switching between SST/MST there.
BR,
Jani.
> + }
> +
> 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..7bd783931199e2e5c7e15358bb4d2c904f28176a
> 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: a15d2a84505eed8dbb58911147e44752734f3a88
> change-id: 20250121-hblank-ad8ce892eb3a
>
> Best regards,
--
Jani Nikula, Intel
