public inbox for intel-gfx@lists.freedesktop.org
 help / color / mirror / Atom feed
* [PATCH 1/3] glamor: Initial commit to introduce glamor acceleration.
@ 2011-11-11  8:31 Zhigang Gong
  2011-11-11  8:31 ` [PATCH 2/3] glamor: turn on glamor Zhigang Gong
                   ` (2 more replies)
  0 siblings, 3 replies; 16+ messages in thread
From: Zhigang Gong @ 2011-11-11  8:31 UTC (permalink / raw)
  To: intel-gfx

Added one configuration option --enable-glamor to control
whether use glamor. Added one new file intel_glamor.c to
wrap glamor egl API for intel driver's usage.
This commit doesn't really change the driver's control path.
It just adds necessary files for glamor and change some
configuration.

Signed-off-by: Zhigang Gong <zhigang.gong@linux.intel.com>
---
 configure.ac       |   17 +++++++
 src/Makefile.am    |    5 ++
 src/intel_glamor.c |  130 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 src/intel_glamor.h |   45 ++++++++++++++++++
 4 files changed, 197 insertions(+), 0 deletions(-)
 create mode 100644 src/intel_glamor.c
 create mode 100644 src/intel_glamor.h

diff --git a/configure.ac b/configure.ac
index fccab56..dc01c46 100644
--- a/configure.ac
+++ b/configure.ac
@@ -126,6 +126,15 @@ AC_ARG_ENABLE(sna,
 	      [SNA="$enableval"],
 	      [SNA=no])
 AM_CONDITIONAL(SNA, test x$SNA != xno)
+
+AC_ARG_ENABLE(glamor,
+	      AS_HELP_STRING([--enable-glamor],
+			     [Enable glamor, a new GL-based acceleration [default=no]]),
+	      [GLAMOR="$enableval"],
+	      [GLAMOR=no])
+
+AM_CONDITIONAL(GLAMOR, test x$GLAMOR != xno)
+
 AC_MSG_CHECKING([whether to include SNA support])
 
 required_xorg_xserver_version=1.6
@@ -137,6 +146,14 @@ if test "x$SNA" != "xno"; then
 fi
 AC_MSG_RESULT([$SNA])
 
+if test "x$GLAMOR" != "xno"; then
+	PKG_CHECK_MODULES(LIBGLAMOR, [glamor])
+	PKG_CHECK_MODULES(LIBGLAMOR_EGL, [glamor-egl])
+	AC_DEFINE(GLAMOR, 1, [Enable glamor acceleration])
+fi
+
+AC_MSG_CHECKING([whether to include GLAMOR support])
+AC_MSG_RESULT([$GLAMOR])
 
 AC_ARG_ENABLE(vmap,
 	      AS_HELP_STRING([--enable-vmap],
diff --git a/src/Makefile.am b/src/Makefile.am
index cd1bb36..1a29390 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -40,6 +40,10 @@ SUBDIRS += sna
 intel_drv_la_LIBADD += sna/libsna.la
 endif
 
+if GLAMOR
+GLAMOR_SOURCE = intel_glamor.c
+endif
+
 NULL:=#
 
 intel_drv_la_SOURCES = \
@@ -70,6 +74,7 @@ intel_drv_la_SOURCES = \
 	 i965_3d.c \
 	 i965_video.c \
 	 i965_render.c \
+	 $(GLAMOR_SOURCE) \
 	 $(NULL)
 
 if DRI
diff --git a/src/intel_glamor.c b/src/intel_glamor.c
new file mode 100644
index 0000000..cadfc71
--- /dev/null
+++ b/src/intel_glamor.c
@@ -0,0 +1,130 @@
+/*
+ * Copyright © 2011 Intel Corporation.
+ *
+ * Permission is hereby granted, free of charge, to any person
+ * obtaining a copy of this software and associated documentation
+ * files (the "Software"), to deal in the Software without
+ * restriction, including without limitation the rights to use, copy,
+ * modify, merge, publish, distribute, sublicense, and/or sell copies
+ * of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including
+ * the next paragraph) shall be included in all copies or substantial
+ * portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
+ * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
+ * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
+ * NONINFRINGEMENT.  IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT
+ * HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY,
+ * WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
+ * DEALINGS IN THE SOFTWARE.
+ *
+ * Authors:
+ *    Zhigang Gong <zhigang.gong@linux.intel.com>
+ *
+ */
+
+#ifdef HAVE_DIX_CONFIG_H
+#include <dix-config.h>
+#endif
+#include <unistd.h>
+#include <fcntl.h>
+#include <sys/ioctl.h>
+#include <errno.h>
+#include <xf86drm.h>
+
+#define GLAMOR_FOR_XORG  1
+
+#include <glamor.h>
+#include "intel.h"
+#include "intel_glamor.h"
+
+Bool
+intel_glamor_create_screen_resources(ScreenPtr screen)
+{
+	ScrnInfoPtr scrn = xf86Screens[screen->myNum];
+	intel_screen_private *intel = intel_get_screen_private(scrn);
+
+	if (!glamor_glyphs_init(screen))
+		return FALSE;
+	if (!glamor_egl_create_textured_screen(screen,
+					       intel->front_buffer->handle,
+					       intel->front_pitch))
+		return FALSE;
+	return TRUE;
+}
+
+Bool
+intel_glamor_pre_init(ScrnInfoPtr scrn)
+{
+	intel_screen_private *intel;
+	intel = intel_get_screen_private(scrn);
+	return glamor_egl_init(scrn, intel->drmSubFD);
+}
+
+Bool
+intel_glamor_create_textured_pixmap(PixmapPtr pixmap)
+{
+	struct intel_pixmap *priv;
+	priv = intel_get_pixmap_private(pixmap);
+	return glamor_egl_create_textured_pixmap(pixmap, priv->bo->handle,
+						 priv->stride);
+}
+
+void
+intel_glamor_destroy_pixmap(PixmapPtr pixmap)
+{
+	glamor_egl_destroy_textured_pixmap(pixmap);
+}
+
+Bool
+intel_glamor_init(ScreenPtr screen)
+{
+	ScrnInfoPtr scrn = xf86Screens[screen->myNum];
+
+	if (!glamor_init(screen, GLAMOR_INVERTED_Y_AXIS)) {
+		xf86DrvMsg(scrn->scrnIndex, X_ERROR,
+			   "Failed to initialize glamor\n");
+		return FALSE;
+	}
+
+	if (!glamor_egl_init_textured_pixmap(screen)) {
+		xf86DrvMsg(scrn->scrnIndex, X_ERROR,
+			   "Failed to initialize glamor\n");
+		return FALSE;
+	}
+
+	return TRUE;
+}
+
+void
+intel_glamor_block_handler(intel_screen_private * intel)
+{
+	ScreenPtr screen = screenInfo.screens[intel->scrn->scrnIndex];
+	glamor_block_handler(screen);
+}
+
+Bool
+intel_glamor_create_screen_image(ScreenPtr screen, int handle, int stride)
+{
+	PixmapPtr pixmap;
+	if (!glamor_egl_create_textured_screen(screen, handle, stride))
+		return FALSE;
+	pixmap = screen->GetScreenPixmap(screen);
+	return TRUE;
+}
+
+Bool
+intel_glamor_close_screen(ScreenPtr screen)
+{
+	return glamor_egl_close_screen(screen);
+}
+
+void
+intel_glamor_free_screen(int scrnIndex, int flags)
+{
+	glamor_egl_free_screen(scrnIndex, GLAMOR_EGL_EXTERNAL_BUFFER);
+}
diff --git a/src/intel_glamor.h b/src/intel_glamor.h
new file mode 100644
index 0000000..d7c2c86
--- /dev/null
+++ b/src/intel_glamor.h
@@ -0,0 +1,45 @@
+/*
+ * Copyright © 2011 Intel Corporation.
+ *
+ * Permission is hereby granted, free of charge, to any person
+ * obtaining a copy of this software and associated documentation
+ * files (the "Software"), to deal in the Software without
+ * restriction, including without limitation the rights to use, copy,
+ * modify, merge, publish, distribute, sublicense, and/or sell copies
+ * of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including
+ * the next paragraph) shall be included in all copies or substantial
+ * portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
+ * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
+ * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
+ * NONINFRINGEMENT.  IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT
+ * HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY,
+ * WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
+ * DEALINGS IN THE SOFTWARE.
+ *
+ * Authors:
+ *    Zhigang Gong <zhigang.gong@linux.intel.com>
+ *
+ */
+Bool intel_glamor_pre_init(ScrnInfoPtr scrn);
+
+Bool intel_glamor_init(ScreenPtr screen);
+
+Bool intel_glamor_create_screen_resources(ScreenPtr screen);
+
+Bool intel_glamor_create_screen_image(ScreenPtr screen, int handle, int stride);
+
+Bool intel_glamor_close_screen(ScreenPtr screen);
+
+void intel_glamor_free_screen(int scrnIndex, int flags);
+
+void intel_glamor_block_handler(intel_screen_private * intel);
+
+Bool intel_glamor_create_textured_pixmap(PixmapPtr pixmap);
+
+void intel_glamor_destroy_pixmap(PixmapPtr pixmap);
-- 
1.7.4.4

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

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH 2/3] glamor: turn on glamor.
  2011-11-11  8:31 [PATCH 1/3] glamor: Initial commit to introduce glamor acceleration Zhigang Gong
@ 2011-11-11  8:31 ` Zhigang Gong
  2011-11-11  9:11   ` Chris Wilson
  2011-11-11 12:58   ` Eugeni Dodonov
  2011-11-11  8:31 ` [PATCH 3/3] glamor: Route fillspans and polyfillrects to glamor Zhigang Gong
  2011-11-11 12:56 ` [PATCH 1/3] glamor: Initial commit to introduce glamor acceleration Eugeni Dodonov
  2 siblings, 2 replies; 16+ messages in thread
From: Zhigang Gong @ 2011-11-11  8:31 UTC (permalink / raw)
  To: intel-gfx

Add glamor's initialization to the uxa's control path.
At preInit stage, it creates and initialize EGL display
context for glamor. At the screenInit stage, it initialize
glamor's internal data structures and shaders.

And this commit enables textured pixmap also. Each pixmap
which has a valid BO can get a cooresponding texture.

After this commit. It's ready to do rendering through
glamor's rendering functions.

Signed-off-by: Zhigang Gong <zhigang.gong@linux.intel.com>
---
 src/intel_driver.c |   25 ++++++++++++++++++++++++-
 src/intel_uxa.c    |   22 +++++++++++++++++++++-
 2 files changed, 45 insertions(+), 2 deletions(-)

diff --git a/src/intel_driver.c b/src/intel_driver.c
index 24696da..cf50d67 100644
--- a/src/intel_driver.c
+++ b/src/intel_driver.c
@@ -75,6 +75,10 @@ USE OR OTHER DEALINGS IN THE SOFTWARE.
 #include "i915_drm.h"
 #include <xf86drmMode.h>
 
+#ifdef GLAMOR
+#include "intel_glamor.h"
+#endif
+
 /* *INDENT-OFF* */
 /*
  * Note: "ColorKey" is provided for compatibility with the i810 driver.
@@ -712,6 +716,20 @@ static Bool I830PreInit(ScrnInfoPtr scrn, int flags)
 		return FALSE;
 	}
 
+#ifdef GLAMOR
+        /* Load glamor module */
+        if (!xf86LoadSubModule(scrn, "glamor_egl")) {
+                PreInitCleanup(scrn);
+                return FALSE;
+        }
+
+        if (!intel_glamor_pre_init(scrn)) {
+               xf86DrvMsg(scrn->scrnIndex, X_ERROR,
+                          "Failed to pre init glamor display.\n");
+               return FALSE;
+        }
+#endif
+
 	/* Load the dri2 module if requested. */
 	if (intel->directRenderingType != DRI_DISABLED)
 		xf86LoadSubModule(scrn, "dri2");
@@ -1109,7 +1127,8 @@ static void I830FreeScreen(int scrnIndex, int flags)
 {
 	ScrnInfoPtr scrn = xf86Screens[scrnIndex];
 	intel_screen_private *intel = intel_get_screen_private(scrn);
-
+#ifdef GLAMOR
+#endif
 	if (intel) {
 		intel_mode_fini(intel);
 		intel_close_drm_master(intel);
@@ -1189,6 +1208,10 @@ static Bool I830CloseScreen(int scrnIndex, ScreenPtr screen)
 
 	DeleteCallback(&FlushCallback, intel_flush_callback, scrn);
 
+#ifdef GLAMOR
+	intel_glamor_close_screen(screen);
+#endif
+
 	if (intel->uxa_driver) {
 		uxa_driver_fini(screen);
 		free(intel->uxa_driver);
diff --git a/src/intel_uxa.c b/src/intel_uxa.c
index 8c6f754..f0e5803 100644
--- a/src/intel_uxa.c
+++ b/src/intel_uxa.c
@@ -40,6 +40,10 @@ SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
 #include <string.h>
 #include <errno.h>
 
+#ifdef GLAMOR
+#include "intel_glamor.h"
+#endif
+
 static const int I830CopyROP[16] = {
 	ROP_0,			/* GXclear */
 	ROP_DSa,		/* GXand */
@@ -965,6 +969,9 @@ void intel_uxa_block_handler(intel_screen_private *intel)
 	 * framebuffer until significantly later.
 	 */
 	intel_flush_rendering(intel);
+#ifdef GLAMOR
+	intel_glamor_block_handler(intel);
+#endif
 }
 
 static PixmapPtr
@@ -1095,6 +1102,10 @@ 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);
+#ifdef GLAMOR
+		priv->pinned = 1;
+                intel_glamor_create_textured_pixmap(pixmap);
+#endif
 	}
 
 	return pixmap;
@@ -1102,8 +1113,12 @@ intel_uxa_create_pixmap(ScreenPtr screen, int w, int h, int depth,
 
 static Bool intel_uxa_destroy_pixmap(PixmapPtr pixmap)
 {
-	if (pixmap->refcnt == 1)
+	if (pixmap->refcnt == 1) {
+#ifdef GLAMOR
+		intel_glamor_destroy_pixmap(pixmap);
+#endif
 		intel_set_pixmap_bo(pixmap, NULL);
+	}
 	fbDestroyPixmap(pixmap);
 	return TRUE;
 }
@@ -1134,6 +1149,9 @@ Bool intel_uxa_create_screen_resources(ScreenPtr screen)
 		scrn->displayWidth = intel->front_pitch / intel->cpp;
 	}
 
+	if (!intel_glamor_create_screen_resources(screen))
+		return FALSE;
+
 	return TRUE;
 }
 
@@ -1296,5 +1314,7 @@ Bool intel_uxa_init(ScreenPtr screen)
 	uxa_set_fallback_debug(screen, intel->fallback_debug);
 	uxa_set_force_fallback(screen, intel->force_fallback);
 
+	intel_glamor_init(screen);
+
 	return TRUE;
 }
-- 
1.7.4.4

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH 3/3] glamor: Route fillspans and polyfillrects to glamor.
  2011-11-11  8:31 [PATCH 1/3] glamor: Initial commit to introduce glamor acceleration Zhigang Gong
  2011-11-11  8:31 ` [PATCH 2/3] glamor: turn on glamor Zhigang Gong
@ 2011-11-11  8:31 ` Zhigang Gong
  2011-11-11  9:07   ` Chris Wilson
  2011-11-11 12:56 ` [PATCH 1/3] glamor: Initial commit to introduce glamor acceleration Eugeni Dodonov
  2 siblings, 1 reply; 16+ messages in thread
From: Zhigang Gong @ 2011-11-11  8:31 UTC (permalink / raw)
  To: intel-gfx

If GLAMOR is enabled, we route UXA's fillspans and
polyfillrects to glamor by default. And if glamor
fail to accelerate it, UXA continue to handle it.

Signed-off-by: Zhigang Gong <zhigang.gong@linux.intel.com>
---
 uxa/uxa-accel.c |   13 ++++++++++++-
 1 files changed, 12 insertions(+), 1 deletions(-)

diff --git a/uxa/uxa-accel.c b/uxa/uxa-accel.c
index 516834f..18fa63b 100644
--- a/uxa/uxa-accel.c
+++ b/uxa/uxa-accel.c
@@ -27,7 +27,6 @@
  *    Michel Dänzer <michel@tungstengraphics.com>
  *
  */
-
 #ifdef HAVE_DIX_CONFIG_H
 #include <dix-config.h>
 #endif
@@ -37,6 +36,10 @@
 #include "uxa.h"
 #include "mipict.h"
 
+#ifdef GLAMOR
+#include "glamor.h"
+#endif
+
 static void
 uxa_fill_spans(DrawablePtr pDrawable, GCPtr pGC, int n,
 	       DDXPointPtr ppt, int *pwidth, int fSorted)
@@ -49,6 +52,10 @@ uxa_fill_spans(DrawablePtr pDrawable, GCPtr pGC, int n,
 	int nbox;
 	int x1, x2, y;
 	int off_x, off_y;
+#ifdef GLAMOR
+	if (glamor_fill_spans_nf(pDrawable, pGC, n, ppt, pwidth, fSorted))
+		return;
+#endif
 
 	if (uxa_screen->swappedOut || uxa_screen->force_fallback)
 		goto fallback;
@@ -673,6 +680,10 @@ uxa_poly_fill_rect(DrawablePtr pDrawable,
 	int n;
 	RegionPtr pReg = RECTS_TO_REGION(pScreen, nrect, prect, CT_UNSORTED);
 
+#ifdef GLAMOR
+	if (glamor_poly_fill_rect_nf(pDrawable, pGC, nrect, prect))
+		return;
+#endif
 	/* Compute intersection of rects and clip region */
 	REGION_TRANSLATE(pScreen, pReg, pDrawable->x, pDrawable->y);
 	REGION_INTERSECT(pScreen, pReg, pClip, pReg);
-- 
1.7.4.4

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

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [PATCH 3/3] glamor: Route fillspans and polyfillrects to glamor.
  2011-11-11  8:31 ` [PATCH 3/3] glamor: Route fillspans and polyfillrects to glamor Zhigang Gong
@ 2011-11-11  9:07   ` Chris Wilson
  2011-11-11 10:48     ` Zhigang Gong
  0 siblings, 1 reply; 16+ messages in thread
From: Chris Wilson @ 2011-11-11  9:07 UTC (permalink / raw)
  To: Zhigang Gong, intel-gfx

On Fri, 11 Nov 2011 16:31:21 +0800, Zhigang Gong <zhigang.gong@linux.intel.com> wrote:
> If GLAMOR is enabled, we route UXA's fillspans and
> polyfillrects to glamor by default. And if glamor
> fail to accelerate it, UXA continue to handle it.

How is serialisation handled between the UXA and glamor acceleration
routines? Don't you need to flush the UXA batch (if the pixmap is active)
before handing over to glamor and similarly flush the glamor pixmap
after failure?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 2/3] glamor: turn on glamor.
  2011-11-11  8:31 ` [PATCH 2/3] glamor: turn on glamor Zhigang Gong
@ 2011-11-11  9:11   ` Chris Wilson
  2011-11-11 10:52     ` Zhigang Gong
  2011-11-11 12:58   ` Eugeni Dodonov
  1 sibling, 1 reply; 16+ messages in thread
From: Chris Wilson @ 2011-11-11  9:11 UTC (permalink / raw)
  To: Zhigang Gong, intel-gfx

On Fri, 11 Nov 2011 16:31:20 +0800, Zhigang Gong <zhigang.gong@linux.intel.com> wrote:
> @@ -965,6 +969,9 @@ void intel_uxa_block_handler(intel_screen_private *intel)
>  	 * framebuffer until significantly later.
>  	 */
>  	intel_flush_rendering(intel);
> +#ifdef GLAMOR
> +	intel_glamor_block_handler(intel);
> +#endif
>  }

I suspect this is the wrong way around as we are not flushing the
render cache of glamor's rendering to the scanout until the next block
handler.

In general, try to keep the #ifdef out of the body of the code. In this
case, and others, make intel_glamor_block_handler() be a no-op if GLAMOR
is not enabled.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 3/3] glamor: Route fillspans and polyfillrects to glamor.
  2011-11-11  9:07   ` Chris Wilson
@ 2011-11-11 10:48     ` Zhigang Gong
  2011-11-11 13:54       ` Chris Wilson
  0 siblings, 1 reply; 16+ messages in thread
From: Zhigang Gong @ 2011-11-11 10:48 UTC (permalink / raw)
  To: 'Chris Wilson', intel-gfx

> -----Original Message-----
> From: Chris Wilson [mailto:chris@chris-wilson.co.uk]
> Sent: Friday, November 11, 2011 5:08 PM
> To: Zhigang Gong; intel-gfx@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH 3/3] glamor: Route fillspans and
polyfillrects
> to glamor.
> 
> On Fri, 11 Nov 2011 16:31:21 +0800, Zhigang Gong
> <zhigang.gong@linux.intel.com> wrote:
> > If GLAMOR is enabled, we route UXA's fillspans and polyfillrects to
> > glamor by default. And if glamor fail to accelerate it, UXA continue
> > to handle it.
> 
> How is serialisation handled between the UXA and glamor acceleration
> routines? Don't you need to flush the UXA batch (if the pixmap is active)
> before handing over to glamor and similarly flush the glamor pixmap after
> failure?
Thanks for pointing this issue out. This is something I want to be discussed
here.

There are three types of access to the pixmap:
1. UXA batch command buffer.
2. Glamor through OpenGL
3. CPU access mapped BO buffer.

My understanding is that the leading two types has the queue mechanism and
need
to be handled carefully. In general, we can treat glamor 's access as
another batch 
buffer. Then in the place where current intel driver need to flush UXA batch
buffer, 
we also need to flush the GL operations there. Right? 

And besides above places we need to flush glamor, we also need to flush UXA
batch
buffer before call into glamor and also need to flush glamor after the
glamor rendering
function really touch the pixmap.

Any comments?

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 2/3] glamor: turn on glamor.
  2011-11-11  9:11   ` Chris Wilson
@ 2011-11-11 10:52     ` Zhigang Gong
  2011-11-11 13:12       ` Chris Wilson
  0 siblings, 1 reply; 16+ messages in thread
From: Zhigang Gong @ 2011-11-11 10:52 UTC (permalink / raw)
  To: 'Chris Wilson', intel-gfx



> -----Original Message-----
> From: Chris Wilson [mailto:chris@chris-wilson.co.uk]
> Sent: Friday, November 11, 2011 5:12 PM
> To: Zhigang Gong; intel-gfx@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH 2/3] glamor: turn on glamor.
> 
> On Fri, 11 Nov 2011 16:31:20 +0800, Zhigang Gong
> <zhigang.gong@linux.intel.com> wrote:
> > @@ -965,6 +969,9 @@ void
> intel_uxa_block_handler(intel_screen_private *intel)
> >  	 * framebuffer until significantly later.
> >  	 */
> >  	intel_flush_rendering(intel);
> > +#ifdef GLAMOR
> > +	intel_glamor_block_handler(intel);
> > +#endif
> >  }
> 
> I suspect this is the wrong way around as we are not flushing the render
> cache of glamor's rendering to the scanout until the next block handler.
I don't understand here. Would you please explain more detail? Thanks.

> 
> In general, try to keep the #ifdef out of the body of the code. In this
case,
> and others, make intel_glamor_block_handler() be a no-op if GLAMOR is
> not enabled.
Agreed, will fix it next version. 

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 1/3] glamor: Initial commit to introduce glamor acceleration.
  2011-11-11  8:31 [PATCH 1/3] glamor: Initial commit to introduce glamor acceleration Zhigang Gong
  2011-11-11  8:31 ` [PATCH 2/3] glamor: turn on glamor Zhigang Gong
  2011-11-11  8:31 ` [PATCH 3/3] glamor: Route fillspans and polyfillrects to glamor Zhigang Gong
@ 2011-11-11 12:56 ` Eugeni Dodonov
  2 siblings, 0 replies; 16+ messages in thread
From: Eugeni Dodonov @ 2011-11-11 12:56 UTC (permalink / raw)
  To: Zhigang Gong; +Cc: intel-gfx


[-- Attachment #1.1: Type: text/plain, Size: 734 bytes --]

On Fri, Nov 11, 2011 at 06:31, Zhigang Gong <zhigang.gong@linux.intel.com>wrote:

> Added one configuration option --enable-glamor to control
> whether use glamor. Added one new file intel_glamor.c to
> wrap glamor egl API for intel driver's usage.
> This commit doesn't really change the driver's control path.
> It just adds necessary files for glamor and change some
> configuration.
>

For the series, I've reviewed the patches and they seem OK for me. I also
like the idea of having Glamor available in main ddx branch, for easier
testing.

So:
Reviewed-by: Eugeni Dodonov <eugeni.dodonov@intel.com>

(However, I think Chris will have more comments, he is The Master of the
DDX).

-- 
Eugeni Dodonov
<http://eugeni.dodonov.net/>

[-- Attachment #1.2: Type: text/html, Size: 1163 bytes --]

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

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 2/3] glamor: turn on glamor.
  2011-11-11  8:31 ` [PATCH 2/3] glamor: turn on glamor Zhigang Gong
  2011-11-11  9:11   ` Chris Wilson
@ 2011-11-11 12:58   ` Eugeni Dodonov
  2011-11-14  5:05     ` Zhigang Gong
  1 sibling, 1 reply; 16+ messages in thread
From: Eugeni Dodonov @ 2011-11-11 12:58 UTC (permalink / raw)
  To: Zhigang Gong; +Cc: intel-gfx


[-- Attachment #1.1: Type: text/plain, Size: 390 bytes --]

On Fri, Nov 11, 2011 at 06:31, Zhigang Gong <zhigang.gong@linux.intel.com>wrote:

> @@ -1109,7 +1127,8 @@ static void I830FreeScreen(int scrnIndex, int flags)
>  {
>        ScrnInfoPtr scrn = xf86Screens[scrnIndex];
>        intel_screen_private *intel = intel_get_screen_private(scrn);
> -
> +#ifdef GLAMOR
> +#endif
>

Empty #ifdef block?

-- 
Eugeni Dodonov
<http://eugeni.dodonov.net/>

[-- Attachment #1.2: Type: text/html, Size: 707 bytes --]

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

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 2/3] glamor: turn on glamor.
  2011-11-11 10:52     ` Zhigang Gong
@ 2011-11-11 13:12       ` Chris Wilson
  2011-11-14  5:01         ` Zhigang Gong
  0 siblings, 1 reply; 16+ messages in thread
From: Chris Wilson @ 2011-11-11 13:12 UTC (permalink / raw)
  To: Zhigang Gong, intel-gfx

On Fri, 11 Nov 2011 18:52:11 +0800, "Zhigang Gong" <zhigang.gong@linux.intel.com> wrote:
> 
> 
> > -----Original Message-----
> > From: Chris Wilson [mailto:chris@chris-wilson.co.uk]
> > Sent: Friday, November 11, 2011 5:12 PM
> > To: Zhigang Gong; intel-gfx@lists.freedesktop.org
> > Subject: Re: [Intel-gfx] [PATCH 2/3] glamor: turn on glamor.
> > 
> > On Fri, 11 Nov 2011 16:31:20 +0800, Zhigang Gong
> > <zhigang.gong@linux.intel.com> wrote:
> > > @@ -965,6 +969,9 @@ void
> > intel_uxa_block_handler(intel_screen_private *intel)
> > >  	 * framebuffer until significantly later.
> > >  	 */
> > >  	intel_flush_rendering(intel);
> > > +#ifdef GLAMOR
> > > +	intel_glamor_block_handler(intel);
> > > +#endif
> > >  }
> > 
> > I suspect this is the wrong way around as we are not flushing the render
> > cache of glamor's rendering to the scanout until the next block handler.
> I don't understand here. Would you please explain more detail? Thanks.

Whenever we render, the data ends up in the Render Cache and needs to be
flushed out to memory before it is coherent with the CPU or in this case
the Display Engine (i.e. scanout).

intel_flush_rendering() does two tasks. The first is to submit any
pending batch, and the second is to flush the Render Cache so that the
modifications land on the scanout in a timely manner. It is probably
best if those two tasks were separated so that we do:

  intel_uxa_block_handler(intel); // flush the UXA batch
  intel_glamor_block_handler(intel); // flush the GL batch
  intel_flush_rendering(intel); // flush the RenderCache to scanout

However, you can simply rearrange the code and achieve it with the
existing functions:

  intel_glamor_block_handler(intel); // mark the front bo as dirty as needbe
  intel_flush_rendering(intel); // flush UXA batch along with RenderCache

-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 3/3] glamor: Route fillspans and polyfillrects to glamor.
  2011-11-11 10:48     ` Zhigang Gong
@ 2011-11-11 13:54       ` Chris Wilson
  2011-11-14  3:32         ` Zhigang Gong
  0 siblings, 1 reply; 16+ messages in thread
From: Chris Wilson @ 2011-11-11 13:54 UTC (permalink / raw)
  To: Zhigang Gong, intel-gfx

On Fri, 11 Nov 2011 18:48:57 +0800, "Zhigang Gong" <zhigang.gong@linux.intel.com> wrote:
> > -----Original Message-----
> > From: Chris Wilson [mailto:chris@chris-wilson.co.uk]
> > Sent: Friday, November 11, 2011 5:08 PM
> > To: Zhigang Gong; intel-gfx@lists.freedesktop.org
> > Subject: Re: [Intel-gfx] [PATCH 3/3] glamor: Route fillspans and
> polyfillrects
> > to glamor.
> > 
> > On Fri, 11 Nov 2011 16:31:21 +0800, Zhigang Gong
> > <zhigang.gong@linux.intel.com> wrote:
> > > If GLAMOR is enabled, we route UXA's fillspans and polyfillrects to
> > > glamor by default. And if glamor fail to accelerate it, UXA continue
> > > to handle it.
> > 
> > How is serialisation handled between the UXA and glamor acceleration
> > routines? Don't you need to flush the UXA batch (if the pixmap is active)
> > before handing over to glamor and similarly flush the glamor pixmap after
> > failure?
> Thanks for pointing this issue out. This is something I want to be discussed
> here.
> 
> There are three types of access to the pixmap:
> 1. UXA batch command buffer.
> 2. Glamor through OpenGL
> 3. CPU access mapped BO buffer.
> 
> My understanding is that the leading two types has the queue mechanism and
> need
> to be handled carefully. In general, we can treat glamor 's access as
> another batch 
> buffer. Then in the place where current intel driver need to flush UXA batch
> buffer, 
> we also need to flush the GL operations there. Right? 
> 
> And besides above places we need to flush glamor, we also need to flush UXA
> batch
> buffer before call into glamor and also need to flush glamor after the
> glamor rendering
> function really touch the pixmap.

Right, we want to consider it as another form of pixmap migration, just
instead of between different regions of memory we are migrating between
different queues. We could envision using fences to coordinate rendering
between the different batches, but that is likely to be overkill and not
possible with most Intel hardware.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 3/3] glamor: Route fillspans and polyfillrects to glamor.
  2011-11-11 13:54       ` Chris Wilson
@ 2011-11-14  3:32         ` Zhigang Gong
  0 siblings, 0 replies; 16+ messages in thread
From: Zhigang Gong @ 2011-11-14  3:32 UTC (permalink / raw)
  To: 'Chris Wilson', intel-gfx

> -----Original Message-----
> From: Chris Wilson [mailto:chris@chris-wilson.co.uk]
> Sent: Friday, November 11, 2011 9:55 PM
> To: Zhigang Gong; intel-gfx@lists.freedesktop.org
> Subject: RE: [Intel-gfx] [PATCH 3/3] glamor: Route fillspans and
polyfillrects
> to glamor.
> 
> On Fri, 11 Nov 2011 18:48:57 +0800, "Zhigang Gong"
> <zhigang.gong@linux.intel.com> wrote:
> > > -----Original Message-----
> > > From: Chris Wilson [mailto:chris@chris-wilson.co.uk]
> > > Sent: Friday, November 11, 2011 5:08 PM
> > > To: Zhigang Gong; intel-gfx@lists.freedesktop.org
> > > Subject: Re: [Intel-gfx] [PATCH 3/3] glamor: Route fillspans and
> > polyfillrects
> > > to glamor.
> > >
> > > On Fri, 11 Nov 2011 16:31:21 +0800, Zhigang Gong
> > > <zhigang.gong@linux.intel.com> wrote:
> > > > If GLAMOR is enabled, we route UXA's fillspans and polyfillrects
> > > > to glamor by default. And if glamor fail to accelerate it, UXA
> > > > continue to handle it.
> > >
> > > How is serialisation handled between the UXA and glamor acceleration
> > > routines? Don't you need to flush the UXA batch (if the pixmap is
> > > active) before handing over to glamor and similarly flush the glamor
> > > pixmap after failure?
> > Thanks for pointing this issue out. This is something I want to be
> > discussed here.
> >
> > There are three types of access to the pixmap:
> > 1. UXA batch command buffer.
> > 2. Glamor through OpenGL
> > 3. CPU access mapped BO buffer.
> >
> > My understanding is that the leading two types has the queue
> mechanism
> > and need to be handled carefully. In general, we can treat glamor 's
> > access as another batch buffer. Then in the place where current intel
> > driver need to flush UXA batch buffer, we also need to flush the GL
> > operations there. Right?
> >
> > And besides above places we need to flush glamor, we also need to
> > flush UXA batch buffer before call into glamor and also need to flush
> > glamor after the glamor rendering function really touch the pixmap.
> 
> Right, we want to consider it as another form of pixmap migration, just
> instead of between different regions of memory we are migrating
> between different queues. We could envision using fences to coordinate
> rendering between the different batches, but that is likely to be overkill
> and not possible with most Intel hardware.

Understand, so we can't expect to solve this overhead problem that way. 
IMO, as eventually, glamor will take over all the rendering functions,
if glamor is enabled. This heavy flushing may be get eliminated then.  

Anyway, currently, I still need to work out another patch to handle all the 
needed flushing OPs. Will submit it to review soon.

- Zhigang

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 2/3] glamor: turn on glamor.
  2011-11-11 13:12       ` Chris Wilson
@ 2011-11-14  5:01         ` Zhigang Gong
  2011-11-14  9:07           ` Chris Wilson
  0 siblings, 1 reply; 16+ messages in thread
From: Zhigang Gong @ 2011-11-14  5:01 UTC (permalink / raw)
  To: 'Chris Wilson', intel-gfx



> -----Original Message-----
> From: Chris Wilson [mailto:chris@chris-wilson.co.uk]
> Sent: Friday, November 11, 2011 9:13 PM
> To: Zhigang Gong; intel-gfx@lists.freedesktop.org
> Subject: RE: [Intel-gfx] [PATCH 2/3] glamor: turn on glamor.
> 
> On Fri, 11 Nov 2011 18:52:11 +0800, "Zhigang Gong"
> <zhigang.gong@linux.intel.com> wrote:
> >
> >
> > > -----Original Message-----
> > > From: Chris Wilson [mailto:chris@chris-wilson.co.uk]
> > > Sent: Friday, November 11, 2011 5:12 PM
> > > To: Zhigang Gong; intel-gfx@lists.freedesktop.org
> > > Subject: Re: [Intel-gfx] [PATCH 2/3] glamor: turn on glamor.
> > >
> > > On Fri, 11 Nov 2011 16:31:20 +0800, Zhigang Gong
> > > <zhigang.gong@linux.intel.com> wrote:
> > > > @@ -965,6 +969,9 @@ void
> > > intel_uxa_block_handler(intel_screen_private *intel)
> > > >  	 * framebuffer until significantly later.
> > > >  	 */
> > > >  	intel_flush_rendering(intel);
> > > > +#ifdef GLAMOR
> > > > +	intel_glamor_block_handler(intel);
> > > > +#endif
> > > >  }
> > >
> > > I suspect this is the wrong way around as we are not flushing the
> > > render cache of glamor's rendering to the scanout until the next block
> handler.
> > I don't understand here. Would you please explain more detail? Thanks.
> 
> Whenever we render, the data ends up in the Render Cache and needs to
> be flushed out to memory before it is coherent with the CPU or in this
> case the Display Engine (i.e. scanout).
> 
> intel_flush_rendering() does two tasks. The first is to submit any pending
> batch, and the second is to flush the Render Cache so that the
> modifications land on the scanout in a timely manner. It is probably best
if
> those two tasks were separated so that we do:
> 
>   intel_uxa_block_handler(intel); // flush the UXA batch
>   intel_glamor_block_handler(intel); // flush the GL batch
>   intel_flush_rendering(intel); // flush the RenderCache to scanout
> 
> However, you can simply rearrange the code and achieve it with the
> existing functions:
> 
>   intel_glamor_block_handler(intel); // mark the front bo as dirty as
> needbe
>   intel_flush_rendering(intel); // flush UXA batch along with RenderCache
Thanks for the explanation here. But I still don't think the original code
is wrong
regard to this cache flushing issue. Here is my analysis:
intel_glamor_block_handler calls to glFlush(), and glFlush is similar with
the 
intel_flush_rendering, it calls intel_flush to flush the batch buffers and
then
call intel_flush_frontbuffer to flush the frontbuffer which flushes the scan
out
buffer. So when the screen pixmap is accessed by glamor, and after we call
intel_glamor_block_handler, the Display Engine should see the correct data

Right?

- Zhigang

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 2/3] glamor: turn on glamor.
  2011-11-11 12:58   ` Eugeni Dodonov
@ 2011-11-14  5:05     ` Zhigang Gong
  0 siblings, 0 replies; 16+ messages in thread
From: Zhigang Gong @ 2011-11-14  5:05 UTC (permalink / raw)
  To: 'Eugeni Dodonov'; +Cc: intel-gfx


[-- Attachment #1.1: Type: text/plain, Size: 747 bytes --]

Thanks for the review. Fixed that empty #ifdef block, missed one line code
there to free glamor screen.

 

 

From: eugeni.dodonov@gmail.com [mailto:eugeni.dodonov@gmail.com] On Behalf
Of Eugeni Dodonov
Sent: Friday, November 11, 2011 8:59 PM
To: Zhigang Gong
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH 2/3] glamor: turn on glamor.

 

 

On Fri, Nov 11, 2011 at 06:31, Zhigang Gong <zhigang.gong@linux.intel.com>
wrote:

@@ -1109,7 +1127,8 @@ static void I830FreeScreen(int scrnIndex, int flags)
 {
       ScrnInfoPtr scrn = xf86Screens[scrnIndex];
       intel_screen_private *intel = intel_get_screen_private(scrn);
-
+#ifdef GLAMOR
+#endif





Empty #ifdef block?

-- 
Eugeni Dodonov <http://eugeni.dodonov.net/> 



[-- Attachment #1.2: Type: text/html, Size: 3930 bytes --]

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

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 2/3] glamor: turn on glamor.
  2011-11-14  5:01         ` Zhigang Gong
@ 2011-11-14  9:07           ` Chris Wilson
  2011-11-14 12:02             ` Zhigang Gong
  0 siblings, 1 reply; 16+ messages in thread
From: Chris Wilson @ 2011-11-14  9:07 UTC (permalink / raw)
  To: Zhigang Gong, intel-gfx

On Mon, 14 Nov 2011 13:01:36 +0800, "Zhigang Gong" <zhigang.gong@linux.intel.com> wrote:
> 
> 
> > -----Original Message-----
> > From: Chris Wilson [mailto:chris@chris-wilson.co.uk]
> > Sent: Friday, November 11, 2011 9:13 PM
> > To: Zhigang Gong; intel-gfx@lists.freedesktop.org
> > Subject: RE: [Intel-gfx] [PATCH 2/3] glamor: turn on glamor.
> > 
> > On Fri, 11 Nov 2011 18:52:11 +0800, "Zhigang Gong"
> > <zhigang.gong@linux.intel.com> wrote:
> > >
> > >
> > > > -----Original Message-----
> > > > From: Chris Wilson [mailto:chris@chris-wilson.co.uk]
> > > > Sent: Friday, November 11, 2011 5:12 PM
> > > > To: Zhigang Gong; intel-gfx@lists.freedesktop.org
> > > > Subject: Re: [Intel-gfx] [PATCH 2/3] glamor: turn on glamor.
> > > >
> > > > On Fri, 11 Nov 2011 16:31:20 +0800, Zhigang Gong
> > > > <zhigang.gong@linux.intel.com> wrote:
> > > > > @@ -965,6 +969,9 @@ void
> > > > intel_uxa_block_handler(intel_screen_private *intel)
> > > > >  	 * framebuffer until significantly later.
> > > > >  	 */
> > > > >  	intel_flush_rendering(intel);
> > > > > +#ifdef GLAMOR
> > > > > +	intel_glamor_block_handler(intel);
> > > > > +#endif
> > > > >  }
> > > >
> > > > I suspect this is the wrong way around as we are not flushing the
> > > > render cache of glamor's rendering to the scanout until the next block
> > handler.
> > > I don't understand here. Would you please explain more detail? Thanks.
> > 
> > Whenever we render, the data ends up in the Render Cache and needs to
> > be flushed out to memory before it is coherent with the CPU or in this
> > case the Display Engine (i.e. scanout).
> > 
> > intel_flush_rendering() does two tasks. The first is to submit any pending
> > batch, and the second is to flush the Render Cache so that the
> > modifications land on the scanout in a timely manner. It is probably best
> if
> > those two tasks were separated so that we do:
> > 
> >   intel_uxa_block_handler(intel); // flush the UXA batch
> >   intel_glamor_block_handler(intel); // flush the GL batch
> >   intel_flush_rendering(intel); // flush the RenderCache to scanout
> > 
> > However, you can simply rearrange the code and achieve it with the
> > existing functions:
> > 
> >   intel_glamor_block_handler(intel); // mark the front bo as dirty as
> > needbe
> >   intel_flush_rendering(intel); // flush UXA batch along with RenderCache
> Thanks for the explanation here. But I still don't think the original code
> is wrong
> regard to this cache flushing issue. Here is my analysis:
> intel_glamor_block_handler calls to glFlush(), and glFlush is similar with
> the 
> intel_flush_rendering, it calls intel_flush to flush the batch buffers and
> then
> call intel_flush_frontbuffer to flush the frontbuffer which flushes the scan
> out
> buffer. So when the screen pixmap is accessed by glamor, and after we call
> intel_glamor_block_handler, the Display Engine should see the correct data
> 
> Right?

No.

glFlush() does call intel_flush_front(). However that in turn calls
dri2->flushFrontBuffer which is implemented for EGL with

static void
dri2_flush_front_buffer(__DRIdrawable * driDrawable, void *loaderPrivate)
{
   /* FIXME: Does EGL support front buffer rendering at all? */
}

Neither does it perform the intended action via GLX (except that
flushing the scanout is handled by the DDX as a normal part of its
operation).
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 2/3] glamor: turn on glamor.
  2011-11-14  9:07           ` Chris Wilson
@ 2011-11-14 12:02             ` Zhigang Gong
  0 siblings, 0 replies; 16+ messages in thread
From: Zhigang Gong @ 2011-11-14 12:02 UTC (permalink / raw)
  To: 'Chris Wilson', intel-gfx

> -----Original Message-----
> From: Chris Wilson [mailto:chris@chris-wilson.co.uk]
> Sent: Monday, November 14, 2011 5:07 PM
> To: Zhigang Gong; intel-gfx@lists.freedesktop.org
> Subject: RE: [Intel-gfx] [PATCH 2/3] glamor: turn on glamor.
> 
> On Mon, 14 Nov 2011 13:01:36 +0800, "Zhigang Gong"
> <zhigang.gong@linux.intel.com> wrote:
> >
> >
> > > -----Original Message-----
> > > From: Chris Wilson [mailto:chris@chris-wilson.co.uk]
> > > Sent: Friday, November 11, 2011 9:13 PM
> > > To: Zhigang Gong; intel-gfx@lists.freedesktop.org
> > > Subject: RE: [Intel-gfx] [PATCH 2/3] glamor: turn on glamor.
> > >
> > > On Fri, 11 Nov 2011 18:52:11 +0800, "Zhigang Gong"
> > > <zhigang.gong@linux.intel.com> wrote:
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Chris Wilson [mailto:chris@chris-wilson.co.uk]
> > > > > Sent: Friday, November 11, 2011 5:12 PM
> > > > > To: Zhigang Gong; intel-gfx@lists.freedesktop.org
> > > > > Subject: Re: [Intel-gfx] [PATCH 2/3] glamor: turn on glamor.
> > > > >
> > > > > On Fri, 11 Nov 2011 16:31:20 +0800, Zhigang Gong
> > > > > <zhigang.gong@linux.intel.com> wrote:
> > > > > > @@ -965,6 +969,9 @@ void
> > > > > intel_uxa_block_handler(intel_screen_private *intel)
> > > > > >  	 * framebuffer until significantly later.
> > > > > >  	 */
> > > > > >  	intel_flush_rendering(intel);
> > > > > > +#ifdef GLAMOR
> > > > > > +	intel_glamor_block_handler(intel);
> > > > > > +#endif
> > > > > >  }
> > > > >
> > > > > I suspect this is the wrong way around as we are not flushing
> > > > > the render cache of glamor's rendering to the scanout until the
> > > > > next block
> > > handler.
> > > > I don't understand here. Would you please explain more detail?
> Thanks.
> > >
> > > Whenever we render, the data ends up in the Render Cache and
> needs
> > > to be flushed out to memory before it is coherent with the CPU or in
> > > this case the Display Engine (i.e. scanout).
> > >
> > > intel_flush_rendering() does two tasks. The first is to submit any
> > > pending batch, and the second is to flush the Render Cache so that
> > > the modifications land on the scanout in a timely manner. It is
> > > probably best
> > if
> > > those two tasks were separated so that we do:
> > >
> > >   intel_uxa_block_handler(intel); // flush the UXA batch
> > >   intel_glamor_block_handler(intel); // flush the GL batch
> > >   intel_flush_rendering(intel); // flush the RenderCache to scanout
> > >
> > > However, you can simply rearrange the code and achieve it with the
> > > existing functions:
> > >
> > >   intel_glamor_block_handler(intel); // mark the front bo as dirty
> > > as needbe
> > >   intel_flush_rendering(intel); // flush UXA batch along with
> > > RenderCache
> > Thanks for the explanation here. But I still don't think the original
> > code is wrong regard to this cache flushing issue. Here is my
> > analysis:
> > intel_glamor_block_handler calls to glFlush(), and glFlush is similar
> > with the intel_flush_rendering, it calls intel_flush to flush the
> > batch buffers and then call intel_flush_frontbuffer to flush the
> > frontbuffer which flushes the scan out buffer. So when the screen
> > pixmap is accessed by glamor, and after we call
> > intel_glamor_block_handler, the Display Engine should see the correct
> > data
> >
> > Right?
> 
> No.
> 
> glFlush() does call intel_flush_front(). However that in turn calls
> dri2->flushFrontBuffer which is implemented for EGL with
> 
> static void
> dri2_flush_front_buffer(__DRIdrawable * driDrawable, void
> *loaderPrivate) {
>    /* FIXME: Does EGL support front buffer rendering at all? */ }
> 
> Neither does it perform the intended action via GLX (except that flushing
> the scanout is handled by the DDX as a normal part of its operation).

You are right. EGL layer will not do a really front buffer flushing. We have
to
let it be done in DDX layer. In my version 2 patch set, I already rearrange
the
code sequence as you suggested please review it again. The remaining work
for this issue is that I need to add new code to set the needs_flush
according
to the access type of glamor. Will do that soon.

Thanks. 

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2011-11-14 12:02 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-11-11  8:31 [PATCH 1/3] glamor: Initial commit to introduce glamor acceleration Zhigang Gong
2011-11-11  8:31 ` [PATCH 2/3] glamor: turn on glamor Zhigang Gong
2011-11-11  9:11   ` Chris Wilson
2011-11-11 10:52     ` Zhigang Gong
2011-11-11 13:12       ` Chris Wilson
2011-11-14  5:01         ` Zhigang Gong
2011-11-14  9:07           ` Chris Wilson
2011-11-14 12:02             ` Zhigang Gong
2011-11-11 12:58   ` Eugeni Dodonov
2011-11-14  5:05     ` Zhigang Gong
2011-11-11  8:31 ` [PATCH 3/3] glamor: Route fillspans and polyfillrects to glamor Zhigang Gong
2011-11-11  9:07   ` Chris Wilson
2011-11-11 10:48     ` Zhigang Gong
2011-11-11 13:54       ` Chris Wilson
2011-11-14  3:32         ` Zhigang Gong
2011-11-11 12:56 ` [PATCH 1/3] glamor: Initial commit to introduce glamor acceleration Eugeni Dodonov

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox