Re: [PATCH 6/7] drm/nouveau: embed gem object in nouveau_bo

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Aug 14, 2013 at 11:07 PM, David Herrmann <dh.herrmann@xxxxxxxxx> 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@xxxxxxxxx>
> Cc: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxx>
> Cc: Martin Peres <martin.peres@xxxxxxxx>
> Signed-off-by: David Herrmann <dh.herrmann@xxxxxxxxx>
Reviewed-by: Ben Skeggs <bskeggs@xxxxxxxxxx>

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
>
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux