Re: [PATCH 07/10] glamor: Add glamor_program PolyPoint implementation
Markus Wick writes: > I've seen a 40% performance boost in x11perf -dot Yup, it's amazing how much time Mesa takes flipping large chunks of state around like this. -- keith.pack...@intel.com pgpXyBRZMhdeK.pgp Description: PGP signature ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH 07/10] glamor: Add glamor_program PolyPoint implementation
Am 2014-03-20 23:35, schrieb Keith Packard: We are used to also disable the gl program here unnecessaryly. I want to strip them all, so I'm fine without it, but it isn't fair for a performance comparison. I stripped them out in an earlier patch; we could measure performance at that point? I've seen a 40% performance boost in x11perf -dot ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH 07/10] glamor: Add glamor_program PolyPoint implementation
Markus Wick writes:
>> +glamor_get_context(glamor_priv);
>> +
>> +if (prog->failed) {
>> +glamor_put_context(glamor_priv);
>
> Why don't you check this issue before calling glamor_get_context?
Could do, but compiles "shouldn't" fail?
> What do you think about a small API change for glamor_build_program?
> imo it would be cleaner to just call glamor_build_program everytime and
> do the fail/successful and already build checks there. Here we don't
> care about everything else but true/false.
Yeah, that sounds like a nice cleanup. A bit at odds with your previous
comment though :-)
> How often does this loop iterate typically?
Once is the most common. It might be nice to capture some statistics
From applications and see if they're using more complicated clip lists
these days though.
> Maybe it's worth to think about a way to also upload all boxes.
Depends if we think drawing with more than one clip box is interesting;
all of my data (from years ago, alas) said that applications drew almost
everything off-screen to a pixmap and then copied to the screen, so
rendering was always done with one rect, but blts sometimes had a few more.
> I guess box must be reseted in the pixmap_loop.
Yeah, I don't remember if there was a bug in this version, but the newer
one makes it clear that both nbox and box are reset for each loop.
> We are used to also disable the gl program here unnecessaryly.
> I want to strip them all, so I'm fine without it, but it isn't fair for
> a performance comparison.
I stripped them out in an earlier patch; we could measure performance at
that point?
--
keith.pack...@intel.com
pgptegHe_em2K.pgp
Description: PGP signature
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH 07/10] glamor: Add glamor_program PolyPoint implementation
Am 2014-03-14 07:30, schrieb Keith Packard:
diff --git a/glamor/glamor_polyops.c b/glamor/glamor_polyops.c
index 1484d80..eac1688 100644
--- a/glamor/glamor_polyops.c
+++ b/glamor/glamor_polyops.c
@@ -27,17 +27,93 @@
*/
#include "glamor_priv.h"
+#include "glamor_transform.h"
+
+static const glamor_facet glamor_facet_point = {
+.name = "poly_point",
+.vs_vars = "attribute vec2 primitive;\n",
+.vs_exec = GLAMOR_POS(gl_Position, primitive),
+};
static Bool
-_glamor_poly_point(DrawablePtr pDrawable, GCPtr pGC, int mode, int
npt,
+_glamor_poly_point(DrawablePtr drawable, GCPtr gc, int mode, int npt,
DDXPointPtr ppt, Bool fallback)
{
-if (!fallback && glamor_ddx_fallback_check_gc(pGC)
-&& glamor_ddx_fallback_check_pixmap(pDrawable))
+ScreenPtr screen = drawable->pScreen;
+glamor_screen_private *glamor_priv =
glamor_get_screen_private(screen);
+PixmapPtr pixmap = glamor_get_drawable_pixmap(drawable);
+glamor_program *prog = &glamor_priv->point_prog;
+glamor_pixmap_private *pixmap_priv;
+int nbox = RegionNumRects(gc->pCompositeClip);
+BoxPtr box = RegionRects(gc->pCompositeClip);
+int off_x, off_y;
+GLshort *vbo_ppt;
+char *vbo_offset;
+int box_x, box_y;
+
+if (!fallback && glamor_ddx_fallback_check_gc(gc)
+&& glamor_ddx_fallback_check_pixmap(drawable))
return FALSE;
-miPolyPoint(pDrawable, pGC, mode, npt, ppt);
+pixmap_priv = glamor_get_pixmap_private(pixmap);
+if (!GLAMOR_PIXMAP_PRIV_HAS_FBO(pixmap_priv))
+goto bail;
+
+glamor_get_context(glamor_priv);
+
+if (prog->failed) {
+glamor_put_context(glamor_priv);
Why don't you check this issue before calling glamor_get_context?
+goto bail;
+}
+
+if (!prog->prog) {
+if (!glamor_build_program(screen, prog,
+ &glamor_facet_point,
+ &glamor_fill_solid))
What do you think about a small API change for glamor_build_program?
imo it would be cleaner to just call glamor_build_program everytime and
do the fail/successful and already build checks there. Here we don't
care about everything else but true/false.
+{
+glamor_put_context(glamor_priv);
+goto bail;
+}
+}
+
+if (!glamor_use_program(pixmap, gc, prog)) {
+glamor_put_context(glamor_priv);
+goto bail;
+}
+
+vbo_ppt = glamor_get_vbo_space(screen, npt * (2 * sizeof
(INT16)), &vbo_offset);
+glEnableVertexAttribArray(GLAMOR_VERTEX_POS);
+glVertexAttribPointer(GLAMOR_VERTEX_POS, 2, GL_SHORT, GL_FALSE,
0, vbo_offset);
+memcpy(vbo_ppt, ppt, npt * (2 * sizeof (INT16)));
+glamor_put_vbo_space(screen);
+
+glEnable(GL_SCISSOR_TEST);
+
+glamor_pixmap_loop(pixmap_priv, box_x, box_y) {
+glamor_set_destination_drawable(drawable, box_x, box_y, TRUE,
prog->matrix_uniform, &off_x, &off_y);
+
+while (nbox--) {
How often does this loop iterate typically?
Maybe it's worth to think about a way to also upload all boxes.
+glScissor(box->x1 + off_x,
+ box->y1 + off_y,
+ box->x2 - box->x1,
+ box->y2 - box->y1);
+box++;
I guess box must be reseted in the pixmap_loop.
+glDrawArrays(GL_POINTS, 0, npt);
+}
+}
+glDisable(GL_SCISSOR_TEST);
+glDisableVertexAttribArray(GLAMOR_VERTEX_POS);
We are used to also disable the gl program here unnecessaryly.
I want to strip them all, so I'm fine without it, but it isn't fair for
a performance comparison.
+
+glamor_put_context(glamor_priv);
+
+glamor_priv->state = RENDER_STATE;
+glamor_priv->render_idle_cnt = 0;
+
+return TRUE;
+
+bail:
+miPolyPoint(drawable, gc, mode, npt, ppt);
return TRUE;
}
@@ -55,6 +131,14 @@ glamor_poly_point_nf(DrawablePtr pDrawable, GCPtr
pGC, int mode, int npt,
return _glamor_poly_point(pDrawable, pGC, mode, npt, ppt, FALSE);
}
+void
+glamor_fini_point_shader(ScreenPtr screen)
+{
+glamor_screen_private *glamor_priv =
glamor_get_screen_private(screen);
+
+glamor_delete_program(screen, &glamor_priv->point_prog);
+}
+
static Bool
_glamor_poly_segment(DrawablePtr pDrawable, GCPtr pGC, int nseg,
xSegment *pSeg, Bool fallback)
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel
[PATCH 07/10] glamor: Add glamor_program PolyPoint implementation
This accelerates poly point when possible by off-loading all geometry
computation to the GPU.
Signed-off-by: Keith Packard
---
glamor/glamor.c | 1 +
glamor/glamor_polyops.c | 92 ++---
glamor/glamor_priv.h| 5 +++
3 files changed, 94 insertions(+), 4 deletions(-)
diff --git a/glamor/glamor.c b/glamor/glamor.c
index c5b6e8c..86b0ba5 100644
--- a/glamor/glamor.c
+++ b/glamor/glamor.c
@@ -550,6 +550,7 @@ glamor_release_screen_priv(ScreenPtr screen)
glamor_fini_vbo(screen);
glamor_fini_pixmap_fbo(screen);
glamor_fini_solid_shader(screen);
+glamor_fini_point_shader(screen);
glamor_fini_tile_shader(screen);
#ifdef GLAMOR_TRAPEZOID_SHADER
glamor_fini_trapezoid_shader(screen);
diff --git a/glamor/glamor_polyops.c b/glamor/glamor_polyops.c
index 1484d80..eac1688 100644
--- a/glamor/glamor_polyops.c
+++ b/glamor/glamor_polyops.c
@@ -27,17 +27,93 @@
*/
#include "glamor_priv.h"
+#include "glamor_transform.h"
+
+static const glamor_facet glamor_facet_point = {
+.name = "poly_point",
+.vs_vars = "attribute vec2 primitive;\n",
+.vs_exec = GLAMOR_POS(gl_Position, primitive),
+};
static Bool
-_glamor_poly_point(DrawablePtr pDrawable, GCPtr pGC, int mode, int npt,
+_glamor_poly_point(DrawablePtr drawable, GCPtr gc, int mode, int npt,
DDXPointPtr ppt, Bool fallback)
{
-if (!fallback && glamor_ddx_fallback_check_gc(pGC)
-&& glamor_ddx_fallback_check_pixmap(pDrawable))
+ScreenPtr screen = drawable->pScreen;
+glamor_screen_private *glamor_priv = glamor_get_screen_private(screen);
+PixmapPtr pixmap = glamor_get_drawable_pixmap(drawable);
+glamor_program *prog = &glamor_priv->point_prog;
+glamor_pixmap_private *pixmap_priv;
+int nbox = RegionNumRects(gc->pCompositeClip);
+BoxPtr box = RegionRects(gc->pCompositeClip);
+int off_x, off_y;
+GLshort *vbo_ppt;
+char *vbo_offset;
+int box_x, box_y;
+
+if (!fallback && glamor_ddx_fallback_check_gc(gc)
+&& glamor_ddx_fallback_check_pixmap(drawable))
return FALSE;
-miPolyPoint(pDrawable, pGC, mode, npt, ppt);
+pixmap_priv = glamor_get_pixmap_private(pixmap);
+if (!GLAMOR_PIXMAP_PRIV_HAS_FBO(pixmap_priv))
+goto bail;
+
+glamor_get_context(glamor_priv);
+
+if (prog->failed) {
+glamor_put_context(glamor_priv);
+goto bail;
+}
+
+if (!prog->prog) {
+if (!glamor_build_program(screen, prog,
+ &glamor_facet_point,
+ &glamor_fill_solid))
+{
+glamor_put_context(glamor_priv);
+goto bail;
+}
+}
+
+if (!glamor_use_program(pixmap, gc, prog)) {
+glamor_put_context(glamor_priv);
+goto bail;
+}
+
+vbo_ppt = glamor_get_vbo_space(screen, npt * (2 * sizeof (INT16)),
&vbo_offset);
+glEnableVertexAttribArray(GLAMOR_VERTEX_POS);
+glVertexAttribPointer(GLAMOR_VERTEX_POS, 2, GL_SHORT, GL_FALSE, 0,
vbo_offset);
+memcpy(vbo_ppt, ppt, npt * (2 * sizeof (INT16)));
+glamor_put_vbo_space(screen);
+
+glEnable(GL_SCISSOR_TEST);
+
+glamor_pixmap_loop(pixmap_priv, box_x, box_y) {
+glamor_set_destination_drawable(drawable, box_x, box_y, TRUE,
prog->matrix_uniform, &off_x, &off_y);
+
+while (nbox--) {
+glScissor(box->x1 + off_x,
+ box->y1 + off_y,
+ box->x2 - box->x1,
+ box->y2 - box->y1);
+box++;
+glDrawArrays(GL_POINTS, 0, npt);
+}
+}
+glDisable(GL_SCISSOR_TEST);
+glDisableVertexAttribArray(GLAMOR_VERTEX_POS);
+
+glamor_put_context(glamor_priv);
+
+glamor_priv->state = RENDER_STATE;
+glamor_priv->render_idle_cnt = 0;
+
+return TRUE;
+
+bail:
+miPolyPoint(drawable, gc, mode, npt, ppt);
return TRUE;
}
@@ -55,6 +131,14 @@ glamor_poly_point_nf(DrawablePtr pDrawable, GCPtr pGC, int
mode, int npt,
return _glamor_poly_point(pDrawable, pGC, mode, npt, ppt, FALSE);
}
+void
+glamor_fini_point_shader(ScreenPtr screen)
+{
+glamor_screen_private *glamor_priv = glamor_get_screen_private(screen);
+
+glamor_delete_program(screen, &glamor_priv->point_prog);
+}
+
static Bool
_glamor_poly_segment(DrawablePtr pDrawable, GCPtr pGC, int nseg,
xSegment *pSeg, Bool fallback)
diff --git a/glamor/glamor_priv.h b/glamor/glamor_priv.h
index 766c7d9..b0ab4e1 100644
--- a/glamor/glamor_priv.h
+++ b/glamor/glamor_priv.h
@@ -226,6 +226,9 @@ typedef struct glamor_screen_private {
GLint solid_prog;
GLint solid_color_uniform_location;
+/* glamor point shader */
+glamor_program point_prog;
+
/* vertext/elment_index buffer object for render */
GLuint vbo, ebo;
/** Next offset within the VBO that glamor_get_vbo_space() will use. */
@@ -992,6 +995,8 @@ void glamor_push_pixels(GCPtr pGC, P
