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

[PATCH 0/6] 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).

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

-- 
2.41.0

[PATCH 1/6] drm: Move drm_atomic_helper_damage_{iter_init,merged}() to helpers

[Javier Martinez Canillas]

We need a similar drm_atomic_helper_buffer_damage_merged() helper function
that takes into account if a framebuffer attached to the plane has changed
since the last plane update (page-flip).

Since both damage helpers will share most of the current logic, move it to
an internal helper. The drm_atomic_helper_buffer_damage_merged() will have
to use a different drm_atomic_helper_buffer_damage_iter_init() function so
move that logic also to an internal helper.

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

 drivers/gpu/drm/drm_damage_helper.c | 95 +++++++++++++++++------------
 1 file changed, 55 insertions(+), 40 deletions(-)

diff --git a/drivers/gpu/drm/drm_damage_helper.c b/drivers/gpu/drm/drm_damage_helper.c
index d8b2955e88fd..aa2325567918 100644
--- a/drivers/gpu/drm/drm_damage_helper.c
+++ b/drivers/gpu/drm/drm_damage_helper.c
@@ -201,28 +201,10 @@ int drm_atomic_helper_dirtyfb(struct drm_framebuffer *fb,
 }
 EXPORT_SYMBOL(drm_atomic_helper_dirtyfb);
 
-/**
- * drm_atomic_helper_damage_iter_init - Initialize the damage iterator.
- * @iter: The iterator to initialize.
- * @old_state: Old plane state for validation.
- * @state: Plane state from which to iterate the damage clips.
- *
- * Initialize an iterator, which clips plane damage
- * &drm_plane_state.fb_damage_clips to plane &drm_plane_state.src. This iterator
- * returns full plane src in case damage is not present because either
- * user-space didn't sent or driver discarded it (it want to do full plane
- * update). Currently this iterator returns full plane src in case plane src
- * changed but that can be changed in future to return damage.
- *
- * For the case when plane is not visible or plane update should not happen the
- * first call to iter_next will return false. Note that this helper use clipped
- * &drm_plane_state.src, so driver calling this helper should have called
- * drm_atomic_helper_check_plane_state() earlier.
- */
-void
-drm_atomic_helper_damage_iter_init(struct drm_atomic_helper_damage_iter *iter,
-				   const struct drm_plane_state *old_state,
-				   const struct drm_plane_state *state)
+static void
+__drm_atomic_helper_damage_iter_init(struct drm_atomic_helper_damage_iter *iter,
+				     const struct drm_plane_state *old_state,
+				     const struct drm_plane_state *state)
 {
 	struct drm_rect src;
 	memset(iter, 0, sizeof(*iter));
@@ -247,6 +229,32 @@ drm_atomic_helper_damage_iter_init(struct drm_atomic_helper_damage_iter *iter,
 		iter->full_update = true;
 	}
 }
+
+/**
+ * drm_atomic_helper_damage_iter_init - Initialize the damage iterator.
+ * @iter: The iterator to initialize.
+ * @old_state: Old plane state for validation.
+ * @state: Plane state from which to iterate the damage clips.
+ *
+ * Initialize an iterator, which clips plane damage
+ * &drm_plane_state.fb_damage_clips to plane &drm_plane_state.src. This iterator
+ * returns full plane src in case damage is not present because either
+ * user-space didn't sent or driver discarded it (it want to do full plane
+ * update). Currently this iterator returns full plane src in case plane src
+ * changed but that can be changed in future to return damage.
+ *
+ * For the case when plane is not visible or plane update should not happen the
+ * first call to iter_next will return false. Note that this helper use clipped
+ * &drm_plane_state.src, so driver calling this helper should have called
+ * drm_atomic_helper_check_plane_state() earlier.
+ */
+void
+drm_atomic_helper_damage_iter_init(struct drm_atomic_helper_damage_iter *iter,
+				   const struct drm_plane_state *old_state,
+				   const struct drm_plane_state *state)
+{
+	__drm_atomic_helper_damage_iter_init(iter, old_state, state);
+}
 EXPORT_SYMBOL(drm_atomic_helper_damage_iter_init);
 
 /**
@@ -291,24 +299,9 @@ drm_atomic_helper_damage_iter_next(struct drm_atomic_helper_damage_iter *iter,
 }
 EXPORT_SYMBOL(drm_atomic_helper_damage_iter_next);
 
-/**
- * drm_atomic_helper_damage_merged - Merged plane damage
- * @old_state: Old plane state for validation.
- * @state: Plane state from which to iterate the damage clips.
- * @rect: Returns the merged damage rectangle
- *
- * This function merges any valid plane damage clips into one rectangle and
- * returns it in @rect.
- *
- * For details see: drm_atomic_helper_damage_iter_init() and
- * drm_atomic_helper_damage_iter_next().
- *
- * Returns:
- * True if there is valid plane damage otherwise false.
- */
-bool drm_atomic_helper_damage_merged(const struct drm_plane_state *old_state,
-				     struct drm_plane_state *state,
-				     struct drm_rect *rect)
+static bool __drm_atomic_helper_damage_merged(const struct drm_plane_state *old_state,
+					      struct drm_plane_state *state,
+					      struct drm_rect *rect)
 {
 	struct drm_atomic_helper_damage_iter iter;
 	struct drm_rect clip;
@@ -330,4 +323,26 @@ bool drm_atomic_helper_damage_merged(const struct drm_plane_state *old_state,
 
 	return valid;
 }
+
+/**
+ * drm_atomic_helper_damage_merged - Merged plane damage
+ * @old_state: Old plane state for validation.
+ * @state: Plane state from which to iterate the damage clips.
+ * @rect: Returns the merged damage rectangle
+ *
+ * This function merges any valid plane damage clips into one rectangle and
+ * returns it in @rect.
+ *
+ * For details see: drm_atomic_helper_damage_iter_init() and
+ * drm_atomic_helper_damage_iter_next().
+ *
+ * Returns:
+ * True if there is valid plane damage otherwise false.
+ */
+bool drm_atomic_helper_damage_merged(const struct drm_plane_state *old_state,
+				     struct drm_plane_state *state,
+				     struct drm_rect *rect)
+{
+	return __drm_atomic_helper_damage_merged(old_state, state, rect);
+}
 EXPORT_SYMBOL(drm_atomic_helper_damage_merged);
-- 
2.41.0

[PATCH 2/6] drm: Add drm_atomic_helper_buffer_damage_{iter_init,merged}() helpers

[Javier Martinez Canillas]

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.

The drm_atomic_helper_buffer_damage_iter_init() has the same logic than
drm_atomic_helper_damage_iter_init() but it also takes into account if the
framebuffer attached to plane's state has changed since the last update.

And the drm_atomic_helper_buffer_damage_merged() is just a version of the
drm_atomic_helper_damage_merged() helper, but it uses the iter_init helper
that is mentioned above.

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

 drivers/gpu/drm/drm_damage_helper.c | 79 ++++++++++++++++++++++++++---
 include/drm/drm_damage_helper.h     |  7 +++
 2 files changed, 80 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/drm_damage_helper.c b/drivers/gpu/drm/drm_damage_helper.c
index aa2325567918..b72062c9d31c 100644
--- a/drivers/gpu/drm/drm_damage_helper.c
+++ b/drivers/gpu/drm/drm_damage_helper.c
@@ -204,7 +204,8 @@ EXPORT_SYMBOL(drm_atomic_helper_dirtyfb);
 static void
 __drm_atomic_helper_damage_iter_init(struct drm_atomic_helper_damage_iter *iter,
 				     const struct drm_plane_state *old_state,
-				     const struct drm_plane_state *state)
+				     const struct drm_plane_state *state,
+				     bool buffer_damage)
 {
 	struct drm_rect src;
 	memset(iter, 0, sizeof(*iter));
@@ -223,7 +224,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 || !drm_rect_equals(&state->src, &old_state->src) ||
+	    (buffer_damage && old_state->fb != state->fb)) {
 		iter->clips = NULL;
 		iter->num_clips = 0;
 		iter->full_update = true;
@@ -243,6 +245,10 @@ __drm_atomic_helper_damage_iter_init(struct drm_atomic_helper_damage_iter *iter,
  * update). Currently this iterator returns full plane src in case plane src
  * changed but that can be changed in future to return damage.
  *
+ * Note that this helper is for drivers that do per-plane uploads and expect
+ * to handle frame damages. Drivers that do per-buffer uploads instead should
+ * use @drm_atomic_helper_buffer_damage_iter_init() that handles buffer damages.
+ *
  * For the case when plane is not visible or plane update should not happen the
  * first call to iter_next will return false. Note that this helper use clipped
  * &drm_plane_state.src, so driver calling this helper should have called
@@ -253,10 +259,37 @@ drm_atomic_helper_damage_iter_init(struct drm_atomic_helper_damage_iter *iter,
 				   const struct drm_plane_state *old_state,
 				   const struct drm_plane_state *state)
 {
-	__drm_atomic_helper_damage_iter_init(iter, old_state, state);
+	__drm_atomic_helper_damage_iter_init(iter, old_state, state, false);
 }
 EXPORT_SYMBOL(drm_atomic_helper_damage_iter_init);
 
+/**
+ * drm_atomic_helper_buffer_damage_iter_init - Initialize the buffer damage iterator.
+ * @iter: The iterator to initialize.
+ * @old_state: Old plane state for validation.
+ * @state: Plane state from which to iterate the damage clips.
+ *
+ * Initialize an iterator, which clips buffer damage
+ * &drm_plane_state.fb_damage_clips to plane &drm_plane_state.src. This iterator
+ * returns full plane src in case buffer damage is not present because user-space
+ * didn't sent, driver discarded it (it want to do full plane update) or the plane
+ * @state has an attached framebuffer that is different than the one in @state (it
+ * has changed since the last plane update).
+ *
+ * For the case when plane is not visible or plane update should not happen the
+ * first call to iter_next will return false. Note that this helper use clipped
+ * &drm_plane_state.src, so driver calling this helper should have called
+ * drm_atomic_helper_check_plane_state() earlier.
+ */
+void
+drm_atomic_helper_buffer_damage_iter_init(struct drm_atomic_helper_damage_iter *iter,
+					  const struct drm_plane_state *old_state,
+					  const struct drm_plane_state *state)
+{
+	__drm_atomic_helper_damage_iter_init(iter, old_state, state, true);
+}
+EXPORT_SYMBOL(drm_atomic_helper_buffer_damage_iter_init);
+
 /**
  * drm_atomic_helper_damage_iter_next - Advance the damage iterator.
  * @iter: The iterator to advance.
@@ -301,7 +334,8 @@ EXPORT_SYMBOL(drm_atomic_helper_damage_iter_next);
 
 static bool __drm_atomic_helper_damage_merged(const struct drm_plane_state *old_state,
 					      struct drm_plane_state *state,
-					      struct drm_rect *rect)
+					      struct drm_rect *rect,
+					      bool buffer_damage)
 {
 	struct drm_atomic_helper_damage_iter iter;
 	struct drm_rect clip;
@@ -312,7 +346,7 @@ static bool __drm_atomic_helper_damage_merged(const struct drm_plane_state *old_
 	rect->x2 = 0;
 	rect->y2 = 0;
 
-	drm_atomic_helper_damage_iter_init(&iter, old_state, state);
+	__drm_atomic_helper_damage_iter_init(&iter, old_state, state, buffer_damage);
 	drm_atomic_for_each_plane_damage(&iter, &clip) {
 		rect->x1 = min(rect->x1, clip.x1);
 		rect->y1 = min(rect->y1, clip.y1);
@@ -336,6 +370,10 @@ static bool __drm_atomic_helper_damage_merged(const struct drm_plane_state *old_
  * For details see: drm_atomic_helper_damage_iter_init() and
  * drm_atomic_helper_damage_iter_next().
  *
+ * Note that this helper is for drivers that do per-plane uploads and expect
+ * to handle frame damages. Drivers that do per-buffer uploads instead should
+ * use @drm_atomic_helper_buffer_damage_merged() that handles buffer damages.
+ *
  * Returns:
  * True if there is valid plane damage otherwise false.
  */
@@ -343,6 +381,35 @@ bool drm_atomic_helper_damage_merged(const struct drm_plane_state *old_state,
 				     struct drm_plane_state *state,
 				     struct drm_rect *rect)
 {
-	return __drm_atomic_helper_damage_merged(old_state, state, rect);
+	return __drm_atomic_helper_damage_merged(old_state, state, rect, false);
 }
 EXPORT_SYMBOL(drm_atomic_helper_damage_merged);
+
+/**
+ * drm_atomic_helper_buffer_damage_merged - Merged buffer damage
+ * @old_state: Old plane state for validation.
+ * @state: Plane state from which to iterate the damage clips.
+ * @rect: Returns the merged buffer damage rectangle
+ *
+ * This function merges any valid buffer damage clips into one rectangle and
+ * returns it in @rect. It checks if the framebuffers attached to @old_state
+ * and @state are the same. If that is not the case then the returned damage
+ * rectangle is the &drm_plane_state.src, since a full update should happen.
+ *
+ * Note that &drm_plane_state.fb_damage_clips == NULL in plane state means that
+ * full plane update should happen. It also ensure helper iterator will return
+ * &drm_plane_state.src as damage.
+ *
+ * For details see: drm_atomic_helper_buffer_damage_iter_init() and
+ * drm_atomic_helper_damage_iter_next().
+ *
+ * Returns:
+ * True if there is valid buffer damage otherwise false.
+ */
+bool drm_atomic_helper_buffer_damage_merged(const struct drm_plane_state *old_state,
+					    struct drm_plane_state *state,
+					    struct drm_rect *rect)
+{
+	return __drm_atomic_helper_damage_merged(old_state, state, rect, true);
+}
+EXPORT_SYMBOL(drm_atomic_helper_buffer_damage_merged);
diff --git a/include/drm/drm_damage_helper.h b/include/drm/drm_damage_helper.h
index effda42cce31..328bb249d68f 100644
--- a/include/drm/drm_damage_helper.h
+++ b/include/drm/drm_damage_helper.h
@@ -74,11 +74,18 @@ void
 drm_atomic_helper_damage_iter_init(struct drm_atomic_helper_damage_iter *iter,
 				   const struct drm_plane_state *old_state,
 				   const struct drm_plane_state *new_state);
+void
+drm_atomic_helper_buffer_damage_iter_init(struct drm_atomic_helper_damage_iter *iter,
+					  const struct drm_plane_state *old_state,
+					  const struct drm_plane_state *new_state);
 bool
 drm_atomic_helper_damage_iter_next(struct drm_atomic_helper_damage_iter *iter,
 				   struct drm_rect *rect);
 bool drm_atomic_helper_damage_merged(const struct drm_plane_state *old_state,
 				     struct drm_plane_state *state,
 				     struct drm_rect *rect);
+bool drm_atomic_helper_buffer_damage_merged(const struct drm_plane_state *old_state,
+					    struct drm_plane_state *state,
+					    struct drm_rect *rect);
 
 #endif
-- 
2.41.0

[PATCH 3/6] drm/virtio: Use drm_atomic_helper_buffer_damage_merged() for buffer damage

[Javier Martinez Canillas]

The driver does per-buffer uploads. It needs to use the damage helper that
handles buffer damages, rather than the helper that handles frame damages.

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

 drivers/gpu/drm/virtio/virtgpu_plane.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/virtio/virtgpu_plane.c b/drivers/gpu/drm/virtio/virtgpu_plane.c
index a2e045f3a000..1adfd9813cde 100644
--- a/drivers/gpu/drm/virtio/virtgpu_plane.c
+++ b/drivers/gpu/drm/virtio/virtgpu_plane.c
@@ -183,7 +183,7 @@ static void virtio_gpu_primary_plane_update(struct drm_plane *plane,
 		return;
 	}
 
-	if (!drm_atomic_helper_damage_merged(old_state, plane->state, &rect))
+	if (!drm_atomic_helper_buffer_damage_merged(old_state, plane->state, &rect))
 		return;
 
 	bo = gem_to_virtio_gpu_obj(plane->state->fb->obj[0]);
-- 
2.41.0

[PATCH 4/6] drm/vmwgfx: Use drm_atomic_helper_buffer_damage_iter_init() for buffer damage

[Javier Martinez Canillas]

The driver does per-buffer uploads. It needs to use the damage helper that
handles buffer damages, rather than the helper that handles frame damages.

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

 drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
index 1489ad73c103..91cda125784e 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
@@ -2912,7 +2912,7 @@ int vmw_du_helper_plane_update(struct vmw_du_update_plane *update)
 	 * Iterate in advance to check if really need plane update and find the
 	 * number of clips that actually are in plane src for fifo allocation.
 	 */
-	drm_atomic_helper_damage_iter_init(&iter, old_state, state);
+	drm_atomic_helper_buffer_damage_iter_init(&iter, old_state, state);
 	drm_atomic_for_each_plane_damage(&iter, &clip)
 		num_hits++;
 
-- 
2.41.0

[PATCH 5/6] 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 that to the section, mention the different helpers that should be used
by drivers depending on the damage handling type used and refer to sites
that have more content about damage types and damage tracking in general.

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

 drivers/gpu/drm/drm_damage_helper.c | 10 ++++++----
 drivers/gpu/drm/drm_plane.c         | 22 +++++++++++++++++++---
 2 files changed, 25 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/drm_damage_helper.c b/drivers/gpu/drm/drm_damage_helper.c
index b72062c9d31c..ac9986da7d7c 100644
--- a/drivers/gpu/drm/drm_damage_helper.c
+++ b/drivers/gpu/drm/drm_damage_helper.c
@@ -367,8 +367,9 @@ static bool __drm_atomic_helper_damage_merged(const struct drm_plane_state *old_
  * This function merges any valid plane damage clips into one rectangle and
  * returns it in @rect.
  *
- * For details see: drm_atomic_helper_damage_iter_init() and
- * drm_atomic_helper_damage_iter_next().
+ * For details see: drm_atomic_helper_damage_iter_init(),
+ * drm_atomic_helper_damage_iter_next() and
+ * `Damage Tracking Properties`_.
  *
  * Note that this helper is for drivers that do per-plane uploads and expect
  * to handle frame damages. Drivers that do per-buffer uploads instead should
@@ -400,8 +401,9 @@ EXPORT_SYMBOL(drm_atomic_helper_damage_merged);
  * full plane update should happen. It also ensure helper iterator will return
  * &drm_plane_state.src as damage.
  *
- * For details see: drm_atomic_helper_buffer_damage_iter_init() and
- * drm_atomic_helper_damage_iter_next().
+ * For details see: drm_atomic_helper_buffer_damage_iter_init(),
+ * drm_atomic_helper_damage_iter_next() and
+ * `Damage Tracking Properties`_.
  *
  * Returns:
  * True if there is valid buffer damage otherwise false.
diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
index 24e7998d1731..f137a99b3435 100644
--- a/drivers/gpu/drm/drm_plane.c
+++ b/drivers/gpu/drm/drm_plane.c
@@ -1439,9 +1439,25 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
  *
  * Drivers that are interested in damage interface for plane should enable
  * FB_DAMAGE_CLIPS property by calling drm_plane_enable_fb_damage_clips().
- * 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 drivers implementing a per-plane or per-CRTC upload target and
+ * need to handle frame damage can use drm_atomic_helper_damage_iter_init(),
+ * but drivers implementing a per-buffer upload target and need to handle buffer
+ * damage should use drm_atomic_helper_buffer_damage_iter_init() helper instead.
+ *
+ * Once the iterator has been initialized by the damage helpers mentioned above,
+ * the drm_atomic_helper_damage_iter_next() helper iterator function can be used
+ * to get damage rectangles clipped to &drm_plane_state.src.
+ *
+ * The type of damage handling implemented depends on the driver's upload target
+ * but notice that when using swap buffers, the returned damage rectangle is the
+ * &drm_plane_state.src, since a full plane update should happen. There is no
+ * buffer age support or similar damage accumulation algorithm implemented yet.
+ *
+ * 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 6/6] 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>
---

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

diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst
index 03fe5d1247be..adaa154210a0 100644
--- a/Documentation/gpu/todo.rst
+++ b/Documentation/gpu/todo.rst
@@ -765,6 +765,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 6/6] drm/todo: Add entry about implementing buffer age for damage tracking

[Simon Ser]

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

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

[Simon Ser]

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

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

[Thomas Zimmermann]

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

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
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 2/6] drm: Add drm_atomic_helper_buffer_damage_{iter_init,merged}() helpers

[Thomas Zimmermann]

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

Hi

Am 09.11.23 um 18:24 schrieb Javier Martinez Canillas:
> 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.
> 
> The drm_atomic_helper_buffer_damage_iter_init() has the same logic than
> drm_atomic_helper_damage_iter_init() but it also takes into account if the
> framebuffer attached to plane's state has changed since the last update.
> 
> And the drm_atomic_helper_buffer_damage_merged() is just a version of the
> drm_atomic_helper_damage_merged() helper, but it uses the iter_init helper
> that is mentioned above.
> 
> 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>
> ---
> 
>   drivers/gpu/drm/drm_damage_helper.c | 79 ++++++++++++++++++++++++++---
>   include/drm/drm_damage_helper.h     |  7 +++
>   2 files changed, 80 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_damage_helper.c b/drivers/gpu/drm/drm_damage_helper.c
> index aa2325567918..b72062c9d31c 100644
> --- a/drivers/gpu/drm/drm_damage_helper.c
> +++ b/drivers/gpu/drm/drm_damage_helper.c
> @@ -204,7 +204,8 @@ EXPORT_SYMBOL(drm_atomic_helper_dirtyfb);
>   static void
>   __drm_atomic_helper_damage_iter_init(struct drm_atomic_helper_damage_iter *iter,
>   				     const struct drm_plane_state *old_state,
> -				     const struct drm_plane_state *state)
> +				     const struct drm_plane_state *state,
> +				     bool buffer_damage)

I think it would be preferable to drop patches one and two and instead 
add this parameter directly to drm_atomic_helper_damage_iter_init() and 
drm_atomic_helper_damage_merged().  That's a bit of churn, but more 
readable code.

Best regards
Thomas

>   {
>   	struct drm_rect src;
>   	memset(iter, 0, sizeof(*iter));
> @@ -223,7 +224,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 || !drm_rect_equals(&state->src, &old_state->src) ||
> +	    (buffer_damage && old_state->fb != state->fb)) {
>   		iter->clips = NULL;
>   		iter->num_clips = 0;
>   		iter->full_update = true;
> @@ -243,6 +245,10 @@ __drm_atomic_helper_damage_iter_init(struct drm_atomic_helper_damage_iter *iter,
>    * update). Currently this iterator returns full plane src in case plane src
>    * changed but that can be changed in future to return damage.
>    *
> + * Note that this helper is for drivers that do per-plane uploads and expect
> + * to handle frame damages. Drivers that do per-buffer uploads instead should
> + * use @drm_atomic_helper_buffer_damage_iter_init() that handles buffer damages.
> + *
>    * For the case when plane is not visible or plane update should not happen the
>    * first call to iter_next will return false. Note that this helper use clipped
>    * &drm_plane_state.src, so driver calling this helper should have called
> @@ -253,10 +259,37 @@ drm_atomic_helper_damage_iter_init(struct drm_atomic_helper_damage_iter *iter,
>   				   const struct drm_plane_state *old_state,
>   				   const struct drm_plane_state *state)
>   {
> -	__drm_atomic_helper_damage_iter_init(iter, old_state, state);
> +	__drm_atomic_helper_damage_iter_init(iter, old_state, state, false);
>   }
>   EXPORT_SYMBOL(drm_atomic_helper_damage_iter_init);
>   
> +/**
> + * drm_atomic_helper_buffer_damage_iter_init - Initialize the buffer damage iterator.
> + * @iter: The iterator to initialize.
> + * @old_state: Old plane state for validation.
> + * @state: Plane state from which to iterate the damage clips.
> + *
> + * Initialize an iterator, which clips buffer damage
> + * &drm_plane_state.fb_damage_clips to plane &drm_plane_state.src. This iterator
> + * returns full plane src in case buffer damage is not present because user-space
> + * didn't sent, driver discarded it (it want to do full plane update) or the plane
> + * @state has an attached framebuffer that is different than the one in @state (it
> + * has changed since the last plane update).
> + *
> + * For the case when plane is not visible or plane update should not happen the
> + * first call to iter_next will return false. Note that this helper use clipped
> + * &drm_plane_state.src, so driver calling this helper should have called
> + * drm_atomic_helper_check_plane_state() earlier.
> + */
> +void
> +drm_atomic_helper_buffer_damage_iter_init(struct drm_atomic_helper_damage_iter *iter,
> +					  const struct drm_plane_state *old_state,
> +					  const struct drm_plane_state *state)
> +{
> +	__drm_atomic_helper_damage_iter_init(iter, old_state, state, true);
> +}
> +EXPORT_SYMBOL(drm_atomic_helper_buffer_damage_iter_init);
> +
>   /**
>    * drm_atomic_helper_damage_iter_next - Advance the damage iterator.
>    * @iter: The iterator to advance.
> @@ -301,7 +334,8 @@ EXPORT_SYMBOL(drm_atomic_helper_damage_iter_next);
>   
>   static bool __drm_atomic_helper_damage_merged(const struct drm_plane_state *old_state,
>   					      struct drm_plane_state *state,
> -					      struct drm_rect *rect)
> +					      struct drm_rect *rect,
> +					      bool buffer_damage)
>   {
>   	struct drm_atomic_helper_damage_iter iter;
>   	struct drm_rect clip;
> @@ -312,7 +346,7 @@ static bool __drm_atomic_helper_damage_merged(const struct drm_plane_state *old_
>   	rect->x2 = 0;
>   	rect->y2 = 0;
>   
> -	drm_atomic_helper_damage_iter_init(&iter, old_state, state);
> +	__drm_atomic_helper_damage_iter_init(&iter, old_state, state, buffer_damage);
>   	drm_atomic_for_each_plane_damage(&iter, &clip) {
>   		rect->x1 = min(rect->x1, clip.x1);
>   		rect->y1 = min(rect->y1, clip.y1);
> @@ -336,6 +370,10 @@ static bool __drm_atomic_helper_damage_merged(const struct drm_plane_state *old_
>    * For details see: drm_atomic_helper_damage_iter_init() and
>    * drm_atomic_helper_damage_iter_next().
>    *
> + * Note that this helper is for drivers that do per-plane uploads and expect
> + * to handle frame damages. Drivers that do per-buffer uploads instead should
> + * use @drm_atomic_helper_buffer_damage_merged() that handles buffer damages.
> + *
>    * Returns:
>    * True if there is valid plane damage otherwise false.
>    */
> @@ -343,6 +381,35 @@ bool drm_atomic_helper_damage_merged(const struct drm_plane_state *old_state,
>   				     struct drm_plane_state *state,
>   				     struct drm_rect *rect)
>   {
> -	return __drm_atomic_helper_damage_merged(old_state, state, rect);
> +	return __drm_atomic_helper_damage_merged(old_state, state, rect, false);
>   }
>   EXPORT_SYMBOL(drm_atomic_helper_damage_merged);
> +
> +/**
> + * drm_atomic_helper_buffer_damage_merged - Merged buffer damage
> + * @old_state: Old plane state for validation.
> + * @state: Plane state from which to iterate the damage clips.
> + * @rect: Returns the merged buffer damage rectangle
> + *
> + * This function merges any valid buffer damage clips into one rectangle and
> + * returns it in @rect. It checks if the framebuffers attached to @old_state
> + * and @state are the same. If that is not the case then the returned damage
> + * rectangle is the &drm_plane_state.src, since a full update should happen.
> + *
> + * Note that &drm_plane_state.fb_damage_clips == NULL in plane state means that
> + * full plane update should happen. It also ensure helper iterator will return
> + * &drm_plane_state.src as damage.
> + *
> + * For details see: drm_atomic_helper_buffer_damage_iter_init() and
> + * drm_atomic_helper_damage_iter_next().
> + *
> + * Returns:
> + * True if there is valid buffer damage otherwise false.
> + */
> +bool drm_atomic_helper_buffer_damage_merged(const struct drm_plane_state *old_state,
> +					    struct drm_plane_state *state,
> +					    struct drm_rect *rect)
> +{
> +	return __drm_atomic_helper_damage_merged(old_state, state, rect, true);
> +}
> +EXPORT_SYMBOL(drm_atomic_helper_buffer_damage_merged);
> diff --git a/include/drm/drm_damage_helper.h b/include/drm/drm_damage_helper.h
> index effda42cce31..328bb249d68f 100644
> --- a/include/drm/drm_damage_helper.h
> +++ b/include/drm/drm_damage_helper.h
> @@ -74,11 +74,18 @@ void
>   drm_atomic_helper_damage_iter_init(struct drm_atomic_helper_damage_iter *iter,
>   				   const struct drm_plane_state *old_state,
>   				   const struct drm_plane_state *new_state);
> +void
> +drm_atomic_helper_buffer_damage_iter_init(struct drm_atomic_helper_damage_iter *iter,
> +					  const struct drm_plane_state *old_state,
> +					  const struct drm_plane_state *new_state);
>   bool
>   drm_atomic_helper_damage_iter_next(struct drm_atomic_helper_damage_iter *iter,
>   				   struct drm_rect *rect);
>   bool drm_atomic_helper_damage_merged(const struct drm_plane_state *old_state,
>   				     struct drm_plane_state *state,
>   				     struct drm_rect *rect);
> +bool drm_atomic_helper_buffer_damage_merged(const struct drm_plane_state *old_state,
> +					    struct drm_plane_state *state,
> +					    struct drm_rect *rect);
>   
>   #endif

-- 
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 2/6] drm: Add drm_atomic_helper_buffer_damage_{iter_init, merged}() helpers

[Thomas Zimmermann]

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

Hi

Am 09.11.23 um 18:24 schrieb Javier Martinez Canillas:
> 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.
> 
> The drm_atomic_helper_buffer_damage_iter_init() has the same logic than
> drm_atomic_helper_damage_iter_init() but it also takes into account if the
> framebuffer attached to plane's state has changed since the last update.
> 
> And the drm_atomic_helper_buffer_damage_merged() is just a version of the
> drm_atomic_helper_damage_merged() helper, but it uses the iter_init helper
> that is mentioned above.
> 
> 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>
> ---
> 
>   drivers/gpu/drm/drm_damage_helper.c | 79 ++++++++++++++++++++++++++---
>   include/drm/drm_damage_helper.h     |  7 +++
>   2 files changed, 80 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_damage_helper.c b/drivers/gpu/drm/drm_damage_helper.c
> index aa2325567918..b72062c9d31c 100644
> --- a/drivers/gpu/drm/drm_damage_helper.c
> +++ b/drivers/gpu/drm/drm_damage_helper.c
> @@ -204,7 +204,8 @@ EXPORT_SYMBOL(drm_atomic_helper_dirtyfb);
>   static void
>   __drm_atomic_helper_damage_iter_init(struct drm_atomic_helper_damage_iter *iter,
>   				     const struct drm_plane_state *old_state,
> -				     const struct drm_plane_state *state)
> +				     const struct drm_plane_state *state,
> +				     bool buffer_damage)
>   {
>   	struct drm_rect src;
>   	memset(iter, 0, sizeof(*iter));
> @@ -223,7 +224,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 || !drm_rect_equals(&state->src, &old_state->src) ||
> +	    (buffer_damage && old_state->fb != state->fb)) {

I'd assume that this change effectivly disables damage handling. AFAICT 
user space often does a page flip with a new framebuffer plus damage 
data. Now, with each change of the framebuffer we ignore the damage 
information. It's not a blocker as that's the behavior before 6.4, but 
we should be aware of it.

Best regards
Thomas

>   		iter->clips = NULL;
>   		iter->num_clips = 0;
>   		iter->full_update = true;
> @@ -243,6 +245,10 @@ __drm_atomic_helper_damage_iter_init(struct drm_atomic_helper_damage_iter *iter,
>    * update). Currently this iterator returns full plane src in case plane src
>    * changed but that can be changed in future to return damage.
>    *
> + * Note that this helper is for drivers that do per-plane uploads and expect
> + * to handle frame damages. Drivers that do per-buffer uploads instead should
> + * use @drm_atomic_helper_buffer_damage_iter_init() that handles buffer damages.
> + *
>    * For the case when plane is not visible or plane update should not happen the
>    * first call to iter_next will return false. Note that this helper use clipped
>    * &drm_plane_state.src, so driver calling this helper should have called
> @@ -253,10 +259,37 @@ drm_atomic_helper_damage_iter_init(struct drm_atomic_helper_damage_iter *iter,
>   				   const struct drm_plane_state *old_state,
>   				   const struct drm_plane_state *state)
>   {
> -	__drm_atomic_helper_damage_iter_init(iter, old_state, state);
> +	__drm_atomic_helper_damage_iter_init(iter, old_state, state, false);
>   }
>   EXPORT_SYMBOL(drm_atomic_helper_damage_iter_init);
>   
> +/**
> + * drm_atomic_helper_buffer_damage_iter_init - Initialize the buffer damage iterator.
> + * @iter: The iterator to initialize.
> + * @old_state: Old plane state for validation.
> + * @state: Plane state from which to iterate the damage clips.
> + *
> + * Initialize an iterator, which clips buffer damage
> + * &drm_plane_state.fb_damage_clips to plane &drm_plane_state.src. This iterator
> + * returns full plane src in case buffer damage is not present because user-space
> + * didn't sent, driver discarded it (it want to do full plane update) or the plane
> + * @state has an attached framebuffer that is different than the one in @state (it
> + * has changed since the last plane update).
> + *
> + * For the case when plane is not visible or plane update should not happen the
> + * first call to iter_next will return false. Note that this helper use clipped
> + * &drm_plane_state.src, so driver calling this helper should have called
> + * drm_atomic_helper_check_plane_state() earlier.
> + */
> +void
> +drm_atomic_helper_buffer_damage_iter_init(struct drm_atomic_helper_damage_iter *iter,
> +					  const struct drm_plane_state *old_state,
> +					  const struct drm_plane_state *state)
> +{
> +	__drm_atomic_helper_damage_iter_init(iter, old_state, state, true);
> +}
> +EXPORT_SYMBOL(drm_atomic_helper_buffer_damage_iter_init);
> +
>   /**
>    * drm_atomic_helper_damage_iter_next - Advance the damage iterator.
>    * @iter: The iterator to advance.
> @@ -301,7 +334,8 @@ EXPORT_SYMBOL(drm_atomic_helper_damage_iter_next);
>   
>   static bool __drm_atomic_helper_damage_merged(const struct drm_plane_state *old_state,
>   					      struct drm_plane_state *state,
> -					      struct drm_rect *rect)
> +					      struct drm_rect *rect,
> +					      bool buffer_damage)
>   {
>   	struct drm_atomic_helper_damage_iter iter;
>   	struct drm_rect clip;
> @@ -312,7 +346,7 @@ static bool __drm_atomic_helper_damage_merged(const struct drm_plane_state *old_
>   	rect->x2 = 0;
>   	rect->y2 = 0;
>   
> -	drm_atomic_helper_damage_iter_init(&iter, old_state, state);
> +	__drm_atomic_helper_damage_iter_init(&iter, old_state, state, buffer_damage);
>   	drm_atomic_for_each_plane_damage(&iter, &clip) {
>   		rect->x1 = min(rect->x1, clip.x1);
>   		rect->y1 = min(rect->y1, clip.y1);
> @@ -336,6 +370,10 @@ static bool __drm_atomic_helper_damage_merged(const struct drm_plane_state *old_
>    * For details see: drm_atomic_helper_damage_iter_init() and
>    * drm_atomic_helper_damage_iter_next().
>    *
> + * Note that this helper is for drivers that do per-plane uploads and expect
> + * to handle frame damages. Drivers that do per-buffer uploads instead should
> + * use @drm_atomic_helper_buffer_damage_merged() that handles buffer damages.
> + *
>    * Returns:
>    * True if there is valid plane damage otherwise false.
>    */
> @@ -343,6 +381,35 @@ bool drm_atomic_helper_damage_merged(const struct drm_plane_state *old_state,
>   				     struct drm_plane_state *state,
>   				     struct drm_rect *rect)
>   {
> -	return __drm_atomic_helper_damage_merged(old_state, state, rect);
> +	return __drm_atomic_helper_damage_merged(old_state, state, rect, false);
>   }
>   EXPORT_SYMBOL(drm_atomic_helper_damage_merged);
> +
> +/**
> + * drm_atomic_helper_buffer_damage_merged - Merged buffer damage
> + * @old_state: Old plane state for validation.
> + * @state: Plane state from which to iterate the damage clips.
> + * @rect: Returns the merged buffer damage rectangle
> + *
> + * This function merges any valid buffer damage clips into one rectangle and
> + * returns it in @rect. It checks if the framebuffers attached to @old_state
> + * and @state are the same. If that is not the case then the returned damage
> + * rectangle is the &drm_plane_state.src, since a full update should happen.
> + *
> + * Note that &drm_plane_state.fb_damage_clips == NULL in plane state means that
> + * full plane update should happen. It also ensure helper iterator will return
> + * &drm_plane_state.src as damage.
> + *
> + * For details see: drm_atomic_helper_buffer_damage_iter_init() and
> + * drm_atomic_helper_damage_iter_next().
> + *
> + * Returns:
> + * True if there is valid buffer damage otherwise false.
> + */
> +bool drm_atomic_helper_buffer_damage_merged(const struct drm_plane_state *old_state,
> +					    struct drm_plane_state *state,
> +					    struct drm_rect *rect)
> +{
> +	return __drm_atomic_helper_damage_merged(old_state, state, rect, true);
> +}
> +EXPORT_SYMBOL(drm_atomic_helper_buffer_damage_merged);
> diff --git a/include/drm/drm_damage_helper.h b/include/drm/drm_damage_helper.h
> index effda42cce31..328bb249d68f 100644
> --- a/include/drm/drm_damage_helper.h
> +++ b/include/drm/drm_damage_helper.h
> @@ -74,11 +74,18 @@ void
>   drm_atomic_helper_damage_iter_init(struct drm_atomic_helper_damage_iter *iter,
>   				   const struct drm_plane_state *old_state,
>   				   const struct drm_plane_state *new_state);
> +void
> +drm_atomic_helper_buffer_damage_iter_init(struct drm_atomic_helper_damage_iter *iter,
> +					  const struct drm_plane_state *old_state,
> +					  const struct drm_plane_state *new_state);
>   bool
>   drm_atomic_helper_damage_iter_next(struct drm_atomic_helper_damage_iter *iter,
>   				   struct drm_rect *rect);
>   bool drm_atomic_helper_damage_merged(const struct drm_plane_state *old_state,
>   				     struct drm_plane_state *state,
>   				     struct drm_rect *rect);
> +bool drm_atomic_helper_buffer_damage_merged(const struct drm_plane_state *old_state,
> +					    struct drm_plane_state *state,
> +					    struct drm_rect *rect);
>   
>   #endif

-- 
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 2/6] drm: Add drm_atomic_helper_buffer_damage_{iter_init,merged}() helpers

[Javier Martinez Canillas]

Thomas Zimmermann <tzimmermann@suse.de> writes:

Hello Thomas,

Thanks a lot for your feedback.

> Hi
>
> Am 09.11.23 um 18:24 schrieb Javier Martinez Canillas:
>> 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.
>> 
>> The drm_atomic_helper_buffer_damage_iter_init() has the same logic than
>> drm_atomic_helper_damage_iter_init() but it also takes into account if the
>> framebuffer attached to plane's state has changed since the last update.
>> 
>> And the drm_atomic_helper_buffer_damage_merged() is just a version of the
>> drm_atomic_helper_damage_merged() helper, but it uses the iter_init helper
>> that is mentioned above.
>> 
>> 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>
>> ---
>> 
>>   drivers/gpu/drm/drm_damage_helper.c | 79 ++++++++++++++++++++++++++---
>>   include/drm/drm_damage_helper.h     |  7 +++
>>   2 files changed, 80 insertions(+), 6 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/drm_damage_helper.c b/drivers/gpu/drm/drm_damage_helper.c
>> index aa2325567918..b72062c9d31c 100644
>> --- a/drivers/gpu/drm/drm_damage_helper.c
>> +++ b/drivers/gpu/drm/drm_damage_helper.c
>> @@ -204,7 +204,8 @@ EXPORT_SYMBOL(drm_atomic_helper_dirtyfb);
>>   static void
>>   __drm_atomic_helper_damage_iter_init(struct drm_atomic_helper_damage_iter *iter,
>>   				     const struct drm_plane_state *old_state,
>> -				     const struct drm_plane_state *state)
>> +				     const struct drm_plane_state *state,
>> +				     bool buffer_damage)
>
> I think it would be preferable to drop patches one and two and instead 
> add this parameter directly to drm_atomic_helper_damage_iter_init() and 
> drm_atomic_helper_damage_merged().  That's a bit of churn, but more 
> readable code.
>

Makes sense. I'll do that in v2.

> Best regards
> Thomas
>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH 2/6] drm: Add drm_atomic_helper_buffer_damage_{iter_init, merged}() helpers

[Javier Martinez Canillas]

Thomas Zimmermann <tzimmermann@suse.de> writes:

> Hi
>
> Am 09.11.23 um 18:24 schrieb Javier Martinez Canillas:

[...]

>>   	struct drm_rect src;
>>   	memset(iter, 0, sizeof(*iter));
>> @@ -223,7 +224,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 || !drm_rect_equals(&state->src, &old_state->src) ||
>> +	    (buffer_damage && old_state->fb != state->fb)) {
>
> I'd assume that this change effectivly disables damage handling. AFAICT 
> user space often does a page flip with a new framebuffer plus damage 
> data. Now, with each change of the framebuffer we ignore the damage 
> information. It's not a blocker as that's the behavior before 6.4, but 
> we should be aware of it.
>

Yes, which is the goal of this patch since page flip with a new framebuffer
attached to a plane plus damage information can't be supported by drivers
that do per-buffer uploads.

This was causing some weston and wlroots to have flickering artifacts, due
the framebuffers being changed since the last plane update.

For now it was decided with Sima, Simon and Pekka that is the best we can
do and the reason why I add a TODO in patch #6.

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

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

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

This is my understanding at least, please Sima or Simon correct me if I
got this wrong.

> Best regards
> Thomas
>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH 2/6] drm: Add drm_atomic_helper_buffer_damage_{iter_init,merged}() helpers

[Thomas Zimmermann]

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

Hi

Am 14.11.23 um 16:58 schrieb Javier Martinez Canillas:
> Thomas Zimmermann <tzimmermann@suse.de> writes:
> 
> Hello Thomas,
> 
> Thanks a lot for your feedback.
> 
>> Hi
>>
>> Am 09.11.23 um 18:24 schrieb Javier Martinez Canillas:
>>> 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.
>>>
>>> The drm_atomic_helper_buffer_damage_iter_init() has the same logic than
>>> drm_atomic_helper_damage_iter_init() but it also takes into account if the
>>> framebuffer attached to plane's state has changed since the last update.
>>>
>>> And the drm_atomic_helper_buffer_damage_merged() is just a version of the
>>> drm_atomic_helper_damage_merged() helper, but it uses the iter_init helper
>>> that is mentioned above.
>>>
>>> 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>
>>> ---
>>>
>>>    drivers/gpu/drm/drm_damage_helper.c | 79 ++++++++++++++++++++++++++---
>>>    include/drm/drm_damage_helper.h     |  7 +++
>>>    2 files changed, 80 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/drm_damage_helper.c b/drivers/gpu/drm/drm_damage_helper.c
>>> index aa2325567918..b72062c9d31c 100644
>>> --- a/drivers/gpu/drm/drm_damage_helper.c
>>> +++ b/drivers/gpu/drm/drm_damage_helper.c
>>> @@ -204,7 +204,8 @@ EXPORT_SYMBOL(drm_atomic_helper_dirtyfb);
>>>    static void
>>>    __drm_atomic_helper_damage_iter_init(struct drm_atomic_helper_damage_iter *iter,
>>>    				     const struct drm_plane_state *old_state,
>>> -				     const struct drm_plane_state *state)
>>> +				     const struct drm_plane_state *state,
>>> +				     bool buffer_damage)
>>
>> I think it would be preferable to drop patches one and two and instead
>> add this parameter directly to drm_atomic_helper_damage_iter_init() and
>> drm_atomic_helper_damage_merged().  That's a bit of churn, but more
>> readable code.
>>
> 
> Makes sense. I'll do that in v2.

Instead of modifying these function interfaces, it might be even better 
to introduce a state flag in struct drm_plane_state that you can modify 
in the plane's atomic_check helper. Something simple like this:

   if (old_fb != new_fb)
     plane_state->ignore_damage_clips = true;

in the affected drivers/planes. In drm_atomic_helper_damage_iter_init() 
you can use it to generate a full update. This avoids the churn and is 
in line with the overall check/commit design of the DRM framework.

Best regards
Thomas

> 
>> 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 0/6] drm: Allow the damage helpers to handle buffer damage

[Thomas Zimmermann]

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

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
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 0/6] drm: Allow the damage helpers to handle buffer damage

[Javier Martinez Canillas]

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
>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH 2/6] drm: Add drm_atomic_helper_buffer_damage_{iter_init,merged}() helpers

[Javier Martinez Canillas]

Thomas Zimmermann <tzimmermann@suse.de> writes:

> Hi
>
> Am 14.11.23 um 16:58 schrieb Javier Martinez Canillas:
>> Thomas Zimmermann <tzimmermann@suse.de> writes:
>> 
>> Hello Thomas,
>> 
>> Thanks a lot for your feedback.
>> 
>>> Hi
>>>
>>> Am 09.11.23 um 18:24 schrieb Javier Martinez Canillas:
>>>> 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.
>>>>
>>>> The drm_atomic_helper_buffer_damage_iter_init() has the same logic than
>>>> drm_atomic_helper_damage_iter_init() but it also takes into account if the
>>>> framebuffer attached to plane's state has changed since the last update.
>>>>
>>>> And the drm_atomic_helper_buffer_damage_merged() is just a version of the
>>>> drm_atomic_helper_damage_merged() helper, but it uses the iter_init helper
>>>> that is mentioned above.
>>>>
>>>> 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>
>>>> ---
>>>>
>>>>    drivers/gpu/drm/drm_damage_helper.c | 79 ++++++++++++++++++++++++++---
>>>>    include/drm/drm_damage_helper.h     |  7 +++
>>>>    2 files changed, 80 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_damage_helper.c b/drivers/gpu/drm/drm_damage_helper.c
>>>> index aa2325567918..b72062c9d31c 100644
>>>> --- a/drivers/gpu/drm/drm_damage_helper.c
>>>> +++ b/drivers/gpu/drm/drm_damage_helper.c
>>>> @@ -204,7 +204,8 @@ EXPORT_SYMBOL(drm_atomic_helper_dirtyfb);
>>>>    static void
>>>>    __drm_atomic_helper_damage_iter_init(struct drm_atomic_helper_damage_iter *iter,
>>>>    				     const struct drm_plane_state *old_state,
>>>> -				     const struct drm_plane_state *state)
>>>> +				     const struct drm_plane_state *state,
>>>> +				     bool buffer_damage)
>>>
>>> I think it would be preferable to drop patches one and two and instead
>>> add this parameter directly to drm_atomic_helper_damage_iter_init() and
>>> drm_atomic_helper_damage_merged().  That's a bit of churn, but more
>>> readable code.
>>>
>> 
>> Makes sense. I'll do that in v2.
>
> Instead of modifying these function interfaces, it might be even better 
> to introduce a state flag in struct drm_plane_state that you can modify 
> in the plane's atomic_check helper. Something simple like this:
>
>    if (old_fb != new_fb)
>      plane_state->ignore_damage_clips = true;
>
> in the affected drivers/planes. In drm_atomic_helper_damage_iter_init() 
> you can use it to generate a full update. This avoids the churn and is 
> in line with the overall check/commit design of the DRM framework.
>

Thanks. That indeed seems more aligned with the rest of the DRM framework.

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH 2/6] drm: Add drm_atomic_helper_buffer_damage_{iter_init, merged}() helpers

[Pekka Paalanen]

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

On Tue, 14 Nov 2023 17:05:12 +0100
Javier Martinez Canillas <javierm@redhat.com> wrote:

> Thomas Zimmermann <tzimmermann@suse.de> writes:
> 
> > Hi
> >
> > Am 09.11.23 um 18:24 schrieb Javier Martinez Canillas:  
> 
> [...]
> 
> >>   	struct drm_rect src;
> >>   	memset(iter, 0, sizeof(*iter));
> >> @@ -223,7 +224,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 || !drm_rect_equals(&state->src, &old_state->src) ||
> >> +	    (buffer_damage && old_state->fb != state->fb)) {  
> >
> > I'd assume that this change effectivly disables damage handling. AFAICT 
> > user space often does a page flip with a new framebuffer plus damage 
> > data. Now, with each change of the framebuffer we ignore the damage 
> > information. It's not a blocker as that's the behavior before 6.4, but 
> > we should be aware of it.
> >  
> 
> Yes, which is the goal of this patch since page flip with a new framebuffer
> attached to a plane plus damage information can't be supported by drivers
> that do per-buffer uploads.
> 
> This was causing some weston and wlroots to have flickering artifacts, due
> the framebuffers being changed since the last plane update.
> 
> For now it was decided with Sima, Simon and Pekka that is the best we can
> do and the reason why I add a TODO in patch #6.
> 

Hi all,

this made me thinking...

The per-buffer damage accumulation that would be needed is per
upload-buffer, not per KMS FB from userspace. So it should not make any
difference whether userspace flips to another FB or not, the damage
will need to be accumulated per upload-buffer anyway if the driver is
flipping upload-buffers, in order to make the upload-buffer fully
up-to-date.

Why was that not more broken than it already was?

Is there a fixed 1:1 relationship between userspace KMS FBs and the
driver upload-buffers?

Userspace is already taking care that the KMS FB is always fully
up-to-date, FWIW, so the kernel can certainly read areas outside of
FB_DAMAGE_CLIPS if it wants to.


Thanks,
pq

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