| Message ID | 20230829053201.423261-2-brpol@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Adds support for ConfigFS to VKMS! | expand |
On Tue, Aug 29, 2023 at 05:30:53AM +0000, Brandon Pollack wrote: > From: Jim Shargo <jshargo@chromium.org> > > 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> > Signed-off-by: Brandon Pollack <brpol@chromium.org> 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 >
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");