[01/59] drm: Add devm_drm_dev_alloc macro
diff mbox series

Message ID 20200415074034.175360-2-daniel.vetter@ffwll.ch
State New
Headers show
Series
  • devm_drm_dev_alloc, v2
Related show

Commit Message

Daniel Vetter April 15, 2020, 7:39 a.m. UTC
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.

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.

v2:
- Actually explain what this is for in the commit message (Sam)
- Fix checkpatch issues (Sam)

Acked-by: Noralf Trønnes <noralf@tronnes.org>
Cc: Noralf Trønnes <noralf@tronnes.org>
Reviewed-by: Sam Ravnborg <sam@ravnborg.org>
Cc: Sam Ravnborg <sam@ravnborg.org>
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

Thomas Zimmermann April 20, 2020, 1:36 p.m. UTC | #1
Hi

Am 15.04.20 um 09:39 schrieb Daniel Vetter:
> 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.
> 
> 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.
> 
> v2:
> - Actually explain what this is for in the commit message (Sam)
> - Fix checkpatch issues (Sam)
> 
> Acked-by: Noralf Trønnes <noralf@tronnes.org>
> Cc: Noralf Trønnes <noralf@tronnes.org>
> Reviewed-by: Sam Ravnborg <sam@ravnborg.org>
> Cc: Sam Ravnborg <sam@ravnborg.org>
> Cc: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>

Sorry for being late. A number of nits are listed below. In any case:

Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>

Best regards
Thomas

> ---
>  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..8e1813d2a12e 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)

Maybe rename 'offset' of 'dev_offset' to make the relationship clear.

> +{
> +	void *container;
> +	struct drm_device *drm;
> +	int ret;
> +
> +	container = kzalloc(size, GFP_KERNEL);
> +	if (!container)
> +		return ERR_PTR(-ENOMEM);
> +
> +	drm = container + offset;

While convenient, I somewhat dislike the use of void* variables. I'd use
unsigned char* for container and do an explicit cast to struct
drm_device* here.

> +	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..f07f15721254 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) \

I'd replace 'member' with 'dev_field' to make the relation ship clear.

> +	((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 21, 2020, 10:45 a.m. UTC | #2
On Mon, Apr 20, 2020 at 3:37 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>
> Hi
>
> Am 15.04.20 um 09:39 schrieb Daniel Vetter:
> > 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.
> >
> > 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.
> >
> > v2:
> > - Actually explain what this is for in the commit message (Sam)
> > - Fix checkpatch issues (Sam)
> >
> > Acked-by: Noralf Trønnes <noralf@tronnes.org>
> > Cc: Noralf Trønnes <noralf@tronnes.org>
> > Reviewed-by: Sam Ravnborg <sam@ravnborg.org>
> > Cc: Sam Ravnborg <sam@ravnborg.org>
> > Cc: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>

Thanks for taking a look, some questions on your suggestions below.

> Sorry for being late. A number of nits are listed below. In any case:
>
> Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>
>
> Best regards
> Thomas
>
> > ---
> >  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..8e1813d2a12e 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)
>
> Maybe rename 'offset' of 'dev_offset' to make the relationship clear.

Hm, I see the point of this (and the dev_field below, although I'd go
with dev_member there for some consistency with other macros using
offset_of or container_of), but I'm not sure about the dev_ prefix.
Drivers use that sometimes for the struct device *, and usage for
struct drm_device * is also very inconsistent. I've seen ddev, drm,
dev and base (that one only for embedded structs ofc). So not sure
which prefix to pick, aside from dev_ seems the most confusing. Got
ideas?

> > +{
> > +     void *container;
> > +     struct drm_device *drm;
> > +     int ret;
> > +
> > +     container = kzalloc(size, GFP_KERNEL);
> > +     if (!container)
> > +             return ERR_PTR(-ENOMEM);
> > +
> > +     drm = container + offset;
>
> While convenient, I somewhat dislike the use of void* variables. I'd use
> unsigned char* for container and do an explicit cast to struct
> drm_device* here.

I thought ever since C89 the explicit recommendation for untyped
pointer math has been void *, and no longer char *, with the spec
being explicit that void * pointer math works exactly like char *. So
not clear on why you think char * is preferred here. I'm also not
aware of any other kernel code that casts to char * for untyped
pointer math. So unless you have some supporting evidence, I'll skip
this one, ok?

Thanks, Daniel

> > +     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..f07f15721254 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) \
>
> I'd replace 'member' with 'dev_field' to make the relation ship clear.
>
> > +     ((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);
> >
>
> --
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Software Solutions Germany GmbH
> Maxfeldstr. 5, 90409 Nürnberg, Germany
> (HRB 36809, AG Nürnberg)
> Geschäftsführer: Felix Imendörffer
>
Thomas Zimmermann April 21, 2020, 2:03 p.m. UTC | #3
Hi

Am 21.04.20 um 12:45 schrieb Daniel Vetter:
> On Mon, Apr 20, 2020 at 3:37 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>>
>> Hi
>>
>> Am 15.04.20 um 09:39 schrieb Daniel Vetter:
>>> 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.
>>>
>>> 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.
>>>
>>> v2:
>>> - Actually explain what this is for in the commit message (Sam)
>>> - Fix checkpatch issues (Sam)
>>>
>>> Acked-by: Noralf Trønnes <noralf@tronnes.org>
>>> Cc: Noralf Trønnes <noralf@tronnes.org>
>>> Reviewed-by: Sam Ravnborg <sam@ravnborg.org>
>>> Cc: Sam Ravnborg <sam@ravnborg.org>
>>> Cc: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
>>> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> 
> Thanks for taking a look, some questions on your suggestions below.
> 
>> Sorry for being late. A number of nits are listed below. In any case:
>>
>> Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>
>>
>> Best regards
>> Thomas
>>
>>> ---
>>>  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..8e1813d2a12e 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)
>>
>> Maybe rename 'offset' of 'dev_offset' to make the relationship clear.
> 
> Hm, I see the point of this (and the dev_field below, although I'd go
> with dev_member there for some consistency with other macros using
> offset_of or container_of), but I'm not sure about the dev_ prefix.
> Drivers use that sometimes for the struct device *, and usage for
> struct drm_device * is also very inconsistent. I've seen ddev, drm,
> dev and base (that one only for embedded structs ofc). So not sure
> which prefix to pick, aside from dev_ seems the most confusing. Got
> ideas?

We have pdev for the PCI device, dev for the abstract device, and things
like mdev for struct mga_device in mgag200. So I'd go with ddev. I don't
like drm, because it could be anything in DRM. I guess struct drm_driver
is more 'drm' than struct drm_device.

But all of this is bikeshedding. It's probably best to keep the patch
as-is, and maybe rename variables later if we ever find consent on the
naming.

> 
>>> +{
>>> +     void *container;
>>> +     struct drm_device *drm;
>>> +     int ret;
>>> +
>>> +     container = kzalloc(size, GFP_KERNEL);
>>> +     if (!container)
>>> +             return ERR_PTR(-ENOMEM);
>>> +
>>> +     drm = container + offset;
>>
>> While convenient, I somewhat dislike the use of void* variables. I'd use
>> unsigned char* for container and do an explicit cast to struct
>> drm_device* here.
> 
> I thought ever since C89 the explicit recommendation for untyped
> pointer math has been void *, and no longer char *, with the spec
> being explicit that void * pointer math works exactly like char *. So

From how I understand the C spec, I think it's the other way around. I
had to look up the sections from the C11 spec:

Sec 6.5.6 Additive operators

 2 For addition, either both operands shall have arithmetic type, or
   one operand shall be a pointer to a complete object type and the
   other shall have integer type. (Incrementing is equivalent to adding
   1.)

About void it says that it's not a complete type.

Sec 6.2.5 Types

 19 The void type comprises an empty set of values; it is an incomplete
    object type that cannot be completed.

Arithmetic on void* is a gcc extension AFAIK.

> not clear on why you think char * is preferred here. I'm also not
> aware of any other kernel code that casts to char * for untyped
> pointer math. So unless you have some supporting evidence, I'll skip
> this one, ok?

I'm really just bikeshedding on things I'd have done differently.

Best regards
Thomas

> 
> Thanks, Daniel
> 
>>> +     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..f07f15721254 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) \
>>
>> I'd replace 'member' with 'dev_field' to make the relation ship clear.
>>
>>> +     ((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);
>>>
>>
>> --
>> Thomas Zimmermann
>> Graphics Driver Developer
>> SUSE Software Solutions Germany GmbH
>> Maxfeldstr. 5, 90409 Nürnberg, Germany
>> (HRB 36809, AG Nürnberg)
>> Geschäftsführer: Felix Imendörffer
>>
> 
>
Sam Ravnborg April 21, 2020, 8:32 p.m. UTC | #4
Hi

> > Hm, I see the point of this (and the dev_field below, although I'd go
> > with dev_member there for some consistency with other macros using
> > offset_of or container_of), but I'm not sure about the dev_ prefix.
> > Drivers use that sometimes for the struct device *, and usage for
> > struct drm_device * is also very inconsistent. I've seen ddev, drm,
> > dev and base (that one only for embedded structs ofc). So not sure
> > which prefix to pick, aside from dev_ seems the most confusing. Got
> > ideas?
> 
> We have pdev for the PCI device, dev for the abstract device, and things
> like mdev for struct mga_device in mgag200. So I'd go with ddev. I don't
> like drm, because it could be anything in DRM. I guess struct drm_driver
> is more 'drm' than struct drm_device.
> 
> But all of this is bikeshedding. It's probably best to keep the patch
> as-is, and maybe rename variables later if we ever find consent on the
> naming.

bikeshedding - I know.
But reading code is is quite natural for me that drm equals the central
drm_device data structure. Maybe thats because this was is in the code
I started looking at.

So as an example:

	drm_err(drm, "bla bla\n");

This parses nicely and is easy to type and get right.
And matches nicely that drm_device => drm.
But bikeshedding  - I will go to bed...
(Whatever is the conclusion we should not hold back the patch in
questions).

	Sam
Daniel Vetter April 28, 2020, 1:06 p.m. UTC | #5
On Tue, Apr 21, 2020 at 10:32:45PM +0200, Sam Ravnborg wrote:
> Hi
> 
> > > Hm, I see the point of this (and the dev_field below, although I'd go
> > > with dev_member there for some consistency with other macros using
> > > offset_of or container_of), but I'm not sure about the dev_ prefix.
> > > Drivers use that sometimes for the struct device *, and usage for
> > > struct drm_device * is also very inconsistent. I've seen ddev, drm,
> > > dev and base (that one only for embedded structs ofc). So not sure
> > > which prefix to pick, aside from dev_ seems the most confusing. Got
> > > ideas?
> > 
> > We have pdev for the PCI device, dev for the abstract device, and things
> > like mdev for struct mga_device in mgag200. So I'd go with ddev. I don't
> > like drm, because it could be anything in DRM. I guess struct drm_driver
> > is more 'drm' than struct drm_device.
> > 
> > But all of this is bikeshedding. It's probably best to keep the patch
> > as-is, and maybe rename variables later if we ever find consent on the
> > naming.
> 
> bikeshedding - I know.
> But reading code is is quite natural for me that drm equals the central
> drm_device data structure. Maybe thats because this was is in the code
> I started looking at.
> 
> So as an example:
> 
> 	drm_err(drm, "bla bla\n");
> 
> This parses nicely and is easy to type and get right.
> And matches nicely that drm_device => drm.
> But bikeshedding  - I will go to bed...
> (Whatever is the conclusion we should not hold back the patch in
> questions).

Ok, since we can't agree on dev vs ddev vs drm vs something else I just
left it as-is. We can always repaint this later on.

Thanks everyone for comments and review.
-Daniel

Patch
diff mbox series

diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 1bb4f636b83c..8e1813d2a12e 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..f07f15721254 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);