[PATCH 0/5] drm/damage-helper: Improve damage-clipping heuristics

[PATCH 0/5] drm/damage-helper: Improve damage-clipping heuristics

[Thomas Zimmermann]

DRM Drivers can minimize a plane update by looking at damage clipping
information provided by userspace. So far, not having damage information
starts a full-plane update. Improve the heuristics for detecting partial
and full-plane updates by looking at the various state variables of a
plane update.

The current scheme of interpreting 'no damage information' as 'do full
update' works for hardware with a single primary plane. For hardware with
multiple planes, missing damage information can generate significant
overhead. For example, ast supports a primary plane and a cursor plane.
DRM performs an atomic update when the cursor plane is being updated in
some way (e.g., moved). The lack of damage information on the primary
plane results in a full-plane update of the primary plane as well. Using
shadow planes that need to be memcpy'd to video memory creates a full
redraw of the entire primary plane whenever the user moves the mouse
cursor.

The patchset improves the current heuristics by looking at various
state variables to detect whether a full update is necessary.

Patch #2 decouples full-plane updates from the (non-)existence of
damage information. Full-plane updates are still the default, but
the dedicated flag in the plane state allows for more fine-grained
control.

Patches #3 to #5 enable partial plane updates on various conditions.

The patchset has been tested by converting ast to SHMEM and running
Gnome on X11, Gnome in Wayland mode, and Weston. The new heuristics
reduce each environment's unnecessary updates of the primary plane to
no-ops: DRM still invokes the primary plane's atomic_update, but the
costly memcpy to video memory is being avoided.

Thomas Zimmermann (5):
  drm/damage-helper: Style changes
  drm/damage-helper: Decouple partial plane updates from damage clipping
  drm/damage-helper: Do partial updates on legacy cursor changes
  drm/damage-helper: Do partial updates if framebuffer has not been
    changed
  drm/damage-helper: Avoid partial updates for DIRTYFB without damage

 drivers/gpu/drm/drm_atomic_helper.c       |  3 +
 drivers/gpu/drm/drm_atomic_state_helper.c |  3 +
 drivers/gpu/drm/drm_damage_helper.c       | 99 +++++++++++++++++------
 include/drm/drm_plane.h                   | 23 ++++++
 4 files changed, 105 insertions(+), 23 deletions(-)


base-commit: d8deedaa0fcd8192715a052a0239bee3f74a8fb1
-- 
2.37.3

[PATCH 1/5] drm/damage-helper: Style changes

[Thomas Zimmermann]

Rename several variables in the damage-helper code to better reflect
the use of old and new state. No functional changes.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/drm_damage_helper.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/drm_damage_helper.c b/drivers/gpu/drm/drm_damage_helper.c
index d8b2955e88fd..4b1f26ef119f 100644
--- a/drivers/gpu/drm/drm_damage_helper.c
+++ b/drivers/gpu/drm/drm_damage_helper.c
@@ -53,7 +53,7 @@ static void convert_clip_rect_to_rect(const struct drm_clip_rect *src,
 /**
  * drm_atomic_helper_check_plane_damage - Verify plane damage on atomic_check.
  * @state: The driver state object.
- * @plane_state: Plane state for which to verify damage.
+ * @new_plane_state: Plane state for which to verify damage.
  *
  * This helper function makes sure that damage from plane state is discarded
  * for full modeset. If there are more reasons a driver would want to do a full
@@ -65,20 +65,19 @@ static void convert_clip_rect_to_rect(const struct drm_clip_rect *src,
  * &drm_plane_state.src as damage.
  */
 void drm_atomic_helper_check_plane_damage(struct drm_atomic_state *state,
-					  struct drm_plane_state *plane_state)
+					  struct drm_plane_state *new_plane_state)
 {
-	struct drm_crtc_state *crtc_state;
+	struct drm_crtc_state *new_crtc_state;
 
-	if (plane_state->crtc) {
-		crtc_state = drm_atomic_get_new_crtc_state(state,
-							   plane_state->crtc);
+	if (new_plane_state->crtc) {
+		new_crtc_state = drm_atomic_get_new_crtc_state(state, new_plane_state->crtc);
 
-		if (WARN_ON(!crtc_state))
+		if (WARN_ON(!new_crtc_state))
 			return;
 
-		if (drm_atomic_crtc_needs_modeset(crtc_state)) {
-			drm_property_blob_put(plane_state->fb_damage_clips);
-			plane_state->fb_damage_clips = NULL;
+		if (drm_atomic_crtc_needs_modeset(new_crtc_state)) {
+			drm_property_blob_put(new_plane_state->fb_damage_clips);
+			new_plane_state->fb_damage_clips = NULL;
 		}
 	}
 }
-- 
2.37.3

[PATCH 2/5] drm/damage-helper: Decouple partial plane updates from damage clipping

[Thomas Zimmermann]

Introduce a distinct flag fb_damage_partial_update in plane state to
signal the option for a partial plane update. Replaces the semantics
of having no damaged areas to trigger a full update. Decoupling the
existence of damage clipping from partial plane updates will allow to
sometimes avoid plane updates at all.

By default the new flag is cleared, which triggers a full update. We
also keep doing a full update on modesetting operations or if the
framebuffer has been moved within the plane. Installing damage clipping
areas on a plane state sets the new flag and triggers a partial update
of the given areas.

Userspace that does not support damage clipping continues to work as
before.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/drm_atomic_state_helper.c |  1 +
 drivers/gpu/drm/drm_damage_helper.c       | 68 ++++++++++++++++-------
 include/drm/drm_plane.h                   |  9 +++
 3 files changed, 59 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c b/drivers/gpu/drm/drm_atomic_state_helper.c
index bf31b9d92094..85b13c221bd8 100644
--- a/drivers/gpu/drm/drm_atomic_state_helper.c
+++ b/drivers/gpu/drm/drm_atomic_state_helper.c
@@ -338,6 +338,7 @@ void __drm_atomic_helper_plane_duplicate_state(struct drm_plane *plane,
 	state->fence = NULL;
 	state->commit = NULL;
 	state->fb_damage_clips = NULL;
+	state->fb_damage_partial_update = false;
 }
 EXPORT_SYMBOL(__drm_atomic_helper_plane_duplicate_state);
 
diff --git a/drivers/gpu/drm/drm_damage_helper.c b/drivers/gpu/drm/drm_damage_helper.c
index 4b1f26ef119f..16f0d5a97ee3 100644
--- a/drivers/gpu/drm/drm_damage_helper.c
+++ b/drivers/gpu/drm/drm_damage_helper.c
@@ -55,30 +55,48 @@ static void convert_clip_rect_to_rect(const struct drm_clip_rect *src,
  * @state: The driver state object.
  * @new_plane_state: Plane state for which to verify damage.
  *
- * This helper function makes sure that damage from plane state is discarded
- * for full modeset. If there are more reasons a driver would want to do a full
- * plane update rather than processing individual damage regions, then those
- * cases should be taken care of here.
+ * This helper function makes sure that damage from plane state is set up
+ * for full plane updates if necessary. This happens for full modesets on the
+ * plane's CRTC, and for pageflips without damage. If there are more reasons
+ * a driver would want to do a full plane update rather than processing
+ * individual damage regions, then those cases should be taken care of here.
  *
- * 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.
+ * Note that &drm_plane_state.fb_damage_partial_update == false in plane state
+ * means that a full plane update should happen. It also ensures the helper
+ * iterator will return &drm_plane_state.src as damage.
  */
 void drm_atomic_helper_check_plane_damage(struct drm_atomic_state *state,
 					  struct drm_plane_state *new_plane_state)
 {
 	struct drm_crtc_state *new_crtc_state;
+	struct drm_crtc *new_crtc = new_plane_state->crtc;
+	bool partial_update = false;
 
-	if (new_plane_state->crtc) {
-		new_crtc_state = drm_atomic_get_new_crtc_state(state, new_plane_state->crtc);
+	if (new_crtc) {
+		new_crtc_state = drm_atomic_get_new_crtc_state(state, new_crtc);
 
 		if (WARN_ON(!new_crtc_state))
-			return;
+			goto out;
 
-		if (drm_atomic_crtc_needs_modeset(new_crtc_state)) {
-			drm_property_blob_put(new_plane_state->fb_damage_clips);
-			new_plane_state->fb_damage_clips = NULL;
-		}
+		/*
+		 * The plane's CRTC does a modeset; always do full update.
+		 */
+		if (drm_atomic_crtc_needs_modeset(new_crtc_state))
+			goto out;
+	}
+
+	/*
+	 * Damage clips are a good indicator for partial updates.
+	 */
+	if (new_plane_state->fb_damage_clips)
+		partial_update = true;
+
+out:
+	new_plane_state->fb_damage_partial_update = partial_update;
+
+	if (!new_plane_state->fb_damage_partial_update) {
+		drm_property_blob_put(new_plane_state->fb_damage_clips);
+		new_plane_state->fb_damage_clips = NULL;
 	}
 }
 EXPORT_SYMBOL(drm_atomic_helper_check_plane_damage);
@@ -224,13 +242,24 @@ drm_atomic_helper_damage_iter_init(struct drm_atomic_helper_damage_iter *iter,
 				   const struct drm_plane_state *state)
 {
 	struct drm_rect src;
+	bool partial_update;
+
 	memset(iter, 0, sizeof(*iter));
 
 	if (!state || !state->crtc || !state->fb || !state->visible)
 		return;
 
-	iter->clips = (struct drm_rect *)drm_plane_get_damage_clips(state);
-	iter->num_clips = drm_plane_get_damage_clips_count(state);
+	partial_update = state->fb_damage_partial_update;
+
+	/*
+	 * Only allow a partial update if the framebuffer did not move
+	 * within the plane. Otherwise do a full update. We have to test
+	 * this here, instead of drm_atomic_helper_check_plane_damage(),
+	 * as the plane's atomic_check helper could meanwhile have changed
+	 * the source coordinates.
+	 */
+	if (partial_update)
+		partial_update = drm_rect_equals(&state->src, &old_state->src);
 
 	/* Round down for x1/y1 and round up for x2/y2 to catch all pixels */
 	src = drm_plane_state_src(state);
@@ -240,9 +269,10 @@ 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)) {
-		iter->clips = NULL;
-		iter->num_clips = 0;
+	if (partial_update) {
+		iter->clips = (struct drm_rect *)drm_plane_get_damage_clips(state);
+		iter->num_clips = drm_plane_get_damage_clips_count(state);
+	} else {
 		iter->full_update = true;
 	}
 }
diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
index 89ea54652e87..3ba91349d799 100644
--- a/include/drm/drm_plane.h
+++ b/include/drm/drm_plane.h
@@ -190,6 +190,15 @@ struct drm_plane_state {
 	 */
 	struct drm_property_blob *fb_damage_clips;
 
+	/**
+	 * @fb_damage_partial_update:
+	 *
+	 * Marks the plane for a partial update. The value of this field
+	 * depends on the supplied atomic state and the operation that
+	 * initiated the update.
+	 */
+	bool fb_damage_partial_update;
+
 	/**
 	 * @src:
 	 *
-- 
2.37.3

[PATCH 3/5] drm/damage-helper: Do partial updates on legacy cursor changes

[Thomas Zimmermann]

In the case of a legacy cursor update, only update the cursor plane. Keep
other planes clear from changes. Setting the 'partial_update' flag when
these planes don't have damage-clipping areas acts as if no update will
be performed.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/drm_damage_helper.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/gpu/drm/drm_damage_helper.c b/drivers/gpu/drm/drm_damage_helper.c
index 16f0d5a97ee3..a603a3563c03 100644
--- a/drivers/gpu/drm/drm_damage_helper.c
+++ b/drivers/gpu/drm/drm_damage_helper.c
@@ -69,6 +69,7 @@ void drm_atomic_helper_check_plane_damage(struct drm_atomic_state *state,
 					  struct drm_plane_state *new_plane_state)
 {
 	struct drm_crtc_state *new_crtc_state;
+	struct drm_plane *plane = new_plane_state->plane;
 	struct drm_crtc *new_crtc = new_plane_state->crtc;
 	bool partial_update = false;
 
@@ -83,6 +84,17 @@ void drm_atomic_helper_check_plane_damage(struct drm_atomic_state *state,
 		 */
 		if (drm_atomic_crtc_needs_modeset(new_crtc_state))
 			goto out;
+
+		/*
+		 * On a legacy cursor update, only update the affected cursor
+		 * plane, but ignore all other planes. The non-cursor planes
+		 * won't have damage-clipping areas, so setting the flag for
+		 * a partial update acts like not doing any update.
+		 */
+		if (state->legacy_cursor_update) {
+			if (plane != new_crtc->cursor)
+				partial_update = true;
+		}
 	}
 
 	/*
-- 
2.37.3

[PATCH 4/5] drm/damage-helper: Do partial updates if framebuffer has not been changed

[Thomas Zimmermann]

Set partial updates on a plane if the framebuffer has not been changed
on an atomic commit. If such a plane has damage clips, the driver will
use them; otherwise the update is effectively empty. Planes that change
their framebuffer still perform a full update.

This heuristic optimizes the case of setting a new framebuffer on a
plane. This would trigger a full update of all other planes. With the
new optimization, only the changed plane performs an update.

The commit adds the flag fb_changed to struct plane_state. Besides
the damage-handling code, drivers can look at the flag to determine
if they need to commit a plane's framebuffer settings.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/drm_atomic_helper.c       |  3 +++
 drivers/gpu/drm/drm_atomic_state_helper.c |  1 +
 drivers/gpu/drm/drm_damage_helper.c       | 19 +++++++++++++++----
 include/drm/drm_plane.h                   |  6 ++++++
 4 files changed, 25 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index ee5fea48b5cb..f19405fbee14 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -99,6 +99,9 @@ drm_atomic_helper_plane_changed(struct drm_atomic_state *state,
 
 		crtc_state->planes_changed = true;
 	}
+
+	if (old_plane_state->fb != plane_state->fb)
+		plane_state->fb_changed = true;
 }
 
 static int handle_conflicting_encoders(struct drm_atomic_state *state,
diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c b/drivers/gpu/drm/drm_atomic_state_helper.c
index 85b13c221bd8..94818fd4dd8f 100644
--- a/drivers/gpu/drm/drm_atomic_state_helper.c
+++ b/drivers/gpu/drm/drm_atomic_state_helper.c
@@ -339,6 +339,7 @@ void __drm_atomic_helper_plane_duplicate_state(struct drm_plane *plane,
 	state->commit = NULL;
 	state->fb_damage_clips = NULL;
 	state->fb_damage_partial_update = false;
+	state->fb_changed = false;
 }
 EXPORT_SYMBOL(__drm_atomic_helper_plane_duplicate_state);
 
diff --git a/drivers/gpu/drm/drm_damage_helper.c b/drivers/gpu/drm/drm_damage_helper.c
index a603a3563c03..f43abf02df5b 100644
--- a/drivers/gpu/drm/drm_damage_helper.c
+++ b/drivers/gpu/drm/drm_damage_helper.c
@@ -97,11 +97,22 @@ void drm_atomic_helper_check_plane_damage(struct drm_atomic_state *state,
 		}
 	}
 
-	/*
-	 * Damage clips are a good indicator for partial updates.
-	 */
-	if (new_plane_state->fb_damage_clips)
+	if (new_plane_state->fb_damage_clips) {
+		/*
+		 * Damage clips are a good indicator for partial updates.
+		 */
 		partial_update = true;
+	} else if (!new_plane_state->fb_changed) {
+		/*
+		 * Also set a partial update if the framebuffer did not
+		 * change. Without damage clips set, this will effectively
+		 * not update the plane. The exception is with full modeset
+		 * operations, where we do full plane update even if the
+		 * framebuffer did not change. We already handled this case
+		 * earlier in the function.
+		 */
+		partial_update = true;
+	}
 
 out:
 	new_plane_state->fb_damage_partial_update = partial_update;
diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
index 3ba91349d799..8c2d0a2eb760 100644
--- a/include/drm/drm_plane.h
+++ b/include/drm/drm_plane.h
@@ -229,6 +229,12 @@ struct drm_plane_state {
 	 */
 	bool visible;
 
+	/**
+	 * @fb_changed: @fb has been changed. Used by the atomic helpers and
+	 * drivers to steer the atomic commit control flow.
+	 */
+	bool fb_changed : 1;
+
 	/**
 	 * @scaling_filter:
 	 *
-- 
2.37.3

Re: [PATCH 4/5] drm/damage-helper: Do partial updates if framebuffer has not been changed

[Ville Syrjälä]

On Tue, Sep 20, 2022 at 03:56:18PM +0200, Thomas Zimmermann wrote:
> Set partial updates on a plane if the framebuffer has not been changed
> on an atomic commit. If such a plane has damage clips, the driver will
> use them; otherwise the update is effectively empty. Planes that change
> their framebuffer still perform a full update.

I have a feeling you're sort of papering over some inefficient
userspace that's asking updates on planes that don't actually
need them. I'm not sure adding more code for that is a particularly
great idea. Wouldn't it be better to just fix the userspace code?

Any property change on the plane could need a full plane
update as well (eg. some color mangement stuff etc.). So you'd
have to keep adding exceptions to that list whenever new
properties are added.

And I think the semantics of the ioctl(s) has so far been that
basically any page flip (whether or not it actually changes
the fb) still ends up doing whatever flushing is needed to
guarantee the fb contents are up to date on the screen (if
someone sneaked in some front buffer rendering in between).
Ie. you could even use basically a nop page flip in place 
of dirtyfb.

Another thing the ioctls have always done is actually perform
the commit whether anything changed or nor. That is, they
still do all the same the same vblanky stuff and the commit
takes the same amount of time. Not sure if your idea is
to potentially short circuit the entire thing and make it 
take no time at all?

-- 
Ville Syrjälä
Intel

Re: [PATCH 4/5] drm/damage-helper: Do partial updates if framebuffer has not been changed

[Daniel Stone]

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

Hi,

On Tue, 20 Sept 2022 at 15:31, Ville Syrjälä <ville.syrjala@linux.intel.com>
wrote:

> On Tue, Sep 20, 2022 at 03:56:18PM +0200, Thomas Zimmermann wrote:
> > Set partial updates on a plane if the framebuffer has not been changed
> > on an atomic commit. If such a plane has damage clips, the driver will
> > use them; otherwise the update is effectively empty. Planes that change
> > their framebuffer still perform a full update.
>
> I have a feeling you're sort of papering over some inefficient
> userspace that's asking updates on planes that don't actually
> need them. I'm not sure adding more code for that is a particularly
> great idea. Wouldn't it be better to just fix the userspace code?
>

I'm not sure why it would need to be 'fixed', when it's never been a
property of the atomic API that you must minimise updates. Weston does this
(dumps the full state in every time), and I know we're not the only ones.

'Fixing' it would require doing a bunch of diffing in userspace, because
maintaining a delta and trying to unwind that delta across multiple
TEST_ONLY runs, isn't much fun. It's certainly a far bigger diff than this
patch.


> Any property change on the plane could need a full plane
> update as well (eg. some color mangement stuff etc.). So you'd
> have to keep adding exceptions to that list whenever new
> properties are added.
>

Eh, it's just a blob ID comparison.


> And I think the semantics of the ioctl(s) has so far been that
> basically any page flip (whether or not it actually changes
> the fb) still ends up doing whatever flushing is needed to
> guarantee the fb contents are up to date on the screen (if
> someone sneaked in some front buffer rendering in between).
> Ie. you could even use basically a nop page flip in place
> of dirtyfb.
>
> Another thing the ioctls have always done is actually perform
> the commit whether anything changed or nor. That is, they
> still do all the same the same vblanky stuff and the commit
> takes the same amount of time. Not sure if your idea is
> to potentially short circuit the entire thing and make it
> take no time at all?
>

I would expect it to always perform a commit, though.

Cheers,
Daniel

[-- Attachment #2: Type: text/html, Size: 3078 bytes --]

Re: [PATCH 4/5] drm/damage-helper: Do partial updates if framebuffer has not been changed

[Ville Syrjälä]

On Tue, Sep 20, 2022 at 03:47:39PM +0100, Daniel Stone wrote:
> Hi,
> 
> On Tue, 20 Sept 2022 at 15:31, Ville Syrjälä <ville.syrjala@linux.intel.com>
> wrote:
> 
> > On Tue, Sep 20, 2022 at 03:56:18PM +0200, Thomas Zimmermann wrote:
> > > Set partial updates on a plane if the framebuffer has not been changed
> > > on an atomic commit. If such a plane has damage clips, the driver will
> > > use them; otherwise the update is effectively empty. Planes that change
> > > their framebuffer still perform a full update.
> >
> > I have a feeling you're sort of papering over some inefficient
> > userspace that's asking updates on planes that don't actually
> > need them. I'm not sure adding more code for that is a particularly
> > great idea. Wouldn't it be better to just fix the userspace code?
> >
> 
> I'm not sure why it would need to be 'fixed', when it's never been a
> property of the atomic API that you must minimise updates. Weston does this
> (dumps the full state in every time), and I know we're not the only ones.

Well, we've never tried to minimize in the kernel either, except
for a few special cases that have one of these ad-hoc "foo_changed"
flags. And I think those are more due to "I felt like adding one"
rather than any real overall design goal. We even have one that
is not used at all, except after this patch series (or at least
I think I saw it in there).

We could of course scan a lot more in the kernel to minimize stuff,
but at least I always end up wondering how many joules are being
wasted rescanning things when userspace might have already done
the same thing. Also at some point it just might be cheaper to
blast the hw with the stuff than to meticulously scan through
it all. At least as long as we can't to the 100% short circuit.

Side rant: I'm also not a huge fan of those current foo_changed
flags because their granularity is a bit weird, and dictated by
the core code rather than by the driver specific cost of updating
each property. Eg. color_mgmt_changed cover all of crtc level
color management, but at least for i915 there are both cheap and
expensive things in there. So not sure the current flag really
helps with anything. We do currently use it in i915, but maybe
we should not and just try to look at each property separately
instead.

> 
> 'Fixing' it would require doing a bunch of diffing in userspace, because
> maintaining a delta and trying to unwind that delta across multiple
> TEST_ONLY runs, isn't much fun. It's certainly a far bigger diff than this
> patch.

OK. If userspace doesn't want to do it at all, then maybe
the kernel should do at least a bit of it.

I wonder how far we should take that. In the olden pre-atomic
days i915 even automagically turned off the primary plane when
fully covered by an opaque sprite plane. I presume modern userspace
should at least try to use the available planes efficiently
and that kind of optimizations would be a waste of time?

> 
> > Any property change on the plane could need a full plane
> > update as well (eg. some color mangement stuff etc.). So you'd
> > have to keep adding exceptions to that list whenever new
> > properties are added.
> >
> 
> Eh, it's just a blob ID comparison.

Or some other integer, yes, but someone must remember to add it.
This patch series certainly seems to have forgotten most of it,
so perhaps a slightly shaky start :) OK, sorry bad pun, moving
along...

If we do this in some common code near the uapi level, then
the driver might still have to repeat a bunch of it due to
interactions with other stuff. But I already ranted about
color_mgmt_changed earlier in the mail, I guess this is
more of the same really.

What would make this sort of discussion really 
interesting would be actual power and/or performance
figures given some typical fixed workloads. But that
sounds like a lot of work...

> 
> 
> > And I think the semantics of the ioctl(s) has so far been that
> > basically any page flip (whether or not it actually changes
> > the fb) still ends up doing whatever flushing is needed to
> > guarantee the fb contents are up to date on the screen (if
> > someone sneaked in some front buffer rendering in between).
> > Ie. you could even use basically a nop page flip in place
> > of dirtyfb.
> >
> > Another thing the ioctls have always done is actually perform
> > the commit whether anything changed or nor. That is, they
> > still do all the same the same vblanky stuff and the commit
> > takes the same amount of time. Not sure if your idea is
> > to potentially short circuit the entire thing and make it
> > take no time at all?
> >
> 
> I would expect it to always perform a commit, though.
> 
> Cheers,
> Daniel

-- 
Ville Syrjälä
Intel

Re: [PATCH 4/5] drm/damage-helper: Do partial updates if framebuffer has not been changed

[Thomas Zimmermann]

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

Hi

Am 20.09.22 um 16:47 schrieb Daniel Stone:
> Hi,
> 
> On Tue, 20 Sept 2022 at 15:31, Ville Syrjälä 
> <ville.syrjala@linux.intel.com <mailto:ville.syrjala@linux.intel.com>> 
> wrote:
> 
>     On Tue, Sep 20, 2022 at 03:56:18PM +0200, Thomas Zimmermann wrote:
>      > Set partial updates on a plane if the framebuffer has not been
>     changed
>      > on an atomic commit. If such a plane has damage clips, the driver
>     will
>      > use them; otherwise the update is effectively empty. Planes that
>     change
>      > their framebuffer still perform a full update.
> 
>     I have a feeling you're sort of papering over some inefficient
>     userspace that's asking updates on planes that don't actually
>     need them. I'm not sure adding more code for that is a particularly
>     great idea. Wouldn't it be better to just fix the userspace code?
> 
> 
> I'm not sure why it would need to be 'fixed', when it's never been a 
> property of the atomic API that you must minimise updates. Weston does 
> this (dumps the full state in every time), and I know we're not the only 
> ones.

I've found a bug in one of the DRM helpers. It unconditionally adds the 
primary plane to the commit, which triggers the repaint. Fixing the bug 
resolves the problem for X11 and Wayland-Gnome.

> 
> 'Fixing' it would require doing a bunch of diffing in userspace, because 
> maintaining a delta and trying to unwind that delta across multiple 
> TEST_ONLY runs, isn't much fun. It's certainly a far bigger diff than 
> this patch.

But even with the fix applied, weston still wants to redraw the whole 
screen on every movement of the mouse cursor. The system is usable, so 
it's not a showstopper.

Still, weston should stop sending the primary plane's framebuffer on 
each cursor update. There's no update to the contents and the fix seems 
simple to do from userspace.

Best regards
Thomas

> 
>     Any property change on the plane could need a full plane
>     update as well (eg. some color mangement stuff etc.). So you'd
>     have to keep adding exceptions to that list whenever new
>     properties are added.
> 
> 
> Eh, it's just a blob ID comparison.
> 
>     And I think the semantics of the ioctl(s) has so far been that
>     basically any page flip (whether or not it actually changes
>     the fb) still ends up doing whatever flushing is needed to
>     guarantee the fb contents are up to date on the screen (if
>     someone sneaked in some front buffer rendering in between).
>     Ie. you could even use basically a nop page flip in place
>     of dirtyfb.
> 
>     Another thing the ioctls have always done is actually perform
>     the commit whether anything changed or nor. That is, they
>     still do all the same the same vblanky stuff and the commit
>     takes the same amount of time. Not sure if your idea is
>     to potentially short circuit the entire thing and make it
>     take no time at all?
> 
> 
> I would expect it to always perform a commit, though.
> 
> Cheers,
> Daniel

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

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

Re: [PATCH 4/5] drm/damage-helper: Do partial updates if framebuffer has not been changed

[Thomas Zimmermann]

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

Hi Ville

Am 20.09.22 um 16:31 schrieb Ville Syrjälä:
> On Tue, Sep 20, 2022 at 03:56:18PM +0200, Thomas Zimmermann wrote:
>> Set partial updates on a plane if the framebuffer has not been changed
>> on an atomic commit. If such a plane has damage clips, the driver will
>> use them; otherwise the update is effectively empty. Planes that change
>> their framebuffer still perform a full update.
> 
> I have a feeling you're sort of papering over some inefficient
> userspace that's asking updates on planes that don't actually
> need them. I'm not sure adding more code for that is a particularly
> great idea. Wouldn't it be better to just fix the userspace code?

Some more context might be in order:

The ast driver currently uses VRAM helpers, which leads to many 
problems; including blank screens from the low amount of video memory. 
The best solution is to switch SHMEM with BOs on system meory. The video 
memory is only the internal buffer for scanout. This update involves a 
mempcy from the BO to video memory.

Ast's hardware is really slow, so it makes sense to reduce the updates 
to video memory to a minimum. There's support for cursor planes, which 
really makes a difference here.

But doing any cursor-plane update (e.g., moving, animation) with SHMEM 
and the current damage helpers always results in a full-screen memcpy 
from the BO to the video memory for the primary plane. As the ast 
hardware is slow, performance goes down to a an estimated 5 frame per 
seconds. After moving the mouse, one can watch the mouse cursor follow 
along the screen for the next seconds. Userspace sends the movement 
information and DRM slowly processes them. The same can be observed for 
cursor animation.

The problem is that each change to the cursor plane results in an 
atomic_update call for the primary plane. Not having damage information 
for the plane just means 'update everything'. Not a problem if redrawing 
consists of reprogramming the scanout address. For a memcpy it's not 
feasible.

So can this be fixed in userspace? No realistically IMHO. I've seen this 
problem on Weston, Wayland-Gnome and X11-Gnome. And they are all 
problematic in their own way. (That's why there are multiple patches 
needed.) For example, X11 uses the legacy mouse ioctl, which one of the 
patches fixes. The other userspace needs the other heuristics. A 
potential userspace fix would mean to always set empty-damage 
information on all planes that don't get an update. And I don't consider 
X11 fixable TBH.

> 
> Any property change on the plane could need a full plane
> update as well (eg. some color mangement stuff etc.). So you'd
> have to keep adding exceptions to that list whenever new
> properties are added.

It's not about the occasional change of a property. It's the constant 
sluggish redraw when the interface is supposed to be snappy, such as 
mouse interaction.

> 
> And I think the semantics of the ioctl(s) has so far been that
> basically any page flip (whether or not it actually changes
> the fb) still ends up doing whatever flushing is needed to
> guarantee the fb contents are up to date on the screen (if
> someone sneaked in some front buffer rendering in between).
> Ie. you could even use basically a nop page flip in place
> of dirtyfb.

That's why full updates are still the default. Only in cases where it's 
save to avoid an update of unaffected planes, we do so.

I know that we don't give performance guarantees to userspace. But using 
cursor/overlay planes should be faster then not using them. I think 
that's a reasonable assumption for userspace to make.

> 
> Another thing the ioctls have always done is actually perform
> the commit whether anything changed or nor. That is, they
> still do all the same the same vblanky stuff and the commit
> takes the same amount of time. Not sure if your idea is
> to potentially short circuit the entire thing and make it
> take no time at all?

The patches don't change the overall commit logics. All they do is to 
set damage updates to a size of 0 if a plane reliably does not need an 
update. A driver's atomic_update still runs, but the damage iterator 
does not return any damaged areas.

Best regards
Thomas


> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

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

Re: [PATCH 4/5] drm/damage-helper: Do partial updates if framebuffer has not been changed

[Ville Syrjälä]

On Wed, Sep 21, 2022 at 09:59:10AM +0200, Thomas Zimmermann wrote:
> Hi Ville
> 
> Am 20.09.22 um 16:31 schrieb Ville Syrjälä:
> > On Tue, Sep 20, 2022 at 03:56:18PM +0200, Thomas Zimmermann wrote:
> >> Set partial updates on a plane if the framebuffer has not been changed
> >> on an atomic commit. If such a plane has damage clips, the driver will
> >> use them; otherwise the update is effectively empty. Planes that change
> >> their framebuffer still perform a full update.
> > 
> > I have a feeling you're sort of papering over some inefficient
> > userspace that's asking updates on planes that don't actually
> > need them. I'm not sure adding more code for that is a particularly
> > great idea. Wouldn't it be better to just fix the userspace code?
> 
> Some more context might be in order:
> 
> The ast driver currently uses VRAM helpers, which leads to many 
> problems; including blank screens from the low amount of video memory. 
> The best solution is to switch SHMEM with BOs on system meory. The video 
> memory is only the internal buffer for scanout. This update involves a 
> mempcy from the BO to video memory.
> 
> Ast's hardware is really slow, so it makes sense to reduce the updates 
> to video memory to a minimum. There's support for cursor planes, which 
> really makes a difference here.
> 
> But doing any cursor-plane update (e.g., moving, animation) with SHMEM 
> and the current damage helpers always results in a full-screen memcpy 
> from the BO to the video memory for the primary plane. As the ast 
> hardware is slow, performance goes down to a an estimated 5 frame per 
> seconds. After moving the mouse, one can watch the mouse cursor follow 
> along the screen for the next seconds. Userspace sends the movement 
> information and DRM slowly processes them. The same can be observed for 
> cursor animation.
> 
> The problem is that each change to the cursor plane results in an 
> atomic_update call for the primary plane. Not having damage information 
> for the plane just means 'update everything'. Not a problem if redrawing 
> consists of reprogramming the scanout address. For a memcpy it's not 
> feasible.
> 
> So can this be fixed in userspace? No realistically IMHO. I've seen this 
> problem on Weston, Wayland-Gnome and X11-Gnome. And they are all 
> problematic in their own way. (That's why there are multiple patches 
> needed.) For example, X11 uses the legacy mouse ioctl, which one of the 
> patches fixes. The other userspace needs the other heuristics. A 
> potential userspace fix would mean to always set empty-damage 
> information on all planes that don't get an update. And I don't consider 
> X11 fixable TBH.
> 
> > 
> > Any property change on the plane could need a full plane
> > update as well (eg. some color mangement stuff etc.). So you'd
> > have to keep adding exceptions to that list whenever new
> > properties are added.
> 
> It's not about the occasional change of a property. It's the constant 
> sluggish redraw when the interface is supposed to be snappy, such as 
> mouse interaction.
> 
> > 
> > And I think the semantics of the ioctl(s) has so far been that
> > basically any page flip (whether or not it actually changes
> > the fb) still ends up doing whatever flushing is needed to
> > guarantee the fb contents are up to date on the screen (if
> > someone sneaked in some front buffer rendering in between).
> > Ie. you could even use basically a nop page flip in place
> > of dirtyfb.
> 
> That's why full updates are still the default. Only in cases where it's 
> save to avoid an update of unaffected planes, we do so.

Umm. Maybe I misread the patch in my haste but it seemed 
that you consider the thing undamaged if the fb pointer
was unchanged. That goes against what I wrote above.

Though I don't really know if a there is software relying on
that behaviuor. I suppose one idea could be to keep that
behaviour for the legacy ioctls but not for atomic. Ee. any
fb directly specified in a legacy setcrtc/setplane/pageflip
is always considered fully damaged. But including an the same
fb in the atomic ioctl does not imply damage. That would
mean atomic has to rely on specifying damage explicitly, and
any userspace that doesn't do that will be broken.

Or we could introduce a client cap for it I guess, but that
would require (minimal) userspace changes.

> 
> I know that we don't give performance guarantees to userspace. But using 
> cursor/overlay planes should be faster then not using them. I think 
> that's a reasonable assumption for userspace to make.
> 
> > 
> > Another thing the ioctls have always done is actually perform
> > the commit whether anything changed or nor. That is, they
> > still do all the same the same vblanky stuff and the commit
> > takes the same amount of time. Not sure if your idea is
> > to potentially short circuit the entire thing and make it
> > take no time at all?
> 
> The patches don't change the overall commit logics. All they do is to 
> set damage updates to a size of 0 if a plane reliably does not need an 
> update.

What I gathered is that you seemed to only consider the fb 
contents as something that needs damage handling. That is not
the case for stuff like eDP PSR and DSI command mode displays
where we need to upload a new full frame whenever also the
non-damaged fb contents would get transformed by the hardware
on the way to the remote frambuffer. That would mean any change
in eg. scanout coordinates, color management, etc.

-- 
Ville Syrjälä
Intel

Re: [PATCH 4/5] drm/damage-helper: Do partial updates if framebuffer has not been changed

[Thomas Zimmermann]

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

Hi

Am 21.09.22 um 10:37 schrieb Ville Syrjälä:
> On Wed, Sep 21, 2022 at 09:59:10AM +0200, Thomas Zimmermann wrote:
>> Hi Ville
>>
>> Am 20.09.22 um 16:31 schrieb Ville Syrjälä:
>>> On Tue, Sep 20, 2022 at 03:56:18PM +0200, Thomas Zimmermann wrote:
>>>> Set partial updates on a plane if the framebuffer has not been changed
>>>> on an atomic commit. If such a plane has damage clips, the driver will
>>>> use them; otherwise the update is effectively empty. Planes that change
>>>> their framebuffer still perform a full update.
>>>
>>> I have a feeling you're sort of papering over some inefficient
>>> userspace that's asking updates on planes that don't actually
>>> need them. I'm not sure adding more code for that is a particularly
>>> great idea. Wouldn't it be better to just fix the userspace code?
>>
>> Some more context might be in order:
>>
>> The ast driver currently uses VRAM helpers, which leads to many
>> problems; including blank screens from the low amount of video memory.
>> The best solution is to switch SHMEM with BOs on system meory. The video
>> memory is only the internal buffer for scanout. This update involves a
>> mempcy from the BO to video memory.
>>
>> Ast's hardware is really slow, so it makes sense to reduce the updates
>> to video memory to a minimum. There's support for cursor planes, which
>> really makes a difference here.
>>
>> But doing any cursor-plane update (e.g., moving, animation) with SHMEM
>> and the current damage helpers always results in a full-screen memcpy
>> from the BO to the video memory for the primary plane. As the ast
>> hardware is slow, performance goes down to a an estimated 5 frame per
>> seconds. After moving the mouse, one can watch the mouse cursor follow
>> along the screen for the next seconds. Userspace sends the movement
>> information and DRM slowly processes them. The same can be observed for
>> cursor animation.
>>
>> The problem is that each change to the cursor plane results in an
>> atomic_update call for the primary plane. Not having damage information
>> for the plane just means 'update everything'. Not a problem if redrawing
>> consists of reprogramming the scanout address. For a memcpy it's not
>> feasible.
>>
>> So can this be fixed in userspace? No realistically IMHO. I've seen this
>> problem on Weston, Wayland-Gnome and X11-Gnome. And they are all
>> problematic in their own way. (That's why there are multiple patches
>> needed.) For example, X11 uses the legacy mouse ioctl, which one of the
>> patches fixes. The other userspace needs the other heuristics. A
>> potential userspace fix would mean to always set empty-damage
>> information on all planes that don't get an update. And I don't consider
>> X11 fixable TBH.
>>
>>>
>>> Any property change on the plane could need a full plane
>>> update as well (eg. some color mangement stuff etc.). So you'd
>>> have to keep adding exceptions to that list whenever new
>>> properties are added.
>>
>> It's not about the occasional change of a property. It's the constant
>> sluggish redraw when the interface is supposed to be snappy, such as
>> mouse interaction.
>>
>>>
>>> And I think the semantics of the ioctl(s) has so far been that
>>> basically any page flip (whether or not it actually changes
>>> the fb) still ends up doing whatever flushing is needed to
>>> guarantee the fb contents are up to date on the screen (if
>>> someone sneaked in some front buffer rendering in between).
>>> Ie. you could even use basically a nop page flip in place
>>> of dirtyfb.
>>
>> That's why full updates are still the default. Only in cases where it's
>> save to avoid an update of unaffected planes, we do so.
> 
> Umm. Maybe I misread the patch in my haste but it seemed
> that you consider the thing undamaged if the fb pointer
> was unchanged. That goes against what I wrote above.

I try to resolve that in patch 5. The fb_dirty flag signals that the 
framebuffer's content changed in some way. Patches 4 and 5 could be seen 
as one change, but that might overload the resulting patch. (Maybe it's 
not well designed overall. :/)

> 
> Though I don't really know if a there is software relying on
> that behaviuor. I suppose one idea could be to keep that
> behaviour for the legacy ioctls but not for atomic. Ee. any
> fb directly specified in a legacy setcrtc/setplane/pageflip
> is always considered fully damaged. But including an the same

Assuming the specified fb to be damaged seems like a non-issue to me.

The problem is with the other framebuffers: if userspace sends us a 
CURSOR_MOVE ioctl, we can safely assume the cursor fb to be damaged. But 
we don't want the primary plane's fb to be damaged. Or else we'd redraw 
the full primary plane.


> fb in the atomic ioctl does not imply damage. That would
> mean atomic has to rely on specifying damage explicitly, and
> any userspace that doesn't do that will be broken.

The problem again is not in the damage information on planes that 
legitimately need a redraw. It's all the other planes that are being 
redrawn as well. Or is there some scenario that I don't see?

> 
> Or we could introduce a client cap for it I guess, but that
> would require (minimal) userspace changes.
> 
>>
>> I know that we don't give performance guarantees to userspace. But using
>> cursor/overlay planes should be faster then not using them. I think
>> that's a reasonable assumption for userspace to make.
>>
>>>
>>> Another thing the ioctls have always done is actually perform
>>> the commit whether anything changed or nor. That is, they
>>> still do all the same the same vblanky stuff and the commit
>>> takes the same amount of time. Not sure if your idea is
>>> to potentially short circuit the entire thing and make it
>>> take no time at all?
>>
>> The patches don't change the overall commit logics. All they do is to
>> set damage updates to a size of 0 if a plane reliably does not need an
>> update.
> 
> What I gathered is that you seemed to only consider the fb
> contents as something that needs damage handling. That is not
> the case for stuff like eDP PSR and DSI command mode displays
> where we need to upload a new full frame whenever also the
> non-damaged fb contents would get transformed by the hardware
> on the way to the remote frambuffer. That would mean any change
> in eg. scanout coordinates, color management, etc.

There is support for changing scanout coordinates in 
drm_atomic_helper_damage_iter_init() in patch 2. In general, maybe the 
heuristic needs to be stricter to only handle updates that only change 
damage.

For now, the problem only happens after converting ast to SHMEM. All the 
other SHMEM-based drivers use a single plane where the problem doesn't 
happen; or only reprogram the scanout address, which is fast. If the 
damage-handling logic imposes problems on other drivers, some of it 
could possibly be moved into ast itself.

Best regards
Thomas

> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

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

Re: [PATCH 4/5] drm/damage-helper: Do partial updates if framebuffer has not been changed

[Ville Syrjälä]

On Wed, Sep 21, 2022 at 11:39:51AM +0200, Thomas Zimmermann wrote:
> Hi
> 
> Am 21.09.22 um 10:37 schrieb Ville Syrjälä:
> > On Wed, Sep 21, 2022 at 09:59:10AM +0200, Thomas Zimmermann wrote:
> >> Hi Ville
> >>
> >> Am 20.09.22 um 16:31 schrieb Ville Syrjälä:
> >>> On Tue, Sep 20, 2022 at 03:56:18PM +0200, Thomas Zimmermann wrote:
> >>>> Set partial updates on a plane if the framebuffer has not been changed
> >>>> on an atomic commit. If such a plane has damage clips, the driver will
> >>>> use them; otherwise the update is effectively empty. Planes that change
> >>>> their framebuffer still perform a full update.
> >>>
> >>> I have a feeling you're sort of papering over some inefficient
> >>> userspace that's asking updates on planes that don't actually
> >>> need them. I'm not sure adding more code for that is a particularly
> >>> great idea. Wouldn't it be better to just fix the userspace code?
> >>
> >> Some more context might be in order:
> >>
> >> The ast driver currently uses VRAM helpers, which leads to many
> >> problems; including blank screens from the low amount of video memory.
> >> The best solution is to switch SHMEM with BOs on system meory. The video
> >> memory is only the internal buffer for scanout. This update involves a
> >> mempcy from the BO to video memory.
> >>
> >> Ast's hardware is really slow, so it makes sense to reduce the updates
> >> to video memory to a minimum. There's support for cursor planes, which
> >> really makes a difference here.
> >>
> >> But doing any cursor-plane update (e.g., moving, animation) with SHMEM
> >> and the current damage helpers always results in a full-screen memcpy
> >> from the BO to the video memory for the primary plane. As the ast
> >> hardware is slow, performance goes down to a an estimated 5 frame per
> >> seconds. After moving the mouse, one can watch the mouse cursor follow
> >> along the screen for the next seconds. Userspace sends the movement
> >> information and DRM slowly processes them. The same can be observed for
> >> cursor animation.
> >>
> >> The problem is that each change to the cursor plane results in an
> >> atomic_update call for the primary plane. Not having damage information
> >> for the plane just means 'update everything'. Not a problem if redrawing
> >> consists of reprogramming the scanout address. For a memcpy it's not
> >> feasible.
> >>
> >> So can this be fixed in userspace? No realistically IMHO. I've seen this
> >> problem on Weston, Wayland-Gnome and X11-Gnome. And they are all
> >> problematic in their own way. (That's why there are multiple patches
> >> needed.) For example, X11 uses the legacy mouse ioctl, which one of the
> >> patches fixes. The other userspace needs the other heuristics. A
> >> potential userspace fix would mean to always set empty-damage
> >> information on all planes that don't get an update. And I don't consider
> >> X11 fixable TBH.
> >>
> >>>
> >>> Any property change on the plane could need a full plane
> >>> update as well (eg. some color mangement stuff etc.). So you'd
> >>> have to keep adding exceptions to that list whenever new
> >>> properties are added.
> >>
> >> It's not about the occasional change of a property. It's the constant
> >> sluggish redraw when the interface is supposed to be snappy, such as
> >> mouse interaction.
> >>
> >>>
> >>> And I think the semantics of the ioctl(s) has so far been that
> >>> basically any page flip (whether or not it actually changes
> >>> the fb) still ends up doing whatever flushing is needed to
> >>> guarantee the fb contents are up to date on the screen (if
> >>> someone sneaked in some front buffer rendering in between).
> >>> Ie. you could even use basically a nop page flip in place
> >>> of dirtyfb.
> >>
> >> That's why full updates are still the default. Only in cases where it's
> >> save to avoid an update of unaffected planes, we do so.
> > 
> > Umm. Maybe I misread the patch in my haste but it seemed
> > that you consider the thing undamaged if the fb pointer
> > was unchanged. That goes against what I wrote above.
> 
> I try to resolve that in patch 5. The fb_dirty flag signals that the 
> framebuffer's content changed in some way. Patches 4 and 5 could be seen 
> as one change, but that might overload the resulting patch. (Maybe it's 
> not well designed overall. :/)
> 
> > 
> > Though I don't really know if a there is software relying on
> > that behaviuor. I suppose one idea could be to keep that
> > behaviour for the legacy ioctls but not for atomic. Ee. any
> > fb directly specified in a legacy setcrtc/setplane/pageflip
> > is always considered fully damaged. But including an the same
> 
> Assuming the specified fb to be damaged seems like a non-issue to me.
> 
> The problem is with the other framebuffers: if userspace sends us a 
> CURSOR_MOVE ioctl, we can safely assume the cursor fb to be damaged. But 
> we don't want the primary plane's fb to be damaged. Or else we'd redraw 
> the full primary plane.
> 
> 
> > fb in the atomic ioctl does not imply damage. That would
> > mean atomic has to rely on specifying damage explicitly, and
> > any userspace that doesn't do that will be broken.
> 
> The problem again is not in the damage information on planes that 
> legitimately need a redraw. It's all the other planes that are being 
> redrawn as well. Or is there some scenario that I don't see?

I thought we're talking about eg. a cursor update that also
includes the primary plane because apparently userspace is lazy.

I think what you're saying is that currently there is no
damage information for the primary plane so you're attempting
to infer it based on whether the fb property changed or not.

And what I was saying is that IIRC historically specifying
the same fb again has still implied full damage. Changing
that behaviour may or may not break exising userspace.

> 
> > 
> > Or we could introduce a client cap for it I guess, but that
> > would require (minimal) userspace changes.
> > 
> >>
> >> I know that we don't give performance guarantees to userspace. But using
> >> cursor/overlay planes should be faster then not using them. I think
> >> that's a reasonable assumption for userspace to make.
> >>
> >>>
> >>> Another thing the ioctls have always done is actually perform
> >>> the commit whether anything changed or nor. That is, they
> >>> still do all the same the same vblanky stuff and the commit
> >>> takes the same amount of time. Not sure if your idea is
> >>> to potentially short circuit the entire thing and make it
> >>> take no time at all?
> >>
> >> The patches don't change the overall commit logics. All they do is to
> >> set damage updates to a size of 0 if a plane reliably does not need an
> >> update.
> > 
> > What I gathered is that you seemed to only consider the fb
> > contents as something that needs damage handling. That is not
> > the case for stuff like eDP PSR and DSI command mode displays
> > where we need to upload a new full frame whenever also the
> > non-damaged fb contents would get transformed by the hardware
> > on the way to the remote frambuffer. That would mean any change
> > in eg. scanout coordinates, color management, etc.
> 
> There is support for changing scanout coordinates in 
> drm_atomic_helper_damage_iter_init() in patch 2. In general, maybe the 
> heuristic needs to be stricter to only handle updates that only change 
> damage.
> 
> For now, the problem only happens after converting ast to SHMEM. All the 
> other SHMEM-based drivers use a single plane where the problem doesn't 
> happen; or only reprogram the scanout address, which is fast. If the 
> damage-handling logic imposes problems on other drivers, some of it 
> could possibly be moved into ast itself.

Maybe we have two clearly separate classes of uses case:
1. devices where only damage to the fb contents matter (eg. some kind of
   shadow fb that is the same size as the real fb).
2. devices where everything about the scanout process matters (eg. PSR)
?

Maybe there should be helpers to deal with just the first case,
and then some more helpers (or just driver code) to pile the rest
on top as well when needed?

-- 
Ville Syrjälä
Intel

Re: [PATCH 4/5] drm/damage-helper: Do partial updates if framebuffer has not been changed

[Thomas Zimmermann]

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

Hi

Am 21.09.22 um 12:18 schrieb Ville Syrjälä:
[...]
>>
>>>
>>> Though I don't really know if a there is software relying on
>>> that behaviuor. I suppose one idea could be to keep that
>>> behaviour for the legacy ioctls but not for atomic. Ee. any
>>> fb directly specified in a legacy setcrtc/setplane/pageflip
>>> is always considered fully damaged. But including an the same
>>
>> Assuming the specified fb to be damaged seems like a non-issue to me.
>>
>> The problem is with the other framebuffers: if userspace sends us a
>> CURSOR_MOVE ioctl, we can safely assume the cursor fb to be damaged. But
>> we don't want the primary plane's fb to be damaged. Or else we'd redraw
>> the full primary plane.
>>
>>
>>> fb in the atomic ioctl does not imply damage. That would
>>> mean atomic has to rely on specifying damage explicitly, and
>>> any userspace that doesn't do that will be broken.
>>
>> The problem again is not in the damage information on planes that
>> legitimately need a redraw. It's all the other planes that are being
>> redrawn as well. Or is there some scenario that I don't see?
> 
> I thought we're talking about eg. a cursor update that also
> includes the primary plane because apparently userspace is lazy.
> 
> I think what you're saying is that currently there is no
> damage information for the primary plane so you're attempting
> to infer it based on whether the fb property changed or not.

Correct.

> 
> And what I was saying is that IIRC historically specifying
> the same fb again has still implied full damage. Changing
> that behaviour may or may not break exising userspace.

I cannot answer the question. The three cases I've seen at least worked 
with the new semantics. I think Daniel once mentioned that we already 
expect userspace to call the DIRTYFB ioctl after changing a 
framebuffer's content.

> 
>>
>>>
>>> Or we could introduce a client cap for it I guess, but that
>>> would require (minimal) userspace changes.
>>>
>>>>
>>>> I know that we don't give performance guarantees to userspace. But using
>>>> cursor/overlay planes should be faster then not using them. I think
>>>> that's a reasonable assumption for userspace to make.
>>>>
>>>>>
>>>>> Another thing the ioctls have always done is actually perform
>>>>> the commit whether anything changed or nor. That is, they
>>>>> still do all the same the same vblanky stuff and the commit
>>>>> takes the same amount of time. Not sure if your idea is
>>>>> to potentially short circuit the entire thing and make it
>>>>> take no time at all?
>>>>
>>>> The patches don't change the overall commit logics. All they do is to
>>>> set damage updates to a size of 0 if a plane reliably does not need an
>>>> update.
>>>
>>> What I gathered is that you seemed to only consider the fb
>>> contents as something that needs damage handling. That is not
>>> the case for stuff like eDP PSR and DSI command mode displays
>>> where we need to upload a new full frame whenever also the
>>> non-damaged fb contents would get transformed by the hardware
>>> on the way to the remote frambuffer. That would mean any change
>>> in eg. scanout coordinates, color management, etc.
>>
>> There is support for changing scanout coordinates in
>> drm_atomic_helper_damage_iter_init() in patch 2. In general, maybe the
>> heuristic needs to be stricter to only handle updates that only change
>> damage.
>>
>> For now, the problem only happens after converting ast to SHMEM. All the
>> other SHMEM-based drivers use a single plane where the problem doesn't
>> happen; or only reprogram the scanout address, which is fast. If the
>> damage-handling logic imposes problems on other drivers, some of it
>> could possibly be moved into ast itself.
> 
> Maybe we have two clearly separate classes of uses case:
> 1. devices where only damage to the fb contents matter (eg. some kind of
>     shadow fb that is the same size as the real fb).
> 2. devices where everything about the scanout process matters (eg. PSR)
> ?
> 
> Maybe there should be helpers to deal with just the first case,
> and then some more helpers (or just driver code) to pile the rest
> on top as well when needed?

Makes sense.

I know that these plane-state are not good style, but with them in 
place, much of the heuristic could be moved from 
drm_atomic_helper_check_plane_damage() into the driver if necessary.

Best regards
Thomas

> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

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

Re: [PATCH 4/5] drm/damage-helper: Do partial updates if framebuffer has not been changed

[Thomas Zimmermann]

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

Hi

Am 21.09.22 um 12:18 schrieb Ville Syrjälä:
> 
> Maybe we have two clearly separate classes of uses case:
> 1. devices where only damage to the fb contents matter (eg. some kind of
>     shadow fb that is the same size as the real fb).
> 2. devices where everything about the scanout process matters (eg. PSR)
> ?
> 
> Maybe there should be helpers to deal with just the first case,
> and then some more helpers (or just driver code) to pile the rest
> on top as well when needed?

Does it help if I include my ast patches for review here? We'd have a 
user of the code.

Best regards
Thomas

> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

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

[PATCH 5/5] drm/damage-helper: Avoid partial updates for DIRTYFB without damage

[Thomas Zimmermann]

Always do a full plane update when userspace sends a DIRTYFB ioctl
without damage information. Userspace not changing the framebuffer
or marking changed regions can easily be interpreted as if there was
no change at all. Therefore set the new fb_dirty flag on all plane's
with a dirty framebuffer and fallback to a full plane update if
necessary.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/drm_atomic_state_helper.c | 1 +
 drivers/gpu/drm/drm_damage_helper.c       | 7 ++++---
 include/drm/drm_plane.h                   | 8 ++++++++
 3 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c b/drivers/gpu/drm/drm_atomic_state_helper.c
index 94818fd4dd8f..b7523d56b06f 100644
--- a/drivers/gpu/drm/drm_atomic_state_helper.c
+++ b/drivers/gpu/drm/drm_atomic_state_helper.c
@@ -340,6 +340,7 @@ void __drm_atomic_helper_plane_duplicate_state(struct drm_plane *plane,
 	state->fb_damage_clips = NULL;
 	state->fb_damage_partial_update = false;
 	state->fb_changed = false;
+	state->fb_dirty = false;
 }
 EXPORT_SYMBOL(__drm_atomic_helper_plane_duplicate_state);
 
diff --git a/drivers/gpu/drm/drm_damage_helper.c b/drivers/gpu/drm/drm_damage_helper.c
index f43abf02df5b..e884987a944c 100644
--- a/drivers/gpu/drm/drm_damage_helper.c
+++ b/drivers/gpu/drm/drm_damage_helper.c
@@ -102,10 +102,10 @@ void drm_atomic_helper_check_plane_damage(struct drm_atomic_state *state,
 		 * Damage clips are a good indicator for partial updates.
 		 */
 		partial_update = true;
-	} else if (!new_plane_state->fb_changed) {
+	} else if (!new_plane_state->fb_changed && !new_plane_state->fb_dirty) {
 		/*
-		 * Also set a partial update if the framebuffer did not
-		 * change. Without damage clips set, this will effectively
+		 * Also set a partial update if the framebuffer or its content
+		 * did not change. Without damage clips set, this will effectively
 		 * not update the plane. The exception is with full modeset
 		 * operations, where we do full plane update even if the
 		 * framebuffer did not change. We already handled this case
@@ -214,6 +214,7 @@ int drm_atomic_helper_dirtyfb(struct drm_framebuffer *fb,
 			goto out;
 		}
 
+		plane_state->fb_dirty = true;
 		drm_property_replace_blob(&plane_state->fb_damage_clips,
 					  damage);
 	}
diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
index 8c2d0a2eb760..2b22707eb116 100644
--- a/include/drm/drm_plane.h
+++ b/include/drm/drm_plane.h
@@ -235,6 +235,14 @@ struct drm_plane_state {
 	 */
 	bool fb_changed : 1;
 
+	/**
+	 * @fb_dirty: @fb's content has been marked as dirty. Used by the
+	 * atomic helpers and drivers to steer the atomic commit control flow.
+	 * The flag signals that the frambuffer's content has been changed even
+	 * if no damage clips have been installed.
+	 */
+	bool fb_dirty : 1;
+
 	/**
 	 * @scaling_filter:
 	 *
-- 
2.37.3