* [PATCH] drm/locking: Allow NULL crtc in drm_modeset_legacy_acquire_ctx @ 2014-11-21 15:40 Daniel Vetter 2014-11-21 16:21 ` Ville Syrjälä 2014-11-21 16:55 ` Daniel Vetter 0 siblings, 2 replies; 5+ messages in thread From: Daniel Vetter @ 2014-11-21 15:40 UTC (permalink / raw) To: Intel Graphics Development Cc: Daniel Vetter, Jasper St. Pierre, DRI Development, Daniel Vetter I've missed checking this and so didn't notice that there's a NULL check missing. Since depending upon calling context the crtc might not even be there (disable-me-harder does happen around planes, especially in cleanup code) we need to dodge the oops and look at the global acquire ctx. Reported-by: "Jasper St. Pierre" <jstpierre@mecheye.net> Cc: "Jasper St. Pierre" <jstpierre@mecheye.net> Cc: Rob Clark <robdclark@gmail.com> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> --- drivers/gpu/drm/drm_modeset_lock.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/drm_modeset_lock.c b/drivers/gpu/drm/drm_modeset_lock.c index 474e4d12a2d8..93d28269e3bd 100644 --- a/drivers/gpu/drm/drm_modeset_lock.c +++ b/drivers/gpu/drm/drm_modeset_lock.c @@ -209,7 +209,7 @@ EXPORT_SYMBOL(drm_modeset_lock_crtc); struct drm_modeset_acquire_ctx * drm_modeset_legacy_acquire_ctx(struct drm_crtc *crtc) { - if (crtc->acquire_ctx) + if (crtc && crtc->acquire_ctx) return crtc->acquire_ctx; WARN_ON(!crtc->dev->mode_config.acquire_ctx); -- 2.1.1 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] drm/locking: Allow NULL crtc in drm_modeset_legacy_acquire_ctx 2014-11-21 15:40 [PATCH] drm/locking: Allow NULL crtc in drm_modeset_legacy_acquire_ctx Daniel Vetter @ 2014-11-21 16:21 ` Ville Syrjälä 2014-11-21 16:55 ` Daniel Vetter 1 sibling, 0 replies; 5+ messages in thread From: Ville Syrjälä @ 2014-11-21 16:21 UTC (permalink / raw) To: Daniel Vetter; +Cc: Daniel Vetter, Intel Graphics Development, DRI Development On Fri, Nov 21, 2014 at 04:40:18PM +0100, Daniel Vetter wrote: > I've missed checking this and so didn't notice that there's a NULL > check missing. Since depending upon calling context the crtc might not > even be there (disable-me-harder does happen around planes, especially > in cleanup code) we need to dodge the oops and look at the global > acquire ctx. > > Reported-by: "Jasper St. Pierre" <jstpierre@mecheye.net> > Cc: "Jasper St. Pierre" <jstpierre@mecheye.net> > Cc: Rob Clark <robdclark@gmail.com> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > --- > drivers/gpu/drm/drm_modeset_lock.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/drm_modeset_lock.c b/drivers/gpu/drm/drm_modeset_lock.c > index 474e4d12a2d8..93d28269e3bd 100644 > --- a/drivers/gpu/drm/drm_modeset_lock.c > +++ b/drivers/gpu/drm/drm_modeset_lock.c > @@ -209,7 +209,7 @@ EXPORT_SYMBOL(drm_modeset_lock_crtc); > struct drm_modeset_acquire_ctx * > drm_modeset_legacy_acquire_ctx(struct drm_crtc *crtc) > { > - if (crtc->acquire_ctx) > + if (crtc && crtc->acquire_ctx) > return crtc->acquire_ctx; > > WARN_ON(!crtc->dev->mode_config.acquire_ctx); ^^^^^^ How's that going to work without the crtc? > -- > 2.1.1 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel -- Ville Syrjälä Intel OTC _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH] drm/locking: Allow NULL crtc in drm_modeset_legacy_acquire_ctx 2014-11-21 15:40 [PATCH] drm/locking: Allow NULL crtc in drm_modeset_legacy_acquire_ctx Daniel Vetter 2014-11-21 16:21 ` Ville Syrjälä @ 2014-11-21 16:55 ` Daniel Vetter 2014-11-23 5:50 ` [PATCH] drm/locking: Allow NULL crtc in shuang.he 2014-11-24 21:38 ` [PATCH] drm/locking: Allow NULL crtc in drm_modeset_legacy_acquire_ctx Jasper St. Pierre 1 sibling, 2 replies; 5+ messages in thread From: Daniel Vetter @ 2014-11-21 16:55 UTC (permalink / raw) To: DRI Development Cc: Daniel Vetter, Intel Graphics Development, Jasper St. Pierre, Daniel Vetter I've missed checking this and so didn't notice that there's a NULL check missing. Since depending upon calling context the crtc might not even be there (disable-me-harder does happen around planes, especially in cleanup code) we need to dodge the oops and look at the global acquire ctx. v2: Actually fix the oops for real and don't just move it two lines down. That requires that we pass a drm_device pointer for the cases where crtc could be NULL. Reported-by: "Jasper St. Pierre" <jstpierre@mecheye.net> Cc: "Jasper St. Pierre" <jstpierre@mecheye.net> Cc: Rob Clark <robdclark@gmail.com> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> --- drivers/gpu/drm/drm_atomic_helper.c | 12 ++++++++---- drivers/gpu/drm/drm_modeset_lock.c | 12 ++++++++---- include/drm/drm_modeset_lock.h | 3 ++- 3 files changed, 18 insertions(+), 9 deletions(-) diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index ca839bd9bb0d..32c34b5d5f68 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -1171,7 +1171,8 @@ int drm_atomic_helper_update_plane(struct drm_plane *plane, if (!state) return -ENOMEM; - state->acquire_ctx = drm_modeset_legacy_acquire_ctx(crtc); + state->acquire_ctx = drm_modeset_legacy_acquire_ctx(crtc, + plane->dev); retry: plane_state = drm_atomic_get_plane_state(state, plane); if (IS_ERR(plane_state)) { @@ -1239,7 +1240,8 @@ int drm_atomic_helper_disable_plane(struct drm_plane *plane) if (!state) return -ENOMEM; - state->acquire_ctx = drm_modeset_legacy_acquire_ctx(plane->crtc); + state->acquire_ctx = drm_modeset_legacy_acquire_ctx(plane->crtc, + plane->dev); retry: plane_state = drm_atomic_get_plane_state(state, plane); if (IS_ERR(plane_state)) { @@ -1391,7 +1393,8 @@ int drm_atomic_helper_set_config(struct drm_mode_set *set) if (!state) return -ENOMEM; - state->acquire_ctx = drm_modeset_legacy_acquire_ctx(crtc); + state->acquire_ctx = drm_modeset_legacy_acquire_ctx(crtc, + crtc->dev); retry: crtc_state = drm_atomic_get_crtc_state(state, crtc); if (IS_ERR(crtc_state)) { @@ -1676,7 +1679,8 @@ int drm_atomic_helper_page_flip(struct drm_crtc *crtc, if (!state) return -ENOMEM; - state->acquire_ctx = drm_modeset_legacy_acquire_ctx(crtc); + state->acquire_ctx = drm_modeset_legacy_acquire_ctx(crtc, + crtc->dev); retry: crtc_state = drm_atomic_get_crtc_state(state, crtc); if (IS_ERR(crtc_state)) { diff --git a/drivers/gpu/drm/drm_modeset_lock.c b/drivers/gpu/drm/drm_modeset_lock.c index 474e4d12a2d8..655958d4f23e 100644 --- a/drivers/gpu/drm/drm_modeset_lock.c +++ b/drivers/gpu/drm/drm_modeset_lock.c @@ -200,21 +200,25 @@ EXPORT_SYMBOL(drm_modeset_lock_crtc); /** * drm_modeset_legacy_acquire_ctx - find acquire ctx for legacy ioctls * @crtc: drm crtc + * @dev: device * * Legacy ioctl operations like cursor updates or page flips only have per-crtc * locking, and store the acquire ctx in the corresponding crtc. All other * legacy operations take all locks and use a global acquire context. This * function grabs the right one. + * + * Note that either @crtc or @dev can be NULL, but not both. */ struct drm_modeset_acquire_ctx * -drm_modeset_legacy_acquire_ctx(struct drm_crtc *crtc) +drm_modeset_legacy_acquire_ctx(struct drm_crtc *crtc, + struct drm_device *dev) { - if (crtc->acquire_ctx) + if (crtc && crtc->acquire_ctx) return crtc->acquire_ctx; - WARN_ON(!crtc->dev->mode_config.acquire_ctx); + WARN_ON(!dev->mode_config.acquire_ctx); - return crtc->dev->mode_config.acquire_ctx; + return dev->mode_config.acquire_ctx; } EXPORT_SYMBOL(drm_modeset_legacy_acquire_ctx); diff --git a/include/drm/drm_modeset_lock.h b/include/drm/drm_modeset_lock.h index 28931a23d96c..cdbfd822e52f 100644 --- a/include/drm/drm_modeset_lock.h +++ b/include/drm/drm_modeset_lock.h @@ -135,7 +135,8 @@ void drm_modeset_lock_crtc(struct drm_crtc *crtc); void drm_modeset_unlock_crtc(struct drm_crtc *crtc); void drm_warn_on_modeset_not_all_locked(struct drm_device *dev); struct drm_modeset_acquire_ctx * -drm_modeset_legacy_acquire_ctx(struct drm_crtc *crtc); +drm_modeset_legacy_acquire_ctx(struct drm_crtc *crtc, + struct drm_device *dev); int drm_modeset_lock_all_crtcs(struct drm_device *dev, struct drm_modeset_acquire_ctx *ctx); -- 2.1.1 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] drm/locking: Allow NULL crtc in 2014-11-21 16:55 ` Daniel Vetter @ 2014-11-23 5:50 ` shuang.he 2014-11-24 21:38 ` [PATCH] drm/locking: Allow NULL crtc in drm_modeset_legacy_acquire_ctx Jasper St. Pierre 1 sibling, 0 replies; 5+ messages in thread From: shuang.he @ 2014-11-23 5:50 UTC (permalink / raw) To: shuang.he, intel-gfx, daniel.vetter Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com) -------------------------------------Summary------------------------------------- Platform Delta drm-intel-nightly Series Applied PNV 367/367 367/367 ILK 373/375 373/375 SNB 450/450 450/450 IVB -2 502/503 500/503 BYT 289/289 289/289 HSW -3 567/567 564/567 BDW 417/417 417/417 -------------------------------------Detailed------------------------------------- Platform Test drm-intel-nightly Series Applied IVB igt_gem_bad_reloc_negative-reloc-lut NSPT(3, M21M34M4)PASS(1, M21) NSPT(1, M21) IVB igt_kms_fence_pin_leak PASS(2, M21) DMESG_WARN(1, M21) HSW igt_gem_bad_reloc_negative-reloc-lut NSPT(10, M40M20)PASS(1, M20) NSPT(1, M40) HSW igt_kms_rotation_crc_primary-rotation PASS(11, M20M40) DMESG_WARN(1, M40) HSW igt_pm_rc6_residency_rc6-accuracy PASS(11, M20M40) FAIL(1, M40) _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] drm/locking: Allow NULL crtc in drm_modeset_legacy_acquire_ctx 2014-11-21 16:55 ` Daniel Vetter 2014-11-23 5:50 ` [PATCH] drm/locking: Allow NULL crtc in shuang.he @ 2014-11-24 21:38 ` Jasper St. Pierre 1 sibling, 0 replies; 5+ messages in thread From: Jasper St. Pierre @ 2014-11-24 21:38 UTC (permalink / raw) To: Daniel Vetter; +Cc: Daniel Vetter, Intel Graphics Development, DRI Development [-- Attachment #1.1: Type: text/plain, Size: 6892 bytes --] This still crashes for me: kernel: ------------[ cut here ]------------ kernel: WARNING: at drivers/gpu/drm/drm_modeset_lock.c:219 drm_modeset_legacy_acquire_ctx+0x38/0x40() kernel: Modules linked in: kernel: CPU: 0 PID: 586 Comm: Xorg Not tainted 3.10.33-02454-g1c4eeb3-dirty #196 kernel: [<c0014c9c>] (unwind_backtrace+0x0/0xec) from [<c0011d34>] (show_stack+0x10/0x14) kernel: [<c0011d34>] (show_stack+0x10/0x14) from [<c002d2d0>] (warn_slowpath_common+0x54/0x6c) kernel: [<c002d2d0>] (warn_slowpath_common+0x54/0x6c) from [<c002d384>] (warn_slowpath_null+0x1c/0x24) kernel: [<c002d384>] (warn_slowpath_null+0x1c/0x24) from [<c02ce818>] (drm_modeset_legacy_acquire_ctx+0x38/0x40) kernel: [<c02ce818>] (drm_modeset_legacy_acquire_ctx+0x38/0x40) from [<c02afa9c>] (drm_atomic_helper_disable_plane+0x24/0xd0) kernel: [<c02afa9c>] (drm_atomic_helper_disable_plane+0x24/0xd0) from [<c02c19a4>] (__setplane_internal+0x28/0x2cc) kernel: [<c02c19a4>] (__setplane_internal+0x28/0x2cc) from [<c02c287c>] (drm_mode_cursor_common+0x1cc/0x2f8) kernel: [<c02c287c>] (drm_mode_cursor_common+0x1cc/0x2f8) from [<c02c53d4>] (drm_mode_cursor_ioctl+0x58/0x60) kernel: [<c02c53d4>] (drm_mode_cursor_ioctl+0x58/0x60) from [<c02b917c>] (drm_ioctl+0x318/0x510) kernel: [<c02b917c>] (drm_ioctl+0x318/0x510) from [<c00e2d9c>] (do_vfs_ioctl+0x55c/0x5b0) kernel: [<c00e2d9c>] (do_vfs_ioctl+0x55c/0x5b0) from [<c00e2e40>] (SyS_ioctl+0x50/0x7c) kernel: [<c00e2e40>] (SyS_ioctl+0x50/0x7c) from [<c000e000>] (ret_fast_syscall+0x0/0x30) This seems to try to acquire the global device lock as instantiated by drm_modeset_lock_all, but nothing in this path calls that. I'm not sure what lock we can acquire when we don't have a CRTC. On Fri, Nov 21, 2014 at 8:55 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > I've missed checking this and so didn't notice that there's a NULL > check missing. Since depending upon calling context the crtc might not > even be there (disable-me-harder does happen around planes, especially > in cleanup code) we need to dodge the oops and look at the global > acquire ctx. > > v2: Actually fix the oops for real and don't just move it two lines > down. That requires that we pass a drm_device pointer for the cases > where crtc could be NULL. > > Reported-by: "Jasper St. Pierre" <jstpierre@mecheye.net> > Cc: "Jasper St. Pierre" <jstpierre@mecheye.net> > Cc: Rob Clark <robdclark@gmail.com> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > --- > drivers/gpu/drm/drm_atomic_helper.c | 12 ++++++++---- > drivers/gpu/drm/drm_modeset_lock.c | 12 ++++++++---- > include/drm/drm_modeset_lock.h | 3 ++- > 3 files changed, 18 insertions(+), 9 deletions(-) > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c > b/drivers/gpu/drm/drm_atomic_helper.c > index ca839bd9bb0d..32c34b5d5f68 100644 > --- a/drivers/gpu/drm/drm_atomic_helper.c > +++ b/drivers/gpu/drm/drm_atomic_helper.c > @@ -1171,7 +1171,8 @@ int drm_atomic_helper_update_plane(struct drm_plane > *plane, > if (!state) > return -ENOMEM; > > - state->acquire_ctx = drm_modeset_legacy_acquire_ctx(crtc); > + state->acquire_ctx = drm_modeset_legacy_acquire_ctx(crtc, > + plane->dev); > retry: > plane_state = drm_atomic_get_plane_state(state, plane); > if (IS_ERR(plane_state)) { > @@ -1239,7 +1240,8 @@ int drm_atomic_helper_disable_plane(struct drm_plane > *plane) > if (!state) > return -ENOMEM; > > - state->acquire_ctx = drm_modeset_legacy_acquire_ctx(plane->crtc); > + state->acquire_ctx = drm_modeset_legacy_acquire_ctx(plane->crtc, > + plane->dev); > retry: > plane_state = drm_atomic_get_plane_state(state, plane); > if (IS_ERR(plane_state)) { > @@ -1391,7 +1393,8 @@ int drm_atomic_helper_set_config(struct drm_mode_set > *set) > if (!state) > return -ENOMEM; > > - state->acquire_ctx = drm_modeset_legacy_acquire_ctx(crtc); > + state->acquire_ctx = drm_modeset_legacy_acquire_ctx(crtc, > + crtc->dev); > retry: > crtc_state = drm_atomic_get_crtc_state(state, crtc); > if (IS_ERR(crtc_state)) { > @@ -1676,7 +1679,8 @@ int drm_atomic_helper_page_flip(struct drm_crtc > *crtc, > if (!state) > return -ENOMEM; > > - state->acquire_ctx = drm_modeset_legacy_acquire_ctx(crtc); > + state->acquire_ctx = drm_modeset_legacy_acquire_ctx(crtc, > + crtc->dev); > retry: > crtc_state = drm_atomic_get_crtc_state(state, crtc); > if (IS_ERR(crtc_state)) { > diff --git a/drivers/gpu/drm/drm_modeset_lock.c > b/drivers/gpu/drm/drm_modeset_lock.c > index 474e4d12a2d8..655958d4f23e 100644 > --- a/drivers/gpu/drm/drm_modeset_lock.c > +++ b/drivers/gpu/drm/drm_modeset_lock.c > @@ -200,21 +200,25 @@ EXPORT_SYMBOL(drm_modeset_lock_crtc); > /** > * drm_modeset_legacy_acquire_ctx - find acquire ctx for legacy ioctls > * @crtc: drm crtc > + * @dev: device > * > * Legacy ioctl operations like cursor updates or page flips only have > per-crtc > * locking, and store the acquire ctx in the corresponding crtc. All other > * legacy operations take all locks and use a global acquire context. This > * function grabs the right one. > + * > + * Note that either @crtc or @dev can be NULL, but not both. > */ > struct drm_modeset_acquire_ctx * > -drm_modeset_legacy_acquire_ctx(struct drm_crtc *crtc) > +drm_modeset_legacy_acquire_ctx(struct drm_crtc *crtc, > + struct drm_device *dev) > { > - if (crtc->acquire_ctx) > + if (crtc && crtc->acquire_ctx) > return crtc->acquire_ctx; > > - WARN_ON(!crtc->dev->mode_config.acquire_ctx); > + WARN_ON(!dev->mode_config.acquire_ctx); > > - return crtc->dev->mode_config.acquire_ctx; > + return dev->mode_config.acquire_ctx; > } > EXPORT_SYMBOL(drm_modeset_legacy_acquire_ctx); > > diff --git a/include/drm/drm_modeset_lock.h > b/include/drm/drm_modeset_lock.h > index 28931a23d96c..cdbfd822e52f 100644 > --- a/include/drm/drm_modeset_lock.h > +++ b/include/drm/drm_modeset_lock.h > @@ -135,7 +135,8 @@ void drm_modeset_lock_crtc(struct drm_crtc *crtc); > void drm_modeset_unlock_crtc(struct drm_crtc *crtc); > void drm_warn_on_modeset_not_all_locked(struct drm_device *dev); > struct drm_modeset_acquire_ctx * > -drm_modeset_legacy_acquire_ctx(struct drm_crtc *crtc); > +drm_modeset_legacy_acquire_ctx(struct drm_crtc *crtc, > + struct drm_device *dev); > > int drm_modeset_lock_all_crtcs(struct drm_device *dev, > struct drm_modeset_acquire_ctx *ctx); > -- > 2.1.1 > > -- Jasper [-- Attachment #1.2: Type: text/html, Size: 8578 bytes --] [-- Attachment #2: Type: text/plain, Size: 159 bytes --] _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2014-11-24 21:38 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-11-21 15:40 [PATCH] drm/locking: Allow NULL crtc in drm_modeset_legacy_acquire_ctx Daniel Vetter 2014-11-21 16:21 ` Ville Syrjälä 2014-11-21 16:55 ` Daniel Vetter 2014-11-23 5:50 ` [PATCH] drm/locking: Allow NULL crtc in shuang.he 2014-11-24 21:38 ` [PATCH] drm/locking: Allow NULL crtc in drm_modeset_legacy_acquire_ctx Jasper St. Pierre
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox