Claude review: drm: client: add splash client

[Claude Code Review Bot <claude-review@example.com>]

Patch Review

**Critical: Crash on unregister if hotplug never fires**

`drm_splash_client_unregister()` unconditionally calls `kthread_stop(splash->thread)`, but `splash->thread` is only assigned in `drm_splash_client_hotplug()`. If the device is removed before any hotplug event, `splash->thread` is NULL (kzalloc zero-init), and `kthread_stop(NULL)` will crash.

```c
+static void drm_splash_client_unregister(struct drm_client_dev *client)
+{
+	struct drm_splash *splash = client_to_drm_splash(client);
+
+	kthread_stop(splash->thread);
```

Fix: guard with `if (splash->thread)`, or check `splash->initialized`.

---

**Critical: Out-of-bounds read in BMP pixel blitting**

All three blit functions (`drm_splash_blit_pix16/24/32`) read 4 bytes from 3-byte-per-pixel BMP data:

```c
+			scolor = *(const u32 *)(&sbuf8[src_offset + 3 * x]);
```

This reads one byte beyond each pixel's data. On the last pixel of each row, this can read past the source buffer entirely. Additionally, this is an unaligned access cast through a pointer, which is undefined behavior and will fault on some architectures (ARM, etc.). The patch cover letter mentions testing on Beagleplay and i.MX93 (both ARM), so this is surprising.

Use `get_unaligned_le32()` or better yet, manually compose the 3-byte pixel:
```c
scolor = sbuf8[src_offset + 3*x] |
         (sbuf8[src_offset + 3*x + 1] << 8) |
         (sbuf8[src_offset + 3*x + 2] << 16);
```

---

**Bug: Render thread sleep/wake race**

The sleep pattern at the bottom of the render loop has a classic race:

```c
+		if (atomic_xchg(&splash->pending, 0) == 0)
+			set_current_state(TASK_UNINTERRUPTIBLE);
+
+		schedule();
```

Between `atomic_xchg` returning 0 and `set_current_state`, a wakeup from `drm_splash_fw_callback` can be lost because the task state is still `TASK_RUNNING` when `wake_up_process` is called. The fix is the standard pattern: set the state *before* testing the condition:

```c
set_current_state(TASK_UNINTERRUPTIBLE);
if (atomic_xchg(&splash->pending, 0) != 0)
    __set_current_state(TASK_RUNNING);
schedule();
```

---

**Bug: Tiled display dimension accumulation looks swapped**

```c
+			tiled = get_scanout_from_tile_group(splash, new_id);
+			if (tiled != NULL) {
+				if (!modeset->x)
+					tiled->width += modeset->mode->vdisplay;
+				if (!modeset->y)
+					tiled->height += modeset->mode->hdisplay;
```

When `modeset->x == 0` (left-edge tile), the code adds `vdisplay` to `width`, but that should presumably be `hdisplay`. Similarly, when `modeset->y == 0` (top-edge tile), it adds `hdisplay` to `height`, but that should be `vdisplay`. These appear to be swapped.

---

**Bug: `modeset_mask` / scanout index tracking is broken**

In `drm_splash_init_client`, the variable `j` is used as both a modeset counter and scanout index, but it increments for every modeset (even skipped tiled ones via `continue`), while scanouts are added via `splash->n_scanout++`:

```c
+	j = 0;
+	drm_client_for_each_modeset(modeset, client) {
+		...
+		if (modeset->connectors[0]->has_tile) {
+			...
+			tiled = get_scanout_from_tile_group(splash, new_id);
+			if (tiled != NULL) {
+				...
+				continue;  // j not incremented here
+			}
+			id = new_id;
+		}
+		...
+		modeset_mask |= BIT(j);
+		j++;
+	}
```

Then in the second loop, `j` is used to index both `modeset_mask` bits and `splash->scanout[]`:

```c
+	j = 0;
+	drm_client_for_each_modeset(modeset, client) {
+		if (!(modeset_mask & BIT(j)))
+			continue;
+		scanout = &splash->scanout[j];
+		j++;
```

If any modeset was skipped (no mode, or continued due to tile dedup), `j` in the second loop won't match the scanout array index. The scanout array is dense (indexed 0..n_scanout-1) but `modeset_mask` bits correspond to the first-loop's `j` which skips some values. A separate scanout index counter is needed in the second loop.

---

**Minor: `drm_splash_fill_solid_color` ignores error from `drm_splash_fill_solid_color` in render thread**

```c
+			if (!scanout->bg_drawn) {
+				...
+				drm_splash_fill_solid_color(scanout->buffer, color);
+				scanout->bg_drawn = true;
```

The return value is ignored. If the vmap fails, `bg_drawn` is still set to true and the background will never be retried.

---

**Minor: Variable declaration after statement (C90 style)**

```c
+			if (!scanout->bg_drawn) {
+				drm_dbg(client->dev, "draw background for scanout %d", i);
+				u32 color = drm_draw_color_from_xrgb8888(splash_color,
+									 scanout->format);
```

While mixed declarations are allowed in kernel code (C11), this specific pattern — a declaration after a function call (`drm_dbg`) within the same block — may trigger warnings with some compiler configurations. Consider moving the declaration to the top of the block.

---

**Minor: `splash_color` module_param type mismatch**

```c
+static unsigned int splash_color = CONFIG_DRM_CLIENT_SPLASH_BACKGROUND_COLOR;
+module_param(splash_color, uint, 0400);
```

`CONFIG_DRM_CLIENT_SPLASH_BACKGROUND_COLOR` is a `hex` Kconfig type which produces an integer literal. This works, but the module parameter uses `uint` (decimal input), while users might expect to pass hex values on the command line. Consider documenting this or using a custom param type.

---

**Minor: `drm_splash_image_cleanup` always calls `memunmap`**

```c
+static void drm_splash_image_cleanup(struct drm_splash *splash)
+{
+	memunmap(splash->map_data);
```

`memunmap(NULL)` is safe, so this is functionally fine, but it's called from both the render thread (after image load) and `drm_splash_client_unregister`. If the render thread already cleaned up, `map_data` should be NULL. This is fine but worth noting — there's no double-free risk since `memunmap` is idempotent on NULL, but `release_firmware` with NULL is also safe. This path is OK.

---

**Nit: Comment style**

Several multi-line comments use the `/* text` style rather than the preferred kernel style:

```c
+		/* If commit returns EBUSY, another master showed up.
+		 * This means that the splash is no more required.
+		 */
```

Kernel preferred style has `/*` on its own line for multi-line comments.

---

---
Generated by Claude Code Patch Reviewer