[PATCH] uxa/glamor: Enable the rest glamor rendering functions.

[PATCH] uxa/glamor: Enable the rest glamor rendering functions.

[zhigang.gong]

From: Zhigang Gong <zhigang.gong@linux.intel.com>

This commit enable all the rest glamor rendering functions.
Tested with latest glamor master branch, can pass rendercheck.

One thing need to be pointed out is the picture's handling.
Pictures support many different color formats, but glamor's
texture only support a few color formats. And the most common
scenario is that we create a pixmap with a color depth and
then attach it to a picture which has a specific color format
with the same color depth. But there is no way to change a
texture's internal format after the texture was allocated.
If you do that, the OpenGL will allocate a new texture. And
then the glamor side and UXA side will be inconsitence. So
for all the picture related operations, we can't fallback to
UXA path directly, even it is rather a strainth forward
operation. So for the get_image, Addtraps.., we have to add
wrappers function for them to jump into glamor firstly.

Signed-off-by: Zhigang Gong <zhigang.gong@linux.intel.com>
---
 src/intel_uxa.c  |   14 +++++++-
 uxa/uxa-accel.c  |   88 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
 uxa/uxa-glamor.h |   16 ++++++++-
 uxa/uxa-glyphs.c |   21 ++++++++++++
 uxa/uxa-priv.h   |    8 +++++
 uxa/uxa-render.c |   90 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 uxa/uxa.c        |    4 +-
 7 files changed, 234 insertions(+), 7 deletions(-)

diff --git a/src/intel_uxa.c b/src/intel_uxa.c
index e4a5270..a79affa 100644
--- a/src/intel_uxa.c
+++ b/src/intel_uxa.c
@@ -1108,6 +1108,9 @@ intel_uxa_create_pixmap(ScreenPtr screen, int w, int h, int depth,
 				list_del(&priv->in_flight);
 				screen->ModifyPixmapHeader(pixmap, w, h, 0, 0, stride, NULL);
 				intel_set_pixmap_private(pixmap, priv);
+
+				if (!intel_glamor_create_textured_pixmap(pixmap))
+					intel_set_pixmap_bo(pixmap, NULL);
 				return pixmap;
 			}
 		}
@@ -1145,8 +1148,15 @@ intel_uxa_create_pixmap(ScreenPtr screen, int w, int h, int depth,
 		list_init(&priv->batch);
 		list_init(&priv->flush);
 		intel_set_pixmap_private(pixmap, priv);
-
-		intel_glamor_create_textured_pixmap(pixmap);
+		/* Create textured pixmap failed means glamor fail to create
+		 * a texture from the BO for some reasons, and then glamor
+		 * create a new texture attached to the pixmap, and all the
+		 * consequent rendering operations on this pixmap will never
+		 * fallback to UXA path, so we don't need to hold the useless
+		 * BO if it is the case.
+		 */
+		if (!intel_glamor_create_textured_pixmap(pixmap))
+			intel_set_pixmap_bo(pixmap, NULL);
 	}
 
 	return pixmap;
diff --git a/uxa/uxa-accel.c b/uxa/uxa-accel.c
index e4afd13..05c64f6 100644
--- a/uxa/uxa-accel.c
+++ b/uxa/uxa-accel.c
@@ -207,8 +207,23 @@ static void
 uxa_put_image(DrawablePtr pDrawable, GCPtr pGC, int depth, int x, int y,
 	      int w, int h, int leftPad, int format, char *bits)
 {
+	uxa_screen_t *uxa_screen = uxa_get_screen(pDrawable->pScreen);
+
+	if (uxa_screen->info->flags & UXA_USE_GLAMOR) {
+		uxa_prepare_access(pDrawable, UXA_GLAMOR_ACCESS_RW);
+		if (glamor_put_image_nf(pDrawable,
+					pGC, depth, x, y, w, h,
+					leftPad, format, bits)) {
+			uxa_finish_access(pDrawable, UXA_GLAMOR_ACCESS_RW);
+			return;
+		}
+		uxa_finish_access(pDrawable, UXA_GLAMOR_ACCESS_RO);
+		goto fallback;
+	}
+
 	if (!uxa_do_put_image(pDrawable, pGC, depth, x, y, w, h, format, bits,
 			      PixmapBytePad(w, pDrawable->depth)))
+fallback:
 		uxa_check_put_image(pDrawable, pGC, depth, x, y, w, h, leftPad,
 				    format, bits);
 }
@@ -352,6 +367,22 @@ uxa_copy_n_to_n(DrawablePtr pSrcDrawable,
 	int dst_off_x, dst_off_y;
 	PixmapPtr pSrcPixmap, pDstPixmap;
 
+	if (uxa_screen->info->flags & UXA_USE_GLAMOR) {
+		uxa_prepare_access(pSrcDrawable, UXA_GLAMOR_ACCESS_RO);
+		uxa_prepare_access(pDstDrawable, UXA_GLAMOR_ACCESS_RW);
+		if (glamor_copy_n_to_n_nf(pSrcDrawable, pDstDrawable,
+					  pGC, pbox, nbox, dx, dy,
+					  reverse, upsidedown, bitplane,
+					  closure)) {
+			uxa_finish_access(pDstDrawable, UXA_GLAMOR_ACCESS_RW);
+			uxa_finish_access(pSrcDrawable, UXA_GLAMOR_ACCESS_RO);
+			return;
+		}
+		uxa_finish_access(pDstDrawable, UXA_GLAMOR_ACCESS_RO);
+		uxa_finish_access(pSrcDrawable, UXA_GLAMOR_ACCESS_RO);
+		goto fallback;
+	}
+
 	if (uxa_screen->swappedOut || uxa_screen->force_fallback)
 		goto fallback;
 
@@ -795,9 +826,52 @@ out:
 	REGION_DESTROY(pScreen, pReg);
 }
 
+void
+uxa_get_spans(DrawablePtr pDrawable,
+	      int wMax,
+	      DDXPointPtr ppt, int *pwidth, int nspans, char *pdstStart)
+
+{
+	ScreenPtr screen = pDrawable->pScreen;
+	uxa_screen_t *uxa_screen = uxa_get_screen(screen);
+
+	if (uxa_screen->info->flags & UXA_USE_GLAMOR) {
+		uxa_prepare_access(pDrawable, UXA_GLAMOR_ACCESS_RW);
+		if (glamor_get_spans_nf(pDrawable, wMax, ppt,
+					pwidth, nspans, pdstStart)) {
+			uxa_finish_access(pDrawable, UXA_GLAMOR_ACCESS_RW);
+			return;
+		}
+		uxa_finish_access(pDrawable, UXA_GLAMOR_ACCESS_RO);
+	}
+
+	uxa_check_get_spans(pDrawable, wMax, ppt, pwidth, nspans, pdstStart);
+}
+
+
+static void
+uxa_set_spans(DrawablePtr pDrawable, GCPtr gc, char *src,
+                 DDXPointPtr points, int *widths, int n, int sorted)
+{
+	ScreenPtr screen = pDrawable->pScreen;
+	uxa_screen_t *uxa_screen = uxa_get_screen(screen);
+
+	if (uxa_screen->info->flags & UXA_USE_GLAMOR) {
+		uxa_prepare_access(pDrawable, UXA_GLAMOR_ACCESS_RW);
+		if (glamor_set_spans_nf(pDrawable, gc, src,
+					points, widths, n, sorted)) {
+			uxa_finish_access(pDrawable, UXA_GLAMOR_ACCESS_RW);
+			return;
+		}
+		uxa_finish_access(pDrawable, UXA_GLAMOR_ACCESS_RO);
+	}
+
+	uxa_check_set_spans(pDrawable, gc, src, points, widths, n, sorted);
+}
+
 const GCOps uxa_ops = {
 	uxa_fill_spans,
-	uxa_check_set_spans,
+	uxa_set_spans,
 	uxa_put_image,
 	uxa_copy_area,
 	uxa_check_copy_plane,
@@ -1002,6 +1076,18 @@ uxa_get_image(DrawablePtr pDrawable, int x, int y, int w, int h,
 	Box.x2 = Box.x1 + w;
 	Box.y2 = Box.y1 + h;
 
+	if (uxa_screen->info->flags & UXA_USE_GLAMOR) {
+		uxa_prepare_access(pDrawable, UXA_GLAMOR_ACCESS_RW);
+		if (glamor_get_image_nf(pDrawable, x, y, w, h,
+					format, planeMask, d)) {
+			uxa_finish_access(pDrawable, UXA_GLAMOR_ACCESS_RW);
+			return;
+		}
+
+		uxa_finish_access(pDrawable, UXA_GLAMOR_ACCESS_RO);
+		goto fallback;
+	}
+
 	if (uxa_screen->swappedOut || uxa_screen->force_fallback)
 		goto fallback;
 
diff --git a/uxa/uxa-glamor.h b/uxa/uxa-glamor.h
index 2c23098..71a9c29 100644
--- a/uxa/uxa-glamor.h
+++ b/uxa/uxa-glamor.h
@@ -37,8 +37,20 @@
 #ifdef USE_GLAMOR
 #include "glamor.h"
 #else
-#define glamor_fill_spans_nf(...)  FALSE
-#define glamor_poly_fill_rect_nf(...) FALSE
+#define glamor_fill_spans_nf(...)	FALSE
+#define glamor_poly_fill_rect_nf(...)	FALSE
+#define glamor_put_image_nf(...)	FALSE
+#define glamor_copy_n_to_n_nf(...)	FALSE
+#define glamor_get_spans_nf(...)	FALSE
+#define glamor_set_spans_nf(...)	FALSE
+#define glamor_get_image_nf(...)	FALSE
+#define glamor_glyphs_nf(...)		FALSE
+#define glamor_glyph_unrealize(...)	;
+#define glamor_composite_nf(...)	FALSE
+#define glamor_composite_rects_nf(...)	FALSE
+#define glamor_trapezoids_nf(...)	FALSE
+#define glamor_triangles_nf(...)	FALSE
+#define glamor_add_traps_nf(...)	FALSE
 #endif
 
 #endif /* UXA_GLAMOR_H */
diff --git a/uxa/uxa-glyphs.c b/uxa/uxa-glyphs.c
index 6c9ea0d..2156578 100644
--- a/uxa/uxa-glyphs.c
+++ b/uxa/uxa-glyphs.c
@@ -65,6 +65,7 @@
 #include <stdlib.h>
 
 #include "uxa-priv.h"
+#include "uxa-glamor.h"
 #include "../src/common.h"
 
 #include "mipict.h"
@@ -304,6 +305,7 @@ uxa_glyph_unrealize(ScreenPtr screen,
 		    GlyphPtr glyph)
 {
 	struct uxa_glyph *priv;
+	uxa_screen_t *uxa_screen = uxa_get_screen(screen);
 
 	/* Use Lookup in case we have not attached to this glyph. */
 	priv = dixLookupPrivate(&glyph->devPrivates, &uxa_glyph_key);
@@ -314,6 +316,9 @@ uxa_glyph_unrealize(ScreenPtr screen,
 
 	uxa_glyph_set_private(glyph, NULL);
 	free(priv);
+
+	if (uxa_screen->info->flags & UXA_USE_GLAMOR)
+		glamor_glyph_unrealize(screen, glyph);
 }
 
 /* Cut and paste from render/glyph.c - probably should export it instead */
@@ -1062,6 +1067,22 @@ uxa_glyphs(CARD8 op,
 	int width, height, ret;
 	PicturePtr localDst = pDst;
 
+	if (uxa_screen->info->flags & UXA_USE_GLAMOR) {
+		uxa_picture_prepare_access(pDst, UXA_GLAMOR_ACCESS_RW);
+		uxa_picture_prepare_access(pSrc, UXA_GLAMOR_ACCESS_RO);
+		if (glamor_glyphs_nf(op,
+				     pSrc, pDst, maskFormat,
+				     xSrc, ySrc, nlist, list, glyphs)) {
+			uxa_picture_finish_access(pSrc, UXA_GLAMOR_ACCESS_RO);
+			uxa_picture_finish_access(pDst, UXA_GLAMOR_ACCESS_RW);
+			return;
+		} else {
+			uxa_picture_finish_access(pSrc, UXA_GLAMOR_ACCESS_RO);
+			uxa_picture_finish_access(pDst, UXA_GLAMOR_ACCESS_RO);
+			goto fallback;
+		}
+	}
+
 	if (!uxa_screen->info->prepare_composite ||
 	    uxa_screen->swappedOut ||
 	    uxa_screen->force_fallback ||
diff --git a/uxa/uxa-priv.h b/uxa/uxa-priv.h
index a455f25..d6d857f 100644
--- a/uxa/uxa-priv.h
+++ b/uxa/uxa-priv.h
@@ -292,6 +292,14 @@ void
 uxa_get_image(DrawablePtr pDrawable, int x, int y, int w, int h,
 	      unsigned int format, unsigned long planeMask, char *d);
 
+void
+uxa_get_spans(DrawablePtr pDrawable, int wMax, DDXPointPtr ppt,
+	      int *pwidth, int nspans, char *pdstStart);
+
+void
+uxa_add_traps(PicturePtr pPicture,
+	      INT16 x_off, INT16 y_off, int ntrap, xTrap * traps);
+
 extern const GCOps uxa_ops;
 
 #ifdef RENDER
diff --git a/uxa/uxa-render.c b/uxa/uxa-render.c
index 51c12f1..7d350bc 100644
--- a/uxa/uxa-render.c
+++ b/uxa/uxa-render.c
@@ -29,6 +29,7 @@
 #include <stdlib.h>
 
 #include "uxa-priv.h"
+#include "uxa-glamor.h"
 #include <xorgVersion.h>
 
 #ifdef RENDER
@@ -986,6 +987,18 @@ uxa_solid_rects (CARD8		op,
 	if (!pixman_region_not_empty(dst->pCompositeClip))
 		return;
 
+	if (uxa_screen->info->flags & UXA_USE_GLAMOR) {
+		uxa_picture_prepare_access(dst, UXA_GLAMOR_ACCESS_RW);
+		if (glamor_composite_rects_nf(op, dst, color,
+					      num_rects, rects)) {
+			uxa_picture_finish_access(dst, UXA_GLAMOR_ACCESS_RW);
+			return;
+		} else {
+			uxa_picture_finish_access(dst, UXA_GLAMOR_ACCESS_RO);
+			goto fallback;
+		}
+	}
+
 	if (dst->alphaMap)
 		goto fallback;
 
@@ -1530,6 +1543,29 @@ uxa_composite(CARD8 op,
 	RegionRec region;
 	int tx, ty;
 
+	if (uxa_screen->info->flags & UXA_USE_GLAMOR) {
+		uxa_picture_prepare_access(pDst, UXA_GLAMOR_ACCESS_RW);
+		uxa_picture_prepare_access(pSrc, UXA_GLAMOR_ACCESS_RO);
+		if (pMask)
+			uxa_picture_prepare_access(pMask, UXA_GLAMOR_ACCESS_RO);
+		if (glamor_composite_nf(op,
+				        pSrc, pMask, pDst, xSrc, ySrc,
+					xMask, yMask, xDst, yDst,
+					width, height)) {
+			if (pMask)
+				uxa_picture_finish_access(pMask, UXA_GLAMOR_ACCESS_RO);
+			uxa_picture_finish_access(pSrc, UXA_GLAMOR_ACCESS_RO);
+			uxa_picture_finish_access(pDst, UXA_GLAMOR_ACCESS_RW);
+			return;
+		} else {
+			if (pMask)
+				uxa_picture_finish_access(pMask, UXA_GLAMOR_ACCESS_RO);
+			uxa_picture_finish_access(pSrc, UXA_GLAMOR_ACCESS_RO);
+			uxa_picture_finish_access(pDst, UXA_GLAMOR_ACCESS_RO);
+			goto fallback;
+		}
+	}
+
 	if (uxa_screen->swappedOut || uxa_screen->force_fallback)
 		goto fallback;
 
@@ -1871,7 +1907,24 @@ uxa_trapezoids(CARD8 op, PicturePtr src, PicturePtr dst,
 	BoxRec bounds;
 	Bool direct;
 
+	if (uxa_screen->info->flags & UXA_USE_GLAMOR) {
+		uxa_picture_prepare_access(dst, UXA_GLAMOR_ACCESS_RW);
+		uxa_picture_prepare_access(src, UXA_GLAMOR_ACCESS_RO);
+		if (glamor_trapezoids_nf(op,
+				         src, dst, maskFormat, xSrc,
+					 ySrc, ntrap, traps)) {
+			uxa_picture_finish_access(src, UXA_GLAMOR_ACCESS_RO);
+			uxa_picture_finish_access(dst, UXA_GLAMOR_ACCESS_RW);
+			return;
+		} else {
+			uxa_picture_finish_access(src, UXA_GLAMOR_ACCESS_RO);
+			uxa_picture_finish_access(dst, UXA_GLAMOR_ACCESS_RO);
+			goto fallback;
+		}
+	}
+
 	if (uxa_screen->swappedOut || uxa_screen->force_fallback) {
+fallback:
 		uxa_check_trapezoids(op, src, dst, maskFormat, xSrc, ySrc, ntrap, traps);
 		return;
 	}
@@ -1995,6 +2048,7 @@ uxa_triangles(CARD8 op, PicturePtr pSrc, PicturePtr pDst,
 	      int ntri, xTriangle * tris)
 {
 	ScreenPtr pScreen = pDst->pDrawable->pScreen;
+	uxa_screen_t *uxa_screen = uxa_get_screen(pScreen);
 	PictureScreenPtr ps = GetPictureScreen(pScreen);
 	BoxRec bounds;
 	Bool direct = op == PictOpAdd && miIsSolidAlpha(pSrc);
@@ -2006,6 +2060,21 @@ uxa_triangles(CARD8 op, PicturePtr pSrc, PicturePtr pDst,
 			return;
 	}
 
+	if (uxa_screen->info->flags & UXA_USE_GLAMOR) {
+		uxa_picture_prepare_access(pDst, UXA_GLAMOR_ACCESS_RW);
+		uxa_picture_prepare_access(pSrc, UXA_GLAMOR_ACCESS_RO);
+		if (glamor_triangles_nf(op,
+				        pSrc, pDst, maskFormat, xSrc,
+					ySrc, ntri, tris)) {
+			uxa_picture_finish_access(pSrc, UXA_GLAMOR_ACCESS_RO);
+			uxa_picture_finish_access(pDst, UXA_GLAMOR_ACCESS_RW);
+			return;
+		} else {
+			uxa_picture_finish_access(pSrc, UXA_GLAMOR_ACCESS_RO);
+			uxa_picture_finish_access(pDst, UXA_GLAMOR_ACCESS_RO);
+		}
+	}
+
 	/*
 	 * Check for solid alpha add
 	 */
@@ -2069,3 +2138,24 @@ uxa_triangles(CARD8 op, PicturePtr pSrc, PicturePtr pDst,
 				      tris);
 	}
 }
+
+void
+uxa_add_traps(PicturePtr pPicture,
+	      INT16 x_off, INT16 y_off, int ntrap, xTrap * traps)
+{
+	ScreenPtr pScreen = pPicture->pDrawable->pScreen;
+	uxa_screen_t *uxa_screen = uxa_get_screen(pScreen);
+
+	if (uxa_screen->info->flags & UXA_USE_GLAMOR) {
+		uxa_picture_prepare_access(pPicture, UXA_GLAMOR_ACCESS_RW);
+		if (glamor_add_traps_nf(pPicture,
+				        x_off, y_off, ntrap, traps)) {
+			uxa_picture_finish_access(pPicture, UXA_GLAMOR_ACCESS_RW);
+			return;
+		} else {
+			uxa_picture_finish_access(pPicture, UXA_GLAMOR_ACCESS_RO);
+		}
+	}
+
+	uxa_check_add_traps(pPicture, x_off, y_off, ntrap, traps);
+}
diff --git a/uxa/uxa.c b/uxa/uxa.c
index 5a3c0be..5b3d709 100644
--- a/uxa/uxa.c
+++ b/uxa/uxa.c
@@ -521,7 +521,7 @@ Bool uxa_driver_init(ScreenPtr screen, uxa_driver_t * uxa_driver)
 	screen->GetImage = uxa_get_image;
 
 	uxa_screen->SavedGetSpans = screen->GetSpans;
-	screen->GetSpans = uxa_check_get_spans;
+	screen->GetSpans = uxa_get_spans;
 
 	uxa_screen->SavedCopyWindow = screen->CopyWindow;
 	screen->CopyWindow = uxa_copy_window;
@@ -559,7 +559,7 @@ Bool uxa_driver_init(ScreenPtr screen, uxa_driver_t * uxa_driver)
 			ps->Trapezoids = uxa_trapezoids;
 
 			uxa_screen->SavedAddTraps = ps->AddTraps;
-			ps->AddTraps = uxa_check_add_traps;
+			ps->AddTraps = uxa_add_traps;
 		}
 	}
 #endif
-- 
1.7.4.4

Re: [PATCH] uxa/glamor: Enable the rest glamor rendering functions.

[Chris Wilson]

On Tue, 13 Dec 2011 22:31:41 +0800, zhigang.gong@linux.intel.com wrote:
> From: Zhigang Gong <zhigang.gong@linux.intel.com>
> 
> This commit enable all the rest glamor rendering functions.
> Tested with latest glamor master branch, can pass rendercheck.
> 
> One thing need to be pointed out is the picture's handling.
> Pictures support many different color formats, but glamor's
> texture only support a few color formats. And the most common
> scenario is that we create a pixmap with a color depth and
> then attach it to a picture which has a specific color format
> with the same color depth. But there is no way to change a
> texture's internal format after the texture was allocated.
> If you do that, the OpenGL will allocate a new texture. And
> then the glamor side and UXA side will be inconsitence. So
> for all the picture related operations, we can't fallback to
> UXA path directly, even it is rather a strainth forward
> operation. So for the get_image, Addtraps.., we have to add
> wrappers function for them to jump into glamor firstly.

Can we create multiple textures referencing the same bo but with
different formats? Or are we going to run afoul of the coherency model
with GL?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] uxa/glamor: Enable the rest glamor rendering functions.

[Chris Wilson]

On Tue, 13 Dec 2011 22:31:41 +0800, zhigang.gong@linux.intel.com wrote:
> From: Zhigang Gong <zhigang.gong@linux.intel.com>
> 
> This commit enable all the rest glamor rendering functions.
> Tested with latest glamor master branch, can pass rendercheck.

Hmm, it exposes an issue with keeping a bo cache independent of mesa and
trying to feed it our own handles:

 Region for name 6 already exists but is not compatible

The w/a for this would be:

diff --git a/src/intel_glamor.c b/src/intel_glamor.c
index 0cf8ed7..2757fd6 100644
--- a/src/intel_glamor.c
+++ b/src/intel_glamor.c
@@ -91,6 +91,7 @@ intel_glamor_create_textured_pixmap(PixmapPtr pixmap)
        priv = intel_get_pixmap_private(pixmap);
        if (glamor_egl_create_textured_pixmap(pixmap, priv->bo->handle,
                                              priv->stride)) {
+               drm_intel_bo_disable_reuse(priv->bo);
                priv->pinned = 1;
                return TRUE;
        } else

but that gives up all pretense of maintaining a bo cache.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] uxa/glamor: Enable the rest glamor rendering functions.

[Zhigang Gong]

> -----Original Message-----
> From: Chris Wilson [mailto:chris@chris-wilson.co.uk]
> Sent: Wednesday, December 14, 2011 2:20 AM
> To: zhigang.gong@linux.intel.com
> Cc: intel-gfx@lists.freedesktop.org; zhigang.gong@gmail.com;
> zhigang.gong@linux.intel.com
> Subject: Re: [PATCH] uxa/glamor: Enable the rest glamor rendering
> functions.
> 
> On Tue, 13 Dec 2011 22:31:41 +0800, zhigang.gong@linux.intel.com
> wrote:
> > From: Zhigang Gong <zhigang.gong@linux.intel.com>
> >
> > This commit enable all the rest glamor rendering functions.
> > Tested with latest glamor master branch, can pass rendercheck.
> >
> > One thing need to be pointed out is the picture's handling.
> > Pictures support many different color formats, but glamor's texture
> > only support a few color formats. And the most common scenario is that
> > we create a pixmap with a color depth and then attach it to a picture
> > which has a specific color format with the same color depth. But there
> > is no way to change a texture's internal format after the texture was
> > allocated.
> > If you do that, the OpenGL will allocate a new texture. And then the
> > glamor side and UXA side will be inconsitence. So for all the picture
> > related operations, we can't fallback to UXA path directly, even it is
> > rather a strainth forward operation. So for the get_image, Addtraps..,
> > we have to add wrappers function for them to jump into glamor firstly.
> 
> Can we create multiple textures referencing the same bo but with
> different formats?
AFAIK, it's impossible to match all possible picture formats to a OpenGL
internal format.
We have to have a new texture attached to glamor for incompatible format.
The
old texture is created from DDX's BO and has incorrect internal format. IMO,
we
can't make any use of this wrong texture within glamor, so I just don't
create it and
return a false to DDX layer to notify the DDX to unlink the BO. All the
consequent 
rendering operation on this pixmap will be handled within glamor scope and
target
to the new texture with correct format.

> Or are we going to run afoul of the coherency model
> with GL?
My understanding is, if the picture's format is incompatible with OpenGL's
internal
format.

--Zhigang

> -Chris
> 
> --
> Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] uxa/glamor: Enable the rest glamor rendering functions.

[Zhigang Gong]

[-- Attachment #1: Type: text/plain, Size: 2842 bytes --]

> -----Original Message-----
> From: Chris Wilson [mailto:chris@chris-wilson.co.uk]
> Sent: Wednesday, December 14, 2011 2:45 AM
> To: zhigang.gong@linux.intel.com
> Cc: intel-gfx@lists.freedesktop.org; zhigang.gong@gmail.com;
> zhigang.gong@linux.intel.com
> Subject: Re: [PATCH] uxa/glamor: Enable the rest glamor rendering
> functions.
> 
> On Tue, 13 Dec 2011 22:31:41 +0800, zhigang.gong@linux.intel.com
> wrote:
> > From: Zhigang Gong <zhigang.gong@linux.intel.com>
> >
> > This commit enable all the rest glamor rendering functions.
> > Tested with latest glamor master branch, can pass rendercheck.
> 
> Hmm, it exposes an issue with keeping a bo cache independent of mesa
> and trying to feed it our own handles:
> 
>  Region for name 6 already exists but is not compatible
> 
> The w/a for this would be:
> 
> diff --git a/src/intel_glamor.c b/src/intel_glamor.c index
0cf8ed7..2757fd6
> 100644
> --- a/src/intel_glamor.c
> +++ b/src/intel_glamor.c
> @@ -91,6 +91,7 @@ intel_glamor_create_textured_pixmap(PixmapPtr
> pixmap)
>         priv = intel_get_pixmap_private(pixmap);
>         if (glamor_egl_create_textured_pixmap(pixmap,
> priv->bo->handle,
>                                               priv->stride)) {
> +               drm_intel_bo_disable_reuse(priv->bo);
>                 priv->pinned = 1;
>                 return TRUE;
>         } else
> 
> but that gives up all pretense of maintaining a bo cache.

Yes, I think this impacts the performance. Actually, I noticed this problem
and I
spent some time to track the root cause. If everything is ok, this error
should
not be triggered. Although the same BO maybe reused to create a new pixmap,
the previous pixmap which own this BO should be already destroyed. And the
previous image created with the previous pixmap should be destroyed either.

And then, when we create a new pixmap/image with this BO, MESA should not
find any exist image/region for this BO. But it does happen. I tracked
further into
mesa internal and found that the previous image was not destroyed when we
call eglDestroyImageKHR, as its reference count is decreased to zero. It's
weird
for me. Further tracking shows that the root cause is when I use the
texture(bind to 
the image) as a shader's source texture, and call glDrawArrays to perform
the
rendering, the texture's reference count will be increased by 1 before
return
from glDrawArrays. And I failed to find any API to decrease it. Then this
texture
can't be freed when destroy that texture and thus the image's reference
count
will also remain 1 and can't be freed either.

The attached is a simple case to show this bug. It was modified from the
eglkms.c
in mesa-demos.

I did send this issue to mesa-dev. Don't have a solution or explanation so
far. Any 
idea?

> -Chris
> 
> --
> Chris Wilson, Intel Open Source Technology Centre

[-- Attachment #2: eglkms_mod.c --]
[-- Type: application/octet-stream, Size: 11038 bytes --]

#include <stdlib.h>
#include <stdio.h>

#define EGL_EGLEXT_PROTOTYPES
#define GL_GLEXT_PROTOTYPES

#include <gbm.h>
#include <GL/gl.h>
#include <GL/glu.h>

#ifndef GLAPIENTRY
#define GLAPIENTRY
#endif

#include <GL/glext.h>
#include <EGL/egl.h>
#include <EGL/eglext.h>
#include <drm.h>
#include <fcntl.h>
#include <unistd.h>
#include <string.h>
#include <sys/ioctl.h>

#ifdef GL_OES_EGL_image
static PFNGLEGLIMAGETARGETTEXTURE2DOESPROC glEGLImageTargetTexture2DOES_func;
static PFNEGLCREATEIMAGEKHRPROC glEGLCreateImageKHR_func; 
#endif

static const char device_name[] = "/dev/dri/card0";
GLint copy_prog;

static GLint
compile_glsl_prog(GLenum type, const char *source)
{
  GLint ok;
  GLint prog;

  prog = glCreateShader(type);
  glShaderSource(prog, 1, (const GLchar **) &source, NULL);
  glCompileShader(prog);
  glGetShaderiv(prog, GL_COMPILE_STATUS, &ok);
  if (!ok) {
    GLchar *info;
    GLint size;

    glGetShaderiv(prog, GL_INFO_LOG_LENGTH, &size);
    info = malloc(size);
    glGetShaderInfoLog(prog, size, NULL, info);
    fprintf(stderr, "Failed to compile %s: %s\n",
	    type == GL_FRAGMENT_SHADER ? "FS" : "VS", info);
    fprintf(stderr, "Program source:\n%s", source);
  }

  return prog;
}

static void
link_glsl_prog(GLint prog)
{
  GLint ok;

  glLinkProgram(prog);
  glGetProgramiv(prog, GL_LINK_STATUS, &ok);
  if (!ok) {
    GLchar *info;
    GLint size;
    glGetProgramiv(prog, GL_INFO_LOG_LENGTH, &size);
    info = malloc(size);
    glGetProgramInfoLog(prog, size, NULL, info);
    fprintf(stderr, "Failed to link: %s\n", info);
    fprintf(stderr, "GLSL link failure\n");
  }
}

#define VERTEX_POS 0
#define VERTEX_SOURCE 1

static void 
init_shader(void)
{
  const char *copy_vs =
    "attribute vec4 v_position;\n"
    "attribute vec4 v_texcoord0;\n"
    "varying vec2 source_texture;\n"
    "void main()\n"
    "{\n"
    "   gl_Position = v_position;\n"
    "   source_texture = v_texcoord0.xy;\n" "}\n";

  const char *copy_fs =
    "varying vec2 source_texture;\n"
    "uniform sampler2D sampler;\n"
    "void main()\n"
    "{\n"
    "     gl_FragColor = texture2D(sampler, source_texture).rgba;\n"
    "    } \n" ;

  GLint sampler_uniform_location;
  GLint fs_prog, vs_prog;

  copy_prog = glCreateProgram();
  vs_prog = compile_glsl_prog(GL_VERTEX_SHADER, copy_vs);
  fs_prog = compile_glsl_prog(GL_FRAGMENT_SHADER, copy_fs);
  glAttachShader(copy_prog, vs_prog);
  glAttachShader(copy_prog, fs_prog);
  glBindAttribLocation(copy_prog, VERTEX_POS, "v_position");
  glBindAttribLocation(copy_prog, VERTEX_SOURCE, "v_texcoord0");
  link_glsl_prog(copy_prog);
  glUseProgram(copy_prog);
  sampler_uniform_location = glGetUniformLocation(copy_prog, "sampler");
  glUniform1i(sampler_uniform_location, 0);
  glUseProgram(0);
}

static GLuint 
create_tex_from_img(EGLImageKHR image)
{
  GLuint tex;

  glGenTextures(1, &tex);
  glBindTexture(GL_TEXTURE_2D, tex);
  glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, GL_NEAREST);
  glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER, GL_NEAREST);
  glEGLImageTargetTexture2DOES_func (GL_TEXTURE_2D, image);
  glBindTexture(GL_TEXTURE_2D, 0);

  return tex;
}

static GLuint 
create_fbo_render2tex(GLint tex)
{
  GLuint fb;

  glGenFramebuffers(1, &fb);
  glBindFramebuffer(GL_FRAMEBUFFER, fb);
  glFramebufferTexture2D(GL_FRAMEBUFFER,
			 GL_COLOR_ATTACHMENT0,
			 GL_TEXTURE_2D, tex,
			 0);

  if (glCheckFramebufferStatusEXT(GL_FRAMEBUFFER_EXT) !=
      GL_FRAMEBUFFER_COMPLETE) {
    fprintf(stderr, "framebuffer not complete\n");
    exit(1);
  }

  return fb;
}

static EGLImageKHR 
create_image_from_handle(EGLDisplay dpy, EGLContext ctx, 
                         int fd, int handle, int width, 
                         int height, int stride, int bpp)
{
  struct drm_gem_flink flink;
  EGLImageKHR image;
  EGLint attribs[] = {
    EGL_WIDTH, 0,
    EGL_HEIGHT, 0,
    EGL_DRM_BUFFER_STRIDE_MESA, 0,
    EGL_DRM_BUFFER_FORMAT_MESA,
    EGL_DRM_BUFFER_FORMAT_ARGB32_MESA,
    EGL_DRM_BUFFER_USE_MESA,
    EGL_DRM_BUFFER_USE_SHARE_MESA,
    EGL_NONE
  };

  attribs[1] = width;
  attribs[3] = height;
  attribs[5] = stride * bpp/8;

  flink.handle = handle;
  if (ioctl(fd, DRM_IOCTL_GEM_FLINK, &flink) < 0)
    return EGL_NO_IMAGE_KHR;

  image = glEGLCreateImageKHR_func(dpy , ctx,
				   EGL_DRM_BUFFER_MESA,
				   (void *) (uintptr_t)flink.name, attribs);
  if (image == EGL_NO_IMAGE_KHR)
    return EGL_NO_IMAGE_KHR;

  return image;
}

/* use shader to copy tex to current fbo. */
static void 
copy_tex(GLint tex)
{
  static float vertices[8] = { -1, -1,
			       1, -1,
			       1, 1,
			       -1, 1
  };
  static float texcoords[8] = { 0, 1,
				1, 1,
				1, 0,
				0, 0
  };

  glVertexAttribPointer(VERTEX_POS, 2, GL_FLOAT,
			GL_FALSE, 2 * sizeof(float),
			vertices);
  glEnableVertexAttribArray(VERTEX_POS);
  glVertexAttribPointer(VERTEX_SOURCE, 2, GL_FLOAT,
			GL_FALSE, 2 * sizeof(float),
			texcoords);
  glEnableVertexAttribArray(VERTEX_SOURCE);

  glActiveTexture(GL_TEXTURE0);
  glBindTexture(GL_TEXTURE_2D, tex);

  glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER,
		  GL_NEAREST);
  glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER,
		  GL_NEAREST);

  glUseProgram(copy_prog);

  glDrawArrays(GL_TRIANGLE_FAN, 0, 4);

  glUseProgram(0);
  glDisableVertexAttribArray(VERTEX_SOURCE);
  glDisableVertexAttribArray(VERTEX_POS);
}



int main(int argc, char *argv[])
{
  EGLDisplay dpy;
  EGLContext ctx;
  EGLImageKHR shadow_image, screen_image;
  EGLint major, minor;
  const char *ver, *extensions;
  uint32_t shadow_handle, shadow_stride;
  int ret, fd;
  struct gbm_device *gbm;
  struct gbm_bo *shadow_bo, *screen_bo;
  GLuint shadow_tex, screen_tex, screen_fb;

  fd = open(device_name, O_RDWR);
  if (fd < 0) {
    /* Probably permissions error */
    fprintf(stderr, "couldn't open %s, skipping\n", device_name);
    return -1;
  }

  gbm = gbm_create_device(fd);
  if (gbm == NULL) {
    fprintf(stderr, "couldn't create gbm device\n");
    ret = -1;
    goto close_fd;
  }

  dpy = eglGetDisplay(gbm);
  if (dpy == EGL_NO_DISPLAY) {
    fprintf(stderr, "eglGetDisplay() failed\n");
    ret = -1;
    goto destroy_gbm_device;
  }
	
  if (!eglInitialize(dpy, &major, &minor)) {
    printf("eglInitialize() failed\n");
    ret = -1;
    goto egl_terminate;
  }

  ver = eglQueryString(dpy, EGL_VERSION);
  printf("EGL_VERSION = %s\n", ver);

  extensions = eglQueryString(dpy, EGL_EXTENSIONS);

  if (!strstr(extensions, "EGL_KHR_surfaceless_opengl")) {
    printf("No support for EGL_KHR_surfaceless_opengl\n");
    ret = -1;
    goto egl_terminate;
  }

  eglBindAPI(EGL_OPENGL_API);
  ctx = eglCreateContext(dpy, NULL, EGL_NO_CONTEXT, NULL);
  if (ctx == NULL) {
    fprintf(stderr, "failed to create context\n");
    ret = -1;
    goto egl_terminate;
  }

  if (!eglMakeCurrent(dpy, EGL_NO_SURFACE, EGL_NO_SURFACE, ctx)) {
    fprintf(stderr, "failed to make context current\n");
    ret = -1;
    goto destroy_context;
  }

  init_shader();

  glEGLImageTargetTexture2DOES_func = 
    (PFNGLEGLIMAGETARGETTEXTURE2DOESPROC)
    eglGetProcAddress("glEGLImageTargetTexture2DOES");

  glEGLCreateImageKHR_func = (PFNEGLCREATEIMAGEKHRPROC)
    eglGetProcAddress("eglCreateImageKHR");


  shadow_bo = gbm_bo_create(gbm, 1024, 768,
			    GBM_BO_FORMAT_XRGB8888,
			    GBM_BO_USE_RENDERING);

  screen_bo = gbm_bo_create(gbm, 1024, 768,
			    GBM_BO_FORMAT_XRGB8888,
			    GBM_BO_USE_SCANOUT | GBM_BO_USE_RENDERING);
  if (screen_bo == NULL) {
    fprintf(stderr, "failed to create gbm screen bo\n");
    ret = -1;
    goto unmake_current;
  }

  shadow_handle = gbm_bo_get_handle(shadow_bo).u32;
  shadow_stride = gbm_bo_get_pitch(shadow_bo);

  shadow_image = create_image_from_handle(dpy, ctx, fd, 
					  shadow_handle, 
					  1024, 
					  768, 
                                          shadow_stride,
					  32); 
  screen_image = eglCreateImageKHR(dpy, NULL, EGL_NATIVE_PIXMAP_KHR, screen_bo, NULL);
  if (screen_image == EGL_NO_IMAGE_KHR || shadow_image == EGL_NO_IMAGE_KHR) {
    fprintf(stderr, "failed to create egl image\n");
    ret = -1;
    goto destroy_gbm_bo;
  }

  shadow_tex = create_tex_from_img(shadow_image);
  screen_tex = create_tex_from_img(screen_image);

  screen_fb = create_fbo_render2tex(screen_tex);

  /* Copy shadow texture to screen texture by using shader. */
  glBindFramebuffer(GL_FRAMEBUFFER, screen_fb);
  copy_tex(shadow_tex);  
  glFlush();

  glDeleteFramebuffers(1, &screen_fb);
  glDeleteTextures(1, &shadow_tex);
  glDeleteTextures(1, &screen_tex);
  eglDestroyImageKHR(dpy, shadow_image);
  eglDestroyImageKHR(dpy, screen_image);
  /* Now create a smaller image from the same handle. */
  shadow_image = create_image_from_handle(dpy, ctx, fd, 
					  shadow_handle, 
					  1024 / 2 , 
					  768 / 2 , 
					  shadow_stride/2,
					  32);
  if (shadow_image == EGL_NO_IMAGE_KHR) {

    /* As we already destroyed the first shadow image which 
       created from the same handle, we should be able to 
       create a smaller image from the same handle again. 
       This is true, if we don't call into

       copy_tex(shadow_tex);

       But if we call into copy_tex, then we fail here. 
       You will get error message from the mesa side as below:
       "Region for name 1 already exists but is not compatible "
       copy_tex(shadow_tex) is a function to use shadow 
       texture as a texture source, and access it in a 
       shader program to render the screen fbo. I traced
       into the code, and found that glDrawArray will 
       implicitly increase the shadow texture's reference count.
       And then latter when we delete the texture, the 
       count will remain 1 rather then zero, and then
       it will not decrease the corresponding image region's
       reference counter and then when we destroy the shadow image,
       the image's reference counter will also be 1 and can't
       be freed. Then next time we use the same handle to
       create a new image, it will find the previou zombie
       image region and find they are not compatible.
     
       simply comment out the glDrawArray can avoid the function
       goes here too. Don't know how to fix this problem.
    */
    fprintf(stderr, "hit a bug?\n");
  } else
    eglDestroyImageKHR(dpy, shadow_image);

 destroy_gbm_bo:
  gbm_bo_destroy(shadow_bo);
  gbm_bo_destroy(screen_bo);
 unmake_current:
  eglMakeCurrent(dpy, EGL_NO_SURFACE, EGL_NO_SURFACE, EGL_NO_CONTEXT);
 destroy_context:
  eglDestroyContext(dpy, ctx);
 egl_terminate:
  eglTerminate(dpy);
 destroy_gbm_device:
  gbm_device_destroy(gbm);
 close_fd:
  close(fd);

  return ret;
}

[-- Attachment #3: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] uxa/glamor: Enable the rest glamor rendering functions.

[Chris Wilson]

On Wed, 14 Dec 2011 12:08:30 +0800, "Zhigang Gong" <zhigang.gong@linux.intel.com> wrote:
> > -----Original Message-----
> > From: Chris Wilson [mailto:chris@chris-wilson.co.uk]
> > Sent: Wednesday, December 14, 2011 2:45 AM
> > To: zhigang.gong@linux.intel.com
> > Cc: intel-gfx@lists.freedesktop.org; zhigang.gong@gmail.com;
> > zhigang.gong@linux.intel.com
> > Subject: Re: [PATCH] uxa/glamor: Enable the rest glamor rendering
> > functions.
> > 
> > On Tue, 13 Dec 2011 22:31:41 +0800, zhigang.gong@linux.intel.com
> > wrote:
> > > From: Zhigang Gong <zhigang.gong@linux.intel.com>
> > >
> > > This commit enable all the rest glamor rendering functions.
> > > Tested with latest glamor master branch, can pass rendercheck.
> > 
> > Hmm, it exposes an issue with keeping a bo cache independent of mesa
> > and trying to feed it our own handles:
> > 
> >  Region for name 6 already exists but is not compatible
> > 
> > The w/a for this would be:
> > 
> > diff --git a/src/intel_glamor.c b/src/intel_glamor.c index
> 0cf8ed7..2757fd6
> > 100644
> > --- a/src/intel_glamor.c
> > +++ b/src/intel_glamor.c
> > @@ -91,6 +91,7 @@ intel_glamor_create_textured_pixmap(PixmapPtr
> > pixmap)
> >         priv = intel_get_pixmap_private(pixmap);
> >         if (glamor_egl_create_textured_pixmap(pixmap,
> > priv->bo->handle,
> >                                               priv->stride)) {
> > +               drm_intel_bo_disable_reuse(priv->bo);
> >                 priv->pinned = 1;
> >                 return TRUE;
> >         } else
> > 
> > but that gives up all pretense of maintaining a bo cache.
> 
> Yes, I think this impacts the performance. Actually, I noticed this problem
> and I
> spent some time to track the root cause. If everything is ok, this error
> should
> not be triggered. Although the same BO maybe reused to create a new pixmap,
> the previous pixmap which own this BO should be already destroyed. And the
> previous image created with the previous pixmap should be destroyed either.

Certainly it looks like glamor is taking all necessary steps to decouple
the bo from the textures and renderbuffer upon pixmap finish. There is
one other potential race here in that the ddx will mark the bo as
purgeable as soon as it releases it back to the cache, but it may not
yet have been submitted by mesa in its execbuffer. The kernel may choose
to free the memory associated with the bo before that happens, and
may rightfully complain the userspace is doing something silly.
 
> And then, when we create a new pixmap/image with this BO, MESA should not
> find any exist image/region for this BO. But it does happen. I tracked
> further into
> mesa internal and found that the previous image was not destroyed when we
> call eglDestroyImageKHR, as its reference count is decreased to zero. It's
> weird
> for me. Further tracking shows that the root cause is when I use the
> texture(bind to 
> the image) as a shader's source texture, and call glDrawArrays to perform
> the
> rendering, the texture's reference count will be increased by 1 before
> return
> from glDrawArrays. And I failed to find any API to decrease it. Then this
> texture
> can't be freed when destroy that texture and thus the image's reference
> count
> will also remain 1 and can't be freed either.

I'm looking at update_texture_state() which appears to hold onto a
reference to the texobj until its slot is reused. If the object is
simply destroyed and the unit disabled, the slot is skipped and the old
reference remains. _mesa_update_state (which calls into
update_texture_state) would seem to be only invoked upon a draw
primitives. Binding a dummy texture/fb and doing a dummy render might be
a little extreme as a workaround!
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] uxa/glamor: Enable the rest glamor rendering functions.

[Zhigang Gong]

> -----Original Message-----
> From: Chris Wilson [mailto:chris@chris-wilson.co.uk]
> Sent: Wednesday, December 14, 2011 7:12 PM
> To: Zhigang Gong
> Cc: intel-gfx@lists.freedesktop.org; zhigang.gong@gmail.com
> Subject: RE: [PATCH] uxa/glamor: Enable the rest glamor rendering
> functions.
> 
> On Wed, 14 Dec 2011 12:08:30 +0800, "Zhigang Gong"
> <zhigang.gong@linux.intel.com> wrote:
> > > -----Original Message-----
> > > From: Chris Wilson [mailto:chris@chris-wilson.co.uk]
> > > Sent: Wednesday, December 14, 2011 2:45 AM
> > > To: zhigang.gong@linux.intel.com
> > > Cc: intel-gfx@lists.freedesktop.org; zhigang.gong@gmail.com;
> > > zhigang.gong@linux.intel.com
> > > Subject: Re: [PATCH] uxa/glamor: Enable the rest glamor rendering
> > > functions.
> > >
> > > On Tue, 13 Dec 2011 22:31:41 +0800, zhigang.gong@linux.intel.com
> > > wrote:
> > > > From: Zhigang Gong <zhigang.gong@linux.intel.com>
> > > >
> > > > This commit enable all the rest glamor rendering functions.
> > > > Tested with latest glamor master branch, can pass rendercheck.
> > >
> > > Hmm, it exposes an issue with keeping a bo cache independent of
> mesa
> > > and trying to feed it our own handles:
> > >
> > >  Region for name 6 already exists but is not compatible
> > >
> > > The w/a for this would be:
> > >
> > > diff --git a/src/intel_glamor.c b/src/intel_glamor.c index
> > 0cf8ed7..2757fd6
> > > 100644
> > > --- a/src/intel_glamor.c
> > > +++ b/src/intel_glamor.c
> > > @@ -91,6 +91,7 @@
> intel_glamor_create_textured_pixmap(PixmapPtr
> > > pixmap)
> > >         priv = intel_get_pixmap_private(pixmap);
> > >         if (glamor_egl_create_textured_pixmap(pixmap,
> > > priv->bo->handle,
> > >                                               priv->stride)) {
> > > +               drm_intel_bo_disable_reuse(priv->bo);
> > >                 priv->pinned = 1;
> > >                 return TRUE;
> > >         } else
> > >
> > > but that gives up all pretense of maintaining a bo cache.
> >
> > Yes, I think this impacts the performance. Actually, I noticed this
> > problem and I spent some time to track the root cause. If everything
> > is ok, this error should not be triggered. Although the same BO maybe
> > reused to create a new pixmap, the previous pixmap which own this BO
> > should be already destroyed. And the previous image created with the
> > previous pixmap should be destroyed either.
> 
> Certainly it looks like glamor is taking all necessary steps to decouple
the
> bo from the textures and renderbuffer upon pixmap finish. There is one
> other potential race here in that the ddx will mark the bo as purgeable as
> soon as it releases it back to the cache, but it may not yet have been
> submitted by mesa in its execbuffer. The kernel may choose to free the
> memory associated with the bo before that happens, and may rightfully
> complain the userspace is doing something silly.

Right, we do have this race if the kernel free the BO's memory prior to
The mesa submit its execbuffer. Hmm. But I think that may not be a real
problem, as once we call intel_set_pixmap_bo(pixmap, NULL) to unlink
the bo from the pixmap, the BO will not be released at DRM layer
immediately, 
instead, it will be put on a in_flight list. And intel_batch_submit will
empty the
list, considering after switching to glamor, each pixmap's batch buffer
should
be empty, then the driver will only call intel_batch_submit at
intel_flush_rendering
which is called from intel_uxa_block_handler and is after the
intel_glamor_flush.
At intel_glamor_flush, it will do a glFlush, my understanding is glFlush
should make sure the execbuffer was submitted to GPU. But I'm not very sure.
Can you confirm that? Thanks.

> 
> > And then, when we create a new pixmap/image with this BO, MESA
> should
> > not find any exist image/region for this BO. But it does happen. I
> > tracked further into mesa internal and found that the previous image
> > was not destroyed when we call eglDestroyImageKHR, as its reference
> > count is decreased to zero. It's weird for me. Further tracking shows
> > that the root cause is when I use the texture(bind to the image) as a
> > shader's source texture, and call glDrawArrays to perform the
> > rendering, the texture's reference count will be increased by 1 before
> > return from glDrawArrays. And I failed to find any API to decrease it.
> > Then this texture can't be freed when destroy that texture and thus
> > the image's reference count will also remain 1 and can't be freed
> > either.
> 
> I'm looking at update_texture_state() which appears to hold onto a
> reference to the texobj until its slot is reused. If the object is simply
> destroyed and the unit disabled, the slot is skipped and the old reference
> remains. _mesa_update_state (which calls into
> update_texture_state) would seem to be only invoked upon a draw
> primitives. Binding a dummy texture/fb and doing a dummy render might
> be a little extreme as a workaround!
Thanks for providing this workaround, I did try that way with my simple test
case, it works. I just wonder whether this is a bug of MESA. 
I will implement it in glamor before we get a better solution.

> -Chris
> 
> --
> Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] uxa/glamor: Enable the rest glamor rendering functions.

[Chris Wilson]

On Tue, 13 Dec 2011 22:31:41 +0800, zhigang.gong@linux.intel.com wrote:
> From: Zhigang Gong <zhigang.gong@linux.intel.com>
> 
> This commit enable all the rest glamor rendering functions.
> Tested with latest glamor master branch, can pass rendercheck.

Sure enough, it passes rendercheck. Well done!

However, glyph rendering is off, the characters look incorrectly scaled
and slightly out-of-position. And it will eventually chase a NULL
pointer inside glamor's glyph cache.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] uxa/glamor: Enable the rest glamor rendering functions.

[Chris Wilson]

On Wed, 14 Dec 2011 19:44:34 +0800, "Zhigang Gong" <zhigang.gong@linux.intel.com> wrote:
> > -----Original Message-----
> > From: Chris Wilson [mailto:chris@chris-wilson.co.uk]
> > Sent: Wednesday, December 14, 2011 7:12 PM
> > To: Zhigang Gong
> > Cc: intel-gfx@lists.freedesktop.org; zhigang.gong@gmail.com
> > Subject: RE: [PATCH] uxa/glamor: Enable the rest glamor rendering
> > functions.
> > 
> > On Wed, 14 Dec 2011 12:08:30 +0800, "Zhigang Gong"
> > <zhigang.gong@linux.intel.com> wrote:
> > > > -----Original Message-----
> > > > From: Chris Wilson [mailto:chris@chris-wilson.co.uk]
> > > > Sent: Wednesday, December 14, 2011 2:45 AM
> > > > To: zhigang.gong@linux.intel.com
> > > > Cc: intel-gfx@lists.freedesktop.org; zhigang.gong@gmail.com;
> > > > zhigang.gong@linux.intel.com
> > > > Subject: Re: [PATCH] uxa/glamor: Enable the rest glamor rendering
> > > > functions.
> > > >
> > > > On Tue, 13 Dec 2011 22:31:41 +0800, zhigang.gong@linux.intel.com
> > > > wrote:
> > > > > From: Zhigang Gong <zhigang.gong@linux.intel.com>
> > > > >
> > > > > This commit enable all the rest glamor rendering functions.
> > > > > Tested with latest glamor master branch, can pass rendercheck.
> > > >
> > > > Hmm, it exposes an issue with keeping a bo cache independent of
> > mesa
> > > > and trying to feed it our own handles:
> > > >
> > > >  Region for name 6 already exists but is not compatible
> > > >
> > > > The w/a for this would be:
> > > >
> > > > diff --git a/src/intel_glamor.c b/src/intel_glamor.c index
> > > 0cf8ed7..2757fd6
> > > > 100644
> > > > --- a/src/intel_glamor.c
> > > > +++ b/src/intel_glamor.c
> > > > @@ -91,6 +91,7 @@
> > intel_glamor_create_textured_pixmap(PixmapPtr
> > > > pixmap)
> > > >         priv = intel_get_pixmap_private(pixmap);
> > > >         if (glamor_egl_create_textured_pixmap(pixmap,
> > > > priv->bo->handle,
> > > >                                               priv->stride)) {
> > > > +               drm_intel_bo_disable_reuse(priv->bo);
> > > >                 priv->pinned = 1;
> > > >                 return TRUE;
> > > >         } else
> > > >
> > > > but that gives up all pretense of maintaining a bo cache.
> > >
> > > Yes, I think this impacts the performance. Actually, I noticed this
> > > problem and I spent some time to track the root cause. If everything
> > > is ok, this error should not be triggered. Although the same BO maybe
> > > reused to create a new pixmap, the previous pixmap which own this BO
> > > should be already destroyed. And the previous image created with the
> > > previous pixmap should be destroyed either.
> > 
> > Certainly it looks like glamor is taking all necessary steps to decouple
> the
> > bo from the textures and renderbuffer upon pixmap finish. There is one
> > other potential race here in that the ddx will mark the bo as purgeable as
> > soon as it releases it back to the cache, but it may not yet have been
> > submitted by mesa in its execbuffer. The kernel may choose to free the
> > memory associated with the bo before that happens, and may rightfully
> > complain the userspace is doing something silly.
> 
> Right, we do have this race if the kernel free the BO's memory prior to
> The mesa submit its execbuffer. Hmm. But I think that may not be a real
> problem, as once we call intel_set_pixmap_bo(pixmap, NULL) to unlink
> the bo from the pixmap, the BO will not be released at DRM layer
> immediately, 
> instead, it will be put on a in_flight list. And intel_batch_submit will
> empty the
> list, considering after switching to glamor, each pixmap's batch buffer
> should
> be empty, then the driver will only call intel_batch_submit at
> intel_flush_rendering
> which is called from intel_uxa_block_handler and is after the
> intel_glamor_flush.
> At intel_glamor_flush, it will do a glFlush, my understanding is glFlush
> should make sure the execbuffer was submitted to GPU. But I'm not very sure.
> Can you confirm that? Thanks.

It shouldn't go on the in-flight list because we're not using
intel_batchbuffer any more and so it will not be referenced by the
batch.

This patch along with calling dispatch->glFlush() after deleting the
textured pixmap is enough to silence the warning inside mesa and
prevent the purgeable race:

diff --git a/src/mesa/drivers/dri/intel/intel_context.c
b/src/mesa/drivers/dri/intel/intel_context.c
index 068b305..347a5d6 100644
--- a/src/mesa/drivers/dri/intel/intel_context.c
+++ b/src/mesa/drivers/dri/intel/intel_context.c
@@ -34,6 +34,7 @@
 #include "main/imports.h"
 #include "main/points.h"
 #include "main/renderbuffer.h"
+#include "main/state.h"
 
 #include "swrast/swrast.h"
 #include "swrast_setup/swrast_setup.h"
@@ -527,6 +528,8 @@ intel_glFlush(struct gl_context *ctx)
    intel_flush_front(ctx);
    if (intel->is_front_buffer_rendering)
       intel->need_throttle = true;
+
+   _mesa_update_state(ctx);
 }

diff --git a/src/mesa/main/texstate.c b/src/mesa/main/texstate.c
index 7cd2858..4bcaec0 100644
--- a/src/mesa/main/texstate.c
+++ b/src/mesa/main/texstate.c
@@ -557,6 +557,7 @@ update_texture_state( struct gl_context *ctx )
 
       if (enabledTargets == 0x0) {
          /* neither vertex nor fragment processing uses this unit */
+	 _mesa_reference_texobj(&texUnit->_Current, NULL);
          continue;
       }
 
@@ -592,6 +593,7 @@ update_texture_state( struct gl_context *ctx )
          }
          else {
             /* fixed-function: texture unit is really disabled */
+	    _mesa_reference_texobj(&texUnit->_Current, NULL);
             continue;
          }
       }

-- 
Chris Wilson, Intel Open Source Technology Centre