| Message ID | 20160925194158.7869-1-marex@denx.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. 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); >>> + 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 ? ------------[ 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); >>>> + 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? 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 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
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 ? > 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); >>>>>> + 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/drivers/gpu/drm/drm_simple_kms_helper.c?id=6dcf0de7ef10244b17442f47956a1d9fabe2abe1 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/drivers/gpu/drm/drm_simple_kms_helper.c?id=6dcf0de7ef10244b17442f47956a1d9fabe2abe1 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/drivers/gpu/drm/drm_simple_kms_helper.c?id=6dcf0de7ef10244b17442f47956a1d9fabe2abe1 > > 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/drivers/gpu/drm/drm_simple_kms_helper.c?id=6dcf0de7ef10244b17442f47956a1d9fabe2abe1 >> >> 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/drivers/gpu/drm/drm_simple_kms_helper.c?id=6dcf0de7ef10244b17442f47956a1d9fabe2abe1 >>> >>> 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/drivers/gpu/drm/drm_simple_kms_helper.c?id=6dcf0de7ef10244b17442f47956a1d9fabe2abe1 >>>> >>>> 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 >
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,
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(+)