[PATCH 1/3] drm/vram: Provide vmap and vunmap operations for GEM VRAM objects

[Daniel Vetter]

On Wed, Jul 24, 2019 at 1:30 PM Thomas Zimmermann <tzimmermann at suse.de> wrote:
>
> The pattern of temporarily pinning and kmap-ing the BO's memory is
> common enough to justify helper functions that do and undo these
> operations.
>
> The implementation of vmap and vunmap for GEM VRAM helpers is
> already in PRIME helpers. The patch moves the operations to separate
> functions and exports them for general use.
>
> The patch also adds a note about possible kmap counting. So far this
> isn't required by drivers, but more complex use cases might make it
> necessary.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann at suse.de>
> ---
>  drivers/gpu/drm/drm_gem_vram_helper.c | 55 ++++++++++++++++++++++-----
>  include/drm/drm_gem_vram_helper.h     | 12 ++++++
>  2 files changed, 57 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c b/drivers/gpu/drm/drm_gem_vram_helper.c
> index e0fbfb6570cf..54758e4debca 100644
> --- a/drivers/gpu/drm/drm_gem_vram_helper.c
> +++ b/drivers/gpu/drm/drm_gem_vram_helper.c
> @@ -340,6 +340,48 @@ void drm_gem_vram_kunmap(struct drm_gem_vram_object *gbo)
>  }
>  EXPORT_SYMBOL(drm_gem_vram_kunmap);
>
> +/**
> + * drm_gem_vram_vmap() - Pins and maps a GEM VRAM object into kernel address
> +       space
> + * @gem:       The GEM VRAM object to map

variable names need to match, kernel-doc should be complaining here.

> + *
> + * The vmap function pins a GEM VRAM object to it's current location, either
> + * system or video memory, and maps it's buffer into kernel address space. As
> + * pinned object cannot be reloacted, you should not permanently pin objects.

Imo also good to point at the corresponding functions, i.e. reference
unmap here and in the unmap function reference the map function. And
please make sure the links work in the generated doc output.

> + *
> + * Returns:
> + * The buffer's virtual address on success, or
> + * an ERR_PTR()-encoded error code otherwise.
> + */
> +void *drm_gem_vram_vmap(struct drm_gem_vram_object *gbo)
> +{
> +       int ret;
> +       void *base;
> +
> +       ret = drm_gem_vram_pin(gbo, 0);
> +       if (ret)
> +               return ERR_PTR(ret);
> +       base = drm_gem_vram_kmap(gbo, true, NULL);
> +       if (IS_ERR(base)) {
> +               drm_gem_vram_unpin(gbo);
> +               return base;
> +       }
> +       return base;
> +}
> +EXPORT_SYMBOL(drm_gem_vram_vmap);
> +
> +/**
> + * drm_gem_vram_vunmap() - Unmaps and unpins a GEM VRAM object
> + * @gem:       The GEM VRAM object to unmap
> + * @vaddr:     The mapping's base address
> + */
> +void drm_gem_vram_vunmap(struct drm_gem_vram_object *gbo, void *vaddr)
> +{
> +       drm_gem_vram_kunmap(gbo);
> +       drm_gem_vram_unpin(gbo);
> +}
> +EXPORT_SYMBOL(drm_gem_vram_vunmap);
> +
>  /**
>   * drm_gem_vram_fill_create_dumb() - \
>         Helper for implementing &struct drm_driver.dumb_create
> @@ -595,17 +637,11 @@ static void drm_gem_vram_object_unpin(struct drm_gem_object *gem)
>  static void *drm_gem_vram_object_vmap(struct drm_gem_object *gem)
>  {
>         struct drm_gem_vram_object *gbo = drm_gem_vram_of_gem(gem);
> -       int ret;
>         void *base;
>
> -       ret = drm_gem_vram_pin(gbo, 0);
> -       if (ret)
> +       base = drm_gem_vram_vmap(gbo);
> +       if (IS_ERR(base))
>                 return NULL;
> -       base = drm_gem_vram_kmap(gbo, true, NULL);
> -       if (IS_ERR(base)) {
> -               drm_gem_vram_unpin(gbo);
> -               return NULL;
> -       }
>         return base;
>  }
>
> @@ -620,8 +656,7 @@ static void drm_gem_vram_object_vunmap(struct drm_gem_object *gem,
>  {
>         struct drm_gem_vram_object *gbo = drm_gem_vram_of_gem(gem);
>
> -       drm_gem_vram_kunmap(gbo);
> -       drm_gem_vram_unpin(gbo);
> +       drm_gem_vram_vunmap(gbo, vaddr);
>  }
>
>  /*
> diff --git a/include/drm/drm_gem_vram_helper.h b/include/drm/drm_gem_vram_helper.h
> index b41d932eb53a..5192c169cec2 100644
> --- a/include/drm/drm_gem_vram_helper.h
> +++ b/include/drm/drm_gem_vram_helper.h
> @@ -44,6 +44,16 @@ struct drm_gem_vram_object {
>         struct ttm_placement placement;
>         struct ttm_place placements[2];
>
> +       /* TODO: Maybe implement a map counter.

Kerneldoc is missing here. You can use the inline style so that the
todo comment here is still included.
-Daniel

> +        *
> +        * So far, drivers based on VRAM helpers don't have overlapping
> +        * mapping operations. A driver temporarily maps an object and
> +        * unmaps it ASAP. This works well for fbdev emulation or cursors.
> +        *
> +        * If we ever have a driver with buffer objects that are mapped
> +        * by multiple code fragments concurrently, we may need a map
> +        * counter to get the mapping right.
> +        */
>         int pin_count;
>  };
>
> @@ -84,6 +94,8 @@ int drm_gem_vram_unpin(struct drm_gem_vram_object *gbo);
>  void *drm_gem_vram_kmap(struct drm_gem_vram_object *gbo, bool map,
>                         bool *is_iomem);
>  void drm_gem_vram_kunmap(struct drm_gem_vram_object *gbo);
> +void *drm_gem_vram_vmap(struct drm_gem_vram_object *gbo);
> +void drm_gem_vram_vunmap(struct drm_gem_vram_object *gbo, void *vaddr);
>
>  int drm_gem_vram_fill_create_dumb(struct drm_file *file,
>                                   struct drm_device *dev,
> --
> 2.22.0
>


-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch