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