Re: [PATCH v8 13/15] drm/msm/dp: make bridge helpers use dp_display to allow re-use

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The refactoring of `msm_dp_display_atomic_enable` hardcodes the primary bridge's encoder, breaking the MST reuse it was explicitly intended to support.
--

--- Patch [13]: [PATCH v8 13/15] drm/msm/dp: make bridge helpers use dp_display to allow re-use ---
commit 94e6d1d4d94637e92b8d6868e076b0bb8d8e32d0
Author: Abhinav Kumar <quic_abhinavk@quicinc.com>
drm/msm/dp: make bridge helpers use dp_display to allow re-use

This commit refactors the DP bridge helpers to directly take a msm_dp_display
object instead of a drm_bridge. The stated goal is to allow the upcoming MST
bridge implementation to reuse these same helpers.

> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c

[ ... ]

> @@ -1408,11 +1405,9 @@ int msm_dp_modeset_init(struct msm_dp *msm_dp_display, struct drm_device *dev,
>  	return 0;
>  }
>  
> -void msm_dp_bridge_atomic_enable(struct drm_bridge *drm_bridge,
> -				 struct drm_atomic_commit *state)
> +void msm_dp_display_atomic_enable(struct msm_dp *msm_dp_display,
> +				  struct drm_atomic_commit *state)
>  {
> -	struct msm_dp_bridge *msm_dp_bridge = to_dp_bridge(drm_bridge);
> -	struct msm_dp *msm_dp_display = msm_dp_bridge->msm_dp_display;
>  	struct drm_crtc *crtc;
>  	struct drm_crtc_state *crtc_state;
>  	int rc = 0;
> @@ -1421,7 +1416,7 @@ void msm_dp_bridge_atomic_enable(struct drm_bridge *drm_bridge,
>  	dp = container_of(msm_dp_display, struct msm_dp_display_private, msm_dp_display);
>  
>  	crtc = drm_atomic_get_new_crtc_for_encoder(state,
> -						   drm_bridge->encoder);
> +						   msm_dp_display->bridge->encoder);

[Severity: Medium]
The commit message mentions this refactoring is done so the MST bridge can
re-use these helpers. However, doesn't replacing the caller-provided
drm_bridge->encoder with the hardcoded primary encoder break that intended
reuse?

If an MST stream calls this helper with its own virtual encoder's state,
drm_atomic_get_new_crtc_for_encoder() will evaluate the primary encoder
instead. Because the primary encoder won't be attached to the MST stream's
CRTC in this atomic state, crtc will be NULL, causing this function to
silently return early.

Would it be better to retain the drm_bridge parameter or pass the drm_encoder
directly so that MST callers can specify their virtual encoder?

>  	if (!crtc)
>  		return;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260720-dp_mstclean-v8-0-bea261bf4e7d@oss.qualcomm.com?part=13