On Wed, Nov 27, 2019 at 01:31:25PM +0100, Thomas Zimmermann wrote:
> Hi Daniel
>
> Am 27.11.19 um 11:49 schrieb Daniel Vetter:
> > On Wed, Nov 27, 2019 at 08:31:09AM +0100, Thomas Zimmermann wrote:
> >> 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
> >>
> >> The fix implements atomic_check for planes and rejects configurations
> >> with an invisible primary plane.
> >>
> >> v2:
> >> * do an atomic check for plane
> >> * reject invisible primary planes
> >>
> >> Signed-off-by: Thomas Zimmermann <tzimmermann@xxxxxxx>
> >> Suggested-by: Daniel Vetter <daniel.vetter@xxxxxxxx>
> >> Fixes: b48e1b6ffd28 ("drm/ast: Add CRTC helpers for atomic modesetting")
> >> Cc: Gerd Hoffmann <kraxel@xxxxxxxxxx>
> >> Cc: Dave Airlie <airlied@xxxxxxxxxx>
> >> Cc: Daniel Vetter <daniel.vetter@xxxxxxxx>
> >> Cc: "Y.C. Chen" <yc_chen@xxxxxxxxxxxxxx>
> >> Cc: Sam Ravnborg <sam@xxxxxxxxxxxx>
> >> ---
> >> drivers/gpu/drm/ast/ast_mode.c | 50 +++++++++++++++++++++++++++++++++-
> >> 1 file changed, 49 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
> >> index 4725ec911a66..8e7bb8ce8130 100644
> >> --- a/drivers/gpu/drm/ast/ast_mode.c
> >> +++ b/drivers/gpu/drm/ast/ast_mode.c
> >> @@ -31,6 +31,7 @@
> >> #include <linux/export.h>
> >> #include <linux/pci.h>
> >>
> >> +#include <drm/drm_atomic.h>
> >> #include <drm/drm_atomic_helper.h>
> >> #include <drm/drm_atomic_state_helper.h>
> >> #include <drm/drm_crtc.h>
> >> @@ -556,6 +557,31 @@ static const uint32_t ast_primary_plane_formats[] = {
> >> int ast_primary_plane_helper_atomic_check(struct drm_plane *plane,
> >> struct drm_plane_state *state)
> >> {
> >> + struct drm_crtc *crtc;
> >> + struct drm_crtc_state *crtc_state;
> >> + int ret;
> >> +
> >> + if (state->crtc)
> >> + crtc = state->crtc;
> >> + else if (plane->state && plane->state->crtc)
> >> + crtc = plane->state->crtc;
> >> + else
> >> + return 0; /* disabling an already disabled plane */
> >> +
> >> + crtc_state = drm_atomic_get_new_crtc_state(state->state, crtc);
> >> + if (WARN_ON_ONCE(!crtc_state))
> >> + return -EINVAL; /* BUG: no new CRTC state allocated */
> >
> > The above is a lot more complicated than necessary, see e.g.
> > atmel_hlcdc_plane_atomic_check. And even that is too complicated, since
> > crtc is set iff fb is set, you only need to check one of them.
>
> Thanks for your review. This comment sounds as if it's at the wrong
> position?
Nope. Instead of the stuff above you have do this:
if (!state->crtc)
return 0;
crtc_state = drm_atomic_get_existing_crtc_state(s->state, s->crtc);
mode = &crtc_state->adjusted_mode;
which is what atmel has, except the redundant fb check dropped. Same for
the cursor version below too.
-Daniel
>
> Best regards
> Thomas
>
> >
> >> +
> >> + ret = drm_atomic_helper_check_plane_state(state, crtc_state,
> >> + DRM_PLANE_HELPER_NO_SCALING,
> >> + DRM_PLANE_HELPER_NO_SCALING,
> >> + false, true);
> >> + if (ret)
> >> + return ret;
> >> +
> >> + if (!state->visible)
> >> + return -EINVAL; /* primary plane must be visible */
> >> +
> >> return 0;
> >> }
> >>
> >> @@ -567,7 +593,7 @@ void ast_primary_plane_helper_atomic_update(struct drm_plane *plane,
> >> struct drm_gem_vram_object *gbo;
> >> s64 gpu_addr;
> >>
> >> - if (!crtc || !state->fb)
> >> + if (!crtc || !state->fb || !state->visible)
> >> return;
> >>
> >> gbo = drm_gem_vram_of_gem(state->fb->obj[0]);
> >> @@ -660,6 +686,28 @@ ast_cursor_plane_helper_prepare_fb(struct drm_plane *plane,
> >> static int ast_cursor_plane_helper_atomic_check(struct drm_plane *plane,
> >> struct drm_plane_state *state)
> >> {
> >> + struct drm_crtc *crtc;
> >> + struct drm_crtc_state *crtc_state;
> >> + int ret;
> >> +
> >> + if (state->crtc)
> >> + crtc = state->crtc;
> >> + else if (plane->state && plane->state->crtc)
> >> + crtc = plane->state->crtc;
> >> + else
> >> + return 0; /* disabling an already disabled plane */
> >> +
> >> + crtc_state = drm_atomic_get_new_crtc_state(state->state, crtc);
> >> + if (WARN_ON_ONCE(!crtc_state))
> >> + return -EINVAL; /* BUG: no new CRTC state allocated */
> >> +
> >> + ret = drm_atomic_helper_check_plane_state(state, crtc_state,
> >> + DRM_PLANE_HELPER_NO_SCALING,
> >> + DRM_PLANE_HELPER_NO_SCALING,
> >> + false, true);
> >
> > Pretty sure you want your cursor to be positionable ...
> > -Daniel
> >
> >> + if (ret)
> >> + return ret;
> >> +
> >> return 0;
> >> }
> >>
> >> --
> >> 2.23.0
> >>
> >
>
> --
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Software Solutions Germany GmbH
> Maxfeldstr. 5, 90409 Nürnberg, Germany
> (HRB 36809, AG Nürnberg)
> Geschäftsführer: Felix Imendörffer
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
