| Message ID | 20200904143941.110665-6-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm_managed, leftovers | expand |
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@intel.com> > Cc: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com> > Cc: Haneen Mohammed <hamohammed.sa@gmail.com> > Cc: Daniel Vetter <daniel@ffwll.ch> > --- > 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@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
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; } 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);
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@intel.com> Cc: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com> Cc: Haneen Mohammed <hamohammed.sa@gmail.com> Cc: Daniel Vetter <daniel@ffwll.ch> --- drivers/gpu/drm/vkms/vkms_drv.c | 54 ++++++++++++++++----------------- 1 file changed, 26 insertions(+), 28 deletions(-)