Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] drm/ssd16xx: hardware desync when refresh_mode is updated on disabled plane - [High] drm/ssd16xx: double vunmap risk due to kmemdup of shadow plane state - [Low] drm/ssd16xx: build failure due to undefined __drm_gem_shadow_plane_state_init --
commit f164596b389c43d5ba1998c30d9e04e66112e4d1 Author: Devarsh Thakkar <[email protected]> drm/solomon: ssd16xx: Expose refresh mode as plane property This commit adds a DRM plane property "refresh_mode" to allow userspace to select between partial, full, and fast display update modes at runtime. It uses an enum to match the internal refresh modes and updates the default setting from the panel configuration. > diff --git a/drivers/gpu/drm/solomon/ssd16xx.c > b/drivers/gpu/drm/solomon/ssd16xx.c > index 6255f434248ae..dc2b8fe536af3 100644 > --- a/drivers/gpu/drm/solomon/ssd16xx.c > +++ b/drivers/gpu/drm/solomon/ssd16xx.c [ ... ] > @@ -1397,11 +1409,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(sizeof(*ssd_state), GFP_KERNEL); > + if (!ssd_state) > + return ERR_PTR(-ENOMEM); > + > + __drm_gem_shadow_plane_state_init(plane, &ssd_state->base); [Severity: Low] Does this cause a build failure? It appears __drm_gem_shadow_plane_state_init() is not defined anywhere in the kernel. If the module were somehow compiled, would failing to initialize the base plane state leave plane_state->plane as NULL and cause a NULL pointer dereference in the core DRM atomic helpers? > + ssd_state->refresh_mode = device->refresh_mode; > + > + return &ssd_state->base.base; > +} > + > +static struct drm_plane_state * > +ssd16xx_plane_atomic_duplicate_state(struct drm_plane *plane) > +{ > + struct ssd16xx_plane_state *old_ssd_state, *ssd_state; > + > + if (WARN_ON(!plane->state)) > + return NULL; > + > + old_ssd_state = to_ssd16xx_plane_state(plane->state); > + ssd_state = kmemdup(old_ssd_state, sizeof(*ssd_state), GFP_KERNEL); [Severity: High] Does this kmemdup() risk a double free or double unmap of the shadow plane state? The struct drm_shadow_plane_state contains map and data arrays which track active kernel memory mappings of the framebuffer. These are explicitly documented as "Transitional state - do not export or duplicate". If the new state encounters an error before drm_gem_begin_shadow_fb_access() overwrites them, won't the error cleanup paths unmap those stale pointers, resulting in a double vunmap of the old state's memory? > + if (!ssd_state) > + return NULL; > + > + __drm_gem_duplicate_shadow_plane_state(plane, &ssd_state->base); > + return &ssd_state->base.base; > +} [ ... ] > @@ -1456,6 +1550,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; [Severity: High] Can this hardware configuration desynchronize from the software state, leading to a missing LUT preload and corrupted display output? Earlier in ssd16xx_plane_atomic_update(), there is an early return when the plane is being disabled: if (!fb) return; If a userspace atomic commit disables the plane while simultaneously changing the refresh_mode property, won't this early return skip the state synchronization here? When the plane is later enabled, wouldn't pm_runtime_resume() call ssd16xx_hw_init(), read the stale device->refresh_mode, and skip the required FAST mode LUT preload? > + > /* > * When refresh_mode_init was set, use the specified mode for this first > * frame only, then restore the user-configured refresh_mode so -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=8
