[PATCH xf86-video-ati] Identify DRM event queue entries by sequence number instead of by pointer
Alex Deucher
alexdeucher at gmail.com
Thu Mar 31 14:09:59 UTC 2016
On Thu, Mar 31, 2016 at 4:40 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.
>
> Signed-off-by: Michel Dänzer <michel.daenzer at amd.com>
Reviewed-by: Alex Deucher <alexander.deucher at amd.com>
> ---
> src/drmmode_display.c | 20 ++++++++++----------
> src/radeon_dri2.c | 47 ++++++++++++++++++++++++-----------------------
> src/radeon_drm_queue.c | 24 ++++++++++++++++++------
> src/radeon_drm_queue.h | 12 +++++-------
> src/radeon_kms.c | 36 ++++++++++++++++++------------------
> src/radeon_present.c | 17 +++++++++--------
> 6 files changed, 84 insertions(+), 72 deletions(-)
>
> diff --git a/src/drmmode_display.c b/src/drmmode_display.c
> index 7331015..a368a41 100644
> --- a/src/drmmode_display.c
> +++ b/src/drmmode_display.c
> @@ -2660,7 +2660,7 @@ Bool radeon_do_pageflip(ScrnInfoPtr scrn, ClientPtr client,
> int i;
> uint32_t tiling_flags = 0;
> drmmode_flipdata_ptr flipdata;
> - struct radeon_drm_queue_entry *drm_queue = NULL;
> + uintptr_t drm_queue_seq = 0;
>
> if (info->allowColorTiling) {
> if (info->ChipFamily >= CHIP_FAMILY_R600)
> @@ -2720,11 +2720,11 @@ Bool radeon_do_pageflip(ScrnInfoPtr scrn, ClientPtr client,
> if (drmmode_crtc->hw_id == ref_crtc_hw_id)
> flipdata->fe_crtc = crtc;
>
> - drm_queue = radeon_drm_queue_alloc(crtc, client, id,
> - flipdata,
> - drmmode_flip_handler,
> - drmmode_flip_abort);
> - if (!drm_queue) {
> + drm_queue_seq = radeon_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;
> @@ -2732,13 +2732,13 @@ Bool radeon_do_pageflip(ScrnInfoPtr scrn, ClientPtr client,
>
> if (drmModePageFlip(drmmode->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)
> @@ -2750,8 +2750,8 @@ error:
> drmmode->fb_id = flipdata->old_fb_id;
> }
>
> - if (drm_queue)
> - radeon_drm_abort_entry(drm_queue);
> + if (drm_queue_seq)
> + radeon_drm_abort_entry(drm_queue_seq);
> else if (crtc)
> drmmode_flip_abort(crtc, flipdata);
> else if (flipdata && flipdata->flip_count <= 1)
> diff --git a/src/radeon_dri2.c b/src/radeon_dri2.c
> index 73e09d0..84cd031 100644
> --- a/src/radeon_dri2.c
> +++ b/src/radeon_dri2.c
> @@ -516,7 +516,7 @@ typedef struct _DRI2FrameEvent {
> unsigned frame;
> xf86CrtcPtr crtc;
> OsTimerPtr timer;
> - struct radeon_drm_queue_entry *drm_queue;
> + uintptr_t drm_queue_seq;
>
> /* for swaps & flips only */
> DRI2SwapEventPtr event_complete;
> @@ -1079,8 +1079,8 @@ CARD32 radeon_dri2_deferred_event(OsTimerPtr timer, CARD32 now, pointer data)
> */
> if (!event_info->crtc) {
> ErrorF("%s no crtc\n", __func__);
> - if (event_info->drm_queue)
> - radeon_drm_abort_entry(event_info->drm_queue);
> + if (event_info->drm_queue_seq)
> + radeon_drm_abort_entry(event_info->drm_queue_seq);
> else
> radeon_dri2_frame_event_abort(NULL, data);
> return 0;
> @@ -1092,9 +1092,9 @@ CARD32 radeon_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)
> radeon_drm_queue_handler(info->dri2.drm_fd, 0, 0, 0,
> - event_info->drm_queue);
> + (void*)event_info->drm_queue_seq);
> else
> radeon_dri2_frame_event_handler(crtc, 0, 0, data);
> return 0;
> @@ -1108,9 +1108,10 @@ CARD32 radeon_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)
> radeon_drm_queue_handler(info->dri2.drm_fd, frame, drm_now / 1000000,
> - drm_now % 1000000, event_info->drm_queue);
> + drm_now % 1000000,
> + (void*)event_info->drm_queue_seq);
> else
> radeon_dri2_frame_event_handler(crtc, frame, drm_now, data);
> return 0;
> @@ -1141,7 +1142,7 @@ static int radeon_dri2_schedule_wait_msc(ClientPtr client, DrawablePtr draw,
> ScrnInfoPtr scrn = xf86ScreenToScrn(screen);
> RADEONInfoPtr info = RADEONPTR(scrn);
> DRI2FrameEventPtr wait_info = NULL;
> - struct radeon_drm_queue_entry *wait = NULL;
> + uintptr_t drm_queue_seq = 0;
> xf86CrtcPtr crtc = radeon_dri2_drawable_crtc(draw, TRUE);
> uint32_t msc_delta;
> drmVBlank vbl;
> @@ -1197,15 +1198,15 @@ static int radeon_dri2_schedule_wait_msc(ClientPtr client, DrawablePtr draw,
> current_msc = vbl.reply.sequence + msc_delta;
> current_msc &= 0xffffffff;
>
> - wait = radeon_drm_queue_alloc(crtc, client, RADEON_DRM_QUEUE_ID_DEFAULT,
> - wait_info, radeon_dri2_frame_event_handler,
> - radeon_dri2_frame_event_abort);
> - if (!wait) {
> + drm_queue_seq = radeon_drm_queue_alloc(crtc, client, RADEON_DRM_QUEUE_ID_DEFAULT,
> + wait_info, radeon_dri2_frame_event_handler,
> + radeon_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,
> @@ -1224,7 +1225,7 @@ static int radeon_dri2_schedule_wait_msc(ClientPtr client, DrawablePtr draw,
> vbl.request.type = DRM_VBLANK_ABSOLUTE | DRM_VBLANK_EVENT;
> vbl.request.type |= radeon_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(info->dri2.drm_fd, &vbl);
> if (ret) {
> xf86DrvMsg(scrn->scrnIndex, X_WARNING,
> @@ -1255,7 +1256,7 @@ static int radeon_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(info->dri2.drm_fd, &vbl);
> if (ret) {
> xf86DrvMsg(scrn->scrnIndex, X_WARNING,
> @@ -1307,7 +1308,7 @@ static int radeon_dri2_schedule_swap(ClientPtr client, DrawablePtr draw,
> drmVBlank vbl;
> int ret, flip = 0;
> DRI2FrameEventPtr swap_info = NULL;
> - struct radeon_drm_queue_entry *swap;
> + uintptr_t drm_queue_seq;
> CARD64 current_msc;
> BoxRec box;
> RegionRec region;
> @@ -1344,15 +1345,15 @@ static int radeon_dri2_schedule_swap(ClientPtr client, DrawablePtr draw,
> swap_info->back = back;
> swap_info->crtc = crtc;
>
> - swap = radeon_drm_queue_alloc(crtc, client, RADEON_DRM_QUEUE_ID_DEFAULT,
> - swap_info, radeon_dri2_frame_event_handler,
> - radeon_dri2_frame_event_abort);
> - if (!swap) {
> + drm_queue_seq = radeon_drm_queue_alloc(crtc, client, RADEON_DRM_QUEUE_ID_DEFAULT,
> + swap_info, radeon_dri2_frame_event_handler,
> + radeon_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
> @@ -1421,7 +1422,7 @@ static int radeon_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(info->dri2.drm_fd, &vbl);
> if (ret) {
> xf86DrvMsg(scrn->scrnIndex, X_WARNING,
> @@ -1466,7 +1467,7 @@ static int radeon_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(info->dri2.drm_fd, &vbl);
> if (ret) {
> xf86DrvMsg(scrn->scrnIndex, X_WARNING,
> diff --git a/src/radeon_drm_queue.c b/src/radeon_drm_queue.c
> index 25fb183..0d999dd 100644
> --- a/src/radeon_drm_queue.c
> +++ b/src/radeon_drm_queue.c
> @@ -40,6 +40,7 @@
> struct radeon_drm_queue_entry {
> struct xorg_list list;
> uint64_t id;
> + uintptr_t seq;
> void *data;
> ClientPtr client;
> xf86CrtcPtr crtc;
> @@ -49,6 +50,7 @@ struct radeon_drm_queue_entry {
>
> static int radeon_drm_queue_refcnt;
> static struct xorg_list radeon_drm_queue;
> +static uintptr_t radeon_drm_queue_seq;
>
>
> /*
> @@ -58,11 +60,11 @@ void
> radeon_drm_queue_handler(int fd, unsigned int frame, unsigned int sec,
> unsigned int usec, void *user_ptr)
> {
> - struct radeon_drm_queue_entry *user_data = user_ptr;
> + uintptr_t seq = (uintptr_t)user_ptr;
> struct radeon_drm_queue_entry *e, *tmp;
>
> xorg_list_for_each_entry_safe(e, tmp, &radeon_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 @@ radeon_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 radeon_drm_queue_entry *
> +uintptr_t
> radeon_drm_queue_alloc(xf86CrtcPtr crtc, ClientPtr client,
> uint64_t id, void *data,
> radeon_drm_handler_proc handler,
> @@ -92,6 +94,9 @@ radeon_drm_queue_alloc(xf86CrtcPtr crtc, ClientPtr client,
> if (!e)
> return NULL;
>
> + if (!radeon_drm_queue_seq)
> + radeon_drm_queue_seq = 1;
> + e->seq = radeon_drm_queue_seq++;
> e->client = client;
> e->crtc = crtc;
> e->id = id;
> @@ -101,7 +106,7 @@ radeon_drm_queue_alloc(xf86CrtcPtr crtc, ClientPtr client,
>
> xorg_list_add(&e->list, &radeon_drm_queue);
>
> - return e;
> + return e->seq;
> }
>
> /*
> @@ -139,9 +144,16 @@ radeon_drm_abort_client(ClientPtr client)
> * Abort specific drm queue entry
> */
> void
> -radeon_drm_abort_entry(struct radeon_drm_queue_entry *entry)
> +radeon_drm_abort_entry(uintptr_t seq)
> {
> - radeon_drm_abort_one(entry);
> + struct radeon_drm_queue_entry *e, *tmp;
> +
> + xorg_list_for_each_entry_safe(e, tmp, &radeon_drm_queue, list) {
> + if (e->seq == seq) {
> + radeon_drm_abort_one(e);
> + break;
> + }
> + }
> }
>
> /*
> diff --git a/src/radeon_drm_queue.h b/src/radeon_drm_queue.h
> index 5d8dedf..0d9d278 100644
> --- a/src/radeon_drm_queue.h
> +++ b/src/radeon_drm_queue.h
> @@ -41,14 +41,12 @@ typedef void (*radeon_drm_abort_proc)(xf86CrtcPtr crtc, void *data);
> void radeon_drm_queue_handler(int fd, unsigned int frame,
> unsigned int tv_sec, unsigned int tv_usec,
> void *user_ptr);
> -struct radeon_drm_queue_entry *radeon_drm_queue_alloc(xf86CrtcPtr crtc,
> - ClientPtr client,
> - uint64_t id,
> - void *data,
> - radeon_drm_handler_proc handler,
> - radeon_drm_abort_proc abort);
> +uintptr_t radeon_drm_queue_alloc(xf86CrtcPtr crtc, ClientPtr client,
> + uint64_t id, void *data,
> + radeon_drm_handler_proc handler,
> + radeon_drm_abort_proc abort);
> void radeon_drm_abort_client(ClientPtr client);
> -void radeon_drm_abort_entry(struct radeon_drm_queue_entry *entry);
> +void radeon_drm_abort_entry(uintptr_t seq);
> void radeon_drm_abort_id(uint64_t id);
> void radeon_drm_queue_init();
> void radeon_drm_queue_close(ScrnInfoPtr scrn);
> diff --git a/src/radeon_kms.c b/src/radeon_kms.c
> index 555d736..c5310ea 100644
> --- a/src/radeon_kms.c
> +++ b/src/radeon_kms.c
> @@ -491,7 +491,7 @@ static void
> radeon_scanout_update(xf86CrtcPtr xf86_crtc)
> {
> drmmode_crtc_private_ptr drmmode_crtc = xf86_crtc->driver_private;
> - struct radeon_drm_queue_entry *drm_queue_entry;
> + uintptr_t drm_queue_seq;
> ScrnInfoPtr scrn;
> drmVBlank vbl;
> DamagePtr pDamage;
> @@ -520,13 +520,13 @@ radeon_scanout_update(xf86CrtcPtr xf86_crtc)
> return;
>
> scrn = xf86_crtc->scrn;
> - drm_queue_entry = radeon_drm_queue_alloc(xf86_crtc,
> - RADEON_DRM_QUEUE_CLIENT_DEFAULT,
> - RADEON_DRM_QUEUE_ID_DEFAULT,
> - drmmode_crtc,
> - radeon_scanout_update_handler,
> - radeon_scanout_update_abort);
> - if (!drm_queue_entry) {
> + drm_queue_seq = radeon_drm_queue_alloc(xf86_crtc,
> + RADEON_DRM_QUEUE_CLIENT_DEFAULT,
> + RADEON_DRM_QUEUE_ID_DEFAULT,
> + drmmode_crtc,
> + radeon_scanout_update_handler,
> + radeon_scanout_update_abort);
> + if (!drm_queue_seq) {
> xf86DrvMsg(scrn->scrnIndex, X_WARNING,
> "radeon_drm_queue_alloc failed for scanout update\n");
> return;
> @@ -535,12 +535,12 @@ radeon_scanout_update(xf86CrtcPtr xf86_crtc)
> vbl.request.type = DRM_VBLANK_RELATIVE | DRM_VBLANK_EVENT;
> vbl.request.type |= radeon_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(RADEONPTR(scrn)->dri2.drm_fd, &vbl)) {
> xf86DrvMsg(scrn->scrnIndex, X_WARNING,
> "drmWaitVBlank failed for scanout update: %s\n",
> strerror(errno));
> - radeon_drm_abort_entry(drm_queue_entry);
> + radeon_drm_abort_entry(drm_queue_seq);
> return;
> }
>
> @@ -562,7 +562,7 @@ radeon_scanout_flip(ScreenPtr pScreen, RADEONInfoPtr info,
> {
> drmmode_crtc_private_ptr drmmode_crtc = xf86_crtc->driver_private;
> ScrnInfoPtr scrn;
> - struct radeon_drm_queue_entry *drm_queue_entry;
> + uintptr_t drm_queue_seq;
> unsigned scanout_id;
>
> if (drmmode_crtc->scanout_update_pending)
> @@ -573,12 +573,12 @@ radeon_scanout_flip(ScreenPtr pScreen, RADEONInfoPtr info,
> return;
>
> scrn = xf86_crtc->scrn;
> - drm_queue_entry = radeon_drm_queue_alloc(xf86_crtc,
> - RADEON_DRM_QUEUE_CLIENT_DEFAULT,
> - RADEON_DRM_QUEUE_ID_DEFAULT,
> - drmmode_crtc, NULL,
> - radeon_scanout_flip_abort);
> - if (!drm_queue_entry) {
> + drm_queue_seq = radeon_drm_queue_alloc(xf86_crtc,
> + RADEON_DRM_QUEUE_CLIENT_DEFAULT,
> + RADEON_DRM_QUEUE_ID_DEFAULT,
> + drmmode_crtc, NULL,
> + radeon_scanout_flip_abort);
> + if (!drm_queue_seq) {
> xf86DrvMsg(scrn->scrnIndex, X_WARNING,
> "Allocating DRM event queue entry failed.\n");
> return;
> @@ -586,7 +586,7 @@ radeon_scanout_flip(ScreenPtr pScreen, RADEONInfoPtr info,
>
> if (drmModePageFlip(drmmode_crtc->drmmode->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/radeon_present.c b/src/radeon_present.c
> index 3be3360..8988fc6 100644
> --- a/src/radeon_present.c
> +++ b/src/radeon_present.c
> @@ -156,7 +156,7 @@ radeon_present_queue_vblank(RRCrtcPtr crtc, uint64_t event_id, uint64_t msc)
> RADEONInfoPtr info = RADEONPTR(scrn);
> int crtc_id = drmmode_get_crtc_id(xf86_crtc);
> struct radeon_present_vblank_event *event;
> - struct radeon_drm_queue_entry *queue;
> + uintptr_t drm_queue_seq;
> drmVBlank vbl;
> int ret;
>
> @@ -164,24 +164,25 @@ radeon_present_queue_vblank(RRCrtcPtr crtc, uint64_t event_id, uint64_t msc)
> if (!event)
> return BadAlloc;
> event->event_id = event_id;
> - queue = radeon_drm_queue_alloc(xf86_crtc, RADEON_DRM_QUEUE_CLIENT_DEFAULT,
> - event_id, event,
> - radeon_present_vblank_handler,
> - radeon_present_vblank_abort);
> - if (!queue) {
> + drm_queue_seq = radeon_drm_queue_alloc(xf86_crtc,
> + RADEON_DRM_QUEUE_CLIENT_DEFAULT,
> + event_id, event,
> + radeon_present_vblank_handler,
> + radeon_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(info->dri2.drm_fd, &vbl);
> if (!ret)
> break;
> if (errno != EBUSY || !radeon_present_flush_drm_events(screen)) {
> - radeon_drm_abort_entry(queue);
> + radeon_drm_abort_entry(drm_queue_seq);
> return BadAlloc;
> }
> }
> --
> 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