[PATCH] drm: Defer disabling the vblank IRQ until the next interrupt (for instant-off)

[Mario Kleiner]

On 04/15/2015 03:03 AM, Mario Kleiner wrote:
> On 04/02/2015 01:34 PM, Chris Wilson wrote:
>> On vblank instant-off systems, we can get into a situation where the cost
>> of enabling and disabling the vblank IRQ around a drmWaitVblank query
>> dominates. However, we know that if the user wants the current vblank
>> counter, they are also very likely to immediately queue a vblank wait
>> and so we can keep the interrupt around and only turn it off if we have
>> no further vblank requests in the interrupt interval.
>>
>> After vblank event delivery there is a shadow of one vblank where the
>> interrupt is kept alive for the user to query and queue another vblank
>> event. Similarly, if the user is using blocking drmWaitVblanks, the
>> interrupt will be disabled on the IRQ following the wait completion.
>> However, if the user is simply querying the current vblank counter and
>> timestamp, the interrupt will be disabled after every IRQ and the user
>> will enabled it again on the first query following the IRQ.
>>
>> Testcase: igt/kms_vblank
>> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
>> Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
>> Cc: Daniel Vetter <daniel at ffwll.ch>
>> Cc: Michel Dänzer <michel at daenzer.net>
>> Cc: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>> Cc: Dave Airlie <airlied at redhat.com>,
>> Cc: Mario Kleiner <mario.kleiner.de at gmail.com>
>> ---
>>   drivers/gpu/drm/drm_irq.c | 15 +++++++++++++--
>>   1 file changed, 13 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
>> index c8a34476570a..6f5dc18779e2 100644
>> --- a/drivers/gpu/drm/drm_irq.c
>> +++ b/drivers/gpu/drm/drm_irq.c
>> @@ -1091,9 +1091,9 @@ void drm_vblank_put(struct drm_device *dev, int
>> crtc)
>>       if (atomic_dec_and_test(&vblank->refcount)) {
>>           if (drm_vblank_offdelay == 0)
>>               return;
>> -        else if (dev->vblank_disable_immediate || drm_vblank_offdelay
>> < 0)
>> +        else if (drm_vblank_offdelay < 0)
>>               vblank_disable_fn((unsigned long)vblank);
>> -        else
>> +        else if (!dev->vblank_disable_immediate)
>>               mod_timer(&vblank->disable_timer,
>>                     jiffies + ((drm_vblank_offdelay * HZ)/1000));
>>       }
>> @@ -1697,6 +1697,17 @@ bool drm_handle_vblank(struct drm_device *dev,
>> int crtc)
>>
>>       spin_lock_irqsave(&dev->event_lock, irqflags);
>>
>
> You could move the code before the spin_lock_irqsave(&dev->event_lock,
> irqflags); i think it doesn't need that lock?
>
>> +    if (dev->vblank_disable_immediate &&
>> !atomic_read(&vblank->refcount)) {
>
> Also check for (drm_vblank_offdelay > 0) to make sure we have a way out
> of instant disable here, and the same meaning of of drm_vblank_offdelay
> like we have in the current implementation.
>
> This hunk ...
>
>> +        unsigned long vbl_lock_irqflags;
>> +
>> +        spin_lock_irqsave(&dev->vbl_lock, vbl_lock_irqflags);
>> +        if (atomic_read(&vblank->refcount) == 0 && vblank->enabled) {
>> +            DRM_DEBUG("disabling vblank on crtc %d\n", crtc);
>> +            vblank_disable_and_save(dev, crtc);
>> +        }
>> +        spin_unlock_irqrestore(&dev->vbl_lock, vbl_lock_irqflags);
>
> ... is the same as a call to vblank_disable_fn((unsigned long) vblank);
> Maybe replace by that call?
>
> You could also return here already, as the code below will just take a
> lock, realize vblanks are now disabled and then release the locks and exit.
>
>> +    }
>> +
>>       /* Need timestamp lock to prevent concurrent execution with
>>        * vblank enable/disable, as this would cause inconsistent
>>        * or corrupted timestamps and vblank counts.
>>
>
> I think the logic itself is fine and at least basic testing of the patch
> on a Intel HD Ironlake didn't show problems, so with the above taken
> into account it would have my slightly uneasy reviewed-by.
>
> One thing that worries me a little bit about the disable inside vblank
> irq are the potential races between the disable code and the display
> engine which could cause really bad off-by-one errors for clients on a
> imperfect driver. These races can only happen if vblank enable or
> disable happens close to or inside the vblank. This approach lets the
> instant disable happen exactly inside vblank when there is the highest
> chance of triggering that condition.
>
> This doesn't seem to be a problem for intel kms, but other drivers don't
> have instant disable yet, so we don't know how well we could do it
> there. Additionally things like dynamic power management tend to operate
> inside vblank, sometimes with "funny" side effects to other stuff, e.g.,
> dpm on AMD, as i remember from some long debug session with Michel and
> Alex last summer where dpm played a role. Therefore it seems more safe
> to me to avoid actions inside vblank that could be done outside. E.g.,
> instead of doing the disable inside the vblank irq one could maybe just
> schedule an exact timer to do the disable a few milliseconds later in
> the middle of active scanout to avoid these potential issues?
>
> -mario

After testing this, one more thing that would make sense is to move the 
disable block at the end of drm_handle_vblank() instead of at the top.

Turns out that if high precision timestaming is disabled or doesn't work 
for some reason (as can be simulated by echo 0 > 
/sys/module/drm/parameters/timestamp_precision_usec), then with your 
delayed disable code at its current place, the vblank counter won't 
increment anymore at all for instant queries, ie. with your other 
"instant query" patches. Clients which repeatedly query the counter and 
wait for it to progress will simply hang, spinning in an endless query 
loop. There's that comment in vblank_disable_and_save:

"* Skip this step if there isn't any high precision timestamp
  * available. In that case we can't account for this and just
  * hope for the best.
  */

With the disable happening after leading edge of vblank (== hw counter 
increment already happened) but before the vblank counter/timestamp 
handling in drm_handle_vblank, that step is needed to keep the counter 
progressing, so skipping it is bad.

Now without high precision timestamping support, a kms driver must not 
set dev->vblank_disable_immediate = true, as this would cause problems 
for clients, so this shouldn't matter, but it would be good to still 
make this robust against a future kms driver which might have unreliable 
high precision timestamping, e.g., high precision timestamping that 
intermittently doesn't work.

For reference, my tweak of your patch looks now like this for the hunk 
at the bottom of drm_handle_vblank():

....
         spin_unlock(&dev->vblank_time_lock);

         wake_up(&vblank->queue);
         drm_handle_vblank_events(dev, crtc);

+        if (dev->vblank_disable_immediate && drm_vblank_offdelay > 0 &&
+                !atomic_read(&vblank->refcount)) {
+                unsigned long vbl_lock_irqflags;
+
+                spin_lock_irqsave(&dev->vbl_lock, vbl_lock_irqflags);
+                if (atomic_read(&vblank->refcount) == 0 && 
vblank->enabled) {
+                        DRM_DEBUG("disabling vblank on crtc %d\n", crtc);
+                        vblank_disable_and_save(dev, crtc);
+                }
+                spin_unlock_irqrestore(&dev->vbl_lock, vbl_lock_irqflags);
+        }
+
         spin_unlock_irqrestore(&dev->event_lock, irqflags);

         return true;


This could be further simplified to

......
         spin_unlock(&dev->vblank_time_lock);

         wake_up(&vblank->queue);
         drm_handle_vblank_events(dev, crtc);
+
+       if (dev->vblank_disable_immediate && drm_vblank_offdelay > 0 &&
+           !atomic_read(&vblank->refcount)) {
+		vblank_disable_fn(vblank);
+        }
+
         spin_unlock_irqrestore(&dev->event_lock, irqflags);

         return true;

The tweaked version worked fine in all my tests.
-mario