| Message ID | 20200403135828.2542770-3-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | devm_drm_dev_alloc, no more drmm_add_final_kfree | expand |
Den 03.04.2020 15.57, skrev Daniel Vetter: > 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. > > Cc: Paul Kocialkowski <paul.kocialkowski@bootlin.com> > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > --- Acked-by: Noralf Trønnes <noralf@tronnes.org>
Hi Daniel, Thank you for the patch. 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. How about drivers that expose another interface towards userspace ? If the other related subsystem also required allocation of the driver private structure through its corresponding API, we'd be stuck. As stated before, I want this API to be optional. > 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. > + * > + * 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 > + * 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. 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);
On Mon, Apr 6, 2020 at 2:01 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Daniel, > > Thank you for the patch. > > 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. > > How about drivers that expose another interface towards userspace ? If > the other related subsystem also required allocation of the driver > private structure through its corresponding API, we'd be stuck. As > stated before, I want this API to be optional. So maybe we need to import a little bit more of our epic irc discussion here, because this is leaving out all the context. There's a few more things: - Since the merging of the previous patch series this is already mandatory, because the kfree(drm_device.managed.final_kfree) is the last thing that will run. So I already killed your use-case (and it seems to work just fine for all the drivers in-tree, and we have a lot). - As part of doing all these patches here and in the previous series and in a next one I've seen that drivers just get this wrong. I'm extremely hesitant to give that rope back to drivers, so I really don't want to merge anything until we're ready to merge a driver that needs it. I've set myself a goal to fix up all 40 odd drivers still using drm_dev_alloc(), and that's itself already pretty stupid idea. It's definitely not going to work if new drivers keep adding bad usage patterns. - Now I'm not opposed to allowing this, if/when we actually need it. I think a very clean long-term solution would be to have a struct kref_res, which augments a kref with automatic resource cleanup. We'd probably need to fully demidlayer that, i.e. if you supply your own ->release hook then it's your job to call kref_release_all() at the right point in there. With that we could then do a devm_drm_dev_init() again, which would take that kref_res structure and the drm_device, both embedded somewhere in your overall driver struture, and use your drivers kref_res to attach all drm related auto-cleanup. In that case it would then also be that kref_res' responsibility to do the final kfree, hence drm wouldn't insist on doing that either. Prototype would be something like: devm_drm_dev_init(struct device *dev, struct drm_device *drm, struct drm_driver *driver, struct kref_res *kref); The trouble with the above idea is that it assumes I have endless amounts of time and that I can convince Greg KH that I understand driver unload lifetime issues. The former is atm the more realistic looking one of these two, so interim solution would be to add some hack or another meanwhile to do this within drm only. Or as an alternative solution, easy to convert over to an eventual kref_res world: #define kref_res_get() drm_dev_get() #define kref_res_put() drm_dev_put() With suitable amounts of casting to make this work out correctly like the real kref_res solution. Until we do have such drivers though I really don't want to open the barn door again to all the bugs that'll bring, while I'm trying to get the other barn doors closed down and fixed meanwhile. -Daniel > > > 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. > > + * > > + * 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 > > + * 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. 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); > > -- > Regards, > > Laurent Pinchart
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. > > 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). > + * > + * 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??? > + * 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
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
Hi Daniel. On Wed, Apr 08, 2020 at 02:11:47PM +0200, Daniel Vetter wrote: > 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. Much better - thanks! > > 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. After discussing this on IRC I took one more look at the code. We have something like this: devm_drm_dev_alloc() | +-> devm_drm_dev_init() | | | +-> drm_dev_init() | | | | | +- kref_init(&dev->ref) | | | | | +- drmm_add_action(drm_dev_init_release) | | | +-> devm_add_action(devm_drm_dev_init_release) | +-> drmm_add_final_kfree() drm_dev_init_release() - does all the release stuff devm_drm_dev_init_release() do a simple drm_dev_put() In other words we use the devres functionality to keep track of the reference count for the structure allocated by devm_drm_dev_alloc() So the naming makes some sense anyway. When there are no users left of drm_dev_init() outside drm_drv.c this can be simplified a little. After re-reading the code it made sense again. With the updated changelog: Reviewed-by: Sam Ravnborg <sam@ravnborg.org> PS. There is a few trivial checkpatch warnings to look at before pushing out. Sam > " > > > + * > > > + * 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 > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
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. + * + * 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 + * 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. 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);
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. 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(+)