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: sta...@vger.kernel.org # v6.1+
Signed-off-by: Berkant Koc <m...@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)