Re: [PATCH 4/5] drm: Add support for subclassing struct drm_device

[Daniel Vetter <daniel@ffwll.ch>]

On Wed, Jan 08, 2014 at 08:01:19PM +0100, David Herrmann wrote:
> Hi
> 
> On Wed, Jan 8, 2014 at 7:31 PM, Damien Lespiau <damien.lespiau@intel.com> wrote:
> > Currently, drivers are expected to allocate private data and attach it
> > to dev_private in struct drm_device.
> >
> > This has the unfortunate property to require driver code to juggle
> > between the pointer to struct drm_device and dev->dev_private instead of
> > using the same pointer if they could embed the device structure.
> >
> > This patch enables drivers to declare the size of the device structure
> > they want DRM core to create for them.
> >
> > Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
> > ---
> >  drivers/gpu/drm/drm_stub.c | 8 +++++++-
> >  include/drm/drmP.h         | 8 ++++++++
> >  2 files changed, 15 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c
> > index 98a33c580..161dd9a 100644
> > --- a/drivers/gpu/drm/drm_stub.c
> > +++ b/drivers/gpu/drm/drm_stub.c
> > @@ -433,8 +433,14 @@ struct drm_device *drm_dev_alloc(struct drm_driver *driver,
> >  {
> >         struct drm_device *dev;
> >         int ret;
> > +       size_t device_struct_size;
> >
> > -       dev = kzalloc(sizeof(*dev), GFP_KERNEL);
> > +       if (driver->device_struct_size)
> > +               device_struct_size = driver->device_struct_size;
> > +       else
> > +               device_struct_size = sizeof(*dev);
> 
> How about a:
> WARN_ON(driver->device_struct_size < sizeof(*dev))
> 
> > +
> > +       dev = kzalloc(device_struct_size, GFP_KERNEL);
> 
> So the parent structure is expected to have "struct drm_device" at
> offset 0? I'd rather like to see a "drm_dev_init()" alongside
> drm_dev_alloc() similar to device_initialize().

Yeah, I think for subclassing we want drivers in charge to kmalloc the
entire thing and embedded struct drm_device wherever they please to do so.
Adding struct_size stuff all over the place still forces us through the
midlayer ...

I'm trying to get there with my giant drm cleanup series (which contains
some of the same dev_priv_size cleanups like yours). Dunno whether it's
worth all to much to start embedding before we have that all ready since
imo the big value in demidlayering is that it allows us to fix up the
init/teardown sequence. That it also allows struct drm_device embedding is
kinda neat, but not my main goal.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch