Message-ID: <Zrue4wWhKmMnJ2fR@louis-chauvet-laptop>
Date: Tue, 13 Aug 2024 19:58:59 +0200
From: Louis Chauvet <louis.chauvet@...tlin.com>
To: José Expósito <jose.exposito89@...il.com>
Cc: rodrigosiqueiramelo@...il.com, melissa.srw@...il.com,
	mairacanal@...eup.net, hamohammed.sa@...il.com, daniel@...ll.ch,
	maarten.lankhorst@...ux.intel.com, mripard@...nel.org,
	tzimmermann@...e.de, airlied@...il.com,
	dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH 11/17] drm/vkms: Add and remove VKMS instances via
 configfs

Le 13/08/24 - 12:44, José Expósito a écrit :
> Allow to create, enable, disable and destroy VKMS instances using
> configfs.
> 
> For the moment, add one primary plane, one CRC, one encoder and one
> connector until we add support to configure them via configfs.
> 
> Signed-off-by: José Expósito <jose.exposito89@...il.com>
> ---
>  Documentation/gpu/vkms.rst           |  32 +++++
>  drivers/gpu/drm/vkms/Kconfig         |   1 +
>  drivers/gpu/drm/vkms/Makefile        |   3 +-
>  drivers/gpu/drm/vkms/vkms_configfs.c | 198 +++++++++++++++++++++++++++
>  drivers/gpu/drm/vkms/vkms_configfs.h |   9 ++
>  drivers/gpu/drm/vkms/vkms_drv.c      |  19 ++-
>  drivers/gpu/drm/vkms/vkms_drv.h      |   4 +
>  7 files changed, 262 insertions(+), 4 deletions(-)
>  create mode 100644 drivers/gpu/drm/vkms/vkms_configfs.c
>  create mode 100644 drivers/gpu/drm/vkms/vkms_configfs.h
> 
> diff --git a/Documentation/gpu/vkms.rst b/Documentation/gpu/vkms.rst
> index ba04ac7c2167..9895a9ae76f4 100644
> --- a/Documentation/gpu/vkms.rst
> +++ b/Documentation/gpu/vkms.rst
> @@ -51,6 +51,38 @@ To disable the driver, use ::
>  
>    sudo modprobe -r vkms
>  
> +Configuring With Configfs
> +=========================
> +
> +It is possible to create and configure multiple VKMS instances via configfs.
> +
> +Start by mounting configfs and loading VKMS::
> +
> +  sudo mount -t configfs none /config
> +  sudo modprobe vkms
> +
> +Once VKMS is loaded, ``/config/vkms`` is created automatically. Each directory
> +under ``/config/vkms`` represents a VKMS instance, create a new one::
> +
> +  sudo mkdir /config/vkms/my-vkms
> +
> +By default, the instance is disabled::
> +
> +  cat /config/vkms/my-vkms/enabled
> +  0
> +
> +Once you are done configuring the VKMS instance, enable it::
> +
> +  echo "1" | sudo tee /config/vkms/my-vkms/enabled
> +
> +Finally, you can remove the VKMS instance disabling it::
> +
> +  echo "0" | sudo tee /config/vkms/my-vkms/enabled
> +
> +Or removing the top level directory::
> +
> +  sudo rmdir /config/vkms/my-vkms
> +

I will explain more in the next commit, but you must not be able to delete 
the device directory without disabling it first.

>  Testing With IGT
>  ================
>  
> diff --git a/drivers/gpu/drm/vkms/Kconfig b/drivers/gpu/drm/vkms/Kconfig
> index b9ecdebecb0b..de218b21a579 100644
> --- a/drivers/gpu/drm/vkms/Kconfig
> +++ b/drivers/gpu/drm/vkms/Kconfig
> @@ -6,6 +6,7 @@ config DRM_VKMS
>  	select DRM_KMS_HELPER
>  	select DRM_GEM_SHMEM_HELPER
>  	select CRC32
> +	select CONFIGFS_FS
>  	default n
>  	help
>  	  Virtual Kernel Mode-Setting (VKMS) is used for testing or for
> diff --git a/drivers/gpu/drm/vkms/Makefile b/drivers/gpu/drm/vkms/Makefile
> index b371b5d70ee3..81c257d37689 100644
> --- a/drivers/gpu/drm/vkms/Makefile
> +++ b/drivers/gpu/drm/vkms/Makefile
> @@ -7,6 +7,7 @@ vkms-y := \
>  	vkms_crtc.o \
>  	vkms_composer.o \
>  	vkms_writeback.o \
> -	vkms_config.o
> +	vkms_config.o \
> +	vkms_configfs.o
>  
>  obj-$(CONFIG_DRM_VKMS) += vkms.o
> diff --git a/drivers/gpu/drm/vkms/vkms_configfs.c b/drivers/gpu/drm/vkms/vkms_configfs.c
> new file mode 100644
> index 000000000000..3f25295f7788
> --- /dev/null
> +++ b/drivers/gpu/drm/vkms/vkms_configfs.c
> @@ -0,0 +1,198 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +
> +#include <linux/configfs.h>
> +#include <linux/mutex.h>
> +#include <linux/slab.h>
> +
> +#include "vkms_drv.h"
> +#include "vkms_config.h"
> +#include "vkms_configfs.h"
> +
> +/* To avoid registering configfs more than once or unregistering on error */
> +static bool is_configfs_registered;
> +
> +/**
> + * struct vkms_configfs - Configfs configuration for a VKMS device
> + *
> + * @vkms_config: Configuration of the VKMS device
> + * @device_group: Top level configuration group that represents a VKMS device.
> + * Initialized when a new directory is created under "/config/vkms/"
> + * @lock: Lock used to project concurrent access to the configuration attributes
> + * @enabled: Protected by @lock. The device is created or destroyed when this
> + * option changes
> + */
> +struct vkms_configfs {
> +	struct vkms_config *vkms_config;
> +	struct config_group device_group;
> +
> +	/* protected by @lock */
> +	struct mutex lock;
> +	bool enabled;
> +};
> +
> +#define config_item_to_vkms_configfs(item) \
> +	container_of(to_config_group(item), struct vkms_configfs, device_group)
> +
> +static ssize_t device_enabled_show(struct config_item *item, char *page)
> +{
> +	struct vkms_configfs *configfs = config_item_to_vkms_configfs(item);
> +
> +	return sprintf(page, "%d\n", configfs->enabled);
> +}
> +
> +static ssize_t device_enabled_store(struct config_item *item, const char *page,
> +				    size_t count)
> +{
> +	struct vkms_configfs *configfs = config_item_to_vkms_configfs(item);
> +	bool enabled;
> +	int ret = 0;
> +
> +	if (kstrtobool(page, &enabled))
> +		return -EINVAL;
> +
> +	mutex_lock(&configfs->lock);
> +
> +	if (!configfs->enabled && enabled)
> +		ret = vkms_create(configfs->vkms_config);
> +	else if (configfs->enabled && !enabled)
> +		vkms_destroy(configfs->vkms_config);
> +
> +	if (ret)
> +		goto err_unlock;
> +
> +	configfs->enabled = enabled;
> +
> +	mutex_unlock(&configfs->lock);
> +
> +	return (ssize_t)count;
> +
> +err_unlock:
> +	mutex_unlock(&configfs->lock);
> +	return ret;
> +}
> +
> +CONFIGFS_ATTR(device_, enabled);
> +
> +static struct configfs_attribute *device_group_attrs[] = {
> +	&device_attr_enabled,
> +	NULL,
> +};
> +
> +static const struct config_item_type device_group_type = {
> +	.ct_attrs = device_group_attrs,
> +	.ct_owner = THIS_MODULE,
> +};
> +
> +static struct config_group *make_device_group(struct config_group *group,
> +					      const char *name)
> +{
> +	struct vkms_configfs *configfs;
> +	struct vkms_config_crtc *crtc_cfg = NULL;
> +	struct vkms_config_encoder *encoder_cfg = NULL;
> +	struct vkms_config_connector *connector_cfg = NULL;
> +	char *config_name;
> +	int ret;
> +
> +	if (strcmp(name, DEFAULT_DEVICE_NAME) == 0)
> +		return ERR_PTR(-EINVAL);
> +
> +	configfs = kzalloc(sizeof(*configfs), GFP_KERNEL);
> +	if (!configfs)
> +		return ERR_PTR(-ENOMEM);
> +
> +	config_group_init_type_name(&configfs->device_group, name, &device_group_type);
> +	mutex_init(&configfs->lock);
> +
> +	config_name = config_item_name(&configfs->device_group.cg_item);
> +	configfs->vkms_config = vkms_config_create(config_name);

Here, config_name is managed by configfs, but you use it in a vkms_config 
structure. I don't think this is a good idea, I would prefer to avoid 
mixing lifetimes.

> +	if (IS_ERR(configfs->vkms_config)) {
> +		ret = PTR_ERR(configfs->vkms_config);
> +		goto err_kfree;
> +	}
> +
> +	crtc_cfg = vkms_config_add_crtc(configfs->vkms_config, false, false);
> +	if (IS_ERR(crtc_cfg)) {
> +		ret = PTR_ERR(crtc_cfg);
> +		goto err_kfree;
> +	}
> +
> +	encoder_cfg = vkms_config_add_encoder(configfs->vkms_config, BIT(0));
> +	if (IS_ERR(encoder_cfg)) {
> +		ret = PTR_ERR(encoder_cfg);
> +		goto err_kfree;
> +	}
> +
> +	connector_cfg = vkms_config_add_connector(configfs->vkms_config, BIT(0),
> +						  connector_status_connected);
> +	if (IS_ERR(connector_cfg)) {
> +		ret = PTR_ERR(connector_cfg);
> +		goto err_kfree;
> +	}
> +
> +	return &configfs->device_group;
> +
> +err_kfree:
> +	kfree(configfs);
> +	kfree(crtc_cfg);
> +	kfree(encoder_cfg);
> +	kfree(connector_cfg);

You are not freeing configfs->vkms_config. And it will automatically free 
*_cfg for you ;)

> +	return ERR_PTR(ret);
> +}
> +
> +static void drop_device_group(struct config_group *group,
> +			      struct config_item *item)
> +{
> +	struct vkms_configfs *configfs = config_item_to_vkms_configfs(item);
> +
> +	mutex_lock(&configfs->lock);
> +
> +	if (configfs->enabled)
> +		vkms_destroy(configfs->vkms_config);
> +
> +	kfree(configfs->vkms_config);

You are not freeing stuff added with vkms_config_add_*

> +
> +	mutex_unlock(&configfs->lock);
> +
> +	kfree(configfs);
> +}

If I understood well the configfs documentation, the free of memory should 
not occur in drop, but in release.

Drop should only call configfs_group_put (default behavior) and configfs 
will manage the call to release for you. The same will apply for all the 
next commits.

> +static struct configfs_group_operations device_group_ops = {
> +	.make_group = &make_device_group,
> +	.drop_item = &drop_device_group,
> +};
> +
> +static struct config_item_type vkms_type = {
> +	.ct_group_ops = &device_group_ops,
> +	.ct_owner = THIS_MODULE,
> +};
> +
> +static struct configfs_subsystem vkms_subsys = {
> +	.su_group = {
> +		.cg_item = {
> +			.ci_name = "vkms",
> +			.ci_type = &vkms_type,
> +		},
> +	},
> +	.su_mutex = __MUTEX_INITIALIZER(vkms_subsys.su_mutex),
> +};
> +
> +int vkms_configfs_register(void)
> +{
> +	int ret;
> +
> +	if (is_configfs_registered)
> +		return 0;
> +
> +	config_group_init(&vkms_subsys.su_group);
> +	ret = configfs_register_subsystem(&vkms_subsys);
> +
> +	is_configfs_registered = ret == 0;
> +
> +	return ret;
> +}
> +
> +void vkms_configfs_unregister(void)
> +{
> +	if (is_configfs_registered)
> +		configfs_unregister_subsystem(&vkms_subsys);

I don't see any other users of configfs_unregister_subsystem protecting 
the call. Is it really needed to have is_configfs_registered?

> +}
> diff --git a/drivers/gpu/drm/vkms/vkms_configfs.h b/drivers/gpu/drm/vkms/vkms_configfs.h
> new file mode 100644
> index 000000000000..b5d7fab877d5
> --- /dev/null
> +++ b/drivers/gpu/drm/vkms/vkms_configfs.h
> @@ -0,0 +1,9 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +
> +#ifndef _VKMS_CONFIGFS_H_
> +#define _VKMS_CONFIGFS_H_
> +
> +int vkms_configfs_register(void);
> +void vkms_configfs_unregister(void);
> +
> +#endif /* _VKMS_CONFIGFS_H_ */
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
> index b0a079eb4598..f524a9550017 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.c
> +++ b/drivers/gpu/drm/vkms/vkms_drv.c
> @@ -27,6 +27,7 @@
>  #include <drm/drm_vblank.h>
>  
>  #include "vkms_config.h"
> +#include "vkms_configfs.h"
>  #include "vkms_drv.h"
>  
>  #define DRIVER_NAME	"vkms"
> @@ -157,7 +158,7 @@ static int vkms_modeset_init(struct vkms_device *vkmsdev)
>  	return vkms_output_init(vkmsdev);
>  }
>  
> -static int vkms_create(struct vkms_config *config)
> +int vkms_create(struct vkms_config *config)
>  {
>  	int ret;
>  	struct platform_device *pdev;
> @@ -231,12 +232,22 @@ static int __init vkms_init(void)
>  
>  	ret = vkms_create(config);
>  	if (ret)
> -		vkms_config_destroy(config);
> +		goto err_kfree;
>  
> +	ret = vkms_configfs_register();
> +	if (ret)
> +		goto err_destroy;
> +
> +	return 0;
> +
> +err_destroy:
> +	vkms_destroy(config);
> +err_kfree:
> +	vkms_config_destroy(config);
>  	return ret;
>  }
>  
> -static void vkms_destroy(struct vkms_config *config)
> +void vkms_destroy(struct vkms_config *config)
>  {
>  	struct platform_device *pdev;
>  
> @@ -257,6 +268,8 @@ static void vkms_destroy(struct vkms_config *config)
>  
>  static void __exit vkms_exit(void)
>  {
> +	vkms_configfs_unregister();
> +
>  	if (default_config->dev)
>  		vkms_destroy(default_config);
>  
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
> index 76394285dc68..eb32440e9f94 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.h
> +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> @@ -143,6 +143,10 @@ struct vkms_device {
>  #define to_vkms_plane_state(target)\
>  	container_of(target, struct vkms_plane_state, base.base)
>  
> +/* VKMS device */
> +int vkms_create(struct vkms_config *config);
> +void vkms_destroy(struct vkms_config *config);
>
>  /* CRTC */
>  struct vkms_crtc *vkms_crtc_init(struct drm_device *dev,
>  				 struct drm_plane *primary,
> -- 
> 2.46.0
> 

-- 
Louis Chauvet, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Powered by blists - more mailing lists