[02/44] drm: Add devm_drm_dev_alloc macro
diff mbox series

Message ID 20200403135828.2542770-3-daniel.vetter@ffwll.ch
State New
Headers show
Series
  • devm_drm_dev_alloc, no more drmm_add_final_kfree
Related show

Commit Message

Daniel Vetter April 3, 2020, 1:57 p.m. UTC
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(+)

Comments

Noralf Trønnes April 5, 2020, 10:20 a.m. UTC | #1
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>
Laurent Pinchart April 6, 2020, noon UTC | #2
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);
Daniel Vetter April 6, 2020, 12:18 p.m. UTC | #3
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
Sam Ravnborg April 8, 2020, 6:57 a.m. UTC | #4
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
Daniel Vetter April 8, 2020, 12:11 p.m. UTC | #5
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
Sam Ravnborg April 8, 2020, 6:55 p.m. UTC | #6
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

Patch
diff mbox series

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);