Re: [PATCH v7 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 3 potential issue(s) to consider:

New issues:
- [High] Hardcoded base encoder in `msm_dp_display_atomic_enable` breaks MST reuse
- [Low] Kernel-doc mismatch for `msm_dp_display_mode_valid`

Pre-existing issues:
- [High] `msm_dp_display_mode_valid` ignores the `info` parameter, validating against the wrong sink in MST
--

commit 79702b8fc2a6d1a516b0d64051edab10a8d7764e
Author: Abhinav Kumar <quic_abhinavk@quicinc.com>
Subject: drm/msm/dp: make bridge helpers use dp_display to allow re-use

This commit modifies the DP bridge helpers to directly take the dp_display
object rather than a drm_bridge pointer. This refactoring is intended to
allow the MST bridge to reuse the same helpers.

> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index f1970e6dd7e46..cdb7dd4cd7cbd 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -738,24 +738,21 @@ static int msm_dp_display_disable(struct msm_dp_display_private *dp)
>  
>  /**
>   * msm_dp_bridge_mode_valid - callback to determine if specified mode is valid
> - * @bridge: Pointer to drm bridge structure
> + * @dp: Pointer to dp display structure
>   * @info: display info
>   * @mode: Pointer to drm mode structure
>   * Returns: Validity status for specified mode
>   */

[Severity: Low]
This isn't a bug, but there is a mismatch in the documentation. The
kernel-doc still refers to the old function name msm_dp_bridge_mode_valid,
while the function was renamed to msm_dp_display_mode_valid.

> -enum drm_mode_status msm_dp_bridge_mode_valid(struct drm_bridge *bridge,
> -					  const struct drm_display_info *info,
> -					  const struct drm_display_mode *mode)
> +enum drm_mode_status msm_dp_display_mode_valid(struct msm_dp *dp,
> +					       const struct drm_display_info *info,
> +					       const struct drm_display_mode *mode)

[Severity: High]
This is a pre-existing issue, but since these helpers are being exposed for
MST bridge reuse, does it cause problems that the info parameter is ignored?

Inside msm_dp_display_mode_valid(), the code checks capabilities against
dp->connector->display_info instead of using the provided info parameter:

	if ((drm_mode_is_420_only(&dp->connector->display_info, mode) &&
	     msm_dp_display->panel->vsc_sdp_supported) ||
	     msm_dp_wide_bus_available(dp))
		mode_pclk_khz /= 2;

If this is used for an MST stream, won't this validate the mode against the
main DP connector's capabilities rather than the specific MST sink's
capabilities?

[ ... ]

> @@ -1406,11 +1403,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;
> @@ -1419,7 +1414,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: High]
Will this hardcoded encoder lookup break MST support?

In an MST topology, each stream operates through its own distinct virtual
drm_bridge and drm_encoder. By hardcoding msm_dp_display->bridge->encoder
here, the primary DP encoder is used instead of the encoder for the specific
bridge being updated.

If the primary encoder isn't participating in the MST stream's atomic commit,
won't drm_atomic_get_new_crtc_for_encoder() return NULL and cause the
helper to bail out silently without programming the mode?

>  	if (!crtc)
>  		return;
>  	crtc_state = drm_atomic_get_new_crtc_state(state, crtc);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260609-dp_mstclean-v7-0-ea04113e8233@oss.qualcomm.com?part=13