Re: [Intel-gfx] [PATCH 52/52] drm: Add docs for managed resources
Hi Daniel. > What I miss in all of this is how do other subsystems deal > with the different lifetime of their stuff? > Or maybe only drm really has this issue? > Anything we could learn from others? Reading through the thread - this is all covered in more than sufficient details in other mails. So forget this comment. Sam ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 52/52] drm: Add docs for managed resources
Hi Daniel.
In general I think I could follow the documentation, this is good.
If the overall design is the best possible I cannot say,
but looks sane to me.
I like the drmm_ naming as a counterpart to devm_, each handling
resources with different lifetimes.
What I miss in all of this is how do other subsystems deal
with the different lifetime of their stuff?
Or maybe only drm really has this issue?
Anything we could learn from others?
Some more or less clueless comments in the following. Filter as usual.
And a bikeshedding detail.
"dev" is in my vocalubary a "struct device". So each time I see a "dev"
I think a struct device.
drm or ddev makes me think of struct drm_device - with the former being
my favorite.
Sam
> + * Note that any allocation or resource which is visible to userspace must be
> + * released only when the final drm_dev_put() is called, and not when the
> + * driver is unbound from the underlying physical struct Best to use
> + * _device managed resources with drmm_add_action(), drmm_kmalloc() and
> + * related functions.
Best to use => Maybe "It is recommended to use ..."
> + *
> + * devres managed resources like devm_kmalloc() can only be used for
> resources
> + * directly related to the underlying hardware device, and only used in code
> + * paths fully protected by drm_dev_enter() and drm_dev_exit().
Maybe this is obvious to others, but maybe add just a bit more about
what is required to be "fully protected". Or maybe this is already
documented somewhere else?
In drm_dev_enter() the term "critical section" is used.
I gues this is the section that is fully protected.
> diff --git a/drivers/gpu/drm/drm_managed.c b/drivers/gpu/drm/drm_managed.c
> index fb44fe65c2cd..7fcbe90d3f46 100644
> --- a/drivers/gpu/drm/drm_managed.c
> +++ b/drivers/gpu/drm/drm_managed.c
> @@ -17,10 +17,22 @@
> /**
> * DOC: managed resources
> *
> - * Inspired by sturct managed resources, but tied to the lifetime of
> + * Inspired by struct managed resources, but tied to the lifetime of
Maybe fix this when the text is added.
> +/**
> + * drmm_add_final_kfree - add release action for the final kfree()
> + * @dev: DRM device
> + * @data: pointer to the kmalloc allocation containing @dev
> + *
> + * Since the allocation containing the struct _device must be allocated
> + * before it can be initialized with drm_dev_init() there's no way to
> allocate
> + * that memory with drmm_kmalloc().
To side-step this chicken-egg problem the
> + * pointer for this final kfree() must be specified by calling this
> function. It
> + * will be released in the final drm_dev_put() for @dev, after all other
> release
> + * actions installed through drmm_add_action() have been processed.
Or maybe
Use drmm_add_final_kfree() to record that the allocation must be
released as the last step when the final drm_dev_put() for @dev is
called.
> + */
> void drmm_add_final_kfree(struct drm_device *dev, void *parent)
> {
> WARN_ON(dev->managed.final_kfree);
> @@ -132,6 +156,14 @@ int __drmm_add_action(struct drm_device *dev,
> }
> EXPORT_SYMBOL(__drmm_add_action);
>
> +/**
> + * drmm_add_action - remave a managed release action to a _device
s/remave/remove/
"action to a _device" => "action from a _device"?
> @@ -160,6 +192,16 @@ void drmm_remove_action(struct drm_device *dev,
> }
> EXPORT_SYMBOL(drmm_remove_action);
>
> +/**
> + * drmm_kmalloc - _device managed kmalloc()
> + * @dev: DRM device
> + * @size: size of the memory allocation
> + * @gfp: GFP allocation flags
> + *
> + * This is a _device managed version of kmalloc(). The allocated memory
> is
> + * automatically freed on the final drm_dev_put(). Memory can also be freed
> + * before the final drm_dev_put() by calling drmm_kfree().
RETURNS: ?
> + */
> void *drmm_kmalloc(struct drm_device *dev, size_t size, gfp_t gfp)
> {
> struct drmres *dr;
> @@ -175,6 +217,16 @@ void *drmm_kmalloc(struct drm_device *dev, size_t size,
> gfp_t gfp)
> }
> EXPORT_SYMBOL(drmm_kmalloc);
>
> +/**
> + * drmm_kstrdup - _device managed kstrdup()
> + * @dev: DRM device
> + * @size: 0 terminated string to be duplicated
> + * @gfp: GFP allocation flags
> + *
> + * This is a _device managed version of kstrdup(). The allocated memory
> is
> + * automatically freed on the final drm_dev_put() and works exactly like a
> + * memory allocation obtained by drmm_kmalloc().
RETURNS: ?
> + */
> char *drmm_kstrdup(struct drm_device *dev, const char *s, gfp_t gfp)
> {
> size_t size;
> @@ -191,6 +243,15 @@ char *drmm_kstrdup(struct drm_device *dev, const char
> *s, gfp_t gfp)
> }
> EXPORT_SYMBOL_GPL(drmm_kstrdup);
>
> diff --git a/include/drm/drm_managed.h b/include/drm/drm_managed.h
> index 573cadca4b3d..0e7616bd0858 100644
> --- a/include/drm/drm_managed.h
> +++ b/include/drm/drm_managed.h
> @@ -8,6 +8,19 @@ struct drm_device;
>
> typedef void (*drmres_release_t)(struct drm_device *dev, void *res);
>
> +/**
> + * drmm_add_action - add a
Re: [Intel-gfx] [PATCH 52/52] drm: Add docs for managed resources
On Wed, Feb 19, 2020 at 4:08 PM Laurent Pinchart wrote: > > Hi Daniel, > > Thank you for the patch. > > On Wed, Feb 19, 2020 at 11:21:22AM +0100, Daniel Vetter wrote: > > All collected together to provide a consistent story in one patch, > > instead of the somewhat bumpy refactor-evolution leading to this. > > > > Also some thoughts on what the next steps could be: > > > > - Create a macro called devm_drm_dev_alloc() which essentially wraps > > the kzalloc(); devm_drm_dev_init(); drmm_add_final_kfree() combo. > > Needs to be a macro since we'll have to do some typeof trickery and > > casting to make this fully generic for all drivers that embed struct > > drm_device into their own thing. > > Do you think it would be hard to do this already, in order to avoid > drmm_add_final_kfree() ? It's another 31 patches on top of this 52 patch series. Not hard per se, but there's a lot going on already here, and a lot of this is very tricky stuff that required endless amounts of reviewing everything. I'd like to do the more automated stuff as a follow-up if possible, I'm pretty sure I've not see a huge amount of exceptions from the normal pattern. Iirc vgem and vkms need a rework to invert their cleanup logic, so add 2 more patches for those. Everything else follows the standard pattern iirc. > > - A lot of the simple drivers now have essentially just > > drm_dev_unplug(); drm_atomic_helper_shutdown(); as their > > $bus_driver->remove hook. We could create a devm_mode_config_reset > > which sets drm_atomic_helper_shutdown as it's cleanup action, and a > > devm_drm_dev_register with drm_dev_unplug as it's cleanup action, > > and simple drivers wouldn't have a need for a ->remove function at > > all, and we could delete them. > > > > - For more complicated drivers we need drmm_ versions of a _lot_ more > > things. All the userspace visible objects (crtc, plane, encoder, > > crtc), anything else hanging of those (maybe a drmm_get_edid, at > > least for panels and other built-in stuff). > > I think it will get messy if we try to use the managed API for too many > things. Hm why? I kinda plan to use it for everything you'd want to clean up from drm_driver.release. > > Also some more thoughts on why we're not reusing devm_ with maybe a > > fake struct device embedded into the drm_device (we can't use the > > kdev, since that's in each drm_minor). > > > > - Code review gets extremely tricky, since every time you see a devm_ > > you need to carefully check whether the fake device (with the > > drm_device lifetim) or the real device (with the lifetim of the > > underlying physical device and driver binding) are used. That's not > > going to help at all, and we have enormous amounts of drivers who > > use devm_ where they really shouldn't. Having different types makes > > sure the compiler type checks this for us and ensures correctness. > > > > - The set of functions are very much non-overlapping. E.g. > > devm_ioremap makes total sense, drmm_ioremap has the wrong lifetime, > > since hw resources need to be cleaned out at driver unbind and wont > > outlive that like a drm_device. Similar, but other way round for > > drmm_connector_init (which is the only correct version, devm_ for > > drm_connector is just buggy). Simply not having the wrong version > > again prevents bugs. > > > > Finally I guess this opens a huge todo for all the drivers. I'm > > semi-tempted to do a tree-wide s/devm_kzalloc/drmm_kzalloc/ since most > > likely that'll fix an enormous amount of bugs and most likely not > > cause any issues at all (aside from maybe holding onto memory slightly > > too long). > > > > Signed-off-by: Daniel Vetter > > --- > > Documentation/gpu/drm-internals.rst | 6 +++ > > drivers/gpu/drm/drm_drv.c | 18 +++-- > > drivers/gpu/drm/drm_managed.c | 63 - > > include/drm/drm_drv.h | 4 ++ > > include/drm/drm_managed.h | 47 + > > 5 files changed, 134 insertions(+), 4 deletions(-) > > > > diff --git a/Documentation/gpu/drm-internals.rst > > b/Documentation/gpu/drm-internals.rst > > index a6b6145fda78..12272b168580 100644 > > --- a/Documentation/gpu/drm-internals.rst > > +++ b/Documentation/gpu/drm-internals.rst > > @@ -138,6 +138,12 @@ Managed Resources > > .. kernel-doc:: drivers/gpu/drm/drm_managed.c > > :doc: managed resources > > > > +.. kernel-doc:: drivers/gpu/drm/drm_managed.c > > + :export: > > + > > +.. kernel-doc:: include/drm/drm_managed.h > > + :internal: > > + > > Bus-specific Device Registration and PCI Support > > > > > > diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c > > index 428c569aaaf1..b1827ba53924 100644 > > --- a/drivers/gpu/drm/drm_drv.c > > +++ b/drivers/gpu/drm/drm_drv.c > > @@ -258,9 +258,15 @@ void drm_minor_release(struct drm_minor *minor) > > * any other resources allocated
Re: [Intel-gfx] [PATCH 52/52] drm: Add docs for managed resources
Hi Daniel, Thank you for the patch. On Wed, Feb 19, 2020 at 11:21:22AM +0100, Daniel Vetter wrote: > All collected together to provide a consistent story in one patch, > instead of the somewhat bumpy refactor-evolution leading to this. > > Also some thoughts on what the next steps could be: > > - Create a macro called devm_drm_dev_alloc() which essentially wraps > the kzalloc(); devm_drm_dev_init(); drmm_add_final_kfree() combo. > Needs to be a macro since we'll have to do some typeof trickery and > casting to make this fully generic for all drivers that embed struct > drm_device into their own thing. Do you think it would be hard to do this already, in order to avoid drmm_add_final_kfree() ? > - A lot of the simple drivers now have essentially just > drm_dev_unplug(); drm_atomic_helper_shutdown(); as their > $bus_driver->remove hook. We could create a devm_mode_config_reset > which sets drm_atomic_helper_shutdown as it's cleanup action, and a > devm_drm_dev_register with drm_dev_unplug as it's cleanup action, > and simple drivers wouldn't have a need for a ->remove function at > all, and we could delete them. > > - For more complicated drivers we need drmm_ versions of a _lot_ more > things. All the userspace visible objects (crtc, plane, encoder, > crtc), anything else hanging of those (maybe a drmm_get_edid, at > least for panels and other built-in stuff). I think it will get messy if we try to use the managed API for too many things. > Also some more thoughts on why we're not reusing devm_ with maybe a > fake struct device embedded into the drm_device (we can't use the > kdev, since that's in each drm_minor). > > - Code review gets extremely tricky, since every time you see a devm_ > you need to carefully check whether the fake device (with the > drm_device lifetim) or the real device (with the lifetim of the > underlying physical device and driver binding) are used. That's not > going to help at all, and we have enormous amounts of drivers who > use devm_ where they really shouldn't. Having different types makes > sure the compiler type checks this for us and ensures correctness. > > - The set of functions are very much non-overlapping. E.g. > devm_ioremap makes total sense, drmm_ioremap has the wrong lifetime, > since hw resources need to be cleaned out at driver unbind and wont > outlive that like a drm_device. Similar, but other way round for > drmm_connector_init (which is the only correct version, devm_ for > drm_connector is just buggy). Simply not having the wrong version > again prevents bugs. > > Finally I guess this opens a huge todo for all the drivers. I'm > semi-tempted to do a tree-wide s/devm_kzalloc/drmm_kzalloc/ since most > likely that'll fix an enormous amount of bugs and most likely not > cause any issues at all (aside from maybe holding onto memory slightly > too long). > > Signed-off-by: Daniel Vetter > --- > Documentation/gpu/drm-internals.rst | 6 +++ > drivers/gpu/drm/drm_drv.c | 18 +++-- > drivers/gpu/drm/drm_managed.c | 63 - > include/drm/drm_drv.h | 4 ++ > include/drm/drm_managed.h | 47 + > 5 files changed, 134 insertions(+), 4 deletions(-) > > diff --git a/Documentation/gpu/drm-internals.rst > b/Documentation/gpu/drm-internals.rst > index a6b6145fda78..12272b168580 100644 > --- a/Documentation/gpu/drm-internals.rst > +++ b/Documentation/gpu/drm-internals.rst > @@ -138,6 +138,12 @@ Managed Resources > .. kernel-doc:: drivers/gpu/drm/drm_managed.c > :doc: managed resources > > +.. kernel-doc:: drivers/gpu/drm/drm_managed.c > + :export: > + > +.. kernel-doc:: include/drm/drm_managed.h > + :internal: > + > Bus-specific Device Registration and PCI Support > > > diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c > index 428c569aaaf1..b1827ba53924 100644 > --- a/drivers/gpu/drm/drm_drv.c > +++ b/drivers/gpu/drm/drm_drv.c > @@ -258,9 +258,15 @@ void drm_minor_release(struct drm_minor *minor) > * any other resources allocated at device initialization and drop the > driver's > * reference to _device using drm_dev_put(). > * > - * Note that the lifetime rules for _device instance has still a lot of > - * historical baggage. Hence use the reference counting provided by > - * drm_dev_get() and drm_dev_put() only carefully. > + * Note that any allocation or resource which is visible to userspace must be > + * released only when the final drm_dev_put() is called, and not when the > + * driver is unbound from the underlying physical struct Best to use > + * _device managed resources with drmm_add_action(), drmm_kmalloc() and > + * related functions. > + * > + * devres managed resources like devm_kmalloc() can only be used for > resources > + * directly related to the underlying hardware device, and only used in code > + * paths fully
[Intel-gfx] [PATCH 52/52] drm: Add docs for managed resources
All collected together to provide a consistent story in one patch, instead of the somewhat bumpy refactor-evolution leading to this. Also some thoughts on what the next steps could be: - Create a macro called devm_drm_dev_alloc() which essentially wraps the kzalloc(); devm_drm_dev_init(); drmm_add_final_kfree() combo. Needs to be a macro since we'll have to do some typeof trickery and casting to make this fully generic for all drivers that embed struct drm_device into their own thing. - A lot of the simple drivers now have essentially just drm_dev_unplug(); drm_atomic_helper_shutdown(); as their $bus_driver->remove hook. We could create a devm_mode_config_reset which sets drm_atomic_helper_shutdown as it's cleanup action, and a devm_drm_dev_register with drm_dev_unplug as it's cleanup action, and simple drivers wouldn't have a need for a ->remove function at all, and we could delete them. - For more complicated drivers we need drmm_ versions of a _lot_ more things. All the userspace visible objects (crtc, plane, encoder, crtc), anything else hanging of those (maybe a drmm_get_edid, at least for panels and other built-in stuff). Also some more thoughts on why we're not reusing devm_ with maybe a fake struct device embedded into the drm_device (we can't use the kdev, since that's in each drm_minor). - Code review gets extremely tricky, since every time you see a devm_ you need to carefully check whether the fake device (with the drm_device lifetim) or the real device (with the lifetim of the underlying physical device and driver binding) are used. That's not going to help at all, and we have enormous amounts of drivers who use devm_ where they really shouldn't. Having different types makes sure the compiler type checks this for us and ensures correctness. - The set of functions are very much non-overlapping. E.g. devm_ioremap makes total sense, drmm_ioremap has the wrong lifetime, since hw resources need to be cleaned out at driver unbind and wont outlive that like a drm_device. Similar, but other way round for drmm_connector_init (which is the only correct version, devm_ for drm_connector is just buggy). Simply not having the wrong version again prevents bugs. Finally I guess this opens a huge todo for all the drivers. I'm semi-tempted to do a tree-wide s/devm_kzalloc/drmm_kzalloc/ since most likely that'll fix an enormous amount of bugs and most likely not cause any issues at all (aside from maybe holding onto memory slightly too long). Signed-off-by: Daniel Vetter --- Documentation/gpu/drm-internals.rst | 6 +++ drivers/gpu/drm/drm_drv.c | 18 +++-- drivers/gpu/drm/drm_managed.c | 63 - include/drm/drm_drv.h | 4 ++ include/drm/drm_managed.h | 47 + 5 files changed, 134 insertions(+), 4 deletions(-) diff --git a/Documentation/gpu/drm-internals.rst b/Documentation/gpu/drm-internals.rst index a6b6145fda78..12272b168580 100644 --- a/Documentation/gpu/drm-internals.rst +++ b/Documentation/gpu/drm-internals.rst @@ -138,6 +138,12 @@ Managed Resources .. kernel-doc:: drivers/gpu/drm/drm_managed.c :doc: managed resources +.. kernel-doc:: drivers/gpu/drm/drm_managed.c + :export: + +.. kernel-doc:: include/drm/drm_managed.h + :internal: + Bus-specific Device Registration and PCI Support diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c index 428c569aaaf1..b1827ba53924 100644 --- a/drivers/gpu/drm/drm_drv.c +++ b/drivers/gpu/drm/drm_drv.c @@ -258,9 +258,15 @@ void drm_minor_release(struct drm_minor *minor) * any other resources allocated at device initialization and drop the driver's * reference to _device using drm_dev_put(). * - * Note that the lifetime rules for _device instance has still a lot of - * historical baggage. Hence use the reference counting provided by - * drm_dev_get() and drm_dev_put() only carefully. + * Note that any allocation or resource which is visible to userspace must be + * released only when the final drm_dev_put() is called, and not when the + * driver is unbound from the underlying physical struct Best to use + * _device managed resources with drmm_add_action(), drmm_kmalloc() and + * related functions. + * + * devres managed resources like devm_kmalloc() can only be used for resources + * directly related to the underlying hardware device, and only used in code + * paths fully protected by drm_dev_enter() and drm_dev_exit(). * * Display driver example * ~~ @@ -604,6 +610,9 @@ static void drm_dev_init_release(struct drm_device *dev, void *res) * arbitrary offset, you must supply a _driver.release callback and control * the finalization explicitly. * + * Note that drivers must call drmm_add_final_kfree() after this function has + * completed successfully. + * * RETURNS: * 0 on success, or error code on
