| Message ID | 20231109172449.1599262-1-javierm@redhat.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | drm: Allow the damage helpers to handle buffer damage | expand |
Hi Javier Am 09.11.23 um 18:24 schrieb Javier Martinez Canillas: > Hello, > > This series is to fix an issue that surfaced after damage clipping was > enabled for the virtio-gpu by commit 01f05940a9a7 ("drm/virtio: Enable > fb damage clips property for the primary plane"). > > After that change, flickering artifacts was reported to be present with > both weston and wlroots wayland compositors when running in a virtual > machine. The cause was identified by Sima Vetter, who pointed out that > virtio-gpu does per-buffer uploads and for this reason it needs to do > a buffer damage handling, instead of frame damage handling. I'm having problem understanding the types of damage. You never say what buffer damage is. I also don't know what a frame is in this context. Regular damage handling marks parts of a plane as dirty/damaged. That is per-plane damage handling. The individual planes more or less independent from each other. Buffer damage, I guess, marks the underlying buffer as dirty and requires synchronization of the buffer with some backing storage. The planes using that buffer are then updated more or less automatically. Is that right? And why does it flicker? Is there old data stored somewhere? Best regards Thomas > > Their suggestion was to extend the damage helpers to cover that case > and given that there's isn't a buffer damage accumulation algorithm > (e.g: buffer age), just do a full plane update if the framebuffer that > is attached to a plane changed since the last plane update (page-flip). > > Patch #1 is just a refactoring to allow the logic of the frame damage > helpers to be shared by the buffer damage helpers. > > Patch #2 adds the helpers that are needed for buffer damage handling. > > Patch #3 fixes the virtio-gpu damage handling logic by using the > helper that is required by drivers that need to handle buffer damage. > > Patch #4 fixes the vmwgfx similarly, since that driver also needs to > handle buffer damage and should have the same issue (although I have > not tested it due not having a VMWare setup). > > Patch #5 adds to the KMS damage tracking kernel-doc some paragraphs > about damage tracking types and references to links that explain > frame damage vs buffer damage. > > Finally patch #6 adds an item to the DRM/KMS todo, about the need to > implement some buffer damage accumulation algorithm instead of just > doing a full plane update in this case. > > Because commit 01f05940a9a7 landed in v6.4, the first three patches > are marked as Fixes and Cc stable. > > I've tested this on a VM with weston, was able to reproduce the issue > reported and the patches did fix the problem. > > Please let me know what you think. Specially on the wording since could > made mistakes due just learning about these concepts yesterday thanks to > Sima, Simon and Pekka. > > Best regards, > Javier > > > Javier Martinez Canillas (6): > drm: Move drm_atomic_helper_damage_{iter_init,merged}() to helpers > drm: Add drm_atomic_helper_buffer_damage_{iter_init,merged}() helpers > drm/virtio: Use drm_atomic_helper_buffer_damage_merged() for buffer > damage > drm/vmwgfx: Use drm_atomic_helper_buffer_damage_iter_init() for buffer > damage > drm/plane: Extend damage tracking kernel-doc > drm/todo: Add entry about implementing buffer age for damage tracking > > Documentation/gpu/todo.rst | 20 +++ > drivers/gpu/drm/drm_damage_helper.c | 166 +++++++++++++++++++------ > drivers/gpu/drm/drm_plane.c | 22 +++- > drivers/gpu/drm/virtio/virtgpu_plane.c | 2 +- > drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 2 +- > include/drm/drm_damage_helper.h | 7 ++ > 6 files changed, 173 insertions(+), 46 deletions(-) >
Thomas Zimmermann <tzimmermann@suse.de> writes: > Hi Javier > > Am 09.11.23 um 18:24 schrieb Javier Martinez Canillas: >> Hello, >> >> This series is to fix an issue that surfaced after damage clipping was >> enabled for the virtio-gpu by commit 01f05940a9a7 ("drm/virtio: Enable >> fb damage clips property for the primary plane"). >> >> After that change, flickering artifacts was reported to be present with >> both weston and wlroots wayland compositors when running in a virtual >> machine. The cause was identified by Sima Vetter, who pointed out that >> virtio-gpu does per-buffer uploads and for this reason it needs to do >> a buffer damage handling, instead of frame damage handling. > > I'm having problem understanding the types of damage. You never say what > buffer damage is. I also don't know what a frame is in this context. > > Regular damage handling marks parts of a plane as dirty/damaged. That is > per-plane damage handling. The individual planes more or less > independent from each other. > > Buffer damage, I guess, marks the underlying buffer as dirty and > requires synchronization of the buffer with some backing storage. The > planes using that buffer are then updated more or less automatically. > > Is that right? > In both cases the damage tracking information is the same, they mark the damaged regions on the plane in framebuffer coordinates of the framebuffer attached to the plane. The problem as far as I understand is whether the driver expects that to determine the area that changed in the plane (and a plane flush is enough) or the area that changed since that same buffer was last used. > And why does it flicker? Is there old data stored somewhere? > It flickers because the framebuffer changed and so the damage tracking is not used correctly to flush the damaged areas to the backing storage. This is my understanding at least, please Sima or Simon correct me if I got this wrong. > Best regards > Thomas >
Hi Am 14.11.23 um 17:28 schrieb Javier Martinez Canillas: > Thomas Zimmermann <tzimmermann@suse.de> writes: > >> Hi Javier >> >> Am 09.11.23 um 18:24 schrieb Javier Martinez Canillas: >>> Hello, >>> >>> This series is to fix an issue that surfaced after damage clipping was >>> enabled for the virtio-gpu by commit 01f05940a9a7 ("drm/virtio: Enable >>> fb damage clips property for the primary plane"). >>> >>> After that change, flickering artifacts was reported to be present with >>> both weston and wlroots wayland compositors when running in a virtual >>> machine. The cause was identified by Sima Vetter, who pointed out that >>> virtio-gpu does per-buffer uploads and for this reason it needs to do >>> a buffer damage handling, instead of frame damage handling. >> >> I'm having problem understanding the types of damage. You never say what >> buffer damage is. I also don't know what a frame is in this context. >> >> Regular damage handling marks parts of a plane as dirty/damaged. That is >> per-plane damage handling. The individual planes more or less >> independent from each other. >> >> Buffer damage, I guess, marks the underlying buffer as dirty and >> requires synchronization of the buffer with some backing storage. The >> planes using that buffer are then updated more or less automatically. >> >> Is that right? >> > > In both cases the damage tracking information is the same, they mark > the damaged regions on the plane in framebuffer coordinates of the > framebuffer attached to the plane. > > The problem as far as I understand is whether the driver expects that > to determine the area that changed in the plane (and a plane flush is > enough) or the area that changed since that same buffer was last used. > >> And why does it flicker? Is there old data stored somewhere? >> > > It flickers because the framebuffer changed and so the damage tracking > is not used correctly to flush the damaged areas to the backing storage. I think I got it from the links in patch 5. In out other drivers, there's a single backing storage for each plane (for example in the video memory). Here, there's a backing storage for each buffer. On page flips, the plane changes its backing storage. Our GEM buffer is up to date, but the respective backing storage is missing all the intermediate changes. If I'm not mistaken, an entirely different solution would be to implement a per-plane back storage in these drivers. Best regards Thomas > > This is my understanding at least, please Sima or Simon correct me if I > got this wrong. > >> Best regards >> Thomas >> >
Thomas Zimmermann <tzimmermann@suse.de> writes: Hello Thomas, > Hi [...] >>> And why does it flicker? Is there old data stored somewhere? >>> >> >> It flickers because the framebuffer changed and so the damage tracking >> is not used correctly to flush the damaged areas to the backing storage. > > I think I got it from the links in patch 5. In out other drivers, > there's a single backing storage for each plane (for example in the > video memory). Here, there's a backing storage for each buffer. On page Correct, that's what I understood too. > flips, the plane changes its backing storage. Our GEM buffer is up to > date, but the respective backing storage is missing all the intermediate > changes. > > If I'm not mistaken, an entirely different solution would be to > implement a per-plane back storage in these drivers. > I believe so but I'm not sure if that's possible since the virtio-gpu spec defines that the VM should send a VIRTIO_GPU_CMD_RESOURCE_FLUSH to the VMM in the host to do an update and the granularity for that is a framebuffer. For that reason the only solution (other than forcing a full plane update like this patch-set does) is to implement tracking suppor for buffer damage. > Best regards > Thomas >
Hello, This series is to fix an issue that surfaced after damage clipping was enabled for the virtio-gpu by commit 01f05940a9a7 ("drm/virtio: Enable fb damage clips property for the primary plane"). After that change, flickering artifacts was reported to be present with both weston and wlroots wayland compositors when running in a virtual machine. The cause was identified by Sima Vetter, who pointed out that virtio-gpu does per-buffer uploads and for this reason it needs to do a buffer damage handling, instead of frame damage handling. Their suggestion was to extend the damage helpers to cover that case and given that there's isn't a buffer damage accumulation algorithm (e.g: buffer age), just do a full plane update if the framebuffer that is attached to a plane changed since the last plane update (page-flip). Patch #1 is just a refactoring to allow the logic of the frame damage helpers to be shared by the buffer damage helpers. Patch #2 adds the helpers that are needed for buffer damage handling. Patch #3 fixes the virtio-gpu damage handling logic by using the helper that is required by drivers that need to handle buffer damage. Patch #4 fixes the vmwgfx similarly, since that driver also needs to handle buffer damage and should have the same issue (although I have not tested it due not having a VMWare setup). Patch #5 adds to the KMS damage tracking kernel-doc some paragraphs about damage tracking types and references to links that explain frame damage vs buffer damage. Finally patch #6 adds an item to the DRM/KMS todo, about the need to implement some buffer damage accumulation algorithm instead of just doing a full plane update in this case. Because commit 01f05940a9a7 landed in v6.4, the first three patches are marked as Fixes and Cc stable. I've tested this on a VM with weston, was able to reproduce the issue reported and the patches did fix the problem. Please let me know what you think. Specially on the wording since could made mistakes due just learning about these concepts yesterday thanks to Sima, Simon and Pekka. Best regards, Javier Javier Martinez Canillas (6): drm: Move drm_atomic_helper_damage_{iter_init,merged}() to helpers drm: Add drm_atomic_helper_buffer_damage_{iter_init,merged}() helpers drm/virtio: Use drm_atomic_helper_buffer_damage_merged() for buffer damage drm/vmwgfx: Use drm_atomic_helper_buffer_damage_iter_init() for buffer damage drm/plane: Extend damage tracking kernel-doc drm/todo: Add entry about implementing buffer age for damage tracking Documentation/gpu/todo.rst | 20 +++ drivers/gpu/drm/drm_damage_helper.c | 166 +++++++++++++++++++------ drivers/gpu/drm/drm_plane.c | 22 +++- drivers/gpu/drm/virtio/virtgpu_plane.c | 2 +- drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 2 +- include/drm/drm_damage_helper.h | 7 ++ 6 files changed, 173 insertions(+), 46 deletions(-)