| Message ID | 20200904143941.110665-5-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm_managed, leftovers | expand |
Hi Daniel, looks good to me, just a few things inline. 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 vgem, we don't bind), and also when unregistering > the device (very much the case for vgem). Therefore we can rely on devres > even though vgem 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@ffwll.ch> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > Cc: Emil Velikov <emil.velikov@collabora.com> > Cc: Sean Paul <seanpaul@chromium.org> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Sam Ravnborg <sam@ravnborg.org> > Cc: Rob Clark <robdclark@chromium.org> > --- > drivers/gpu/drm/vgem/vgem_drv.c | 55 ++++++++++++++------------------- > 1 file changed, 24 insertions(+), 31 deletions(-) > > diff --git a/drivers/gpu/drm/vgem/vgem_drv.c b/drivers/gpu/drm/vgem/vgem_drv.c > index 313339bbff90..f95537627463 100644 > --- a/drivers/gpu/drm/vgem/vgem_drv.c > +++ b/drivers/gpu/drm/vgem/vgem_drv.c > @@ -401,16 +401,8 @@ static int vgem_prime_mmap(struct drm_gem_object *obj, > return 0; > } > > -static void vgem_release(struct drm_device *dev) > -{ > - struct vgem_device *vgem = container_of(dev, typeof(*vgem), drm); > - > - platform_device_unregister(vgem->platform); > -} > - > static struct drm_driver vgem_driver = { > .driver_features = DRIVER_GEM | DRIVER_RENDER, > - .release = vgem_release, > .open = vgem_open, > .postclose = vgem_postclose, > .gem_free_object_unlocked = vgem_gem_free_object, > @@ -442,48 +434,49 @@ static struct drm_driver vgem_driver = { > static int __init vgem_init(void) > { > int ret; > + struct platform_device *pdev; > > - vgem_device = kzalloc(sizeof(*vgem_device), GFP_KERNEL); > - if (!vgem_device) > - return -ENOMEM; > + pdev = platform_device_register_simple("vgem", -1, NULL, 0); > + if (IS_ERR(pdev)) > + return PTR_ERR(vgem_device->platform); I caught this line right above. It should be: return PTR_ERR (pdev), right? > > - vgem_device->platform = > - platform_device_register_simple("vgem", -1, NULL, 0); > - if (IS_ERR(vgem_device->platform)) { > - ret = PTR_ERR(vgem_device->platform); > - goto out_free; > + if (!devres_open_group(&pdev->dev, NULL, GFP_KERNEL)) { > + ret = -ENOMEM; > + goto out_unregister; > } > > - dma_coerce_mask_and_coherent(&vgem_device->platform->dev, > + dma_coerce_mask_and_coherent(&pdev->dev, > DMA_BIT_MASK(64)); > - ret = drm_dev_init(&vgem_device->drm, &vgem_driver, > - &vgem_device->platform->dev); > - if (ret) > - goto out_unregister; > - drmm_add_final_kfree(&vgem_device->drm, vgem_device); > + > + vgem_device = devm_drm_dev_alloc(&pdev->dev, &vgem_driver, > + struct vgem_device, drm); > + if (IS_ERR(vgem_device)) { > + ret = PTR_ERR(vgem_device); > + goto out_devres; > + } > + vgem_device->platform = pdev; > > /* Final step: expose the device/driver to userspace */ > ret = drm_dev_register(&vgem_device->drm, 0); > if (ret) > - goto out_put; > + goto out_devres; > > return 0; > > -out_put: > - drm_dev_put(&vgem_device->drm); > - platform_device_unregister(vgem_device->platform); > - return ret; > +out_devres: > + devres_release_group(&pdev->dev, NULL); > out_unregister: > - platform_device_unregister(vgem_device->platform); > -out_free: > - kfree(vgem_device); > + platform_device_unregister(pdev); > return ret; > } > > static void __exit vgem_exit(void) > { > + struct platform_device *pdev = vgem_device->platform; > + Well, there has never been a check for a null vgem_device here before, as in vkms. Should? > drm_dev_unregister(&vgem_device->drm); > - drm_dev_put(&vgem_device->drm); > + devres_release_group(&pdev->dev, NULL); > + platform_device_unregister(pdev); > } > > module_init(vgem_init); > -- > 2.28.0 Apart from these two points, Reviewed-by: Melissa Wen <melissa.srw@gmail.com> Thanks! > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Wed, Sep 9, 2020 at 1:01 PM Melissa Wen <melissa.srw@gmail.com> wrote: > > Hi Daniel, > > looks good to me, just a few things inline. > > 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 vgem, we don't bind), and also when unregistering > > the device (very much the case for vgem). Therefore we can rely on devres > > even though vgem 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@ffwll.ch> > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > > Cc: Emil Velikov <emil.velikov@collabora.com> > > Cc: Sean Paul <seanpaul@chromium.org> > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > > Cc: Sam Ravnborg <sam@ravnborg.org> > > Cc: Rob Clark <robdclark@chromium.org> > > --- > > drivers/gpu/drm/vgem/vgem_drv.c | 55 ++++++++++++++------------------- > > 1 file changed, 24 insertions(+), 31 deletions(-) > > > > diff --git a/drivers/gpu/drm/vgem/vgem_drv.c b/drivers/gpu/drm/vgem/vgem_drv.c > > index 313339bbff90..f95537627463 100644 > > --- a/drivers/gpu/drm/vgem/vgem_drv.c > > +++ b/drivers/gpu/drm/vgem/vgem_drv.c > > @@ -401,16 +401,8 @@ static int vgem_prime_mmap(struct drm_gem_object *obj, > > return 0; > > } > > > > -static void vgem_release(struct drm_device *dev) > > -{ > > - struct vgem_device *vgem = container_of(dev, typeof(*vgem), drm); > > - > > - platform_device_unregister(vgem->platform); > > -} > > - > > static struct drm_driver vgem_driver = { > > .driver_features = DRIVER_GEM | DRIVER_RENDER, > > - .release = vgem_release, > > .open = vgem_open, > > .postclose = vgem_postclose, > > .gem_free_object_unlocked = vgem_gem_free_object, > > @@ -442,48 +434,49 @@ static struct drm_driver vgem_driver = { > > static int __init vgem_init(void) > > { > > int ret; > > + struct platform_device *pdev; > > > > - vgem_device = kzalloc(sizeof(*vgem_device), GFP_KERNEL); > > - if (!vgem_device) > > - return -ENOMEM; > > + pdev = platform_device_register_simple("vgem", -1, NULL, 0); > > + if (IS_ERR(pdev)) > > + return PTR_ERR(vgem_device->platform); > I caught this line right above. > It should be: return PTR_ERR (pdev), right? Yes I will fix. > > - vgem_device->platform = > > - platform_device_register_simple("vgem", -1, NULL, 0); > > - if (IS_ERR(vgem_device->platform)) { > > - ret = PTR_ERR(vgem_device->platform); > > - goto out_free; > > + if (!devres_open_group(&pdev->dev, NULL, GFP_KERNEL)) { > > + ret = -ENOMEM; > > + goto out_unregister; > > } > > > > - dma_coerce_mask_and_coherent(&vgem_device->platform->dev, > > + dma_coerce_mask_and_coherent(&pdev->dev, > > DMA_BIT_MASK(64)); > > - ret = drm_dev_init(&vgem_device->drm, &vgem_driver, > > - &vgem_device->platform->dev); > > - if (ret) > > - goto out_unregister; > > - drmm_add_final_kfree(&vgem_device->drm, vgem_device); > > + > > + vgem_device = devm_drm_dev_alloc(&pdev->dev, &vgem_driver, > > + struct vgem_device, drm); > > + if (IS_ERR(vgem_device)) { > > + ret = PTR_ERR(vgem_device); > > + goto out_devres; > > + } > > + vgem_device->platform = pdev; > > > > /* Final step: expose the device/driver to userspace */ > > ret = drm_dev_register(&vgem_device->drm, 0); > > if (ret) > > - goto out_put; > > + goto out_devres; > > > > return 0; > > > > -out_put: > > - drm_dev_put(&vgem_device->drm); > > - platform_device_unregister(vgem_device->platform); > > - return ret; > > +out_devres: > > + devres_release_group(&pdev->dev, NULL); > > out_unregister: > > - platform_device_unregister(vgem_device->platform); > > -out_free: > > - kfree(vgem_device); > > + platform_device_unregister(pdev); > > return ret; > > } > > > > static void __exit vgem_exit(void) > > { > > + struct platform_device *pdev = vgem_device->platform; > > + > Well, there has never been a check for a null vgem_device here before, > as in vkms. Should? I think it should, but that's kinda a separate patch. Want to type it? -Daniel > > drm_dev_unregister(&vgem_device->drm); > > - drm_dev_put(&vgem_device->drm); > > + devres_release_group(&pdev->dev, NULL); > > + platform_device_unregister(pdev); > > } > > > > module_init(vgem_init); > > -- > > 2.28.0 > > Apart from these two points, > > Reviewed-by: Melissa Wen <melissa.srw@gmail.com> > > Thanks! > > > > _______________________________________________ > > dri-devel mailing list > > dri-devel@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
On 09/09, Daniel Vetter wrote: > On Wed, Sep 9, 2020 at 1:01 PM Melissa Wen <melissa.srw@gmail.com> wrote: > > > > Hi Daniel, > > > > looks good to me, just a few things inline. > > > > 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 vgem, we don't bind), and also when unregistering > > > the device (very much the case for vgem). Therefore we can rely on devres > > > even though vgem 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@ffwll.ch> > > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > > > Cc: Emil Velikov <emil.velikov@collabora.com> > > > Cc: Sean Paul <seanpaul@chromium.org> > > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > > > Cc: Sam Ravnborg <sam@ravnborg.org> > > > Cc: Rob Clark <robdclark@chromium.org> > > > --- > > > drivers/gpu/drm/vgem/vgem_drv.c | 55 ++++++++++++++------------------- > > > 1 file changed, 24 insertions(+), 31 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/vgem/vgem_drv.c b/drivers/gpu/drm/vgem/vgem_drv.c > > > index 313339bbff90..f95537627463 100644 > > > --- a/drivers/gpu/drm/vgem/vgem_drv.c > > > +++ b/drivers/gpu/drm/vgem/vgem_drv.c > > > @@ -401,16 +401,8 @@ static int vgem_prime_mmap(struct drm_gem_object *obj, > > > return 0; > > > } > > > > > > -static void vgem_release(struct drm_device *dev) > > > -{ > > > - struct vgem_device *vgem = container_of(dev, typeof(*vgem), drm); > > > - > > > - platform_device_unregister(vgem->platform); > > > -} > > > - > > > static struct drm_driver vgem_driver = { > > > .driver_features = DRIVER_GEM | DRIVER_RENDER, > > > - .release = vgem_release, > > > .open = vgem_open, > > > .postclose = vgem_postclose, > > > .gem_free_object_unlocked = vgem_gem_free_object, > > > @@ -442,48 +434,49 @@ static struct drm_driver vgem_driver = { > > > static int __init vgem_init(void) > > > { > > > int ret; > > > + struct platform_device *pdev; > > > > > > - vgem_device = kzalloc(sizeof(*vgem_device), GFP_KERNEL); > > > - if (!vgem_device) > > > - return -ENOMEM; > > > + pdev = platform_device_register_simple("vgem", -1, NULL, 0); > > > + if (IS_ERR(pdev)) > > > + return PTR_ERR(vgem_device->platform); > > I caught this line right above. > > It should be: return PTR_ERR (pdev), right? > > Yes I will fix. > > > > - vgem_device->platform = > > > - platform_device_register_simple("vgem", -1, NULL, 0); > > > - if (IS_ERR(vgem_device->platform)) { > > > - ret = PTR_ERR(vgem_device->platform); > > > - goto out_free; > > > + if (!devres_open_group(&pdev->dev, NULL, GFP_KERNEL)) { > > > + ret = -ENOMEM; > > > + goto out_unregister; > > > } > > > > > > - dma_coerce_mask_and_coherent(&vgem_device->platform->dev, > > > + dma_coerce_mask_and_coherent(&pdev->dev, > > > DMA_BIT_MASK(64)); > > > - ret = drm_dev_init(&vgem_device->drm, &vgem_driver, > > > - &vgem_device->platform->dev); > > > - if (ret) > > > - goto out_unregister; > > > - drmm_add_final_kfree(&vgem_device->drm, vgem_device); > > > + > > > + vgem_device = devm_drm_dev_alloc(&pdev->dev, &vgem_driver, > > > + struct vgem_device, drm); > > > + if (IS_ERR(vgem_device)) { > > > + ret = PTR_ERR(vgem_device); > > > + goto out_devres; > > > + } > > > + vgem_device->platform = pdev; > > > > > > /* Final step: expose the device/driver to userspace */ > > > ret = drm_dev_register(&vgem_device->drm, 0); > > > if (ret) > > > - goto out_put; > > > + goto out_devres; > > > > > > return 0; > > > > > > -out_put: > > > - drm_dev_put(&vgem_device->drm); > > > - platform_device_unregister(vgem_device->platform); > > > - return ret; > > > +out_devres: > > > + devres_release_group(&pdev->dev, NULL); > > > out_unregister: > > > - platform_device_unregister(vgem_device->platform); > > > -out_free: > > > - kfree(vgem_device); > > > + platform_device_unregister(pdev); > > > return ret; > > > } > > > > > > static void __exit vgem_exit(void) > > > { > > > + struct platform_device *pdev = vgem_device->platform; > > > + > > Well, there has never been a check for a null vgem_device here before, > > as in vkms. Should? > > I think it should, but that's kinda a separate patch. Want to type it? Make sense. Ok, I can do that. > -Daniel > > > > drm_dev_unregister(&vgem_device->drm); > > > - drm_dev_put(&vgem_device->drm); > > > + devres_release_group(&pdev->dev, NULL); > > > + platform_device_unregister(pdev); > > > } > > > > > > module_init(vgem_init); > > > -- > > > 2.28.0 > > > > Apart from these two points, > > > > Reviewed-by: Melissa Wen <melissa.srw@gmail.com> > > > > Thanks! > > > > > > _______________________________________________ > > > dri-devel mailing list > > > dri-devel@lists.freedesktop.org > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel > > > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch
diff --git a/drivers/gpu/drm/vgem/vgem_drv.c b/drivers/gpu/drm/vgem/vgem_drv.c index 313339bbff90..f95537627463 100644 --- a/drivers/gpu/drm/vgem/vgem_drv.c +++ b/drivers/gpu/drm/vgem/vgem_drv.c @@ -401,16 +401,8 @@ static int vgem_prime_mmap(struct drm_gem_object *obj, return 0; } -static void vgem_release(struct drm_device *dev) -{ - struct vgem_device *vgem = container_of(dev, typeof(*vgem), drm); - - platform_device_unregister(vgem->platform); -} - static struct drm_driver vgem_driver = { .driver_features = DRIVER_GEM | DRIVER_RENDER, - .release = vgem_release, .open = vgem_open, .postclose = vgem_postclose, .gem_free_object_unlocked = vgem_gem_free_object, @@ -442,48 +434,49 @@ static struct drm_driver vgem_driver = { static int __init vgem_init(void) { int ret; + struct platform_device *pdev; - vgem_device = kzalloc(sizeof(*vgem_device), GFP_KERNEL); - if (!vgem_device) - return -ENOMEM; + pdev = platform_device_register_simple("vgem", -1, NULL, 0); + if (IS_ERR(pdev)) + return PTR_ERR(vgem_device->platform); - vgem_device->platform = - platform_device_register_simple("vgem", -1, NULL, 0); - if (IS_ERR(vgem_device->platform)) { - ret = PTR_ERR(vgem_device->platform); - goto out_free; + if (!devres_open_group(&pdev->dev, NULL, GFP_KERNEL)) { + ret = -ENOMEM; + goto out_unregister; } - dma_coerce_mask_and_coherent(&vgem_device->platform->dev, + dma_coerce_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64)); - ret = drm_dev_init(&vgem_device->drm, &vgem_driver, - &vgem_device->platform->dev); - if (ret) - goto out_unregister; - drmm_add_final_kfree(&vgem_device->drm, vgem_device); + + vgem_device = devm_drm_dev_alloc(&pdev->dev, &vgem_driver, + struct vgem_device, drm); + if (IS_ERR(vgem_device)) { + ret = PTR_ERR(vgem_device); + goto out_devres; + } + vgem_device->platform = pdev; /* Final step: expose the device/driver to userspace */ ret = drm_dev_register(&vgem_device->drm, 0); if (ret) - goto out_put; + goto out_devres; return 0; -out_put: - drm_dev_put(&vgem_device->drm); - platform_device_unregister(vgem_device->platform); - return ret; +out_devres: + devres_release_group(&pdev->dev, NULL); out_unregister: - platform_device_unregister(vgem_device->platform); -out_free: - kfree(vgem_device); + platform_device_unregister(pdev); return ret; } static void __exit vgem_exit(void) { + struct platform_device *pdev = vgem_device->platform; + drm_dev_unregister(&vgem_device->drm); - drm_dev_put(&vgem_device->drm); + devres_release_group(&pdev->dev, NULL); + platform_device_unregister(pdev); } module_init(vgem_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 vgem, we don't bind), and also when unregistering the device (very much the case for vgem). Therefore we can rely on devres even though vgem 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@ffwll.ch> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> Cc: Emil Velikov <emil.velikov@collabora.com> Cc: Sean Paul <seanpaul@chromium.org> Cc: Chris Wilson <chris@chris-wilson.co.uk> Cc: Sam Ravnborg <sam@ravnborg.org> Cc: Rob Clark <robdclark@chromium.org> --- drivers/gpu/drm/vgem/vgem_drv.c | 55 ++++++++++++++------------------- 1 file changed, 24 insertions(+), 31 deletions(-)