Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
New issues: - [High] Moving mode setup to `.atomic_enable` causes the upstream DPU encoder to read stale wide-bus configuration data during its own enablement. Pre-existing issues: - [High] Mode validation callback incorrectly evaluates wide bus availability based on the state of the currently active mode rather than the candidate mode. -- commit 1b0de5b490fbc6b7e19b00f71948fa7d36954fe7 Author: Yongxing Mou <[email protected]> drm/msm/dp: drop deprecated .mode_set() and use .atomic_enable The bridge .mode_set() callback is deprecated. Remove it and move all mode setup logic to .atomic_enable(), where the adjusted_mode is available from the atomic CRTC state. > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c > b/drivers/gpu/drm/msm/dp/dp_display.c > index bce165bac106b..57eafd4724627 100644 > --- a/drivers/gpu/drm/msm/dp/dp_display.c > +++ b/drivers/gpu/drm/msm/dp/dp_display.c [ ... ] > @@ -1309,7 +1325,7 @@ bool msm_dp_wide_bus_available(const struct msm_dp > *msm_dp_display) > > dp = container_of(msm_dp_display, struct msm_dp_display_private, > msm_dp_display); > > - if (dp->msm_dp_mode.out_fmt_is_yuv_420) > + if (dp->panel->msm_dp_mode.out_fmt_is_yuv_420) [Severity: High] This isn't a bug introduced by this patch, but doesn't this function check the currently active mode's format rather than the candidate mode's format? During mode validation in msm_dp_display_mode_valid(), the driver calls msm_dp_wide_bus_available() to adjust pixel clock limits. Since this reads the active mode's YUV420 state, wouldn't it incorrectly assume wide bus is unavailable for a candidate RGB mode if the currently active mode happens to be YUV420? This might cause valid high-resolution RGB modes to be rejected because their clock exceeds DP_MAX_PIXEL_CLK_KHZ. Additionally, isn't this reading a state field that could be locklessly mutated by concurrent atomic commits? > return false; > > return dp->wide_bus_supported; [ ... ] > @@ -1386,7 +1406,7 @@ void msm_dp_bridge_atomic_enable(struct drm_bridge > *drm_bridge, > if (msm_dp_display->link->sink_count == 0) > return; > > - rc = msm_dp_display_set_mode(dp, &msm_dp_display->msm_dp_mode); > + rc = msm_dp_display_set_mode(dp, &crtc_state->adjusted_mode, > msm_dp_display->panel); [Severity: High] By moving the mode setup here into the bridge's .atomic_enable callback, could this cause the upstream DPU encoder to read stale wide-bus configuration data? The DRM atomic helper sequence executes the encoder's .atomic_enable hook before the bridge's .atomic_enable hook. In the encoder's enable sequence: drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c:dpu_encoder_virt_atomic_enable() dpu_enc->wide_bus_en = dpu_encoder_is_widebus_enabled(drm_enc); This calls down to msm_dp_wide_bus_available(), which checks the out_fmt_is_yuv_420 flag. Since the bridge hasn't updated its mode state yet via msm_dp_display_set_mode(), won't the encoder read the stale value from the previous modeset? This seems like it could incorrectly enable or disable wide bus support when switching between RGB and YUV420 modes, potentially leading to a hardware configuration mismatch and display corruption. > if (rc) { > DRM_ERROR("Failed to perform a mode set, rc=%d\n", rc); > return; -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
