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
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,
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 = {
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,
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];
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
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
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
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
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
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
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
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
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
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
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
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_pla... [2] https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/drm_gem_atomi...
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
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Melissa Wen -
Sumera Priyadarsini -
Thomas Zimmermann