Discussion:
[PATCH 01/11] loader_dri3/glx/egl: Remove the loader_dri3_vtable get_dri_screen callback
Add Reply
Thomas Hellstrom
2017-08-11 14:14:10 UTC
Reply
Permalink
Raw Message
It's not very usable since in the rare, but definitely existing case that
we don't have a current context, it will return NULL.

Presumably it will always be safe to use the dri screen the drawable was
created with for operations on that drawable.

Signed-off-by: Thomas Hellstrom <***@vmware.com>
---
src/egl/drivers/dri2/platform_x11_dri3.c | 12 ------------
src/glx/dri3_glx.c | 11 -----------
src/loader/loader_dri3_helper.c | 12 +-----------
src/loader/loader_dri3_helper.h | 1 -
4 files changed, 1 insertion(+), 35 deletions(-)

diff --git a/src/egl/drivers/dri2/platform_x11_dri3.c b/src/egl/drivers/dri2/platform_x11_dri3.c
index 9c01816..9917498 100644
--- a/src/egl/drivers/dri2/platform_x11_dri3.c
+++ b/src/egl/drivers/dri2/platform_x11_dri3.c
@@ -75,17 +75,6 @@ egl_dri3_get_dri_context(struct loader_dri3_drawable *draw)
return dri2_ctx->dri_context;
}

-static __DRIscreen *
-egl_dri3_get_dri_screen(struct loader_dri3_drawable *draw)
-{
- _EGLContext *ctx = _eglGetCurrentContext();
- struct dri2_egl_context *dri2_ctx;
- if (!ctx)
- return NULL;
- dri2_ctx = dri2_egl_context(ctx);
- return dri2_egl_display(dri2_ctx->base.Resource.Display)->dri_screen;
-}
-
static void
egl_dri3_flush_drawable(struct loader_dri3_drawable *draw, unsigned flags)
{
@@ -99,7 +88,6 @@ static const struct loader_dri3_vtable egl_dri3_vtable = {
.set_drawable_size = egl_dri3_set_drawable_size,
.in_current_context = egl_dri3_in_current_context,
.get_dri_context = egl_dri3_get_dri_context,
- .get_dri_screen = egl_dri3_get_dri_screen,
.flush_drawable = egl_dri3_flush_drawable,
.show_fps = NULL,
};
diff --git a/src/glx/dri3_glx.c b/src/glx/dri3_glx.c
index 024e8ab..dc94740 100644
--- a/src/glx/dri3_glx.c
+++ b/src/glx/dri3_glx.c
@@ -116,16 +116,6 @@ glx_dri3_get_dri_context(struct loader_dri3_drawable *draw)
return (gc != &dummyContext) ? dri3Ctx->driContext : NULL;
}

-static __DRIscreen *
-glx_dri3_get_dri_screen(struct loader_dri3_drawable *draw)
-{
- struct glx_context *gc = __glXGetCurrentContext();
- struct dri3_context *pcp = (struct dri3_context *) gc;
- struct dri3_screen *psc = (struct dri3_screen *) pcp->base.psc;
-
- return (gc != &dummyContext && psc) ? psc->driScreen : NULL;
-}
-
static void
glx_dri3_flush_drawable(struct loader_dri3_drawable *draw, unsigned flags)
{
@@ -160,7 +150,6 @@ static const struct loader_dri3_vtable glx_dri3_vtable = {
.set_drawable_size = glx_dri3_set_drawable_size,
.in_current_context = glx_dri3_in_current_context,
.get_dri_context = glx_dri3_get_dri_context,
- .get_dri_screen = glx_dri3_get_dri_screen,
.flush_drawable = glx_dri3_flush_drawable,
.show_fps = glx_dri3_show_fps,
};
diff --git a/src/loader/loader_dri3_helper.c b/src/loader/loader_dri3_helper.c
index 9d24130..5346d07 100644
--- a/src/loader/loader_dri3_helper.c
+++ b/src/loader/loader_dri3_helper.c
@@ -1116,7 +1116,6 @@ dri3_get_pixmap_buffer(__DRIdrawable *driDrawable, unsigned int format,
xcb_sync_fence_t sync_fence;
struct xshmfence *shm_fence;
int fence_fd;
- __DRIscreen *cur_screen;

if (buffer)
return buffer;
@@ -1147,17 +1146,8 @@ dri3_get_pixmap_buffer(__DRIdrawable *driDrawable, unsigned int format,
if (!bp_reply)
goto no_image;

- /* Get the currently-bound screen or revert to using the drawable's screen if
- * no contexts are currently bound. The latter case is at least necessary for
- * obs-studio, when using Window Capture (Xcomposite) as a Source.
- */
- cur_screen = draw->vtable->get_dri_screen(draw);
- if (!cur_screen) {
- cur_screen = draw->dri_screen;
- }
-
buffer->image = loader_dri3_create_image(draw->conn, bp_reply, format,
- cur_screen, draw->ext->image,
+ draw->dri_screen, draw->ext->image,
buffer);
if (!buffer->image)
goto no_image;
diff --git a/src/loader/loader_dri3_helper.h b/src/loader/loader_dri3_helper.h
index af42425..34498c9 100644
--- a/src/loader/loader_dri3_helper.h
+++ b/src/loader/loader_dri3_helper.h
@@ -100,7 +100,6 @@ struct loader_dri3_vtable {
void (*set_drawable_size)(struct loader_dri3_drawable *, int, int);
bool (*in_current_context)(struct loader_dri3_drawable *);
__DRIcontext *(*get_dri_context)(struct loader_dri3_drawable *);
- __DRIscreen *(*get_dri_screen)(struct loader_dri3_drawable *);
void (*flush_drawable)(struct loader_dri3_drawable *, unsigned);
void (*show_fps)(struct loader_dri3_drawable *, uint64_t);
};
--
2.7.4
Thomas Hellstrom
2017-08-11 14:14:11 UTC
Reply
Permalink
Raw Message
The code was relying on us always having a current context for client local
image blit operations. Otherwise the blit would be skipped. However,
glxSwapBuffers, for example, doesn't require a current context and that was a
common problem in the dri1 era. It seems the problem has resurfaced with dri3.

If we don't have a current context when we want to blit, try creating a private
dri context and maintain a context cache of a single context.

Signed-off-by: Thomas Hellstrom <***@vmware.com>
---
src/loader/loader_dri3_helper.c | 237 ++++++++++++++++++++++++++++------------
src/loader/loader_dri3_helper.h | 5 +
2 files changed, 175 insertions(+), 67 deletions(-)

diff --git a/src/loader/loader_dri3_helper.c b/src/loader/loader_dri3_helper.c
index 5346d07..4959e95 100644
--- a/src/loader/loader_dri3_helper.c
+++ b/src/loader/loader_dri3_helper.c
@@ -32,6 +32,7 @@

#include <X11/Xlib-xcb.h>

+#include <c11/threads.h>
#include "loader_dri3_helper.h"

/* From xmlpool/options.h, user exposed so should be stable */
@@ -40,6 +41,110 @@
#define DRI_CONF_VBLANK_DEF_INTERVAL_1 2
#define DRI_CONF_VBLANK_ALWAYS_SYNC 3

+/**
+ * A cached blit context.
+ */
+struct loader_dri3_blit_context {
+ mtx_t mtx;
+ __DRIcontext *ctx;
+ __DRIscreen *cur_screen;
+ const __DRIcoreExtension *core;
+};
+
+/* For simplicity we maintain the cache only for a single screen at a time */
+static struct loader_dri3_blit_context blit_context = {
+ _MTX_INITIALIZER_NP, NULL
+};
+
+
+/**
+ * Get and lock (for use with the current thread) a dri context associated
+ * with the drawable's dri screen. The context is intended to be used with
+ * the dri image extension's blitImage method.
+ *
+ * \param draw[in] Pointer to the drawable whose dri screen we want a
+ * dri context for.
+ * \return A dri context or NULL if context creation failed.
+ *
+ * When the caller is done with the context (even if the context returned was
+ * NULL), the caller must call loader_dri3_blit_context_put.
+ */
+static __DRIcontext *
+loader_dri3_blit_context_get(struct loader_dri3_drawable *draw)
+{
+ mtx_lock(&blit_context.mtx);
+
+ if (blit_context.ctx && blit_context.cur_screen != draw->dri_screen) {
+ blit_context.core->destroyContext(blit_context.ctx);
+ blit_context.ctx = NULL;
+ }
+
+ if (!blit_context.ctx) {
+ blit_context.ctx = draw->ext->core->createNewContext(draw->dri_screen,
+ NULL, NULL, NULL);
+ blit_context.cur_screen = draw->dri_screen;
+ blit_context.core = draw->ext->core;
+ }
+
+ return blit_context.ctx;
+}
+
+/**
+ * Release (for use with other threads) a dri context previously obtained using
+ * loader_dri3_blit_context_get.
+ */
+static void
+loader_dri3_blit_context_put(void)
+{
+ mtx_unlock(&blit_context.mtx);
+}
+
+/**
+ * Blit (parts of) the contents of a DRI image to another dri image
+ *
+ * \param draw[in] The drawable which owns the images.
+ * \param dst[in] The destination image.
+ * \param src[in] The source image.
+ * \param dstx0[in] Start destination coordinate.
+ * \param dsty0[in] Start destination coordinate.
+ * \param width[in] Blit width.
+ * \param height[in] Blit height.
+ * \param srcx0[in] Start source coordinate.
+ * \param srcy0[in] Start source coordinate.
+ * \param flush_flag[in] Image blit flush flag.
+ * \return true iff successful.
+ */
+static bool
+loader_dri3_blit_image(struct loader_dri3_drawable *draw,
+ __DRIimage *dst, __DRIimage *src,
+ int dstx0, int dsty0, int width, int height,
+ int srcx0, int srcy0, int flush_flag)
+{
+ __DRIcontext *dri_context;
+ bool use_blit_context = false;
+
+ if (!draw->have_image_blit)
+ return false;
+
+ dri_context = draw->vtable->get_dri_context(draw);
+
+ if (!dri_context || !draw->vtable->in_current_context(draw)) {
+ dri_context = loader_dri3_blit_context_get(draw);
+ use_blit_context = true;
+ flush_flag |= __BLIT_FLAG_FLUSH;
+ }
+
+ if (dri_context)
+ draw->ext->image->blitImage(dri_context, dst, src, dstx0, dsty0,
+ width, height, srcx0, srcy0,
+ width, height, flush_flag);
+
+ if (use_blit_context)
+ loader_dri3_blit_context_put();
+
+ return dri_context != NULL;
+}
+
static inline void
dri3_fence_reset(xcb_connection_t *c, struct loader_dri3_buffer *buffer)
{
@@ -149,6 +254,9 @@ loader_dri3_drawable_init(xcb_connection_t *conn,
draw->have_fake_front = 0;
draw->first_init = true;

+ draw->have_image_blit = draw->ext->image->base.version >= 9 &&
+ draw->ext->image->blitImage != NULL;
+
if (draw->ext->config)
draw->ext->config->configQueryi(draw->dri_screen,
"vblank_mode", &vblank_mode);
@@ -467,9 +575,6 @@ loader_dri3_copy_sub_buffer(struct loader_dri3_drawable *draw,
{
struct loader_dri3_buffer *back;
unsigned flags = __DRI2_FLUSH_DRAWABLE;
- __DRIcontext *dri_context;
-
- dri_context = draw->vtable->get_dri_context(draw);

/* Check we have the right attachments */
if (!draw->have_back || draw->is_pixmap)
@@ -482,25 +587,23 @@ loader_dri3_copy_sub_buffer(struct loader_dri3_drawable *draw,
back = dri3_back_buffer(draw);
y = draw->height - y - height;

- if (draw->is_different_gpu && draw->vtable->in_current_context(draw)) {
+ if (draw->is_different_gpu) {
/* Update the linear buffer part of the back buffer
* for the dri3_copy_area operation
*/
- draw->ext->image->blitImage(dri_context,
- back->linear_buffer,
- back->image,
- 0, 0, back->width,
- back->height,
- 0, 0, back->width,
- back->height, __BLIT_FLAG_FLUSH);
- /* We use blitImage to update our fake front,
+ (void) loader_dri3_blit_image(draw,
+ back->linear_buffer,
+ back->image,
+ 0, 0, back->width, back->height,
+ 0, 0, __BLIT_FLAG_FLUSH);
+ /* We use blit_image to update our fake front,
*/
if (draw->have_fake_front)
- draw->ext->image->blitImage(dri_context,
- dri3_fake_front_buffer(draw)->image,
- back->image,
- x, y, width, height,
- x, y, width, height, __BLIT_FLAG_FLUSH);
+ (void) loader_dri3_blit_image(draw,
+ dri3_fake_front_buffer(draw)->image,
+ back->image,
+ x, y, width, height,
+ x, y, __BLIT_FLAG_FLUSH);
}

loader_dri3_swapbuffer_barrier(draw);
@@ -547,13 +650,11 @@ void
loader_dri3_wait_x(struct loader_dri3_drawable *draw)
{
struct loader_dri3_buffer *front;
- __DRIcontext *dri_context;

if (draw == NULL || !draw->have_fake_front)
return;

front = dri3_fake_front_buffer(draw);
- dri_context = draw->vtable->get_dri_context(draw);

loader_dri3_copy_drawable(draw, front->pixmap, draw->drawable);

@@ -562,39 +663,33 @@ loader_dri3_wait_x(struct loader_dri3_drawable *draw)
* Copy back to the tiled buffer we use for rendering.
* Note that we don't need flushing.
*/
- if (draw->is_different_gpu && draw->vtable->in_current_context(draw))
- draw->ext->image->blitImage(dri_context,
- front->image,
- front->linear_buffer,
- 0, 0, front->width,
- front->height,
- 0, 0, front->width,
- front->height, 0);
+ if (draw->is_different_gpu)
+ (void) loader_dri3_blit_image(draw,
+ front->image,
+ front->linear_buffer,
+ 0, 0, front->width, front->height,
+ 0, 0, 0);
}

void
loader_dri3_wait_gl(struct loader_dri3_drawable *draw)
{
struct loader_dri3_buffer *front;
- __DRIcontext *dri_context;

if (draw == NULL || !draw->have_fake_front)
return;

front = dri3_fake_front_buffer(draw);
- dri_context = draw->vtable->get_dri_context(draw);

/* In the psc->is_different_gpu case, we update the linear_buffer
* before updating the real front.
*/
- if (draw->is_different_gpu && draw->vtable->in_current_context(draw))
- draw->ext->image->blitImage(dri_context,
- front->linear_buffer,
- front->image,
- 0, 0, front->width,
- front->height,
- 0, 0, front->width,
- front->height, __BLIT_FLAG_FLUSH);
+ if (draw->is_different_gpu)
+ (void) loader_dri3_blit_image(draw,
+ front->linear_buffer,
+ front->image,
+ 0, 0, front->width, front->height,
+ 0, 0, __BLIT_FLAG_FLUSH);
loader_dri3_swapbuffer_barrier(draw);
loader_dri3_copy_drawable(draw, draw->drawable, front->pixmap);
}
@@ -631,32 +726,26 @@ loader_dri3_swap_buffers_msc(struct loader_dri3_drawable *draw,
bool force_copy)
{
struct loader_dri3_buffer *back;
- __DRIcontext *dri_context;
int64_t ret = 0;
uint32_t options = XCB_PRESENT_OPTION_NONE;

- dri_context = draw->vtable->get_dri_context(draw);
-
draw->vtable->flush_drawable(draw, flush_flags);

back = draw->buffers[dri3_find_back(draw)];
if (draw->is_different_gpu && back) {
/* Update the linear buffer before presenting the pixmap */
- draw->ext->image->blitImage(dri_context,
- back->linear_buffer,
- back->image,
- 0, 0, back->width,
- back->height,
- 0, 0, back->width,
- back->height, __BLIT_FLAG_FLUSH);
+ (void) loader_dri3_blit_image(draw,
+ back->linear_buffer,
+ back->image,
+ 0, 0, back->width, back->height,
+ 0, 0, __BLIT_FLAG_FLUSH);
/* Update the fake front */
if (draw->have_fake_front)
- draw->ext->image->blitImage(dri_context,
- draw->buffers[LOADER_DRI3_FRONT_ID]->image,
- back->image,
- 0, 0, draw->width, draw->height,
- 0, 0, draw->width, draw->height,
- __BLIT_FLAG_FLUSH);
+ (void) loader_dri3_blit_image(draw,
+ draw->buffers[LOADER_DRI3_FRONT_ID]->image,
+ back->image,
+ 0, 0, draw->width, draw->height,
+ 0, 0, __BLIT_FLAG_FLUSH);
}

dri3_flush_present_events(draw);
@@ -1188,9 +1277,6 @@ dri3_get_buffer(__DRIdrawable *driDrawable,
{
struct loader_dri3_buffer *buffer;
int buf_id;
- __DRIcontext *dri_context;
-
- dri_context = draw->vtable->get_dri_context(draw);

if (buffer_type == loader_dri3_buffer_back) {
buf_id = dri3_find_back(draw);
@@ -1237,11 +1323,11 @@ dri3_get_buffer(__DRIdrawable *driDrawable,
draw->width, draw->height);
dri3_fence_trigger(draw->conn, new_buffer);
} else if (draw->vtable->in_current_context(draw)) {
- draw->ext->image->blitImage(dri_context,
- new_buffer->image,
- buffer->image,
- 0, 0, draw->width, draw->height,
- 0, 0, draw->width, draw->height, 0);
+ (void) loader_dri3_blit_image(draw,
+ new_buffer->image,
+ buffer->image,
+ 0, 0, draw->width, draw->height,
+ 0, 0, 0);
}
dri3_free_render_buffer(draw, buffer);
}
@@ -1260,11 +1346,11 @@ dri3_get_buffer(__DRIdrawable *driDrawable,
if (new_buffer->linear_buffer &&
draw->vtable->in_current_context(draw)) {
dri3_fence_await(draw->conn, new_buffer);
- draw->ext->image->blitImage(dri_context,
- new_buffer->image,
- new_buffer->linear_buffer,
- 0, 0, draw->width, draw->height,
- 0, 0, draw->width, draw->height, 0);
+ (void) loader_dri3_blit_image(draw,
+ new_buffer->image,
+ new_buffer->linear_buffer,
+ 0, 0, draw->width, draw->height,
+ 0, 0, 0);
}
break;
}
@@ -1340,7 +1426,7 @@ loader_dri3_get_buffers(__DRIdrawable *driDrawable,
return false;

/* pixmaps always have front buffers */
- if (draw->is_pixmap)
+// if (draw->is_pixmap)
buffer_mask |= __DRI_IMAGE_BUFFER_FRONT;

if (buffer_mask & __DRI_IMAGE_BUFFER_FRONT) {
@@ -1436,3 +1522,20 @@ loader_dri3_swapbuffer_barrier(struct loader_dri3_drawable *draw)

(void) loader_dri3_wait_for_sbc(draw, 0, &ust, &msc, &sbc);
}
+
+/**
+ * Perform any cleanup associated with a close screen operation.
+ * \param dri_screen[in,out] Pointer to __DRIscreen about to be closed.
+ *
+ * This function destroys the screen's cached swap context if any.
+ */
+void
+loader_dri3_close_screen(__DRIscreen *dri_screen)
+{
+ mtx_lock(&blit_context.mtx);
+ if (blit_context.ctx && blit_context.cur_screen == dri_screen) {
+ blit_context.core->destroyContext(blit_context.ctx);
+ blit_context.ctx = NULL;
+ }
+ mtx_unlock(&blit_context.mtx);
+}
diff --git a/src/loader/loader_dri3_helper.h b/src/loader/loader_dri3_helper.h
index 34498c9..be41da9 100644
--- a/src/loader/loader_dri3_helper.h
+++ b/src/loader/loader_dri3_helper.h
@@ -157,6 +157,8 @@ struct loader_dri3_drawable {

struct loader_dri3_extensions *ext;
const struct loader_dri3_vtable *vtable;
+
+ bool have_image_blit;
};

void
@@ -241,4 +243,7 @@ loader_dri3_update_drawable_geometry(struct loader_dri3_drawable *draw);

void
loader_dri3_swapbuffer_barrier(struct loader_dri3_drawable *draw);
+
+void
+loader_dri3_close_screen(__DRIscreen *dri_screen);
#endif
--
2.7.4
Michel Dänzer
2017-08-15 06:57:18 UTC
Reply
Permalink
Raw Message
Post by Thomas Hellstrom
The code was relying on us always having a current context for client local
image blit operations. Otherwise the blit would be skipped. However,
glxSwapBuffers, for example, doesn't require a current context and that was a
common problem in the dri1 era. It seems the problem has resurfaced with dri3.
If we don't have a current context when we want to blit, try creating a private
dri context and maintain a context cache of a single context.
[...]
Post by Thomas Hellstrom
+/**
+ * A cached blit context.
+ */
+struct loader_dri3_blit_context {
+ mtx_t mtx;
+ __DRIcontext *ctx;
+ __DRIscreen *cur_screen;
The cur_screen field seems redundant with __DRIcontextRec::driScreenPriv.
Post by Thomas Hellstrom
@@ -149,6 +254,9 @@ loader_dri3_drawable_init(xcb_connection_t *conn,
draw->have_fake_front = 0;
draw->first_init = true;
+ draw->have_image_blit = draw->ext->image->base.version >= 9 &&
+ draw->ext->image->blitImage != NULL;
Is it really worth having a dedicated drawable field for this? Seems
like open-coding this in loader_dri3_blit_image should be fine.
Post by Thomas Hellstrom
@@ -1340,7 +1426,7 @@ loader_dri3_get_buffers(__DRIdrawable *driDrawable,
return false;
/* pixmaps always have front buffers */
- if (draw->is_pixmap)
+// if (draw->is_pixmap)
buffer_mask |= __DRI_IMAGE_BUFFER_FRONT;
Either leave the line uncommented, or remove it and fix up the
indentation of the following line.
--
Earthling Michel Dänzer | http://www.amd.com
Libre software enthusiast | Mesa and X developer
Michel Dänzer
2017-08-15 07:33:29 UTC
Reply
Permalink
Raw Message
Post by Michel Dänzer
Post by Thomas Hellstrom
The code was relying on us always having a current context for client local
image blit operations. Otherwise the blit would be skipped. However,
glxSwapBuffers, for example, doesn't require a current context and that was a
common problem in the dri1 era. It seems the problem has resurfaced with dri3.
If we don't have a current context when we want to blit, try creating a private
dri context and maintain a context cache of a single context.
[...]
Post by Thomas Hellstrom
@@ -149,6 +254,9 @@ loader_dri3_drawable_init(xcb_connection_t *conn,
draw->have_fake_front = 0;
draw->first_init = true;
+ draw->have_image_blit = draw->ext->image->base.version >= 9 &&
+ draw->ext->image->blitImage != NULL;
Is it really worth having a dedicated drawable field for this? Seems
like open-coding this in loader_dri3_blit_image should be fine.
I see the field is used in more places in later patches, but I still
think making it a helper function instead of a dedicated field should be
fine.
--
Earthling Michel Dänzer | http://www.amd.com
Libre software enthusiast | Mesa and X developer
Thomas Hellstrom
2017-08-15 12:38:38 UTC
Reply
Permalink
Raw Message
Hi, Michel, Thanks for reviewing.
Post by Michel Dänzer
Post by Thomas Hellstrom
The code was relying on us always having a current context for client local
image blit operations. Otherwise the blit would be skipped. However,
glxSwapBuffers, for example, doesn't require a current context and that was a
common problem in the dri1 era. It seems the problem has resurfaced with dri3.
If we don't have a current context when we want to blit, try creating a private
dri context and maintain a context cache of a single context.
[...]
Post by Thomas Hellstrom
+/**
+ * A cached blit context.
+ */
+struct loader_dri3_blit_context {
+ mtx_t mtx;
+ __DRIcontext *ctx;
+ __DRIscreen *cur_screen;
The cur_screen field seems redundant with __DRIcontextRec::driScreenPriv.
Problem is __DRIcontextRec is opaque here, so we can't access its members.
Post by Michel Dänzer
Post by Thomas Hellstrom
@@ -149,6 +254,9 @@ loader_dri3_drawable_init(xcb_connection_t *conn,
draw->have_fake_front = 0;
draw->first_init = true;
+ draw->have_image_blit = draw->ext->image->base.version >= 9 &&
+ draw->ext->image->blitImage != NULL;
Is it really worth having a dedicated drawable field for this? Seems
like open-coding this in loader_dri3_blit_image should be fine.
Sure, will do.
Post by Michel Dänzer
Post by Thomas Hellstrom
@@ -1340,7 +1426,7 @@ loader_dri3_get_buffers(__DRIdrawable *driDrawable,
return false;
/* pixmaps always have front buffers */
- if (draw->is_pixmap)
+// if (draw->is_pixmap)
buffer_mask |= __DRI_IMAGE_BUFFER_FRONT;
Hmm, this shouldn't be here. It's a leftover debug thing. I'll fix it up
to the next version.
Post by Michel Dänzer
Either leave the line uncommented, or remove it and fix up the
indentation of the following line.
Thanks,

Thomas
Thomas Hellstrom
2017-08-11 14:14:13 UTC
Reply
Permalink
Raw Message
This gives the dri3 loader a chance to clean up the blit context cache.

Signed-off-by: Thomas Hellstrom <***@vmware.com>
---
src/egl/drivers/dri2/egl_dri2.c | 5 ++++-
src/egl/drivers/dri2/egl_dri2.h | 2 ++
src/egl/drivers/dri2/platform_x11_dri3.c | 9 +++++++++
3 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/src/egl/drivers/dri2/egl_dri2.c b/src/egl/drivers/dri2/egl_dri2.c
index f584740..a6ac49a 100644
--- a/src/egl/drivers/dri2/egl_dri2.c
+++ b/src/egl/drivers/dri2/egl_dri2.c
@@ -945,8 +945,11 @@ dri2_display_destroy(_EGLDisplay *disp)
{
struct dri2_egl_display *dri2_dpy = dri2_egl_display(disp);

- if (dri2_dpy->own_dri_screen)
+ if (dri2_dpy->own_dri_screen) {
+ if (dri2_dpy->vtbl->close_screen_notify)
+ dri2_dpy->vtbl->close_screen_notify(disp);
dri2_dpy->core->destroyScreen(dri2_dpy->dri_screen);
+ }
if (dri2_dpy->fd >= 0)
close(dri2_dpy->fd);
if (dri2_dpy->driver)
diff --git a/src/egl/drivers/dri2/egl_dri2.h b/src/egl/drivers/dri2/egl_dri2.h
index 751e7a4..f471561 100644
--- a/src/egl/drivers/dri2/egl_dri2.h
+++ b/src/egl/drivers/dri2/egl_dri2.h
@@ -154,6 +154,8 @@ struct dri2_egl_display_vtbl {
EGLuint64KHR *sbc);

__DRIdrawable *(*get_dri_drawable)(_EGLSurface *surf);
+
+ void (*close_screen_notify)(_EGLDisplay *dpy);
};

struct dri2_egl_display
diff --git a/src/egl/drivers/dri2/platform_x11_dri3.c b/src/egl/drivers/dri2/platform_x11_dri3.c
index 9917498..290b150 100644
--- a/src/egl/drivers/dri2/platform_x11_dri3.c
+++ b/src/egl/drivers/dri2/platform_x11_dri3.c
@@ -401,6 +401,14 @@ dri3_get_dri_drawable(_EGLSurface *surf)
return dri3_surf->loader_drawable.dri_drawable;
}

+static void
+dri3_close_screen_notify(_EGLDisplay *dpy)
+{
+ struct dri2_egl_display *dri2_dpy = dri2_egl_display(dpy);
+
+ loader_dri3_close_screen(dri2_dpy->dri_screen);
+}
+
struct dri2_egl_display_vtbl dri3_x11_display_vtbl = {
.authenticate = dri3_authenticate,
.create_window_surface = dri3_create_window_surface,
@@ -420,6 +428,7 @@ struct dri2_egl_display_vtbl dri3_x11_display_vtbl = {
.create_wayland_buffer_from_image = dri2_fallback_create_wayland_buffer_from_image,
.get_sync_values = dri3_get_sync_values,
.get_dri_drawable = dri3_get_dri_drawable,
+ .close_screen_notify = dri3_close_screen_notify,
};

EGLBoolean
--
2.7.4
Thomas Hellstrom
2017-08-11 14:14:12 UTC
Reply
Permalink
Raw Message
This gives the dri3 loader a chance to clean up the blit context cache.

Signed-off-by: Thomas Hellstrom <***@vmware.com>
---
src/glx/dri3_glx.c | 1 +
1 file changed, 1 insertion(+)

diff --git a/src/glx/dri3_glx.c b/src/glx/dri3_glx.c
index dc94740..b79fec7 100644
--- a/src/glx/dri3_glx.c
+++ b/src/glx/dri3_glx.c
@@ -572,6 +572,7 @@ dri3_destroy_screen(struct glx_screen *base)
struct dri3_screen *psc = (struct dri3_screen *) base;

/* Free the direct rendering per screen data */
+ loader_dri3_close_screen(psc->driScreen);
(*psc->core->destroyScreen) (psc->driScreen);
driDestroyConfigs(psc->driver_configs);
close(psc->fd);
--
2.7.4
Thomas Hellstrom
2017-08-11 14:14:17 UTC
Reply
Permalink
Raw Message
It's not used anywhere and now that we're about to exchange back- and
fake fronts it doesn't serve a purpose.

Signed-off-by: Thomas Hellstrom <***@vmware.com>
---
src/loader/loader_dri3_helper.c | 2 --
src/loader/loader_dri3_helper.h | 2 --
2 files changed, 4 deletions(-)

diff --git a/src/loader/loader_dri3_helper.c b/src/loader/loader_dri3_helper.c
index 774256f..2787f9f 100644
--- a/src/loader/loader_dri3_helper.c
+++ b/src/loader/loader_dri3_helper.c
@@ -1279,7 +1279,6 @@ dri3_get_pixmap_buffer(__DRIdrawable *driDrawable, unsigned int format,
buffer->own_pixmap = false;
buffer->width = bp_reply->width;
buffer->height = bp_reply->height;
- buffer->buffer_type = buffer_type;
buffer->shm_fence = shm_fence;
buffer->sync_fence = sync_fence;

@@ -1389,7 +1388,6 @@ dri3_get_buffer(__DRIdrawable *driDrawable,
break;
}
buffer = new_buffer;
- buffer->buffer_type = buffer_type;
draw->buffers[buf_id] = buffer;
}
dri3_fence_await(draw->conn, buffer);
diff --git a/src/loader/loader_dri3_helper.h b/src/loader/loader_dri3_helper.h
index f7db45d..c8f63fd 100644
--- a/src/loader/loader_dri3_helper.h
+++ b/src/loader/loader_dri3_helper.h
@@ -67,8 +67,6 @@ struct loader_dri3_buffer {
uint32_t flags;
uint32_t width, height;
uint64_t last_swap;
-
- enum loader_dri3_buffer_type buffer_type;
};
--
2.7.4
Thomas Hellstrom
2017-08-11 14:14:16 UTC
Reply
Permalink
Raw Message
Support the GLX_SWAP_COPY_OML method. When this method is requested, we use
the same swapbuffer code path as EGL_BUFFER_PRESERVED.

Signed-off-by: Thomas Hellstrom <***@vmware.com>
---
src/loader/loader_dri3_helper.c | 9 ++++++++-
src/loader/loader_dri3_helper.h | 1 +
2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/src/loader/loader_dri3_helper.c b/src/loader/loader_dri3_helper.c
index 812adcc..774256f 100644
--- a/src/loader/loader_dri3_helper.c
+++ b/src/loader/loader_dri3_helper.c
@@ -301,6 +301,13 @@ loader_dri3_drawable_init(xcb_connection_t *conn,
draw->vtable->set_drawable_size(draw, draw->width, draw->height);
free(reply);

+ draw->swap_method = __DRI_ATTRIB_SWAP_UNDEFINED;
+ if (draw->ext->core->base.version >= 2) {
+ (void )draw->ext->core->getConfigAttrib(dri_config,
+ __DRI_ATTRIB_SWAP_METHOD,
+ &draw->swap_method);
+ }
+
/*
* Make sure server has the same swap interval we do for the new
* drawable.
@@ -767,7 +774,7 @@ loader_dri3_swap_buffers_msc(struct loader_dri3_drawable *draw,
* The force_copy parameter is used by EGL to attempt to preserve
* the back buffer across a call to this function.
*/
- if (force_copy)
+ if (draw->swap_method == __DRI_ATTRIB_SWAP_COPY || force_copy)
draw->cur_blit_source = LOADER_DRI3_BACK_ID(draw->cur_back);

dri3_flush_present_events(draw);
diff --git a/src/loader/loader_dri3_helper.h b/src/loader/loader_dri3_helper.h
index 82c5363..f7db45d 100644
--- a/src/loader/loader_dri3_helper.h
+++ b/src/loader/loader_dri3_helper.h
@@ -160,6 +160,7 @@ struct loader_dri3_drawable {
const struct loader_dri3_vtable *vtable;

bool have_image_blit;
+ unsigned int swap_method;
};

void
--
2.7.4
Thomas Hellstrom
2017-08-11 14:14:18 UTC
Reply
Permalink
Raw Message
Eliminate the back-to-fake-front copy by exchanging the previous back buffer
and the fake front buffer. This is a gain except when we need to preserve
the back buffer content but in that case we still typically gain by replacing
a server-side blit by a client side non-flushing blit.

Signed-off-by: Thomas Hellstrom <***@vmware.com>
---
src/loader/loader_dri3_helper.c | 50 +++++++++++++++++++++++++----------------
1 file changed, 31 insertions(+), 19 deletions(-)

diff --git a/src/loader/loader_dri3_helper.c b/src/loader/loader_dri3_helper.c
index 2787f9f..7056f46 100644
--- a/src/loader/loader_dri3_helper.c
+++ b/src/loader/loader_dri3_helper.c
@@ -761,13 +761,6 @@ loader_dri3_swap_buffers_msc(struct loader_dri3_drawable *draw,
back->image,
0, 0, back->width, back->height,
0, 0, __BLIT_FLAG_FLUSH);
- /* Update the fake front */
- if (draw->have_fake_front)
- (void) loader_dri3_blit_image(draw,
- draw->buffers[LOADER_DRI3_FRONT_ID]->image,
- back->image,
- 0, 0, draw->width, draw->height,
- 0, 0, __BLIT_FLAG_FLUSH);
}

/* If we need to preload the new back buffer, remember the source.
@@ -777,6 +770,20 @@ loader_dri3_swap_buffers_msc(struct loader_dri3_drawable *draw,
if (draw->swap_method == __DRI_ATTRIB_SWAP_COPY || force_copy)
draw->cur_blit_source = LOADER_DRI3_BACK_ID(draw->cur_back);

+ /* Exchange the back and fake front. Even though the server knows about these
+ * buffers, it has no notion of back and fake front.
+ */
+ if (back && draw->have_fake_front) {
+ struct loader_dri3_buffer *tmp;
+
+ tmp = dri3_fake_front_buffer(draw);
+ draw->buffers[LOADER_DRI3_FRONT_ID] = back;
+ draw->buffers[LOADER_DRI3_BACK_ID(draw->cur_back)] = tmp;
+
+ if (draw->swap_method == __DRI_ATTRIB_SWAP_COPY || force_copy)
+ draw->cur_blit_source = LOADER_DRI3_FRONT_ID;
+ }
+
dri3_flush_present_events(draw);

if (back && !draw->is_pixmap) {
@@ -842,21 +849,26 @@ loader_dri3_swap_buffers_msc(struct loader_dri3_drawable *draw,
remainder, 0, NULL);
ret = (int64_t) draw->send_sbc;

- /* If there's a fake front, then copy the source back buffer
- * to the fake front to keep it up to date. This needs
- * to reset the fence and make future users block until
- * the X server is done copying the bits
+ /* Schedule a server-side back-preserving blit if necessary.
+ * This happens iff all conditions below are satisfied:
+ * a) We have a fake front,
+ * b) We need to preserve the back buffer,
+ * c) We don't have local blit capabilities.
*/
- if (draw->have_fake_front && !draw->is_different_gpu) {
- dri3_fence_reset(draw->conn, draw->buffers[LOADER_DRI3_FRONT_ID]);
- dri3_copy_area(draw->conn,
- back->pixmap,
- draw->buffers[LOADER_DRI3_FRONT_ID]->pixmap,
+ if (!draw->have_image_blit && draw->cur_blit_source != -1 &&
+ draw->cur_blit_source != LOADER_DRI3_BACK_ID(draw->cur_back)) {
+ struct loader_dri3_buffer *new_back = dri3_back_buffer(draw);
+ struct loader_dri3_buffer *src = draw->buffers[draw->cur_blit_source];
+
+ dri3_fence_reset(draw->conn, new_back);
+ dri3_copy_area(draw->conn, src->pixmap,
+ new_back->pixmap,
dri3_drawable_gc(draw),
- 0, 0, 0, 0,
- draw->width, draw->height);
- dri3_fence_trigger(draw->conn, draw->buffers[LOADER_DRI3_FRONT_ID]);
+ 0, 0, 0, 0, draw->width, draw->height);
+ dri3_fence_trigger(draw->conn, new_back);
+ new_back->last_swap = src->last_swap;
}
+
xcb_flush(draw->conn);
if (draw->stamp)
++(*draw->stamp);
--
2.7.4
Thomas Hellstrom
2017-08-11 14:14:15 UTC
Reply
Permalink
Raw Message
EGL uses the force_copy parameter to loader_dri3_swap_buffers_msc() to indicate
that it wants to preserve back buffer contents across a buffer swap.

While the loader then turns off server-side page-flipping there's nothing to
guarantee that a new backbuffer isn't chosen when EGL starts to render again,
and that buffer's content is of course undefined.

So rework the functionality:
If the client supports local blits, allow server-side page flipping and when
a new back is grabbed, if needed, blit the old back's content to the new back.
If the client doesn't support local blits, disallow server-side page-flipping
to avoid a client deadlock and then, when grabbing a new back buffer, sleep
until the old back is idle, which may take a substantial time depending on
swap interval.

Signed-off-by: Thomas Hellstrom <***@vmware.com>
---
src/loader/loader_dri3_helper.c | 55 ++++++++++++++++++++++++++++++++++++++---
src/loader/loader_dri3_helper.h | 1 +
2 files changed, 52 insertions(+), 4 deletions(-)

diff --git a/src/loader/loader_dri3_helper.c b/src/loader/loader_dri3_helper.c
index 6f84a43..812adcc 100644
--- a/src/loader/loader_dri3_helper.c
+++ b/src/loader/loader_dri3_helper.c
@@ -258,6 +258,7 @@ loader_dri3_drawable_init(xcb_connection_t *conn,

draw->have_image_blit = draw->ext->image->base.version >= 9 &&
draw->ext->image->blitImage != NULL;
+ draw->cur_blit_source = -1;

if (draw->ext->config)
draw->ext->config->configQueryi(draw->dri_screen,
@@ -475,12 +476,21 @@ dri3_find_back(struct loader_dri3_drawable *draw)
int b;
xcb_generic_event_t *ev;
xcb_present_generic_event_t *ge;
+ int num_to_consider = draw->num_back;

/* Increase the likelyhood of reusing current buffer */
dri3_flush_present_events(draw);

+ /* Check whether we need to reuse the current back buffer as new back.
+ * In that case, wait until it's not busy anymore.
+ */
+ if (!draw->have_image_blit && draw->cur_blit_source != -1) {
+ num_to_consider = 1;
+ draw->cur_blit_source = -1;
+ }
+
for (;;) {
- for (b = 0; b < draw->num_back; b++) {
+ for (b = 0; b < num_to_consider; b++) {
int id = LOADER_DRI3_BACK_ID((b + draw->cur_back) % draw->num_back);
struct loader_dri3_buffer *buffer = draw->buffers[id];

@@ -753,6 +763,13 @@ loader_dri3_swap_buffers_msc(struct loader_dri3_drawable *draw,
0, 0, __BLIT_FLAG_FLUSH);
}

+ /* If we need to preload the new back buffer, remember the source.
+ * The force_copy parameter is used by EGL to attempt to preserve
+ * the back buffer across a call to this function.
+ */
+ if (force_copy)
+ draw->cur_blit_source = LOADER_DRI3_BACK_ID(draw->cur_back);
+
dri3_flush_present_events(draw);

if (back && !draw->is_pixmap) {
@@ -791,8 +808,13 @@ loader_dri3_swap_buffers_msc(struct loader_dri3_drawable *draw,
*/
if (draw->swap_interval == 0)
options |= XCB_PRESENT_OPTION_ASYNC;
- if (force_copy)
- options |= XCB_PRESENT_OPTION_COPY;
+
+ /* If we need to populate the new back, but need to reuse the back
+ * buffer slot due to lack of local blit capabilities, make sure
+ * the server doesn't flip and we deadlock.
+ */
+ if (!draw->have_image_blit && draw->cur_blit_source != -1)
+ options |= XCB_PRESENT_OPTION_COPY;

back->busy = 1;
back->last_swap = draw->send_sbc;
@@ -1365,6 +1387,31 @@ dri3_get_buffer(__DRIdrawable *driDrawable,
}
dri3_fence_await(draw->conn, buffer);

+ /*
+ * Do we need to preserve the content of a previous buffer?
+ *
+ * Note that this blit is needed only to avoid a wait for a buffer that
+ * is currently in the flip chain or being scanned out from. That's really
+ * a tradeoff. If we're ok with the wait we can reduce the number of back
+ * buffers to 1 for SWAP_EXCHANGE, and 1 for SWAP_COPY,
+ * but in the latter case we must disallow page-flipping.
+ */
+ if (buffer_type == loader_dri3_buffer_back &&
+ draw->cur_blit_source != -1 &&
+ draw->buffers[draw->cur_blit_source] &&
+ buffer != draw->buffers[draw->cur_blit_source]) {
+
+ struct loader_dri3_buffer *source = draw->buffers[draw->cur_blit_source];
+
+ /* Avoid flushing here. Will propably do good for tiling hardware. */
+ (void) loader_dri3_blit_image(draw,
+ buffer->image,
+ source->image,
+ 0, 0, draw->width, draw->height,
+ 0, 0, 0);
+ buffer->last_swap = source->last_swap;
+ draw->cur_blit_source = -1;
+ }
/* Return the requested buffer */
return buffer;
}
@@ -1431,7 +1478,7 @@ loader_dri3_get_buffers(__DRIdrawable *driDrawable,
return false;

/* pixmaps always have front buffers */
-// if (draw->is_pixmap)
+ if (draw->is_pixmap)
buffer_mask |= __DRI_IMAGE_BUFFER_FRONT;

if (buffer_mask & __DRI_IMAGE_BUFFER_FRONT) {
diff --git a/src/loader/loader_dri3_helper.h b/src/loader/loader_dri3_helper.h
index be41da9..82c5363 100644
--- a/src/loader/loader_dri3_helper.h
+++ b/src/loader/loader_dri3_helper.h
@@ -145,6 +145,7 @@ struct loader_dri3_drawable {
struct loader_dri3_buffer *buffers[LOADER_DRI3_NUM_BUFFERS];
int cur_back;
int num_back;
+ int cur_blit_source;

uint32_t *stamp;
--
2.7.4
Michel Dänzer
2017-08-15 07:33:53 UTC
Reply
Permalink
Raw Message
Post by Thomas Hellstrom
EGL uses the force_copy parameter to loader_dri3_swap_buffers_msc() to indicate
that it wants to preserve back buffer contents across a buffer swap.
While the loader then turns off server-side page-flipping there's nothing to
guarantee that a new backbuffer isn't chosen when EGL starts to render again,
and that buffer's content is of course undefined.
If the client supports local blits, allow server-side page flipping and when
a new back is grabbed, if needed, blit the old back's content to the new back.
If the client doesn't support local blits, disallow server-side page-flipping
to avoid a client deadlock and then, when grabbing a new back buffer, sleep
until the old back is idle, which may take a substantial time depending on
swap interval.
[...]
Post by Thomas Hellstrom
@@ -475,12 +476,21 @@ dri3_find_back(struct loader_dri3_drawable *draw)
int b;
xcb_generic_event_t *ev;
xcb_present_generic_event_t *ge;
+ int num_to_consider = draw->num_back;
/* Increase the likelyhood of reusing current buffer */
dri3_flush_present_events(draw);
+ /* Check whether we need to reuse the current back buffer as new back.
+ * In that case, wait until it's not busy anymore.
+ */
+ if (!draw->have_image_blit && draw->cur_blit_source != -1) {
+ num_to_consider = 1;
+ draw->cur_blit_source = -1;
+ }
Is it possible that dri3_find_back gets called with
draw->cur_blit_source != -1, entering this block (assuming
!draw->have_image_blit)...
Post by Thomas Hellstrom
for (;;) {
- for (b = 0; b < draw->num_back; b++) {
+ for (b = 0; b < num_to_consider; b++) {
int id = LOADER_DRI3_BACK_ID((b + draw->cur_back) % draw->num_back);
struct loader_dri3_buffer *buffer = draw->buffers[id];
... Then later gets called again with draw->cur_blit_source == -1,
choosing a different back buffer here, because the previous one is still
busy?
--
Earthling Michel Dänzer | http://www.amd.com
Libre software enthusiast | Mesa and X developer
Thomas Hellstrom
2017-08-15 12:45:15 UTC
Reply
Permalink
Raw Message
Post by Michel Dänzer
Post by Thomas Hellstrom
EGL uses the force_copy parameter to loader_dri3_swap_buffers_msc() to indicate
that it wants to preserve back buffer contents across a buffer swap.
While the loader then turns off server-side page-flipping there's nothing to
guarantee that a new backbuffer isn't chosen when EGL starts to render again,
and that buffer's content is of course undefined.
If the client supports local blits, allow server-side page flipping and when
a new back is grabbed, if needed, blit the old back's content to the new back.
If the client doesn't support local blits, disallow server-side page-flipping
to avoid a client deadlock and then, when grabbing a new back buffer, sleep
until the old back is idle, which may take a substantial time depending on
swap interval.
[...]
Post by Thomas Hellstrom
@@ -475,12 +476,21 @@ dri3_find_back(struct loader_dri3_drawable *draw)
int b;
xcb_generic_event_t *ev;
xcb_present_generic_event_t *ge;
+ int num_to_consider = draw->num_back;
/* Increase the likelyhood of reusing current buffer */
dri3_flush_present_events(draw);
+ /* Check whether we need to reuse the current back buffer as new back.
+ * In that case, wait until it's not busy anymore.
+ */
+ if (!draw->have_image_blit && draw->cur_blit_source != -1) {
+ num_to_consider = 1;
+ draw->cur_blit_source = -1;
+ }
Is it possible that dri3_find_back gets called with
draw->cur_blit_source != -1, entering this block (assuming
!draw->have_image_blit)...
Post by Thomas Hellstrom
for (;;) {
- for (b = 0; b < draw->num_back; b++) {
+ for (b = 0; b < num_to_consider; b++) {
int id = LOADER_DRI3_BACK_ID((b + draw->cur_back) % draw->num_back);
struct loader_dri3_buffer *buffer = draw->buffers[id];
... Then later gets called again with draw->cur_blit_source == -1,
choosing a different back buffer here, because the previous one is still
busy?
I don't think that's possible. If dri3_find_back returns a back buffer,
it should be idle, in the sense that it's not currently in the swap
chain or being scanned out from. It may still have a non-signaled fence
though. If a later call to dri_find_back() decides it's busy then
someone must have called swapBuffer in between and it's time to change
back anyway...

/Thomas
Michel Dänzer
2017-08-16 01:34:44 UTC
Reply
Permalink
Raw Message
Post by Thomas Hellstrom
Post by Michel Dänzer
Post by Thomas Hellstrom
EGL uses the force_copy parameter to loader_dri3_swap_buffers_msc() to indicate
that it wants to preserve back buffer contents across a buffer swap.
While the loader then turns off server-side page-flipping there's nothing to
guarantee that a new backbuffer isn't chosen when EGL starts to render again,
and that buffer's content is of course undefined.
If the client supports local blits, allow server-side page flipping and when
a new back is grabbed, if needed, blit the old back's content to the new back.
If the client doesn't support local blits, disallow server-side page-flipping
to avoid a client deadlock and then, when grabbing a new back buffer, sleep
until the old back is idle, which may take a substantial time depending on
swap interval.
[...]
Post by Thomas Hellstrom
@@ -475,12 +476,21 @@ dri3_find_back(struct loader_dri3_drawable *draw)
int b;
xcb_generic_event_t *ev;
xcb_present_generic_event_t *ge;
+ int num_to_consider = draw->num_back;
/* Increase the likelyhood of reusing current buffer */
dri3_flush_present_events(draw);
+ /* Check whether we need to reuse the current back buffer as new back.
+ * In that case, wait until it's not busy anymore.
+ */
+ if (!draw->have_image_blit && draw->cur_blit_source != -1) {
+ num_to_consider = 1;
+ draw->cur_blit_source = -1;
+ }
Is it possible that dri3_find_back gets called with
draw->cur_blit_source != -1, entering this block (assuming
!draw->have_image_blit)...
Post by Thomas Hellstrom
for (;;) {
- for (b = 0; b < draw->num_back; b++) {
+ for (b = 0; b < num_to_consider; b++) {
int id = LOADER_DRI3_BACK_ID((b + draw->cur_back) % draw->num_back);
struct loader_dri3_buffer *buffer = draw->buffers[id];
... Then later gets called again with draw->cur_blit_source == -1,
choosing a different back buffer here, because the previous one is still
busy?
I don't think that's possible. If dri3_find_back returns a back buffer,
it should be idle, in the sense that it's not currently in the swap
chain or being scanned out from. It may still have a non-signaled fence
though. If a later call to dri_find_back() decides it's busy then
someone must have called swapBuffer in between and it's time to change
back anyway...
Okay, then this patch is also

Reviewed-by: Michel Dänzer <***@amd.com>
--
Earthling Michel Dänzer | http://www.amd.com
Libre software enthusiast | Mesa and X developer
Thomas Hellstrom
2017-08-11 14:14:20 UTC
Reply
Permalink
Raw Message
With GLX_SWAP_COPY_OML and GLX_SWAP_EXCHANGE_OML it may happen in situations
when glXSwapBuffers() is immediately followed by for example another
glXSwapBuffers() or glXCopyBuffers() or back buffer age querying, that we
haven't yet allocated and initialized a new back buffer because there was
no GL rendering in between.

Make sure that we have a back buffer in those situations.

Signed-off-by: Thomas Hellstrom <***@vmware.com>
---
src/loader/loader_dri3_helper.c | 64 ++++++++++++++++++++++++++++++++++-------
src/loader/loader_dri3_helper.h | 3 ++
2 files changed, 57 insertions(+), 10 deletions(-)

diff --git a/src/loader/loader_dri3_helper.c b/src/loader/loader_dri3_helper.c
index dee0df8..0440633 100644
--- a/src/loader/loader_dri3_helper.c
+++ b/src/loader/loader_dri3_helper.c
@@ -35,6 +35,7 @@
#include <c11/threads.h>
#include "loader_dri3_helper.h"

+
/* From xmlpool/options.h, user exposed so should be stable */
#define DRI_CONF_VBLANK_NEVER 0
#define DRI_CONF_VBLANK_DEF_INTERVAL_0 1
@@ -59,6 +60,9 @@ static struct loader_dri3_blit_context blit_context = {
static void
dri3_flush_present_events(struct loader_dri3_drawable *draw);

+static struct loader_dri3_buffer *
+dri3_find_back_alloc(struct loader_dri3_drawable *draw);
+
/**
* Get and lock (for use with the current thread) a dri context associated
* with the drawable's dri screen. The context is intended to be used with
@@ -259,6 +263,7 @@ loader_dri3_drawable_init(xcb_connection_t *conn,
draw->have_image_blit = draw->ext->image->base.version >= 9 &&
draw->ext->image->blitImage != NULL;
draw->cur_blit_source = -1;
+ draw->have_back_format = false;

if (draw->ext->config)
draw->ext->config->configQueryi(draw->dri_screen,
@@ -606,7 +611,10 @@ loader_dri3_copy_sub_buffer(struct loader_dri3_drawable *draw,
flags |= __DRI2_FLUSH_CONTEXT;
loader_dri3_flush(draw, flags, __DRI2_THROTTLE_SWAPBUFFER);

- back = dri3_back_buffer(draw);
+ back = dri3_find_back_alloc(draw);
+ if (!back)
+ return;
+
y = draw->height - y - height;

if (draw->is_different_gpu) {
@@ -631,7 +639,7 @@ loader_dri3_copy_sub_buffer(struct loader_dri3_drawable *draw,
loader_dri3_swapbuffer_barrier(draw);
dri3_fence_reset(draw->conn, back);
dri3_copy_area(draw->conn,
- dri3_back_buffer(draw)->pixmap,
+ back->pixmap,
draw->drawable,
dri3_drawable_gc(draw),
x, y, x, y, width, height);
@@ -642,7 +650,7 @@ loader_dri3_copy_sub_buffer(struct loader_dri3_drawable *draw,
if (draw->have_fake_front && !draw->is_different_gpu) {
dri3_fence_reset(draw->conn, dri3_fake_front_buffer(draw));
dri3_copy_area(draw->conn,
- dri3_back_buffer(draw)->pixmap,
+ back->pixmap,
dri3_fake_front_buffer(draw)->pixmap,
dri3_drawable_gc(draw),
x, y, x, y, width, height);
@@ -753,7 +761,8 @@ loader_dri3_swap_buffers_msc(struct loader_dri3_drawable *draw,

draw->vtable->flush_drawable(draw, flush_flags);

- back = draw->buffers[dri3_find_back(draw)];
+ back = dri3_find_back_alloc(draw);
+
if (draw->is_different_gpu && back) {
/* Update the linear buffer before presenting the pixmap */
(void) loader_dri3_blit_image(draw,
@@ -882,15 +891,12 @@ loader_dri3_swap_buffers_msc(struct loader_dri3_drawable *draw,
int
loader_dri3_query_buffer_age(struct loader_dri3_drawable *draw)
{
- int back_id = LOADER_DRI3_BACK_ID(dri3_find_back(draw));
+ struct loader_dri3_buffer *back = dri3_find_back_alloc(draw);

- if (back_id < 0 || !draw->buffers[back_id])
+ if (!back || back->last_swap == 0)
return 0;

- if (draw->buffers[back_id]->last_swap != 0)
- return draw->send_sbc - draw->buffers[back_id]->last_swap + 1;
- else
- return 0;
+ return draw->send_sbc - back->last_swap + 1;
}

/** loader_dri3_open
@@ -1324,6 +1330,9 @@ dri3_get_buffer(__DRIdrawable *driDrawable,
int buf_id;

if (buffer_type == loader_dri3_buffer_back) {
+ draw->have_back_format = true;
+ draw->back_format = format;
+
buf_id = dri3_find_back(draw);

if (buf_id < 0)
@@ -1610,3 +1619,38 @@ loader_dri3_close_screen(__DRIscreen *dri_screen)
}
mtx_unlock(&blit_context.mtx);
}
+
+/**
+ * Find a backbuffer slot - potentially allocating a back buffer
+ *
+ * \param draw[in,out] Pointer to the drawable for which to find back.
+ * \return Pointer to a new back buffer or NULL if allocation failed or was
+ * not mandated.
+ *
+ * Find a potentially new back buffer, and if it's not been allocated yet and
+ * in addition needs initializing, then try to allocate and initialize it.
+ */
+static struct loader_dri3_buffer *
+dri3_find_back_alloc(struct loader_dri3_drawable *draw)
+{
+ struct loader_dri3_buffer *back;
+ int id;
+
+ id = dri3_find_back(draw);
+ back = (id >= 0) ? draw->buffers[id] : NULL;
+
+ if (!back && draw->have_back_format) {
+ (void) dri3_get_buffer(draw->dri_drawable,
+ draw->back_format,
+ loader_dri3_buffer_back,
+ draw);
+ } else if (back) {
+ /* Make sure any server side operations are flushed.
+ * For example glXSwapBuffers server side fake-front-to-back copy
+ * followed by glXCopyBuffers
+ */
+ dri3_fence_await(draw->conn, back);
+ }
+
+ return back;
+}
diff --git a/src/loader/loader_dri3_helper.h b/src/loader/loader_dri3_helper.h
index c8f63fd..b06a9ad 100644
--- a/src/loader/loader_dri3_helper.h
+++ b/src/loader/loader_dri3_helper.h
@@ -159,6 +159,9 @@ struct loader_dri3_drawable {

bool have_image_blit;
unsigned int swap_method;
+
+ bool have_back_format;
+ unsigned int back_format;
};

void
--
2.7.4
Michel Dänzer
2017-08-15 07:32:44 UTC
Reply
Permalink
Raw Message
Post by Thomas Hellstrom
With GLX_SWAP_COPY_OML and GLX_SWAP_EXCHANGE_OML it may happen in situations
when glXSwapBuffers() is immediately followed by for example another
glXSwapBuffers() or glXCopyBuffers() or back buffer age querying, that we
haven't yet allocated and initialized a new back buffer because there was
no GL rendering in between.
Make sure that we have a back buffer in those situations.
---
src/loader/loader_dri3_helper.c | 64 ++++++++++++++++++++++++++++++++++-------
src/loader/loader_dri3_helper.h | 3 ++
2 files changed, 57 insertions(+), 10 deletions(-)
diff --git a/src/loader/loader_dri3_helper.c b/src/loader/loader_dri3_helper.c
index dee0df8..0440633 100644
--- a/src/loader/loader_dri3_helper.c
+++ b/src/loader/loader_dri3_helper.c
@@ -35,6 +35,7 @@
#include <c11/threads.h>
#include "loader_dri3_helper.h"
+
/* From xmlpool/options.h, user exposed so should be stable */
#define DRI_CONF_VBLANK_NEVER 0
#define DRI_CONF_VBLANK_DEF_INTERVAL_0 1
Drop this hunk.
Post by Thomas Hellstrom
diff --git a/src/loader/loader_dri3_helper.h b/src/loader/loader_dri3_helper.h
index c8f63fd..b06a9ad 100644
--- a/src/loader/loader_dri3_helper.h
+++ b/src/loader/loader_dri3_helper.h
@@ -159,6 +159,9 @@ struct loader_dri3_drawable {
bool have_image_blit;
unsigned int swap_method;
+
+ bool have_back_format;
+ unsigned int back_format;
};
Is 0 a valid value for the back_format field? If not, the
have_back_format field is redundant.
--
Earthling Michel Dänzer | http://www.amd.com
Libre software enthusiast | Mesa and X developer
Thomas Hellstrom
2017-08-15 12:41:16 UTC
Reply
Permalink
Raw Message
Post by Michel Dänzer
Post by Thomas Hellstrom
With GLX_SWAP_COPY_OML and GLX_SWAP_EXCHANGE_OML it may happen in situations
when glXSwapBuffers() is immediately followed by for example another
glXSwapBuffers() or glXCopyBuffers() or back buffer age querying, that we
haven't yet allocated and initialized a new back buffer because there was
no GL rendering in between.
Make sure that we have a back buffer in those situations.
---
src/loader/loader_dri3_helper.c | 64 ++++++++++++++++++++++++++++++++++-------
src/loader/loader_dri3_helper.h | 3 ++
2 files changed, 57 insertions(+), 10 deletions(-)
diff --git a/src/loader/loader_dri3_helper.c b/src/loader/loader_dri3_helper.c
index dee0df8..0440633 100644
--- a/src/loader/loader_dri3_helper.c
+++ b/src/loader/loader_dri3_helper.c
@@ -35,6 +35,7 @@
#include <c11/threads.h>
#include "loader_dri3_helper.h"
+
/* From xmlpool/options.h, user exposed so should be stable */
#define DRI_CONF_VBLANK_NEVER 0
#define DRI_CONF_VBLANK_DEF_INTERVAL_0 1
Drop this hunk.
Sure.
Post by Michel Dänzer
Post by Thomas Hellstrom
diff --git a/src/loader/loader_dri3_helper.h b/src/loader/loader_dri3_helper.h
index c8f63fd..b06a9ad 100644
--- a/src/loader/loader_dri3_helper.h
+++ b/src/loader/loader_dri3_helper.h
@@ -159,6 +159,9 @@ struct loader_dri3_drawable {
bool have_image_blit;
unsigned int swap_method;
+
+ bool have_back_format;
+ unsigned int back_format;
};
Is 0 a valid value for the back_format field? If not, the
have_back_format field is redundant.
I'll see if I can eliminate that field.

Thomas
Thomas Hellstrom
2017-08-11 14:14:14 UTC
Reply
Permalink
Raw Message
Signed-off-by: Thomas Hellstrom <***@vmware.com>
---
src/loader/loader_dri3_helper.c | 5 +++++
1 file changed, 5 insertions(+)

diff --git a/src/loader/loader_dri3_helper.c b/src/loader/loader_dri3_helper.c
index 4959e95..6f84a43 100644
--- a/src/loader/loader_dri3_helper.c
+++ b/src/loader/loader_dri3_helper.c
@@ -56,6 +56,8 @@ static struct loader_dri3_blit_context blit_context = {
_MTX_INITIALIZER_NP, NULL
};

+static void
+dri3_flush_present_events(struct loader_dri3_drawable *draw);

/**
* Get and lock (for use with the current thread) a dri context associated
@@ -474,6 +476,9 @@ dri3_find_back(struct loader_dri3_drawable *draw)
xcb_generic_event_t *ev;
xcb_present_generic_event_t *ge;

+ /* Increase the likelyhood of reusing current buffer */
+ dri3_flush_present_events(draw);
+
for (;;) {
for (b = 0; b < draw->num_back; b++) {
int id = LOADER_DRI3_BACK_ID((b + draw->cur_back) % draw->num_back);
--
2.7.4
Thomas Hellstrom
2017-08-11 14:14:19 UTC
Reply
Permalink
Raw Message
Add support for the exchange swap method. Since we're now forcing a fake front
buffer and we exchange the back and fake front on swaps, we don't need to add
much code.

Signed-off-by: Thomas Hellstrom <***@vmware.com>
---
src/loader/loader_dri3_helper.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/src/loader/loader_dri3_helper.c b/src/loader/loader_dri3_helper.c
index 7056f46..dee0df8 100644
--- a/src/loader/loader_dri3_helper.c
+++ b/src/loader/loader_dri3_helper.c
@@ -767,7 +767,7 @@ loader_dri3_swap_buffers_msc(struct loader_dri3_drawable *draw,
* The force_copy parameter is used by EGL to attempt to preserve
* the back buffer across a call to this function.
*/
- if (draw->swap_method == __DRI_ATTRIB_SWAP_COPY || force_copy)
+ if (draw->swap_method != __DRI_ATTRIB_SWAP_UNDEFINED || force_copy)
draw->cur_blit_source = LOADER_DRI3_BACK_ID(draw->cur_back);

/* Exchange the back and fake front. Even though the server knows about these
@@ -1494,8 +1494,10 @@ loader_dri3_get_buffers(__DRIdrawable *driDrawable,
if (!dri3_update_drawable(driDrawable, draw))
return false;

- /* pixmaps always have front buffers */
- if (draw->is_pixmap)
+ /* pixmaps always have front buffers.
+ * Exchange swaps also mandate fake front buffers.
+ */
+ if (draw->is_pixmap || draw->swap_method == __DRI_ATTRIB_SWAP_EXCHANGE)
buffer_mask |= __DRI_IMAGE_BUFFER_FRONT;

if (buffer_mask & __DRI_IMAGE_BUFFER_FRONT) {
--
2.7.4
Michel Dänzer
2017-08-15 07:34:02 UTC
Reply
Permalink
Raw Message
Implement back-to-fake-front flips,
Fix EGL_BUFFER_PRESERVED path.
Implement dri3 support for GLX_SWAP_EXCHANGE_OML and GLX_SWAP_COPY_OML.
The back-to-fake-front flips will save a full buffer copy in the case of a
fake front being enabled and GLX_SWAP_UNDEFINED_OML.
Support for EGL_BUFFER_PRESERVED and GLX_SWAP_X_OML are mostly useful for
things like glretrace if traces are capured with applications relying on a
specific swapbuffer behavior.
The EGL_BUFFER_PRESERVED path previously made sure the present was done as
a copy, but there was nothing making sure that after the present,
the same back buffer was chosen.
This has now been changed so that if the previous back buffer is
idle, we reuse it. Otherwise we grab a new and copy the contents and
buffer age from the previous back buffer. Server side flips are allowed.
GLX_SWAP_COPY_OML will behave like EGL_BUFFER_PRESERVED.
GLX_SWAP_EXCHANGE_OML will behave similarly, except that we try to reuse the
previous fake front as the new back buffer if it's idle. If not, we grab
a new back buffer and copy the contents and buffer age from the old fake front.
- Split the original patch,
- Make sure we have a context for blitImage even if we don't have a
current context.
- Make sure the delayed backbuffer allocation is performed before
glXSwapBuffers, glXCopyBuffers and querying buffer age.
piglit tests/quick without regressions on svga.
A modified piglit glx-swap-exchange posted for review on the piglit list.
That test required modifying the dri2 state tracke to advertise unconditional
support for GLX_SWAP_EXCHANGE_OML
A piglit glx-swap-copy test derived from the glx-swap-exchange test.
Not posted yet.
Patches 3 & 4 should probably be squashed into patch 2 in order to avoid
leaking the cached blit context.

Patches 1, 5, 7-10 are

Reviewed-by: Michel Dänzer <***@amd.com>
--
Earthling Michel Dänzer | http://www.amd.com
Libre software enthusiast | Mesa and X developer
Loading...