[PATCH] drm/atomic: determine the hotspots attribute first

[PATCH] drm/atomic: determine the hotspots attribute first

[yaolu]

From: Lu Yao <yaolu@kylinos.cn>

If drm driver has both 'DRIVER_CURSOR_HOTSPOT' and
'atomic_set/get_property' will cause hotspots property
set/get error.

Signed-off-by: Lu Yao <yaolu@kylinos.cn>
---
 drivers/gpu/drm/drm_atomic_uapi.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
index 9e9b2f3f106c..21b6b79f5f9e 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -548,9 +548,6 @@ static int drm_atomic_plane_set_property(struct drm_plane *plane,
 		return ret;
 	} else if (property == plane->scaling_filter_property) {
 		state->scaling_filter = val;
-	} else if (plane->funcs->atomic_set_property) {
-		return plane->funcs->atomic_set_property(plane, state,
-				property, val);
 	} else if (property == plane->hotspot_x_property) {
 		if (plane->type != DRM_PLANE_TYPE_CURSOR) {
 			drm_dbg_atomic(plane->dev,
@@ -567,6 +564,9 @@ static int drm_atomic_plane_set_property(struct drm_plane *plane,
 			return -EINVAL;
 		}
 		state->hotspot_y = val;
+	} else if (plane->funcs->atomic_set_property) {
+		return plane->funcs->atomic_set_property(plane, state,
+				property, val);
 	} else {
 		drm_dbg_atomic(plane->dev,
 			       "[PLANE:%d:%s] unknown property [PROP:%d:%s]\n",
@@ -625,12 +625,12 @@ drm_atomic_plane_get_property(struct drm_plane *plane,
 			state->fb_damage_clips->base.id : 0;
 	} else if (property == plane->scaling_filter_property) {
 		*val = state->scaling_filter;
-	} else if (plane->funcs->atomic_get_property) {
-		return plane->funcs->atomic_get_property(plane, state, property, val);
 	} else if (property == plane->hotspot_x_property) {
 		*val = state->hotspot_x;
 	} else if (property == plane->hotspot_y_property) {
 		*val = state->hotspot_y;
+	} else if (plane->funcs->atomic_get_property) {
+		return plane->funcs->atomic_get_property(plane, state, property, val);
 	} else {
 		drm_dbg_atomic(dev,
 			       "[PLANE:%d:%s] unknown property [PROP:%d:%s]\n",
-- 
2.25.1

Claude review: drm/atomic: determine the hotspots attribute first

[Claude Code Review Bot]

Patch Review

**Correctness: Good.** The reordering is correct in both `drm_atomic_plane_set_property()` and `drm_atomic_plane_get_property()`. The `hotspot_x_property` / `hotspot_y_property` checks are moved above the `plane->funcs->atomic_set/get_property` fallback, which is the right position — core properties should always be matched before falling through to driver callbacks.

In the set path (line 633-635 after patch):
```c
	} else if (plane->funcs->atomic_set_property) {
		return plane->funcs->atomic_set_property(plane, state,
				property, val);
```
Now correctly sits after all core property checks and just before the final `else` error branch. Same pattern applies to the get path (lines 700-701).

**Issues to address:**

1. **Commit message is vague and unclear.** "determine the hotspots attribute first" doesn't accurately describe the change. A better subject would be something like:
   > `drm/atomic: check hotspot properties before driver callback fallback`

   The body text is also unclear:
   > "If drm driver has both 'DRIVER_CURSOR_HOTSPOT' and 'atomic_set/get_property' will cause hotspots property set/get error."

   This should explain *why* the error occurs: the driver's `atomic_set_property` callback matches the hotspot property before the core code can handle it, and the driver callback doesn't know how to handle it, so it returns an error. A suggested rewrite:

   > The driver's `atomic_set/get_property` callback is a catch-all for
   > driver-specific properties. It was checked before the hotspot
   > properties in the if-else chain, so for any driver that implements
   > both `DRIVER_CURSOR_HOTSPOT` and `plane->funcs->atomic_set_property`,
   > the hotspot properties would be dispatched to the driver callback
   > instead of being handled by the core code, resulting in errors.
   >
   > Move the hotspot property checks before the driver callback fallback,
   > consistent with all other core property checks in the chain.

2. **Missing `Fixes:` tag.** This fixes a bug introduced when hotspot support was added (commit `4f49a260d4e5` "drm/atomic: Add support for mouse hotspots" or similar — the commit that added the hotspot handling to `drm_atomic_uapi.c`). A Fixes tag is needed for stable backport consideration.

3. **No indication of testing.** The commit message should mention how this was tested, or at least which driver exhibited the problem (if any). If this was found by code inspection, say so.

**Verdict:** The code change itself is correct and clean — no functional objection. The commit message and metadata need rework before this is merge-ready. Request the author to v2 with improved commit message, a Fixes tag, and a note about how this was found/tested.

---
Generated by Claude Code Patch Reviewer

Claude review: drm/atomic: determine the hotspots attribute first

[Claude Code Review Bot]

Overall Series Review

Subject: drm/atomic: determine the hotspots attribute first
Author: yaolu@kylinos.cn
Patches: 1
Reviewed: 2026-05-16T15:55:01.344626

---

This is a single-patch series that reorders the `if-else` chain in `drm_atomic_plane_set_property()` and `drm_atomic_plane_get_property()` so that the hotspot property checks happen *before* the driver `atomic_set/get_property` callback fallback.

**The fix is logically correct.** The driver callback is a catch-all fallback for driver-specific properties and should always come last, just before the "unknown property" error case. Having it in the middle of core property checks was a latent bug — if any driver with `DRIVER_CURSOR_HOTSPOT` also implemented `plane->funcs->atomic_set_property`, the hotspot properties would be swallowed by the driver callback and never reach the core handling code.

**Practical impact is currently low.** I checked the four drivers that set `DRIVER_CURSOR_HOTSPOT` (vmwgfx, virtio-gpu, vboxvideo, qxl) and none of them implement plane-level `atomic_set/get_property`. vmwgfx has connector-level callbacks only. So this is a defensive fix against a currently-theoretical bug, but still worth doing — the ordering was clearly wrong and fragile.

**The commit message and metadata need significant improvement** before this should be merged.

---
Generated by Claude Code Patch Reviewer