[Intel-gfx] [PATCH 1/4] drm/i915/dp: Account for channel coding efficiency on UHBR links

[Intel-gfx] [PATCH 1/4] drm/i915/dp: Account for channel coding efficiency on UHBR links

[Imre Deak]

Apply the correct BW allocation overhead and channel coding efficiency
on UHBR link rates, similarly to DP1.4 link rates.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/display/intel_display.c | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index 3effafcbb411a..24aebdb715e7d 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -2398,16 +2398,6 @@ add_bw_alloc_overhead(int link_clock, int bw_overhead,
 	int ch_coding_efficiency =
 		drm_dp_bw_channel_coding_efficiency(is_uhbr);
 
-	/*
-	 * TODO: adjust for actual UHBR channel coding efficiency and BW
-	 * overhead.
-	 */
-	if (is_uhbr) {
-		*data_m = pixel_data_rate;
-		*data_n = link_data_rate * 8 / 10;
-		return;
-	}
-
 	*data_m = DIV_ROUND_UP_ULL(mul_u32_u32(pixel_data_rate, bw_overhead),
 				   1000000);
 	*data_n = DIV_ROUND_DOWN_ULL(mul_u32_u32(link_data_rate, ch_coding_efficiency),
-- 
2.39.2

[Intel-gfx] [PATCH 2/4] drm/i915/dp: Fix UHBR link M/N values

[Imre Deak]

The link M/N ratio is the data rate / link symbol clock rate, fix things
up accordingly. On DP 1.4 this ratio was correct as the link symbol clock
rate in that case matched the link data rate (in bytes/sec units, the
symbol size being 8 bits), however it wasn't correct for UHBR rates
where the symbol size is 32 bits.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/display/intel_display.c | 16 ++++++++-----
 drivers/gpu/drm/i915/display/intel_dp.c      | 24 ++++++++++++++++++++
 drivers/gpu/drm/i915/display/intel_dp.h      |  2 ++
 3 files changed, 36 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index 24aebdb715e7d..c059eb0170a5b 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -2411,6 +2411,7 @@ intel_link_compute_m_n(u16 bits_per_pixel, int nlanes,
 		       struct intel_link_m_n *m_n)
 {
 	u32 data_clock = bits_per_pixel * pixel_clock;
+	u32 link_symbol_clock = intel_dp_link_symbol_clock(link_clock);
 	u32 data_m;
 	u32 data_n;
 
@@ -2431,7 +2432,7 @@ intel_link_compute_m_n(u16 bits_per_pixel, int nlanes,
 		    0x8000000);
 
 	compute_m_n(&m_n->link_m, &m_n->link_n,
-		    pixel_clock, link_clock,
+		    pixel_clock, link_symbol_clock,
 		    0x80000);
 }
 
@@ -3943,20 +3944,23 @@ int intel_dotclock_calculate(int link_freq,
 			     const struct intel_link_m_n *m_n)
 {
 	/*
-	 * The calculation for the data clock is:
+	 * The calculation for the data clock -> pixel clock is:
 	 * pixel_clock = ((m/n)*(link_clock * nr_lanes))/bpp
 	 * But we want to avoid losing precison if possible, so:
 	 * pixel_clock = ((m * link_clock * nr_lanes)/(n*bpp))
 	 *
-	 * and the link clock is simpler:
-	 * link_clock = (m * link_clock) / n
+	 * and for link freq (10kbs units) -> pixel clock it is:
+	 * link_symbol_clock = link_freq * 10 / link_symbol_size
+	 * pixel_clock = (m * link_symbol_clock) / n
+	 *    or for more precision:
+	 * pixel_clock = (m * link_freq * 10) / (n * link_symbol_size)
 	 */
 
 	if (!m_n->link_n)
 		return 0;
 
-	return DIV_ROUND_UP_ULL(mul_u32_u32(m_n->link_m, link_freq),
-				m_n->link_n);
+	return DIV_ROUND_UP_ULL(mul_u32_u32(m_n->link_m, link_freq * 10),
+				m_n->link_n * intel_dp_link_symbol_size(link_freq));
 }
 
 int intel_crtc_dotclock(const struct intel_crtc_state *pipe_config)
diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
index f662d1ce5f72c..80e1e887432fa 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -132,6 +132,30 @@ bool intel_dp_is_uhbr(const struct intel_crtc_state *crtc_state)
 	return intel_dp_is_uhbr_rate(crtc_state->port_clock);
 }
 
+/**
+ * intel_dp_link_symbol_size - get the link symbol size for a given link rate
+ * @rate: link rate in 10kbit/s units
+ *
+ * Returns the link symbol size in bits/symbol units depending on the link
+ * rate -> channel coding.
+ */
+int intel_dp_link_symbol_size(int rate)
+{
+	return intel_dp_is_uhbr_rate(rate) ? 32 : 10;
+}
+
+/**
+ * intel_dp_link_symbol_clock - convert link rate to link symbol clock
+ * @rate: link rate in 10kbit/s units
+ *
+ * Returns the link symbol clock frequency in kHz units depending on the
+ * link rate and channel coding.
+ */
+int intel_dp_link_symbol_clock(int rate)
+{
+	return DIV_ROUND_CLOSEST(rate * 10, intel_dp_link_symbol_size(rate));
+}
+
 static void intel_dp_set_default_sink_rates(struct intel_dp *intel_dp)
 {
 	intel_dp->sink_rates[0] = 162000;
diff --git a/drivers/gpu/drm/i915/display/intel_dp.h b/drivers/gpu/drm/i915/display/intel_dp.h
index e80da67554196..64dbf8f192708 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.h
+++ b/drivers/gpu/drm/i915/display/intel_dp.h
@@ -82,6 +82,8 @@ bool intel_dp_has_hdmi_sink(struct intel_dp *intel_dp);
 bool intel_dp_is_edp(struct intel_dp *intel_dp);
 bool intel_dp_is_uhbr_rate(int rate);
 bool intel_dp_is_uhbr(const struct intel_crtc_state *crtc_state);
+int intel_dp_link_symbol_size(int rate);
+int intel_dp_link_symbol_clock(int rate);
 bool intel_dp_is_port_edp(struct drm_i915_private *dev_priv, enum port port);
 enum irqreturn intel_dp_hpd_pulse(struct intel_digital_port *dig_port,
 				  bool long_hpd);
-- 
2.39.2

[Intel-gfx] [PATCH 3/4] drm/i915/dp_mst: Fix PBN / MTP_TU size calculation for UHBR rates

[Imre Deak]

Atm the allocated MST PBN value is calculated from the TU size (number
of allocated MTP slots) as

  PBN = TU * pbn_div

pbn_div being the link BW for each MTP slot. For DP 1.4 link rates this
worked, as pbn_div there is guraranteed to be an integer number, however
on UHBR this isn't the case. To get a PBN, TU pair where TU is a
properly rounded-up value covering all the BW corresponding to PBN,
calculate first PBN and from PBN the TU value.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/display/intel_dp_mst.c | 10 ++++++++--
 1 file changed, 8 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 b943dbf394a22..a32ab0b4fc9d7 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
@@ -170,6 +170,7 @@ static int intel_dp_mst_find_vcpi_slots_for_bpp(struct intel_encoder *encoder,
 
 	for (bpp = max_bpp; bpp >= min_bpp; bpp -= step) {
 		struct intel_link_m_n remote_m_n;
+		int alloc_tu;
 		int link_bpp;
 
 		drm_dbg_kms(&i915->drm, "Trying bpp %d\n", bpp);
@@ -200,9 +201,14 @@ static int intel_dp_mst_find_vcpi_slots_for_bpp(struct intel_encoder *encoder,
 		 * crtc_state->dp_m_n.tu), provided that the driver doesn't
 		 * enable SSC on the corresponding link.
 		 */
+		crtc_state->pbn = DIV_ROUND_UP_ULL(mul_u32_u32(mst_state->pbn_div * 64,
+							       remote_m_n.data_m),
+						   remote_m_n.data_n);
+
+		alloc_tu = DIV_ROUND_UP_ULL(crtc_state->pbn, mst_state->pbn_div);
+		drm_WARN_ON(&i915->drm, alloc_tu < remote_m_n.tu);
 		drm_WARN_ON(&i915->drm, remote_m_n.tu < crtc_state->dp_m_n.tu);
-		crtc_state->dp_m_n.tu = remote_m_n.tu;
-		crtc_state->pbn = remote_m_n.tu * mst_state->pbn_div;
+		crtc_state->dp_m_n.tu = alloc_tu;
 
 		slots = drm_dp_atomic_find_time_slots(state, &intel_dp->mst_mgr,
 						      connector->port,
-- 
2.39.2

[Intel-gfx] [PATCH 4/4] drm/dp_mst: Fix PBN divider calculation for UHBR rates

[Imre Deak]

The current way of calculating the pbn_div value, the link BW per each
MTP slot, worked only for DP 1.4 link rates. Fix things up for UHBR
rates calculating with the correct channel coding efficiency based on
the link rate.

On UHBR the resulting pbn_div value is not an integer (vs. DP 1.4 where
the value is always an integer), so ideally a scaled value containing
the fractional part should be returned, so that the PBN -> MTP slot
count (aka TU size) conversion can be done with less error. For now
return a rounded-down value - which can result in +1 excess MTP slot
getting allocated on UHBR links.

Cc: Lyude Paul <lyude@redhat.com>
Cc: dri-devel@lists.freedesktop.org
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/display/drm_dp_mst_topology.c | 15 +++++++++++++--
 include/drm/display/drm_dp_helper.h           | 13 +++++++++++++
 2 files changed, 26 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c b/drivers/gpu/drm/display/drm_dp_mst_topology.c
index 4d72c9a32026e..940a9fc0d0244 100644
--- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
@@ -3582,12 +3582,23 @@ static int drm_dp_send_up_ack_reply(struct drm_dp_mst_topology_mgr *mgr,
 int drm_dp_get_vc_payload_bw(const struct drm_dp_mst_topology_mgr *mgr,
 			     int link_rate, int link_lane_count)
 {
+	int ret;
+
 	if (link_rate == 0 || link_lane_count == 0)
 		drm_dbg_kms(mgr->dev, "invalid link rate/lane count: (%d / %d)\n",
 			    link_rate, link_lane_count);
 
-	/* See DP v2.0 2.6.4.2, VCPayload_Bandwidth_for_OneTimeSlotPer_MTP_Allocation */
-	return link_rate * link_lane_count / 54000;
+	/* See DP v2.0 2.6.4.2, 2.7.6.3 VCPayload_Bandwidth_for_OneTimeSlotPer_MTP_Allocation */
+	/*
+	 * TODO: Return the value with a higher precision, allowing a better
+	 * slots per MTP allocation granularity. With the current returned
+	 * value +1 slot/MTP can get allocated on UHBR links.
+	 */
+	ret = mul_u32_u32(link_rate * link_lane_count,
+			  drm_dp_bw_channel_coding_efficiency(drm_dp_is_uhbr_rate(link_rate))) /
+	      (1000000ULL * 8 * 5400);
+
+	return ret;
 }
 EXPORT_SYMBOL(drm_dp_get_vc_payload_bw);
 
diff --git a/include/drm/display/drm_dp_helper.h b/include/drm/display/drm_dp_helper.h
index caee29d28463c..18ff6af0b5a31 100644
--- a/include/drm/display/drm_dp_helper.h
+++ b/include/drm/display/drm_dp_helper.h
@@ -251,6 +251,19 @@ drm_edp_backlight_supported(const u8 edp_dpcd[EDP_DISPLAY_CTL_CAP_SIZE])
 	return !!(edp_dpcd[1] & DP_EDP_TCON_BACKLIGHT_ADJUSTMENT_CAP);
 }
 
+/**
+ * drm_dp_is_uhbr_rate - Determine if a link rate is UHBR
+ * @link_rate: link rate in 10kbits/s units
+ *
+ * Determine if the provided link rate is an UHBR rate.
+ *
+ * Returns: %True if @link_rate is an UHBR rate.
+ */
+static inline bool drm_dp_is_uhbr_rate(int link_rate)
+{
+	return link_rate >= 1000000;
+}
+
 /*
  * DisplayPort AUX channel
  */
-- 
2.39.2

[Intel-gfx] ✗ Fi.CI.SPARSE: warning for series starting with [1/4] drm/i915/dp: Account for channel coding efficiency on UHBR links

[Patchwork]

== Series Details ==

Series: series starting with [1/4] drm/i915/dp: Account for channel coding efficiency on UHBR links
URL   : https://patchwork.freedesktop.org/series/126350/
State : warning

== Summary ==

Error: dim sparse failed
Sparse version: v0.6.2
Fast mode used, each commit won't be checked separately.

[Intel-gfx] ✗ Fi.CI.BAT: failure for series starting with [1/4] drm/i915/dp: Account for channel coding efficiency on UHBR links

[Patchwork]

[-- Attachment #1: Type: text/plain, Size: 6347 bytes --]

== Series Details ==

Series: series starting with [1/4] drm/i915/dp: Account for channel coding efficiency on UHBR links
URL   : https://patchwork.freedesktop.org/series/126350/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_13870 -> Patchwork_126350v1
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_126350v1 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_126350v1, please notify your bug team (lgci.bug.filing@intel.com) to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_126350v1/index.html

Participating hosts (34 -> 34)
------------------------------

  Additional (2): fi-hsw-4770 bat-kbl-2 
  Missing    (2): bat-dg2-9 fi-snb-2520m 

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_126350v1:

### IGT changes ###

#### Possible regressions ####

  * igt@i915_selftest@live@gem:
    - fi-hsw-4770:        NOTRUN -> [INCOMPLETE][1]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_126350v1/fi-hsw-4770/igt@i915_selftest@live@gem.html

  
Known issues
------------

  Here are the changes found in Patchwork_126350v1 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@fbdev@info:
    - bat-kbl-2:          NOTRUN -> [SKIP][2] ([fdo#109271] / [i915#1849])
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_126350v1/bat-kbl-2/igt@fbdev@info.html

  * igt@gem_lmem_swapping@parallel-random-engines:
    - bat-kbl-2:          NOTRUN -> [SKIP][3] ([fdo#109271]) +24 other tests skip
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_126350v1/bat-kbl-2/igt@gem_lmem_swapping@parallel-random-engines.html

  * igt@i915_module_load@load:
    - bat-adlp-6:         [PASS][4] -> [DMESG-WARN][5] ([i915#8449])
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13870/bat-adlp-6/igt@i915_module_load@load.html
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_126350v1/bat-adlp-6/igt@i915_module_load@load.html

  * igt@i915_selftest@live@gt_heartbeat:
    - fi-apl-guc:         [PASS][6] -> [DMESG-FAIL][7] ([i915#5334])
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13870/fi-apl-guc/igt@i915_selftest@live@gt_heartbeat.html
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_126350v1/fi-apl-guc/igt@i915_selftest@live@gt_heartbeat.html

  * igt@i915_selftest@live@hangcheck:
    - fi-skl-guc:         [PASS][8] -> [DMESG-FAIL][9] ([i915#9549])
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13870/fi-skl-guc/igt@i915_selftest@live@hangcheck.html
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_126350v1/fi-skl-guc/igt@i915_selftest@live@hangcheck.html

  * igt@kms_addfb_basic@addfb25-y-tiled-small-legacy:
    - fi-hsw-4770:        NOTRUN -> [SKIP][10] ([fdo#109271] / [i915#5190])
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_126350v1/fi-hsw-4770/igt@kms_addfb_basic@addfb25-y-tiled-small-legacy.html

  * igt@kms_pipe_crc_basic@compare-crc-sanitycheck-nv12@pipe-a-vga-1:
    - fi-hsw-4770:        NOTRUN -> [SKIP][11] ([fdo#109271]) +12 other tests skip
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_126350v1/fi-hsw-4770/igt@kms_pipe_crc_basic@compare-crc-sanitycheck-nv12@pipe-a-vga-1.html

  * igt@kms_pipe_crc_basic@read-crc-frame-sequence:
    - bat-kbl-2:          NOTRUN -> [SKIP][12] ([fdo#109271] / [i915#1845]) +14 other tests skip
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_126350v1/bat-kbl-2/igt@kms_pipe_crc_basic@read-crc-frame-sequence.html

  * igt@kms_pipe_crc_basic@read-crc@pipe-c-dp-5:
    - bat-adlp-11:        [PASS][13] -> [DMESG-WARN][14] ([i915#4309])
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13870/bat-adlp-11/igt@kms_pipe_crc_basic@read-crc@pipe-c-dp-5.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_126350v1/bat-adlp-11/igt@kms_pipe_crc_basic@read-crc@pipe-c-dp-5.html

  * igt@kms_psr@sprite_plane_onoff:
    - fi-hsw-4770:        NOTRUN -> [SKIP][15] ([fdo#109271] / [i915#1072]) +3 other tests skip
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_126350v1/fi-hsw-4770/igt@kms_psr@sprite_plane_onoff.html

  
#### Possible fixes ####

  * igt@i915_selftest@live@dmabuf:
    - bat-mtlp-8:         [DMESG-FAIL][16] -> [PASS][17]
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13870/bat-mtlp-8/igt@i915_selftest@live@dmabuf.html
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_126350v1/bat-mtlp-8/igt@i915_selftest@live@dmabuf.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [i915#1072]: https://gitlab.freedesktop.org/drm/intel/issues/1072
  [i915#1845]: https://gitlab.freedesktop.org/drm/intel/issues/1845
  [i915#1849]: https://gitlab.freedesktop.org/drm/intel/issues/1849
  [i915#4309]: https://gitlab.freedesktop.org/drm/intel/issues/4309
  [i915#5190]: https://gitlab.freedesktop.org/drm/intel/issues/5190
  [i915#5334]: https://gitlab.freedesktop.org/drm/intel/issues/5334
  [i915#8449]: https://gitlab.freedesktop.org/drm/intel/issues/8449
  [i915#9549]: https://gitlab.freedesktop.org/drm/intel/issues/9549


Build changes
-------------

  * Linux: CI_DRM_13870 -> Patchwork_126350v1

  CI-20190529: 20190529
  CI_DRM_13870: 30cf0be8023394a90d58bcff7803d427909de6d8 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_7584: 30e6ded90039edde8aa6c435001f8d63159356bb @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
  Patchwork_126350v1: 30cf0be8023394a90d58bcff7803d427909de6d8 @ git://anongit.freedesktop.org/gfx-ci/linux


### Linux commits

07584a2fc355 drm/dp_mst: Fix PBN divider calculation for UHBR rates
7e64fcabaa6b drm/i915/dp_mst: Fix PBN / MTP_TU size calculation for UHBR rates
a42d6f54d4c0 drm/i915/dp: Fix UHBR link M/N values
feddebada956 drm/i915/dp: Account for channel coding efficiency on UHBR links

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_126350v1/index.html

[-- Attachment #2: Type: text/html, Size: 7615 bytes --]

Re: [Intel-gfx] [PATCH 4/4] drm/dp_mst: Fix PBN divider calculation for UHBR rates

[kernel test robot]

Hi Imre,

kernel test robot noticed the following build errors:

[auto build test ERROR on drm-tip/drm-tip]

url:    https://github.com/intel-lab-lkp/linux/commits/Imre-Deak/drm-i915-dp-Fix-UHBR-link-M-N-values/20231114-043135
base:   git://anongit.freedesktop.org/drm/drm-tip drm-tip
patch link:    https://lore.kernel.org/r/20231113201110.510724-4-imre.deak%40intel.com
patch subject: [PATCH 4/4] drm/dp_mst: Fix PBN divider calculation for UHBR rates
config: i386-buildonly-randconfig-002-20231114 (https://download.01.org/0day-ci/archive/20231114/202311140620.1gHQRb4g-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231114/202311140620.1gHQRb4g-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202311140620.1gHQRb4g-lkp@intel.com/

All errors (new ones prefixed by >>):

   ld: drivers/gpu/drm/display/drm_dp_mst_topology.o: in function `drm_dp_get_vc_payload_bw':
>> drm_dp_mst_topology.c:(.text+0x931): undefined reference to `__udivdi3'

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [Intel-gfx] [PATCH 4/4] drm/dp_mst: Fix PBN divider calculation for UHBR rates

[kernel test robot]

Hi Imre,

kernel test robot noticed the following build errors:

[auto build test ERROR on drm-tip/drm-tip]

url:    https://github.com/intel-lab-lkp/linux/commits/Imre-Deak/drm-i915-dp-Fix-UHBR-link-M-N-values/20231114-043135
base:   git://anongit.freedesktop.org/drm/drm-tip drm-tip
patch link:    https://lore.kernel.org/r/20231113201110.510724-4-imre.deak%40intel.com
patch subject: [PATCH 4/4] drm/dp_mst: Fix PBN divider calculation for UHBR rates
config: i386-randconfig-002-20231114 (https://download.01.org/0day-ci/archive/20231114/202311140621.sw31vG8M-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231114/202311140621.sw31vG8M-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202311140621.sw31vG8M-lkp@intel.com/

All errors (new ones prefixed by >>):

   ld: drivers/gpu/drm/display/drm_dp_mst_topology.o: in function `drm_dp_get_vc_payload_bw':
>> drivers/gpu/drm/display/drm_dp_mst_topology.c:3598: undefined reference to `__udivdi3'


vim +3598 drivers/gpu/drm/display/drm_dp_mst_topology.c

  3570	
  3571	/**
  3572	 * drm_dp_get_vc_payload_bw - get the VC payload BW for an MST link
  3573	 * @mgr: The &drm_dp_mst_topology_mgr to use
  3574	 * @link_rate: link rate in 10kbits/s units
  3575	 * @link_lane_count: lane count
  3576	 *
  3577	 * Calculate the total bandwidth of a MultiStream Transport link. The returned
  3578	 * value is in units of PBNs/(timeslots/1 MTP). This value can be used to
  3579	 * convert the number of PBNs required for a given stream to the number of
  3580	 * timeslots this stream requires in each MTP.
  3581	 */
  3582	int drm_dp_get_vc_payload_bw(const struct drm_dp_mst_topology_mgr *mgr,
  3583				     int link_rate, int link_lane_count)
  3584	{
  3585		int ret;
  3586	
  3587		if (link_rate == 0 || link_lane_count == 0)
  3588			drm_dbg_kms(mgr->dev, "invalid link rate/lane count: (%d / %d)\n",
  3589				    link_rate, link_lane_count);
  3590	
  3591		/* See DP v2.0 2.6.4.2, 2.7.6.3 VCPayload_Bandwidth_for_OneTimeSlotPer_MTP_Allocation */
  3592		/*
  3593		 * TODO: Return the value with a higher precision, allowing a better
  3594		 * slots per MTP allocation granularity. With the current returned
  3595		 * value +1 slot/MTP can get allocated on UHBR links.
  3596		 */
  3597		ret = mul_u32_u32(link_rate * link_lane_count,
> 3598				  drm_dp_bw_channel_coding_efficiency(drm_dp_is_uhbr_rate(link_rate))) /
  3599		      (1000000ULL * 8 * 5400);
  3600	
  3601		return ret;
  3602	}
  3603	EXPORT_SYMBOL(drm_dp_get_vc_payload_bw);
  3604	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [Intel-gfx] [PATCH 1/4] drm/i915/dp: Account for channel coding efficiency on UHBR links

[Murthy, Arun R]

> -----Original Message-----
> From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of Imre
> Deak
> Sent: Tuesday, November 14, 2023 1:41 AM
> To: intel-gfx@lists.freedesktop.org
> Subject: [Intel-gfx] [PATCH 1/4] drm/i915/dp: Account for channel coding
> efficiency on UHBR links
> 
> Apply the correct BW allocation overhead and channel coding efficiency on
> UHBR link rates, similarly to DP1.4 link rates.
> 
> Signed-off-by: Imre Deak <imre.deak@intel.com>
Reviewed-by: Arun R Murthy <arun.r.murthy@intel.com>

Thanks and Regards,
Arun R Murthy
--------------------
> ---
>  drivers/gpu/drm/i915/display/intel_display.c | 10 ----------
>  1 file changed, 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> b/drivers/gpu/drm/i915/display/intel_display.c
> index 3effafcbb411a..24aebdb715e7d 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -2398,16 +2398,6 @@ add_bw_alloc_overhead(int link_clock, int
> bw_overhead,
>  	int ch_coding_efficiency =
>  		drm_dp_bw_channel_coding_efficiency(is_uhbr);
> 
> -	/*
> -	 * TODO: adjust for actual UHBR channel coding efficiency and BW
> -	 * overhead.
> -	 */
> -	if (is_uhbr) {
> -		*data_m = pixel_data_rate;
> -		*data_n = link_data_rate * 8 / 10;
> -		return;
> -	}
> -
>  	*data_m = DIV_ROUND_UP_ULL(mul_u32_u32(pixel_data_rate,
> bw_overhead),
>  				   1000000);
>  	*data_n = DIV_ROUND_DOWN_ULL(mul_u32_u32(link_data_rate,
> ch_coding_efficiency),
> --
> 2.39.2

Re: [Intel-gfx] [PATCH 2/4] drm/i915/dp: Fix UHBR link M/N values

[Murthy, Arun R]

> -----Original Message-----
> From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of Imre
> Deak
> Sent: Tuesday, November 14, 2023 1:41 AM
> To: intel-gfx@lists.freedesktop.org
> Subject: [Intel-gfx] [PATCH 2/4] drm/i915/dp: Fix UHBR link M/N values
> 
> The link M/N ratio is the data rate / link symbol clock rate, fix things up
> accordingly. On DP 1.4 this ratio was correct as the link symbol clock rate in
> that case matched the link data rate (in bytes/sec units, the symbol size being 8
> bits), however it wasn't correct for UHBR rates where the symbol size is 32 bits.
> 
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display.c | 16 ++++++++-----
>  drivers/gpu/drm/i915/display/intel_dp.c      | 24 ++++++++++++++++++++
>  drivers/gpu/drm/i915/display/intel_dp.h      |  2 ++
>  3 files changed, 36 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> b/drivers/gpu/drm/i915/display/intel_display.c
> index 24aebdb715e7d..c059eb0170a5b 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -2411,6 +2411,7 @@ intel_link_compute_m_n(u16 bits_per_pixel, int
> nlanes,
>  		       struct intel_link_m_n *m_n)
>  {
>  	u32 data_clock = bits_per_pixel * pixel_clock;
> +	u32 link_symbol_clock = intel_dp_link_symbol_clock(link_clock);
>  	u32 data_m;
>  	u32 data_n;
> 
> @@ -2431,7 +2432,7 @@ intel_link_compute_m_n(u16 bits_per_pixel, int
> nlanes,
>  		    0x8000000);
> 
>  	compute_m_n(&m_n->link_m, &m_n->link_n,
> -		    pixel_clock, link_clock,
> +		    pixel_clock, link_symbol_clock,
>  		    0x80000);
>  }
Better if this can be moved to intel_dp.c
Also per the spec the constant N values is 0x800000
The calculation of data M has dependency with DP symbol
> 
> @@ -3943,20 +3944,23 @@ int intel_dotclock_calculate(int link_freq,
>  			     const struct intel_link_m_n *m_n)  {
>  	/*
> -	 * The calculation for the data clock is:
> +	 * The calculation for the data clock -> pixel clock is:
>  	 * pixel_clock = ((m/n)*(link_clock * nr_lanes))/bpp
>  	 * But we want to avoid losing precison if possible, so:
>  	 * pixel_clock = ((m * link_clock * nr_lanes)/(n*bpp))
>  	 *
> -	 * and the link clock is simpler:
> -	 * link_clock = (m * link_clock) / n
> +	 * and for link freq (10kbs units) -> pixel clock it is:
> +	 * link_symbol_clock = link_freq * 10 / link_symbol_size
> +	 * pixel_clock = (m * link_symbol_clock) / n
> +	 *    or for more precision:
> +	 * pixel_clock = (m * link_freq * 10) / (n * link_symbol_size)
>  	 */
> 
>  	if (!m_n->link_n)
>  		return 0;
> 
> -	return DIV_ROUND_UP_ULL(mul_u32_u32(m_n->link_m, link_freq),
> -				m_n->link_n);
> +	return DIV_ROUND_UP_ULL(mul_u32_u32(m_n->link_m, link_freq *
> 10),
> +				m_n->link_n *
> intel_dp_link_symbol_size(link_freq));
>  }
> 
>  int intel_crtc_dotclock(const struct intel_crtc_state *pipe_config) diff --git
> a/drivers/gpu/drm/i915/display/intel_dp.c
> b/drivers/gpu/drm/i915/display/intel_dp.c
> index f662d1ce5f72c..80e1e887432fa 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -132,6 +132,30 @@ bool intel_dp_is_uhbr(const struct intel_crtc_state
> *crtc_state)
>  	return intel_dp_is_uhbr_rate(crtc_state->port_clock);
>  }
> 
> +/**
> + * intel_dp_link_symbol_size - get the link symbol size for a given
> +link rate
> + * @rate: link rate in 10kbit/s units
> + *
> + * Returns the link symbol size in bits/symbol units depending on the
> +link
> + * rate -> channel coding.
> + */
> +int intel_dp_link_symbol_size(int rate) {
> +	return intel_dp_is_uhbr_rate(rate) ? 32 : 10; }
As per the spec this DP symbol is 32 for DP2.0 and 8 for DP1.4

Thanks and Regards,
Arun R Murthy
--------------------
> +
> +/**
> + * intel_dp_link_symbol_clock - convert link rate to link symbol clock
> + * @rate: link rate in 10kbit/s units
> + *
> + * Returns the link symbol clock frequency in kHz units depending on
> +the
> + * link rate and channel coding.
> + */
> +int intel_dp_link_symbol_clock(int rate) {
> +	return DIV_ROUND_CLOSEST(rate * 10,
> intel_dp_link_symbol_size(rate));
> +}
> +
>  static void intel_dp_set_default_sink_rates(struct intel_dp *intel_dp)  {
>  	intel_dp->sink_rates[0] = 162000;
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.h
> b/drivers/gpu/drm/i915/display/intel_dp.h
> index e80da67554196..64dbf8f192708 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.h
> +++ b/drivers/gpu/drm/i915/display/intel_dp.h
> @@ -82,6 +82,8 @@ bool intel_dp_has_hdmi_sink(struct intel_dp *intel_dp);
> bool intel_dp_is_edp(struct intel_dp *intel_dp);  bool intel_dp_is_uhbr_rate(int
> rate);  bool intel_dp_is_uhbr(const struct intel_crtc_state *crtc_state);
> +int intel_dp_link_symbol_size(int rate); int
> +intel_dp_link_symbol_clock(int rate);
>  bool intel_dp_is_port_edp(struct drm_i915_private *dev_priv, enum port
> port);  enum irqreturn intel_dp_hpd_pulse(struct intel_digital_port *dig_port,
>  				  bool long_hpd);
> --
> 2.39.2

Re: [Intel-gfx] [PATCH 2/4] drm/i915/dp: Fix UHBR link M/N values

[Imre Deak]

On Tue, Nov 14, 2023 at 05:29:35AM +0200, Murthy, Arun R wrote:
> 
> > -----Original Message-----
> > From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of Imre
> > Deak
> > Sent: Tuesday, November 14, 2023 1:41 AM
> > To: intel-gfx@lists.freedesktop.org
> > Subject: [Intel-gfx] [PATCH 2/4] drm/i915/dp: Fix UHBR link M/N values
> >
> > The link M/N ratio is the data rate / link symbol clock rate, fix things up
> > accordingly. On DP 1.4 this ratio was correct as the link symbol clock rate in
> > that case matched the link data rate (in bytes/sec units, the symbol size being 8
> > bits), however it wasn't correct for UHBR rates where the symbol size is 32 bits.
> >
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_display.c | 16 ++++++++-----
> >  drivers/gpu/drm/i915/display/intel_dp.c      | 24 ++++++++++++++++++++
> >  drivers/gpu/drm/i915/display/intel_dp.h      |  2 ++
> >  3 files changed, 36 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > b/drivers/gpu/drm/i915/display/intel_display.c
> > index 24aebdb715e7d..c059eb0170a5b 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -2411,6 +2411,7 @@ intel_link_compute_m_n(u16 bits_per_pixel, int
> > nlanes,
> >                      struct intel_link_m_n *m_n)
> >  {
> >       u32 data_clock = bits_per_pixel * pixel_clock;
> > +     u32 link_symbol_clock = intel_dp_link_symbol_clock(link_clock);
> >       u32 data_m;
> >       u32 data_n;
> >
> > @@ -2431,7 +2432,7 @@ intel_link_compute_m_n(u16 bits_per_pixel, int
> > nlanes,
> >                   0x8000000);
> >
> >       compute_m_n(&m_n->link_m, &m_n->link_n,
> > -                 pixel_clock, link_clock,
> > +                 pixel_clock, link_symbol_clock,
> >                   0x80000);
> >  }
> Better if this can be moved to intel_dp.c

The function is also used by non-DP outputs, so not sure. In any case
it would need to be a separate change.

> Also per the spec the constant N values is 0x800000

For the link M/N values I can't see this in the spec. It's mentioned in
the context of calculating data M/N. Changing that - if it makes sense -
should be in a separate patch.

> The calculation of data M has dependency with DP symbol
> >
> > @@ -3943,20 +3944,23 @@ int intel_dotclock_calculate(int link_freq,
> >                            const struct intel_link_m_n *m_n)  {
> >       /*
> > -      * The calculation for the data clock is:
> > +      * The calculation for the data clock -> pixel clock is:
> >        * pixel_clock = ((m/n)*(link_clock * nr_lanes))/bpp
> >        * But we want to avoid losing precison if possible, so:
> >        * pixel_clock = ((m * link_clock * nr_lanes)/(n*bpp))
> >        *
> > -      * and the link clock is simpler:
> > -      * link_clock = (m * link_clock) / n
> > +      * and for link freq (10kbs units) -> pixel clock it is:
> > +      * link_symbol_clock = link_freq * 10 / link_symbol_size
> > +      * pixel_clock = (m * link_symbol_clock) / n
> > +      *    or for more precision:
> > +      * pixel_clock = (m * link_freq * 10) / (n * link_symbol_size)
> >        */
> >
> >       if (!m_n->link_n)
> >               return 0;
> >
> > -     return DIV_ROUND_UP_ULL(mul_u32_u32(m_n->link_m, link_freq),
> > -                             m_n->link_n);
> > +     return DIV_ROUND_UP_ULL(mul_u32_u32(m_n->link_m, link_freq *
> > 10),
> > +                             m_n->link_n *
> > intel_dp_link_symbol_size(link_freq));
> >  }
> >
> >  int intel_crtc_dotclock(const struct intel_crtc_state *pipe_config) diff --git
> > a/drivers/gpu/drm/i915/display/intel_dp.c
> > b/drivers/gpu/drm/i915/display/intel_dp.c
> > index f662d1ce5f72c..80e1e887432fa 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > @@ -132,6 +132,30 @@ bool intel_dp_is_uhbr(const struct intel_crtc_state
> > *crtc_state)
> >       return intel_dp_is_uhbr_rate(crtc_state->port_clock);
> >  }
> >
> > +/**
> > + * intel_dp_link_symbol_size - get the link symbol size for a given
> > +link rate
> > + * @rate: link rate in 10kbit/s units
> > + *
> > + * Returns the link symbol size in bits/symbol units depending on the
> > +link
> > + * rate -> channel coding.
> > + */
> > +int intel_dp_link_symbol_size(int rate) {
> > +     return intel_dp_is_uhbr_rate(rate) ? 32 : 10; }
> As per the spec this DP symbol is 32 for DP2.0 and 8 for DP1.4

On DP1.4 before the 8b/10b conversion the symbol size is 8 bits, after
the conversion (which is what @rate describes and for which the symbol
size is returned for) it's 10 bits.

> 
> Thanks and Regards,
> Arun R Murthy
> --------------------
> > +
> > +/**
> > + * intel_dp_link_symbol_clock - convert link rate to link symbol clock
> > + * @rate: link rate in 10kbit/s units
> > + *
> > + * Returns the link symbol clock frequency in kHz units depending on
> > +the
> > + * link rate and channel coding.
> > + */
> > +int intel_dp_link_symbol_clock(int rate) {
> > +     return DIV_ROUND_CLOSEST(rate * 10,
> > intel_dp_link_symbol_size(rate));
> > +}
> > +
> >  static void intel_dp_set_default_sink_rates(struct intel_dp *intel_dp)  {
> >       intel_dp->sink_rates[0] = 162000;
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp.h
> > b/drivers/gpu/drm/i915/display/intel_dp.h
> > index e80da67554196..64dbf8f192708 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp.h
> > +++ b/drivers/gpu/drm/i915/display/intel_dp.h
> > @@ -82,6 +82,8 @@ bool intel_dp_has_hdmi_sink(struct intel_dp *intel_dp);
> > bool intel_dp_is_edp(struct intel_dp *intel_dp);  bool intel_dp_is_uhbr_rate(int
> > rate);  bool intel_dp_is_uhbr(const struct intel_crtc_state *crtc_state);
> > +int intel_dp_link_symbol_size(int rate); int
> > +intel_dp_link_symbol_clock(int rate);
> >  bool intel_dp_is_port_edp(struct drm_i915_private *dev_priv, enum port
> > port);  enum irqreturn intel_dp_hpd_pulse(struct intel_digital_port *dig_port,
> >                                 bool long_hpd);
> > --
> > 2.39.2
> 

Re: [Intel-gfx] [PATCH 1/4] drm/i915/dp: Account for channel coding efficiency on UHBR links

[Jani Nikula]

On Mon, 13 Nov 2023, Imre Deak <imre.deak@intel.com> wrote:
> Apply the correct BW allocation overhead and channel coding efficiency
> on UHBR link rates, similarly to DP1.4 link rates.
>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display.c | 10 ----------
>  1 file changed, 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 3effafcbb411a..24aebdb715e7d 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -2398,16 +2398,6 @@ add_bw_alloc_overhead(int link_clock, int bw_overhead,
>  	int ch_coding_efficiency =
>  		drm_dp_bw_channel_coding_efficiency(is_uhbr);

Why do we have this and intel_dp_max_data_rate() separately?

BR,
Jani.


>  
> -	/*
> -	 * TODO: adjust for actual UHBR channel coding efficiency and BW
> -	 * overhead.
> -	 */
> -	if (is_uhbr) {
> -		*data_m = pixel_data_rate;
> -		*data_n = link_data_rate * 8 / 10;
> -		return;
> -	}
> -
>  	*data_m = DIV_ROUND_UP_ULL(mul_u32_u32(pixel_data_rate, bw_overhead),
>  				   1000000);
>  	*data_n = DIV_ROUND_DOWN_ULL(mul_u32_u32(link_data_rate, ch_coding_efficiency),

-- 
Jani Nikula, Intel

Re: [Intel-gfx] [PATCH 1/4] drm/i915/dp: Account for channel coding efficiency on UHBR links

[Imre Deak]

On Tue, Nov 14, 2023 at 11:00:49AM +0200, Jani Nikula wrote:
> On Mon, 13 Nov 2023, Imre Deak <imre.deak@intel.com> wrote:
> > Apply the correct BW allocation overhead and channel coding efficiency
> > on UHBR link rates, similarly to DP1.4 link rates.
> >
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_display.c | 10 ----------
> >  1 file changed, 10 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > index 3effafcbb411a..24aebdb715e7d 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -2398,16 +2398,6 @@ add_bw_alloc_overhead(int link_clock, int bw_overhead,
> >  	int ch_coding_efficiency =
> >  		drm_dp_bw_channel_coding_efficiency(is_uhbr);
> 
> Why do we have this and intel_dp_max_data_rate() separately?

This function calculates an m/n ratio for a given pixel/data rate,
applying both the allocation overhead (FEC, SSC, etc.) and the channel
coding efficiency. intel_dp_max_data_rate() calculates a maximum data
rate applying only the channel coding efficiency.

I think intel_dp_max_data_rate() could be simplified, so the two
functions use the same channel coding efficiency to:

    DIV_ROUND_UP_ULL(mul_u32_u32(max_link_rate_kbps * max_lanes,
    				 drm_dp_bw_channel_coding_efficiency(is_uhbr)),
		     1000000ULL * 8)

--Imre

> 
> BR,
> Jani.
> 
> 
> >  
> > -	/*
> > -	 * TODO: adjust for actual UHBR channel coding efficiency and BW
> > -	 * overhead.
> > -	 */
> > -	if (is_uhbr) {
> > -		*data_m = pixel_data_rate;
> > -		*data_n = link_data_rate * 8 / 10;
> > -		return;
> > -	}
> > -
> >  	*data_m = DIV_ROUND_UP_ULL(mul_u32_u32(pixel_data_rate, bw_overhead),
> >  				   1000000);
> >  	*data_n = DIV_ROUND_DOWN_ULL(mul_u32_u32(link_data_rate, ch_coding_efficiency),
> 
> -- 
> Jani Nikula, Intel

Re: [Intel-gfx] [PATCH 2/4] drm/i915/dp: Fix UHBR link M/N values

[Murthy, Arun R]

> -----Original Message-----
> From: Deak, Imre <imre.deak@intel.com>
> Sent: Tuesday, November 14, 2023 1:13 PM
> To: Murthy, Arun R <arun.r.murthy@intel.com>
> Cc: intel-gfx@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH 2/4] drm/i915/dp: Fix UHBR link M/N values
> 
> On Tue, Nov 14, 2023 at 05:29:35AM +0200, Murthy, Arun R wrote:
> >
> > > -----Original Message-----
> > > From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf
> > > Of Imre Deak
> > > Sent: Tuesday, November 14, 2023 1:41 AM
> > > To: intel-gfx@lists.freedesktop.org
> > > Subject: [Intel-gfx] [PATCH 2/4] drm/i915/dp: Fix UHBR link M/N
> > > values
> > >
> > > The link M/N ratio is the data rate / link symbol clock rate, fix
> > > things up accordingly. On DP 1.4 this ratio was correct as the link
> > > symbol clock rate in that case matched the link data rate (in
> > > bytes/sec units, the symbol size being 8 bits), however it wasn't correct for
> UHBR rates where the symbol size is 32 bits.
> > >
> > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_display.c | 16 ++++++++-----
> > >  drivers/gpu/drm/i915/display/intel_dp.c      | 24 ++++++++++++++++++++
> > >  drivers/gpu/drm/i915/display/intel_dp.h      |  2 ++
> > >  3 files changed, 36 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > > b/drivers/gpu/drm/i915/display/intel_display.c
> > > index 24aebdb715e7d..c059eb0170a5b 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > @@ -2411,6 +2411,7 @@ intel_link_compute_m_n(u16 bits_per_pixel, int
> > > nlanes,
> > >                      struct intel_link_m_n *m_n)  {
> > >       u32 data_clock = bits_per_pixel * pixel_clock;
> > > +     u32 link_symbol_clock =
> > > + intel_dp_link_symbol_clock(link_clock);
> > >       u32 data_m;
> > >       u32 data_n;
> > >
> > > @@ -2431,7 +2432,7 @@ intel_link_compute_m_n(u16 bits_per_pixel, int
> > > nlanes,
> > >                   0x8000000);
> > >
> > >       compute_m_n(&m_n->link_m, &m_n->link_n,
> > > -                 pixel_clock, link_clock,
> > > +                 pixel_clock, link_symbol_clock,
> > >                   0x80000);
> > >  }
> > Better if this can be moved to intel_dp.c
> 
> The function is also used by non-DP outputs, so not sure. In any case it would
> need to be a separate change.
> 
> > Also per the spec the constant N values is 0x800000
> 
> For the link M/N values I can't see this in the spec. It's mentioned in the context
> of calculating data M/N. Changing that - if it makes sense - should be in a
> separate patch.

Agree!

Reviewed-by: Arun R Murthy <arun.r.murthy@intel.com>

Thanks and Regards,
Arun R Murthy
-------------------
> 
> > The calculation of data M has dependency with DP symbol
> > >
> > > @@ -3943,20 +3944,23 @@ int intel_dotclock_calculate(int link_freq,
> > >                            const struct intel_link_m_n *m_n)  {
> > >       /*
> > > -      * The calculation for the data clock is:
> > > +      * The calculation for the data clock -> pixel clock is:
> > >        * pixel_clock = ((m/n)*(link_clock * nr_lanes))/bpp
> > >        * But we want to avoid losing precison if possible, so:
> > >        * pixel_clock = ((m * link_clock * nr_lanes)/(n*bpp))
> > >        *
> > > -      * and the link clock is simpler:
> > > -      * link_clock = (m * link_clock) / n
> > > +      * and for link freq (10kbs units) -> pixel clock it is:
> > > +      * link_symbol_clock = link_freq * 10 / link_symbol_size
> > > +      * pixel_clock = (m * link_symbol_clock) / n
> > > +      *    or for more precision:
> > > +      * pixel_clock = (m * link_freq * 10) / (n * link_symbol_size)
> > >        */
> > >
> > >       if (!m_n->link_n)
> > >               return 0;
> > >
> > > -     return DIV_ROUND_UP_ULL(mul_u32_u32(m_n->link_m, link_freq),
> > > -                             m_n->link_n);
> > > +     return DIV_ROUND_UP_ULL(mul_u32_u32(m_n->link_m, link_freq *
> > > 10),
> > > +                             m_n->link_n *
> > > intel_dp_link_symbol_size(link_freq));
> > >  }
> > >
> > >  int intel_crtc_dotclock(const struct intel_crtc_state *pipe_config)
> > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> > > b/drivers/gpu/drm/i915/display/intel_dp.c
> > > index f662d1ce5f72c..80e1e887432fa 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > > @@ -132,6 +132,30 @@ bool intel_dp_is_uhbr(const struct
> > > intel_crtc_state
> > > *crtc_state)
> > >       return intel_dp_is_uhbr_rate(crtc_state->port_clock);
> > >  }
> > >
> > > +/**
> > > + * intel_dp_link_symbol_size - get the link symbol size for a given
> > > +link rate
> > > + * @rate: link rate in 10kbit/s units
> > > + *
> > > + * Returns the link symbol size in bits/symbol units depending on
> > > +the link
> > > + * rate -> channel coding.
> > > + */
> > > +int intel_dp_link_symbol_size(int rate) {
> > > +     return intel_dp_is_uhbr_rate(rate) ? 32 : 10; }
> > As per the spec this DP symbol is 32 for DP2.0 and 8 for DP1.4
> 
> On DP1.4 before the 8b/10b conversion the symbol size is 8 bits, after the
> conversion (which is what @rate describes and for which the symbol size is
> returned for) it's 10 bits.
> 
> >
> > Thanks and Regards,
> > Arun R Murthy
> > --------------------
> > > +
> > > +/**
> > > + * intel_dp_link_symbol_clock - convert link rate to link symbol
> > > +clock
> > > + * @rate: link rate in 10kbit/s units
> > > + *
> > > + * Returns the link symbol clock frequency in kHz units depending
> > > +on the
> > > + * link rate and channel coding.
> > > + */
> > > +int intel_dp_link_symbol_clock(int rate) {
> > > +     return DIV_ROUND_CLOSEST(rate * 10,
> > > intel_dp_link_symbol_size(rate));
> > > +}
> > > +
> > >  static void intel_dp_set_default_sink_rates(struct intel_dp *intel_dp)  {
> > >       intel_dp->sink_rates[0] = 162000; diff --git
> > > a/drivers/gpu/drm/i915/display/intel_dp.h
> > > b/drivers/gpu/drm/i915/display/intel_dp.h
> > > index e80da67554196..64dbf8f192708 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_dp.h
> > > +++ b/drivers/gpu/drm/i915/display/intel_dp.h
> > > @@ -82,6 +82,8 @@ bool intel_dp_has_hdmi_sink(struct intel_dp
> > > *intel_dp); bool intel_dp_is_edp(struct intel_dp *intel_dp);  bool
> > > intel_dp_is_uhbr_rate(int rate);  bool intel_dp_is_uhbr(const struct
> > > intel_crtc_state *crtc_state);
> > > +int intel_dp_link_symbol_size(int rate); int
> > > +intel_dp_link_symbol_clock(int rate);
> > >  bool intel_dp_is_port_edp(struct drm_i915_private *dev_priv, enum
> > > port port);  enum irqreturn intel_dp_hpd_pulse(struct intel_digital_port
> *dig_port,
> > >                                 bool long_hpd);
> > > --
> > > 2.39.2
> >

Re: [Intel-gfx] [PATCH 3/4] drm/i915/dp_mst: Fix PBN / MTP_TU size calculation for UHBR rates

[Imre Deak]

On Mon, Nov 13, 2023 at 10:11:09PM +0200, Imre Deak wrote:
> Atm the allocated MST PBN value is calculated from the TU size (number
> of allocated MTP slots) as
> 
>   PBN = TU * pbn_div
> 
> pbn_div being the link BW for each MTP slot. For DP 1.4 link rates this
> worked, as pbn_div there is guraranteed to be an integer number, however
> on UHBR this isn't the case. To get a PBN, TU pair where TU is a
> properly rounded-up value covering all the BW corresponding to PBN,
> calculate first PBN and from PBN the TU value.
> 
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_dp_mst.c | 10 ++++++++--
>  1 file changed, 8 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 b943dbf394a22..a32ab0b4fc9d7 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> @@ -170,6 +170,7 @@ static int intel_dp_mst_find_vcpi_slots_for_bpp(struct intel_encoder *encoder,
>  
>  	for (bpp = max_bpp; bpp >= min_bpp; bpp -= step) {
>  		struct intel_link_m_n remote_m_n;
> +		int alloc_tu;
>  		int link_bpp;
>  
>  		drm_dbg_kms(&i915->drm, "Trying bpp %d\n", bpp);
> @@ -200,9 +201,14 @@ static int intel_dp_mst_find_vcpi_slots_for_bpp(struct intel_encoder *encoder,
>  		 * crtc_state->dp_m_n.tu), provided that the driver doesn't
>  		 * enable SSC on the corresponding link.
>  		 */
> +		crtc_state->pbn = DIV_ROUND_UP_ULL(mul_u32_u32(mst_state->pbn_div * 64,
> +							       remote_m_n.data_m),
> +						   remote_m_n.data_n);

I realized this may allocate fewer PBNs than required, since the actual
pbn_div value is not an integer. Also PBN can be calculated in a more direct
way from the effective pixel data rate, so I'd like to do that instead.

I'll send a new version with the above changes.

> +
> +		alloc_tu = DIV_ROUND_UP_ULL(crtc_state->pbn, mst_state->pbn_div);


> +		drm_WARN_ON(&i915->drm, alloc_tu < remote_m_n.tu);
>  		drm_WARN_ON(&i915->drm, remote_m_n.tu < crtc_state->dp_m_n.tu);
> -		crtc_state->dp_m_n.tu = remote_m_n.tu;
> -		crtc_state->pbn = remote_m_n.tu * mst_state->pbn_div;
> +		crtc_state->dp_m_n.tu = alloc_tu;
>  
>  		slots = drm_dp_atomic_find_time_slots(state, &intel_dp->mst_mgr,
>  						      connector->port,
> -- 
> 2.39.2
> 

Re: [Intel-gfx] [PATCH 3/4] drm/i915/dp_mst: Fix PBN / MTP_TU size calculation for UHBR rates

[Murthy, Arun R]

> -----Original Message-----
> From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of Imre
> Deak
> Sent: Wednesday, November 15, 2023 7:08 PM
> To: intel-gfx@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH 3/4] drm/i915/dp_mst: Fix PBN / MTP_TU size
> calculation for UHBR rates
> 
> On Mon, Nov 13, 2023 at 10:11:09PM +0200, Imre Deak wrote:
> > Atm the allocated MST PBN value is calculated from the TU size (number
> > of allocated MTP slots) as
> >
> >   PBN = TU * pbn_div
> >
> > pbn_div being the link BW for each MTP slot. For DP 1.4 link rates
> > this worked, as pbn_div there is guraranteed to be an integer number,
> > however on UHBR this isn't the case. To get a PBN, TU pair where TU is
> > a properly rounded-up value covering all the BW corresponding to PBN,
> > calculate first PBN and from PBN the TU value.
> >
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_dp_mst.c | 10 ++++++++--
> >  1 file changed, 8 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 b943dbf394a22..a32ab0b4fc9d7 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > @@ -170,6 +170,7 @@ static int
> > intel_dp_mst_find_vcpi_slots_for_bpp(struct intel_encoder *encoder,
> >
> >  	for (bpp = max_bpp; bpp >= min_bpp; bpp -= step) {
> >  		struct intel_link_m_n remote_m_n;
> > +		int alloc_tu;
> >  		int link_bpp;
> >
> >  		drm_dbg_kms(&i915->drm, "Trying bpp %d\n", bpp); @@ -
> 200,9 +201,14
> > @@ static int intel_dp_mst_find_vcpi_slots_for_bpp(struct intel_encoder
> *encoder,
> >  		 * crtc_state->dp_m_n.tu), provided that the driver doesn't
> >  		 * enable SSC on the corresponding link.
> >  		 */
> > +		crtc_state->pbn =
> DIV_ROUND_UP_ULL(mul_u32_u32(mst_state->pbn_div * 64,
> > +
> remote_m_n.data_m),
> > +						   remote_m_n.data_n);
> 
> I realized this may allocate fewer PBNs than required, since the actual pbn_div
> value is not an integer. Also PBN can be calculated in a more direct way from
> the effective pixel data rate, so I'd like to do that instead.
> 
> I'll send a new version with the above changes.
> 
Also spec says about a constant value of 64 for TU size.

Thanks and Regards,
Arun R Murthy
-------------------
> > +
> > +		alloc_tu = DIV_ROUND_UP_ULL(crtc_state->pbn, mst_state-
> >pbn_div);
> 
> 
> > +		drm_WARN_ON(&i915->drm, alloc_tu < remote_m_n.tu);
> >  		drm_WARN_ON(&i915->drm, remote_m_n.tu < crtc_state-
> >dp_m_n.tu);
> > -		crtc_state->dp_m_n.tu = remote_m_n.tu;
> > -		crtc_state->pbn = remote_m_n.tu * mst_state->pbn_div;
> > +		crtc_state->dp_m_n.tu = alloc_tu;
> >
> >  		slots = drm_dp_atomic_find_time_slots(state, &intel_dp-
> >mst_mgr,
> >  						      connector->port,
> > --
> > 2.39.2
> >

Re: [Intel-gfx] [PATCH 1/4] drm/i915/dp: Account for channel coding efficiency on UHBR links

[Imre Deak]

On Tue, Nov 14, 2023 at 03:07:52PM +0200, Imre Deak wrote:
> On Tue, Nov 14, 2023 at 11:00:49AM +0200, Jani Nikula wrote:
> > On Mon, 13 Nov 2023, Imre Deak <imre.deak@intel.com> wrote:
> > > Apply the correct BW allocation overhead and channel coding efficiency
> > > on UHBR link rates, similarly to DP1.4 link rates.
> > >
> > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_display.c | 10 ----------
> > >  1 file changed, 10 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > > index 3effafcbb411a..24aebdb715e7d 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > @@ -2398,16 +2398,6 @@ add_bw_alloc_overhead(int link_clock, int bw_overhead,
> > >  	int ch_coding_efficiency =
> > >  		drm_dp_bw_channel_coding_efficiency(is_uhbr);
> > 
> > Why do we have this and intel_dp_max_data_rate() separately?
> 
> This function calculates an m/n ratio for a given pixel/data rate,
> applying both the allocation overhead (FEC, SSC, etc.) and the channel
> coding efficiency. intel_dp_max_data_rate() calculates a maximum data
> rate applying only the channel coding efficiency.
> 
> I think intel_dp_max_data_rate() could be simplified, so the two
> functions use the same channel coding efficiency to:
> 
>     DIV_ROUND_UP_ULL(mul_u32_u32(max_link_rate_kbps * max_lanes,
>     				 drm_dp_bw_channel_coding_efficiency(is_uhbr)),
> 		     1000000ULL * 8)

Actually, it does make sense to reuse intel_dp_max_data_rate() in 
intel_link_compute_m_n() -> add_bw_alloc_overhead(), I'll send a new
version with that (and the above simplification).
> 
> --Imre
> 
> > 
> > BR,
> > Jani.
> > 
> > 
> > >  
> > > -	/*
> > > -	 * TODO: adjust for actual UHBR channel coding efficiency and BW
> > > -	 * overhead.
> > > -	 */
> > > -	if (is_uhbr) {
> > > -		*data_m = pixel_data_rate;
> > > -		*data_n = link_data_rate * 8 / 10;
> > > -		return;
> > > -	}
> > > -
> > >  	*data_m = DIV_ROUND_UP_ULL(mul_u32_u32(pixel_data_rate, bw_overhead),
> > >  				   1000000);
> > >  	*data_n = DIV_ROUND_DOWN_ULL(mul_u32_u32(link_data_rate, ch_coding_efficiency),
> > 
> > -- 
> > Jani Nikula, Intel

Re: [Intel-gfx] [PATCH 3/4] drm/i915/dp_mst: Fix PBN / MTP_TU size calculation for UHBR rates

[Imre Deak]

On Wed, Nov 15, 2023 at 03:41:08PM +0200, Murthy, Arun R wrote:
> 
> 
> > -----Original Message-----
> > From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of Imre
> > Deak
> > Sent: Wednesday, November 15, 2023 7:08 PM
> > To: intel-gfx@lists.freedesktop.org
> > Subject: Re: [Intel-gfx] [PATCH 3/4] drm/i915/dp_mst: Fix PBN / MTP_TU size
> > calculation for UHBR rates
> >
> > On Mon, Nov 13, 2023 at 10:11:09PM +0200, Imre Deak wrote:
> > > Atm the allocated MST PBN value is calculated from the TU size (number
> > > of allocated MTP slots) as
> > >
> > >   PBN = TU * pbn_div
> > >
> > > pbn_div being the link BW for each MTP slot. For DP 1.4 link rates
> > > this worked, as pbn_div there is guraranteed to be an integer number,
> > > however on UHBR this isn't the case. To get a PBN, TU pair where TU is
> > > a properly rounded-up value covering all the BW corresponding to PBN,
> > > calculate first PBN and from PBN the TU value.
> > >
> > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_dp_mst.c | 10 ++++++++--
> > >  1 file changed, 8 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 b943dbf394a22..a32ab0b4fc9d7 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > > @@ -170,6 +170,7 @@ static int
> > > intel_dp_mst_find_vcpi_slots_for_bpp(struct intel_encoder *encoder,
> > >
> > >     for (bpp = max_bpp; bpp >= min_bpp; bpp -= step) {
> > >             struct intel_link_m_n remote_m_n;
> > > +           int alloc_tu;
> > >             int link_bpp;
> > >
> > >             drm_dbg_kms(&i915->drm, "Trying bpp %d\n", bpp); @@ -
> > 200,9 +201,14
> > > @@ static int intel_dp_mst_find_vcpi_slots_for_bpp(struct intel_encoder
> > *encoder,
> > >              * crtc_state->dp_m_n.tu), provided that the driver doesn't
> > >              * enable SSC on the corresponding link.
> > >              */
> > > +           crtc_state->pbn =
> > DIV_ROUND_UP_ULL(mul_u32_u32(mst_state->pbn_div * 64,
> > > +
> > remote_m_n.data_m),
> > > +                                              remote_m_n.data_n);
> >
> > I realized this may allocate fewer PBNs than required, since the actual pbn_div
> > value is not an integer. Also PBN can be calculated in a more direct way from
> > the effective pixel data rate, so I'd like to do that instead.
> >
> > I'll send a new version with the above changes.
> >
> Also spec says about a constant value of 64 for TU size.

I suppose you refer to WA 14013163432 (Bspec / 54369), yes we considered
this with Ville. For that one data M/N needs to be configured for full
BW utilization, that is M=TU-size N=64. It's for the case where FEC is
enabled and applies to ADLP A0-C0 only (need to check other platforms).
The corresponding issue is supposed to be fixed on ADLP D0 (which
requires enabling some HW workarounds), so not sure if/how we should
enable this SW WA.

> Thanks and Regards,
> Arun R Murthy
> -------------------
> > > +
> > > +           alloc_tu = DIV_ROUND_UP_ULL(crtc_state->pbn, mst_state-
> > >pbn_div);
> >
> >
> > > +           drm_WARN_ON(&i915->drm, alloc_tu < remote_m_n.tu);
> > >             drm_WARN_ON(&i915->drm, remote_m_n.tu < crtc_state-
> > >dp_m_n.tu);
> > > -           crtc_state->dp_m_n.tu = remote_m_n.tu;
> > > -           crtc_state->pbn = remote_m_n.tu * mst_state->pbn_div;
> > > +           crtc_state->dp_m_n.tu = alloc_tu;
> > >
> > >             slots = drm_dp_atomic_find_time_slots(state, &intel_dp-
> > >mst_mgr,
> > >                                                   connector->port,
> > > --
> > > 2.39.2
> > >

Re: [Intel-gfx] [PATCH 3/4] drm/i915/dp_mst: Fix PBN / MTP_TU size calculation for UHBR rates

[Murthy, Arun R]

> -----Original Message-----
> From: Deak, Imre <imre.deak@intel.com>
> Sent: Wednesday, November 15, 2023 7:56 PM
> To: Murthy, Arun R <arun.r.murthy@intel.com>
> Cc: intel-gfx@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH 3/4] drm/i915/dp_mst: Fix PBN / MTP_TU size
> calculation for UHBR rates
> 
> On Wed, Nov 15, 2023 at 03:41:08PM +0200, Murthy, Arun R wrote:
> >
> >
> > > -----Original Message-----
> > > From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf
> > > Of Imre Deak
> > > Sent: Wednesday, November 15, 2023 7:08 PM
> > > To: intel-gfx@lists.freedesktop.org
> > > Subject: Re: [Intel-gfx] [PATCH 3/4] drm/i915/dp_mst: Fix PBN /
> > > MTP_TU size calculation for UHBR rates
> > >
> > > On Mon, Nov 13, 2023 at 10:11:09PM +0200, Imre Deak wrote:
> > > > Atm the allocated MST PBN value is calculated from the TU size
> > > > (number of allocated MTP slots) as
> > > >
> > > >   PBN = TU * pbn_div
> > > >
> > > > pbn_div being the link BW for each MTP slot. For DP 1.4 link rates
> > > > this worked, as pbn_div there is guraranteed to be an integer
> > > > number, however on UHBR this isn't the case. To get a PBN, TU pair
> > > > where TU is a properly rounded-up value covering all the BW
> > > > corresponding to PBN, calculate first PBN and from PBN the TU value.
> > > >
> > > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/display/intel_dp_mst.c | 10 ++++++++--
> > > >  1 file changed, 8 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 b943dbf394a22..a32ab0b4fc9d7 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > > > @@ -170,6 +170,7 @@ static int
> > > > intel_dp_mst_find_vcpi_slots_for_bpp(struct intel_encoder
> > > > *encoder,
> > > >
> > > >     for (bpp = max_bpp; bpp >= min_bpp; bpp -= step) {
> > > >             struct intel_link_m_n remote_m_n;
> > > > +           int alloc_tu;
> > > >             int link_bpp;
> > > >
> > > >             drm_dbg_kms(&i915->drm, "Trying bpp %d\n", bpp); @@ -
> > > 200,9 +201,14
> > > > @@ static int intel_dp_mst_find_vcpi_slots_for_bpp(struct
> > > > intel_encoder
> > > *encoder,
> > > >              * crtc_state->dp_m_n.tu), provided that the driver doesn't
> > > >              * enable SSC on the corresponding link.
> > > >              */
> > > > +           crtc_state->pbn =
> > > DIV_ROUND_UP_ULL(mul_u32_u32(mst_state->pbn_div * 64,
> > > > +
> > > remote_m_n.data_m),
> > > > +                                              remote_m_n.data_n);
> > >
> > > I realized this may allocate fewer PBNs than required, since the
> > > actual pbn_div value is not an integer. Also PBN can be calculated
> > > in a more direct way from the effective pixel data rate, so I'd like to do that
> instead.
> > >
> > > I'll send a new version with the above changes.
> > >
> > Also spec says about a constant value of 64 for TU size.
> 
> I suppose you refer to WA 14013163432 (Bspec / 54369), yes we considered
> this with Ville. For that one data M/N needs to be configured for full BW
> utilization, that is M=TU-size N=64. It's for the case where FEC is enabled and
> applies to ADLP A0-C0 only (need to check other platforms).
> The corresponding issue is supposed to be fixed on ADLP D0 (which requires
> enabling some HW workarounds), so not sure if/how we should enable this SW
> WA.
> 
Can we have this as a TODO so that we don't forget!
With the above TODO
Reviewed-by: Arun R Murthy <arun.r.murthy@intel.com>

Thanks and Regards,,
Arun R Murthy
--------------------

> > Thanks and Regards,
> > Arun R Murthy
> > -------------------
> > > > +
> > > > +           alloc_tu = DIV_ROUND_UP_ULL(crtc_state->pbn,
> > > > + mst_state-
> > > >pbn_div);
> > >
> > >
> > > > +           drm_WARN_ON(&i915->drm, alloc_tu < remote_m_n.tu);
> > > >             drm_WARN_ON(&i915->drm, remote_m_n.tu < crtc_state-
> > > >dp_m_n.tu);
> > > > -           crtc_state->dp_m_n.tu = remote_m_n.tu;
> > > > -           crtc_state->pbn = remote_m_n.tu * mst_state->pbn_div;
> > > > +           crtc_state->dp_m_n.tu = alloc_tu;
> > > >
> > > >             slots = drm_dp_atomic_find_time_slots(state,
> > > >&intel_dp- mst_mgr,
> > > >                                                   connector->port,
> > > > --
> > > > 2.39.2
> > > >