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

[Rob Clark]

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..

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

BR,
-R

> -mario
>
>
>
>> -               event->event.tv_sec = now.tv_sec;
>> -               event->event.tv_usec = now.tv_usec;
>> -               list_add_tail(&event->base.link,
>> -                               &event->base.file_priv->event_list);
>> -               wake_up_interruptible(&event->base.file_priv->event_wait);
>> -               spin_unlock_irqrestore(&dev->event_lock, flags);
>> -       }
>> +       spin_unlock_irqrestore(&dev->event_lock, flags);
>>   }
>>
>>   static void page_flip_cb(void *arg)
>>
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel