Re: [Intel-gfx] [PATCH 02/44] drm: Add devm_drm_dev_alloc macro

[Sam Ravnborg <sam@ravnborg.org>]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx