Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider: - [High] drm/solomon: ssd16xx: lock inversion between device->lock and drm_dev_enter() - [Medium] drm/solomon: ssd16xx: session properties not synced on disable or idle commits - [Medium] drm/solomon: ssd16xx: disabling refresh_mode_init sets refresh_mode to -1 - [Medium] drm/solomon: ssd16xx: fast LUT preload missed or clobbered by session modes - [Medium] drm/solomon: ssd16xx: runtime PM calls outside device->lock race with master_drop() --
commit 96a2ccec634e73c5b82d2f3b3b5d2292c57d3429 Author: Devarsh Thakkar <[email protected]> drm/solomon: ssd16xx: Expose session management as plane properties This commit exposes the clear_on_init, clear_on_close, clear_on_disable, and refresh_mode_init session management settings as DRM plane enum properties. It also introduces a device mutex to serialize session state and panel access between atomic commits and DRM master callbacks. > diff --git a/drivers/gpu/drm/solomon/ssd16xx.c > b/drivers/gpu/drm/solomon/ssd16xx.c > index 6037717635a48..13bd825bf3f17 100644 > --- a/drivers/gpu/drm/solomon/ssd16xx.c > +++ b/drivers/gpu/drm/solomon/ssd16xx.c [ ... ] > @@ -1576,6 +1664,24 @@ static int ssd16xx_plane_atomic_check(struct drm_plane > *plane, > false, false); > } > > +/* > + * Copy the session properties of a plane state into the device. Called from > + * both crtc_atomic_enable() and plane_atomic_update(): the former runs first > + * in a commit, so it must not rely on values only synced by the latter. > + */ > +static void ssd16xx_sync_session_props(struct ssd16xx_device *device, > + const struct ssd16xx_plane_state > *ssd_state) > +{ > + lockdep_assert_held(&device->lock); > + > + device->refresh_mode = ssd_state->refresh_mode; > + device->color_mode = ssd_state->color_mode; > + device->clear_on_init = ssd_state->clear_on_init; > + device->clear_on_close = ssd_state->clear_on_close; > + device->clear_on_disable = ssd_state->clear_on_disable; > + device->refresh_mode_init = ssd_state->refresh_mode_init; > +} > + > static void ssd16xx_plane_atomic_update(struct drm_plane *plane, > struct drm_atomic_commit *state) > { > @@ -1619,6 +1725,8 @@ static void ssd16xx_plane_atomic_update(struct > drm_plane *plane, > drm_dbg(&device->drm, "plane_atomic_update: calling fb_dirty > rect=(%d,%d)-(%d,%d)\n", > rect.x1, rect.y1, rect.x2, rect.y2); > > + mutex_lock(&device->lock); > + > /* Diff refresh_mode against the plane's own previous state to see > * whether fast LUT preload is needed. > */ > @@ -1635,8 +1743,7 @@ static void ssd16xx_plane_atomic_update(struct > drm_plane *plane, > device->fast_lut_pending = false; > } > > - device->refresh_mode = new_ssd->refresh_mode; > - device->color_mode = new_ssd->color_mode; > + ssd16xx_sync_session_props(device, new_ssd); [Severity: Medium] Can session property updates be missed when a commit has no framebuffer damage, detaches the framebuffer, or occurs while the CRTC is inactive? In ssd16xx_plane_atomic_update(), ssd16xx_sync_session_props() is called after the early exits for a NULL framebuffer and empty damage: if (!fb) goto out_exit; /* Nothing to draw: don't wake the panel up. */ if (!drm_atomic_helper_damage_merged(old_state, new_state, &rect)) goto out_exit; In addition, drm_atomic_helper_commit_tail_rpm() passes DRM_PLANE_COMMIT_ACTIVE_ONLY to drm_atomic_helper_commit_planes(), which skips ssd16xx_plane_atomic_update() whenever the CRTC is inactive. If userspace updates clear_on_close or clear_on_disable in a property-only commit with no damage, while fb is NULL, or while the CRTC is disabled, will ssd16xx_device retain the stale values when ssd16xx_drm_master_drop() or ssd16xx_crtc_atomic_disable() later runs? [Severity: Medium] Does fast_lut_pending account for refresh_mode_init and the clear_on_* session modes? In ssd16xx_plane_atomic_update(), device->fast_lut_pending is computed from old_ssd->refresh_mode and new_ssd->refresh_mode before device->refresh_mode is overridden by device->refresh_mode_init: if (old_ssd->refresh_mode != new_ssd->refresh_mode) { if (new_ssd->refresh_mode == SSD16XX_REFRESH_FAST && old_ssd->refresh_mode != SSD16XX_REFRESH_FULL) device->fast_lut_pending = true; else device->fast_lut_pending = false; } ssd16xx_sync_session_props(device, new_ssd); saved_mode = device->refresh_mode; saved_border_waveform_idx = device->border_waveform_update_idx; if (device->init_refresh_pending) { device->refresh_mode = device->refresh_mode_init; device->border_waveform_update_idx = device->border_waveform_init_idx; } if (device->fast_lut_pending) { ret = ssd16xx_preload_fast_lut(device); ... device->fast_lut_pending = false; } If new_ssd->refresh_mode is SSD16XX_REFRESH_PARTIAL and refresh_mode_init is set to fast, fast_lut_pending remains false and the first frame runs a fast refresh (0xC7) without preloading the fast LUT. Conversely, if new_ssd->refresh_mode transitions to SSD16XX_REFRESH_FAST while refresh_mode_init is set to partial, ssd16xx_preload_fast_lut() runs and clears fast_lut_pending before the init frame, which then runs a partial refresh (0xFF) that overwrites the LUT in the controller, leaving subsequent fast frames without the fast LUT. Similarly, if clear_on_init, clear_on_disable, or clear_on_close is set to fast, ssd16xx_clear_display() sends SSD1683_CTRL2_FAST_REFRESH without preloading the fast LUT, and if clear_on_init is set to partial while refresh_mode is fast, ssd16xx_clear_display_on_init() overwrites the LUT loaded by ssd16xx_hw_init() without re-arming fast_lut_pending. > > /* > * When refresh_mode_init was set, use the specified mode for this first > @@ -1693,6 +1800,8 @@ static void ssd16xx_plane_atomic_update(struct > drm_plane *plane, > device->border_waveform_pending = true; > } > > + mutex_unlock(&device->lock); > + > pm_runtime_mark_last_busy(device->drm.dev); > pm_runtime_put_autosuspend(device->drm.dev); > > @@ -1754,6 +1863,8 @@ static void ssd16xx_crtc_atomic_disable(struct drm_crtc > *crtc, > if (!drm_dev_enter(&device->drm, &idx)) > return; > > + mutex_lock(&device->lock); > + > if (device->clear_on_disable < 0 || device->cleared_on_disable) > goto out; [Severity: Medium] Should ssd16xx_crtc_atomic_disable() also sync the session properties from the commit's new primary plane state before checking device->clear_on_disable? Because the driver uses drm_atomic_helper_commit_tail_rpm(), drm_atomic_helper_commit_modeset_disables() runs ssd16xx_crtc_atomic_disable() before drm_atomic_helper_commit_planes(), and drm_atomic_helper_commit_planes() skips disabled CRTCs due to DRM_PLANE_COMMIT_ACTIVE_ONLY. If userspace sets clear_on_disable or clear_on_close in the same atomic commit that disables the CRTC, ssd16xx_crtc_atomic_disable() and ssd16xx_drm_master_drop() will not see the updated plane state. > > @@ -1775,6 +1886,7 @@ static void ssd16xx_crtc_atomic_disable(struct drm_crtc > *crtc, > device->cleared_on_disable = true; > pm_runtime_put_sync_suspend(device->drm.dev); > out: > + mutex_unlock(&device->lock); > drm_dev_exit(idx); > } > > @@ -1782,6 +1894,8 @@ static void ssd16xx_crtc_atomic_enable(struct drm_crtc > *crtc, > struct drm_atomic_commit *state) > { > struct ssd16xx_device *device = crtc_to_ssd16xx_device(crtc); > + struct drm_plane_state *new_plane_state = > + drm_atomic_get_new_plane_state(state, &device->primary_plane); > int ret, idx; > > if (!drm_dev_enter(&device->drm, &idx)) > @@ -1797,16 +1911,28 @@ static void ssd16xx_crtc_atomic_enable(struct > drm_crtc *crtc, > drm_dbg(&device->drm, "atomic_enable: %dx%d orientation=%u°\n", > device->width, device->height, device->orientation); > > + /* Serialise hw_init and session-prop setup against master_drop(). */ > + mutex_lock(&device->lock); > + > if (!device->initialized) { > ret = ssd16xx_hw_init(device); > if (ret) { > drm_err(&device->drm, "atomic_enable: hw_init failed: > %d\n", ret); > + mutex_unlock(&device->lock); > pm_runtime_put_autosuspend(device->drm.dev); > drm_dev_exit(idx); > return; > } > device->initialized = true; > } > + > + /* > + * atomic_enable runs before plane_atomic_update, so pick up the session > + * properties of this very commit instead of the previous one's. > + */ > + if (new_plane_state) > + ssd16xx_sync_session_props(device, > to_ssd16xx_plane_state(new_plane_state)); [Severity: Medium] Should ssd16xx_sync_session_props() be called before ssd16xx_hw_init() and pm_runtime_resume_and_get()? Inside ssd16xx_hw_init(), device->refresh_mode is checked to decide whether to preload the fast LUT: if (device->refresh_mode == SSD16XX_REFRESH_FAST) { ... } Because ssd16xx_hw_init() runs before ssd16xx_sync_session_props(), it reads the previous commit's refresh_mode rather than the new plane state's refresh_mode. [Severity: Medium] Additionally, in both ssd16xx_crtc_atomic_enable() and ssd16xx_plane_atomic_update(), pm_runtime_resume_and_get() is called before mutex_lock(&device->lock), and pm_runtime_put_autosuspend() is called after mutex_unlock(&device->lock): ret = pm_runtime_resume_and_get(device->drm.dev); ... mutex_lock(&device->lock); ... mutex_unlock(&device->lock); pm_runtime_mark_last_busy(device->drm.dev); pm_runtime_put_autosuspend(device->drm.dev); When the panel is runtime-suspended, pm_runtime_resume_and_get() synchronously runs ssd16xx_pm_runtime_resume() -> ssd16xx_hw_init() and sets device->initialized = true without holding device->lock, racing with ssd16xx_drm_master_drop(). Furthermore, if a nonblocking commit in ssd16xx_plane_atomic_update() holds the runtime PM reference while waiting on mutex_lock(&device->lock), ssd16xx_drm_master_drop()'s call to pm_runtime_put_sync_suspend() will see usage_count > 0 and fail to suspend the panel, and once ssd16xx_drm_master_drop() releases device->lock, the unblocked ssd16xx_plane_atomic_update() will draw over the screen that clear_on_close just cleared. > + > device->cleared_on_disable = false; > > /* Clear display on first app launch if configured */ > @@ -1826,6 +1952,8 @@ static void ssd16xx_crtc_atomic_enable(struct drm_crtc > *crtc, > device->init_refresh_pending = true; > } [Severity: Medium] What happens when a client opens the DRM device and then sets the refresh_mode_init plane property to disabled? When the device is opened on a panel where refresh_mode_init >= 0 by default (such as GDEY042T81, where default_refresh_mode_init is SSD16XX_REFRESH_FULL), ssd16xx_drm_master_set() sets device->init_refresh_pending = true before the client can change plane properties: if (device->refresh_mode_init >= 0) device->init_refresh_pending = true; If the client then sets refresh_mode_init to SSD16XX_SESSION_DISABLED (which maps to internal value -1), ssd16xx_sync_session_props() sets device->refresh_mode_init = -1, but neither ssd16xx_sync_session_props() nor ssd16xx_crtc_atomic_enable() clears device->init_refresh_pending when device->refresh_mode_init < 0. When ssd16xx_plane_atomic_update() subsequently runs: if (device->init_refresh_pending) { device->refresh_mode = device->refresh_mode_init; device->border_waveform_update_idx = device->border_waveform_init_idx; } device->refresh_mode is set to -1, causing ssd16xx_fb_dirty() to fall through to the default partial refresh case with the init border waveform (and skip 3-color red buffer setup) instead of using the configured refresh_mode. Conversely, if device->refresh_mode_init was -1 when ssd16xx_drm_master_set() ran and the client enables refresh_mode_init in a plane update on an already-active CRTC where ssd16xx_crtc_atomic_enable() does not run, device->init_refresh_pending is never armed. > > + mutex_unlock(&device->lock); > + > pm_runtime_mark_last_busy(device->drm.dev); > pm_runtime_put_autosuspend(device->drm.dev); > drm_dev_exit(idx); [ ... ] > @@ -1963,6 +2140,12 @@ static void ssd16xx_drm_master_drop(struct drm_device > *drm, > struct ssd16xx_device *device = to_ssd16xx_device(drm); > int ret, idx; > > + /* > + * Not serialized against nonblocking commits by the DRM core (e.g. on > + * DROP_MASTER), so hold the lock across the panel clear too. > + */ > + guard(mutex)(&device->lock); > + > device->init_refresh_pending = false; > device->first_clear_done = false; [Severity: High] Does acquiring device->lock before drm_dev_enter() in ssd16xx_drm_master_drop() create a lock inversion with the atomic commit callbacks? In ssd16xx_drm_master_drop(), guard(mutex)(&device->lock) acquires device->lock before calling drm_dev_enter(drm, &idx) (which acquires drm_unplug_srcu): guard(mutex)(&device->lock); ... if (!drm_dev_enter(drm, &idx)) return; However, ssd16xx_plane_atomic_update(), ssd16xx_crtc_atomic_disable(), and ssd16xx_crtc_atomic_enable() all call drm_dev_enter(&device->drm, &idx) before acquiring mutex_lock(&device->lock): if (!drm_dev_enter(&device->drm, &idx)) return; mutex_lock(&device->lock); Will this reverse locking order between device->lock and drm_unplug_srcu trigger a lockdep circular locking dependency warning when a client commits a frame and then drops master with clear_on_close enabled? > [ ... ] > > -- > 2.34.1 -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=10
