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

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

[Arun R Murthy]

Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
---
Changes in v3:
- EDITME: describe what is new in this series revision.
- EDITME: use bulletpoints and terse descriptions.
- Link to v2: https://lore.kernel.org/r/20250415-hblank-v2-0-1a23e9d97360@intel.com

Changes in v2:
- EDITME: describe what is new in this series revision.
- EDITME: use bulletpoints and terse descriptions.
- Link to v1: https://lore.kernel.org/r/20250408-hblank-v1-0-4ba17aebee65@intel.com

---
Arun R Murthy (3):
      drm/display/dp: Export fn to calculate link symbol cycles
      drm/i915/display: export function to count dsc slices
      drm/i915/display: move min_hblank from dp_mst.c to dp.c

 drivers/gpu/drm/display/drm_dp_helper.c      | 53 ++++++++++++-----------
 drivers/gpu/drm/i915/display/intel_display.c | 18 ++++++++
 drivers/gpu/drm/i915/display/intel_dp.c      | 64 ++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/display/intel_dp.h      |  4 ++
 drivers/gpu/drm/i915/display/intel_dp_mst.c  | 56 ++----------------------
 drivers/gpu/drm/i915/display/intel_dp_mst.h  |  3 ++
 include/drm/display/drm_dp_helper.h          |  2 +
 7 files changed, 122 insertions(+), 78 deletions(-)
---
base-commit: c4fc93b0ec49f4b0105c142502b7d1d5de379950
change-id: 20250407-hblank-49b340aeba31

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

[PATCH v3 1/3] drm/display/dp: Export fn to calculate link symbol cycles

[Arun R Murthy]

Unify the function to calculate the link symbol cycles for both dsc and
non-dsc case and export the function so that it can be used in the
respective platform display drivers for other calculations.

v2: unify the fn for both dsc and non-dsc case (Imre)
v3: rename drm_dp_link_symbol_cycles to drm_dp_link_data_symbol_cycles
    retain slice_eoc_cycles as is (Imre)

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

diff --git a/drivers/gpu/drm/display/drm_dp_helper.c b/drivers/gpu/drm/display/drm_dp_helper.c
index 57828f2b7b5a0582ca4a6f2a9be2d5909fe8ad24..5ce8ccc3310fb71b39ea5f74c4022474c180f727 100644
--- a/drivers/gpu/drm/display/drm_dp_helper.c
+++ b/drivers/gpu/drm/display/drm_dp_helper.c
@@ -4392,26 +4392,33 @@ 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 cycles = DIV_ROUND_UP(pixels * bpp_x16, 16 * symbol_size * lane_count);
-	int align = is_mst ? 4 / lane_count : 1;
-
-	return ALIGN(cycles, align);
-}
-
-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 slice_pixels = DIV_ROUND_UP(pixels, slice_count);
-	int slice_data_cycles = drm_dp_link_symbol_cycles(lane_count, slice_pixels,
-							  bpp_x16, symbol_size, is_mst);
+/**
+ * drm_dp_link_data_symbol_cycles - calculate the link symbol count
+ * @lane_coount: DP link lane count
+ * @pixels: horizontal active pixels
+ * @bpp_x16: bits per pixel in .4 binary fixed format
+ * @symbol_size: DP symbol size
+ * @is_mst: is mst or sst
+ * @slice_count: number of slices
+ *
+ * Calculate the link symbol cycles for both dsc and non dsc case and
+ * return the count.
+ */
+int drm_dp_link_data_symbol_cycles(int lane_count, int pixels, int bpp_x16,
+				   int symbol_size, bool is_mst, int slice_count)
+{
+	int slice_pixels = slice_count ? DIV_ROUND_UP(pixels, slice_count) :
+					 pixels;
+	int cycles = DIV_ROUND_UP(slice_pixels * bpp_x16,
+				  (6 * symbol_size * lane_count));
+	int slice_data_cycles = ALIGN(cycles, is_mst ? (4 / lane_count) : 1);
 	int slice_eoc_cycles = is_mst ? 4 / lane_count : 1;
 
-	return slice_count * (slice_data_cycles + slice_eoc_cycles);
+	return slice_count ? (slice_count *
+			      (slice_data_cycles + slice_eoc_cycles)) :
+			      slice_data_cycles;
 }
+EXPORT_SYMBOL(drm_dp_link_data_symbol_cycles);
 
 /**
  * drm_dp_bw_overhead - Calculate the BW overhead of a DP link stream
@@ -4486,15 +4493,9 @@ int drm_dp_bw_overhead(int lane_count, int hactive,
 	WARN_ON((flags & DRM_DP_BW_OVERHEAD_UHBR) &&
 		(flags & DRM_DP_BW_OVERHEAD_FEC));
 
-	if (flags & DRM_DP_BW_OVERHEAD_DSC)
-		symbol_cycles = drm_dp_link_dsc_symbol_cycles(lane_count, hactive,
-							      dsc_slice_count,
-							      bpp_x16, symbol_size,
-							      is_mst);
-	else
-		symbol_cycles = drm_dp_link_symbol_cycles(lane_count, hactive,
-							  bpp_x16, symbol_size,
-							  is_mst);
+	symbol_cycles = drm_dp_link_data_symbol_cycles(lane_count, hactive,
+						       bpp_x16, symbol_size,
+						       is_mst, dsc_slice_count);
 
 	return DIV_ROUND_UP_ULL(mul_u32_u32(symbol_cycles * symbol_size * lane_count,
 					    overhead * 16),
diff --git a/include/drm/display/drm_dp_helper.h b/include/drm/display/drm_dp_helper.h
index d9614e2c89397536f44bb7258e894628ae1dccc9..98bbbe98e5bc0ce0f9cdf513b2c5ea90bb5caffb 100644
--- a/include/drm/display/drm_dp_helper.h
+++ b/include/drm/display/drm_dp_helper.h
@@ -971,5 +971,7 @@ int drm_dp_bw_channel_coding_efficiency(bool is_uhbr);
 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_data_symbol_cycles(int lane_count, int pixels, int bpp_x16,
+				   int symbol_size, bool is_mst, int slice_count);
 
 #endif /* _DRM_DP_HELPER_H_ */

-- 
2.25.1

RE: [PATCH v3 1/3] drm/display/dp: Export fn to calculate link symbol cycles

[Kandpal, Suraj]

> -----Original Message-----
> From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of Arun R
> Murthy
> Sent: Thursday, April 17, 2025 4:22 PM
> To: dri-devel@lists.freedesktop.org; intel-gfx@lists.freedesktop.org; intel-
> xe@lists.freedesktop.org
> Cc: Govindapillai, Vinod <vinod.govindapillai@intel.com>; Deak, Imre
> <imre.deak@intel.com>; Murthy, Arun R <arun.r.murthy@intel.com>
> Subject: [PATCH v3 1/3] drm/display/dp: Export fn to calculate link symbol
> cycles
> 
> Unify the function to calculate the link symbol cycles for both dsc and non-dsc
> case and export the function so that it can be used in the respective platform
> display drivers for other calculations.
> 
> v2: unify the fn for both dsc and non-dsc case (Imre)
> v3: rename drm_dp_link_symbol_cycles to drm_dp_link_data_symbol_cycles
>     retain slice_eoc_cycles as is (Imre)
> 
> Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
> ---
>  drivers/gpu/drm/display/drm_dp_helper.c | 53 +++++++++++++++++--------------
> --
>  include/drm/display/drm_dp_helper.h     |  2 ++
>  2 files changed, 29 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/gpu/drm/display/drm_dp_helper.c
> b/drivers/gpu/drm/display/drm_dp_helper.c
> index
> 57828f2b7b5a0582ca4a6f2a9be2d5909fe8ad24..5ce8ccc3310fb71b39ea5f74c4
> 022474c180f727 100644
> --- a/drivers/gpu/drm/display/drm_dp_helper.c
> +++ b/drivers/gpu/drm/display/drm_dp_helper.c
> @@ -4392,26 +4392,33 @@ 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 cycles = DIV_ROUND_UP(pixels * bpp_x16, 16 * symbol_size *
> lane_count);
> -	int align = is_mst ? 4 / lane_count : 1;
> -
> -	return ALIGN(cycles, align);
> -}
> -
> -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 slice_pixels = DIV_ROUND_UP(pixels, slice_count);
> -	int slice_data_cycles = drm_dp_link_symbol_cycles(lane_count,
> slice_pixels,
> -							  bpp_x16,
> symbol_size, is_mst);
> +/**
> + * drm_dp_link_data_symbol_cycles - calculate the link symbol count
> + * @lane_coount: DP link lane count
 
Typo "lane_count"

> + * @pixels: horizontal active pixels
> + * @bpp_x16: bits per pixel in .4 binary fixed format
> + * @symbol_size: DP symbol size
> + * @is_mst: is mst or sst
> + * @slice_count: number of slices
> + *
> + * Calculate the link symbol cycles for both dsc and non dsc case and
> + * return the count.

Lets add the DP spec to be referred seems like it was missed

> + */
> +int drm_dp_link_data_symbol_cycles(int lane_count, int pixels, int bpp_x16,
> +				   int symbol_size, bool is_mst, int slice_count)
> {
> +	int slice_pixels = slice_count ? DIV_ROUND_UP(pixels, slice_count) :
> +					 pixels;
> +	int cycles = DIV_ROUND_UP(slice_pixels * bpp_x16,
> +				  (6 * symbol_size * lane_count));

Shouldn't this be 16
Also one thing I see which was there previously to while calculating is we ignore the two ceils 
Inside the function and merge it into a single div_round_up which may bring as slight variation in calculation
For example for non dsc case
Spec says
HACT_LL_SYM_CYC_CNT
= CEIL(CEIL(HACT_WIDTH / 4) × PIX_BPP / SYMBOL_SIZE)
HACT_ML_SYM_CYC_CNT
= HACT_LL_SYM_CYC_CNT × 4 / PHY_LANE_CNT

But we do
DIV_ROUND_UP(slice_pixels * bpp_x16,	  (6 * symbol_size * lane_count));

Which translates to 
CEIL( (HACT_WIDTH* BPP*4)/(16 *SYMBOL_SIZE *LANECOUNT))

Which does not seem to match the calculation exactly as what was said in the spec
Lets have an intermediate ll_symbol_cycle variable too should make the calculations
More clearer and precise according to me.

Also for dsc case lets have chunk size instead of reusing slice pixels.

Regards,
Suraj Kandpal

> +	int slice_data_cycles = ALIGN(cycles, is_mst ? (4 / lane_count) : 1);
>  	int slice_eoc_cycles = is_mst ? 4 / lane_count : 1;
> 
> -	return slice_count * (slice_data_cycles + slice_eoc_cycles);
> +	return slice_count ? (slice_count *
> +			      (slice_data_cycles + slice_eoc_cycles)) :
> +			      slice_data_cycles;
>  }
> +EXPORT_SYMBOL(drm_dp_link_data_symbol_cycles);
> 
>  /**
>   * drm_dp_bw_overhead - Calculate the BW overhead of a DP link stream @@
> -4486,15 +4493,9 @@ int drm_dp_bw_overhead(int lane_count, int hactive,
>  	WARN_ON((flags & DRM_DP_BW_OVERHEAD_UHBR) &&
>  		(flags & DRM_DP_BW_OVERHEAD_FEC));
> 
> -	if (flags & DRM_DP_BW_OVERHEAD_DSC)
> -		symbol_cycles = drm_dp_link_dsc_symbol_cycles(lane_count,
> hactive,
> -							      dsc_slice_count,
> -							      bpp_x16,
> symbol_size,
> -							      is_mst);
> -	else
> -		symbol_cycles = drm_dp_link_symbol_cycles(lane_count,
> hactive,
> -							  bpp_x16,
> symbol_size,
> -							  is_mst);
> +	symbol_cycles = drm_dp_link_data_symbol_cycles(lane_count,
> hactive,
> +						       bpp_x16, symbol_size,
> +						       is_mst, dsc_slice_count);
> 
>  	return DIV_ROUND_UP_ULL(mul_u32_u32(symbol_cycles *
> symbol_size * lane_count,
>  					    overhead * 16),
> diff --git a/include/drm/display/drm_dp_helper.h
> b/include/drm/display/drm_dp_helper.h
> index
> d9614e2c89397536f44bb7258e894628ae1dccc9..98bbbe98e5bc0ce0f9cdf513b
> 2c5ea90bb5caffb 100644
> --- a/include/drm/display/drm_dp_helper.h
> +++ b/include/drm/display/drm_dp_helper.h
> @@ -971,5 +971,7 @@ int drm_dp_bw_channel_coding_efficiency(bool
> is_uhbr);  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_data_symbol_cycles(int lane_count, int pixels, int bpp_x16,
> +				   int symbol_size, bool is_mst, int slice_count);
> 
>  #endif /* _DRM_DP_HELPER_H_ */
> 
> --
> 2.25.1

Re: [PATCH v3 1/3] drm/display/dp: Export fn to calculate link symbol cycles

[Imre Deak]

On Tue, Apr 22, 2025 at 08:03:52AM +0300, Kandpal, Suraj wrote:
> 
> 
> > -----Original Message-----
> > From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of Arun R
> > Murthy
> > Sent: Thursday, April 17, 2025 4:22 PM
> > To: dri-devel@lists.freedesktop.org; intel-gfx@lists.freedesktop.org; intel-
> > xe@lists.freedesktop.org
> > Cc: Govindapillai, Vinod <vinod.govindapillai@intel.com>; Deak, Imre
> > <imre.deak@intel.com>; Murthy, Arun R <arun.r.murthy@intel.com>
> > Subject: [PATCH v3 1/3] drm/display/dp: Export fn to calculate link symbol
> > cycles
> > 
> > Unify the function to calculate the link symbol cycles for both dsc and non-dsc
> > case and export the function so that it can be used in the respective platform
> > display drivers for other calculations.
> > 
> > v2: unify the fn for both dsc and non-dsc case (Imre)
> > v3: rename drm_dp_link_symbol_cycles to drm_dp_link_data_symbol_cycles
> >     retain slice_eoc_cycles as is (Imre)
> > 
> > Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
> > ---
> >  drivers/gpu/drm/display/drm_dp_helper.c | 53 +++++++++++++++++--------------
> > --
> >  include/drm/display/drm_dp_helper.h     |  2 ++
> >  2 files changed, 29 insertions(+), 26 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/display/drm_dp_helper.c
> > b/drivers/gpu/drm/display/drm_dp_helper.c
> > index
> > 57828f2b7b5a0582ca4a6f2a9be2d5909fe8ad24..5ce8ccc3310fb71b39ea5f74c4
> > 022474c180f727 100644
> > --- a/drivers/gpu/drm/display/drm_dp_helper.c
> > +++ b/drivers/gpu/drm/display/drm_dp_helper.c
> > @@ -4392,26 +4392,33 @@ 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 cycles = DIV_ROUND_UP(pixels * bpp_x16, 16 * symbol_size *
> > lane_count);
> > -	int align = is_mst ? 4 / lane_count : 1;
> > -
> > -	return ALIGN(cycles, align);
> > -}
> > -
> > -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 slice_pixels = DIV_ROUND_UP(pixels, slice_count);
> > -	int slice_data_cycles = drm_dp_link_symbol_cycles(lane_count,
> > slice_pixels,
> > -							  bpp_x16,
> > symbol_size, is_mst);
> > +/**
> > + * drm_dp_link_data_symbol_cycles - calculate the link symbol count
> > + * @lane_coount: DP link lane count
>  
> Typo "lane_count"
> 
> > + * @pixels: horizontal active pixels
> > + * @bpp_x16: bits per pixel in .4 binary fixed format
> > + * @symbol_size: DP symbol size
> > + * @is_mst: is mst or sst
> > + * @slice_count: number of slices
> > + *
> > + * Calculate the link symbol cycles for both dsc and non dsc case and
> > + * return the count.
> 
> Lets add the DP spec to be referred seems like it was missed
> 
> > + */
> > +int drm_dp_link_data_symbol_cycles(int lane_count, int pixels, int bpp_x16,
> > +				   int symbol_size, bool is_mst, int slice_count)
> > {
> > +	int slice_pixels = slice_count ? DIV_ROUND_UP(pixels, slice_count) :
> > +					 pixels;
> > +	int cycles = DIV_ROUND_UP(slice_pixels * bpp_x16,
> > +				  (6 * symbol_size * lane_count));
> 
> Shouldn't this be 16
> Also one thing I see which was there previously to while calculating is we ignore the two ceils 
> Inside the function and merge it into a single div_round_up which may bring as slight variation in calculation
> For example for non dsc case
> Spec says
> HACT_LL_SYM_CYC_CNT
> = CEIL(CEIL(HACT_WIDTH / 4) × PIX_BPP / SYMBOL_SIZE)
> HACT_ML_SYM_CYC_CNT
> = HACT_LL_SYM_CYC_CNT × 4 / PHY_LANE_CNT
> 
> But we do
> DIV_ROUND_UP(slice_pixels * bpp_x16,	  (6 * symbol_size * lane_count));

and we align this to 4 / lane_count. So the code does what the DP
standard describes.

> Which translates to 
> CEIL( (HACT_WIDTH* BPP*4)/(16 *SYMBOL_SIZE *LANECOUNT))
> 
> Which does not seem to match the calculation exactly as what was said in the spec
> Lets have an intermediate ll_symbol_cycle variable too should make the calculations
> More clearer and precise according to me.
>
> Also for dsc case lets have chunk size instead of reusing slice pixels.

Let's not add now other changes besides the original request of one
thing, that is to make drm_dp_link_symbol_cycles() handle both the
DSC and non-DSC cases without changing the calculation, see [1]. That
corresponds to the diff I added in [2].

[1] https://lore.kernel.org/all/Z_UvdB05S0sPbs6l@ideak-desk.fi.intel.com
[2] https://lore.kernel.org/all/aAdiU3K5EV6Oq81a@ideak-desk.fi.intel.com

> Regards,
> Suraj Kandpal
> 
> > +	int slice_data_cycles = ALIGN(cycles, is_mst ? (4 / lane_count) : 1);
> >  	int slice_eoc_cycles = is_mst ? 4 / lane_count : 1;
> > 
> > -	return slice_count * (slice_data_cycles + slice_eoc_cycles);
> > +	return slice_count ? (slice_count *
> > +			      (slice_data_cycles + slice_eoc_cycles)) :
> > +			      slice_data_cycles;
> >  }
> > +EXPORT_SYMBOL(drm_dp_link_data_symbol_cycles);
> > 
> >  /**
> >   * drm_dp_bw_overhead - Calculate the BW overhead of a DP link stream @@
> > -4486,15 +4493,9 @@ int drm_dp_bw_overhead(int lane_count, int hactive,
> >  	WARN_ON((flags & DRM_DP_BW_OVERHEAD_UHBR) &&
> >  		(flags & DRM_DP_BW_OVERHEAD_FEC));
> > 
> > -	if (flags & DRM_DP_BW_OVERHEAD_DSC)
> > -		symbol_cycles = drm_dp_link_dsc_symbol_cycles(lane_count,
> > hactive,
> > -							      dsc_slice_count,
> > -							      bpp_x16,
> > symbol_size,
> > -							      is_mst);
> > -	else
> > -		symbol_cycles = drm_dp_link_symbol_cycles(lane_count,
> > hactive,
> > -							  bpp_x16,
> > symbol_size,
> > -							  is_mst);
> > +	symbol_cycles = drm_dp_link_data_symbol_cycles(lane_count,
> > hactive,
> > +						       bpp_x16, symbol_size,
> > +						       is_mst, dsc_slice_count);
> > 
> >  	return DIV_ROUND_UP_ULL(mul_u32_u32(symbol_cycles *
> > symbol_size * lane_count,
> >  					    overhead * 16),
> > diff --git a/include/drm/display/drm_dp_helper.h
> > b/include/drm/display/drm_dp_helper.h
> > index
> > d9614e2c89397536f44bb7258e894628ae1dccc9..98bbbe98e5bc0ce0f9cdf513b
> > 2c5ea90bb5caffb 100644
> > --- a/include/drm/display/drm_dp_helper.h
> > +++ b/include/drm/display/drm_dp_helper.h
> > @@ -971,5 +971,7 @@ int drm_dp_bw_channel_coding_efficiency(bool
> > is_uhbr);  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_data_symbol_cycles(int lane_count, int pixels, int bpp_x16,
> > +				   int symbol_size, bool is_mst, int slice_count);
> > 
> >  #endif /* _DRM_DP_HELPER_H_ */
> > 
> > --
> > 2.25.1
> 

Re: [PATCH v3 1/3] drm/display/dp: Export fn to calculate link symbol cycles

[Imre Deak]

On Thu, Apr 17, 2025 at 04:22:28PM +0530, Arun R Murthy wrote:
> Unify the function to calculate the link symbol cycles for both dsc and
> non-dsc case and export the function so that it can be used in the
> respective platform display drivers for other calculations.
> 
> v2: unify the fn for both dsc and non-dsc case (Imre)
> v3: rename drm_dp_link_symbol_cycles to drm_dp_link_data_symbol_cycles
>     retain slice_eoc_cycles as is (Imre)
> 
> Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
> ---
>  drivers/gpu/drm/display/drm_dp_helper.c | 53 +++++++++++++++++----------------
>  include/drm/display/drm_dp_helper.h     |  2 ++
>  2 files changed, 29 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/gpu/drm/display/drm_dp_helper.c b/drivers/gpu/drm/display/drm_dp_helper.c
> index 57828f2b7b5a0582ca4a6f2a9be2d5909fe8ad24..5ce8ccc3310fb71b39ea5f74c4022474c180f727 100644
> --- a/drivers/gpu/drm/display/drm_dp_helper.c
> +++ b/drivers/gpu/drm/display/drm_dp_helper.c
> @@ -4392,26 +4392,33 @@ 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 cycles = DIV_ROUND_UP(pixels * bpp_x16, 16 * symbol_size * lane_count);
> -	int align = is_mst ? 4 / lane_count : 1;
> -
> -	return ALIGN(cycles, align);
> -}
> -
> -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 slice_pixels = DIV_ROUND_UP(pixels, slice_count);
> -	int slice_data_cycles = drm_dp_link_symbol_cycles(lane_count, slice_pixels,
> -							  bpp_x16, symbol_size, is_mst);
> +/**
> + * drm_dp_link_data_symbol_cycles - calculate the link symbol count
> + * @lane_coount: DP link lane count
> + * @pixels: horizontal active pixels
> + * @bpp_x16: bits per pixel in .4 binary fixed format
> + * @symbol_size: DP symbol size
> + * @is_mst: is mst or sst
> + * @slice_count: number of slices
> + *
> + * Calculate the link symbol cycles for both dsc and non dsc case and
> + * return the count.
> + */
> +int drm_dp_link_data_symbol_cycles(int lane_count, int pixels, int bpp_x16,
> +				   int symbol_size, bool is_mst, int slice_count)
> +{
> +	int slice_pixels = slice_count ? DIV_ROUND_UP(pixels, slice_count) :
> +					 pixels;
> +	int cycles = DIV_ROUND_UP(slice_pixels * bpp_x16,
> +				  (6 * symbol_size * lane_count));
> +	int slice_data_cycles = ALIGN(cycles, is_mst ? (4 / lane_count) : 1);
>  	int slice_eoc_cycles = is_mst ? 4 / lane_count : 1;
>  
> -	return slice_count * (slice_data_cycles + slice_eoc_cycles);
> +	return slice_count ? (slice_count *
> +			      (slice_data_cycles + slice_eoc_cycles)) :
> +			      slice_data_cycles;
>  }

This is not what I meant. Please keep the two functions separate, don't merge
them and don't introduce changes unrelated to what was requested. The following
is needed here:

--- a/drivers/gpu/drm/display/drm_dp_helper.c
+++ b/drivers/gpu/drm/display/drm_dp_helper.c
@@ -4393,8 +4393,8 @@ 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)
+static int drm_dp_link_data_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;
@@ -4402,13 +4402,17 @@ static int drm_dp_link_symbol_cycles(int lane_count, int pixels, int bpp_x16,
        return ALIGN(cycles, align);
 }
 
-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_symbol_cycles(int lane_count, int pixels, int dsc_slice_count,
+                             int bpp_x16, int symbol_size, bool is_mst)
 {
+       int slice_count = dsc_slice_count ? : 1;
        int slice_pixels = DIV_ROUND_UP(pixels, slice_count);
-       int slice_data_cycles = drm_dp_link_symbol_cycles(lane_count, slice_pixels,
-                                                         bpp_x16, symbol_size, is_mst);
-       int slice_eoc_cycles = is_mst ? 4 / lane_count : 1;
+       int slice_data_cycles = drm_dp_link_data_symbol_cycles(lane_count, slice_pixels,
+                                                              bpp_x16, symbol_size, is_mst);
+       int slice_eoc_cycles = 0;
+
+       if (dsc_slice_count)
+               slice_eoc_cycles = is_mst ? 4 / lane_count : 1;
 
        return slice_count * (slice_data_cycles + slice_eoc_cycles);
 }

> +EXPORT_SYMBOL(drm_dp_link_data_symbol_cycles);
>  
>  /**
>   * drm_dp_bw_overhead - Calculate the BW overhead of a DP link stream
> @@ -4486,15 +4493,9 @@ int drm_dp_bw_overhead(int lane_count, int hactive,
>  	WARN_ON((flags & DRM_DP_BW_OVERHEAD_UHBR) &&
>  		(flags & DRM_DP_BW_OVERHEAD_FEC));
>  
> -	if (flags & DRM_DP_BW_OVERHEAD_DSC)
> -		symbol_cycles = drm_dp_link_dsc_symbol_cycles(lane_count, hactive,
> -							      dsc_slice_count,
> -							      bpp_x16, symbol_size,
> -							      is_mst);
> -	else
> -		symbol_cycles = drm_dp_link_symbol_cycles(lane_count, hactive,
> -							  bpp_x16, symbol_size,
> -							  is_mst);
> +	symbol_cycles = drm_dp_link_data_symbol_cycles(lane_count, hactive,
> +						       bpp_x16, symbol_size,
> +						       is_mst, dsc_slice_count);
>  
>  	return DIV_ROUND_UP_ULL(mul_u32_u32(symbol_cycles * symbol_size * lane_count,
>  					    overhead * 16),
> diff --git a/include/drm/display/drm_dp_helper.h b/include/drm/display/drm_dp_helper.h
> index d9614e2c89397536f44bb7258e894628ae1dccc9..98bbbe98e5bc0ce0f9cdf513b2c5ea90bb5caffb 100644
> --- a/include/drm/display/drm_dp_helper.h
> +++ b/include/drm/display/drm_dp_helper.h
> @@ -971,5 +971,7 @@ int drm_dp_bw_channel_coding_efficiency(bool is_uhbr);
>  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_data_symbol_cycles(int lane_count, int pixels, int bpp_x16,
> +				   int symbol_size, bool is_mst, int slice_count);
>  
>  #endif /* _DRM_DP_HELPER_H_ */
> 
> -- 
> 2.25.1
> 

[PATCH v3 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 v3 3/3] drm/i915/display: move min_hblank from dp_mst.c to dp.c

[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.

TODO: Add the min_hblank calculation for hdmi as well.

v2: move from intel_audio.c to intel_dp.c
    some correction in link_bpp_x16 (Imre)
v3: min_hblank for 8b/10b MST and 128b/132b SST/MST
    handle error for intel_dp_mst_dsc_get_slice_count
    reset min_hblank before disabling transcoder (Imre)

Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
---
 drivers/gpu/drm/i915/display/intel_display.c | 18 ++++++++
 drivers/gpu/drm/i915/display/intel_dp.c      | 64 ++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/display/intel_dp.h      |  4 ++
 drivers/gpu/drm/i915/display/intel_dp_mst.c  | 52 +---------------------
 4 files changed, 88 insertions(+), 50 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index dc7517da2ed5c75fb1715d43e6bfc792a8420f30..ad41382981299dffab5cc592b43cdab7eebb7b5b 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -1782,8 +1782,11 @@ static void hsw_crtc_disable(struct intel_atomic_state *state,
 	const struct intel_crtc_state *old_crtc_state =
 		intel_atomic_get_old_crtc_state(state, crtc);
 	struct intel_crtc *pipe_crtc;
+	enum transcoder cpu_transcoder = old_crtc_state->cpu_transcoder;
 	int i;
 
+	if (DISPLAY_VER(display) >= 30)
+		intel_de_write(display, DP_MIN_HBLANK_CTL(cpu_transcoder), 0);
 	/*
 	 * FIXME collapse everything to one hook.
 	 * Need care with mst->ddi interactions.
@@ -2726,6 +2729,19 @@ static void intel_set_transcoder_timings(const struct intel_crtc_state *crtc_sta
 		intel_de_write(display, TRANS_VTOTAL(display, pipe),
 			       VACTIVE(crtc_vdisplay - 1) |
 			       VTOTAL(crtc_vtotal - 1));
+
+	if (DISPLAY_VER(display) >= 30 && crtc_state->min_hblank) {
+		/*
+		 * 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);
+	}
 }
 
 static void intel_set_transcoder_timings_lrr(const struct intel_crtc_state *crtc_state)
@@ -5221,6 +5237,8 @@ intel_pipe_config_compare(const struct intel_crtc_state *current_config,
 	PIPE_CONF_CHECK_I(lane_count);
 	PIPE_CONF_CHECK_X(lane_lat_optim_mask);
 
+	PIPE_CONF_CHECK_I(min_hblank);
+
 	if (HAS_DOUBLE_BUFFERED_M_N(display)) {
 		if (!fastset || !pipe_config->update_m_n)
 			PIPE_CONF_CHECK_M_N(dp_m_n);
diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
index 7b95d62730e6f0c27c07fb59f16476369c4762a4..7b114b7cc86cc78088ca27e2cb7b08b8491d8283 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -3103,6 +3103,67 @@ intel_dp_queue_modeset_retry_for_link(struct intel_atomic_state *state,
 	}
 }
 
+void intel_dp_compute_min_hblank(int link_bpp_x16,
+				 struct intel_crtc_state *crtc_state,
+				 struct drm_connector_state *conn_state,
+				 bool is_dsc)
+{
+	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;
+	/*
+	 * min symbol cycles is 3(BS,VBID, BE) for 128b/132b and
+	 * 5(BS, VBID, MVID, MAUD) for 8b/10b
+	 */
+	int min_sym_cycles = intel_dp_is_uhbr(crtc_state) ? 3 : 5;
+	bool is_mst = intel_crtc_has_type(crtc_state, INTEL_OUTPUT_DP_MST);
+	int min_hblank;
+	int max_lane_count = 4;
+	int hactive_sym_cycles, htotal_sym_cycles;
+	int dsc_slices = 0;
+
+	if (DISPLAY_VER(display) < 30)
+		return;
+
+	/* MIN_HBLANK should be set only for 8b/10b MST or for 128b/132b SST/MST */
+	if (intel_dp_is_uhbr(crtc_state) || (is_mst && !intel_dp_is_uhbr(crtc_state)))
+		return;
+
+	if (is_dsc) {
+		dsc_slices = intel_dp_mst_dsc_get_slice_count(connector,
+							      crtc_state);
+		if (!dsc_slices) {
+			drm_dbg(display->drm, "failed to calculate dsc slice count\n");
+			return;
+		}
+	}
+
+	/* Calculate min Hblank Link Layer Symbol Cycle Count for 8b/10b MST & 128b/132b */
+	hactive_sym_cycles = drm_dp_link_data_symbol_cycles(max_lane_count,
+							    adjusted_mode->hdisplay,
+							    link_bpp_x16,
+							    symbol_size,
+							    is_mst,
+							    dsc_slices);
+	htotal_sym_cycles = adjusted_mode->htotal *
+			    (hactive_sym_cycles / adjusted_mode->hdisplay);
+
+	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;
+
+	min_hblank = min(10, min_hblank);
+	crtc_state->min_hblank = min_hblank;
+}
+
 int
 intel_dp_compute_config(struct intel_encoder *encoder,
 			struct intel_crtc_state *pipe_config,
@@ -3202,6 +3263,9 @@ intel_dp_compute_config(struct intel_encoder *encoder,
 				       &pipe_config->dp_m_n);
 	}
 
+	intel_dp_compute_min_hblank(link_bpp_x16, pipe_config, conn_state,
+				    pipe_config->dsc.compression_enable);
+
 	/* FIXME: abstract this better */
 	if (pipe_config->splitter.enable)
 		pipe_config->dp_m_n.data_m *= pipe_config->splitter.link_count;
diff --git a/drivers/gpu/drm/i915/display/intel_dp.h b/drivers/gpu/drm/i915/display/intel_dp.h
index 9189db4c25946a0f082223ce059c242e80cc32dc..43624aead998a8a330a244bb9c85f026e203171b 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.h
+++ b/drivers/gpu/drm/i915/display/intel_dp.h
@@ -208,5 +208,9 @@ bool intel_dp_has_connector(struct intel_dp *intel_dp,
 			    const struct drm_connector_state *conn_state);
 int intel_dp_dsc_max_src_input_bpc(struct intel_display *display);
 int intel_dp_dsc_min_src_input_bpc(void);
+void intel_dp_compute_min_hblank(int link_bpp_x16,
+				 struct intel_crtc_state *crtc_state,
+				 struct drm_connector_state *conn_state,
+				 bool is_dsc);
 
 #endif /* __INTEL_DP_H__ */
diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
index af98a0d0e8376a79ce1ab6ff3c4f6af30f4d3e73..4153afa13c618bb4db6dbcdc6e59faddcbeade6b 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,12 +281,11 @@ 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,
 					 &crtc_state->dp_m_n);
+		intel_dp_compute_min_hblank(link_bpp_x16, crtc_state, conn_state, dsc);
 
 		if (is_mst) {
 			int remote_bw_overhead;
@@ -998,7 +977,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 +987,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 +1261,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 +1276,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

Re: [PATCH v3 3/3] drm/i915/display: move min_hblank from dp_mst.c to dp.c

[Govindapillai, Vinod]

Hi Arun

I was trying to use this series for the underrun issue! But this patch didnt apply cleanly!
And had some compilation issues because of an unused variable!
Also I did not get expected traces on min hblank! Not sure if there was any issue on my conflicts
resolution to get this applied on my branch. 

Anyway pls have a look at few comments..

On Thu, 2025-04-17 at 16:22 +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.
> 
> TODO: Add the min_hblank calculation for hdmi as well.
> 
> v2: move from intel_audio.c to intel_dp.c
>     some correction in link_bpp_x16 (Imre)
> v3: min_hblank for 8b/10b MST and 128b/132b SST/MST
>     handle error for intel_dp_mst_dsc_get_slice_count
>     reset min_hblank before disabling transcoder (Imre)
> 
> Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display.c | 18 ++++++++
>  drivers/gpu/drm/i915/display/intel_dp.c      | 64 ++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/display/intel_dp.h      |  4 ++
>  drivers/gpu/drm/i915/display/intel_dp_mst.c  | 52 +---------------------
>  4 files changed, 88 insertions(+), 50 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> b/drivers/gpu/drm/i915/display/intel_display.c
> index dc7517da2ed5c75fb1715d43e6bfc792a8420f30..ad41382981299dffab5cc592b43cdab7eebb7b5b 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -1782,8 +1782,11 @@ static void hsw_crtc_disable(struct intel_atomic_state *state,
>  	const struct intel_crtc_state *old_crtc_state =
>  		intel_atomic_get_old_crtc_state(state, crtc);
>  	struct intel_crtc *pipe_crtc;
> +	enum transcoder cpu_transcoder = old_crtc_state->cpu_transcoder;
>  	int i;
>  
> +	if (DISPLAY_VER(display) >= 30)
> +		intel_de_write(display, DP_MIN_HBLANK_CTL(cpu_transcoder), 0);

Please have a look at comments from Imre for the last revision.
>  	/*
>  	 * FIXME collapse everything to one hook.
>  	 * Need care with mst->ddi interactions.
> @@ -2726,6 +2729,19 @@ static void intel_set_transcoder_timings(const struct intel_crtc_state
> *crtc_sta
>  		intel_de_write(display, TRANS_VTOTAL(display, pipe),
>  			       VACTIVE(crtc_vdisplay - 1) |
>  			       VTOTAL(crtc_vtotal - 1));
> +
> +	if (DISPLAY_VER(display) >= 30 && crtc_state->min_hblank) {

Again pls have a look at Imre's comments in the prev version.

Update the crtc_state->min_hblank calculations so that it can have 0 for non supported cases and
have a non zero for 8b/10b MST and 128b/132b and program here.


> +		/*
> +		 * 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);
> +	}

>  }
>  
>  static void intel_set_transcoder_timings_lrr(const struct intel_crtc_state *crtc_state)
> @@ -5221,6 +5237,8 @@ intel_pipe_config_compare(const struct intel_crtc_state *current_config,
>  	PIPE_CONF_CHECK_I(lane_count);
>  	PIPE_CONF_CHECK_X(lane_lat_optim_mask);
>  
> +	PIPE_CONF_CHECK_I(min_hblank);
> +
>  	if (HAS_DOUBLE_BUFFERED_M_N(display)) {
>  		if (!fastset || !pipe_config->update_m_n)
>  			PIPE_CONF_CHECK_M_N(dp_m_n);
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> index 7b95d62730e6f0c27c07fb59f16476369c4762a4..7b114b7cc86cc78088ca27e2cb7b08b8491d8283 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -3103,6 +3103,67 @@ intel_dp_queue_modeset_retry_for_link(struct intel_atomic_state *state,
>  	}
>  }
>  
> +void intel_dp_compute_min_hblank(int link_bpp_x16,
> +				 struct intel_crtc_state *crtc_state,
> +				 struct drm_connector_state *conn_state,
> +				 bool is_dsc)
> +{
> +	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;
> +	/*
> +	 * min symbol cycles is 3(BS,VBID, BE) for 128b/132b and
> +	 * 5(BS, VBID, MVID, MAUD) for 8b/10b
> +	 */
> +	int min_sym_cycles = intel_dp_is_uhbr(crtc_state) ? 3 : 5;
> +	bool is_mst = intel_crtc_has_type(crtc_state, INTEL_OUTPUT_DP_MST);
> +	int min_hblank;
> +	int max_lane_count = 4;
> +	int hactive_sym_cycles, htotal_sym_cycles;
> +	int dsc_slices = 0;
> +
> +	if (DISPLAY_VER(display) < 30)
> +		return;
> +
> +	/* MIN_HBLANK should be set only for 8b/10b MST or for 128b/132b SST/MST */
> +	if (intel_dp_is_uhbr(crtc_state) || (is_mst && !intel_dp_is_uhbr(crtc_state)))
> +		return;
> +
> +	if (is_dsc) {
> +		dsc_slices = intel_dp_mst_dsc_get_slice_count(connector,
> +							      crtc_state);
> +		if (!dsc_slices) {
> +			drm_dbg(display->drm, "failed to calculate dsc slice count\n");
> +			return;
> +		}
> +	}
> +
> +	/* Calculate min Hblank Link Layer Symbol Cycle Count for 8b/10b MST & 128b/132b */
> +	hactive_sym_cycles = drm_dp_link_data_symbol_cycles(max_lane_count,
> +							    adjusted_mode->hdisplay,
> +							    link_bpp_x16,
> +							    symbol_size,
> +							    is_mst,
> +							    dsc_slices);
> +	htotal_sym_cycles = adjusted_mode->htotal *
> +			    (hactive_sym_cycles / adjusted_mode->hdisplay);
> +
> +	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;
> +
> +	min_hblank = min(10, min_hblank);
> +	crtc_state->min_hblank = min_hblank;
> +}
> +
>  int
>  intel_dp_compute_config(struct intel_encoder *encoder,
>  			struct intel_crtc_state *pipe_config,
> @@ -3202,6 +3263,9 @@ intel_dp_compute_config(struct intel_encoder *encoder,
>  				       &pipe_config->dp_m_n);
>  	}
>  
> +	intel_dp_compute_min_hblank(link_bpp_x16, pipe_config, conn_state,
> +				    pipe_config->dsc.compression_enable);
> +
>  	/* FIXME: abstract this better */
>  	if (pipe_config->splitter.enable)
>  		pipe_config->dp_m_n.data_m *= pipe_config->splitter.link_count;
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.h b/drivers/gpu/drm/i915/display/intel_dp.h
> index 9189db4c25946a0f082223ce059c242e80cc32dc..43624aead998a8a330a244bb9c85f026e203171b 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.h
> +++ b/drivers/gpu/drm/i915/display/intel_dp.h
> @@ -208,5 +208,9 @@ bool intel_dp_has_connector(struct intel_dp *intel_dp,
>  			    const struct drm_connector_state *conn_state);
>  int intel_dp_dsc_max_src_input_bpc(struct intel_display *display);
>  int intel_dp_dsc_min_src_input_bpc(void);
> +void intel_dp_compute_min_hblank(int link_bpp_x16,
> +				 struct intel_crtc_state *crtc_state,
> +				 struct drm_connector_state *conn_state,
> +				 bool is_dsc);
>  
>  #endif /* __INTEL_DP_H__ */
> diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> index af98a0d0e8376a79ce1ab6ff3c4f6af30f4d3e73..4153afa13c618bb4db6dbcdc6e59faddcbeade6b 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,12 +281,11 @@ 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,
>  					 &crtc_state->dp_m_n);
> +		intel_dp_compute_min_hblank(link_bpp_x16, crtc_state, conn_state, dsc);

Also I wanted to check if this is the right place to call intel_dp_compute_min_hblank()? Can't this
be called once we settle a link_bpp. May be from mst_stream_compute_config() before calling
intel_dp_audio_compute_config(). @Imre pls comment.

>  
>  		if (is_mst) {
>  			int remote_bw_overhead;
> @@ -998,7 +977,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 +987,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);

After removing this, "display" is not used anymore in this function. So need to get rid of unused
local variable display as well.

BR
Vinod

>  }
>  
>  static void mst_stream_post_disable(struct intel_atomic_state *state,
> @@ -1286,7 +1261,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 +1276,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);
> 

RE: [PATCH v3 3/3] drm/i915/display: move min_hblank from dp_mst.c to dp.c

[Murthy, Arun R]

> -----Original Message-----
> From: Govindapillai, Vinod <vinod.govindapillai@intel.com>
> Sent: Tuesday, April 22, 2025 4:17 AM
> To: Murthy, Arun R <arun.r.murthy@intel.com>; dri-
> devel@lists.freedesktop.org; intel-xe@lists.freedesktop.org; intel-
> gfx@lists.freedesktop.org
> Cc: Deak, Imre <imre.deak@intel.com>
> Subject: Re: [PATCH v3 3/3] drm/i915/display: move min_hblank from dp_mst.c
> to dp.c
> 
> Hi Arun
> 
> I was trying to use this series for the underrun issue! But this patch didnt apply
> cleanly!
> And had some compilation issues because of an unused variable!
Not sure, even CI is claiming so, but the said unused variable is used in the function for dbg print.

> Also I did not get expected traces on min hblank! Not sure if there was any
No, there are no traces/dbg prints added for this.

> issue on my conflicts resolution to get this applied on my branch.
> 
> Anyway pls have a look at few comments..
> 
> On Thu, 2025-04-17 at 16:22 +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.
> >
> > TODO: Add the min_hblank calculation for hdmi as well.
> >
> > v2: move from intel_audio.c to intel_dp.c
> >     some correction in link_bpp_x16 (Imre)
> > v3: min_hblank for 8b/10b MST and 128b/132b SST/MST
> >     handle error for intel_dp_mst_dsc_get_slice_count
> >     reset min_hblank before disabling transcoder (Imre)
> >
> > Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_display.c | 18 ++++++++
> >  drivers/gpu/drm/i915/display/intel_dp.c      | 64
> > ++++++++++++++++++++++++++++
> >  drivers/gpu/drm/i915/display/intel_dp.h      |  4 ++
> >  drivers/gpu/drm/i915/display/intel_dp_mst.c  | 52
> > +---------------------
> >  4 files changed, 88 insertions(+), 50 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > b/drivers/gpu/drm/i915/display/intel_display.c
> > index
> >
> dc7517da2ed5c75fb1715d43e6bfc792a8420f30..ad41382981299dffab5cc592b
> 43c
> > dab7eebb7b5b 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -1782,8 +1782,11 @@ static void hsw_crtc_disable(struct
> intel_atomic_state *state,
> >  	const struct intel_crtc_state *old_crtc_state =
> >  		intel_atomic_get_old_crtc_state(state, crtc);
> >  	struct intel_crtc *pipe_crtc;
> > +	enum transcoder cpu_transcoder = old_crtc_state->cpu_transcoder;
> >  	int i;
> >
> > +	if (DISPLAY_VER(display) >= 30)
> > +		intel_de_write(display, DP_MIN_HBLANK_CTL(cpu_transcoder),
> 0);
> 
> Please have a look at comments from Imre for the last revision.
In my earlier version min_hblank was being disabled after transcoder disable, here the min_hblank is being disabled before the transcoder is being disabled.

> >  	/*
> >  	 * FIXME collapse everything to one hook.
> >  	 * Need care with mst->ddi interactions.
> > @@ -2726,6 +2729,19 @@ static void intel_set_transcoder_timings(const
> > struct intel_crtc_state *crtc_sta
> >  		intel_de_write(display, TRANS_VTOTAL(display, pipe),
> >  			       VACTIVE(crtc_vdisplay - 1) |
> >  			       VTOTAL(crtc_vtotal - 1));
> > +
> > +	if (DISPLAY_VER(display) >= 30 && crtc_state->min_hblank) {
> 
> Again pls have a look at Imre's comments in the prev version.
> 
> Update the crtc_state->min_hblank calculations so that it can have 0 for non
> supported cases and have a non zero for 8b/10b MST and 128b/132b and
> program here.
No non-supported cases we don’t have the value calculated and we don’t have to write to this register for 8b/10b SST case. In supported cases 8b/10b MST and 128b/132b the value cannot be 0, if its 0 then it means the feature is disabled.

> 
> 
> > +		/*
> > +		 * 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);
> > +	}
> 
> >  }
> >
> >  static void intel_set_transcoder_timings_lrr(const struct
> > intel_crtc_state *crtc_state) @@ -5221,6 +5237,8 @@
> intel_pipe_config_compare(const struct intel_crtc_state *current_config,
> >  	PIPE_CONF_CHECK_I(lane_count);
> >  	PIPE_CONF_CHECK_X(lane_lat_optim_mask);
> >
> > +	PIPE_CONF_CHECK_I(min_hblank);
> > +
> >  	if (HAS_DOUBLE_BUFFERED_M_N(display)) {
> >  		if (!fastset || !pipe_config->update_m_n)
> >  			PIPE_CONF_CHECK_M_N(dp_m_n);
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> > b/drivers/gpu/drm/i915/display/intel_dp.c
> > index
> >
> 7b95d62730e6f0c27c07fb59f16476369c4762a4..7b114b7cc86cc78088ca27e2c
> b7b
> > 08b8491d8283 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > @@ -3103,6 +3103,67 @@ intel_dp_queue_modeset_retry_for_link(struct
> intel_atomic_state *state,
> >  	}
> >  }
> >
> > +void intel_dp_compute_min_hblank(int link_bpp_x16,
> > +				 struct intel_crtc_state *crtc_state,
> > +				 struct drm_connector_state *conn_state,
> > +				 bool is_dsc)
> > +{
> > +	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;
> > +	/*
> > +	 * min symbol cycles is 3(BS,VBID, BE) for 128b/132b and
> > +	 * 5(BS, VBID, MVID, MAUD) for 8b/10b
> > +	 */
> > +	int min_sym_cycles = intel_dp_is_uhbr(crtc_state) ? 3 : 5;
> > +	bool is_mst = intel_crtc_has_type(crtc_state, INTEL_OUTPUT_DP_MST);
> > +	int min_hblank;
> > +	int max_lane_count = 4;
> > +	int hactive_sym_cycles, htotal_sym_cycles;
> > +	int dsc_slices = 0;
> > +
> > +	if (DISPLAY_VER(display) < 30)
> > +		return;
> > +
> > +	/* MIN_HBLANK should be set only for 8b/10b MST or for 128b/132b
> SST/MST */
> > +	if (intel_dp_is_uhbr(crtc_state) || (is_mst &&
> !intel_dp_is_uhbr(crtc_state)))
> > +		return;
> > +
> > +	if (is_dsc) {
> > +		dsc_slices = intel_dp_mst_dsc_get_slice_count(connector,
> > +							      crtc_state);
> > +		if (!dsc_slices) {
> > +			drm_dbg(display->drm, "failed to calculate dsc slice
> count\n");
> > +			return;
> > +		}
> > +	}
> > +
> > +	/* Calculate min Hblank Link Layer Symbol Cycle Count for 8b/10b MST
> & 128b/132b */
> > +	hactive_sym_cycles =
> drm_dp_link_data_symbol_cycles(max_lane_count,
> > +							    adjusted_mode-
> >hdisplay,
> > +							    link_bpp_x16,
> > +							    symbol_size,
> > +							    is_mst,
> > +							    dsc_slices);
> > +	htotal_sym_cycles = adjusted_mode->htotal *
> > +			    (hactive_sym_cycles / adjusted_mode->hdisplay);
> > +
> > +	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;
> > +
> > +	min_hblank = min(10, min_hblank);
> > +	crtc_state->min_hblank = min_hblank; }
> > +
> >  int
> >  intel_dp_compute_config(struct intel_encoder *encoder,
> >  			struct intel_crtc_state *pipe_config, @@ -3202,6
> +3263,9 @@
> > intel_dp_compute_config(struct intel_encoder *encoder,
> >  				       &pipe_config->dp_m_n);
> >  	}
> >
> > +	intel_dp_compute_min_hblank(link_bpp_x16, pipe_config, conn_state,
> > +				    pipe_config->dsc.compression_enable);
> > +
> >  	/* FIXME: abstract this better */
> >  	if (pipe_config->splitter.enable)
> >  		pipe_config->dp_m_n.data_m *= pipe_config-
> >splitter.link_count;
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp.h
> > b/drivers/gpu/drm/i915/display/intel_dp.h
> > index
> >
> 9189db4c25946a0f082223ce059c242e80cc32dc..43624aead998a8a330a244b
> b9c85
> > f026e203171b 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp.h
> > +++ b/drivers/gpu/drm/i915/display/intel_dp.h
> > @@ -208,5 +208,9 @@ bool intel_dp_has_connector(struct intel_dp
> *intel_dp,
> >  			    const struct drm_connector_state *conn_state);
> >  int intel_dp_dsc_max_src_input_bpc(struct intel_display *display);
> >  int intel_dp_dsc_min_src_input_bpc(void);
> > +void intel_dp_compute_min_hblank(int link_bpp_x16,
> > +				 struct intel_crtc_state *crtc_state,
> > +				 struct drm_connector_state *conn_state,
> > +				 bool is_dsc);
> >
> >  #endif /* __INTEL_DP_H__ */
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > index
> >
> af98a0d0e8376a79ce1ab6ff3c4f6af30f4d3e73..4153afa13c618bb4db6dbcdc6e
> 59
> > faddcbeade6b 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,12 +281,11 @@
> > 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,
> >  					 &crtc_state->dp_m_n);
> > +		intel_dp_compute_min_hblank(link_bpp_x16, crtc_state,
> conn_state,
> > +dsc);
> 
> Also I wanted to check if this is the right place to call
> intel_dp_compute_min_hblank()? Can't this be called once we settle a link_bpp.
> May be from mst_stream_compute_config() before calling
> intel_dp_audio_compute_config(). @Imre pls comment.
> 
Here the calculation is done as part of the m and n calculation and this function is called from mst_stream_compute_config and is done before dp_audio_compute_config().
Let me know if any change required will do.

> >
> >  		if (is_mst) {
> >  			int remote_bw_overhead;
> > @@ -998,7 +977,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 +987,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);
> 
> After removing this, "display" is not used anymore in this function. So need to
> get rid of unused local variable display as well.
> 
There is a drm_dbg_kms which used the struct display. Let me try rebasing with the latest drm-tip.

Thanks and Regards,
Arun R Murthy
--------------------
> BR
> Vinod
> 
> >  }
> >
> >  static void mst_stream_post_disable(struct intel_atomic_state *state,
> > @@ -1286,7 +1261,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 +1276,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);
> >

Re: [PATCH v3 3/3] drm/i915/display: move min_hblank from dp_mst.c to dp.c

[Govindapillai, Vinod]

On Tue, 2025-04-22 at 04:49 +0000, Murthy, Arun R wrote:
> 
> > -----Original Message-----
> > From: Govindapillai, Vinod <vinod.govindapillai@intel.com>
> > Sent: Tuesday, April 22, 2025 4:17 AM
> > To: Murthy, Arun R <arun.r.murthy@intel.com>; dri-
> > devel@lists.freedesktop.org; intel-xe@lists.freedesktop.org; intel-
> > gfx@lists.freedesktop.org
> > Cc: Deak, Imre <imre.deak@intel.com>
> > Subject: Re: [PATCH v3 3/3] drm/i915/display: move min_hblank from dp_mst.c
> > to dp.c
> > 
> > Hi Arun
> > 
> > I was trying to use this series for the underrun issue! But this patch didnt apply
> > cleanly!
> > And had some compilation issues because of an unused variable!
> Not sure, even CI is claiming so, but the said unused variable is used in the function for dbg
> print.

May be you need to rebase?

Also how do we validate this? And how can we know that we are programming the write values?

> 
> > Also I did not get expected traces on min hblank! Not sure if there was any
> No, there are no traces/dbg prints added for this.

I added some traces to see what is being calculated for min_hblank

> 
> > issue on my conflicts resolution to get this applied on my branch.
> > 
> > Anyway pls have a look at few comments..
> > 
> > On Thu, 2025-04-17 at 16:22 +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.
> > > 
> > > TODO: Add the min_hblank calculation for hdmi as well.
> > > 
> > > v2: move from intel_audio.c to intel_dp.c
> > >     some correction in link_bpp_x16 (Imre)
> > > v3: min_hblank for 8b/10b MST and 128b/132b SST/MST
> > >     handle error for intel_dp_mst_dsc_get_slice_count
> > >     reset min_hblank before disabling transcoder (Imre)
> > > 
> > > Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_display.c | 18 ++++++++
> > >  drivers/gpu/drm/i915/display/intel_dp.c      | 64
> > > ++++++++++++++++++++++++++++
> > >  drivers/gpu/drm/i915/display/intel_dp.h      |  4 ++
> > >  drivers/gpu/drm/i915/display/intel_dp_mst.c  | 52
> > > +---------------------
> > >  4 files changed, 88 insertions(+), 50 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > > b/drivers/gpu/drm/i915/display/intel_display.c
> > > index
> > > 
> > dc7517da2ed5c75fb1715d43e6bfc792a8420f30..ad41382981299dffab5cc592b
> > 43c
> > > dab7eebb7b5b 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > @@ -1782,8 +1782,11 @@ static void hsw_crtc_disable(struct
> > intel_atomic_state *state,
> > >  	const struct intel_crtc_state *old_crtc_state =
> > >  		intel_atomic_get_old_crtc_state(state, crtc);
> > >  	struct intel_crtc *pipe_crtc;
> > > +	enum transcoder cpu_transcoder = old_crtc_state->cpu_transcoder;
> > >  	int i;
> > > 
> > > +	if (DISPLAY_VER(display) >= 30)
> > > +		intel_de_write(display, DP_MIN_HBLANK_CTL(cpu_transcoder),
> > 0);
> > 
> > Please have a look at comments from Imre for the last revision.
> In my earlier version min_hblank was being disabled after transcoder disable, here the min_hblank
> is being disabled before the transcoder is being disabled.
> 
> > >  	/*
> > >  	 * FIXME collapse everything to one hook.
> > >  	 * Need care with mst->ddi interactions.
> > > @@ -2726,6 +2729,19 @@ static void intel_set_transcoder_timings(const
> > > struct intel_crtc_state *crtc_sta
> > >  		intel_de_write(display, TRANS_VTOTAL(display, pipe),
> > >  			       VACTIVE(crtc_vdisplay - 1) |
> > >  			       VTOTAL(crtc_vtotal - 1));
> > > +
> > > +	if (DISPLAY_VER(display) >= 30 && crtc_state->min_hblank) {
> > 
> > Again pls have a look at Imre's comments in the prev version.
> > 
> > Update the crtc_state->min_hblank calculations so that it can have 0 for non
> > supported cases and have a non zero for 8b/10b MST and 128b/132b and
> > program here.
> No non-supported cases we don’t have the value calculated and we don’t have to write to this
> register for 8b/10b SST case. In supported cases 8b/10b MST and 128b/132b the value cannot be 0,
> if its 0 then it means the feature is disabled.

Update crtc_state->min_hblank to 0 or non-zero in intel_dp_compute_min_hblank() based on the
conditions you listed above and write the value here (If this is the right place) if
(DISPLAY_VER(display) >= 30). So you don't have to explicitly set it 0 in hsw_crtc_disable(). Also
pls note there aren't any direct transcoder register accesses being done in this function.

> 
> > 
> > 
> > > +		/*
> > > +		 * 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);
> > > +	}
> > 
> > >  }
> > > 
> > >  static void intel_set_transcoder_timings_lrr(const struct
> > > intel_crtc_state *crtc_state) @@ -5221,6 +5237,8 @@
> > intel_pipe_config_compare(const struct intel_crtc_state *current_config,
> > >  	PIPE_CONF_CHECK_I(lane_count);
> > >  	PIPE_CONF_CHECK_X(lane_lat_optim_mask);
> > > 
> > > +	PIPE_CONF_CHECK_I(min_hblank);
> > > +
> > >  	if (HAS_DOUBLE_BUFFERED_M_N(display)) {
> > >  		if (!fastset || !pipe_config->update_m_n)
> > >  			PIPE_CONF_CHECK_M_N(dp_m_n);
> > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> > > b/drivers/gpu/drm/i915/display/intel_dp.c
> > > index
> > > 
> > 7b95d62730e6f0c27c07fb59f16476369c4762a4..7b114b7cc86cc78088ca27e2c
> > b7b
> > > 08b8491d8283 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > > @@ -3103,6 +3103,67 @@ intel_dp_queue_modeset_retry_for_link(struct
> > intel_atomic_state *state,
> > >  	}
> > >  }
> > > 
> > > +void intel_dp_compute_min_hblank(int link_bpp_x16,
> > > +				 struct intel_crtc_state *crtc_state,
> > > +				 struct drm_connector_state *conn_state,
> > > +				 bool is_dsc)
> > > +{
> > > +	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;
> > > +	/*
> > > +	 * min symbol cycles is 3(BS,VBID, BE) for 128b/132b and
> > > +	 * 5(BS, VBID, MVID, MAUD) for 8b/10b
> > > +	 */
> > > +	int min_sym_cycles = intel_dp_is_uhbr(crtc_state) ? 3 : 5;
> > > +	bool is_mst = intel_crtc_has_type(crtc_state, INTEL_OUTPUT_DP_MST);
> > > +	int min_hblank;
> > > +	int max_lane_count = 4;
> > > +	int hactive_sym_cycles, htotal_sym_cycles;
> > > +	int dsc_slices = 0;
> > > +
> > > +	if (DISPLAY_VER(display) < 30)
> > > +		return;
> > > +
> > > +	/* MIN_HBLANK should be set only for 8b/10b MST or for 128b/132b
> > SST/MST */
> > > +	if (intel_dp_is_uhbr(crtc_state) || (is_mst &&
> > !intel_dp_is_uhbr(crtc_state)))
> > > +		return;
> > > +
> > > +	if (is_dsc) {
> > > +		dsc_slices = intel_dp_mst_dsc_get_slice_count(connector,
> > > +							      crtc_state);
> > > +		if (!dsc_slices) {
> > > +			drm_dbg(display->drm, "failed to calculate dsc slice
> > count\n");
> > > +			return;
> > > +		}
> > > +	}
> > > +
> > > +	/* Calculate min Hblank Link Layer Symbol Cycle Count for 8b/10b MST
> > & 128b/132b */
> > > +	hactive_sym_cycles =
> > drm_dp_link_data_symbol_cycles(max_lane_count,
> > > +							    adjusted_mode-
> > > hdisplay,
> > > +							    link_bpp_x16,
> > > +							    symbol_size,
> > > +							    is_mst,
> > > +							    dsc_slices);
> > > +	htotal_sym_cycles = adjusted_mode->htotal *
> > > +			    (hactive_sym_cycles / adjusted_mode->hdisplay);
> > > +
> > > +	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;
> > > +
> > > +	min_hblank = min(10, min_hblank);
> > > +	crtc_state->min_hblank = min_hblank; }
> > > +
> > >  int
> > >  intel_dp_compute_config(struct intel_encoder *encoder,
> > >  			struct intel_crtc_state *pipe_config, @@ -3202,6
> > +3263,9 @@
> > > intel_dp_compute_config(struct intel_encoder *encoder,
> > >  				       &pipe_config->dp_m_n);
> > >  	}
> > > 
> > > +	intel_dp_compute_min_hblank(link_bpp_x16, pipe_config, conn_state,
> > > +				    pipe_config->dsc.compression_enable);
> > > +
> > >  	/* FIXME: abstract this better */
> > >  	if (pipe_config->splitter.enable)
> > >  		pipe_config->dp_m_n.data_m *= pipe_config-
> > > splitter.link_count;
> > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.h
> > > b/drivers/gpu/drm/i915/display/intel_dp.h
> > > index
> > > 
> > 9189db4c25946a0f082223ce059c242e80cc32dc..43624aead998a8a330a244b
> > b9c85
> > > f026e203171b 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_dp.h
> > > +++ b/drivers/gpu/drm/i915/display/intel_dp.h
> > > @@ -208,5 +208,9 @@ bool intel_dp_has_connector(struct intel_dp
> > *intel_dp,
> > >  			    const struct drm_connector_state *conn_state);
> > >  int intel_dp_dsc_max_src_input_bpc(struct intel_display *display);
> > >  int intel_dp_dsc_min_src_input_bpc(void);
> > > +void intel_dp_compute_min_hblank(int link_bpp_x16,
> > > +				 struct intel_crtc_state *crtc_state,
> > > +				 struct drm_connector_state *conn_state,
> > > +				 bool is_dsc);
> > > 
> > >  #endif /* __INTEL_DP_H__ */
> > > diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > > b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > > index
> > > 
> > af98a0d0e8376a79ce1ab6ff3c4f6af30f4d3e73..4153afa13c618bb4db6dbcdc6e
> > 59
> > > faddcbeade6b 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,12 +281,11 @@
> > > 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,
> > >  					 &crtc_state->dp_m_n);
> > > +		intel_dp_compute_min_hblank(link_bpp_x16, crtc_state,
> > conn_state,
> > > +dsc);
> > 
> > Also I wanted to check if this is the right place to call
> > intel_dp_compute_min_hblank()? Can't this be called once we settle a link_bpp.
> > May be from mst_stream_compute_config() before calling
> > intel_dp_audio_compute_config(). @Imre pls comment.
> > 
> Here the calculation is done as part of the m and n calculation and this function is called from
> mst_stream_compute_config and is done before dp_audio_compute_config().
> Let me know if any change required will do.

Why do we need to do this at this point where we iterate through for each bpp_x16
"for (bpp_x16 = max_bpp_x16; bpp_x16 >= min_bpp_x16; bpp_x16 -= bpp_step_x16) {"
as it doesn't influence any decision on in this loop! 

BR
Vinod

> 
> > > 
> > >  		if (is_mst) {
> > >  			int remote_bw_overhead;
> > > @@ -998,7 +977,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 +987,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);
> > 
> > After removing this, "display" is not used anymore in this function. So need to
> > get rid of unused local variable display as well.
> > 
> There is a drm_dbg_kms which used the struct display. Let me try rebasing with the latest drm-tip.
> 
> Thanks and Regards,
> Arun R Murthy
> --------------------
> > BR
> > Vinod
> > 
> > >  }
> > > 
> > >  static void mst_stream_post_disable(struct intel_atomic_state *state,
> > > @@ -1286,7 +1261,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 +1276,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);
> > > 
> 

Re: [PATCH v3 3/3] drm/i915/display: move min_hblank from dp_mst.c to dp.c

[Imre Deak]

On Tue, Apr 22, 2025 at 10:25:48AM +0300, Govindapillai, Vinod wrote:
> [...]
>
> > > +3263,9 @@ intel_dp_compute_config(struct intel_encoder *encoder,
> > > >  				       &pipe_config->dp_m_n);
> > > >  	}
> > > > 
> > > > +	intel_dp_compute_min_hblank(link_bpp_x16, pipe_config, conn_state,
> > > > +				    pipe_config->dsc.compression_enable);
> > > > +
> > > >  	/* FIXME: abstract this better */
> > > >  	if (pipe_config->splitter.enable)
> > > >  		pipe_config->dp_m_n.data_m *= pipe_config->splitter.link_count;
>
> [...]
>
> > > @@ -301,12 +281,11 @@ 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,
> > > >  					 &crtc_state->dp_m_n);
> > > > +		intel_dp_compute_min_hblank(link_bpp_x16, crtc_state, conn_state, dsc);
> > > 
> > > Also I wanted to check if this is the right place to call
> > > intel_dp_compute_min_hblank()? Can't this be called once we settle a link_bpp.
> > > May be from mst_stream_compute_config() before calling
> > > intel_dp_audio_compute_config(). @Imre pls comment.

Yes, it would be better not to recompute the min_hblank value for each bpp.
So just moving intel_dp_compute_min_hblank() before
intel_vrr_compute_config() in mst_stream_compute_config(), similarly how
this is done for the SST case above in intel_dp_compute_config().

> > Here the calculation is done as part of the m and n calculation and
> > this function is called from mst_stream_compute_config and is done
> > before dp_audio_compute_config().  Let me know if any change
> > required will do.

> Why do we need to do this at this point where we iterate through for
> each bpp_x16 "for (bpp_x16 = max_bpp_x16; bpp_x16 >= min_bpp_x16;
> bpp_x16 -= bpp_step_x16) {" as it doesn't influence any decision on in
> this loop! 
> 
> BR
> Vinod

RE: [PATCH v3 3/3] drm/i915/display: move min_hblank from dp_mst.c to dp.c

[Kandpal, Suraj]

> -----Original Message-----
> From: Intel-xe <intel-xe-bounces@lists.freedesktop.org> On Behalf Of Arun R
> Murthy
> Sent: Thursday, April 17, 2025 4:23 PM
> To: dri-devel@lists.freedesktop.org; intel-gfx@lists.freedesktop.org; intel-
> xe@lists.freedesktop.org
> Cc: Govindapillai, Vinod <vinod.govindapillai@intel.com>; Deak, Imre
> <imre.deak@intel.com>; Murthy, Arun R <arun.r.murthy@intel.com>
> Subject: [PATCH v3 3/3] drm/i915/display: move min_hblank from dp_mst.c to
> dp.c
> 
> 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.
> 
> TODO: Add the min_hblank calculation for hdmi as well.
> 
> v2: move from intel_audio.c to intel_dp.c
>     some correction in link_bpp_x16 (Imre)
> v3: min_hblank for 8b/10b MST and 128b/132b SST/MST
>     handle error for intel_dp_mst_dsc_get_slice_count
>     reset min_hblank before disabling transcoder (Imre)
> 
> Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display.c | 18 ++++++++
>  drivers/gpu/drm/i915/display/intel_dp.c      | 64
> ++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/display/intel_dp.h      |  4 ++
>  drivers/gpu/drm/i915/display/intel_dp_mst.c  | 52 +---------------------
>  4 files changed, 88 insertions(+), 50 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> b/drivers/gpu/drm/i915/display/intel_display.c
> index
> dc7517da2ed5c75fb1715d43e6bfc792a8420f30..ad41382981299dffab5cc592b4
> 3cdab7eebb7b5b 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -1782,8 +1782,11 @@ static void hsw_crtc_disable(struct
> intel_atomic_state *state,
>  	const struct intel_crtc_state *old_crtc_state =
>  		intel_atomic_get_old_crtc_state(state, crtc);
>  	struct intel_crtc *pipe_crtc;
> +	enum transcoder cpu_transcoder = old_crtc_state->cpu_transcoder;
>  	int i;
> 
> +	if (DISPLAY_VER(display) >= 30)
> +		intel_de_write(display, DP_MIN_HBLANK_CTL(cpu_transcoder),
> 0);
>  	/*
>  	 * FIXME collapse everything to one hook.
>  	 * Need care with mst->ddi interactions.
> @@ -2726,6 +2729,19 @@ static void intel_set_transcoder_timings(const
> struct intel_crtc_state *crtc_sta
>  		intel_de_write(display, TRANS_VTOTAL(display, pipe),
>  			       VACTIVE(crtc_vdisplay - 1) |
>  			       VTOTAL(crtc_vtotal - 1));
> +
> +	if (DISPLAY_VER(display) >= 30 && crtc_state->min_hblank) {
> +		/*
> +		 * 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);
> +	}
>  }
> 
>  static void intel_set_transcoder_timings_lrr(const struct intel_crtc_state
> *crtc_state) @@ -5221,6 +5237,8 @@ intel_pipe_config_compare(const struct
> intel_crtc_state *current_config,
>  	PIPE_CONF_CHECK_I(lane_count);
>  	PIPE_CONF_CHECK_X(lane_lat_optim_mask);
> 
> +	PIPE_CONF_CHECK_I(min_hblank);
> +
>  	if (HAS_DOUBLE_BUFFERED_M_N(display)) {
>  		if (!fastset || !pipe_config->update_m_n)
>  			PIPE_CONF_CHECK_M_N(dp_m_n);
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> b/drivers/gpu/drm/i915/display/intel_dp.c
> index
> 7b95d62730e6f0c27c07fb59f16476369c4762a4..7b114b7cc86cc78088ca27e2cb
> 7b08b8491d8283 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -3103,6 +3103,67 @@ intel_dp_queue_modeset_retry_for_link(struct
> intel_atomic_state *state,
>  	}
>  }
> 
> +void intel_dp_compute_min_hblank(int link_bpp_x16,
> +				 struct intel_crtc_state *crtc_state,
> +				 struct drm_connector_state *conn_state,
> +				 bool is_dsc)
> +{
> +	struct intel_display *display = to_intel_display(crtc_state);

Not needed.


> +	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;
> +	/*
> +	 * min symbol cycles is 3(BS,VBID, BE) for 128b/132b and
> +	 * 5(BS, VBID, MVID, MAUD) for 8b/10b
> +	 */
> +	int min_sym_cycles = intel_dp_is_uhbr(crtc_state) ? 3 : 5;
> +	bool is_mst = intel_crtc_has_type(crtc_state,
> INTEL_OUTPUT_DP_MST);
> +	int min_hblank;
> +	int max_lane_count = 4;
> +	int hactive_sym_cycles, htotal_sym_cycles;
> +	int dsc_slices = 0;
> +
> +	if (DISPLAY_VER(display) < 30)
> +		return;
> +
> +	/* MIN_HBLANK should be set only for 8b/10b MST or for 128b/132b
> SST/MST */
> +	if (intel_dp_is_uhbr(crtc_state) || (is_mst &&
> !intel_dp_is_uhbr(crtc_state)))
> +		return;
> +
> +	if (is_dsc) {
> +		dsc_slices = intel_dp_mst_dsc_get_slice_count(connector,
> +							      crtc_state);
> +		if (!dsc_slices) {
> +			drm_dbg(display->drm, "failed to calculate dsc slice
> count\n");
> +			return;
> +		}
> +	}
> +
> +	/* Calculate min Hblank Link Layer Symbol Cycle Count for 8b/10b MST
> & 128b/132b */
> +	hactive_sym_cycles =
> drm_dp_link_data_symbol_cycles(max_lane_count,
> +							    adjusted_mode-
> >hdisplay,
> +							    link_bpp_x16,
> +							    symbol_size,
> +							    is_mst,
> +							    dsc_slices);
> +	htotal_sym_cycles = adjusted_mode->htotal *
> +			    (hactive_sym_cycles / adjusted_mode->hdisplay);
> +
> +	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);

From the solution I see the way to calculate min hblank as 
HACT_ML_SYM_CYC_CNT = CEIL(CEIL(HACT_WIDTH / 4) × PIX_BPP / SYMBOL_SIZE) × 4 / PHY_LANE_CNT 
HBLNK_ML_SYM_CYC_CNT =  CEIL(CEIL(HBLNK_WIDTH / 4) × PIX_BPP / SYMBOL_SIZE) × 4 / PHY_LANE_CNT 
HTOTAL_ML_SYM_CYC_CNT = HACT_ML_SYM_CYC_CNT+ HBLNK_ML_SYM_CYC_CNT
EFF_PIX_BPP = HTOTAL_ML_SYM_CYC_CNT × SYMBOL_SIZE × PHY_LANE_CNT / HTOTAL_WIDTH
 
Which is similar to how we calculate hactive_sym_cycles so lets use drm_dp_link_data_symbol_cycles
and pass htotal-hdisplay to get min hblank we wont need to calculate htotal sym cycles that way too

> +	/*
> +	 * adjust the BlankingStart/BlankingEnd framing control from
> +	 * the calculated value
> +	 */
> +	min_hblank = min_hblank - 2;
> +
> +	min_hblank = min(10, min_hblank);

Is this 10 or 0x10 since previously 0x10 was coded in

Regards,
Suraj Kandpal

> +	crtc_state->min_hblank = min_hblank;
> +}
> +
>  int
>  intel_dp_compute_config(struct intel_encoder *encoder,
>  			struct intel_crtc_state *pipe_config, @@ -3202,6
> +3263,9 @@ intel_dp_compute_config(struct intel_encoder *encoder,
>  				       &pipe_config->dp_m_n);
>  	}
> 
> +	intel_dp_compute_min_hblank(link_bpp_x16, pipe_config, conn_state,
> +				    pipe_config->dsc.compression_enable);
> +
>  	/* FIXME: abstract this better */
>  	if (pipe_config->splitter.enable)
>  		pipe_config->dp_m_n.data_m *= pipe_config-
> >splitter.link_count; diff --git a/drivers/gpu/drm/i915/display/intel_dp.h
> b/drivers/gpu/drm/i915/display/intel_dp.h
> index
> 9189db4c25946a0f082223ce059c242e80cc32dc..43624aead998a8a330a244bb9
> c85f026e203171b 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.h
> +++ b/drivers/gpu/drm/i915/display/intel_dp.h
> @@ -208,5 +208,9 @@ bool intel_dp_has_connector(struct intel_dp *intel_dp,
>  			    const struct drm_connector_state *conn_state);  int
> intel_dp_dsc_max_src_input_bpc(struct intel_display *display);  int
> intel_dp_dsc_min_src_input_bpc(void);
> +void intel_dp_compute_min_hblank(int link_bpp_x16,
> +				 struct intel_crtc_state *crtc_state,
> +				 struct drm_connector_state *conn_state,
> +				 bool is_dsc);
> 
>  #endif /* __INTEL_DP_H__ */
> diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> index
> af98a0d0e8376a79ce1ab6ff3c4f6af30f4d3e73..4153afa13c618bb4db6dbcdc6e5
> 9faddcbeade6b 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,12 +281,11 @@ 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,
>  					 &crtc_state->dp_m_n);
> +		intel_dp_compute_min_hblank(link_bpp_x16, crtc_state,
> conn_state,
> +dsc);
> 
>  		if (is_mst) {
>  			int remote_bw_overhead;
> @@ -998,7 +977,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 +987,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 +1261,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 +1276,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

Re: [PATCH v3 3/3] drm/i915/display: move min_hblank from dp_mst.c to dp.c

[Imre Deak]

On Tue, Apr 22, 2025 at 08:28:31AM +0300, Kandpal, Suraj wrote:
> [...]
>
> > +void intel_dp_compute_min_hblank(int link_bpp_x16,
> > +				 struct intel_crtc_state *crtc_state,
> > +				 struct drm_connector_state *conn_state,
> > +				 bool is_dsc)
> > +{
> >
> > [...]
> > 
> > +
> > +	/* Calculate min Hblank Link Layer Symbol Cycle Count for 8b/10b MST & 128b/132b */
> > +	hactive_sym_cycles = drm_dp_link_data_symbol_cycles(max_lane_count,
> > +							    adjusted_mode->hdisplay,
> > +							    link_bpp_x16,
> > +							    symbol_size,
> > +							    is_mst,
> > +							    dsc_slices);
> > +	htotal_sym_cycles = adjusted_mode->htotal *
> > +			    (hactive_sym_cycles / adjusted_mode->hdisplay);

Here the ( ) around the divisor should be dropped for the div-round-down
by adjusted_mode->hdisplay to work as expected.

> > +
> > +	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);
> 
> From the solution I see the way to calculate min hblank as 
> HACT_ML_SYM_CYC_CNT = CEIL(CEIL(HACT_WIDTH / 4) × PIX_BPP / SYMBOL_SIZE) × 4 / PHY_LANE_CNT 
> HBLNK_ML_SYM_CYC_CNT =  CEIL(CEIL(HBLNK_WIDTH / 4) × PIX_BPP / SYMBOL_SIZE) × 4 / PHY_LANE_CNT 
> HTOTAL_ML_SYM_CYC_CNT = HACT_ML_SYM_CYC_CNT+ HBLNK_ML_SYM_CYC_CNT
> EFF_PIX_BPP = HTOTAL_ML_SYM_CYC_CNT × SYMBOL_SIZE × PHY_LANE_CNT / HTOTAL_WIDTH
>  
> Which is similar to how we calculate hactive_sym_cycles so lets use drm_dp_link_data_symbol_cycles
> and pass htotal-hdisplay to get min hblank we wont need to calculate htotal sym cycles that way too

That would be calculating EFF_HBLNK_ML_SYM_CYC_CNT (or the LL version of
this), so a different value than MIN_HBLNK_LL_SYM_CYC_CNT. The latter is
based on a rounded-down htotal_sym_cycles value, see:

MIN_HBLNK_LL_SYM_CYC_CNT_128B132B_DPTX
= MAX((FLOOR(HTOTAL_WIDTH × EFF_PIX_BPP / (4 × SYMBOL_SIZE))
– HACT_LL_SYM_CYC_CNT), 3)

Using the

EFF_PIX_BPP
= HACT_ML_SYM_CYC_CNT × SYMBOL_SIZE × PHY_LANE_CNT / HACT_WIDTH

equation in the standard, this matches the above way in the patch
calculating htotal_sym_cycles, subtracting hactive_sym_cycles from it.

> > +	/*
> > +	 * adjust the BlankingStart/BlankingEnd framing control from
> > +	 * the calculated value
> > +	 */
> > +	min_hblank = min_hblank - 2;
> > +
> > +	min_hblank = min(10, min_hblank);
> 
> Is this 10 or 0x10 since previously 0x10 was coded in
> 
> Regards,
> Suraj Kandpal
> 
> > +	crtc_state->min_hblank = min_hblank;
> > +}
> > +
> >  int
> >  intel_dp_compute_config(struct intel_encoder *encoder,
> >  			struct intel_crtc_state *pipe_config, @@ -3202,6
> > +3263,9 @@ intel_dp_compute_config(struct intel_encoder *encoder,
> >  				       &pipe_config->dp_m_n);
> >  	}
> > 
> > +	intel_dp_compute_min_hblank(link_bpp_x16, pipe_config, conn_state,
> > +				    pipe_config->dsc.compression_enable);
> > +
> >  	/* FIXME: abstract this better */
> >  	if (pipe_config->splitter.enable)
> >  		pipe_config->dp_m_n.data_m *= pipe_config-
> > >splitter.link_count; diff --git a/drivers/gpu/drm/i915/display/intel_dp.h
> > b/drivers/gpu/drm/i915/display/intel_dp.h
> > index
> > 9189db4c25946a0f082223ce059c242e80cc32dc..43624aead998a8a330a244bb9
> > c85f026e203171b 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp.h
> > +++ b/drivers/gpu/drm/i915/display/intel_dp.h
> > @@ -208,5 +208,9 @@ bool intel_dp_has_connector(struct intel_dp *intel_dp,
> >  			    const struct drm_connector_state *conn_state);  int
> > intel_dp_dsc_max_src_input_bpc(struct intel_display *display);  int
> > intel_dp_dsc_min_src_input_bpc(void);
> > +void intel_dp_compute_min_hblank(int link_bpp_x16,
> > +				 struct intel_crtc_state *crtc_state,
> > +				 struct drm_connector_state *conn_state,
> > +				 bool is_dsc);
> > 
> >  #endif /* __INTEL_DP_H__ */
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > index
> > af98a0d0e8376a79ce1ab6ff3c4f6af30f4d3e73..4153afa13c618bb4db6dbcdc6e5
> > 9faddcbeade6b 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,12 +281,11 @@ 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,
> >  					 &crtc_state->dp_m_n);
> > +		intel_dp_compute_min_hblank(link_bpp_x16, crtc_state,
> > conn_state,
> > +dsc);
> > 
> >  		if (is_mst) {
> >  			int remote_bw_overhead;
> > @@ -998,7 +977,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 +987,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 +1261,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 +1276,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
> 

✗ Fi.CI.BUILD: failure for Rework/Correction on minimum hblank calculation (rev3)

[Patchwork]

== Series Details ==

Series: Rework/Correction on minimum hblank calculation (rev3)
URL   : https://patchwork.freedesktop.org/series/147361/
State : failure

== Summary ==

Error: make failed
  CALL    scripts/checksyscalls.sh
  DESCEND objtool
  INSTALL libsubcmd_headers
  CC [M]  drivers/gpu/drm/i915/display/intel_dp_mst.o
drivers/gpu/drm/i915/display/intel_dp_mst.c: In function ‘mst_stream_disable’:
drivers/gpu/drm/i915/display/intel_dp_mst.c:1003:31: error: unused variable ‘display’ [-Werror=unused-variable]
 1003 |         struct intel_display *display = to_intel_display(state);
      |                               ^~~~~~~
cc1: all warnings being treated as errors
make[6]: *** [scripts/Makefile.build:203: drivers/gpu/drm/i915/display/intel_dp_mst.o] Error 1
make[5]: *** [scripts/Makefile.build:461: drivers/gpu/drm/i915] Error 2
make[4]: *** [scripts/Makefile.build:461: drivers/gpu/drm] Error 2
make[3]: *** [scripts/Makefile.build:461: drivers/gpu] Error 2
make[2]: *** [scripts/Makefile.build:461: drivers] Error 2
make[1]: *** [/home/kbuild2/kernel/Makefile:2009: .] Error 2
make: *** [Makefile:248: __sub-make] Error 2
Build failed, no error log produced