On Wed, Feb 3, 2021 at 3:26 PM Thomas Zimmermann <tzimmermann@xxxxxxx> wrote:
>
> Hi
>
> Am 03.02.21 um 15:01 schrieb Daniel Vetter:
> > On Wed, Feb 03, 2021 at 02:10:42PM +0100, Thomas Zimmermann wrote:
> >> Several drivers use GEM SHMEM buffer objects as shadow buffers for
> >> the actual framebuffer memory. Right now, drivers do these vmap
> >> operations in their commit tail, which is actually not allowed by the
> >> locking rules for the dma-buf reservation lock. The involved SHMEM
> >> BO has to be vmapped in the plane's prepare_fb callback and vunmapped
> >> in cleanup_fb.
> >>
> >> This patch introduces a DRM library that implements KMS helpers for
> >> GEM SHMEM buffer objects. The first set of helpers is the plane state
> >> for shadow planes. The provided implementations for prepare_fb and
> >> cleanup_fb vmap and vunmap all BOs of struct drm_plane_state.fb. The
> >> mappings are afterwards available in the plane's commit-tail functions.
> >>
> >> All rsp drivers use the simple KMS helpers, so we add the plane callbacks
> >> and wrappers for simple KMS.
> >>
> >> Signed-off-by: Thomas Zimmermann <tzimmermann@xxxxxxx>
> >> ---
> >> drivers/gpu/drm/Kconfig | 7 +
> >> drivers/gpu/drm/Makefile | 1 +
> >> drivers/gpu/drm/drm_gem_shmem_kms_helper.c | 159 +++++++++++++++++++++
> >> include/drm/drm_gem_shmem_kms_helper.h | 56 ++++++++
> >> 4 files changed, 223 insertions(+)
> >> create mode 100644 drivers/gpu/drm/drm_gem_shmem_kms_helper.c
> >> create mode 100644 include/drm/drm_gem_shmem_kms_helper.h
> >>
> >> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> >> index 8bf103de1594..b8d8b00ab5d4 100644
> >> --- a/drivers/gpu/drm/Kconfig
> >> +++ b/drivers/gpu/drm/Kconfig
> >> @@ -214,6 +214,13 @@ config DRM_GEM_SHMEM_HELPER
> >> help
> >> Choose this if you need the GEM shmem helper functions
> >>
> >> +config DRM_GEM_SHMEM_KMS_HELPER
> >> + bool
> >> + depends on DRM_GEM_SHMEM_HELPER
> >> + help
> >> + help
> >> + Choose this if you need the GEM SHMEM helper functions for KMS
> >
> > I think we should just stuff this into simple pipe helpers directly. Also
> > needs some kerneldoc and ideally intro section of some sorts, but I guess
> > that was just missing for RFC.
>
> The missing docs is why it's an RFC. i was concerned about the
> additional simple-pipe callbacks, but I'm glad you're OK with them. I
> thought about simple_pipe state (as you mentioned below) or
> drm_private_state, but found it all too complex for this simple problem.
Yeah I think for this it's overkill, and it shouldn't be hard to add a
simple_pipe_state later on. Imo once you need a private state your
driver has proably outgrown simple pipe helpers.
> > Thinking a bit bigger and looking at the first patch, I wonder whether we
> > shouldn't outright integrate this simple pipe helpers. Adding all the
> > hooks for plane_state (instead of a simple_pipe_state or something like
> > that) feels a bit icky to me. But on the driver side the integration with
> > just the single macro line is very neat, so that's redeeming feature.
>
> I do disagree with integrating the shadow-plane code into simple-pipe.
> Right now it's SHMEM-depended and CMA-based simple-pipe drivers would
> probably not want to depend on this. The other reason is that I can
> certainly generalize the shadow planes away from SHMEM helpers; and use
> them for these vbox and ast patchsets as well. These drivers use VRAM
> helpers and full KMS helpers. I guess shadow planes could then be moved
> into drm_gem_framebuffer.c. There's other plane-helper code there already.
>
> Unfortunately, I only realized this after sending out the patch set. :)
Yeah I also noticed we have the current simple gem fb helper in there
already, so maybe just stuff it in there. Whether the simple glue for
shme/gem/cma/whatever integration is in drm_simple_pipe.c or
drm_gem_framebuffer.c kinda doesn't matter much. Just yet another
library felt a bit like overkill.
Cheers, Daniel
>
> Best regards
> Thomas
>
> >
> > Note doing a drm_simple_display_pipe_state would be a bit tricky to do,
> > since we'd need custome duplicate_state functions to make sure there's
> > only ever exactly one:
> >
> > struct drm_simple_display_pipe_state {
> > struct drm_crtc_state crtc_state;
> > struct drm_plane_state plane_state;
> >
> > struct dma_buf_map map[4];
> > };
> >
> > But that's a bit a bigger step, maybe when we also need to subclass crtc
> > stuff we can look into this. Or maybe that's then too much feature creep,
> > dunno. Implemenation shouldn't be too tricky:
> > - crtc state just duplicates itself (including plane_state duplication).
> > In release it also cleans up the plane state.
> > - plane state grabs the crtc state instead, and then does a cast. destroy
> > hook does nothing (to avoid touching freed memory, since we always dupe
> > the crtc state when the plane state exists we know the crtc destroy hook
> > will clean things up).
> >
> > That means drm_atomic_state has 2 pointers pointing into the same
> > allocation, but that should be all fine.
> >
> > Aside from this bikeshed here pondering a bit how this best first into our
> > simple pipe helpers I think this all looks very clean.
> > -Daniel
> >
> >> +
> >> config DRM_SCHED
> >> tristate
> >> depends on DRM
> >> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> >> index 926adef289db..37a73dee5baf 100644
> >> --- a/drivers/gpu/drm/Makefile
> >> +++ b/drivers/gpu/drm/Makefile
> >> @@ -53,6 +53,7 @@ drm_kms_helper-$(CONFIG_DRM_FBDEV_EMULATION) += drm_fb_helper.o
> >> drm_kms_helper-$(CONFIG_DRM_KMS_CMA_HELPER) += drm_fb_cma_helper.o
> >> drm_kms_helper-$(CONFIG_DRM_DP_AUX_CHARDEV) += drm_dp_aux_dev.o
> >> drm_kms_helper-$(CONFIG_DRM_DP_CEC) += drm_dp_cec.o
> >> +drm_kms_helper-$(CONFIG_DRM_GEM_SHMEM_KMS_HELPER) += drm_gem_shmem_kms_helper.o
> >>
> >> obj-$(CONFIG_DRM_KMS_HELPER) += drm_kms_helper.o
> >> obj-$(CONFIG_DRM_DEBUG_SELFTEST) += selftests/
> >> diff --git a/drivers/gpu/drm/drm_gem_shmem_kms_helper.c b/drivers/gpu/drm/drm_gem_shmem_kms_helper.c
> >> new file mode 100644
> >> index 000000000000..8843c5837f98
> >> --- /dev/null
> >> +++ b/drivers/gpu/drm/drm_gem_shmem_kms_helper.c
> >> @@ -0,0 +1,159 @@
> >> +// SPDX-License-Identifier: GPL-2.0
> >> +
> >> +#include <drm/drm_atomic_state_helper.h>
> >> +#include <drm/drm_gem_framebuffer_helper.h>
> >> +#include <drm/drm_gem_shmem_helper.h>
> >> +#include <drm/drm_gem_shmem_kms_helper.h>
> >> +#include <drm/drm_simple_kms_helper.h>
> >> +
> >> +/*
> >> + * Helpers for struct drm_plane_funcs
> >> + *
> >> + */
> >> +
> >> +static struct drm_plane_state *
> >> +drm_gem_shmem_duplicate_shadow_plane_state(struct drm_plane *plane,
> >> + struct drm_plane_state *plane_state)
> >> +{
> >> + struct drm_gem_shmem_shadow_plane_state *new_shadow_plane_state;
> >> +
> >> + if (!plane_state)
> >> + return NULL;
> >> +
> >> + 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);
> >> +
> >> + return &new_shadow_plane_state->base;
> >> +}
> >> +
> >> +static void drm_gem_shmem_destroy_shadow_plane_state(struct drm_plane *plane,
> >> + struct drm_plane_state *plane_state)
> >> +{
> >> + struct drm_gem_shmem_shadow_plane_state *shadow_plane_state =
> >> + to_drm_gem_shmem_shadow_plane_state(plane_state);
> >> +
> >> + __drm_atomic_helper_plane_destroy_state(&shadow_plane_state->base);
> >> + kfree(shadow_plane_state);
> >> +}
> >> +
> >> +static void drm_gem_shmem_reset_shadow_plane(struct drm_plane *plane)
> >> +{
> >> + struct drm_gem_shmem_shadow_plane_state *shadow_plane_state;
> >> +
> >> + if (plane->state) {
> >> + drm_gem_shmem_destroy_shadow_plane_state(plane, plane->state);
> >> + plane->state = NULL; /* must be set to NULL here */
> >> + }
> >> +
> >> + 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);
> >> +}
> >> +
> >> +/*
> >> + * Helpers for struct drm_plane_helper_funcs
> >> + */
> >> +
> >> +static int drm_gem_shmem_prepare_shadow_fb(struct drm_plane *plane,
> >> + struct drm_plane_state *plane_state)
> >> +{
> >> + struct drm_gem_shmem_shadow_plane_state *shadow_plane_state =
> >> + to_drm_gem_shmem_shadow_plane_state(plane_state);
> >> + struct drm_framebuffer *fb = plane_state->fb;
> >> + struct drm_gem_object *obj;
> >> + struct dma_buf_map map;
> >> + int ret;
> >> + size_t i;
> >> +
> >> + if (!fb)
> >> + return 0;
> >> +
> >> + ret = drm_gem_fb_prepare_fb(plane, plane_state);
> >> + if (ret)
> >> + return ret;
> >> +
> >> + for (i = 0; i < ARRAY_SIZE(shadow_plane_state->map); ++i) {
> >> + obj = drm_gem_fb_get_obj(fb, i);
> >> + if (!obj)
> >> + continue;
> >> + ret = drm_gem_shmem_vmap(obj, &map);
> >> + if (ret)
> >> + goto err_drm_gem_shmem_vunmap;
> >> + shadow_plane_state->map[i] = map;
> >> + }
> >> +
> >> + return 0;
> >> +
> >> +err_drm_gem_shmem_vunmap:
> >> + while (i) {
> >> + --i;
> >> + obj = drm_gem_fb_get_obj(fb, i);
> >> + if (!obj)
> >> + continue;
> >> + drm_gem_shmem_vunmap(obj, &shadow_plane_state->map[i]);
> >> + }
> >> + return ret;
> >> +}
> >> +
> >> +static void drm_gem_shmem_cleanup_shadow_fb(struct drm_plane *plane,
> >> + struct drm_plane_state *plane_state)
> >> +{
> >> + struct drm_gem_shmem_shadow_plane_state *shadow_plane_state =
> >> + to_drm_gem_shmem_shadow_plane_state(plane_state);
> >> + struct drm_framebuffer *fb = plane_state->fb;
> >> + size_t i = ARRAY_SIZE(shadow_plane_state->map);
> >> + struct drm_gem_object *obj;
> >> +
> >> + if (!fb)
> >> + return;
> >> +
> >> + while (i) {
> >> + --i;
> >> + obj = drm_gem_fb_get_obj(fb, i);
> >> + if (!obj)
> >> + continue;
> >> + drm_gem_shmem_vunmap(obj, &shadow_plane_state->map[i]);
> >> + }
> >> +}
> >> +
> >> +/*
> >> + * Simple KMS helpers
> >> + */
> >> +
> >> +int drm_gem_shmem_simple_kms_prepare_shadow_fb(struct drm_simple_display_pipe *pipe,
> >> + struct drm_plane_state *plane_state)
> >> +{
> >> + return drm_gem_shmem_prepare_shadow_fb(&pipe->plane, plane_state);
> >> +}
> >> +EXPORT_SYMBOL(drm_gem_shmem_simple_kms_prepare_shadow_fb);
> >> +
> >> +void drm_gem_shmem_simple_kms_cleanup_shadow_fb(struct drm_simple_display_pipe *pipe,
> >> + struct drm_plane_state *plane_state)
> >> +{
> >> + drm_gem_shmem_cleanup_shadow_fb(&pipe->plane, plane_state);
> >> +}
> >> +EXPORT_SYMBOL(drm_gem_shmem_simple_kms_cleanup_shadow_fb);
> >> +
> >> +void drm_gem_shmem_simple_kms_reset_shadow_plane(struct drm_simple_display_pipe *pipe)
> >> +{
> >> + drm_gem_shmem_reset_shadow_plane(&pipe->plane);
> >> +}
> >> +EXPORT_SYMBOL(drm_gem_shmem_simple_kms_reset_shadow_plane);
> >> +
> >> +struct drm_plane_state *
> >> +drm_gem_shmem_simple_kms_duplicate_shadow_plane_state(struct drm_simple_display_pipe *pipe,
> >> + struct drm_plane_state *plane_state)
> >> +{
> >> + return drm_gem_shmem_duplicate_shadow_plane_state(&pipe->plane, plane_state);
> >> +}
> >> +EXPORT_SYMBOL(drm_gem_shmem_simple_kms_duplicate_shadow_plane_state);
> >> +
> >> +void drm_gem_shmem_simple_kms_destroy_shadow_plane_state(struct drm_simple_display_pipe *pipe,
> >> + struct drm_plane_state *plane_state)
> >> +{
> >> + drm_gem_shmem_destroy_shadow_plane_state(&pipe->plane, plane_state);
> >> +}
> >> +EXPORT_SYMBOL(drm_gem_shmem_simple_kms_destroy_shadow_plane_state);
> >> diff --git a/include/drm/drm_gem_shmem_kms_helper.h b/include/drm/drm_gem_shmem_kms_helper.h
> >> new file mode 100644
> >> index 000000000000..bd42c9c0a39e
> >> --- /dev/null
> >> +++ b/include/drm/drm_gem_shmem_kms_helper.h
> >> @@ -0,0 +1,56 @@
> >> +/* SPDX-License-Identifier: GPL-2.0 */
> >> +
> >> +#ifndef __DRM_GEM_SHMEM_KMS_HELPER_H__
> >> +#define __DRM_GEM_SHMEM_KMS_HELPER_H__
> >> +
> >> +#include <linux/dma-buf-map.h>
> >> +
> >> +#include <drm/drm_plane.h>
> >> +
> >> +struct drm_simple_display_pipe;
> >> +
> >> +struct drm_gem_shmem_shadow_plane_state {
> >> + struct drm_plane_state base;
> >> +
> >> + /* Transitional state - do not export or duplicate */
> >> +
> >> + struct dma_buf_map map[4];
> >> +};
> >> +
> >> +static inline struct drm_gem_shmem_shadow_plane_state *
> >> +to_drm_gem_shmem_shadow_plane_state(struct drm_plane_state *state)
> >> +{
> >> + return container_of(state, struct drm_gem_shmem_shadow_plane_state, base);
> >> +}
> >> +
> >> +/*
> >> + * Simple KMS helpers
> >> + */
> >> +
> >> +int drm_gem_shmem_simple_kms_prepare_shadow_fb(struct drm_simple_display_pipe *pipe,
> >> + struct drm_plane_state *plane_state);
> >> +void drm_gem_shmem_simple_kms_cleanup_shadow_fb(struct drm_simple_display_pipe *pipe,
> >> + struct drm_plane_state *plane_state);
> >> +void drm_gem_shmem_simple_kms_reset_shadow_plane(struct drm_simple_display_pipe *pipe);
> >> +struct drm_plane_state *
> >> +drm_gem_shmem_simple_kms_duplicate_shadow_plane_state(struct drm_simple_display_pipe *pipe,
> >> + struct drm_plane_state *plane_state);
> >> +void
> >> +drm_gem_shmem_simple_kms_destroy_shadow_plane_state(struct drm_simple_display_pipe *pipe,
> >> + struct drm_plane_state *plane_state);
> >> +
> >> +/**
> >> + * DRM_GEM_SHMEM_SIMPLE_DISPLAY_PIPE_SHADOW_PLANE_FUNCS -
> >> + * Initializes struct drm_simple_display_pipe_funcs for SHMEM shadow planes
> >> + *
> >> + * Drivers may use GEM SHMEM BOs as shadow buffers over the framebuffer memory. This
> >> + * macro initializes struct drm_simple_display_pipe_funcs to use the rsp helper functions.
> >> + */
> >> +#define DRM_GEM_SHMEM_SIMPLE_DISPLAY_PIPE_SHADOW_PLANE_FUNCS \
> >> + .prepare_fb = drm_gem_shmem_simple_kms_prepare_shadow_fb, \
> >> + .cleanup_fb = drm_gem_shmem_simple_kms_cleanup_shadow_fb, \
> >> + .reset_plane = drm_gem_shmem_simple_kms_reset_shadow_plane, \
> >> + .duplicate_plane_state = drm_gem_shmem_simple_kms_duplicate_shadow_plane_state, \
> >> + .destroy_plane_state = drm_gem_shmem_simple_kms_destroy_shadow_plane_state
> >> +
> >> +#endif /* __DRM_GEM_SHMEM_KMS_HELPER_H__ */
> >> --
> >> 2.30.0
> >>
> >
>
> --
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Software Solutions Germany GmbH
> Maxfeldstr. 5, 90409 Nürnberg, Germany
> (HRB 36809, AG Nürnberg)
> Geschäftsführer: Felix Imendörffer
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Virtualization mailing list
Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
