Re: [PATCH v6 08/29] 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] Calling a sleeping function msleep() from an atomic context during DRM atomic commits.
- [High] Build/link error on 32-bit platforms due to 64-bit division using DIV_ROUND_UP.
- [Medium] Incorrect memory barrier used to flush MMIO posted write.
--

commit c7be0c8d679031927a088cf466f79335ab3bce4b
Author: Abhinav Kumar <quic_abhinavk@quicinc.com>
drm/msm/dp: add support to send ACT packets for MST

This commit adds the infrastructure to send action control trigger (ACT)
sequences for the DP controller when operating in MST mode. This is used
to notify the sink whenever virtual channel slot allocation changes during
the start and stop of the pixel stream.

> 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: High]
Does this cause a build error on 32-bit platforms due to a 64-bit division?

The macro DIV_ROUND_UP() performs a division using the / operator. Because
the numerator is explicitly cast to a 64-bit unsigned integer, this attempts a
64-bit division, which can fail to link on 32-bit architectures (e.g., ARM32)
with an undefined reference to __udivdi3.

Should this use DIV_ROUND_UP_ULL() or div_u64() instead when performing 64-bit
division?

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

[Severity: Medium]
Is this the correct way to flush an MMIO posted write?

wmb() only enforces CPU memory ordering; it does not flush posted MMIO writes
to the hardware. The ACT signal write may sit in the bus fabric's posted write
buffer while the CPU goes to sleep. This reduces the effective hardware wait
time and could cause the hardware timeout check to fail incorrectly.

Should a dummy read from the same device (e.g., msm_dp_read_link()) be used
instead to ensure the write is flushed?

> +
> +	msleep(frame_time_ms);

[Severity: High]
Is it safe to use a sleeping function here?

msm_dp_ctrl_mst_send_act() is invoked from the bridge's atomic disable callback
(msm_dp_display_atomic_disable) and atomic enable callback
(msm_dp_ctrl_on_stream). The DRM subsystem guidelines explicitly dictate that
DRM atomic enable/disable callbacks run in an atomic context where sleeping
functions are strictly forbidden.

Calling msleep() here can cause kernel warnings, system instability, and
potential deadlocks. Should a non-sleeping delay function like udelay() or
ndelay() be used 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;
> +}
[ ... ]
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index d0fc4ff508dc6..52c13823e8d7d 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
[ ... ]
> @@ -1497,6 +1497,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_idle(msm_dp_display->ctrl);
> +	msm_dp_ctrl_mst_send_act(msm_dp_display->ctrl, msm_dp_display->panel);
>  }

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