[PATCH] drm/mgag200: reject pixel clocks outside PLL range

[PATCH] drm/mgag200: reject pixel clocks outside PLL range

[Berkant Koc]

Commit 877507bb954e ("drm/mgag200: Provide per-device callbacks for
PIXPLLC") split the PLL compute code into per-model source files. The
g200se variant kept the existing permitteddelta sanity check and
returns -EINVAL when no register combination matches the requested
pixel clock within tolerance, which lets atomic_check reject the mode.

The matching helpers in g200wb, g200eh, g200er, g200ev, g200eh3 and
g200ew3 omit that check. When the search loop finds no candidate, m,
n, p and s stay at their initial zero values and the function still
returns success. atomic_update then programs those zero values into
the PIXPLLCM/N/P registers, which yields a corrupted output clock on
the affected models.

Apply the same permitteddelta test (clock * 5 / 1000, matching the
g200se reference) in all six helpers so that out-of-range modes are
rejected during atomic_check instead of producing garbage register
writes during atomic_update.

Fixes: 877507bb954e ("drm/mgag200: Provide per-device callbacks for PIXPLLC")
Cc: stable@vger.kernel.org # v6.1+
Signed-off-by: Berkant Koc <me@berkoc.com>
---

Build-tested with CONFIG_DRM_MGAG200=m, allmodconfig defaults; no new
warnings. checkpatch.pl --no-tree clean (0 errors, 0 warnings).

 drivers/gpu/drm/mgag200/mgag200_g200eh.c  | 8 +++++++-
 drivers/gpu/drm/mgag200/mgag200_g200eh3.c | 8 +++++++-
 drivers/gpu/drm/mgag200/mgag200_g200er.c  | 8 +++++++-
 drivers/gpu/drm/mgag200/mgag200_g200ev.c  | 8 +++++++-
 drivers/gpu/drm/mgag200/mgag200_g200ew3.c | 8 +++++++-
 drivers/gpu/drm/mgag200/mgag200_g200wb.c  | 8 +++++++-
 6 files changed, 42 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/mgag200/mgag200_g200eh.c b/drivers/gpu/drm/mgag200/mgag200_g200eh.c
index d2aa931f579d..2387ff87550a 100644
--- a/drivers/gpu/drm/mgag200/mgag200_g200eh.c
+++ b/drivers/gpu/drm/mgag200/mgag200_g200eh.c
@@ -51,13 +51,14 @@ static int mgag200_g200eh_pixpllc_atomic_check(struct drm_crtc *crtc,
 	struct mgag200_crtc_state *new_mgag200_crtc_state = to_mgag200_crtc_state(new_crtc_state);
 	long clock = new_crtc_state->mode.clock;
 	struct mgag200_pll_values *pixpllc = &new_mgag200_crtc_state->pixpllc;
-	unsigned int delta, tmpdelta;
+	unsigned int delta, tmpdelta, permitteddelta;
 	unsigned int testp, testm, testn;
 	unsigned int p, m, n, s;
 	unsigned int computed;
 
 	m = n = p = s = 0;
 	delta = 0xffffffff;
+	permitteddelta = clock * 5 / 1000;
 
 	for (testp = 16; testp > 0; testp >>= 1) {
 		if (clock * testp > vcomax)
@@ -82,6 +83,11 @@ static int mgag200_g200eh_pixpllc_atomic_check(struct drm_crtc *crtc,
 		}
 	}
 
+	if (delta > permitteddelta) {
+		pr_warn("PLL delta too large\n");
+		return -EINVAL;
+	}
+
 	pixpllc->m = m;
 	pixpllc->n = n;
 	pixpllc->p = p;
diff --git a/drivers/gpu/drm/mgag200/mgag200_g200eh3.c b/drivers/gpu/drm/mgag200/mgag200_g200eh3.c
index 7bea7a728f56..322e93982ea2 100644
--- a/drivers/gpu/drm/mgag200/mgag200_g200eh3.c
+++ b/drivers/gpu/drm/mgag200/mgag200_g200eh3.c
@@ -26,13 +26,14 @@ static int mgag200_g200eh3_pixpllc_atomic_check(struct drm_crtc *crtc,
 	struct mgag200_crtc_state *new_mgag200_crtc_state = to_mgag200_crtc_state(new_crtc_state);
 	long clock = new_crtc_state->mode.clock;
 	struct mgag200_pll_values *pixpllc = &new_mgag200_crtc_state->pixpllc;
-	unsigned int delta, tmpdelta;
+	unsigned int delta, tmpdelta, permitteddelta;
 	unsigned int testp, testm, testn;
 	unsigned int p, m, n, s;
 	unsigned int computed;
 
 	m = n = p = s = 0;
 	delta = 0xffffffff;
+	permitteddelta = clock * 5 / 1000;
 	testp = 0;
 
 	for (testm = 150; testm >= 6; testm--) {
@@ -59,6 +60,11 @@ static int mgag200_g200eh3_pixpllc_atomic_check(struct drm_crtc *crtc,
 			break;
 	}
 
+	if (delta > permitteddelta) {
+		pr_warn("PLL delta too large\n");
+		return -EINVAL;
+	}
+
 	pixpllc->m = m;
 	pixpllc->n = n;
 	pixpllc->p = p;
diff --git a/drivers/gpu/drm/mgag200/mgag200_g200er.c b/drivers/gpu/drm/mgag200/mgag200_g200er.c
index 8fa8fe943abf..e180db21902b 100644
--- a/drivers/gpu/drm/mgag200/mgag200_g200er.c
+++ b/drivers/gpu/drm/mgag200/mgag200_g200er.c
@@ -69,13 +69,14 @@ static int mgag200_g200er_pixpllc_atomic_check(struct drm_crtc *crtc,
 	struct mgag200_crtc_state *new_mgag200_crtc_state = to_mgag200_crtc_state(new_crtc_state);
 	long clock = new_crtc_state->mode.clock;
 	struct mgag200_pll_values *pixpllc = &new_mgag200_crtc_state->pixpllc;
-	unsigned int delta, tmpdelta;
+	unsigned int delta, tmpdelta, permitteddelta;
 	int testr, testn, testm, testo;
 	unsigned int p, m, n, s;
 	unsigned int computed, vco;
 
 	m = n = p = s = 0;
 	delta = 0xffffffff;
+	permitteddelta = clock * 5 / 1000;
 
 	for (testr = 0; testr < 4; testr++) {
 		if (delta == 0)
@@ -110,6 +111,11 @@ static int mgag200_g200er_pixpllc_atomic_check(struct drm_crtc *crtc,
 		}
 	}
 
+	if (delta > permitteddelta) {
+		pr_warn("PLL delta too large\n");
+		return -EINVAL;
+	}
+
 	pixpllc->m = m;
 	pixpllc->n = n;
 	pixpllc->p = p;
diff --git a/drivers/gpu/drm/mgag200/mgag200_g200ev.c b/drivers/gpu/drm/mgag200/mgag200_g200ev.c
index 3fadbeb10af9..0e882872e968 100644
--- a/drivers/gpu/drm/mgag200/mgag200_g200ev.c
+++ b/drivers/gpu/drm/mgag200/mgag200_g200ev.c
@@ -57,13 +57,14 @@ static int mgag200_g200ev_pixpllc_atomic_check(struct drm_crtc *crtc,
 	struct mgag200_crtc_state *new_mgag200_crtc_state = to_mgag200_crtc_state(new_crtc_state);
 	long clock = new_crtc_state->mode.clock;
 	struct mgag200_pll_values *pixpllc = &new_mgag200_crtc_state->pixpllc;
-	unsigned int delta, tmpdelta;
+	unsigned int delta, tmpdelta, permitteddelta;
 	unsigned int testp, testm, testn;
 	unsigned int p, m, n, s;
 	unsigned int computed;
 
 	m = n = p = s = 0;
 	delta = 0xffffffff;
+	permitteddelta = clock * 5 / 1000;
 
 	for (testp = 16; testp > 0; testp--) {
 		if (clock * testp > vcomax)
@@ -89,6 +90,11 @@ static int mgag200_g200ev_pixpllc_atomic_check(struct drm_crtc *crtc,
 		}
 	}
 
+	if (delta > permitteddelta) {
+		pr_warn("PLL delta too large\n");
+		return -EINVAL;
+	}
+
 	pixpllc->m = m;
 	pixpllc->n = n;
 	pixpllc->p = p;
diff --git a/drivers/gpu/drm/mgag200/mgag200_g200ew3.c b/drivers/gpu/drm/mgag200/mgag200_g200ew3.c
index e387a455eae5..da8f2a66cde3 100644
--- a/drivers/gpu/drm/mgag200/mgag200_g200ew3.c
+++ b/drivers/gpu/drm/mgag200/mgag200_g200ew3.c
@@ -33,13 +33,14 @@ static int mgag200_g200ew3_pixpllc_atomic_check(struct drm_crtc *crtc,
 	struct mgag200_crtc_state *new_mgag200_crtc_state = to_mgag200_crtc_state(new_crtc_state);
 	long clock = new_crtc_state->mode.clock;
 	struct mgag200_pll_values *pixpllc = &new_mgag200_crtc_state->pixpllc;
-	unsigned int delta, tmpdelta;
+	unsigned int delta, tmpdelta, permitteddelta;
 	unsigned int testp, testm, testn, testp2;
 	unsigned int p, m, n, s;
 	unsigned int computed;
 
 	m = n = p = s = 0;
 	delta = 0xffffffff;
+	permitteddelta = clock * 5 / 1000;
 
 	for (testp = 1; testp < 8; testp++) {
 		for (testp2 = 1; testp2 < 8; testp2++) {
@@ -68,6 +69,11 @@ static int mgag200_g200ew3_pixpllc_atomic_check(struct drm_crtc *crtc,
 		}
 	}
 
+	if (delta > permitteddelta) {
+		pr_warn("PLL delta too large\n");
+		return -EINVAL;
+	}
+
 	pixpllc->m = m;
 	pixpllc->n = n;
 	pixpllc->p = p;
diff --git a/drivers/gpu/drm/mgag200/mgag200_g200wb.c b/drivers/gpu/drm/mgag200/mgag200_g200wb.c
index d847fa8ded8c..ca4775173ff0 100644
--- a/drivers/gpu/drm/mgag200/mgag200_g200wb.c
+++ b/drivers/gpu/drm/mgag200/mgag200_g200wb.c
@@ -49,13 +49,14 @@ static int mgag200_g200wb_pixpllc_atomic_check(struct drm_crtc *crtc,
 	struct mgag200_crtc_state *new_mgag200_crtc_state = to_mgag200_crtc_state(new_crtc_state);
 	long clock = new_crtc_state->mode.clock;
 	struct mgag200_pll_values *pixpllc = &new_mgag200_crtc_state->pixpllc;
-	unsigned int delta, tmpdelta;
+	unsigned int delta, tmpdelta, permitteddelta;
 	unsigned int testp, testm, testn;
 	unsigned int p, m, n, s;
 	unsigned int computed;
 
 	m = n = p = s = 0;
 	delta = 0xffffffff;
+	permitteddelta = clock * 5 / 1000;
 
 	for (testp = 1; testp < 9; testp++) {
 		if (clock * testp > vcomax)
@@ -81,6 +82,11 @@ static int mgag200_g200wb_pixpllc_atomic_check(struct drm_crtc *crtc,
 		}
 	}
 
+	if (delta > permitteddelta) {
+		pr_warn("PLL delta too large\n");
+		return -EINVAL;
+	}
+
 	pixpllc->m = m;
 	pixpllc->n = n;
 	pixpllc->p = p;
-- 

Claude review: drm/mgag200: reject pixel clocks outside PLL range

[Claude Code Review Bot]

Overall Series Review

Subject: drm/mgag200: reject pixel clocks outside PLL range
Author: Berkant Koc <me@berkoc.com>
Patches: 1
Reviewed: 2026-05-18T16:11:33.330911

---

This is a single patch that adds a `permitteddelta` sanity check to six mgag200 PLL compute helpers (g200wb, g200eh, g200er, g200ev, g200eh3, g200ew3), matching what g200se already has. The problem is real: without the check, when no PLL register combination matches the requested pixel clock, `m`, `n`, `p`, `s` remain at zero, and `atomic_update` programs garbage into the PIXPLLC registers.

The fix is correct and mechanically consistent with the g200se reference. The `Fixes:` tag and `Cc: stable` are appropriate. One style concern worth raising, and one minor functional observation.

---
Generated by Claude Code Patch Reviewer

Claude review: drm/mgag200: reject pixel clocks outside PLL range

[Claude Code Review Bot]

Patch Review

**Correctness: Good.** The check is placed at exactly the right spot — after the search loop exhausts all candidates and before the results are written to `pixpllc->m/n/p/s`. The tolerance formula `clock * 5 / 1000` (0.5% of the requested clock) matches the g200se reference at `mgag200_g200se.c:140` and `mgag200_g200se.c:232`. Verified across all six files that the insertion point is between the end of the search loop and the `pixpllc->m = m` assignment.

**Why the bug matters:** In `atomic_update`, the register values are derived as e.g. `pixpllcm = pixpllc->m - 1` (`mgag200_g200eh.c:112`). With `m = 0`, this underflows to `0xffffffff` (truncated to a `u8`), producing corrupted PLL register writes.

**Style concern — `pr_warn` vs `drm_warn`:** The patch uses bare `pr_warn("PLL delta too large\n")` to match the g200se reference. However, the rest of the driver consistently uses `drm_err`/`drm_warn`/`drm_dbg` (which prepend device identification). A `struct drm_device *` is available in `atomic_check` via `crtc->dev`. This is a pre-existing issue in g200se too, but since six new call sites are being added, it would be worth upgrading to:

```c
drm_warn(crtc->dev, "PLL delta too large\n");
```

across all seven sites (the six new ones plus the existing g200se). This makes log messages attributable to a specific device, which matters on multi-GPU or multi-BMC systems.

**Minor functional observation:** The `pr_warn` fires during `atomic_check`, which is a validation path — userspace probing supported modes will trigger this message for any unsupported clock. This could be chatty. A `drm_dbg_kms` might be more appropriate, since the `-EINVAL` return already communicates the rejection to the DRM core. The g200se code has the same issue, so this is more of a follow-up suggestion than a blocker for this patch.

**No logic bugs found.** The patch is a straightforward, correct backport of the existing g200se pattern to the six files that were missing it. The integer arithmetic is safe for the clock ranges these PLLs support (VCO maxima are all under 1.6 GHz = 1,600,000 kHz, so `clock * 5` won't overflow a `long`).

**Verdict:** The patch fixes a real bug with the right approach. Suggest switching `pr_warn` to `drm_dbg_kms(crtc->dev, ...)` (or at minimum `drm_warn(crtc->dev, ...)`), but the logic itself is correct and ready.

---
Generated by Claude Code Patch Reviewer