Re: [PATCH v2 08/14] drm/solomon: ssd16xx: Expose refresh mode as plane property

[sashiko-bot@kernel.org]

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 <devarsht@ti.com>

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/20260927182329.4193961-1-devarsht@ti.com?part=8