All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2 0/2] drm/vboxvideo: Use struct drm_shadow_plane_state for cursor plane
@ 2021-02-08 13:50 Thomas Zimmermann
  2021-02-08 13:50 ` [PATCH v2 1/2] drm/gem: Export helpers for shadow-buffered planes Thomas Zimmermann
                   ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Thomas Zimmermann @ 2021-02-08 13:50 UTC (permalink / raw)
  To: hdegoede, daniel, airlied, mripard, maarten.lankhorst
  Cc: Thomas Zimmermann, dri-devel

(was: drm/vboxvideo: Vmap/vunmap cursor BO in prepare_fb and cleanup_fb)

Functions in the atomic commit tail are not allowed to acquire the
dmabuf's reservation lock. So we cannot legally call the GEM object's
vmap functionality in atomic_update.

But, much better, we can use drm_shadow_plane_state to do all the mapping
for us. Patch 1 exports the helpers for shadow-buffered planes from the
DRM KMS helper library and adds documentation on how to use them. Patch 2
replaces the vmap code in vbox' cursor update code with a the helpers for
shadow-buffered planes.

Thomas Zimmermann (2):
  drm/gem: Export helpers for shadow-buffered planes
  drm/vboxvideo: Implement cursor plane with struct
    drm_shadow_plane_state

 drivers/gpu/drm/drm_gem_atomic_helper.c | 148 +++++++++++++++++++++++-
 drivers/gpu/drm/vboxvideo/vbox_mode.c   |  28 ++---
 include/drm/drm_gem_atomic_helper.h     |  32 +++++
 3 files changed, 181 insertions(+), 27 deletions(-)

--
2.30.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH v2 1/2] drm/gem: Export helpers for shadow-buffered planes
  2021-02-08 13:50 [PATCH v2 0/2] drm/vboxvideo: Use struct drm_shadow_plane_state for cursor plane Thomas Zimmermann
@ 2021-02-08 13:50 ` Thomas Zimmermann
  2021-02-09  9:44   ` Daniel Vetter
  2021-02-08 13:50 ` [PATCH v2 2/2] drm/vboxvideo: Implement cursor plane with struct drm_shadow_plane_state Thomas Zimmermann
  2021-02-08 17:43 ` [PATCH v2 0/2] drm/vboxvideo: Use struct drm_shadow_plane_state for cursor plane Hans de Goede
  2 siblings, 1 reply; 8+ messages in thread
From: Thomas Zimmermann @ 2021-02-08 13:50 UTC (permalink / raw)
  To: hdegoede, daniel, airlied, mripard, maarten.lankhorst
  Cc: Thomas Zimmermann, dri-devel

Export the helpers for shadow-buffered planes. These will be used by
several drivers.

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

diff --git a/drivers/gpu/drm/drm_gem_atomic_helper.c b/drivers/gpu/drm/drm_gem_atomic_helper.c
index e27762cef360..79b4d3f0495a 100644
--- a/drivers/gpu/drm/drm_gem_atomic_helper.c
+++ b/drivers/gpu/drm/drm_gem_atomic_helper.c
@@ -14,13 +14,101 @@
  * functions for drivers that use GEM objects. Currently, it provides
  * plane state and framebuffer BO mappings for planes with shadow
  * buffers.
+ *
+ * A driver using a shadow buffer copies the content of the shadow buffers
+ * into the HW's framebuffer memory during an atomic update. This requires
+ * a mapping of the shadow buffer into kernel address space. The mappings
+ * cannot be established by commit-tail functions, such as atomic_update,
+ * as this would violate locking rules vmap.
+ *
+ * The helpers for shadow-buffered planes establish and release mappings,
+ * and provide struct drm_shadow_plane_state, which stores the plane's mapping
+ * for commit-tail functons.
+ *
+ * Shadow-buffered planes can easily be enabled by using the provided macros
+ * DRM_GEM_PLANE_SHADOW_FUNCS and DRM_GEM_SHADOE_PLANE_HELPER_FUNCS.
+ * These macros set up the plane and plane-helper callbacks to point to the
+ * shadow-buffer helpers.
+ *
+ * .. code-block:: c
+ *
+ *	#include <drm/drm/gem_atomic_helper.h>
+ *
+ *	struct drm_plane_funcs driver_plane_funcs = {
+ *		...,
+ *		DRM_GEM_SHADOW_PLANE_FUNCS,
+ *	};
+ *
+ *	struct drm_plane_helper_funcs driver_plane_helper_funcs = {
+ *		...,
+ *		DRM_GEM_SHADOW_PLANE_HELPER_FUNCS,
+ *	};
+ *
+ * In the driver's atomic-update function, shadow-buffer mappings are available
+ * from the plane state. Use to_drm_shadow_plane_state() to upcast from
+ * struct drm_plane_state.
+ *
+ * .. code-block:: c
+ *
+ *	void driver_plane_atomic_update(struct drm_plane *plane,
+ *					struct drm_plane_state *old_plane_state)
+ *	{
+ *		struct drm_plane_state *plane_state = plane->state;
+ *		struct drm_shadow_plane_state *shadow_plane_state =
+ *			to_drm_shadow_plane_state(plane_state);
+ *
+ *		// access shadow buffer via shadow_plane_state->map
+ *	}
+ *
+ * A mapping address for each of the framebuffer's buffer object is stored in
+ * struct drm_shadow_plane_state.map. The mappings are valid while the state
+ * is being used.
+ *
+ * Drivers that use struct drm_simple_display_pipe can use
+ * DRM_GEM_SIMPLE_DISPLAY_PIPE_SHADOW_PLANE_FUNCS to initialize the rsp
+ * callbacks. Access to shadow-buffer mappings is similar to regular
+ * atomic_update.
+ *
+ * .. code-block:: c
+ *
+ *	struct drm_simple_display_pipe_funcs driver_pipe_funcs = {
+ *		...,
+ *		DRM_GEM_SIMPLE_DISPLAY_PIPE_SHADOW_PLANE_FUNCS,
+ *	};
+ *
+ *	void driver_pipe_enable(struct drm_simple_display_pipe *pipe,
+ *				struct drm_crtc_state *crtc_state,
+ *				struct drm_plane_state *plane_state)
+ *	{
+ *		struct drm_shadow_plane_state *shadow_plane_state =
+ *			to_drm_shadow_plane_state(plane_state);
+ *
+ *		// access shadow buffer via shadow_plane_state->map
+ *	}
  */
 
 /*
  * Shadow-buffered Planes
  */
 
-static struct drm_plane_state *
+/**
+ * drm_gem_duplicate_shadow_plane_state - duplicates shadow-buffered plane state
+ * @plane: the plane
+ *
+ * This function implements struct drm_plane_funcs.atomic_duplicate_state for
+ * shadow-buffered planes. It assumes the existing state to be of type
+ * struct drm_shadow_plane_state and it allocates the new state to be of this
+ * type.
+ *
+ * The function does not duplicate existing mappings of the shadow buffers.
+ * Mappings are maintained during the atomic commit by the plane's prepare_fb
+ * and cleanup_fb helpers. See drm_gem_prepare_shadow_fb() and drm_gem_cleanup_shadow_fb()
+ * for corresponding helpers.
+ *
+ * Returns:
+ * A pointer to a new plane state on success, or NULL otherwise.
+ */
+struct drm_plane_state *
 drm_gem_duplicate_shadow_plane_state(struct drm_plane *plane)
 {
 	struct drm_plane_state *plane_state = plane->state;
@@ -36,9 +124,19 @@ drm_gem_duplicate_shadow_plane_state(struct drm_plane *plane)
 
 	return &new_shadow_plane_state->base;
 }
+EXPORT_SYMBOL(drm_gem_duplicate_shadow_plane_state);
 
-static void drm_gem_destroy_shadow_plane_state(struct drm_plane *plane,
-					       struct drm_plane_state *plane_state)
+/**
+ * drm_gem_destroy_shadow_plane_state - deletes shadow-buffered plane state
+ * @plane: the plane
+ * @plane_state: the plane state of type struct drm_shadow_plane_state
+ *
+ * This function implements struct drm_plane_funcs.atomic_destroy_state
+ * for shadow-buffered planes. It expects that mappings of shadow buffers
+ * have been released already.
+ */
+void drm_gem_destroy_shadow_plane_state(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);
@@ -46,8 +144,18 @@ static void drm_gem_destroy_shadow_plane_state(struct drm_plane *plane,
 	__drm_atomic_helper_plane_destroy_state(&shadow_plane_state->base);
 	kfree(shadow_plane_state);
 }
+EXPORT_SYMBOL(drm_gem_destroy_shadow_plane_state);
 
-static void drm_gem_reset_shadow_plane(struct drm_plane *plane)
+/**
+ * drm_gem_reset_shadow_plane - resets a shadow-buffered plane
+ * @plane: the plane
+ *
+ * This function implements struct drm_plane_funcs.reset_plane for
+ * shadow-buffered planes. It assumes the current plane state to be
+ * of type struct drm_shadow_plane and it allocates the new state of
+ * this type.
+ */
+void drm_gem_reset_shadow_plane(struct drm_plane *plane)
 {
 	struct drm_shadow_plane_state *shadow_plane_state;
 
@@ -61,8 +169,24 @@ static void drm_gem_reset_shadow_plane(struct drm_plane *plane)
 		return;
 	__drm_atomic_helper_plane_reset(plane, &shadow_plane_state->base);
 }
+EXPORT_SYMBOL(drm_gem_reset_shadow_plane);
 
-static int drm_gem_prepare_shadow_fb(struct drm_plane *plane, struct drm_plane_state *plane_state)
+/**
+ * drm_gem_prepare_shadow_fb - prepares shadow framebuffers
+ * @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
+ * 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.
+ *
+ * See drm_gem_cleanup_shadow_fb() 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)
 {
 	struct drm_shadow_plane_state *shadow_plane_state = to_drm_shadow_plane_state(plane_state);
 	struct drm_framebuffer *fb = plane_state->fb;
@@ -100,8 +224,19 @@ static int drm_gem_prepare_shadow_fb(struct drm_plane *plane, struct drm_plane_s
 	}
 	return ret;
 }
+EXPORT_SYMBOL(drm_gem_prepare_shadow_fb);
 
-static void drm_gem_cleanup_shadow_fb(struct drm_plane *plane, struct drm_plane_state *plane_state)
+/**
+ * drm_gem_cleanup_shadow_fb - releases shadow framebuffers
+ * @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.
+ *
+ * See drm_gem_prepare_shadow_fb() for more inforamtion.
+ */
+void drm_gem_cleanup_shadow_fb(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;
@@ -119,6 +254,7 @@ static void drm_gem_cleanup_shadow_fb(struct drm_plane *plane, struct drm_plane_
 		drm_gem_vunmap(obj, &shadow_plane_state->map[i]);
 	}
 }
+EXPORT_SYMBOL(drm_gem_cleanup_shadow_fb);
 
 /**
  * drm_gem_simple_kms_prepare_shadow_fb - prepares shadow framebuffers
diff --git a/include/drm/drm_gem_atomic_helper.h b/include/drm/drm_gem_atomic_helper.h
index 08b96ccea325..7abf40bdab3d 100644
--- a/include/drm/drm_gem_atomic_helper.h
+++ b/include/drm/drm_gem_atomic_helper.h
@@ -45,6 +45,38 @@ to_drm_shadow_plane_state(struct drm_plane_state *state)
 	return container_of(state, struct drm_shadow_plane_state, base);
 }
 
+void drm_gem_reset_shadow_plane(struct drm_plane *plane);
+struct drm_plane_state *drm_gem_duplicate_shadow_plane_state(struct drm_plane *plane);
+void drm_gem_destroy_shadow_plane_state(struct drm_plane *plane,
+					struct drm_plane_state *plane_state);
+
+/**
+ * DRM_GEM_SHADOW_PLANE_FUNCS -
+ *	Initializes struct drm_plane_funcs for shadow-buffered planes
+ *
+ * Drivers may use GEM BOs as shadow buffers over the framebuffer memory. This
+ * macro initializes struct drm_plane_funcs to use the rsp helper functions.
+ */
+#define DRM_GEM_SHADOW_PLANE_FUNCS \
+	.reset = drm_gem_reset_shadow_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);
+
+/**
+ * DRM_GEM_SHADOW_PLANE_HELPER_FUNCS -
+ *	Initializes struct drm_plane_helper_funcs for shadow-buffered planes
+ *
+ * Drivers may use GEM BOs as shadow buffers over the framebuffer memory. This
+ * macro initializes struct drm_plane_helper_funcs to use the rsp helper
+ * functions.
+ */
+#define DRM_GEM_SHADOW_PLANE_HELPER_FUNCS \
+	.prepare_fb = drm_gem_prepare_shadow_fb, \
+	.cleanup_fb = drm_gem_cleanup_shadow_fb
+
 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,
-- 
2.30.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH v2 2/2] drm/vboxvideo: Implement cursor plane with struct drm_shadow_plane_state
  2021-02-08 13:50 [PATCH v2 0/2] drm/vboxvideo: Use struct drm_shadow_plane_state for cursor plane Thomas Zimmermann
  2021-02-08 13:50 ` [PATCH v2 1/2] drm/gem: Export helpers for shadow-buffered planes Thomas Zimmermann
@ 2021-02-08 13:50 ` Thomas Zimmermann
  2021-02-08 17:43 ` [PATCH v2 0/2] drm/vboxvideo: Use struct drm_shadow_plane_state for cursor plane Hans de Goede
  2 siblings, 0 replies; 8+ messages in thread
From: Thomas Zimmermann @ 2021-02-08 13:50 UTC (permalink / raw)
  To: hdegoede, daniel, airlied, mripard, maarten.lankhorst
  Cc: Thomas Zimmermann, dri-devel

Functions in the atomic commit tail are not allowed to acquire the
dmabuf's reservation lock. So we cannot legally call the GEM object's
vmap functionality in atomic_update.

Instead use struct drm_shadow_plane_state and friends. It vmaps the
framebuffer BOs in prepare_fb and vunmaps them in cleanup_fb. The
cursor plane state stores the mapping's address. The pinning of the
BO is implicitly done by vmap.

As an extra benefit, there's no source of runtime errors left in
atomic_update.

v2:
	* rebase patch onto struct drm_shadow_plane_state

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/vboxvideo/vbox_mode.c | 28 +++++++--------------------
 1 file changed, 7 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/vboxvideo/vbox_mode.c b/drivers/gpu/drm/vboxvideo/vbox_mode.c
index dbc0dd53c69e..6e4ad966be71 100644
--- a/drivers/gpu/drm/vboxvideo/vbox_mode.c
+++ b/drivers/gpu/drm/vboxvideo/vbox_mode.c
@@ -17,6 +17,7 @@
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_fb_helper.h>
 #include <drm/drm_fourcc.h>
+#include <drm/drm_gem_atomic_helper.h>
 #include <drm/drm_gem_framebuffer_helper.h>
 #include <drm/drm_plane_helper.h>
 #include <drm/drm_probe_helper.h>
@@ -381,14 +382,14 @@ static void vbox_cursor_atomic_update(struct drm_plane *plane,
 		container_of(plane->dev, struct vbox_private, ddev);
 	struct vbox_crtc *vbox_crtc = to_vbox_crtc(plane->state->crtc);
 	struct drm_framebuffer *fb = plane->state->fb;
-	struct drm_gem_vram_object *gbo = drm_gem_vram_of_gem(fb->obj[0]);
 	u32 width = plane->state->crtc_w;
 	u32 height = plane->state->crtc_h;
+	struct drm_shadow_plane_state *shadow_plane_state =
+		to_drm_shadow_plane_state(plane->state);
+	struct dma_buf_map map = shadow_plane_state->map[0];
+	u8 *src = map.vaddr; /* TODO: Use mapping abstraction properly */
 	size_t data_size, mask_size;
 	u32 flags;
-	struct dma_buf_map map;
-	int ret;
-	u8 *src;
 
 	/*
 	 * VirtualBox uses the host windowing system to draw the cursor so
@@ -401,17 +402,6 @@ static void vbox_cursor_atomic_update(struct drm_plane *plane,
 
 	vbox_crtc->cursor_enabled = true;
 
-	ret = drm_gem_vram_vmap(gbo, &map);
-	if (ret) {
-		/*
-		 * BUG: we should have pinned the BO in prepare_fb().
-		 */
-		mutex_unlock(&vbox->hw_mutex);
-		DRM_WARN("Could not map cursor bo, skipping update\n");
-		return;
-	}
-	src = map.vaddr; /* TODO: Use mapping abstraction properly */
-
 	/*
 	 * The mask must be calculated based on the alpha
 	 * channel, one bit per ARGB word, and must be 32-bit
@@ -421,7 +411,6 @@ static void vbox_cursor_atomic_update(struct drm_plane *plane,
 	data_size = width * height * 4 + mask_size;
 
 	copy_cursor_image(src, vbox->cursor_data, width, height, mask_size);
-	drm_gem_vram_vunmap(gbo, &map);
 
 	flags = VBOX_MOUSE_POINTER_VISIBLE | VBOX_MOUSE_POINTER_SHAPE |
 		VBOX_MOUSE_POINTER_ALPHA;
@@ -466,17 +455,14 @@ static const struct drm_plane_helper_funcs vbox_cursor_helper_funcs = {
 	.atomic_check	= vbox_cursor_atomic_check,
 	.atomic_update	= vbox_cursor_atomic_update,
 	.atomic_disable	= vbox_cursor_atomic_disable,
-	.prepare_fb	= drm_gem_vram_plane_helper_prepare_fb,
-	.cleanup_fb	= drm_gem_vram_plane_helper_cleanup_fb,
+	DRM_GEM_SHADOW_PLANE_HELPER_FUNCS,
 };
 
 static const struct drm_plane_funcs vbox_cursor_plane_funcs = {
 	.update_plane	= drm_atomic_helper_update_plane,
 	.disable_plane	= drm_atomic_helper_disable_plane,
 	.destroy	= drm_primary_helper_destroy,
-	.reset		= drm_atomic_helper_plane_reset,
-	.atomic_duplicate_state = drm_atomic_helper_plane_duplicate_state,
-	.atomic_destroy_state = drm_atomic_helper_plane_destroy_state,
+	DRM_GEM_SHADOW_PLANE_FUNCS,
 };
 
 static const u32 vbox_primary_plane_formats[] = {
-- 
2.30.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 0/2] drm/vboxvideo: Use struct drm_shadow_plane_state for cursor plane
  2021-02-08 13:50 [PATCH v2 0/2] drm/vboxvideo: Use struct drm_shadow_plane_state for cursor plane Thomas Zimmermann
  2021-02-08 13:50 ` [PATCH v2 1/2] drm/gem: Export helpers for shadow-buffered planes Thomas Zimmermann
  2021-02-08 13:50 ` [PATCH v2 2/2] drm/vboxvideo: Implement cursor plane with struct drm_shadow_plane_state Thomas Zimmermann
@ 2021-02-08 17:43 ` Hans de Goede
  2021-02-09  8:13   ` Thomas Zimmermann
  2 siblings, 1 reply; 8+ messages in thread
From: Hans de Goede @ 2021-02-08 17:43 UTC (permalink / raw)
  To: Thomas Zimmermann, daniel, airlied, mripard, maarten.lankhorst; +Cc: dri-devel

Hi,

On 2/8/21 2:50 PM, Thomas Zimmermann wrote:
> (was: drm/vboxvideo: Vmap/vunmap cursor BO in prepare_fb and cleanup_fb)
> 
> Functions in the atomic commit tail are not allowed to acquire the
> dmabuf's reservation lock. So we cannot legally call the GEM object's
> vmap functionality in atomic_update.
> 
> But, much better, we can use drm_shadow_plane_state to do all the mapping
> for us. Patch 1 exports the helpers for shadow-buffered planes from the
> DRM KMS helper library and adds documentation on how to use them. Patch 2
> replaces the vmap code in vbox' cursor update code with a the helpers for
> shadow-buffered planes.
> 
> Thomas Zimmermann (2):
>   drm/gem: Export helpers for shadow-buffered planes
>   drm/vboxvideo: Implement cursor plane with struct
>     drm_shadow_plane_state

I've given this a test spin in a virtualbox vm using VboxSVGA graphics
and I've not found any problems:

Tested-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans



> 
>  drivers/gpu/drm/drm_gem_atomic_helper.c | 148 +++++++++++++++++++++++-
>  drivers/gpu/drm/vboxvideo/vbox_mode.c   |  28 ++---
>  include/drm/drm_gem_atomic_helper.h     |  32 +++++
>  3 files changed, 181 insertions(+), 27 deletions(-)
> 
> --
> 2.30.0
> 

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 0/2] drm/vboxvideo: Use struct drm_shadow_plane_state for cursor plane
  2021-02-08 17:43 ` [PATCH v2 0/2] drm/vboxvideo: Use struct drm_shadow_plane_state for cursor plane Hans de Goede
@ 2021-02-09  8:13   ` Thomas Zimmermann
  2021-02-09 11:04     ` Hans de Goede
  0 siblings, 1 reply; 8+ messages in thread
From: Thomas Zimmermann @ 2021-02-09  8:13 UTC (permalink / raw)
  To: Hans de Goede, daniel, airlied, mripard, maarten.lankhorst; +Cc: dri-devel


[-- Attachment #1.1.1: Type: text/plain, Size: 1841 bytes --]

Hi

Am 08.02.21 um 18:43 schrieb Hans de Goede:
> Hi,
> 
> On 2/8/21 2:50 PM, Thomas Zimmermann wrote:
>> (was: drm/vboxvideo: Vmap/vunmap cursor BO in prepare_fb and cleanup_fb)
>>
>> Functions in the atomic commit tail are not allowed to acquire the
>> dmabuf's reservation lock. So we cannot legally call the GEM object's
>> vmap functionality in atomic_update.
>>
>> But, much better, we can use drm_shadow_plane_state to do all the mapping
>> for us. Patch 1 exports the helpers for shadow-buffered planes from the
>> DRM KMS helper library and adds documentation on how to use them. Patch 2
>> replaces the vmap code in vbox' cursor update code with a the helpers for
>> shadow-buffered planes.
>>
>> Thomas Zimmermann (2):
>>    drm/gem: Export helpers for shadow-buffered planes
>>    drm/vboxvideo: Implement cursor plane with struct
>>      drm_shadow_plane_state
> 
> I've given this a test spin in a virtualbox vm using VboxSVGA graphics
> and I've not found any problems:
> 
> Tested-by: Hans de Goede <hdegoede@redhat.com>

Thanks a lot. Can I add your Acked-by as well?

Best regards
Thomas

> 
> Regards,
> 
> Hans
> 
> 
> 
>>
>>   drivers/gpu/drm/drm_gem_atomic_helper.c | 148 +++++++++++++++++++++++-
>>   drivers/gpu/drm/vboxvideo/vbox_mode.c   |  28 ++---
>>   include/drm/drm_gem_atomic_helper.h     |  32 +++++
>>   3 files changed, 181 insertions(+), 27 deletions(-)
>>
>> --
>> 2.30.0
>>
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 

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


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

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 1/2] drm/gem: Export helpers for shadow-buffered planes
  2021-02-08 13:50 ` [PATCH v2 1/2] drm/gem: Export helpers for shadow-buffered planes Thomas Zimmermann
@ 2021-02-09  9:44   ` Daniel Vetter
  2021-02-09 12:04     ` Thomas Zimmermann
  0 siblings, 1 reply; 8+ messages in thread
From: Daniel Vetter @ 2021-02-09  9:44 UTC (permalink / raw)
  To: Thomas Zimmermann; +Cc: airlied, dri-devel, hdegoede

On Mon, Feb 08, 2021 at 02:50:43PM +0100, Thomas Zimmermann wrote:
> Export the helpers for shadow-buffered planes. These will be used by
> several drivers.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>  drivers/gpu/drm/drm_gem_atomic_helper.c | 148 +++++++++++++++++++++++-
>  include/drm/drm_gem_atomic_helper.h     |  32 +++++
>  2 files changed, 174 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_gem_atomic_helper.c b/drivers/gpu/drm/drm_gem_atomic_helper.c
> index e27762cef360..79b4d3f0495a 100644
> --- a/drivers/gpu/drm/drm_gem_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_gem_atomic_helper.c
> @@ -14,13 +14,101 @@
>   * functions for drivers that use GEM objects. Currently, it provides
>   * plane state and framebuffer BO mappings for planes with shadow
>   * buffers.
> + *
> + * A driver using a shadow buffer copies the content of the shadow buffers
> + * into the HW's framebuffer memory during an atomic update. This requires
> + * a mapping of the shadow buffer into kernel address space. The mappings
> + * cannot be established by commit-tail functions, such as atomic_update,
> + * as this would violate locking rules vmap.

"... locking rules around dma_buf_vmap()"?

> + *
> + * The helpers for shadow-buffered planes establish and release mappings,
> + * and provide struct drm_shadow_plane_state, which stores the plane's mapping
> + * for commit-tail functons.
> + *
> + * Shadow-buffered planes can easily be enabled by using the provided macros
> + * DRM_GEM_PLANE_SHADOW_FUNCS and DRM_GEM_SHADOE_PLANE_HELPER_FUNCS.

I think for hyperlinks/highlights we need %CONSTANT? Maybe check what works.

> + * These macros set up the plane and plane-helper callbacks to point to the
> + * shadow-buffer helpers.
> + *
> + * .. code-block:: c
> + *
> + *	#include <drm/drm/gem_atomic_helper.h>
> + *
> + *	struct drm_plane_funcs driver_plane_funcs = {
> + *		...,
> + *		DRM_GEM_SHADOW_PLANE_FUNCS,
> + *	};
> + *
> + *	struct drm_plane_helper_funcs driver_plane_helper_funcs = {
> + *		...,
> + *		DRM_GEM_SHADOW_PLANE_HELPER_FUNCS,
> + *	};
> + *
> + * In the driver's atomic-update function, shadow-buffer mappings are available
> + * from the plane state. Use to_drm_shadow_plane_state() to upcast from
> + * struct drm_plane_state.
> + *
> + * .. code-block:: c
> + *
> + *	void driver_plane_atomic_update(struct drm_plane *plane,
> + *					struct drm_plane_state *old_plane_state)
> + *	{
> + *		struct drm_plane_state *plane_state = plane->state;
> + *		struct drm_shadow_plane_state *shadow_plane_state =
> + *			to_drm_shadow_plane_state(plane_state);
> + *
> + *		// access shadow buffer via shadow_plane_state->map
> + *	}
> + *
> + * A mapping address for each of the framebuffer's buffer object is stored in
> + * struct drm_shadow_plane_state.map. The mappings are valid while the state
> + * is being used.
> + *
> + * Drivers that use struct drm_simple_display_pipe can use
> + * DRM_GEM_SIMPLE_DISPLAY_PIPE_SHADOW_PLANE_FUNCS to initialize the rsp
> + * callbacks. Access to shadow-buffer mappings is similar to regular
> + * atomic_update.
> + *
> + * .. code-block:: c
> + *
> + *	struct drm_simple_display_pipe_funcs driver_pipe_funcs = {
> + *		...,
> + *		DRM_GEM_SIMPLE_DISPLAY_PIPE_SHADOW_PLANE_FUNCS,
> + *	};
> + *
> + *	void driver_pipe_enable(struct drm_simple_display_pipe *pipe,
> + *				struct drm_crtc_state *crtc_state,
> + *				struct drm_plane_state *plane_state)
> + *	{
> + *		struct drm_shadow_plane_state *shadow_plane_state =
> + *			to_drm_shadow_plane_state(plane_state);
> + *
> + *		// access shadow buffer via shadow_plane_state->map
> + *	}
>   */
>  
>  /*
>   * Shadow-buffered Planes
>   */
>  
> -static struct drm_plane_state *
> +/**
> + * drm_gem_duplicate_shadow_plane_state - duplicates shadow-buffered plane state
> + * @plane: the plane
> + *
> + * This function implements struct drm_plane_funcs.atomic_duplicate_state for

Does this hyperlink automatically? I didn't know it works since for
members I just always use &struct.member myself.

> + * shadow-buffered planes. It assumes the existing state to be of type
> + * struct drm_shadow_plane_state and it allocates the new state to be of this
> + * type.
> + *
> + * The function does not duplicate existing mappings of the shadow buffers.
> + * Mappings are maintained during the atomic commit by the plane's prepare_fb
> + * and cleanup_fb helpers. See drm_gem_prepare_shadow_fb() and drm_gem_cleanup_shadow_fb()
> + * for corresponding helpers.
> + *
> + * Returns:
> + * A pointer to a new plane state on success, or NULL otherwise.
> + */
> +struct drm_plane_state *
>  drm_gem_duplicate_shadow_plane_state(struct drm_plane *plane)
>  {
>  	struct drm_plane_state *plane_state = plane->state;
> @@ -36,9 +124,19 @@ drm_gem_duplicate_shadow_plane_state(struct drm_plane *plane)
>  
>  	return &new_shadow_plane_state->base;
>  }
> +EXPORT_SYMBOL(drm_gem_duplicate_shadow_plane_state);
>  
> -static void drm_gem_destroy_shadow_plane_state(struct drm_plane *plane,
> -					       struct drm_plane_state *plane_state)
> +/**
> + * drm_gem_destroy_shadow_plane_state - deletes shadow-buffered plane state
> + * @plane: the plane
> + * @plane_state: the plane state of type struct drm_shadow_plane_state
> + *
> + * This function implements struct drm_plane_funcs.atomic_destroy_state
> + * for shadow-buffered planes. It expects that mappings of shadow buffers
> + * have been released already.
> + */
> +void drm_gem_destroy_shadow_plane_state(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);
> @@ -46,8 +144,18 @@ static void drm_gem_destroy_shadow_plane_state(struct drm_plane *plane,
>  	__drm_atomic_helper_plane_destroy_state(&shadow_plane_state->base);
>  	kfree(shadow_plane_state);
>  }
> +EXPORT_SYMBOL(drm_gem_destroy_shadow_plane_state);
>  
> -static void drm_gem_reset_shadow_plane(struct drm_plane *plane)
> +/**
> + * drm_gem_reset_shadow_plane - resets a shadow-buffered plane
> + * @plane: the plane
> + *
> + * This function implements struct drm_plane_funcs.reset_plane for
> + * shadow-buffered planes. It assumes the current plane state to be
> + * of type struct drm_shadow_plane and it allocates the new state of
> + * this type.
> + */
> +void drm_gem_reset_shadow_plane(struct drm_plane *plane)
>  {
>  	struct drm_shadow_plane_state *shadow_plane_state;
>  
> @@ -61,8 +169,24 @@ static void drm_gem_reset_shadow_plane(struct drm_plane *plane)
>  		return;
>  	__drm_atomic_helper_plane_reset(plane, &shadow_plane_state->base);
>  }
> +EXPORT_SYMBOL(drm_gem_reset_shadow_plane);
>  
> -static int drm_gem_prepare_shadow_fb(struct drm_plane *plane, struct drm_plane_state *plane_state)
> +/**
> + * drm_gem_prepare_shadow_fb - prepares shadow framebuffers
> + * @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
> + * 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.
> + *
> + * See drm_gem_cleanup_shadow_fb() 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)
>  {
>  	struct drm_shadow_plane_state *shadow_plane_state = to_drm_shadow_plane_state(plane_state);
>  	struct drm_framebuffer *fb = plane_state->fb;
> @@ -100,8 +224,19 @@ static int drm_gem_prepare_shadow_fb(struct drm_plane *plane, struct drm_plane_s
>  	}
>  	return ret;
>  }
> +EXPORT_SYMBOL(drm_gem_prepare_shadow_fb);
>  
> -static void drm_gem_cleanup_shadow_fb(struct drm_plane *plane, struct drm_plane_state *plane_state)
> +/**
> + * drm_gem_cleanup_shadow_fb - releases shadow framebuffers
> + * @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.
> + *
> + * See drm_gem_prepare_shadow_fb() for more inforamtion.
> + */
> +void drm_gem_cleanup_shadow_fb(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;
> @@ -119,6 +254,7 @@ static void drm_gem_cleanup_shadow_fb(struct drm_plane *plane, struct drm_plane_
>  		drm_gem_vunmap(obj, &shadow_plane_state->map[i]);
>  	}
>  }
> +EXPORT_SYMBOL(drm_gem_cleanup_shadow_fb);
>  
>  /**
>   * drm_gem_simple_kms_prepare_shadow_fb - prepares shadow framebuffers
> diff --git a/include/drm/drm_gem_atomic_helper.h b/include/drm/drm_gem_atomic_helper.h
> index 08b96ccea325..7abf40bdab3d 100644
> --- a/include/drm/drm_gem_atomic_helper.h
> +++ b/include/drm/drm_gem_atomic_helper.h
> @@ -45,6 +45,38 @@ to_drm_shadow_plane_state(struct drm_plane_state *state)
>  	return container_of(state, struct drm_shadow_plane_state, base);
>  }
>  
> +void drm_gem_reset_shadow_plane(struct drm_plane *plane);
> +struct drm_plane_state *drm_gem_duplicate_shadow_plane_state(struct drm_plane *plane);
> +void drm_gem_destroy_shadow_plane_state(struct drm_plane *plane,
> +					struct drm_plane_state *plane_state);
> +
> +/**
> + * DRM_GEM_SHADOW_PLANE_FUNCS -
> + *	Initializes struct drm_plane_funcs for shadow-buffered planes
> + *
> + * Drivers may use GEM BOs as shadow buffers over the framebuffer memory. This
> + * macro initializes struct drm_plane_funcs to use the rsp helper functions.
> + */
> +#define DRM_GEM_SHADOW_PLANE_FUNCS \
> +	.reset = drm_gem_reset_shadow_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);
> +
> +/**
> + * DRM_GEM_SHADOW_PLANE_HELPER_FUNCS -
> + *	Initializes struct drm_plane_helper_funcs for shadow-buffered planes
> + *
> + * Drivers may use GEM BOs as shadow buffers over the framebuffer memory. This
> + * macro initializes struct drm_plane_helper_funcs to use the rsp helper
> + * functions.
> + */
> +#define DRM_GEM_SHADOW_PLANE_HELPER_FUNCS \
> +	.prepare_fb = drm_gem_prepare_shadow_fb, \
> +	.cleanup_fb = drm_gem_cleanup_shadow_fb
> +
>  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,


Very nice and thoroughly explained docs!

Thanks, Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> -- 
> 2.30.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 0/2] drm/vboxvideo: Use struct drm_shadow_plane_state for cursor plane
  2021-02-09  8:13   ` Thomas Zimmermann
@ 2021-02-09 11:04     ` Hans de Goede
  0 siblings, 0 replies; 8+ messages in thread
From: Hans de Goede @ 2021-02-09 11:04 UTC (permalink / raw)
  To: Thomas Zimmermann, daniel, airlied, mripard, maarten.lankhorst; +Cc: dri-devel

Hi,

On 2/9/21 9:13 AM, Thomas Zimmermann wrote:
> Hi
> 
> Am 08.02.21 um 18:43 schrieb Hans de Goede:
>> Hi,
>>
>> On 2/8/21 2:50 PM, Thomas Zimmermann wrote:
>>> (was: drm/vboxvideo: Vmap/vunmap cursor BO in prepare_fb and cleanup_fb)
>>>
>>> Functions in the atomic commit tail are not allowed to acquire the
>>> dmabuf's reservation lock. So we cannot legally call the GEM object's
>>> vmap functionality in atomic_update.
>>>
>>> But, much better, we can use drm_shadow_plane_state to do all the mapping
>>> for us. Patch 1 exports the helpers for shadow-buffered planes from the
>>> DRM KMS helper library and adds documentation on how to use them. Patch 2
>>> replaces the vmap code in vbox' cursor update code with a the helpers for
>>> shadow-buffered planes.
>>>
>>> Thomas Zimmermann (2):
>>>    drm/gem: Export helpers for shadow-buffered planes
>>>    drm/vboxvideo: Implement cursor plane with struct
>>>      drm_shadow_plane_state
>>
>> I've given this a test spin in a virtualbox vm using VboxSVGA graphics
>> and I've not found any problems:
>>
>> Tested-by: Hans de Goede <hdegoede@redhat.com>
> 
> Thanks a lot. Can I add your Acked-by as well?

Patch 1/2 is a bit outside of my area of expertise, but I see you already have
a Reviewed-by from Daniel there, so that one should be good to go.

I've just reviewed 2/2 and it looks good to me, so for 2/2 you may add my:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans



>>>
>>>   drivers/gpu/drm/drm_gem_atomic_helper.c | 148 +++++++++++++++++++++++-
>>>   drivers/gpu/drm/vboxvideo/vbox_mode.c   |  28 ++---
>>>   include/drm/drm_gem_atomic_helper.h     |  32 +++++
>>>   3 files changed, 181 insertions(+), 27 deletions(-)
>>>
>>> -- 
>>> 2.30.0
>>>
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>
> 

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2 1/2] drm/gem: Export helpers for shadow-buffered planes
  2021-02-09  9:44   ` Daniel Vetter
@ 2021-02-09 12:04     ` Thomas Zimmermann
  0 siblings, 0 replies; 8+ messages in thread
From: Thomas Zimmermann @ 2021-02-09 12:04 UTC (permalink / raw)
  To: Daniel Vetter; +Cc: airlied, hdegoede, dri-devel


[-- Attachment #1.1.1: Type: text/plain, Size: 12023 bytes --]

Hi

Am 09.02.21 um 10:44 schrieb Daniel Vetter:
> On Mon, Feb 08, 2021 at 02:50:43PM +0100, Thomas Zimmermann wrote:
>> Export the helpers for shadow-buffered planes. These will be used by
>> several drivers.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
>>   drivers/gpu/drm/drm_gem_atomic_helper.c | 148 +++++++++++++++++++++++-
>>   include/drm/drm_gem_atomic_helper.h     |  32 +++++
>>   2 files changed, 174 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_gem_atomic_helper.c b/drivers/gpu/drm/drm_gem_atomic_helper.c
>> index e27762cef360..79b4d3f0495a 100644
>> --- a/drivers/gpu/drm/drm_gem_atomic_helper.c
>> +++ b/drivers/gpu/drm/drm_gem_atomic_helper.c
>> @@ -14,13 +14,101 @@
>>    * functions for drivers that use GEM objects. Currently, it provides
>>    * plane state and framebuffer BO mappings for planes with shadow
>>    * buffers.
>> + *
>> + * A driver using a shadow buffer copies the content of the shadow buffers
>> + * into the HW's framebuffer memory during an atomic update. This requires
>> + * a mapping of the shadow buffer into kernel address space. The mappings
>> + * cannot be established by commit-tail functions, such as atomic_update,
>> + * as this would violate locking rules vmap.
> 
> "... locking rules around dma_buf_vmap()"?
> 
>> + *
>> + * The helpers for shadow-buffered planes establish and release mappings,
>> + * and provide struct drm_shadow_plane_state, which stores the plane's mapping
>> + * for commit-tail functons.
>> + *
>> + * Shadow-buffered planes can easily be enabled by using the provided macros
>> + * DRM_GEM_PLANE_SHADOW_FUNCS and DRM_GEM_SHADOE_PLANE_HELPER_FUNCS.
> 
> I think for hyperlinks/highlights we need %CONSTANT? Maybe check what works.
> 
>> + * These macros set up the plane and plane-helper callbacks to point to the
>> + * shadow-buffer helpers.
>> + *
>> + * .. code-block:: c
>> + *
>> + *	#include <drm/drm/gem_atomic_helper.h>
>> + *
>> + *	struct drm_plane_funcs driver_plane_funcs = {
>> + *		...,
>> + *		DRM_GEM_SHADOW_PLANE_FUNCS,
>> + *	};
>> + *
>> + *	struct drm_plane_helper_funcs driver_plane_helper_funcs = {
>> + *		...,
>> + *		DRM_GEM_SHADOW_PLANE_HELPER_FUNCS,
>> + *	};
>> + *
>> + * In the driver's atomic-update function, shadow-buffer mappings are available
>> + * from the plane state. Use to_drm_shadow_plane_state() to upcast from
>> + * struct drm_plane_state.
>> + *
>> + * .. code-block:: c
>> + *
>> + *	void driver_plane_atomic_update(struct drm_plane *plane,
>> + *					struct drm_plane_state *old_plane_state)
>> + *	{
>> + *		struct drm_plane_state *plane_state = plane->state;
>> + *		struct drm_shadow_plane_state *shadow_plane_state =
>> + *			to_drm_shadow_plane_state(plane_state);
>> + *
>> + *		// access shadow buffer via shadow_plane_state->map
>> + *	}
>> + *
>> + * A mapping address for each of the framebuffer's buffer object is stored in
>> + * struct drm_shadow_plane_state.map. The mappings are valid while the state
>> + * is being used.
>> + *
>> + * Drivers that use struct drm_simple_display_pipe can use
>> + * DRM_GEM_SIMPLE_DISPLAY_PIPE_SHADOW_PLANE_FUNCS to initialize the rsp
>> + * callbacks. Access to shadow-buffer mappings is similar to regular
>> + * atomic_update.
>> + *
>> + * .. code-block:: c
>> + *
>> + *	struct drm_simple_display_pipe_funcs driver_pipe_funcs = {
>> + *		...,
>> + *		DRM_GEM_SIMPLE_DISPLAY_PIPE_SHADOW_PLANE_FUNCS,
>> + *	};
>> + *
>> + *	void driver_pipe_enable(struct drm_simple_display_pipe *pipe,
>> + *				struct drm_crtc_state *crtc_state,
>> + *				struct drm_plane_state *plane_state)
>> + *	{
>> + *		struct drm_shadow_plane_state *shadow_plane_state =
>> + *			to_drm_shadow_plane_state(plane_state);
>> + *
>> + *		// access shadow buffer via shadow_plane_state->map
>> + *	}
>>    */
>>   
>>   /*
>>    * Shadow-buffered Planes
>>    */
>>   
>> -static struct drm_plane_state *
>> +/**
>> + * drm_gem_duplicate_shadow_plane_state - duplicates shadow-buffered plane state
>> + * @plane: the plane
>> + *
>> + * This function implements struct drm_plane_funcs.atomic_duplicate_state for
> 
> Does this hyperlink automatically? I didn't know it works since for
> members I just always use &struct.member myself.

Ah, ok. Fixed. This work with struct &name.field. The % only adds 
formatting to constants.

Best regards
Thomas

> 
>> + * shadow-buffered planes. It assumes the existing state to be of type
>> + * struct drm_shadow_plane_state and it allocates the new state to be of this
>> + * type.
>> + *
>> + * The function does not duplicate existing mappings of the shadow buffers.
>> + * Mappings are maintained during the atomic commit by the plane's prepare_fb
>> + * and cleanup_fb helpers. See drm_gem_prepare_shadow_fb() and drm_gem_cleanup_shadow_fb()
>> + * for corresponding helpers.
>> + *
>> + * Returns:
>> + * A pointer to a new plane state on success, or NULL otherwise.
>> + */
>> +struct drm_plane_state *
>>   drm_gem_duplicate_shadow_plane_state(struct drm_plane *plane)
>>   {
>>   	struct drm_plane_state *plane_state = plane->state;
>> @@ -36,9 +124,19 @@ drm_gem_duplicate_shadow_plane_state(struct drm_plane *plane)
>>   
>>   	return &new_shadow_plane_state->base;
>>   }
>> +EXPORT_SYMBOL(drm_gem_duplicate_shadow_plane_state);
>>   
>> -static void drm_gem_destroy_shadow_plane_state(struct drm_plane *plane,
>> -					       struct drm_plane_state *plane_state)
>> +/**
>> + * drm_gem_destroy_shadow_plane_state - deletes shadow-buffered plane state
>> + * @plane: the plane
>> + * @plane_state: the plane state of type struct drm_shadow_plane_state
>> + *
>> + * This function implements struct drm_plane_funcs.atomic_destroy_state
>> + * for shadow-buffered planes. It expects that mappings of shadow buffers
>> + * have been released already.
>> + */
>> +void drm_gem_destroy_shadow_plane_state(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);
>> @@ -46,8 +144,18 @@ static void drm_gem_destroy_shadow_plane_state(struct drm_plane *plane,
>>   	__drm_atomic_helper_plane_destroy_state(&shadow_plane_state->base);
>>   	kfree(shadow_plane_state);
>>   }
>> +EXPORT_SYMBOL(drm_gem_destroy_shadow_plane_state);
>>   
>> -static void drm_gem_reset_shadow_plane(struct drm_plane *plane)
>> +/**
>> + * drm_gem_reset_shadow_plane - resets a shadow-buffered plane
>> + * @plane: the plane
>> + *
>> + * This function implements struct drm_plane_funcs.reset_plane for
>> + * shadow-buffered planes. It assumes the current plane state to be
>> + * of type struct drm_shadow_plane and it allocates the new state of
>> + * this type.
>> + */
>> +void drm_gem_reset_shadow_plane(struct drm_plane *plane)
>>   {
>>   	struct drm_shadow_plane_state *shadow_plane_state;
>>   
>> @@ -61,8 +169,24 @@ static void drm_gem_reset_shadow_plane(struct drm_plane *plane)
>>   		return;
>>   	__drm_atomic_helper_plane_reset(plane, &shadow_plane_state->base);
>>   }
>> +EXPORT_SYMBOL(drm_gem_reset_shadow_plane);
>>   
>> -static int drm_gem_prepare_shadow_fb(struct drm_plane *plane, struct drm_plane_state *plane_state)
>> +/**
>> + * drm_gem_prepare_shadow_fb - prepares shadow framebuffers
>> + * @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
>> + * 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.
>> + *
>> + * See drm_gem_cleanup_shadow_fb() 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)
>>   {
>>   	struct drm_shadow_plane_state *shadow_plane_state = to_drm_shadow_plane_state(plane_state);
>>   	struct drm_framebuffer *fb = plane_state->fb;
>> @@ -100,8 +224,19 @@ static int drm_gem_prepare_shadow_fb(struct drm_plane *plane, struct drm_plane_s
>>   	}
>>   	return ret;
>>   }
>> +EXPORT_SYMBOL(drm_gem_prepare_shadow_fb);
>>   
>> -static void drm_gem_cleanup_shadow_fb(struct drm_plane *plane, struct drm_plane_state *plane_state)
>> +/**
>> + * drm_gem_cleanup_shadow_fb - releases shadow framebuffers
>> + * @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.
>> + *
>> + * See drm_gem_prepare_shadow_fb() for more inforamtion.
>> + */
>> +void drm_gem_cleanup_shadow_fb(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;
>> @@ -119,6 +254,7 @@ static void drm_gem_cleanup_shadow_fb(struct drm_plane *plane, struct drm_plane_
>>   		drm_gem_vunmap(obj, &shadow_plane_state->map[i]);
>>   	}
>>   }
>> +EXPORT_SYMBOL(drm_gem_cleanup_shadow_fb);
>>   
>>   /**
>>    * drm_gem_simple_kms_prepare_shadow_fb - prepares shadow framebuffers
>> diff --git a/include/drm/drm_gem_atomic_helper.h b/include/drm/drm_gem_atomic_helper.h
>> index 08b96ccea325..7abf40bdab3d 100644
>> --- a/include/drm/drm_gem_atomic_helper.h
>> +++ b/include/drm/drm_gem_atomic_helper.h
>> @@ -45,6 +45,38 @@ to_drm_shadow_plane_state(struct drm_plane_state *state)
>>   	return container_of(state, struct drm_shadow_plane_state, base);
>>   }
>>   
>> +void drm_gem_reset_shadow_plane(struct drm_plane *plane);
>> +struct drm_plane_state *drm_gem_duplicate_shadow_plane_state(struct drm_plane *plane);
>> +void drm_gem_destroy_shadow_plane_state(struct drm_plane *plane,
>> +					struct drm_plane_state *plane_state);
>> +
>> +/**
>> + * DRM_GEM_SHADOW_PLANE_FUNCS -
>> + *	Initializes struct drm_plane_funcs for shadow-buffered planes
>> + *
>> + * Drivers may use GEM BOs as shadow buffers over the framebuffer memory. This
>> + * macro initializes struct drm_plane_funcs to use the rsp helper functions.
>> + */
>> +#define DRM_GEM_SHADOW_PLANE_FUNCS \
>> +	.reset = drm_gem_reset_shadow_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);
>> +
>> +/**
>> + * DRM_GEM_SHADOW_PLANE_HELPER_FUNCS -
>> + *	Initializes struct drm_plane_helper_funcs for shadow-buffered planes
>> + *
>> + * Drivers may use GEM BOs as shadow buffers over the framebuffer memory. This
>> + * macro initializes struct drm_plane_helper_funcs to use the rsp helper
>> + * functions.
>> + */
>> +#define DRM_GEM_SHADOW_PLANE_HELPER_FUNCS \
>> +	.prepare_fb = drm_gem_prepare_shadow_fb, \
>> +	.cleanup_fb = drm_gem_cleanup_shadow_fb
>> +
>>   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,
> 
> 
> Very nice and thoroughly explained docs!
> 
> Thanks, Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
>> -- 
>> 2.30.0
>>
> 

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


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

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2021-02-09 12:04 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-08 13:50 [PATCH v2 0/2] drm/vboxvideo: Use struct drm_shadow_plane_state for cursor plane Thomas Zimmermann
2021-02-08 13:50 ` [PATCH v2 1/2] drm/gem: Export helpers for shadow-buffered planes Thomas Zimmermann
2021-02-09  9:44   ` Daniel Vetter
2021-02-09 12:04     ` Thomas Zimmermann
2021-02-08 13:50 ` [PATCH v2 2/2] drm/vboxvideo: Implement cursor plane with struct drm_shadow_plane_state Thomas Zimmermann
2021-02-08 17:43 ` [PATCH v2 0/2] drm/vboxvideo: Use struct drm_shadow_plane_state for cursor plane Hans de Goede
2021-02-09  8:13   ` Thomas Zimmermann
2021-02-09 11:04     ` Hans de Goede

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.