Handle the vblank events in the simple_kms_helper driver, otherwise the drm_atomic_helper flip_done event never happens.
Signed-off-by: Marek Vasut marex@denx.de Cc: Noralf Trønnes noralf@tronnes.org Cc: Daniel Vetter daniel@ffwll.ch Cc: David Airlie airlied@linux.ie --- drivers/gpu/drm/drm_simple_kms_helper.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)
diff --git a/drivers/gpu/drm/drm_simple_kms_helper.c b/drivers/gpu/drm/drm_simple_kms_helper.c index 7b6d26e..3345b40 100644 --- a/drivers/gpu/drm/drm_simple_kms_helper.c +++ b/drivers/gpu/drm/drm_simple_kms_helper.c @@ -34,6 +34,23 @@ static const struct drm_encoder_funcs drm_simple_kms_encoder_funcs = { .destroy = drm_encoder_cleanup, };
+static void drm_simple_kms_crtc_begin(struct drm_crtc *crtc, + struct drm_crtc_state *state) +{ + struct drm_pending_vblank_event *event = crtc->state->event; + + if (event) { + crtc->state->event = NULL; + + spin_lock_irq(&crtc->dev->event_lock); + if (drm_crtc_vblank_get(crtc) == 0) + drm_crtc_arm_vblank_event(crtc, event); + else + drm_crtc_send_vblank_event(crtc, event); + spin_unlock_irq(&crtc->dev->event_lock); + } +} + static int drm_simple_kms_crtc_check(struct drm_crtc *crtc, struct drm_crtc_state *state) { @@ -63,6 +80,7 @@ static void drm_simple_kms_crtc_disable(struct drm_crtc *crtc) }
static const struct drm_crtc_helper_funcs drm_simple_kms_crtc_helper_funcs = { + .atomic_begin = drm_simple_kms_crtc_begin, .atomic_check = drm_simple_kms_crtc_check, .disable = drm_simple_kms_crtc_disable, .enable = drm_simple_kms_crtc_enable,
On Sun, Sep 25, 2016 at 09:41:58PM +0200, Marek Vasut wrote:
Handle the vblank events in the simple_kms_helper driver, otherwise the drm_atomic_helper flip_done event never happens.
Signed-off-by: Marek Vasut marex@denx.de Cc: Noralf Trønnes noralf@tronnes.org Cc: Daniel Vetter daniel@ffwll.ch Cc: David Airlie airlied@linux.ie
drivers/gpu/drm/drm_simple_kms_helper.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)
diff --git a/drivers/gpu/drm/drm_simple_kms_helper.c b/drivers/gpu/drm/drm_simple_kms_helper.c index 7b6d26e..3345b40 100644 --- a/drivers/gpu/drm/drm_simple_kms_helper.c +++ b/drivers/gpu/drm/drm_simple_kms_helper.c @@ -34,6 +34,23 @@ static const struct drm_encoder_funcs drm_simple_kms_encoder_funcs = { .destroy = drm_encoder_cleanup, };
+static void drm_simple_kms_crtc_begin(struct drm_crtc *crtc,
struct drm_crtc_state *state)+{
- struct drm_pending_vblank_event *event = crtc->state->event;
- if (event) {
crtc->state->event = NULL;spin_lock_irq(&crtc->dev->event_lock);if (drm_crtc_vblank_get(crtc) == 0)drm_crtc_arm_vblank_event(crtc, event);elsedrm_crtc_send_vblank_event(crtc, event);spin_unlock_irq(&crtc->dev->event_lock);- }
+}
This should be done by drivers, in the ->update hook. At least if we want to pretend that it's semi-accurate and not racy (which the above is). -Daniel
static int drm_simple_kms_crtc_check(struct drm_crtc *crtc, struct drm_crtc_state *state) { @@ -63,6 +80,7 @@ static void drm_simple_kms_crtc_disable(struct drm_crtc *crtc) }
static const struct drm_crtc_helper_funcs drm_simple_kms_crtc_helper_funcs = {
- .atomic_begin = drm_simple_kms_crtc_begin, .atomic_check = drm_simple_kms_crtc_check, .disable = drm_simple_kms_crtc_disable, .enable = drm_simple_kms_crtc_enable,
-- 2.9.3
On 09/25/2016 11:00 PM, Daniel Vetter wrote:
On Sun, Sep 25, 2016 at 09:41:58PM +0200, Marek Vasut wrote:
Handle the vblank events in the simple_kms_helper driver, otherwise the drm_atomic_helper flip_done event never happens.
Signed-off-by: Marek Vasut marex@denx.de Cc: Noralf Trønnes noralf@tronnes.org Cc: Daniel Vetter daniel@ffwll.ch Cc: David Airlie airlied@linux.ie
drivers/gpu/drm/drm_simple_kms_helper.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)
diff --git a/drivers/gpu/drm/drm_simple_kms_helper.c b/drivers/gpu/drm/drm_simple_kms_helper.c index 7b6d26e..3345b40 100644 --- a/drivers/gpu/drm/drm_simple_kms_helper.c +++ b/drivers/gpu/drm/drm_simple_kms_helper.c @@ -34,6 +34,23 @@ static const struct drm_encoder_funcs drm_simple_kms_encoder_funcs = { .destroy = drm_encoder_cleanup, };
+static void drm_simple_kms_crtc_begin(struct drm_crtc *crtc,
struct drm_crtc_state *state)+{
- struct drm_pending_vblank_event *event = crtc->state->event;
- if (event) {
crtc->state->event = NULL;spin_lock_irq(&crtc->dev->event_lock);if (drm_crtc_vblank_get(crtc) == 0)drm_crtc_arm_vblank_event(crtc, event);elsedrm_crtc_send_vblank_event(crtc, event);spin_unlock_irq(&crtc->dev->event_lock);- }
+}
This should be done by drivers, in the ->update hook. At least if we want to pretend that it's semi-accurate and not racy (which the above is). -Daniel
Got it and wrapped into mxsfb, thanks.
But then, I see a few drivers (arm hdlcd, fsl-dcu,...) doing the same thing at random callbacks of CRTC . Shouldn't this event handling be consolidated into some generic function and those drivers fixed to call it from atomic update ?
On 09/26/2016 11:41 AM, Marek Vasut wrote:
On 09/25/2016 11:00 PM, Daniel Vetter wrote:
On Sun, Sep 25, 2016 at 09:41:58PM +0200, Marek Vasut wrote:
Handle the vblank events in the simple_kms_helper driver, otherwise the drm_atomic_helper flip_done event never happens.
Signed-off-by: Marek Vasut marex@denx.de Cc: Noralf Trønnes noralf@tronnes.org Cc: Daniel Vetter daniel@ffwll.ch Cc: David Airlie airlied@linux.ie
drivers/gpu/drm/drm_simple_kms_helper.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)
diff --git a/drivers/gpu/drm/drm_simple_kms_helper.c b/drivers/gpu/drm/drm_simple_kms_helper.c index 7b6d26e..3345b40 100644 --- a/drivers/gpu/drm/drm_simple_kms_helper.c +++ b/drivers/gpu/drm/drm_simple_kms_helper.c @@ -34,6 +34,23 @@ static const struct drm_encoder_funcs drm_simple_kms_encoder_funcs = { .destroy = drm_encoder_cleanup, };
+static void drm_simple_kms_crtc_begin(struct drm_crtc *crtc,
struct drm_crtc_state *state)+{
- struct drm_pending_vblank_event *event = crtc->state->event;
- if (event) {
crtc->state->event = NULL;spin_lock_irq(&crtc->dev->event_lock);if (drm_crtc_vblank_get(crtc) == 0)drm_crtc_arm_vblank_event(crtc, event);elsedrm_crtc_send_vblank_event(crtc, event);spin_unlock_irq(&crtc->dev->event_lock);- }
+}
This should be done by drivers, in the ->update hook. At least if we want to pretend that it's semi-accurate and not racy (which the above is). -Daniel
Got it and wrapped into mxsfb, thanks.
Hrm, while this works most of the time, I managed to get a page_flip timeout when terminating the kmscube OpenGL demo (see below). I'm not getting this splat with this patch applied. Any idea what this could mean ?
------------[ cut here ]------------ WARNING: CPU: 0 PID: 77 at drivers/gpu/drm/drm_atomic_helper.c:1579 drm_atomic_helper_commit_hw_done+0xa8/0xb8 Modules linked in: CPU: 0 PID: 77 Comm: kworker/0:2 Tainted: G W 4.8.0-rc7-next-20160923-00033-ge3f15d6 #56 Hardware name: Freescale i.MX6 SoloX (Device Tree) Workqueue: events drm_mode_rmfb_work_fn Backtrace: [<c010c180>] (dump_backtrace) from [<c010c378>] (show_stack+0x18/0x1c) r7:00000000 r6:600e0013 r5:00000000 r4:c0e2342c [<c010c360>] (show_stack) from [<c03f4ad8>] (dump_stack+0xb4/0xe8) [<c03f4a24>] (dump_stack) from [<c0124a90>] (__warn+0xd8/0x104) r9:c04bdb60 r8:0000062b r7:00000009 r6:c0c38298 r5:00000000 r4:00000000 [<c01249b8>] (__warn) from [<c0124b70>] (warn_slowpath_null+0x28/0x30) r9:db43639c r8:0000062b r7:c0c38298 r6:da962e40 r5:00000000 r4:00000000 [<c0124b48>] (warn_slowpath_null) from [<c04bdb60>] (drm_atomic_helper_commit_hw_done+0xa8/0xb8) [<c04bdab8>] (drm_atomic_helper_commit_hw_done) from [<c04c0568>] (drm_atomic_helper_commit_tail+0x44/0x60) r10:c0c3bd84 r9:c0c371cc r8:db770000 r7:db770000 r6:00000000 r5:db770000 r4:daffa780 r3:daffa3c0 [<c04c0524>] (drm_atomic_helper_commit_tail) from [<c04c05e8>] (commit_tail+0x64/0x68) r5:00000000 r4:daffa780 [<c04c0584>] (commit_tail) from [<c04c06b0>] (drm_atomic_helper_commit+0xac/0xec) r5:00000000 r4:daffa780 [<c04c0604>] (drm_atomic_helper_commit) from [<c04e11b4>] (drm_atomic_commit+0x58/0x78) r7:db436028 r6:db770000 r5:daffa780 r4:00000000 [<c04e115c>] (drm_atomic_commit) from [<c04c0be4>] (drm_atomic_helper_set_config+0x80/0xa4) r6:dad37e14 r5:00000000 r4:daffa780 [<c04c0b64>] (drm_atomic_helper_set_config) from [<c04d35fc>] (drm_mode_set_config_internal+0x6c/0xfc) r7:db77048c r6:db770480 r5:db436028 r4:00000000 [<c04d3590>] (drm_mode_set_config_internal) from [<c04d36c4>] (drm_crtc_force_disable+0x38/0x40) r7:db77048c r6:db770480 r5:da962c00 r4:db436028 [<c04d368c>] (drm_crtc_force_disable) from [<c04e2b60>] (drm_framebuffer_remove+0xd8/0x124) [<c04e2a88>] (drm_framebuffer_remove) from [<c04e2bec>] (drm_mode_rmfb_work_fn+0x40/0x50) r10:00000001 r9:00000000 r8:dbbb2a00 r7:dad37ea8 r6:dbbaf800 r5:db60be68 r4:db60be48 [<c04e2bac>] (drm_mode_rmfb_work_fn) from [<c013fa10>] (process_one_work+0x24c/0x4e8) r5:dad12f00 r4:db60be48 [<c013f7c4>] (process_one_work) from [<c01408dc>] (worker_thread+0x40/0x574) r10:dad12f00 r9:dad36000 r8:c0e02100 r7:dbbaf834 r6:00000008 r5:dad12f18 r4:dbbaf800 [<c014089c>] (worker_thread) from [<c0146908>] (kthread+0xd8/0xf8) r10:00000000 r9:00000000 r8:00000000 r7:c014089c r6:dad12f00 r5:dad28500 r4:00000000 [<c0146830>] (kthread) from [<c0107cb0>] (ret_from_fork+0x14/0x24) r7:00000000 r6:00000000 r5:c0146830 r4:dad28500 ---[ end trace 2b6e3065f0299ba5 ]---
But then, I see a few drivers (arm hdlcd, fsl-dcu,...) doing the same thing at random callbacks of CRTC . Shouldn't this event handling be consolidated into some generic function and those drivers fixed to call it from atomic update ?
On Mon, Sep 26, 2016 at 2:59 PM, Marek Vasut marex@denx.de wrote:
On 09/26/2016 11:41 AM, Marek Vasut wrote:
On 09/25/2016 11:00 PM, Daniel Vetter wrote:
On Sun, Sep 25, 2016 at 09:41:58PM +0200, Marek Vasut wrote:
Handle the vblank events in the simple_kms_helper driver, otherwise the drm_atomic_helper flip_done event never happens.
Signed-off-by: Marek Vasut marex@denx.de Cc: Noralf Trønnes noralf@tronnes.org Cc: Daniel Vetter daniel@ffwll.ch Cc: David Airlie airlied@linux.ie
drivers/gpu/drm/drm_simple_kms_helper.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)
diff --git a/drivers/gpu/drm/drm_simple_kms_helper.c b/drivers/gpu/drm/drm_simple_kms_helper.c index 7b6d26e..3345b40 100644 --- a/drivers/gpu/drm/drm_simple_kms_helper.c +++ b/drivers/gpu/drm/drm_simple_kms_helper.c @@ -34,6 +34,23 @@ static const struct drm_encoder_funcs drm_simple_kms_encoder_funcs = { .destroy = drm_encoder_cleanup, };
+static void drm_simple_kms_crtc_begin(struct drm_crtc *crtc,
struct drm_crtc_state *state)+{
- struct drm_pending_vblank_event *event = crtc->state->event;
- if (event) {
crtc->state->event = NULL;spin_lock_irq(&crtc->dev->event_lock);if (drm_crtc_vblank_get(crtc) == 0)drm_crtc_arm_vblank_event(crtc, event);elsedrm_crtc_send_vblank_event(crtc, event);spin_unlock_irq(&crtc->dev->event_lock);- }
+}
This should be done by drivers, in the ->update hook. At least if we want to pretend that it's semi-accurate and not racy (which the above is). -Daniel
Got it and wrapped into mxsfb, thanks.
Hrm, while this works most of the time, I managed to get a page_flip timeout when terminating the kmscube OpenGL demo (see below). I'm not getting this splat with this patch applied. Any idea what this could mean ?
Are you on latest drm-next? There was a bug fixed for 4.9 which resulted in the primary plane not always being added to the state, which means that the ->update hook would not get called when the plane is getting disabled.
At least that's my guess, the WARN_ON just tells you that no one sent out the event yet, but by that point it should have happened. -Daniel
------------[ cut here ]------------ WARNING: CPU: 0 PID: 77 at drivers/gpu/drm/drm_atomic_helper.c:1579 drm_atomic_helper_commit_hw_done+0xa8/0xb8 Modules linked in: CPU: 0 PID: 77 Comm: kworker/0:2 Tainted: G W 4.8.0-rc7-next-20160923-00033-ge3f15d6 #56 Hardware name: Freescale i.MX6 SoloX (Device Tree) Workqueue: events drm_mode_rmfb_work_fn Backtrace: [<c010c180>] (dump_backtrace) from [<c010c378>] (show_stack+0x18/0x1c) r7:00000000 r6:600e0013 r5:00000000 r4:c0e2342c [<c010c360>] (show_stack) from [<c03f4ad8>] (dump_stack+0xb4/0xe8) [<c03f4a24>] (dump_stack) from [<c0124a90>] (__warn+0xd8/0x104) r9:c04bdb60 r8:0000062b r7:00000009 r6:c0c38298 r5:00000000 r4:00000000 [<c01249b8>] (__warn) from [<c0124b70>] (warn_slowpath_null+0x28/0x30) r9:db43639c r8:0000062b r7:c0c38298 r6:da962e40 r5:00000000 r4:00000000 [<c0124b48>] (warn_slowpath_null) from [<c04bdb60>] (drm_atomic_helper_commit_hw_done+0xa8/0xb8) [<c04bdab8>] (drm_atomic_helper_commit_hw_done) from [<c04c0568>] (drm_atomic_helper_commit_tail+0x44/0x60) r10:c0c3bd84 r9:c0c371cc r8:db770000 r7:db770000 r6:00000000 r5:db770000 r4:daffa780 r3:daffa3c0 [<c04c0524>] (drm_atomic_helper_commit_tail) from [<c04c05e8>] (commit_tail+0x64/0x68) r5:00000000 r4:daffa780 [<c04c0584>] (commit_tail) from [<c04c06b0>] (drm_atomic_helper_commit+0xac/0xec) r5:00000000 r4:daffa780 [<c04c0604>] (drm_atomic_helper_commit) from [<c04e11b4>] (drm_atomic_commit+0x58/0x78) r7:db436028 r6:db770000 r5:daffa780 r4:00000000 [<c04e115c>] (drm_atomic_commit) from [<c04c0be4>] (drm_atomic_helper_set_config+0x80/0xa4) r6:dad37e14 r5:00000000 r4:daffa780 [<c04c0b64>] (drm_atomic_helper_set_config) from [<c04d35fc>] (drm_mode_set_config_internal+0x6c/0xfc) r7:db77048c r6:db770480 r5:db436028 r4:00000000 [<c04d3590>] (drm_mode_set_config_internal) from [<c04d36c4>] (drm_crtc_force_disable+0x38/0x40) r7:db77048c r6:db770480 r5:da962c00 r4:db436028 [<c04d368c>] (drm_crtc_force_disable) from [<c04e2b60>] (drm_framebuffer_remove+0xd8/0x124) [<c04e2a88>] (drm_framebuffer_remove) from [<c04e2bec>] (drm_mode_rmfb_work_fn+0x40/0x50) r10:00000001 r9:00000000 r8:dbbb2a00 r7:dad37ea8 r6:dbbaf800 r5:db60be68 r4:db60be48 [<c04e2bac>] (drm_mode_rmfb_work_fn) from [<c013fa10>] (process_one_work+0x24c/0x4e8) r5:dad12f00 r4:db60be48 [<c013f7c4>] (process_one_work) from [<c01408dc>] (worker_thread+0x40/0x574) r10:dad12f00 r9:dad36000 r8:c0e02100 r7:dbbaf834 r6:00000008 r5:dad12f18 r4:dbbaf800 [<c014089c>] (worker_thread) from [<c0146908>] (kthread+0xd8/0xf8) r10:00000000 r9:00000000 r8:00000000 r7:c014089c r6:dad12f00 r5:dad28500 r4:00000000 [<c0146830>] (kthread) from [<c0107cb0>] (ret_from_fork+0x14/0x24) r7:00000000 r6:00000000 r5:c0146830 r4:dad28500 ---[ end trace 2b6e3065f0299ba5 ]---
But then, I see a few drivers (arm hdlcd, fsl-dcu,...) doing the same thing at random callbacks of CRTC . Shouldn't this event handling be consolidated into some generic function and those drivers fixed to call it from atomic update ?
-- Best regards, Marek Vasut
On 09/27/2016 09:49 AM, Daniel Vetter wrote:
On Mon, Sep 26, 2016 at 2:59 PM, Marek Vasut marex@denx.de wrote:
On 09/26/2016 11:41 AM, Marek Vasut wrote:
On 09/25/2016 11:00 PM, Daniel Vetter wrote:
On Sun, Sep 25, 2016 at 09:41:58PM +0200, Marek Vasut wrote:
Handle the vblank events in the simple_kms_helper driver, otherwise the drm_atomic_helper flip_done event never happens.
Signed-off-by: Marek Vasut marex@denx.de Cc: Noralf Trønnes noralf@tronnes.org Cc: Daniel Vetter daniel@ffwll.ch Cc: David Airlie airlied@linux.ie
drivers/gpu/drm/drm_simple_kms_helper.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)
diff --git a/drivers/gpu/drm/drm_simple_kms_helper.c b/drivers/gpu/drm/drm_simple_kms_helper.c index 7b6d26e..3345b40 100644 --- a/drivers/gpu/drm/drm_simple_kms_helper.c +++ b/drivers/gpu/drm/drm_simple_kms_helper.c @@ -34,6 +34,23 @@ static const struct drm_encoder_funcs drm_simple_kms_encoder_funcs = { .destroy = drm_encoder_cleanup, };
+static void drm_simple_kms_crtc_begin(struct drm_crtc *crtc,
struct drm_crtc_state *state)+{
- struct drm_pending_vblank_event *event = crtc->state->event;
- if (event) {
crtc->state->event = NULL;spin_lock_irq(&crtc->dev->event_lock);if (drm_crtc_vblank_get(crtc) == 0)drm_crtc_arm_vblank_event(crtc, event);elsedrm_crtc_send_vblank_event(crtc, event);spin_unlock_irq(&crtc->dev->event_lock);- }
+}
This should be done by drivers, in the ->update hook. At least if we want to pretend that it's semi-accurate and not racy (which the above is). -Daniel
Got it and wrapped into mxsfb, thanks.
Hrm, while this works most of the time, I managed to get a page_flip timeout when terminating the kmscube OpenGL demo (see below). I'm not getting this splat with this patch applied. Any idea what this could mean ?
Are you on latest drm-next?
No, I'm on next/master from 20160927 . Is this the right tree? https://cgit.freedesktop.org/~airlied/linux/log/?h=drm-next
There was a bug fixed for 4.9 which resulted in the primary plane not always being added to the state, which means that the ->update hook would not get called when the plane is getting disabled.
Ah, which patch ?
At least that's my guess, the WARN_ON just tells you that no one sent out the event yet, but by that point it should have happened.
Yup :)
-Daniel
------------[ cut here ]------------ WARNING: CPU: 0 PID: 77 at drivers/gpu/drm/drm_atomic_helper.c:1579 drm_atomic_helper_commit_hw_done+0xa8/0xb8 Modules linked in: CPU: 0 PID: 77 Comm: kworker/0:2 Tainted: G W 4.8.0-rc7-next-20160923-00033-ge3f15d6 #56 Hardware name: Freescale i.MX6 SoloX (Device Tree) Workqueue: events drm_mode_rmfb_work_fn Backtrace: [<c010c180>] (dump_backtrace) from [<c010c378>] (show_stack+0x18/0x1c) r7:00000000 r6:600e0013 r5:00000000 r4:c0e2342c [<c010c360>] (show_stack) from [<c03f4ad8>] (dump_stack+0xb4/0xe8) [<c03f4a24>] (dump_stack) from [<c0124a90>] (__warn+0xd8/0x104) r9:c04bdb60 r8:0000062b r7:00000009 r6:c0c38298 r5:00000000 r4:00000000 [<c01249b8>] (__warn) from [<c0124b70>] (warn_slowpath_null+0x28/0x30) r9:db43639c r8:0000062b r7:c0c38298 r6:da962e40 r5:00000000 r4:00000000 [<c0124b48>] (warn_slowpath_null) from [<c04bdb60>] (drm_atomic_helper_commit_hw_done+0xa8/0xb8) [<c04bdab8>] (drm_atomic_helper_commit_hw_done) from [<c04c0568>] (drm_atomic_helper_commit_tail+0x44/0x60) r10:c0c3bd84 r9:c0c371cc r8:db770000 r7:db770000 r6:00000000 r5:db770000 r4:daffa780 r3:daffa3c0 [<c04c0524>] (drm_atomic_helper_commit_tail) from [<c04c05e8>] (commit_tail+0x64/0x68) r5:00000000 r4:daffa780 [<c04c0584>] (commit_tail) from [<c04c06b0>] (drm_atomic_helper_commit+0xac/0xec) r5:00000000 r4:daffa780 [<c04c0604>] (drm_atomic_helper_commit) from [<c04e11b4>] (drm_atomic_commit+0x58/0x78) r7:db436028 r6:db770000 r5:daffa780 r4:00000000 [<c04e115c>] (drm_atomic_commit) from [<c04c0be4>] (drm_atomic_helper_set_config+0x80/0xa4) r6:dad37e14 r5:00000000 r4:daffa780 [<c04c0b64>] (drm_atomic_helper_set_config) from [<c04d35fc>] (drm_mode_set_config_internal+0x6c/0xfc) r7:db77048c r6:db770480 r5:db436028 r4:00000000 [<c04d3590>] (drm_mode_set_config_internal) from [<c04d36c4>] (drm_crtc_force_disable+0x38/0x40) r7:db77048c r6:db770480 r5:da962c00 r4:db436028 [<c04d368c>] (drm_crtc_force_disable) from [<c04e2b60>] (drm_framebuffer_remove+0xd8/0x124) [<c04e2a88>] (drm_framebuffer_remove) from [<c04e2bec>] (drm_mode_rmfb_work_fn+0x40/0x50) r10:00000001 r9:00000000 r8:dbbb2a00 r7:dad37ea8 r6:dbbaf800 r5:db60be68 r4:db60be48 [<c04e2bac>] (drm_mode_rmfb_work_fn) from [<c013fa10>] (process_one_work+0x24c/0x4e8) r5:dad12f00 r4:db60be48 [<c013f7c4>] (process_one_work) from [<c01408dc>] (worker_thread+0x40/0x574) r10:dad12f00 r9:dad36000 r8:c0e02100 r7:dbbaf834 r6:00000008 r5:dad12f18 r4:dbbaf800 [<c014089c>] (worker_thread) from [<c0146908>] (kthread+0xd8/0xf8) r10:00000000 r9:00000000 r8:00000000 r7:c014089c r6:dad12f00 r5:dad28500 r4:00000000 [<c0146830>] (kthread) from [<c0107cb0>] (ret_from_fork+0x14/0x24) r7:00000000 r6:00000000 r5:c0146830 r4:dad28500 ---[ end trace 2b6e3065f0299ba5 ]---
But then, I see a few drivers (arm hdlcd, fsl-dcu,...) doing the same thing at random callbacks of CRTC . Shouldn't this event handling be consolidated into some generic function and those drivers fixed to call it from atomic update ?
-- Best regards, Marek Vasut
Den 27.09.2016 12:29, skrev Marek Vasut:
On 09/27/2016 09:49 AM, Daniel Vetter wrote:
On Mon, Sep 26, 2016 at 2:59 PM, Marek Vasut marex@denx.de wrote:
On 09/26/2016 11:41 AM, Marek Vasut wrote:
On 09/25/2016 11:00 PM, Daniel Vetter wrote:
On Sun, Sep 25, 2016 at 09:41:58PM +0200, Marek Vasut wrote:
Handle the vblank events in the simple_kms_helper driver, otherwise the drm_atomic_helper flip_done event never happens.
Signed-off-by: Marek Vasut marex@denx.de Cc: Noralf Trønnes noralf@tronnes.org Cc: Daniel Vetter daniel@ffwll.ch Cc: David Airlie airlied@linux.ie
drivers/gpu/drm/drm_simple_kms_helper.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)
diff --git a/drivers/gpu/drm/drm_simple_kms_helper.c b/drivers/gpu/drm/drm_simple_kms_helper.c index 7b6d26e..3345b40 100644 --- a/drivers/gpu/drm/drm_simple_kms_helper.c +++ b/drivers/gpu/drm/drm_simple_kms_helper.c @@ -34,6 +34,23 @@ static const struct drm_encoder_funcs drm_simple_kms_encoder_funcs = { .destroy = drm_encoder_cleanup, };
+static void drm_simple_kms_crtc_begin(struct drm_crtc *crtc,
struct drm_crtc_state *state)+{
- struct drm_pending_vblank_event *event = crtc->state->event;
- if (event) {
crtc->state->event = NULL;spin_lock_irq(&crtc->dev->event_lock);if (drm_crtc_vblank_get(crtc) == 0)drm_crtc_arm_vblank_event(crtc, event);elsedrm_crtc_send_vblank_event(crtc, event);spin_unlock_irq(&crtc->dev->event_lock);- }
+}
This should be done by drivers, in the ->update hook. At least if we want to pretend that it's semi-accurate and not racy (which the above is). -Daniel
Got it and wrapped into mxsfb, thanks.
Hrm, while this works most of the time, I managed to get a page_flip timeout when terminating the kmscube OpenGL demo (see below). I'm not getting this splat with this patch applied. Any idea what this could mean ?
Are you on latest drm-next?
No, I'm on next/master from 20160927 . Is this the right tree? https://cgit.freedesktop.org/~airlied/linux/log/?h=drm-next
There was a bug fixed for 4.9 which resulted in the primary plane not always being added to the state, which means that the ->update hook would not get called when the plane is getting disabled.
Ah, which patch ?
I believe this is the one:
drm/simple-helpers: Always add planes to the state update https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/driv...
Noralf.
At least that's my guess, the WARN_ON just tells you that no one sent out the event yet, but by that point it should have happened.
Yup :)
-Daniel
------------[ cut here ]------------ WARNING: CPU: 0 PID: 77 at drivers/gpu/drm/drm_atomic_helper.c:1579 drm_atomic_helper_commit_hw_done+0xa8/0xb8 Modules linked in: CPU: 0 PID: 77 Comm: kworker/0:2 Tainted: G W 4.8.0-rc7-next-20160923-00033-ge3f15d6 #56 Hardware name: Freescale i.MX6 SoloX (Device Tree) Workqueue: events drm_mode_rmfb_work_fn Backtrace: [<c010c180>] (dump_backtrace) from [<c010c378>] (show_stack+0x18/0x1c) r7:00000000 r6:600e0013 r5:00000000 r4:c0e2342c [<c010c360>] (show_stack) from [<c03f4ad8>] (dump_stack+0xb4/0xe8) [<c03f4a24>] (dump_stack) from [<c0124a90>] (__warn+0xd8/0x104) r9:c04bdb60 r8:0000062b r7:00000009 r6:c0c38298 r5:00000000 r4:00000000 [<c01249b8>] (__warn) from [<c0124b70>] (warn_slowpath_null+0x28/0x30) r9:db43639c r8:0000062b r7:c0c38298 r6:da962e40 r5:00000000 r4:00000000 [<c0124b48>] (warn_slowpath_null) from [<c04bdb60>] (drm_atomic_helper_commit_hw_done+0xa8/0xb8) [<c04bdab8>] (drm_atomic_helper_commit_hw_done) from [<c04c0568>] (drm_atomic_helper_commit_tail+0x44/0x60) r10:c0c3bd84 r9:c0c371cc r8:db770000 r7:db770000 r6:00000000 r5:db770000 r4:daffa780 r3:daffa3c0 [<c04c0524>] (drm_atomic_helper_commit_tail) from [<c04c05e8>] (commit_tail+0x64/0x68) r5:00000000 r4:daffa780 [<c04c0584>] (commit_tail) from [<c04c06b0>] (drm_atomic_helper_commit+0xac/0xec) r5:00000000 r4:daffa780 [<c04c0604>] (drm_atomic_helper_commit) from [<c04e11b4>] (drm_atomic_commit+0x58/0x78) r7:db436028 r6:db770000 r5:daffa780 r4:00000000 [<c04e115c>] (drm_atomic_commit) from [<c04c0be4>] (drm_atomic_helper_set_config+0x80/0xa4) r6:dad37e14 r5:00000000 r4:daffa780 [<c04c0b64>] (drm_atomic_helper_set_config) from [<c04d35fc>] (drm_mode_set_config_internal+0x6c/0xfc) r7:db77048c r6:db770480 r5:db436028 r4:00000000 [<c04d3590>] (drm_mode_set_config_internal) from [<c04d36c4>] (drm_crtc_force_disable+0x38/0x40) r7:db77048c r6:db770480 r5:da962c00 r4:db436028 [<c04d368c>] (drm_crtc_force_disable) from [<c04e2b60>] (drm_framebuffer_remove+0xd8/0x124) [<c04e2a88>] (drm_framebuffer_remove) from [<c04e2bec>] (drm_mode_rmfb_work_fn+0x40/0x50) r10:00000001 r9:00000000 r8:dbbb2a00 r7:dad37ea8 r6:dbbaf800 r5:db60be68 r4:db60be48 [<c04e2bac>] (drm_mode_rmfb_work_fn) from [<c013fa10>] (process_one_work+0x24c/0x4e8) r5:dad12f00 r4:db60be48 [<c013f7c4>] (process_one_work) from [<c01408dc>] (worker_thread+0x40/0x574) r10:dad12f00 r9:dad36000 r8:c0e02100 r7:dbbaf834 r6:00000008 r5:dad12f18 r4:dbbaf800 [<c014089c>] (worker_thread) from [<c0146908>] (kthread+0xd8/0xf8) r10:00000000 r9:00000000 r8:00000000 r7:c014089c r6:dad12f00 r5:dad28500 r4:00000000 [<c0146830>] (kthread) from [<c0107cb0>] (ret_from_fork+0x14/0x24) r7:00000000 r6:00000000 r5:c0146830 r4:dad28500 ---[ end trace 2b6e3065f0299ba5 ]---
But then, I see a few drivers (arm hdlcd, fsl-dcu,...) doing the same thing at random callbacks of CRTC . Shouldn't this event handling be consolidated into some generic function and those drivers fixed to call it from atomic update ?
-- Best regards, Marek Vasut
On 09/27/2016 02:16 PM, Noralf Trønnes wrote:
Den 27.09.2016 12:29, skrev Marek Vasut:
On 09/27/2016 09:49 AM, Daniel Vetter wrote:
On Mon, Sep 26, 2016 at 2:59 PM, Marek Vasut marex@denx.de wrote:
On 09/26/2016 11:41 AM, Marek Vasut wrote:
On 09/25/2016 11:00 PM, Daniel Vetter wrote:
On Sun, Sep 25, 2016 at 09:41:58PM +0200, Marek Vasut wrote: > Handle the vblank events in the simple_kms_helper driver, otherwise > the drm_atomic_helper flip_done event never happens. > > Signed-off-by: Marek Vasut marex@denx.de > Cc: Noralf Trønnes noralf@tronnes.org > Cc: Daniel Vetter daniel@ffwll.ch > Cc: David Airlie airlied@linux.ie > --- > drivers/gpu/drm/drm_simple_kms_helper.c | 18 ++++++++++++++++++ > 1 file changed, 18 insertions(+) > > diff --git a/drivers/gpu/drm/drm_simple_kms_helper.c > b/drivers/gpu/drm/drm_simple_kms_helper.c > index 7b6d26e..3345b40 100644 > --- a/drivers/gpu/drm/drm_simple_kms_helper.c > +++ b/drivers/gpu/drm/drm_simple_kms_helper.c > @@ -34,6 +34,23 @@ static const struct drm_encoder_funcs > drm_simple_kms_encoder_funcs = { > .destroy = drm_encoder_cleanup, > }; > > +static void drm_simple_kms_crtc_begin(struct drm_crtc *crtc, > + struct drm_crtc_state *state) > +{ > + struct drm_pending_vblank_event *event = crtc->state->event; > + > + if (event) { > + crtc->state->event = NULL; > + > + spin_lock_irq(&crtc->dev->event_lock); > + if (drm_crtc_vblank_get(crtc) == 0) > + drm_crtc_arm_vblank_event(crtc, event); > + else > + drm_crtc_send_vblank_event(crtc, event); > + spin_unlock_irq(&crtc->dev->event_lock); > + } > +} This should be done by drivers, in the ->update hook. At least if we want to pretend that it's semi-accurate and not racy (which the above is). -Daniel
Got it and wrapped into mxsfb, thanks.
Hrm, while this works most of the time, I managed to get a page_flip timeout when terminating the kmscube OpenGL demo (see below). I'm not getting this splat with this patch applied. Any idea what this could mean ?
Are you on latest drm-next?
No, I'm on next/master from 20160927 . Is this the right tree? https://cgit.freedesktop.org/~airlied/linux/log/?h=drm-next
There was a bug fixed for 4.9 which resulted in the primary plane not always being added to the state, which means that the ->update hook would not get called when the plane is getting disabled.
Ah, which patch ?
I believe this is the one:
drm/simple-helpers: Always add planes to the state update https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/driv...
Thanks. I have that one in my tree already.
On Tue, Sep 27, 2016 at 02:20:49PM +0200, Marek Vasut wrote:
On 09/27/2016 02:16 PM, Noralf Trønnes wrote:
Den 27.09.2016 12:29, skrev Marek Vasut:
On 09/27/2016 09:49 AM, Daniel Vetter wrote:
On Mon, Sep 26, 2016 at 2:59 PM, Marek Vasut marex@denx.de wrote:
On 09/26/2016 11:41 AM, Marek Vasut wrote:
On 09/25/2016 11:00 PM, Daniel Vetter wrote: > On Sun, Sep 25, 2016 at 09:41:58PM +0200, Marek Vasut wrote: >> Handle the vblank events in the simple_kms_helper driver, otherwise >> the drm_atomic_helper flip_done event never happens. >> >> Signed-off-by: Marek Vasut marex@denx.de >> Cc: Noralf Trønnes noralf@tronnes.org >> Cc: Daniel Vetter daniel@ffwll.ch >> Cc: David Airlie airlied@linux.ie >> --- >> drivers/gpu/drm/drm_simple_kms_helper.c | 18 ++++++++++++++++++ >> 1 file changed, 18 insertions(+) >> >> diff --git a/drivers/gpu/drm/drm_simple_kms_helper.c >> b/drivers/gpu/drm/drm_simple_kms_helper.c >> index 7b6d26e..3345b40 100644 >> --- a/drivers/gpu/drm/drm_simple_kms_helper.c >> +++ b/drivers/gpu/drm/drm_simple_kms_helper.c >> @@ -34,6 +34,23 @@ static const struct drm_encoder_funcs >> drm_simple_kms_encoder_funcs = { >> .destroy = drm_encoder_cleanup, >> }; >> >> +static void drm_simple_kms_crtc_begin(struct drm_crtc *crtc, >> + struct drm_crtc_state *state) >> +{ >> + struct drm_pending_vblank_event *event = crtc->state->event; >> + >> + if (event) { >> + crtc->state->event = NULL; >> + >> + spin_lock_irq(&crtc->dev->event_lock); >> + if (drm_crtc_vblank_get(crtc) == 0) >> + drm_crtc_arm_vblank_event(crtc, event); >> + else >> + drm_crtc_send_vblank_event(crtc, event); >> + spin_unlock_irq(&crtc->dev->event_lock); >> + } >> +} > This should be done by drivers, in the ->update hook. At least if > we want > to pretend that it's semi-accurate and not racy (which the above is). > -Daniel Got it and wrapped into mxsfb, thanks.
Hrm, while this works most of the time, I managed to get a page_flip timeout when terminating the kmscube OpenGL demo (see below). I'm not getting this splat with this patch applied. Any idea what this could mean ?
Are you on latest drm-next?
No, I'm on next/master from 20160927 . Is this the right tree? https://cgit.freedesktop.org/~airlied/linux/log/?h=drm-next
There was a bug fixed for 4.9 which resulted in the primary plane not always being added to the state, which means that the ->update hook would not get called when the plane is getting disabled.
Ah, which patch ?
I believe this is the one:
drm/simple-helpers: Always add planes to the state update https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/driv...
Thanks. I have that one in my tree already.
Can you pls check that for the case where you hit the WARN the simple pipe's ->update function is called, and that you're indeed handling the event in all cases? -Daniel
On 09/29/2016 11:28 AM, Daniel Vetter wrote:
On Tue, Sep 27, 2016 at 02:20:49PM +0200, Marek Vasut wrote:
On 09/27/2016 02:16 PM, Noralf Trønnes wrote:
Den 27.09.2016 12:29, skrev Marek Vasut:
On 09/27/2016 09:49 AM, Daniel Vetter wrote:
On Mon, Sep 26, 2016 at 2:59 PM, Marek Vasut marex@denx.de wrote:
On 09/26/2016 11:41 AM, Marek Vasut wrote: > On 09/25/2016 11:00 PM, Daniel Vetter wrote: >> On Sun, Sep 25, 2016 at 09:41:58PM +0200, Marek Vasut wrote: >>> Handle the vblank events in the simple_kms_helper driver, otherwise >>> the drm_atomic_helper flip_done event never happens. >>> >>> Signed-off-by: Marek Vasut marex@denx.de >>> Cc: Noralf Trønnes noralf@tronnes.org >>> Cc: Daniel Vetter daniel@ffwll.ch >>> Cc: David Airlie airlied@linux.ie >>> --- >>> drivers/gpu/drm/drm_simple_kms_helper.c | 18 ++++++++++++++++++ >>> 1 file changed, 18 insertions(+) >>> >>> diff --git a/drivers/gpu/drm/drm_simple_kms_helper.c >>> b/drivers/gpu/drm/drm_simple_kms_helper.c >>> index 7b6d26e..3345b40 100644 >>> --- a/drivers/gpu/drm/drm_simple_kms_helper.c >>> +++ b/drivers/gpu/drm/drm_simple_kms_helper.c >>> @@ -34,6 +34,23 @@ static const struct drm_encoder_funcs >>> drm_simple_kms_encoder_funcs = { >>> .destroy = drm_encoder_cleanup, >>> }; >>> >>> +static void drm_simple_kms_crtc_begin(struct drm_crtc *crtc, >>> + struct drm_crtc_state *state) >>> +{ >>> + struct drm_pending_vblank_event *event = crtc->state->event; >>> + >>> + if (event) { >>> + crtc->state->event = NULL; >>> + >>> + spin_lock_irq(&crtc->dev->event_lock); >>> + if (drm_crtc_vblank_get(crtc) == 0) >>> + drm_crtc_arm_vblank_event(crtc, event); >>> + else >>> + drm_crtc_send_vblank_event(crtc, event); >>> + spin_unlock_irq(&crtc->dev->event_lock); >>> + } >>> +} >> This should be done by drivers, in the ->update hook. At least if >> we want >> to pretend that it's semi-accurate and not racy (which the above is). >> -Daniel > Got it and wrapped into mxsfb, thanks. Hrm, while this works most of the time, I managed to get a page_flip timeout when terminating the kmscube OpenGL demo (see below). I'm not getting this splat with this patch applied. Any idea what this could mean ?
Are you on latest drm-next?
No, I'm on next/master from 20160927 . Is this the right tree? https://cgit.freedesktop.org/~airlied/linux/log/?h=drm-next
There was a bug fixed for 4.9 which resulted in the primary plane not always being added to the state, which means that the ->update hook would not get called when the plane is getting disabled.
Ah, which patch ?
I believe this is the one:
drm/simple-helpers: Always add planes to the state update https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/driv...
Thanks. I have that one in my tree already.
Can you pls check that for the case where you hit the WARN the simple pipe's ->update function is called, and that you're indeed handling the event in all cases?
The update is called, but I check whether I have plane->state->crtc non-NULL in the update function of mxsfb as I need to fish out the crtc->state->event from it. This plane->state->crtc is NULL in case when I get the backtrace. I can access the crtc via &pipe->crtc though, which is what I should probably do, right ?
-Daniel
On Thu, Sep 29, 2016 at 11:04 PM, Marek Vasut marex@denx.de wrote:
On 09/29/2016 11:28 AM, Daniel Vetter wrote:
On Tue, Sep 27, 2016 at 02:20:49PM +0200, Marek Vasut wrote:
On 09/27/2016 02:16 PM, Noralf Trønnes wrote:
Den 27.09.2016 12:29, skrev Marek Vasut:
On 09/27/2016 09:49 AM, Daniel Vetter wrote:
On Mon, Sep 26, 2016 at 2:59 PM, Marek Vasut marex@denx.de wrote: > On 09/26/2016 11:41 AM, Marek Vasut wrote: >> On 09/25/2016 11:00 PM, Daniel Vetter wrote: >>> On Sun, Sep 25, 2016 at 09:41:58PM +0200, Marek Vasut wrote: >>>> Handle the vblank events in the simple_kms_helper driver, otherwise >>>> the drm_atomic_helper flip_done event never happens. >>>> >>>> Signed-off-by: Marek Vasut marex@denx.de >>>> Cc: Noralf Trønnes noralf@tronnes.org >>>> Cc: Daniel Vetter daniel@ffwll.ch >>>> Cc: David Airlie airlied@linux.ie >>>> --- >>>> drivers/gpu/drm/drm_simple_kms_helper.c | 18 ++++++++++++++++++ >>>> 1 file changed, 18 insertions(+) >>>> >>>> diff --git a/drivers/gpu/drm/drm_simple_kms_helper.c >>>> b/drivers/gpu/drm/drm_simple_kms_helper.c >>>> index 7b6d26e..3345b40 100644 >>>> --- a/drivers/gpu/drm/drm_simple_kms_helper.c >>>> +++ b/drivers/gpu/drm/drm_simple_kms_helper.c >>>> @@ -34,6 +34,23 @@ static const struct drm_encoder_funcs >>>> drm_simple_kms_encoder_funcs = { >>>> .destroy = drm_encoder_cleanup, >>>> }; >>>> >>>> +static void drm_simple_kms_crtc_begin(struct drm_crtc *crtc, >>>> + struct drm_crtc_state *state) >>>> +{ >>>> + struct drm_pending_vblank_event *event = crtc->state->event; >>>> + >>>> + if (event) { >>>> + crtc->state->event = NULL; >>>> + >>>> + spin_lock_irq(&crtc->dev->event_lock); >>>> + if (drm_crtc_vblank_get(crtc) == 0) >>>> + drm_crtc_arm_vblank_event(crtc, event); >>>> + else >>>> + drm_crtc_send_vblank_event(crtc, event); >>>> + spin_unlock_irq(&crtc->dev->event_lock); >>>> + } >>>> +} >>> This should be done by drivers, in the ->update hook. At least if >>> we want >>> to pretend that it's semi-accurate and not racy (which the above is). >>> -Daniel >> Got it and wrapped into mxsfb, thanks. > Hrm, while this works most of the time, I managed to get a page_flip > timeout when terminating the kmscube OpenGL demo (see below). I'm not > getting this splat with this patch applied. Any idea what this could > mean ? Are you on latest drm-next?
No, I'm on next/master from 20160927 . Is this the right tree? https://cgit.freedesktop.org/~airlied/linux/log/?h=drm-next
There was a bug fixed for 4.9 which resulted in the primary plane not always being added to the state, which means that the ->update hook would not get called when the plane is getting disabled.
Ah, which patch ?
I believe this is the one:
drm/simple-helpers: Always add planes to the state update https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/driv...
Thanks. I have that one in my tree already.
Can you pls check that for the case where you hit the WARN the simple pipe's ->update function is called, and that you're indeed handling the event in all cases?
The update is called, but I check whether I have plane->state->crtc non-NULL in the update function of mxsfb as I need to fish out the crtc->state->event from it. This plane->state->crtc is NULL in case when I get the backtrace. I can access the crtc via &pipe->crtc though, which is what I should probably do, right ?
Yup. I would in general just always go through pipe->crtc ... One thing we could look into for simple pipe helpers is to have a drm_simple_pipe_state which contains both states, and pass that simple_state into all callbacks. Needs a bit of trickery in the atomic_duplicate_state hooks though, so not sure whether that's worth it. But merging plane/crtc state for simple pipe would be nice, since the plane/crtc itself are also merged. -Daniel
On 09/30/2016 11:50 AM, Daniel Vetter wrote:
On Thu, Sep 29, 2016 at 11:04 PM, Marek Vasut marex@denx.de wrote:
On 09/29/2016 11:28 AM, Daniel Vetter wrote:
On Tue, Sep 27, 2016 at 02:20:49PM +0200, Marek Vasut wrote:
On 09/27/2016 02:16 PM, Noralf Trønnes wrote:
Den 27.09.2016 12:29, skrev Marek Vasut:
On 09/27/2016 09:49 AM, Daniel Vetter wrote: > On Mon, Sep 26, 2016 at 2:59 PM, Marek Vasut marex@denx.de wrote: >> On 09/26/2016 11:41 AM, Marek Vasut wrote: >>> On 09/25/2016 11:00 PM, Daniel Vetter wrote: >>>> On Sun, Sep 25, 2016 at 09:41:58PM +0200, Marek Vasut wrote: >>>>> Handle the vblank events in the simple_kms_helper driver, otherwise >>>>> the drm_atomic_helper flip_done event never happens. >>>>> >>>>> Signed-off-by: Marek Vasut marex@denx.de >>>>> Cc: Noralf Trønnes noralf@tronnes.org >>>>> Cc: Daniel Vetter daniel@ffwll.ch >>>>> Cc: David Airlie airlied@linux.ie >>>>> --- >>>>> drivers/gpu/drm/drm_simple_kms_helper.c | 18 ++++++++++++++++++ >>>>> 1 file changed, 18 insertions(+) >>>>> >>>>> diff --git a/drivers/gpu/drm/drm_simple_kms_helper.c >>>>> b/drivers/gpu/drm/drm_simple_kms_helper.c >>>>> index 7b6d26e..3345b40 100644 >>>>> --- a/drivers/gpu/drm/drm_simple_kms_helper.c >>>>> +++ b/drivers/gpu/drm/drm_simple_kms_helper.c >>>>> @@ -34,6 +34,23 @@ static const struct drm_encoder_funcs >>>>> drm_simple_kms_encoder_funcs = { >>>>> .destroy = drm_encoder_cleanup, >>>>> }; >>>>> >>>>> +static void drm_simple_kms_crtc_begin(struct drm_crtc *crtc, >>>>> + struct drm_crtc_state *state) >>>>> +{ >>>>> + struct drm_pending_vblank_event *event = crtc->state->event; >>>>> + >>>>> + if (event) { >>>>> + crtc->state->event = NULL; >>>>> + >>>>> + spin_lock_irq(&crtc->dev->event_lock); >>>>> + if (drm_crtc_vblank_get(crtc) == 0) >>>>> + drm_crtc_arm_vblank_event(crtc, event); >>>>> + else >>>>> + drm_crtc_send_vblank_event(crtc, event); >>>>> + spin_unlock_irq(&crtc->dev->event_lock); >>>>> + } >>>>> +} >>>> This should be done by drivers, in the ->update hook. At least if >>>> we want >>>> to pretend that it's semi-accurate and not racy (which the above is). >>>> -Daniel >>> Got it and wrapped into mxsfb, thanks. >> Hrm, while this works most of the time, I managed to get a page_flip >> timeout when terminating the kmscube OpenGL demo (see below). I'm not >> getting this splat with this patch applied. Any idea what this could >> mean ? > Are you on latest drm-next? No, I'm on next/master from 20160927 . Is this the right tree? https://cgit.freedesktop.org/~airlied/linux/log/?h=drm-next
> There was a bug fixed for 4.9 which > resulted in the primary plane not always being added to the state, > which means that the ->update hook would not get called when the plane > is getting disabled. Ah, which patch ?
I believe this is the one:
drm/simple-helpers: Always add planes to the state update https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/driv...
Thanks. I have that one in my tree already.
Can you pls check that for the case where you hit the WARN the simple pipe's ->update function is called, and that you're indeed handling the event in all cases?
The update is called, but I check whether I have plane->state->crtc non-NULL in the update function of mxsfb as I need to fish out the crtc->state->event from it. This plane->state->crtc is NULL in case when I get the backtrace. I can access the crtc via &pipe->crtc though, which is what I should probably do, right ?
Yup. I would in general just always go through pipe->crtc ...
Got it, thanks.
One thing we could look into for simple pipe helpers is to have a drm_simple_pipe_state which contains both states, and pass that simple_state into all callbacks. Needs a bit of trickery in the atomic_duplicate_state hooks though, so not sure whether that's worth it. But merging plane/crtc state for simple pipe would be nice, since the plane/crtc itself are also merged.
I took a brief look into this, but I don't think I see the benefit of it. What am I missing here ?
Thanks
-Daniel
On Sun, Oct 2, 2016 at 7:15 PM, Marek Vasut marex@denx.de wrote:
One thing we could look into for simple pipe helpers is to have a drm_simple_pipe_state which contains both states, and pass that simple_state into all callbacks. Needs a bit of trickery in the atomic_duplicate_state hooks though, so not sure whether that's worth it. But merging plane/crtc state for simple pipe would be nice, since the plane/crtc itself are also merged.
I took a brief look into this, but I don't think I see the benefit of it. What am I missing here ?
Well if there's no benefit, then let's not do it - it was just an idea I had. -Daniel
On 10/03/2016 03:51 PM, Daniel Vetter wrote:
On Sun, Oct 2, 2016 at 7:15 PM, Marek Vasut marex@denx.de wrote:
One thing we could look into for simple pipe helpers is to have a drm_simple_pipe_state which contains both states, and pass that simple_state into all callbacks. Needs a bit of trickery in the atomic_duplicate_state hooks though, so not sure whether that's worth it. But merging plane/crtc state for simple pipe would be nice, since the plane/crtc itself are also merged.
I took a brief look into this, but I don't think I see the benefit of it. What am I missing here ?
Well if there's no benefit, then let's not do it - it was just an idea I had.
But I'm a newcomer to DRM/KMS framework, most of the time I barely know what I'm doing, so it's easily possible I missed something obvious. I can only think of reducing the number of memory allocations a little, but I am not convinced this is worth it.
-Daniel
On Mon, Sep 26, 2016 at 11:41 AM, Marek Vasut marex@denx.de wrote:
But then, I see a few drivers (arm hdlcd, fsl-dcu,...) doing the same thing at random callbacks of CRTC . Shouldn't this event handling be consolidated into some generic function and those drivers fixed to call it from atomic update ?
tbh they all look a bit suspect. Most of those are probably wrong, since I rolled them out semi-blindly just to give them some event handling - they entirely lacked that and wouldn't ever have run on e.g. wayland. -Daniel
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Marek Vasut -
Noralf Trønnes