Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider: - [High] drm: sleeping in atomic context during CRTC enable/disable - [High] drm/solomon: unsynchronized clear in master_drop races with atomic commits - [High] drm/solomon: clear_display hardcodes RAM address counters to 0x00 - [Medium] drm/solomon: clear_display writes 0xFF to RED RAM causing red screen - [Medium] drm/solomon: display_cleared_on_deinit flag is not reset on enable - [Medium] drm/solomon: clear_on_init bypasses active CRTCs during master handover --
commit 90ac1ffc13e17b560cc95b731a7e77c4602e7b77 Author: Devarsh Thakkar <[email protected]> drm/solomon: ssd16xx: Add clear_on_init/close/disable session management E-paper displays are bistable: the last rendered image persists indefinitely across sessions and power cycles. This creates a session management choice with no LCD/OLED equivalent since unlike volatile displays, content visibility outlives the rendering process. Add session-lifetime display clearing with three configurable hooks: clear_on_init, clear_on_close, and clear_on_disable. > diff --git a/drivers/gpu/drm/solomon/ssd16xx.c > b/drivers/gpu/drm/solomon/ssd16xx.c > index d3af055c67398..c478309d08e9f 100644 > --- a/drivers/gpu/drm/solomon/ssd16xx.c > +++ b/drivers/gpu/drm/solomon/ssd16xx.c [ ... ] > @@ -746,6 +766,111 @@ static int ssd16xx_hw_init(struct ssd16xx_device > *device) > 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 = (device->width * device->height) / 8; > + u8 *white_buffer = device->tx_buf; > + > + memset(white_buffer, 0xFF, data_size); [Severity: High] Can this unsynchronized memset cause a data race? When a DRM master drops, ssd16xx_drm_master_drop calls ssd16xx_clear_display_on_exit. If an atomic commit is concurrently executing in a background worker, both paths will mutate device->tx_buf without locks, which could interleave SPI commands and corrupt the controller state. > + > + 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: High] Does hardcoding the RAM X and Y address counters to 0x00 break decrement-mode writes and omit window setup? In portrait orientation (decrement entry modes), starting at 0x00 might immediately underflow the counter and write out-of-bounds. Also, if the previous operation was a partial update, could the RAM window remain restricted, causing the clear operation to wrap around and fail to clear the full screen? > + > + ssd16xx_send_cmd(device, SSD16XX_CMD_WRITE_RAM_BW, &err); > + ssd16xx_send_data_bulk(device, white_buffer, data_size, &err); > + > + ssd16xx_send_cmd(device, SSD1683_CMD_WRITE_RAM_RED, &err); > + ssd16xx_send_data_bulk(device, white_buffer, data_size, &err); [Severity: Medium] Does this write 0xFF to the RED RAM unconditionally? On 3-color (BWR) panels where a 1 bit represents red ink, this would clear the screen to solid red instead of white. On 2-color panels, it sends an unsupported command (0x26), which could corrupt hardware state. [ ... ] > @@ -1356,11 +1481,26 @@ 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 idx; > + int ret, idx; > > if (!drm_dev_enter(&device->drm, &idx)) > return; > > + if (device->clear_on_disable < 0 || device->display_cleared_on_deinit) > + 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)); [Severity: High] Can this cause a sleep in atomic context? The CRTC atomic_disable callback executes in atomic context where sleeping is forbidden, but it conditionally calls ssd16xx_clear_display, which relies on SPI transactions and polling with usleep_range in ssd16xx_wait_for_device. > + if (ret) { > + drm_err(&device->drm, "atomic_disable: clear failed: %d\n", > ret); > + goto out; > + } > + > + device->display_cleared_on_deinit = true; [Severity: Medium] Is the display_cleared_on_deinit flag missing a reset on enable? DPMS off sets the flag to true, but since atomic_enable does not reset it, subsequent DPMS toggles during a single session might silently skip the clear operation. > +out: > drm_dev_exit(idx); > } > > @@ -1383,6 +1523,11 @@ static void ssd16xx_crtc_atomic_enable(struct drm_crtc > *crtc, > } > device->initialized = true; > > + /* Clear display on first app launch if configured */ > + ret = ssd16xx_clear_display_on_init(device); [Severity: High] Can this cause a sleep in atomic context? Similar to atomic_disable, the CRTC atomic_enable callback executes in atomic context where sleeping is forbidden, but calling ssd16xx_clear_display_on_init leads to ssd16xx_clear_display which eventually polls with usleep_range. [Severity: Medium] Does this bypass active CRTCs during master handover? The clear_on_init hook is inside atomic_enable, which is only called on transitions from disabled to enabled. If a new DRM master takes over without triggering a full modeset (e.g. the CRTC remains active), the clean-slate clear might not fire. > + if (ret) > + drm_err(&device->drm, "crtc_atomic_enable: clear on init > failed: %d\n", ret); > + > /* > * If refresh_mode_init is set, arm init_refresh_pending so > * plane_atomic_update uses the specified mode for the first frame -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=5
