[PATCH] drm: simple_kms_helper: Handle the vblank events

[Marek Vasut]

On 09/30/2016 11:50 AM, Daniel Vetter wrote:
> 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 ...

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
> 


-- 
Best regards,
Marek Vasut