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
