[PATCH 0/3] Rework/Correction on minimum hblank calculation

[PATCH 0/3] Rework/Correction on minimum hblank calculation

[Arun R Murthy]

Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
---
Arun R Murthy (3):
      drm/drm_dp_helper: export link symbol cycles calculation
      drm/i915/display: export function to count dsc slices
      drm/i915/audio: move min_hblank from dp_mst to audio

 drivers/gpu/drm/display/drm_dp_helper.c     | 10 ++--
 drivers/gpu/drm/i915/display/intel_audio.c  | 78 +++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/display/intel_dp_mst.c | 55 ++------------------
 drivers/gpu/drm/i915/display/intel_dp_mst.h |  3 ++
 include/drm/display/drm_dp_helper.h         |  5 ++
 5 files changed, 95 insertions(+), 56 deletions(-)
---
base-commit: c4fc93b0ec49f4b0105c142502b7d1d5de379950
change-id: 20250407-hblank-49b340aeba31

Best regards,
-- 
Arun R Murthy <arun.r.murthy@intel.com>

[PATCH 1/3] drm/drm_dp_helper: export link symbol cycles calculation

[Arun R Murthy]

Link symbol cycles are required for the calculation of the minimum
HBlank calculation.

Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
---
 drivers/gpu/drm/display/drm_dp_helper.c | 10 ++++++----
 include/drm/display/drm_dp_helper.h     |  5 +++++
 2 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/display/drm_dp_helper.c b/drivers/gpu/drm/display/drm_dp_helper.c
index 57828f2b7b5a0582ca4a6f2a9be2d5909fe8ad24..488aaef61e3aa8b25975dfc98cbccdf5b7f083f8 100644
--- a/drivers/gpu/drm/display/drm_dp_helper.c
+++ b/drivers/gpu/drm/display/drm_dp_helper.c
@@ -4393,17 +4393,18 @@ EXPORT_SYMBOL(drm_panel_dp_aux_backlight);
 #endif
 
 /* See DP Standard v2.1 2.6.4.4.1.1, 2.8.4.4, 2.8.7 */
-static int drm_dp_link_symbol_cycles(int lane_count, int pixels, int bpp_x16,
-				     int symbol_size, bool is_mst)
+int drm_dp_link_symbol_cycles(int lane_count, int pixels, int bpp_x16,
+			      int symbol_size, bool is_mst)
 {
 	int cycles = DIV_ROUND_UP(pixels * bpp_x16, 16 * symbol_size * lane_count);
 	int align = is_mst ? 4 / lane_count : 1;
 
 	return ALIGN(cycles, align);
 }
+EXPORT_SYMBOL(drm_dp_link_symbol_cycles);
 
-static int drm_dp_link_dsc_symbol_cycles(int lane_count, int pixels, int slice_count,
-					 int bpp_x16, int symbol_size, bool is_mst)
+int drm_dp_link_dsc_symbol_cycles(int lane_count, int pixels, int slice_count,
+				  int bpp_x16, int symbol_size, bool is_mst)
 {
 	int slice_pixels = DIV_ROUND_UP(pixels, slice_count);
 	int slice_data_cycles = drm_dp_link_symbol_cycles(lane_count, slice_pixels,
@@ -4412,6 +4413,7 @@ static int drm_dp_link_dsc_symbol_cycles(int lane_count, int pixels, int slice_c
 
 	return slice_count * (slice_data_cycles + slice_eoc_cycles);
 }
+EXPORT_SYMBOL(drm_dp_link_dsc_symbol_cycles);
 
 /**
  * drm_dp_bw_overhead - Calculate the BW overhead of a DP link stream
diff --git a/include/drm/display/drm_dp_helper.h b/include/drm/display/drm_dp_helper.h
index d9614e2c89397536f44bb7258e894628ae1dccc9..f6bfa3b74810438b837540dccb08a987bcdb5d03 100644
--- a/include/drm/display/drm_dp_helper.h
+++ b/include/drm/display/drm_dp_helper.h
@@ -972,4 +972,9 @@ int drm_dp_max_dprx_data_rate(int max_link_rate, int max_lanes);
 
 ssize_t drm_dp_vsc_sdp_pack(const struct drm_dp_vsc_sdp *vsc, struct dp_sdp *sdp);
 
+int drm_dp_link_symbol_cycles(int lane_count, int pixels, int bpp_x16,
+			      int symbol_size, bool is_mst);
+int drm_dp_link_dsc_symbol_cycles(int lane_count, int pixels, int slice_count,
+				  int bpp_x16, int symbol_size, bool is_mst);
+
 #endif /* _DRM_DP_HELPER_H_ */

-- 
2.25.1

[PATCH 2/3] drm/i915/display: export function to count dsc slices

[Arun R Murthy]

Export the function to calculate dsc slice count. This will be used in
minimum hblank calculation.

Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
---
 drivers/gpu/drm/i915/display/intel_dp_mst.c | 4 ++--
 drivers/gpu/drm/i915/display/intel_dp_mst.h | 3 +++
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
index d2988b9a6e7bd55d2ea4d34230c4d017e1f4cc93..af98a0d0e8376a79ce1ab6ff3c4f6af30f4d3e73 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
@@ -198,8 +198,8 @@ static int intel_dp_mst_calc_pbn(int pixel_clock, int bpp_x16, int bw_overhead)
 	return DIV_ROUND_UP(effective_data_rate * 64, 54 * 1000);
 }
 
-static int intel_dp_mst_dsc_get_slice_count(const struct intel_connector *connector,
-					    const struct intel_crtc_state *crtc_state)
+int intel_dp_mst_dsc_get_slice_count(const struct intel_connector *connector,
+				     const struct intel_crtc_state *crtc_state)
 {
 	const struct drm_display_mode *adjusted_mode =
 		&crtc_state->hw.adjusted_mode;
diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.h b/drivers/gpu/drm/i915/display/intel_dp_mst.h
index c1bbfeb02ca9e7afb96156f58143275225245956..b3d5b347c4b5258cefb0e559a5cb8d0d769111cc 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_mst.h
+++ b/drivers/gpu/drm/i915/display/intel_dp_mst.h
@@ -12,6 +12,7 @@ struct drm_connector_state;
 struct intel_atomic_state;
 struct intel_crtc;
 struct intel_crtc_state;
+struct intel_connector;
 struct intel_digital_port;
 struct intel_dp;
 struct intel_link_bw_limits;
@@ -35,5 +36,7 @@ int intel_dp_mtp_tu_compute_config(struct intel_dp *intel_dp,
 				   struct intel_crtc_state *crtc_state,
 				   struct drm_connector_state *conn_state,
 				   int min_bpp_x16, int max_bpp_x16, int bpp_step_x16, bool dsc);
+int intel_dp_mst_dsc_get_slice_count(const struct intel_connector *connector,
+				     const struct intel_crtc_state *crtc_state);
 
 #endif /* __INTEL_DP_MST_H__ */

-- 
2.25.1

[PATCH 3/3] drm/i915/audio: move min_hblank from dp_mst to audio

[Arun R Murthy]

Minimum HBlank is programmed to address jitter for high resolutions with
high refresh rates that have small Hblank, specifically where Hblank is
smaller than one MTP.

Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
---
 drivers/gpu/drm/i915/display/intel_audio.c  | 78 +++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/display/intel_dp_mst.c | 51 +------------------
 2 files changed, 79 insertions(+), 50 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_audio.c b/drivers/gpu/drm/i915/display/intel_audio.c
index ea935a5d94c87202a68f84b28b0152835f47fe73..b73cb208a37c6d4928c1ce16d2ab689626dcbbc5 100644
--- a/drivers/gpu/drm/i915/display/intel_audio.c
+++ b/drivers/gpu/drm/i915/display/intel_audio.c
@@ -27,9 +27,11 @@
 #include <drm/drm_edid.h>
 #include <drm/drm_eld.h>
 #include <drm/drm_fixed.h>
+#include <drm/display/drm_dp_helper.h>
 #include <drm/intel/i915_component.h>
 
 #include "i915_drv.h"
+#include "i915_reg.h"
 #include "intel_atomic.h"
 #include "intel_audio.h"
 #include "intel_audio_regs.h"
@@ -37,6 +39,8 @@
 #include "intel_crtc.h"
 #include "intel_de.h"
 #include "intel_display_types.h"
+#include "intel_dp.h"
+#include "intel_dp_mst.h"
 #include "intel_lpe_audio.h"
 
 /**
@@ -184,6 +188,62 @@ static const struct hdmi_aud_ncts hdmi_aud_ncts_36bpp[] = {
 	{ 192000, TMDS_445_5M, 20480, 371250 },
 };
 
+static void intel_audio_compute_min_hblank(struct intel_crtc_state *crtc_state,
+					   struct drm_connector_state *conn_state)
+{
+	struct intel_display *display = to_intel_display(crtc_state);
+	const struct drm_display_mode *adjusted_mode =
+					&crtc_state->hw.adjusted_mode;
+	struct intel_connector *connector = to_intel_connector(conn_state->connector);
+	int symbol_size = intel_dp_is_uhbr(crtc_state) ? 32 : 8;
+	int min_sym_cycles = intel_dp_is_uhbr(crtc_state) ? 3 : 5;
+	bool is_dsc = crtc_state->dsc.compression_enable;
+	int bpp = is_dsc ? crtc_state->dsc.compressed_bpp_x16 :
+			   crtc_state->pipe_bpp;
+	bool is_mst = intel_crtc_has_type(crtc_state, INTEL_OUTPUT_DP_MST);
+	u8 lane_count = crtc_state->lane_count;
+	int min_hblank, htotal, hactive;
+	int hactive_sym_cycles, htotal_sym_cycles;
+	int dsc_slices = intel_dp_mst_dsc_get_slice_count(connector,
+							  crtc_state);
+
+	if (DISPLAY_VER(display) < 30)
+		return;
+
+	/* Calculate min Hblank Link Layer Symbol Cycle Count for 8b/10b MST & 128b/132b */
+	hactive = adjusted_mode->hdisplay;
+	htotal = adjusted_mode->htotal;
+
+	hactive_sym_cycles = is_dsc ? drm_dp_link_dsc_symbol_cycles(lane_count,
+								    hactive,
+								    dsc_slices,
+								    bpp,
+								    symbol_size,
+								    is_mst) :
+				      drm_dp_link_symbol_cycles(lane_count,
+								hactive,
+								bpp,
+								symbol_size,
+								is_mst);
+	htotal_sym_cycles = htotal * hactive_sym_cycles / hactive;
+
+	min_hblank = DIV_ROUND_UP((htotal_sym_cycles * bpp),
+				  (4 * symbol_size)) - hactive_sym_cycles;
+	/* minimum Hblank calculation: https://groups.vesa.org/wg/DP/document/20494 */
+	min_hblank = max(min_hblank, min_sym_cycles);
+
+	/*
+	 * adjust the BlankingStart/BlankingEnd framing control from
+	 * the calculated value
+	 */
+	min_hblank = min_hblank - 2;
+
+	/* Maximum value to be programmed is limited to 10 */
+	min_hblank = min(10, min_hblank);
+	crtc_state->min_hblank = min_hblank;
+}
+
+
 /*
  * WA_14020863754: Implement Audio Workaround
  * Corner case with Min Hblank Fix can cause audio hang
@@ -715,6 +775,8 @@ bool intel_audio_compute_config(struct intel_encoder *encoder,
 	memcpy(crtc_state->eld, connector->eld, sizeof(crtc_state->eld));
 
 	crtc_state->eld[6] = drm_av_sync_delay(connector, adjusted_mode) / 2;
+
+	intel_audio_compute_min_hblank(crtc_state, conn_state);
 	mutex_unlock(&connector->eld_mutex);
 
 	return true;
@@ -778,6 +840,19 @@ void intel_audio_codec_enable(struct intel_encoder *encoder,
 	intel_lpe_audio_notify(display, cpu_transcoder, port, crtc_state->eld,
 			       crtc_state->port_clock,
 			       intel_crtc_has_dp_encoder(crtc_state));
+
+	if (DISPLAY_VER(display) >= 30) {
+		/*
+		 * Address issues for resolutions with high refresh rate that
+		 * have small Hblank, specifically where Hblank is smaller than
+		 * one MTP. Simulations indicate this will address the
+		 * jitter issues that currently causes BS to be immediately
+		 * followed by BE which DPRX devices are unable to handle.
+		 * https://groups.vesa.org/wg/DP/document/20494
+		 */
+		intel_de_write(display, DP_MIN_HBLANK_CTL(cpu_transcoder),
+			       crtc_state->min_hblank);
+	}
 }
 
 /**
@@ -834,6 +909,9 @@ void intel_audio_codec_disable(struct intel_encoder *encoder,
 	}
 
 	intel_lpe_audio_notify(display, cpu_transcoder, port, NULL, 0, false);
+
+	if (DISPLAY_VER(display) >= 30)
+		intel_de_write(display, DP_MIN_HBLANK_CTL(cpu_transcoder), 0);
 }
 
 static void intel_acomp_get_config(struct intel_encoder *encoder,
diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
index af98a0d0e8376a79ce1ab6ff3c4f6af30f4d3e73..dae74c645c1a1d716504b7843fe1a5c8ace0d87d 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
@@ -211,26 +211,6 @@ int intel_dp_mst_dsc_get_slice_count(const struct intel_connector *connector,
 					    num_joined_pipes);
 }
 
-static void intel_dp_mst_compute_min_hblank(struct intel_crtc_state *crtc_state,
-					    int bpp_x16)
-{
-	struct intel_display *display = to_intel_display(crtc_state);
-	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,
 				   struct drm_connector_state *conn_state,
@@ -301,8 +281,6 @@ 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, link_bpp_x16);
-
 		intel_dp_mst_compute_m_n(crtc_state,
 					 local_bw_overhead,
 					 link_bpp_x16,
@@ -998,7 +976,6 @@ static void mst_stream_disable(struct intel_atomic_state *state,
 	struct intel_dp *intel_dp = to_primary_dp(encoder);
 	struct intel_connector *connector =
 		to_intel_connector(old_conn_state->connector);
-	enum transcoder trans = old_crtc_state->cpu_transcoder;
 
 	drm_dbg_kms(display->drm, "active links %d\n",
 		    intel_dp->mst.active_links);
@@ -1009,9 +986,6 @@ static void mst_stream_disable(struct intel_atomic_state *state,
 	intel_hdcp_disable(intel_mst->connector);
 
 	intel_dp_sink_disable_decompression(state, connector, old_crtc_state);
-
-	if (DISPLAY_VER(display) >= 20)
-		intel_de_write(display, DP_MIN_HBLANK_CTL(trans), 0);
 }
 
 static void mst_stream_post_disable(struct intel_atomic_state *state,
@@ -1286,7 +1260,7 @@ static void mst_stream_enable(struct intel_atomic_state *state,
 	enum transcoder trans = pipe_config->cpu_transcoder;
 	bool first_mst_stream = intel_dp->mst.active_links == 1;
 	struct intel_crtc *pipe_crtc;
-	int ret, i, min_hblank;
+	int ret, i;
 
 	drm_WARN_ON(display->drm, pipe_config->has_pch_encoder);
 
@@ -1301,29 +1275,6 @@ static void mst_stream_enable(struct intel_atomic_state *state,
 			       TRANS_DP2_VFREQ_PIXEL_CLOCK(crtc_clock_hz & 0xffffff));
 	}
 
-	if (DISPLAY_VER(display) >= 20) {
-		/*
-		 * adjust the BlankingStart/BlankingEnd framing control from
-		 * the calculated value
-		 */
-		min_hblank = pipe_config->min_hblank - 2;
-
-		/* Maximum value to be programmed is limited to 0x10 */
-		min_hblank = min(0x10, min_hblank);
-
-		/*
-		 * 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);
-	}
-
 	enable_bs_jitter_was(pipe_config);
 
 	intel_ddi_enable_transcoder_func(encoder, pipe_config);

-- 
2.25.1

✗ CI.Patch_applied: failure for Rework/Correction on minimum hblank calculation

[Patchwork]

== Series Details ==

Series: Rework/Correction on minimum hblank calculation
URL   : https://patchwork.freedesktop.org/series/147360/
State : failure

== Summary ==

=== Applying kernel patches on branch 'drm-tip' with base: ===
Base commit: 6fda8f56691f drm-tip: 2025y-04m-07d-21h-23m-26s UTC integration manifest
=== git am output follows ===
error: patch failed: drivers/gpu/drm/i915/display/intel_dp_mst.c:998
error: drivers/gpu/drm/i915/display/intel_dp_mst.c: patch does not apply
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Applying: drm/drm_dp_helper: export link symbol cycles calculation
Applying: drm/i915/display: export function to count dsc slices
Applying: drm/i915/audio: move min_hblank from dp_mst to audio
Patch failed at 0003 drm/i915/audio: move min_hblank from dp_mst to audio
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

Re: [PATCH 0/3] Rework/Correction on minimum hblank calculation

[Imre Deak]

Hi,

thanks for following up on this.

You could've added some detail about what the patchset is doing.

--Imre

On Tue, Apr 08, 2025 at 09:40:33AM +0530, Arun R Murthy wrote:
> Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
> ---
> Arun R Murthy (3):
>       drm/drm_dp_helper: export link symbol cycles calculation
>       drm/i915/display: export function to count dsc slices
>       drm/i915/audio: move min_hblank from dp_mst to audio
> 
>  drivers/gpu/drm/display/drm_dp_helper.c     | 10 ++--
>  drivers/gpu/drm/i915/display/intel_audio.c  | 78 +++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/display/intel_dp_mst.c | 55 ++------------------
>  drivers/gpu/drm/i915/display/intel_dp_mst.h |  3 ++
>  include/drm/display/drm_dp_helper.h         |  5 ++
>  5 files changed, 95 insertions(+), 56 deletions(-)
> ---
> base-commit: c4fc93b0ec49f4b0105c142502b7d1d5de379950
> change-id: 20250407-hblank-49b340aeba31
> 
> Best regards,
> -- 
> Arun R Murthy <arun.r.murthy@intel.com>
> 

Re: [PATCH 1/3] drm/drm_dp_helper: export link symbol cycles calculation

[Imre Deak]

On Tue, Apr 08, 2025 at 09:40:34AM +0530, Arun R Murthy wrote:
> Link symbol cycles are required for the calculation of the minimum
> HBlank calculation.
> 
> Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
> ---
>  drivers/gpu/drm/display/drm_dp_helper.c | 10 ++++++----
>  include/drm/display/drm_dp_helper.h     |  5 +++++
>  2 files changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/display/drm_dp_helper.c b/drivers/gpu/drm/display/drm_dp_helper.c
> index 57828f2b7b5a0582ca4a6f2a9be2d5909fe8ad24..488aaef61e3aa8b25975dfc98cbccdf5b7f083f8 100644
> --- a/drivers/gpu/drm/display/drm_dp_helper.c
> +++ b/drivers/gpu/drm/display/drm_dp_helper.c
> @@ -4393,17 +4393,18 @@ EXPORT_SYMBOL(drm_panel_dp_aux_backlight);
>  #endif
>  
>  /* See DP Standard v2.1 2.6.4.4.1.1, 2.8.4.4, 2.8.7 */
> -static int drm_dp_link_symbol_cycles(int lane_count, int pixels, int bpp_x16,
> -				     int symbol_size, bool is_mst)
> +int drm_dp_link_symbol_cycles(int lane_count, int pixels, int bpp_x16,
> +			      int symbol_size, bool is_mst)
>  {
>  	int cycles = DIV_ROUND_UP(pixels * bpp_x16, 16 * symbol_size * lane_count);
>  	int align = is_mst ? 4 / lane_count : 1;
>  
>  	return ALIGN(cycles, align);
>  }
> +EXPORT_SYMBOL(drm_dp_link_symbol_cycles);
>  
> -static int drm_dp_link_dsc_symbol_cycles(int lane_count, int pixels, int slice_count,
> -					 int bpp_x16, int symbol_size, bool is_mst)
> +int drm_dp_link_dsc_symbol_cycles(int lane_count, int pixels, int slice_count,
> +				  int bpp_x16, int symbol_size, bool is_mst)
>  {
>  	int slice_pixels = DIV_ROUND_UP(pixels, slice_count);
>  	int slice_data_cycles = drm_dp_link_symbol_cycles(lane_count, slice_pixels,

Could you simplify this i/f by calculating the DSC/non-DSC specific
symbol count based on whether the passed in dsc_slice_count is non-zero
or zero respectively (renaming this function to drm_dp_link_symbol_cycles() and the
original drm_dp_link_symbol_cycles() to stg else)?

That way you'd need to export only one function and drm_dp_bw_overhead()
would also get simplified.

> @@ -4412,6 +4413,7 @@ static int drm_dp_link_dsc_symbol_cycles(int lane_count, int pixels, int slice_c
>  
>  	return slice_count * (slice_data_cycles + slice_eoc_cycles);
>  }
> +EXPORT_SYMBOL(drm_dp_link_dsc_symbol_cycles);
>  
>  /**
>   * drm_dp_bw_overhead - Calculate the BW overhead of a DP link stream
> diff --git a/include/drm/display/drm_dp_helper.h b/include/drm/display/drm_dp_helper.h
> index d9614e2c89397536f44bb7258e894628ae1dccc9..f6bfa3b74810438b837540dccb08a987bcdb5d03 100644
> --- a/include/drm/display/drm_dp_helper.h
> +++ b/include/drm/display/drm_dp_helper.h
> @@ -972,4 +972,9 @@ int drm_dp_max_dprx_data_rate(int max_link_rate, int max_lanes);
>  
>  ssize_t drm_dp_vsc_sdp_pack(const struct drm_dp_vsc_sdp *vsc, struct dp_sdp *sdp);
>  
> +int drm_dp_link_symbol_cycles(int lane_count, int pixels, int bpp_x16,
> +			      int symbol_size, bool is_mst);
> +int drm_dp_link_dsc_symbol_cycles(int lane_count, int pixels, int slice_count,
> +				  int bpp_x16, int symbol_size, bool is_mst);
> +
>  #endif /* _DRM_DP_HELPER_H_ */
> 
> -- 
> 2.25.1
> 

Re: [PATCH 3/3] drm/i915/audio: move min_hblank from dp_mst to audio

[Imre Deak]

On Tue, Apr 08, 2025 at 09:40:36AM +0530, Arun R Murthy wrote:
> Minimum HBlank is programmed to address jitter for high resolutions with
> high refresh rates that have small Hblank, specifically where Hblank is
> smaller than one MTP.

I wondered if following the standard practice of one change per commit
is indeed not practical as you do it in this case.

> 
> Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_audio.c  | 78 +++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/display/intel_dp_mst.c | 51 +------------------
>  2 files changed, 79 insertions(+), 50 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_audio.c b/drivers/gpu/drm/i915/display/intel_audio.c
> index ea935a5d94c87202a68f84b28b0152835f47fe73..b73cb208a37c6d4928c1ce16d2ab689626dcbbc5 100644
> --- a/drivers/gpu/drm/i915/display/intel_audio.c
> +++ b/drivers/gpu/drm/i915/display/intel_audio.c
> @@ -27,9 +27,11 @@
>  #include <drm/drm_edid.h>
>  #include <drm/drm_eld.h>
>  #include <drm/drm_fixed.h>
> +#include <drm/display/drm_dp_helper.h>
>  #include <drm/intel/i915_component.h>
>  
>  #include "i915_drv.h"
> +#include "i915_reg.h"
>  #include "intel_atomic.h"
>  #include "intel_audio.h"
>  #include "intel_audio_regs.h"
> @@ -37,6 +39,8 @@
>  #include "intel_crtc.h"
>  #include "intel_de.h"
>  #include "intel_display_types.h"
> +#include "intel_dp.h"
> +#include "intel_dp_mst.h"
>  #include "intel_lpe_audio.h"
>  
>  /**
> @@ -184,6 +188,62 @@ static const struct hdmi_aud_ncts hdmi_aud_ncts_36bpp[] = {
>  	{ 192000, TMDS_445_5M, 20480, 371250 },
>  };
>  
> +static void intel_audio_compute_min_hblank(struct intel_crtc_state *crtc_state,
> +					   struct drm_connector_state *conn_state)

I don't see any audio specific parameters in the calculation. This
function is also called for non-DP encoders (HDMI) and for DP encoders
it's called only if the DP sink supports audio.

Isn't it supposed to be a generic (not audio specific) DP state
calculated elsewhere (intel_dp.c)?

> +{
> +	struct intel_display *display = to_intel_display(crtc_state);
> +	const struct drm_display_mode *adjusted_mode =
> +					&crtc_state->hw.adjusted_mode;
> +	struct intel_connector *connector = to_intel_connector(conn_state->connector);
> +	int symbol_size = intel_dp_is_uhbr(crtc_state) ? 32 : 8;
> +	int min_sym_cycles = intel_dp_is_uhbr(crtc_state) ? 3 : 5;

Would be good to figure out and comment here what the 3 and 5 symbols
stand for.

> +	bool is_dsc = crtc_state->dsc.compression_enable;
> +	int bpp = is_dsc ? crtc_state->dsc.compressed_bpp_x16 :
> +			   crtc_state->pipe_bpp;

This looks incorrect as the units for compressed_bpp_x16 and pipe_bpp
are different.

> +	bool is_mst = intel_crtc_has_type(crtc_state, INTEL_OUTPUT_DP_MST);
> +	u8 lane_count = crtc_state->lane_count;
> +	int min_hblank, htotal, hactive;

I don't think it's worth adding separate variables for lane_count,
htotal, hactive.

> +	int hactive_sym_cycles, htotal_sym_cycles;
> +	int dsc_slices = intel_dp_mst_dsc_get_slice_count(connector,
> +							  crtc_state);
> +
> +	if (DISPLAY_VER(display) < 30)
> +		return;
> +
> +	/* Calculate min Hblank Link Layer Symbol Cycle Count for 8b/10b MST & 128b/132b */
> +	hactive = adjusted_mode->hdisplay;
> +	htotal = adjusted_mode->htotal;
> +
> +	hactive_sym_cycles = is_dsc ? drm_dp_link_dsc_symbol_cycles(lane_count,

To get LL instead of ML symbol cycles as required by bspec, you should
pass here lane_count=4 always, independent of the actual lane_count in
crtc_state.

> +								    hactive,
> +								    dsc_slices,
> +								    bpp,
> +								    symbol_size,
> +								    is_mst) :
> +				      drm_dp_link_symbol_cycles(lane_count,
> +								hactive,
> +								bpp,

This expects bpp_x16 units, but pipe_bpp that would be passed here has
different units.

> +								symbol_size,
> +								is_mst);
> +	htotal_sym_cycles = htotal * hactive_sym_cycles / hactive;
> +
> +	min_hblank = DIV_ROUND_UP((htotal_sym_cycles * bpp),
> +				  (4 * symbol_size)) - hactive_sym_cycles;

Not sure what the above division adjusts for, I think it should be just

	min_hblank = htotal_sym_cycles - hactive_sym_cycles

> +	/* minimum Hblank calculation: https://groups.vesa.org/wg/DP/document/20494 */
> +	min_hblank = max(min_hblank, min_sym_cycles);
> +
> +	/*
> +	 * adjust the BlankingStart/BlankingEnd framing control from
> +	 * the calculated value
> +	 */
> +	min_hblank = min_hblank - 2;
> +
> +	/* Maximum value to be programmed is limited to 10 */
> +	min_hblank = min(10, min_hblank);
> +	crtc_state->min_hblank = min_hblank;
> +}
> +
> +
>  /*
>   * WA_14020863754: Implement Audio Workaround
>   * Corner case with Min Hblank Fix can cause audio hang
> @@ -715,6 +775,8 @@ bool intel_audio_compute_config(struct intel_encoder *encoder,
>  	memcpy(crtc_state->eld, connector->eld, sizeof(crtc_state->eld));
>  
>  	crtc_state->eld[6] = drm_av_sync_delay(connector, adjusted_mode) / 2;
> +
> +	intel_audio_compute_min_hblank(crtc_state, conn_state);
>  	mutex_unlock(&connector->eld_mutex);
>  
>  	return true;
> @@ -778,6 +840,19 @@ void intel_audio_codec_enable(struct intel_encoder *encoder,
>  	intel_lpe_audio_notify(display, cpu_transcoder, port, crtc_state->eld,
>  			       crtc_state->port_clock,
>  			       intel_crtc_has_dp_encoder(crtc_state));
> +
> +	if (DISPLAY_VER(display) >= 30) {
> +		/*
> +		 * Address issues for resolutions with high refresh rate that
> +		 * have small Hblank, specifically where Hblank is smaller than
> +		 * one MTP. Simulations indicate this will address the
> +		 * jitter issues that currently causes BS to be immediately
> +		 * followed by BE which DPRX devices are unable to handle.
> +		 * https://groups.vesa.org/wg/DP/document/20494
> +		 */
> +		intel_de_write(display, DP_MIN_HBLANK_CTL(cpu_transcoder),
> +			       crtc_state->min_hblank);
> +	}

It's not clear why this programming is audio specific, based on my
earlier comment above.

>  }
>  
>  /**
> @@ -834,6 +909,9 @@ void intel_audio_codec_disable(struct intel_encoder *encoder,
>  	}
>  
>  	intel_lpe_audio_notify(display, cpu_transcoder, port, NULL, 0, false);
> +
> +	if (DISPLAY_VER(display) >= 30)
> +		intel_de_write(display, DP_MIN_HBLANK_CTL(cpu_transcoder), 0);
>  }
>  
>  static void intel_acomp_get_config(struct intel_encoder *encoder,
> diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> index af98a0d0e8376a79ce1ab6ff3c4f6af30f4d3e73..dae74c645c1a1d716504b7843fe1a5c8ace0d87d 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> @@ -211,26 +211,6 @@ int intel_dp_mst_dsc_get_slice_count(const struct intel_connector *connector,
>  					    num_joined_pipes);
>  }
>  
> -static void intel_dp_mst_compute_min_hblank(struct intel_crtc_state *crtc_state,
> -					    int bpp_x16)
> -{
> -	struct intel_display *display = to_intel_display(crtc_state);
> -	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,
>  				   struct drm_connector_state *conn_state,
> @@ -301,8 +281,6 @@ 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, link_bpp_x16);
> -
>  		intel_dp_mst_compute_m_n(crtc_state,
>  					 local_bw_overhead,
>  					 link_bpp_x16,
> @@ -998,7 +976,6 @@ static void mst_stream_disable(struct intel_atomic_state *state,
>  	struct intel_dp *intel_dp = to_primary_dp(encoder);
>  	struct intel_connector *connector =
>  		to_intel_connector(old_conn_state->connector);
> -	enum transcoder trans = old_crtc_state->cpu_transcoder;
>  
>  	drm_dbg_kms(display->drm, "active links %d\n",
>  		    intel_dp->mst.active_links);
> @@ -1009,9 +986,6 @@ static void mst_stream_disable(struct intel_atomic_state *state,
>  	intel_hdcp_disable(intel_mst->connector);
>  
>  	intel_dp_sink_disable_decompression(state, connector, old_crtc_state);
> -
> -	if (DISPLAY_VER(display) >= 20)
> -		intel_de_write(display, DP_MIN_HBLANK_CTL(trans), 0);
>  }
>  
>  static void mst_stream_post_disable(struct intel_atomic_state *state,
> @@ -1286,7 +1260,7 @@ static void mst_stream_enable(struct intel_atomic_state *state,
>  	enum transcoder trans = pipe_config->cpu_transcoder;
>  	bool first_mst_stream = intel_dp->mst.active_links == 1;
>  	struct intel_crtc *pipe_crtc;
> -	int ret, i, min_hblank;
> +	int ret, i;
>  
>  	drm_WARN_ON(display->drm, pipe_config->has_pch_encoder);
>  
> @@ -1301,29 +1275,6 @@ static void mst_stream_enable(struct intel_atomic_state *state,
>  			       TRANS_DP2_VFREQ_PIXEL_CLOCK(crtc_clock_hz & 0xffffff));
>  	}
>  
> -	if (DISPLAY_VER(display) >= 20) {
> -		/*
> -		 * adjust the BlankingStart/BlankingEnd framing control from
> -		 * the calculated value
> -		 */
> -		min_hblank = pipe_config->min_hblank - 2;
> -
> -		/* Maximum value to be programmed is limited to 0x10 */
> -		min_hblank = min(0x10, min_hblank);
> -
> -		/*
> -		 * 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);
> -	}
> -
>  	enable_bs_jitter_was(pipe_config);
>  
>  	intel_ddi_enable_transcoder_func(encoder, pipe_config);
> 
> -- 
> 2.25.1
>