Re: [PATCH v7 13/35] drm/msm/dp: add support for MST channel slot allocation

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] drm/msm/dp: ts_enum initialization inflates slot count for exact integers
- [Medium] drm/msm/dp: y_frac_enum rounds up to 256 causing register truncation
--

commit 70d204acda89b0781eca4707fd51efbc7d294508
Author: Abhinav Kumar <quic_abhinavk@quicinc.com>

drm/msm/dp: add support for MST channel slot allocation
    
DP MST streams share 64 MTP slots in a time-multiplexed manner. Add
support for calculating the rate governor, slot allocation, and slot
reservation in the DP controller.

> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> index 929e0f94f8ddb..02732bd4efa1b 100644
> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c

[ ... ]

> @@ -2701,6 +2741,117 @@ static void msm_dp_ctrl_config_msa(struct msm_dp_ctrl_private *ctrl,

[ ... ]

> +	ts_enum = 256 * lanes;
> +	ts_denom = drm_fixp_from_fraction(256 * lanes, 1);
> +	ts_int = drm_fixp2int(target_sc_fixp);
> +
> +	temp = drm_fixp2int_ceil(raw_target_sc);
> +	if (temp != ts_int) {
> +		temp = drm_fixp_from_fraction(ts_int, 1);
> +		temp1 = raw_target_sc - temp;
> +		temp2 = drm_fixp_mul(temp1, ts_denom);
> +		ts_enum = drm_fixp2int(temp2);
> +	}
> +
> +	/* target_strm_sym */
> +	ts_int_fixp = drm_fixp_from_fraction(ts_int, 1);
> +	ts_frac_fixp = drm_fixp_from_fraction(ts_enum, drm_fixp2int(ts_denom));
> +	temp = ts_int_fixp + ts_frac_fixp;

[Severity: Medium]
Does this initialization inflate the target slot count when raw_target_sc
evaluates to an exact integer?

Since ts_enum is initialized to 256 * lanes, it means the default fraction
is 1.0. When raw_target_sc is an exact integer, the condition temp != ts_int
evaluates to false, causing the recalculation of ts_enum to be skipped.

This leads to ts_frac_fixp incorrectly evaluating to 1.0 (instead of 0.0),
which mistakenly adds an extra symbol per MTP to target_strm_sym and
artificially inflates the required slot count.

> +	temp1 = drm_fixp_from_fraction(lanes, 1);
> +	target_strm_sym = drm_fixp_mul(temp, temp1);
> +
> +	/* x_int */
> +	x_int = drm_fixp2int(target_strm_sym);
> +
> +	/* y_enum_frac */
> +	temp = drm_fixp_from_fraction(x_int, 1);
> +	temp1 = target_strm_sym - temp;
> +	temp2 = drm_fixp_from_fraction(256, 1);
> +	y_frac_enum_fixp = drm_fixp_mul(temp1, temp2);
> +
> +	temp1 = drm_fixp2int(y_frac_enum_fixp);
> +	temp2 = drm_fixp2int_ceil(y_frac_enum_fixp);
> +
> +	y_frac_enum = (u32)((temp1 == temp2) ? temp1 : temp1 + 1);

[Severity: Medium]
Can this rounding logic overflow the fraction register and truncate it?

If the fractional part of target_strm_sym is very close to 1.0 
(e.g., 0.999), the ceiling calculation yields 256. When packed into 
the hardware register via rg = y_frac_enum | (x_int << 16) in
msm_dp_ctrl_update_rg(), the value 256 is represented as 0x100.

If the DP controller's y_frac_enum field only accepts an 8-bit fraction,
writing 0x100 could overflow the field and truncate it to 0 without
carrying the increment over to x_int, resulting in a subtle bandwidth
deficit and potential display underflows.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260929-msm-dp-mst-v7-0-4b81473185e7@oss.qualcomm.com?part=13