Reviewed-by: Lyude Paul <lyude@xxxxxxxxxx>
On Mon, 2023-10-30 at 17:58 +0200, Imre Deak wrote:
> From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>
> The current code does '(bpp << 4) / 16' in the MST PBN
> calculation, but that is just the same as 'bpp' so the
> DSC codepath achieves absolutely nothing. Fix it up so that
> the fractional part of the bpp value is actually used instead
> of truncated away. 64*1006 has enough zero lsbs that we can
> just shift that down in the dividend and thus still manage
> to stick to a 32bit divisor.
>
> And while touching this, let's just make the whole thing more
> straightforward by making the passed in bpp value .4 binary
> fixed point always, instead of having to pass in different
> things based on whether DSC is enabled or not.
>
> v2:
> - Fix DSC kunit test cases.
>
> Cc: Manasi Navare <manasi.d.navare@xxxxxxxxx>
> Cc: Lyude Paul <lyude@xxxxxxxxxx>
> Cc: Harry Wentland <harry.wentland@xxxxxxx>
> Cc: David Francis <David.Francis@xxxxxxx>
> Cc: Mikita Lipski <mikita.lipski@xxxxxxx>
> Cc: Alex Deucher <alexander.deucher@xxxxxxx>
> Fixes: dc48529fb14e ("drm/dp_mst: Add PBN calculation for DSC modes")
> Reviewed-by: Lyude Paul <lyude@xxxxxxxxxx> (v1)
> Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> [Imre: Fix kunit test cases]
> Signed-off-by: Imre Deak <imre.deak@xxxxxxxxx>
> ---
> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 2 +-
> .../display/amdgpu_dm/amdgpu_dm_mst_types.c | 2 +-
> drivers/gpu/drm/display/drm_dp_mst_topology.c | 20 +++++--------------
> drivers/gpu/drm/i915/display/intel_dp_mst.c | 5 ++---
> drivers/gpu/drm/nouveau/dispnv50/disp.c | 3 +--
> .../gpu/drm/tests/drm_dp_mst_helper_test.c | 6 +++---
> include/drm/display/drm_dp_mst_helper.h | 2 +-
> 7 files changed, 14 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index 9a712791f309f..ada3773869ff0 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -6918,7 +6918,7 @@ static int dm_encoder_helper_atomic_check(struct drm_encoder *encoder,
> max_bpc);
> bpp = convert_dc_color_depth_into_bpc(color_depth) * 3;
> clock = adjusted_mode->clock;
> - dm_new_connector_state->pbn = drm_dp_calc_pbn_mode(clock, bpp, false);
> + dm_new_connector_state->pbn = drm_dp_calc_pbn_mode(clock, bpp << 4);
> }
>
> dm_new_connector_state->vcpi_slots =
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> index d3b13d362edac..9a58e1a4c5f49 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
> @@ -1642,7 +1642,7 @@ enum dc_status dm_dp_mst_is_port_support_mode(
> } else {
> /* check if mode could be supported within full_pbn */
> bpp = convert_dc_color_depth_into_bpc(stream->timing.display_color_depth) * 3;
> - pbn = drm_dp_calc_pbn_mode(stream->timing.pix_clk_100hz / 10, bpp, false);
> + pbn = drm_dp_calc_pbn_mode(stream->timing.pix_clk_100hz / 10, bpp << 4);
>
> if (pbn > aconnector->mst_output_port->full_pbn)
> return DC_FAIL_BANDWIDTH_VALIDATE;
> diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> index 0e0d0e76de065..772b00ebd57bd 100644
> --- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> @@ -4718,13 +4718,12 @@ EXPORT_SYMBOL(drm_dp_check_act_status);
>
> /**
> * drm_dp_calc_pbn_mode() - Calculate the PBN for a mode.
> - * @clock: dot clock for the mode
> - * @bpp: bpp for the mode.
> - * @dsc: DSC mode. If true, bpp has units of 1/16 of a bit per pixel
> + * @clock: dot clock
> + * @bpp: bpp as .4 binary fixed point
> *
> * This uses the formula in the spec to calculate the PBN value for a mode.
> */
> -int drm_dp_calc_pbn_mode(int clock, int bpp, bool dsc)
> +int drm_dp_calc_pbn_mode(int clock, int bpp)
> {
> /*
> * margin 5300ppm + 300ppm ~ 0.6% as per spec, factor is 1.006
> @@ -4735,18 +4734,9 @@ int drm_dp_calc_pbn_mode(int clock, int bpp, bool dsc)
> * peak_kbps *= (1006/1000)
> * peak_kbps *= (64/54)
> * peak_kbps *= 8 convert to bytes
> - *
> - * If the bpp is in units of 1/16, further divide by 16. Put this
> - * factor in the numerator rather than the denominator to avoid
> - * integer overflow
> */
> -
> - if (dsc)
> - return DIV_ROUND_UP_ULL(mul_u32_u32(clock * (bpp / 16), 64 * 1006),
> - 8 * 54 * 1000 * 1000);
> -
> - return DIV_ROUND_UP_ULL(mul_u32_u32(clock * bpp, 64 * 1006),
> - 8 * 54 * 1000 * 1000);
> + return DIV_ROUND_UP_ULL(mul_u32_u32(clock * bpp, 64 * 1006 >> 4),
> + 1000 * 8 * 54 * 1000);
> }
> EXPORT_SYMBOL(drm_dp_calc_pbn_mode);
>
> diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> index 851b312bd8449..5bf45a2a85b0e 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> @@ -106,8 +106,7 @@ static int intel_dp_mst_find_vcpi_slots_for_bpp(struct intel_encoder *encoder,
> continue;
>
> crtc_state->pbn = drm_dp_calc_pbn_mode(adjusted_mode->crtc_clock,
> - dsc ? bpp << 4 : bpp,
> - dsc);
> + bpp << 4);
>
> slots = drm_dp_atomic_find_time_slots(state, &intel_dp->mst_mgr,
> connector->port,
> @@ -975,7 +974,7 @@ intel_dp_mst_mode_valid_ctx(struct drm_connector *connector,
> return ret;
>
> if (mode_rate > max_rate || mode->clock > max_dotclk ||
> - drm_dp_calc_pbn_mode(mode->clock, min_bpp, false) > port->full_pbn) {
> + drm_dp_calc_pbn_mode(mode->clock, min_bpp << 4) > port->full_pbn) {
> *status = MODE_CLOCK_HIGH;
> return 0;
> }
> diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c b/drivers/gpu/drm/nouveau/dispnv50/disp.c
> index d2be40337b92e..153717e1df1a2 100644
> --- a/drivers/gpu/drm/nouveau/dispnv50/disp.c
> +++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c
> @@ -982,8 +982,7 @@ nv50_msto_atomic_check(struct drm_encoder *encoder,
> const int clock = crtc_state->adjusted_mode.clock;
>
> asyh->or.bpc = connector->display_info.bpc;
> - asyh->dp.pbn = drm_dp_calc_pbn_mode(clock, asyh->or.bpc * 3,
> - false);
> + asyh->dp.pbn = drm_dp_calc_pbn_mode(clock, asyh->or.bpc * 3 << 4);
> }
>
> mst_state = drm_atomic_get_mst_topology_state(state, &mstm->mgr);
> diff --git a/drivers/gpu/drm/tests/drm_dp_mst_helper_test.c b/drivers/gpu/drm/tests/drm_dp_mst_helper_test.c
> index 545beea33e8c7..e3c818dfc0e6d 100644
> --- a/drivers/gpu/drm/tests/drm_dp_mst_helper_test.c
> +++ b/drivers/gpu/drm/tests/drm_dp_mst_helper_test.c
> @@ -42,13 +42,13 @@ static const struct drm_dp_mst_calc_pbn_mode_test drm_dp_mst_calc_pbn_mode_cases
> .clock = 332880,
> .bpp = 24,
> .dsc = true,
> - .expected = 50
> + .expected = 1191
> },
> {
> .clock = 324540,
> .bpp = 24,
> .dsc = true,
> - .expected = 49
> + .expected = 1161
> },
> };
>
> @@ -56,7 +56,7 @@ static void drm_test_dp_mst_calc_pbn_mode(struct kunit *test)
> {
> const struct drm_dp_mst_calc_pbn_mode_test *params = test->param_value;
>
> - KUNIT_EXPECT_EQ(test, drm_dp_calc_pbn_mode(params->clock, params->bpp, params->dsc),
> + KUNIT_EXPECT_EQ(test, drm_dp_calc_pbn_mode(params->clock, params->bpp << 4),
> params->expected);
> }
>
> diff --git a/include/drm/display/drm_dp_mst_helper.h b/include/drm/display/drm_dp_mst_helper.h
> index 4429d3b1745b6..655862b3d2a49 100644
> --- a/include/drm/display/drm_dp_mst_helper.h
> +++ b/include/drm/display/drm_dp_mst_helper.h
> @@ -842,7 +842,7 @@ struct edid *drm_dp_mst_get_edid(struct drm_connector *connector,
> int drm_dp_get_vc_payload_bw(const struct drm_dp_mst_topology_mgr *mgr,
> int link_rate, int link_lane_count);
>
> -int drm_dp_calc_pbn_mode(int clock, int bpp, bool dsc);
> +int drm_dp_calc_pbn_mode(int clock, int bpp);
>
> void drm_dp_mst_update_slots(struct drm_dp_mst_topology_state *mst_state, uint8_t link_encoding_cap);
>
--
Cheers,
Lyude Paul (she/her)
Software Engineer at Red Hat
