Re: [PATCH v2] uxa/glamor: Create glamor pixmap by default.

Re: [PATCH v2] uxa/glamor: Create glamor pixmap by default.

[Chris Wilson]

On Wed, 11 Jan 2012 10:01:09 +0800, zhigang.gong@gmail.com wrote:
> From: Zhigang Gong <zhigang.gong@linux.intel.com>
> 
> A minor fix, after convert the old pixmap to the textured-drm pixmap,
> we need to modify the old pixmap's header to make sure the width/height
> and stride are the same as the new textured-drm BO.

We can not just call FatalError there, but should just propagate the error
so that the client at least sees BadMatch and X doesn't simply die... How
does this resolve the issue of mesa replacing a bo for a glamor pixmap
even though we've exported it? Or is that simply an orthogonal problem
to be tackled later?

> ---
> As create glamor pixmap by default will get much better performance
> by using the textured-drm pixmap, this commit is to make that the
> default behaviour when configure to use glamor.
> 
> A side effect is that for those glamor pixmaps, they don't have a
> valid BO attached to it and thus it fails to get a DRI drawable. This
> commit also fixes that problem by copy the fixup_shadow mechanism. I
> tested this with mutter, and it seems work fine.
> 
> The performance gain to apply this patch is about 20% to 40% with
> different workload.

Waiting to see if I get any results to support that claim... ;-)
You can also mention that by using glamor to allocate the pixmaps, we
reduce the risk of encountering the "incompatible region exists for this name"
and the associated render corruption. Until that is resolved, every time
we export a DRI pixmap and create a BO we still may trigger that bug.
However, since we now never intentionally allocate a reusable pixmap we
could just make all (intel_glamor) allocations non-reusable without
incurring too great an overhead.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH v2] uxa/glamor: Create glamor pixmap by default.

[zhigang.gong]

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

A minor fix, after convert the old pixmap to the textured-drm pixmap,
we need to modify the old pixmap's header to make sure the width/height
and stride are the same as the new textured-drm BO.
---
As create glamor pixmap by default will get much better performance
by using the textured-drm pixmap, this commit is to make that the
default behaviour when configure to use glamor.

A side effect is that for those glamor pixmaps, they don't have a
valid BO attached to it and thus it fails to get a DRI drawable. This
commit also fixes that problem by copy the fixup_shadow mechanism. I
tested this with mutter, and it seems work fine.

The performance gain to apply this patch is about 20% to 40% with
different workload.

Signed-off-by: Zhigang Gong <zhigang.gong@linux.intel.com>
---
 src/intel_dri.c |   83 ++++++++++++++++++++++++++++++++++++++++++++++++++----
 src/intel_uxa.c |    6 ++++
 2 files changed, 82 insertions(+), 7 deletions(-)

diff --git a/src/intel_dri.c b/src/intel_dri.c
index df3338f..8a25ae8 100644
--- a/src/intel_dri.c
+++ b/src/intel_dri.c
@@ -66,6 +66,8 @@ USE OR OTHER DEALINGS IN THE SOFTWARE.
 
 #include "dri2.h"
 
+#include "intel_glamor.h"
+
 typedef struct {
 	int refcnt;
 	PixmapPtr pixmap;
@@ -125,6 +127,59 @@ static PixmapPtr get_front_buffer(DrawablePtr drawable)
 	return pixmap;
 }
 
+static PixmapPtr fixup_glamor(DrawablePtr drawable, PixmapPtr pixmap)
+{
+	ScreenPtr screen = drawable->pScreen;
+	PixmapPtr old = get_drawable_pixmap(drawable);
+	struct intel_pixmap *priv = intel_get_pixmap_private(pixmap);
+	GCPtr gc;
+
+	/* With a glamor pixmap, 2D pixmaps are created in texture
+	 * and without a static BO attached to it. To support DRI,
+	 * we need to create a new textured-drm pixmap and
+	 * need to copy the original content to this new textured-drm
+	 * pixmap, and then convert the old pixmap to a coherent
+	 * textured-drm pixmap which has a valid BO attached to it
+	 * and also has a valid texture, thus both glamor and DRI2
+	 * can access it.
+	 *
+	 */
+
+	/* Copy the current contents of the pixmap to the bo. */
+	gc = GetScratchGC(drawable->depth, screen);
+	if (gc) {
+		ValidateGC(&pixmap->drawable, gc);
+		gc->ops->CopyArea(drawable, &pixmap->drawable,
+				  gc,
+				  0, 0,
+				  drawable->width,
+				  drawable->height,
+				  0, 0);
+		FreeScratchGC(gc);
+	}
+
+	intel_set_pixmap_private(pixmap, NULL);
+	screen->DestroyPixmap(pixmap);
+
+	/* And redirect the pixmap to the new bo (for 3D). */
+	intel_set_pixmap_private(old, priv);
+	old->refcnt++;
+
+	if (!intel_glamor_create_textured_pixmap(old)) {
+		FatalError("Failed to get DRI drawable for glamor pixmap.\n");
+	}
+
+	screen->ModifyPixmapHeader(pixmap,
+				   drawable->width,
+				   drawable->height,
+				   0, 0,
+				   priv->stride,
+				   NULL);
+
+	intel_get_screen_private(xf86Screens[screen->myNum])->needs_flush = TRUE;
+	return old;
+}
+
 static PixmapPtr fixup_shadow(DrawablePtr drawable, PixmapPtr pixmap)
 {
 	ScreenPtr screen = drawable->pScreen;
@@ -203,6 +258,7 @@ I830DRI2CreateBuffers(DrawablePtr drawable, unsigned int *attachments,
 	I830DRI2BufferPrivatePtr privates;
 	PixmapPtr pixmap, pDepthPixmap;
 	int i;
+	int is_glamor_pixmap;
 
 	buffers = calloc(count, sizeof *buffers);
 	if (buffers == NULL)
@@ -222,7 +278,10 @@ I830DRI2CreateBuffers(DrawablePtr drawable, unsigned int *attachments,
 			pixmap = pDepthPixmap;
 			pixmap->refcnt++;
 		}
-		if (pixmap == NULL) {
+
+		is_glamor_pixmap = pixmap && (intel_get_pixmap_private(pixmap) == NULL);
+
+		if (pixmap == NULL || is_glamor_pixmap) {
 			unsigned int hint = INTEL_CREATE_PIXMAP_DRI2;
 
 			if (intel->tiling & INTEL_TILING_3D) {
@@ -255,8 +314,12 @@ I830DRI2CreateBuffers(DrawablePtr drawable, unsigned int *attachments,
 				goto unwind;
 			}
 
-			if (attachment == DRI2BufferFrontLeft)
-				pixmap = fixup_shadow(drawable, pixmap);
+			if (attachment == DRI2BufferFrontLeft) {
+				if (!is_glamor_pixmap)
+					pixmap = fixup_shadow(drawable, pixmap);
+				else
+					pixmap = fixup_glamor(drawable, pixmap);
+			}
 		}
 
 		if (attachments[i] == DRI2BufferDepth)
@@ -317,6 +380,7 @@ I830DRI2CreateBuffer(DrawablePtr drawable, unsigned int attachment,
 	DRI2Buffer2Ptr buffer;
 	I830DRI2BufferPrivatePtr privates;
 	PixmapPtr pixmap;
+	int is_glamor_pixmap;
 
 	buffer = calloc(1, sizeof *buffer);
 	if (buffer == NULL)
@@ -330,7 +394,9 @@ I830DRI2CreateBuffer(DrawablePtr drawable, unsigned int attachment,
 	pixmap = NULL;
 	if (attachment == DRI2BufferFrontLeft)
 		pixmap = get_front_buffer(drawable);
-	if (pixmap == NULL) {
+
+	is_glamor_pixmap = pixmap && (intel_get_pixmap_private(pixmap) == NULL);
+	if (pixmap == NULL || is_glamor_pixmap) {
 		unsigned int hint = INTEL_CREATE_PIXMAP_DRI2;
 		int pixmap_width = drawable->width;
 		int pixmap_height = drawable->height;
@@ -400,9 +466,12 @@ I830DRI2CreateBuffer(DrawablePtr drawable, unsigned int attachment,
 			free(buffer);
 			return NULL;
 		}
-
-		if (attachment == DRI2BufferFrontLeft)
-			pixmap = fixup_shadow(drawable, pixmap);
+		if (attachment == DRI2BufferFrontLeft) {
+			if (!is_glamor_pixmap)
+				pixmap = fixup_shadow(drawable, pixmap);
+			else
+				pixmap = fixup_glamor(drawable, pixmap);
+		}
 	}
 
 	buffer->attachment = attachment;
diff --git a/src/intel_uxa.c b/src/intel_uxa.c
index 9d74554..f04a2ef 100644
--- a/src/intel_uxa.c
+++ b/src/intel_uxa.c
@@ -1026,6 +1026,12 @@ intel_uxa_create_pixmap(ScreenPtr screen, int w, int h, int depth,
 	struct intel_pixmap *priv;
 	PixmapPtr pixmap, new_pixmap = NULL;
 
+	if (!(usage & INTEL_CREATE_PIXMAP_DRI2)) {
+		pixmap = intel_glamor_create_pixmap(screen, w, h, depth, usage);
+		if (pixmap)
+			return pixmap;
+	}
+
 	if (w > 32767 || h > 32767)
 		return NullPixmap;
 
-- 
1.7.4.4

Re: [Glamor] [PATCH v2] uxa/glamor: Create glamor pixmap by default.

[Zhigang Gong]

> -----Original Message-----
> From:
> glamor-bounces+zhigang.gong=linux.intel.com@lists.freedesktop.org
> [mailto:glamor-bounces+zhigang.gong=linux.intel.com@lists.freedesktop.o
> rg] On Behalf Of Chris Wilson
> Sent: Wednesday, January 11, 2012 2:01 AM
> To: zhigang.gong@gmail.com
> Cc: intel-gfx@lists.freedesktop.org; zhigang.gong@linux.intel.com;
> zhigang.gong@gmail.com; glamor@lists.freedesktop.org
> Subject: Re: [Glamor] [PATCH v2] uxa/glamor: Create glamor pixmap by
> default.
> 
> On Wed, 11 Jan 2012 10:01:09 +0800, zhigang.gong@gmail.com wrote:
> > From: Zhigang Gong <zhigang.gong@linux.intel.com>
> >
> > A minor fix, after convert the old pixmap to the textured-drm pixmap,
> > we need to modify the old pixmap's header to make sure the
> > width/height and stride are the same as the new textured-drm BO.
> 
> We can not just call FatalError there, but should just propagate the error
> so that the client at least sees BadMatch and X doesn't simply die... How
Agree. X server should not die for a unsupported client application. Maybe I
can just change it to put a warning indicator there.

As to propagate the error to client, I have no idea how to pass a BadMatch
error To client. 
Just return the old texture_only pixmap, and the pixmap_flink will get a 0
name, and then
the client will get a 0 buffer name. If the client check the buffer name
then it can
find something bad happened. If it doesn't, then client may trigger a
segfault which
is what mutter does currently.  Any hint here?

> does this resolve the issue of mesa replacing a bo for a glamor pixmap
> even though we've exported it? Or is that simply an orthogonal problem to
> be tackled later?

My understanding is for the texture which is bound to KHR Image by using
EGLlImageTargetTexture2DOES, mesa will not replacing its BO to a new one. 
For those pure glamor pixmap, as we never export a pure glamor pixmap's BO,
it
will not be a problem. The fixup_glamor will convert a texture only
pixmap to a textured_drm pixmap eventually, then after we export its BO,
mesa will not change it.  Right?

> 
> > ---
> > As create glamor pixmap by default will get much better performance by
> > using the textured-drm pixmap, this commit is to make that the default
> > behaviour when configure to use glamor.
> >
> > A side effect is that for those glamor pixmaps, they don't have a
> > valid BO attached to it and thus it fails to get a DRI drawable. This
> > commit also fixes that problem by copy the fixup_shadow mechanism. I
> > tested this with mutter, and it seems work fine.
> >
> > The performance gain to apply this patch is about 20% to 40% with
> > different workload.
> 
> Waiting to see if I get any results to support that claim... ;-) You can
also
> mention that by using glamor to allocate the pixmaps, we reduce the risk
> of encountering the "incompatible region exists for this name"
> and the associated render corruption. Until that is resolved, every time
we
> export a DRI pixmap and create a BO we still may trigger that bug.
Yeah, will add that at v3 patch.

> However, since we now never intentionally allocate a reusable pixmap we
> could just make all (intel_glamor) allocations non-reusable without
> incurring too great an overhead.
Agree, will disable reusable when create textured_drm pixmap at v3 patch.

> -Chris
> 
> --
> Chris Wilson, Intel Open Source Technology Centre
> _______________________________________________
> Glamor mailing list
> Glamor@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/glamor

Re: [Glamor] [PATCH v2] uxa/glamor: Create glamor pixmap by default.

[He Junyan]

Use XTS for basic check and passed.
No regression found.
Tested-by: He Junyan<junyan.he@linux.intel.com>

>>   -----Original Message-----
>>   From:
>>   glamor-bounces+zhigang.gong=linux.intel.com@lists.freedesktop.org
>>   [mailto:glamor-bounces+zhigang.gong=linux.intel.com@lists.freedesktop.o
>>   rg] On Behalf Of Chris Wilson
>>   Sent: Wednesday, January 11, 2012 2:01 AM
>>   To: zhigang.gong@gmail.com
>>   Cc: intel-gfx@lists.freedesktop.org; zhigang.gong@linux.intel.com;
>>   zhigang.gong@gmail.com; glamor@lists.freedesktop.org
>>   Subject: Re: [Glamor] [PATCH v2] uxa/glamor: Create glamor pixmap by
>>   default.
>>
>>   On Wed, 11 Jan 2012 10:01:09 +0800, zhigang.gong@gmail.com wrote:
>>>   From: Zhigang Gong<zhigang.gong@linux.intel.com>
>>>
>>>   A minor fix, after convert the old pixmap to the textured-drm pixmap,
>>>   we need to modify the old pixmap's header to make sure the
>>>   width/height and stride are the same as the new textured-drm BO.
>>   We can not just call FatalError there, but should just propagate the error
>>   so that the client at least sees BadMatch and X doesn't simply die... How
>   Agree. X server should not die for a unsupported client application. Maybe I
>   can just change it to put a warning indicator there.
>
>   As to propagate the error to client, I have no idea how to pass a BadMatch
>   error To client.
>   Just return the old texture_only pixmap, and the pixmap_flink will get a 0
>   name, and then
>   the client will get a 0 buffer name. If the client check the buffer name
>   then it can
>   find something bad happened. If it doesn't, then client may trigger a
>   segfault which
>   is what mutter does currently.  Any hint here?
>
>>   does this resolve the issue of mesa replacing a bo for a glamor pixmap
>>   even though we've exported it? Or is that simply an orthogonal problem to
>>   be tackled later?
>   My understanding is for the texture which is bound to KHR Image by using
>   EGLlImageTargetTexture2DOES, mesa will not replacing its BO to a new one.
>   For those pure glamor pixmap, as we never export a pure glamor pixmap's BO,
>   it
>   will not be a problem. The fixup_glamor will convert a texture only
>   pixmap to a textured_drm pixmap eventually, then after we export its BO,
>   mesa will not change it.  Right?
>
>>>   ---
>>>   As create glamor pixmap by default will get much better performance by
>>>   using the textured-drm pixmap, this commit is to make that the default
>>>   behaviour when configure to use glamor.
>>>
>>>   A side effect is that for those glamor pixmaps, they don't have a
>>>   valid BO attached to it and thus it fails to get a DRI drawable. This
>>>   commit also fixes that problem by copy the fixup_shadow mechanism. I
>>>   tested this with mutter, and it seems work fine.
>>>
>>>   The performance gain to apply this patch is about 20% to 40% with
>>>   different workload.
>>   Waiting to see if I get any results to support that claim... ;-) You can
>   also
>>   mention that by using glamor to allocate the pixmaps, we reduce the risk
>>   of encountering the "incompatible region exists for this name"
>>   and the associated render corruption. Until that is resolved, every time
>   we
>>   export a DRI pixmap and create a BO we still may trigger that bug.
>   Yeah, will add that at v3 patch.
>
>>   However, since we now never intentionally allocate a reusable pixmap we
>>   could just make all (intel_glamor) allocations non-reusable without
>>   incurring too great an overhead.
>   Agree, will disable reusable when create textured_drm pixmap at v3 patch.
>
>>   -Chris
>>
>>   --
>>   Chris Wilson, Intel Open Source Technology Centre
>>   _______________________________________________
>>   Glamor mailing list
>>   Glamor@lists.freedesktop.org
>>   http://lists.freedesktop.org/mailman/listinfo/glamor
>   _______________________________________________
>   Glamor mailing list
>   Glamor@lists.freedesktop.org
>   http://lists.freedesktop.org/mailman/listinfo/glamor
>