[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;
-- 

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

[Thomas Zimmermann]

Hi

Am 17.05.26 um 15:03 schrieb 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")

The other models never did this check. Why is this now a problem?

Best regards
Thomas

> 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;

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
GF: Jochen Jaser, Andrew McDonald, Werner Knoblich, (HRB 36809, AG Nürnberg)

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

[Berkant Koc]

> The other models never did this check. Why is this now a problem?

You are right, and my Fixes tag is wrong. I checked the split again.
877507bb954e moved the per-model compute routines into separate files; it
did not remove a check. Only g200se ever had the permitteddelta guard (the
0.5% VESA comment lives only there). The other six helpers never had it,
before or after the split. So this is not a regression. Please drop the
Fixes tag and the stable Cc; I should not have framed it that way.

What remains is a latent robustness gap, not a bug users hit.
mgag200_crtc_helper_mode_valid does not bound the pixel clock, so a
hand-crafted out-of-range modeline can reach the search loop, match
nothing, and leave m/n/p/s at 0. atomic_update then does pixpllc->m - 1,
which underflows, and programs garbage into PIXPLLC. Normal EDID modes stay
inside the VCO window and never trigger this.

If you think hardening the six helpers to match g200se is still worth it, I
will resend without the Fixes/stable tags. Do you prefer a per-helper guard
like g200se, or one shared check after the loop? I will follow your call on
scope.

Thanks for catching the framing.

Berkant Koc

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

[Thomas Zimmermann]

Hi

Am 23.05.26 um 15:58 schrieb Berkant Koc:
>> The other models never did this check. Why is this now a problem?
> You are right, and my Fixes tag is wrong. I checked the split again.
> 877507bb954e moved the per-model compute routines into separate files; it
> did not remove a check. Only g200se ever had the permitteddelta guard (the
> 0.5% VESA comment lives only there). The other six helpers never had it,
> before or after the split. So this is not a regression. Please drop the
> Fixes tag and the stable Cc; I should not have framed it that way.
>
> What remains is a latent robustness gap, not a bug users hit.
> mgag200_crtc_helper_mode_valid does not bound the pixel clock, so a
> hand-crafted out-of-range modeline can reach the search loop, match
> nothing, and leave m/n/p/s at 0. atomic_update then does pixpllc->m - 1,
> which underflows, and programs garbage into PIXPLLC. Normal EDID modes stay
> inside the VCO window and never trigger this.
>
> If you think hardening the six helpers to match g200se is still worth it, I
> will resend without the Fixes/stable tags. Do you prefer a per-helper guard
> like g200se, or one shared check after the loop? I will follow your call on
> scope.

Adding that test could possibly regress someone's system. If they run 
the display at a higher delta than 5%, it might still work.

Before rejecting those modes, there needs to be a check in the CRTC's 
mode_valid [1] against the limits of PIXPLLC, similar to 
pixpllc_atomic_check. That would filter out display modes that are not 
supported anyway.

https://elixir.bootlin.com/linux/v7.0.10/source/drivers/gpu/drm/mgag200/mgag200_mode.c#L581

Best regards
Thomas

>
> Thanks for catching the framing.
>
> Berkant Koc

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
GF: Jochen Jaser, Andrew McDonald, Werner Knoblich, (HRB 36809, AG Nürnberg)