Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] drm/solomon: ssd16xx: missing helper in ssd16xx_plane_atomic_create_state() - [High] drm/solomon: ssd16xx: missed fast LUT preload in ssd16xx_plane_atomic_update() --
commit 5297d32536b98e77de269756d1d61d02dc735b4b Author: Devarsh Thakkar <[email protected]> drm/solomon: ssd16xx: Expose refresh mode as plane property This commit exposes a "refresh_mode" enum property on the primary plane to allow userspace to select between partial, full, and fast e-paper display update modes at runtime. It subclasses drm_shadow_plane_state into ssd16xx_plane_state and tracks refresh mode transitions in ssd16xx_plane_atomic_update(). > diff --git a/drivers/gpu/drm/solomon/ssd16xx.c > b/drivers/gpu/drm/solomon/ssd16xx.c > index 99f11b7ec03e9..f53c67ae7d317 100644 > --- a/drivers/gpu/drm/solomon/ssd16xx.c > +++ b/drivers/gpu/drm/solomon/ssd16xx.c > @@ -397,8 +397,21 @@ struct ssd16xx_device { > u8 *tx_red_buf; /* 1bpp red-channel buffer (3-color panels only) */ > u16 *tx_buf9; /* 9-bit SPI expansion buffer (3-wire mode only) */ > u8 spi_buf[64] ____cacheline_aligned; /* DMA-safe buffer for SPI > transfers */ > + > + struct drm_property *refresh_mode_property; > +}; > + > +/* Plane state subclass: refresh_mode lives on the plane, not the connector, > + * so plane_atomic_update() can diff against its own old/new state. > + */ > +struct ssd16xx_plane_state { > + struct drm_shadow_plane_state base; > + enum ssd16xx_refresh_mode refresh_mode; /* Pending refresh mode */ > }; > > +#define to_ssd16xx_plane_state(s) \ > + container_of((s), struct ssd16xx_plane_state, base.base) > + > static inline struct ssd16xx_device *to_ssd16xx_device(struct drm_device > *drm) > { > return container_of(drm, struct ssd16xx_device, drm); > @@ -1430,11 +1443,91 @@ static void ssd16xx_plane_destroy(struct drm_plane > *plane) > drm_plane_cleanup(plane); > } > > +static void ssd16xx_plane_atomic_destroy_state(struct drm_plane *plane, > + struct drm_plane_state *state); > + > +static struct drm_plane_state *ssd16xx_plane_atomic_create_state(struct > drm_plane *plane) > +{ > + struct ssd16xx_device *device = plane_to_ssd16xx_device(plane); > + struct ssd16xx_plane_state *ssd_state; > + > + ssd_state = kzalloc_obj(*ssd_state, GFP_KERNEL); > + if (!ssd_state) > + return ERR_PTR(-ENOMEM); > + > + __drm_gem_shadow_plane_state_init(plane, &ssd_state->base); > + ssd_state->refresh_mode = device->refresh_mode; > + > + return &ssd_state->base.base; > +} [Severity: High] Does this compile when CONFIG_DRM_SSD16XX is enabled? In drivers/gpu/drm/solomon/ssd16xx.c, ssd16xx_plane_atomic_create_state() calls __drm_gem_shadow_plane_state_init(), which does not exist in include/drm/drm_gem_atomic_helper.h or anywhere else in the tree: include/drm/drm_gem_atomic_helper.h: void __drm_gem_duplicate_shadow_plane_state(struct drm_plane *plane, struct drm_shadow_plane_state *new_shadow_plane_state); void __drm_gem_destroy_shadow_plane_state(struct drm_shadow_plane_state *shadow_plane_state); void __drm_gem_reset_shadow_plane(struct drm_plane *plane, struct drm_shadow_plane_state *shadow_plane_state); Should this implement a plane .reset callback calling __drm_gem_reset_shadow_plane() instead of .atomic_create_state (and drop the unused forward declaration of ssd16xx_plane_atomic_destroy_state() above)? [ ... ] > @@ -1462,6 +1555,8 @@ static void ssd16xx_plane_atomic_update(struct > drm_plane *plane, > struct drm_plane_state *new_state = > drm_atomic_get_new_plane_state(state, plane); > struct drm_shadow_plane_state *shadow_state = > to_drm_shadow_plane_state(new_state); > struct ssd16xx_device *device = plane_to_ssd16xx_device(plane); > + struct ssd16xx_plane_state *new_ssd = to_ssd16xx_plane_state(new_state); > + struct ssd16xx_plane_state *old_ssd = to_ssd16xx_plane_state(old_state); > enum ssd16xx_refresh_mode saved_mode; > u8 saved_border_waveform_idx; > struct drm_framebuffer *fb = new_state->fb; > @@ -1495,6 +1590,25 @@ 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); > + > + /* 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; > + } > + > + device->refresh_mode = new_ssd->refresh_mode; > + > /* > * When refresh_mode_init was set, use the specified mode for this first > * frame only, then restore the user-configured refresh_mode so [Severity: High] Can ssd16xx_plane_atomic_update() in drivers/gpu/drm/solomon/ssd16xx.c miss preloading the Mode 1 LUT before executing SSD16XX_REFRESH_FAST? There appear to be three scenarios where the LUT state and plane state get out of sync: 1. If userspace sets refresh_mode to SSD16XX_REFRESH_FAST while the CRTC is inactive (where drm_atomic_helper_commit_tail_rpm() passes DRM_PLANE_COMMIT_ACTIVE_ONLY and skips ssd16xx_plane_atomic_update()), or when ssd16xx_plane_atomic_update() returns early before this diff check: drivers/gpu/drm/solomon/ssd16xx.c:ssd16xx_plane_atomic_update() { ... 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; ... } drm_atomic_helper_swap_state() has already updated plane->state to new_state, while device->refresh_mode and device->fast_lut_pending remain unchanged. On the next commit that actually draws a frame, ssd16xx_plane_atomic_duplicate_state() copies plane->state, so both old_ssd->refresh_mode and new_ssd->refresh_mode are SSD16XX_REFRESH_FAST. As a result, neither ssd16xx_crtc_atomic_enable() -> ssd16xx_hw_init() (which still sees the stale device->refresh_mode) nor ssd16xx_plane_atomic_update() (which sees old_ssd->refresh_mode == new_ssd->refresh_mode) will preload the fast LUT. 2. In ssd16xx_plane_atomic_update(), pm_runtime_resume_and_get() is called before device->refresh_mode is updated from new_ssd->refresh_mode. If the panel was in SSD16XX_REFRESH_FULL mode and entered runtime suspend, waking the device on a commit that switches from SSD16XX_REFRESH_FULL to SSD16XX_REFRESH_FAST runs: pm_runtime_resume_and_get() -> ssd16xx_pm_runtime_resume() -> ssd16xx_hw_init() ssd16xx_hw_init() resets the controller (wiping the LUT) and checks device->refresh_mode == SSD16XX_REFRESH_FAST, which is still SSD16XX_REFRESH_FULL, so it skips the LUT preload. Then ssd16xx_plane_atomic_update() checks old_ssd->refresh_mode != SSD16XX_REFRESH_FULL, which is false, and sets device->fast_lut_pending = false even though the controller was just reset. 3. Right below this hunk in ssd16xx_plane_atomic_update(), when device->init_refresh_pending is true on the first frame, device->refresh_mode is temporarily overridden with device->refresh_mode_init, but device->fast_lut_pending is still consumed and cleared before ssd16xx_fb_dirty() runs: drivers/gpu/drm/solomon/ssd16xx.c:ssd16xx_plane_atomic_update() { ... 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 refresh_mode_init is SSD16XX_REFRESH_PARTIAL, ssd16xx_fb_dirty() will load the Mode 2 partial LUT for that first frame, overwriting the preloaded Mode 1 LUT before subsequent SSD16XX_REFRESH_FAST frames execute. Would it be more reliable to track what LUT mode is currently loaded in the hardware (resetting that tracking in ssd16xx_hw_init() and updating it when ssd16xx_fb_dirty() or ssd16xx_preload_fast_lut() runs), rather than diffing old_ssd->refresh_mode against new_ssd->refresh_mode? [ ... ] > @@ -2057,6 +2194,9 @@ static int ssd16xx_probe(struct spi_device *spi) > if (ret) > return ret; > > + ret = ssd16xx_plane_create_properties(device); > + if (ret) > + return ret; > drm_mode_config_reset(drm); > > /* [ ... ] -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=8
