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