[PATCH] drm: simple_kms_helper: Handle the vblank events

[Daniel Vetter]

On Thu, Sep 29, 2016 at 11:04 PM, Marek Vasut <marex at 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 at 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 at denx.de>
>>>>>>>>>> Cc: Noralf Trønnes <noralf at tronnes.org>
>>>>>>>>>> Cc: Daniel Vetter <daniel at ffwll.ch>
>>>>>>>>>> Cc: David Airlie <airlied at 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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch