* [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 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.