Repository navigation
evdi_painter: Access to scanout FB, vblank event, dirty rects racy #511
Description
When an atomic commit is being processed through the atomic commit helpers, the details of the coming frame is recorded on the evdi_painter through individual calls to evdi_painter_set_vblank, evdi_painter_set_scanout_buffer, evdi_painter_mark_dirty, etc.
Each of these functions take the evdi_painter's mutex through painter_lock, perform cleanup if needed, record the new value, and release the lock again. Once a user-space driver comes along and calls DRM_EVDI_GRABPIX, the painter lock is taken and all the values are observed.
The problem is that the values are recorded under individually held locks, meaning that there is no guarantee that the whole set of values are logically consistent. The caller may observe the dirty rect from the previous commit, but the scanout buffer from the current commit.
New values must be applied atomically. The simplest way to do this would be to accumulate values in a pending struct, and at the end of the atomic commit do something like:
struct evdi_painter_state old;
painter_lock(painter);
old = *painter->current;
*painter->current = *painter->pending;
evdi_painter_state_init(painter->pending);
painter_unlock(painter);
evdi_painter_state_finish(&old);
Activity
synaptics-lspintzyk commentedon Mar 28, 2025
Hi
@kennylevinsen, you are right that evdi_painter should care for consistency of the data that userspace driver reads.
However I am not sure that we have problem at the moment with dirty rects. evdi_painter role is to accumulate dirty rects and keep pointer to last framebuffer, so that userspace driver will not miss any dirty rects. Here we have assumption that framebuffer content is valid and latest in the areas outside of the dirty rects region! Please let me know I am wrong here.
I understand that you are raising concerns about design or code quality. Thank you for that.
We can consider that improvements if this would fix some issues you see. Can you elaborate on that? E.g. what is your desktop environment or steps to reproduce issue?
kennylevinsen commentedon Mar 28, 2025
If
DRM_EVDI_GRABPIXis called after the buffer is recorded but before the commit flushes, vblank will not yet be stored and will therefore not be fired byevdi_painter_grabpix_ioctl. Without a pageflip event, no further commits will occur and the fallback fire inevdi_painter_set_vblankwill also not fire. Later calls toevdi_painter_grabpix_ioctlwill fail early due to there being no dirty regions. This would stall rendering for that output.If a newer damage clip could end up used with an older buffer, issues would occur where content new content does not end up copied as the old buffer was used as source, and later buffers while containing the new content does not damage the area.
If an older damage clip is used with a new buffer, the output will show stale and/or partial content until the output changes again.
Maybe other things could happen. It's definitely broken, and it doesn't feel like it would be that much work to fix. I discovered the issue while looking at why displaylink was tearing despite running under a strictly double-buffering display server.
synaptics-lspintzyk commentedon Mar 28, 2025
Thanks @kennylevinsen
Can you describe steps to reproduce that problems?
e.g.
What distro you have?
What primary gpu (e.g. amdgpu has some known tearing issues to me. maybe they are related)?
What compositor you have? How it is configured. Is it installed from sources of from packages.
Any relevant informations.
Thanks for rising that.
kennylevinsen commentedon Mar 28, 2025
The tearing was observed with AMDGPU yes. It could be an implicit sync issue, made worse by the very high display refresh evdi is driving.
WLR_RENDERER=vulkanset) or niri (the development branch linked in the other issueThere's some details about the discovery here and in the other issue about vblank. This issue was just about the race conditions I found when glancing at evdi.