| Message ID | 20221101094222.22091-7-stanislav.lisovskiy@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add DP MST DSC support to i915 | expand |
Hi Stanislav,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on drm-tip/drm-tip]
[also build test WARNING on drm/drm-next linus/master v6.1-rc3 next-20221101]
[cannot apply to drm-intel/for-linux-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Stanislav-Lisovskiy/Add-DP-MST-DSC-support-to-i915/20221101-174550
base: git://anongit.freedesktop.org/drm/drm-tip drm-tip
patch link: https://lore.kernel.org/r/20221101094222.22091-7-stanislav.lisovskiy%40intel.com
patch subject: [PATCH 6/6] drm/i915: Bpp/timeslot calculation fixes for DP MST DSC
config: i386-randconfig-a004
compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project f28c006a5895fc0e329fe15fead81e37457cb1d1)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/effdb0df885df736ad89da1a602dc43f82598408
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Stanislav-Lisovskiy/Add-DP-MST-DSC-support-to-i915/20221101-174550
git checkout effdb0df885df736ad89da1a602dc43f82598408
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 SHELL=/bin/bash drivers/gpu/drm/i915/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
>> drivers/gpu/drm/i915/display/intel_dp.c:1543:7: warning: variable 'dsc_max_output_bpp' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
if (compute_pipe_bpp) {
^~~~~~~~~~~~~~~~
drivers/gpu/drm/i915/display/intel_dp.c:1559:9: note: uninitialized use occurs here
if ((!dsc_max_output_bpp && compute_pipe_bpp) || !dsc_dp_slice_count) {
^~~~~~~~~~~~~~~~~~
drivers/gpu/drm/i915/display/intel_dp.c:1543:3: note: remove the 'if' if its condition is always true
if (compute_pipe_bpp) {
^~~~~~~~~~~~~~~~~~~~~~
drivers/gpu/drm/i915/display/intel_dp.c:1540:25: note: initialize the variable 'dsc_max_output_bpp' to silence this warning
u16 dsc_max_output_bpp;
^
= 0
1 warning generated.
vim +1543 drivers/gpu/drm/i915/display/intel_dp.c
1485
1486 int intel_dp_dsc_compute_config(struct intel_dp *intel_dp,
1487 struct intel_crtc_state *pipe_config,
1488 struct drm_connector_state *conn_state,
1489 struct link_config_limits *limits,
1490 int timeslots,
1491 bool compute_pipe_bpp)
1492 {
1493 struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
1494 struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev);
1495 const struct drm_display_mode *adjusted_mode =
1496 &pipe_config->hw.adjusted_mode;
1497 int pipe_bpp;
1498 int ret;
1499
1500 pipe_config->fec_enable = !intel_dp_is_edp(intel_dp) &&
1501 intel_dp_supports_fec(intel_dp, pipe_config);
1502
1503 if (!intel_dp_supports_dsc(intel_dp, pipe_config))
1504 return -EINVAL;
1505
1506 if (compute_pipe_bpp)
1507 pipe_bpp = intel_dp_dsc_compute_bpp(intel_dp, conn_state->max_requested_bpc);
1508 else
1509 pipe_bpp = pipe_config->pipe_bpp;
1510
1511 if (intel_dp->force_dsc_bpc) {
1512 pipe_bpp = intel_dp->force_dsc_bpc * 3;
1513 drm_dbg_kms(&dev_priv->drm, "Input DSC BPP forced to %d", pipe_bpp);
1514 }
1515
1516 /* Min Input BPC for ICL+ is 8 */
1517 if (pipe_bpp < 8 * 3) {
1518 drm_dbg_kms(&dev_priv->drm,
1519 "No DSC support for less than 8bpc\n");
1520 return -EINVAL;
1521 }
1522
1523 /*
1524 * For now enable DSC for max bpp, max link rate, max lane count.
1525 * Optimize this later for the minimum possible link rate/lane count
1526 * with DSC enabled for the requested mode.
1527 */
1528 pipe_config->pipe_bpp = pipe_bpp;
1529 pipe_config->port_clock = limits->max_rate;
1530 pipe_config->lane_count = limits->max_lane_count;
1531
1532 if (intel_dp_is_edp(intel_dp)) {
1533 pipe_config->dsc.compressed_bpp =
1534 min_t(u16, drm_edp_dsc_sink_output_bpp(intel_dp->dsc_dpcd) >> 4,
1535 pipe_config->pipe_bpp);
1536 pipe_config->dsc.slice_count =
1537 drm_dp_dsc_sink_max_slice_count(intel_dp->dsc_dpcd,
1538 true);
1539 } else {
1540 u16 dsc_max_output_bpp;
1541 u8 dsc_dp_slice_count;
1542
> 1543 if (compute_pipe_bpp) {
1544 dsc_max_output_bpp =
1545 intel_dp_dsc_get_output_bpp(dev_priv,
1546 pipe_config->port_clock,
1547 pipe_config->lane_count,
1548 adjusted_mode->crtc_clock,
1549 adjusted_mode->crtc_hdisplay,
1550 pipe_config->bigjoiner_pipes,
1551 pipe_bpp,
1552 timeslots);
1553 }
1554 dsc_dp_slice_count =
1555 intel_dp_dsc_get_slice_count(intel_dp,
1556 adjusted_mode->crtc_clock,
1557 adjusted_mode->crtc_hdisplay,
1558 pipe_config->bigjoiner_pipes);
1559 if ((!dsc_max_output_bpp && compute_pipe_bpp) || !dsc_dp_slice_count) {
1560 drm_dbg_kms(&dev_priv->drm,
1561 "Compressed BPP/Slice Count not supported\n");
1562 return -EINVAL;
1563 }
1564 if (compute_pipe_bpp)
1565 pipe_config->dsc.compressed_bpp = min_t(u16,
1566 dsc_max_output_bpp >> 4,
1567 pipe_config->pipe_bpp);
1568 pipe_config->dsc.slice_count = dsc_dp_slice_count;
1569 drm_dbg_kms(&dev_priv->drm, "DSC: compressed bpp %d slice count %d\n",
1570 pipe_config->dsc.compressed_bpp,
1571 pipe_config->dsc.slice_count);
1572 }
1573 /*
1574 * VDSC engine operates at 1 Pixel per clock, so if peak pixel rate
1575 * is greater than the maximum Cdclock and if slice count is even
1576 * then we need to use 2 VDSC instances.
1577 */
1578 if (adjusted_mode->crtc_clock > dev_priv->display.cdclk.max_cdclk_freq ||
1579 pipe_config->bigjoiner_pipes) {
1580 if (pipe_config->dsc.slice_count > 1) {
1581 pipe_config->dsc.dsc_split = true;
1582 } else {
1583 drm_dbg_kms(&dev_priv->drm,
1584 "Cannot split stream to use 2 VDSC instances\n");
1585 return -EINVAL;
1586 }
1587 }
1588
1589 ret = intel_dp_dsc_compute_params(&dig_port->base, pipe_config);
1590 if (ret < 0) {
1591 drm_dbg_kms(&dev_priv->drm,
1592 "Cannot compute valid DSC parameters for Input Bpp = %d "
1593 "Compressed BPP = %d\n",
1594 pipe_config->pipe_bpp,
1595 pipe_config->dsc.compressed_bpp);
1596 return ret;
1597 }
1598
1599 pipe_config->dsc.compression_enable = true;
1600 drm_dbg_kms(&dev_priv->drm, "DP DSC computed with Input Bpp = %d "
1601 "Compressed Bpp = %d Slice Count = %d\n",
1602 pipe_config->pipe_bpp,
1603 pipe_config->dsc.compressed_bpp,
1604 pipe_config->dsc.slice_count);
1605
1606 return 0;
1607 }
1608
diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c index 71e08e665065..706fa4f7ea62 100644 --- a/drivers/gpu/drm/i915/display/intel_dp.c +++ b/drivers/gpu/drm/i915/display/intel_dp.c @@ -717,9 +717,14 @@ u16 intel_dp_dsc_get_output_bpp(struct drm_i915_private *i915, * for SST -> TimeSlotsPerMTP is 1, * for MST -> TimeSlotsPerMTP has to be calculated */ - bits_per_pixel = (link_clock * lane_count * 8) * timeslots / - intel_dp_mode_to_fec_clock(mode_clock); - drm_dbg_kms(&i915->drm, "Max link bpp: %u\n", bits_per_pixel); + bits_per_pixel = DIV_ROUND_UP((link_clock * lane_count) * timeslots, + intel_dp_mode_to_fec_clock(mode_clock) * 8); + + drm_dbg_kms(&i915->drm, "Max link bpp is %u for %u timeslots " + "total bw %u pixel clock %u\n", + bits_per_pixel, timeslots, + (link_clock * lane_count * 8), + intel_dp_mode_to_fec_clock(mode_clock)); /* Small Joiner Check: output bpp <= joiner RAM (bits) / Horiz. width */ max_bpp_small_joiner_ram = small_joiner_ram_size_bits(i915) / @@ -1048,7 +1053,7 @@ intel_dp_mode_valid(struct drm_connector *_connector, target_clock, mode->hdisplay, bigjoiner, - pipe_bpp, 1) >> 4; + pipe_bpp, 64) >> 4; dsc_slice_count = intel_dp_dsc_get_slice_count(intel_dp, target_clock, @@ -1482,7 +1487,8 @@ int intel_dp_dsc_compute_config(struct intel_dp *intel_dp, struct intel_crtc_state *pipe_config, struct drm_connector_state *conn_state, struct link_config_limits *limits, - int timeslots) + int timeslots, + bool compute_pipe_bpp) { struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp); struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev); @@ -1497,7 +1503,10 @@ int intel_dp_dsc_compute_config(struct intel_dp *intel_dp, if (!intel_dp_supports_dsc(intel_dp, pipe_config)) return -EINVAL; - pipe_bpp = intel_dp_dsc_compute_bpp(intel_dp, conn_state->max_requested_bpc); + if (compute_pipe_bpp) + pipe_bpp = intel_dp_dsc_compute_bpp(intel_dp, conn_state->max_requested_bpc); + else + pipe_bpp = pipe_config->pipe_bpp; if (intel_dp->force_dsc_bpc) { pipe_bpp = intel_dp->force_dsc_bpc * 3; @@ -1531,28 +1540,31 @@ int intel_dp_dsc_compute_config(struct intel_dp *intel_dp, u16 dsc_max_output_bpp; u8 dsc_dp_slice_count; - dsc_max_output_bpp = - intel_dp_dsc_get_output_bpp(dev_priv, - pipe_config->port_clock, - pipe_config->lane_count, - adjusted_mode->crtc_clock, - adjusted_mode->crtc_hdisplay, - pipe_config->bigjoiner_pipes, - pipe_bpp, - timeslots); + if (compute_pipe_bpp) { + dsc_max_output_bpp = + intel_dp_dsc_get_output_bpp(dev_priv, + pipe_config->port_clock, + pipe_config->lane_count, + adjusted_mode->crtc_clock, + adjusted_mode->crtc_hdisplay, + pipe_config->bigjoiner_pipes, + pipe_bpp, + timeslots); + } dsc_dp_slice_count = intel_dp_dsc_get_slice_count(intel_dp, adjusted_mode->crtc_clock, adjusted_mode->crtc_hdisplay, pipe_config->bigjoiner_pipes); - if (!dsc_max_output_bpp || !dsc_dp_slice_count) { + if ((!dsc_max_output_bpp && compute_pipe_bpp) || !dsc_dp_slice_count) { drm_dbg_kms(&dev_priv->drm, "Compressed BPP/Slice Count not supported\n"); return -EINVAL; } - pipe_config->dsc.compressed_bpp = min_t(u16, - dsc_max_output_bpp >> 4, - pipe_config->pipe_bpp); + if (compute_pipe_bpp) + pipe_config->dsc.compressed_bpp = min_t(u16, + dsc_max_output_bpp >> 4, + pipe_config->pipe_bpp); pipe_config->dsc.slice_count = dsc_dp_slice_count; drm_dbg_kms(&dev_priv->drm, "DSC: compressed bpp %d slice count %d\n", pipe_config->dsc.compressed_bpp, @@ -1660,7 +1672,7 @@ intel_dp_compute_link_config(struct intel_encoder *encoder, str_yes_no(ret), str_yes_no(joiner_needs_dsc), str_yes_no(intel_dp->force_dsc_en)); ret = intel_dp_dsc_compute_config(intel_dp, pipe_config, - conn_state, &limits, 1); + conn_state, &limits, 1, true); if (ret < 0) return ret; } diff --git a/drivers/gpu/drm/i915/display/intel_dp.h b/drivers/gpu/drm/i915/display/intel_dp.h index 0fe10d93b75c..75098001685a 100644 --- a/drivers/gpu/drm/i915/display/intel_dp.h +++ b/drivers/gpu/drm/i915/display/intel_dp.h @@ -60,7 +60,8 @@ int intel_dp_dsc_compute_config(struct intel_dp *intel_dp, struct intel_crtc_state *pipe_config, struct drm_connector_state *conn_state, struct link_config_limits *limits, - int timeslots); + int timeslots, + bool recompute_pipe_bpp); bool intel_dp_is_edp(struct intel_dp *intel_dp); bool intel_dp_is_uhbr(const struct intel_crtc_state *crtc_state); bool intel_dp_is_port_edp(struct drm_i915_private *dev_priv, enum port port); diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c index 61b2bd504e80..5b081659ca7a 100644 --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c @@ -80,12 +80,12 @@ static int intel_dp_mst_find_vcpi_slots_for_bpp(struct intel_encoder *encoder, } for (bpp = max_bpp; bpp >= min_bpp; bpp -= step) { - crtc_state->pipe_bpp = bpp; - crtc_state->pbn = drm_dp_calc_pbn_mode(adjusted_mode->crtc_clock, - dsc ? bpp << 4 : crtc_state->pipe_bpp, + dsc ? bpp << 4 : bpp, dsc); + drm_dbg_kms(&i915->drm, "Trying bpp %d\n", bpp); + slots = drm_dp_atomic_find_time_slots(state, &intel_dp->mst_mgr, connector->port, crtc_state->pbn); @@ -110,6 +110,13 @@ static int intel_dp_mst_find_vcpi_slots_for_bpp(struct intel_encoder *encoder, if (slots < 0) drm_dbg_kms(&i915->drm, "failed finding vcpi slots:%d\n", slots); + else { + if (!dsc) + crtc_state->pipe_bpp = bpp; + else + crtc_state->dsc.compressed_bpp = bpp; + drm_dbg_kms(&i915->drm, "Got %d slots for pipe bpp %d dsc %d\n", slots, bpp, dsc); + } return slots; } @@ -157,8 +164,10 @@ static int intel_dp_dsc_mst_compute_link_config(struct intel_encoder *encoder, int slots = -EINVAL; int i, num_bpc; u8 dsc_bpc[3] = {0}; - int min_bpp, max_bpp; + int min_bpp, max_bpp, sink_min_bpp, sink_max_bpp; u8 dsc_max_bpc; + bool need_timeslot_recalc = false; + u32 last_compressed_bpp; /* Max DSC Input BPC for ICL is 10 and for TGL+ is 12 */ if (DISPLAY_VER(i915) >= 12) @@ -171,14 +180,26 @@ static int intel_dp_dsc_mst_compute_link_config(struct intel_encoder *encoder, num_bpc = drm_dp_dsc_sink_supported_input_bpcs(intel_dp->dsc_dpcd, dsc_bpc); - for (i = 0; i < num_bpc; i++) { - if (max_bpp >= dsc_bpc[i] * 3) - if (min_bpp > dsc_bpc[i] * 3) - min_bpp = dsc_bpc[i] * 3; + + drm_dbg_kms(&i915->drm, "DSC Source supported min bpp %d max bpp %d\n", + min_bpp, max_bpp); + + sink_min_bpp = sink_max_bpp = dsc_bpc[0] * 3; + for (i = 1; i < num_bpc; i++) { + if (sink_min_bpp > dsc_bpc[i] * 3) + sink_min_bpp = dsc_bpc[i] * 3; + if (sink_max_bpp < dsc_bpc[i] * 3) + sink_max_bpp = dsc_bpc[i] * 3; } drm_dbg_kms(&i915->drm, "DSC Sink supported min bpp %d max bpp %d\n", - min_bpp, max_bpp); + sink_min_bpp, sink_max_bpp); + + if (min_bpp < sink_min_bpp) + min_bpp = sink_min_bpp; + + if (max_bpp > sink_max_bpp) + max_bpp = sink_max_bpp; slots = intel_dp_mst_find_vcpi_slots_for_bpp(encoder, crtc_state, max_bpp, min_bpp, limits, @@ -187,6 +208,28 @@ static int intel_dp_dsc_mst_compute_link_config(struct intel_encoder *encoder, if (slots < 0) return slots; + last_compressed_bpp = crtc_state->dsc.compressed_bpp; + + crtc_state->dsc.compressed_bpp = intel_dp_dsc_nearest_vesa_bpp(i915, + last_compressed_bpp, + crtc_state->pipe_bpp); + + if (crtc_state->dsc.compressed_bpp != last_compressed_bpp) + need_timeslot_recalc = true; + + /* + * Apparently some MST hubs dislike if vcpi slots are not matching precisely + * the actual compressed bpp we use. + */ + if (need_timeslot_recalc) { + slots = intel_dp_mst_find_vcpi_slots_for_bpp(encoder, crtc_state, + crtc_state->dsc.compressed_bpp, + crtc_state->dsc.compressed_bpp, + limits, conn_state, 2 * 3, true); + if (slots < 0) + return slots; + } + intel_link_compute_m_n(crtc_state->pipe_bpp, crtc_state->lane_count, adjusted_mode->crtc_clock, @@ -293,7 +336,7 @@ static int intel_dp_mst_compute_config(struct intel_encoder *encoder, ret = intel_dp_dsc_compute_config(intel_dp, pipe_config, conn_state, &limits, - pipe_config->dp_m_n.tu); + pipe_config->dp_m_n.tu, false); } if (ret) @@ -868,7 +911,7 @@ intel_dp_mst_mode_valid_ctx(struct drm_connector *connector, target_clock, mode->hdisplay, bigjoiner, - pipe_bpp, 1) >> 4; + pipe_bpp, 64) >> 4; dsc_slice_count = intel_dp_dsc_get_slice_count(intel_dp, target_clock,
Fix intel_dp_dsc_compute_config, previously timeslots parameter was used in fact not as a timeslots, but more like a ratio timeslots/64, which of course didn't have any effect for SST DSC, but causes now issues for MST DSC. Secondly we need to calculate pipe_bpp using intel_dp_dsc_compute_bpp only for SST DSC case, while for MST case it has been calculated earlier already with intel_dp_dsc_mst_compute_link_config. Third we also were wrongly determining sink min bpp/max bpp, those limites should be intersected with our limits to find common acceptable bpp's, plus on top of that we should align those with VESA bpps and only then calculate required timeslots amount. Some MST hubs started to work only after third change was made. Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> --- drivers/gpu/drm/i915/display/intel_dp.c | 52 ++++++++++------- drivers/gpu/drm/i915/display/intel_dp.h | 3 +- drivers/gpu/drm/i915/display/intel_dp_mst.c | 65 +++++++++++++++++---- 3 files changed, 88 insertions(+), 32 deletions(-)