ecore_drm: Change page flipping logic so we can't tear

Summary:
Previously if we ever tried to queue up two page flips in less than a
retrace interval (which can easily happen since the evas clock isn't
based on vblank) we'd give up on ever using page flips again, and tear
on every screen update.

This fixes that by using a vblank callback for custom ticks and using
page flips whenever possible.

If a page flip fails it means a page flip raced with the vblank ticker,
so we need to queue up that frame when the current page flip completes.
This ensures that while we might drop interim frames, we will never
lose the most recent.

Now it should only be possible to tear if two ticks fire during the
wait for a page flip to complete.  This would result in rendering
taking place in the front buffer.  I don't think this can happen,
but an error is logged if it does.

Reviewers: zmike, devilhorns

Subscribers: cedric, jpeg

Differential Revision: https://phab.enlightenment.org/D3594

5 files changed, 116 insertions, 154 deletions

diff --git a/src/lib/ecore_drm/Ecore_Drm.h b/src/lib/ecore_drm/Ecore_Drm.h
index 0366899232..8d2787b187 100644
--- a/src/lib/ecore_drm/Ecore_Drm.h
+++ b/src/lib/ecore_drm/Ecore_Drm.h
@@ -671,10 +671,12 @@ EAPI void ecore_drm_fb_dirty(Ecore_Drm_Fb *fb, Eina_Rectangle *rects, unsigned i
  * @param dev The Ecore_Drm_Device to use
  * @param fb The Ecore_Drm_Fb to make the current framebuffer
  *
+ * @deprecated just call ecore_drm_fb_send() instead.
+ *
  * @ingroup Ecore_Drm_Fb_Group
  * @since 1.14
  */
-EAPI void ecore_drm_fb_set(Ecore_Drm_Device *dev, Ecore_Drm_Fb *fb);
+EINA_DEPRECATED EAPI void ecore_drm_fb_set(Ecore_Drm_Device *dev, Ecore_Drm_Fb *fb);
 
 /**
  * Send an Ecore_Drm_Fb to the Ecore_Drm_Device
diff --git a/src/lib/ecore_drm/ecore_drm_device.c b/src/lib/ecore_drm/ecore_drm_device.c
index cca3332124..756a5fcac9 100644
--- a/src/lib/ecore_drm/ecore_drm_device.c
+++ b/src/lib/ecore_drm/ecore_drm_device.c
@@ -10,93 +10,86 @@
        ((x) >= (xx)) && ((y) >= (yy)))
 
 static Eina_List *drm_devices;
-static int flip_count = 0;
+static int ticking = 0;
 
-static void 
-_ecore_drm_device_cb_page_flip(int fd EINA_UNUSED, unsigned int frame EINA_UNUSED, unsigned int sec EINA_UNUSED, unsigned int usec EINA_UNUSED, void *data)
+static void
+_ecore_drm_tick_schedule(Ecore_Drm_Device *dev)
 {
-   Ecore_Drm_Pageflip_Callback *cb;
-
-   /* DBG("Drm Page Flip Event"); */
-
-   if (!(cb = data)) return;
-
-   flip_count++;
-   if (flip_count < cb->count) return;
+   drmVBlank vbl;
 
-   cb->dev->current = cb->dev->next;
-   cb->dev->next = NULL;
+   if (!ticking) return;
 
-   flip_count = 0;
-   if (cb->func) cb->func(cb->data);
-   /* free(cb); */
+   vbl.request.type = (DRM_VBLANK_RELATIVE | DRM_VBLANK_EVENT);
+   vbl.request.sequence = 1;
+   vbl.request.signal = (unsigned long)dev;
+   drmWaitVBlank(dev->drm.fd, &vbl);
+}
 
-   /* Ecore_Drm_Output *output; */
+static void
+_ecore_drm_tick_begin(void *data)
+{
+   ticking = 1;
+   _ecore_drm_tick_schedule(data);
+}
 
-   /* DBG("Drm Page Flip Event"); */
+static void
+_ecore_drm_tick_end(void *data EINA_UNUSED)
+{
+   ticking = 0;
+}
 
-   /* if (!(output = data)) return; */
+static void
+_ecore_drm_tick_source_set(Ecore_Drm_Device *dev)
+{
+   ecore_animator_custom_source_tick_begin_callback_set
+     (_ecore_drm_tick_begin, dev);
+   ecore_animator_custom_source_tick_end_callback_set
+     (_ecore_drm_tick_end, dev);
+   ecore_animator_source_set(ECORE_ANIMATOR_SOURCE_CUSTOM);
+}
 
-   /* if (output->pending_flip) */
-   /*   { */
-   /*      if (output->dev->current) */
-   /*        ecore_drm_output_fb_release(output, output->dev->current); */
-   /*      output->dev->current = output->dev->next; */
-   /*      output->dev->next = NULL; */
-   /*   } */
+static void
+_ecore_drm_device_cb_page_flip(int fd EINA_UNUSED, unsigned int frame EINA_UNUSED, unsigned int sec EINA_UNUSED, unsigned int usec EINA_UNUSED, void *data)
+{
+   Ecore_Drm_Output *output = data;
+   Ecore_Drm_Fb *next;
 
-   /* output->pending_flip = EINA_FALSE; */
-   /* if (output->pending_destroy) */
-   /*   { */
-   /*      output->pending_destroy = EINA_FALSE; */
-   /*      ecore_drm_output_free(output); */
-   /*   } */
-   /* else if (!output->pending_vblank) */
-   /*   ecore_drm_output_repaint(output); */
+   if (output->pending_destroy)
+     {
+        ecore_drm_output_free(output);
+        return;
+     }
+   /* We were unable to queue a page on the last flip attempt, so we'll
+    * try again now. */
+   next = output->next;
+   if (next)
+     {
+        output->next = NULL;
+        ecore_drm_fb_send(output->dev, next, NULL, NULL);
+     }
 }
 
-static void 
+static void
 _ecore_drm_device_cb_vblank(int fd EINA_UNUSED, unsigned int frame EINA_UNUSED, unsigned int sec EINA_UNUSED, unsigned int usec EINA_UNUSED, void *data)
 {
-   Ecore_Drm_Sprite *sprite;
-   Ecore_Drm_Output *output;
-
-   /* DBG("Drm VBlank Event"); */
-
-   if (!(sprite = data)) return;
-
-   output = sprite->output;
-   output->pending_vblank = EINA_FALSE;
-
-   ecore_drm_output_fb_release(output, sprite->current_fb);
-   sprite->current_fb = sprite->next_fb;
-   sprite->next_fb = NULL;
-
-   if (!output->pending_flip) _ecore_drm_output_frame_finish(output);
+   ecore_animator_custom_tick();
+   if (ticking) _ecore_drm_tick_schedule(data);
 }
 
-#if 0
-static Eina_Bool 
-_ecore_drm_device_cb_idle(void *data)
+static Eina_Bool
+_cb_drm_event_handle(void *data, Ecore_Fd_Handler *hdlr EINA_UNUSED)
 {
-   Ecore_Drm_Device *dev;
-   Ecore_Drm_Output *output;
-   Eina_List *l;
-
-   if (!(dev = data)) return ECORE_CALLBACK_CANCEL;
+   Ecore_Drm_Device *dev = data;
+   int err;
 
-   if (!dev->active) return ECORE_CALLBACK_RENEW;
-
-   EINA_LIST_FOREACH(dev->outputs, l, output)
+   err = drmHandleEvent(dev->drm.fd, &dev->drm_ctx);
+   if (err)
      {
-        if ((!output->enabled) || (!output->need_repaint)) continue;
-        if (output->repaint_scheduled) continue;
-        _ecore_drm_output_repaint_start(output);
+        ERR("drmHandleEvent failed to read an event");
+        return EINA_FALSE;
      }
-
-   return ECORE_CALLBACK_RENEW;
+   return EINA_TRUE;
 }
-#endif
 
 static void
 _ecore_drm_device_cb_output_event(const char *device EINA_UNUSED, Eeze_Udev_Event event EINA_UNUSED, void *data, Eeze_Udev_Watch *watch EINA_UNUSED)
@@ -379,6 +372,10 @@ ecore_drm_device_open(Ecore_Drm_Device *dev)
      eeze_udev_watch_add(EEZE_UDEV_TYPE_DRM, events,
                          _ecore_drm_device_cb_output_event, dev);
 
+   dev->drm.hdlr =
+     ecore_main_fd_handler_add(dev->drm.fd, ECORE_FD_READ,
+                               _cb_drm_event_handle, dev, NULL, NULL);
+
    /* dev->drm.idler =  */
    /*   ecore_idle_enterer_add(_ecore_drm_device_cb_idle, dev); */
 
@@ -546,6 +543,7 @@ ecore_drm_device_software_setup(Ecore_Drm_Device *dev)
         DBG("\tSize: %d", dev->dumb[i]->size);
         DBG("\tW: %d\tH: %d", dev->dumb[i]->w, dev->dumb[i]->h);
      }
+   _ecore_drm_tick_source_set(dev);
 
    return EINA_TRUE;
 
diff --git a/src/lib/ecore_drm/ecore_drm_fb.c b/src/lib/ecore_drm/ecore_drm_fb.c
index 234b06f17c..8b7ca52fa9 100644
--- a/src/lib/ecore_drm/ecore_drm_fb.c
+++ b/src/lib/ecore_drm/ecore_drm_fb.c
@@ -167,7 +167,15 @@ ecore_drm_fb_dirty(Ecore_Drm_Fb *fb, Eina_Rectangle *rects, unsigned int count)
 }
 
 EAPI void
-ecore_drm_fb_set(Ecore_Drm_Device *dev, Ecore_Drm_Fb *fb)
+ecore_drm_fb_set(Ecore_Drm_Device *dev EINA_UNUSED, Ecore_Drm_Fb *fb EINA_UNUSED)
+{
+  /* ecore_drm_fb_set no longer has any functionality distinct from
+   * ecore_drm_fb_send so it is reduced to NO-OP to prevent messing up state
+   */
+}
+
+EAPI void
+ecore_drm_fb_send(Ecore_Drm_Device *dev, Ecore_Drm_Fb *fb, Ecore_Drm_Pageflip_Cb func EINA_UNUSED, void *data EINA_UNUSED)
 {
    Ecore_Drm_Output *output;
    Eina_List *l;
@@ -185,88 +193,58 @@ ecore_drm_fb_set(Ecore_Drm_Device *dev, Ecore_Drm_Fb *fb)
           }
      }
 
-   if (!dev->next) dev->next = fb;
-   if (!dev->next) return;
+   if (!dev->outputs) return;
 
    EINA_LIST_FOREACH(dev->outputs, l, output)
      {
-        int x = 0, y = 0;
-
         if ((!output->enabled) || (!output->current_mode)) continue;
 
-        if (!output->cloned)
-          {
-             x = output->x;
-             y = output->y;
-          }
+        if (output->next) WRN("fb reused too soon, tearing may be visible");
 
-        if ((!dev->current) ||
-            (dev->current->stride != dev->next->stride))
+        /* If we changed display parameters or haven't displayed anything
+         * yet we need to do a SetCrtc
+         */
+        if ((!output->current) ||
+            (output->current->stride != fb->stride))
           {
-             if (drmModeSetCrtc(dev->drm.fd, output->crtc_id, dev->next->id,
+             int x = 0, y = 0;
+
+             if (!output->cloned)
+               {
+                  x = output->x;
+                  y = output->y;
+               }
+             if (drmModeSetCrtc(dev->drm.fd, output->crtc_id, fb->id,
                                 x, y, &output->conn_id, 1,
                                 &output->current_mode->info))
                {
                   ERR("Failed to set Mode %dx%d for Output %s: %m",
                       output->current_mode->width, output->current_mode->height,
                       output->name);
+                  continue;
                }
+               output->current = fb;
+               output->next = NULL;
 
-             /* TODO: set dpms on ?? */
+               /* TODO: set dpms on ?? */
+               continue;
           }
-     }
-}
-
-EAPI void
-ecore_drm_fb_send(Ecore_Drm_Device *dev, Ecore_Drm_Fb *fb, Ecore_Drm_Pageflip_Cb func, void *data)
-{
-   Ecore_Drm_Output *output;
-   Eina_List *l;
-   Ecore_Drm_Pageflip_Callback *cb;
-
-   EINA_SAFETY_ON_NULL_RETURN(dev);
-   EINA_SAFETY_ON_NULL_RETURN(fb);
-   EINA_SAFETY_ON_NULL_RETURN(func);
-
-   if (eina_list_count(dev->outputs) < 1) return;
-
-   if (fb->pending_flip) return;
-
-   if (!(cb = calloc(1, sizeof(Ecore_Drm_Pageflip_Callback))))
-     return;
-
-   cb->dev = dev;
-   cb->func = func;
-   cb->data = data;
-
-   EINA_LIST_FOREACH(dev->outputs, l, output)
-     if (output->enabled) cb->count++;
-
-   EINA_LIST_FOREACH(dev->outputs, l, output)
-     {
-        if ((!output->enabled) || (!output->current_mode)) continue;
 
+        /* The normal case: We do a flip which waits for vblank and
+         * posts an event.
+         */
         if (drmModePageFlip(dev->drm.fd, output->crtc_id, fb->id,
-                            DRM_MODE_PAGE_FLIP_EVENT, cb) < 0)
+                            DRM_MODE_PAGE_FLIP_EVENT, output) < 0)
           {
-             ERR("Cannot flip crtc %u for connector %u: %m",
+             /* Failure to flip - likely there's already a flip
+              * queued, and we can't cancel, so just store this
+              * fb for later and it'll be queued in the flip
+              * handler */
+             DBG("flip crtc %u for connector %u failed, re-queued",
                  output->crtc_id, output->conn_id);
+             output->next = fb;
              continue;
           }
-
-        fb->pending_flip = EINA_TRUE;
-     }
-
-   while (fb->pending_flip)
-     {
-        int ret = 0;
-
-        ret = drmHandleEvent(dev->drm.fd, &dev->drm_ctx);
-        if (ret < 0)
-          {
-             ERR("drmHandleEvent Failed");
-             free(cb);
-             break;
-          }
+        output->current = fb;
      }
 }
diff --git a/src/lib/ecore_drm/ecore_drm_private.h b/src/lib/ecore_drm/ecore_drm_private.h
index de78d6d307..4271a7e481 100644
--- a/src/lib/ecore_drm/ecore_drm_private.h
+++ b/src/lib/ecore_drm/ecore_drm_private.h
@@ -150,6 +150,7 @@ struct _Ecore_Drm_Output
      } edid;
 
    Ecore_Drm_Backlight *backlight;   
+   Ecore_Drm_Fb *current, *next;
 
    Eina_Bool primary : 1;
    Eina_Bool connected : 1;
diff --git a/src/modules/evas/engines/drm/evas_outbuf.c b/src/modules/evas/engines/drm/evas_outbuf.c
index 11d3dc6aab..64b3ba9a52 100644
--- a/src/modules/evas/engines/drm/evas_outbuf.c
+++ b/src/modules/evas/engines/drm/evas_outbuf.c
@@ -8,23 +8,6 @@
 #define GREEN_MASK 0x00ff00
 #define BLUE_MASK 0x0000ff
 
-static void
-_evas_outbuf_cb_pageflip(void *data)
-{
-   Outbuf *ob;
-   Ecore_Drm_Fb *fb;
-
-   if (!(ob = data)) return;
-
-   /* DBG("Outbuf Pagelip Done"); */
-
-   if ((fb = ob->priv.buffer[ob->priv.curr]))
-     fb->pending_flip = EINA_FALSE;
-
-   ob->priv.last = ob->priv.curr;
-   ob->priv.curr = (ob->priv.curr + 1) % ob->priv.num;
-}
-
 static void 
 _evas_outbuf_buffer_swap(Outbuf *ob, Eina_Rectangle *rects, unsigned int count)
 {
@@ -35,11 +18,11 @@ _evas_outbuf_buffer_swap(Outbuf *ob, Eina_Rectangle *rects, unsigned int count)
    /* mark the fb as dirty */
    ecore_drm_fb_dirty(buff, rects, count);
 
-   /* if this buffer is not valid, we need to set it */
-   ecore_drm_fb_set(ob->info->info.dev, buff);
-
    /* send this buffer to the crtc */
-   ecore_drm_fb_send(ob->info->info.dev, buff, _evas_outbuf_cb_pageflip, ob);
+   ecore_drm_fb_send(ob->info->info.dev, buff, NULL, NULL);
+
+   ob->priv.last = ob->priv.curr;
+   ob->priv.curr = (ob->priv.curr + 1) % ob->priv.num;
 }
 
 Outbuf *
@@ -62,8 +45,8 @@ evas_outbuf_setup(Evas_Engine_Info_Drm *info, int w, int h)
    ob->destination_alpha = info->info.destination_alpha;
    ob->vsync = info->info.vsync;
 
-   /* default to double-buffer */
-   ob->priv.num = 2;
+   /* we must triple-buffer to prevent problems with the page flip handler */
+   ob->priv.num = 3;
 
    /* check for buffer override */
    if ((num = getenv("EVAS_DRM_BUFFERS")))
@@ -98,7 +81,7 @@ evas_outbuf_setup(Evas_Engine_Info_Drm *info, int w, int h)
      }
 
    /* set the front buffer to be the one on the crtc */
-   ecore_drm_fb_set(info->info.dev, ob->priv.buffer[0]);
+   ecore_drm_fb_send(info->info.dev, ob->priv.buffer[0], NULL, NULL);
 
    return ob;
 }