[Mesa-dev] [PATCH] RFC: drisw/glx: use XShm if possible

[Adam Jackson]

On Fri, 2015-06-05 at 19:14 +0200, 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.

Almost certainly will, the XPutImage implementation is fairly comic.

> +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;
> +}
> 

This should only set the error flag if the error really did come from
XShmAttach, and (ideally) should call through to the old handler
otherwise.  X is async, there might be requests before the ShmAttach in
the queue that fail.

> +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);

XFlush after XSync is a no-op.  (Well, unless other threads have
touched the display, but even if they have there's no reason to flush
here.)

> +      XErrorFlag = 0;
> +      XDestroyImage(pdp->ximage);
> +      pdp->ximage = NULL;
> +      (void) XSetErrorHandler(old_handler);
> +   }

This is the non-shm fallback path; it's also the only place where
you're restoring old_handler.  Should do it right after XSync.

> @@ -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;
> +   }
> +

Can you explain why you're doing image realization lazily instead of at
createDrawable time?

Also, you're punishing remote clients pretty harshly here.  You've got
dri_sw_displaytarget_display calling put_image_shm if the storage was
allocated as a shm segment (shmid != -1), but pdp->shminfo.shmid is
only set to shmid if ShmAttach succeeds.  On remote connections it
won't, but you take an XSync on every SwapBuffers to find that out.

> @@ -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);

Pretty sure this is broken for non-32bpp.

- ajax