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

[sashiko-bot@kernel.org]

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

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