[Mesa-dev] [PATCH] RFC: drisw/glx: use XShm if possible
Jose Fonseca
jfonseca at vmware.com
Fri Jun 5 12:13:51 PDT 2015
Sounds good in principle. I'm actually surprised this was happening
already. I always use llvmpipe the the non-DRI pure XLIB state tracker
which already does this.
Two comments:
- There's src/gallium/winsys/sw/xlib/xlib_sw_winsys.c which implements
this. I wonder if it would be simpler/cleaner for
src/gallium/winsys/sw/dri/dri_sw_winsys.c to just piggy back on it somehow.
- I think that more care must be taken to pre-emptively remove the
shmids, as these can leak if application doesn't terminate cleanly,
causing the system to run out of shmids, which causes the rendering
faling back to slow XPutImage and even more stability problems.
See also
http://cgit.freedesktop.org/mesa/mesa/commit/?id=e29525f79fdb62993e14a4bc87a9e0955b838de0
BTW, this is the garbagge collection script I use on my Jenkins setup
to ensure we don't leak shmids:
$ cat shmgc.sh
#!/bin/sh
ipcs -m | awk '{ if ($6 == 0) print "-m " $2; }' | xargs
--no-run-if-empty ipcrm
I suggest you run piglit, and compare the output of `$ cat shmgc.sh
#!/bin/sh
ipcs -m | awk '{ if ($6 == 0) print "-m " $2; }' | xargs
--no-run-if-empty ipcrm
` before and after, to see if they are leaking or not.
One question about the commit message: What do you mean by "video
frames"? Are you talking about video players using GL backend? Or
general 3D?
Jose
On 05/06/15 18:14, Marc-André Lureau wrote:
> XPutImage requires to copy the images around, and the request may be
> split over several chunks. Using XShm may improve performance.
>
> In particular, the performances are bad when using gnome-shell with
> Spice and playing video. Chunking the update confuses the video
> detection heuristic: unfortunately it's not easy to change it without
> breaking further other cases. Making one big request per video frames
> solves most of the issues.
> ---
> include/GL/internal/dri_interface.h | 12 ++-
> src/gallium/include/state_tracker/drisw_api.h | 3 +
> src/gallium/state_trackers/dri/drisw.c | 37 +++++--
> src/gallium/winsys/sw/dri/dri_sw_winsys.c | 69 ++++++++++---
> src/glx/drisw_glx.c | 141 +++++++++++++++++++++++---
> src/glx/drisw_priv.h | 7 +-
> 6 files changed, 231 insertions(+), 38 deletions(-)
>
> diff --git a/include/GL/internal/dri_interface.h b/include/GL/internal/dri_interface.h
> index c827bb6..61cb58f 100644
> --- a/include/GL/internal/dri_interface.h
> +++ b/include/GL/internal/dri_interface.h
> @@ -502,7 +502,7 @@ struct __DRIdamageExtensionRec {
> * SWRast Loader extension.
> */
> #define __DRI_SWRAST_LOADER "DRI_SWRastLoader"
> -#define __DRI_SWRAST_LOADER_VERSION 2
> +#define __DRI_SWRAST_LOADER_VERSION 3
> struct __DRIswrastLoaderExtensionRec {
> __DRIextension base;
>
> @@ -535,6 +535,16 @@ struct __DRIswrastLoaderExtensionRec {
> void (*putImage2)(__DRIdrawable *drawable, int op,
> int x, int y, int width, int height, int stride,
> char *data, void *loaderPrivate);
> +
> + /**
> + * Put image to drawable
> + *
> + * \since 3
> + */
> + void (*putImageShm)(__DRIdrawable *drawable, int op,
> + int x, int y, int width, int height, int stride,
> + int shmid, char *shmaddr, unsigned offset,
> + void *loaderPrivate);
> };
>
> /**
> diff --git a/src/gallium/include/state_tracker/drisw_api.h b/src/gallium/include/state_tracker/drisw_api.h
> index 328440c..010a603 100644
> --- a/src/gallium/include/state_tracker/drisw_api.h
> +++ b/src/gallium/include/state_tracker/drisw_api.h
> @@ -15,6 +15,9 @@ struct drisw_loader_funcs
> void *data, unsigned width, unsigned height);
> void (*put_image2) (struct dri_drawable *dri_drawable,
> void *data, int x, int y, unsigned width, unsigned height, unsigned stride);
> + void (*put_image_shm) (struct dri_drawable *dri_drawable,
> + int shmid, char *shmaddr, unsigned offset,
> + int x, int y, unsigned width, unsigned height, unsigned stride);
> };
>
> /**
> diff --git a/src/gallium/state_trackers/dri/drisw.c b/src/gallium/state_trackers/dri/drisw.c
> index 4a2c1bb..75c8e85e 100644
> --- a/src/gallium/state_trackers/dri/drisw.c
> +++ b/src/gallium/state_trackers/dri/drisw.c
> @@ -26,14 +26,6 @@
> *
> **************************************************************************/
>
> -/* TODO:
> - *
> - * xshm / EGLImage:
> - *
> - * Allow the loaders to use the XSHM extension. It probably requires callbacks
> - * for createImage/destroyImage similar to DRI2 getBuffers.
> - */
> -
> #include "util/u_format.h"
> #include "util/u_memory.h"
> #include "util/u_inlines.h"
> @@ -85,6 +77,19 @@ put_image2(__DRIdrawable *dPriv, void *data, int x, int y,
> }
>
> static INLINE void
> +put_image_shm(__DRIdrawable *dPriv, int shmid, char *shmaddr,
> + unsigned offset, int x, int y,
> + unsigned width, unsigned height, unsigned stride)
> +{
> + __DRIscreen *sPriv = dPriv->driScreenPriv;
> + const __DRIswrastLoaderExtension *loader = sPriv->swrast_loader;
> +
> + loader->putImageShm(dPriv, __DRI_SWRAST_IMAGE_OP_SWAP,
> + x, y, width, height, stride,
> + shmid, shmaddr, offset, dPriv->loaderPrivate);
> +}
> +
> +static INLINE void
> get_image(__DRIdrawable *dPriv, int x, int y, int width, int height, void *data)
> {
> __DRIscreen *sPriv = dPriv->driScreenPriv;
> @@ -123,6 +128,17 @@ drisw_put_image2(struct dri_drawable *drawable,
> put_image2(dPriv, data, x, y, width, height, stride);
> }
>
> +static void
> +drisw_put_image_shm(struct dri_drawable *drawable,
> + int shmid, char *shmaddr, unsigned offset,
> + int x, int y, unsigned width, unsigned height,
> + unsigned stride)
> +{
> + __DRIdrawable *dPriv = drawable->dPriv;
> +
> + put_image_shm(dPriv, shmid, shmaddr, offset, x, y, width, height, stride);
> +}
> +
> static INLINE void
> drisw_present_texture(__DRIdrawable *dPriv,
> struct pipe_resource *ptex, struct pipe_box *sub_box)
> @@ -345,6 +361,7 @@ static struct drisw_loader_funcs drisw_lf = {
> static const __DRIconfig **
> drisw_init_screen(__DRIscreen * sPriv)
> {
> + const __DRIswrastLoaderExtension *loader = sPriv->swrast_loader;
> const __DRIconfig **configs;
> struct dri_screen *screen;
> struct pipe_screen *pscreen;
> @@ -360,6 +377,10 @@ drisw_init_screen(__DRIscreen * sPriv)
>
> sPriv->driverPrivate = (void *)screen;
> sPriv->extensions = drisw_screen_extensions;
> + if (loader->base.version >= 3) {
> + if (loader->putImageShm)
> + drisw_lf.put_image_shm = drisw_put_image_shm;
> + }
>
> pscreen = drisw_create_screen(&drisw_lf);
> /* dri_init_screen_helper checks pscreen for us */
> diff --git a/src/gallium/winsys/sw/dri/dri_sw_winsys.c b/src/gallium/winsys/sw/dri/dri_sw_winsys.c
> index 6fed22b..d35218a 100644
> --- a/src/gallium/winsys/sw/dri/dri_sw_winsys.c
> +++ b/src/gallium/winsys/sw/dri/dri_sw_winsys.c
> @@ -26,17 +26,20 @@
> *
> **************************************************************************/
>
> +#include <sys/ipc.h>
> +#include <sys/shm.h>
> +
> #include "pipe/p_compiler.h"
> #include "pipe/p_format.h"
> #include "util/u_inlines.h"
> #include "util/u_format.h"
> #include "util/u_math.h"
> #include "util/u_memory.h"
> +#include "util/u_debug.h"
>
> #include "state_tracker/sw_winsys.h"
> #include "dri_sw_winsys.h"
>
> -
> struct dri_sw_displaytarget
> {
> enum pipe_format format;
> @@ -44,6 +47,7 @@ struct dri_sw_displaytarget
> unsigned height;
> unsigned stride;
>
> + int shmid;
> void *data;
> void *mapped;
> };
> @@ -77,6 +81,25 @@ dri_sw_is_displaytarget_format_supported( struct sw_winsys *ws,
> return TRUE;
> }
>
> +static char *
> +alloc_shm(struct dri_sw_displaytarget *dri_sw_dt, unsigned size)
> +{
> + char *addr;
> +
> + dri_sw_dt->shmid = shmget(IPC_PRIVATE, size, IPC_CREAT|0777);
> + if (dri_sw_dt->shmid < 0) {
> + return NULL;
> + }
> +
> + addr = (char *) shmat(dri_sw_dt->shmid, 0, 0);
> + if (addr == (char *) -1) {
> + shmctl(dri_sw_dt->shmid, IPC_RMID, 0);
> + return NULL;
> + }
> +
> + return addr;
> +}
> +
> static struct sw_displaytarget *
> dri_sw_displaytarget_create(struct sw_winsys *winsys,
> unsigned tex_usage,
> @@ -85,6 +108,7 @@ dri_sw_displaytarget_create(struct sw_winsys *winsys,
> unsigned alignment,
> unsigned *stride)
> {
> + struct dri_sw_winsys *ws = dri_sw_winsys(winsys);
> struct dri_sw_displaytarget *dri_sw_dt;
> unsigned nblocksy, size, format_stride;
>
> @@ -102,7 +126,13 @@ dri_sw_displaytarget_create(struct sw_winsys *winsys,
> nblocksy = util_format_get_nblocksy(format, height);
> size = dri_sw_dt->stride * nblocksy;
>
> - dri_sw_dt->data = align_malloc(size, alignment);
> + dri_sw_dt->shmid = -1;
> + if (ws->lf->put_image_shm)
> + dri_sw_dt->data = alloc_shm(dri_sw_dt, size);
> +
> + if(!dri_sw_dt->data)
> + dri_sw_dt->data = align_malloc(size, alignment);
> +
> if(!dri_sw_dt->data)
> goto no_data;
>
> @@ -121,7 +151,12 @@ dri_sw_displaytarget_destroy(struct sw_winsys *ws,
> {
> struct dri_sw_displaytarget *dri_sw_dt = dri_sw_displaytarget(dt);
>
> - FREE(dri_sw_dt->data);
> + if (dri_sw_dt->shmid >= 0) {
> + shmdt(dri_sw_dt->data);
> + shmctl(dri_sw_dt->shmid, IPC_RMID, 0);
> + } else {
> + FREE(dri_sw_dt->data);
> + }
>
> FREE(dri_sw_dt);
> }
> @@ -172,25 +207,33 @@ dri_sw_displaytarget_display(struct sw_winsys *ws,
> struct dri_sw_winsys *dri_sw_ws = dri_sw_winsys(ws);
> struct dri_sw_displaytarget *dri_sw_dt = dri_sw_displaytarget(dt);
> struct dri_drawable *dri_drawable = (struct dri_drawable *)context_private;
> - unsigned width, height;
> + unsigned width, height, x = 0, y = 0;
> unsigned blsize = util_format_get_blocksize(dri_sw_dt->format);
> + unsigned offset = 0;
> + void *data = dri_sw_dt->data;
>
> /* Set the width to 'stride / cpp'.
> *
> * PutImage correctly clips to the width of the dst drawable.
> */
> - width = dri_sw_dt->stride / blsize;
> -
> - height = dri_sw_dt->height;
> -
> if (box) {
> - void *data;
> - data = dri_sw_dt->data + (dri_sw_dt->stride * box->y) + box->x * blsize;
> - dri_sw_ws->lf->put_image2(dri_drawable, data,
> - box->x, box->y, box->width, box->height, dri_sw_dt->stride);
> + offset = (dri_sw_dt->stride * box->y) + box->x * blsize;
> + data += offset;
> + x = box->x;
> + y = box->y;
> + width = box->width;
> + height = box->height;
> } else {
> - dri_sw_ws->lf->put_image(dri_drawable, dri_sw_dt->data, width, height);
> + width = dri_sw_dt->stride / blsize;
> + height = dri_sw_dt->height;
> }
> +
> + if (dri_sw_dt->shmid == -1)
> + dri_sw_ws->lf->put_image2(dri_drawable, data,
> + x, y, width, height, dri_sw_dt->stride);
> + else
> + dri_sw_ws->lf->put_image_shm(dri_drawable, dri_sw_dt->shmid, dri_sw_dt->data, offset,
> + x, y, width, height, dri_sw_dt->stride);
> }
>
> static void
> diff --git a/src/glx/drisw_glx.c b/src/glx/drisw_glx.c
> index 749ceb0..89b9525 100644
> --- a/src/glx/drisw_glx.c
> +++ b/src/glx/drisw_glx.c
> @@ -30,8 +30,8 @@
> #include "drisw_priv.h"
>
> static Bool
> -XCreateDrawable(struct drisw_drawable * pdp,
> - Display * dpy, XID drawable, int visualid)
> +XCreateGCs(struct drisw_drawable * pdp,
> + Display * dpy, XID drawable, int visualid)
> {
> XGCValues gcvalues;
> long visMask;
> @@ -56,7 +56,63 @@ XCreateDrawable(struct drisw_drawable * pdp,
> if (!pdp->visinfo || num_visuals == 0)
> return False;
>
> + return True;
> +}
> +
> +static volatile int XErrorFlag = 0;
> +
> +/**
> + * Catches potential Xlib errors.
> + */
> +static int
> +handle_xerror(Display *dpy, XErrorEvent *event)
> +{
> + (void) dpy;
> + (void) event;
> + XErrorFlag = 1;
> + return 0;
> +}
> +
> +static Bool
> +XCreateDrawable(struct drisw_drawable * pdp, int shmid, Display * dpy)
> +{
> + if (pdp->ximage)
> + XDestroyImage(pdp->ximage);
> +
> + if (shmid >= 0) {
> + int (*old_handler)(Display *, XErrorEvent *);
> +
> + pdp->shminfo.shmid = shmid;
> + pdp->ximage = XShmCreateImage(dpy,
> + pdp->visinfo->visual,
> + pdp->visinfo->depth,
> + ZPixmap,
> + NULL,
> + &pdp->shminfo,
> + 0, 0);
> + if (pdp->ximage == NULL)
> + goto ximage;
> +
> + XErrorFlag = 0;
> + old_handler = XSetErrorHandler(handle_xerror);
> + /* This may trigger the X protocol error we're ready to catch: */
> + XShmAttach(dpy, &pdp->shminfo);
> + XSync(dpy, False);
> +
> + if (!XErrorFlag)
> + goto end;
> +
> + /* we are on a remote display, this error is normal, don't print it */
> + XFlush(dpy);
> + XErrorFlag = 0;
> + XDestroyImage(pdp->ximage);
> + pdp->ximage = NULL;
> + (void) XSetErrorHandler(old_handler);
> + }
> +
> +ximage:
> /* create XImage */
> + pdp->shminfo.shmid = -1;
> pdp->ximage = XCreateImage(dpy,
> pdp->visinfo->visual,
> pdp->visinfo->depth,
> @@ -66,6 +122,7 @@ XCreateDrawable(struct drisw_drawable * pdp,
> 32, /* bitmap_pad */
> 0); /* bytes_per_line */
>
> +end:
> /**
> * swrast does not handle 24-bit depth with 24 bpp, so let X do the
> * the conversion for us.
> @@ -79,7 +136,9 @@ XCreateDrawable(struct drisw_drawable * pdp,
> static void
> XDestroyDrawable(struct drisw_drawable * pdp, Display * dpy, XID drawable)
> {
> - XDestroyImage(pdp->ximage);
> + if (pdp->ximage)
> + XDestroyImage(pdp->ximage);
> +
> free(pdp->visinfo);
>
> XFreeGC(dpy, pdp->gc);
> @@ -133,9 +192,9 @@ bytes_per_line(unsigned pitch_bits, unsigned mul)
> }
>
> static void
> -swrastPutImage2(__DRIdrawable * draw, int op,
> +swrastXPutImage(__DRIdrawable * draw, int op,
> int x, int y, int w, int h, int stride,
> - char *data, void *loaderPrivate)
> + int shmid, char *data, void *loaderPrivate)
> {
> struct drisw_drawable *pdp = loaderPrivate;
> __GLXDRIdrawable *pdraw = &(pdp->base);
> @@ -144,6 +203,11 @@ swrastPutImage2(__DRIdrawable * draw, int op,
> XImage *ximage;
> GC gc;
>
> + if (!pdp->ximage || shmid != pdp->shminfo.shmid) {
> + if (!XCreateDrawable(pdp, shmid, dpy))
> + return;
> + }
> +
> switch (op) {
> case __DRI_SWRAST_IMAGE_OP_DRAW:
> gc = pdp->gc;
> @@ -156,24 +220,51 @@ swrastPutImage2(__DRIdrawable * draw, int op,
> }
>
> drawable = pdraw->xDrawable;
> -
> ximage = pdp->ximage;
> - ximage->data = data;
> - ximage->width = w;
> - ximage->height = h;
> ximage->bytes_per_line = stride ? stride : bytes_per_line(w * ximage->bits_per_pixel, 32);
> + ximage->data = data;
>
> - XPutImage(dpy, drawable, gc, ximage, 0, 0, x, y, w, h);
> -
> + if (pdp->shminfo.shmid >= 0) {
> + ximage->width = ximage->bytes_per_line / 4;
> + ximage->height = h;
> + XShmPutImage(dpy, drawable, gc, ximage, 0, 0, x, y, w, h, False);
> + } else {
> + ximage->width = w;
> + ximage->height = h;
> + XPutImage(dpy, drawable, gc, ximage, 0, 0, x, y, w, h);
> + }
> ximage->data = NULL;
> }
>
> static void
> +swrastPutImageShm(__DRIdrawable * draw, int op,
> + int x, int y, int w, int h, int stride,
> + int shmid, char *shmaddr, unsigned offset,
> + void *loaderPrivate)
> +{
> + struct drisw_drawable *pdp = loaderPrivate;
> +
> + pdp->shminfo.shmaddr = shmaddr;
> + swrastXPutImage(draw, op, x, y, w, h, stride, shmid,
> + shmaddr + offset, loaderPrivate);
> +}
> +
> +static void
> +swrastPutImage2(__DRIdrawable * draw, int op,
> + int x, int y, int w, int h, int stride,
> + char *data, void *loaderPrivate)
> +{
> + swrastXPutImage(draw, op, x, y, w, h, stride, -1,
> + data, loaderPrivate);
> +}
> +
> +static void
> swrastPutImage(__DRIdrawable * draw, int op,
> int x, int y, int w, int h,
> char *data, void *loaderPrivate)
> {
> - swrastPutImage2(draw, op, x, y, w, h, 0, data, loaderPrivate);
> + swrastXPutImage(draw, op, x, y, w, h, 0, -1,
> + data, loaderPrivate);
> }
>
> static void
> @@ -187,6 +278,11 @@ swrastGetImage(__DRIdrawable * read,
> Drawable readable;
> XImage *ximage;
>
> + if (!prp->ximage || prp->shminfo.shmid >= 0) {
> + if (!XCreateDrawable(prp, -1, dpy))
> + return;
> + }
> +
> readable = pread->xDrawable;
>
> ximage = prp->ximage;
> @@ -200,13 +296,14 @@ swrastGetImage(__DRIdrawable * read,
> ximage->data = NULL;
> }
>
> -static const __DRIswrastLoaderExtension swrastLoaderExtension = {
> - .base = {__DRI_SWRAST_LOADER, 2 },
> +static __DRIswrastLoaderExtension swrastLoaderExtension = {
> + .base = {__DRI_SWRAST_LOADER, 3 },
>
> .getDrawableInfo = swrastGetDrawableInfo,
> .putImage = swrastPutImage,
> .getImage = swrastGetImage,
> .putImage2 = swrastPutImage2,
> + .putImageShm = swrastPutImageShm,
> };
>
> static const __DRIextension *loader_extensions[] = {
> @@ -512,7 +609,7 @@ driswCreateDrawable(struct glx_screen *base, XID xDrawable,
> pdp->base.drawable = drawable;
> pdp->base.psc = &psc->base;
>
> - ret = XCreateDrawable(pdp, psc->base.dpy, xDrawable, modes->visualID);
> + ret = XCreateGCs(pdp, psc->base.dpy, xDrawable, modes->visualID);
> if (!ret) {
> free(pdp);
> return NULL;
> @@ -640,6 +737,17 @@ driswBindExtensions(struct drisw_screen *psc, const __DRIextension **extensions)
> }
> }
>
> +static int
> +check_xshm(Display *dpy)
> +{
> + int ignore;
> +
> + /* XXX: could check XShmAttach too for remote case */
> + /* handles it fine when calling later though */
> +
> + return XQueryExtension(dpy, "MIT-SHM", &ignore, &ignore, &ignore);
> +}
> +
> static struct glx_screen *
> driswCreateScreen(int screen, struct glx_display *priv)
> {
> @@ -667,6 +775,9 @@ driswCreateScreen(int screen, struct glx_display *priv)
> if (extensions == NULL)
> goto handle_error;
>
> + if (!check_xshm(psc->base.dpy))
> + swrastLoaderExtension.putImageShm = NULL;
> +
> for (i = 0; extensions[i]; i++) {
> if (strcmp(extensions[i]->name, __DRI_CORE) == 0)
> psc->core = (__DRIcoreExtension *) extensions[i];
> diff --git a/src/glx/drisw_priv.h b/src/glx/drisw_priv.h
> index 5d47900..dc1fc58 100644
> --- a/src/glx/drisw_priv.h
> +++ b/src/glx/drisw_priv.h
> @@ -22,6 +22,11 @@
> * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
> * SOFTWARE.
> */
> +#include <assert.h>
> +
> +#include <sys/ipc.h>
> +#include <sys/shm.h>
> +#include <X11/extensions/XShm.h>
>
> struct drisw_display
> {
> @@ -32,7 +37,6 @@ struct drisw_context
> {
> struct glx_context base;
> __DRIcontext *driContext;
> -
> };
>
> struct drisw_screen
> @@ -62,6 +66,7 @@ struct drisw_drawable
> __DRIdrawable *driDrawable;
> XVisualInfo *visinfo;
> XImage *ximage;
> + XShmSegmentInfo shminfo;
> };
>
> _X_HIDDEN int
>
More information about the mesa-dev
mailing list