Re: [PATCH 2/6] drm/shmem-helper: Add additional KMS helpers

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [KVM Development]     [Libvirt Development]     [Libvirt Users]     [CentOS Virtualization]     [Netdev]     [Ethernet Bridging]     [Linux Wireless]     [Kernel Newbies]     [Security]     [Linux for Hams]     [Netfilter]     [Bugtraq]     [Yosemite Forum]     [MIPS Linux]     [ARM Linux]     [Linux RAID]     [Linux Admin]     [Samba]

  Powered by Linux