[PATCH xserver 0/6] modesetting: add DRI2 page flip support

[PATCH xserver 0/6] modesetting: add DRI2 page flip support

[Qiang Yu]

Hi guys,

This patch set is for adding DRI2 page flip support to modesetting
driver. I mainly take reference of amdgpu DDX and reuse present
page flip code in the modesetting driver.

Regards,
Qiang

Qiang Yu (6):
  modesetting: make ms_do_pageflip generic for share with DRI2
  modesetting: move ms_do_pageflip to pageflip.c
  modesetting: add DRI2 page flip support
  modesetting: exclude DRI2 and Present page flip
  modesetting: merge common page flip code for present and dri2
  modesetting: remove redundent pixmap destroy

 hw/xfree86/drivers/modesetting/Makefile.am       |   1 +
 hw/xfree86/drivers/modesetting/dri2.c            | 237 +++++++++++++++-
 hw/xfree86/drivers/modesetting/driver.h          |  21 ++
 hw/xfree86/drivers/modesetting/drmmode_display.h |   3 +
 hw/xfree86/drivers/modesetting/pageflip.c        | 344 +++++++++++++++++++++++
 hw/xfree86/drivers/modesetting/present.c         | 333 ++--------------------
 6 files changed, 624 insertions(+), 315 deletions(-)
 create mode 100644 hw/xfree86/drivers/modesetting/pageflip.c

-- 
2.7.4

_______________________________________________
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

[PATCH xserver 1/6] modesetting: make ms_do_pageflip generic for share with DRI2

[Qiang Yu]

Signed-off-by: Qiang Yu <Qiang.Yu@amd.com>
---
 hw/xfree86/drivers/modesetting/present.c | 36 +++++++++++++++++++-------------
 1 file changed, 22 insertions(+), 14 deletions(-)

diff --git a/hw/xfree86/drivers/modesetting/present.c b/hw/xfree86/drivers/modesetting/present.c
index 0093fb5..ff24594 100644
--- a/hw/xfree86/drivers/modesetting/present.c
+++ b/hw/xfree86/drivers/modesetting/present.c
@@ -235,7 +235,7 @@ ms_present_flush(WindowPtr window)
  */
 struct ms_flipdata {
     ScreenPtr screen;
-    struct ms_present_vblank_event *event;
+    void *event;
     /* number of CRTC events referencing this */
     int flip_count;
     uint64_t fe_msc;
@@ -254,6 +254,12 @@ struct ms_crtc_pageflip {
     struct ms_flipdata *flipdata;
 };
 
+typedef void (*ms_pageflip_handler_proc)(uint64_t frame,
+                                         uint64_t usec,
+                                         void *data);
+
+typedef void (*ms_pageflip_abort_proc)(void *data);
+
 /**
  * Free an ms_crtc_pageflip.
  *
@@ -277,7 +283,7 @@ ms_present_flip_free(struct ms_crtc_pageflip *flip)
  * extension code telling it when that happened
  */
 static void
-ms_flip_handler(uint64_t msc, uint64_t ust, void *data)
+ms_present_flip_handler(uint64_t msc, uint64_t ust, void *data)
 {
     struct ms_crtc_pageflip *flip = data;
     ScreenPtr screen = flip->flipdata->screen;
@@ -331,7 +337,9 @@ ms_present_flip_abort(void *data)
 static Bool
 queue_flip_on_crtc(ScreenPtr screen, xf86CrtcPtr crtc,
                    struct ms_flipdata *flipdata,
-                   int ref_crtc_vblank_pipe, uint32_t flags)
+                   int ref_crtc_vblank_pipe, uint32_t flags,
+                   ms_pageflip_handler_proc pageflip_handler,
+                   ms_pageflip_abort_proc pageflip_abort)
 {
     ScrnInfoPtr scrn = xf86ScreenToScrn(screen);
     modesettingPtr ms = modesettingPTR(scrn);
@@ -353,17 +361,12 @@ queue_flip_on_crtc(ScreenPtr screen, xf86CrtcPtr crtc,
     flip->on_reference_crtc = (drmmode_crtc->vblank_pipe == ref_crtc_vblank_pipe);
     flip->flipdata = flipdata;
 
-    seq = ms_drm_queue_alloc(crtc, flip, ms_flip_handler, ms_present_flip_abort);
+    seq = ms_drm_queue_alloc(crtc, flip, pageflip_handler, pageflip_abort);
     if (!seq) {
         free(flip);
         return FALSE;
     }
 
-    DebugPresent(("\t\tms:fq %lld c %d -> %d seq %llu\n",
-                  (long long) flipdata->event->event_id,
-                  flipdata->flip_count, flipdata->flip_count + 1,
-                  (long long) seq));
-
     /* take a reference on flipdata for use in flip */
     flipdata->flip_count++;
 
@@ -394,9 +397,11 @@ queue_flip_on_crtc(ScreenPtr screen, xf86CrtcPtr crtc,
 static Bool
 ms_do_pageflip(ScreenPtr screen,
                PixmapPtr new_front,
-               struct ms_present_vblank_event *event,
+               void *event,
                int ref_crtc_vblank_pipe,
-               Bool async)
+               Bool async,
+               ms_pageflip_handler_proc pageflip_handler,
+               ms_pageflip_abort_proc pageflip_abort)
 {
 #ifndef GLAMOR_HAS_GBM
     return FALSE;
@@ -470,7 +475,8 @@ ms_do_pageflip(ScreenPtr screen,
 
         if (!queue_flip_on_crtc(screen, crtc, flipdata,
                                 ref_crtc_vblank_pipe,
-                                flags)) {
+                                flags, pageflip_handler,
+                                pageflip_abort)) {
             goto error_undo;
         }
     }
@@ -589,7 +595,8 @@ ms_present_flip(RRCrtcPtr crtc,
         return FALSE;
 
     event->event_id = event_id;
-    ret = ms_do_pageflip(screen, pixmap, event, drmmode_crtc->vblank_pipe, !sync_flip);
+    ret = ms_do_pageflip(screen, pixmap, event, drmmode_crtc->vblank_pipe, !sync_flip,
+                         ms_present_flip_handler, ms_present_flip_abort);
     if (!ret)
         xf86DrvMsg(scrn->scrnIndex, X_ERROR, "present flip failed\n");
 
@@ -615,7 +622,8 @@ ms_present_unflip(ScreenPtr screen, uint64_t event_id)
     event->event_id = event_id;
 
     if (ms_present_check_flip(NULL, screen->root, pixmap, TRUE) &&
-        ms_do_pageflip(screen, pixmap, event, -1, FALSE)) {
+        ms_do_pageflip(screen, pixmap, event, -1, FALSE,
+                       ms_present_flip_handler, ms_present_flip_abort)) {
         return;
     }
 
-- 
2.7.4

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH xserver 1/6] modesetting: make ms_do_pageflip generic for share with DRI2

[Michel Dänzer]

On 17/08/16 07:29 PM, Qiang Yu wrote:
> 
> -    DebugPresent(("\t\tms:fq %lld c %d -> %d seq %llu\n",
> -                  (long long) flipdata->event->event_id,
> -                  flipdata->flip_count, flipdata->flip_count + 1,
> -                  (long long) seq));

Might be nice to move this debugging output to ms_present_flip instead
of removing it completely.


With that fixed, this patch is

Reviewed-by: Michel Dänzer <michel.daenzer@amd.com>


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

[PATCH xserver 2/6] modesetting: move ms_do_pageflip to pageflip.c

[Qiang Yu]

Signed-off-by: Qiang Yu <Qiang.Yu@amd.com>
---
 hw/xfree86/drivers/modesetting/Makefile.am |   1 +
 hw/xfree86/drivers/modesetting/driver.h    |  48 ++++++
 hw/xfree86/drivers/modesetting/pageflip.c  | 251 ++++++++++++++++++++++++++++
 hw/xfree86/drivers/modesetting/present.c   | 252 -----------------------------
 4 files changed, 300 insertions(+), 252 deletions(-)
 create mode 100644 hw/xfree86/drivers/modesetting/pageflip.c

diff --git a/hw/xfree86/drivers/modesetting/Makefile.am b/hw/xfree86/drivers/modesetting/Makefile.am
index ca7e05a..44823b0 100644
--- a/hw/xfree86/drivers/modesetting/Makefile.am
+++ b/hw/xfree86/drivers/modesetting/Makefile.am
@@ -54,6 +54,7 @@ modesetting_drv_la_SOURCES = \
 	 sh3224.c \
 	 sh3224.h \
 	 vblank.c \
+	 pageflip.c \
 	 $(NULL)
 
 drivermandir = $(DRIVER_MAN_DIR)
diff --git a/hw/xfree86/drivers/modesetting/driver.h b/hw/xfree86/drivers/modesetting/driver.h
index 498b9bf..c0d80a8 100644
--- a/hw/xfree86/drivers/modesetting/driver.h
+++ b/hw/xfree86/drivers/modesetting/driver.h
@@ -153,3 +153,51 @@ Bool ms_vblank_screen_init(ScreenPtr screen);
 void ms_vblank_close_screen(ScreenPtr screen);
 
 Bool ms_present_screen_init(ScreenPtr screen);
+
+#ifdef GLAMOR
+
+/*
+ * Event data for an in progress flip.
+ * This contains a pointer to the vblank event,
+ * and information about the flip in progress.
+ * a reference to this is stored in the per-crtc
+ * flips.
+ */
+struct ms_flipdata {
+    ScreenPtr screen;
+    void *event;
+    /* number of CRTC events referencing this */
+    int flip_count;
+    uint64_t fe_msc;
+    uint64_t fe_usec;
+    uint32_t old_fb_id;
+};
+
+/*
+ * Per crtc pageflipping infomation,
+ * These are submitted to the queuing code
+ * one of them per crtc per flip.
+ */
+struct ms_crtc_pageflip {
+    Bool on_reference_crtc;
+    /* reference to the ms_flipdata */
+    struct ms_flipdata *flipdata;
+};
+
+typedef void (*ms_pageflip_handler_proc)(uint64_t frame,
+                                         uint64_t usec,
+                                         void *data);
+
+typedef void (*ms_pageflip_abort_proc)(void *data);
+
+int ms_flush_drm_events(ScreenPtr screen);
+
+Bool ms_do_pageflip(ScreenPtr screen,
+                    PixmapPtr new_front,
+                    void *event,
+                    int ref_crtc_vblank_pipe,
+                    Bool async,
+                    ms_pageflip_handler_proc pageflip_handler,
+                    ms_pageflip_abort_proc pageflip_abort);
+
+#endif
diff --git a/hw/xfree86/drivers/modesetting/pageflip.c b/hw/xfree86/drivers/modesetting/pageflip.c
new file mode 100644
index 0000000..4549792
--- /dev/null
+++ b/hw/xfree86/drivers/modesetting/pageflip.c
@@ -0,0 +1,251 @@
+/*
+ * Copyright © 2014 Intel Corporation
+ *
+ * Permission to use, copy, modify, distribute, and sell this software and its
+ * documentation for any purpose is hereby granted without fee, provided that
+ * the above copyright notice appear in all copies and that both that copyright
+ * notice and this permission notice appear in supporting documentation, and
+ * that the name of the copyright holders not be used in advertising or
+ * publicity pertaining to distribution of the software without specific,
+ * written prior permission.  The copyright holders make no representations
+ * about the suitability of this software for any purpose.  It is provided "as
+ * is" without express or implied warranty.
+ *
+ * THE COPYRIGHT HOLDERS DISCLAIM ALL WARRANTIES WITH REGARD TO THIS SOFTWARE,
+ * INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS, IN NO
+ * EVENT SHALL THE COPYRIGHT HOLDERS BE LIABLE FOR ANY SPECIAL, INDIRECT OR
+ * CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM LOSS OF USE,
+ * DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE OR OTHER
+ * TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR PERFORMANCE
+ * OF THIS SOFTWARE.
+ */
+
+#ifdef HAVE_DIX_CONFIG_H
+#include "dix-config.h"
+#endif
+
+#include <xserver_poll.h>
+#include <xf86drm.h>
+
+#include "driver.h"
+
+#ifdef GLAMOR
+
+/*
+ * Flush the DRM event queue when full; makes space for new events.
+ *
+ * Returns a negative value on error, 0 if there was nothing to process,
+ * or 1 if we handled any events.
+ */
+int
+ms_flush_drm_events(ScreenPtr screen)
+{
+    ScrnInfoPtr scrn = xf86ScreenToScrn(screen);
+    modesettingPtr ms = modesettingPTR(scrn);
+
+    struct pollfd p = { .fd = ms->fd, .events = POLLIN };
+    int r;
+
+    do {
+            r = xserver_poll(&p, 1, 0);
+    } while (r == -1 && (errno == EINTR || errno == EAGAIN));
+
+    /* If there was an error, r will be < 0.  Return that.  If there was
+     * nothing to process, r == 0.  Return that.
+     */
+    if (r <= 0)
+        return r;
+
+    /* Try to handle the event.  If there was an error, return it. */
+    r = drmHandleEvent(ms->fd, &ms->event_context);
+    if (r < 0)
+        return r;
+
+    /* Otherwise return 1 to indicate that we handled an event. */
+    return 1;
+}
+
+static Bool
+queue_flip_on_crtc(ScreenPtr screen, xf86CrtcPtr crtc,
+                   struct ms_flipdata *flipdata,
+                   int ref_crtc_vblank_pipe, uint32_t flags,
+                   ms_pageflip_handler_proc pageflip_handler,
+                   ms_pageflip_abort_proc pageflip_abort)
+{
+    ScrnInfoPtr scrn = xf86ScreenToScrn(screen);
+    modesettingPtr ms = modesettingPTR(scrn);
+    drmmode_crtc_private_ptr drmmode_crtc = crtc->driver_private;
+    struct ms_crtc_pageflip *flip;
+    uint32_t seq;
+    int err;
+
+    flip = calloc(1, sizeof(struct ms_crtc_pageflip));
+    if (flip == NULL) {
+        xf86DrvMsg(scrn->scrnIndex, X_WARNING,
+                   "flip queue: carrier alloc failed.\n");
+        return FALSE;
+    }
+
+    /* Only the reference crtc will finally deliver its page flip
+     * completion event. All other crtc's events will be discarded.
+     */
+    flip->on_reference_crtc = (drmmode_crtc->vblank_pipe == ref_crtc_vblank_pipe);
+    flip->flipdata = flipdata;
+
+    seq = ms_drm_queue_alloc(crtc, flip, pageflip_handler, pageflip_abort);
+    if (!seq) {
+        free(flip);
+        return FALSE;
+    }
+
+    /* take a reference on flipdata for use in flip */
+    flipdata->flip_count++;
+
+    while (drmModePageFlip(ms->fd, drmmode_crtc->mode_crtc->crtc_id,
+                           ms->drmmode.fb_id, flags, (void *) (uintptr_t) seq)) {
+        err = errno;
+        /* We may have failed because the event queue was full.  Flush it
+         * and retry.  If there was nothing to flush, then we failed for
+         * some other reason and should just return an error.
+         */
+        if (ms_flush_drm_events(screen) <= 0) {
+            xf86DrvMsg(scrn->scrnIndex, X_WARNING,
+                       "flip queue failed: %s\n", strerror(err));
+            /* Aborting will also decrement flip_count and free(flip). */
+            ms_drm_abort_seq(scrn, seq);
+            return FALSE;
+        }
+
+        /* We flushed some events, so try again. */
+        xf86DrvMsg(scrn->scrnIndex, X_WARNING, "flip queue retry\n");
+    }
+
+    /* The page flip succeded. */
+    return TRUE;
+}
+
+
+Bool
+ms_do_pageflip(ScreenPtr screen,
+               PixmapPtr new_front,
+               void *event,
+               int ref_crtc_vblank_pipe,
+               Bool async,
+               ms_pageflip_handler_proc pageflip_handler,
+               ms_pageflip_abort_proc pageflip_abort)
+{
+#ifndef GLAMOR_HAS_GBM
+    return FALSE;
+#else
+    ScrnInfoPtr scrn = xf86ScreenToScrn(screen);
+    modesettingPtr ms = modesettingPTR(scrn);
+    xf86CrtcConfigPtr config = XF86_CRTC_CONFIG_PTR(scrn);
+    drmmode_bo new_front_bo;
+    uint32_t flags;
+    int i;
+    struct ms_flipdata *flipdata;
+    glamor_block_handler(screen);
+
+    new_front_bo.gbm = glamor_gbm_bo_from_pixmap(screen, new_front);
+    new_front_bo.dumb = NULL;
+    if (!new_front_bo.gbm) {
+        xf86DrvMsg(scrn->scrnIndex, X_ERROR,
+                   "Failed to get GBM bo for flip to new front.\n");
+        return FALSE;
+    }
+
+    flipdata = calloc(1, sizeof(struct ms_flipdata));
+    if (!flipdata) {
+        drmmode_bo_destroy(&ms->drmmode, &new_front_bo);
+        xf86DrvMsg(scrn->scrnIndex, X_ERROR,
+                   "Failed to allocate flipdata.\n");
+        return FALSE;
+    }
+
+    flipdata->event = event;
+    flipdata->screen = screen;
+
+    /*
+     * Take a local reference on flipdata.
+     * if the first flip fails, the sequence abort
+     * code will free the crtc flip data, and drop
+     * it's reference which would cause this to be
+     * freed when we still required it.
+     */
+    flipdata->flip_count++;
+
+    /* Create a new handle for the back buffer */
+    flipdata->old_fb_id = ms->drmmode.fb_id;
+    if (drmModeAddFB(ms->fd, scrn->virtualX, scrn->virtualY,
+                     scrn->depth, scrn->bitsPerPixel,
+                     drmmode_bo_get_pitch(&new_front_bo),
+                     drmmode_bo_get_handle(&new_front_bo), &ms->drmmode.fb_id)) {
+        goto error_out;
+    }
+
+    drmmode_bo_destroy(&ms->drmmode, &new_front_bo);
+
+    flags = DRM_MODE_PAGE_FLIP_EVENT;
+    if (async)
+        flags |= DRM_MODE_PAGE_FLIP_ASYNC;
+
+    /* Queue flips on all enabled CRTCs.
+     *
+     * Note that if/when we get per-CRTC buffers, we'll have to update this.
+     * Right now it assumes a single shared fb across all CRTCs, with the
+     * kernel fixing up the offset of each CRTC as necessary.
+     *
+     * Also, flips queued on disabled or incorrectly configured displays
+     * may never complete; this is a configuration error.
+     */
+    for (i = 0; i < config->num_crtc; i++) {
+        xf86CrtcPtr crtc = config->crtc[i];
+
+        if (!ms_crtc_on(crtc))
+            continue;
+
+        if (!queue_flip_on_crtc(screen, crtc, flipdata,
+                                ref_crtc_vblank_pipe,
+                                flags, pageflip_handler,
+                                pageflip_abort)) {
+            goto error_undo;
+        }
+    }
+
+    /*
+     * Do we have more than our local reference,
+     * if so and no errors, then drop our local
+     * reference and return now.
+     */
+    if (flipdata->flip_count > 1) {
+        flipdata->flip_count--;
+        return TRUE;
+    }
+
+error_undo:
+
+    /*
+     * Have we just got the local reference?
+     * free the framebuffer if so since nobody successfully
+     * submitted anything
+     */
+    if (flipdata->flip_count == 1) {
+        drmModeRmFB(ms->fd, ms->drmmode.fb_id);
+        ms->drmmode.fb_id = flipdata->old_fb_id;
+    }
+
+error_out:
+    xf86DrvMsg(scrn->scrnIndex, X_WARNING, "Page flip failed: %s\n",
+               strerror(errno));
+    /* if only the local reference - free the structure,
+     * else drop the local reference and return */
+    if (flipdata->flip_count == 1)
+        free(flipdata);
+    else
+        flipdata->flip_count--;
+
+    return FALSE;
+#endif /* GLAMOR_HAS_GBM */
+}
+
+#endif
diff --git a/hw/xfree86/drivers/modesetting/present.c b/hw/xfree86/drivers/modesetting/present.c
index ff24594..f4318b4 100644
--- a/hw/xfree86/drivers/modesetting/present.c
+++ b/hw/xfree86/drivers/modesetting/present.c
@@ -27,7 +27,6 @@
 #include <assert.h>
 #include <errno.h>
 #include <fcntl.h>
-#include <xserver_poll.h>
 #include <unistd.h>
 #include <stdio.h>
 #include <stdint.h>
@@ -72,40 +71,6 @@ ms_present_get_ust_msc(RRCrtcPtr crtc, CARD64 *ust, CARD64 *msc)
 }
 
 /*
- * Flush the DRM event queue when full; makes space for new events.
- *
- * Returns a negative value on error, 0 if there was nothing to process,
- * or 1 if we handled any events.
- */
-static int
-ms_flush_drm_events(ScreenPtr screen)
-{
-    ScrnInfoPtr scrn = xf86ScreenToScrn(screen);
-    modesettingPtr ms = modesettingPTR(scrn);
-
-    struct pollfd p = { .fd = ms->fd, .events = POLLIN };
-    int r;
-
-    do {
-            r = xserver_poll(&p, 1, 0);
-    } while (r == -1 && (errno == EINTR || errno == EAGAIN));
-
-    /* If there was an error, r will be < 0.  Return that.  If there was
-     * nothing to process, r == 0.  Return that.
-     */
-    if (r <= 0)
-        return r;
-
-    /* Try to handle the event.  If there was an error, return it. */
-    r = drmHandleEvent(ms->fd, &ms->event_context);
-    if (r < 0)
-        return r;
-
-    /* Otherwise return 1 to indicate that we handled an event. */
-    return 1;
-}
-
-/*
  * Called when the queued vblank event has occurred
  */
 static void
@@ -226,40 +191,6 @@ ms_present_flush(WindowPtr window)
 
 #ifdef GLAMOR
 
-/*
- * Event data for an in progress flip.
- * This contains a pointer to the vblank event,
- * and information about the flip in progress.
- * a reference to this is stored in the per-crtc
- * flips.
- */
-struct ms_flipdata {
-    ScreenPtr screen;
-    void *event;
-    /* number of CRTC events referencing this */
-    int flip_count;
-    uint64_t fe_msc;
-    uint64_t fe_usec;
-    uint32_t old_fb_id;
-};
-
-/*
- * Per crtc pageflipping infomation,
- * These are submitted to the queuing code
- * one of them per crtc per flip.
- */
-struct ms_crtc_pageflip {
-    Bool on_reference_crtc;
-    /* reference to the ms_flipdata */
-    struct ms_flipdata *flipdata;
-};
-
-typedef void (*ms_pageflip_handler_proc)(uint64_t frame,
-                                         uint64_t usec,
-                                         void *data);
-
-typedef void (*ms_pageflip_abort_proc)(void *data);
-
 /**
  * Free an ms_crtc_pageflip.
  *
@@ -334,189 +265,6 @@ ms_present_flip_abort(void *data)
     ms_present_flip_free(flip);
 }
 
-static Bool
-queue_flip_on_crtc(ScreenPtr screen, xf86CrtcPtr crtc,
-                   struct ms_flipdata *flipdata,
-                   int ref_crtc_vblank_pipe, uint32_t flags,
-                   ms_pageflip_handler_proc pageflip_handler,
-                   ms_pageflip_abort_proc pageflip_abort)
-{
-    ScrnInfoPtr scrn = xf86ScreenToScrn(screen);
-    modesettingPtr ms = modesettingPTR(scrn);
-    drmmode_crtc_private_ptr drmmode_crtc = crtc->driver_private;
-    struct ms_crtc_pageflip *flip;
-    uint32_t seq;
-    int err;
-
-    flip = calloc(1, sizeof(struct ms_crtc_pageflip));
-    if (flip == NULL) {
-        xf86DrvMsg(scrn->scrnIndex, X_WARNING,
-                   "flip queue: carrier alloc failed.\n");
-        return FALSE;
-    }
-
-    /* Only the reference crtc will finally deliver its page flip
-     * completion event. All other crtc's events will be discarded.
-     */
-    flip->on_reference_crtc = (drmmode_crtc->vblank_pipe == ref_crtc_vblank_pipe);
-    flip->flipdata = flipdata;
-
-    seq = ms_drm_queue_alloc(crtc, flip, pageflip_handler, pageflip_abort);
-    if (!seq) {
-        free(flip);
-        return FALSE;
-    }
-
-    /* take a reference on flipdata for use in flip */
-    flipdata->flip_count++;
-
-    while (drmModePageFlip(ms->fd, drmmode_crtc->mode_crtc->crtc_id,
-                           ms->drmmode.fb_id, flags, (void *) (uintptr_t) seq)) {
-        err = errno;
-        /* We may have failed because the event queue was full.  Flush it
-         * and retry.  If there was nothing to flush, then we failed for
-         * some other reason and should just return an error.
-         */
-        if (ms_flush_drm_events(screen) <= 0) {
-            xf86DrvMsg(scrn->scrnIndex, X_WARNING,
-                       "flip queue failed: %s\n", strerror(err));
-            /* Aborting will also decrement flip_count and free(flip). */
-            ms_drm_abort_seq(scrn, seq);
-            return FALSE;
-        }
-
-        /* We flushed some events, so try again. */
-        xf86DrvMsg(scrn->scrnIndex, X_WARNING, "flip queue retry\n");
-    }
-
-    /* The page flip succeded. */
-    return TRUE;
-}
-
-
-static Bool
-ms_do_pageflip(ScreenPtr screen,
-               PixmapPtr new_front,
-               void *event,
-               int ref_crtc_vblank_pipe,
-               Bool async,
-               ms_pageflip_handler_proc pageflip_handler,
-               ms_pageflip_abort_proc pageflip_abort)
-{
-#ifndef GLAMOR_HAS_GBM
-    return FALSE;
-#else
-    ScrnInfoPtr scrn = xf86ScreenToScrn(screen);
-    modesettingPtr ms = modesettingPTR(scrn);
-    xf86CrtcConfigPtr config = XF86_CRTC_CONFIG_PTR(scrn);
-    drmmode_bo new_front_bo;
-    uint32_t flags;
-    int i;
-    struct ms_flipdata *flipdata;
-    glamor_block_handler(screen);
-
-    new_front_bo.gbm = glamor_gbm_bo_from_pixmap(screen, new_front);
-    new_front_bo.dumb = NULL;
-    if (!new_front_bo.gbm) {
-        xf86DrvMsg(scrn->scrnIndex, X_ERROR,
-                   "Failed to get GBM bo for flip to new front.\n");
-        return FALSE;
-    }
-
-    flipdata = calloc(1, sizeof(struct ms_flipdata));
-    if (!flipdata) {
-        drmmode_bo_destroy(&ms->drmmode, &new_front_bo);
-        xf86DrvMsg(scrn->scrnIndex, X_ERROR,
-                   "Failed to allocate flipdata.\n");
-        return FALSE;
-    }
-
-    flipdata->event = event;
-    flipdata->screen = screen;
-
-    /*
-     * Take a local reference on flipdata.
-     * if the first flip fails, the sequence abort
-     * code will free the crtc flip data, and drop
-     * it's reference which would cause this to be
-     * freed when we still required it.
-     */
-    flipdata->flip_count++;
-
-    /* Create a new handle for the back buffer */
-    flipdata->old_fb_id = ms->drmmode.fb_id;
-    if (drmModeAddFB(ms->fd, scrn->virtualX, scrn->virtualY,
-                     scrn->depth, scrn->bitsPerPixel,
-                     drmmode_bo_get_pitch(&new_front_bo),
-                     drmmode_bo_get_handle(&new_front_bo), &ms->drmmode.fb_id)) {
-        goto error_out;
-    }
-
-    drmmode_bo_destroy(&ms->drmmode, &new_front_bo);
-
-    flags = DRM_MODE_PAGE_FLIP_EVENT;
-    if (async)
-        flags |= DRM_MODE_PAGE_FLIP_ASYNC;
-
-    /* Queue flips on all enabled CRTCs.
-     *
-     * Note that if/when we get per-CRTC buffers, we'll have to update this.
-     * Right now it assumes a single shared fb across all CRTCs, with the
-     * kernel fixing up the offset of each CRTC as necessary.
-     *
-     * Also, flips queued on disabled or incorrectly configured displays
-     * may never complete; this is a configuration error.
-     */
-    for (i = 0; i < config->num_crtc; i++) {
-        xf86CrtcPtr crtc = config->crtc[i];
-
-        if (!ms_crtc_on(crtc))
-            continue;
-
-        if (!queue_flip_on_crtc(screen, crtc, flipdata,
-                                ref_crtc_vblank_pipe,
-                                flags, pageflip_handler,
-                                pageflip_abort)) {
-            goto error_undo;
-        }
-    }
-
-    /*
-     * Do we have more than our local reference,
-     * if so and no errors, then drop our local
-     * reference and return now.
-     */
-    if (flipdata->flip_count > 1) {
-        flipdata->flip_count--;
-        return TRUE;
-    }
-
-error_undo:
-
-    /*
-     * Have we just got the local reference?
-     * free the framebuffer if so since nobody successfully
-     * submitted anything
-     */
-    if (flipdata->flip_count == 1) {
-        drmModeRmFB(ms->fd, ms->drmmode.fb_id);
-        ms->drmmode.fb_id = flipdata->old_fb_id;
-    }
-
-error_out:
-    xf86DrvMsg(scrn->scrnIndex, X_WARNING, "Page flip failed: %s\n",
-               strerror(errno));
-    /* if only the local reference - free the structure,
-     * else drop the local reference and return */
-    if (flipdata->flip_count == 1)
-        free(flipdata);
-    else
-        flipdata->flip_count--;
-
-    return FALSE;
-#endif /* GLAMOR_HAS_GBM */
-}
-
 /*
  * Test to see if page flipping is possible on the target crtc
  */
-- 
2.7.4

_______________________________________________
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

[PATCH xserver 3/6] modesetting: add DRI2 page flip support

[Qiang Yu]

Signed-off-by: Qiang Yu <Qiang.Yu@amd.com>
---
 hw/xfree86/drivers/modesetting/dri2.c | 260 ++++++++++++++++++++++++++++++++--
 1 file changed, 251 insertions(+), 9 deletions(-)

diff --git a/hw/xfree86/drivers/modesetting/dri2.c b/hw/xfree86/drivers/modesetting/dri2.c
index 83cb3e0..11e0e6f 100644
--- a/hw/xfree86/drivers/modesetting/dri2.c
+++ b/hw/xfree86/drivers/modesetting/dri2.c
@@ -46,6 +46,7 @@
 
 enum ms_dri2_frame_event_type {
     MS_DRI2_QUEUE_SWAP,
+    MS_DRI2_QUEUE_FLIP,
     MS_DRI2_WAIT_MSC,
 };
 
@@ -399,6 +400,220 @@ ms_dri2_blit_swap(DrawablePtr drawable,
     ms_dri2_copy_region(drawable, &region, dst, src);
 }
 
+struct ms_dri2_vblank_event {
+    XID drawable_id;
+    ClientPtr client;
+    DRI2SwapEventPtr event_complete;
+    void *event_data;
+};
+
+static void
+ms_dri2_flip_free(struct ms_crtc_pageflip *flip)
+{
+    struct ms_flipdata *flipdata = flip->flipdata;
+
+    free(flip);
+    if (--flipdata->flip_count > 0)
+        return;
+    free(flipdata);
+}
+
+static void
+ms_dri2_flip_abort(void *data)
+{
+    struct ms_crtc_pageflip *flip = data;
+    struct ms_flipdata *flipdata = flip->flipdata;
+
+    if (flipdata->flip_count == 1)
+        free(flipdata->event);
+
+    ms_dri2_flip_free(flip);
+}
+
+static void
+ms_dri2_flip_handler(uint64_t msc, uint64_t ust, void *data)
+{
+    struct ms_crtc_pageflip *flip = data;
+    ScreenPtr screen = flip->flipdata->screen;
+    ScrnInfoPtr scrn = xf86ScreenToScrn(screen);
+    modesettingPtr ms = modesettingPTR(scrn);
+    struct ms_flipdata *flipdata = flip->flipdata;
+    struct ms_dri2_vblank_event *event = flipdata->event;
+
+    if (flip->on_reference_crtc) {
+        flipdata->fe_msc = msc;
+        flipdata->fe_usec = ust;
+    }
+
+    if (flipdata->flip_count == 1) {
+        uint32_t frame = flipdata->fe_msc;
+        uint32_t tv_sec = flipdata->fe_usec / 1000000;
+        uint32_t tv_usec = flipdata->fe_usec % 1000000;
+        DrawablePtr drawable;
+        int status;
+
+        status = dixLookupDrawable(&drawable, event->drawable_id, serverClient,
+                                   M_ANY, DixWriteAccess);
+        if (status == Success)
+            DRI2SwapComplete(event->client, drawable, frame, tv_sec, tv_usec,
+                             DRI2_FLIP_COMPLETE, event->event_complete,
+                             event->event_data);
+        drmModeRmFB(ms->fd, flipdata->old_fb_id);
+    }
+
+    ms_dri2_flip_free(flip);
+}
+
+static Bool
+ms_dri2_schedule_flip(ms_dri2_frame_event_ptr info)
+{
+    DrawablePtr draw = info->drawable;
+    ScreenPtr screen = draw->pScreen;
+    ms_dri2_buffer_private_ptr back_priv = info->back->driverPrivate;
+    struct ms_dri2_vblank_event *event;
+    drmmode_crtc_private_ptr drmmode_crtc = info->crtc->driver_private;
+
+    event = calloc(1, sizeof(struct ms_dri2_vblank_event));
+    if (!event)
+        return FALSE;
+
+    event->drawable_id = draw->id;
+    event->client = info->client;
+    event->event_complete = info->event_complete;
+    event->event_data = info->event_data;
+
+    if (ms_do_pageflip(screen, back_priv->pixmap, event,
+                       drmmode_crtc->vblank_pipe, FALSE,
+                       ms_dri2_flip_handler,
+                       ms_dri2_flip_abort)) {
+        return TRUE;
+    }
+    return FALSE;
+}
+
+static Bool
+update_front(DrawablePtr draw, DRI2BufferPtr front)
+{
+    ScreenPtr screen = draw->pScreen;
+    PixmapPtr pixmap = get_drawable_pixmap(draw);
+    ms_dri2_buffer_private_ptr priv = front->driverPrivate;
+    CARD32 size;
+    CARD16 pitch;
+
+    front->name = glamor_name_from_pixmap(pixmap, &pitch, &size);
+    if (front->name < 0)
+        return FALSE;
+
+    (*screen->DestroyPixmap) (priv->pixmap);
+    front->pitch = pixmap->devKind;
+    front->cpp = pixmap->drawable.bitsPerPixel / 8;
+    priv->pixmap = pixmap;
+    pixmap->refcnt++;
+
+    return TRUE;
+}
+
+static Bool
+can_exchange(ScrnInfoPtr scrn, DrawablePtr draw,
+	     DRI2BufferPtr front, DRI2BufferPtr back)
+{
+    ms_dri2_buffer_private_ptr front_priv = front->driverPrivate;
+    ms_dri2_buffer_private_ptr back_priv = back->driverPrivate;
+    PixmapPtr front_pixmap;
+    PixmapPtr back_pixmap = back_priv->pixmap;
+    xf86CrtcConfigPtr config = XF86_CRTC_CONFIG_PTR(scrn);
+    int num_crtcs_on = 0;
+    int i;
+
+    for (i = 0; i < config->num_crtc; i++) {
+        drmmode_crtc_private_ptr drmmode_crtc = config->crtc[i]->driver_private;
+
+        /* Don't do pageflipping if CRTCs are rotated. */
+#ifdef GLAMOR_HAS_GBM
+        if (drmmode_crtc->rotate_bo.gbm)
+            return FALSE;
+#endif
+
+        if (ms_crtc_on(config->crtc[i]))
+            num_crtcs_on++;
+    }
+
+    /* We can't do pageflipping if all the CRTCs are off. */
+    if (num_crtcs_on == 0)
+        return FALSE;
+
+    if (!update_front(draw, front))
+        return FALSE;
+
+    front_pixmap = front_priv->pixmap;
+
+    if (front_pixmap->drawable.width != back_pixmap->drawable.width)
+        return FALSE;
+
+    if (front_pixmap->drawable.height != back_pixmap->drawable.height)
+        return FALSE;
+
+    if (front_pixmap->drawable.bitsPerPixel !=
+        back_pixmap->drawable.bitsPerPixel)
+        return FALSE;
+
+    if (front_pixmap->devKind != back_pixmap->devKind)
+        return FALSE;
+
+    return TRUE;
+}
+
+static Bool
+can_flip(ScrnInfoPtr scrn, DrawablePtr draw,
+	 DRI2BufferPtr front, DRI2BufferPtr back)
+{
+    modesettingPtr ms = modesettingPTR(scrn);
+
+    return draw->type == DRAWABLE_WINDOW &&
+        ms->drmmode.pageflip &&
+        scrn->vtSema &&
+        DRI2CanFlip(draw) && can_exchange(scrn, draw, front, back);
+}
+
+static void
+ms_dri2_exchange_buffers(DrawablePtr draw, DRI2BufferPtr front,
+                         DRI2BufferPtr back)
+{
+    ms_dri2_buffer_private_ptr front_priv = front->driverPrivate;
+    ms_dri2_buffer_private_ptr back_priv = back->driverPrivate;
+    ScreenPtr screen = draw->pScreen;
+    ScrnInfoPtr scrn = xf86ScreenToScrn(screen);
+    modesettingPtr ms = modesettingPTR(scrn);
+    msPixmapPrivPtr front_pix = msGetPixmapPriv(&ms->drmmode, front_priv->pixmap);
+    msPixmapPrivPtr back_pix = msGetPixmapPriv(&ms->drmmode, back_priv->pixmap);
+    msPixmapPrivRec tmp_pix;
+    RegionRec region;
+    int tmp;
+
+    /* Swap BO names so DRI works */
+    tmp = front->name;
+    front->name = back->name;
+    back->name = tmp;
+
+    /* Swap pixmap privates */
+    tmp_pix = *front_pix;
+    *front_pix = *back_pix;
+    *back_pix = tmp_pix;
+
+    glamor_egl_exchange_buffers(front_priv->pixmap, back_priv->pixmap);
+
+    /* Post damage on the front buffer so that listeners, such
+     * as DisplayLink know take a copy and shove it over the USB.
+     * also for sw cursors.
+     */
+    region.extents.x1 = region.extents.y1 = 0;
+    region.extents.x2 = front_priv->pixmap->drawable.width;
+    region.extents.y2 = front_priv->pixmap->drawable.width;
+    region.data = NULL;
+    DamageRegionAppend(&front_priv->pixmap->drawable, &region);
+    DamageRegionProcessPending(&front_priv->pixmap->drawable);
+}
+
 static void
 ms_dri2_frame_event_handler(uint64_t msc,
                             uint64_t usec,
@@ -417,6 +632,13 @@ ms_dri2_frame_event_handler(uint64_t msc,
     }
 
     switch (frame_info->type) {
+    case MS_DRI2_QUEUE_FLIP:
+        if (can_flip(scrn, drawable, frame_info->front, frame_info->back) &&
+            ms_dri2_schedule_flip(frame_info)) {
+            ms_dri2_exchange_buffers(drawable, frame_info->front, frame_info->back);
+            break;
+        }
+        /* else fall through to blit */
     case MS_DRI2_QUEUE_SWAP:
         ms_dri2_blit_swap(drawable, frame_info->front, frame_info->back);
         DRI2SwapComplete(frame_info->client, drawable, msc, tv_sec, tv_usec,
@@ -607,7 +829,7 @@ ms_dri2_schedule_swap(ClientPtr client, DrawablePtr draw,
     ScrnInfoPtr scrn = xf86ScreenToScrn(screen);
     modesettingPtr ms = modesettingPTR(scrn);
     drmVBlank vbl;
-    int ret;
+    int ret, flip = 0;
     xf86CrtcPtr crtc = ms_dri2_crtc_covering_drawable(draw);
     drmmode_crtc_private_ptr drmmode_crtc;
     ms_dri2_frame_event_ptr frame_info = NULL;
@@ -645,20 +867,36 @@ ms_dri2_schedule_swap(ClientPtr client, DrawablePtr draw,
 
     ret = ms_get_crtc_ust_msc(crtc, &current_ust, &current_msc);
 
+    /* Flips need to be submitted one frame before */
+    if (can_flip(scrn, draw, front, back)) {
+        frame_info->type = MS_DRI2_QUEUE_FLIP;
+        flip = 1;
+    }
+
+    /* Correct target_msc by 'flip' if frame_info->type == MS_DRI2_QUEUE_FLIP.
+     * Do it early, so handling of different timing constraints
+     * for divisor, remainder and msc vs. target_msc works.
+     */
+    if (*target_msc > 0)
+        *target_msc -= flip;
+
     /*
      * If divisor is zero, or current_msc is smaller than target_msc
      * we just need to make sure target_msc passes before initiating
      * the swap.
      */
     if (divisor == 0 || current_msc < *target_msc) {
-        /* We need to use DRM_VBLANK_NEXTONMISS to avoid unreliable
-         * timestamping later on.
-         */
         vbl.request.type = (DRM_VBLANK_ABSOLUTE |
-                            DRM_VBLANK_NEXTONMISS |
                             DRM_VBLANK_EVENT |
                             drmmode_crtc->vblank_pipe);
 
+        /* If non-pageflipping, but blitting/exchanging, we need to use
+         * DRM_VBLANK_NEXTONMISS to avoid unreliable timestamping later
+         * on.
+         */
+        if (flip == 0)
+            vbl.request.type |= DRM_VBLANK_NEXTONMISS;
+
         /* If target_msc already reached or passed, set it to
          * current_msc to ensure we return a reasonable value back
          * to the caller. This makes swap_interval logic more robust.
@@ -683,7 +921,8 @@ ms_dri2_schedule_swap(ClientPtr client, DrawablePtr draw,
             goto blit_fallback;
         }
 
-        *target_msc = ms_kernel_msc_to_crtc_msc(crtc, vbl.reply.sequence);
+        *target_msc = ms_kernel_msc_to_crtc_msc(crtc,
+                                                vbl.reply.sequence + flip);
         frame_info->frame = *target_msc;
 
         return TRUE;
@@ -695,9 +934,10 @@ ms_dri2_schedule_swap(ClientPtr client, DrawablePtr draw,
      * equation.
      */
     vbl.request.type = (DRM_VBLANK_ABSOLUTE |
-                        DRM_VBLANK_NEXTONMISS |
                         DRM_VBLANK_EVENT |
                         drmmode_crtc->vblank_pipe);
+    if (flip == 0)
+        vbl.request.type |= DRM_VBLANK_NEXTONMISS;
 
     request_msc = current_msc - (current_msc % divisor) +
         remainder;
@@ -721,7 +961,8 @@ ms_dri2_schedule_swap(ClientPtr client, DrawablePtr draw,
     if (!seq)
         goto blit_fallback;
 
-    vbl.request.sequence = ms_crtc_msc_to_kernel_msc(crtc, request_msc);
+    /* Account for 1 frame extra pageflip delay if flip > 0 */
+    vbl.request.sequence = ms_crtc_msc_to_kernel_msc(crtc, request_msc) - flip;
     vbl.request.signal = (unsigned long)seq;
 
     ret = drmWaitVBlank(ms->fd, &vbl);
@@ -732,7 +973,8 @@ ms_dri2_schedule_swap(ClientPtr client, DrawablePtr draw,
         goto blit_fallback;
     }
 
-    *target_msc = ms_kernel_msc_to_crtc_msc(crtc, vbl.reply.sequence);
+    /* Adjust returned value for 1 fame pageflip offset of flip > 0 */
+    *target_msc = ms_kernel_msc_to_crtc_msc(crtc, vbl.reply.sequence + flip);
     frame_info->frame = *target_msc;
 
     return TRUE;
-- 
2.7.4

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH xserver 3/6] modesetting: add DRI2 page flip support

[Michel Dänzer]

On 17/08/16 07:29 PM, Qiang Yu wrote:
> 
> +static void
> +ms_dri2_flip_free(struct ms_crtc_pageflip *flip)
> +{
> +    struct ms_flipdata *flipdata = flip->flipdata;
> +
> +    free(flip);
> +    if (--flipdata->flip_count > 0)
> +        return;
> +    free(flipdata);
> +}
> +
> +static void
> +ms_dri2_flip_abort(void *data)
> +{
> +    struct ms_crtc_pageflip *flip = data;
> +    struct ms_flipdata *flipdata = flip->flipdata;
> +
> +    if (flipdata->flip_count == 1)
> +        free(flipdata->event);

This should be moved to ms_dri2_flip_free, or flipdata->event will be
leaked for successful flips?


> +static Bool
> +can_exchange(ScrnInfoPtr scrn, DrawablePtr draw,
> +	     DRI2BufferPtr front, DRI2BufferPtr back)
> +{

[...]

> +    if (!update_front(draw, front))
> +        return FALSE;

I know you just copied this from -ati/amdgpu, but: I don't think
can_exchange should call update_front, or the front buffer may be
updated even though the flip later fails.


> +    /* Post damage on the front buffer so that listeners, such
> +     * as DisplayLink know take a copy and shove it over the USB.
> +     * also for sw cursors.
> +     */

SW cursors cannot work correctly with page flipping. For that reason,
xf86-video-ati/amdgpu disable page flipping while there's an SW cursor.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH xserver 3/6] modesetting: add DRI2 page flip support

[Yu, Qiang]

> +static Bool
> +can_exchange(ScrnInfoPtr scrn, DrawablePtr draw,
> +          DRI2BufferPtr front, DRI2BufferPtr back)
> +{

[...]

> +    if (!update_front(draw, front))
> +        return FALSE;

I know you just copied this from -ati/amdgpu, but: I don't think
can_exchange should call update_front, or the front buffer may be
updated even though the flip later fails.
[yuq] so you mean I can just drop update_front here and use "front"
directly?


> +    /* Post damage on the front buffer so that listeners, such
> +     * as DisplayLink know take a copy and shove it over the USB.
> +     * also for sw cursors.
> +     */

SW cursors cannot work correctly with page flipping. For that reason,
xf86-video-ati/amdgpu disable page flipping while there's an SW cursor.
[yuq] why? Is the problem in kernel or xserver?

Regards,
Qiang 
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH xserver 3/6] modesetting: add DRI2 page flip support

[Michel Dänzer]

On 19/08/16 04:06 PM, Yu, Qiang wrote:
> 
>>> +static Bool
>>> +can_exchange(ScrnInfoPtr scrn, DrawablePtr draw,
>>> +          DRI2BufferPtr front, DRI2BufferPtr back)
>>> +{
>> 
>> [...]
>> 
>>> +    if (!update_front(draw, front))
>>> +        return FALSE;
>> 
>> I know you just copied this from -ati/amdgpu, but: I don't think
>> can_exchange should call update_front, or the front buffer may be
>> updated even though the flip later fails.
> [yuq] so you mean I can just drop update_front here and use "front"
> directly?

I mean update_front should only be called later, when we know for sure
there's no other reason preventing the flip. I haven't thought about
what if anything else would need to be done here instead.

Anyway, since this is just copying working code from other drivers, it
can be addressed (preferably in all affected drivers) later.


>>> +    /* Post damage on the front buffer so that listeners, such
>>> +     * as DisplayLink know take a copy and shove it over the USB.
>>> +     * also for sw cursors.
>>> +     */
>> 
>> SW cursors cannot work correctly with page flipping. For that reason,
>> xf86-video-ati/amdgpu disable page flipping while there's an SW cursor.
> [yuq] why? Is the problem in kernel or xserver?

The latter, due to the way SW cursor works. There's a fundamental (and
likely unfixable) conflict between the SW cursor and page flipping concepts.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH xserver 3/6] modesetting: add DRI2 page flip support

[Yu, Qiang]

>>> +    /* Post damage on the front buffer so that listeners, such
>>> +     * as DisplayLink know take a copy and shove it over the USB.
>>> +     * also for sw cursors.
>>> +     */
>>
>> SW cursors cannot work correctly with page flipping. For that reason,
>> xf86-video-ati/amdgpu disable page flipping while there's an SW cursor.
> [yuq] why? Is the problem in kernel or xserver?

The latter, due to the way SW cursor works. There's a fundamental (and
likely unfixable) conflict between the SW cursor and page flipping concepts.

[yuq] So the same problem for modesetting? I see no checking SW cursor
in original present page flip.

Regards,
Qiang
_______________________________________________
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH xserver 3/6] modesetting: add DRI2 page flip support

[Michel Dänzer]

On 19/08/16 04:48 PM, Yu, Qiang wrote:
> 
>>>>> +    /* Post damage on the front buffer so that listeners, such
>>>>> +     * as DisplayLink know take a copy and shove it over the USB.
>>>>> +     * also for sw cursors.
>>>>> +     */
>>>>
>>>> SW cursors cannot work correctly with page flipping. For that reason,
>>>> xf86-video-ati/amdgpu disable page flipping while there's an SW cursor.
>>> [yuq] why? Is the problem in kernel or xserver?
>> 
>> The latter, due to the way SW cursor works. There's a fundamental (and
>> likely unfixable) conflict between the SW cursor and page flipping concepts.
> 
> [yuq] So the same problem for modesetting? I see no checking SW cursor
> in original present page flip.

Right, it's currently broken with the modesetting driver.

Note that I'm not saying you need to address that as part of this
series, just that the comment above shouldn't mention SW cursors.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH xserver 4/6] modesetting: exclude DRI2 and Present page flip

[Qiang Yu]

Signed-off-by: Qiang Yu <Qiang.Yu@amd.com>
---
 hw/xfree86/drivers/modesetting/dri2.c            | 13 +++++++++++--
 hw/xfree86/drivers/modesetting/drmmode_display.h |  3 +++
 hw/xfree86/drivers/modesetting/present.c         | 15 +++++++++++++++
 3 files changed, 29 insertions(+), 2 deletions(-)

diff --git a/hw/xfree86/drivers/modesetting/dri2.c b/hw/xfree86/drivers/modesetting/dri2.c
index 11e0e6f..1abf98c 100644
--- a/hw/xfree86/drivers/modesetting/dri2.c
+++ b/hw/xfree86/drivers/modesetting/dri2.c
@@ -423,9 +423,14 @@ ms_dri2_flip_abort(void *data)
 {
     struct ms_crtc_pageflip *flip = data;
     struct ms_flipdata *flipdata = flip->flipdata;
+    ScreenPtr screen = flipdata->screen;
+    ScrnInfoPtr scrn = xf86ScreenToScrn(screen);
+    modesettingPtr ms = modesettingPTR(scrn);
 
-    if (flipdata->flip_count == 1)
-        free(flipdata->event);
+    if (flipdata->flip_count == 1) {
+            ms->drmmode.dri2_flipping = FALSE;
+            free(flipdata->event);
+    }
 
     ms_dri2_flip_free(flip);
 }
@@ -469,6 +474,8 @@ ms_dri2_schedule_flip(ms_dri2_frame_event_ptr info)
 {
     DrawablePtr draw = info->drawable;
     ScreenPtr screen = draw->pScreen;
+    ScrnInfoPtr scrn = xf86ScreenToScrn(screen);
+    modesettingPtr ms = modesettingPTR(scrn);
     ms_dri2_buffer_private_ptr back_priv = info->back->driverPrivate;
     struct ms_dri2_vblank_event *event;
     drmmode_crtc_private_ptr drmmode_crtc = info->crtc->driver_private;
@@ -486,6 +493,7 @@ ms_dri2_schedule_flip(ms_dri2_frame_event_ptr info)
                        drmmode_crtc->vblank_pipe, FALSE,
                        ms_dri2_flip_handler,
                        ms_dri2_flip_abort)) {
+        ms->drmmode.dri2_flipping = TRUE;
         return TRUE;
     }
     return FALSE;
@@ -571,6 +579,7 @@ can_flip(ScrnInfoPtr scrn, DrawablePtr draw,
 
     return draw->type == DRAWABLE_WINDOW &&
         ms->drmmode.pageflip &&
+        !ms->drmmode.present_flipping &&
         scrn->vtSema &&
         DRI2CanFlip(draw) && can_exchange(scrn, draw, front, back);
 }
diff --git a/hw/xfree86/drivers/modesetting/drmmode_display.h b/hw/xfree86/drivers/modesetting/drmmode_display.h
index 5499c16..f774250 100644
--- a/hw/xfree86/drivers/modesetting/drmmode_display.h
+++ b/hw/xfree86/drivers/modesetting/drmmode_display.h
@@ -80,6 +80,9 @@ typedef struct {
     Bool is_secondary;
 
     PixmapPtr fbcon_pixmap;
+
+    Bool dri2_flipping;
+    Bool present_flipping;
 } drmmode_rec, *drmmode_ptr;
 
 typedef struct {
diff --git a/hw/xfree86/drivers/modesetting/present.c b/hw/xfree86/drivers/modesetting/present.c
index f4318b4..d10c674 100644
--- a/hw/xfree86/drivers/modesetting/present.c
+++ b/hw/xfree86/drivers/modesetting/present.c
@@ -53,6 +53,7 @@
 
 struct ms_present_vblank_event {
     uint64_t        event_id;
+    Bool            unflip;
 };
 
 static RRCrtcPtr
@@ -221,6 +222,7 @@ ms_present_flip_handler(uint64_t msc, uint64_t ust, void *data)
     ScrnInfoPtr scrn = xf86ScreenToScrn(screen);
     modesettingPtr ms = modesettingPTR(scrn);
     struct ms_flipdata *flipdata = flip->flipdata;
+    struct ms_present_vblank_event *event = flipdata->event;
 
     DebugPresent(("\t\tms:fh %lld c %d msc %llu ust %llu\n",
                   (long long) flipdata->event->event_id,
@@ -238,6 +240,8 @@ ms_present_flip_handler(uint64_t msc, uint64_t ust, void *data)
                       flipdata->flip_count,
                       (long long) flipdata->fe_msc, (long long) flipdata->fe_usec));
 
+        if (event->unflip)
+            ms->drmmode.present_flipping = FALSE;
 
         ms_present_vblank_handler(flipdata->fe_msc,
                                   flipdata->fe_usec,
@@ -284,6 +288,9 @@ ms_present_check_flip(RRCrtcPtr crtc,
     if (!ms->drmmode.pageflip)
         return FALSE;
 
+    if (ms->drmmode.dri2_flipping)
+        return FALSE;
+
     if (!scrn->vtSema)
         return FALSE;
 
@@ -330,6 +337,7 @@ ms_present_flip(RRCrtcPtr crtc,
 {
     ScreenPtr screen = crtc->pScreen;
     ScrnInfoPtr scrn = xf86ScreenToScrn(screen);
+    modesettingPtr ms = modesettingPTR(scrn);
     xf86CrtcPtr xf86_crtc = crtc->devPrivate;
     drmmode_crtc_private_ptr drmmode_crtc = xf86_crtc->driver_private;
     Bool ret;
@@ -343,10 +351,14 @@ ms_present_flip(RRCrtcPtr crtc,
         return FALSE;
 
     event->event_id = event_id;
+    event->unflip = FALSE;
+
     ret = ms_do_pageflip(screen, pixmap, event, drmmode_crtc->vblank_pipe, !sync_flip,
                          ms_present_flip_handler, ms_present_flip_abort);
     if (!ret)
         xf86DrvMsg(scrn->scrnIndex, X_ERROR, "present flip failed\n");
+    else
+        ms->drmmode.present_flipping = TRUE;
 
     return ret;
 }
@@ -358,6 +370,7 @@ static void
 ms_present_unflip(ScreenPtr screen, uint64_t event_id)
 {
     ScrnInfoPtr scrn = xf86ScreenToScrn(screen);
+    modesettingPtr ms = modesettingPTR(scrn);
     PixmapPtr pixmap = screen->GetScreenPixmap(screen);
     xf86CrtcConfigPtr config = XF86_CRTC_CONFIG_PTR(scrn);
     int i;
@@ -368,6 +381,7 @@ ms_present_unflip(ScreenPtr screen, uint64_t event_id)
         return;
 
     event->event_id = event_id;
+    event->unflip = TRUE;
 
     if (ms_present_check_flip(NULL, screen->root, pixmap, TRUE) &&
         ms_do_pageflip(screen, pixmap, event, -1, FALSE,
@@ -399,6 +413,7 @@ ms_present_unflip(ScreenPtr screen, uint64_t event_id)
     }
 
     present_event_notify(event_id, 0, 0);
+    ms->drmmode.present_flipping = FALSE;
 }
 #endif
 
-- 
2.7.4

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH xserver 5/6] modesetting: merge common page flip code for present and dri2

[Qiang Yu]

Signed-off-by: Qiang Yu <Qiang.Yu@amd.com>
---
 hw/xfree86/drivers/modesetting/dri2.c     |  72 ++++++--------------
 hw/xfree86/drivers/modesetting/driver.h   |  33 +---------
 hw/xfree86/drivers/modesetting/pageflip.c | 105 ++++++++++++++++++++++++++++--
 hw/xfree86/drivers/modesetting/present.c  |  72 ++++----------------
 4 files changed, 134 insertions(+), 148 deletions(-)

diff --git a/hw/xfree86/drivers/modesetting/dri2.c b/hw/xfree86/drivers/modesetting/dri2.c
index 1abf98c..4cf638e 100644
--- a/hw/xfree86/drivers/modesetting/dri2.c
+++ b/hw/xfree86/drivers/modesetting/dri2.c
@@ -408,65 +408,31 @@ struct ms_dri2_vblank_event {
 };
 
 static void
-ms_dri2_flip_free(struct ms_crtc_pageflip *flip)
+ms_dri2_flip_abort(modesettingPtr ms, void *data)
 {
-    struct ms_flipdata *flipdata = flip->flipdata;
+    struct ms_present_vblank_event *event = data;
 
-    free(flip);
-    if (--flipdata->flip_count > 0)
-        return;
-    free(flipdata);
-}
-
-static void
-ms_dri2_flip_abort(void *data)
-{
-    struct ms_crtc_pageflip *flip = data;
-    struct ms_flipdata *flipdata = flip->flipdata;
-    ScreenPtr screen = flipdata->screen;
-    ScrnInfoPtr scrn = xf86ScreenToScrn(screen);
-    modesettingPtr ms = modesettingPTR(scrn);
-
-    if (flipdata->flip_count == 1) {
-            ms->drmmode.dri2_flipping = FALSE;
-            free(flipdata->event);
-    }
-
-    ms_dri2_flip_free(flip);
+    ms->drmmode.dri2_flipping = FALSE;
+    free(event);
 }
 
 static void
-ms_dri2_flip_handler(uint64_t msc, uint64_t ust, void *data)
+ms_dri2_flip_handler(modesettingPtr ms, uint64_t msc, 
+                     uint64_t ust, void *data)
 {
-    struct ms_crtc_pageflip *flip = data;
-    ScreenPtr screen = flip->flipdata->screen;
-    ScrnInfoPtr scrn = xf86ScreenToScrn(screen);
-    modesettingPtr ms = modesettingPTR(scrn);
-    struct ms_flipdata *flipdata = flip->flipdata;
-    struct ms_dri2_vblank_event *event = flipdata->event;
-
-    if (flip->on_reference_crtc) {
-        flipdata->fe_msc = msc;
-        flipdata->fe_usec = ust;
-    }
-
-    if (flipdata->flip_count == 1) {
-        uint32_t frame = flipdata->fe_msc;
-        uint32_t tv_sec = flipdata->fe_usec / 1000000;
-        uint32_t tv_usec = flipdata->fe_usec % 1000000;
-        DrawablePtr drawable;
-        int status;
-
-        status = dixLookupDrawable(&drawable, event->drawable_id, serverClient,
-                                   M_ANY, DixWriteAccess);
-        if (status == Success)
-            DRI2SwapComplete(event->client, drawable, frame, tv_sec, tv_usec,
-                             DRI2_FLIP_COMPLETE, event->event_complete,
-                             event->event_data);
-        drmModeRmFB(ms->fd, flipdata->old_fb_id);
-    }
-
-    ms_dri2_flip_free(flip);
+    struct ms_dri2_vblank_event *event = data;
+    uint32_t frame = msc;
+    uint32_t tv_sec = ust / 1000000;
+    uint32_t tv_usec = ust % 1000000;
+    DrawablePtr drawable;
+    int status;
+
+    status = dixLookupDrawable(&drawable, event->drawable_id, serverClient,
+                               M_ANY, DixWriteAccess);
+    if (status == Success)
+        DRI2SwapComplete(event->client, drawable, frame, tv_sec, tv_usec,
+                         DRI2_FLIP_COMPLETE, event->event_complete,
+                         event->event_data);
 }
 
 static Bool
diff --git a/hw/xfree86/drivers/modesetting/driver.h b/hw/xfree86/drivers/modesetting/driver.h
index c0d80a8..761490a 100644
--- a/hw/xfree86/drivers/modesetting/driver.h
+++ b/hw/xfree86/drivers/modesetting/driver.h
@@ -156,39 +156,12 @@ Bool ms_present_screen_init(ScreenPtr screen);
 
 #ifdef GLAMOR
 
-/*
- * Event data for an in progress flip.
- * This contains a pointer to the vblank event,
- * and information about the flip in progress.
- * a reference to this is stored in the per-crtc
- * flips.
- */
-struct ms_flipdata {
-    ScreenPtr screen;
-    void *event;
-    /* number of CRTC events referencing this */
-    int flip_count;
-    uint64_t fe_msc;
-    uint64_t fe_usec;
-    uint32_t old_fb_id;
-};
-
-/*
- * Per crtc pageflipping infomation,
- * These are submitted to the queuing code
- * one of them per crtc per flip.
- */
-struct ms_crtc_pageflip {
-    Bool on_reference_crtc;
-    /* reference to the ms_flipdata */
-    struct ms_flipdata *flipdata;
-};
-
-typedef void (*ms_pageflip_handler_proc)(uint64_t frame,
+typedef void (*ms_pageflip_handler_proc)(modesettingPtr ms,
+                                         uint64_t frame,
                                          uint64_t usec,
                                          void *data);
 
-typedef void (*ms_pageflip_abort_proc)(void *data);
+typedef void (*ms_pageflip_abort_proc)(modesettingPtr ms, void *data);
 
 int ms_flush_drm_events(ScreenPtr screen);
 
diff --git a/hw/xfree86/drivers/modesetting/pageflip.c b/hw/xfree86/drivers/modesetting/pageflip.c
index 4549792..a82e0d6 100644
--- a/hw/xfree86/drivers/modesetting/pageflip.c
+++ b/hw/xfree86/drivers/modesetting/pageflip.c
@@ -32,6 +32,100 @@
 #ifdef GLAMOR
 
 /*
+ * Event data for an in progress flip.
+ * This contains a pointer to the vblank event,
+ * and information about the flip in progress.
+ * a reference to this is stored in the per-crtc
+ * flips.
+ */
+struct ms_flipdata {
+    ScreenPtr screen;
+    void *event;
+    ms_pageflip_handler_proc event_handler;
+    ms_pageflip_abort_proc abort_handler;
+    /* number of CRTC events referencing this */
+    int flip_count;
+    uint64_t fe_msc;
+    uint64_t fe_usec;
+    uint32_t old_fb_id;
+};
+
+/*
+ * Per crtc pageflipping infomation,
+ * These are submitted to the queuing code
+ * one of them per crtc per flip.
+ */
+struct ms_crtc_pageflip {
+    Bool on_reference_crtc;
+    /* reference to the ms_flipdata */
+    struct ms_flipdata *flipdata;
+};
+
+/**
+ * Free an ms_crtc_pageflip.
+ *
+ * Drops the reference count on the flipdata.
+ */
+static void
+ms_pageflip_free(struct ms_crtc_pageflip *flip)
+{
+    struct ms_flipdata *flipdata = flip->flipdata;
+
+    free(flip);
+    if (--flipdata->flip_count > 0)
+        return;
+    free(flipdata);
+}
+
+/**
+ * Callback for the DRM event queue when a single flip has completed
+ *
+ * Once the flip has been completed on all pipes, notify the
+ * extension code telling it when that happened
+ */
+static void
+ms_pageflip_handler(uint64_t msc, uint64_t ust, void *data)
+{
+    struct ms_crtc_pageflip *flip = data;
+    struct ms_flipdata *flipdata = flip->flipdata;
+    ScreenPtr screen = flipdata->screen;
+    ScrnInfoPtr scrn = xf86ScreenToScrn(screen);
+    modesettingPtr ms = modesettingPTR(scrn);
+
+    if (flip->on_reference_crtc) {
+        flipdata->fe_msc = msc;
+        flipdata->fe_usec = ust;
+    }
+
+    if (flipdata->flip_count == 1) {
+        flipdata->event_handler(ms, flipdata->fe_msc,
+                                flipdata->fe_usec,
+                                flipdata->event);
+
+        drmModeRmFB(ms->fd, flipdata->old_fb_id);
+    }
+    ms_pageflip_free(flip);
+}
+
+/*
+ * Callback for the DRM queue abort code.  A flip has been aborted.
+ */
+static void
+ms_pageflip_abort(void *data)
+{
+    struct ms_crtc_pageflip *flip = data;
+    struct ms_flipdata *flipdata = flip->flipdata;
+    ScreenPtr screen = flipdata->screen;
+    ScrnInfoPtr scrn = xf86ScreenToScrn(screen);
+    modesettingPtr ms = modesettingPTR(scrn);
+
+    if (flipdata->flip_count == 1)
+        flipdata->abort_handler(ms, flipdata->event);
+
+    ms_pageflip_free(flip);
+}
+
+/*
  * Flush the DRM event queue when full; makes space for new events.
  *
  * Returns a negative value on error, 0 if there was nothing to process,
@@ -68,9 +162,7 @@ ms_flush_drm_events(ScreenPtr screen)
 static Bool
 queue_flip_on_crtc(ScreenPtr screen, xf86CrtcPtr crtc,
                    struct ms_flipdata *flipdata,
-                   int ref_crtc_vblank_pipe, uint32_t flags,
-                   ms_pageflip_handler_proc pageflip_handler,
-                   ms_pageflip_abort_proc pageflip_abort)
+                   int ref_crtc_vblank_pipe, uint32_t flags)
 {
     ScrnInfoPtr scrn = xf86ScreenToScrn(screen);
     modesettingPtr ms = modesettingPTR(scrn);
@@ -92,7 +184,7 @@ queue_flip_on_crtc(ScreenPtr screen, xf86CrtcPtr crtc,
     flip->on_reference_crtc = (drmmode_crtc->vblank_pipe == ref_crtc_vblank_pipe);
     flip->flipdata = flipdata;
 
-    seq = ms_drm_queue_alloc(crtc, flip, pageflip_handler, pageflip_abort);
+    seq = ms_drm_queue_alloc(crtc, flip, ms_pageflip_handler, ms_pageflip_abort);
     if (!seq) {
         free(flip);
         return FALSE;
@@ -164,6 +256,8 @@ ms_do_pageflip(ScreenPtr screen,
 
     flipdata->event = event;
     flipdata->screen = screen;
+    flipdata->event_handler = pageflip_handler;
+    flipdata->abort_handler = pageflip_abort;
 
     /*
      * Take a local reference on flipdata.
@@ -206,8 +300,7 @@ ms_do_pageflip(ScreenPtr screen,
 
         if (!queue_flip_on_crtc(screen, crtc, flipdata,
                                 ref_crtc_vblank_pipe,
-                                flags, pageflip_handler,
-                                pageflip_abort)) {
+                                flags)) {
             goto error_undo;
         }
     }
diff --git a/hw/xfree86/drivers/modesetting/present.c b/hw/xfree86/drivers/modesetting/present.c
index d10c674..1101808 100644
--- a/hw/xfree86/drivers/modesetting/present.c
+++ b/hw/xfree86/drivers/modesetting/present.c
@@ -193,80 +193,34 @@ ms_present_flush(WindowPtr window)
 #ifdef GLAMOR
 
 /**
- * Free an ms_crtc_pageflip.
- *
- * Drops the reference count on the flipdata.
- */
-static void
-ms_present_flip_free(struct ms_crtc_pageflip *flip)
-{
-    struct ms_flipdata *flipdata = flip->flipdata;
-
-    free(flip);
-    if (--flipdata->flip_count > 0)
-        return;
-    free(flipdata);
-}
-
-/**
- * Callback for the DRM event queue when a single flip has completed
- *
- * Once the flip has been completed on all pipes, notify the
+ * Callback for the flip has been completed on all pipes, notify the
  * extension code telling it when that happened
  */
 static void
-ms_present_flip_handler(uint64_t msc, uint64_t ust, void *data)
+ms_present_flip_handler(modesettingPtr ms, uint64_t msc, 
+                        uint64_t ust, void *data)
 {
-    struct ms_crtc_pageflip *flip = data;
-    ScreenPtr screen = flip->flipdata->screen;
-    ScrnInfoPtr scrn = xf86ScreenToScrn(screen);
-    modesettingPtr ms = modesettingPTR(scrn);
-    struct ms_flipdata *flipdata = flip->flipdata;
-    struct ms_present_vblank_event *event = flipdata->event;
+    struct ms_present_vblank_event *event = data;
 
-    DebugPresent(("\t\tms:fh %lld c %d msc %llu ust %llu\n",
-                  (long long) flipdata->event->event_id,
-                  flipdata->flip_count,
+    DebugPresent(("\t\tms:fc %lld msc %llu ust %llu\n",
+                  (long long) event->event_id,
                   (long long) msc, (long long) ust));
 
-    if (flip->on_reference_crtc) {
-        flipdata->fe_msc = msc;
-        flipdata->fe_usec = ust;
-    }
-
-    if (flipdata->flip_count == 1) {
-        DebugPresent(("\t\tms:fc %lld c %d msc %llu ust %llu\n",
-                      (long long) flipdata->event->event_id,
-                      flipdata->flip_count,
-                      (long long) flipdata->fe_msc, (long long) flipdata->fe_usec));
-
-        if (event->unflip)
-            ms->drmmode.present_flipping = FALSE;
-
-        ms_present_vblank_handler(flipdata->fe_msc,
-                                  flipdata->fe_usec,
-                                  flipdata->event);
+    if (event->unflip)
+        ms->drmmode.present_flipping = FALSE;
 
-        drmModeRmFB(ms->fd, flipdata->old_fb_id);
-    }
-    ms_present_flip_free(flip);
+    ms_present_vblank_handler(msc, ust, event);
 }
 
 /*
- * Callback for the DRM queue abort code.  A flip has been aborted.
+ * Callback for the flip has been aborted.
  */
 static void
-ms_present_flip_abort(void *data)
+ms_present_flip_abort(modesettingPtr ms, void *data)
 {
-    struct ms_crtc_pageflip *flip = data;
-    struct ms_flipdata *flipdata = flip->flipdata;
-
-    DebugPresent(("\t\tms:fa %lld c %d\n", (long long) flipdata->event->event_id, flipdata->flip_count));
-
-    if (flipdata->flip_count == 1)
-        free(flipdata->event);
+    struct ms_present_vblank_event *event = data;
 
-    ms_present_flip_free(flip);
+    free(event);
 }
 
 /*
-- 
2.7.4

_______________________________________________
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

[PATCH xserver 6/6] modesetting: remove redundent pixmap destroy

[Qiang Yu]

Signed-off-by: Qiang Yu <Qiang.Yu@amd.com>
---
 hw/xfree86/drivers/modesetting/dri2.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/hw/xfree86/drivers/modesetting/dri2.c b/hw/xfree86/drivers/modesetting/dri2.c
index 4cf638e..b18f0b8 100644
--- a/hw/xfree86/drivers/modesetting/dri2.c
+++ b/hw/xfree86/drivers/modesetting/dri2.c
@@ -193,8 +193,6 @@ ms_dri2_create_buffer(DrawablePtr drawable, unsigned int attachment,
                                       pixmap_cpp,
                                       0);
         if (pixmap == NULL) {
-            if (pixmap)
-                screen->DestroyPixmap(pixmap);
             free(private);
             free(buffer);
             return NULL;
-- 
2.7.4

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH xserver 0/6] modesetting: add DRI2 page flip support

[Martin Peres]

On 17/08/16 13:29, Qiang Yu wrote:
> Hi guys,
>
> This patch set is for adding DRI2 page flip support to modesetting
> driver. I mainly take reference of amdgpu DDX and reuse present
> page flip code in the modesetting driver.
>
> Regards,
> Qiang
>
> Qiang Yu (6):
>   modesetting: make ms_do_pageflip generic for share with DRI2
>   modesetting: move ms_do_pageflip to pageflip.c
>   modesetting: add DRI2 page flip support
>   modesetting: exclude DRI2 and Present page flip
>   modesetting: merge common page flip code for present and dri2
>   modesetting: remove redundent pixmap destroy
>
>  hw/xfree86/drivers/modesetting/Makefile.am       |   1 +
>  hw/xfree86/drivers/modesetting/dri2.c            | 237 +++++++++++++++-
>  hw/xfree86/drivers/modesetting/driver.h          |  21 ++
>  hw/xfree86/drivers/modesetting/drmmode_display.h |   3 +
>  hw/xfree86/drivers/modesetting/pageflip.c        | 344 +++++++++++++++++++++++
>  hw/xfree86/drivers/modesetting/present.c         | 333 ++--------------------
>  6 files changed, 624 insertions(+), 315 deletions(-)
>  create mode 100644 hw/xfree86/drivers/modesetting/pageflip.c
>

Thanks for doing this! I will have a look at it whenever I can, but this 
is great!
_______________________________________________
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH xserver 0/6] modesetting: add DRI2 page flip support

[Michel Dänzer]

On 17/08/16 07:29 PM, Qiang Yu wrote:
> Hi guys,
> 
> This patch set is for adding DRI2 page flip support to modesetting
> driver. I mainly take reference of amdgpu DDX and reuse present
> page flip code in the modesetting driver.
> 
> Regards,
> Qiang
> 
> Qiang Yu (6):
>   modesetting: make ms_do_pageflip generic for share with DRI2
>   modesetting: move ms_do_pageflip to pageflip.c
>   modesetting: add DRI2 page flip support
>   modesetting: exclude DRI2 and Present page flip
>   modesetting: merge common page flip code for present and dri2
>   modesetting: remove redundent pixmap destroy

Patches 2, 4 & 5 are

Reviewed-by: Michel Dänzer <michel.daenzer@amd.com>


Patch 6 was already submitted by Hans de Goede:

https://patchwork.freedesktop.org/patch/105351/


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH xserver 0/6] modesetting: add DRI2 page flip support

[Yu, Qiang]

Thanks Michel. I'll update patches in V2 according to your comments.

Regards,
Qiang
________________________________________
From: Michel Dänzer <michel@daenzer.net>
Sent: Thursday, August 18, 2016 4:18:11 PM
To: Yu, Qiang
Cc: xorg-devel@lists.x.org; amd-gfx@lists.freedesktop.org
Subject: Re: [PATCH xserver 0/6] modesetting: add DRI2 page flip support

On 17/08/16 07:29 PM, Qiang Yu wrote:
> Hi guys,
>
> This patch set is for adding DRI2 page flip support to modesetting
> driver. I mainly take reference of amdgpu DDX and reuse present
> page flip code in the modesetting driver.
>
> Regards,
> Qiang
>
> Qiang Yu (6):
>   modesetting: make ms_do_pageflip generic for share with DRI2
>   modesetting: move ms_do_pageflip to pageflip.c
>   modesetting: add DRI2 page flip support
>   modesetting: exclude DRI2 and Present page flip
>   modesetting: merge common page flip code for present and dri2
>   modesetting: remove redundent pixmap destroy

Patches 2, 4 & 5 are

Reviewed-by: Michel Dänzer <michel.daenzer@amd.com>


Patch 6 was already submitted by Hans de Goede:

https://patchwork.freedesktop.org/patch/105351/


--
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH xserver 0/6] modesetting: add DRI2 page flip support

[Emil Velikov]

Hi Qiang,

On 17 August 2016 at 11:29, Qiang Yu <Qiang.Yu@amd.com> wrote:
> Hi guys,
>
> This patch set is for adding DRI2 page flip support to modesetting
> driver. I mainly take reference of amdgpu DDX and reuse present
> page flip code in the modesetting driver.
>
> Regards,
> Qiang
>
> Qiang Yu (6):
>   modesetting: make ms_do_pageflip generic for share with DRI2
>   modesetting: move ms_do_pageflip to pageflip.c
>   modesetting: add DRI2 page flip support
>   modesetting: exclude DRI2 and Present page flip
>   modesetting: merge common page flip code for present and dri2
>   modesetting: remove redundent pixmap destroy
>
The work itself is quite interesting thanks for that !

Is it me or the newly added code (with patch 3) seems intermittently
broken (fixed with patch 4), and then removed with a later patch (5) ?
Should one factor the common pageflip code first, then add the
distinction between dri2 and present flips, then finally add the dri2
implementation ?

Regards,
Emil
_______________________________________________
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH xserver 0/6] modesetting: add DRI2 page flip support

[Yu, Qiang]

Hi Emil,

Each point of the patch set is not broken. Patches are arranged like
this to show how I do it:
1. create a pageflip.c to host common page flip code
2. copy amdgpu DDX DRI2 page flip code to modesetting dri2.c
3. merge common DRI2 and present page flip code to pageflip.c

Patch 4 is not a fix for broken code, just a commit from amdgpu DDX
to prevent present and DRI2 from flipping at the same time.

Regards,
Qiang
________________________________________
From: Emil Velikov <emil.l.velikov@gmail.com>
Sent: Thursday, August 18, 2016 7:03:12 PM
To: Yu, Qiang
Cc: ML xorg-devel; amd-gfx@lists.freedesktop.org
Subject: Re: [PATCH xserver 0/6] modesetting: add DRI2 page flip support

Hi Qiang,

On 17 August 2016 at 11:29, Qiang Yu <Qiang.Yu@amd.com> wrote:
> Hi guys,
>
> This patch set is for adding DRI2 page flip support to modesetting
> driver. I mainly take reference of amdgpu DDX and reuse present
> page flip code in the modesetting driver.
>
> Regards,
> Qiang
>
> Qiang Yu (6):
>   modesetting: make ms_do_pageflip generic for share with DRI2
>   modesetting: move ms_do_pageflip to pageflip.c
>   modesetting: add DRI2 page flip support
>   modesetting: exclude DRI2 and Present page flip
>   modesetting: merge common page flip code for present and dri2
>   modesetting: remove redundent pixmap destroy
>
The work itself is quite interesting thanks for that !

Is it me or the newly added code (with patch 3) seems intermittently
broken (fixed with patch 4), and then removed with a later patch (5) ?
Should one factor the common pageflip code first, then add the
distinction between dri2 and present flips, then finally add the dri2
implementation ?

Regards,
Emil
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH xserver 0/6] modesetting: add DRI2 page flip support

[Michel Dänzer]

On 19/08/16 11:02 AM, Yu, Qiang wrote:
> 
> Each point of the patch set is not broken. Patches are arranged like
> this to show how I do it:
> 1. create a pageflip.c to host common page flip code
> 2. copy amdgpu DDX DRI2 page flip code to modesetting dri2.c
> 3. merge common DRI2 and present page flip code to pageflip.c
> 
> Patch 4 is not a fix for broken code, just a commit from amdgpu DDX
> to prevent present and DRI2 from flipping at the same time.

This does mean though that if one has only up to patch 3 applied (e.g.
during a bisection), one is exposed to the issues fixed by patch 4. So
maybe patch 4 should be squashed into patch 3.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH xserver 0/6] modesetting: add DRI2 page flip support

[Keith Packard]

[-- Attachment #1.1: Type: text/plain, Size: 1963 bytes --]

Michel Dänzer <michel-otUistvHUpPR7s880joybQ@public.gmane.org> writes:

> This does mean though that if one has only up to patch 3 applied (e.g.
> during a bisection), one is exposed to the issues fixed by patch 4. So
> maybe patch 4 should be squashed into patch 3.

That's a very important point -- developing code in small logical steps
is a most excellent plan, but the resulting set of commits in git need
to satisfy different requirements:

 1) Make forward progress; no commit should regress any functionality,
    no commit should introduce compiler warnings (or, even worse,
    compiler errors). Adam Jackson has reminded me several times to use
    'git rebase -x make' to check a long sequence of patches with the
    compiler. Sometimes I remember on my own.

 2) Each patch should be a single change, self contained and easy to
    understand. If you can't summarize the patch in one line, it's
    probably too complicated for anyone to review effectively. If you
    ever use the word 'and' in the summary line, that's a good sign that
    the patch does more than one thing and should be split apart.

 3) The series should tell a good story. Just like writing a book, the
    final product (sequence of patches or storyline) almost certainly
    will not be presented in the original development
    order. Splitting/merging patch chunks and reordering commits is
    almost always necessary in a long patch series.

I think review takes time related to some high order polynomial function
of the patch length; maybe just quadratic, but possibly more. Long
patches that are purely mechanical changes (replacing function names,
cleaning up whitespace) might be an exception to this rule. Remember
that development is a shared activity, and that spending time making the
patches easy to review moves work from reviewer to committer, and that
is almost always a net win in total development time.

-- 
-keith

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 810 bytes --]

[-- Attachment #2: Type: text/plain, Size: 192 bytes --]

_______________________________________________
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH xserver 0/6] modesetting: add DRI2 page flip support

[Emil Velikov]

On 19 August 2016 at 03:09, Michel Dänzer <michel@daenzer.net> wrote:
> On 19/08/16 11:02 AM, Yu, Qiang wrote:
>>
>> Each point of the patch set is not broken. Patches are arranged like
>> this to show how I do it:
>> 1. create a pageflip.c to host common page flip code
>> 2. copy amdgpu DDX DRI2 page flip code to modesetting dri2.c
>> 3. merge common DRI2 and present page flip code to pageflip.c
>>
>> Patch 4 is not a fix for broken code, just a commit from amdgpu DDX
>> to prevent present and DRI2 from flipping at the same time.
>
> This does mean though that if one has only up to patch 3 applied (e.g.
> during a bisection), one is exposed to the issues fixed by patch 4. So
> maybe patch 4 should be squashed into patch 3.
>
Precisely what I meant with "broken" - new code introduces a
functionality which conflicts with existing one.

And yes, one can see the code flow (and that it's partially copied
from amdgpu/ati) As-is the series brings unnecessary churn, since it
adds new code only to (re)move it.
Something many developers/projects consider a bad way to split patches.

Thanks
Emil
_______________________________________________
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH xserver 0/6] modesetting: add DRI2 page flip support

[Michel Dänzer]

On 19/08/16 04:57 PM, Emil Velikov wrote:
> On 19 August 2016 at 03:09, Michel Dänzer <michel@daenzer.net> wrote:
>> On 19/08/16 11:02 AM, Yu, Qiang wrote:
>>>
>>> Each point of the patch set is not broken. Patches are arranged like
>>> this to show how I do it:
>>> 1. create a pageflip.c to host common page flip code
>>> 2. copy amdgpu DDX DRI2 page flip code to modesetting dri2.c
>>> 3. merge common DRI2 and present page flip code to pageflip.c
>>>
>>> Patch 4 is not a fix for broken code, just a commit from amdgpu DDX
>>> to prevent present and DRI2 from flipping at the same time.
>>
>> This does mean though that if one has only up to patch 3 applied (e.g.
>> during a bisection), one is exposed to the issues fixed by patch 4. So
>> maybe patch 4 should be squashed into patch 3.
>>
> Precisely what I meant with "broken" - new code introduces a
> functionality which conflicts with existing one.
> 
> And yes, one can see the code flow (and that it's partially copied
> from amdgpu/ati) As-is the series brings unnecessary churn, since it
> adds new code only to (re)move it.
> Something many developers/projects consider a bad way to split patches.

It makes some sense to add the code copied from another driver as
verbatim as possible first. That makes it easier to compare it with the
other driver.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx