From mboxrd@z Thu Jan 1 00:00:00 1970 From: Eric Anholt Subject: Re: [PATCH 8/8] Add DRI3+Present loader Date: Tue, 05 Nov 2013 15:10:11 -0800 Message-ID: <87vc06fnsc.fsf@eliezer.anholt.net> References: <1383618208-21310-1-git-send-email-keithp@keithp.com> <1383618208-21310-9-git-send-email-keithp@keithp.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============1266618964==" Return-path: In-Reply-To: <1383618208-21310-9-git-send-email-keithp@keithp.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: dri-devel-bounces@lists.freedesktop.org Errors-To: dri-devel-bounces@lists.freedesktop.org To: Keith Packard , mesa3d-dev@lists.freedesktop.org Cc: dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org --===============1266618964== Content-Type: multipart/signed; boundary="=-=-="; micalg=pgp-sha256; protocol="application/pgp-signature" --=-=-= Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable I think I'm going to be griping about code duplication... Keith Packard writes: > diff --git a/configure.ac b/configure.ac > index 0a25047..074368c 100644 > --- a/configure.ac > +++ b/configure.ac > @@ -38,6 +38,9 @@ LIBDRM_NVVIEUX_REQUIRED=3D2.4.33 > LIBDRM_NOUVEAU_REQUIRED=3D"2.4.33 libdrm >=3D 2.4.41" > LIBDRM_FREEDRENO_REQUIRED=3D2.4.39 > DRI2PROTO_REQUIRED=3D2.6 > +DRI3PROTO_REQUIRED=3D1.0 > +PRESENTPROTO_REQUIRED=3D1.0 > +LIBUDEV_REQUIRED=3D151 > GLPROTO_REQUIRED=3D1.4.14 > LIBDRM_XORG_REQUIRED=3D2.4.24 > LIBKMS_XORG_REQUIRED=3D1.0.0 > @@ -820,10 +823,13 @@ xyesno) > fi > PKG_CHECK_MODULES([DRI2PROTO], [dri2proto >=3D $DRI2PROTO_REQUIR= ED]) > GL_PC_REQ_PRIV=3D"$GL_PC_REQ_PRIV libdrm >=3D $LIBDRM_REQUIRED" > + PKG_CHECK_MODULES([DRI3PROTO], [dri3proto >=3D $DRI3PROTO_REQUIR= ED]) > + PKG_CHECK_MODULES([PRESENTPROTO], [presentproto >=3D $PRESENTPRO= TO_REQUIRED]) > + PKG_CHECK_MODULES([LIBUDEV], [libudev >=3D $LIBUDEV_REQUIRED]) > fi >=20=20 > # find the DRI deps for libGL > - dri_modules=3D"x11 xext xdamage xfixes x11-xcb xcb-glx >=3D 1.8.1 xc= b-dri2 >=3D 1.8" > + dri_modules=3D"x11 xext xdamage xfixes x11-xcb xcb-glx >=3D 1.8.1 xc= b-dri2 >=3D 1.8 xcb-dri3 xcb-present xcb-sync xshmfence" Patches need to land in XCB and get released before this can land. I don't even see patches on the xcb list yet. > diff --git a/src/glx/dri3_common.c b/src/glx/dri3_common.c > new file mode 100644 > index 0000000..c758f96 > --- /dev/null > +++ b/src/glx/dri3_common.c > @@ -0,0 +1,146 @@ > +/* > + * Copyright =C2=A9 2013 Keith Packard > + * > + * Permission to use, copy, modify, distribute, and sell this software a= nd its > + * documentation for any purpose is hereby granted without fee, provided= that > + * the above copyright notice appear in all copies and that both that co= pyright > + * notice and this permission notice appear in supporting documentation,= and > + * that the name of the copyright holders not be used in advertising or > + * publicity pertaining to distribution of the software without specific, > + * written prior permission. The copyright holders make no representati= ons > + * about the suitability of this software for any purpose. It is provid= ed "as > + * is" without express or implied warranty. > + * > + * THE COPYRIGHT HOLDERS DISCLAIM ALL WARRANTIES WITH REGARD TO THIS SOF= TWARE, > + * INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS, IN NO > + * EVENT SHALL THE COPYRIGHT HOLDERS BE LIABLE FOR ANY SPECIAL, INDIRECT= OR > + * CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM LOSS O= F USE, > + * DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE OR OTHER > + * TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR PERF= ORMANCE > + * OF THIS SOFTWARE. > + */ > + > +/* > + * This code is derived from src/egl/drivers/dri2/common.c which > + * carries the following copyright: > + *=20 > + * Copyright =C2=A9 2011 Intel Corporation > + * > + * Permission is hereby granted, free of charge, to any person obtaining= a > + * copy of this software and associated documentation files (the "Softwa= re"), > + * to deal in the Software without restriction, including without limita= tion > + * the rights to use, copy, modify, merge, publish, distribute, sublicen= se, > + * and/or sell copies of the Software, and to permit persons to whom the > + * Software is furnished to do so, subject to the following conditions: > + * > + * The above copyright notice and this permission notice (including the = next > + * paragraph) shall be included in all copies or substantial portions of= the > + * Software. > + * > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, > + * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF > + * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND > + * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT > + * HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, > + * WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, > + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER > + * DEALINGS IN THE SOFTWARE. > + * > + * Authors: > + * Kristian H=C3=B8gsberg > + * Benjamin Franzke > + */ > + > +#include > +#include > + > +#include > +#include > +#include > +#include > +#include "glapi.h" > +#include "glxclient.h" > +#include "xf86dri.h" > +#include > +#include > +#include > +#include > +#include > +#include > +#include "xf86drm.h" > +#include "dri_common.h" > +#include "dri3_priv.h" > + > +#define DRIVER_MAP_DRI3_ONLY What does this define do? > diff --git a/src/glx/dri3_glx.c b/src/glx/dri3_glx.c > new file mode 100644 > index 0000000..4021baa > --- /dev/null > +++ b/src/glx/dri3_glx.c > @@ -0,0 +1,1722 @@ > +static inline void > +dri3_fence_reset(xcb_connection_t *c, struct dri3_buffer *buffer) { > + xshmfence_reset(buffer->shm_fence); > +} > + > +static inline void > +dri3_fence_set(struct dri3_buffer *buffer) { > + xshmfence_trigger(buffer->shm_fence); > +} > + > +static inline void > +dri3_fence_trigger(xcb_connection_t *c, struct dri3_buffer *buffer) { > + xcb_sync_trigger_fence(c, buffer->sync_fence); > +} > + > +static inline void > +dri3_fence_await(xcb_connection_t *c, struct dri3_buffer *buffer) { > + xcb_flush(c); > + xshmfence_await(buffer->shm_fence); > +} > + > +static inline Bool > +dri3_fence_triggered(struct dri3_buffer *buffer) { > + return xshmfence_query(buffer->shm_fence); > +} '{' on a separate line, please. > +static void > +dri3_destroy_context(struct glx_context *context) > +{ > + struct dri3_context *pcp =3D (struct dri3_context *) context; > + struct dri3_screen *psc =3D (struct dri3_screen *) context->psc; > + > + driReleaseDrawables(&pcp->base); > + > + free((char *) context->extensions); > + > + (*psc->core->destroyContext) (pcp->driContext); > + > + free(pcp); > +} > + > +static Bool > +dri3_bind_context(struct glx_context *context, struct glx_context *old, > + GLXDrawable draw, GLXDrawable read) > +{ > + struct dri3_context *pcp =3D (struct dri3_context *) context; > + struct dri3_screen *psc =3D (struct dri3_screen *) pcp->base.psc; > + struct dri3_drawable *pdraw, *pread; > + > + pdraw =3D (struct dri3_drawable *) driFetchDrawable(context, draw); > + pread =3D (struct dri3_drawable *) driFetchDrawable(context, read); > + > + driReleaseDrawables(&pcp->base); > + > + if (pdraw =3D=3D NULL || pread =3D=3D NULL) > + return GLXBadDrawable; > + > + if (!(*psc->core->bindContext) (pcp->driContext, > + pdraw->driDrawable, pread->driDrawable)) > + return GLXBadContext; > + > + return Success; > +} > + > +static void > +dri3_unbind_context(struct glx_context *context, struct glx_context *new) > +{ > + struct dri3_context *pcp =3D (struct dri3_context *) context; > + struct dri3_screen *psc =3D (struct dri3_screen *) pcp->base.psc; > + > + (*psc->core->unbindContext) (pcp->driContext); > +} > + > +static struct glx_context * > +dri3_create_context(struct glx_screen *base, > + struct glx_config *config_base, > + struct glx_context *shareList, int renderType) > +{ > + struct dri3_context *pcp, *pcp_shared; > + struct dri3_screen *psc =3D (struct dri3_screen *) base; > + __GLXDRIconfigPrivate *config =3D (__GLXDRIconfigPrivate *) config_ba= se; > + __DRIcontext *shared =3D NULL; > + > + if (shareList) { > + /* If the shareList context is not a DRI3 context, we cannot possi= bly > + * create a DRI3 context that shares it. > + */ > + if (shareList->vtable->destroy !=3D dri3_destroy_context) { > + return NULL; > + } > + > + pcp_shared =3D (struct dri3_context *) shareList; > + shared =3D pcp_shared->driContext; > + } > + > + pcp =3D calloc(1, sizeof *pcp); > + if (pcp =3D=3D NULL) > + return NULL; > + > + if (!glx_context_init(&pcp->base, &psc->base, &config->base)) { > + free(pcp); > + return NULL; > + } > + > + pcp->driContext =3D > + (*psc->image_driver->createNewContext) (psc->driScreen, > + config->driConfig, shared,= pcp); > + > + if (pcp->driContext =3D=3D NULL) { > + free(pcp); > + return NULL; > + } > + > + pcp->base.vtable =3D &dri3_context_vtable; > + > + return &pcp->base; > +} This looks completely like dri2_create_context, except for missing rendertype validation and a different calloc size. > +static struct glx_context * > +dri3_create_context_attribs(struct glx_screen *base, > + struct glx_config *config_base, > + struct glx_context *shareList, > + unsigned num_attribs, > + const uint32_t *attribs, > + unsigned *error) > +{ > + struct dri3_context *pcp =3D NULL; > + struct dri3_context *pcp_shared =3D NULL; > + struct dri3_screen *psc =3D (struct dri3_screen *) base; > + __GLXDRIconfigPrivate *config =3D (__GLXDRIconfigPrivate *) config_ba= se; > + __DRIcontext *shared =3D NULL; > + > + uint32_t minor_ver =3D 1; > + uint32_t major_ver =3D 2; > + uint32_t flags =3D 0; > + unsigned api; > + int reset =3D __DRI_CTX_RESET_NO_NOTIFICATION; > + uint32_t ctx_attribs[2 * 5]; > + unsigned num_ctx_attribs =3D 0; > + uint32_t render_type; > + > + /* Remap the GLX tokens to DRI2 tokens. > + */ > + if (!dri2_convert_glx_attribs(num_attribs, attribs, > + &major_ver, &minor_ver, tabs :( > + &render_type, &flags, &api, > + &reset, error)) > + goto error_exit; > + > + /* Check the renderType value */ > + if (!validate_renderType_against_config(config_base, render_type)) > + goto error_exit; > + > + if (shareList) { > + pcp_shared =3D (struct dri3_context *) shareList; > + shared =3D pcp_shared->driContext; > + } > + > + pcp =3D calloc(1, sizeof *pcp); > + if (pcp =3D=3D NULL) { > + *error =3D __DRI_CTX_ERROR_NO_MEMORY; > + goto error_exit; > + } > + > + if (!glx_context_init(&pcp->base, &psc->base, &config->base)) > + goto error_exit; > + > + ctx_attribs[num_ctx_attribs++] =3D __DRI_CTX_ATTRIB_MAJOR_VERSION; > + ctx_attribs[num_ctx_attribs++] =3D major_ver; > + ctx_attribs[num_ctx_attribs++] =3D __DRI_CTX_ATTRIB_MINOR_VERSION; > + ctx_attribs[num_ctx_attribs++] =3D minor_ver; > + > + /* Only send a value when the non-default value is requested. By doi= ng > + * this we don't have to check the driver's DRI3 version before sendi= ng the > + * default value. > + */ > + if (reset !=3D __DRI_CTX_RESET_NO_NOTIFICATION) { > + ctx_attribs[num_ctx_attribs++] =3D __DRI_CTX_ATTRIB_RESET_STRATEGY; > + ctx_attribs[num_ctx_attribs++] =3D reset; > + } > + > + if (flags !=3D 0) { > + ctx_attribs[num_ctx_attribs++] =3D __DRI_CTX_ATTRIB_FLAGS; > + > + /* The current __DRI_CTX_FLAG_* values are identical to the > + * GLX_CONTEXT_*_BIT values. > + */ > + ctx_attribs[num_ctx_attribs++] =3D flags; > + } > + > + pcp->driContext =3D > + (*psc->image_driver->createContextAttribs) (psc->driScreen, > + api, > + config->driConfig, > + shared, > + num_ctx_attribs / 2, > + ctx_attribs, > + error, > + pcp); > + > + if (pcp->driContext =3D=3D NULL) > + goto error_exit; > + > + pcp->base.vtable =3D &dri3_context_vtable; > + > + return &pcp->base; > + > +error_exit: > + free(pcp); > + > + return NULL; > +} This looks like an exact copy of dri2_create_context_attribs except for the vtable, the calloc size being different, the reset initialization, and the createContextAttribs looking in image_driver instead of dri2. This sucks. > + > +static __GLXDRIdrawable * > +dri3_create_drawable(struct glx_screen *base, XID xDrawable, > + GLXDrawable drawable, struct glx_config *config_bas= e) > +{ > + struct dri3_drawable *pdraw; > + struct dri3_screen *psc =3D (struct dri3_screen *) base; > + __GLXDRIconfigPrivate *config =3D (__GLXDRIconfigPrivate *) config_ba= se; > + GLint vblank_mode =3D DRI_CONF_VBLANK_DEF_INTERVAL_1; > + > + pdraw =3D calloc(1, sizeof(*pdraw)); > + if (!pdraw) > + return NULL; > + > + pdraw->base.destroyDrawable =3D dri3_destroy_drawable; > + pdraw->base.xDrawable =3D xDrawable; > + pdraw->base.drawable =3D drawable; > + pdraw->base.psc =3D &psc->base; > +// pdraw->bufferCount =3D 0; Leftover debug code? > + pdraw->swap_interval =3D 1; /* default may be overridden below */ > + pdraw->have_back =3D 0; > + pdraw->have_fake_front =3D 0; > + > + if (psc->config) > + psc->config->configQueryi(psc->driScreen, > + "vblank_mode", &vblank_mode); > + > + switch (vblank_mode) { > + case DRI_CONF_VBLANK_NEVER: > + case DRI_CONF_VBLANK_DEF_INTERVAL_0: > + pdraw->swap_interval =3D 0; > + break; > + case DRI_CONF_VBLANK_DEF_INTERVAL_1: > + case DRI_CONF_VBLANK_ALWAYS_SYNC: > + default: > + pdraw->swap_interval =3D 1; > + break; > + } > + > + (void) __glXInitialize(psc->base.dpy); > + > + /* Create a new drawable */ > + pdraw->driDrawable =3D > + (*psc->image_driver->createNewDrawable) (psc->driScreen, > + config->driConfig, pdraw); > + > + if (!pdraw->driDrawable) { > + free(pdraw); > + return NULL; > + } > + > + /* > + * Make sure server has the same swap interval we do for the new > + * drawable. > + */ > + if (psc->vtable.setSwapInterval) > + psc->vtable.setSwapInterval(&pdraw->base, pdraw->swap_interval); > + > + return &pdraw->base; > +} Finally, a function different enough that I think it merits being a new implementation :) > +static int > +dri3_wait_for_msc(__GLXDRIdrawable *pdraw, int64_t target_msc, int64_t d= ivisor, > + int64_t remainder, int64_t *ust, int64_t *msc, int64_t= *sbc) > +{ > + xcb_connection_t *c =3D XGetXCBConnection(pdraw->psc->dpy); > + struct dri3_drawable *priv =3D (struct dri3_drawable *) pdraw; > + xcb_generic_event_t *ev; > + xcb_present_generic_event_t *ge; > + > + /* Ask for the an event for the target MSC */ > + ++priv->present_msc_request_serial; > + xcb_present_notify_msc(c, > + priv->base.xDrawable, > + priv->present_msc_request_serial, > + target_msc, > + divisor, > + remainder); > + > + xcb_flush(c); > + > + /* Wait for the event */ > + if (priv->special_event) { > + while (priv->present_msc_request_serial !=3D priv->present_msc_eve= nt_serial) { > + ev =3D xcb_wait_for_special_event(c, priv->special_event); > + if (!ev) > + break; > + ge =3D (void *) ev; > + present_handle_special_event(priv, ge); > + } > + } > + > + *ust =3D priv->ust; > + *msc =3D priv->msc; > + funny extra newline. > + *sbc =3D priv->sbc; > + > + return 1; > +} > +static int > +dri3_wait_for_sbc(__GLXDRIdrawable *pdraw, int64_t target_sbc, int64_t *= ust, > + int64_t *msc, int64_t *sbc) > +{ > + struct dri3_drawable *priv =3D (struct dri3_drawable *) pdraw; > + > + while (priv->sbc < target_sbc) { > + sleep(1); > + } Some sort of comment about what's going on here? Seems like sleep(1) would always be a wrong thing to execute. > + return dri3_wait_for_msc(pdraw, 0, 0, 0, ust, msc, sbc); > +} > +/** > + * dri3Throttle - Request driver throttling > + * > + * This function uses the DRI2 throttle extension to give the > + * driver the opportunity to throttle on flush front, copysubbuffer > + * and swapbuffers. > + */ > +static void > +dri3_throttle(struct dri3_screen *psc, > + struct dri3_drawable *draw, > + enum __DRI2throttleReason reason) > +{ > + if (psc->throttle) { > + __DRIcontext *ctx =3D dri3_get_current_context(); > + > + psc->throttle->throttle(ctx, draw->driDrawable, reason); > + } > +} I think we can drop this entirely thanks to flush_with_flags (see below). The gallium-only implementation of this driver extension is just a call to flush_with_flags. > + > +/** > + * Asks the driver to flush any queued work necessary for serializing wi= th the > + * X command stream, and optionally the slightly more strict requirement= of > + * glFlush() equivalence (which would require flushing even if nothing h= ad > + * been drawn to a window system framebuffer, for example). > + */ > +static void > +dri3_flush(struct dri3_screen *psc, > + __DRIcontext *ctx, > + struct dri3_drawable *draw, > + unsigned flags, > + enum __DRI2throttleReason throttle_reason) > +{ > + if (ctx && psc->f && psc->f->base.version >=3D 4) { > + psc->f->flush_with_flags(ctx, draw->driDrawable, flags, throttle_r= eason); > + } else { > + if (flags & __DRI2_FLUSH_CONTEXT) > + glFlush(); > + > + if (psc->f) > + psc->f->flush(draw->driDrawable); > + > + dri3_throttle(psc, draw, throttle_reason); > + } > +} I'd rather insist that the driver supports flush_with_flags if you do DRI3. > +static void > +dri3_copy_sub_buffer(__GLXDRIdrawable *pdraw, int x, int y, > + int width, int height, Bool flush) > +{ > + _dri3_copy_sub_buffer(pdraw, x, y, width, height, > + __DRI2_THROTTLE_COPYSUBBUFFER, flush); > +} This appears to be a pointless wrapper. > +static void > +dri3_copy_drawable(struct dri3_drawable *priv, Drawable dest, Drawable s= rc) > +{ > + struct dri3_screen *psc =3D (struct dri3_screen *) priv->base.psc; > + xcb_connection_t *c =3D XGetXCBConnection(priv->base.psc->dpy); > + > + if (psc->f) > + (*psc->f->flush) (priv->driDrawable); Use flush_with_flags instead. > + dri3_copy_area(c, > + src, dest, > + dri3_drawable_gc(priv), > + 0, 0, 0, 0, priv->width, priv->height); > +} DRI2CopyRegion round-tripped, while this call doesn't. As a result, I think dri3_wait_x is broken because it doesn't ensure that the copyarea actually happens before your driver goes rendering again. dri3_wait_gl may be similarly wrong in the other way. We don't have testing for glXWaitGL() or glXWaitX() at all, and that's bad. > +static void > +dri3_wait_x(struct glx_context *gc) > +{ > + struct dri3_drawable *priv =3D (struct dri3_drawable *) > + GetGLXDRIDrawable(gc->currentDpy, gc->currentDrawable); > + > + if (priv =3D=3D NULL || !priv->have_fake_front) > + return; > + > + dri3_copy_drawable(priv, dri3_fake_front_buffer(priv)->pixmap, priv->= base.xDrawable); > +} > + > +static void > +dri3_wait_gl(struct glx_context *gc) > +{ > + struct dri3_drawable *priv =3D (struct dri3_drawable *) > + GetGLXDRIDrawable(gc->currentDpy, gc->currentDrawable); > + > + if (priv =3D=3D NULL || !priv->have_fake_front) > + return; > + > + dri3_copy_drawable(priv, priv->base.xDrawable, dri3_fake_front_buffer= (priv)->pixmap); > +} What's going on with fence reset/triggering being present in copysubbuffer but not these entrypoints? > +static struct dri3_buffer * > +dri3_alloc_render_buffer(struct glx_screen *glx_screen, Drawable draw, u= nsigned int format, int width, int height, int depth) 80-column wrap > +{ > + struct dri3_screen *psc =3D (struct dri3_screen *) glx_screen; > + Display *dpy =3D glx_screen->dpy; > + struct dri3_buffer *buffer; > + xcb_connection_t *c =3D XGetXCBConnection(dpy); > + xcb_pixmap_t pixmap; > + xcb_sync_fence_t sync_fence; > + int32_t *shm_fence; > + int buffer_fd, fence_fd; > + int stride; > + > + fence_fd =3D xshmfence_alloc_shm(); > + if (fence_fd < 0) > + return NULL; > + shm_fence =3D xshmfence_map_shm(fence_fd); > + if (shm_fence =3D=3D NULL) > + goto no_shm_fence; > + > + buffer =3D calloc(1, sizeof (struct dri3_buffer)); > + if (!buffer) > + goto no_buffer; > + > + buffer->image =3D (*psc->image->createImage) (psc->driScreen, > + width, height, > + format, > + __DRI_IMAGE_USE_SHARE|__D= RI_IMAGE_USE_SCANOUT, > + buffer); > + > +=20=20=20 trailing whitespace > + if (!buffer->image) > + goto no_image; > + > + if (!(*psc->image->queryImage)(buffer->image, __DRI_IMAGE_ATTRIB_STRI= DE, &stride)) > + goto no_buffer_attrib; > + > + buffer->pitch =3D stride; > + > + if (!(*psc->image->queryImage)(buffer->image, __DRI_IMAGE_ATTRIB_FD, = &buffer_fd)) > + goto no_buffer_attrib; > + > + xcb_dri3_pixmap_from_buffer(c, > + (pixmap =3D xcb_generate_id(c)), > + draw, > + buffer->size, > + width, height, buffer->pitch, > + depth, buffer->cpp * 8, I don't see buffer->cpp initialized anywhere. > + /* Mark the buffer as idle */ > + dri3_fence_set(buffer); > + > + return buffer; > +=20=20=20 trailing whitespace > +static void > +dri3_free_render_buffer(struct dri3_drawable *pdraw, struct dri3_buffer = *buffer) > +{ > + struct dri3_screen *psc =3D (struct dri3_screen *) pdraw->base.psc; > + xcb_connection_t *c =3D XGetXCBConnection(pdraw->base.psc->dpy); > + > + xcb_free_pixmap(c, buffer->pixmap); > + xcb_sync_destroy_fence(c, buffer->sync_fence); > + xshmfence_unmap_shm(buffer->shm_fence); > + (*psc->image->destroyImage)(buffer->image); > + free(buffer); > +} > + > + > + > +static void > +present_flush_events(struct dri3_drawable *priv) > +{ > + xcb_connection_t *c =3D XGetXCBConnection(priv->base.psc->dpy); > + > + /* Check to see if any configuration changes have occurred > + * since we were last invoked > + */ > + if (priv->special_event) { > + xcb_generic_event_t *ev; > + > + while ((ev =3D xcb_check_for_special_event(c, priv->special_event)= ) !=3D NULL) { > + xcb_present_generic_event_t *ge =3D (void *) ev; > + present_handle_special_event(priv, ge); > + } > + } > +} > + > +static int > +dri3_update_drawable(__DRIdrawable *driDrawable, void *loaderPrivate) > +{ > + struct dri3_drawable *priv =3D loaderPrivate; > + xcb_connection_t *c =3D XGetXCBConnection(priv->base.psc->dpy); > + > + /* First time through, go get the current drawable geometry > + */ > + if (priv->width =3D=3D 0 || priv->height =3D=3D 0 || priv->depth =3D= =3D 0) { > + xcb_get_geometry_cookie_t geom_cookie; > + xcb_get_geometry_reply_t *geom_reply; > + xcb_void_cookie_t cookie; > + xcb_generic_error_t *error; > + > + cookie =3D xcb_present_select_input_checked(c, > + (priv->eid =3D xcb_gener= ate_id(c)), > + priv->base.xDrawable, > + XCB_PRESENT_EVENT_MASK_C= ONFIGURE_NOTIFY| > + XCB_PRESENT_EVENT_MASK_C= OMPLETE_NOTIFY| > + XCB_PRESENT_EVENT_MASK_I= DLE_NOTIFY); > +=20=20=20=20=20=20=20=20=20 > + if (!priv->present_extension) { > + priv->present_extension =3D xcb_get_extension_data(c, &xcb_pres= ent_id); > + if (!priv->present_extension) > + return false; > + } > + > + priv->special_event =3D xcb_register_for_special_event(c, > + priv->present= _extension->major_opcode, > + priv->eid, > + priv->stamp); > + > + geom_cookie =3D xcb_get_geometry(c, priv->base.xDrawable); > + > + geom_reply =3D xcb_get_geometry_reply(c, geom_cookie, NULL); > + > + if (!geom_reply) > + return false; > + > + priv->width =3D geom_reply->width; > + priv->height =3D geom_reply->height; > + priv->depth =3D geom_reply->depth; > + priv->is_pixmap =3D false; > + > + free(geom_reply); > + > + error =3D xcb_request_check(c, cookie); > + > + if (error) { > + if (error->error_code !=3D BadWindow) { > + free(error); > + return false; > + } > + priv->is_pixmap =3D true; > + xcb_unregister_for_special_event(c, priv->special_event); > + priv->special_event =3D NULL; > + } > + } You should probably comment what's going on here. Is an error going to be returned iff it's a pixmap? > +=20=20=20 trailing whitespace. > +static int > +image_format_to_fourcc(int format) > +{ > + > + /* Convert from __DRI_IMAGE_FORMAT to __DRI_IMAGE_FOURCC (sigh) */ > + switch (format) { > + case __DRI_IMAGE_FORMAT_RGB565: return __DRI_IMAGE_FOURCC_RGB565; > + case __DRI_IMAGE_FORMAT_XRGB8888: return __DRI_IMAGE_FOURCC_XRGB8888; > + case __DRI_IMAGE_FORMAT_ARGB8888: return __DRI_IMAGE_FOURCC_ARGB8888; > + case __DRI_IMAGE_FORMAT_ABGR8888: return __DRI_IMAGE_FOURCC_ABGR8888; > + case __DRI_IMAGE_FORMAT_XBGR8888: return __DRI_IMAGE_FOURCC_XBGR8888; > +// case __DRI_IMAGE_FORMAT_R8: return __DRI_IMAGE_FOURCC_R8; > +// case __DRI_IMAGE_FORMAT_GR88: return __DRI_IMAGE_FOURCC_GR88; > +// case __DRI_IMAGE_FORMAT_NONE: return __DRI_IMAGE_FOURCC_NONE; > +// case __DRI_IMAGE_FORMAT_XRGB2101010: return __DRI_IMAGE_FOURCC_XRGB= 2101010; > +// case __DRI_IMAGE_FORMAT_ARGB2101010: return __DRI_IMAGE_FOURCC_ARGB= 2101010; What's up with commented out formats? > + } > + return 0; > +} > + > +static struct dri3_buffer * > +dri3_get_pixmap_buffer(__DRIdrawable *driDrawable, > + unsigned int format, > + enum dri3_buffer_type buffer_type, > + void *loaderPrivate) > +{ > + struct dri3_drawable *pdraw =3D loaderPrivate; > + int buf_id =3D buffer_type =3D=3D dr= i3_pixmap_buf_id(buffer_type); > + struct dri3_buffer *buffer =3D pdraw->buffers[buf_i= d]; > + Pixmap pixmap; > + xcb_dri3_buffer_from_pixmap_cookie_t bp_cookie; > + xcb_dri3_buffer_from_pixmap_reply_t *bp_reply; > + int *fds; > + int buffer_fd; > + Display *dpy; > + struct dri3_screen *psc; > + xcb_connection_t *c; > + xcb_sync_fence_t sync_fence; > + int32_t *shm_fence; > + int fence_fd; > + __DRIimage *image_planar; > + int stride, offset; > + > + if (buffer) > + return buffer; > + > + pixmap =3D pdraw->base.xDrawable; > + psc =3D (struct dri3_screen *) pdraw->base.psc; > + dpy =3D psc->base.dpy; > + c =3D XGetXCBConnection(dpy); > + > + buffer =3D calloc(1, sizeof (struct dri3_buffer)); > + if (!buffer) > + goto no_buffer; > + > + image_planar =3D (*psc->image->createImageFromFds) (psc->driScreen, > + bp_reply->width, > + bp_reply->height, > + image_format_to_fou= rcc(format), > + fds, 1, > + &stride, &offset, b= uffer); > + close(buffer_fd); just drop buffer_fd and reference fds[0] again? > + if (!image_planar) > + goto no_image; > + > + buffer->image =3D (*psc->image->fromPlanar)(image_planar, 0, buffer); > + > + (*psc->image->destroyImage)(image_planar); Is the fromPlanar step here actually needed? It looks like since num_fds =3D=3D 1 you get a functional image from the first step. > +static int > +dri3_get_buffers(__DRIdrawable *driDrawable, > + int *width, int *height, > + unsigned int format, > + uint32_t *stamp, > + void *loaderPrivate, > + uint32_t buffer_mask, > + struct __DRIimageList *buffers) > +{ > + struct dri3_drawable *priv =3D loaderPrivate; > + struct dri3_buffer *front, *back; > + > + buffers->front =3D NULL; > + buffers->back =3D NULL; > + > + front =3D NULL; > + back =3D NULL; > + > + if (!dri3_update_drawable(driDrawable, loaderPrivate)) > + return false; > + > + if (priv->is_pixmap) > + buffer_mask |=3D __DRI_IMAGE_BUFFER_FRONT; > + > + if (buffer_mask & __DRI_IMAGE_BUFFER_FRONT) { > + if (priv->is_pixmap) > + front =3D dri3_get_pixmap_buffer(driDrawable, > + format, > + dri3_buffer_front, > + loaderPrivate); > + else > + front =3D dri3_get_buffer(driDrawable, > + format, > + dri3_buffer_front, > + loaderPrivate); > + > + if (!front) > + return false; > + priv->have_fake_front =3D !priv->is_pixmap; > + } else { > + dri3_free_buffers(driDrawable, dri3_buffer_front, loaderPrivate); > + priv->have_fake_front =3D 0; > + } > + > + if (buffer_mask & __DRI_IMAGE_BUFFER_BACK) { > + back =3D dri3_get_buffer(driDrawable, > + format, > + dri3_buffer_back, > + loaderPrivate); > + if (!back) > + return false; I think this early return leaks front. Also have_fake_front's setting leaked in even though we're not updating priv->front, should it have? > + priv->have_back =3D 1; > + } else { > + dri3_free_buffers(driDrawable, dri3_buffer_back, loaderPrivate); > + priv->have_back =3D 0; > + } > + > + if (front) > + buffers->front =3D front->image; > +=20=20=20 > + if (back) > + buffers->back =3D back->image; > + > + priv->stamp =3D stamp; > + > + /* Report back current geometry */ > + *width =3D priv->width; > + *height =3D priv->height; > + return true; > +} > + > + > +static int > +dri3_query_version(Display *dpy, int *major, int *minor) > +{ > + xcb_dri3_query_version_cookie_t cookie; > + xcb_dri3_query_version_reply_t *reply;=20=20=20=20=20=20=20 trailing whitespace > + xcb_connection_t *c =3D XGetXCBConnection(dpy); > + xcb_generic_error_t *error; > + > + cookie =3D xcb_dri3_query_version(c, > + XCB_DRI3_MAJOR_VERSION, > + XCB_DRI3_MINOR_VERSION); > + reply =3D xcb_dri3_query_version_reply(c, cookie, &error); > + if (!reply) { > + if (error) { > + free(error); > + } No need for NULL-checking free(). Same 2 comments apply to the next 2 functions. > + return 0; > + } > + *major =3D reply->major_version; > + *minor =3D reply->minor_version; > + free(reply); > + return 1; > +} > +static const __DRIimageLoaderExtension imageLoaderExtension =3D { > + {__DRI_IMAGE_LOADER, __DRI_IMAGE_LOADER_VERSION}, > + .getBuffers =3D dri3_get_buffers, > + .flushFrontBuffer =3D dri3_flush_front_buffer, > +}; > + > +static void > +dri3_bind_tex_image(Display * dpy, > + GLXDrawable drawable, > + int buffer, const int *attrib_list) > +{ > + struct glx_context *gc =3D __glXGetCurrentContext(); > + struct dri3_context *pcp =3D (struct dri3_context *) gc; > + __GLXDRIdrawable *base =3D GetGLXDRIDrawable(dpy, drawable); > + struct dri3_drawable *pdraw =3D (struct dri3_drawable *) base; > + struct dri3_screen *psc; > + > + if (pdraw !=3D NULL) { > + psc =3D (struct dri3_screen *) base->psc; > + > + if (psc->f && > + psc->f->base.version >=3D 3 && psc->f->invalidate) > + psc->f->invalidate(pdraw->driDrawable); > + > + XSync(dpy, false); > + if (psc->texBuffer->base.version >=3D 2 && > + psc->texBuffer->setTexBuffer2 !=3D NULL) { > + (*psc->texBuffer->setTexBuffer2) (pcp->driContext, > + pdraw->base.textureTarget, > + pdraw->base.textureFormat, > + pdraw->driDrawable); > + } > + else { > + (*psc->texBuffer->setTexBuffer) (pcp->driContext, > + pdraw->base.textureTarget, > + pdraw->driDrawable); > + } > + } > +} Tab indentation :( I'd really like to see less loader code duplication. But if you have to, at least don't support old setTexBuffer when you know the driver's new enough that it's got DRI3. > +static void > +dri3_release_tex_image(Display * dpy, GLXDrawable drawable, int buffer) > +{ > +#if __DRI_TEX_BUFFER_VERSION >=3D 3 > + struct glx_context *gc =3D __glXGetCurrentContext(); > + struct dri3_context *pcp =3D (struct dri3_context *) gc; > + __GLXDRIdrawable *base =3D GetGLXDRIDrawable(dpy, drawable); > + struct glx_display *dpyPriv =3D __glXInitialize(dpy); > + struct dri3_drawable *pdraw =3D (struct dri3_drawable *) base; > + struct dri3_display *pdp =3D > + (struct dri3_display *) dpyPriv->dri3Display; > + struct dri3_screen *psc; > + > + if (pdraw !=3D NULL) { > + psc =3D (struct dri3_screen *) base->psc; > + > + if (psc->texBuffer->base.version >=3D 3 && > + psc->texBuffer->releaseTexBuffer !=3D NULL) { > + (*psc->texBuffer->releaseTexBuffer) (pcp->driContext, > + pdraw->base.textureTarget, > + pdraw->driDrawable); > + } > + } > +#endif Remove the #ifdef. You're in the tree, you know its value. > +} > + > +static const struct glx_context_vtable dri3_context_vtable =3D { > + dri3_destroy_context, > + dri3_bind_context, > + dri3_unbind_context, > + dri3_wait_gl, > + dri3_wait_x, > + DRI_glXUseXFont, > + dri3_bind_tex_image, > + dri3_release_tex_image, > + NULL, /* get_proc_address */ > +}; > + > +static void > +dri3_bind_extensions(struct dri3_screen *psc, struct glx_display * priv, > + const char *driverName) > +{ > +// const struct dri3_display *const pdp =3D (struct dri3_display *) pr= iv->dri3Display; more commented leftovers. > + const __DRIextension **extensions; > + unsigned mask; > + int i; > + > + extensions =3D psc->core->getExtensions(psc->driScreen); > + > + __glXEnableDirectExtension(&psc->base, "GLX_SGI_video_sync"); > + __glXEnableDirectExtension(&psc->base, "GLX_SGI_swap_control"); > + __glXEnableDirectExtension(&psc->base, "GLX_MESA_swap_control"); > + __glXEnableDirectExtension(&psc->base, "GLX_SGI_make_current_read"); > + > + /* > + * GLX_INTEL_swap_event is broken on the server side, where it's > + * currently unconditionally enabled. This completely breaks > + * systems running on drivers which don't support that extension. > + * There's no way to test for its presence on this side, so instead > + * of disabling it unconditionally, just disable it for drivers > + * which are known to not support it, or for DDX drivers supporting > + * only an older (pre-ScheduleSwap) version of DRI2. > + * > + * This is a hack which is required until: > + * http://lists.x.org/archives/xorg-devel/2013-February/035449.html > + * is merged and updated xserver makes it's way into distros: > + */ > +// if (pdp->swapAvailable && strcmp(driverName, "vmwgfx") !=3D 0) { > +// __glXEnableDirectExtension(&psc->base, "GLX_INTEL_swap_event"); > +// } more commented leftovers. Are you dropping swap_event support? > + > + mask =3D psc->image_driver->getAPIMask(psc->driScreen); > + > + __glXEnableDirectExtension(&psc->base, "GLX_ARB_create_context"); > + __glXEnableDirectExtension(&psc->base, "GLX_ARB_create_context_profil= e"); > + > + if ((mask & (1 << __DRI_API_GLES2)) !=3D 0) > + __glXEnableDirectExtension(&psc->base, > + "GLX_EXT_create_context_es2_profile"); > + > + for (i =3D 0; extensions[i]; i++) { > + if ((strcmp(extensions[i]->name, __DRI_TEX_BUFFER) =3D=3D 0)) { > + psc->texBuffer =3D (__DRItexBufferExtension *) extensions[i]; > + __glXEnableDirectExtension(&psc->base, "GLX_EXT_texture_from_pixmap"); > + } > + > + if ((strcmp(extensions[i]->name, __DRI2_FLUSH) =3D=3D 0)) { > + psc->f =3D (__DRI2flushExtension *) extensions[i]; > + /* internal driver extension, no GL extension exposed */ > + } > + > + if ((strcmp(extensions[i]->name, __DRI2_CONFIG_QUERY) =3D=3D 0)) > + psc->config =3D (__DRI2configQueryExtension *) extensions[i]; > + > + if (((strcmp(extensions[i]->name, __DRI2_THROTTLE) =3D=3D 0))) > + psc->throttle =3D (__DRI2throttleExtension *) extensions[i]; > + > + if (strcmp(extensions[i]->name, __DRI2_ROBUSTNESS) =3D=3D 0) > + __glXEnableDirectExtension(&psc->base, > + "GLX_ARB_create_context_robustness"); > + } > +} > + This is horribly duplicated with dri2, but that's also horribly duplicated with EGL's dri3. I really think we need to do a dri_loader_common between all of them with a bunch of this crap. > + tmp =3D getenv("LIBGL_SHOW_FPS"); > + psc->show_fps =3D tmp && strcmp(tmp, "1") =3D=3D 0; Dead code. > +/* > + * Allocate, initialize and return a __DRIdisplayPrivate object. > + * This is called from __glXInitialize() when we are given a new > + * display pointer. > + */ > +_X_HIDDEN __GLXDRIdisplay * > +dri3_create_display(Display * dpy) > +{ > + struct dri3_display *pdp; > + int i; > + > + pdp =3D malloc(sizeof *pdp); > + if (pdp =3D=3D NULL) > + return NULL; > + > + if (!dri3_query_version(dpy, &pdp->dri3Major, &pdp->dri3Minor)) > + goto no_extension; > + > + if (!present_query_version(dpy, &pdp->presentMajor, &pdp->presentMino= r)) > + goto no_extension; > + > + pdp->base.destroyDisplay =3D dri3_destroy_display; > + pdp->base.createScreen =3D dri3_create_screen; > + > + i =3D 0; > + > + pdp->loader_extensions[i++] =3D &imageLoaderExtension.base; > +=20=20=20 trailing whitespace > diff --git a/src/glx/dri3_priv.h b/src/glx/dri3_priv.h > new file mode 100644 > index 0000000..2873919 > --- /dev/null > +++ b/src/glx/dri3_priv.h > + > +struct dri3_buffer { > + __DRIimage *image; > + uint32_t pixmap; > + uint32_t sync_fence; > + int32_t *shm_fence; > + GLboolean busy; Can we get some comments on what these 3 fields do? These synchronization details were a huge part of the dri3 discussions, and I know you've gone several ways about things in the process of development, but I see just a single comment about what fences do: + /* Mark the buffer as idle */ That's... not enough. > +struct dri3_drawable > +{ > + /* For WaitMSC */ > + uint32_t present_msc_request_serial; > + uint32_t present_msc_event_serial; > +=20=20=20 whitespace --=-=-= Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.15 (GNU/Linux) iQIcBAEBCAAGBQJSeXrTAAoJELXWKTbR/J7oa2sQAKbDoahocFfS25TAlX3R8KSW 1ssEaXtjtbGTY+Th9xybhukyMJttNXGmk5SLZi+mmU9buAtDXBDXomTrK+xESIQY 8G2GdxqQZBy0NAW3IEnHpES3mUBhkigGtqMIXBl4vSa4QbHv0RpXTKmPaWf65Mq/ dVXO6RR2NKt6LfTBKBLYmlJAs78x/zfhnfMJ6X4t95hdX8h3vxI8pMhLuk++EZYr PDgs1YYhaDR2G2bDMXDqmiOO/gJf/uv4RF25lZMv2TekTWGZ3hL/ajS5df/HBDUb iYT0PtdaVD/bb3rza7Dgb2MY6lMTVuFWlnDL7EFOAtESdFu2X938P7MRS2kL+t4f 6UYqj/cFWr1R6Y2yTNm9mi7mtJWEmvA1jQMKSGWFl0m+hItJYYrno4QWsoCkxF40 kzcKn85dmz49G03kavFN31eGRYLelGMlbDeWyUYicUxESVaulLoc1EjpbPr9mM6L EKBeyaeb7IDbeAgPHQWgD5U7iSMzTxD50zj52EY1fNKkWIX4lTLEdyxJT5LbIY15 ZeX3pfOYQznp9Z2L09Crj3ZF/KJeAyvQURJPjND2zxOxx89cIQdIy+PsiuOtkOLX ZSEsDXgb8usL0Uyh64QSNCgjRqosX0Cvo7B7VqDWKYMoWVhBIS4yvYqNZaKdCarT lCLpO0B8wyKpkIGiHzYF =IV6s -----END PGP SIGNATURE----- --=-=-=-- --===============1266618964== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel --===============1266618964==--