[Intel-gfx] [PATCH] drm: Update drm_device docs about embedding.

[Chris Wilson]

On Thu, Dec 08, 2016 at 12:53:07PM +0100, Daniel Vetter wrote:
> On Thu, Dec 08, 2016 at 10:50:48AM +0000, Chris Wilson wrote:
> > On Thu, Dec 08, 2016 at 11:28:47AM +0100, Daniel Vetter wrote:
> > > It's supported now! Spotted while reviewing Chris' patch to add a
> > > release hook.
> > > 
> > > Cc: Chris Wilson <chris at chris-wilson.co.uk>
> > > Signed-off-by: Daniel Vetter <daniel.vetter at intel.com>
> > > ---
> > >  drivers/gpu/drm/drm_drv.c | 11 +++++++----
> > >  1 file changed, 7 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> > > index f74b7d06ec01..4ec61ac27477 100644
> > > --- a/drivers/gpu/drm/drm_drv.c
> > > +++ b/drivers/gpu/drm/drm_drv.c
> > > @@ -323,9 +323,8 @@ void drm_minor_release(struct drm_minor *minor)
> > >   * historical baggage. Hence use the reference counting provided by
> > >   * drm_dev_ref() and drm_dev_unref() only carefully.
> > >   *
> > > - * Also note that embedding of &drm_device is currently not (yet) supported (but
> > > - * it would be easy to add). Drivers can store driver-private data in the
> > > - * dev_priv field of &drm_device.
> > > + * It is recommended that drivers embed struct &drm_device into their own device
> > > + * structure, which is supported through drm_dev_init().
> > >   */
> > >  
> > >  /**
> > > @@ -462,7 +461,11 @@ static void drm_fs_inode_free(struct inode *inode)
> > >   * Note that for purely virtual devices @parent can be NULL.
> > >   *
> > >   * Drivers that do not want to allocate their own device struct
> > > - * embedding struct &drm_device can call drm_dev_alloc() instead.
> > > + * embedding struct &drm_device can call drm_dev_alloc() instead. For drivers
> > > + * that do embed struct &drm_device it must be placed first in the overall
> > > + * structure, and the overall structure must be allocated using kmalloc(): The
> > > + * drm core's release function unconditionally calls kfree() on the @dev pointer
> > > + * when the final reference is released.
> > 
> > Hmm, the privates are getting pretty big (drm_i915_private fits inside
> > malloc-32678). We should start considering using drm_free_large() instead
> > as that more or less work transparently and allows fallback to vmalloc.
> 
> Yeah, I wondered whether your new ->release hook should be the very last
> thing in drm_dev_release, and that it would replace the final kfree. Then
> we could do custom stuff in drivers that have an oversized dev struct ;-)

Also considered that, placing it first helped with the onion of

dev = kmalloc()
drm_dev_init(dev, &driver);
i915_driver_init(dev);
...

So we would teardown i915, then drm, then free. For now saying that it
can either be kmalloc or vmalloc and having
if (is_virt) vfree() else kfree() should suffice. That is until we start
allocating new devices so regularly that they want a dedicated per-cpu
slab ;)
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre