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