[PATCH] radeon: proper DRI2 pending events handling when client gone. (v3)
Christopher James Halse Rogers
christopher.halse.rogers at canonical.com
Sat Sep 18 03:03:23 PDT 2010
On Tue, 2010-09-14 at 22:55 +0200, Oldřich Jedlička wrote:
> Properly handle asynchronous DRI2 events for disconnected clients.
> Track client's pending requests and mark them as invalid when the
> client disconnects.
>
> This is based on the version from Alban Browaeys in bug #29065.
>
> v1 (Alban Browaeys): Based upon a detailed explanation from Oldřich
> Jedlička and comments from Christopher James Halse Rogers.
> on http://lists.x.org/archives/xorg-driver-ati/2010-August/016780.html .
>
> v2: Updated version to apply on master. Removed unnecessary
> client_index field from _DRI2FrameEvent. Added freeing/removing from
> list to failed paths of radeon_dri2_schedule_wait_msc and
> radeon_dri2_schedule_swap.
>
> v3: Adopt to older xorg-server that doesn't have dixRegisterPrivateKey.
> ---
> src/radeon_dri2.c | 149 +++++++++++++++++++++++++++++++++++++++++++++--------
> 1 files changed, 128 insertions(+), 21 deletions(-)
>
> diff --git a/src/radeon_dri2.c b/src/radeon_dri2.c
> index ed7fdd6..054b405 100644
> --- a/src/radeon_dri2.c
> +++ b/src/radeon_dri2.c
> @@ -37,6 +37,7 @@
> #include "radeon.h"
> #include "radeon_dri2.h"
> #include "radeon_version.h"
> +#include "list.h"
This header was only introduced in xserver 1.8. I presume that it's
expected that radeon can build against older X servers, so you might
need to conditionally copy this into the DDX.
>
> #ifdef RADEON_DRI2
>
> @@ -376,7 +377,6 @@ enum DRI2FrameEventType {
> typedef struct _DRI2FrameEvent {
> XID drawable_id;
> ClientPtr client;
> - int client_index;
> enum DRI2FrameEventType type;
> int frame;
>
> @@ -385,8 +385,86 @@ typedef struct _DRI2FrameEvent {
> void *event_data;
> DRI2BufferPtr front;
> DRI2BufferPtr back;
> +
> + Bool valid;
> +
> + struct list link;
> } DRI2FrameEventRec, *DRI2FrameEventPtr;
>
> +typedef struct _DRI2ClientEvents {
> + struct list reference_list;
> +} DRI2ClientEventsRec, *DRI2ClientEventsPtr;
> +
> +#if HAS_DEVPRIVATEKEYREC
> +
> +static DevPrivateKeyRec DRI2ClientEventsPrivateKeyRec;
> +#define DRI2ClientEventsPrivateKey (&DRI2ClientEventsPrivateKeyRec)
> +
> +#else
> +
> +static int DRI2ClientEventsPrivateKeyIndex;
> +DevPrivateKey DRI2ClientEventsPrivateKey = &DRI2ClientEventsPrivateKeyIndex;
> +
> +#endif /* HAS_DEVPRIVATEKEYREC */
> +
> +#define GetDRI2ClientEvents(pClient) ((DRI2ClientEventsPtr) \
> + dixLookupPrivate(&(pClient)->devPrivates, DRI2ClientEventsPrivateKey))
> +
> +static int
> +ListAddDRI2ClientEvents(ClientPtr client, struct list *entry)
> +{
> + DRI2ClientEventsPtr pClientPriv;
> + pClientPriv = GetDRI2ClientEvents(client);
> +
> + if (!pClientPriv) {
> + return BadAlloc;
> + }
> +
> + list_add(entry, &pClientPriv->reference_list);
> + return 0;
> +}
> +
> +static void
> +ListDelDRI2ClientEvents(ClientPtr client, struct list *entry)
> +{
> + DRI2ClientEventsPtr pClientPriv;
> + pClientPriv = GetDRI2ClientEvents(client);
> +
> + if (!pClientPriv) {
> + return;
> + }
> + list_del(entry);
> +}
> +
> +static void
> +radeon_dri2_client_state_changed(CallbackListPtr *ClientStateCallback, pointer data, pointer calldata)
> +{
> + DRI2ClientEventsPtr pClientEventsPriv;
> + DRI2FrameEventPtr ref;
> + NewClientInfoRec *clientinfo = calldata;
> + ClientPtr pClient = clientinfo->client;
> + pClientEventsPriv = GetDRI2ClientEvents(pClient);
> +
> + switch (pClient->clientState) {
> + case ClientStateInitial:
> + list_init(&pClientEventsPriv->reference_list);
> + break;
> + case ClientStateRunning:
> + break;
> +
> + case ClientStateRetained:
> + case ClientStateGone:
> + if (pClientEventsPriv) {
> + list_for_each_entry(ref, &pClientEventsPriv->reference_list, link) {
> + ref->valid = FALSE;
> + }
> + }
> + break;
> + default:
> + break;
> + }
> +}
> +
> static void
> radeon_dri2_ref_buffer(BufferPtr buffer)
> {
> @@ -416,28 +494,20 @@ void radeon_dri2_frame_event_handler(unsigned int frame, unsigned int tv_sec,
> BoxRec box;
> RegionRec region;
>
> + if (!event->valid) {
> + goto cleanup;
> + }
> +
> status = dixLookupDrawable(&drawable, event->drawable_id, serverClient,
> M_ANY, DixWriteAccess);
> if (status != Success) {
> - radeon_dri2_unref_buffer(event->front);
> - radeon_dri2_unref_buffer(event->back);
> - free(event);
> + goto cleanup;
> return;
You should remove the unreachable return here.
> }
>
> screen = drawable->pScreen;
> scrn = xf86Screens[screen->myNum];
> -
> - /* event->client may have quit between submitting a request
> - * and this callback being triggered.
> - *
> - * Check our saved client pointer against the client in the saved client
> - * slot. This will catch almost all cases where the client that requested
> - * SwapBuffers has gone away, and will guarantee that there is at least a
> - * valid client to write the BufferSwapComplete event to.
> - */
> - client = event->client == clients[event->client_index] ?
> - event->client : NULL;
> + client = event->client;
>
> switch (event->type) {
> case DRI2_FLIP:
> @@ -453,8 +523,6 @@ void radeon_dri2_frame_event_handler(unsigned int frame, unsigned int tv_sec,
> DRI2SwapComplete(client, drawable, frame, tv_sec, tv_usec,
> swap_type, event->event_complete, event->event_data);
>
> - radeon_dri2_unref_buffer(event->front);
> - radeon_dri2_unref_buffer(event->back);
> break;
> case DRI2_WAITMSC:
> DRI2WaitMSCComplete(client, drawable, frame, tv_sec, tv_usec);
> @@ -466,6 +534,11 @@ void radeon_dri2_frame_event_handler(unsigned int frame, unsigned int tv_sec,
> break;
> }
>
> +cleanup:
> + radeon_dri2_unref_buffer(event->front);
> + radeon_dri2_unref_buffer(event->back);
> + if (event->valid)
> + ListDelDRI2ClientEvents(event->client, &event->link);
> free(event);
> }
>
> @@ -539,7 +612,7 @@ static int radeon_dri2_schedule_wait_msc(ClientPtr client, DrawablePtr draw,
> ScreenPtr screen = draw->pScreen;
> ScrnInfoPtr scrn = xf86Screens[screen->myNum];
> RADEONInfoPtr info = RADEONPTR(scrn);
> - DRI2FrameEventPtr wait_info;
> + DRI2FrameEventPtr wait_info = NULL;
> drmVBlank vbl;
> int ret, crtc = radeon_dri2_drawable_crtc(draw);
> CARD64 current_msc;
> @@ -560,8 +633,14 @@ static int radeon_dri2_schedule_wait_msc(ClientPtr client, DrawablePtr draw,
>
> wait_info->drawable_id = draw->id;
> wait_info->client = client;
> - wait_info->client_index = client->index;
> wait_info->type = DRI2_WAITMSC;
> + wait_info->valid = TRUE;
> +
> + if (ListAddDRI2ClientEvents(client, &wait_info->link)) {
> + xf86DrvMsg(scrn->scrnIndex, X_WARNING,
> + "add events to client private failed.\n");
> + goto out_complete;
> + }
>
> /* Get current count */
> vbl.request.type = DRM_VBLANK_RELATIVE;
> @@ -642,6 +721,10 @@ static int radeon_dri2_schedule_wait_msc(ClientPtr client, DrawablePtr draw,
> return TRUE;
>
> out_complete:
> + if (wait_info) {
> + ListDelDRI2ClientEvents(wait_info->client, &wait_info->link);
> + free(wait_info);
> + }
> DRI2WaitMSCComplete(client, draw, target_msc, 0, 0);
> return TRUE;
> }
> @@ -704,11 +787,16 @@ static int radeon_dri2_schedule_swap(ClientPtr client, DrawablePtr draw,
>
> swap_info->drawable_id = draw->id;
> swap_info->client = client;
> - swap_info->client_index = client->index;
> swap_info->event_complete = func;
> swap_info->event_data = data;
> swap_info->front = front;
> swap_info->back = back;
> + swap_info->valid = TRUE;
> + if (ListAddDRI2ClientEvents(client, &swap_info->link)) {
> + xf86DrvMsg(scrn->scrnIndex, X_WARNING,
> + "add events to client private failed.\n");
> + goto blit_fallback;
> + }
>
> /* Get current count */
> vbl.request.type = DRM_VBLANK_RELATIVE;
> @@ -831,8 +919,10 @@ blit_fallback:
> radeon_dri2_copy_region(draw, ®ion, front, back);
>
> DRI2SwapComplete(client, draw, 0, 0, 0, DRI2_BLIT_COMPLETE, func, data);
> - if (swap_info)
> + if (swap_info) {
> + ListDelDRI2ClientEvents(swap_info->client, &swap_info->link);
> free(swap_info);
> + }
>
> radeon_dri2_unref_buffer(front);
> radeon_dri2_unref_buffer(back);
> @@ -895,6 +985,20 @@ radeon_dri2_screen_init(ScreenPtr pScreen)
> } else {
> xf86DrvMsg(pScrn->scrnIndex, X_WARNING, "You need a newer kernel for sync extension\n");
> }
> +
> +#if HAS_DIXREGISTERPRIVATEKEY
> + if (!dixRegisterPrivateKey(DRI2ClientEventsPrivateKey, PRIVATE_CLIENT, sizeof(DRI2ClientEventsRec))) {
> + xf86DrvMsg(pScrn->scrnIndex, X_WARNING, "DRI2 registering private key to client failed\n");
> + return FALSE;
> + }
> +#else
> + if (!dixRequestPrivate(DRI2ClientEventsPrivateKey, sizeof(DRI2ClientEventsRec))) {
> + xf86DrvMsg(pScrn->scrnIndex, X_WARNING, "DRI2 requesting private key to client failed\n");
> + return FALSE;
> + }
> +#endif
> +
> + AddCallback(&ClientStateCallback, radeon_dri2_client_state_changed, 0);
> #endif
>
> info->dri2.enabled = DRI2ScreenInit(pScreen, &dri2_info);
> @@ -906,6 +1010,9 @@ void radeon_dri2_close_screen(ScreenPtr pScreen)
> ScrnInfoPtr pScrn = xf86Screens[pScreen->myNum];
> RADEONInfoPtr info = RADEONPTR(pScrn);
>
> +#if DRI2INFOREC_VERSION >= 4
> + DeleteCallback(&ClientStateCallback, radeon_dri2_client_state_changed, 0);
> +#endif
> DRI2CloseScreen(pScreen);
> drmFree(info->dri2.device_name);
> }
Apart from the above, this looks right to me, although I can't test it
at the moment.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 490 bytes
Desc: This is a digitally signed message part
URL: <http://lists.x.org/archives/xorg-driver-ati/attachments/20100918/1bad9107/attachment.pgp>
More information about the xorg-driver-ati
mailing list