| Message ID | 20161208102847.3063-1-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@chris-wilson.co.uk> > Signed-off-by: Daniel Vetter <daniel.vetter@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. As it stands the doc update is correct, so Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> -Chris
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@chris-wilson.co.uk> > > Signed-off-by: Daniel Vetter <daniel.vetter@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 ;-) > As it stands the doc update is correct, so > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> Thanks for the review, applied to drm-misc. -Daniel
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@chris-wilson.co.uk> > > > Signed-off-by: Daniel Vetter <daniel.vetter@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
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. * * RETURNS: * 0 on success, or error code on failure.
It's supported now! Spotted while reviewing Chris' patch to add a release hook. Cc: Chris Wilson <chris@chris-wilson.co.uk> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> --- drivers/gpu/drm/drm_drv.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-)