[PATCH 0/4] vkms: Switch to shadow-buffered plane state

[PATCH 0/4] vkms: Switch to shadow-buffered plane state

[Thomas Zimmermann]

Vkms copies each plane's framebuffer into the output buffer; essentially
using a shadow buffer. DRM provides struct drm_shadow_plane_state, which
handles the details of mapping/unmapping shadow buffers into memory for
active planes.

Convert vkms to the helpers. Makes vkms use shared code and gives more
test exposure to shadow-plane helpers.

Thomas Zimmermann (4):
  drm/gem: Export implementation of shadow-plane helpers
  drm/vkms: Inherit plane state from struct drm_shadow_plane_state
  drm/vkms: Let shadow-plane helpers prepare the plane's FB
  drm/vkms: Use dma-buf mapping from shadow-plane state for composing

 drivers/gpu/drm/drm_gem_atomic_helper.c | 55 ++++++++++++++++++++++--
 drivers/gpu/drm/vkms/vkms_composer.c    | 26 ++++++-----
 drivers/gpu/drm/vkms/vkms_drv.h         |  6 ++-
 drivers/gpu/drm/vkms/vkms_plane.c       | 57 ++++++-------------------
 include/drm/drm_gem_atomic_helper.h     |  6 +++
 5 files changed, 86 insertions(+), 64 deletions(-)


base-commit: 3d3b5479895dd6dd133571ded4318adf595708ba
--
2.32.0

[PATCH 1/4] drm/gem: Export implementation of shadow-plane helpers

[Thomas Zimmermann]

Export the implementation of duplicate, destroy and reset helpers for
shadow-buffered plane state. Useful for drivers that subclass struct
drm_shadow_plane_state.

The exported functions are wrappers around plane-state implementation,
but using them is the correct thing to do for drivers.

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

diff --git a/drivers/gpu/drm/drm_gem_atomic_helper.c b/drivers/gpu/drm/drm_gem_atomic_helper.c
index bc9396f2a0ed..26af09b959d4 100644
--- a/drivers/gpu/drm/drm_gem_atomic_helper.c
+++ b/drivers/gpu/drm/drm_gem_atomic_helper.c
@@ -182,6 +182,27 @@ EXPORT_SYMBOL(drm_gem_simple_display_pipe_prepare_fb);
  * Shadow-buffered Planes
  */
 
+/**
+ * __drm_gem_duplicate_shadow_plane_state - duplicates shadow-buffered plane state
+ * @plane: the plane
+ * @new_shadow_plane_state: the new shadow-buffered plane state
+ *
+ * This function duplicates shadow-buffered plane state. This is helpful for drivers
+ * that subclass struct drm_shadow_plane_state.
+ *
+ * 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.
+ */
+void
+__drm_gem_duplicate_shadow_plane_state(struct drm_plane *plane,
+				       struct drm_shadow_plane_state *new_shadow_plane_state)
+{
+	__drm_atomic_helper_plane_duplicate_state(plane, &new_shadow_plane_state->base);
+}
+EXPORT_SYMBOL(__drm_gem_duplicate_shadow_plane_state);
+
 /**
  * drm_gem_duplicate_shadow_plane_state - duplicates shadow-buffered plane state
  * @plane: the plane
@@ -211,12 +232,25 @@ drm_gem_duplicate_shadow_plane_state(struct drm_plane *plane)
 	new_shadow_plane_state = kzalloc(sizeof(*new_shadow_plane_state), GFP_KERNEL);
 	if (!new_shadow_plane_state)
 		return NULL;
-	__drm_atomic_helper_plane_duplicate_state(plane, &new_shadow_plane_state->base);
+	__drm_gem_duplicate_shadow_plane_state(plane, new_shadow_plane_state);
 
 	return &new_shadow_plane_state->base;
 }
 EXPORT_SYMBOL(drm_gem_duplicate_shadow_plane_state);
 
+/**
+ * __drm_gem_destroy_shadow_plane_state - cleans up shadow-buffered plane state
+ * @shadow_plane_state: the shadow-buffered plane state
+ *
+ * This function cleans up shadow-buffered plane state. Helpful for drivers that
+ * subclass struct drm_shadow_plane_state.
+ */
+void __drm_gem_destroy_shadow_plane_state(struct drm_shadow_plane_state *shadow_plane_state)
+{
+	__drm_atomic_helper_plane_destroy_state(&shadow_plane_state->base);
+}
+EXPORT_SYMBOL(__drm_gem_destroy_shadow_plane_state);
+
 /**
  * drm_gem_destroy_shadow_plane_state - deletes shadow-buffered plane state
  * @plane: the plane
@@ -232,11 +266,26 @@ void drm_gem_destroy_shadow_plane_state(struct drm_plane *plane,
 	struct drm_shadow_plane_state *shadow_plane_state =
 		to_drm_shadow_plane_state(plane_state);
 
-	__drm_atomic_helper_plane_destroy_state(&shadow_plane_state->base);
+	__drm_gem_destroy_shadow_plane_state(shadow_plane_state);
 	kfree(shadow_plane_state);
 }
 EXPORT_SYMBOL(drm_gem_destroy_shadow_plane_state);
 
+/**
+ * __drm_gem_reset_shadow_plane - resets a shadow-buffered plane
+ * @plane: the plane
+ * @shadow_plane_state: the shadow-buffered plane state
+ *
+ * This function resets state for shadow-buffered planes. Helpful
+ * for drivers that subclass struct drm_shadow_plane_state.
+ */
+void __drm_gem_reset_shadow_plane(struct drm_plane *plane,
+				  struct drm_shadow_plane_state *shadow_plane_state)
+{
+	__drm_atomic_helper_plane_reset(plane, &shadow_plane_state->base);
+}
+EXPORT_SYMBOL(__drm_gem_reset_shadow_plane);
+
 /**
  * drm_gem_reset_shadow_plane - resets a shadow-buffered plane
  * @plane: the plane
@@ -258,7 +307,7 @@ void drm_gem_reset_shadow_plane(struct drm_plane *plane)
 	shadow_plane_state = kzalloc(sizeof(*shadow_plane_state), GFP_KERNEL);
 	if (!shadow_plane_state)
 		return;
-	__drm_atomic_helper_plane_reset(plane, &shadow_plane_state->base);
+	__drm_gem_reset_shadow_plane(plane, shadow_plane_state);
 }
 EXPORT_SYMBOL(drm_gem_reset_shadow_plane);
 
diff --git a/include/drm/drm_gem_atomic_helper.h b/include/drm/drm_gem_atomic_helper.h
index cfc5adee3d13..d82c23622156 100644
--- a/include/drm/drm_gem_atomic_helper.h
+++ b/include/drm/drm_gem_atomic_helper.h
@@ -53,6 +53,12 @@ to_drm_shadow_plane_state(struct drm_plane_state *state)
 	return container_of(state, struct drm_shadow_plane_state, base);
 }
 
+void __drm_gem_duplicate_shadow_plane_state(struct drm_plane *plane,
+					    struct drm_shadow_plane_state *new_shadow_plane_state);
+void __drm_gem_destroy_shadow_plane_state(struct drm_shadow_plane_state *shadow_plane_state);
+void __drm_gem_reset_shadow_plane(struct drm_plane *plane,
+				  struct drm_shadow_plane_state *shadow_plane_state);
+
 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,
-- 
2.32.0

[PATCH 2/4] drm/vkms: Inherit plane state from struct drm_shadow_plane_state

[Thomas Zimmermann]

Subclass struct drm_shadow_plane_state for VKMS planes and update
all plane-state callbacks accordingly.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/vkms/vkms_composer.c |  2 +-
 drivers/gpu/drm/vkms/vkms_drv.h      |  5 +++--
 drivers/gpu/drm/vkms/vkms_plane.c    | 16 ++++++++--------
 3 files changed, 12 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
index e49523866e1d..3ade0df173d2 100644
--- a/drivers/gpu/drm/vkms/vkms_composer.c
+++ b/drivers/gpu/drm/vkms/vkms_composer.c
@@ -251,7 +251,7 @@ void vkms_composer_worker(struct work_struct *work)
 
 	if (crtc_state->num_active_planes >= 1) {
 		act_plane = crtc_state->active_planes[0];
-		if (act_plane->base.plane->type == DRM_PLANE_TYPE_PRIMARY)
+		if (act_plane->base.base.plane->type == DRM_PLANE_TYPE_PRIMARY)
 			primary_composer = act_plane->composer;
 	}
 
diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
index ac8c9c2fa4ed..7a76dccd7316 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.h
+++ b/drivers/gpu/drm/vkms/vkms_drv.h
@@ -7,6 +7,7 @@
 
 #include <drm/drm.h>
 #include <drm/drm_gem.h>
+#include <drm/drm_gem_atomic_helper.h>
 #include <drm/drm_encoder.h>
 #include <drm/drm_writeback.h>
 
@@ -33,7 +34,7 @@ struct vkms_composer {
  * @composer: data required for composing computation
  */
 struct vkms_plane_state {
-	struct drm_plane_state base;
+	struct drm_shadow_plane_state base;
 	struct vkms_composer *composer;
 };
 
@@ -111,7 +112,7 @@ struct vkms_device {
 	container_of(target, struct vkms_crtc_state, base)
 
 #define to_vkms_plane_state(target)\
-	container_of(target, struct vkms_plane_state, base)
+	container_of(target, struct vkms_plane_state, base.base)
 
 /* CRTC */
 int vkms_crtc_init(struct drm_device *dev, struct drm_crtc *crtc,
diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
index 107521ace597..6ee4fa71bd87 100644
--- a/drivers/gpu/drm/vkms/vkms_plane.c
+++ b/drivers/gpu/drm/vkms/vkms_plane.c
@@ -40,17 +40,16 @@ vkms_plane_duplicate_state(struct drm_plane *plane)
 
 	vkms_state->composer = composer;
 
-	__drm_atomic_helper_plane_duplicate_state(plane,
-						  &vkms_state->base);
+	__drm_gem_duplicate_shadow_plane_state(plane, &vkms_state->base);
 
-	return &vkms_state->base;
+	return &vkms_state->base.base;
 }
 
 static void vkms_plane_destroy_state(struct drm_plane *plane,
 				     struct drm_plane_state *old_state)
 {
 	struct vkms_plane_state *vkms_state = to_vkms_plane_state(old_state);
-	struct drm_crtc *crtc = vkms_state->base.crtc;
+	struct drm_crtc *crtc = vkms_state->base.base.crtc;
 
 	if (crtc) {
 		/* dropping the reference we acquired in
@@ -63,7 +62,7 @@ static void vkms_plane_destroy_state(struct drm_plane *plane,
 	kfree(vkms_state->composer);
 	vkms_state->composer = NULL;
 
-	__drm_atomic_helper_plane_destroy_state(old_state);
+	__drm_gem_destroy_shadow_plane_state(&vkms_state->base);
 	kfree(vkms_state);
 }
 
@@ -71,8 +70,10 @@ static void vkms_plane_reset(struct drm_plane *plane)
 {
 	struct vkms_plane_state *vkms_state;
 
-	if (plane->state)
+	if (plane->state) {
 		vkms_plane_destroy_state(plane, plane->state);
+		plane->state = NULL; /* must be set to NULL here */
+	}
 
 	vkms_state = kzalloc(sizeof(*vkms_state), GFP_KERNEL);
 	if (!vkms_state) {
@@ -80,8 +81,7 @@ static void vkms_plane_reset(struct drm_plane *plane)
 		return;
 	}
 
-	plane->state = &vkms_state->base;
-	plane->state->plane = plane;
+	__drm_gem_reset_shadow_plane(plane, &vkms_state->base);
 }
 
 static const struct drm_plane_funcs vkms_plane_funcs = {
-- 
2.32.0

[PATCH 3/4] drm/vkms: Let shadow-plane helpers prepare the plane's FB

[Thomas Zimmermann]

Replace vkms' prepare_fb and cleanup_fb functions with the generic
code for shadow-buffered planes. No functional changes.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/vkms/vkms_plane.c | 38 +------------------------------
 1 file changed, 1 insertion(+), 37 deletions(-)

diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
index 6ee4fa71bd87..8fbbd148163d 100644
--- a/drivers/gpu/drm/vkms/vkms_plane.c
+++ b/drivers/gpu/drm/vkms/vkms_plane.c
@@ -8,7 +8,6 @@
 #include <drm/drm_gem_atomic_helper.h>
 #include <drm/drm_gem_framebuffer_helper.h>
 #include <drm/drm_plane_helper.h>
-#include <drm/drm_gem_shmem_helper.h>
 
 #include "vkms_drv.h"
 
@@ -150,45 +149,10 @@ static int vkms_plane_atomic_check(struct drm_plane *plane,
 	return 0;
 }
 
-static int vkms_prepare_fb(struct drm_plane *plane,
-			   struct drm_plane_state *state)
-{
-	struct drm_gem_object *gem_obj;
-	struct dma_buf_map map;
-	int ret;
-
-	if (!state->fb)
-		return 0;
-
-	gem_obj = drm_gem_fb_get_obj(state->fb, 0);
-	ret = drm_gem_shmem_vmap(gem_obj, &map);
-	if (ret)
-		DRM_ERROR("vmap failed: %d\n", ret);
-
-	return drm_gem_plane_helper_prepare_fb(plane, state);
-}
-
-static void vkms_cleanup_fb(struct drm_plane *plane,
-			    struct drm_plane_state *old_state)
-{
-	struct drm_gem_object *gem_obj;
-	struct drm_gem_shmem_object *shmem_obj;
-	struct dma_buf_map map;
-
-	if (!old_state->fb)
-		return;
-
-	gem_obj = drm_gem_fb_get_obj(old_state->fb, 0);
-	shmem_obj = to_drm_gem_shmem_obj(drm_gem_fb_get_obj(old_state->fb, 0));
-	dma_buf_map_set_vaddr(&map, shmem_obj->vaddr);
-	drm_gem_shmem_vunmap(gem_obj, &map);
-}
-
 static const struct drm_plane_helper_funcs vkms_primary_helper_funcs = {
 	.atomic_update		= vkms_plane_atomic_update,
 	.atomic_check		= vkms_plane_atomic_check,
-	.prepare_fb		= vkms_prepare_fb,
-	.cleanup_fb		= vkms_cleanup_fb,
+	DRM_GEM_SHADOW_PLANE_HELPER_FUNCS,
 };
 
 struct vkms_plane *vkms_plane_init(struct vkms_device *vkmsdev,
-- 
2.32.0

[PATCH 4/4] drm/vkms: Use dma-buf mapping from shadow-plane state for composing

[Thomas Zimmermann]

Store the shadow-buffer mapping's address in struct vkms_composer and
use the value when composing the output. It's the same value as stored
in the GEM SHMEM BO, but frees the composer code from its dependency
on GEM SHMEM.

Using struct dma_buf_map is how framebuffer access is supposed to be.
The long-term plan is to perform all framebuffer access via struct
dma_buf_map and avoid the details of accessing I/O and system memory.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/vkms/vkms_composer.c | 24 +++++++++++-------------
 drivers/gpu/drm/vkms/vkms_drv.h      |  1 +
 drivers/gpu/drm/vkms/vkms_plane.c    |  3 +++
 3 files changed, 15 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
index 3ade0df173d2..ead8fff81f30 100644
--- a/drivers/gpu/drm/vkms/vkms_composer.c
+++ b/drivers/gpu/drm/vkms/vkms_composer.c
@@ -6,7 +6,6 @@
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_fourcc.h>
 #include <drm/drm_gem_framebuffer_helper.h>
-#include <drm/drm_gem_shmem_helper.h>
 #include <drm/drm_vblank.h>
 
 #include "vkms_drv.h"
@@ -154,24 +153,21 @@ static void compose_plane(struct vkms_composer *primary_composer,
 			  struct vkms_composer *plane_composer,
 			  void *vaddr_out)
 {
-	struct drm_gem_object *plane_obj;
-	struct drm_gem_shmem_object *plane_shmem_obj;
 	struct drm_framebuffer *fb = &plane_composer->fb;
+	void *vaddr;
 	void (*pixel_blend)(const u8 *p_src, u8 *p_dst);
 
-	plane_obj = drm_gem_fb_get_obj(&plane_composer->fb, 0);
-	plane_shmem_obj = to_drm_gem_shmem_obj(plane_obj);
-
-	if (WARN_ON(!plane_shmem_obj->vaddr))
+	if (WARN_ON(dma_buf_map_is_null(&primary_composer->map[0])))
 		return;
 
+	vaddr = plane_composer->map[0].vaddr;
+
 	if (fb->format->format == DRM_FORMAT_ARGB8888)
 		pixel_blend = &alpha_blend;
 	else
 		pixel_blend = &x_blend;
 
-	blend(vaddr_out, plane_shmem_obj->vaddr, primary_composer,
-	      plane_composer, pixel_blend);
+	blend(vaddr_out, vaddr, primary_composer, plane_composer, pixel_blend);
 }
 
 static int compose_active_planes(void **vaddr_out,
@@ -180,21 +176,23 @@ static int compose_active_planes(void **vaddr_out,
 {
 	struct drm_framebuffer *fb = &primary_composer->fb;
 	struct drm_gem_object *gem_obj = drm_gem_fb_get_obj(fb, 0);
-	struct drm_gem_shmem_object *shmem_obj = to_drm_gem_shmem_obj(gem_obj);
+	const void *vaddr;
 	int i;
 
 	if (!*vaddr_out) {
-		*vaddr_out = kzalloc(shmem_obj->base.size, GFP_KERNEL);
+		*vaddr_out = kzalloc(gem_obj->size, GFP_KERNEL);
 		if (!*vaddr_out) {
 			DRM_ERROR("Cannot allocate memory for output frame.");
 			return -ENOMEM;
 		}
 	}
 
-	if (WARN_ON(!shmem_obj->vaddr))
+	if (WARN_ON(dma_buf_map_is_null(&primary_composer->map[0])))
 		return -EINVAL;
 
-	memcpy(*vaddr_out, shmem_obj->vaddr, shmem_obj->base.size);
+	vaddr = primary_composer->map[0].vaddr;
+
+	memcpy(*vaddr_out, vaddr, gem_obj->size);
 
 	/* If there are other planes besides primary, we consider the active
 	 * planes should be in z-order and compose them associatively:
diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
index 7a76dccd7316..8c731b6dcba7 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.h
+++ b/drivers/gpu/drm/vkms/vkms_drv.h
@@ -23,6 +23,7 @@
 struct vkms_composer {
 	struct drm_framebuffer fb;
 	struct drm_rect src, dst;
+	struct dma_buf_map map[4];
 	unsigned int offset;
 	unsigned int pitch;
 	unsigned int cpp;
diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
index 8fbbd148163d..8a56fbf572b0 100644
--- a/drivers/gpu/drm/vkms/vkms_plane.c
+++ b/drivers/gpu/drm/vkms/vkms_plane.c
@@ -97,6 +97,7 @@ static void vkms_plane_atomic_update(struct drm_plane *plane,
 	struct drm_plane_state *new_state = drm_atomic_get_new_plane_state(state,
 									   plane);
 	struct vkms_plane_state *vkms_plane_state;
+	struct drm_shadow_plane_state *shadow_plane_state;
 	struct drm_framebuffer *fb = new_state->fb;
 	struct vkms_composer *composer;
 
@@ -104,11 +105,13 @@ static void vkms_plane_atomic_update(struct drm_plane *plane,
 		return;
 
 	vkms_plane_state = to_vkms_plane_state(new_state);
+	shadow_plane_state = &vkms_plane_state->base;
 
 	composer = vkms_plane_state->composer;
 	memcpy(&composer->src, &new_state->src, sizeof(struct drm_rect));
 	memcpy(&composer->dst, &new_state->dst, sizeof(struct drm_rect));
 	memcpy(&composer->fb, fb, sizeof(struct drm_framebuffer));
+	memcpy(&composer->map, &shadow_plane_state->map, sizeof(composer->map));
 	drm_framebuffer_get(&composer->fb);
 	composer->offset = fb->offsets[0];
 	composer->pitch = fb->pitches[0];
-- 
2.32.0

Re: [PATCH 0/4] vkms: Switch to shadow-buffered plane state

[Daniel Vetter]

On Mon, Jul 05, 2021 at 09:46:29AM +0200, Thomas Zimmermann wrote:
> Vkms copies each plane's framebuffer into the output buffer; essentially
> using a shadow buffer. DRM provides struct drm_shadow_plane_state, which
> handles the details of mapping/unmapping shadow buffers into memory for
> active planes.
> 
> Convert vkms to the helpers. Makes vkms use shared code and gives more
> test exposure to shadow-plane helpers.
> 
> Thomas Zimmermann (4):
>   drm/gem: Export implementation of shadow-plane helpers
>   drm/vkms: Inherit plane state from struct drm_shadow_plane_state
>   drm/vkms: Let shadow-plane helpers prepare the plane's FB
>   drm/vkms: Use dma-buf mapping from shadow-plane state for composing

So I think right now this fits, but I think it'll mismit going forward: We
don't really have a shadow-plane that we then toss to the hw, it's a
shadow-crtc-area. Right now there's no difference, because we don't
support positioning/scaling the primary plane. But that's all kinda stuff
that's on the table.

But conceptually at least the compositioning buffer should bet part of the
crtc, not of the primary plane.

So not sure what to do, but also coffee hasn't kicked in yet, so maybe I'm
just confused.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 0/4] vkms: Switch to shadow-buffered plane state

[Thomas Zimmermann]

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

Hi

Am 05.07.21 um 11:27 schrieb Daniel Vetter:
> On Mon, Jul 05, 2021 at 09:46:29AM +0200, Thomas Zimmermann wrote:
>> Vkms copies each plane's framebuffer into the output buffer; essentially
>> using a shadow buffer. DRM provides struct drm_shadow_plane_state, which
>> handles the details of mapping/unmapping shadow buffers into memory for
>> active planes.
>>
>> Convert vkms to the helpers. Makes vkms use shared code and gives more
>> test exposure to shadow-plane helpers.
>>
>> Thomas Zimmermann (4):
>>    drm/gem: Export implementation of shadow-plane helpers
>>    drm/vkms: Inherit plane state from struct drm_shadow_plane_state
>>    drm/vkms: Let shadow-plane helpers prepare the plane's FB
>>    drm/vkms: Use dma-buf mapping from shadow-plane state for composing
> 
> So I think right now this fits, but I think it'll mismit going forward: We
> don't really have a shadow-plane that we then toss to the hw, it's a
> shadow-crtc-area. Right now there's no difference, because we don't
> support positioning/scaling the primary plane. But that's all kinda stuff
> that's on the table.
> 
> But conceptually at least the compositioning buffer should bet part of the
> crtc, not of the primary plane.
> 
> So not sure what to do, but also coffee hasn't kicked in yet, so maybe I'm
> just confused.

I'm not sure if I understand your concern. Can you elaborate? The 
compositing output buffer is not affected by this patchset. Only the 
input frambuffers of the planes. Those are shadow buffers. AFAICT the 
composer code memcpy's the primary plane and then blends the other 
planes on top. Supporting transformation of the primary plane doesn't 
really change much wrt to the vmaping of input fbs.

Best regards
Thomas

> -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: Felix Imendörffer


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

Re: [PATCH 0/4] vkms: Switch to shadow-buffered plane state

[Daniel Vetter]

On Mon, Jul 05, 2021 at 12:05:28PM +0200, Thomas Zimmermann wrote:
> Hi
> 
> Am 05.07.21 um 11:27 schrieb Daniel Vetter:
> > On Mon, Jul 05, 2021 at 09:46:29AM +0200, Thomas Zimmermann wrote:
> > > Vkms copies each plane's framebuffer into the output buffer; essentially
> > > using a shadow buffer. DRM provides struct drm_shadow_plane_state, which
> > > handles the details of mapping/unmapping shadow buffers into memory for
> > > active planes.
> > > 
> > > Convert vkms to the helpers. Makes vkms use shared code and gives more
> > > test exposure to shadow-plane helpers.
> > > 
> > > Thomas Zimmermann (4):
> > >    drm/gem: Export implementation of shadow-plane helpers
> > >    drm/vkms: Inherit plane state from struct drm_shadow_plane_state
> > >    drm/vkms: Let shadow-plane helpers prepare the plane's FB
> > >    drm/vkms: Use dma-buf mapping from shadow-plane state for composing
> > 
> > So I think right now this fits, but I think it'll mismit going forward: We
> > don't really have a shadow-plane that we then toss to the hw, it's a
> > shadow-crtc-area. Right now there's no difference, because we don't
> > support positioning/scaling the primary plane. But that's all kinda stuff
> > that's on the table.
> > 
> > But conceptually at least the compositioning buffer should bet part of the
> > crtc, not of the primary plane.
> > 
> > So not sure what to do, but also coffee hasn't kicked in yet, so maybe I'm
> > just confused.
> 
> I'm not sure if I understand your concern. Can you elaborate? The
> compositing output buffer is not affected by this patchset. Only the input
> frambuffers of the planes. Those are shadow buffers. AFAICT the composer
> code memcpy's the primary plane and then blends the other planes on top.
> Supporting transformation of the primary plane doesn't really change much
> wrt to the vmaping of input fbs.

Yeah that's the current implementation, because that's easier. But
fundamentally we don't need a copy of the input shadow plane, we need a
scratch area that's sized for the crtc.

So if the primary plane is smaller than the crtc window (because we use
plane hw for compositing, or maybe primary plane shows a vidoe with black
borders or whatever), then the primary plane shadow isn't the right size.

And yes this means some surgery, vkms isn't there yet at all. But still it
would mean we're going right here, but then have to backtrack before we
can go left again. So a detour.

Also I don't think any other driver will ever need this, you really only
need it when you want to composite planes in software - which defeats the
purpose of planes. Except when the goal of your driver is to be a software
model.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 0/4] vkms: Switch to shadow-buffered plane state

[Melissa Wen]

On 07/05, Daniel Vetter wrote:
> On Mon, Jul 05, 2021 at 12:05:28PM +0200, Thomas Zimmermann wrote:
> > Hi
> > 
> > Am 05.07.21 um 11:27 schrieb Daniel Vetter:
> > > On Mon, Jul 05, 2021 at 09:46:29AM +0200, Thomas Zimmermann wrote:
> > > > Vkms copies each plane's framebuffer into the output buffer; essentially
> > > > using a shadow buffer. DRM provides struct drm_shadow_plane_state, which
> > > > handles the details of mapping/unmapping shadow buffers into memory for
> > > > active planes.
> > > > 
> > > > Convert vkms to the helpers. Makes vkms use shared code and gives more
> > > > test exposure to shadow-plane helpers.
> > > > 
> > > > Thomas Zimmermann (4):
> > > >    drm/gem: Export implementation of shadow-plane helpers
> > > >    drm/vkms: Inherit plane state from struct drm_shadow_plane_state
> > > >    drm/vkms: Let shadow-plane helpers prepare the plane's FB
> > > >    drm/vkms: Use dma-buf mapping from shadow-plane state for composing
> > > 
> > > So I think right now this fits, but I think it'll mismit going forward: We
> > > don't really have a shadow-plane that we then toss to the hw, it's a
> > > shadow-crtc-area. Right now there's no difference, because we don't
> > > support positioning/scaling the primary plane. But that's all kinda stuff
> > > that's on the table.
> > > 
> > > But conceptually at least the compositioning buffer should bet part of the
> > > crtc, not of the primary plane.
> > > 
> > > So not sure what to do, but also coffee hasn't kicked in yet, so maybe I'm
> > > just confused.
> > 
> > I'm not sure if I understand your concern. Can you elaborate? The
> > compositing output buffer is not affected by this patchset. Only the input
> > frambuffers of the planes. Those are shadow buffers. AFAICT the composer
> > code memcpy's the primary plane and then blends the other planes on top.
> > Supporting transformation of the primary plane doesn't really change much
> > wrt to the vmaping of input fbs.
> 
> Yeah that's the current implementation, because that's easier. But
> fundamentally we don't need a copy of the input shadow plane, we need a
> scratch area that's sized for the crtc.

Maybe I'm missing something, but I am not sure the relevance for vkms to
switch to shadow-buffered plane. (?)

Btw, in terms of code changes, it works well and also vmap error stops
to happen (reported in the last update of todo list). This fix seems to
be a side-effect because it replaces the vkms_prepare_fb that got
problematic since (I guess) the last switch to shmem. 

Thanks,
Melissa

> So if the primary plane is smaller than the crtc window (because we use
> plane hw for compositing, or maybe primary plane shows a vidoe with black
> borders or whatever), then the primary plane shadow isn't the right size.
> 
> And yes this means some surgery, vkms isn't there yet at all. But still it
> would mean we're going right here, but then have to backtrack before we
> can go left again. So a detour.
> 
> Also I don't think any other driver will ever need this, you really only
> need it when you want to composite planes in software - which defeats the
> purpose of planes. Except when the goal of your driver is to be a software
> model.
> -Daniel
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

Re: [PATCH 0/4] vkms: Switch to shadow-buffered plane state

[Thomas Zimmermann]

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

Hi

Am 05.07.21 um 23:29 schrieb Melissa Wen:
> On 07/05, Daniel Vetter wrote:
>> On Mon, Jul 05, 2021 at 12:05:28PM +0200, Thomas Zimmermann wrote:
>>> Hi
>>>
>>> Am 05.07.21 um 11:27 schrieb Daniel Vetter:
>>>> On Mon, Jul 05, 2021 at 09:46:29AM +0200, Thomas Zimmermann wrote:
>>>>> Vkms copies each plane's framebuffer into the output buffer; essentially
>>>>> using a shadow buffer. DRM provides struct drm_shadow_plane_state, which
>>>>> handles the details of mapping/unmapping shadow buffers into memory for
>>>>> active planes.
>>>>>
>>>>> Convert vkms to the helpers. Makes vkms use shared code and gives more
>>>>> test exposure to shadow-plane helpers.
>>>>>
>>>>> Thomas Zimmermann (4):
>>>>>     drm/gem: Export implementation of shadow-plane helpers
>>>>>     drm/vkms: Inherit plane state from struct drm_shadow_plane_state
>>>>>     drm/vkms: Let shadow-plane helpers prepare the plane's FB
>>>>>     drm/vkms: Use dma-buf mapping from shadow-plane state for composing
>>>>
>>>> So I think right now this fits, but I think it'll mismit going forward: We
>>>> don't really have a shadow-plane that we then toss to the hw, it's a
>>>> shadow-crtc-area. Right now there's no difference, because we don't
>>>> support positioning/scaling the primary plane. But that's all kinda stuff
>>>> that's on the table.
>>>>
>>>> But conceptually at least the compositioning buffer should bet part of the
>>>> crtc, not of the primary plane.
>>>>
>>>> So not sure what to do, but also coffee hasn't kicked in yet, so maybe I'm
>>>> just confused.
>>>
>>> I'm not sure if I understand your concern. Can you elaborate? The
>>> compositing output buffer is not affected by this patchset. Only the input
>>> frambuffers of the planes. Those are shadow buffers. AFAICT the composer
>>> code memcpy's the primary plane and then blends the other planes on top.
>>> Supporting transformation of the primary plane doesn't really change much
>>> wrt to the vmaping of input fbs.
>>
>> Yeah that's the current implementation, because that's easier. But
>> fundamentally we don't need a copy of the input shadow plane, we need a
>> scratch area that's sized for the crtc.
> 
> Maybe I'm missing something, but I am not sure the relevance for vkms to
> switch to shadow-buffered plane. (?)

It replaces the vkms code with shared code. Nothing else. For the shared 
shadow-buffer code it means more testing. If vkms ever supports color 
formats that use multiple planes, the new code will be ready.

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: Felix Imendörffer


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

Re: [PATCH 0/4] vkms: Switch to shadow-buffered plane state

[Melissa Wen]

On 07/06, Thomas Zimmermann wrote:
> Hi
> 
> Am 05.07.21 um 23:29 schrieb Melissa Wen:
> > On 07/05, Daniel Vetter wrote:
> > > On Mon, Jul 05, 2021 at 12:05:28PM +0200, Thomas Zimmermann wrote:
> > > > Hi
> > > > 
> > > > Am 05.07.21 um 11:27 schrieb Daniel Vetter:
> > > > > On Mon, Jul 05, 2021 at 09:46:29AM +0200, Thomas Zimmermann wrote:
> > > > > > Vkms copies each plane's framebuffer into the output buffer; essentially
> > > > > > using a shadow buffer. DRM provides struct drm_shadow_plane_state, which
> > > > > > handles the details of mapping/unmapping shadow buffers into memory for
> > > > > > active planes.
> > > > > > 
> > > > > > Convert vkms to the helpers. Makes vkms use shared code and gives more
> > > > > > test exposure to shadow-plane helpers.
> > > > > > 
> > > > > > Thomas Zimmermann (4):
> > > > > >     drm/gem: Export implementation of shadow-plane helpers
> > > > > >     drm/vkms: Inherit plane state from struct drm_shadow_plane_state
> > > > > >     drm/vkms: Let shadow-plane helpers prepare the plane's FB
> > > > > >     drm/vkms: Use dma-buf mapping from shadow-plane state for composing
> > > > > 
> > > > > So I think right now this fits, but I think it'll mismit going forward: We
> > > > > don't really have a shadow-plane that we then toss to the hw, it's a
> > > > > shadow-crtc-area. Right now there's no difference, because we don't
> > > > > support positioning/scaling the primary plane. But that's all kinda stuff
> > > > > that's on the table.
> > > > > 
> > > > > But conceptually at least the compositioning buffer should bet part of the
> > > > > crtc, not of the primary plane.
> > > > > 
> > > > > So not sure what to do, but also coffee hasn't kicked in yet, so maybe I'm
> > > > > just confused.
> > > > 
> > > > I'm not sure if I understand your concern. Can you elaborate? The
> > > > compositing output buffer is not affected by this patchset. Only the input
> > > > frambuffers of the planes. Those are shadow buffers. AFAICT the composer
> > > > code memcpy's the primary plane and then blends the other planes on top.
> > > > Supporting transformation of the primary plane doesn't really change much
> > > > wrt to the vmaping of input fbs.
> > > 
> > > Yeah that's the current implementation, because that's easier. But
> > > fundamentally we don't need a copy of the input shadow plane, we need a
> > > scratch area that's sized for the crtc.
> > 
> > Maybe I'm missing something, but I am not sure the relevance for vkms to
> > switch to shadow-buffered plane. (?)
> 
> It replaces the vkms code with shared code. Nothing else. For the shared
> shadow-buffer code it means more testing. If vkms ever supports color
> formats that use multiple planes, the new code will be ready.
Hi,

lgtm.

For debug history, can you please include in the description of the
third patch (shadow-plane helpers to prepare fb) that vmap failure is no
longer displayed in the execution some IGT kms_flip testcases?

For the series:
Reviewed-by: Melissa Wen <melissa.srw@gmail.com>

Thanks,
Melissa
> 
> 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: Felix Imendörffer
> 

Re: [PATCH 0/4] vkms: Switch to shadow-buffered plane state

[Thomas Zimmermann]

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

Hi

Am 05.07.21 um 16:20 schrieb Daniel Vetter:
> On Mon, Jul 05, 2021 at 12:05:28PM +0200, Thomas Zimmermann wrote:
>> Hi
>>
>> Am 05.07.21 um 11:27 schrieb Daniel Vetter:
>>> On Mon, Jul 05, 2021 at 09:46:29AM +0200, Thomas Zimmermann wrote:
>>>> Vkms copies each plane's framebuffer into the output buffer; essentially
>>>> using a shadow buffer. DRM provides struct drm_shadow_plane_state, which
>>>> handles the details of mapping/unmapping shadow buffers into memory for
>>>> active planes.
>>>>
>>>> Convert vkms to the helpers. Makes vkms use shared code and gives more
>>>> test exposure to shadow-plane helpers.
>>>>
>>>> Thomas Zimmermann (4):
>>>>     drm/gem: Export implementation of shadow-plane helpers
>>>>     drm/vkms: Inherit plane state from struct drm_shadow_plane_state
>>>>     drm/vkms: Let shadow-plane helpers prepare the plane's FB
>>>>     drm/vkms: Use dma-buf mapping from shadow-plane state for composing
>>>
>>> So I think right now this fits, but I think it'll mismit going forward: We
>>> don't really have a shadow-plane that we then toss to the hw, it's a
>>> shadow-crtc-area. Right now there's no difference, because we don't
>>> support positioning/scaling the primary plane. But that's all kinda stuff
>>> that's on the table.
>>>
>>> But conceptually at least the compositioning buffer should bet part of the
>>> crtc, not of the primary plane.
>>>
>>> So not sure what to do, but also coffee hasn't kicked in yet, so maybe I'm
>>> just confused.
>>
>> I'm not sure if I understand your concern. Can you elaborate? The
>> compositing output buffer is not affected by this patchset. Only the input
>> frambuffers of the planes. Those are shadow buffers. AFAICT the composer
>> code memcpy's the primary plane and then blends the other planes on top.
>> Supporting transformation of the primary plane doesn't really change much
>> wrt to the vmaping of input fbs.
> 
> Yeah that's the current implementation, because that's easier. But
> fundamentally we don't need a copy of the input shadow plane, we need a
> scratch area that's sized for the crtc.

I stll don't understand. Are you talking about the whole patchset or 
just patch 4? Because if you want to do anything with vkms planes, you 
have to vmap the framebuffer BOs into memory. (right?) That's all that 
shadow-buffer planes do. Patches 1 to 3 have nothing to do with memcpy. 
Admittedly, Patch 4 does some minor refactoring, but only because it 
became a low-hanging fruit.

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: Felix Imendörffer


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

Re: [PATCH 0/4] vkms: Switch to shadow-buffered plane state

[Daniel Vetter]

On Tue, Jul 06, 2021 at 06:59:07AM +0200, Thomas Zimmermann wrote:
> Am 05.07.21 um 16:20 schrieb Daniel Vetter:
> > On Mon, Jul 05, 2021 at 12:05:28PM +0200, Thomas Zimmermann wrote:
> > > Hi
> > > 
> > > Am 05.07.21 um 11:27 schrieb Daniel Vetter:
> > > > On Mon, Jul 05, 2021 at 09:46:29AM +0200, Thomas Zimmermann wrote:
> > > > > Vkms copies each plane's framebuffer into the output buffer; essentially
> > > > > using a shadow buffer. DRM provides struct drm_shadow_plane_state, which
> > > > > handles the details of mapping/unmapping shadow buffers into memory for
> > > > > active planes.
> > > > > 
> > > > > Convert vkms to the helpers. Makes vkms use shared code and gives more
> > > > > test exposure to shadow-plane helpers.
> > > > > 
> > > > > Thomas Zimmermann (4):
> > > > >     drm/gem: Export implementation of shadow-plane helpers
> > > > >     drm/vkms: Inherit plane state from struct drm_shadow_plane_state
> > > > >     drm/vkms: Let shadow-plane helpers prepare the plane's FB
> > > > >     drm/vkms: Use dma-buf mapping from shadow-plane state for composing
> > > > 
> > > > So I think right now this fits, but I think it'll mismit going forward: We
> > > > don't really have a shadow-plane that we then toss to the hw, it's a
> > > > shadow-crtc-area. Right now there's no difference, because we don't
> > > > support positioning/scaling the primary plane. But that's all kinda stuff
> > > > that's on the table.
> > > > 
> > > > But conceptually at least the compositioning buffer should bet part of the
> > > > crtc, not of the primary plane.
> > > > 
> > > > So not sure what to do, but also coffee hasn't kicked in yet, so maybe I'm
> > > > just confused.
> > > 
> > > I'm not sure if I understand your concern. Can you elaborate? The
> > > compositing output buffer is not affected by this patchset. Only the input
> > > frambuffers of the planes. Those are shadow buffers. AFAICT the composer
> > > code memcpy's the primary plane and then blends the other planes on top.
> > > Supporting transformation of the primary plane doesn't really change much
> > > wrt to the vmaping of input fbs.
> > 
> > Yeah that's the current implementation, because that's easier. But
> > fundamentally we don't need a copy of the input shadow plane, we need a
> > scratch area that's sized for the crtc.
> 
> I stll don't understand. Are you talking about the whole patchset or just
> patch 4? Because if you want to do anything with vkms planes, you have to
> vmap the framebuffer BOs into memory. (right?) That's all that shadow-buffer
> planes do. Patches 1 to 3 have nothing to do with memcpy. Admittedly, Patch
> 4 does some minor refactoring, but only because it became a low-hanging
> fruit.

Oh man I need to hand in my patch reading license last few days, this aint
the first one :-(

Yeah looks all good to me, and makes total sense. Maybe I'll leave
detailed review to Melissa since I got this all wrong this much.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 0/4] vkms: Switch to shadow-buffered plane state

[Sumera Priyadarsini]

On Mon, Jul 5, 2021 at 1:16 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>
> Vkms copies each plane's framebuffer into the output buffer; essentially
> using a shadow buffer. DRM provides struct drm_shadow_plane_state, which
> handles the details of mapping/unmapping shadow buffers into memory for
> active planes.
>
> Convert vkms to the helpers. Makes vkms use shared code and gives more
> test exposure to shadow-plane helpers.
>
> Thomas Zimmermann (4):
>   drm/gem: Export implementation of shadow-plane helpers
>   drm/vkms: Inherit plane state from struct drm_shadow_plane_state
>   drm/vkms: Let shadow-plane helpers prepare the plane's FB
>   drm/vkms: Use dma-buf mapping from shadow-plane state for composing
>
>  drivers/gpu/drm/drm_gem_atomic_helper.c | 55 ++++++++++++++++++++++--
>  drivers/gpu/drm/vkms/vkms_composer.c    | 26 ++++++-----
>  drivers/gpu/drm/vkms/vkms_drv.h         |  6 ++-
>  drivers/gpu/drm/vkms/vkms_plane.c       | 57 ++++++-------------------
>  include/drm/drm_gem_atomic_helper.h     |  6 +++
>  5 files changed, 86 insertions(+), 64 deletions(-)
>
>
> base-commit: 3d3b5479895dd6dd133571ded4318adf595708ba
> --
> 2.32.0
>
Hi,

Thanks for the patches. The switch to shadow-plane helpers also solved
a bug that was causing a kernel
panic during some IGT kms_flip subtests on the vkms virtual hw patch.

Tested-by: Sumera Priyadarsini <sylphrenadin@gmail.com>

Cheers,
Sumera

Re: [PATCH 0/4] vkms: Switch to shadow-buffered plane state

[Thomas Zimmermann]

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

Hi

Am 12.07.21 um 13:56 schrieb Sumera Priyadarsini:
> On Mon, Jul 5, 2021 at 1:16 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>>
>> Vkms copies each plane's framebuffer into the output buffer; essentially
>> using a shadow buffer. DRM provides struct drm_shadow_plane_state, which
>> handles the details of mapping/unmapping shadow buffers into memory for
>> active planes.
>>
>> Convert vkms to the helpers. Makes vkms use shared code and gives more
>> test exposure to shadow-plane helpers.
>>
>> Thomas Zimmermann (4):
>>    drm/gem: Export implementation of shadow-plane helpers
>>    drm/vkms: Inherit plane state from struct drm_shadow_plane_state
>>    drm/vkms: Let shadow-plane helpers prepare the plane's FB
>>    drm/vkms: Use dma-buf mapping from shadow-plane state for composing
>>
>>   drivers/gpu/drm/drm_gem_atomic_helper.c | 55 ++++++++++++++++++++++--
>>   drivers/gpu/drm/vkms/vkms_composer.c    | 26 ++++++-----
>>   drivers/gpu/drm/vkms/vkms_drv.h         |  6 ++-
>>   drivers/gpu/drm/vkms/vkms_plane.c       | 57 ++++++-------------------
>>   include/drm/drm_gem_atomic_helper.h     |  6 +++
>>   5 files changed, 86 insertions(+), 64 deletions(-)
>>
>>
>> base-commit: 3d3b5479895dd6dd133571ded4318adf595708ba
>> --
>> 2.32.0
>>
> Hi,
> 
> Thanks for the patches. The switch to shadow-plane helpers also solved
> a bug that was causing a kernel
> panic during some IGT kms_flip subtests on the vkms virtual hw patch.

Melissa mention something like that as well and I don't really 
understand. Patch 3 removes an error message from the code, but is the 
actual bug also gone?

There's little difference between vkms' original code and the shared 
helper; except for the order of operations in prepare_fb. The shared 
helper synchronizes fences before mapping; vkms mapped first.

(Maybe the shared helper should warn about failed vmaps as well. But 
that's for another patch.)

Best regards
Thomas

> 
> Tested-by: Sumera Priyadarsini <sylphrenadin@gmail.com>
> 
> Cheers,
> Sumera
> 

-- 
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 #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH 0/4] vkms: Switch to shadow-buffered plane state

[Sumera Priyadarsini]

On Mon, Jul 12, 2021 at 6:53 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>
> Hi
>
> Am 12.07.21 um 13:56 schrieb Sumera Priyadarsini:
> > On Mon, Jul 5, 2021 at 1:16 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
> >>
> >> Vkms copies each plane's framebuffer into the output buffer; essentially
> >> using a shadow buffer. DRM provides struct drm_shadow_plane_state, which
> >> handles the details of mapping/unmapping shadow buffers into memory for
> >> active planes.
> >>
> >> Convert vkms to the helpers. Makes vkms use shared code and gives more
> >> test exposure to shadow-plane helpers.
> >>
> >> Thomas Zimmermann (4):
> >>    drm/gem: Export implementation of shadow-plane helpers
> >>    drm/vkms: Inherit plane state from struct drm_shadow_plane_state
> >>    drm/vkms: Let shadow-plane helpers prepare the plane's FB
> >>    drm/vkms: Use dma-buf mapping from shadow-plane state for composing
> >>
> >>   drivers/gpu/drm/drm_gem_atomic_helper.c | 55 ++++++++++++++++++++++--
> >>   drivers/gpu/drm/vkms/vkms_composer.c    | 26 ++++++-----
> >>   drivers/gpu/drm/vkms/vkms_drv.h         |  6 ++-
> >>   drivers/gpu/drm/vkms/vkms_plane.c       | 57 ++++++-------------------
> >>   include/drm/drm_gem_atomic_helper.h     |  6 +++
> >>   5 files changed, 86 insertions(+), 64 deletions(-)
> >>
> >>
> >> base-commit: 3d3b5479895dd6dd133571ded4318adf595708ba
> >> --
> >> 2.32.0
> >>
> > Hi,
> >
> > Thanks for the patches. The switch to shadow-plane helpers also solved
> > a bug that was causing a kernel
> > panic during some IGT kms_flip subtests on the vkms virtual hw patch.
>
> Melissa mention something like that as well and I don't really
> understand. Patch 3 removes an error message from the code, but is the
> actual bug also gone?

Yes, I think so. Earlier, while testing the vkms virtual hw patch, the
tests were
not just failing, but the vmap fail also preceeded a page fault which required a
whole restart. Check these logs around line 303:
https://pastebin.pl/view/03b750be.

I could be wrong but I think if the same bug was still present, then
the kernel panic
would also happen even if the error message was not being returned.

Cheers,
Sumera

>
> There's little difference between vkms' original code and the shared
> helper; except for the order of operations in prepare_fb. The shared
> helper synchronizes fences before mapping; vkms mapped first.
>
> (Maybe the shared helper should warn about failed vmaps as well. But
> that's for another patch.)
>
> Best regards
> Thomas
>
> >
> > Tested-by: Sumera Priyadarsini <sylphrenadin@gmail.com>
> >
> > Cheers,
> > Sumera
> >
>
> --
> 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
>

Re: [PATCH 0/4] vkms: Switch to shadow-buffered plane state

[Thomas Zimmermann]

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

Hi

Am 12.07.21 um 16:26 schrieb Sumera Priyadarsini:
> On Mon, Jul 12, 2021 at 6:53 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>>
>> Hi
>>
>> Am 12.07.21 um 13:56 schrieb Sumera Priyadarsini:
>>> On Mon, Jul 5, 2021 at 1:16 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>>>>
>>>> Vkms copies each plane's framebuffer into the output buffer; essentially
>>>> using a shadow buffer. DRM provides struct drm_shadow_plane_state, which
>>>> handles the details of mapping/unmapping shadow buffers into memory for
>>>> active planes.
>>>>
>>>> Convert vkms to the helpers. Makes vkms use shared code and gives more
>>>> test exposure to shadow-plane helpers.
>>>>
>>>> Thomas Zimmermann (4):
>>>>     drm/gem: Export implementation of shadow-plane helpers
>>>>     drm/vkms: Inherit plane state from struct drm_shadow_plane_state
>>>>     drm/vkms: Let shadow-plane helpers prepare the plane's FB
>>>>     drm/vkms: Use dma-buf mapping from shadow-plane state for composing
>>>>
>>>>    drivers/gpu/drm/drm_gem_atomic_helper.c | 55 ++++++++++++++++++++++--
>>>>    drivers/gpu/drm/vkms/vkms_composer.c    | 26 ++++++-----
>>>>    drivers/gpu/drm/vkms/vkms_drv.h         |  6 ++-
>>>>    drivers/gpu/drm/vkms/vkms_plane.c       | 57 ++++++-------------------
>>>>    include/drm/drm_gem_atomic_helper.h     |  6 +++
>>>>    5 files changed, 86 insertions(+), 64 deletions(-)
>>>>
>>>>
>>>> base-commit: 3d3b5479895dd6dd133571ded4318adf595708ba
>>>> --
>>>> 2.32.0
>>>>
>>> Hi,
>>>
>>> Thanks for the patches. The switch to shadow-plane helpers also solved
>>> a bug that was causing a kernel
>>> panic during some IGT kms_flip subtests on the vkms virtual hw patch.
>>
>> Melissa mention something like that as well and I don't really
>> understand. Patch 3 removes an error message from the code, but is the
>> actual bug also gone?
> 
> Yes, I think so. Earlier, while testing the vkms virtual hw patch, the
> tests were
> not just failing, but the vmap fail also preceeded a page fault which required a
> whole restart. Check these logs around line 303:
> https://pastebin.pl/view/03b750be.
> 

With the help of your log, I can see what's happening. The current vkms 
code reports an error in vmap, but does nothing about it. [1] So later 
during the commit, it operates with a bogus value for vaddr.

The shared helper returns the error into the atomic modesetting 
machinery, [2] which then aborts the commit. It never gets to the point 
of using an invalid address. So no kernel panic occurs.

> I could be wrong but I think if the same bug was still present, then
> the kernel panic
> would also happen even if the error message was not being returned.

I'm pretty sure the vmap issue is still there. But as the shared code 
handles it correctly without a notice to the kernel log; and it doesn't 
crash the kernel any longer.

But the vmap problem is caused by some other factor unrelated to vkms.
Booting the test kernel with drm.debug=0x1ff on the kernel command line 
would probably turn up some sort of error message.

Best regards
Thomas

[1] 
https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/vkms/vkms_plane.c#L166
[2] 
https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/drm_gem_atomic_helper.c#L299

> 
> Cheers,
> Sumera
> 
>>
>> There's little difference between vkms' original code and the shared
>> helper; except for the order of operations in prepare_fb. The shared
>> helper synchronizes fences before mapping; vkms mapped first.
>>
>> (Maybe the shared helper should warn about failed vmaps as well. But
>> that's for another patch.)
>>
>> Best regards
>> Thomas
>>
>>>
>>> Tested-by: Sumera Priyadarsini <sylphrenadin@gmail.com>
>>>
>>> Cheers,
>>> Sumera
>>>
>>
>> --
>> 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
>>

-- 
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 #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]