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

Message ID 1349725849-22433-11-git-send-email-rob.clark@linaro.org
State New
Headers show

Commit Message

Rob Clark Oct. 8, 2012, 7:50 p.m.
From: Rob Clark <rob@ti.com>

Signed-off-by: Rob Clark <rob@ti.com>
---
 drivers/staging/omapdrm/omap_crtc.c |   31 ++++++-------------------------
 1 file changed, 6 insertions(+), 25 deletions(-)

Comments

Mario Kleiner Oct. 10, 2012, 3:33 a.m. | #1
On 08.10.12 21:50, Rob Clark wrote:
> From: Rob Clark <rob@ti.com>
>
> Signed-off-by: Rob Clark <rob@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.

-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)
>
Rob Clark Oct. 10, 2012, 11:03 a.m. | #2
On Tue, Oct 9, 2012 at 10:33 PM, Mario Kleiner
<mario.kleiner@tuebingen.mpg.de> wrote:
> On 08.10.12 21:50, Rob Clark wrote:
>>
>> From: Rob Clark <rob@ti.com>
>>
>> Signed-off-by: Rob Clark <rob@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@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
Mario Kleiner Oct. 12, 2012, 11:38 p.m. | #3
On 10.10.12 13:03, Rob Clark wrote:
> On Tue, Oct 9, 2012 at 10:33 PM, Mario Kleiner
> <mario.kleiner@tuebingen.mpg.de> wrote:
>> On 08.10.12 21:50, Rob Clark wrote:
>>>
>>> From: Rob Clark <rob@ti.com>
>>>
>>> Signed-off-by: Rob Clark <rob@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

Patch

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);
-		 */
-		event->event.sequence = sequence++;
-		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)