Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] drm/tegra: rgb: unlocked drm_bridge_get_current_state() in encoder enable --
commit 62888cceae72e28c0425a5560b48d0bf56dccc9e Author: Svyatoslav Ryhel <[email protected]> drm/tegra: rgb: Parameterize configuration based on bus flags This commit parameterizes the Tegra RGB encoder configuration based on bus flags from the bridge state, including pixel clock polarity, display enable polarity, and data alignment. > diff --git a/drivers/gpu/drm/tegra/rgb.c b/drivers/gpu/drm/tegra/rgb.c > index b6d09a8d8e5ea..da9e37fe36689 100644 > --- a/drivers/gpu/drm/tegra/rgb.c > +++ b/drivers/gpu/drm/tegra/rgb.c [ ... ] > @@ -104,14 +105,24 @@ static void tegra_rgb_encoder_enable(struct drm_encoder > *encoder) > struct drm_display_mode *mode = &encoder->crtc->state->adjusted_mode; > struct tegra_output *output = encoder_to_output(encoder); > struct tegra_rgb *rgb = to_rgb(output); > - u32 value; > + struct drm_bridge_state *bridge_state; > + u32 bus_flags, value; > + > + /* Get but flags from the bridge state. */ > + bridge_state = drm_bridge_get_current_state(output->bridge); > + if (bridge_state) > + bus_flags = bridge_state->input_bus_cfg.flags; > + else > + bus_flags = DRM_BUS_FLAG_PIXDATA_SAMPLE_NEGEDGE | > + DRM_BUS_FLAG_DE_HIGH | > + DRM_BUS_FLAG_DATA_MSB_TO_LSB; [Severity: High] Is it safe to call drm_bridge_get_current_state() from tegra_rgb_encoder_enable()? Looking at include/drm/drm_bridge.h, drm_bridge_get_current_state() asserts that bridge->base.lock is held: static inline struct drm_bridge_state * drm_bridge_get_current_state(struct drm_bridge *bridge) { if (!bridge) return NULL; drm_modeset_lock_assert_held(&bridge->base.lock); if (!bridge->base.state) return NULL; return drm_priv_to_bridge_state(bridge->base.state); } When userspace submits a non-blocking atomic modeset commit (DRM_MODE_ATOMIC_NONBLOCK | DRM_MODE_ATOMIC_ALLOW_MODESET), drm_atomic_helper_commit() swaps the state and schedules commit_work on system_dfl_wq, after which drm_mode_atomic_ioctl() drops all modeset locks. The worker thread then executes without holding bridge->base.lock: commit_work() -> commit_tail() -> tegra_atomic_commit_tail() -> drm_atomic_helper_commit_tail_rpm() -> drm_atomic_helper_commit_modeset_enables() -> drm_atomic_helper_commit_encoder_bridge_enable() -> tegra_rgb_encoder_enable() Won't calling drm_bridge_get_current_state(output->bridge) here trigger a lockdep warning when CONFIG_PROVE_LOCKING is enabled and read bridge->base.state without synchronization against concurrent atomic state swaps? Should this encoder implement .atomic_enable and retrieve the bridge state via drm_atomic_get_new_bridge_state(state, output->bridge), or cache bus_flags during .atomic_check instead? [ ... ] > > tegra_dc_commit(rgb->dc); -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
