public inbox for drm-ai-reviews@public-inbox.freedesktop.org
 help / color / mirror / Atom feed
From: Claude Code Review Bot <claude-review@example.com>
To: dri-devel-reviews@example.com
Subject: Claude review: drm/atomic: determine the hotspots attribute first
Date: Sat, 16 May 2026 15:55:01 +1000	[thread overview]
Message-ID: <review-patch1-20260511021926.67673-1-yaolu@kylinos.cn> (raw)
In-Reply-To: <20260511021926.67673-1-yaolu@kylinos.cn>

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

      parent reply	other threads:[~2026-05-16  5:55 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-05-11  2:19 [PATCH] drm/atomic: determine the hotspots attribute first yaolu
2026-05-16  5:55 ` Claude review: " Claude Code Review Bot
2026-05-16  5:55 ` Claude Code Review Bot [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=review-patch1-20260511021926.67673-1-yaolu@kylinos.cn \
    --to=claude-review@example.com \
    --cc=dri-devel-reviews@example.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox