[PATCH v2 0/2] drm: Add new plane helpers to begin/end FB access

[PATCH v2 0/2] drm: Add new plane helpers to begin/end FB access

[Thomas Zimmermann]

This patchset adds the callbacks begin_fb_access and end_fb_access
to struct drm_plane_helper_funcs. They provide hooks to acquire and
release resources that are only held during the commit. It adds
related simple-KMS helpers and converts shadow-plane helpers.

As resource allocation often can fail, we have to do this before an
atomic commit starts the update in order to handle the error. But
prepare_fb is only reverted after the next pageflip. We want to
reduce the time resources are held for a commit, which requires the
new hooks.

With this in place, move shadow-plane helpers' automatic vmap/vunmap
behind the new callbacks. The shadow-plane mapping is only required
during the atomic commit.

Tested with combinations of radeon, udl and simpledrm under X11, Weston
and Wayland-Gnome.

Thomas Zimmermann (2):
  drm/atomic-helper: Add {begin,end}_fb_access to plane helpers
  drm/gem: Implement shadow-plane {begin,end}_fb_access with vmap

 drivers/gpu/drm/drm_atomic_helper.c      | 34 ++++++++++--
 drivers/gpu/drm/drm_gem_atomic_helper.c  | 66 +++++++++++-------------
 drivers/gpu/drm/drm_simple_kms_helper.c  | 26 ++++++++++
 include/drm/drm_gem_atomic_helper.h      | 20 +++----
 include/drm/drm_modeset_helper_vtables.h | 41 ++++++++++++++-
 include/drm/drm_simple_kms_helper.h      | 20 +++++++
 6 files changed, 157 insertions(+), 50 deletions(-)


base-commit: 746559738f1335241ea686566cb654847c20d7a4
-- 
2.38.0

[PATCH v2 1/2] drm/atomic-helper: Add {begin, end}_fb_access to plane helpers

[Thomas Zimmermann]

Add {begin,end}_fb_access helpers to run at the beginning and end of
an atomic commit. The begin_fb_access helper aquires resources that
are necessary to perform the atomic commit. It it similar to prepare_fb,
except that the resources are to be released at the end of the commit.
Resources acquired by prepare_fb are held until after the next pageflip.

The end_fb_access helper performs the corresponding resource cleanup.
Atomic helpers call it with the new plane state. This is differnt from
cleanup_fb, which releases resources of the old plane state.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/drm_atomic_helper.c      | 34 ++++++++++++++++++--
 drivers/gpu/drm/drm_gem_atomic_helper.c  | 19 ++++++-----
 drivers/gpu/drm/drm_simple_kms_helper.c  | 26 +++++++++++++++
 include/drm/drm_modeset_helper_vtables.h | 41 +++++++++++++++++++++++-
 include/drm/drm_simple_kms_helper.h      | 20 ++++++++++++
 5 files changed, 126 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 1a586b3c454b4..d579fd8f7cb83 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -2536,7 +2536,7 @@ int drm_atomic_helper_prepare_planes(struct drm_device *dev,
 		if (funcs->prepare_fb) {
 			ret = funcs->prepare_fb(plane, new_plane_state);
 			if (ret)
-				goto fail;
+				goto fail_prepare_fb;
 		} else {
 			WARN_ON_ONCE(funcs->cleanup_fb);
 
@@ -2545,13 +2545,34 @@ int drm_atomic_helper_prepare_planes(struct drm_device *dev,
 
 			ret = drm_gem_plane_helper_prepare_fb(plane, new_plane_state);
 			if (ret)
-				goto fail;
+				goto fail_prepare_fb;
+		}
+	}
+
+	for_each_new_plane_in_state(state, plane, new_plane_state, i) {
+		const struct drm_plane_helper_funcs *funcs = plane->helper_private;
+
+		if (funcs->begin_fb_access) {
+			ret = funcs->begin_fb_access(plane, new_plane_state);
+			if (ret)
+				goto fail_begin_fb_access;
 		}
 	}
 
 	return 0;
 
-fail:
+fail_begin_fb_access:
+	for_each_new_plane_in_state(state, plane, new_plane_state, j) {
+		const struct drm_plane_helper_funcs *funcs = plane->helper_private;
+
+		if (j >= i)
+			continue;
+
+		if (funcs->end_fb_access)
+			funcs->end_fb_access(plane, new_plane_state);
+	}
+	i = j; /* set i to upper limit to cleanup all planes */
+fail_prepare_fb:
 	for_each_new_plane_in_state(state, plane, new_plane_state, j) {
 		const struct drm_plane_helper_funcs *funcs;
 
@@ -2827,6 +2848,13 @@ void drm_atomic_helper_cleanup_planes(struct drm_device *dev,
 	struct drm_plane_state *old_plane_state, *new_plane_state;
 	int i;
 
+	for_each_oldnew_plane_in_state(old_state, plane, old_plane_state, new_plane_state, i) {
+		const struct drm_plane_helper_funcs *funcs = plane->helper_private;
+
+		if (funcs->end_fb_access)
+			funcs->end_fb_access(plane, new_plane_state);
+	}
+
 	for_each_oldnew_plane_in_state(old_state, plane, old_plane_state, new_plane_state, i) {
 		const struct drm_plane_helper_funcs *funcs;
 		struct drm_plane_state *plane_state;
diff --git a/drivers/gpu/drm/drm_gem_atomic_helper.c b/drivers/gpu/drm/drm_gem_atomic_helper.c
index b6a0110eb64af..1de0a08afd86e 100644
--- a/drivers/gpu/drm/drm_gem_atomic_helper.c
+++ b/drivers/gpu/drm/drm_gem_atomic_helper.c
@@ -414,16 +414,14 @@ void drm_gem_cleanup_shadow_fb(struct drm_plane *plane, struct drm_plane_state *
 EXPORT_SYMBOL(drm_gem_cleanup_shadow_fb);
 
 /**
- * drm_gem_simple_kms_prepare_shadow_fb - prepares shadow framebuffers
+ * drm_gem_simple_kms_begin_shadow_fb_access - prepares shadow framebuffers for CPU access
  * @pipe: the simple display pipe
  * @plane_state: the plane state of type struct drm_shadow_plane_state
  *
- * This function implements struct drm_simple_display_funcs.prepare_fb. It
- * maps all buffer objects of the plane's framebuffer into kernel address
- * space and stores them in struct drm_shadow_plane_state.map. The
- * framebuffer will be synchronized as part of the atomic commit.
+ * This function implements struct drm_simple_display_funcs.begin_fb_access.
  *
- * See drm_gem_simple_kms_cleanup_shadow_fb() for cleanup.
+ * See drm_gem_begin_shadow_fb_access() for details and
+ * drm_gem_simple_kms_cleanup_shadow_fb() for cleanup.
  *
  * Returns:
  * 0 on success, or a negative errno code otherwise.
@@ -436,14 +434,15 @@ int drm_gem_simple_kms_prepare_shadow_fb(struct drm_simple_display_pipe *pipe,
 EXPORT_SYMBOL(drm_gem_simple_kms_prepare_shadow_fb);
 
 /**
- * drm_gem_simple_kms_cleanup_shadow_fb - releases shadow framebuffers
+ * drm_gem_simple_kms_end_shadow_fb_access - releases shadow framebuffers from CPU access
  * @pipe: the simple display pipe
  * @plane_state: the plane state of type struct drm_shadow_plane_state
  *
- * This function implements struct drm_simple_display_funcs.cleanup_fb.
- * This function unmaps all buffer objects of the plane's framebuffer.
+ * This function implements struct drm_simple_display_funcs.end_fb_access.
+ * It undoes all effects of drm_gem_simple_kms_begin_shadow_fb_access() in
+ * reverse order.
  *
- * See drm_gem_simple_kms_prepare_shadow_fb().
+ * See drm_gem_simple_kms_begin_shadow_fb_access().
  */
 void drm_gem_simple_kms_cleanup_shadow_fb(struct drm_simple_display_pipe *pipe,
 					  struct drm_plane_state *plane_state)
diff --git a/drivers/gpu/drm/drm_simple_kms_helper.c b/drivers/gpu/drm/drm_simple_kms_helper.c
index 31233c6ae3c42..3ef420ec4534a 100644
--- a/drivers/gpu/drm/drm_simple_kms_helper.c
+++ b/drivers/gpu/drm/drm_simple_kms_helper.c
@@ -285,6 +285,30 @@ static void drm_simple_kms_plane_cleanup_fb(struct drm_plane *plane,
 	pipe->funcs->cleanup_fb(pipe, state);
 }
 
+static int drm_simple_kms_plane_begin_fb_access(struct drm_plane *plane,
+						struct drm_plane_state *new_plane_state)
+{
+	struct drm_simple_display_pipe *pipe;
+
+	pipe = container_of(plane, struct drm_simple_display_pipe, plane);
+	if (!pipe->funcs || !pipe->funcs->begin_fb_access)
+		return 0;
+
+	return pipe->funcs->begin_fb_access(pipe, new_plane_state);
+}
+
+static void drm_simple_kms_plane_end_fb_access(struct drm_plane *plane,
+					       struct drm_plane_state *new_plane_state)
+{
+	struct drm_simple_display_pipe *pipe;
+
+	pipe = container_of(plane, struct drm_simple_display_pipe, plane);
+	if (!pipe->funcs || !pipe->funcs->end_fb_access)
+		return;
+
+	pipe->funcs->end_fb_access(pipe, new_plane_state);
+}
+
 static bool drm_simple_kms_format_mod_supported(struct drm_plane *plane,
 						uint32_t format,
 						uint64_t modifier)
@@ -295,6 +319,8 @@ static bool drm_simple_kms_format_mod_supported(struct drm_plane *plane,
 static const struct drm_plane_helper_funcs drm_simple_kms_plane_helper_funcs = {
 	.prepare_fb = drm_simple_kms_plane_prepare_fb,
 	.cleanup_fb = drm_simple_kms_plane_cleanup_fb,
+	.begin_fb_access = drm_simple_kms_plane_begin_fb_access,
+	.end_fb_access = drm_simple_kms_plane_end_fb_access,
 	.atomic_check = drm_simple_kms_plane_atomic_check,
 	.atomic_update = drm_simple_kms_plane_atomic_update,
 };
diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h
index fafa70ac1337f..d9f2254a039aa 100644
--- a/include/drm/drm_modeset_helper_vtables.h
+++ b/include/drm/drm_modeset_helper_vtables.h
@@ -1184,11 +1184,20 @@ struct drm_plane_helper_funcs {
 	 * can call drm_gem_plane_helper_prepare_fb() from their @prepare_fb
 	 * hook.
 	 *
+	 * The resources acquired in @prepare_fb persist after the end of
+	 * the atomic commit. Resources that can be release at the commit's end
+	 * should be acquired in @begin_fb_access and released in @end_fb_access.
+	 * For example, a GEM buffer's pin operation belongs into @prepare_fb to
+	 * keep the buffer pinned after the commit. But a vmap operation for
+	 * shadow-plane helpers belongs into @begin_fb_access, so that atomic
+	 * helpers remove the mapping at the end of the commit.
+	 *
 	 * The helpers will call @cleanup_fb with matching arguments for every
 	 * successful call to this hook.
 	 *
 	 * This callback is used by the atomic modeset helpers and by the
-	 * transitional plane helpers, but it is optional.
+	 * transitional plane helpers, but it is optional. See @begin_fb_access
+	 * for preparing per-commit resources.
 	 *
 	 * RETURNS:
 	 *
@@ -1211,6 +1220,36 @@ struct drm_plane_helper_funcs {
 	void (*cleanup_fb)(struct drm_plane *plane,
 			   struct drm_plane_state *old_state);
 
+	/**
+	 * @begin_fb_access:
+	 *
+	 * This hook prepares the plane for access during an atomic commit.
+	 * In contrast to @prepare_fb, resources acquired in @begin_fb_access,
+	 * are released at the end of the atomic commit in @end_fb_access.
+	 *
+	 * For example, with shadow-plane helpers, the GEM buffer's vmap
+	 * operation belongs into @begin_fb_access, so that the buffer's
+	 * memory will be unmapped at the end of the commit in @end_fb_access.
+	 * But a GEM buffer's pin operation belongs into @prepare_fb
+	 * to keep the buffer pinned after the commit.
+	 *
+	 * The callback is used by the atomic modeset helpers, but it is optional.
+	 * See @end_fb_cleanup for undoing the effects of @begin_fb_access and
+	 * @prepare_fb for acquiring resources until the next pageflip.
+	 *
+	 * Returns:
+	 * 0 on success, or a negative errno code otherwise.
+	 */
+	int (*begin_fb_access)(struct drm_plane *plane, struct drm_plane_state *new_plane_state);
+
+	/**
+	 * @end_fb_access:
+	 *
+	 * This hook cleans up resources allocated by @begin_fb_access. It it called
+	 * at the end of a commit for the new plane state.
+	 */
+	void (*end_fb_access)(struct drm_plane *plane, struct drm_plane_state *new_plane_state);
+
 	/**
 	 * @atomic_check:
 	 *
diff --git a/include/drm/drm_simple_kms_helper.h b/include/drm/drm_simple_kms_helper.h
index 0b3647e614dd3..2298fe3af4cd7 100644
--- a/include/drm/drm_simple_kms_helper.h
+++ b/include/drm/drm_simple_kms_helper.h
@@ -135,6 +135,26 @@ struct drm_simple_display_pipe_funcs {
 	void (*cleanup_fb)(struct drm_simple_display_pipe *pipe,
 			   struct drm_plane_state *plane_state);
 
+	/**
+	 * @begin_fb_access:
+	 *
+	 * Optional, called by &drm_plane_helper_funcs.begin_fb_access. Please read
+	 * the documentation for the &drm_plane_helper_funcs.begin_fb_access hook for
+	 * more details.
+	 */
+	int (*begin_fb_access)(struct drm_simple_display_pipe *pipe,
+			       struct drm_plane_state *new_plane_state);
+
+	/**
+	 * @end_fb_access:
+	 *
+	 * Optional, called by &drm_plane_helper_funcs.end_fb_access. Please read
+	 * the documentation for the &drm_plane_helper_funcs.end_fb_access hook for
+	 * more details.
+	 */
+	void (*end_fb_access)(struct drm_simple_display_pipe *pipe,
+			      struct drm_plane_state *plane_state);
+
 	/**
 	 * @enable_vblank:
 	 *
-- 
2.38.0

Re: [PATCH v2 1/2] drm/atomic-helper: Add {begin,end}_fb_access to plane helpers

[Javier Martinez Canillas]

Hello Thomas,

On 10/25/22 12:17, Thomas Zimmermann wrote:> Add {begin,end}_fb_access helpers to run at the beginning and end of
> an atomic commit. The begin_fb_access helper aquires resources that

acquires

> are necessary to perform the atomic commit. It it similar to prepare_fb,
> except that the resources are to be released at the end of the commit.
> Resources acquired by prepare_fb are held until after the next pageflip.
> 
> The end_fb_access helper performs the corresponding resource cleanup.
> Atomic helpers call it with the new plane state. This is differnt from

different

> cleanup_fb, which releases resources of the old plane state.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---

The change makes sense to me.

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

[PATCH v2 2/2] drm/gem: Implement shadow-plane {begin, end}_fb_access with vmap

[Thomas Zimmermann]

Move the vmap code for shadow-plane helpers from prepare_fb to
begin_fb_access helpers. Vunmap is now performed at the end of
the current pageflip, instead of the end of the following pageflip.

Reduces the duration of the mapping from while the framebuffer is
being displayed to just the atomic commit. This is safe as outside
of the pageflip, nothing should access the mapped buffer memory.
Unmapping the framebuffer BO memory early allows reduces address-space
consumption and possibly allows for evicting the memory pages.

The change is effectively a rename of prepare_fb and cleanup_fb
implementations, plus updates to the shadow-plane init macro. As
there's no longer a prepare_fb helper for shadow planes, atomic
helpers will call drm_gem_plane_helper_prepare_fb() automatically.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/drm_gem_atomic_helper.c | 47 +++++++++++--------------
 include/drm/drm_gem_atomic_helper.h     | 20 +++++------
 2 files changed, 31 insertions(+), 36 deletions(-)

diff --git a/drivers/gpu/drm/drm_gem_atomic_helper.c b/drivers/gpu/drm/drm_gem_atomic_helper.c
index 1de0a08afd86e..e42800718f515 100644
--- a/drivers/gpu/drm/drm_gem_atomic_helper.c
+++ b/drivers/gpu/drm/drm_gem_atomic_helper.c
@@ -360,48 +360,43 @@ void drm_gem_reset_shadow_plane(struct drm_plane *plane)
 EXPORT_SYMBOL(drm_gem_reset_shadow_plane);
 
 /**
- * drm_gem_prepare_shadow_fb - prepares shadow framebuffers
+ * drm_gem_begin_shadow_fb_access - prepares shadow framebuffers for CPU access
  * @plane: the plane
  * @plane_state: the plane state of type struct drm_shadow_plane_state
  *
- * This function implements struct &drm_plane_helper_funcs.prepare_fb. It
+ * This function implements struct &drm_plane_helper_funcs.begin_fb_access. It
  * maps all buffer objects of the plane's framebuffer into kernel address
- * space and stores them in &struct drm_shadow_plane_state.map. The
- * framebuffer will be synchronized as part of the atomic commit.
+ * space and stores them in struct &drm_shadow_plane_state.map. The first data
+ * bytes are available in struct &drm_shadow_plane_state.data.
  *
- * See drm_gem_cleanup_shadow_fb() for cleanup.
+ * See drm_gem_end_shadow_fb_access() for cleanup.
  *
  * Returns:
  * 0 on success, or a negative errno code otherwise.
  */
-int drm_gem_prepare_shadow_fb(struct drm_plane *plane, struct drm_plane_state *plane_state)
+int drm_gem_begin_shadow_fb_access(struct drm_plane *plane, struct drm_plane_state *plane_state)
 {
 	struct drm_shadow_plane_state *shadow_plane_state = to_drm_shadow_plane_state(plane_state);
 	struct drm_framebuffer *fb = plane_state->fb;
-	int ret;
 
 	if (!fb)
 		return 0;
 
-	ret = drm_gem_plane_helper_prepare_fb(plane, plane_state);
-	if (ret)
-		return ret;
-
 	return drm_gem_fb_vmap(fb, shadow_plane_state->map, shadow_plane_state->data);
 }
-EXPORT_SYMBOL(drm_gem_prepare_shadow_fb);
+EXPORT_SYMBOL(drm_gem_begin_shadow_fb_access);
 
 /**
- * drm_gem_cleanup_shadow_fb - releases shadow framebuffers
+ * drm_gem_end_shadow_fb_access - releases shadow framebuffers from CPU access
  * @plane: the plane
  * @plane_state: the plane state of type struct drm_shadow_plane_state
  *
- * This function implements struct &drm_plane_helper_funcs.cleanup_fb.
- * This function unmaps all buffer objects of the plane's framebuffer.
+ * This function implements struct &drm_plane_helper_funcs.end_fb_access. It
+ * undoes all effects of drm_gem_begin_shadow_fb_access() in reverse order.
  *
- * See drm_gem_prepare_shadow_fb() for more information.
+ * See drm_gem_begin_shadow_fb_access() for more information.
  */
-void drm_gem_cleanup_shadow_fb(struct drm_plane *plane, struct drm_plane_state *plane_state)
+void drm_gem_end_shadow_fb_access(struct drm_plane *plane, struct drm_plane_state *plane_state)
 {
 	struct drm_shadow_plane_state *shadow_plane_state = to_drm_shadow_plane_state(plane_state);
 	struct drm_framebuffer *fb = plane_state->fb;
@@ -411,7 +406,7 @@ void drm_gem_cleanup_shadow_fb(struct drm_plane *plane, struct drm_plane_state *
 
 	drm_gem_fb_vunmap(fb, shadow_plane_state->map);
 }
-EXPORT_SYMBOL(drm_gem_cleanup_shadow_fb);
+EXPORT_SYMBOL(drm_gem_end_shadow_fb_access);
 
 /**
  * drm_gem_simple_kms_begin_shadow_fb_access - prepares shadow framebuffers for CPU access
@@ -426,12 +421,12 @@ EXPORT_SYMBOL(drm_gem_cleanup_shadow_fb);
  * Returns:
  * 0 on success, or a negative errno code otherwise.
  */
-int drm_gem_simple_kms_prepare_shadow_fb(struct drm_simple_display_pipe *pipe,
-					 struct drm_plane_state *plane_state)
+int drm_gem_simple_kms_begin_shadow_fb_access(struct drm_simple_display_pipe *pipe,
+					      struct drm_plane_state *plane_state)
 {
-	return drm_gem_prepare_shadow_fb(&pipe->plane, plane_state);
+	return drm_gem_begin_shadow_fb_access(&pipe->plane, plane_state);
 }
-EXPORT_SYMBOL(drm_gem_simple_kms_prepare_shadow_fb);
+EXPORT_SYMBOL(drm_gem_simple_kms_begin_shadow_fb_access);
 
 /**
  * drm_gem_simple_kms_end_shadow_fb_access - releases shadow framebuffers from CPU access
@@ -444,12 +439,12 @@ EXPORT_SYMBOL(drm_gem_simple_kms_prepare_shadow_fb);
  *
  * See drm_gem_simple_kms_begin_shadow_fb_access().
  */
-void drm_gem_simple_kms_cleanup_shadow_fb(struct drm_simple_display_pipe *pipe,
-					  struct drm_plane_state *plane_state)
+void drm_gem_simple_kms_end_shadow_fb_access(struct drm_simple_display_pipe *pipe,
+					     struct drm_plane_state *plane_state)
 {
-	drm_gem_cleanup_shadow_fb(&pipe->plane, plane_state);
+	drm_gem_end_shadow_fb_access(&pipe->plane, plane_state);
 }
-EXPORT_SYMBOL(drm_gem_simple_kms_cleanup_shadow_fb);
+EXPORT_SYMBOL(drm_gem_simple_kms_end_shadow_fb_access);
 
 /**
  * drm_gem_simple_kms_reset_shadow_plane - resets a shadow-buffered plane
diff --git a/include/drm/drm_gem_atomic_helper.h b/include/drm/drm_gem_atomic_helper.h
index 6e3319e9001a7..6970ccb787e23 100644
--- a/include/drm/drm_gem_atomic_helper.h
+++ b/include/drm/drm_gem_atomic_helper.h
@@ -103,8 +103,8 @@ void drm_gem_destroy_shadow_plane_state(struct drm_plane *plane,
 	.atomic_duplicate_state = drm_gem_duplicate_shadow_plane_state, \
 	.atomic_destroy_state = drm_gem_destroy_shadow_plane_state
 
-int drm_gem_prepare_shadow_fb(struct drm_plane *plane, struct drm_plane_state *plane_state);
-void drm_gem_cleanup_shadow_fb(struct drm_plane *plane, struct drm_plane_state *plane_state);
+int drm_gem_begin_shadow_fb_access(struct drm_plane *plane, struct drm_plane_state *plane_state);
+void drm_gem_end_shadow_fb_access(struct drm_plane *plane, struct drm_plane_state *plane_state);
 
 /**
  * DRM_GEM_SHADOW_PLANE_HELPER_FUNCS -
@@ -115,13 +115,13 @@ void drm_gem_cleanup_shadow_fb(struct drm_plane *plane, struct drm_plane_state *
  * functions.
  */
 #define DRM_GEM_SHADOW_PLANE_HELPER_FUNCS \
-	.prepare_fb = drm_gem_prepare_shadow_fb, \
-	.cleanup_fb = drm_gem_cleanup_shadow_fb
+	.begin_fb_access = drm_gem_begin_shadow_fb_access, \
+	.end_fb_access = drm_gem_end_shadow_fb_access
 
-int drm_gem_simple_kms_prepare_shadow_fb(struct drm_simple_display_pipe *pipe,
-					 struct drm_plane_state *plane_state);
-void drm_gem_simple_kms_cleanup_shadow_fb(struct drm_simple_display_pipe *pipe,
-					  struct drm_plane_state *plane_state);
+int drm_gem_simple_kms_begin_shadow_fb_access(struct drm_simple_display_pipe *pipe,
+					      struct drm_plane_state *plane_state);
+void drm_gem_simple_kms_end_shadow_fb_access(struct drm_simple_display_pipe *pipe,
+					     struct drm_plane_state *plane_state);
 void drm_gem_simple_kms_reset_shadow_plane(struct drm_simple_display_pipe *pipe);
 struct drm_plane_state *
 drm_gem_simple_kms_duplicate_shadow_plane_state(struct drm_simple_display_pipe *pipe);
@@ -137,8 +137,8 @@ void drm_gem_simple_kms_destroy_shadow_plane_state(struct drm_simple_display_pip
  * functions.
  */
 #define DRM_GEM_SIMPLE_DISPLAY_PIPE_SHADOW_PLANE_FUNCS \
-	.prepare_fb = drm_gem_simple_kms_prepare_shadow_fb, \
-	.cleanup_fb = drm_gem_simple_kms_cleanup_shadow_fb, \
+	.begin_fb_access = drm_gem_simple_kms_begin_shadow_fb_access, \
+	.end_fb_access = drm_gem_simple_kms_end_shadow_fb_access, \
 	.reset_plane = drm_gem_simple_kms_reset_shadow_plane, \
 	.duplicate_plane_state = drm_gem_simple_kms_duplicate_shadow_plane_state, \
 	.destroy_plane_state = drm_gem_simple_kms_destroy_shadow_plane_state
-- 
2.38.0

Re: [PATCH v2 2/2] drm/gem: Implement shadow-plane {begin,end}_fb_access with vmap

[Javier Martinez Canillas]

On 10/25/22 12:17, Thomas Zimmermann wrote:
> Move the vmap code for shadow-plane helpers from prepare_fb to
> begin_fb_access helpers. Vunmap is now performed at the end of
> the current pageflip, instead of the end of the following pageflip.
> 
> Reduces the duration of the mapping from while the framebuffer is
> being displayed to just the atomic commit. This is safe as outside
> of the pageflip, nothing should access the mapped buffer memory.
> Unmapping the framebuffer BO memory early allows reduces address-space

"allows to reduce" or "allows a reduced" ?

> consumption and possibly allows for evicting the memory pages.
> 
> The change is effectively a rename of prepare_fb and cleanup_fb
> implementations, plus updates to the shadow-plane init macro. As
> there's no longer a prepare_fb helper for shadow planes, atomic
> helpers will call drm_gem_plane_helper_prepare_fb() automatically.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH v2 0/2] drm: Add new plane helpers to begin/end FB access

[Thomas Zimmermann]

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

ping. Any comments?

Am 25.10.22 um 12:17 schrieb Thomas Zimmermann:
> This patchset adds the callbacks begin_fb_access and end_fb_access
> to struct drm_plane_helper_funcs. They provide hooks to acquire and
> release resources that are only held during the commit. It adds
> related simple-KMS helpers and converts shadow-plane helpers.
> 
> As resource allocation often can fail, we have to do this before an
> atomic commit starts the update in order to handle the error. But
> prepare_fb is only reverted after the next pageflip. We want to
> reduce the time resources are held for a commit, which requires the
> new hooks.
> 
> With this in place, move shadow-plane helpers' automatic vmap/vunmap
> behind the new callbacks. The shadow-plane mapping is only required
> during the atomic commit.
> 
> Tested with combinations of radeon, udl and simpledrm under X11, Weston
> and Wayland-Gnome.
> 
> Thomas Zimmermann (2):
>    drm/atomic-helper: Add {begin,end}_fb_access to plane helpers
>    drm/gem: Implement shadow-plane {begin,end}_fb_access with vmap
> 
>   drivers/gpu/drm/drm_atomic_helper.c      | 34 ++++++++++--
>   drivers/gpu/drm/drm_gem_atomic_helper.c  | 66 +++++++++++-------------
>   drivers/gpu/drm/drm_simple_kms_helper.c  | 26 ++++++++++
>   include/drm/drm_gem_atomic_helper.h      | 20 +++----
>   include/drm/drm_modeset_helper_vtables.h | 41 ++++++++++++++-
>   include/drm/drm_simple_kms_helper.h      | 20 +++++++
>   6 files changed, 157 insertions(+), 50 deletions(-)
> 
> 
> base-commit: 746559738f1335241ea686566cb654847c20d7a4

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