| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <aadd1da1-8684-ca28-a3e9-9abeaf258aff@riseup.net>
Date: Sun, 25 Jun 2023 14:40:55 -0300
From: Maira Canal <mairacanal@...eup.net>
To: Jim Shargo <jshargo@...omium.org>, Daniel Vetter <daniel@...ll.ch>,
David Airlie <airlied@...il.com>,
Haneen Mohammed <hamohammed.sa@...il.com>,
Jonathan Corbet <corbet@....net>,
Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
Maxime Ripard <mripard@...nel.org>,
Melissa Wen <melissa.srw@...il.com>,
Rodrigo Siqueira <rodrigosiqueiramelo@...il.com>,
Thomas Zimmermann <tzimmermann@...e.de>
Cc: dri-devel@...ts.freedesktop.org, linux-doc@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/6] drm/vkms: Back VKMS with DRM memory management
instead of static objects
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@...omium.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@...ora ~]# modprobe vkms enable_overlay=1 enable_cursor=1
enable_writeback=1
[root@...ora ~]# cd /sys/kernel/debug/dri/0
[root@...ora 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");
Powered by blists - more mailing lists