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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Calling a sleeping function (`msleep`) from an atomic commit callback violates DRM display guidelines.
- [Medium] Incorrect initialization of `ts_enum` causes an off-by-one error, artificially increasing the target symbol count when `raw_target_sc` is an exact integer.
- [Medium] The fixed-point math intended to quantize `raw_target_sc` is a mathematical no-op, defeating its documented purpose.
- [Medium] Potential division by zero kernel panic if DP link parameters (`lanes` or `lclk`) are zero.
--

commit 104a5d3e2340f37c5f0003fb078fcd9efe33ba4d
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 109a841675997..73aabd219125d 100644
> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c

[ ... ]

> @@ -2675,6 +2715,117 @@ static void msm_dp_ctrl_config_msa(struct msm_dp_ctrl_private *ctrl,
>  	msm_dp_write_stream_link(ctrl, panel->stream_id, REG_DP_SOFTWARE_NVID, nvid);
>  }
>  
> +/*
> + * Calculate MST Rate Governor parameters x_int and y_frac_enum.
> + *
> + * The RG paces symbol delivery per MTP via: M = x_int + y_frac_enum/256

[ ... ]

> +static void msm_dp_ctrl_mst_calculate_rg(struct msm_dp_ctrl_private *ctrl,
> +					 struct msm_dp_panel *panel,
> +					 u32 *p_x_int, u32 *p_y_frac_enum)
> +{
> +	u64 min_slot_cnt, max_slot_cnt;

[ ... ]

> +	lclk = ctrl->link->link_params.rate;
> +
> +	/* min_slot_cnt */
> +	numerator = pclk * bpp * 64 * 1000;
> +	denominator = lclk * lanes * 8 * 1000;
> +	min_slot_cnt = drm_fixp_from_fraction(numerator, denominator);

[Severity: Medium]
Is there a risk of a division by zero kernel panic here if the DP link
parameters fall back to zero?

In msm_dp_ctrl_mst_calculate_rg(), the denominator is computed directly
from lclk and lanes. If link training fails completely or a buggy DP sink
causes the driver to fall back to a state with zero lanes or zero link clock,
passing this zero denominator to drm_fixp_from_fraction() could trigger a
panic inside div64_u64_rem().

Should these link parameters be validated before attempting this division?

> +
> +	/* max_slot_cnt */
> +	numerator = pbn * 54 * 1000;
> +	denominator = lclk * lanes;
> +	max_slot_cnt = drm_fixp_from_fraction(numerator, denominator);
> +
> +	/* raw_target_sc */
> +	numerator = max_slot_cnt + min_slot_cnt;
> +	denominator = drm_fixp_from_fraction(2, 1);
> +	raw_target_sc = drm_fixp_div(numerator, denominator);
> +
> +	/* target_sc */
> +	temp = drm_fixp_from_fraction(256 * lanes, 1);
> +	numerator = drm_fixp_mul(raw_target_sc, temp);
> +	denominator = drm_fixp_from_fraction(256 * lanes, 1);
> +	target_sc_fixp = drm_fixp_div(numerator, denominator);

[Severity: Medium]
Does this fixed-point math actually quantize the target symbol count as
intended?

The code computes target_sc_fixp by multiplying raw_target_sc by
(256 * lanes) and then dividing it by the exact same fixed-point constant.
Because drm_fixp operations preserve the fixed-point fractional precision and
do not implicitly coerce to integers, this division restores the unquantized
raw value, acting as a mathematical no-op.

To correctly quantize raw_target_sc into 1/(256*lanes) steps, does the
numerator need to be converted to an integer before the division?

> +
> +	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) {

[Severity: Medium]
Will this initialization of ts_enum cause an off-by-one error when
raw_target_sc evaluates to an exact integer?

If raw_target_sc is an exact integer, temp != ts_int evaluates to false,
skipping the recalculation block. Because ts_enum was initially set to
256 * lanes instead of 0, ts_frac_fixp subsequently evaluates to exactly 1.0
in fixed-point space.

This extraneous 1.0 adds an extra target slot, which when multiplied by lanes,
artificially increases the final target_strm_sym count and causes the rate
governor to pace symbols faster than mathematically intended.

> +		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);
> +	}

[ ... ]

> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index 00c9b55925f93..6f79328c8b790 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c

[ ... ]

> @@ -1497,6 +1509,7 @@ void msm_dp_display_atomic_disable(struct msm_dp *dp)
>  	msm_dp_display = container_of(dp, struct msm_dp_display_private, msm_dp_display);
>  
>  	msm_dp_ctrl_push_vcpf(msm_dp_display->ctrl, msm_dp_display->panel);
> +	msm_dp_ctrl_mst_timeslot_setup(msm_dp_display->ctrl);
>  	msm_dp_ctrl_mst_send_act(msm_dp_display->ctrl, msm_dp_display->panel);
>  }

[Severity: High]
Can this call chain lead to sleeping in an atomic context?

The atomic modesetting core calls the atomic disable hooks which leads to
msm_dp_display_atomic_disable(). This function unconditionally calls
msm_dp_ctrl_mst_send_act().

Looking at msm_dp_ctrl_mst_send_act() in dp_ctrl.c:

int msm_dp_ctrl_mst_send_act(...) {
    ...
    msm_dp_write_link(ctrl, REG_DP_MST_ACT, 0x1);
    wmb();
    msleep(frame_time_ms);
    ...
}

Since DRM encoder and bridge atomic callbacks run in atomic context during
non-blocking commits, sleeping here with msleep() violates the atomic
context contract and could lead to system instability, deadlocks, and
kernel warnings.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260831-msm-dp-mst-v6-0-c91d35d6fb9e@oss.qualcomm.com?part=13