_glamor_upload_bits_to_pixmap_texture currently ignores the stride
parameter, but __glamor_upload_pixmap_to_texture uses 4-byte alignment
via glPixelStorei(GL_UNPACK_ALIGNMENT, 4).
Also fix up the stride argument passed in though, in case it starts
being used properly in the future.
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=87455
Signed-off-by: Michel Dänzer <michel.daenzer@amd.com>
Reviewed-by: Keith Packard <keithp@keithp.com>
Signed-off-by: Keith Packard <keithp@keithp.com>
Only called from glamor_fbo.c now.
Signed-off-by: Michel Dänzer <michel.daenzer@amd.com>
Reviewed-by: Keith Packard <keithp@keithp.com>
Signed-off-by: Keith Packard <keithp@keithp.com>
Calling glamor_purge_fbo directly was incorrect for large pixmaps.
Fixes use-after free with large pixmaps:
==2029== Invalid write of size 8 ~
==2029== at 0x85F93AD: __xorg_list_del (list.h:184)
==2029== by 0x85F93AD: xorg_list_del (list.h:204)
==2029== by 0x85F93AD: glamor_fbo_expire (glamor_fbo.c:280)
==2029== by 0x85F95CA: glamor_pixmap_fbo_cache_put (glamor_fbo.c:159)
==2029== by 0x85D7AB5: glamor_destroy_textured_pixmap (glamor.c:228)
==2029== by 0xC1BDDC4: radeon_glamor_destroy_pixmap (radeon_glamor.c:272)
==2029== by 0x519D00: damageDestroyPixmap (damage.c:1473)
==2029== by 0x4DD307: XvDestroyPixmap (xvmain.c:370)
==2029== by 0x4DB975: ShmDestroyPixmap (shm.c:258)
==2029== by 0x5098F6: FreePicture (picture.c:1425)
==2029== by 0x85E678E: glamor_composite_clipped_region (glamor_render.c:1558)
==2029== by 0x85F763A: glamor_composite_largepixmap_region (glamor_largepixmap.c:1347)
==2029== by 0x85E7964: _glamor_composite (glamor_render.c:1679)
==2029== by 0x85E7A38: glamor_composite (glamor_render.c:1758)
==2029== Address 0x1141d3c0 is 0 bytes inside a block of size 64 free'd
==2029== at 0x4C29E90: free (vg_replace_malloc.c:473)
==2029== by 0x85D7167: glamor_set_pixmap_private (glamor.c:570)
==2029== by 0xC1BDDC4: radeon_glamor_destroy_pixmap (radeon_glamor.c:272)
==2029== by 0x519D00: damageDestroyPixmap (damage.c:1473)
==2029== by 0x4DD307: XvDestroyPixmap (xvmain.c:370)
==2029== by 0x4DB975: ShmDestroyPixmap (shm.c:258)
==2029== by 0x45B246: doFreeResource (resource.c:875)
==2029== by 0x45BD5E: FreeResource (resource.c:905)
==2029== by 0x43444B: ProcFreePixmap (dispatch.c:1422)
==2029== by 0x43856E: Dispatch (dispatch.c:432)
==2029== by 0x43C96F: dix_main (main.c:298)
==2029== by 0x6CFAB44: (below main) (libc-start.c:287)
Signed-off-by: Michel Dänzer <michel.daenzer@amd.com>
Reviewed-by: Keith Packard <keithp@keithp.com>
Signed-off-by: Keith Packard <keithp@keithp.com>
The other way around fails to destroy the screen pixmap EGL image:
==1782== 80 (32 direct, 48 indirect) bytes in 1 blocks are definitely lost in loss record 981 of 2,171
==1782== at 0x4C28C20: malloc (vg_replace_malloc.c:296)
==1782== by 0xF9D4BD2: dri2_create_image_from_dri (egl_dri2.c:1264)
==1782== by 0xF9D4BD2: dri2_create_image_dma_buf (egl_dri2.c:1764)
==1782== by 0xF9D4BD2: dri2_create_image_khr (egl_dri2.c:1798)
==1782== by 0xF9C7937: eglCreateImageKHR (eglapi.c:1494)
==1782== by 0x85D5655: _glamor_egl_create_image (glamor_egl.c:134)
==1782== by 0x85D5655: glamor_egl_create_textured_pixmap (glamor_egl.c:302)
==1782== by 0x85D579B: glamor_egl_create_textured_screen (glamor_egl.c:225)
==1782== by 0xC1BE05D: radeon_glamor_create_screen_resources (radeon_glamor.c:67)
==1782== by 0xC1B6153: RADEONCreateScreenResources_KMS (radeon_kms.c:258)
==1782== by 0x4B2105: xf86CrtcCreateScreenResources (xf86Crtc.c:709)
==1782== by 0x43C823: dix_main (main.c:223)
==1782== by 0x6CFAB44: (below main) (libc-start.c:287)
Signed-off-by: Michel Dänzer <michel.daenzer@amd.com>
Reviewed-by: Keith Packard <keithp@keithp.com>
Signed-off-by: Keith Packard <keithp@keithp.com>
For robustness against drivers which may call both
glamor_(egl_)destroy_textured_pixmap and glamor_destroy_pixmap.
Signed-off-by: Michel Dänzer <michel.daenzer@amd.com>
Reviewed-by: Keith Packard <keithp@keithp.com>
Signed-off-by: Keith Packard <keithp@keithp.com>
==25551== Invalid read of size 8
==25551== at 0x85D5F2C: glamor_egl_destroy_pixmap_image (glamor_egl.c:527)
==25551== by 0x85D7750: glamor_destroy_pixmap (glamor.c:235)
==25551== by 0xC1BDD9B: radeon_glamor_destroy_pixmap (radeon_glamor.c:278)
==25551== by 0x5098F6: FreePicture (picture.c:1425)
==25551== by 0x85DD7A9: glamor_unrealize_glyph_caches (glamor_glyphs.c:257)
==25551== by 0x85D7B50: glamor_close_screen (glamor.c:586)
==25551== by 0x4B1A82: xf86CrtcCloseScreen (xf86Crtc.c:734)
==25551== by 0x4CFFC7: CursorCloseScreen (cursor.c:187)
==25551== by 0x513A44: AnimCurCloseScreen (animcur.c:106)
==25551== by 0x51529B: present_close_screen (present_screen.c:64)
==25551== by 0x43CA83: dix_main (main.c:351)
==25551== by 0x6CFAB44: (below main) (libc-start.c:287)
==25551== Address 0x83dafa0 is 96 bytes inside a block of size 152 free'd
==25551== at 0x4C29E90: free (vg_replace_malloc.c:473)
==25551== by 0x85D76B4: glamor_destroy_textured_pixmap (glamor.c:225)
==25551== by 0x85D7750: glamor_destroy_pixmap (glamor.c:235)
==25551== by 0xC1BDD9B: radeon_glamor_destroy_pixmap (radeon_glamor.c:278)
==25551== by 0x5098F6: FreePicture (picture.c:1425)
==25551== by 0x85DD7A9: glamor_unrealize_glyph_caches (glamor_glyphs.c:257)
==25551== by 0x85D7B50: glamor_close_screen (glamor.c:586)
==25551== by 0x4B1A82: xf86CrtcCloseScreen (xf86Crtc.c:734)
==25551== by 0x4CFFC7: CursorCloseScreen (cursor.c:187)
==25551== by 0x513A44: AnimCurCloseScreen (animcur.c:106)
==25551== by 0x51529B: present_close_screen (present_screen.c:64)
==25551== by 0x43CA83: dix_main (main.c:351)
Signed-off-by: Michel Dänzer <michel.daenzer@amd.com>
Reviewed-by: Keith Packard <keithp@keithp.com>
Signed-off-by: Keith Packard <keithp@keithp.com>
(Originally written by Dave Airlie; split into a separate patch by
Kenneth Graunke.)
Signed-off-by: Dave Airlie <airlied@redhat.com>
Signed-off-by: Kenneth Graunke <kenneth@whitecape.org>
Tested-by: Jason Ekstrand <jason.ekstrand@intel.com>
Reviewed-by: Keith Packard <keithp@keithp.com>
They are part of the ABI.
Signed-off-by: Michel Dänzer <michel.daenzer@amd.com>
Reviewed-by: Keith Packard <keithp@keithp.com>
Signed-off-by: Keith Packard <keithp@keithp.com>
When reallocating the framebuffer on screen resize, the old EGL image
was getting leaked. Check for an existing EGL image and free it in
this case.
Signed-off-by: Keith Packard <keithp@keithp.com>
Revewied-by: Zhigang Gong <zhigang.gong@linux.intel.com>
There's no reason to store this in the egl screen private as the
screen pixmap will always hold a reference to it anyways.
Signed-off-by: Keith Packard <keithp@keithp.com>
Revewied-by: Zhigang Gong <zhigang.gong@linux.intel.com>
There were three paths that called eglDestroyImageKHR:
* The front buffer
* The intel driver's flip buffer
* pixmaps under DRI3
This patch unifies the second two by having glamor_destroy_pixmap
always destroy any associaged EGL image. This allows us to stop
storing the back_pixmap pointer in glamor as that was only used to
make sure that buffer was freed at server reset time.
v2: check for valid pixmap_priv before using it in
glamor_egl_destroy_pixmap_image
Signed-off-by: Keith Packard <keithp@keithp.com>
Reviewed-by: Zhigang Gong <zhigang.gong@linux.intel.com>
Mark fbos created from external buffers so that when the associated
pixmap is destroyed, they aren't put into the fbo cache for later
re-use and are instead freed immediately.
Signed-off-by: Keith Packard <keithp@keithp.com>
Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
When uploading bits to a texture which need reformatting to match a
supported GL format, a temporary buffer is allocated to hold the
reformatted bits. This gets freed in the general path, but is not
freed in the fast path because that includes an early return before
the call to free.
This patch removes the early return and places the general case under
an 'else' block, so that both paths reach the call to free.
Signed-off-by: Keith Packard <keithp@keithp.com>
Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
glamor_color_convert_to_bits() returns its second argument on
success, NULL on error, and need_free_bits already makes sure that
"bits" aliasing converted_bits is freed in the success case.
Looks like the memory leak that was supposed to be fixed in
6e50bfa706 only occurred in the error
case.
Reviewed-by: Michel Dänzer <michel.daenzer@amd.com>
Signed-off-by: Keith Packard <keithp@keithp.com>
This existed to be passed to the bs recovery routine; since we back all
planes, we don't care.
Reviewed-by: Julien Cristau <jcristau@debian.org>
Signed-off-by: Adam Jackson <ajax@redhat.com>
Otherwise the CPU may end up reading from non-cacheable memory, which is
very slow.
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=84178
Signed-off-by: Michel Dänzer <michel.daenzer@amd.com>
Reviewed-by: Markus Wick <markus@selfnet.de>
Reviewed-by: Eric Anholt <eric@anholt.net>
Signed-off-by: Keith Packard <keithp@keithp.com>
==9530== 808,575,600 bytes in 5,904 blocks are definitely lost in loss record 4,602 of 4,602
==9530== at 0x4C28C20: malloc (vg_replace_malloc.c:296)
==9530== by 0xAD29C98: _glamor_upload_bits_to_pixmap_texture (glamor_pixmap.c:771)
==9530== by 0xAD2AE95: glamor_upload_sub_pixmap_to_texture (glamor_pixmap.c:1031)
==9530== by 0xAD2BD55: glamor_upload_pixmap_to_texture (glamor_pixmap.c:1057)
==9530== by 0xAD1C2E6: glamor_composite_choose_shader (glamor_render.c:1025)
==9530== by 0xAD1C629: glamor_composite_with_shader (glamor_render.c:1174)
==9530== by 0xAD1DA77: glamor_composite_clipped_region (glamor_render.c:1542)
==9530== by 0xAD1E849: _glamor_composite (glamor_render.c:1689)
==9530== by 0xAD1ED90: glamor_composite (glamor_render.c:1758)
==9530== by 0x519FD6: damageComposite (damage.c:502)
==9530== by 0xAD27AA3: glamor_trapezoids (glamor_trapezoid.c:147)
==9530== by 0xAD27B51: glamor_trapezoids (glamor_trapezoid.c:101)
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=84176
Signed-off-by: Michel Dänzer <michel.daenzer@amd.com>
Reviewed-by: Eric Anholt <eric@anholt.net>
Signed-off-by: Keith Packard <keithp@keithp.com>
I can't find any performance benefit to using the GL path and the code
renders this trapezoid incorrectly:
top: FIXED 29.50
bottom: FIXED 30.00
left top: POINT 0.00, 29.50
left bottom: POINT 0.00, 30.50
right top: POINT -127.50, 29.50
right bottom: POINT 52.50, 30.00
This should render a solid line from 0,30 to 52,30 but draws nothing.
The code also uses an area computation for trapezoid coverage which
does not conform to the Render specification which requires a specific
point sampling technique.
Signed-off-by: Keith Packard <keithp@keithp.com>
Reviewed-by: Adam Jackson <ajax@redhat.com>
This enables the assertion that all users of the large pixmap member
are restricted to pixmaps which are actually large.
Signed-off-by: Keith Packard <keithp@keithp.com>
Reviewed-by: Adam Jackson <ajax@redhat.com>
glamor_composite_largepixmap_region is given the job of dealing with
compositing between a mixture of large and small pixmaps. However, it
was assuming that the destination pixmap was large and fetching
members of the large structure even for small pixmaps.
This manifested with assertion failures when compositing from a large
pixmap to a small pixmap.
Fixed by using the pixmap size for the destination block size for
small pixmaps.
Signed-off-by: Keith Packard <keithp@keithp.com>
Reviewed-by: Adam Jackson <ajax@redhat.com>
glamor_compute_clipped_regions_ext wants to treat small and large
pixmaps uniformly and did that by writing into the large pixmap
union member in small pixmaps to construct something that looks like a
one texture large pixmap.
Instead of doing that, simply allocate the necessary elements locally
on the stack and use them from there.
Signed-off-by: Keith Packard <keithp@keithp.com>
Reviewed-by: Adam Jackson <ajax@redhat.com>
For now, this simply fetches the large member of the pixmap private.
It will be changed to assert that the pixmap is large once bugs
related to that have been fixed.
Signed-off-by: Keith Packard <keithp@keithp.com>
Reviewed-by: Adam Jackson <ajax@redhat.com>
This is the last function-like macro in glamor_priv.h; change to
static inline like all of the other functions there.
Signed-off-by: Keith Packard <keithp@keithp.com>
Reviewed-by: Adam Jackson <ajax@redhat.com>
It references a pixmap, which is a per-screen resource.
Fixes broken text rendering in xfwm4-tweak-settings in Zaphod mode.
Signed-off-by: Michel Dänzer <michel.daenzer@amd.com>
Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
Signed-off-by: Keith Packard <keithp@keithp.com>
This is necessary because the glyph caches are per screen.
Fixes broken menu text in gnome-terminal in Zaphod mode.
Signed-off-by: Michel Dänzer <michel.daenzer@amd.com>
Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
Signed-off-by: Keith Packard <keithp@keithp.com>
And rename the boolean to reflect what it's about.
Signed-off-by: Michel Dänzer <michel.daenzer@amd.com>
Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
Signed-off-by: Keith Packard <keithp@keithp.com>
The comment above glamor_glyphs_init was already saying so.
Signed-off-by: Michel Dänzer <michel.daenzer@amd.com>
Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
Signed-off-by: Keith Packard <keithp@keithp.com>
It results in a crash.
Signed-off-by: Michel Dänzer <michel.daenzer@amd.com>
Reviewed-by: Eric Anholt <eric@anholt.net>
Signed-off-by: Keith Packard <keithp@keithp.com>
The Xv StopVideo callback is not invoked on textured video ports, so
the temporary pixmaps allocated for the video planes are never freed.
Freeing the storage immediately after use is a simple solution to this
problem which doesn't appear to have any visible performance impact.
Signed-off-by: Keith Packard <keithp@keithp.com>
Reviewed-by: Eric Anholt <eric@anholt.net>
This happens to avoid GL errors on hardware without
EXT_texture_integer (which implies < GLSL 130, and thus glamor_text.c
programs not compiling anyway).
Signed-off-by: Eric Anholt <eric@anholt.net>
Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
Signed-off-by: Keith Packard <keithp@keithp.com>
This provides a speedup e.g. when the destination is an SHM pixmap.
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=76285
Signed-off-by: Michel Dänzer <michel.daenzer@amd.com>
Reviewed-by: Eric Anholt <eric@anholt.net>
Signed-off-by: Keith Packard <keithp@keithp.com>
A few files in the server are including xorg-server.h, which is only
for use by Xorg server drivers. This fixes those errors and then adds
a check to make sure it doesn't happen again.
Signed-off-by: Keith Packard <keithp@keithp.com>
Reviewed-by: Adam Jackson <ajax@redhat.com>
This uses a single large triangle and a scissor to draw the video
instead of two triangles.
Signed-off-by: Keith Packard <keithp@keithp.com>
Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
I intended to use glFlush all along, but somehow managed to type
glFinish instead. glFlush is sufficient (for a single-queue GPU) to
ensure serialization between queued rendering in the X server and
future rendering from the client.
Signed-off-by: Keith Packard <keithp@keithp.com>
Reviewed-by: Jasper St. Pierre <jstpierre@mecheye.net>
This hooks up SHM sync fences to complete the requirements for DRI3
running on Glamor.
Signed-off-by: Keith Packard <keithp@keithp.com>
Reviewed-by: Eric Anholt <eric@anholt.net>
It's unused since keithp's copy acceleration code completely replaced
glamor_copyarea.c and removed the blit path.
Signed-off-by: Eric Anholt <eric@anholt.net>
Reviewed-by: Keith Packard <keithp@keithp.com>
Unused since the glamor_prepare.c replacement of glamor_finish_access().
Signed-off-by: Eric Anholt <eric@anholt.net>
Reviewed-by: Keith Packard <keithp@keithp.com>
All users of glamor had the same value set, and it complicated things
for no reason.
Signed-off-by: Eric Anholt <eric@anholt.net>
Reviewed-by: Keith Packard <keithp@keithp.com>
After keithp's change to drop the old glamor_fill() code, nothing ever
changed these values.
Signed-off-by: Eric Anholt <eric@anholt.net>
Reviewed-by: Keith Packard <keithp@keithp.com>
We're not drawing, and we're not initially setting up the texture for
later drawing.
Signed-off-by: Eric Anholt <eric@anholt.net>
Reviewed-by: Keith Packard <keithp@keithp.com>
If this path needed the filters set, so would all the other
glDrawArrays() callers. But they don't.
Signed-off-by: Eric Anholt <eric@anholt.net>
Reviewed-by: Keith Packard <keithp@keithp.com>
To understand this patch, let's start at the protocol interface where
the relationship between the coordinate spaces is documented:
static Bool
_glamor_composite(CARD8 op,
PicturePtr source,
PicturePtr mask,
PicturePtr dest,
INT16 x_source,
INT16 y_source,
INT16 x_mask,
INT16 y_mask,
INT16 x_dest, INT16 y_dest,
CARD16 width, CARD16 height, Bool fallback)
The coordinates are passed to this function directly off the wire and
are all relative to their respective drawables. For Windows, this means
that they are relative to the upper left corner of the window, in
whatever pixmap that window is getting drawn to.
_glamor_composite calls miComputeCompositeRegion to construct a clipped
region to actually render to. In reality, miComputeCompositeRegion clips
only to the destination these days; source clip region based clipping
would have to respect the transform, which isn't really possible. The
returned region is relative to the screen in which dest lives; offset by
dest->drawable.x and dest->drawable.y.
What is important to realize here is that, because of clipping, the
composite region may not have the same position within the destination
drawable as x_dest, y_dest. The protocol coordinates now exist solely to
'pin' the three objects together.
extents->x1,y1 Screen origin of clipped operation
width,height Extents of the clipped operation
x_dest,y_dest Unclipped destination-relative operation coordinate
x_source,y_source Unclipped source-relative operation coordinate
x_mask,y_mask Unclipped mask-relative operation coordinate
One thing we want to know is what the offset is from the original
operation origin to the clipped origin
Destination drawable relative coordinates of the clipped operation:
x_dest_clipped = extents->x1 - dest->drawable.x
y_dest_clipped = extents->y1 - dest->drawable.y
Offset from the original operation origin:
x_off_clipped = x_dest_clipped - x_dest
y_off_clipped = y_dest_clipped - y_dest
Source drawable relative coordinates of the clipped operation:
x_source_clipped = x_source + x_off_clipped;
y_source_clipped = y_source + y_off_clipped;
Mask drawable relative coordinates of the clipped operation:
x_mask_clipped = x_source + x_off_clipped;
y_mask_clipped = y_source + y_off_clipped;
This is where the original code fails -- it doesn't subtract the
destination drawable location when computing the distance that the
operation has been moved by clipping. Here's what it does when
constructing a temporary source picture:
temp_src =
glamor_convert_gradient_picture(screen, source,
extent->x1 + x_source - x_dest,
extent->y1 + y_source - y_dest,
width, height);
...
x_temp_src = -extent->x1 + x_dest;
y_temp_src = -extent->y1 + y_dest;
glamor_convert_gradient_picture needs source drawable relative
coordinates, but that is not what it's getting; it's getting
screen-relative coordinates for the destination, adjusted by the
distance between the provided source and destination operation
coordinates. We want x_source_clipped and y_source_clipped:
x_source_clipped = x_source + x_off_clipped
= x_source + x_dest_clipped - x_dest
= x_source + extents->x1 - dest->drawable.x - x_dest
x_temp_src/y_temp_src are supposed to be the coordinates of the original
operation translated to the temporary picture:
x_temp_src = x_source - x_source_clipped;
y_temp_src = y_source - y_source_clipped;
Note that x_source_clipped/y_source_clipped will never be less than
x_source/y_source because all we're doing is clipping. This means that
x_temp_src/y_temp_src will always be non-positive; the original source
coordinate can never be strictly *inside* the temporary image or we
could have made the temporary image smaller.
x_temp_src = x_source - x_source_clipped
= x_source - (x_source + x_off_clipped)
= -x_off_clipped
= x_dest - x_dest_clipped
= x_dest - (extents->x1 - dest->drawable.x)
Again, this is off by the destination origin within the screen
coordinate space.
The code should look like:
temp_src =
glamor_convert_gradient_picture(screen, source,
extent->x1 + x_source - x_dest - dest->pDrawable->x,
extent->y1 + y_source - y_dest - dest->pDrawable->y,
width, height);
x_temp_src = -extent->x1 + x_dest + dest->pDrawable->x;
y_temp_src = -extent->y1 + y_dest + dest->pDrawable->y;
Signed-off-by: Keith Packard <keithp@keithp.com>
Reviewed-by: Markus Wick <markus@selfnet.de>
Michel Dänzer
Dave Airlie
Keith Packard
Peter Hutterer
Andreas Hartmetz
Adam Jackson
Eric Anholt