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

[Yongxing Mou <yongxing.mou@oss.qualcomm.com>]

On 6/9/2026 6:09 PM, sashiko-bot@kernel.org wrote:
> 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.
> 
Will update it.
>> -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?
> 
When MST call it , will pass corresponding *info.
> [ ... ]
> 
>> @@ -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?
> 
We will further refactor this in a follow-up patch series so that MST 
can use the corresponding APIs.
>>   	if (!crtc)
>>   		return;
>>   	crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
>