<html> <head> <meta http-equiv="Content-Type" content="text/html; charset=iso-8859-1"> <style type="text/css" style="display:none;"></style> </head> <body dir="ltr"> <div id="divtagdefaultwrapper" style="font-size:12pt;color:#000000;background-color:#FFFFFF;font-family:Calibri,Arial,Helvetica,sans-serif;"> Sorry, disregard. The pointer doesn't point inside the struct. That part of the patch is fine. Tom <div style="color: rgb(0, 0, 0);"> <hr tabindex="-1" style="display:inline-block; width:98%"> <div id="divRplyFwdMsg" dir="ltr">From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> on behalf of StDenis, Tom <Tom.StDenis@amd.com> Sent: Friday, August 19, 2016 07:30 To: Yu, Qiang; xorg-devel@lists.x.org Cc: michel@daenzer.net; emil.l.velikov@gmail.com; amd-gfx@lists.freedesktop.org Subject: Re: [PATCH v2 xserver 3/5] modesetting: move common page flip handle to pageflip.c <div> </div> </div> <div> <div id="divtagdefaultwrapper" style="font-size:12pt; color:#000000; background-color:#FFFFFF; font-family:Calibri,Arial,Helvetica,sans-serif"> <div>In ms_pageflip_free() you cannot free the parent structure before freeing things it points to. That's undefined behaviour. </div> <div> </div> <div>Tom</div> <div style="color:rgb(0,0,0)"> <div> <hr tabindex="-1" style="display:inline-block; width:98%"> <div id="x_divRplyFwdMsg" dir="ltr">From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> on behalf of Qiang Yu <Qiang.Yu@amd.com> Sent: Friday, August 19, 2016 08:50 To: xorg-devel@lists.x.org Cc: Yu, Qiang; michel@daenzer.net; emil.l.velikov@gmail.com; amd-gfx@lists.freedesktop.org Subject: [PATCH v2 xserver 3/5] modesetting: move common page flip handle to pageflip.c <div> </div> </div> </div> <div class="PlainText">The common page flip handle framework can be shared with DRI2 page flip. Signed-off-by: Qiang Yu <Qiang.Yu@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-gfx@lists.freedesktop.org <a href="https://lists.freedesktop.org/mailman/listinfo/amd-gfx" id="LPlnk437166">https://lists.freedesktop.org/mailman/listinfo/amd-gfx</a> <div id="LPBorder_GT_14716061893940.8420357393682676" style="margin-bottom:20px; overflow:auto; width:100%; text-indent:0px"> <table id="LPContainer_14716061893930.7108288203263247" cellspacing="0" style="width:90%; overflow:auto; padding-top:20px; padding-bottom:20px; margin-top:20px; border-top:1px dotted rgb(200,200,200); border-bottom:1px dotted rgb(200,200,200); background-color:rgb(255,255,255)"> <tbody> <tr valign="top" style="border-spacing:0px"> <td id="TextCell_14716061893930.9899862651724711" colspan="2" style="vertical-align: top; padding: 0px; display: table-cell; position: relative;"> <div id="LPRemovePreviewContainer_14716061893930.47806653578636316"></div> <div id="LPTitle_14716061893930.16493228848820385" style="top:0px; color:rgb(59,87,119); font-weight:normal; font-size:21px; font-family:wf_segoe-ui_light,"Segoe UI Light","Segoe WP Light","Segoe UI","Segoe WP",Tahoma,Arial,sans-serif; line-height:21px"> <a id="LPUrlAnchor_14716061893930.6212120889357053" href="https://lists.freedesktop.org/mailman/listinfo/amd-gfx" target="_blank" style="text-decoration:none">amd-gfx Info Page - lists.freedesktop.org</a></div> <div id="LPMetadata_14716061893930.2849713450368425" style="margin:10px 0px 16px; color:rgb(102,102,102); font-weight:normal; font-family:wf_segoe-ui_normal,"Segoe UI","Segoe WP",Tahoma,Arial,sans-serif; font-size:14px; line-height:14px"> lists.freedesktop.org</div> <div id="LPDescription_14716061893940.8733822341303554" style="display:block; color:rgb(102,102,102); font-weight:normal; font-family:wf_segoe-ui_normal,"Segoe UI","Segoe WP",Tahoma,Arial,sans-serif; font-size:14px; line-height:20px; max-height:100px; overflow:hidden"> 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 ...</div> </td> </tr> </tbody> </table> </div> </div> </div> </div> </div> </div> </div> </body> </html>