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);
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
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);
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
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Jasper St. Pierre -
Ville Syrjälä