Re: [PATCH xserver 2/2] glamor: Always purge the FBO cache in BlockHandler
On 03/06/2017 07:02 PM, Eric Anholt wrote: > For re-adding an FBO cache, I would need to see that we have > APPLE_object_purgeable used so that the FBO cache can be evicted, [...] When I checked a few months ago, only the i915 Mesa and kernel drivers implemented this. Maybe we can enable the FBO cache *iff* the driver supports this extension? Max ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH xserver 1/2] Revert "glamor: Remove the FBO cache."
On 03/03/2017 09:46 AM, Michel Dänzer wrote: > This reverts commit 950ffb8d6fd1480f305e38c571bda44f247f1de2 (and parts > of commit 4b5326aeba539249fcded91bf7806a708eeca651). Oh no. Glamor's BO cache was horribly broken, and I tried to fix it before: https://lists.x.org/archives/xorg-devel/2016-July/050403.html It was then decided to drop it altogether. The thing is, at least on the Raspberry Pi (vc4) we get 3 BO caches: - kernel - Mesa - GLAMOR That's quite a lot of memory kept around "just in case". If you insist on bringing it back, please have a thorough look at my old series. There was a lot of broken, as well as dead (!) code that I had to fix to make this workable on VC4. If we reintroduce a FBO cache, at least let's do it right this time. Max ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH xserver] glamor: Declare "pos" in glamor_dash.c
On 02/23/2017 07:59 PM, Adam Jackson wrote: > I pushed essentially this version of the patch (pos not gl_Position), > having not read far enough down in the mailbox to see this version. > (And, Dieter's right, picking on the little shit really is > counterproductive for one-liners.) > > That said, gl_Position is probably right. But I think dashing is broken > already, so at least now we're just rendering wrong instead of > crashing. If someone wants to get rendering correct too, please do... Uh-oh - for me, this patch actually introduces another possibility to crash Xorg. I've compiled Xephyr from Git d8161aeb50891ae10c5656487ce8f982deed5f9f and then I ran the reproducer from this bug: https://bugs.freedesktop.org/show_bug.cgi?id=99708 The bug is totally unrelated AFAICT, but still: A simple program that barely draws a dashed line makes Xephyr crash. Can we revert the commit for the time being? Given that we had a discussion about the correctness of the GLAMOR line code in the bugs https://bugs.freedesktop.org/show_bug.cgi?id=99705 https://bugs.freedesktop.org/show_bug.cgi?id=99708 maybe we should reconsider switching to software rendering for now, at least as the default setting of an xorg.conf option... Max ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH xserver] glamor: Paint first and last pixel of lines
On 02/14/2017 12:10 AM, Adam Jackson wrote: > fb has been the only extant renderer for nearly nine years. Clearly > there exists code that expects it. I'm not convinced that pushing back > on that expectation is a good move, technically correct though it may > be. I'll second that. So shall we use the fallback code for now, until the wrinkles are ironed out of GLAMOR? Here's something else to consider: With hardware specific 2D acceleration on the way out, clients mostly pushing huge images (GTK, Qt, ...), and thus Wayland around the corner, I see the Xorg server mvoe away from its original role as a driver layer. Instead, it's becoming a pure 2D rendering framework for the apps that still use raw X11 commands. These programs expect it to work precisely and reproducibly. Thus, how about focusing on doing precise 2D rendering - it is the reference X server after all - and making sure that everything behaves like mi? I'd even leave the existing GL lines code intact, as it's really cool to have. But disabled by default, so users get the precise experience first, and can decide to switch to a quick-but-imprecise OpenGL based experience if they wish to do so. Max ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH xserver] glamor: Paint first and last pixel of lines
On 02/13/2017 11:53 PM, Keith Packard wrote: > afaict, GL suggests 'not > last' as the only option. It sounds like some drivers are doing both > 'not last' and 'not first' though? In the case that I reproduced, the 'not first' only happens occasionally, but reproducibly. Max ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH xserver] glamor: Paint first and last pixel of lines
On 02/08/2017 06:30 PM, Adam Jackson wrote: > I think, practically speaking, that glamor needs to match fb even along > the corner cases where the X spec allows driver-dependent behavior. The > simplest way to do this for this case would be to fall down to the mi > line code for the case of non-trivial cap style and rop that reads the > destination; if done correctly this will still be "accelerated" in that > we'll still hit glamor's SetSpans path. Okay, so do you think we can take the patch for the GXcopy case, and fall back to mi otherwise? And how about https://bugs.freedesktop.org/show_bug.cgi?id=99708 - should we fall back to mi until we have a better approximation in GLAMOR? It's sad to see acceleration go, but currently it seems to create a lot of confusion. And raw X11 ops are barely used (if at all) in average modern desktop environments. Max ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH xserver] glamor: Paint first and last pixel of lines
On 02/07/2017 09:06 PM, Keith Packard wrote: > Max Staudt <msta...@suse.de> writes: > >> OpenGL implementations are allowed to be imprecise in drawing line caps. >> This patch expands on the original workaround in dc9fa908. > > Yeah, finding a way to work around GL differences seems like a good > idea. In this case, however, I think you're fixing some drivers and > breaking others -- when drawing with non-idempotent raster > ops. > > Idempotent raster ops are those for which multiple draws generate the > same result, like GXcopy; hence non-idempotent operations are those > which do not, such as GXxor. What does this mean in practice - is the manual pixel painting the problem you're seeing here, because it may draw over the previously drawn lines? How does a GXor line draw operation currently work in GLAMOR? Maybe it would be easier to limit acceleration to GXcopy (which I suppose is by far the most useful case) and do that case as correct as possible? > I'm not sure what we should do here; there's no requirement in the > protocol that we do anything at all as the server is allowed to draw > pretty much whatever it wants for zero-width lines. The X11 client developer who filed the openSUSE bug https://bugzilla.opensuse.org/show_bug.cgi?id=1021803 is confused because the software fallback and the GLAMOR version produce different results. IMHO, since the Xorg server's software fallback tends to be regarded as the reference X11 implementation, we should try to emulate that as closely as possible. I have to agree that this is a funny situation. Neither X11 nor GL are rigidly specified, yet we're connecting them :) > If you've found specific problems with Mesa drivers, I'd suggest we > actually go fix those instead of working around them in the X > server. The GL spec seems pretty clear in what it wants, and I think > that aligns with what the X server currently expects. I'm not sure, do you think this is a problem in the OpenGL drivers used? It seems within the GL spec, so I'd say Mesa is not the right thing to fix. Given that there are plenty of binary drivers, and that we may end up doing GLAMOR in an Xwayland session, I think we should aim for GLAMOR being as precise as the fallback. Or disable the acceleration (we could make it a configuration option). Most modern clients just display images anyway, and the few remaining ones probably expect rather precise output from Xorg as a rasterizer. Consistency is important, as anything else can severely confuse users and debugging developers alike - I think I've once seen that last line pixel (which you use to draw the cap) being drawn on some backend. Now that's a weird graphics glitch! And we can avoid it by setting the cap pixel manually. Though as you say, only in case of GXcopy. Max ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
[PATCH xserver] glamor: Paint first and last pixel of lines
OpenGL implementations are allowed to be imprecise in drawing line caps. This patch expands on the original workaround in dc9fa908. Fixes: https://bugs.freedesktop.org/show_bug.cgi?id=99705 Signed-off-by: Max Staudt <msta...@suse.de> --- glamor/glamor_lines.c | 21 + glamor/glamor_segs.c | 43 --- 2 files changed, 33 insertions(+), 31 deletions(-) diff --git a/glamor/glamor_lines.c b/glamor/glamor_lines.c index ca3cccf..47f8ed2 100644 --- a/glamor/glamor_lines.c +++ b/glamor/glamor_lines.c @@ -45,7 +45,6 @@ glamor_poly_lines_gl(DrawablePtr drawable, GCPtr gc, DDXPointPtr v; char *vbo_offset; int box_x, box_y; -int add_last; pixmap_priv = glamor_get_pixmap_private(pixmap); if (!GLAMOR_PIXMAP_PRIV_HAS_FBO(pixmap_priv)) @@ -57,10 +56,6 @@ glamor_poly_lines_gl(DrawablePtr drawable, GCPtr gc, if (gc->lineStyle != LineSolid) goto bail; -add_last = 0; -if (gc->capStyle != CapNotLast) -add_last = 1; - if (n < 2) return TRUE; @@ -76,7 +71,7 @@ glamor_poly_lines_gl(DrawablePtr drawable, GCPtr gc, /* Set up the vertex buffers for the points */ v = glamor_get_vbo_space(drawable->pScreen, - (n + add_last) * sizeof (DDXPointRec), + n * sizeof (DDXPointRec), _offset); glEnableVertexAttribArray(GLAMOR_VERTEX_POS); @@ -96,11 +91,6 @@ glamor_poly_lines_gl(DrawablePtr drawable, GCPtr gc, memcpy(v, points, n * sizeof (DDXPointRec)); } -if (add_last) { -v[n].x = v[n-1].x + 1; -v[n].y = v[n-1].y; -} - glamor_put_vbo_space(screen); glEnable(GL_SCISSOR_TEST); @@ -118,7 +108,14 @@ glamor_poly_lines_gl(DrawablePtr drawable, GCPtr gc, box->x2 - box->x1, box->y2 - box->y1); box++; -glDrawArrays(GL_LINE_STRIP, 0, n + add_last); +glDrawArrays(GL_LINE_STRIP, 0, n); + +/* Draw the first and last pixels, as implementations are + * allowed to be inaccurate there. */ +if (gc->capStyle == CapNotLast) +glDrawArrays(GL_POINTS, 0, n-1); +else +glDrawArrays(GL_POINTS, 0, n); } } diff --git a/glamor/glamor_segs.c b/glamor/glamor_segs.c index 0168d05..b571b10 100644 --- a/glamor/glamor_segs.c +++ b/glamor/glamor_segs.c @@ -45,7 +45,6 @@ glamor_poly_segment_gl(DrawablePtr drawable, GCPtr gc, xSegment *v; char *vbo_offset; int box_x, box_y; -int add_last; pixmap_priv = glamor_get_pixmap_private(pixmap); if (!GLAMOR_PIXMAP_PRIV_HAS_FBO(pixmap_priv)) @@ -57,10 +56,6 @@ glamor_poly_segment_gl(DrawablePtr drawable, GCPtr gc, if (gc->lineStyle != LineSolid) goto bail; -add_last = 0; -if (gc->capStyle != CapNotLast) -add_last = 1; - glamor_make_current(glamor_priv); prog = glamor_use_program_fill(pixmap, gc, @@ -71,27 +66,25 @@ glamor_poly_segment_gl(DrawablePtr drawable, GCPtr gc, goto bail_ctx; /* Set up the vertex buffers for the points */ - +/* We need 1.5 times the space in case of CapNotLast, + * so let's just always allocate 2 times the space. */ v = glamor_get_vbo_space(drawable->pScreen, - (nseg << add_last) * sizeof (xSegment), + nseg * sizeof (xSegment) * 2, _offset); glEnableVertexAttribArray(GLAMOR_VERTEX_POS); glVertexAttribPointer(GLAMOR_VERTEX_POS, 2, GL_SHORT, GL_FALSE, sizeof(DDXPointRec), vbo_offset); -if (add_last) { -int i, j; -for (i = 0, j=0; i < nseg; i++) { -v[j++] = segs[i]; -v[j].x1 = segs[i].x2; -v[j].y1 = segs[i].y2; -v[j].x2 = segs[i].x2+1; -v[j].y2 = segs[i].y2; -j++; +memcpy(v, segs, nseg * sizeof (xSegment)); + +/* In case of CapNotLast, copy the line starting points for later */ +if (gc->capStyle != CapNotLast) { +int i; +for (i = 0; i < nseg; i++) { +v[2*nseg + i] = v[2*i]; } -} else -memcpy(v, segs, nseg * sizeof (xSegment)); +} glamor_put_vbo_space(screen); @@ -110,7 +103,19 @@ glamor_poly_segment_gl(DrawablePtr drawable, GCPtr gc, box->x2 - box->x1, box->y2 - box->y1); box++; -glDrawArrays(GL_LINES, 0, nseg << (1 + add_last)); +glDrawArrays(GL_LINES, 0, nseg * 2); + +/* Draw the first and last pixel of each line, as OpenGL + * implementations are allowed to be inaccurate there. + * Since we're drawing all line ends, we can just reuse + * the array
Re: [PATCH xserver 0/4] glamor: Bugfixes in memory management
On 07/18/2016 09:24 PM, Adam Jackson wrote: > On Mon, 2016-07-18 at 20:38 +0200, Max Staudt wrote: > >> What happened to this patch? I cannot find any discussion on it on >> [xorg-devel]. > > You have correctly identified what happened to this patch: it was sent, > there was no discussion, nothing further happened until now. > > Fortunately it's an easy rebase to master, so, merged: > > remote: E: failed to find patch for rev > 950ffb8d6fd1480f305e38c571bda44f247f1de2. > remote: I: 0 patch(es) updated to state Accepted. > To ssh://git.freedesktop.org/git/xorg/xserver >e8e3675..950ffb8 master -> master > > Patchwork number was 64074 originally, for the record. Great, thank you! Max ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH xserver 0/4] glamor: Bugfixes in memory management
On 07/18/2016 07:10 PM, Eric Anholt wrote: > Adam Jackson <a...@nwnk.net> writes: > >> On Mon, 2016-07-18 at 14:36 +0200, Max Staudt wrote: >>> Shall I send a patch to delete GLAMOR's BO cache instead? >> >> Yes please, that would be lovely. > > https://lists.freedesktop.org/archives/xorg-devel/2015-November/047776.html Oh, so this topic has come up before. What happened to this patch? I cannot find any discussion on it on [xorg-devel]. ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH xserver 0/4] glamor: Bugfixes in memory management
On 07/15/2016 09:00 PM, Eric Anholt wrote: > I still want to just delete the FBO cache entirely. We shouldn't hog > all that system memory for a 1.7% performance improvement on cairo-perf. Oh, I was looking into analyzing the performance difference. If it's just that, then I'm for deleting it, too. Shall I send a patch to delete GLAMOR's BO cache instead? Comments? Max ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
[PATCH xserver 4/4] glamor: Call glamor_block_handler() in _glamor_block_handler()
Deduplify code and make sure glamor_priv->tick++ actually happens. This ensures that GLAMOR's BO cache is expunged every now and then even if it's not full yet. Note: glamor_block_handler() contains both lines removed in this change, so the old functionality persists. Signed-off-by: Max Staudt <msta...@suse.de> --- glamor/glamor.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/glamor/glamor.c b/glamor/glamor.c index 9c6a0d1..77bc150 100644 --- a/glamor/glamor.c +++ b/glamor/glamor.c @@ -241,8 +241,7 @@ _glamor_block_handler(ScreenPtr screen, void *timeout, void *readmask) glamor_priv->saved_procs.block_handler = screen->BlockHandler; screen->BlockHandler = _glamor_block_handler; -glamor_make_current(glamor_priv); -glFlush(); +glamor_block_handler(screen); } static void -- 2.6.6 ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
[PATCH xserver 1/4] glamor: Flush BO cache on GL_OUT_OF_MEMORY
If we run out of memory, we have most likely hogged it with our huge cache. DRI drivers flush their caches in OOM situations, so we should do so, too. NOTE: This may still not free the memory we need as the DRI drivers typically have a time delay until they actually release the buffers that GLAMOR just handed back - and thus, until there is a large contiguous area of memory available to allocate the new buffer from. In practice this means that the current allocation will probably still fail, but after two or three seconds the next ones will work. This could be improved upon by using GL_APPLE_object_purgeable, as implemented by the i915/i965 driver. Signed-off-by: Max Staudt <msta...@suse.de> --- glamor/glamor_fbo.c | 36 1 file changed, 28 insertions(+), 8 deletions(-) diff --git a/glamor/glamor_fbo.c b/glamor/glamor_fbo.c index c6ba095..f80c20d 100644 --- a/glamor/glamor_fbo.c +++ b/glamor/glamor_fbo.c @@ -143,6 +143,13 @@ glamor_purge_fbo(glamor_screen_private *glamor_priv, } static void +glamor_fbo_flush_cache(glamor_screen_private *glamor_priv) +{ +glamor_priv->tick += GLAMOR_CACHE_EXPIRE_MAX; +glamor_fbo_expire(glamor_priv); +} + +static void glamor_pixmap_fbo_cache_put(glamor_screen_private *glamor_priv, glamor_pixmap_fbo *fbo) { @@ -157,8 +164,7 @@ glamor_pixmap_fbo_cache_put(glamor_screen_private *glamor_priv, if (fbo->fb == 0 || fbo->external || n_format == -1 || glamor_priv->fbo_cache_watermark >= FBO_CACHE_THRESHOLD) { -glamor_priv->tick += GLAMOR_CACHE_EXPIRE_MAX; -glamor_fbo_expire(glamor_priv); +glamor_fbo_flush_cache(glamor_priv); glamor_purge_fbo(glamor_priv, fbo); return; } @@ -344,18 +350,32 @@ _glamor_create_tex(glamor_screen_private *glamor_priv, glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_SWIZZLE_R, GL_ZERO); glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_SWIZZLE_A, GL_RED); } + +/* Allocate the texture. */ glamor_priv->suppress_gl_out_of_memory_logging = true; glTexImage2D(GL_TEXTURE_2D, 0, format, w, h, 0, format, GL_UNSIGNED_BYTE, NULL); glamor_priv->suppress_gl_out_of_memory_logging = false; if (glGetError() == GL_OUT_OF_MEMORY) { -if (!glamor_priv->logged_any_fbo_allocation_failure) { -LogMessageVerb(X_WARNING, 0, "glamor: Failed to allocate %dx%d " - "FBO due to GL_OUT_OF_MEMORY.\n", w, h); -LogMessageVerb(X_WARNING, 0, - "glamor: Expect reduced performance.\n"); -glamor_priv->logged_any_fbo_allocation_failure = true; +LogMessageVerb(X_WARNING, 0, + "glamor: glTexImage2D() failed, flushing caches and trying again...\n"); +glamor_fbo_flush_cache(glamor_priv); + +/* Retry allocating the texture. */ +glamor_priv->suppress_gl_out_of_memory_logging = true; +glTexImage2D(GL_TEXTURE_2D, 0, format, w, h, 0, + format, GL_UNSIGNED_BYTE, NULL); +glamor_priv->suppress_gl_out_of_memory_logging = false; + +if (glGetError() == GL_OUT_OF_MEMORY) { +if (!glamor_priv->logged_any_fbo_allocation_failure) { +LogMessageVerb(X_WARNING, 0, "glamor: Failed to allocate %dx%d " + "FBO due to GL_OUT_OF_MEMORY.\n", w, h); +LogMessageVerb(X_WARNING, 0, + "glamor: Expect reduced performance.\n"); +glamor_priv->logged_any_fbo_allocation_failure = true; +} } glDeleteTextures(1, ); return 0; -- 2.6.6 ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
[PATCH xserver 2/4] glamor: Reduce BO cache size to 16 MB
With the previous size, we would quickly accumulate >1GB of unused memory. This was because the code didn't take into account: - Pixels usually take 4 bytes these days, not 1 byte. - BOs are allocated larger than the actual width*height used. - Few GPUs have this much VRAM and the cache easily makes them run into GL_OUT_OF_MEMORY. Reproducible by moving large windows outside the visible area and back in. glamor_copy_fbo_fbo_temp() will allocate a large temporary buffer, which is "freed" into the cache. The GPU will quickly run out of VRAM, as every redraw of the window dragging may allocate megabytes of memory. Signed-off-by: Max Staudt <msta...@suse.de> --- glamor/glamor_fbo.c | 9 - 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/glamor/glamor_fbo.c b/glamor/glamor_fbo.c index f80c20d..b27f652 100644 --- a/glamor/glamor_fbo.c +++ b/glamor/glamor_fbo.c @@ -36,7 +36,14 @@ #define GLAMOR_CACHE_EXACT_SIZE 1 //#define NO_FBO_CACHE 1 -#define FBO_CACHE_THRESHOLD (256*1024*1024) + +/* Assume 4 bytes per pixel when calculating cache size. + * Since glamor_pixmap_fbo_cache_put() calculates the size of a BO as + * width*height, we take the pixel size into account here. + * Note that actual BOs are somewhat larger -- we ignore this as an + * approximate cache limit is sufficient. + */ +#define FBO_CACHE_THRESHOLD (16*1024*1024 / 4) /* Loop from the tail to the head. */ #define xorg_list_for_each_entry_reverse(pos, head, member) \ -- 2.6.6 ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
[PATCH xserver 0/4] glamor: Bugfixes in memory management
Low-spec'd platforms such as the Raspberry Pi tend to run out of VRAM since GLAMOR only frees its BO cache once it grows beyond 1 GB. These patches reduce the cache and also flush it more often (based on BO age and GL_OUT_OF_MEMORY). Is there a reason why GLAMOR's cache was set at 1 GB in size? Thanks, Max ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
[PATCH xserver 3/4] glamor: Fix compilation with NO_FBO_CACHE
Signed-off-by: Max Staudt <msta...@suse.de> --- glamor/glamor_fbo.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/glamor/glamor_fbo.c b/glamor/glamor_fbo.c index b27f652..733c3a8 100644 --- a/glamor/glamor_fbo.c +++ b/glamor/glamor_fbo.c @@ -164,7 +164,7 @@ glamor_pixmap_fbo_cache_put(glamor_screen_private *glamor_priv, int n_format; #ifdef NO_FBO_CACHE -glamor_purge_fbo(fbo); +glamor_purge_fbo(glamor_priv, fbo); return; #else n_format = cache_format(fbo->format); -- 2.6.6 ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel