[PATCH xf86-video-amdgpu] Identify DRM event queue entries by sequence number instead of by pointer
Alex Deucher
alexdeucher at gmail.com
Fri Apr 1 13:56:11 UTC 2016
On Fri, Apr 1, 2016 at 2:54 AM, Michel Dänzer <michel at daenzer.net> wrote:
> From: Michel Dänzer <michel.daenzer at amd.com>
>
> If the memory for an entry was allocated at the same address as that for
> a previously cancelled entry, the handler could theoretically be called
> prematurely, triggered by the DRM event which was submitted for the
> cancelled entry.
>
> (Ported from radeon commit 4693b1bd5b5c381e8b7b68a6f7f0c6696d6a68df)
>
> Signed-off-by: Michel Dänzer <michel.daenzer at amd.com>
Reviewed-by: Alex Deucher <alexander.deucher at amd.com>
> ---
> src/amdgpu_dri2.c | 47 ++++++++++++++++++++++++-----------------------
> src/amdgpu_drm_queue.c | 24 ++++++++++++++++++------
> src/amdgpu_drm_queue.h | 12 +++++-------
> src/amdgpu_kms.c | 36 ++++++++++++++++++------------------
> src/amdgpu_present.c | 17 +++++++++--------
> src/drmmode_display.c | 20 ++++++++++----------
> 6 files changed, 84 insertions(+), 72 deletions(-)
>
> diff --git a/src/amdgpu_dri2.c b/src/amdgpu_dri2.c
> index 4478b16..29f60ba 100644
> --- a/src/amdgpu_dri2.c
> +++ b/src/amdgpu_dri2.c
> @@ -395,7 +395,7 @@ typedef struct _DRI2FrameEvent {
> unsigned frame;
> xf86CrtcPtr crtc;
> OsTimerPtr timer;
> - struct amdgpu_drm_queue_entry *drm_queue;
> + uintptr_t drm_queue_seq;
>
> /* for swaps & flips only */
> DRI2SwapEventPtr event_complete;
> @@ -961,8 +961,8 @@ CARD32 amdgpu_dri2_deferred_event(OsTimerPtr timer, CARD32 now, pointer data)
> */
> if (!event_info->crtc) {
> ErrorF("%s no crtc\n", __func__);
> - if (event_info->drm_queue)
> - amdgpu_drm_abort_entry(event_info->drm_queue);
> + if (event_info->drm_queue_seq)
> + amdgpu_drm_abort_entry(event_info->drm_queue_seq);
> else
> amdgpu_dri2_frame_event_abort(NULL, data);
> return 0;
> @@ -974,9 +974,9 @@ CARD32 amdgpu_dri2_deferred_event(OsTimerPtr timer, CARD32 now, pointer data)
> if (ret) {
> xf86DrvMsg(scrn->scrnIndex, X_ERROR,
> "%s cannot get current time\n", __func__);
> - if (event_info->drm_queue)
> + if (event_info->drm_queue_seq)
> amdgpu_drm_queue_handler(pAMDGPUEnt->fd, 0, 0, 0,
> - event_info->drm_queue);
> + (void*)event_info->drm_queue_seq);
> else
> amdgpu_dri2_frame_event_handler(crtc, 0, 0, data);
> return 0;
> @@ -990,9 +990,10 @@ CARD32 amdgpu_dri2_deferred_event(OsTimerPtr timer, CARD32 now, pointer data)
> delta_seq = delta_t * drmmode_crtc->dpms_last_fps;
> delta_seq /= 1000000;
> frame = (CARD64) drmmode_crtc->dpms_last_seq + delta_seq;
> - if (event_info->drm_queue)
> + if (event_info->drm_queue_seq)
> amdgpu_drm_queue_handler(pAMDGPUEnt->fd, frame, drm_now / 1000000,
> - drm_now % 1000000, event_info->drm_queue);
> + drm_now % 1000000,
> + (void*)event_info->drm_queue_seq);
> else
> amdgpu_dri2_frame_event_handler(crtc, frame, drm_now, data);
> return 0;
> @@ -1023,7 +1024,7 @@ static int amdgpu_dri2_schedule_wait_msc(ClientPtr client, DrawablePtr draw,
> ScrnInfoPtr scrn = xf86ScreenToScrn(screen);
> AMDGPUEntPtr pAMDGPUEnt = AMDGPUEntPriv(scrn);
> DRI2FrameEventPtr wait_info = NULL;
> - struct amdgpu_drm_queue_entry *wait = NULL;
> + uintptr_t drm_queue_seq = 0;
> xf86CrtcPtr crtc = amdgpu_dri2_drawable_crtc(draw, TRUE);
> uint32_t msc_delta;
> drmVBlank vbl;
> @@ -1079,15 +1080,15 @@ static int amdgpu_dri2_schedule_wait_msc(ClientPtr client, DrawablePtr draw,
> current_msc = vbl.reply.sequence + msc_delta;
> current_msc &= 0xffffffff;
>
> - wait = amdgpu_drm_queue_alloc(crtc, client, AMDGPU_DRM_QUEUE_ID_DEFAULT,
> - wait_info, amdgpu_dri2_frame_event_handler,
> - amdgpu_dri2_frame_event_abort);
> - if (!wait) {
> + drm_queue_seq = amdgpu_drm_queue_alloc(crtc, client, AMDGPU_DRM_QUEUE_ID_DEFAULT,
> + wait_info, amdgpu_dri2_frame_event_handler,
> + amdgpu_dri2_frame_event_abort);
> + if (!drm_queue_seq) {
> xf86DrvMsg(scrn->scrnIndex, X_WARNING,
> "Allocating DRM queue event entry failed.\n");
> goto out_complete;
> }
> - wait_info->drm_queue = wait;
> + wait_info->drm_queue_seq = drm_queue_seq;
>
> /*
> * If divisor is zero, or current_msc is smaller than target_msc,
> @@ -1106,7 +1107,7 @@ static int amdgpu_dri2_schedule_wait_msc(ClientPtr client, DrawablePtr draw,
> vbl.request.type = DRM_VBLANK_ABSOLUTE | DRM_VBLANK_EVENT;
> vbl.request.type |= amdgpu_populate_vbl_request_type(crtc);
> vbl.request.sequence = target_msc - msc_delta;
> - vbl.request.signal = (unsigned long)wait;
> + vbl.request.signal = drm_queue_seq;
> ret = drmWaitVBlank(pAMDGPUEnt->fd, &vbl);
> if (ret) {
> xf86DrvMsg(scrn->scrnIndex, X_WARNING,
> @@ -1138,7 +1139,7 @@ static int amdgpu_dri2_schedule_wait_msc(ClientPtr client, DrawablePtr draw,
> if ((current_msc % divisor) >= remainder)
> vbl.request.sequence += divisor;
>
> - vbl.request.signal = (unsigned long)wait;
> + vbl.request.signal = drm_queue_seq;
> ret = drmWaitVBlank(pAMDGPUEnt->fd, &vbl);
> if (ret) {
> xf86DrvMsg(scrn->scrnIndex, X_WARNING,
> @@ -1190,7 +1191,7 @@ static int amdgpu_dri2_schedule_swap(ClientPtr client, DrawablePtr draw,
> drmVBlank vbl;
> int ret, flip = 0;
> DRI2FrameEventPtr swap_info = NULL;
> - struct amdgpu_drm_queue_entry *swap;
> + uintptr_t drm_queue_seq;
> CARD64 current_msc;
> BoxRec box;
> RegionRec region;
> @@ -1227,15 +1228,15 @@ static int amdgpu_dri2_schedule_swap(ClientPtr client, DrawablePtr draw,
> swap_info->back = back;
> swap_info->crtc = crtc;
>
> - swap = amdgpu_drm_queue_alloc(crtc, client, AMDGPU_DRM_QUEUE_ID_DEFAULT,
> - swap_info, amdgpu_dri2_frame_event_handler,
> - amdgpu_dri2_frame_event_abort);
> - if (!swap) {
> + drm_queue_seq = amdgpu_drm_queue_alloc(crtc, client, AMDGPU_DRM_QUEUE_ID_DEFAULT,
> + swap_info, amdgpu_dri2_frame_event_handler,
> + amdgpu_dri2_frame_event_abort);
> + if (!drm_queue_seq) {
> xf86DrvMsg(scrn->scrnIndex, X_WARNING,
> "Allocating DRM queue entry failed.\n");
> goto blit_fallback;
> }
> - swap_info->drm_queue = swap;
> + swap_info->drm_queue_seq = drm_queue_seq;
>
> /*
> * CRTC is in DPMS off state, fallback to blit, but calculate
> @@ -1304,7 +1305,7 @@ static int amdgpu_dri2_schedule_swap(ClientPtr client, DrawablePtr draw,
> *target_msc = current_msc;
>
> vbl.request.sequence = *target_msc - msc_delta;
> - vbl.request.signal = (unsigned long)swap;
> + vbl.request.signal = drm_queue_seq;
> ret = drmWaitVBlank(pAMDGPUEnt->fd, &vbl);
> if (ret) {
> xf86DrvMsg(scrn->scrnIndex, X_WARNING,
> @@ -1350,7 +1351,7 @@ static int amdgpu_dri2_schedule_swap(ClientPtr client, DrawablePtr draw,
> /* Account for 1 frame extra pageflip delay if flip > 0 */
> vbl.request.sequence -= flip;
>
> - vbl.request.signal = (unsigned long)swap;
> + vbl.request.signal = drm_queue_seq;
> ret = drmWaitVBlank(pAMDGPUEnt->fd, &vbl);
> if (ret) {
> xf86DrvMsg(scrn->scrnIndex, X_WARNING,
> diff --git a/src/amdgpu_drm_queue.c b/src/amdgpu_drm_queue.c
> index 11b74a0..25b7114 100644
> --- a/src/amdgpu_drm_queue.c
> +++ b/src/amdgpu_drm_queue.c
> @@ -40,6 +40,7 @@
> struct amdgpu_drm_queue_entry {
> struct xorg_list list;
> uint64_t id;
> + uintptr_t seq;
> void *data;
> ClientPtr client;
> xf86CrtcPtr crtc;
> @@ -49,6 +50,7 @@ struct amdgpu_drm_queue_entry {
>
> static int amdgpu_drm_queue_refcnt;
> static struct xorg_list amdgpu_drm_queue;
> +static uintptr_t amdgpu_drm_queue_seq;
>
>
> /*
> @@ -58,11 +60,11 @@ void
> amdgpu_drm_queue_handler(int fd, unsigned int frame, unsigned int sec,
> unsigned int usec, void *user_ptr)
> {
> - struct amdgpu_drm_queue_entry *user_data = user_ptr;
> + uintptr_t seq = (uintptr_t)user_ptr;
> struct amdgpu_drm_queue_entry *e, *tmp;
>
> xorg_list_for_each_entry_safe(e, tmp, &amdgpu_drm_queue, list) {
> - if (e == user_data) {
> + if (e->seq == seq) {
> xorg_list_del(&e->list);
> if (e->handler)
> e->handler(e->crtc, frame,
> @@ -80,7 +82,7 @@ amdgpu_drm_queue_handler(int fd, unsigned int frame, unsigned int sec,
> * Enqueue a potential drm response; when the associated response
> * appears, we've got data to pass to the handler from here
> */
> -struct amdgpu_drm_queue_entry *
> +uintptr_t
> amdgpu_drm_queue_alloc(xf86CrtcPtr crtc, ClientPtr client,
> uint64_t id, void *data,
> amdgpu_drm_handler_proc handler,
> @@ -92,6 +94,9 @@ amdgpu_drm_queue_alloc(xf86CrtcPtr crtc, ClientPtr client,
> if (!e)
> return NULL;
>
> + if (!amdgpu_drm_queue_seq)
> + amdgpu_drm_queue_seq = 1;
> + e->seq = amdgpu_drm_queue_seq++;
> e->client = client;
> e->crtc = crtc;
> e->id = id;
> @@ -101,7 +106,7 @@ amdgpu_drm_queue_alloc(xf86CrtcPtr crtc, ClientPtr client,
>
> xorg_list_add(&e->list, &amdgpu_drm_queue);
>
> - return e;
> + return e->seq;
> }
>
> /*
> @@ -139,9 +144,16 @@ amdgpu_drm_abort_client(ClientPtr client)
> * Abort specific drm queue entry
> */
> void
> -amdgpu_drm_abort_entry(struct amdgpu_drm_queue_entry *entry)
> +amdgpu_drm_abort_entry(uintptr_t seq)
> {
> - amdgpu_drm_abort_one(entry);
> + struct amdgpu_drm_queue_entry *e, *tmp;
> +
> + xorg_list_for_each_entry_safe(e, tmp, &amdgpu_drm_queue, list) {
> + if (e->seq == seq) {
> + amdgpu_drm_abort_one(e);
> + break;
> + }
> + }
> }
>
> /*
> diff --git a/src/amdgpu_drm_queue.h b/src/amdgpu_drm_queue.h
> index 892ba13..b4d4009 100644
> --- a/src/amdgpu_drm_queue.h
> +++ b/src/amdgpu_drm_queue.h
> @@ -43,14 +43,12 @@ typedef void (*amdgpu_drm_abort_proc)(xf86CrtcPtr crtc, void *data);
> void amdgpu_drm_queue_handler(int fd, unsigned int frame,
> unsigned int tv_sec, unsigned int tv_usec,
> void *user_ptr);
> -struct amdgpu_drm_queue_entry *amdgpu_drm_queue_alloc(xf86CrtcPtr crtc,
> - ClientPtr client,
> - uint64_t id,
> - void *data,
> - amdgpu_drm_handler_proc handler,
> - amdgpu_drm_abort_proc abort);
> +uintptr_t amdgpu_drm_queue_alloc(xf86CrtcPtr crtc, ClientPtr client,
> + uint64_t id, void *data,
> + amdgpu_drm_handler_proc handler,
> + amdgpu_drm_abort_proc abort);
> void amdgpu_drm_abort_client(ClientPtr client);
> -void amdgpu_drm_abort_entry(struct amdgpu_drm_queue_entry *entry);
> +void amdgpu_drm_abort_entry(uintptr_t seq);
> void amdgpu_drm_abort_id(uint64_t id);
> void amdgpu_drm_queue_init();
> void amdgpu_drm_queue_close(ScrnInfoPtr scrn);
> diff --git a/src/amdgpu_kms.c b/src/amdgpu_kms.c
> index 6dcec69..ae98cf1 100644
> --- a/src/amdgpu_kms.c
> +++ b/src/amdgpu_kms.c
> @@ -397,7 +397,7 @@ static void
> amdgpu_scanout_update(xf86CrtcPtr xf86_crtc)
> {
> drmmode_crtc_private_ptr drmmode_crtc = xf86_crtc->driver_private;
> - struct amdgpu_drm_queue_entry *drm_queue_entry;
> + uintptr_t drm_queue_seq;
> ScrnInfoPtr scrn;
> AMDGPUEntPtr pAMDGPUEnt;
> drmVBlank vbl;
> @@ -427,13 +427,13 @@ amdgpu_scanout_update(xf86CrtcPtr xf86_crtc)
> return;
>
> scrn = xf86_crtc->scrn;
> - drm_queue_entry = amdgpu_drm_queue_alloc(xf86_crtc,
> - AMDGPU_DRM_QUEUE_CLIENT_DEFAULT,
> - AMDGPU_DRM_QUEUE_ID_DEFAULT,
> - drmmode_crtc,
> - amdgpu_scanout_update_handler,
> - amdgpu_scanout_update_abort);
> - if (!drm_queue_entry) {
> + drm_queue_seq = amdgpu_drm_queue_alloc(xf86_crtc,
> + AMDGPU_DRM_QUEUE_CLIENT_DEFAULT,
> + AMDGPU_DRM_QUEUE_ID_DEFAULT,
> + drmmode_crtc,
> + amdgpu_scanout_update_handler,
> + amdgpu_scanout_update_abort);
> + if (!drm_queue_seq) {
> xf86DrvMsg(scrn->scrnIndex, X_WARNING,
> "amdgpu_drm_queue_alloc failed for scanout update\n");
> return;
> @@ -443,12 +443,12 @@ amdgpu_scanout_update(xf86CrtcPtr xf86_crtc)
> vbl.request.type = DRM_VBLANK_RELATIVE | DRM_VBLANK_EVENT;
> vbl.request.type |= amdgpu_populate_vbl_request_type(xf86_crtc);
> vbl.request.sequence = 1;
> - vbl.request.signal = (unsigned long)drm_queue_entry;
> + vbl.request.signal = drm_queue_seq;
> if (drmWaitVBlank(pAMDGPUEnt->fd, &vbl)) {
> xf86DrvMsg(scrn->scrnIndex, X_WARNING,
> "drmWaitVBlank failed for scanout update: %s\n",
> strerror(errno));
> - amdgpu_drm_abort_entry(drm_queue_entry);
> + amdgpu_drm_abort_entry(drm_queue_seq);
> return;
> }
>
> @@ -471,7 +471,7 @@ amdgpu_scanout_flip(ScreenPtr pScreen, AMDGPUInfoPtr info,
> drmmode_crtc_private_ptr drmmode_crtc = xf86_crtc->driver_private;
> ScrnInfoPtr scrn;
> AMDGPUEntPtr pAMDGPUEnt;
> - struct amdgpu_drm_queue_entry *drm_queue_entry;
> + uintptr_t drm_queue_seq;
> unsigned scanout_id;
>
> if (drmmode_crtc->scanout_update_pending)
> @@ -482,12 +482,12 @@ amdgpu_scanout_flip(ScreenPtr pScreen, AMDGPUInfoPtr info,
> return;
>
> scrn = xf86_crtc->scrn;
> - drm_queue_entry = amdgpu_drm_queue_alloc(xf86_crtc,
> - AMDGPU_DRM_QUEUE_CLIENT_DEFAULT,
> - AMDGPU_DRM_QUEUE_ID_DEFAULT,
> - drmmode_crtc, NULL,
> - amdgpu_scanout_flip_abort);
> - if (!drm_queue_entry) {
> + drm_queue_seq = amdgpu_drm_queue_alloc(xf86_crtc,
> + AMDGPU_DRM_QUEUE_CLIENT_DEFAULT,
> + AMDGPU_DRM_QUEUE_ID_DEFAULT,
> + drmmode_crtc, NULL,
> + amdgpu_scanout_flip_abort);
> + if (!drm_queue_seq) {
> xf86DrvMsg(scrn->scrnIndex, X_WARNING,
> "Allocating DRM event queue entry failed.\n");
> return;
> @@ -496,7 +496,7 @@ amdgpu_scanout_flip(ScreenPtr pScreen, AMDGPUInfoPtr info,
> pAMDGPUEnt = AMDGPUEntPriv(scrn);
> if (drmModePageFlip(pAMDGPUEnt->fd, drmmode_crtc->mode_crtc->crtc_id,
> drmmode_crtc->scanout[scanout_id].fb_id,
> - DRM_MODE_PAGE_FLIP_EVENT, drm_queue_entry)) {
> + DRM_MODE_PAGE_FLIP_EVENT, (void*)drm_queue_seq)) {
> xf86DrvMsg(scrn->scrnIndex, X_WARNING, "flip queue failed in %s: %s\n",
> __func__, strerror(errno));
> return;
> diff --git a/src/amdgpu_present.c b/src/amdgpu_present.c
> index 4b33ce2..4aa0708 100644
> --- a/src/amdgpu_present.c
> +++ b/src/amdgpu_present.c
> @@ -157,7 +157,7 @@ amdgpu_present_queue_vblank(RRCrtcPtr crtc, uint64_t event_id, uint64_t msc)
> AMDGPUEntPtr pAMDGPUEnt = AMDGPUEntPriv(scrn);
> int crtc_id = drmmode_get_crtc_id(xf86_crtc);
> struct amdgpu_present_vblank_event *event;
> - struct amdgpu_drm_queue_entry *queue;
> + uintptr_t drm_queue_seq;
> drmVBlank vbl;
> int ret;
>
> @@ -165,24 +165,25 @@ amdgpu_present_queue_vblank(RRCrtcPtr crtc, uint64_t event_id, uint64_t msc)
> if (!event)
> return BadAlloc;
> event->event_id = event_id;
> - queue = amdgpu_drm_queue_alloc(xf86_crtc, AMDGPU_DRM_QUEUE_CLIENT_DEFAULT,
> - event_id, event,
> - amdgpu_present_vblank_handler,
> - amdgpu_present_vblank_abort);
> - if (!queue) {
> + drm_queue_seq = amdgpu_drm_queue_alloc(xf86_crtc,
> + AMDGPU_DRM_QUEUE_CLIENT_DEFAULT,
> + event_id, event,
> + amdgpu_present_vblank_handler,
> + amdgpu_present_vblank_abort);
> + if (!drm_queue_seq) {
> free(event);
> return BadAlloc;
> }
>
> vbl.request.type = DRM_VBLANK_ABSOLUTE | DRM_VBLANK_EVENT | crtc_select(crtc_id);
> vbl.request.sequence = msc;
> - vbl.request.signal = (unsigned long)queue;
> + vbl.request.signal = drm_queue_seq;
> for (;;) {
> ret = drmWaitVBlank(pAMDGPUEnt->fd, &vbl);
> if (!ret)
> break;
> if (errno != EBUSY || !amdgpu_present_flush_drm_events(screen)) {
> - amdgpu_drm_abort_entry(queue);
> + amdgpu_drm_abort_entry(drm_queue_seq);
> return BadAlloc;
> }
> }
> diff --git a/src/drmmode_display.c b/src/drmmode_display.c
> index 2aea542..07ae9b2 100644
> --- a/src/drmmode_display.c
> +++ b/src/drmmode_display.c
> @@ -2504,7 +2504,7 @@ Bool amdgpu_do_pageflip(ScrnInfoPtr scrn, ClientPtr client,
> drmmode_ptr drmmode = drmmode_crtc->drmmode;
> int i;
> drmmode_flipdata_ptr flipdata;
> - struct amdgpu_drm_queue_entry *drm_queue = NULL;
> + uintptr_t drm_queue_seq = 0;
> uint32_t new_front_handle;
>
> if (!amdgpu_pixmap_get_handle(new_front, &new_front_handle)) {
> @@ -2559,11 +2559,11 @@ Bool amdgpu_do_pageflip(ScrnInfoPtr scrn, ClientPtr client,
> if (drmmode_crtc->hw_id == ref_crtc_hw_id)
> flipdata->fe_crtc = crtc;
>
> - drm_queue = amdgpu_drm_queue_alloc(crtc, client, id,
> - flipdata,
> - drmmode_flip_handler,
> - drmmode_flip_abort);
> - if (!drm_queue) {
> + drm_queue_seq = amdgpu_drm_queue_alloc(crtc, client, id,
> + flipdata,
> + drmmode_flip_handler,
> + drmmode_flip_abort);
> + if (!drm_queue_seq) {
> xf86DrvMsg(scrn->scrnIndex, X_WARNING,
> "Allocating DRM queue event entry failed.\n");
> goto error;
> @@ -2571,13 +2571,13 @@ Bool amdgpu_do_pageflip(ScrnInfoPtr scrn, ClientPtr client,
>
> if (drmModePageFlip(pAMDGPUEnt->fd, drmmode_crtc->mode_crtc->crtc_id,
> drmmode->fb_id, DRM_MODE_PAGE_FLIP_EVENT,
> - drm_queue)) {
> + (void*)drm_queue_seq)) {
> xf86DrvMsg(scrn->scrnIndex, X_WARNING,
> "flip queue failed: %s\n", strerror(errno));
> goto error;
> }
> drmmode_crtc->flip_pending = TRUE;
> - drm_queue = NULL;
> + drm_queue_seq = 0;
> }
>
> if (flipdata->flip_count > 0)
> @@ -2589,8 +2589,8 @@ error:
> drmmode->fb_id = flipdata->old_fb_id;
> }
>
> - if (drm_queue)
> - amdgpu_drm_abort_entry(drm_queue);
> + if (drm_queue_seq)
> + amdgpu_drm_abort_entry(drm_queue_seq);
> else if (crtc)
> drmmode_flip_abort(crtc, flipdata);
> else if (flipdata && flipdata->flip_count <= 1)
> --
> 2.8.0.rc3
>
> _______________________________________________
> xorg-driver-ati mailing list
> xorg-driver-ati at lists.x.org
> https://lists.x.org/mailman/listinfo/xorg-driver-ati
More information about the xorg-driver-ati
mailing list