Re: [PATCH v6 1/7] drm/vkms: Back VKMS with DRM memory management instead of static objects

From: Daniel Vetter
Date: Tue Apr 30 2024 - 03:47:13 EST

Next message: Krzysztof Kozlowski: "Re: [PATCH v3 1/2] dt-bindings: hwmon: Add Sophgo SG2042 external hardware monitor support"
Previous message: Jian-Hong Pan: "Re: [PATCH v5 4/4] PCI/ASPM: Fix L1.2 parameters when enable link state"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Tue, Aug 29, 2023 at 05:30:53AM +0000, Brandon Pollack wrote:
> From: Jim Shargo <jshargo@xxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxx>
> Signed-off-by: Brandon Pollack <brpol@xxxxxxxxxxxx>

Ok a few things on more the design of this all:

- This patch switches vkms over to be a real platform driver with
probe/remove hooks. Fine with me, but would need to be split out and
properly justified with some reason.

Also if we do this, we don't need our own devres group anymore, that's
redundant.

- Now on the actual configfs prep work, the issue is that configfs and
drm_device have independent lifetimes, and we cannot have links between
the two. The configfs structures might have shorter lifetime thatn the
vkms_device (and there's no refcounting on configfs afaict), which means
we cannot have a backpointer from vkms_device to configfs. We can only
rely on configfs being around during the probe/init code, thanks to the
configfs mutex.

This is why the refactoring to use vkms_config was done, so that
configfs becomes a two-step process: 1. convert the configfs data into a
struct vkms_config 2. initialize the vkms device using that stand-alone
vkms_config.

Now I understand that this is quite a bit of work, but I think it would
also lead to somewhat more robust code (stuff like the lifetime bugs in
the connector hotplug patch become impossible). So before we go into
details, we need to make a decision whether we stick with the
vkms_config design, or whether we just have two separate paths: a)
default device setup and b) configfs dynamic setup. Imo this also needs
an ack from Maíra.

- If we stick with the split approach then we need a few changes.

- The vkms_device->configfs backpointer needs to go, it's fundamentally
busted. Instead I think it's best we pass the configfs pointer around
directly during init time.

- I think we should clearly split the default and configfs paths with
explicit suffixes in the relevant functions (like
vkms_output_init_default/configfs) and explicitly passing parameters
around.

- the vkms_config design doesn't have a use anymore, imo we should
simplify the _default() paths to again just look at the module
parameters. But only if we go the route of having split setup paths.

- Data that's only relevant for one or the other paths needs to be
removed from global structures. This would be vkms_config and
vkms_output in vkms_device (the latter is left around, which is very
confusing since the configfs path doesn't use that at all).

- Going with a real platform driver means passing arguments to
probe/init becomes tricky. Using the platform data seems like a good
approach, but we must ensure that we clear that again when we remove
the platform_device. Note that with a real driver userspace can
unbind/rebind the driver through sysfs, which means we must keep the
configfs pointer valid until the device is removed, to make sure that
these subsequent re-probe calls still work. These complications are
also the reasons why I think we should only switch to a real platform
driver if there's a reason, a lot of things become a lot more
complicated and dynamic and tricky with this.

Also this means that driver code could try to look at configfs even
after probe/init is finished, which is buggy but would mostly work
(unless you really carefully race hotunplug against other driver
code). So potential for some tricky bugs. That's why I think we should
explicitly pass the configfs pointer as a function parameter and only
grab it from the platform_data in ->probe once (and put a comment
there why this is really tricky).

Anyway before we proceed I think we need consensus on these 2 design
questions.
-Sima

> ---
> drivers/gpu/drm/vkms/vkms_drv.c | 128 +++++++++++++++--------------
> drivers/gpu/drm/vkms/vkms_drv.h | 4 +-
> drivers/gpu/drm/vkms/vkms_output.c | 6 +-
> 3 files changed, 71 insertions(+), 67 deletions(-)
>
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
> index dd0af086e7fa..387c832f5dc9 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");
> @@ -96,9 +96,9 @@ static int vkms_config_show(struct seq_file *m, void *data)
> struct drm_device *dev = entry->dev;
> struct vkms_device *vkmsdev = drm_device_to_vkms_device(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", vkmsdev->config.writeback);
> + seq_printf(m, "cursor=%d\n", vkmsdev->config.cursor);
> + seq_printf(m, "overlay=%d\n", vkmsdev->config.overlay);
>
> return 0;
> }
> @@ -166,121 +166,127 @@ static int vkms_modeset_init(struct vkms_device *vkmsdev)
> dev->mode_config.cursor_height = 512;
> /* FIXME: There's a confusion between bpp and depth between this and
> * fbdev helpers. We have to go with 0, meaning "pick the default",
> - * which ix XRGB8888 in all cases. */
> + * which ix XRGB8888 in all cases.
> + */
> dev->mode_config.preferred_depth = 0;
> dev->mode_config.helper_private = &vkms_mode_config_helpers;
>
> 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 with id %d\n", pdev->id);
> + goto out_release_group;
> + }
>
> 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 c7ae6c2ba1df..4c35d6305f2a 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.h
> +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> @@ -124,15 +124,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");
> --
> 2.42.0.rc2.253.gd59a3bf2b4-goog
>

--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Next message: Krzysztof Kozlowski: "Re: [PATCH v3 1/2] dt-bindings: hwmon: Add Sophgo SG2042 external hardware monitor support"
Previous message: Jian-Hong Pan: "Re: [PATCH v5 4/4] PCI/ASPM: Fix L1.2 parameters when enable link state"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]