From: Thomas Zimmermann <tzimmermann@suse.de>
To: Berkant Koc <me@berkoc.com>, dri-devel@lists.freedesktop.org
Cc: Jocelyn Falempe <jfalempe@redhat.com>,
Dave Airlie <airlied@redhat.com>,
Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
Maxime Ripard <mripard@kernel.org>,
Simona Vetter <simona@ffwll.ch>, Sam Ravnborg <sam@ravnborg.org>,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH] drm/mgag200: reject pixel clocks outside PLL range
Date: Fri, 22 May 2026 10:16:06 +0200 [thread overview]
Message-ID: <041bae47-bb5d-4db7-9435-f6c5d9c2fda1@suse.de> (raw)
In-Reply-To: <28e476e896dafdd31518dc5360ab7766@berkoc.com>
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)
next prev parent reply other threads:[~2026-05-22 8:16 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-05-17 13:03 Berkant Koc
2026-05-22 8:16 ` Thomas Zimmermann [this message]
2026-05-23 13:58 ` Berkant Koc
2026-05-29 8:29 ` Thomas Zimmermann
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=041bae47-bb5d-4db7-9435-f6c5d9c2fda1@suse.de \
--to=tzimmermann@suse.de \
--cc=airlied@redhat.com \
--cc=dri-devel@lists.freedesktop.org \
--cc=jfalempe@redhat.com \
--cc=linux-kernel@vger.kernel.org \
--cc=maarten.lankhorst@linux.intel.com \
--cc=me@berkoc.com \
--cc=mripard@kernel.org \
--cc=sam@ravnborg.org \
--cc=simona@ffwll.ch \
/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
all inboxes | Powered by JetHome®