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