| Message ID | 1389205912-16632-5-git-send-email-damien.lespiau@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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(). Thanks David > if (!dev) > return NULL; > > diff --git a/include/drm/drmP.h b/include/drm/drmP.h > index 6800c20..219b153 100644 > --- a/include/drm/drmP.h > +++ b/include/drm/drmP.h > @@ -996,6 +996,14 @@ struct drm_driver { > u32 driver_features; > /* size of the private data attached to a struct drm_buf */ > int buf_priv_size; > + /* > + * DRM drivers can subclass struct drm_device to have their own device > + * structure to store private data. In this case, they need to declare > + * the size of the child structure (ie the structure embedding a struct > + * drm_device as first field) for the DRM core to allocate a big > + * enough device structure. > + */ > + size_t device_struct_size; > const struct drm_ioctl_desc *ioctls; > int num_ioctls; > const struct file_operations *fops; > -- > 1.8.3.1 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
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
On Wed, Jan 08, 2014 at 09:26:51PM +0100, Daniel Vetter wrote: > > 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 I'm not sure why would people want struct drm_device at a non-0 offset, but in any case, if Daniel is already looking into this, let's scrap that series. At least we know that it doesn't have to be a long term plan and we can do it as soon as we want.
Hi On Thu, Jan 9, 2014 at 1:11 PM, Damien Lespiau <damien.lespiau@intel.com> wrote: > On Wed, Jan 08, 2014 at 09:26:51PM +0100, Daniel Vetter wrote: >> > 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 > > I'm not sure why would people want struct drm_device at a non-0 offset, > but in any case, if Daniel is already looking into this, let's scrap > that series. At least we know that it doesn't have to be a long term > plan and we can do it as soon as we want. In case you embed multiple objects in your parent device and both have this requirement, you're screwed. Obviously, this only works if at most one of them has a ref-count, but see for instance the gem+ttm combinations, which embed both, gem_object and ttm_bo in their private bo. Besides, it's more about clean code here than functionality. The code doesn't get more complicated if we remove the restriction so lets use the cleaner way similar to "struct device". Thanks David
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); + + dev = kzalloc(device_struct_size, GFP_KERNEL); if (!dev) return NULL; diff --git a/include/drm/drmP.h b/include/drm/drmP.h index 6800c20..219b153 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -996,6 +996,14 @@ struct drm_driver { u32 driver_features; /* size of the private data attached to a struct drm_buf */ int buf_priv_size; + /* + * DRM drivers can subclass struct drm_device to have their own device + * structure to store private data. In this case, they need to declare + * the size of the child structure (ie the structure embedding a struct + * drm_device as first field) for the DRM core to allocate a big + * enough device structure. + */ + size_t device_struct_size; const struct drm_ioctl_desc *ioctls; int num_ioctls; const struct file_operations *fops;
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(-)