On Wed, Apr 08, 2020 at 08:57:14AM +0200, Sam Ravnborg wrote:
Hi Daniel.
Finally managed to dive into this..
Maybe I need more coffee, it is still morning here. But alas this patch triggered a few comments.
Sam
On Fri, Apr 03, 2020 at 03:57:46PM +0200, Daniel Vetter wrote:
The kerneldoc is only added for this new function. Existing kerneldoc and examples will be udated at the very end, since once all drivers are converted over to devm_drm_dev_alloc we can unexport a lot of interim functions and make the documentation for driver authors a lot cleaner and less confusing. There will be only one true way to initialize a drm_device at the end of this, which is going to be devm_drm_dev_alloc.
This changelog entry does a poor job describing what the purpose of this change is. Try to read it outside context.
Something like:
Add a new macro helper to combine the usual init sequence in drivers, consisting of a kzalloc + devm_drm_dev_init + drmm_add_final_kfree triplet. This allows us to remove the rather unsightly drmm_add_final_kfree from all currently merged drivers.
This good enough, as an intro paragraph?
Cc: Paul Kocialkowski paul.kocialkowski@bootlin.com Cc: Laurent Pinchart laurent.pinchart@ideasonboard.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com
drivers/gpu/drm/drm_drv.c | 23 +++++++++++++++++++++++ include/drm/drm_drv.h | 33 +++++++++++++++++++++++++++++++++ 2 files changed, 56 insertions(+)
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c index 1bb4f636b83c..9e60b784b3ac 100644 --- a/drivers/gpu/drm/drm_drv.c +++ b/drivers/gpu/drm/drm_drv.c @@ -739,6 +739,29 @@ int devm_drm_dev_init(struct device *parent, } EXPORT_SYMBOL(devm_drm_dev_init);
+void* __devm_drm_dev_alloc(struct device *parent, struct drm_driver *driver,
size_t size, size_t offset)+{
- void *container;
- struct drm_device *drm;
- int ret;
- container = kzalloc(size, GFP_KERNEL);
- if (!container)
return ERR_PTR(-ENOMEM);- drm = container + offset;
- ret = devm_drm_dev_init(parent, drm, driver);
- if (ret) {
kfree(container);return ERR_PTR(ret);- }
- drmm_add_final_kfree(drm, container);
- return container;
+} +EXPORT_SYMBOL(__devm_drm_dev_alloc);
/**
- drm_dev_alloc - Allocate new DRM device
- @driver: DRM driver to allocate device for
diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h index e7c6ea261ed1..26776be5a21e 100644 --- a/include/drm/drm_drv.h +++ b/include/drm/drm_drv.h @@ -626,6 +626,39 @@ int devm_drm_dev_init(struct device *parent, struct drm_device *dev, struct drm_driver *driver);
+void* __devm_drm_dev_alloc(struct device *parent, struct drm_driver *driver,
size_t size, size_t offset);+/**
- devm_drm_dev_alloc - Resource managed allocation of a &drm_device instance
- @parent: Parent device object
- @driver: DRM driver
- @type: the type of the struct which contains struct &drm_device
- @member: the name of the &drm_device within @type.
I am confused about the naming here. devm_ implies we allocate something with a lifetime equal that of a driver. So when the driver are gone what we allocate is also gone. Like everythign else devm_ prefixed.
But the lifetime of a drm_device is until the last userspace reference is gone (final drm_dev_put() is called).
The kerneldoc for this is largely copied from the existing devm_drm_dev_init. And yes the lifetime is bound to the device, we do the drm_dev_put() when that disappears. Now other users of drm_device might still hold references and delay cleanup, but "cleanup is done as a devres action" is very much what devm_ signifies. "
- This allocates and initialize a new DRM device. No device registration is done.
- Call drm_dev_register() to advertice the device to user space and register it
- with other core subsystems. This should be done last in the device
s/This/Calling drm_dev_register()/ will make this sentence a bit more explicit.
- initialization sequence to make sure userspace can't access an inconsistent
- state.
- The initial ref-count of the object is 1. Use drm_dev_get() and
- drm_dev_put() to take and drop further ref-counts.
- It is recommended that drivers embed &struct drm_device into their own device
- structure.
- Note that this manages the lifetime of the resulting &drm_device
- automatically using devres.
Hmm, no this is managed by drmres???
Yup, the next sentence explains how. And note that we're already using this in the form of devm_drm_dev_init. So not clear what's unclear here ...
Thanks for your comments. -Daniel
- The DRM device initialized with this function is
- automatically put on driver detach using drm_dev_put().
- RETURNS:
- Pointer to new DRM device, or ERR_PTR on failure.
- */
+#define devm_drm_dev_alloc(parent, driver, type, member) \
- ((type *) __devm_drm_dev_alloc(parent, driver, sizeof(type), \
offsetof(type, member)))struct drm_device *drm_dev_alloc(struct drm_driver *driver, struct device *parent); int drm_dev_register(struct drm_device *dev, unsigned long flags); -- 2.25.1
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel