This patch acts as bandaid in the core compositor to avoid the renderer
doing a flush after the buffer has been released. Flushing after release
can happen due to problems in the internal damage tracking, is violating
the protocol, and causes visible glitches.
A more proper fix would be to handle compositor side damage correctly.
Suggested-by: Pekka Paalanen <pekka.paalanen@collabora.com>
Acked-by: Daniel Stone <daniel.stone@collabora.com>
Signed-off-by: Marius Vlad <marius.vlad@collabora.com>
Since b38b735e20, 'backend-drm: Remove Pixman conditional
for keep_buffer' the Pixman renderer keeps its own reference to buffers
when attached to surfaces, rather than flipping keep_buffer variable for
the surface. Problem is that when switching from the Pixman render to
the GL would not work and could result in a crash upon first repaint.
Signed-off-by: Marius Vlad <marius.vlad@collabora.com>
Between fork() and exec() in the child process it is only safe to use
async-signal-safe functions. Painfully, setenv() is not listed as such.
Therefore we must craft our own custom environment, and we get no help
from libc with that.
Signed-off-by: Pekka Paalanen <pekka.paalanen@collabora.com>
Constructing argv before-hand is a little easier to look at, but this is
mostly just anticipating more changes to how Weston spawns processes in
general.
Signed-off-by: Pekka Paalanen <pekka.paalanen@collabora.com>
We are already using pipe2() in many places, even in libweston, so let's
simplify the code here as well - not to mention avoid a theoretical
race.
Signed-off-by: Pekka Paalanen <pekka.paalanen@collabora.com>
Between fork() and exec() in the child process it is only safe to use
async-signal-safe functions. Surprisingly, snprintf() is not such
function. See e.g. https://stackoverflow.com/a/6771799 and how snprintf
is not listed in signal-safety(7) manual.
Therefore we must prepare the fd argument strings before fork(). That is
only possible if we also do not dup() fd in the child process. Hence we
remove the close-on-exec flag instead in the child process which has
copies of the parent's file descriptors. Fortunately fcntl() is safe.
struct fdstr is helping to reduce code clutter a bit.
Additionally, if fork() fails, we now clean up the fds we created.
Signed-off-by: Pekka Paalanen <pekka.paalanen@collabora.com>
This function will be used between fork() and exec() to remove the
close-on-exec flag. The first user will be compositor/xwayland.c.
Signed-off-by: Pekka Paalanen <pekka.paalanen@collabora.com>
fcntl(2) manual says the return type is int, and that F_SETFD takes an
int. So use int.
Noticed by code inspection.
Signed-off-by: Pekka Paalanen <pekka.paalanen@collabora.com>
Between fork() and exec() in the child process it is only safe to use
async-signal-safe functions. weston_log() definitely is not one, it
allocates memory and does whatnot.
weston_log() is also inappropriate for other reasons: the child process
has its own stream buffers and flight-recorder. No-one looks into the
child process' flight recorder, so messages would be lost there. The
logging machinery might also attempt to write into debug streams,
meaning both parent and child could be writing simultaneously.
It seems that the best we can do is to pre-bake an error message and
only write() it out if exec() fails. There is no mention that even
strerror_r() might be safe to call, so we don't.
Signed-off-by: Pekka Paalanen <pekka.paalanen@collabora.com>
Doing any kind of memory allocation calls between fork() and exec() in
the child process is prone to deadlocks and explosions. In general, only
async-signal-safe functions are safe there.
Move the config access to the parent process before fork() to avoid
problems.
See also:
https://gitlab.freedesktop.org/wayland/weston/-/merge_requests/941#note_1457053
Signed-off-by: Pekka Paalanen <pekka.paalanen@collabora.com>
This reverts commit 4aa885d4af.
Turns out the problem was not about dupping fds at all, but calling
non-async-signal-safe functions like strdup() between fork() and exec()
in the child process.
For more discussion, see:
https://gitlab.freedesktop.org/wayland/weston/-/merge_requests/941#note_1457053
Signed-off-by: Pekka Paalanen <pekka.paalanen@collabora.com>
This patch fixes the following:
AddressSanitizer:DEADLYSIGNAL
=================================================================
==528956==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000008 (pc 0x7fbc5d66bdd7 bp 0x7ffd465573c0 sp 0x7ffd46557398 T0)
==528956==The signal is caused by a WRITE memory access.
==528956==Hint: address points to the zero page.
#0 0x7fbc5d66bdd7 in wl_list_remove ../../git/wayland/src/wayland-util.c:56
#1 0x7fbc5cb8869e in wxw_compositor_destroy ../../git/weston/compositor/xwayland.c:357
#2 0x7fbc5baf3ca6 in weston_signal_emit_mutable ../../git/weston/shared/signal.c:62
#3 0x7fbc5ba4d6f9 in weston_compositor_destroy ../../git/weston/libweston/compositor.c:8639
#4 0x7fbc5cb7a5f2 in wet_main ../../git/weston/compositor/main.c:3772
#5 0x55bd13de2179 in main ../../git/weston/compositor/executable.c:33
#6 0x7fbc5be61d09 in __libc_start_main ../csu/libc-start.c:308
#7 0x55bd13de2099 in _start (/home/pq/local/bin/weston+0x1099)
The problem is triggered by configuring a bad path to Xwayland in
weston.ini, which causes exec() to fail. The fork() succeeded though,
which means the weston_process was already on the watch list, and the
watch can be handled, making sigchl_handler() leave the link
uninitialized.
Making sure the link remains removable fixes this.
Fixes: 18897253d4
Signed-off-by: Pekka Paalanen <pekka.paalanen@collabora.com>
While developing the new color management, keeping these old plugins
working would require extra work. Let's deprecate these to see if anyone
cares about them, pending removal after the Weston 11.0.0 release.
CI will keep building these in the "Full build" configuration only. Doc
and no-GL builds are no different for these plugins, so there these are
no longer built.
See https://gitlab.freedesktop.org/wayland/weston/-/issues/634
Signed-off-by: Pekka Paalanen <pekka.paalanen@collabora.com>
There are two versions of WM_NORMAL_HINTS: the original pre-ICCCM
version (standardised by Xlib itself?) provides 15 elements of 32 bits
each, with the ICCCM v1 extending this by 3 additional elements.
Since the flags are enough to identify which elements are present, and
the structure is append-only, we only need to read the minimum length
between what the user provided and what we support.
Fixes a heap overrun found with ASan.
Signed-off-by: Daniel Stone <daniels@collabora.com>
Setting up the arguments may consume some of the arguments, e.g. if we
provide a config file or extra modules, then the test harness is
expected to be responsible for freeing those arguments.
Checking the requirements and bailing first means that we never do that,
and thus skipped tests result in leaks. Flip the order so we set up the
args first and skip last, so we can consistently take ownership of all
the provided setup parameters.
Signed-off-by: Daniel Stone <daniels@collabora.com>
ZUC's default mode is to fork for every test case. Unfortunately on
AArch64, fork in an ASan-traced program usually takes around 3.6 entire
seconds. This was leading to the config-parser test timing out.
As none of our remaining ZUC tests even need to fork, just remove all
support for it.
Signed-off-by: Daniel Stone <daniels@collabora.com>
This commit is truly horrible.
We want to run ASan with leak checking enabled in CI so we can catch
memory leaks before they're introduced. This works well with Pixman, and
with NIR-only drivers like iris or Panfrost.
But when we run under llvmpipe - which we do under CI - we start failing
because:
- Mesa pulls in llvmpipe via dlopen
- llvmpipe pulls in LLVM itself via DT_NEEDED
- initialising LLVM's global type/etc systems performs thread-local
allocations
- llvmpipe can't free those allocations since the application might
also be using LLVM
- Weston stops using GL and destroys all GL objects, leading to Mesa
unloading llvmpipe like it should
- with everything disappearing from the process's vmap, ASan can no
longer keep track of still-reachable pointers
- tests fail because LLVM is 'leaking'
Usually, an alternative is to LD_PRELOAD a shim which overrides
dlclose() to be a no-op. This is not usable here, because when
$LD_PRELOAD is not empty and ASan is not first in it, ASan immediately
errors out. Prepending ASan doesn't work, because we run our tests
through Meson (which also invokes Ninja), leading to LSan exploding
over CPython and Ninja, which is not what we're interested in.
It would be possible to inject _both_ ASan and a dlclose-does-nothing
shim DSO into the LD_PRELOAD environment for every test, but that seems
even worse, especially as Meson strongly discourages globbing for random
files in the root.
So, here we are, doing what we can: finding where swrast_dri.so (aka
llvmpipe) lives, stashing that in an environment variable, and
deliberately leaking a dlopen handle which we never close to ensure that
neither llvmpipe nor LLVM leave our process's address space before we
exit.
Signed-off-by: Daniel Stone <daniels@collabora.com>
Unfortunately just adding suppressions isn't enough; the build of Expat
we have in our CI system does not have frame pointers, so ASan's fast
unwinder can't see through it. This means that the suppressions we've
added won't be taken into account.
For now, disable the fast unwinder for the Xwayland test only. Disabling
it globally is not practical as it massively increases the per-test
runtime, so here (to avoid polluting the build system), we use a
per-test wrapper to selectively choose the unwinder.
Signed-off-by: Daniel Stone <daniels@collabora.com>
For some reason, the Debian CI setup leaks fontconfig allocations in a
way which it does not for me on Fedora. On the assumption that this has
been fixed between our older CI Debian and Fedora 36, suppress the leak
warning, because we do already call FcFini() which should free it.
Signed-off-by: Daniel Stone <daniels@collabora.com>
When an output is destroyed then the output state is freed immediately. In this
case, the plane state is only partially destroyed because it is the currently
active state. This includes the buffer reference.
Without the output, the plane will not be updated any more until it is used by a
different output (if possible) or the output returns and the plane is used
again.
As a result, the buffer reference is kept for a long time. This will cause some
applications to stall because weston now keeps two buffers (the one here and
another one for a different output where the application is now displayed).
To avoid this, do a synchronous commit that disables the output. The output
needs to be disabled anyways and this way the current state contains no
buffers that would remain.
`device->state_invalid = true` in drm_output_detach_crtc() is no longer
needed, because drm_output_detach_crtc() is called only when initialization
failed and the crtc was not yet used or in drm_output_deinit() when the
crtc was already disabled with the new synchronous commit.
Signed-off-by: Michael Olbrich <m.olbrich@pengutronix.de>
This way the backends will the actual outputs. And at that point the backend
knows the compositor is shutting down so it can handle this differently if
necessary.
Afterwards wet_compositor_destroy_layout() just deletes the remaining
datastructures.
Signed-off-by: Michael Olbrich <m.olbrich@pengutronix.de>
Replace an oft-duplicated pattern with a trivial helper function. In
doing so, we observe that the one special case (displayfd 'didn't need
to be CLOEXEC') was wrong, because the X server does fork itself
internally, so there is nothing wrong with setting CLOEXEC.
Signed-off-by: Daniel Stone <daniels@collabora.com>
CLASS_DIAGRAM has been obsolete in newer version of doxygen, and
it's enabled if HAVE_DOT and CLASS_GRAPH are set.
This increase DOT_GRAPH_MAX_NODES to avoid dot complaning,
and include dot/graphviz for doxygen.
Signed-off-by: Marius Vlad <marius.vlad@collabora.com>
Same as LaTeX, RTF is being made obsolete in newer version of doxygen.
Also, we weren't really using it so there's no harm in removing it
entirely.
Signed-off-by: Marius Vlad <marius.vlad@collabora.com>
LaTeX has become obsolete in newer doxygen version, and we weren't using
it at all so remove it entirely.
Signed-off-by: Marius Vlad <marius.vlad@collabora.com>
Introduced with e0a858a5f2, commit 'weston-debug: Introduce
weston_log_subscription and weston_log_subscriber objects'. We don't
really return a weston_log_subscription so let's remove it.
Some newer doxygen detects this and we are treating warning as errors.
Fixes#594
Signed-off-by: Marius Vlad <marius.vlad@collabora.com>
This is adding basically a copy of alpha-blending-test.c. The difference
is that here we use ICC files to set up the output color profile, and
then test light-linear blending only. BLOCK_WIDTH is set to 1 to fit
inside the output size used by the fixture setup, which is smaller than
in the original, but does not change the results.
The test is aimed at testing how color-lcms module succeeds in
linearizing the output of different ICC output profiles. Incorrect
linearization should cause changes in blending results.
The tolerance is taken from the currently achieved error statistics
(1.40908) and rounded up a little to achieve a suitable margin.
Signed-off-by: Pekka Paalanen <pekka.paalanen@collabora.com>
It was binding to any advertised version, but it can't actually work
with version 4 (because it doesn't handle the new configure_bounds
event).
Other sample clients in the tree are hard-coding version 1, so do the
same here.
Fixes: 6d9fda7156 ("clients/presentation-shm: use xdg_shell instead of wl_shell")
Signed-off-by: Michel Dänzer <mdaenzer@redhat.com>
Switch from per-channel max error tolerance to max two-norm (Euclidean
distance) error. Geometrically this means that previously the accepted
volume was a +/- tolerance cube around the reference point, and now it
is a sphere with tolerance radius.
The real benefit is simplifying the code.
The error tolerance are also changed to float. Integers cannot represent
values between 1 and 2, and the jump from 1 to 2 would have been too
much. AdobeRGB tolerance gets relaxed a bit, while BT2020 tolerance
becomes stricter. The new tolerance values are the reported achieved
two-norm max errors plus a bit of margin.
Surprisingly the sRGB case tolerances remain strictly at zero, and
that's no bug in the test.
Signed-off-by: Pekka Paalanen <pekka.paalanen@collabora.com>
compare_float() was an ad hoc max error logger with optional debug
logging.
Now that we have rgb_diff_stat, we can get the same statistics and more
with less code. It looks like we would lose the pixel index x, but that
can be recovered from the dump file line number.
This patch takes care to keep the test condition exactly the same as it
was before. The statistics print-out has more details now.
Signed-off-by: Pekka Paalanen <pekka.paalanen@collabora.com>
Switch from per-channel max error tolerance to max two-norm (Euclidean
distance) error. Geometrically this means that previously the accepted
volume was a +/- tolerance cube around the reference point, and now it
is a sphere with tolerance radius. This makes the check slightly
stricter.
The real benefit is simplifying the code.
Signed-off-by: Pekka Paalanen <pekka.paalanen@collabora.com>
compare_float() was an ad hoc max error logger with optional debug
logging.
Now that we have rgb_diff_stat, we can get the same statistics and more
with less code. It looks like we would lose the pixel index x, but that
can be recovered from the dump file line number.
This patch takes care to keep the test condition exactly the same as it
was before. The statistics print-out has more details now.
The recorded dump position is the foreground color as that varies while
the background color is constant.
An example Octave function is included to show how to visualize the
rgb_diff_stat dump.
Signed-off-by: Pekka Paalanen <pekka.paalanen@collabora.com>
This is a special case of scalar_stat dumps to record all of two-norm
and RGB differences on the same line in the dump file.
This makes the dump file easier to handle when you want full RGB errors
recorded.
Signed-off-by: Pekka Paalanen <pekka.paalanen@collabora.com>
The recently introduced rgb_diff_stat value dumping feature logs the
"position" where the value or error was measured. The reference value
was used as the position, but the problem with the reference value is
that it is an output value and not an input value. Therefore mapping
that back to which input values promoted the error is not easy.
Fix that problem by passing the position explicitly into
rgb_diff_stat_update(), just like it is already passed in to
scalar_stat_update().
Currently the only user simply passes the reference value as position,
because there the input value is also the reference value. This is not
true for future uses of rgb_diff_stat.
Signed-off-by: Pekka Paalanen <pekka.paalanen@collabora.com>
The new field in struct scalar_stat allows recording all tested values
into a file. This is intended to replace ad hoc dumping code like in
alpha-blending-test.c.
To make it easy to set up, also offer a helper to open a writable file
whose name consists of a custom prefix and test name.
Signed-off-by: Pekka Paalanen <pekka.paalanen@collabora.com>
Seems it will be common to print all four min/max/avg sets of errors, so
move the printing code into a shared place.
While 0.0-1.0 is the natural range for color values, people are often
accustomed to working with 8-bit or even 10-bit pixel values. An error
of +/- 1 in 8-bit is more intuitive than +/- 0.004 in floating-point.
Hence 'scaling_bits' is added so the caller can determine the value
scaling. This will scale both the reported error numbers, and the
recorded error positions (rgb-tuples), so they are all comparable.
I'm happy to get rid of those two macros as well.
Signed-off-by: Pekka Paalanen <pekka.paalanen@collabora.com>
More tests are going to need this.
The API is changed to work by copy in and copy out to match the other
color_util API. Hopefully this makes the caller code easier to read.
Signed-off-by: Pekka Paalanen <pekka.paalanen@collabora.com>
We treat the argv we pass into the compositor as its to mangle, just as
it is free to do so for POSIX argv. To support this, we stash argv away
and free the saved copy later so as to not leak.
This works perfectly, except when we never call the compositor at all,
and have no saved array to free. Make sure we free the args in this
case, which can be seen as a leak of any generated args when a test
skips on preflight checks, e.g. drm-smoke when not running in CI.
Signed-off-by: Daniel Stone <daniels@collabora.com>
Daniel Stone
Marius Vlad
Pekka Paalanen
Michael Olbrich
Michel Dänzer