On Thu, 02 Jan 2025, Jani Nikula <jani.nikula@xxxxxxxxx> wrote:
> The struct drm_dp_mst_topology_mgr *mgr parameter is only used for debug
> logging in case the passed in link rate or lane count are zero. There's
> no further error checking as such, and the function returns 0.
>
> There should be no case where the parameters are zero. The returned
> value is generally used as a divisor, and if we were hitting this, we'd
> be seeing division by zero.
>
> Just remove the debug logging altogether, along with the mgr parameter,
> so that the function can be used in non-MST contexts without the
> topology manager.
>
> v2: Also remove drm_dp_mst_helper_tests_init as unnecessary (Imre)
>
> Cc: Imre Deak <imre.deak@xxxxxxxxx>
> Cc: Lyude Paul <lyude@xxxxxxxxxx>
> Reviewed-by: Imre Deak <imre.deak@xxxxxxxxx>
> Signed-off-by: Jani Nikula <jani.nikula@xxxxxxxxx>
Maarten, Maxime, Thomas, ack for merging this via drm-intel along with
the rest of the series?
BR,
Jani.
> ---
> drivers/gpu/drm/display/drm_dp_mst_topology.c | 10 ++--------
> drivers/gpu/drm/i915/display/intel_dp_mst.c | 3 +--
> drivers/gpu/drm/nouveau/dispnv50/disp.c | 3 +--
> drivers/gpu/drm/tests/drm_dp_mst_helper_test.c | 17 +----------------
> include/drm/display/drm_dp_mst_helper.h | 3 +--
> 5 files changed, 6 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/gpu/drm/display/drm_dp_mst_topology.c b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> index f8cd094efa3c..06c91c5b7f7c 100644
> --- a/drivers/gpu/drm/display/drm_dp_mst_topology.c
> +++ b/drivers/gpu/drm/display/drm_dp_mst_topology.c
> @@ -3572,8 +3572,7 @@ static int drm_dp_send_up_ack_reply(struct drm_dp_mst_topology_mgr *mgr,
> }
>
> /**
> - * drm_dp_get_vc_payload_bw - get the VC payload BW for an MST link
> - * @mgr: The &drm_dp_mst_topology_mgr to use
> + * drm_dp_get_vc_payload_bw - get the VC payload BW for an MTP link
> * @link_rate: link rate in 10kbits/s units
> * @link_lane_count: lane count
> *
> @@ -3584,17 +3583,12 @@ static int drm_dp_send_up_ack_reply(struct drm_dp_mst_topology_mgr *mgr,
> *
> * Returns the BW / timeslot value in 20.12 fixed point format.
> */
> -fixed20_12 drm_dp_get_vc_payload_bw(const struct drm_dp_mst_topology_mgr *mgr,
> - int link_rate, int link_lane_count)
> +fixed20_12 drm_dp_get_vc_payload_bw(int link_rate, int link_lane_count)
> {
> int ch_coding_efficiency =
> drm_dp_bw_channel_coding_efficiency(drm_dp_is_uhbr_rate(link_rate));
> fixed20_12 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, 2.7.6.3 VCPayload_Bandwidth_for_OneTimeSlotPer_MTP_Allocation */
> ret.full = DIV_ROUND_DOWN_ULL(mul_u32_u32(link_rate * link_lane_count,
> ch_coding_efficiency),
> diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> index fffd199999e0..ca091ed291d5 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> @@ -244,8 +244,7 @@ static int mst_stream_find_vcpi_slots_for_bpp(struct intel_dp *intel_dp,
> crtc_state->fec_enable = !intel_dp_is_uhbr(crtc_state);
> }
>
> - mst_state->pbn_div = drm_dp_get_vc_payload_bw(&intel_dp->mst_mgr,
> - crtc_state->port_clock,
> + mst_state->pbn_div = drm_dp_get_vc_payload_bw(crtc_state->port_clock,
> crtc_state->lane_count);
>
> max_dpt_bpp = intel_dp_mst_max_dpt_bpp(crtc_state, dsc);
> diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c b/drivers/gpu/drm/nouveau/dispnv50/disp.c
> index 8097249612bc..62d72b7a8d04 100644
> --- a/drivers/gpu/drm/nouveau/dispnv50/disp.c
> +++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c
> @@ -992,8 +992,7 @@ nv50_msto_atomic_check(struct drm_encoder *encoder,
> if (!mst_state->pbn_div.full) {
> struct nouveau_encoder *outp = mstc->mstm->outp;
>
> - mst_state->pbn_div = drm_dp_get_vc_payload_bw(&mstm->mgr,
> - outp->dp.link_bw, outp->dp.link_nr);
> + mst_state->pbn_div = drm_dp_get_vc_payload_bw(outp->dp.link_bw, outp->dp.link_nr);
> }
>
> slots = drm_dp_atomic_find_time_slots(state, &mstm->mgr, mstc->port, asyh->dp.pbn);
> 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 89cd9e4f4d32..9e0e2fb65944 100644
> --- a/drivers/gpu/drm/tests/drm_dp_mst_helper_test.c
> +++ b/drivers/gpu/drm/tests/drm_dp_mst_helper_test.c
> @@ -199,10 +199,8 @@ static const struct drm_dp_mst_calc_pbn_div_test drm_dp_mst_calc_pbn_div_dp1_4_c
> static void drm_test_dp_mst_calc_pbn_div(struct kunit *test)
> {
> const struct drm_dp_mst_calc_pbn_div_test *params = test->param_value;
> - /* mgr->dev is only needed by drm_dbg_kms(), but it's not called for the test cases. */
> - struct drm_dp_mst_topology_mgr *mgr = test->priv;
>
> - KUNIT_EXPECT_EQ(test, drm_dp_get_vc_payload_bw(mgr, params->link_rate, params->lane_count).full,
> + KUNIT_EXPECT_EQ(test, drm_dp_get_vc_payload_bw(params->link_rate, params->lane_count).full,
> params->expected.full);
> }
>
> @@ -568,21 +566,8 @@ static struct kunit_case drm_dp_mst_helper_tests[] = {
> { }
> };
>
> -static int drm_dp_mst_helper_tests_init(struct kunit *test)
> -{
> - struct drm_dp_mst_topology_mgr *mgr;
> -
> - mgr = kunit_kzalloc(test, sizeof(*mgr), GFP_KERNEL);
> - KUNIT_ASSERT_NOT_ERR_OR_NULL(test, mgr);
> -
> - test->priv = mgr;
> -
> - return 0;
> -}
> -
> static struct kunit_suite drm_dp_mst_helper_test_suite = {
> .name = "drm_dp_mst_helper",
> - .init = drm_dp_mst_helper_tests_init,
> .test_cases = drm_dp_mst_helper_tests,
> };
>
> diff --git a/include/drm/display/drm_dp_mst_helper.h b/include/drm/display/drm_dp_mst_helper.h
> index a80ba457a858..e39de161c938 100644
> --- a/include/drm/display/drm_dp_mst_helper.h
> +++ b/include/drm/display/drm_dp_mst_helper.h
> @@ -867,8 +867,7 @@ struct edid *drm_dp_mst_get_edid(struct drm_connector *connector,
> struct drm_dp_mst_topology_mgr *mgr,
> struct drm_dp_mst_port *port);
>
> -fixed20_12 drm_dp_get_vc_payload_bw(const struct drm_dp_mst_topology_mgr *mgr,
> - int link_rate, int link_lane_count);
> +fixed20_12 drm_dp_get_vc_payload_bw(int link_rate, int link_lane_count);
>
> int drm_dp_calc_pbn_mode(int clock, int bpp);
--
Jani Nikula, Intel
