Re: [PATCH 2/3] drm/vma_manage: Drop has_offset

[David Herrmann <dh.herrmann@gmail.com>]

Hi

On Thu, Oct 22, 2015 at 7:11 PM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> It's racy, creating mmap offsets is a slowpath, so better to remove it
> to avoid drivers doing broken things.
>
> The only user is i915, and it's ok there because everything (well
> almost) is protected by dev->struct_mutex in i915-gem.
>
> While at it add a note in the create_mmap_offset kerneldoc that
> drivers must release it again. And then I also noticed that
> drm_gem_object_release entirely lacks kerneldoc.
>
> Cc: David Herrmann <dh.herrmann@gmail.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>

I'd even argue that no driver should ever call into
drm_vma_offset_add() nor drm_vma_offset_remove(). For TTM this is
already the case, for plain old gem drivers still call
drm_vma_offset_add() (which is fine to me, but could be turned into a
gem helper).

Anyway, if TTM wasn't a module, we should drop the export of
drm_vma_offset_remove(), but that'll never happen, I guess.

Long story short: If anyone calls drm_vma_offset_remove() somewhere
but in the destructor, it's probably racy, so I fully support this
patch. The vma helpers are also made fail-safe on purpose, it has
never been a fast-path.

Reviewed-by: David Herrmann <dh.herrmann@gmail.com>

Thanks
David

> ---
>  drivers/gpu/drm/drm_gem.c       | 17 +++++++++++++++++
>  drivers/gpu/drm/i915/i915_gem.c |  3 ---
>  include/drm/drm_vma_manager.h   | 15 +--------------
>  3 files changed, 18 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> index 64353d40db53..38680380c6b3 100644
> --- a/drivers/gpu/drm/drm_gem.c
> +++ b/drivers/gpu/drm/drm_gem.c
> @@ -387,6 +387,10 @@ EXPORT_SYMBOL(drm_gem_handle_create);
>   * @obj: obj in question
>   *
>   * This routine frees fake offsets allocated by drm_gem_create_mmap_offset().
> + *
> + * Note that drm_gem_object_release() already calls this function, so drivers
> + * don't have to take care of releasing the mmap offset themselves when freeing
> + * the GEM object.
>   */
>  void
>  drm_gem_free_mmap_offset(struct drm_gem_object *obj)
> @@ -410,6 +414,9 @@ EXPORT_SYMBOL(drm_gem_free_mmap_offset);
>   * This routine allocates and attaches a fake offset for @obj, in cases where
>   * the virtual size differs from the physical size (ie. obj->size).  Otherwise
>   * just use drm_gem_create_mmap_offset().
> + *
> + * This function is idempotent and handles an already allocated mmap offset
> + * transparently. Drivers do not need to check for this case.
>   */
>  int
>  drm_gem_create_mmap_offset_size(struct drm_gem_object *obj, size_t size)
> @@ -431,6 +438,9 @@ EXPORT_SYMBOL(drm_gem_create_mmap_offset_size);
>   * structures.
>   *
>   * This routine allocates and attaches a fake offset for @obj.
> + *
> + * Drivers can call drm_gem_free_mmap_offset() before freeing @obj to release
> + * the fake offset again.
>   */
>  int drm_gem_create_mmap_offset(struct drm_gem_object *obj)
>  {
> @@ -739,6 +749,13 @@ drm_gem_release(struct drm_device *dev, struct drm_file *file_private)
>         idr_destroy(&file_private->object_idr);
>  }
>
> +/**
> + * drm_gem_object_release - release GEM buffer object resources
> + * @obj: GEM buffer object
> + *
> + * This releases any structures and resources used by @obj and is the invers of
> + * drm_gem_object_init().
> + */
>  void
>  drm_gem_object_release(struct drm_gem_object *obj)
>  {
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index d0fa5481543c..01fef54ecb2d 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -1972,9 +1972,6 @@ static int i915_gem_object_create_mmap_offset(struct drm_i915_gem_object *obj)
>         struct drm_i915_private *dev_priv = obj->base.dev->dev_private;
>         int ret;
>
> -       if (drm_vma_node_has_offset(&obj->base.vma_node))
> -               return 0;
> -
>         dev_priv->mm.shrinker_no_lock_stealing = true;
>
>         ret = drm_gem_create_mmap_offset(&obj->base);
> diff --git a/include/drm/drm_vma_manager.h b/include/drm/drm_vma_manager.h
> index 2f63dd5e05eb..06ea8e077ec2 100644
> --- a/include/drm/drm_vma_manager.h
> +++ b/include/drm/drm_vma_manager.h
> @@ -176,19 +176,6 @@ static inline unsigned long drm_vma_node_size(struct drm_vma_offset_node *node)
>  }
>
>  /**
> - * drm_vma_node_has_offset() - Check whether node is added to offset manager
> - * @node: Node to be checked
> - *
> - * RETURNS:
> - * true iff the node was previously allocated an offset and added to
> - * an vma offset manager.
> - */
> -static inline bool drm_vma_node_has_offset(struct drm_vma_offset_node *node)
> -{
> -       return drm_mm_node_allocated(&node->vm_node);
> -}
> -
> -/**
>   * drm_vma_node_offset_addr() - Return sanitized offset for user-space mmaps
>   * @node: Linked offset node
>   *
> @@ -220,7 +207,7 @@ static inline __u64 drm_vma_node_offset_addr(struct drm_vma_offset_node *node)
>  static inline void drm_vma_node_unmap(struct drm_vma_offset_node *node,
>                                       struct address_space *file_mapping)
>  {
> -       if (drm_vma_node_has_offset(node))
> +       if (drm_mm_node_allocated(&node->vm_node))
>                 unmap_mapping_range(file_mapping,
>                                     drm_vma_node_offset_addr(node),
>                                     drm_vma_node_size(node) << PAGE_SHIFT, 1);
> --
> 2.5.1
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel