Re: [PATCH v3 10/14] drm/solomon: ssd16xx: Expose session management as plane properties

[sashiko-bot@kernel.org]

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 <devarsht@ti.com>

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/20261019213726.4145532-1-devarsht@ti.com?part=10