Re: [PATCH v7 08/35] drm/msm/dp: add support to send ACT packets for MST

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] drm/msm: msleep() called from atomic modesetting context
- [Low] msm: linker error on 32-bit platforms due to 64-bit DIV_ROUND_UP
- [Low] msm: ineffective memory barrier used for MMIO write ordering
--

commit 9d59610b2e4fe65438a505df688ad0556d4e6917
Author: Abhinav Kumar <quic_abhinavk@quicinc.com>

    drm/msm/dp: add support to send ACT packets for MST
    
    Whenever virtual channel slot allocation changes, the DP
    source must send the action control trigger sequence to notify
    the sink about the same. This would be applicable during the
    start and stop of the pixel stream. Add the infrastructure
    to be able to send ACT packets for the DP controller when
    operating in MST mode.

> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> index e1614bc49a7ca..65d22505c2b67 100644
> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
[ ... ]
> @@ -244,6 +245,37 @@ static int msm_dp_aux_link_configure(struct drm_dp_aux *aux,
>  	return err;
>  }
>  
> +int msm_dp_ctrl_mst_send_act(struct msm_dp_ctrl *msm_dp_ctrl,
> +			     struct msm_dp_panel *panel)
> +{
> +	struct msm_dp_ctrl_private *ctrl;
> +	const struct drm_display_mode *mode;
> +	u32 frame_time_ms;
> +
> +	ctrl = container_of(msm_dp_ctrl, struct msm_dp_ctrl_private, msm_dp_ctrl);
> +
> +	if (!ctrl->mst_active)
> +		return 0;
> +
> +	mode = &panel->msm_dp_mode.drm_mode;
> +	frame_time_ms = DIV_ROUND_UP((u64)mode->htotal * mode->vtotal,
> +				     mode->clock) + 1;

[Severity: Low]
Does casting the dividend to u64 here force a 64-bit division that causes
a linker error for the missing __udivdi3 symbol on 32-bit platforms?

Should we use the DIV_ROUND_UP_ULL() macro instead for 64-bit dividends?

> +
> +	msm_dp_write_link(ctrl, REG_DP_MST_ACT, 0x1);
> +	/* make sure ACT signal is performed */
> +	wmb();

[Severity: Low]
Does this wmb() guarantee the MMIO write has reached the device?

Since msm_dp_write_link() uses writel() which already handles pre-write
barriers, the trailing CPU memory barrier does not flush interconnect write
buffers.

Should we perform a dummy read-back of the register instead to ensure the
write has reached the device synchronously?

> +
> +	msleep(frame_time_ms);

[Severity: High]
Is it safe to call msleep() here?

This function is called from the atomic modesetting context:

msm_dp_display_atomic_disable() ->
  msm_dp_ctrl_mst_send_act()

Sleeping in atomic context can lead to kernel warnings and system
instability. Should we use a non-sleeping delay function like udelay()
instead?

> +
> +	/* Hardware clears this bit after sending 4 ACT headers */
> +	if (msm_dp_read_link(ctrl, REG_DP_MST_ACT)) {
> +		drm_dbg_dp(ctrl->drm_dev, "MST ACT trigger complete failed\n");
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}

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