| Message ID | 1376485640-711-7-git-send-email-dh.herrmann@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Op 14-08-13 15:07, David Herrmann schreef: > There is no reason to keep the gem object separately allocated. nouveau is > the last user of gem_obj->driver_private, so if we embed it, we can get > rid of 8bytes per gem-object. > > The implementation follows the radeon driver. bo->gem is only valid, iff > the bo was created via the gem helpers _and_ iff the user holds a valid > gem reference. That is, as the gem object holds a reference to the > nouveau_bo. If you use nouveau_ref() to gain a bo reference, you are not > guaranteed to also hold a gem reference. The gem object might get > destroyed after the last user drops the gem-ref via > drm_gem_object_unreference(). Use drm_gem_object_reference() to gain a > gem-reference. > > For debugging, we can use bo->gem.filp != NULL to test whether a gem-bo is > valid. However, this shouldn't be used for real functionality to avoid > gem-internal dependencies. > > Note that the implementation follows the previous style. However, we no > longer can check for bo->gem != NULL to test for a valid gem object. This > wasn't done before, so we should be safe now. I'm all for getting rid of it, but I think it's not thorough enough. :P There's still a separate refcount in ttm for the bo, and I think it doesn't make sense to keep it like that. Instead of having that refcount in ttm, could it be put entirely in gem? ttm_buffer_object_transfer is the only time ttm creates a bo, and it's immediately destroyed, so instead of calling ttm_bo_unref it could call ttm_bo_release directly, in which case it doesn't matter that refcount is managed by gem. Oh except for vmwgfx of course, I always forget that 'special' case. Maybe something like this in memory? struct { struct ttm_buffer_object bo; struct drm_gem_object gem; // Can be anything, as long as first member is a refcount, and no hole between ttm and this.. }; This would allow ttm to do kref_put((kref_t*)(bo +1), driver->releasefn), where driver->releasefn has to call ttm_bo_release again. Unfortunately unless drm is fixed dma-buf is still going to be as buggy as before, not much I can do about that. :/ But this is something for a future where dma-buf in drm doesn't suck.. ~Maarten
Hi On Wed, Aug 14, 2013 at 4:31 PM, Maarten Lankhorst <maarten.lankhorst@canonical.com> wrote: > Op 14-08-13 15:07, David Herrmann schreef: >> There is no reason to keep the gem object separately allocated. nouveau is >> the last user of gem_obj->driver_private, so if we embed it, we can get >> rid of 8bytes per gem-object. >> >> The implementation follows the radeon driver. bo->gem is only valid, iff >> the bo was created via the gem helpers _and_ iff the user holds a valid >> gem reference. That is, as the gem object holds a reference to the >> nouveau_bo. If you use nouveau_ref() to gain a bo reference, you are not >> guaranteed to also hold a gem reference. The gem object might get >> destroyed after the last user drops the gem-ref via >> drm_gem_object_unreference(). Use drm_gem_object_reference() to gain a >> gem-reference. >> >> For debugging, we can use bo->gem.filp != NULL to test whether a gem-bo is >> valid. However, this shouldn't be used for real functionality to avoid >> gem-internal dependencies. >> >> Note that the implementation follows the previous style. However, we no >> longer can check for bo->gem != NULL to test for a valid gem object. This >> wasn't done before, so we should be safe now. > I'm all for getting rid of it, but I think it's not thorough enough. :P Does that mean you are in favor of this patch? Or do you want me to rework it right now for v2? Because I'd like to see this cleanup applied first so all TTM drivers work the same way. On top of that we can then do whatever we want regarding TTM refcounts. I am willing to spend some time on this. > There's still a separate refcount in ttm for the bo, and I think it doesn't make sense to keep it like that. > Instead of having that refcount in ttm, could it be put entirely in gem? > > ttm_buffer_object_transfer is the only time ttm creates a bo, and it's immediately destroyed, so instead > of calling ttm_bo_unref it could call ttm_bo_release directly, in which case it doesn't matter that refcount > is managed by gem. The bo_vm code also takes bo references. > Oh except for vmwgfx of course, I always forget that 'special' case. > > Maybe something like this in memory? > > struct { > struct ttm_buffer_object bo; > struct drm_gem_object gem; // Can be anything, as long as first member is a refcount, and no hole between ttm and this.. > }; Ugh.. This won't particularly win any beauty contest.. > This would allow ttm to do kref_put((kref_t*)(bo +1), driver->releasefn), where driver->releasefn has to call ttm_bo_release again. > Unfortunately unless drm is fixed dma-buf is still going to be as buggy as before, not much I can do about that. :/ > But this is something for a future where dma-buf in drm doesn't suck.. This same issue came up with the vma-access-management patches. I dislike adding hacks to TTM to allow accessing the surrounding GEM object only to keep TTM gem-agnostic. We could fix all this with a simple gem_object* pointer in the ttm-bo (or a flag indicating container_of is valid). Or we make gem TTM-aware by optionally embedding either a ttm_bo or a kref object. Hm. Ideas welcome! Regards David
On Thu, Aug 15, 2013 at 2:32 PM, David Herrmann <dh.herrmann@gmail.com> wrote: >> This would allow ttm to do kref_put((kref_t*)(bo +1), driver->releasefn), where driver->releasefn has to call ttm_bo_release again. >> Unfortunately unless drm is fixed dma-buf is still going to be as buggy as before, not much I can do about that. :/ >> But this is something for a future where dma-buf in drm doesn't suck.. > > This same issue came up with the vma-access-management patches. I > dislike adding hacks to TTM to allow accessing the surrounding GEM > object only to keep TTM gem-agnostic. We could fix all this with a > simple gem_object* pointer in the ttm-bo (or a flag indicating > container_of is valid). > > Or we make gem TTM-aware by optionally embedding either a ttm_bo or a > kref object. Hm. Ideas welcome! Imo embedding a gem bo into a ttm bo should be too harmful. I think we already have funny lifetime issues around ttm_bo->presistent_swap_storage, so this would imo be a win. Four downsides afaik: - gem shows up in drm/ttm/ ;-) - i915 gem crap like the read/write domains get splattered even more around. We could just move them to the i915 gem object struct and call it a day I think. - vmwgfx doesn't expose a gem interface. Imo this doesn't apply any more since we have support for driver private gem objects since a while. So shouldn't be too hard to do the final untangling and allow non-gem drivers to init gem objects, but not link them up anywhere. - gem_object_unref uses dev->struct_mutex. The last reason for that is the vma manager (which can be fixed to use the same kref_get_unless_zero trick ttm uses) and then the locking pushed down into drivers (probably needs a deferred free list to get going). Cheers, Daniel
Op 15-08-13 14:44, Daniel Vetter schreef: > On Thu, Aug 15, 2013 at 2:32 PM, David Herrmann <dh.herrmann@gmail.com> wrote: >>> This would allow ttm to do kref_put((kref_t*)(bo +1), driver->releasefn), where driver->releasefn has to call ttm_bo_release again. >>> Unfortunately unless drm is fixed dma-buf is still going to be as buggy as before, not much I can do about that. :/ >>> But this is something for a future where dma-buf in drm doesn't suck.. >> This same issue came up with the vma-access-management patches. I >> dislike adding hacks to TTM to allow accessing the surrounding GEM >> object only to keep TTM gem-agnostic. We could fix all this with a >> simple gem_object* pointer in the ttm-bo (or a flag indicating >> container_of is valid). >> >> Or we make gem TTM-aware by optionally embedding either a ttm_bo or a >> kref object. Hm. Ideas welcome! Oh original patch: Acked-by: Maarten Lankhorst <maarten.lankhorst@canonical.com> I think with this series the normal ttm refcount could be removed entirely. The only place in ttm that uses it is ttm_bo_mmap and ttm_bo_init. If struct vm_operations_struct is set by the driver the refcount isn't required in ttm any more at all. The other places in core ttm that use refcount now can be cleaned up. Radeon was already overriding the mmap ops for ttm, so this could now be done in a cleaner way. :-) That leaves vmwgfx of course, I'll probably kill ttm_bo_create because that will no longer work without refcount, and fixup their code. ttm_bo_io and ttm_bo_fbdev_io can be removed entirely, nothing uses it (I'll send a patch for it). > Imo embedding a gem bo into a ttm bo should be too harmful. I think we > already have funny lifetime issues around > ttm_bo->presistent_swap_storage, so this would imo be a win. Four > downsides afaik: > - gem shows up in drm/ttm/ ;-) > - i915 gem crap like the read/write domains get splattered even more > around. We could just move them to the i915 gem object struct and call > it a day I think. > - vmwgfx doesn't expose a gem interface. Imo this doesn't apply any > more since we have support for driver private gem objects since a > while. So shouldn't be too hard to do the final untangling and allow > non-gem drivers to init gem objects, but not link them up anywhere. > - gem_object_unref uses dev->struct_mutex. The last reason for that is > the vma manager (which can be fixed to use the same > kref_get_unless_zero trick ttm uses) and then the locking pushed down > into drivers (probably needs a deferred free list to get going). It's not required to move ttm to gem to kill the refcount. :-) ~Maarten
On Wed, Aug 14, 2013 at 11:07 PM, David Herrmann <dh.herrmann@gmail.com> wrote: > There is no reason to keep the gem object separately allocated. nouveau is > the last user of gem_obj->driver_private, so if we embed it, we can get > rid of 8bytes per gem-object. > > The implementation follows the radeon driver. bo->gem is only valid, iff > the bo was created via the gem helpers _and_ iff the user holds a valid > gem reference. That is, as the gem object holds a reference to the > nouveau_bo. If you use nouveau_ref() to gain a bo reference, you are not > guaranteed to also hold a gem reference. The gem object might get > destroyed after the last user drops the gem-ref via > drm_gem_object_unreference(). Use drm_gem_object_reference() to gain a > gem-reference. > > For debugging, we can use bo->gem.filp != NULL to test whether a gem-bo is > valid. However, this shouldn't be used for real functionality to avoid > gem-internal dependencies. > > Note that the implementation follows the previous style. However, we no > longer can check for bo->gem != NULL to test for a valid gem object. This > wasn't done before, so we should be safe now. > > Cc: Ben Skeggs <skeggsb@gmail.com> > Cc: Maarten Lankhorst <maarten.lankhorst@canonical.com> > Cc: Martin Peres <martin.peres@labri.fr> > Signed-off-by: David Herrmann <dh.herrmann@gmail.com> Reviewed-by: Ben Skeggs <bskeggs@redhat.com> Dave, are you taking this through your tree? Thanks David, Ben. > --- > drivers/gpu/drm/nouveau/nouveau_abi16.c | 4 ++-- > drivers/gpu/drm/nouveau/nouveau_bo.c | 2 +- > drivers/gpu/drm/nouveau/nouveau_bo.h | 5 ++++- > drivers/gpu/drm/nouveau/nouveau_display.c | 10 ++++----- > drivers/gpu/drm/nouveau/nouveau_fbcon.c | 2 +- > drivers/gpu/drm/nouveau/nouveau_gem.c | 36 +++++++++++++++---------------- > drivers/gpu/drm/nouveau/nouveau_gem.h | 2 +- > drivers/gpu/drm/nouveau/nouveau_prime.c | 10 +++++---- > 8 files changed, 37 insertions(+), 34 deletions(-) > > diff --git a/drivers/gpu/drm/nouveau/nouveau_abi16.c b/drivers/gpu/drm/nouveau/nouveau_abi16.c > index 8f467e7..3897549 100644 > --- a/drivers/gpu/drm/nouveau/nouveau_abi16.c > +++ b/drivers/gpu/drm/nouveau/nouveau_abi16.c > @@ -130,7 +130,7 @@ nouveau_abi16_chan_fini(struct nouveau_abi16 *abi16, > if (chan->ntfy) { > nouveau_bo_vma_del(chan->ntfy, &chan->ntfy_vma); > nouveau_bo_unpin(chan->ntfy); > - drm_gem_object_unreference_unlocked(chan->ntfy->gem); > + drm_gem_object_unreference_unlocked(&chan->ntfy->gem); > } > > if (chan->heap.block_size) > @@ -320,7 +320,7 @@ nouveau_abi16_ioctl_channel_alloc(ABI16_IOCTL_ARGS) > goto done; > } > > - ret = drm_gem_handle_create(file_priv, chan->ntfy->gem, > + ret = drm_gem_handle_create(file_priv, &chan->ntfy->gem, > &init->notifier_handle); > if (ret) > goto done; > diff --git a/drivers/gpu/drm/nouveau/nouveau_bo.c b/drivers/gpu/drm/nouveau/nouveau_bo.c > index 4e7ee5f..7767a89 100644 > --- a/drivers/gpu/drm/nouveau/nouveau_bo.c > +++ b/drivers/gpu/drm/nouveau/nouveau_bo.c > @@ -146,7 +146,7 @@ nouveau_bo_del_ttm(struct ttm_buffer_object *bo) > struct drm_device *dev = drm->dev; > struct nouveau_bo *nvbo = nouveau_bo(bo); > > - if (unlikely(nvbo->gem)) > + if (unlikely(nvbo->gem.filp)) > DRM_ERROR("bo %p still attached to GEM object\n", bo); > WARN_ON(nvbo->pin_refcnt > 0); > nv10_bo_put_tile_region(dev, nvbo->tile, NULL); > diff --git a/drivers/gpu/drm/nouveau/nouveau_bo.h b/drivers/gpu/drm/nouveau/nouveau_bo.h > index 653dbbb..ff17c1f 100644 > --- a/drivers/gpu/drm/nouveau/nouveau_bo.h > +++ b/drivers/gpu/drm/nouveau/nouveau_bo.h > @@ -27,7 +27,10 @@ struct nouveau_bo { > u32 tile_flags; > struct nouveau_drm_tile *tile; > > - struct drm_gem_object *gem; > + /* Only valid if allocated via nouveau_gem_new() and iff you hold a > + * gem reference to it! For debugging, use gem.filp != NULL to test > + * whether it is valid. */ > + struct drm_gem_object gem; > > /* protect by the ttm reservation lock */ > int pin_refcnt; > diff --git a/drivers/gpu/drm/nouveau/nouveau_display.c b/drivers/gpu/drm/nouveau/nouveau_display.c > index 78637af..85571ed 100644 > --- a/drivers/gpu/drm/nouveau/nouveau_display.c > +++ b/drivers/gpu/drm/nouveau/nouveau_display.c > @@ -50,7 +50,7 @@ nouveau_user_framebuffer_destroy(struct drm_framebuffer *drm_fb) > struct nouveau_framebuffer *fb = nouveau_framebuffer(drm_fb); > > if (fb->nvbo) > - drm_gem_object_unreference_unlocked(fb->nvbo->gem); > + drm_gem_object_unreference_unlocked(&fb->nvbo->gem); > > drm_framebuffer_cleanup(drm_fb); > kfree(fb); > @@ -63,7 +63,7 @@ nouveau_user_framebuffer_create_handle(struct drm_framebuffer *drm_fb, > { > struct nouveau_framebuffer *fb = nouveau_framebuffer(drm_fb); > > - return drm_gem_handle_create(file_priv, fb->nvbo->gem, handle); > + return drm_gem_handle_create(file_priv, &fb->nvbo->gem, handle); > } > > static const struct drm_framebuffer_funcs nouveau_framebuffer_funcs = { > @@ -668,8 +668,8 @@ nouveau_display_dumb_create(struct drm_file *file_priv, struct drm_device *dev, > if (ret) > return ret; > > - ret = drm_gem_handle_create(file_priv, bo->gem, &args->handle); > - drm_gem_object_unreference_unlocked(bo->gem); > + ret = drm_gem_handle_create(file_priv, &bo->gem, &args->handle); > + drm_gem_object_unreference_unlocked(&bo->gem); > return ret; > } > > @@ -682,7 +682,7 @@ nouveau_display_dumb_map_offset(struct drm_file *file_priv, > > gem = drm_gem_object_lookup(dev, file_priv, handle); > if (gem) { > - struct nouveau_bo *bo = gem->driver_private; > + struct nouveau_bo *bo = nouveau_gem_object(gem); > *poffset = drm_vma_node_offset_addr(&bo->bo.vma_node); > drm_gem_object_unreference_unlocked(gem); > return 0; > diff --git a/drivers/gpu/drm/nouveau/nouveau_fbcon.c b/drivers/gpu/drm/nouveau/nouveau_fbcon.c > index 4c1bc06..e20b695 100644 > --- a/drivers/gpu/drm/nouveau/nouveau_fbcon.c > +++ b/drivers/gpu/drm/nouveau/nouveau_fbcon.c > @@ -419,7 +419,7 @@ nouveau_fbcon_destroy(struct drm_device *dev, struct nouveau_fbdev *fbcon) > nouveau_bo_unmap(nouveau_fb->nvbo); > nouveau_bo_vma_del(nouveau_fb->nvbo, &nouveau_fb->vma); > nouveau_bo_unpin(nouveau_fb->nvbo); > - drm_gem_object_unreference_unlocked(nouveau_fb->nvbo->gem); > + drm_gem_object_unreference_unlocked(&nouveau_fb->nvbo->gem); > nouveau_fb->nvbo = NULL; > } > drm_fb_helper_fini(&fbcon->helper); > diff --git a/drivers/gpu/drm/nouveau/nouveau_gem.c b/drivers/gpu/drm/nouveau/nouveau_gem.c > index 487242f..39278e9 100644 > --- a/drivers/gpu/drm/nouveau/nouveau_gem.c > +++ b/drivers/gpu/drm/nouveau/nouveau_gem.c > @@ -43,20 +43,17 @@ nouveau_gem_object_new(struct drm_gem_object *gem) > void > nouveau_gem_object_del(struct drm_gem_object *gem) > { > - struct nouveau_bo *nvbo = gem->driver_private; > + struct nouveau_bo *nvbo = nouveau_gem_object(gem); > struct ttm_buffer_object *bo = &nvbo->bo; > > - if (!nvbo) > - return; > - nvbo->gem = NULL; > - > if (gem->import_attach) > drm_prime_gem_destroy(gem, nvbo->bo.sg); > > - ttm_bo_unref(&bo); > - > drm_gem_object_release(gem); > - kfree(gem); > + > + /* reset filp so nouveau_bo_del_ttm() can test for it */ > + gem->filp = NULL; > + ttm_bo_unref(&bo); > } > > int > @@ -186,14 +183,15 @@ nouveau_gem_new(struct drm_device *dev, int size, int align, uint32_t domain, > if (nv_device(drm->device)->card_type >= NV_50) > nvbo->valid_domains &= domain; > > - nvbo->gem = drm_gem_object_alloc(dev, nvbo->bo.mem.size); > - if (!nvbo->gem) { > + /* Initialize the embedded gem-object. We return a single gem-reference > + * to the caller, instead of a normal nouveau_bo ttm reference. */ > + ret = drm_gem_object_init(dev, &nvbo->gem, nvbo->bo.mem.size); > + if (ret) { > nouveau_bo_ref(NULL, pnvbo); > return -ENOMEM; > } > > - nvbo->bo.persistent_swap_storage = nvbo->gem->filp; > - nvbo->gem->driver_private = nvbo; > + nvbo->bo.persistent_swap_storage = nvbo->gem.filp; > return 0; > } > > @@ -250,15 +248,15 @@ nouveau_gem_ioctl_new(struct drm_device *dev, void *data, > if (ret) > return ret; > > - ret = drm_gem_handle_create(file_priv, nvbo->gem, &req->info.handle); > + ret = drm_gem_handle_create(file_priv, &nvbo->gem, &req->info.handle); > if (ret == 0) { > - ret = nouveau_gem_info(file_priv, nvbo->gem, &req->info); > + ret = nouveau_gem_info(file_priv, &nvbo->gem, &req->info); > if (ret) > drm_gem_handle_delete(file_priv, req->info.handle); > } > > /* drop reference from allocate - handle holds it now */ > - drm_gem_object_unreference_unlocked(nvbo->gem); > + drm_gem_object_unreference_unlocked(&nvbo->gem); > return ret; > } > > @@ -266,7 +264,7 @@ static int > nouveau_gem_set_domain(struct drm_gem_object *gem, uint32_t read_domains, > uint32_t write_domains, uint32_t valid_domains) > { > - struct nouveau_bo *nvbo = gem->driver_private; > + struct nouveau_bo *nvbo = nouveau_gem_object(gem); > struct ttm_buffer_object *bo = &nvbo->bo; > uint32_t domains = valid_domains & nvbo->valid_domains & > (write_domains ? write_domains : read_domains); > @@ -327,7 +325,7 @@ validate_fini_list(struct list_head *list, struct nouveau_fence *fence, > list_del(&nvbo->entry); > nvbo->reserved_by = NULL; > ttm_bo_unreserve_ticket(&nvbo->bo, ticket); > - drm_gem_object_unreference_unlocked(nvbo->gem); > + drm_gem_object_unreference_unlocked(&nvbo->gem); > } > } > > @@ -376,7 +374,7 @@ retry: > validate_fini(op, NULL); > return -ENOENT; > } > - nvbo = gem->driver_private; > + nvbo = nouveau_gem_object(gem); > if (nvbo == res_bo) { > res_bo = NULL; > drm_gem_object_unreference_unlocked(gem); > @@ -478,7 +476,7 @@ validate_list(struct nouveau_channel *chan, struct nouveau_cli *cli, > return ret; > } > > - ret = nouveau_gem_set_domain(nvbo->gem, b->read_domains, > + ret = nouveau_gem_set_domain(&nvbo->gem, b->read_domains, > b->write_domains, > b->valid_domains); > if (unlikely(ret)) { > diff --git a/drivers/gpu/drm/nouveau/nouveau_gem.h b/drivers/gpu/drm/nouveau/nouveau_gem.h > index 502e429..b535895 100644 > --- a/drivers/gpu/drm/nouveau/nouveau_gem.h > +++ b/drivers/gpu/drm/nouveau/nouveau_gem.h > @@ -12,7 +12,7 @@ > static inline struct nouveau_bo * > nouveau_gem_object(struct drm_gem_object *gem) > { > - return gem ? gem->driver_private : NULL; > + return gem ? container_of(gem, struct nouveau_bo, gem) : NULL; > } > > /* nouveau_gem.c */ > diff --git a/drivers/gpu/drm/nouveau/nouveau_prime.c b/drivers/gpu/drm/nouveau/nouveau_prime.c > index e90468d..51a2cb1 100644 > --- a/drivers/gpu/drm/nouveau/nouveau_prime.c > +++ b/drivers/gpu/drm/nouveau/nouveau_prime.c > @@ -71,14 +71,16 @@ struct drm_gem_object *nouveau_gem_prime_import_sg_table(struct drm_device *dev, > return ERR_PTR(ret); > > nvbo->valid_domains = NOUVEAU_GEM_DOMAIN_GART; > - nvbo->gem = drm_gem_object_alloc(dev, nvbo->bo.mem.size); > - if (!nvbo->gem) { > + > + /* Initialize the embedded gem-object. We return a single gem-reference > + * to the caller, instead of a normal nouveau_bo ttm reference. */ > + ret = drm_gem_object_init(dev, &nvbo->gem, nvbo->bo.mem.size); > + if (ret) { > nouveau_bo_ref(NULL, &nvbo); > return ERR_PTR(-ENOMEM); > } > > - nvbo->gem->driver_private = nvbo; > - return nvbo->gem; > + return &nvbo->gem; > } > > int nouveau_gem_prime_pin(struct drm_gem_object *obj) > -- > 1.8.3.4 >
diff --git a/drivers/gpu/drm/nouveau/nouveau_abi16.c b/drivers/gpu/drm/nouveau/nouveau_abi16.c index 8f467e7..3897549 100644 --- a/drivers/gpu/drm/nouveau/nouveau_abi16.c +++ b/drivers/gpu/drm/nouveau/nouveau_abi16.c @@ -130,7 +130,7 @@ nouveau_abi16_chan_fini(struct nouveau_abi16 *abi16, if (chan->ntfy) { nouveau_bo_vma_del(chan->ntfy, &chan->ntfy_vma); nouveau_bo_unpin(chan->ntfy); - drm_gem_object_unreference_unlocked(chan->ntfy->gem); + drm_gem_object_unreference_unlocked(&chan->ntfy->gem); } if (chan->heap.block_size) @@ -320,7 +320,7 @@ nouveau_abi16_ioctl_channel_alloc(ABI16_IOCTL_ARGS) goto done; } - ret = drm_gem_handle_create(file_priv, chan->ntfy->gem, + ret = drm_gem_handle_create(file_priv, &chan->ntfy->gem, &init->notifier_handle); if (ret) goto done; diff --git a/drivers/gpu/drm/nouveau/nouveau_bo.c b/drivers/gpu/drm/nouveau/nouveau_bo.c index 4e7ee5f..7767a89 100644 --- a/drivers/gpu/drm/nouveau/nouveau_bo.c +++ b/drivers/gpu/drm/nouveau/nouveau_bo.c @@ -146,7 +146,7 @@ nouveau_bo_del_ttm(struct ttm_buffer_object *bo) struct drm_device *dev = drm->dev; struct nouveau_bo *nvbo = nouveau_bo(bo); - if (unlikely(nvbo->gem)) + if (unlikely(nvbo->gem.filp)) DRM_ERROR("bo %p still attached to GEM object\n", bo); WARN_ON(nvbo->pin_refcnt > 0); nv10_bo_put_tile_region(dev, nvbo->tile, NULL); diff --git a/drivers/gpu/drm/nouveau/nouveau_bo.h b/drivers/gpu/drm/nouveau/nouveau_bo.h index 653dbbb..ff17c1f 100644 --- a/drivers/gpu/drm/nouveau/nouveau_bo.h +++ b/drivers/gpu/drm/nouveau/nouveau_bo.h @@ -27,7 +27,10 @@ struct nouveau_bo { u32 tile_flags; struct nouveau_drm_tile *tile; - struct drm_gem_object *gem; + /* Only valid if allocated via nouveau_gem_new() and iff you hold a + * gem reference to it! For debugging, use gem.filp != NULL to test + * whether it is valid. */ + struct drm_gem_object gem; /* protect by the ttm reservation lock */ int pin_refcnt; diff --git a/drivers/gpu/drm/nouveau/nouveau_display.c b/drivers/gpu/drm/nouveau/nouveau_display.c index 78637af..85571ed 100644 --- a/drivers/gpu/drm/nouveau/nouveau_display.c +++ b/drivers/gpu/drm/nouveau/nouveau_display.c @@ -50,7 +50,7 @@ nouveau_user_framebuffer_destroy(struct drm_framebuffer *drm_fb) struct nouveau_framebuffer *fb = nouveau_framebuffer(drm_fb); if (fb->nvbo) - drm_gem_object_unreference_unlocked(fb->nvbo->gem); + drm_gem_object_unreference_unlocked(&fb->nvbo->gem); drm_framebuffer_cleanup(drm_fb); kfree(fb); @@ -63,7 +63,7 @@ nouveau_user_framebuffer_create_handle(struct drm_framebuffer *drm_fb, { struct nouveau_framebuffer *fb = nouveau_framebuffer(drm_fb); - return drm_gem_handle_create(file_priv, fb->nvbo->gem, handle); + return drm_gem_handle_create(file_priv, &fb->nvbo->gem, handle); } static const struct drm_framebuffer_funcs nouveau_framebuffer_funcs = { @@ -668,8 +668,8 @@ nouveau_display_dumb_create(struct drm_file *file_priv, struct drm_device *dev, if (ret) return ret; - ret = drm_gem_handle_create(file_priv, bo->gem, &args->handle); - drm_gem_object_unreference_unlocked(bo->gem); + ret = drm_gem_handle_create(file_priv, &bo->gem, &args->handle); + drm_gem_object_unreference_unlocked(&bo->gem); return ret; } @@ -682,7 +682,7 @@ nouveau_display_dumb_map_offset(struct drm_file *file_priv, gem = drm_gem_object_lookup(dev, file_priv, handle); if (gem) { - struct nouveau_bo *bo = gem->driver_private; + struct nouveau_bo *bo = nouveau_gem_object(gem); *poffset = drm_vma_node_offset_addr(&bo->bo.vma_node); drm_gem_object_unreference_unlocked(gem); return 0; diff --git a/drivers/gpu/drm/nouveau/nouveau_fbcon.c b/drivers/gpu/drm/nouveau/nouveau_fbcon.c index 4c1bc06..e20b695 100644 --- a/drivers/gpu/drm/nouveau/nouveau_fbcon.c +++ b/drivers/gpu/drm/nouveau/nouveau_fbcon.c @@ -419,7 +419,7 @@ nouveau_fbcon_destroy(struct drm_device *dev, struct nouveau_fbdev *fbcon) nouveau_bo_unmap(nouveau_fb->nvbo); nouveau_bo_vma_del(nouveau_fb->nvbo, &nouveau_fb->vma); nouveau_bo_unpin(nouveau_fb->nvbo); - drm_gem_object_unreference_unlocked(nouveau_fb->nvbo->gem); + drm_gem_object_unreference_unlocked(&nouveau_fb->nvbo->gem); nouveau_fb->nvbo = NULL; } drm_fb_helper_fini(&fbcon->helper); diff --git a/drivers/gpu/drm/nouveau/nouveau_gem.c b/drivers/gpu/drm/nouveau/nouveau_gem.c index 487242f..39278e9 100644 --- a/drivers/gpu/drm/nouveau/nouveau_gem.c +++ b/drivers/gpu/drm/nouveau/nouveau_gem.c @@ -43,20 +43,17 @@ nouveau_gem_object_new(struct drm_gem_object *gem) void nouveau_gem_object_del(struct drm_gem_object *gem) { - struct nouveau_bo *nvbo = gem->driver_private; + struct nouveau_bo *nvbo = nouveau_gem_object(gem); struct ttm_buffer_object *bo = &nvbo->bo; - if (!nvbo) - return; - nvbo->gem = NULL; - if (gem->import_attach) drm_prime_gem_destroy(gem, nvbo->bo.sg); - ttm_bo_unref(&bo); - drm_gem_object_release(gem); - kfree(gem); + + /* reset filp so nouveau_bo_del_ttm() can test for it */ + gem->filp = NULL; + ttm_bo_unref(&bo); } int @@ -186,14 +183,15 @@ nouveau_gem_new(struct drm_device *dev, int size, int align, uint32_t domain, if (nv_device(drm->device)->card_type >= NV_50) nvbo->valid_domains &= domain; - nvbo->gem = drm_gem_object_alloc(dev, nvbo->bo.mem.size); - if (!nvbo->gem) { + /* Initialize the embedded gem-object. We return a single gem-reference + * to the caller, instead of a normal nouveau_bo ttm reference. */ + ret = drm_gem_object_init(dev, &nvbo->gem, nvbo->bo.mem.size); + if (ret) { nouveau_bo_ref(NULL, pnvbo); return -ENOMEM; } - nvbo->bo.persistent_swap_storage = nvbo->gem->filp; - nvbo->gem->driver_private = nvbo; + nvbo->bo.persistent_swap_storage = nvbo->gem.filp; return 0; } @@ -250,15 +248,15 @@ nouveau_gem_ioctl_new(struct drm_device *dev, void *data, if (ret) return ret; - ret = drm_gem_handle_create(file_priv, nvbo->gem, &req->info.handle); + ret = drm_gem_handle_create(file_priv, &nvbo->gem, &req->info.handle); if (ret == 0) { - ret = nouveau_gem_info(file_priv, nvbo->gem, &req->info); + ret = nouveau_gem_info(file_priv, &nvbo->gem, &req->info); if (ret) drm_gem_handle_delete(file_priv, req->info.handle); } /* drop reference from allocate - handle holds it now */ - drm_gem_object_unreference_unlocked(nvbo->gem); + drm_gem_object_unreference_unlocked(&nvbo->gem); return ret; } @@ -266,7 +264,7 @@ static int nouveau_gem_set_domain(struct drm_gem_object *gem, uint32_t read_domains, uint32_t write_domains, uint32_t valid_domains) { - struct nouveau_bo *nvbo = gem->driver_private; + struct nouveau_bo *nvbo = nouveau_gem_object(gem); struct ttm_buffer_object *bo = &nvbo->bo; uint32_t domains = valid_domains & nvbo->valid_domains & (write_domains ? write_domains : read_domains); @@ -327,7 +325,7 @@ validate_fini_list(struct list_head *list, struct nouveau_fence *fence, list_del(&nvbo->entry); nvbo->reserved_by = NULL; ttm_bo_unreserve_ticket(&nvbo->bo, ticket); - drm_gem_object_unreference_unlocked(nvbo->gem); + drm_gem_object_unreference_unlocked(&nvbo->gem); } } @@ -376,7 +374,7 @@ retry: validate_fini(op, NULL); return -ENOENT; } - nvbo = gem->driver_private; + nvbo = nouveau_gem_object(gem); if (nvbo == res_bo) { res_bo = NULL; drm_gem_object_unreference_unlocked(gem); @@ -478,7 +476,7 @@ validate_list(struct nouveau_channel *chan, struct nouveau_cli *cli, return ret; } - ret = nouveau_gem_set_domain(nvbo->gem, b->read_domains, + ret = nouveau_gem_set_domain(&nvbo->gem, b->read_domains, b->write_domains, b->valid_domains); if (unlikely(ret)) { diff --git a/drivers/gpu/drm/nouveau/nouveau_gem.h b/drivers/gpu/drm/nouveau/nouveau_gem.h index 502e429..b535895 100644 --- a/drivers/gpu/drm/nouveau/nouveau_gem.h +++ b/drivers/gpu/drm/nouveau/nouveau_gem.h @@ -12,7 +12,7 @@ static inline struct nouveau_bo * nouveau_gem_object(struct drm_gem_object *gem) { - return gem ? gem->driver_private : NULL; + return gem ? container_of(gem, struct nouveau_bo, gem) : NULL; } /* nouveau_gem.c */ diff --git a/drivers/gpu/drm/nouveau/nouveau_prime.c b/drivers/gpu/drm/nouveau/nouveau_prime.c index e90468d..51a2cb1 100644 --- a/drivers/gpu/drm/nouveau/nouveau_prime.c +++ b/drivers/gpu/drm/nouveau/nouveau_prime.c @@ -71,14 +71,16 @@ struct drm_gem_object *nouveau_gem_prime_import_sg_table(struct drm_device *dev, return ERR_PTR(ret); nvbo->valid_domains = NOUVEAU_GEM_DOMAIN_GART; - nvbo->gem = drm_gem_object_alloc(dev, nvbo->bo.mem.size); - if (!nvbo->gem) { + + /* Initialize the embedded gem-object. We return a single gem-reference + * to the caller, instead of a normal nouveau_bo ttm reference. */ + ret = drm_gem_object_init(dev, &nvbo->gem, nvbo->bo.mem.size); + if (ret) { nouveau_bo_ref(NULL, &nvbo); return ERR_PTR(-ENOMEM); } - nvbo->gem->driver_private = nvbo; - return nvbo->gem; + return &nvbo->gem; } int nouveau_gem_prime_pin(struct drm_gem_object *obj)
There is no reason to keep the gem object separately allocated. nouveau is the last user of gem_obj->driver_private, so if we embed it, we can get rid of 8bytes per gem-object. The implementation follows the radeon driver. bo->gem is only valid, iff the bo was created via the gem helpers _and_ iff the user holds a valid gem reference. That is, as the gem object holds a reference to the nouveau_bo. If you use nouveau_ref() to gain a bo reference, you are not guaranteed to also hold a gem reference. The gem object might get destroyed after the last user drops the gem-ref via drm_gem_object_unreference(). Use drm_gem_object_reference() to gain a gem-reference. For debugging, we can use bo->gem.filp != NULL to test whether a gem-bo is valid. However, this shouldn't be used for real functionality to avoid gem-internal dependencies. Note that the implementation follows the previous style. However, we no longer can check for bo->gem != NULL to test for a valid gem object. This wasn't done before, so we should be safe now. Cc: Ben Skeggs <skeggsb@gmail.com> Cc: Maarten Lankhorst <maarten.lankhorst@canonical.com> Cc: Martin Peres <martin.peres@labri.fr> Signed-off-by: David Herrmann <dh.herrmann@gmail.com> --- drivers/gpu/drm/nouveau/nouveau_abi16.c | 4 ++-- drivers/gpu/drm/nouveau/nouveau_bo.c | 2 +- drivers/gpu/drm/nouveau/nouveau_bo.h | 5 ++++- drivers/gpu/drm/nouveau/nouveau_display.c | 10 ++++----- drivers/gpu/drm/nouveau/nouveau_fbcon.c | 2 +- drivers/gpu/drm/nouveau/nouveau_gem.c | 36 +++++++++++++++---------------- drivers/gpu/drm/nouveau/nouveau_gem.h | 2 +- drivers/gpu/drm/nouveau/nouveau_prime.c | 10 +++++---- 8 files changed, 37 insertions(+), 34 deletions(-)