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

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

[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(-)

-- 
2.41.0

[PATCH v2 1/5] drm: Allow drivers to indicate the damage helpers to ignore damage clips

[Javier Martinez Canillas]

It allows drivers to set a struct drm_plane_state .ignore_damage_clips in
their plane's .atomic_check callback, as an indication to damage helpers
such as drm_atomic_helper_damage_iter_init() that the damage clips should
be ignored.

To be used by drivers that do per-buffer (e.g: virtio-gpu) uploads (rather
than per-plane uploads), since these type of drivers need to handle buffer
damages instead of frame damages.

That way, these drivers could force a full plane update if the framebuffer
attached to a plane's state has changed since the last update (page-flip).

Fixes: 01f05940a9a7 ("drm/virtio: Enable fb damage clips property for the primary plane")
Cc: <stable@vger.kernel.org> # v6.4+
Reported-by: nerdopolis <bluescreen_avenger@verizon.net>
Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218115
Suggested-by: Thomas Zimmermann <tzimmermann@suse.de>
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
---

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

 drivers/gpu/drm/drm_damage_helper.c | 3 ++-
 include/drm/drm_plane.h             | 8 ++++++++
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_damage_helper.c b/drivers/gpu/drm/drm_damage_helper.c
index d8b2955e88fd..afb02aae707b 100644
--- a/drivers/gpu/drm/drm_damage_helper.c
+++ b/drivers/gpu/drm/drm_damage_helper.c
@@ -241,7 +241,8 @@ drm_atomic_helper_damage_iter_init(struct drm_atomic_helper_damage_iter *iter,
 	iter->plane_src.x2 = (src.x2 >> 16) + !!(src.x2 & 0xFFFF);
 	iter->plane_src.y2 = (src.y2 >> 16) + !!(src.y2 & 0xFFFF);
 
-	if (!iter->clips || !drm_rect_equals(&state->src, &old_state->src)) {
+	if (!iter->clips || state->ignore_damage_clips ||
+	    !drm_rect_equals(&state->src, &old_state->src)) {
 		iter->clips = NULL;
 		iter->num_clips = 0;
 		iter->full_update = true;
diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
index 79d62856defb..cc2e8fc35fd2 100644
--- a/include/drm/drm_plane.h
+++ b/include/drm/drm_plane.h
@@ -190,6 +190,14 @@ struct drm_plane_state {
 	 */
 	struct drm_property_blob *fb_damage_clips;
 
+	/**
+	 * @ignore_damage_clips:
+	 *
+	 * Set by drivers to indicate the drm_atomic_helper_damage_iter_init()
+	 * helper that the @fb_damage_clips blob property should be ignored.
+	 */
+	bool ignore_damage_clips;
+
 	/**
 	 * @src:
 	 *
-- 
2.41.0

[PATCH v2 2/5] drm/virtio: Disable damage clipping if FB changed since last page-flip

[Javier Martinez Canillas]

The driver does per-buffer uploads and needs to force a full plane update
if the plane's attached framebuffer has change since the last page-flip.

Fixes: 01f05940a9a7 ("drm/virtio: Enable fb damage clips property for the primary plane")
Cc: <stable@vger.kernel.org> # v6.4+
Reported-by: nerdopolis <bluescreen_avenger@verizon.net>
Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218115
Suggested-by: Sima Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
---

Changes in v2:
- Set struct drm_plane_state .ignore_damage_clips in virtio-gpu plane's
  .atomic_check instead of using a different helpers (Thomas Zimmermann).

 drivers/gpu/drm/virtio/virtgpu_plane.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/gpu/drm/virtio/virtgpu_plane.c b/drivers/gpu/drm/virtio/virtgpu_plane.c
index a2e045f3a000..a1ef657eba07 100644
--- a/drivers/gpu/drm/virtio/virtgpu_plane.c
+++ b/drivers/gpu/drm/virtio/virtgpu_plane.c
@@ -79,6 +79,8 @@ static int virtio_gpu_plane_atomic_check(struct drm_plane *plane,
 {
 	struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(state,
 										 plane);
+	struct drm_plane_state *old_plane_state = drm_atomic_get_old_plane_state(state,
+										 plane);
 	bool is_cursor = plane->type == DRM_PLANE_TYPE_CURSOR;
 	struct drm_crtc_state *crtc_state;
 	int ret;
@@ -86,6 +88,14 @@ static int virtio_gpu_plane_atomic_check(struct drm_plane *plane,
 	if (!new_plane_state->fb || WARN_ON(!new_plane_state->crtc))
 		return 0;
 
+	/*
+	 * Ignore damage clips if the framebuffer attached to the plane's state
+	 * has changed since the last plane update (page-flip). In this case, a
+	 * full plane update should happen because uploads are done per-buffer.
+	 */
+	if (old_plane_state->fb != new_plane_state->fb)
+		new_plane_state->ignore_damage_clips = true;
+
 	crtc_state = drm_atomic_get_crtc_state(state,
 					       new_plane_state->crtc);
 	if (IS_ERR(crtc_state))
-- 
2.41.0

[PATCH v2 3/5] drm/vmwgfx: Disable damage clipping if FB changed since last page-flip

[Javier Martinez Canillas]

The driver does per-buffer uploads and needs to force a full plane update
if the plane's attached framebuffer has change since the last page-flip.

Suggested-by: Sima Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
---

Changes in v2:
- Set struct drm_plane_state .ignore_damage_clips in vmwgfx plane's
  .atomic_check instead of using a different helpers (Thomas Zimmermann).

 drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
index 818b7f109f53..f9364bf222e3 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
@@ -837,10 +837,21 @@ int vmw_du_primary_plane_atomic_check(struct drm_plane *plane,
 {
 	struct drm_plane_state *new_state = drm_atomic_get_new_plane_state(state,
 									   plane);
+	struct drm_plane_state *old_state = drm_atomic_get_old_plane_state(state,
+									   plane);
 	struct drm_crtc_state *crtc_state = NULL;
 	struct drm_framebuffer *new_fb = new_state->fb;
+	struct drm_framebuffer *old_fb = old_state->fb;
 	int ret;
 
+	/*
+	 * Ignore damage clips if the framebuffer attached to the plane's state
+	 * has changed since the last plane update (page-flip). In this case, a
+	 * full plane update should happen because uploads are done per-buffer.
+	 */
+	if (old_fb != new_fb)
+		new_state->ignore_damage_clips = true;
+
 	if (new_state->crtc)
 		crtc_state = drm_atomic_get_new_crtc_state(state,
 							   new_state->crtc);
-- 
2.41.0

[PATCH v2 4/5] drm/plane: Extend damage tracking kernel-doc

[Javier Martinez Canillas]

The "Damage Tracking Properties" section in the documentation doesn't have
info about the two type of damage handling: frame damage vs buffer damage.

Add it to the section and mention that helpers only support frame damage,
and how drivers handling buffer damage can indicate that the damage clips
should be ignored.

Also add references to further documentation about the two damage types.

Suggested-by: Simon Ser <contact@emersion.fr>
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
---
- Dropped Simon Ser's Reviwed-by tag because the text changed to adapt
  to the approach Thomas Zimmermann suggested for v2.

(no changes since v1)

 drivers/gpu/drm/drm_plane.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
index 24e7998d1731..3b1b8bca3065 100644
--- a/drivers/gpu/drm/drm_plane.c
+++ b/drivers/gpu/drm/drm_plane.c
@@ -1442,6 +1442,26 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
  * Drivers implementing damage can use drm_atomic_helper_damage_iter_init() and
  * drm_atomic_helper_damage_iter_next() helper iterator function to get damage
  * rectangles clipped to &drm_plane_state.src.
+ *
+ * Note that there are two types of damage handling: frame damage and buffer
+ * damage. The type of damage handling implemented depends on a driver's upload
+ * target. Drivers implementing a per-plane or per-CRTC upload target need to
+ * handle frame damage while drivers implementing a per-buffer upload target
+ * need to handle buffer damage.
+ *
+ * The existing damage helpers only support the frame damage type, there is no
+ * buffer age support or similar damage accumulation algorithm implemented yet.
+ *
+ * Only drivers handling frame damage can use the mentiored damage helpers to
+ * iterate over the damaged regions. Drivers that handle buffer damage, need to
+ * set &struct drm_plane_state.ignore_damage_clips as an indication to
+ * drm_atomic_helper_damage_iter_init() that the damage clips should be ignored.
+ * In that case, the returned damage rectangle is the &drm_plane_state.src since
+ * a full plane update should happen.
+ *
+ * For more information about the two type of damage, see:
+ *     https://registry.khronos.org/EGL/extensions/KHR/EGL_KHR_swap_buffers_with_damage.txt
+ *     https://emersion.fr/blog/2019/intro-to-damage-tracking/
  */
 
 /**
-- 
2.41.0

[PATCH v2 5/5] drm/todo: Add entry about implementing buffer age for damage tracking

[Javier Martinez Canillas]

Currently, only damage tracking for frame damage is supported. If a driver
needs to do buffer damage (e.g: the framebuffer attached to plane's state
has changed since the last page-flip), the damage helpers just fallback to
a full plane update.

Add en entry in the TODO about implementing buffer age or any other damage
accumulation algorithm for buffer damage handling.

Suggested-by: Simon Ser <contact@emersion.fr>
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
Reviewed-by: Simon Ser <contact@emersion.fr>
---

(no changes since v1)

 Documentation/gpu/todo.rst | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst
index b62c7fa0c2bc..5c43a958814b 100644
--- a/Documentation/gpu/todo.rst
+++ b/Documentation/gpu/todo.rst
@@ -782,6 +782,26 @@ Contact: Hans de Goede
 
 Level: Advanced
 
+Buffer age or other damage accumulation algorithm for buffer damage handling
+============================================================================
+
+Drivers that do per-buffer uploads, need a buffer damage handling (rather than
+frame damage like drivers that do per-plane or per-CRTC uploads), but there is
+no support to get the buffer age or any other damage accumulation algorithm.
+
+For this reason, the damage helpers just fallback to a full plane update if the
+framebuffer attached to a plane has changed since the last page-flip.
+
+This should be improved to get damage tracking properly working on drivers that
+do per-buffer uploads.
+
+More information about damage tracking and references to learning materials in
+`Damage Tracking Properties <https://docs.kernel.org/gpu/drm-kms.html#damage-tracking-properties>`_
+
+Contact: Javier Martinez Canillas <javierm@redhat.com>
+
+Level: Advanced
+
 Outside DRM
 ===========
 
-- 
2.41.0

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

[Zack Rusin]

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

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

[Javier Martinez Canillas]

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

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH v2 4/5] drm/plane: Extend damage tracking kernel-doc

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 3415 bytes --]

Hi

Am 15.11.23 um 14:15 schrieb Javier Martinez Canillas:
> The "Damage Tracking Properties" section in the documentation doesn't have
> info about the two type of damage handling: frame damage vs buffer damage.
> 
> Add it to the section and mention that helpers only support frame damage,
> and how drivers handling buffer damage can indicate that the damage clips
> should be ignored.
> 
> Also add references to further documentation about the two damage types.
> 
> Suggested-by: Simon Ser <contact@emersion.fr>
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> ---
> - Dropped Simon Ser's Reviwed-by tag because the text changed to adapt
>    to the approach Thomas Zimmermann suggested for v2.
> 
> (no changes since v1)
> 
>   drivers/gpu/drm/drm_plane.c | 20 ++++++++++++++++++++
>   1 file changed, 20 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> index 24e7998d1731..3b1b8bca3065 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -1442,6 +1442,26 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
>    * Drivers implementing damage can use drm_atomic_helper_damage_iter_init() and
>    * drm_atomic_helper_damage_iter_next() helper iterator function to get damage
>    * rectangles clipped to &drm_plane_state.src.
> + *
> + * Note that there are two types of damage handling: frame damage and buffer
> + * damage. The type of damage handling implemented depends on a driver's upload
> + * target. Drivers implementing a per-plane or per-CRTC upload target need to
> + * handle frame damage while drivers implementing a per-buffer upload target
> + * need to handle buffer damage.
> + *
> + * The existing damage helpers only support the frame damage type, there is no
> + * buffer age support or similar damage accumulation algorithm implemented yet.
> + *
> + * Only drivers handling frame damage can use the mentiored damage helpers to
> + * iterate over the damaged regions. Drivers that handle buffer damage, need to
> + * set &struct drm_plane_state.ignore_damage_clips as an indication to
> + * drm_atomic_helper_damage_iter_init() that the damage clips should be ignored.
> + * In that case, the returned damage rectangle is the &drm_plane_state.src since
> + * a full plane update should happen.
> + *
> + * For more information about the two type of damage, see:
> + *     https://registry.khronos.org/EGL/extensions/KHR/EGL_KHR_swap_buffers_with_damage.txt
> + *     https://emersion.fr/blog/2019/intro-to-damage-tracking/

One thought you might want to consider.

These URLs are helpful. The only issue I have is that frame damage and 
buffer damage are user-space concepts. The kernel bug is that damage 
handling expects the backing storage/upload buffer not to change for a 
given plane. If the upload buffer changes between page flips, the new 
upload buffer has to be updated as a whole. Hence no damage handling then.

And then maybe say that the effects of frame damage and buffer damage in 
userspace illustrate this problem and give the URLs.

Best regards
Thomas

>    */
>   
>   /**

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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

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

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 3985 bytes --]

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)

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH v2 4/5] drm/plane: Extend damage tracking kernel-doc

[Simon Ser]

On Thursday, November 16th, 2023 at 13:06, Thomas Zimmermann <tzimmermann@suse.de> wrote:

> > + * Note that there are two types of damage handling: frame damage and buffer
> > + * damage. The type of damage handling implemented depends on a driver's upload
> > + * target. Drivers implementing a per-plane or per-CRTC upload target need to
> > + * handle frame damage while drivers implementing a per-buffer upload target
> > + * need to handle buffer damage.
> > + *
> > + * The existing damage helpers only support the frame damage type, there is no
> > + * buffer age support or similar damage accumulation algorithm implemented yet.
> > + *
> > + * Only drivers handling frame damage can use the mentiored damage helpers to

Typo: mentioned

> > + * iterate over the damaged regions. Drivers that handle buffer damage, need to
> > + * set &struct drm_plane_state.ignore_damage_clips as an indication to
> > + * drm_atomic_helper_damage_iter_init() that the damage clips should be ignored.
> > + * In that case, the returned damage rectangle is the &drm_plane_state.src since
> > + * a full plane update should happen.
> > + *
> > + * For more information about the two type of damage, see:
> > + * https://registry.khronos.org/EGL/extensions/KHR/EGL_KHR_swap_buffers_with_damage.txt
> > + * https://emersion.fr/blog/2019/intro-to-damage-tracking/
> 
> One thought you might want to consider.
> 
> These URLs are helpful. The only issue I have is that frame damage and
> buffer damage are user-space concepts. The kernel bug is that damage
> handling expects the backing storage/upload buffer not to change for a
> given plane. If the upload buffer changes between page flips, the new
> upload buffer has to be updated as a whole. Hence no damage handling then.

Why would these concepts be specific to user-space? The kernel could
better handle buffer damage instead of forcing full damage, by doing
something similar to what user-space does.

Anyways:

Reviewed-by: Simon Ser <contact@emersion.fr>

Re: [PATCH v2 4/5] drm/plane: Extend damage tracking kernel-doc

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 2559 bytes --]

Hi

Am 16.11.23 um 13:14 schrieb Simon Ser:
> On Thursday, November 16th, 2023 at 13:06, Thomas Zimmermann <tzimmermann@suse.de> wrote:
> 
>>> + * Note that there are two types of damage handling: frame damage and buffer
>>> + * damage. The type of damage handling implemented depends on a driver's upload
>>> + * target. Drivers implementing a per-plane or per-CRTC upload target need to
>>> + * handle frame damage while drivers implementing a per-buffer upload target
>>> + * need to handle buffer damage.
>>> + *
>>> + * The existing damage helpers only support the frame damage type, there is no
>>> + * buffer age support or similar damage accumulation algorithm implemented yet.
>>> + *
>>> + * Only drivers handling frame damage can use the mentiored damage helpers to
> 
> Typo: mentioned
> 
>>> + * iterate over the damaged regions. Drivers that handle buffer damage, need to
>>> + * set &struct drm_plane_state.ignore_damage_clips as an indication to
>>> + * drm_atomic_helper_damage_iter_init() that the damage clips should be ignored.
>>> + * In that case, the returned damage rectangle is the &drm_plane_state.src since
>>> + * a full plane update should happen.
>>> + *
>>> + * For more information about the two type of damage, see:
>>> + * https://registry.khronos.org/EGL/extensions/KHR/EGL_KHR_swap_buffers_with_damage.txt
>>> + * https://emersion.fr/blog/2019/intro-to-damage-tracking/
>>
>> One thought you might want to consider.
>>
>> These URLs are helpful. The only issue I have is that frame damage and
>> buffer damage are user-space concepts. The kernel bug is that damage
>> handling expects the backing storage/upload buffer not to change for a
>> given plane. If the upload buffer changes between page flips, the new
>> upload buffer has to be updated as a whole. Hence no damage handling then.
> 
> Why would these concepts be specific to user-space? The kernel could
> better handle buffer damage instead of forcing full damage, by doing
> something similar to what user-space does.

The terms 'frame damage' and 'buffer damage' do not exist in the kernel. 
The problem can be better described in wording that is common within the 
context of the kernel drivers.

Best regards
Thomas

> 
> Anyways:
> 
> Reviewed-by: Simon Ser <contact@emersion.fr>

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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH v2 4/5] drm/plane: Extend damage tracking kernel-doc

[Pekka Paalanen]

[-- Attachment #1: Type: text/plain, Size: 2519 bytes --]

On Thu, 16 Nov 2023 13:34:07 +0100
Thomas Zimmermann <tzimmermann@suse.de> wrote:

> Hi
> 
> Am 16.11.23 um 13:14 schrieb Simon Ser:
> > On Thursday, November 16th, 2023 at 13:06, Thomas Zimmermann <tzimmermann@suse.de> wrote:
> >   
> >>> + * Note that there are two types of damage handling: frame damage and buffer
> >>> + * damage. The type of damage handling implemented depends on a driver's upload
> >>> + * target. Drivers implementing a per-plane or per-CRTC upload target need to
> >>> + * handle frame damage while drivers implementing a per-buffer upload target
> >>> + * need to handle buffer damage.
> >>> + *
> >>> + * The existing damage helpers only support the frame damage type, there is no
> >>> + * buffer age support or similar damage accumulation algorithm implemented yet.
> >>> + *
> >>> + * Only drivers handling frame damage can use the mentiored damage helpers to  
> > 
> > Typo: mentioned
> >   
> >>> + * iterate over the damaged regions. Drivers that handle buffer damage, need to
> >>> + * set &struct drm_plane_state.ignore_damage_clips as an indication to
> >>> + * drm_atomic_helper_damage_iter_init() that the damage clips should be ignored.
> >>> + * In that case, the returned damage rectangle is the &drm_plane_state.src since
> >>> + * a full plane update should happen.
> >>> + *
> >>> + * For more information about the two type of damage, see:
> >>> + * https://registry.khronos.org/EGL/extensions/KHR/EGL_KHR_swap_buffers_with_damage.txt
> >>> + * https://emersion.fr/blog/2019/intro-to-damage-tracking/  
> >>
> >> One thought you might want to consider.
> >>
> >> These URLs are helpful. The only issue I have is that frame damage and
> >> buffer damage are user-space concepts. The kernel bug is that damage
> >> handling expects the backing storage/upload buffer not to change for a
> >> given plane. If the upload buffer changes between page flips, the new
> >> upload buffer has to be updated as a whole. Hence no damage handling then.  
> > 
> > Why would these concepts be specific to user-space? The kernel could
> > better handle buffer damage instead of forcing full damage, by doing
> > something similar to what user-space does.  
> 
> The terms 'frame damage' and 'buffer damage' do not exist in the kernel. 
> The problem can be better described in wording that is common within the 
> context of the kernel drivers.

What terms does the kernel use for these two different concepts of
damage?


Thanks,
pq

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 4/5] drm/plane: Extend damage tracking kernel-doc

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 3008 bytes --]

Hi

Am 16.11.23 um 16:24 schrieb Pekka Paalanen:
> On Thu, 16 Nov 2023 13:34:07 +0100
> Thomas Zimmermann <tzimmermann@suse.de> wrote:
> 
>> Hi
>>
>> Am 16.11.23 um 13:14 schrieb Simon Ser:
>>> On Thursday, November 16th, 2023 at 13:06, Thomas Zimmermann <tzimmermann@suse.de> wrote:
>>>    
>>>>> + * Note that there are two types of damage handling: frame damage and buffer
>>>>> + * damage. The type of damage handling implemented depends on a driver's upload
>>>>> + * target. Drivers implementing a per-plane or per-CRTC upload target need to
>>>>> + * handle frame damage while drivers implementing a per-buffer upload target
>>>>> + * need to handle buffer damage.
>>>>> + *
>>>>> + * The existing damage helpers only support the frame damage type, there is no
>>>>> + * buffer age support or similar damage accumulation algorithm implemented yet.
>>>>> + *
>>>>> + * Only drivers handling frame damage can use the mentiored damage helpers to
>>>
>>> Typo: mentioned
>>>    
>>>>> + * iterate over the damaged regions. Drivers that handle buffer damage, need to
>>>>> + * set &struct drm_plane_state.ignore_damage_clips as an indication to
>>>>> + * drm_atomic_helper_damage_iter_init() that the damage clips should be ignored.
>>>>> + * In that case, the returned damage rectangle is the &drm_plane_state.src since
>>>>> + * a full plane update should happen.
>>>>> + *
>>>>> + * For more information about the two type of damage, see:
>>>>> + * https://registry.khronos.org/EGL/extensions/KHR/EGL_KHR_swap_buffers_with_damage.txt
>>>>> + * https://emersion.fr/blog/2019/intro-to-damage-tracking/
>>>>
>>>> One thought you might want to consider.
>>>>
>>>> These URLs are helpful. The only issue I have is that frame damage and
>>>> buffer damage are user-space concepts. The kernel bug is that damage
>>>> handling expects the backing storage/upload buffer not to change for a
>>>> given plane. If the upload buffer changes between page flips, the new
>>>> upload buffer has to be updated as a whole. Hence no damage handling then.
>>>
>>> Why would these concepts be specific to user-space? The kernel could
>>> better handle buffer damage instead of forcing full damage, by doing
>>> something similar to what user-space does.
>>
>> The terms 'frame damage' and 'buffer damage' do not exist in the kernel.
>> The problem can be better described in wording that is common within the
>> context of the kernel drivers.
> 
> What terms does the kernel use for these two different concepts of
> damage?

None AFAIK. Damage is relative to the plane's backing storage/upload 
buffer. That's frame damage. We never needed a name for buffer damage as 
we don't support it.

Best regards
Thomas

> 
> 
> Thanks,
> pq

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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]