All of lore.kernel.org
 help / color / mirror / Atom feed
From: Thomas Zimmermann <tzimmermann@suse.de>
To: kraxel@redhat.com, airlied@redhat.com, daniel.vetter@ffwll.ch,
	yc_chen@aspeedtech.com, sam@ravnborg.org
Cc: Thomas Zimmermann <tzimmermann@suse.de>, dri-devel@lists.freedesktop.org
Subject: [PATCH v3 1/7] drm/ast: Move modesetting code to CRTC's atomic_flush()
Date: Mon,  2 Dec 2019 12:15:51 +0100	[thread overview]
Message-ID: <20191202111557.15176-2-tzimmermann@suse.de> (raw)
In-Reply-To: <20191202111557.15176-1-tzimmermann@suse.de>

When enabling the CRTC after waking up from a power-saving mode, the
primary plane's framebuffer might be NULL, which leads to a stack trace
as shown below.

  [  632.624608] BUG: kernel NULL pointer dereference, address: 0000000000000048
  [  632.624631] #PF: supervisor read access in kernel mode
  [  632.624639] #PF: error_code(0x0000) - not-present page
  [  632.624647] PGD 0 P4D 0
  [  632.624654] Oops: 0000 [#1] SMP PTI
  [  632.624662] CPU: 0 PID: 2082 Comm: gnome-shell Tainted: G            E     5.4.0-rc7-1-default+ #114
  [  632.624673] Hardware name: Sun Microsystems SUN FIRE X2270 M2/SUN FIRE X2270 M2, BIOS 2.05    07/01/2010
  [  632.624689] RIP: 0010:ast_crtc_helper_atomic_enable+0x7d/0x680 [ast]
  [  632.624698] Code: 48 8b 80 e0 02 00 00 4c 8b 60 10 31 c0 f3 48 ab 48 8b 83 78 04 00 00 4c 89 ef 48 8d 70 18 e8 9a e9 55 ce 48 8b 83 78 04 00 00 <49> 8b 7c 24 48 4c 89 ea 4c 8d 44 24 28 48 8d 4c 24 20 48 8d 70 18
  [  632.624718] RSP: 0018:ffffbe9ec123fa40 EFLAGS: 00010246
  [  632.624726] RAX: ffff95a13cfd3400 RBX: ffff95a13cf32000 RCX: 0000000000000000
  [  632.624735] RDX: 0000000000000000 RSI: ffff95a13cfd34e8 RDI: ffffbe9ec123fb40
  [  632.624744] RBP: ffffbe9ec123fb80 R08: 0000000000000000 R09: 0000000000000003
  [  632.624753] R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000000
  [  632.624762] R13: ffffbe9ec123fa70 R14: ffff95a13beb7000 R15: ffff95a13cf32800
  [  632.624772] FS:  00007f6d2763e140(0000) GS:ffff95a134000000(0000) knlGS:0000000000000000
  [  632.624782] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
  [  632.624790] CR2: 0000000000000048 CR3: 00000001192f8004 CR4: 00000000000206f0
  [  632.624800] Call Trace:
  [  632.624811]  ? __lock_acquire+0x409/0x7c0
  [  632.624830]  drm_atomic_helper_commit_modeset_enables+0x1af/0x200
  [  632.624840]  drm_atomic_helper_commit_tail+0x32/0x70
  [  632.624849]  commit_tail+0xc7/0x110
  [  632.624857]  drm_atomic_helper_commit+0x121/0x130
  [  632.624867]  drm_atomic_connector_commit_dpms+0xd7/0x100
  [  632.624878]  set_property_atomic+0xaf/0x110
  [  632.624890]  drm_mode_obj_set_property_ioctl+0xbb/0x190
  [  632.624899]  ? drm_mode_obj_find_prop_id+0x40/0x40
  [  632.624909]  drm_ioctl_kernel+0x86/0xd0
  [  632.624918]  drm_ioctl+0x1e4/0x36b
  [  632.624925]  ? drm_mode_obj_find_prop_id+0x40/0x40
  [  632.624939]  do_vfs_ioctl+0x4bd/0x6e0
  [  632.624949]  ksys_ioctl+0x5e/0x90
  [  632.624957]  __x64_sys_ioctl+0x16/0x20
  [  632.624966]  do_syscall_64+0x5a/0x220
  [  632.624976]  entry_SYSCALL_64_after_hwframe+0x49/0xbe
  [  632.624984] RIP: 0033:0x7f6d2b0de387
  [  632.624991] Code: 00 00 90 48 8b 05 f9 9a 0c 00 64 c7 00 26 00 00 00 48 c7 c0 ff ff ff ff c3 66 2e 0f 1f 84 00 00 00 00 00 b8 10 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d c9 9a 0c 00 f7 d8 64 89 01 48
  [  632.625011] RSP: 002b:00007fffb49def38 EFLAGS: 00000246 ORIG_RAX: 0000000000000010
  [  632.625021] RAX: ffffffffffffffda RBX: 00007fffb49def70 RCX: 00007f6d2b0de387
  [  632.625030] RDX: 00007fffb49def70 RSI: 00000000c01864ba RDI: 0000000000000009
  [  632.625040] RBP: 00000000c01864ba R08: 0000000000000000 R09: 00000000c0c0c0c0
  [  632.625049] R10: 0000000000000030 R11: 0000000000000246 R12: 000055bc367eb920
  [  632.625058] R13: 0000000000000009 R14: 0000000000000002 R15: 0000000000000000
  [  632.625071] Modules linked in: ebtable_filter(E) ebtables(E) ip6table_filter(E) ip6_tables(E) iptable_filter(E) ip_tables(E) x_tables(E) af_packet(E) scsi_transport_iscsi(E) dmi_sysfs(E) msr(E) xfs(E) intel_powerclamp(E) coretemp(E) k)
  [  632.625185] CR2: 0000000000000048

The STR is

	* start gdm and wait for it to switch off the display
	* wake up the display by pressing a key

CRTC modesetting depends on the new state of the CRTC and the primary
plane's framebuffer. The bugfix moves the modesetting code into the
CRTC's atomic_flush() function, where it is protected from the plane's
framebuffer being NULL.

The CRTC's atomic-enable function, which is the modesetting's original
location, still contains DPMS state handling. It's exactly the inverse
of the atomic-disable function.

v3:
	* protect modesetting from from fb == NULL
v2:
	* do an atomic check for plane
	* reject invisible primary planes

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Fixes: b48e1b6ffd28 ("drm/ast: Add CRTC helpers for atomic modesetting")
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Dave Airlie <airlied@redhat.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: "Y.C. Chen" <yc_chen@aspeedtech.com>
Cc: Sam Ravnborg <sam@ravnborg.org>
---
 drivers/gpu/drm/ast/ast_mode.c | 31 ++++++++++---------------------
 1 file changed, 10 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
index 4725ec911a66..33aca817b686 100644
--- a/drivers/gpu/drm/ast/ast_mode.c
+++ b/drivers/gpu/drm/ast/ast_mode.c
@@ -790,6 +790,8 @@ static void ast_crtc_helper_atomic_begin(struct drm_crtc *crtc,
 static void ast_crtc_helper_atomic_flush(struct drm_crtc *crtc,
 					 struct drm_crtc_state *old_crtc_state)
 {
+	struct drm_device *dev = crtc->dev;
+	struct ast_private *ast = dev->dev_private;
 	const struct drm_framebuffer *fb = crtc->primary->state->fb;
 	struct drm_display_mode adjusted_mode;
 	struct ast_vbios_mode_info vbios_mode;
@@ -800,36 +802,18 @@ static void ast_crtc_helper_atomic_flush(struct drm_crtc *crtc,
 	if (!fb)
 		return;
 
-	ast_set_color_reg(crtc, fb);
-
 	memset(&adjusted_mode, 0, sizeof(adjusted_mode));
 	drm_mode_copy(&adjusted_mode, &crtc->state->adjusted_mode);
 
 	succ = ast_get_vbios_mode_info(fb, &crtc->state->adjusted_mode,
 				       &adjusted_mode, &vbios_mode);
 	if (WARN_ON_ONCE(!succ))
-		return;
+		return; /* BUG: didn't validate this in atomic_check() */
 
+	ast_set_color_reg(crtc, fb);
 	ast_set_vbios_color_reg(crtc, fb, &vbios_mode);
-}
 
-static void
-ast_crtc_helper_atomic_enable(struct drm_crtc *crtc,
-			      struct drm_crtc_state *old_crtc_state)
-{
-	struct drm_device *dev = crtc->dev;
-	struct ast_private *ast = crtc->dev->dev_private;
-	const struct drm_framebuffer *fb = crtc->primary->state->fb;
-	struct drm_display_mode adjusted_mode;
-	struct ast_vbios_mode_info vbios_mode;
-	bool succ;
-
-	memset(&adjusted_mode, 0, sizeof(adjusted_mode));
-	drm_mode_copy(&adjusted_mode, &crtc->state->adjusted_mode);
-
-	succ = ast_get_vbios_mode_info(fb, &crtc->state->adjusted_mode,
-				       &adjusted_mode, &vbios_mode);
-	if (WARN_ON_ONCE(!succ))
+	if (!crtc->state->mode_changed)
 		return;
 
 	ast_set_vbios_mode_reg(crtc, &adjusted_mode, &vbios_mode);
@@ -840,7 +824,12 @@ ast_crtc_helper_atomic_enable(struct drm_crtc *crtc,
 	ast_set_crtthd_reg(crtc);
 	ast_set_sync_reg(dev, &adjusted_mode, &vbios_mode);
 	ast_set_dac_reg(crtc, &adjusted_mode, &vbios_mode);
+}
 
+static void
+ast_crtc_helper_atomic_enable(struct drm_crtc *crtc,
+			      struct drm_crtc_state *old_crtc_state)
+{
 	ast_crtc_dpms(crtc, DRM_MODE_DPMS_ON);
 }
 
-- 
2.23.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

  reply	other threads:[~2019-12-02 11:16 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-12-02 11:15 [PATCH v3 0/7] drm/ast: Fix modesetting's framebuffer usage Thomas Zimmermann
2019-12-02 11:15 ` Thomas Zimmermann [this message]
2019-12-02 11:15 ` [PATCH v3 2/7] drm/ast: Enable and disable screen in primary-plane functions Thomas Zimmermann
2019-12-02 11:15 ` [PATCH v3 3/7] drm/ast: Clean up arguments of register functions Thomas Zimmermann
2019-12-02 11:15 ` [PATCH v3 4/7] drm/ast: Add plane atomic_check() functions Thomas Zimmermann
2019-12-02 11:15 ` [PATCH v3 5/7] drm/ast: Introduce struct ast_crtc_state Thomas Zimmermann
2019-12-02 11:15 ` [PATCH v3 6/7] drm/ast: Store VBIOS mode info in " Thomas Zimmermann
2019-12-02 11:15 ` [PATCH v3 7/7] drm/ast: Store primary-plane format " Thomas Zimmermann
2019-12-05  8:10 ` [PATCH v3 0/7] drm/ast: Fix modesetting's framebuffer usage Gerd Hoffmann

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=20191202111557.15176-2-tzimmermann@suse.de \
    --to=tzimmermann@suse.de \
    --cc=airlied@redhat.com \
    --cc=daniel.vetter@ffwll.ch \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=kraxel@redhat.com \
    --cc=sam@ravnborg.org \
    --cc=yc_chen@aspeedtech.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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.