Re: [PATCH] drm/mst: remove mgr parameter and debug logging from drm_dp_get_vc_payload_bw()

[Jani Nikula <jani.nikula@intel.com>]

On Thu, 02 Jan 2025, Jani Nikula <jani.nikula@intel.com> 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@intel.com>
> Cc: Lyude Paul <lyude@redhat.com>
> Reviewed-by: Imre Deak <imre.deak@intel.com>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>

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