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 <[email protected]> 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/[email protected]?part=13
