Hi Daniel,
Thanks for this work.
This change works smoothly to me. However, there is a check in the vkms_exit
that doesn't look very good. Please, see inline comment.
On 09/04, Daniel Vetter wrote:
> This means we also need to slightly restructure the exit code, so that
> final cleanup of the drm_device is triggered by unregistering the
> platform device. Note that devres is both clean up when the driver is
> unbound (not the case for vkms, we don't bind), and also when unregistering
> the device (very much the case for vkms). Therefore we can rely on devres
> even though vkms isn't a proper platform device driver.
>
> This also somewhat untangles the load code, since the drm and platform device
> setup are no longer interleaved, but two distinct steps.
>
> v2: use devres_open/release_group so we can use devm without real
> hacks in the driver core or having to create an entire fake bus for
> testing drivers. Might want to extract this into helpers eventually,
> maybe as a mock_drm_dev_alloc or test_drm_dev_alloc.
>
> Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxxx>
> Cc: Rodrigo Siqueira <rodrigosiqueiramelo@xxxxxxxxx>
> Cc: Haneen Mohammed <hamohammed.sa@xxxxxxxxx>
> Cc: Daniel Vetter <daniel@xxxxxxxx>
> ---
> drivers/gpu/drm/vkms/vkms_drv.c | 54 ++++++++++++++++-----------------
> 1 file changed, 26 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
> index 83dd5567de8b..0d2c6dcf73c3 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.c
> +++ b/drivers/gpu/drm/vkms/vkms_drv.c
> @@ -61,9 +61,6 @@ static void vkms_release(struct drm_device *dev)
> {
> struct vkms_device *vkms = container_of(dev, struct vkms_device, drm);
>
> - platform_device_unregister(vkms->platform);
> - drm_atomic_helper_shutdown(&vkms->drm);
> - drm_mode_config_cleanup(&vkms->drm);
> destroy_workqueue(vkms->output.composer_workq);
> }
>
> @@ -144,30 +141,31 @@ static int vkms_modeset_init(struct vkms_device *vkmsdev)
> static int __init vkms_init(void)
> {
> int ret;
> + struct platform_device *pdev;
>
> - vkms_device = kzalloc(sizeof(*vkms_device), GFP_KERNEL);
> - if (!vkms_device)
> - return -ENOMEM;
> + pdev = platform_device_register_simple(DRIVER_NAME, -1, NULL, 0);
> + if (IS_ERR(pdev))
> + return PTR_ERR(pdev);
>
> - vkms_device->platform =
> - platform_device_register_simple(DRIVER_NAME, -1, NULL, 0);
> - if (IS_ERR(vkms_device->platform)) {
> - ret = PTR_ERR(vkms_device->platform);
> - goto out_free;
> + if (!devres_open_group(&pdev->dev, NULL, GFP_KERNEL)) {
> + ret = -ENOMEM;
> + goto out_unregister;
> }
>
> - ret = drm_dev_init(&vkms_device->drm, &vkms_driver,
> - &vkms_device->platform->dev);
> - if (ret)
> - goto out_unregister;
> - drmm_add_final_kfree(&vkms_device->drm, vkms_device);
> + 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;
> + }
> + vkms_device->platform = pdev;
>
> 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_put;
> + goto out_devres;
> }
>
> vkms_device->drm.irq_enabled = true;
> @@ -175,39 +173,39 @@ static int __init vkms_init(void)
> ret = drm_vblank_init(&vkms_device->drm, 1);
> if (ret) {
> DRM_ERROR("Failed to vblank\n");
> - goto out_put;
> + goto out_devres;
> }
>
> ret = vkms_modeset_init(vkms_device);
> if (ret)
> - goto out_put;
> + goto out_devres;
>
> ret = drm_dev_register(&vkms_device->drm, 0);
> if (ret)
> - goto out_put;
> + goto out_devres;
>
> return 0;
>
> -out_put:
> - drm_dev_put(&vkms_device->drm);
> - platform_device_unregister(vkms_device->platform);
> - return ret;
> +out_devres:
> + devres_release_group(&pdev->dev, NULL);
> out_unregister:
> - platform_device_unregister(vkms_device->platform);
> -out_free:
> - kfree(vkms_device);
> + platform_device_unregister(pdev);
> return ret;
> }
>
> static void __exit vkms_exit(void)
> {
> + struct platform_device *pdev = vkms_device->platform;
> +
> if (!vkms_device) {
> DRM_INFO("vkms_device is NULL.\n");
> return;
> }
I think it would be better to check vkms_device before assigning
vkms_device->platform to the pdev above. I don't know if the compiler
handles this, but doing the validation first seems more clear.
>
> drm_dev_unregister(&vkms_device->drm);
> - drm_dev_put(&vkms_device->drm);
> + drm_atomic_helper_shutdown(&vkms_device->drm);
> + devres_release_group(&pdev->dev, NULL);
> + platform_device_unregister(pdev);
> }
>
> module_init(vkms_init);
> --
> 2.28.0
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
