diff mbox series

[v2,1/6] drm/vkms: Back VKMS with DRM memory management instead of static objects

Message ID 20230623222353.97283-2-jshargo@chromium.org (mailing list archive)
State New, archived
Headers show
Series Adds support for ConfigFS to VKMS! | expand

Commit Message

Jim Shargo June 23, 2023, 10:23 p.m. UTC 
This is a small refactor to make ConfigFS support easier. Once we
support ConfigFS, there can be multiple devices instantiated by the
driver, and so moving everything into managed memory makes things much
easier.

This should be a no-op refactor.

Signed-off-by: Jim Shargo <jshargo@chromium.org>
---
 drivers/gpu/drm/vkms/vkms_drv.c    | 130 +++++++++++++++--------------
 drivers/gpu/drm/vkms/vkms_drv.h    |   4 +-
 drivers/gpu/drm/vkms/vkms_output.c |   6 +-
 3 files changed, 72 insertions(+), 68 deletions(-)

Comments

Maíra Canal June 25, 2023, 5:40 p.m. UTC | #1 
Hi Jim,

Thanks for working on this great feature for the VKMS!

On 6/23/23 19:23, Jim Shargo wrote:
> This is a small refactor to make ConfigFS support easier. Once we
> support ConfigFS, there can be multiple devices instantiated by the
> driver, and so moving everything into managed memory makes things much
> easier.
> 
> This should be a no-op refactor.
> 
> Signed-off-by: Jim Shargo <jshargo@chromium.org>
> ---
>   drivers/gpu/drm/vkms/vkms_drv.c    | 130 +++++++++++++++--------------
>   drivers/gpu/drm/vkms/vkms_drv.h    |   4 +-
>   drivers/gpu/drm/vkms/vkms_output.c |   6 +-
>   3 files changed, 72 insertions(+), 68 deletions(-)
> 
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
> index e3c9c9571c8d..f07454fff046 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.c
> +++ b/drivers/gpu/drm/vkms/vkms_drv.c
> @@ -9,10 +9,12 @@
>    * the GPU in DRM API tests.
>    */
>   
> +#include <linux/device.h>
>   #include <linux/module.h>
>   #include <linux/platform_device.h>
>   #include <linux/dma-mapping.h>
>   
> +#include <drm/drm_device.h>
>   #include <drm/drm_gem.h>
>   #include <drm/drm_atomic.h>
>   #include <drm/drm_atomic_helper.h>
> @@ -37,8 +39,6 @@
>   #define DRIVER_MAJOR	1
>   #define DRIVER_MINOR	0
>   
> -static struct vkms_config *default_config;
> -
>   static bool enable_cursor = true;
>   module_param_named(enable_cursor, enable_cursor, bool, 0444);
>   MODULE_PARM_DESC(enable_cursor, "Enable/Disable cursor support");
> @@ -92,13 +92,13 @@ static void vkms_atomic_commit_tail(struct drm_atomic_state *old_state)
>   
>   static int vkms_config_show(struct seq_file *m, void *data)
>   {
> -	struct drm_debugfs_entry *entry = m->private;
> -	struct drm_device *dev = entry->dev;
> -	struct vkms_device *vkmsdev = drm_device_to_vkms_device(dev);
> +	struct drm_info_node *drm_info = m->private;

The VKMS already moved to the new DRM debugfs interface, there is no
need to move back to the old interface. I believe you should keep
using struct drm_debugfs_entry.

Also, that's probably the reason why the vkms_config file is not being
properly updated:

[root@fedora ~]# modprobe vkms enable_overlay=1 enable_cursor=1 
enable_writeback=1
[root@fedora ~]# cd /sys/kernel/debug/dri/0
[root@fedora 0]# cat vkms_config
writeback=0
cursor=0
overlay=0

> +	struct vkms_device *vkms_device =
> +		drm_device_to_vkms_device(drm_info->minor->dev);
>   
> -	seq_printf(m, "writeback=%d\n", vkmsdev->config->writeback);
> -	seq_printf(m, "cursor=%d\n", vkmsdev->config->cursor);
> -	seq_printf(m, "overlay=%d\n", vkmsdev->config->overlay);
> +	seq_printf(m, "writeback=%d\n", vkms_device->config.writeback);
> +	seq_printf(m, "cursor=%d\n", vkms_device->config.cursor);
> +	seq_printf(m, "overlay=%d\n", vkms_device->config.overlay);
>   
>   	return 0;
>   }
> @@ -155,114 +155,120 @@ static int vkms_modeset_init(struct vkms_device *vkmsdev)
>   	return vkms_output_init(vkmsdev, 0);
>   }
>   
> -static int vkms_create(struct vkms_config *config)
> +static int vkms_platform_probe(struct platform_device *pdev)
>   {
>   	int ret;
> -	struct platform_device *pdev;
>   	struct vkms_device *vkms_device;
> +	void *grp;
>   
> -	pdev = platform_device_register_simple(DRIVER_NAME, -1, NULL, 0);
> -	if (IS_ERR(pdev))
> -		return PTR_ERR(pdev);
> -
> -	if (!devres_open_group(&pdev->dev, NULL, GFP_KERNEL)) {
> -		ret = -ENOMEM;
> -		goto out_unregister;
> +	grp = devres_open_group(&pdev->dev, NULL, GFP_KERNEL);
> +	if (!grp) {
> +		return -ENOMEM;
>   	}
>   
>   	vkms_device = devm_drm_dev_alloc(&pdev->dev, &vkms_driver,
>   					 struct vkms_device, drm);
>   	if (IS_ERR(vkms_device)) {
>   		ret = PTR_ERR(vkms_device);
> -		goto out_devres;
> +		goto out_release_group;
>   	}
> +
>   	vkms_device->platform = pdev;
> -	vkms_device->config = config;
> -	config->dev = vkms_device;
> +	vkms_device->config.cursor = enable_cursor;
> +	vkms_device->config.writeback = enable_writeback;
> +	vkms_device->config.overlay = enable_overlay;
>   
>   	ret = dma_coerce_mask_and_coherent(vkms_device->drm.dev,
>   					   DMA_BIT_MASK(64));
> -
>   	if (ret) {
>   		DRM_ERROR("Could not initialize DMA support\n");
> -		goto out_devres;
> +		goto out_release_group;
>   	}
>   
>   	ret = drm_vblank_init(&vkms_device->drm, 1);
>   	if (ret) {
>   		DRM_ERROR("Failed to vblank\n");
> -		goto out_devres;
> +		goto out_release_group;
>   	}
>   
>   	ret = vkms_modeset_init(vkms_device);
> -	if (ret)
> -		goto out_devres;
> +	if (ret) {
> +		DRM_ERROR("Unable to initialize modesetting\n");
> +		goto out_release_group;
> +	}
>   
>   	drm_debugfs_add_files(&vkms_device->drm, vkms_config_debugfs_list,
>   			      ARRAY_SIZE(vkms_config_debugfs_list));
>   
>   	ret = drm_dev_register(&vkms_device->drm, 0);
> -	if (ret)
> -		goto out_devres;
> +	if (ret) {
> +		DRM_ERROR("Unable to register device\n");
> +		return ret;

Shouldn't you go to out_release_group inside of just returning?

Best Regards,
- Maíra

> +	}
>   
>   	drm_fbdev_generic_setup(&vkms_device->drm, 0);
> +	platform_set_drvdata(pdev, vkms_device);
> +	devres_close_group(&pdev->dev, grp);
>   
>   	return 0;
>   
> -out_devres:
> -	devres_release_group(&pdev->dev, NULL);
> -out_unregister:
> -	platform_device_unregister(pdev);
> +out_release_group:
> +	devres_release_group(&pdev->dev, grp);
>   	return ret;
>   }
>   
> -static int __init vkms_init(void)
> +static int vkms_platform_remove(struct platform_device *pdev)
>   {
> -	int ret;
> -	struct vkms_config *config;
> -
> -	config = kmalloc(sizeof(*config), GFP_KERNEL);
> -	if (!config)
> -		return -ENOMEM;
> -
> -	default_config = config;
> +	struct vkms_device *vkms_device;
>   
> -	config->cursor = enable_cursor;
> -	config->writeback = enable_writeback;
> -	config->overlay = enable_overlay;
> +	vkms_device = platform_get_drvdata(pdev);
> +	if (!vkms_device)
> +		return 0;
>   
> -	ret = vkms_create(config);
> -	if (ret)
> -		kfree(config);
> -
> -	return ret;
> +	drm_dev_unregister(&vkms_device->drm);
> +	drm_atomic_helper_shutdown(&vkms_device->drm);
> +	return 0;
>   }
>   
> -static void vkms_destroy(struct vkms_config *config)
> +static struct platform_driver vkms_platform_driver = {
> +	.probe = vkms_platform_probe,
> +	.remove = vkms_platform_remove,
> +	.driver.name = DRIVER_NAME,
> +};
> +
> +static int __init vkms_init(void)
>   {
> +	int ret;
>   	struct platform_device *pdev;
>   
> -	if (!config->dev) {
> -		DRM_INFO("vkms_device is NULL.\n");
> -		return;
> +	ret = platform_driver_register(&vkms_platform_driver);
> +	if (ret) {
> +		DRM_ERROR("Unable to register platform driver\n");
> +		return ret;
>   	}
>   
> -	pdev = config->dev->platform;
> -
> -	drm_dev_unregister(&config->dev->drm);
> -	drm_atomic_helper_shutdown(&config->dev->drm);
> -	devres_release_group(&pdev->dev, NULL);
> -	platform_device_unregister(pdev);
> +	pdev = platform_device_register_simple(DRIVER_NAME, -1, NULL, 0);
> +	if (IS_ERR(pdev)) {
> +		platform_driver_unregister(&vkms_platform_driver);
> +		return PTR_ERR(pdev);
> +	}
>   
> -	config->dev = NULL;
> +	return 0;
>   }
>   
>   static void __exit vkms_exit(void)
>   {
> -	if (default_config->dev)
> -		vkms_destroy(default_config);
> +	struct device *dev;
> +
> +	while ((dev = platform_find_device_by_driver(
> +			NULL, &vkms_platform_driver.driver))) {
> +		// platform_find_device_by_driver increments the refcount. Drop
> +		// it so we don't leak memory.
> +		put_device(dev);
> +		platform_device_unregister(to_platform_device(dev));
> +	}
>   
> -	kfree(default_config);
> +	platform_driver_unregister(&vkms_platform_driver);
>   }
>   
>   module_init(vkms_init);
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
> index 5f1a0a44a78c..e87c8aea6fb3 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.h
> +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> @@ -114,15 +114,13 @@ struct vkms_config {
>   	bool writeback;
>   	bool cursor;
>   	bool overlay;
> -	/* only set when instantiated */
> -	struct vkms_device *dev;
>   };
>   
>   struct vkms_device {
>   	struct drm_device drm;
>   	struct platform_device *platform;
>   	struct vkms_output output;
> -	const struct vkms_config *config;
> +	struct vkms_config config;
>   };
>   
>   #define drm_crtc_to_vkms_output(target) \
> diff --git a/drivers/gpu/drm/vkms/vkms_output.c b/drivers/gpu/drm/vkms/vkms_output.c
> index 5ce70dd946aa..963a64cf068b 100644
> --- a/drivers/gpu/drm/vkms/vkms_output.c
> +++ b/drivers/gpu/drm/vkms/vkms_output.c
> @@ -62,7 +62,7 @@ int vkms_output_init(struct vkms_device *vkmsdev, int index)
>   	if (IS_ERR(primary))
>   		return PTR_ERR(primary);
>   
> -	if (vkmsdev->config->overlay) {
> +	if (vkmsdev->config.overlay) {
>   		for (n = 0; n < NUM_OVERLAY_PLANES; n++) {
>   			ret = vkms_add_overlay_plane(vkmsdev, index, crtc);
>   			if (ret)
> @@ -70,7 +70,7 @@ int vkms_output_init(struct vkms_device *vkmsdev, int index)
>   		}
>   	}
>   
> -	if (vkmsdev->config->cursor) {
> +	if (vkmsdev->config.cursor) {
>   		cursor = vkms_plane_init(vkmsdev, DRM_PLANE_TYPE_CURSOR, index);
>   		if (IS_ERR(cursor))
>   			return PTR_ERR(cursor);
> @@ -103,7 +103,7 @@ int vkms_output_init(struct vkms_device *vkmsdev, int index)
>   		goto err_attach;
>   	}
>   
> -	if (vkmsdev->config->writeback) {
> +	if (vkmsdev->config.writeback) {
>   		writeback = vkms_enable_writeback_connector(vkmsdev);
>   		if (writeback)
>   			DRM_ERROR("Failed to init writeback connector\n");
diff mbox series

Patch

diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
index e3c9c9571c8d..f07454fff046 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.c
+++ b/drivers/gpu/drm/vkms/vkms_drv.c
@@ -9,10 +9,12 @@ 
  * the GPU in DRM API tests.
  */
 
+#include <linux/device.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
 #include <linux/dma-mapping.h>
 
+#include <drm/drm_device.h>
 #include <drm/drm_gem.h>
 #include <drm/drm_atomic.h>
 #include <drm/drm_atomic_helper.h>
@@ -37,8 +39,6 @@ 
 #define DRIVER_MAJOR	1
 #define DRIVER_MINOR	0
 
-static struct vkms_config *default_config;
-
 static bool enable_cursor = true;
 module_param_named(enable_cursor, enable_cursor, bool, 0444);
 MODULE_PARM_DESC(enable_cursor, "Enable/Disable cursor support");
@@ -92,13 +92,13 @@  static void vkms_atomic_commit_tail(struct drm_atomic_state *old_state)
 
 static int vkms_config_show(struct seq_file *m, void *data)
 {
-	struct drm_debugfs_entry *entry = m->private;
-	struct drm_device *dev = entry->dev;
-	struct vkms_device *vkmsdev = drm_device_to_vkms_device(dev);
+	struct drm_info_node *drm_info = m->private;
+	struct vkms_device *vkms_device =
+		drm_device_to_vkms_device(drm_info->minor->dev);
 
-	seq_printf(m, "writeback=%d\n", vkmsdev->config->writeback);
-	seq_printf(m, "cursor=%d\n", vkmsdev->config->cursor);
-	seq_printf(m, "overlay=%d\n", vkmsdev->config->overlay);
+	seq_printf(m, "writeback=%d\n", vkms_device->config.writeback);
+	seq_printf(m, "cursor=%d\n", vkms_device->config.cursor);
+	seq_printf(m, "overlay=%d\n", vkms_device->config.overlay);
 
 	return 0;
 }
@@ -155,114 +155,120 @@  static int vkms_modeset_init(struct vkms_device *vkmsdev)
 	return vkms_output_init(vkmsdev, 0);
 }
 
-static int vkms_create(struct vkms_config *config)
+static int vkms_platform_probe(struct platform_device *pdev)
 {
 	int ret;
-	struct platform_device *pdev;
 	struct vkms_device *vkms_device;
+	void *grp;
 
-	pdev = platform_device_register_simple(DRIVER_NAME, -1, NULL, 0);
-	if (IS_ERR(pdev))
-		return PTR_ERR(pdev);
-
-	if (!devres_open_group(&pdev->dev, NULL, GFP_KERNEL)) {
-		ret = -ENOMEM;
-		goto out_unregister;
+	grp = devres_open_group(&pdev->dev, NULL, GFP_KERNEL);
+	if (!grp) {
+		return -ENOMEM;
 	}
 
 	vkms_device = devm_drm_dev_alloc(&pdev->dev, &vkms_driver,
 					 struct vkms_device, drm);
 	if (IS_ERR(vkms_device)) {
 		ret = PTR_ERR(vkms_device);
-		goto out_devres;
+		goto out_release_group;
 	}
+
 	vkms_device->platform = pdev;
-	vkms_device->config = config;
-	config->dev = vkms_device;
+	vkms_device->config.cursor = enable_cursor;
+	vkms_device->config.writeback = enable_writeback;
+	vkms_device->config.overlay = enable_overlay;
 
 	ret = dma_coerce_mask_and_coherent(vkms_device->drm.dev,
 					   DMA_BIT_MASK(64));
-
 	if (ret) {
 		DRM_ERROR("Could not initialize DMA support\n");
-		goto out_devres;
+		goto out_release_group;
 	}
 
 	ret = drm_vblank_init(&vkms_device->drm, 1);
 	if (ret) {
 		DRM_ERROR("Failed to vblank\n");
-		goto out_devres;
+		goto out_release_group;
 	}
 
 	ret = vkms_modeset_init(vkms_device);
-	if (ret)
-		goto out_devres;
+	if (ret) {
+		DRM_ERROR("Unable to initialize modesetting\n");
+		goto out_release_group;
+	}
 
 	drm_debugfs_add_files(&vkms_device->drm, vkms_config_debugfs_list,
 			      ARRAY_SIZE(vkms_config_debugfs_list));
 
 	ret = drm_dev_register(&vkms_device->drm, 0);
-	if (ret)
-		goto out_devres;
+	if (ret) {
+		DRM_ERROR("Unable to register device\n");
+		return ret;
+	}
 
 	drm_fbdev_generic_setup(&vkms_device->drm, 0);
+	platform_set_drvdata(pdev, vkms_device);
+	devres_close_group(&pdev->dev, grp);
 
 	return 0;
 
-out_devres:
-	devres_release_group(&pdev->dev, NULL);
-out_unregister:
-	platform_device_unregister(pdev);
+out_release_group:
+	devres_release_group(&pdev->dev, grp);
 	return ret;
 }
 
-static int __init vkms_init(void)
+static int vkms_platform_remove(struct platform_device *pdev)
 {
-	int ret;
-	struct vkms_config *config;
-
-	config = kmalloc(sizeof(*config), GFP_KERNEL);
-	if (!config)
-		return -ENOMEM;
-
-	default_config = config;
+	struct vkms_device *vkms_device;
 
-	config->cursor = enable_cursor;
-	config->writeback = enable_writeback;
-	config->overlay = enable_overlay;
+	vkms_device = platform_get_drvdata(pdev);
+	if (!vkms_device)
+		return 0;
 
-	ret = vkms_create(config);
-	if (ret)
-		kfree(config);
-
-	return ret;
+	drm_dev_unregister(&vkms_device->drm);
+	drm_atomic_helper_shutdown(&vkms_device->drm);
+	return 0;
 }
 
-static void vkms_destroy(struct vkms_config *config)
+static struct platform_driver vkms_platform_driver = {
+	.probe = vkms_platform_probe,
+	.remove = vkms_platform_remove,
+	.driver.name = DRIVER_NAME,
+};
+
+static int __init vkms_init(void)
 {
+	int ret;
 	struct platform_device *pdev;
 
-	if (!config->dev) {
-		DRM_INFO("vkms_device is NULL.\n");
-		return;
+	ret = platform_driver_register(&vkms_platform_driver);
+	if (ret) {
+		DRM_ERROR("Unable to register platform driver\n");
+		return ret;
 	}
 
-	pdev = config->dev->platform;
-
-	drm_dev_unregister(&config->dev->drm);
-	drm_atomic_helper_shutdown(&config->dev->drm);
-	devres_release_group(&pdev->dev, NULL);
-	platform_device_unregister(pdev);
+	pdev = platform_device_register_simple(DRIVER_NAME, -1, NULL, 0);
+	if (IS_ERR(pdev)) {
+		platform_driver_unregister(&vkms_platform_driver);
+		return PTR_ERR(pdev);
+	}
 
-	config->dev = NULL;
+	return 0;
 }
 
 static void __exit vkms_exit(void)
 {
-	if (default_config->dev)
-		vkms_destroy(default_config);
+	struct device *dev;
+
+	while ((dev = platform_find_device_by_driver(
+			NULL, &vkms_platform_driver.driver))) {
+		// platform_find_device_by_driver increments the refcount. Drop 
+		// it so we don't leak memory.
+		put_device(dev);
+		platform_device_unregister(to_platform_device(dev));
+	}
 
-	kfree(default_config);
+	platform_driver_unregister(&vkms_platform_driver);
 }
 
 module_init(vkms_init);
diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
index 5f1a0a44a78c..e87c8aea6fb3 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.h
+++ b/drivers/gpu/drm/vkms/vkms_drv.h
@@ -114,15 +114,13 @@  struct vkms_config {
 	bool writeback;
 	bool cursor;
 	bool overlay;
-	/* only set when instantiated */
-	struct vkms_device *dev;
 };
 
 struct vkms_device {
 	struct drm_device drm;
 	struct platform_device *platform;
 	struct vkms_output output;
-	const struct vkms_config *config;
+	struct vkms_config config;
 };
 
 #define drm_crtc_to_vkms_output(target) \
diff --git a/drivers/gpu/drm/vkms/vkms_output.c b/drivers/gpu/drm/vkms/vkms_output.c
index 5ce70dd946aa..963a64cf068b 100644
--- a/drivers/gpu/drm/vkms/vkms_output.c
+++ b/drivers/gpu/drm/vkms/vkms_output.c
@@ -62,7 +62,7 @@  int vkms_output_init(struct vkms_device *vkmsdev, int index)
 	if (IS_ERR(primary))
 		return PTR_ERR(primary);
 
-	if (vkmsdev->config->overlay) {
+	if (vkmsdev->config.overlay) {
 		for (n = 0; n < NUM_OVERLAY_PLANES; n++) {
 			ret = vkms_add_overlay_plane(vkmsdev, index, crtc);
 			if (ret)
@@ -70,7 +70,7 @@  int vkms_output_init(struct vkms_device *vkmsdev, int index)
 		}
 	}
 
-	if (vkmsdev->config->cursor) {
+	if (vkmsdev->config.cursor) {
 		cursor = vkms_plane_init(vkmsdev, DRM_PLANE_TYPE_CURSOR, index);
 		if (IS_ERR(cursor))
 			return PTR_ERR(cursor);
@@ -103,7 +103,7 @@  int vkms_output_init(struct vkms_device *vkmsdev, int index)
 		goto err_attach;
 	}
 
-	if (vkmsdev->config->writeback) {
+	if (vkmsdev->config.writeback) {
 		writeback = vkms_enable_writeback_connector(vkmsdev);
 		if (writeback)
 			DRM_ERROR("Failed to init writeback connector\n");