Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
New issues: - [High] Moving mode state updates to `.atomic_enable` causes the DPU encoder to read stale wide bus configuration. Pre-existing issues: - [High] `mode_valid` incorrectly evaluates wide bus availability based on the currently active mode's state rather than the candidate mode, causing valid modes to be rejected. -- --- Patch [2]: [PATCH v8 02/15] drm/msm/dp: drop deprecated .mode_set() and use .atomic_enable --- commit f8a619167d17d8d750e695eca53077efafe8ab7e 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) > return false; > > return dp->wide_bus_supported; [Severity: High] This is a pre-existing issue, but does msm_dp_wide_bus_available() evaluate the candidate mode correctly during mode probing? This function checks the currently active mode's format (out_fmt_is_yuv_420) instead of the candidate mode's format. When msm_dp_bridge_mode_valid() (or msm_edp_bridge_mode_valid()) calls this during mode probing, won't it incorrectly reject valid high-resolution RGB modes if the user is currently running a lower-resolution YUV420 mode? [ ... ] > @@ -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); > if (rc) { > DRM_ERROR("Failed to perform a mode set, rc=%d\n", rc); > return; [Severity: High] Does moving this mode setup into .atomic_enable() cause the DPU encoder to read a stale wide bus configuration? In the DRM atomic commit flow, the encoder's .atomic_enable() (which executes dpu_encoder_virt_atomic_enable()) is called before the bridge's .atomic_enable(). Because msm_dp_display_set_mode() updates the format state (msm_dp_panel->msm_dp_mode.out_fmt_is_yuv_420), the DPU encoder will query msm_dp_wide_bus_available() before this new mode's format has been stored. Won't this cause the hardware to be programmed using the wide bus state from the previously active mode, potentially causing display corruption or a blank screen when switching between RGB and YUV420? [ ... ] -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
