Re: [PATCH v3 14/14] arm64: dts: ti: [DO_NOT_MERGE] Add AM62L3 EVM overlay for GDEY042T81 e-paper display

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Medium] drm/solomon: ssd16xx: broken RAM window in ssd16xx_clear_display() at 90/180 deg
- [Medium] drm/solomon: ssd16xx: missing fast refresh LUT preload on init and resume
- [Medium] drm/solomon: ssd16xx: broken deep_sleep_mode state and error rollback in ssd16xx_pm_suspend()
- [Medium] drm/solomon: ssd16xx: missing RED RAM baseline restore after mode 0x03 sleep
--

commit a52cd0ebb1dc317dd86b82ae1b73fbcb5d5fa3b6
Author: Devarsh Thakkar <devarsht@ti.com>

arm64: dts: ti: [DO_NOT_MERGE] Add AM62L3 EVM overlay for GDEY042T81 e-paper display

This series adds a DRM driver for the Solomon SSD16xx family of e-paper
display controllers (including SSD1683 and SSD1677) with runtime and system
power management, session management, and plane properties, along with the
AM62L3 EVM device tree overlay for the GDEY042T81 panel.

> diff --git a/drivers/gpu/drm/solomon/ssd16xx.c b/drivers/gpu/drm/solomon/ssd16xx.c
> new file mode 100644
> index 0000000000000..29dca237378f8
> --- /dev/null
> +++ b/drivers/gpu/drm/solomon/ssd16xx.c

[ ... ]

> +	[SSD1677] = {
> +		/*
> +		 * 10-bit pixel-level X/Y addressing; only deep sleep mode
> +		 * 0x03 is supported (no RAM-retain mode).  Border waveform
> +		 * encoding matches SSD1683.
> +		 */
> +		.max_width  = 960,
> +		.max_height = 680,
> +		.ram_x_address_bits = 10,
> +		.ram_y_address_bits = 10,
> +		.has_temp_sensor_ctrl    = true,
> +		.deep_sleep_mode_level1  = SSD1683_DEEP_SLEEP_MODE_2,  /* 0x03 */
> +		.deep_sleep_mode_level2  = SSD1683_DEEP_SLEEP_MODE_2,  /* 0x03 */

[ ... ]

> +	switch (device->orientation) {
> +	case 90:
> +	case 180:
> +		data_entry_mode = SSD16XX_DATA_ENTRY_XDEC_YDEC;
> +		break;
> +	default: /* 0°/270° */
> +		data_entry_mode = SSD16XX_DATA_ENTRY_XINC_YINC;
> +		break;
> +	}
> +
> +	ssd16xx_send_cmd(device, SSD16XX_CMD_DATA_ENTRY_MODE, &err);
> +	ssd16xx_send_data(device, data_entry_mode, &err);
> +	drm_dbg(&device->drm, "hw_init: orientation=%u° data_entry=0x%02x\n",
> +		device->orientation, data_entry_mode);
> +
> +	ssd16xx_wait_for_device(device, &err);
> +
> +	if (err)
> +		drm_err(&device->drm, "Hardware initialization failed: %d\n", err);
> +
> +	return err;
> +}
> +
> +/*
> + * Clear display by writing all-white to both BW and RED RAM.
> + * The ctrl2 argument selects the waveform (full/partial/fast refresh).
> + * Border waveform is set to init value before clearing, then restored
> + * to the update value to preserve the border during subsequent updates.
> + */
> +static int ssd16xx_clear_display(struct ssd16xx_device *device, u8 ctrl2)
> +{
> +	const u8 *bw_tbl = device->controller_cfg->border_waveform_table;
> +	int err = 0;
> +	unsigned int data_size = DIV_ROUND_UP(device->width, 8) * device->height;
> +	u8 *white_buffer = device->tx_buf;
> +	u8 *red_buffer = white_buffer;
> +	u16 ram_x_end, ram_y_end;
> +
> +	memset(white_buffer, 0xFF, data_size);
> +
> +	if (device->controller_cfg->ram_x_address_bits == 8)
> +		ram_x_end = (device->width / 8) - 1;
> +	else
> +		ram_x_end = device->width - 1;
> +	ram_y_end = device->height - 1;
> +
> +	/* Reset full display RAM window before clearing */
> +	ssd16xx_send_cmd(device, SSD16XX_CMD_SET_RAM_X_ADDRESS_START_END, &err);
> +	ssd16xx_send_x_param(device, 0x00, &err);
> +	ssd16xx_send_x_param(device, ram_x_end, &err);
> +
> +	ssd16xx_send_cmd(device, SSD16XX_CMD_SET_RAM_Y_ADDRESS_START_END, &err);
> +	ssd16xx_send_y_param(device, 0x00, &err);
> +	ssd16xx_send_y_param(device, ram_y_end, &err);
> +
> +	ssd16xx_send_cmd(device, SSD16XX_CMD_SET_RAM_X_ADDRESS_COUNTER, &err);
> +	ssd16xx_send_x_param(device, 0x00, &err);
> +
> +	ssd16xx_send_cmd(device, SSD16XX_CMD_SET_RAM_Y_ADDRESS_COUNTER, &err);
> +	ssd16xx_send_y_param(device, 0x00, &err);

[Severity: Medium]
When device->orientation is 90 or 180, ssd16xx_hw_init() sets
SSD16XX_CMD_DATA_ENTRY_MODE to SSD16XX_DATA_ENTRY_XDEC_YDEC, where the
controller decrements X and Y on every byte written.

In ssd16xx_fb_dirty(), 90 and 180 degree orientations program the RAM window
with end-before-start (ram_x_end to ram_x_start, ram_y_end to ram_y_start)
and place the initial address counters at (ram_x_end, ram_y_end).

Here in ssd16xx_clear_display(), the window is unconditionally programmed
from 0x00 to ram_x_end and 0x00 to ram_y_end with the counters starting at
(0x00, 0x00). When invoked via ssd16xx_clear_display_on_init(),
ssd16xx_clear_display_on_exit(), or ssd16xx_crtc_atomic_disable() on a panel
rotated 90 or 180 degrees, won't this fail to clear the display RAM and
leave RED RAM out of sync for subsequent partial updates?

[ ... ]

> +	/* Resume lets the runtime_resume callback re-run hw_init if needed. */
> +	ret = pm_runtime_resume_and_get(device->drm.dev);
> +	if (ret < 0) {
> +		drm_err(&device->drm, "plane_atomic_update: failed to resume: %d\n", ret);
> +		goto out_exit;
> +	}
> +
> +	if (!device->initialized) {
> +		drm_err(&device->drm, "plane_atomic_update: hw not initialized after resume\n");
> +		pm_runtime_put_autosuspend(device->drm.dev);
> +		goto out_exit;
> +	}
> +
> +	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.
> +	 */
> +	if (old_ssd->refresh_mode != new_ssd->refresh_mode) {
> +		/*
> +		 * Fast refresh omits LOAD_LUT each update; arm the
> +		 * one-shot flag so LUT is pre-loaded before the first
> +		 * fast frame.
> +		 */
> +		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);
> +
> +	/*
> +	 * When refresh_mode_init was set, use the specified mode for this first
> +	 * frame only, then restore the user-configured refresh_mode so
> +	 * subsequent updates continue with the configured mode.
> +	 */
> +	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;
> +	}
> +
> +	/*
> +	 * Fast refresh (0xC7) omits LOAD_LUT on every update cycle and relies
> +	 * on the LUT being pre-loaded upfront.  The property setter arms
> +	 * fast_lut_pending whenever the user switches into fast mode.  Consume
> +	 * the flag here (once) before the first fast-refresh frame so the
> +	 * controller's LUT is in the correct state.
> +	 */
> +	if (device->fast_lut_pending) {
> +		ret = ssd16xx_preload_fast_lut(device);
> +		if (ret) {
> +			drm_err(&device->drm,
> +				"plane_atomic_update: fast LUT preload failed: %d\n", ret);
> +		}
> +
> +		device->fast_lut_pending = false;
> +	}

[Severity: Medium]
Can SSD16XX_REFRESH_FAST run without pre-loading the LUT in a few paths
here and in ssd16xx_crtc_atomic_enable()?

First, in ssd16xx_plane_atomic_update(), pm_runtime_resume_and_get() runs
ssd16xx_pm_runtime_resume() -> ssd16xx_hw_init() before
ssd16xx_sync_session_props() updates device->refresh_mode. If the device
resumes from runtime suspend when old_ssd->refresh_mode was
SSD16XX_REFRESH_FULL and new_ssd->refresh_mode is SSD16XX_REFRESH_FAST,
ssd16xx_hw_init() resets the controller (wiping the LUT) and sees the stale
FULL mode, while the check above also skips setting device->fast_lut_pending
because old_ssd->refresh_mode == SSD16XX_REFRESH_FULL.

Second, in ssd16xx_crtc_atomic_enable(), both pm_runtime_resume_and_get()
and ssd16xx_hw_init() run before ssd16xx_sync_session_props(), so
ssd16xx_hw_init() checks the stale device->refresh_mode rather than the new
plane state's mode (and ssd16xx_clear_display_on_init() can also run
SSD16XX_REFRESH_FAST without a pre-loaded LUT).

Third, when device->init_refresh_pending is true, device->refresh_mode is
overridden with device->refresh_mode_init after device->fast_lut_pending is
computed from old_ssd and new_ssd. If refresh_mode_init is
SSD16XX_REFRESH_FAST while new_ssd->refresh_mode is SSD16XX_REFRESH_PARTIAL,
fast_lut_pending is not armed for the init frame. Conversely, if
new_ssd->refresh_mode switches from PARTIAL to FAST while refresh_mode_init
is PARTIAL, fast_lut_pending is consumed before the PARTIAL init frame
(which loads the Mode 2 partial LUT), leaving subsequent FAST frames without
a Mode 1 LUT preload.

[ ... ]

> +static void ssd16xx_crtc_atomic_disable(struct drm_crtc *crtc,
> +					struct drm_atomic_commit *state)
> +{
> +	struct ssd16xx_device *device = crtc_to_ssd16xx_device(crtc);
> +	int ret, idx;
> +
> +	if (!drm_dev_enter(&device->drm, &idx))
> +		return;
> +
> +	mutex_lock(&device->lock);
> +
> +	if (device->clear_on_disable < 0 || device->cleared_on_disable)
> +		goto out;
> +
> +	ret = pm_runtime_resume_and_get(device->drm.dev);
> +	if (ret < 0)
> +		goto out;
> +
> +	drm_dbg(&device->drm, "clear_on_disable: running, mode=%d\n",
> +		device->clear_on_disable);
> +	ret = ssd16xx_clear_display(device,
> +				    ssd16xx_refresh_mode_to_ctrl2(device,
> +								  device->clear_on_disable));
> +	if (ret) {
> +		drm_err(&device->drm, "atomic_disable: clear failed: %d\n", ret);
> +		pm_runtime_put_autosuspend(device->drm.dev);
> +		goto out;
> +	}
> +
> +	device->cleared_on_disable = true;
> +	pm_runtime_put_sync_suspend(device->drm.dev);
> +out:
> +	mutex_unlock(&device->lock);
> +	drm_dev_exit(idx);
> +}

[ ... ]

> +static int ssd16xx_pm_suspend(struct device *dev)
> +{
> +	struct ssd16xx_device *device = dev_get_drvdata(dev);
> +	struct drm_device *drm = &device->drm;
> +	int ret;
> +
> +	/* Use level2 deep sleep for system suspend; SSD1683 loses RAM but saves more power. */
> +	device->deep_sleep_mode = device->controller_cfg->deep_sleep_mode_level2;
> +	dev_dbg(dev, "system suspend: entering deep sleep mode 0x%02x\n",
> +		device->deep_sleep_mode);
> +
> +	ret = drm_mode_config_helper_suspend(drm);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * If the autosuspend timer already fired the device is RPM_SUSPENDED
> +	 * at level1.  pm_runtime_force_suspend() would skip its callback, so
> +	 * level2 would never reach the hardware.
> +	 *
> +	 * SPI is unresponsive in deep sleep; HWRESET is the only exit.
> +	 * After the reset, level2 can be sent directly without a full hw_init.
> +	 *
> +	 * Skip this for controllers where level1 == level2 (e.g. SSD1677 which
> +	 * has only one deep sleep mode): the device is already in the deepest
> +	 * available state and no upgrade is needed.
> +	 */
> +	if (pm_runtime_status_suspended(dev)) {
> +		int err = 0;
> +
> +		if (device->controller_cfg->deep_sleep_mode_level1 !=
> +		    device->controller_cfg->deep_sleep_mode_level2) {
> +			dev_dbg(dev, "system suspend: upgrading level1->level2 via HWRESET\n");
> +			ssd16xx_hw_reset(device);
> +			ssd16xx_wait_for_device(device, &err);
> +			ssd16xx_send_cmd(device, SSD16XX_CMD_DEEP_SLEEP_MODE, &err);
> +			ssd16xx_send_data(device,
> +					  device->controller_cfg->deep_sleep_mode_level2,
> +					  &err);
> +		} else {
> +			dev_dbg(dev, "system suspend: already at deepest sleep mode\n");
> +		}
> +		device->initialized = false;
> +		device->pm_force_suspended = false;  /* Skip force_resume on resume */
> +		return err;
> +	}
> +
> +	/* Force runtime-suspended state for clean resume. */
> +	ret = pm_runtime_force_suspend(dev);
> +	if (ret) {
> +		drm_mode_config_helper_resume(drm);
> +		return ret;
> +	}
> +	device->pm_force_suspended = true;  /* Resume must call force_resume */
> +
> +	return 0;
> +}

[Severity: Medium]
In ssd16xx_pm_suspend(), device->deep_sleep_mode is updated to
deep_sleep_mode_level2 before calling drm_mode_config_helper_suspend(drm).
Could this cause three issues during system suspend?

1. When clear_on_disable >= 0, drm_mode_config_helper_suspend(drm) calls
ssd16xx_crtc_atomic_disable(), which clears the display and invokes
pm_runtime_put_sync_suspend(). Because device->deep_sleep_mode was already
set to level2, ssd16xx_pm_runtime_suspend() puts the panel into level2 sleep
and marks the device RPM_SUSPENDED. When control returns to
ssd16xx_pm_suspend(), pm_runtime_status_suspended(dev) is true, so it
assumes the panel was in level1 sleep and performs a second hardware reset
and level2 sleep command while skipping pm_runtime_force_suspend(dev).

2. If drm_mode_config_helper_suspend(drm) or pm_runtime_force_suspend(dev)
returns an error, ssd16xx_pm_suspend() exits without restoring
device->deep_sleep_mode to deep_sleep_mode_level1, leaving subsequent
runtime PM autosuspends using level2 sleep (0x03) and wiping controller RAM.

3. In the pm_runtime_status_suspended(dev) branch, if
ssd16xx_wait_for_device(), ssd16xx_send_cmd(), or ssd16xx_send_data() fails,
ssd16xx_pm_suspend() returns err without calling
drm_mode_config_helper_resume(drm) or restoring device->deep_sleep_mode,
leaving the DRM mode config suspended and leaking
drm->mode_config.suspend_state.

> +static int ssd16xx_pm_resume(struct device *dev)
> +{
> +	struct ssd16xx_device *device = dev_get_drvdata(dev);
> +	struct drm_device *drm = &device->drm;
> +
> +	dev_dbg(dev, "system resume: restoring state\n");
> +
> +	/* Only force_resume if we force_suspended; the level1→level2 upgrade path did not. */
> +	if (device->pm_force_suspended) {
> +		pm_runtime_force_resume(dev);
> +		device->pm_force_suspended = false;
> +	}
> +
> +	/* Restore level1 for subsequent runtime PM cycles. */
> +	device->deep_sleep_mode = device->controller_cfg->deep_sleep_mode_level1;
> +
> +	return drm_mode_config_helper_resume(drm);
> +}
> +
> +static int ssd16xx_pm_runtime_suspend(struct device *dev)
> +{
> +	struct ssd16xx_device *device = dev_get_drvdata(dev);
> +	int err = 0;
> +
> +	dev_dbg(dev, "runtime suspend: entering deep sleep mode 0x%02x\n",
> +		device->deep_sleep_mode);
> +
> +	ssd16xx_send_cmd(device, SSD16XX_CMD_DEEP_SLEEP_MODE, &err);
> +	ssd16xx_send_data(device, device->deep_sleep_mode, &err);
> +	device->initialized = false;
> +
> +	/*
> +	 * Don't propagate SPI errors: a failure here would latch runtime_error
> +	 * and block all future resumes; the next resume resets the panel anyway.
> +	 */
> +	if (err)
> +		dev_warn(dev, "failed to enter deep sleep: %d\n", err);
> +
> +	return 0;
> +}
> +
> +static int ssd16xx_pm_runtime_resume(struct device *dev)
> +{
> +	struct ssd16xx_device *device = dev_get_drvdata(dev);
> +	int ret;
> +
> +	dev_dbg(dev, "runtime resume: initialized=%d%s\n",
> +		device->initialized,
> +		!device->initialized ? " (running hw_init)" : "");
> +
> +	/*
> +	 * pm_runtime_suspend clears initialized after sending the deep sleep
> +	 * command.  If a new app opens before the suspend fires (the put was
> +	 * cancelled), initialized is still true and hw_init is unnecessary.
> +	 */
> +	if (!device->initialized) {
> +		ret = ssd16xx_hw_init(device);
> +		if (ret)
> +			return ret;
> +
> +		device->initialized = true;
> +	}
> +
> +	return 0;
> +}

[Severity: Medium]
When resuming from SSD1683_DEEP_SLEEP_MODE_2 (0x03), the controller loses
its RAM contents, including the RED RAM reference frame required by
SSD16XX_REFRESH_PARTIAL in ssd16xx_fb_dirty().

On SSD1677, deep_sleep_mode_level1 is SSD1683_DEEP_SLEEP_MODE_2 (0x03).
After runtime autosuspend fires while the CRTC remains enabled, the next
plane update calls ssd16xx_plane_atomic_update() ->
pm_runtime_resume_and_get() -> ssd16xx_pm_runtime_resume() ->
ssd16xx_hw_init() without running ssd16xx_crtc_atomic_enable(), so
init_refresh_pending remains false and ssd16xx_fb_dirty() performs a partial
refresh with ctrl1_normal against lost RED RAM.

Similarly, across system suspend and resume (which uses mode 0x03 on SSD1683
as well), if the userspace refresh_mode_init plane property is set to
disabled (-1) or partial (0) while refresh_mode is partial,
ssd16xx_pm_resume() -> drm_mode_config_helper_resume() ->
ssd16xx_crtc_atomic_enable() -> ssd16xx_plane_atomic_update() will also
execute a partial refresh with ctrl1_normal without re-establishing the RED
RAM baseline.

Should resuming from mode 0x03 sleep force a baseline full/fast refresh or
restore RED RAM before the next partial update?

[ ... ]

> -- 
> 2.34.1

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261019213726.4145532-1-devarsht@ti.com?part=14