Re: [Mesa-dev] [PATCH 02/11] loader/dri3: Make sure we always have a context for image blit operations

2017-08-15 Thread Thomas Hellstrom 

Hi, Michel, Thanks for reviewing.

On 08/15/2017 08:57 AM, Michel Dänzer wrote:

On 11/08/17 11:14 PM, Thomas Hellstrom wrote:

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 

[...]


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





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





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



Either leave the line uncommented, or remove it and fix up the
indentation of the following line.



Thanks,

Thomas


___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 02/11] loader/dri3: Make sure we always have a context for image blit operations

2017-08-15 Thread Michel Dänzer 
On 11/08/17 11:14 PM, Thomas Hellstrom wrote:
> 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 

[...]

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


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


> @@ -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
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH 02/11] loader/dri3: Make sure we always have a context for image blit operations

2017-08-11 Thread 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.

Signed-off-by: Thomas Hellstrom 
---
 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 
 
+#include 
 #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(_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(_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", _mode);
@@ -467,9 +575,6 @@ loader_dri3_copy_sub_buffer(struct loader_dri3_drawable 
*draw,
 {
struct loader_dri3_buffer *back;
unsigned flags =