Re: [PATCH v2 xserver 3/5] modesetting: move common page flip handle to pageflip.c

* Re: [PATCH v2 xserver 3/5] modesetting: move common page flip handle to pageflip.c
       [not found]     ` <1471611050-27207-4-git-send-email-Qiang.Yu-5C7GfCeVMHo@public.gmane.org>
@ 2016-08-19 11:30       ` StDenis, Tom
       [not found]         ` <DM5PR12MB1132187DDE5132521F18628FF7160-2J9CzHegvk+UzrhdoDeimQdYzm3356FpvxpqHgZTriW3zl9H0oFU5g@public.gmane.org>
  0 siblings, 1 reply; 9+ messages in thread
From: StDenis, Tom @ 2016-08-19 11:30 UTC (permalink / raw)
  To: Yu, Qiang, xorg-devel-go0+a7rfsptAfugRpC6u6w
  Cc: michel-otUistvHUpPR7s880joybQ,
	emil.l.velikov-Re5JQEeQqe8AvxtiuMwx3w,
	amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW


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

In ms_pageflip_free() you cannot free the parent structure before freeing things it points to.  That's undefined behaviour.

Tom

________________________________
From: amd-gfx <amd-gfx-bounces-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org> on behalf of Qiang Yu <Qiang.Yu-5C7GfCeVMHo@public.gmane.org>
Sent: Friday, August 19, 2016 08:50
To: xorg-devel-go0+a7rfsptAfugRpC6u6w@public.gmane.org
Cc: Yu, Qiang; michel-otUistvHUpPR7s880joybQ@public.gmane.org; emil.l.velikov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org; amd-gfx@lists.freedesktop.org
Subject: [PATCH v2 xserver 3/5] modesetting: move common page flip handle to pageflip.c

The common page flip handle framework can be shared with DRI2
page flip.

Signed-off-by: Qiang Yu <Qiang.Yu-5C7GfCeVMHo@public.gmane.org>
---
 hw/xfree86/drivers/modesetting/driver.h   |  28 --------
 hw/xfree86/drivers/modesetting/pageflip.c | 102 ++++++++++++++++++++++++++++--
 hw/xfree86/drivers/modesetting/present.c  |  63 +++---------------
 3 files changed, 104 insertions(+), 89 deletions(-)

diff --git a/hw/xfree86/drivers/modesetting/driver.h b/hw/xfree86/drivers/modesetting/driver.h
index c0d80a8..38f0ec0 100644
--- a/hw/xfree86/drivers/modesetting/driver.h
+++ b/hw/xfree86/drivers/modesetting/driver.h
@@ -156,34 +156,6 @@ 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);
diff --git a/hw/xfree86/drivers/modesetting/pageflip.c b/hw/xfree86/drivers/modesetting/pageflip.c
index 4549792..5f35999 100644
--- a/hw/xfree86/drivers/modesetting/pageflip.c
+++ b/hw/xfree86/drivers/modesetting/pageflip.c
@@ -32,6 +32,97 @@
 #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(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;
+
+    if (flipdata->flip_count == 1)
+        flipdata->abort_handler(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 +159,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 +181,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 +253,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 +297,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 18c82cd..deee1f1 100644
--- a/hw/xfree86/drivers/modesetting/present.c
+++ b/hw/xfree86/drivers/modesetting/present.c
@@ -192,77 +192,30 @@ 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)
 {
-    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 = 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));
-
-
-        ms_present_vblank_handler(flipdata->fe_msc,
-                                  flipdata->fe_usec,
-                                  flipdata->event);
-
-        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)
 {
-    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

_______________________________________________
amd-gfx mailing list
amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
amd-gfx Info Page - lists.freedesktop.org<https://lists.freedesktop.org/mailman/listinfo/amd-gfx>
lists.freedesktop.org
To see the collection of prior postings to the list, visit the amd-gfx Archives. Using amd-gfx: To post a message to all the list members, send email ...




[-- Attachment #1.2: Type: text/html, Size: 19729 bytes --]

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

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

^ permalink raw reply related	[flat|nested] 9+ messages in thread