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

[Thomas Zimmermann <tzimmermann@suse.de>]

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

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@suse.de>
>> ---
>>   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.

> 
> 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. :)

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


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

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

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization