Sorry, disregard. The pointer doesn't point inside the struct. That part of the patch is fine.
Tom ________________________________ From: amd-gfx <amd-gfx-boun...@lists.freedesktop.org> on behalf of StDenis, Tom <tom.stde...@amd.com> Sent: Friday, August 19, 2016 07:30 To: Yu, Qiang; xorg-devel@lists.x.org Cc: mic...@daenzer.net; emil.l.veli...@gmail.com; amd-...@lists.freedesktop.org Subject: Re: [PATCH v2 xserver 3/5] modesetting: move common page flip handle to pageflip.c 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-boun...@lists.freedesktop.org> on behalf of Qiang Yu <qiang...@amd.com> Sent: Friday, August 19, 2016 08:50 To: xorg-devel@lists.x.org Cc: Yu, Qiang; mic...@daenzer.net; emil.l.veli...@gmail.com; amd-...@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...@amd.com> --- 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-...@lists.freedesktop.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 ...
_______________________________________________ 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