[PATCH 10/11] drm/omap: use drm_send_vblank_event() helper

[Mario Kleiner]

On 10.10.12 13:03, Rob Clark wrote:
> On Tue, Oct 9, 2012 at 10:33 PM, Mario Kleiner
> <mario.kleiner at tuebingen.mpg.de> wrote:
>> On 08.10.12 21:50, Rob Clark wrote:
>>>
>>> From: Rob Clark <rob at ti.com>
>>>
>>> Signed-off-by: Rob Clark <rob at ti.com>
>>> ---
>>>    drivers/staging/omapdrm/omap_crtc.c |   31
>>> ++++++-------------------------
>>>    1 file changed, 6 insertions(+), 25 deletions(-)
>>>
>>> diff --git a/drivers/staging/omapdrm/omap_crtc.c
>>> b/drivers/staging/omapdrm/omap_crtc.c
>>> index 732f2ad..74e019a 100644
>>> --- a/drivers/staging/omapdrm/omap_crtc.c
>>> +++ b/drivers/staging/omapdrm/omap_crtc.c
>>> @@ -114,40 +114,21 @@ static void omap_crtc_load_lut(struct drm_crtc
>>> *crtc)
>>>
>>>    static void vblank_cb(void *arg)
>>>    {
>>> -       static uint32_t sequence = 0;
>>>          struct drm_crtc *crtc = arg;
>>>          struct drm_device *dev = crtc->dev;
>>>          struct omap_crtc *omap_crtc = to_omap_crtc(crtc);
>>> -       struct drm_pending_vblank_event *event = omap_crtc->event;
>>>          unsigned long flags;
>>> -       struct timeval now;
>>>
>>>          WARN_ON(!event);
>>> +       spin_lock_irqsave(&dev->event_lock, flags);
>>> +
>>> +       /* wakeup userspace */
>>> +       if (omap_crtc->event)
>>> +               drm_send_vblank_event(dev, -1, omap_crtc->event);
>>>
>>>          omap_crtc->event = NULL;
>>>
>>> -       /* wakeup userspace */
>>> -       if (event) {
>>> -               do_gettimeofday(&now);
>>> -
>>> -               spin_lock_irqsave(&dev->event_lock, flags);
>>> -               /* TODO: we can't yet use the vblank time accounting,
>>> -                * because omapdss lower layer is the one that knows
>>> -                * the irq # and registers the handler, which more or
>>> -                * less defeats how drm_irq works.. for now just fake
>>> -                * the sequence number and use gettimeofday..
>>> -                *
>>> -               event->event.sequence = drm_vblank_count_and_time(
>>> -                               dev, omap_crtc->id, &now);
>>> -                */
>>
>>
>> I think this TOO comment should be retained as a reminder that there is work
>> to do.
>>
>>
>>> -               event->event.sequence = sequence++;
>>
>>
>> This is problematic. You lose the pseudo vblank counter implemented here,
>> which is a violation of the spec, and from my own experience it causes extra
>> pain and the need for awful workarounds in userspace clients. Nouveau-kms
>> has the same problem for no good reason.
>>
>> But then, on second thought, the way it is implemented here in the original
>> is even more wrong, returning zero might be better.
>
> I was actually debating whether or not to bother sending the omap
> patches, because I'm in middle of a re-write of the kms code in
> omapdrm that will (among many other things) give us proper vblank
> accounting..
>

Cool, that makes me happy :)

> There are a surprising # of other drivers that are just using
> do_gettimeofday() + seqn=0..  I guess, at least to be consistent,
> using seqn=0 is better than the completely bogus seqn.  Esp. when you
> consider having multiple CRTCs, they would end up not even having
> successive sequence numbers with the existing scheme.  But like I
> said, it's about to be replaced with something sane anyways :-P
>

Yes, zero is better in the meantime. E.g., to cope with nouveau's 
deficiency there, my app takes zero as "vblank count unsupported" and 
uses a fallback path, instead of getting confused.

Thanks for this nice cleanup.
-mario