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 <[email protected]> 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/[email protected]?part=14
