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

[Marc-André Lureau]

Hi Adam,

Thanks for the feedback

On Mon, Jun 8, 2015 at 5:12 PM, Adam Jackson <ajax at redhat.com> wrote:

> 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.
>

This was based off similar code in xlib_sw_winsys.c

I'll try to improve that code, dispatching pending errors.

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

ok


>
> > +      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.
>

ok


>
> > @@ -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?
>

There is no allocation yet when createDrawable() is called


> 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.
>

That's why there is a FIXME in check_xshm() :) But I suppose we could cache
the first result too.


> > @@ -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.
>
>
I'll try to check that too.

thanks



-- 
Marc-André Lureau
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20150608/fb7715dd/attachment-0001.html>