[v2,0/5] drm: Allow the damage helpers to handle buffer damage

  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).

It is a v2 that addresses issues pointed out by Thomas Zimmermann in v1:
https://lists.freedesktop.org/archives/dri-devel/2023-November/430138.html

Patch #1 adds a ignore_damage_clips field to struct drm_plane_state to be
set by drivers that want the damage helpers to ignore the damage clips.

Patch #2 fixes the virtio-gpu damage handling logic by asking the damage
helper to ignore the damage clips if the framebuffer attached to a plane
has changed since the last page-flip.

Patch #3 does the same but for the vmwgfx driver that also needs to handle
buffer damage and should have the same issue (although I haven't tested it
due not having a VMWare setup).

Patch #4 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 #5 adds an item to the DRM todo, about the need to implement
some buffer damage accumulation algorithm instead of just doing full plane
updates in this case.

Because commit 01f05940a9a7 landed in v6.4, the first 2 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.

Best regards,
Javier

Changes in v2:
- Add a struct drm_plane_state .ignore_damage_clips to set in the plane's
  .atomic_check, instead of having different helpers (Thomas Zimmermann).
- Set struct drm_plane_state .ignore_damage_clips in virtio-gpu plane's
  .atomic_check instead of using a different helpers (Thomas Zimmermann).
- Set struct drm_plane_state .ignore_damage_clips in vmwgfx plane's
  .atomic_check instead of using a different helpers (Thomas Zimmermann).

Javier Martinez Canillas (5):
  drm: Allow drivers to indicate the damage helpers to ignore damage
    clips
  drm/virtio: Disable damage clipping if FB changed since last page-flip
  drm/vmwgfx: Disable damage clipping if FB changed since last page-flip
  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    |  3 ++-
 drivers/gpu/drm/drm_plane.c            | 20 ++++++++++++++++++++
 drivers/gpu/drm/virtio/virtgpu_plane.c | 10 ++++++++++
 drivers/gpu/drm/vmwgfx/vmwgfx_kms.c    | 11 +++++++++++
 include/drm/drm_plane.h                |  8 ++++++++
 6 files changed, 71 insertions(+), 1 deletion(-)
  

On Wed, 2023-11-15 at 14:15 +0100, Javier Martinez Canillas wrote:
> 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).
>
> It is a v2 that addresses issues pointed out by Thomas Zimmermann in v1:
> https://lists.freedesktop.org/archives/dri-devel/2023-November/430138.html
>
> Patch #1 adds a ignore_damage_clips field to struct drm_plane_state to be
> set by drivers that want the damage helpers to ignore the damage clips.
>
> Patch #2 fixes the virtio-gpu damage handling logic by asking the damage
> helper to ignore the damage clips if the framebuffer attached to a plane
> has changed since the last page-flip.
>
> Patch #3 does the same but for the vmwgfx driver that also needs to handle
> buffer damage and should have the same issue (although I haven't tested it
> due not having a VMWare setup).
>
> Patch #4 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 #5 adds an item to the DRM todo, about the need to implement
> some buffer damage accumulation algorithm instead of just doing full plane
> updates in this case.
>
> Because commit 01f05940a9a7 landed in v6.4, the first 2 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.
>
> Best regards,
> Javier
>
> Changes in v2:
> - Add a struct drm_plane_state .ignore_damage_clips to set in the plane's
>   .atomic_check, instead of having different helpers (Thomas Zimmermann).
> - Set struct drm_plane_state .ignore_damage_clips in virtio-gpu plane's
>   .atomic_check instead of using a different helpers (Thomas Zimmermann).
> - Set struct drm_plane_state .ignore_damage_clips in vmwgfx plane's
>   .atomic_check instead of using a different helpers (Thomas Zimmermann).

The series looks good to me, thanks for tackling this. I'm surprised that we don't
have any IGT tests for this. Seems like it shouldn't be too hard to test it in a
generic way with just a couple of dumb buffers.

z
  

Zack Rusin <zackr@vmware.com> writes:

Hello Zack,

> On Wed, 2023-11-15 at 14:15 +0100, Javier Martinez Canillas wrote:

[...]

>>
>> Changes in v2:
>> - Add a struct drm_plane_state .ignore_damage_clips to set in the plane's
>>   .atomic_check, instead of having different helpers (Thomas Zimmermann).
>> - Set struct drm_plane_state .ignore_damage_clips in virtio-gpu plane's
>>   .atomic_check instead of using a different helpers (Thomas Zimmermann).
>> - Set struct drm_plane_state .ignore_damage_clips in vmwgfx plane's
>>   .atomic_check instead of using a different helpers (Thomas Zimmermann).
>
> The series looks good to me, thanks for tackling this. I'm surprised that we don't

Thanks. Can I get your r-b or a-b ?

> have any IGT tests for this. Seems like it shouldn't be too hard to test it in a
> generic way with just a couple of dumb buffers.
>

Yes, I haven't looked at it but also think that shouldn't be that hard.

> z
  

Hi Javier,

please take my

Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>

for the whole patch set. Maybe consider my comment on patch 4.

Best regards
Thomas

Am 15.11.23 um 14:15 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.
> 
> 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).
> 
> It is a v2 that addresses issues pointed out by Thomas Zimmermann in v1:
> https://lists.freedesktop.org/archives/dri-devel/2023-November/430138.html
> 
> Patch #1 adds a ignore_damage_clips field to struct drm_plane_state to be
> set by drivers that want the damage helpers to ignore the damage clips.
> 
> Patch #2 fixes the virtio-gpu damage handling logic by asking the damage
> helper to ignore the damage clips if the framebuffer attached to a plane
> has changed since the last page-flip.
> 
> Patch #3 does the same but for the vmwgfx driver that also needs to handle
> buffer damage and should have the same issue (although I haven't tested it
> due not having a VMWare setup).
> 
> Patch #4 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 #5 adds an item to the DRM todo, about the need to implement
> some buffer damage accumulation algorithm instead of just doing full plane
> updates in this case.
> 
> Because commit 01f05940a9a7 landed in v6.4, the first 2 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.
> 
> Best regards,
> Javier
> 
> Changes in v2:
> - Add a struct drm_plane_state .ignore_damage_clips to set in the plane's
>    .atomic_check, instead of having different helpers (Thomas Zimmermann).
> - Set struct drm_plane_state .ignore_damage_clips in virtio-gpu plane's
>    .atomic_check instead of using a different helpers (Thomas Zimmermann).
> - Set struct drm_plane_state .ignore_damage_clips in vmwgfx plane's
>    .atomic_check instead of using a different helpers (Thomas Zimmermann).
> 
> Javier Martinez Canillas (5):
>    drm: Allow drivers to indicate the damage helpers to ignore damage
>      clips
>    drm/virtio: Disable damage clipping if FB changed since last page-flip
>    drm/vmwgfx: Disable damage clipping if FB changed since last page-flip
>    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    |  3 ++-
>   drivers/gpu/drm/drm_plane.c            | 20 ++++++++++++++++++++
>   drivers/gpu/drm/virtio/virtgpu_plane.c | 10 ++++++++++
>   drivers/gpu/drm/vmwgfx/vmwgfx_kms.c    | 11 +++++++++++
>   include/drm/drm_plane.h                |  8 ++++++++
>   6 files changed, 71 insertions(+), 1 deletion(-)
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)