[PATCH v3 xserver] xwayland: Don't block in DRI2 authentication mechanism
Kristian Høgsberg
hoegsberg at gmail.com
Wed Feb 6 13:56:09 PST 2013
On Wed, Feb 06, 2013 at 03:09:18PM +0200, Tiago Vignatti wrote:
> Signed-off-by: Tiago Vignatti <tiago.vignatti at intel.com>
> ---
> v2: - added AuthMagic3, for passing ClientPtr
> - fixed server internal ABI issue, by maintaining DRI2Authenticate intact
> - fixed multiple clients accesses to DRI2 authenticate, using Wayland
> callback mechanism to ensure they all go atomically
>
> v3: - fixed AuthMagic and LegacyAuthMagic semantics when adding the new
> DRI2AuthMagic3ProcPtr type
> - unexported DRI2Authenticate
> - fixed DRI2SendAuthReply to be more strict about its args ("Bool status")
> - changed the new nonblocking dri2 authentication to process only one
> request at time
Ok, this looks good now, there's just one thing left below...
> hw/xfree86/dri2/dri2.c | 18 ++++++----
> hw/xfree86/dri2/dri2.h | 17 ++++++----
> hw/xfree86/dri2/dri2ext.c | 25 ++++++++++----
> hw/xfree86/xwayland/xwayland-drm.c | 58 +++++++++++++++++++++++++++-----
> hw/xfree86/xwayland/xwayland-private.h | 1 +
> hw/xfree86/xwayland/xwayland.c | 2 ++
> hw/xfree86/xwayland/xwayland.h | 2 +-
> 7 files changed, 93 insertions(+), 30 deletions(-)
>
> diff --git a/hw/xfree86/dri2/dri2.c b/hw/xfree86/dri2/dri2.c
> index b08618a..142619c 100644
> --- a/hw/xfree86/dri2/dri2.c
> +++ b/hw/xfree86/dri2/dri2.c
> @@ -106,8 +106,9 @@ typedef struct _DRI2Screen {
> DRI2ScheduleSwapProcPtr ScheduleSwap;
> DRI2GetMSCProcPtr GetMSC;
> DRI2ScheduleWaitMSCProcPtr ScheduleWaitMSC;
> - DRI2AuthMagic2ProcPtr AuthMagic;
> DRI2AuthMagicProcPtr LegacyAuthMagic;
> + DRI2AuthMagic2ProcPtr LegacyAuthMagic2;
> + DRI2AuthMagic3ProcPtr AuthMagic;
> DRI2ReuseBufferNotifyProcPtr ReuseBufferNotify;
> DRI2SwapLimitValidateProcPtr SwapLimitValidate;
>
> @@ -1123,11 +1124,11 @@ DRI2AuthMagic (ScreenPtr pScreen, uint32_t magic)
> }
>
> Bool
> -DRI2Authenticate(ScreenPtr pScreen, uint32_t magic)
> +DRI2Authenticate(ClientPtr client, ScreenPtr pScreen, uint32_t magic)
> {
> DRI2ScreenPtr ds = DRI2GetScreen(pScreen);
>
> - if (ds == NULL || (*ds->AuthMagic) (pScreen, magic))
> + if (ds == NULL || (*ds->AuthMagic) (client, pScreen, magic))
> return FALSE;
>
> return TRUE;
> @@ -1222,12 +1223,15 @@ DRI2ScreenInit(ScreenPtr pScreen, DRI2InfoPtr info)
> *
> * If a driver is built without xwayland support, we'll die before we get
> * here. If a driver is built with xwayland support, it'll support
> - * AuthMagic2, or crash; we don't care about xwayland ABI yet.
> + * AuthMagic3, or crash; we don't care about xwayland ABI yet.
> */
> if (xorgWayland) {
> - ds->AuthMagic = info->AuthMagic2;
> + ds->AuthMagic = info->AuthMagic3;
> + }
> +
> + if (info->version >= 8) {
> + ds->LegacyAuthMagic2 = info->AuthMagic2;
> }
> -
> if (info->version >= 5) {
> ds->LegacyAuthMagic = info->AuthMagic;
> }
> @@ -1247,7 +1251,7 @@ DRI2ScreenInit(ScreenPtr pScreen, DRI2InfoPtr info)
> * If the driver doesn't provide an AuthMagic function
> * it relies on the old method (using libdrm) or fails
> */
> - if (!ds->LegacyAuthMagic)
> + if (!ds->LegacyAuthMagic2 && !ds->LegacyAuthMagic)
> #ifdef WITH_LIBDRM
> ds->LegacyAuthMagic = drmAuthMagic;
> #else
> diff --git a/hw/xfree86/dri2/dri2.h b/hw/xfree86/dri2/dri2.h
> index cec9634..617cbfb 100644
> --- a/hw/xfree86/dri2/dri2.h
> +++ b/hw/xfree86/dri2/dri2.h
> @@ -65,6 +65,8 @@ typedef void (*DRI2CopyRegionProcPtr) (DrawablePtr pDraw,
> typedef void (*DRI2WaitProcPtr) (WindowPtr pWin, unsigned int sequence);
> typedef int (*DRI2AuthMagicProcPtr) (int fd, uint32_t magic);
> typedef int (*DRI2AuthMagic2ProcPtr) (ScreenPtr pScreen, uint32_t magic);
> +typedef int (*DRI2AuthMagic3ProcPtr) (ClientPtr client,
> + ScreenPtr pScreen, uint32_t magic);
>
> /**
> * Schedule a buffer swap
> @@ -213,15 +215,14 @@ typedef struct {
> DRI2ReuseBufferNotifyProcPtr ReuseBufferNotify;
> DRI2SwapLimitValidateProcPtr SwapLimitValidate;
>
> - /* added in version 8 AND in xwayland branch */
> - /*
> - * MERGE NOTE: when merging xwayland to master, the commit adding this
> - * is unnecessary, but it's slightly different in master.
> - */
> + /* added in version 8 */
> /* AuthMagic callback which passes extra context */
> /* If this is NULL the AuthMagic callback is used */
> /* If this is non-NULL the AuthMagic callback is ignored */
> DRI2AuthMagic2ProcPtr AuthMagic2;
> +
> + /* MERGE NOTE: added in xwayland branch */
> + DRI2AuthMagic3ProcPtr AuthMagic3;
> } DRI2InfoRec, *DRI2InfoPtr;
>
> extern _X_EXPORT int DRI2EventBase;
> @@ -238,7 +239,11 @@ extern _X_EXPORT Bool DRI2Connect(ScreenPtr pScreen,
> const char **driverName,
> const char **deviceName);
>
> -extern _X_EXPORT Bool DRI2Authenticate(ScreenPtr pScreen, uint32_t magic);
> +extern Bool DRI2Authenticate(ClientPtr client,
> + ScreenPtr pScreen,
> + uint32_t magic);
> +
> +extern _X_EXPORT void DRI2SendAuthReply(ClientPtr client, Bool status);
>
> extern _X_EXPORT int DRI2CreateDrawable(ClientPtr client,
> DrawablePtr pDraw,
> diff --git a/hw/xfree86/dri2/dri2ext.c b/hw/xfree86/dri2/dri2ext.c
> index 2579a5c..246f55e 100644
> --- a/hw/xfree86/dri2/dri2ext.c
> +++ b/hw/xfree86/dri2/dri2ext.c
> @@ -133,25 +133,36 @@ ProcDRI2Connect(ClientPtr client)
> return Success;
> }
>
> +void
> +DRI2SendAuthReply(ClientPtr client, Bool status)
> +{
> + xDRI2AuthenticateReply rep;
> +
> + REQUEST_SIZE_MATCH(xDRI2AuthenticateReq);
> + rep.type = X_Reply;
> + rep.sequenceNumber = client->sequence;
> + rep.length = 0;
> + rep.authenticated = status;
> +
> + WriteToClient(client, sizeof(xDRI2AuthenticateReply), &rep);
> +}
> +
> static int
> ProcDRI2Authenticate(ClientPtr client)
> {
> REQUEST(xDRI2AuthenticateReq);
> - xDRI2AuthenticateReply rep;
> DrawablePtr pDraw;
> int status;
>
> - REQUEST_SIZE_MATCH(xDRI2AuthenticateReq);
> if (!validDrawable(client, stuff->window, DixGetAttrAccess,
> &pDraw, &status))
> return status;
>
> - rep.type = X_Reply;
> - rep.sequenceNumber = client->sequence;
> - rep.length = 0;
> - rep.authenticated = DRI2Authenticate(pDraw->pScreen, stuff->magic);
> - WriteToClient(client, sizeof(xDRI2AuthenticateReply), &rep);
> + if (DRI2Authenticate(client, pDraw->pScreen, stuff->magic))
> + return Success;
For this to work with non-xwayland, we can't just assume that a TRUE
return from DRI2Authenticate means "non-blocking auth in progress,
don't send reply". What I suggested was that we can look at
client->blockCount after returning from DRI2Authenticate. If it's > 0
(the auth handler called IgnoreClient) we don't send the reply,
otherwise we send the reply with the return value of DRI2Authenticate.
That way it works like before for non-xwayland, and in xwayland we
just need to call IgnoreClient to delay the reply.
> + /* BadAccess or BadDrawable */
> + DRI2SendAuthReply(client, FALSE);
> return Success;
> }
>
> diff --git a/hw/xfree86/xwayland/xwayland-drm.c b/hw/xfree86/xwayland/xwayland-drm.c
> index c19eb84..26a5448 100644
> --- a/hw/xfree86/xwayland/xwayland-drm.c
> +++ b/hw/xfree86/xwayland/xwayland-drm.c
> @@ -45,6 +45,15 @@
>
> #include "xwayland.h"
> #include "xwayland-private.h"
> +#include "../dri2/dri2.h"
> +
> +struct xwl_auth_req {
> + struct xorg_list link;
> +
> + ClientPtr client;
> + struct xwl_screen *xwl_screen;
> + uint32_t magic;
> +};
>
> static void
> drm_handle_device (void *data, struct wl_drm *drm, const char *device)
> @@ -63,8 +72,31 @@ static void
> drm_handle_authenticated (void *data, struct wl_drm *drm)
> {
> struct xwl_screen *xwl_screen = data;
> + struct xwl_auth_req *req;
>
> xwl_screen->authenticated = 1;
> +
> + /* it does one authentication transaction at a time, so if there's an
> + * element in the list, we call DRI2SendAuthReply for that client, remove
> + * the head and free the struct. If there are still elements in the list,
> + * it means that we have one or more clients waiting to be authenticated
> + * and we send out a wl_drm authenticate request for the first client in
> + * the list */
> + if (xorg_list_is_empty(&xwl_screen->authenticate_client_list))
> + return;
> +
> + req = xorg_list_first_entry(&xwl_screen->authenticate_client_list,
> + struct xwl_auth_req, link);
> + DRI2SendAuthReply(req->client, TRUE);
> + AttendClient(req->client);
> + xorg_list_del(&req->link);
> + free(req);
> +
> + xorg_list_for_each_entry(req, &xwl_screen->authenticate_client_list,
> + link) {
> + wl_drm_authenticate (xwl_screen->drm, req->magic);
> + return;
> + }
> }
>
> static const struct wl_drm_listener xwl_drm_listener =
> @@ -143,21 +175,29 @@ int xwl_screen_get_drm_fd(struct xwl_screen *xwl_screen)
> return xwl_screen->drm_fd;
> }
>
> -int xwl_drm_authenticate(struct xwl_screen *xwl_screen,
> +int xwl_drm_authenticate(ClientPtr client, struct xwl_screen *xwl_screen,
> uint32_t magic)
> {
> - int ret;
> + struct xwl_auth_req *req;
>
> - xwl_screen->authenticated = 0;
> + if (!xwl_screen->drm)
> + return BadAccess;
>
> - if (xwl_screen->drm)
> + req = malloc (sizeof *req);
> + if (req == NULL)
> + return BadAlloc;
> +
> + req->client = client;
> + req->xwl_screen = xwl_screen;
> + req->magic = magic;
> +
> + if (xorg_list_is_empty(&xwl_screen->authenticate_client_list))
> wl_drm_authenticate (xwl_screen->drm, magic);
>
> - ret = wl_display_roundtrip(xwl_screen->display);
> - if (ret == -1)
> - return BadAlloc;
> - if (!xwl_screen->authenticated)
> - return BadAlloc;
> + xorg_list_add(&req->link, &xwl_screen->authenticate_client_list);
> +
> + IgnoreClient(req->client);
> + xwl_screen->authenticated = 0;
>
> return Success;
> }
> diff --git a/hw/xfree86/xwayland/xwayland-private.h b/hw/xfree86/xwayland/xwayland-private.h
> index 23263d0..5b602d7 100644
> --- a/hw/xfree86/xwayland/xwayland-private.h
> +++ b/hw/xfree86/xwayland/xwayland-private.h
> @@ -61,6 +61,7 @@ struct xwl_screen {
> struct xorg_list seat_list;
> struct xorg_list damage_window_list;
> struct xorg_list window_list;
> + struct xorg_list authenticate_client_list;
> uint32_t serial;
>
> CreateWindowProcPtr CreateWindow;
> diff --git a/hw/xfree86/xwayland/xwayland.c b/hw/xfree86/xwayland/xwayland.c
> index 98e036e..d97f4ee 100644
> --- a/hw/xfree86/xwayland/xwayland.c
> +++ b/hw/xfree86/xwayland/xwayland.c
> @@ -233,6 +233,7 @@ xwl_screen_pre_init(ScrnInfoPtr scrninfo, struct xwl_screen *xwl_screen,
> xorg_list_init(&xwl_screen->seat_list);
> xorg_list_init(&xwl_screen->damage_window_list);
> xorg_list_init(&xwl_screen->window_list);
> + xorg_list_init(&xwl_screen->authenticate_client_list);
> xwl_screen->scrninfo = scrninfo;
> xwl_screen->driver = driver;
> xwl_screen->flags = flags;
> @@ -307,6 +308,7 @@ void xwl_screen_close(struct xwl_screen *xwl_screen)
> xorg_list_init(&xwl_screen->seat_list);
> xorg_list_init(&xwl_screen->damage_window_list);
> xorg_list_init(&xwl_screen->window_list);
> + xorg_list_init(&xwl_screen->authenticate_client_list);
>
> wl_display_roundtrip(xwl_screen->display);
> }
> diff --git a/hw/xfree86/xwayland/xwayland.h b/hw/xfree86/xwayland/xwayland.h
> index e536c42..f268366 100644
> --- a/hw/xfree86/xwayland/xwayland.h
> +++ b/hw/xfree86/xwayland/xwayland.h
> @@ -69,7 +69,7 @@ extern _X_EXPORT void
> xwl_screen_post_damage(struct xwl_screen *xwl_screen);
>
> extern _X_EXPORT int
> -xwl_drm_authenticate(struct xwl_screen *xwl_screen,
> +xwl_drm_authenticate(ClientPtr client, struct xwl_screen *xwl_screen,
> uint32_t magic);
>
> extern _X_EXPORT int
> --
> 1.7.9.5
>
> _______________________________________________
> wayland-devel mailing list
> wayland-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel
More information about the wayland-devel
mailing list