<div dir="ltr"><div>Hi Adam,<br><br></div>Thanks for the feedback<br><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Jun 8, 2015 at 5:12 PM, Adam Jackson <span dir="ltr"><<a href="mailto:ajax@redhat.com" target="_blank">ajax@redhat.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span class="">On Fri, 2015-06-05 at 19:14 +0200, Marc-André Lureau wrote:<br>
> XPutImage requires to copy the images around, and the request may be<br>
> split over several chunks. Using XShm may improve performance.<br>
<br>
</span>Almost certainly will, the XPutImage implementation is fairly comic.<br>
<span class=""><br>
> +static volatile int XErrorFlag = 0;<br>
> +<br>
> +/**<br>
> + * Catches potential Xlib errors.<br>
> + */<br>
> +static int<br>
> +handle_xerror(Display *dpy, XErrorEvent *event)<br>
> +{<br>
> + (void) dpy;<br>
> + (void) event;<br>
> + XErrorFlag = 1;<br>
> + return 0;<br>
> +}<br>
><br>
<br>
</span>This should only set the error flag if the error really did come from<br>
XShmAttach, and (ideally) should call through to the old handler<br>
otherwise. X is async, there might be requests before the ShmAttach in<br>
the queue that fail.<br></blockquote><div><br></div><div>This was based off similar code in xlib_sw_winsys.c<br><br></div><div>I'll try to improve that code, dispatching pending errors. <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div><div class="h5"><br>
> +static Bool<br>
> +XCreateDrawable(struct drisw_drawable * pdp, int shmid, Display * dpy)<br>
> +{<br>
> + if (pdp->ximage)<br>
> + XDestroyImage(pdp->ximage);<br>
> +<br>
> + if (shmid >= 0) {<br>
> + int (*old_handler)(Display *, XErrorEvent *);<br>
> +<br>
> + pdp->shminfo.shmid = shmid;<br>
> + pdp->ximage = XShmCreateImage(dpy,<br>
> + pdp->visinfo->visual,<br>
> + pdp->visinfo->depth,<br>
> + ZPixmap,<br>
> + NULL,<br>
> + &pdp->shminfo,<br>
> + 0, 0);<br>
> + if (pdp->ximage == NULL)<br>
> + goto ximage;<br>
> +<br>
> + XErrorFlag = 0;<br>
> + old_handler = XSetErrorHandler(handle_xerror);<br>
> + /* This may trigger the X protocol error we're ready to catch: */<br>
> + XShmAttach(dpy, &pdp->shminfo);<br>
> + XSync(dpy, False);<br>
> +<br>
> + if (!XErrorFlag)<br>
> + goto end;<br>
> +<br>
> + /* we are on a remote display, this error is normal, don't print it */<br>
> + XFlush(dpy);<br>
<br>
</div></div>XFlush after XSync is a no-op. (Well, unless other threads have<br>
touched the display, but even if they have there's no reason to flush<br>
here.)<br></blockquote><div><br></div><div>ok<br> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<span class=""><br>
> + XErrorFlag = 0;<br>
> + XDestroyImage(pdp->ximage);<br>
> + pdp->ximage = NULL;<br>
> + (void) XSetErrorHandler(old_handler);<br>
> + }<br>
<br>
</span>This is the non-shm fallback path; it's also the only place where<br>
you're restoring old_handler. Should do it right after XSync.<br></blockquote><div><br></div><div>ok<br> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<span class=""><br>
> @@ -144,6 +203,11 @@ swrastPutImage2(__DRIdrawable * draw, int op,<br>
> XImage *ximage;<br>
> GC gc;<br>
><br>
> + if (!pdp->ximage || shmid != pdp->shminfo.shmid) {<br>
> + if (!XCreateDrawable(pdp, shmid, dpy))<br>
> + return;<br>
> + }<br>
> +<br>
<br>
</span>Can you explain why you're doing image realization lazily instead of at<br>
createDrawable time?<br></blockquote><div><br></div><div>There is no allocation yet when createDrawable() is called<br><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
Also, you're punishing remote clients pretty harshly here. You've got<br>
dri_sw_displaytarget_display calling put_image_shm if the storage was<br>
allocated as a shm segment (shmid != -1), but pdp->shminfo.shmid is<br>
only set to shmid if ShmAttach succeeds. On remote connections it<br>
won't, but you take an XSync on every SwapBuffers to find that out.<br></blockquote><div><br></div><div>That's why there is a FIXME in check_xshm() :) But I suppose we could cache the first result too.<br><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<span class=""><br>
> @@ -156,24 +220,51 @@ swrastPutImage2(__DRIdrawable * draw, int op,<br>
> }<br>
><br>
> drawable = pdraw->xDrawable;<br>
> -<br>
> ximage = pdp->ximage;<br>
> - ximage->data = data;<br>
> - ximage->width = w;<br>
> - ximage->height = h;<br>
> ximage->bytes_per_line = stride ? stride : bytes_per_line(w * ximage->bits_per_pixel, 32);<br>
> + ximage->data = data;<br>
><br>
> - XPutImage(dpy, drawable, gc, ximage, 0, 0, x, y, w, h);<br>
> -<br>
> + if (pdp->shminfo.shmid >= 0) {<br>
> + ximage->width = ximage->bytes_per_line / 4;<br>
> + ximage->height = h;<br>
> + XShmPutImage(dpy, drawable, gc, ximage, 0, 0, x, y, w, h, False);<br>
<br>
</span>Pretty sure this is broken for non-32bpp.<br>
<br></blockquote><div><br></div><div>I'll try to check that too.<br><br></div><div>thanks<br></div></div><br><br clear="all"><br>-- <br><div class="gmail_signature">Marc-André Lureau</div>
</div></div>