[v3,01/19] drm: Add |struct drm_gem_vram_object| and helpers

Commit Message

Thomas Zimmermann April 29, 2019, 2:43 p.m. UTC

The type |struct drm_gem_vram_object| implements a GEM object for simple
framebuffer devices with dedicated video memory. The BO is either located
in VRAM or system memory.

The implementation has been created from the respective code in ast,
bochs and mgag200. These drivers copy their implementation from each
other; except for the names of several data types. The helpers are
currently build with TTM, but this is considered an implementation
detail and may change in future updates.

v2:
	* rename to |struct drm_gem_vram_object|
	* move drm_is_gem_ttm() to a later patch in the series
	* add drm_gem_vram_kmap_at()
	* return is_iomem from kmap functions
	* redefine TTM placement flags for public interface
	* documentation fixes

Signed-off-by: Thomas Zimmermann <[e-mail]>
---
 Documentation/gpu/drm-mm.rst             |  12 +
 drivers/gpu/drm/Kconfig                  |  13 +
 drivers/gpu/drm/Makefile                 |   4 +
 drivers/gpu/drm/drm_gem_vram_helper.c    | 410 +++++++++++++++++++++++
 drivers/gpu/drm/drm_vram_helper_common.c |   6 +
 include/drm/drm_gem_vram_helper.h        |  92 +++++
 6 files changed, 537 insertions(+)
 create mode 100644 drivers/gpu/drm/drm_gem_vram_helper.c
 create mode 100644 drivers/gpu/drm/drm_vram_helper_common.c
 create mode 100644 include/drm/drm_gem_vram_helper.h

Comments

Sam Ravnborg April 29, 2019, 7:58 p.m. UTC | #1

Hi Thomas.

Some minor things and some bikeshedding too.

One general^Wbikeshedding thing - unint32_t is used in many places.
And then s64 in one place.
Seems like two concepts are mixed.
Maybe be consistent and use u32, s32 everywhere?

I did not read the code carefully enough to understand it.
I cannot give a r-b or a-b - as I feel I need to understand it to do
so.

	Sam

On Mon, Apr 29, 2019 at 04:43:23PM +0200, Thomas Zimmermann wrote:
> The type |struct drm_gem_vram_object| implements a GEM object for simple
> framebuffer devices with dedicated video memory. The BO is either located
> in VRAM or system memory.
> 
> The implementation has been created from the respective code in ast,
> bochs and mgag200. These drivers copy their implementation from each
> other; except for the names of several data types. The helpers are
> currently build with TTM, but this is considered an implementation
> detail and may change in future updates.
> 
> v2:
> 	* rename to |struct drm_gem_vram_object|
> 	* move drm_is_gem_ttm() to a later patch in the series
> 	* add drm_gem_vram_kmap_at()
> 	* return is_iomem from kmap functions
> 	* redefine TTM placement flags for public interface
> 	* documentation fixes
> 
> Signed-off-by: Thomas Zimmermann <[e-mail]>
> ---
>  Documentation/gpu/drm-mm.rst             |  12 +
>  drivers/gpu/drm/Kconfig                  |  13 +
>  drivers/gpu/drm/Makefile                 |   4 +
>  drivers/gpu/drm/drm_gem_vram_helper.c    | 410 +++++++++++++++++++++++
>  drivers/gpu/drm/drm_vram_helper_common.c |   6 +
>  include/drm/drm_gem_vram_helper.h        |  92 +++++
>  6 files changed, 537 insertions(+)
>  create mode 100644 drivers/gpu/drm/drm_gem_vram_helper.c
>  create mode 100644 drivers/gpu/drm/drm_vram_helper_common.c
>  create mode 100644 include/drm/drm_gem_vram_helper.h
> 
> diff --git a/Documentation/gpu/drm-mm.rst b/Documentation/gpu/drm-mm.rst
> index 54a696d961a7..d5327ed608d7 100644
> --- a/Documentation/gpu/drm-mm.rst
> +++ b/Documentation/gpu/drm-mm.rst
> @@ -380,6 +380,18 @@ GEM CMA Helper Functions Reference
>  .. kernel-doc:: drivers/gpu/drm/drm_gem_cma_helper.c
>     :export:
>  
> +GEM VRAM Helper Functions Reference
> +----------------------------------
> +
> +.. kernel-doc:: drivers/gpu/drm/drm_gem_vram_helper.c
> +   :doc: overview
> +
> +.. kernel-doc:: include/drm/drm_gem_vram_helper.h
> +   :internal:
> +
> +.. kernel-doc:: drivers/gpu/drm/drm_gem_vram_helper.c
> +   :export:
> +
>  VMA Offset Manager
>  ==================
>  
> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> index 2267e84d5cb4..c0d49a6c09d2 100644
> --- a/drivers/gpu/drm/Kconfig
> +++ b/drivers/gpu/drm/Kconfig
> @@ -160,6 +160,12 @@ config DRM_TTM
>  	  GPU memory types. Will be enabled automatically if a device driver
>  	  uses it.
>  
> +config DRM_VRAM_HELPER
> +	tristate
> +	depends on DRM && DRM_TTM
> +	help
> +	  Helpers for VRAM memory management
> +
>  config DRM_GEM_CMA_HELPER
>  	bool
>  	depends on DRM
> @@ -179,6 +185,13 @@ config DRM_GEM_SHMEM_HELPER
>  	help
>  	  Choose this if you need the GEM shmem helper functions
>  
> +config DRM_GEM_VRAM_HELPER
> +	bool
> +	depends on DRM
> +	select DRM_VRAM_HELPER
> +	help
> +	  Choose this if you need the GEM VRAM helper functions
> +
I cannot remember how select will deal with symbols whos has
a  "depends on".
But if I recall correct then the "depends on" will be ignored
or in best case trigger a warning.
In other words - when we have symbols we select they should not
have a depends on.

What can be done is something like:

symbol foo
	bool

symbol bar
	depends on foo


symbol foobar
	bool "This is what you need - select me"
	select foo

So when one chooses "foobar" then we will select "foo" and this will
satisfy bar.

But maybe this rambling is irrelevant - maybe check what we do with
other selectable symbols in DRM.


> +/**
> + * DOC: overview
> + *
> + * This library provides a GEM object that is backed by VRAM. It
> + * can be used for simple framebuffer devices with dedicated memory.
> + */
It is likely only me, but...
I could use a short explanation what is GEM and maybe also VRAM.

VRAM as video RAM, but maybe there is more constraints?
(When I first looked at DRM I wondered what you used Virtual RAM for.
But thats a long time ago so it counts only as a funny story.

> + * Buffer-objects helpers
> + */
> +
> +static void drm_gem_vram_cleanup(struct drm_gem_vram_object *gbo)
> +{
> +	/* We got here via ttm_bo_put(), which means that the
> +	 * TTM buffer object in 'bo' has already been cleaned
> +	 * up; only release the GEM object. */
> +	drm_gem_object_release(&gbo->gem);
> +}
> +
> +static void drm_gem_vram_destroy(struct drm_gem_vram_object *gbo)
> +{
> +	drm_gem_vram_cleanup(gbo);
> +	kfree(gbo);
> +}
> +
> +static void ttm_buffer_object_destroy(struct ttm_buffer_object *bo)
> +{
> +	struct drm_gem_vram_object *gbo = drm_gem_vram_of_bo(bo);
> +	drm_gem_vram_destroy(gbo);
> +}
> +
> +static void drm_gem_vram_placement(struct drm_gem_vram_object *gbo, int pl_flag)
> +{
> +	unsigned int i;
> +	unsigned int c = 0;
> +
> +	gbo->placement.placement = gbo->placements;
> +	gbo->placement.busy_placement = gbo->placements;
> +
> +	if (pl_flag & TTM_PL_FLAG_VRAM)
> +		gbo->placements[c++].flags = TTM_PL_FLAG_WC |
> +					     TTM_PL_FLAG_UNCACHED |
> +					     TTM_PL_FLAG_VRAM;
> +
> +	if (pl_flag & TTM_PL_FLAG_SYSTEM)
> +		gbo->placements[c++].flags = TTM_PL_MASK_CACHING |
> +					     TTM_PL_FLAG_SYSTEM;
> +
> +	if (!c)
> +		gbo->placements[c++].flags = TTM_PL_MASK_CACHING |
> +					     TTM_PL_FLAG_SYSTEM;
> +
> +	gbo->placement.num_placement = c;
> +	gbo->placement.num_busy_placement = c;
> +
> +	for (i = 0; i < c; ++i) {
> +		gbo->placements[i].fpfn = 0;
> +		gbo->placements[i].lpfn = 0;
> +	}
> +}
> +
> +static int drm_gem_vram_init(struct drm_device *dev,
> +			     struct ttm_bo_device *bdev,
> +			     struct drm_gem_vram_object *gbo,
> +			     unsigned long size, uint32_t pg_align,
> +			     bool interruptible)
> +{
> +	int ret;
> +	size_t acc_size;
> +
> +	ret = drm_gem_object_init(dev, &gbo->gem, size);
> +	if (ret)
> +		return ret;
> +
> +	acc_size = ttm_bo_dma_acc_size(bdev, size, sizeof(*gbo));
> +
> +	gbo->bo.bdev = bdev;
> +	drm_gem_vram_placement(gbo, TTM_PL_FLAG_VRAM | TTM_PL_FLAG_SYSTEM);
> +
> +	ret = ttm_bo_init(bdev, &gbo->bo, size, ttm_bo_type_device,
> +			  &gbo->placement, pg_align, interruptible, acc_size,
> +			  NULL, NULL, ttm_buffer_object_destroy);
> +	if (ret)
> +		goto err_drm_gem_object_release;
> +
> +	return 0;
> +
> +err_drm_gem_object_release:
> +	drm_gem_object_release(&gbo->gem);
> +	return ret;
> +}
> +
> +/**
> + * drm_gem_vram_create() - Creates a VRAM-backed GEM object
> + * @dev:		the DRM device
> + * @bdev:		the TTM BO device backing the object
> + * @size:		the buffer size in bytes
> + * @pg_align:		the buffer's alignment in multiples of the page size
> + * @interruptible:	sleep interruptible if waiting for memory
> + *
> + * Returns:
> + * A new instance of &struct drm_gem_vram_object on success, or
> + * an ERR_PTR()-encoded error code otherwise.
> + */
> +struct drm_gem_vram_object* drm_gem_vram_create(struct drm_device *dev,
> +						struct ttm_bo_device *bdev,
> +						unsigned long size,
> +						uint32_t pg_align,
> +						bool interruptible)
> +{
> +	struct drm_gem_vram_object *gbo;
> +	int ret;
> +
> +	gbo = kzalloc(sizeof(*gbo), GFP_KERNEL);
> +	if (!gbo)
> +		return ERR_PTR(-ENOMEM);
> +
> +	ret = drm_gem_vram_init(dev, bdev, gbo, size, pg_align, interruptible);
> +	if (ret < 0)
> +		goto err_kfree;
> +
> +	return gbo;
> +
> +err_kfree:
> +	kfree(gbo);
> +	return ERR_PTR(ret);
> +}
> +EXPORT_SYMBOL(drm_gem_vram_create);
> +
> +/**
> + * drm_gem_vram_put() - Releases a reference to a VRAM-backed GEM object
> + * @gbo:	the GEM VRAM object
> + *
> + * See ttm_bo_put() for more information.
> + */
> +void drm_gem_vram_put(struct drm_gem_vram_object *gbo)
> +{
> +	ttm_bo_put(&gbo->bo);
> +}
> +EXPORT_SYMBOL(drm_gem_vram_put);
> +
> +/**
> + * drm_gem_vram_reserve() - Reserves a VRAM-backed GEM object
> + * @gbo:	the GEM VRAM object
> + * @no_wait:	don't wait for buffer object to become available
> + *
> + * See ttm_bo_reserve() for more information.
> + *
> + * Returns:
> + * 0 on success, or
> + * a negative error code otherwise
> + */
> +int drm_gem_vram_reserve(struct drm_gem_vram_object *gbo, bool no_wait)
> +{
> +	return ttm_bo_reserve(&gbo->bo, true, no_wait, NULL);
> +}
> +EXPORT_SYMBOL(drm_gem_vram_reserve);
> +
> +/**
> + * drm_gem_vram_unreserve() - \
> +	Release a reservation acquired by drm_gem_vram_reserve()
> + * @gbo:	the GEM VRAM object
> + *
> + * See ttm_bo_unreserve() for more information.
> + */
> +void drm_gem_vram_unreserve(struct drm_gem_vram_object *gbo)
> +{
> +	ttm_bo_unreserve(&gbo->bo);
> +}
> +EXPORT_SYMBOL(drm_gem_vram_unreserve);
> +
> +/**
> + * drm_gem_vram_mmap_offset() - Returns a GEM VRAM object's mmap offset
> + * @gbo:	the GEM VRAM object
> + *
> + * See drm_vma_node_offset_addr() for more information.
> + *
> + * Returns:
> + * The buffer object's offset for userspace mappings on success, or
> + * 0 if no offset is allocated.
> + */
> +u64 drm_gem_vram_mmap_offset(struct drm_gem_vram_object *gbo)
> +{
> +	return drm_vma_node_offset_addr(&gbo->bo.vma_node);
> +}
> +EXPORT_SYMBOL(drm_gem_vram_mmap_offset);
> +
> +/**
> + * drm_gem_vram_offset() - \
> +	Returns a GEM VRAM object's offset in video memory
> + * @gbo:	the GEM VRAM object
> + *
> + * This function returns the buffer object's offset in the device's video
> + * memory. The buffer object has to be pinned to %TTM_PL_VRAM.
> + *
> + * Returns:
> + * The buffer object's offset in video memory on success, or
> + * a negative error code otherwise.
> + */
> +s64 drm_gem_vram_offset(struct drm_gem_vram_object *gbo)
> +{
> +	if (!gbo->pin_count)
> +		return (s64)-ENODEV;
> +	return gbo->bo.offset;
> +}
> +EXPORT_SYMBOL(drm_gem_vram_offset);
> +
> +/**
> + * drm_gem_vram_pin() - Pins a GEM VRAM object in a region.
> + * @gbo:	the GEM VRAM object
> + * @pl_flag:	a bitmask of possible memory regions
> + *
> + * Pinning a buffer object ensures that it is not evicted from
> + * a memory region. A pinned buffer object has to be unpinned before
> + * it can be pinned to another region.
> + *
> + * Returns:
> + * 0 on success, or
> + * a negative error code otherwise.
> + */
> +int drm_gem_vram_pin(struct drm_gem_vram_object *gbo, u32 pl_flag)
> +{
> +	int i, ret;
> +	struct ttm_operation_ctx ctx = { false, false };
> +
> +	if (gbo->pin_count) {
> +		++gbo->pin_count;
> +		return 0;
> +	}
> +
> +	drm_gem_vram_placement(gbo, pl_flag);
> +	for (i = 0; i < gbo->placement.num_placement; ++i)
> +		gbo->placements[i].flags |= TTM_PL_FLAG_NO_EVICT;
> +
> +	ret = ttm_bo_validate(&gbo->bo, &gbo->placement, &ctx);
> +	if (ret < 0)
> +		return ret;
> +
> +	gbo->pin_count = 1;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_gem_vram_pin);
> +
> +/**
> + * drm_gem_vram_unpin() - Unpins a GEM VRAM object
> + * @gbo:	the GEM VRAM object
> + *
> + * Returns:
> + * 0 on success, or
> + * a negative error code otherwise.
> + */
> +int drm_gem_vram_unpin(struct drm_gem_vram_object *gbo)
> +{
> +	int i, ret;
> +	struct ttm_operation_ctx ctx = { false, false };
> +
> +	if (!gbo->pin_count)
> +		return 0;
> +
> +	--gbo->pin_count;
> +	if (gbo->pin_count)
> +		return 0;
> +
> +	for (i = 0; i < gbo->placement.num_placement ; ++i)
> +		gbo->placements[i].flags &= ~TTM_PL_FLAG_NO_EVICT;
> +
> +	ret = ttm_bo_validate(&gbo->bo, &gbo->placement, &ctx);
> +	if (ret < 0)
> +		return ret;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_gem_vram_unpin);
> +
> +/**
> + * drm_gem_vram_push_to_system() - \
> +	Unpins a GEM VRAM object and moves it to system memory
> + * @gbo:	the GEM VRAM object
> + *
> + * This operation only works if the caller holds the final pin on the
> + * buffer object.
> + *
> + * Returns:
> + * 0 on success, or
> + * a negative error code otherwise.
> + */
> +int drm_gem_vram_push_to_system(struct drm_gem_vram_object *gbo)
> +{
> +	int i, ret;
> +	struct ttm_operation_ctx ctx = { false, false };
> +
> +	if (!gbo->pin_count)
> +		return 0;
> +
> +	--gbo->pin_count;
> +	if (gbo->pin_count)
> +		return 0;
> +
> +	if (gbo->kmap.virtual)
> +		ttm_bo_kunmap(&gbo->kmap);
> +
> +	drm_gem_vram_placement(gbo, TTM_PL_FLAG_SYSTEM);
> +	for (i = 0; i < gbo->placement.num_placement ; ++i)
> +		gbo->placements[i].flags |= TTM_PL_FLAG_NO_EVICT;
> +
> +	ret = ttm_bo_validate(&gbo->bo, &gbo->placement, &ctx);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_gem_vram_push_to_system);
> +
> +/**
> + * drm_gem_vram_kmap_at() - Maps a GEM VRAM object into kernel address space
> + * @gbo:	the GEM VRAM object
> + * @map:	establish a mapping if necessary
> + * @is_iomem:	returns true if the mapped memory is I/O memory, or false \
> +	otherwise; can be NULL
> + * @kmap:	the mapping's kmap object
> + *
> + * This function maps the buffer object into the kernel's address space
> + * or returns the current mapping. If the parameter map is false, the
> + * function only queries the current mapping, but does not establish a
> + * new one.
> + *
> + * Returns:
> + * The buffers virtual address if mapped, or
> + * NULL if not mapped, or
> + * an ERR_PTR()-encoded error code otherwise.
> + */
> +void* drm_gem_vram_kmap_at(struct drm_gem_vram_object *gbo, bool map,
> +			   bool *is_iomem, struct ttm_bo_kmap_obj *kmap)
> +{
> +	int ret;
> +
> +	if (kmap->virtual || !map)
> +		goto out;
> +
> +	ret = ttm_bo_kmap(&gbo->bo, 0, gbo->bo.num_pages, kmap);
> +	if (ret)
> +		return ERR_PTR(ret);
> +
> +out:
> +	if (!is_iomem) {
> +		return kmap->virtual;
> +	}
> +	if (!kmap->virtual) {
> +		*is_iomem = false;
> +		return NULL;
> +	}
> +	return ttm_kmap_obj_virtual(kmap, is_iomem);
> +}
> +EXPORT_SYMBOL(drm_gem_vram_kmap_at);
> +
> +/**
> + * drm_gem_vram_kmap() - Maps a GEM VRAM object into kernel address space
> + * @gbo:	the GEM VRAM object
> + * @map:	establish a mapping if necessary
> + * @is_iomem:	returns true if the mapped memory is I/O memory, or false \
> +	otherwise; can be NULL
> + *
> + * This function maps the buffer object into the kernel's address space
> + * or returns the current mapping. If the parameter map is false, the
> + * function only queries the current mapping, but does not establish a
> + * new one.
> + *
> + * Returns:
> + * The buffers virtual address if mapped, or
> + * NULL if not mapped, or
> + * an ERR_PTR()-encoded error code otherwise.
> + */
> +void* drm_gem_vram_kmap(struct drm_gem_vram_object *gbo, bool map,
> +			bool *is_iomem)
> +{
> +	return drm_gem_vram_kmap_at(gbo, map, is_iomem, &gbo->kmap);
> +}
> +EXPORT_SYMBOL(drm_gem_vram_kmap);
> +
> +/**
> + * drm_gem_vram_kunmap_at() - Unmaps a GEM VRAM object
> + * @gbo:	the GEM VRAM object
> + * @kmap:	the mapping's kmap object
> + */
> +void drm_gem_vram_kunmap_at(struct drm_gem_vram_object *gbo,
> +			    struct ttm_bo_kmap_obj *kmap)
> +{
> +	if (!kmap->virtual)
> +		return;
> +
> +	ttm_bo_kunmap(kmap);
> +	kmap->virtual = NULL;
> +}
> +EXPORT_SYMBOL(drm_gem_vram_kunmap_at);
> +
> +/**
> + * drm_gem_vram_kunmap() - Unmaps a GEM VRAM object
> + * @gbo:	the GEM VRAM object
> + */
> +void drm_gem_vram_kunmap(struct drm_gem_vram_object *gbo)
> +{
> +	drm_gem_vram_kunmap_at(gbo, &gbo->kmap);
> +}
> +EXPORT_SYMBOL(drm_gem_vram_kunmap);
> diff --git a/drivers/gpu/drm/drm_vram_helper_common.c b/drivers/gpu/drm/drm_vram_helper_common.c
> new file mode 100644
> index 000000000000..76b6569c9aad
> --- /dev/null
> +++ b/drivers/gpu/drm/drm_vram_helper_common.c
> @@ -0,0 +1,6 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +
> +#include <linux/module.h>
> +
> +MODULE_DESCRIPTION("DRM VRAM memory-management helpers");
> +MODULE_LICENSE("GPL");
> diff --git a/include/drm/drm_gem_vram_helper.h b/include/drm/drm_gem_vram_helper.h
> new file mode 100644
> index 000000000000..167616f552e5
> --- /dev/null
> +++ b/include/drm/drm_gem_vram_helper.h
> @@ -0,0 +1,92 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +
> +#ifndef DRM_GEM_VRAM_HELPER_H
> +#define DRM_GEM_VRAM_HELPER_H
> +
> +#include <drm/drm_gem.h>
> +#include <drm/ttm/ttm_bo_api.h>
> +#include <drm/ttm/ttm_placement.h>
> +#include <linux/kernel.h> /* for container_of() */
> +
> +struct filp;
> +
> +#define DRM_GEM_VRAM_PL_FLAG_VRAM	TTM_PL_FLAG_VRAM
> +#define DRM_GEM_VRAM_PL_FLAG_SYSTEM	TTM_PL_FLAG_SYSTEM
> +
> +/*
> + * Buffer-object helpers
> + */
> +
> +/**
> + * struct drm_gem_vram_object - GEM object backed by VRAM
> + * @gem:	GEM object
> + * @bo:		TTM buffer object
> + * @kmap:	Mapping information for @bo
> + * @placement:	TTM placement information. Supported placements are \
> +	%TTM_PL_VRAM and %TTM_PL_SYSTEM
> + * @placements:	TTM placement information.
> + * @pin_count:	Pin counter
> + *
> + * The type struct drm_gem_vram_object represents a GEM object that is
> + * backed by VRAM. It can be used for simple frambuffer devices with
> + * dedicated memory. The buffer object can be evicted to system memory if
> + * video memory becomes scarce.
> + */
> +struct drm_gem_vram_object {
> +        struct drm_gem_object gem;
> +        struct ttm_buffer_object bo;
> +        struct ttm_bo_kmap_obj kmap;
> +
> +	/* Supported placements are %TTM_PL_VRAM and %TTM_PL_SYSTEM */
> +        struct ttm_placement placement;
> +        struct ttm_place placements[3];
> +
> +        int pin_count;
> +};
Use tabs for indent - not spaces.
Ask checkpatch if anything similar needs to be adjusted.


> +
> +/**
> + * Returns the container of type &struct drm_gem_vram_object
> + * for field bo.
> + * @bo:		the VRAM buffer object
> + * Returns:	The containing GEM VRAM object
> + */
> +static inline struct drm_gem_vram_object* drm_gem_vram_of_bo(
> +	struct ttm_buffer_object *bo)
> +{
> +	return container_of(bo, struct drm_gem_vram_object, bo);
> +}
Indent funny. USe same indent as used in other parts of file for
function arguments.

> +
> +/**
> + * Returns the container of type &struct drm_gem_vram_object
> + * for field gem.
> + * @gem:	the GEM object
> + * Returns:	The containing GEM VRAM object
> + */
> +static inline struct drm_gem_vram_object* drm_gem_vram_of_gem(
> +	struct drm_gem_object *gem)
> +{
> +	return container_of(gem, struct drm_gem_vram_object, gem);
> +}
ditto

> +
> +struct drm_gem_vram_object* drm_gem_vram_create(struct drm_device *dev,
> +						struct ttm_bo_device* bdev,
> +						unsigned long size,
> +						uint32_t pg_align,
> +						bool interruptible);

Here is is "normal"

Thomas Zimmermann April 30, 2019, 7:18 a.m. UTC | #2

Hi,

thanks for the feedback.

Am 29.04.19 um 21:58 schrieb Sam Ravnborg:
> Hi Thomas.
> 
> Some minor things and some bikeshedding too.
> 
> One general^Wbikeshedding thing - unint32_t is used in many places.
> And then s64 in one place.
> Seems like two concepts are mixed.
> Maybe be consistent and use u32, s32 everywhere?

The DRM API already has a mixture of such types and I tried to use the
type that best fits the current context. But yeah, I don't mind some
consistency. I'll see if I can replace some of these instances.

>> +config DRM_GEM_VRAM_HELPER
>> +	bool
>> +	depends on DRM
>> +	select DRM_VRAM_HELPER
>> +	help
>> +	  Choose this if you need the GEM VRAM helper functions
>> +
> I cannot remember how select will deal with symbols whos has
> a  "depends on".
> But if I recall correct then the "depends on" will be ignored
> or in best case trigger a warning.
> In other words - when we have symbols we select they should not
> have a depends on.
> 
> What can be done is something like:
> 
> symbol foo
> 	bool
> 
> symbol bar
> 	depends on foo
> 
> 
> symbol foobar
> 	bool "This is what you need - select me"
> 	select foo
> 
> So when one chooses "foobar" then we will select "foo" and this will
> satisfy bar.
> 
> But maybe this rambling is irrelevant - maybe check what we do with
> other selectable symbols in DRM.

It may not strictly be necessary here, but the other helpers' symbols
depend on DRM. I'd like to keep it consistent unless there's a strong
reason not to.

> 
> 
>> +/**
>> + * DOC: overview
>> + *
>> + * This library provides a GEM object that is backed by VRAM. It
>> + * can be used for simple framebuffer devices with dedicated memory.
>> + */
> It is likely only me, but...
> I could use a short explanation what is GEM and maybe also VRAM.
> 
> VRAM as video RAM, but maybe there is more constraints?
> (When I first looked at DRM I wondered what you used Virtual RAM for.
> But thats a long time ago so it counts only as a funny story.

OK :)

>> +/*
>> + * Buffer-object helpers
>> + */
>> +
>> +/**
>> + * struct drm_gem_vram_object - GEM object backed by VRAM
>> + * @gem:	GEM object
>> + * @bo:		TTM buffer object
>> + * @kmap:	Mapping information for @bo
>> + * @placement:	TTM placement information. Supported placements are \
>> +	%TTM_PL_VRAM and %TTM_PL_SYSTEM
>> + * @placements:	TTM placement information.
>> + * @pin_count:	Pin counter
>> + *
>> + * The type struct drm_gem_vram_object represents a GEM object that is
>> + * backed by VRAM. It can be used for simple frambuffer devices with
>> + * dedicated memory. The buffer object can be evicted to system memory if
>> + * video memory becomes scarce.
>> + */
>> +struct drm_gem_vram_object {
>> +        struct drm_gem_object gem;
>> +        struct ttm_buffer_object bo;
>> +        struct ttm_bo_kmap_obj kmap;
>> +
>> +	/* Supported placements are %TTM_PL_VRAM and %TTM_PL_SYSTEM */
>> +        struct ttm_placement placement;
>> +        struct ttm_place placements[3];
>> +
>> +        int pin_count;
>> +};
> Use tabs for indent - not spaces.
> Ask checkpatch if anything similar needs to be adjusted.

Oh well, I should have checked this. Thanks for reporting.

>> +
>> +/**
>> + * Returns the container of type &struct drm_gem_vram_object
>> + * for field bo.
>> + * @bo:		the VRAM buffer object
>> + * Returns:	The containing GEM VRAM object
>> + */
>> +static inline struct drm_gem_vram_object* drm_gem_vram_of_bo(
>> +	struct ttm_buffer_object *bo)
>> +{
>> +	return container_of(bo, struct drm_gem_vram_object, bo);
>> +}
> Indent funny. USe same indent as used in other parts of file for
> function arguments.

If I put the argument next to the function's name, it will exceed the
80-character limit. From the coding-style document, I could not see what
to do in this case. One solution would move the return type to a
separate line before the function name. I've not seen that anywhere in
the source code, so moving the argument onto a separate line and
indenting by one tab appears to be the next best solution. Please let me
know if there's if there's a preferred style for cases like this one.

Best regards
Thomas

>> +
>> +/**
>> + * Returns the container of type &struct drm_gem_vram_object
>> + * for field gem.
>> + * @gem:	the GEM object
>> + * Returns:	The containing GEM VRAM object
>> + */
>> +static inline struct drm_gem_vram_object* drm_gem_vram_of_gem(
>> +	struct drm_gem_object *gem)
>> +{
>> +	return container_of(gem, struct drm_gem_vram_object, gem);
>> +}
> ditto
> 
>> +
>> +struct drm_gem_vram_object* drm_gem_vram_create(struct drm_device *dev,
>> +						struct ttm_bo_device* bdev,
>> +						unsigned long size,
>> +						uint32_t pg_align,
>> +						bool interruptible);
> 
> Here is is "normal"
>

Sam Ravnborg April 30, 2019, 9:23 a.m. UTC | #3

Hi Thomas.

> >> +
> >> +/**
> >> + * Returns the container of type &struct drm_gem_vram_object
> >> + * for field bo.
> >> + * @bo:		the VRAM buffer object
> >> + * Returns:	The containing GEM VRAM object
> >> + */
> >> +static inline struct drm_gem_vram_object* drm_gem_vram_of_bo(
> >> +	struct ttm_buffer_object *bo)
> >> +{
> >> +	return container_of(bo, struct drm_gem_vram_object, bo);
> >> +}
> > Indent funny. USe same indent as used in other parts of file for
> > function arguments.
> 
> If I put the argument next to the function's name, it will exceed the
> 80-character limit. From the coding-style document, I could not see what
> to do in this case. One solution would move the return type to a
> separate line before the function name. I've not seen that anywhere in
> the source code, so moving the argument onto a separate line and
> indenting by one tab appears to be the next best solution. Please let me
> know if there's if there's a preferred style for cases like this one.

Readability has IMO higher priority than some limit of 80 chars.
And it hurts readability (at least my OCD) when style changes
as you do with indent here. So my personal preference is to fix
indent and accect longer lines.

But you ask for a preferred style - which I do not think we have in this
case. So it boils down to what you prefer.

Enough bikeshedding, thanks for the quick response.

	Sam

Christian König April 30, 2019, 9:35 a.m. UTC | #4

Am 30.04.19 um 11:23 schrieb Sam Ravnborg:
> [CAUTION: External Email]
>
> Hi Thomas.
>
>>>> +
>>>> +/**
>>>> + * Returns the container of type &struct drm_gem_vram_object
>>>> + * for field bo.
>>>> + * @bo:           the VRAM buffer object
>>>> + * Returns:       The containing GEM VRAM object
>>>> + */
>>>> +static inline struct drm_gem_vram_object* drm_gem_vram_of_bo(
>>>> +  struct ttm_buffer_object *bo)
>>>> +{
>>>> +  return container_of(bo, struct drm_gem_vram_object, bo);
>>>> +}
>>> Indent funny. USe same indent as used in other parts of file for
>>> function arguments.
>> If I put the argument next to the function's name, it will exceed the
>> 80-character limit. From the coding-style document, I could not see what
>> to do in this case. One solution would move the return type to a
>> separate line before the function name. I've not seen that anywhere in
>> the source code, so moving the argument onto a separate line and
>> indenting by one tab appears to be the next best solution. Please let me
>> know if there's if there's a preferred style for cases like this one.
> Readability has IMO higher priority than some limit of 80 chars.
> And it hurts readability (at least my OCD) when style changes
> as you do with indent here. So my personal preference is to fix
> indent and accect longer lines.

In this case the an often used convention (which is also kind of 
readable) is to add a newline after the return values, but before the 
function name. E.g. something like this:

static inline struct drm_gem_vram_object*
drm_gem_vram_of_bo(struct ttm_buffer_object *bo)

Regards,
Christian.

>
> But you ask for a preferred style - which I do not think we have in this
> case. So it boils down to what you prefer.
>
> Enough bikeshedding, thanks for the quick response.
>
>          Sam

Daniel Vetter April 30, 2019, 2:27 p.m. UTC | #5

On Mon, Apr 29, 2019 at 09:58:55PM +0200, Sam Ravnborg wrote:
> Hi Thomas.
> 
> Some minor things and some bikeshedding too.
> 
> One general^Wbikeshedding thing - unint32_t is used in many places.
> And then s64 in one place.
> Seems like two concepts are mixed.
> Maybe be consistent and use u32, s32 everywhere?
> 
> I did not read the code carefully enough to understand it.
> I cannot give a r-b or a-b - as I feel I need to understand it to do
> so.
> 
> 	Sam
> 
> On Mon, Apr 29, 2019 at 04:43:23PM +0200, Thomas Zimmermann wrote:
> > The type |struct drm_gem_vram_object| implements a GEM object for simple
> > framebuffer devices with dedicated video memory. The BO is either located
> > in VRAM or system memory.
> > 
> > The implementation has been created from the respective code in ast,
> > bochs and mgag200. These drivers copy their implementation from each
> > other; except for the names of several data types. The helpers are
> > currently build with TTM, but this is considered an implementation
> > detail and may change in future updates.
> > 
> > v2:
> > 	* rename to |struct drm_gem_vram_object|
> > 	* move drm_is_gem_ttm() to a later patch in the series
> > 	* add drm_gem_vram_kmap_at()
> > 	* return is_iomem from kmap functions
> > 	* redefine TTM placement flags for public interface
> > 	* documentation fixes
> > 
> > Signed-off-by: Thomas Zimmermann <[e-mail]>
> > ---
> >  Documentation/gpu/drm-mm.rst             |  12 +
> >  drivers/gpu/drm/Kconfig                  |  13 +
> >  drivers/gpu/drm/Makefile                 |   4 +
> >  drivers/gpu/drm/drm_gem_vram_helper.c    | 410 +++++++++++++++++++++++
> >  drivers/gpu/drm/drm_vram_helper_common.c |   6 +
> >  include/drm/drm_gem_vram_helper.h        |  92 +++++
> >  6 files changed, 537 insertions(+)
> >  create mode 100644 drivers/gpu/drm/drm_gem_vram_helper.c
> >  create mode 100644 drivers/gpu/drm/drm_vram_helper_common.c
> >  create mode 100644 include/drm/drm_gem_vram_helper.h
> > 
> > diff --git a/Documentation/gpu/drm-mm.rst b/Documentation/gpu/drm-mm.rst
> > index 54a696d961a7..d5327ed608d7 100644
> > --- a/Documentation/gpu/drm-mm.rst
> > +++ b/Documentation/gpu/drm-mm.rst
> > @@ -380,6 +380,18 @@ GEM CMA Helper Functions Reference
> >  .. kernel-doc:: drivers/gpu/drm/drm_gem_cma_helper.c
> >     :export:
> >  
> > +GEM VRAM Helper Functions Reference
> > +----------------------------------
> > +
> > +.. kernel-doc:: drivers/gpu/drm/drm_gem_vram_helper.c
> > +   :doc: overview
> > +
> > +.. kernel-doc:: include/drm/drm_gem_vram_helper.h
> > +   :internal:
> > +
> > +.. kernel-doc:: drivers/gpu/drm/drm_gem_vram_helper.c
> > +   :export:
> > +
> >  VMA Offset Manager
> >  ==================
> >  
> > diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> > index 2267e84d5cb4..c0d49a6c09d2 100644
> > --- a/drivers/gpu/drm/Kconfig
> > +++ b/drivers/gpu/drm/Kconfig
> > @@ -160,6 +160,12 @@ config DRM_TTM
> >  	  GPU memory types. Will be enabled automatically if a device driver
> >  	  uses it.
> >  
> > +config DRM_VRAM_HELPER
> > +	tristate
> > +	depends on DRM && DRM_TTM
> > +	help
> > +	  Helpers for VRAM memory management
> > +
> >  config DRM_GEM_CMA_HELPER
> >  	bool
> >  	depends on DRM
> > @@ -179,6 +185,13 @@ config DRM_GEM_SHMEM_HELPER
> >  	help
> >  	  Choose this if you need the GEM shmem helper functions
> >  
> > +config DRM_GEM_VRAM_HELPER
> > +	bool
> > +	depends on DRM
> > +	select DRM_VRAM_HELPER
> > +	help
> > +	  Choose this if you need the GEM VRAM helper functions
> > +
> I cannot remember how select will deal with symbols whos has
> a  "depends on".

It doesn't, randomized builds will go kaboom.

> But if I recall correct then the "depends on" will be ignored
> or in best case trigger a warning.
> In other words - when we have symbols we select they should not
> have a depends on.
> 
> What can be done is something like:
> 
> symbol foo
> 	bool
> 
> symbol bar
> 	depends on foo
> 
> 
> symbol foobar
> 	bool "This is what you need - select me"
> 	select foo
> 
> So when one chooses "foobar" then we will select "foo" and this will
> satisfy bar.
> 
> But maybe this rambling is irrelevant - maybe check what we do with
> other selectable symbols in DRM.

I'm not even clear on why you need 2 symbols here ... If the goal is to
create a drm_gem_helper.ko module, then that's a bit more work. Plus
anyone needed it would always need to select both Kconfig symbols (because
select isn't recursive at all).

I'd just ditch one of them for now and got with DRM_GEM_VRAM_HELPER.
-Daniel

> 
> 
> > +/**
> > + * DOC: overview
> > + *
> > + * This library provides a GEM object that is backed by VRAM. It
> > + * can be used for simple framebuffer devices with dedicated memory.
> > + */
> It is likely only me, but...
> I could use a short explanation what is GEM and maybe also VRAM.
> 
> VRAM as video RAM, but maybe there is more constraints?
> (When I first looked at DRM I wondered what you used Virtual RAM for.
> But thats a long time ago so it counts only as a funny story.
> 
> > + * Buffer-objects helpers
> > + */
> > +
> > +static void drm_gem_vram_cleanup(struct drm_gem_vram_object *gbo)
> > +{
> > +	/* We got here via ttm_bo_put(), which means that the
> > +	 * TTM buffer object in 'bo' has already been cleaned
> > +	 * up; only release the GEM object. */
> > +	drm_gem_object_release(&gbo->gem);
> > +}
> > +
> > +static void drm_gem_vram_destroy(struct drm_gem_vram_object *gbo)
> > +{
> > +	drm_gem_vram_cleanup(gbo);
> > +	kfree(gbo);
> > +}
> > +
> > +static void ttm_buffer_object_destroy(struct ttm_buffer_object *bo)
> > +{
> > +	struct drm_gem_vram_object *gbo = drm_gem_vram_of_bo(bo);
> > +	drm_gem_vram_destroy(gbo);
> > +}
> > +
> > +static void drm_gem_vram_placement(struct drm_gem_vram_object *gbo, int pl_flag)
> > +{
> > +	unsigned int i;
> > +	unsigned int c = 0;
> > +
> > +	gbo->placement.placement = gbo->placements;
> > +	gbo->placement.busy_placement = gbo->placements;
> > +
> > +	if (pl_flag & TTM_PL_FLAG_VRAM)
> > +		gbo->placements[c++].flags = TTM_PL_FLAG_WC |
> > +					     TTM_PL_FLAG_UNCACHED |
> > +					     TTM_PL_FLAG_VRAM;
> > +
> > +	if (pl_flag & TTM_PL_FLAG_SYSTEM)
> > +		gbo->placements[c++].flags = TTM_PL_MASK_CACHING |
> > +					     TTM_PL_FLAG_SYSTEM;
> > +
> > +	if (!c)
> > +		gbo->placements[c++].flags = TTM_PL_MASK_CACHING |
> > +					     TTM_PL_FLAG_SYSTEM;
> > +
> > +	gbo->placement.num_placement = c;
> > +	gbo->placement.num_busy_placement = c;
> > +
> > +	for (i = 0; i < c; ++i) {
> > +		gbo->placements[i].fpfn = 0;
> > +		gbo->placements[i].lpfn = 0;
> > +	}
> > +}
> > +
> > +static int drm_gem_vram_init(struct drm_device *dev,
> > +			     struct ttm_bo_device *bdev,
> > +			     struct drm_gem_vram_object *gbo,
> > +			     unsigned long size, uint32_t pg_align,
> > +			     bool interruptible)
> > +{
> > +	int ret;
> > +	size_t acc_size;
> > +
> > +	ret = drm_gem_object_init(dev, &gbo->gem, size);
> > +	if (ret)
> > +		return ret;
> > +
> > +	acc_size = ttm_bo_dma_acc_size(bdev, size, sizeof(*gbo));
> > +
> > +	gbo->bo.bdev = bdev;
> > +	drm_gem_vram_placement(gbo, TTM_PL_FLAG_VRAM | TTM_PL_FLAG_SYSTEM);
> > +
> > +	ret = ttm_bo_init(bdev, &gbo->bo, size, ttm_bo_type_device,
> > +			  &gbo->placement, pg_align, interruptible, acc_size,
> > +			  NULL, NULL, ttm_buffer_object_destroy);
> > +	if (ret)
> > +		goto err_drm_gem_object_release;
> > +
> > +	return 0;
> > +
> > +err_drm_gem_object_release:
> > +	drm_gem_object_release(&gbo->gem);
> > +	return ret;
> > +}
> > +
> > +/**
> > + * drm_gem_vram_create() - Creates a VRAM-backed GEM object
> > + * @dev:		the DRM device
> > + * @bdev:		the TTM BO device backing the object
> > + * @size:		the buffer size in bytes
> > + * @pg_align:		the buffer's alignment in multiples of the page size
> > + * @interruptible:	sleep interruptible if waiting for memory
> > + *
> > + * Returns:
> > + * A new instance of &struct drm_gem_vram_object on success, or
> > + * an ERR_PTR()-encoded error code otherwise.
> > + */
> > +struct drm_gem_vram_object* drm_gem_vram_create(struct drm_device *dev,
> > +						struct ttm_bo_device *bdev,
> > +						unsigned long size,
> > +						uint32_t pg_align,
> > +						bool interruptible)
> > +{
> > +	struct drm_gem_vram_object *gbo;
> > +	int ret;
> > +
> > +	gbo = kzalloc(sizeof(*gbo), GFP_KERNEL);
> > +	if (!gbo)
> > +		return ERR_PTR(-ENOMEM);
> > +
> > +	ret = drm_gem_vram_init(dev, bdev, gbo, size, pg_align, interruptible);
> > +	if (ret < 0)
> > +		goto err_kfree;
> > +
> > +	return gbo;
> > +
> > +err_kfree:
> > +	kfree(gbo);
> > +	return ERR_PTR(ret);
> > +}
> > +EXPORT_SYMBOL(drm_gem_vram_create);
> > +
> > +/**
> > + * drm_gem_vram_put() - Releases a reference to a VRAM-backed GEM object
> > + * @gbo:	the GEM VRAM object
> > + *
> > + * See ttm_bo_put() for more information.
> > + */
> > +void drm_gem_vram_put(struct drm_gem_vram_object *gbo)
> > +{
> > +	ttm_bo_put(&gbo->bo);
> > +}
> > +EXPORT_SYMBOL(drm_gem_vram_put);
> > +
> > +/**
> > + * drm_gem_vram_reserve() - Reserves a VRAM-backed GEM object
> > + * @gbo:	the GEM VRAM object
> > + * @no_wait:	don't wait for buffer object to become available
> > + *
> > + * See ttm_bo_reserve() for more information.
> > + *
> > + * Returns:
> > + * 0 on success, or
> > + * a negative error code otherwise
> > + */
> > +int drm_gem_vram_reserve(struct drm_gem_vram_object *gbo, bool no_wait)
> > +{
> > +	return ttm_bo_reserve(&gbo->bo, true, no_wait, NULL);
> > +}
> > +EXPORT_SYMBOL(drm_gem_vram_reserve);
> > +
> > +/**
> > + * drm_gem_vram_unreserve() - \
> > +	Release a reservation acquired by drm_gem_vram_reserve()
> > + * @gbo:	the GEM VRAM object
> > + *
> > + * See ttm_bo_unreserve() for more information.
> > + */
> > +void drm_gem_vram_unreserve(struct drm_gem_vram_object *gbo)
> > +{
> > +	ttm_bo_unreserve(&gbo->bo);
> > +}
> > +EXPORT_SYMBOL(drm_gem_vram_unreserve);
> > +
> > +/**
> > + * drm_gem_vram_mmap_offset() - Returns a GEM VRAM object's mmap offset
> > + * @gbo:	the GEM VRAM object
> > + *
> > + * See drm_vma_node_offset_addr() for more information.
> > + *
> > + * Returns:
> > + * The buffer object's offset for userspace mappings on success, or
> > + * 0 if no offset is allocated.
> > + */
> > +u64 drm_gem_vram_mmap_offset(struct drm_gem_vram_object *gbo)
> > +{
> > +	return drm_vma_node_offset_addr(&gbo->bo.vma_node);
> > +}
> > +EXPORT_SYMBOL(drm_gem_vram_mmap_offset);
> > +
> > +/**
> > + * drm_gem_vram_offset() - \
> > +	Returns a GEM VRAM object's offset in video memory
> > + * @gbo:	the GEM VRAM object
> > + *
> > + * This function returns the buffer object's offset in the device's video
> > + * memory. The buffer object has to be pinned to %TTM_PL_VRAM.
> > + *
> > + * Returns:
> > + * The buffer object's offset in video memory on success, or
> > + * a negative error code otherwise.
> > + */
> > +s64 drm_gem_vram_offset(struct drm_gem_vram_object *gbo)
> > +{
> > +	if (!gbo->pin_count)
> > +		return (s64)-ENODEV;
> > +	return gbo->bo.offset;
> > +}
> > +EXPORT_SYMBOL(drm_gem_vram_offset);
> > +
> > +/**
> > + * drm_gem_vram_pin() - Pins a GEM VRAM object in a region.
> > + * @gbo:	the GEM VRAM object
> > + * @pl_flag:	a bitmask of possible memory regions
> > + *
> > + * Pinning a buffer object ensures that it is not evicted from
> > + * a memory region. A pinned buffer object has to be unpinned before
> > + * it can be pinned to another region.
> > + *
> > + * Returns:
> > + * 0 on success, or
> > + * a negative error code otherwise.
> > + */
> > +int drm_gem_vram_pin(struct drm_gem_vram_object *gbo, u32 pl_flag)
> > +{
> > +	int i, ret;
> > +	struct ttm_operation_ctx ctx = { false, false };
> > +
> > +	if (gbo->pin_count) {
> > +		++gbo->pin_count;
> > +		return 0;
> > +	}
> > +
> > +	drm_gem_vram_placement(gbo, pl_flag);
> > +	for (i = 0; i < gbo->placement.num_placement; ++i)
> > +		gbo->placements[i].flags |= TTM_PL_FLAG_NO_EVICT;
> > +
> > +	ret = ttm_bo_validate(&gbo->bo, &gbo->placement, &ctx);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	gbo->pin_count = 1;
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL(drm_gem_vram_pin);
> > +
> > +/**
> > + * drm_gem_vram_unpin() - Unpins a GEM VRAM object
> > + * @gbo:	the GEM VRAM object
> > + *
> > + * Returns:
> > + * 0 on success, or
> > + * a negative error code otherwise.
> > + */
> > +int drm_gem_vram_unpin(struct drm_gem_vram_object *gbo)
> > +{
> > +	int i, ret;
> > +	struct ttm_operation_ctx ctx = { false, false };
> > +
> > +	if (!gbo->pin_count)
> > +		return 0;
> > +
> > +	--gbo->pin_count;
> > +	if (gbo->pin_count)
> > +		return 0;
> > +
> > +	for (i = 0; i < gbo->placement.num_placement ; ++i)
> > +		gbo->placements[i].flags &= ~TTM_PL_FLAG_NO_EVICT;
> > +
> > +	ret = ttm_bo_validate(&gbo->bo, &gbo->placement, &ctx);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL(drm_gem_vram_unpin);
> > +
> > +/**
> > + * drm_gem_vram_push_to_system() - \
> > +	Unpins a GEM VRAM object and moves it to system memory
> > + * @gbo:	the GEM VRAM object
> > + *
> > + * This operation only works if the caller holds the final pin on the
> > + * buffer object.
> > + *
> > + * Returns:
> > + * 0 on success, or
> > + * a negative error code otherwise.
> > + */
> > +int drm_gem_vram_push_to_system(struct drm_gem_vram_object *gbo)
> > +{
> > +	int i, ret;
> > +	struct ttm_operation_ctx ctx = { false, false };
> > +
> > +	if (!gbo->pin_count)
> > +		return 0;
> > +
> > +	--gbo->pin_count;
> > +	if (gbo->pin_count)
> > +		return 0;
> > +
> > +	if (gbo->kmap.virtual)
> > +		ttm_bo_kunmap(&gbo->kmap);
> > +
> > +	drm_gem_vram_placement(gbo, TTM_PL_FLAG_SYSTEM);
> > +	for (i = 0; i < gbo->placement.num_placement ; ++i)
> > +		gbo->placements[i].flags |= TTM_PL_FLAG_NO_EVICT;
> > +
> > +	ret = ttm_bo_validate(&gbo->bo, &gbo->placement, &ctx);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL(drm_gem_vram_push_to_system);
> > +
> > +/**
> > + * drm_gem_vram_kmap_at() - Maps a GEM VRAM object into kernel address space
> > + * @gbo:	the GEM VRAM object
> > + * @map:	establish a mapping if necessary
> > + * @is_iomem:	returns true if the mapped memory is I/O memory, or false \
> > +	otherwise; can be NULL
> > + * @kmap:	the mapping's kmap object
> > + *
> > + * This function maps the buffer object into the kernel's address space
> > + * or returns the current mapping. If the parameter map is false, the
> > + * function only queries the current mapping, but does not establish a
> > + * new one.
> > + *
> > + * Returns:
> > + * The buffers virtual address if mapped, or
> > + * NULL if not mapped, or
> > + * an ERR_PTR()-encoded error code otherwise.
> > + */
> > +void* drm_gem_vram_kmap_at(struct drm_gem_vram_object *gbo, bool map,
> > +			   bool *is_iomem, struct ttm_bo_kmap_obj *kmap)
> > +{
> > +	int ret;
> > +
> > +	if (kmap->virtual || !map)
> > +		goto out;
> > +
> > +	ret = ttm_bo_kmap(&gbo->bo, 0, gbo->bo.num_pages, kmap);
> > +	if (ret)
> > +		return ERR_PTR(ret);
> > +
> > +out:
> > +	if (!is_iomem) {
> > +		return kmap->virtual;
> > +	}
> > +	if (!kmap->virtual) {
> > +		*is_iomem = false;
> > +		return NULL;
> > +	}
> > +	return ttm_kmap_obj_virtual(kmap, is_iomem);
> > +}
> > +EXPORT_SYMBOL(drm_gem_vram_kmap_at);
> > +
> > +/**
> > + * drm_gem_vram_kmap() - Maps a GEM VRAM object into kernel address space
> > + * @gbo:	the GEM VRAM object
> > + * @map:	establish a mapping if necessary
> > + * @is_iomem:	returns true if the mapped memory is I/O memory, or false \
> > +	otherwise; can be NULL
> > + *
> > + * This function maps the buffer object into the kernel's address space
> > + * or returns the current mapping. If the parameter map is false, the
> > + * function only queries the current mapping, but does not establish a
> > + * new one.
> > + *
> > + * Returns:
> > + * The buffers virtual address if mapped, or
> > + * NULL if not mapped, or
> > + * an ERR_PTR()-encoded error code otherwise.
> > + */
> > +void* drm_gem_vram_kmap(struct drm_gem_vram_object *gbo, bool map,
> > +			bool *is_iomem)
> > +{
> > +	return drm_gem_vram_kmap_at(gbo, map, is_iomem, &gbo->kmap);
> > +}
> > +EXPORT_SYMBOL(drm_gem_vram_kmap);
> > +
> > +/**
> > + * drm_gem_vram_kunmap_at() - Unmaps a GEM VRAM object
> > + * @gbo:	the GEM VRAM object
> > + * @kmap:	the mapping's kmap object
> > + */
> > +void drm_gem_vram_kunmap_at(struct drm_gem_vram_object *gbo,
> > +			    struct ttm_bo_kmap_obj *kmap)
> > +{
> > +	if (!kmap->virtual)
> > +		return;
> > +
> > +	ttm_bo_kunmap(kmap);
> > +	kmap->virtual = NULL;
> > +}
> > +EXPORT_SYMBOL(drm_gem_vram_kunmap_at);
> > +
> > +/**
> > + * drm_gem_vram_kunmap() - Unmaps a GEM VRAM object
> > + * @gbo:	the GEM VRAM object
> > + */
> > +void drm_gem_vram_kunmap(struct drm_gem_vram_object *gbo)
> > +{
> > +	drm_gem_vram_kunmap_at(gbo, &gbo->kmap);
> > +}
> > +EXPORT_SYMBOL(drm_gem_vram_kunmap);
> > diff --git a/drivers/gpu/drm/drm_vram_helper_common.c b/drivers/gpu/drm/drm_vram_helper_common.c
> > new file mode 100644
> > index 000000000000..76b6569c9aad
> > --- /dev/null
> > +++ b/drivers/gpu/drm/drm_vram_helper_common.c
> > @@ -0,0 +1,6 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +
> > +#include <linux/module.h>
> > +
> > +MODULE_DESCRIPTION("DRM VRAM memory-management helpers");
> > +MODULE_LICENSE("GPL");
> > diff --git a/include/drm/drm_gem_vram_helper.h b/include/drm/drm_gem_vram_helper.h
> > new file mode 100644
> > index 000000000000..167616f552e5
> > --- /dev/null
> > +++ b/include/drm/drm_gem_vram_helper.h
> > @@ -0,0 +1,92 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +
> > +#ifndef DRM_GEM_VRAM_HELPER_H
> > +#define DRM_GEM_VRAM_HELPER_H
> > +
> > +#include <drm/drm_gem.h>
> > +#include <drm/ttm/ttm_bo_api.h>
> > +#include <drm/ttm/ttm_placement.h>
> > +#include <linux/kernel.h> /* for container_of() */
> > +
> > +struct filp;
> > +
> > +#define DRM_GEM_VRAM_PL_FLAG_VRAM	TTM_PL_FLAG_VRAM
> > +#define DRM_GEM_VRAM_PL_FLAG_SYSTEM	TTM_PL_FLAG_SYSTEM
> > +
> > +/*
> > + * Buffer-object helpers
> > + */
> > +
> > +/**
> > + * struct drm_gem_vram_object - GEM object backed by VRAM
> > + * @gem:	GEM object
> > + * @bo:		TTM buffer object
> > + * @kmap:	Mapping information for @bo
> > + * @placement:	TTM placement information. Supported placements are \
> > +	%TTM_PL_VRAM and %TTM_PL_SYSTEM
> > + * @placements:	TTM placement information.
> > + * @pin_count:	Pin counter
> > + *
> > + * The type struct drm_gem_vram_object represents a GEM object that is
> > + * backed by VRAM. It can be used for simple frambuffer devices with
> > + * dedicated memory. The buffer object can be evicted to system memory if
> > + * video memory becomes scarce.
> > + */
> > +struct drm_gem_vram_object {
> > +        struct drm_gem_object gem;
> > +        struct ttm_buffer_object bo;
> > +        struct ttm_bo_kmap_obj kmap;
> > +
> > +	/* Supported placements are %TTM_PL_VRAM and %TTM_PL_SYSTEM */
> > +        struct ttm_placement placement;
> > +        struct ttm_place placements[3];
> > +
> > +        int pin_count;
> > +};
> Use tabs for indent - not spaces.
> Ask checkpatch if anything similar needs to be adjusted.
> 
> 
> > +
> > +/**
> > + * Returns the container of type &struct drm_gem_vram_object
> > + * for field bo.
> > + * @bo:		the VRAM buffer object
> > + * Returns:	The containing GEM VRAM object
> > + */
> > +static inline struct drm_gem_vram_object* drm_gem_vram_of_bo(
> > +	struct ttm_buffer_object *bo)
> > +{
> > +	return container_of(bo, struct drm_gem_vram_object, bo);
> > +}
> Indent funny. USe same indent as used in other parts of file for
> function arguments.
> 
> > +
> > +/**
> > + * Returns the container of type &struct drm_gem_vram_object
> > + * for field gem.
> > + * @gem:	the GEM object
> > + * Returns:	The containing GEM VRAM object
> > + */
> > +static inline struct drm_gem_vram_object* drm_gem_vram_of_gem(
> > +	struct drm_gem_object *gem)
> > +{
> > +	return container_of(gem, struct drm_gem_vram_object, gem);
> > +}
> ditto
> 
> > +
> > +struct drm_gem_vram_object* drm_gem_vram_create(struct drm_device *dev,
> > +						struct ttm_bo_device* bdev,
> > +						unsigned long size,
> > +						uint32_t pg_align,
> > +						bool interruptible);
> 
> Here is is "normal"
>

Daniel Vetter April 30, 2019, 2:40 p.m. UTC | #6

On Mon, Apr 29, 2019 at 09:58:55PM +0200, Sam Ravnborg wrote:
> Hi Thomas.
> 
> Some minor things and some bikeshedding too.
> 
> One general^Wbikeshedding thing - unint32_t is used in many places.
> And then s64 in one place.
> Seems like two concepts are mixed.
> Maybe be consistent and use u32, s32 everywhere?
> 
> I did not read the code carefully enough to understand it.
> I cannot give a r-b or a-b - as I feel I need to understand it to do
> so.
> 
> 	Sam
> 
> On Mon, Apr 29, 2019 at 04:43:23PM +0200, Thomas Zimmermann wrote:
> > The type |struct drm_gem_vram_object| implements a GEM object for simple
> > framebuffer devices with dedicated video memory. The BO is either located
> > in VRAM or system memory.
> > 
> > The implementation has been created from the respective code in ast,
> > bochs and mgag200. These drivers copy their implementation from each
> > other; except for the names of several data types. The helpers are
> > currently build with TTM, but this is considered an implementation
> > detail and may change in future updates.
> > 
> > v2:
> > 	* rename to |struct drm_gem_vram_object|
> > 	* move drm_is_gem_ttm() to a later patch in the series
> > 	* add drm_gem_vram_kmap_at()
> > 	* return is_iomem from kmap functions
> > 	* redefine TTM placement flags for public interface
> > 	* documentation fixes
> > 
> > Signed-off-by: Thomas Zimmermann <[e-mail]>
> > ---
> >  Documentation/gpu/drm-mm.rst             |  12 +
> >  drivers/gpu/drm/Kconfig                  |  13 +
> >  drivers/gpu/drm/Makefile                 |   4 +
> >  drivers/gpu/drm/drm_gem_vram_helper.c    | 410 +++++++++++++++++++++++
> >  drivers/gpu/drm/drm_vram_helper_common.c |   6 +
> >  include/drm/drm_gem_vram_helper.h        |  92 +++++
> >  6 files changed, 537 insertions(+)
> >  create mode 100644 drivers/gpu/drm/drm_gem_vram_helper.c
> >  create mode 100644 drivers/gpu/drm/drm_vram_helper_common.c
> >  create mode 100644 include/drm/drm_gem_vram_helper.h
> > 
> > diff --git a/Documentation/gpu/drm-mm.rst b/Documentation/gpu/drm-mm.rst
> > index 54a696d961a7..d5327ed608d7 100644
> > --- a/Documentation/gpu/drm-mm.rst
> > +++ b/Documentation/gpu/drm-mm.rst
> > @@ -380,6 +380,18 @@ GEM CMA Helper Functions Reference
> >  .. kernel-doc:: drivers/gpu/drm/drm_gem_cma_helper.c
> >     :export:
> >  
> > +GEM VRAM Helper Functions Reference
> > +----------------------------------
> > +
> > +.. kernel-doc:: drivers/gpu/drm/drm_gem_vram_helper.c
> > +   :doc: overview
> > +
> > +.. kernel-doc:: include/drm/drm_gem_vram_helper.h
> > +   :internal:
> > +
> > +.. kernel-doc:: drivers/gpu/drm/drm_gem_vram_helper.c
> > +   :export:
> > +
> >  VMA Offset Manager
> >  ==================
> >  
> > diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> > index 2267e84d5cb4..c0d49a6c09d2 100644
> > --- a/drivers/gpu/drm/Kconfig
> > +++ b/drivers/gpu/drm/Kconfig
> > @@ -160,6 +160,12 @@ config DRM_TTM
> >  	  GPU memory types. Will be enabled automatically if a device driver
> >  	  uses it.
> >  
> > +config DRM_VRAM_HELPER
> > +	tristate
> > +	depends on DRM && DRM_TTM
> > +	help
> > +	  Helpers for VRAM memory management
> > +
> >  config DRM_GEM_CMA_HELPER
> >  	bool
> >  	depends on DRM
> > @@ -179,6 +185,13 @@ config DRM_GEM_SHMEM_HELPER
> >  	help
> >  	  Choose this if you need the GEM shmem helper functions
> >  
> > +config DRM_GEM_VRAM_HELPER
> > +	bool
> > +	depends on DRM
> > +	select DRM_VRAM_HELPER
> > +	help
> > +	  Choose this if you need the GEM VRAM helper functions
> > +
> I cannot remember how select will deal with symbols whos has
> a  "depends on".
> But if I recall correct then the "depends on" will be ignored
> or in best case trigger a warning.
> In other words - when we have symbols we select they should not
> have a depends on.
> 
> What can be done is something like:
> 
> symbol foo
> 	bool
> 
> symbol bar
> 	depends on foo
> 
> 
> symbol foobar
> 	bool "This is what you need - select me"
> 	select foo
> 
> So when one chooses "foobar" then we will select "foo" and this will
> satisfy bar.
> 
> But maybe this rambling is irrelevant - maybe check what we do with
> other selectable symbols in DRM.
> 
> 
> > +/**
> > + * DOC: overview
> > + *
> > + * This library provides a GEM object that is backed by VRAM. It
> > + * can be used for simple framebuffer devices with dedicated memory.
> > + */
> It is likely only me, but...
> I could use a short explanation what is GEM and maybe also VRAM.

It exists:

https://dri.freedesktop.org/docs/drm/gpu/drm-mm.html#the-graphics-execution-manager-gem

In the same chapter even where this will be added. I do agree that
explaining a bit more what's meant with VRAM would be good though.
-Daniel

> VRAM as video RAM, but maybe there is more constraints?
> (When I first looked at DRM I wondered what you used Virtual RAM for.
> But thats a long time ago so it counts only as a funny story.

> 
> > + * Buffer-objects helpers
> > + */
> > +
> > +static void drm_gem_vram_cleanup(struct drm_gem_vram_object *gbo)
> > +{
> > +	/* We got here via ttm_bo_put(), which means that the
> > +	 * TTM buffer object in 'bo' has already been cleaned
> > +	 * up; only release the GEM object. */
> > +	drm_gem_object_release(&gbo->gem);
> > +}
> > +
> > +static void drm_gem_vram_destroy(struct drm_gem_vram_object *gbo)
> > +{
> > +	drm_gem_vram_cleanup(gbo);
> > +	kfree(gbo);
> > +}
> > +
> > +static void ttm_buffer_object_destroy(struct ttm_buffer_object *bo)
> > +{
> > +	struct drm_gem_vram_object *gbo = drm_gem_vram_of_bo(bo);
> > +	drm_gem_vram_destroy(gbo);
> > +}
> > +
> > +static void drm_gem_vram_placement(struct drm_gem_vram_object *gbo, int pl_flag)
> > +{
> > +	unsigned int i;
> > +	unsigned int c = 0;
> > +
> > +	gbo->placement.placement = gbo->placements;
> > +	gbo->placement.busy_placement = gbo->placements;
> > +
> > +	if (pl_flag & TTM_PL_FLAG_VRAM)
> > +		gbo->placements[c++].flags = TTM_PL_FLAG_WC |
> > +					     TTM_PL_FLAG_UNCACHED |
> > +					     TTM_PL_FLAG_VRAM;
> > +
> > +	if (pl_flag & TTM_PL_FLAG_SYSTEM)
> > +		gbo->placements[c++].flags = TTM_PL_MASK_CACHING |
> > +					     TTM_PL_FLAG_SYSTEM;
> > +
> > +	if (!c)
> > +		gbo->placements[c++].flags = TTM_PL_MASK_CACHING |
> > +					     TTM_PL_FLAG_SYSTEM;
> > +
> > +	gbo->placement.num_placement = c;
> > +	gbo->placement.num_busy_placement = c;
> > +
> > +	for (i = 0; i < c; ++i) {
> > +		gbo->placements[i].fpfn = 0;
> > +		gbo->placements[i].lpfn = 0;
> > +	}
> > +}
> > +
> > +static int drm_gem_vram_init(struct drm_device *dev,
> > +			     struct ttm_bo_device *bdev,
> > +			     struct drm_gem_vram_object *gbo,
> > +			     unsigned long size, uint32_t pg_align,
> > +			     bool interruptible)
> > +{
> > +	int ret;
> > +	size_t acc_size;
> > +
> > +	ret = drm_gem_object_init(dev, &gbo->gem, size);
> > +	if (ret)
> > +		return ret;
> > +
> > +	acc_size = ttm_bo_dma_acc_size(bdev, size, sizeof(*gbo));
> > +
> > +	gbo->bo.bdev = bdev;
> > +	drm_gem_vram_placement(gbo, TTM_PL_FLAG_VRAM | TTM_PL_FLAG_SYSTEM);
> > +
> > +	ret = ttm_bo_init(bdev, &gbo->bo, size, ttm_bo_type_device,
> > +			  &gbo->placement, pg_align, interruptible, acc_size,
> > +			  NULL, NULL, ttm_buffer_object_destroy);
> > +	if (ret)
> > +		goto err_drm_gem_object_release;
> > +
> > +	return 0;
> > +
> > +err_drm_gem_object_release:
> > +	drm_gem_object_release(&gbo->gem);
> > +	return ret;
> > +}
> > +
> > +/**
> > + * drm_gem_vram_create() - Creates a VRAM-backed GEM object
> > + * @dev:		the DRM device
> > + * @bdev:		the TTM BO device backing the object
> > + * @size:		the buffer size in bytes
> > + * @pg_align:		the buffer's alignment in multiples of the page size
> > + * @interruptible:	sleep interruptible if waiting for memory
> > + *
> > + * Returns:
> > + * A new instance of &struct drm_gem_vram_object on success, or
> > + * an ERR_PTR()-encoded error code otherwise.
> > + */
> > +struct drm_gem_vram_object* drm_gem_vram_create(struct drm_device *dev,
> > +						struct ttm_bo_device *bdev,
> > +						unsigned long size,
> > +						uint32_t pg_align,
> > +						bool interruptible)
> > +{
> > +	struct drm_gem_vram_object *gbo;
> > +	int ret;
> > +
> > +	gbo = kzalloc(sizeof(*gbo), GFP_KERNEL);
> > +	if (!gbo)
> > +		return ERR_PTR(-ENOMEM);
> > +
> > +	ret = drm_gem_vram_init(dev, bdev, gbo, size, pg_align, interruptible);
> > +	if (ret < 0)
> > +		goto err_kfree;
> > +
> > +	return gbo;
> > +
> > +err_kfree:
> > +	kfree(gbo);
> > +	return ERR_PTR(ret);
> > +}
> > +EXPORT_SYMBOL(drm_gem_vram_create);
> > +
> > +/**
> > + * drm_gem_vram_put() - Releases a reference to a VRAM-backed GEM object
> > + * @gbo:	the GEM VRAM object
> > + *
> > + * See ttm_bo_put() for more information.
> > + */
> > +void drm_gem_vram_put(struct drm_gem_vram_object *gbo)
> > +{
> > +	ttm_bo_put(&gbo->bo);
> > +}
> > +EXPORT_SYMBOL(drm_gem_vram_put);
> > +
> > +/**
> > + * drm_gem_vram_reserve() - Reserves a VRAM-backed GEM object
> > + * @gbo:	the GEM VRAM object
> > + * @no_wait:	don't wait for buffer object to become available
> > + *
> > + * See ttm_bo_reserve() for more information.
> > + *
> > + * Returns:
> > + * 0 on success, or
> > + * a negative error code otherwise
> > + */
> > +int drm_gem_vram_reserve(struct drm_gem_vram_object *gbo, bool no_wait)
> > +{
> > +	return ttm_bo_reserve(&gbo->bo, true, no_wait, NULL);
> > +}
> > +EXPORT_SYMBOL(drm_gem_vram_reserve);
> > +
> > +/**
> > + * drm_gem_vram_unreserve() - \
> > +	Release a reservation acquired by drm_gem_vram_reserve()
> > + * @gbo:	the GEM VRAM object
> > + *
> > + * See ttm_bo_unreserve() for more information.
> > + */
> > +void drm_gem_vram_unreserve(struct drm_gem_vram_object *gbo)
> > +{
> > +	ttm_bo_unreserve(&gbo->bo);
> > +}
> > +EXPORT_SYMBOL(drm_gem_vram_unreserve);
> > +
> > +/**
> > + * drm_gem_vram_mmap_offset() - Returns a GEM VRAM object's mmap offset
> > + * @gbo:	the GEM VRAM object
> > + *
> > + * See drm_vma_node_offset_addr() for more information.
> > + *
> > + * Returns:
> > + * The buffer object's offset for userspace mappings on success, or
> > + * 0 if no offset is allocated.
> > + */
> > +u64 drm_gem_vram_mmap_offset(struct drm_gem_vram_object *gbo)
> > +{
> > +	return drm_vma_node_offset_addr(&gbo->bo.vma_node);
> > +}
> > +EXPORT_SYMBOL(drm_gem_vram_mmap_offset);
> > +
> > +/**
> > + * drm_gem_vram_offset() - \
> > +	Returns a GEM VRAM object's offset in video memory
> > + * @gbo:	the GEM VRAM object
> > + *
> > + * This function returns the buffer object's offset in the device's video
> > + * memory. The buffer object has to be pinned to %TTM_PL_VRAM.
> > + *
> > + * Returns:
> > + * The buffer object's offset in video memory on success, or
> > + * a negative error code otherwise.
> > + */
> > +s64 drm_gem_vram_offset(struct drm_gem_vram_object *gbo)
> > +{
> > +	if (!gbo->pin_count)
> > +		return (s64)-ENODEV;
> > +	return gbo->bo.offset;
> > +}
> > +EXPORT_SYMBOL(drm_gem_vram_offset);
> > +
> > +/**
> > + * drm_gem_vram_pin() - Pins a GEM VRAM object in a region.
> > + * @gbo:	the GEM VRAM object
> > + * @pl_flag:	a bitmask of possible memory regions
> > + *
> > + * Pinning a buffer object ensures that it is not evicted from
> > + * a memory region. A pinned buffer object has to be unpinned before
> > + * it can be pinned to another region.
> > + *
> > + * Returns:
> > + * 0 on success, or
> > + * a negative error code otherwise.
> > + */
> > +int drm_gem_vram_pin(struct drm_gem_vram_object *gbo, u32 pl_flag)
> > +{
> > +	int i, ret;
> > +	struct ttm_operation_ctx ctx = { false, false };
> > +
> > +	if (gbo->pin_count) {
> > +		++gbo->pin_count;
> > +		return 0;
> > +	}
> > +
> > +	drm_gem_vram_placement(gbo, pl_flag);
> > +	for (i = 0; i < gbo->placement.num_placement; ++i)
> > +		gbo->placements[i].flags |= TTM_PL_FLAG_NO_EVICT;
> > +
> > +	ret = ttm_bo_validate(&gbo->bo, &gbo->placement, &ctx);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	gbo->pin_count = 1;
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL(drm_gem_vram_pin);
> > +
> > +/**
> > + * drm_gem_vram_unpin() - Unpins a GEM VRAM object
> > + * @gbo:	the GEM VRAM object
> > + *
> > + * Returns:
> > + * 0 on success, or
> > + * a negative error code otherwise.
> > + */
> > +int drm_gem_vram_unpin(struct drm_gem_vram_object *gbo)
> > +{
> > +	int i, ret;
> > +	struct ttm_operation_ctx ctx = { false, false };
> > +
> > +	if (!gbo->pin_count)
> > +		return 0;
> > +
> > +	--gbo->pin_count;
> > +	if (gbo->pin_count)
> > +		return 0;
> > +
> > +	for (i = 0; i < gbo->placement.num_placement ; ++i)
> > +		gbo->placements[i].flags &= ~TTM_PL_FLAG_NO_EVICT;
> > +
> > +	ret = ttm_bo_validate(&gbo->bo, &gbo->placement, &ctx);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL(drm_gem_vram_unpin);
> > +
> > +/**
> > + * drm_gem_vram_push_to_system() - \
> > +	Unpins a GEM VRAM object and moves it to system memory
> > + * @gbo:	the GEM VRAM object
> > + *
> > + * This operation only works if the caller holds the final pin on the
> > + * buffer object.
> > + *
> > + * Returns:
> > + * 0 on success, or
> > + * a negative error code otherwise.
> > + */
> > +int drm_gem_vram_push_to_system(struct drm_gem_vram_object *gbo)
> > +{
> > +	int i, ret;
> > +	struct ttm_operation_ctx ctx = { false, false };
> > +
> > +	if (!gbo->pin_count)
> > +		return 0;
> > +
> > +	--gbo->pin_count;
> > +	if (gbo->pin_count)
> > +		return 0;
> > +
> > +	if (gbo->kmap.virtual)
> > +		ttm_bo_kunmap(&gbo->kmap);
> > +
> > +	drm_gem_vram_placement(gbo, TTM_PL_FLAG_SYSTEM);
> > +	for (i = 0; i < gbo->placement.num_placement ; ++i)
> > +		gbo->placements[i].flags |= TTM_PL_FLAG_NO_EVICT;
> > +
> > +	ret = ttm_bo_validate(&gbo->bo, &gbo->placement, &ctx);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL(drm_gem_vram_push_to_system);
> > +
> > +/**
> > + * drm_gem_vram_kmap_at() - Maps a GEM VRAM object into kernel address space
> > + * @gbo:	the GEM VRAM object
> > + * @map:	establish a mapping if necessary
> > + * @is_iomem:	returns true if the mapped memory is I/O memory, or false \
> > +	otherwise; can be NULL
> > + * @kmap:	the mapping's kmap object
> > + *
> > + * This function maps the buffer object into the kernel's address space
> > + * or returns the current mapping. If the parameter map is false, the
> > + * function only queries the current mapping, but does not establish a
> > + * new one.
> > + *
> > + * Returns:
> > + * The buffers virtual address if mapped, or
> > + * NULL if not mapped, or
> > + * an ERR_PTR()-encoded error code otherwise.
> > + */
> > +void* drm_gem_vram_kmap_at(struct drm_gem_vram_object *gbo, bool map,
> > +			   bool *is_iomem, struct ttm_bo_kmap_obj *kmap)
> > +{
> > +	int ret;
> > +
> > +	if (kmap->virtual || !map)
> > +		goto out;
> > +
> > +	ret = ttm_bo_kmap(&gbo->bo, 0, gbo->bo.num_pages, kmap);
> > +	if (ret)
> > +		return ERR_PTR(ret);
> > +
> > +out:
> > +	if (!is_iomem) {
> > +		return kmap->virtual;
> > +	}
> > +	if (!kmap->virtual) {
> > +		*is_iomem = false;
> > +		return NULL;
> > +	}
> > +	return ttm_kmap_obj_virtual(kmap, is_iomem);
> > +}
> > +EXPORT_SYMBOL(drm_gem_vram_kmap_at);
> > +
> > +/**
> > + * drm_gem_vram_kmap() - Maps a GEM VRAM object into kernel address space
> > + * @gbo:	the GEM VRAM object
> > + * @map:	establish a mapping if necessary
> > + * @is_iomem:	returns true if the mapped memory is I/O memory, or false \
> > +	otherwise; can be NULL
> > + *
> > + * This function maps the buffer object into the kernel's address space
> > + * or returns the current mapping. If the parameter map is false, the
> > + * function only queries the current mapping, but does not establish a
> > + * new one.
> > + *
> > + * Returns:
> > + * The buffers virtual address if mapped, or
> > + * NULL if not mapped, or
> > + * an ERR_PTR()-encoded error code otherwise.
> > + */
> > +void* drm_gem_vram_kmap(struct drm_gem_vram_object *gbo, bool map,
> > +			bool *is_iomem)
> > +{
> > +	return drm_gem_vram_kmap_at(gbo, map, is_iomem, &gbo->kmap);
> > +}
> > +EXPORT_SYMBOL(drm_gem_vram_kmap);
> > +
> > +/**
> > + * drm_gem_vram_kunmap_at() - Unmaps a GEM VRAM object
> > + * @gbo:	the GEM VRAM object
> > + * @kmap:	the mapping's kmap object
> > + */
> > +void drm_gem_vram_kunmap_at(struct drm_gem_vram_object *gbo,
> > +			    struct ttm_bo_kmap_obj *kmap)
> > +{
> > +	if (!kmap->virtual)
> > +		return;
> > +
> > +	ttm_bo_kunmap(kmap);
> > +	kmap->virtual = NULL;
> > +}
> > +EXPORT_SYMBOL(drm_gem_vram_kunmap_at);
> > +
> > +/**
> > + * drm_gem_vram_kunmap() - Unmaps a GEM VRAM object
> > + * @gbo:	the GEM VRAM object
> > + */
> > +void drm_gem_vram_kunmap(struct drm_gem_vram_object *gbo)
> > +{
> > +	drm_gem_vram_kunmap_at(gbo, &gbo->kmap);
> > +}
> > +EXPORT_SYMBOL(drm_gem_vram_kunmap);
> > diff --git a/drivers/gpu/drm/drm_vram_helper_common.c b/drivers/gpu/drm/drm_vram_helper_common.c
> > new file mode 100644
> > index 000000000000..76b6569c9aad
> > --- /dev/null
> > +++ b/drivers/gpu/drm/drm_vram_helper_common.c
> > @@ -0,0 +1,6 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +
> > +#include <linux/module.h>
> > +
> > +MODULE_DESCRIPTION("DRM VRAM memory-management helpers");
> > +MODULE_LICENSE("GPL");
> > diff --git a/include/drm/drm_gem_vram_helper.h b/include/drm/drm_gem_vram_helper.h
> > new file mode 100644
> > index 000000000000..167616f552e5
> > --- /dev/null
> > +++ b/include/drm/drm_gem_vram_helper.h
> > @@ -0,0 +1,92 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +
> > +#ifndef DRM_GEM_VRAM_HELPER_H
> > +#define DRM_GEM_VRAM_HELPER_H
> > +
> > +#include <drm/drm_gem.h>
> > +#include <drm/ttm/ttm_bo_api.h>
> > +#include <drm/ttm/ttm_placement.h>
> > +#include <linux/kernel.h> /* for container_of() */
> > +
> > +struct filp;
> > +
> > +#define DRM_GEM_VRAM_PL_FLAG_VRAM	TTM_PL_FLAG_VRAM
> > +#define DRM_GEM_VRAM_PL_FLAG_SYSTEM	TTM_PL_FLAG_SYSTEM
> > +
> > +/*
> > + * Buffer-object helpers
> > + */
> > +
> > +/**
> > + * struct drm_gem_vram_object - GEM object backed by VRAM
> > + * @gem:	GEM object
> > + * @bo:		TTM buffer object
> > + * @kmap:	Mapping information for @bo
> > + * @placement:	TTM placement information. Supported placements are \
> > +	%TTM_PL_VRAM and %TTM_PL_SYSTEM
> > + * @placements:	TTM placement information.
> > + * @pin_count:	Pin counter
> > + *
> > + * The type struct drm_gem_vram_object represents a GEM object that is
> > + * backed by VRAM. It can be used for simple frambuffer devices with
> > + * dedicated memory. The buffer object can be evicted to system memory if
> > + * video memory becomes scarce.
> > + */
> > +struct drm_gem_vram_object {
> > +        struct drm_gem_object gem;
> > +        struct ttm_buffer_object bo;
> > +        struct ttm_bo_kmap_obj kmap;
> > +
> > +	/* Supported placements are %TTM_PL_VRAM and %TTM_PL_SYSTEM */
> > +        struct ttm_placement placement;
> > +        struct ttm_place placements[3];
> > +
> > +        int pin_count;
> > +};
> Use tabs for indent - not spaces.
> Ask checkpatch if anything similar needs to be adjusted.
> 
> 
> > +
> > +/**
> > + * Returns the container of type &struct drm_gem_vram_object
> > + * for field bo.
> > + * @bo:		the VRAM buffer object
> > + * Returns:	The containing GEM VRAM object
> > + */
> > +static inline struct drm_gem_vram_object* drm_gem_vram_of_bo(
> > +	struct ttm_buffer_object *bo)
> > +{
> > +	return container_of(bo, struct drm_gem_vram_object, bo);
> > +}
> Indent funny. USe same indent as used in other parts of file for
> function arguments.
> 
> > +
> > +/**
> > + * Returns the container of type &struct drm_gem_vram_object
> > + * for field gem.
> > + * @gem:	the GEM object
> > + * Returns:	The containing GEM VRAM object
> > + */
> > +static inline struct drm_gem_vram_object* drm_gem_vram_of_gem(
> > +	struct drm_gem_object *gem)
> > +{
> > +	return container_of(gem, struct drm_gem_vram_object, gem);
> > +}
> ditto
> 
> > +
> > +struct drm_gem_vram_object* drm_gem_vram_create(struct drm_device *dev,
> > +						struct ttm_bo_device* bdev,
> > +						unsigned long size,
> > +						uint32_t pg_align,
> > +						bool interruptible);
> 
> Here is is "normal"
>

Thomas Zimmermann May 3, 2019, 10:14 a.m. UTC | #7

Hi Christian,

would you review the whole patch set? Daniel mentioned that he'd prefer
to leave the review to memory-mgmt developers.

Best regards
Thomas

Am 30.04.19 um 11:35 schrieb Koenig, Christian:
> Am 30.04.19 um 11:23 schrieb Sam Ravnborg:
>> [CAUTION: External Email]
>>
>> Hi Thomas.
>>
>>>>> +
>>>>> +/**
>>>>> + * Returns the container of type &struct drm_gem_vram_object
>>>>> + * for field bo.
>>>>> + * @bo:           the VRAM buffer object
>>>>> + * Returns:       The containing GEM VRAM object
>>>>> + */
>>>>> +static inline struct drm_gem_vram_object* drm_gem_vram_of_bo(
>>>>> +  struct ttm_buffer_object *bo)
>>>>> +{
>>>>> +  return container_of(bo, struct drm_gem_vram_object, bo);
>>>>> +}
>>>> Indent funny. USe same indent as used in other parts of file for
>>>> function arguments.
>>> If I put the argument next to the function's name, it will exceed the
>>> 80-character limit. From the coding-style document, I could not see what
>>> to do in this case. One solution would move the return type to a
>>> separate line before the function name. I've not seen that anywhere in
>>> the source code, so moving the argument onto a separate line and
>>> indenting by one tab appears to be the next best solution. Please let me
>>> know if there's if there's a preferred style for cases like this one.
>> Readability has IMO higher priority than some limit of 80 chars.
>> And it hurts readability (at least my OCD) when style changes
>> as you do with indent here. So my personal preference is to fix
>> indent and accect longer lines.
> 
> In this case the an often used convention (which is also kind of 
> readable) is to add a newline after the return values, but before the 
> function name. E.g. something like this:
> 
> static inline struct drm_gem_vram_object*
> drm_gem_vram_of_bo(struct ttm_buffer_object *bo)
> 
> Regards,
> Christian.
> 
>>
>> But you ask for a preferred style - which I do not think we have in this
>> case. So it boils down to what you prefer.
>>
>> Enough bikeshedding, thanks for the quick response.
>>
>>          Sam
>

Daniel Vetter May 3, 2019, 12:01 p.m. UTC | #8

On Fri, May 3, 2019 at 12:15 PM Thomas Zimmermann <[e-mail]> wrote:
>
> Hi Christian,
>
> would you review the whole patch set? Daniel mentioned that he'd prefer
> to leave the review to memory-mgmt developers.

I think Noralf Tronnes or Gerd Hoffmann would also make good reviewers
for this, fairly close to what they've been working on in the past.
-Daniel

>
> Best regards
> Thomas
>
> Am 30.04.19 um 11:35 schrieb Koenig, Christian:
> > Am 30.04.19 um 11:23 schrieb Sam Ravnborg:
> >> [CAUTION: External Email]
> >>
> >> Hi Thomas.
> >>
> >>>>> +
> >>>>> +/**
> >>>>> + * Returns the container of type &struct drm_gem_vram_object
> >>>>> + * for field bo.
> >>>>> + * @bo:           the VRAM buffer object
> >>>>> + * Returns:       The containing GEM VRAM object
> >>>>> + */
> >>>>> +static inline struct drm_gem_vram_object* drm_gem_vram_of_bo(
> >>>>> +  struct ttm_buffer_object *bo)
> >>>>> +{
> >>>>> +  return container_of(bo, struct drm_gem_vram_object, bo);
> >>>>> +}
> >>>> Indent funny. USe same indent as used in other parts of file for
> >>>> function arguments.
> >>> If I put the argument next to the function's name, it will exceed the
> >>> 80-character limit. From the coding-style document, I could not see what
> >>> to do in this case. One solution would move the return type to a
> >>> separate line before the function name. I've not seen that anywhere in
> >>> the source code, so moving the argument onto a separate line and
> >>> indenting by one tab appears to be the next best solution. Please let me
> >>> know if there's if there's a preferred style for cases like this one.
> >> Readability has IMO higher priority than some limit of 80 chars.
> >> And it hurts readability (at least my OCD) when style changes
> >> as you do with indent here. So my personal preference is to fix
> >> indent and accect longer lines.
> >
> > In this case the an often used convention (which is also kind of
> > readable) is to add a newline after the return values, but before the
> > function name. E.g. something like this:
> >
> > static inline struct drm_gem_vram_object*
> > drm_gem_vram_of_bo(struct ttm_buffer_object *bo)
> >
> > Regards,
> > Christian.
> >
> >>
> >> But you ask for a preferred style - which I do not think we have in this
> >> case. So it boils down to what you prefer.
> >>
> >> Enough bikeshedding, thanks for the quick response.
> >>
> >>          Sam
> >
>
> --
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Linux GmbH, Maxfeldstrasse 5, 90409 Nuernberg, Germany
> GF: Felix Imendörffer, Mary Higgins, Sri Rasiah
> HRB 21284 (AG Nürnberg)
>
> _______________________________________________
> dri-devel mailing list
> [e-mail]
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Christian König May 3, 2019, 12:07 p.m. UTC | #9

Am 03.05.19 um 14:01 schrieb Daniel Vetter:
> [CAUTION: External Email]
>
> On Fri, May 3, 2019 at 12:15 PM Thomas Zimmermann <[e-mail]> wrote:
>> Hi Christian,
>>
>> would you review the whole patch set? Daniel mentioned that he'd prefer
>> to leave the review to memory-mgmt developers.
> I think Noralf Tronnes or Gerd Hoffmann would also make good reviewers
> for this, fairly close to what they've been working on in the past.

I will try to take another look next week. Busy as usual here.

Christian.

> -Daniel
>
>> Best regards
>> Thomas
>>
>> Am 30.04.19 um 11:35 schrieb Koenig, Christian:
>>> Am 30.04.19 um 11:23 schrieb Sam Ravnborg:
>>>> [CAUTION: External Email]
>>>>
>>>> Hi Thomas.
>>>>
>>>>>>> +
>>>>>>> +/**
>>>>>>> + * Returns the container of type &struct drm_gem_vram_object
>>>>>>> + * for field bo.
>>>>>>> + * @bo:           the VRAM buffer object
>>>>>>> + * Returns:       The containing GEM VRAM object
>>>>>>> + */
>>>>>>> +static inline struct drm_gem_vram_object* drm_gem_vram_of_bo(
>>>>>>> +  struct ttm_buffer_object *bo)
>>>>>>> +{
>>>>>>> +  return container_of(bo, struct drm_gem_vram_object, bo);
>>>>>>> +}
>>>>>> Indent funny. USe same indent as used in other parts of file for
>>>>>> function arguments.
>>>>> If I put the argument next to the function's name, it will exceed the
>>>>> 80-character limit. From the coding-style document, I could not see what
>>>>> to do in this case. One solution would move the return type to a
>>>>> separate line before the function name. I've not seen that anywhere in
>>>>> the source code, so moving the argument onto a separate line and
>>>>> indenting by one tab appears to be the next best solution. Please let me
>>>>> know if there's if there's a preferred style for cases like this one.
>>>> Readability has IMO higher priority than some limit of 80 chars.
>>>> And it hurts readability (at least my OCD) when style changes
>>>> as you do with indent here. So my personal preference is to fix
>>>> indent and accect longer lines.
>>> In this case the an often used convention (which is also kind of
>>> readable) is to add a newline after the return values, but before the
>>> function name. E.g. something like this:
>>>
>>> static inline struct drm_gem_vram_object*
>>> drm_gem_vram_of_bo(struct ttm_buffer_object *bo)
>>>
>>> Regards,
>>> Christian.
>>>
>>>> But you ask for a preferred style - which I do not think we have in this
>>>> case. So it boils down to what you prefer.
>>>>
>>>> Enough bikeshedding, thanks for the quick response.
>>>>
>>>>           Sam
>> --
>> Thomas Zimmermann
>> Graphics Driver Developer
>> SUSE Linux GmbH, Maxfeldstrasse 5, 90409 Nuernberg, Germany
>> GF: Felix Imendörffer, Mary Higgins, Sri Rasiah
>> HRB 21284 (AG Nürnberg)
>>
>> _______________________________________________
>> dri-devel mailing list
>> [e-mail]
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> [phone] - http://blog.ffwll.ch

Thomas Zimmermann May 3, 2019, 12:27 p.m. UTC | #10

cc: [e-mail]

Am 03.05.19 um 14:07 schrieb Koenig, Christian:
> Am 03.05.19 um 14:01 schrieb Daniel Vetter:
>> [CAUTION: External Email]
>>
>> On Fri, May 3, 2019 at 12:15 PM Thomas Zimmermann <[e-mail]> wrote:
>>> Hi Christian,
>>>
>>> would you review the whole patch set? Daniel mentioned that he'd prefer
>>> to leave the review to memory-mgmt developers.
>> I think Noralf Tronnes or Gerd Hoffmann would also make good reviewers
>> for this, fairly close to what they've been working on in the past.
> 
> I will try to take another look next week. Busy as usual here.

Thanks, I'll post v4 of the patches early next week.

> Christian.
> 
>> -Daniel
>>
>>> Best regards
>>> Thomas
>>>
>>> Am 30.04.19 um 11:35 schrieb Koenig, Christian:
>>>> Am 30.04.19 um 11:23 schrieb Sam Ravnborg:
>>>>> [CAUTION: External Email]
>>>>>
>>>>> Hi Thomas.
>>>>>
>>>>>>>> +
>>>>>>>> +/**
>>>>>>>> + * Returns the container of type &struct drm_gem_vram_object
>>>>>>>> + * for field bo.
>>>>>>>> + * @bo:           the VRAM buffer object
>>>>>>>> + * Returns:       The containing GEM VRAM object
>>>>>>>> + */
>>>>>>>> +static inline struct drm_gem_vram_object* drm_gem_vram_of_bo(
>>>>>>>> +  struct ttm_buffer_object *bo)
>>>>>>>> +{
>>>>>>>> +  return container_of(bo, struct drm_gem_vram_object, bo);
>>>>>>>> +}
>>>>>>> Indent funny. USe same indent as used in other parts of file for
>>>>>>> function arguments.
>>>>>> If I put the argument next to the function's name, it will exceed the
>>>>>> 80-character limit. From the coding-style document, I could not see what
>>>>>> to do in this case. One solution would move the return type to a
>>>>>> separate line before the function name. I've not seen that anywhere in
>>>>>> the source code, so moving the argument onto a separate line and
>>>>>> indenting by one tab appears to be the next best solution. Please let me
>>>>>> know if there's if there's a preferred style for cases like this one.
>>>>> Readability has IMO higher priority than some limit of 80 chars.
>>>>> And it hurts readability (at least my OCD) when style changes
>>>>> as you do with indent here. So my personal preference is to fix
>>>>> indent and accect longer lines.
>>>> In this case the an often used convention (which is also kind of
>>>> readable) is to add a newline after the return values, but before the
>>>> function name. E.g. something like this:
>>>>
>>>> static inline struct drm_gem_vram_object*
>>>> drm_gem_vram_of_bo(struct ttm_buffer_object *bo)
>>>>
>>>> Regards,
>>>> Christian.
>>>>
>>>>> But you ask for a preferred style - which I do not think we have in this
>>>>> case. So it boils down to what you prefer.
>>>>>
>>>>> Enough bikeshedding, thanks for the quick response.
>>>>>
>>>>>           Sam
>>> --
>>> Thomas Zimmermann
>>> Graphics Driver Developer
>>> SUSE Linux GmbH, Maxfeldstrasse 5, 90409 Nuernberg, Germany
>>> GF: Felix Imendörffer, Mary Higgins, Sri Rasiah
>>> HRB 21284 (AG Nürnberg)
>>>
>>> _______________________________________________
>>> dri-devel mailing list
>>> [e-mail]
>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>
>>
>> --
>> Daniel Vetter
>> Software Engineer, Intel Corporation
>> [phone] - http://blog.ffwll.ch
>

Thomas Zimmermann May 3, 2019, 1:29 p.m. UTC | #11

Am 03.05.19 um 14:27 schrieb Thomas Zimmermann:
> cc: [e-mail]

Actually cc him

> Am 03.05.19 um 14:07 schrieb Koenig, Christian:
>> Am 03.05.19 um 14:01 schrieb Daniel Vetter:
>>> [CAUTION: External Email]
>>>
>>> On Fri, May 3, 2019 at 12:15 PM Thomas Zimmermann <[e-mail]> wrote:
>>>> Hi Christian,
>>>>
>>>> would you review the whole patch set? Daniel mentioned that he'd prefer
>>>> to leave the review to memory-mgmt developers.
>>> I think Noralf Tronnes or Gerd Hoffmann would also make good reviewers
>>> for this, fairly close to what they've been working on in the past.
>>
>> I will try to take another look next week. Busy as usual here.
> 
> Thanks, I'll post v4 of the patches early next week.
> 
>> Christian.
>>
>>> -Daniel
>>>
>>>> Best regards
>>>> Thomas
>>>>
>>>> Am 30.04.19 um 11:35 schrieb Koenig, Christian:
>>>>> Am 30.04.19 um 11:23 schrieb Sam Ravnborg:
>>>>>> [CAUTION: External Email]
>>>>>>
>>>>>> Hi Thomas.
>>>>>>
>>>>>>>>> +
>>>>>>>>> +/**
>>>>>>>>> + * Returns the container of type &struct drm_gem_vram_object
>>>>>>>>> + * for field bo.
>>>>>>>>> + * @bo:           the VRAM buffer object
>>>>>>>>> + * Returns:       The containing GEM VRAM object
>>>>>>>>> + */
>>>>>>>>> +static inline struct drm_gem_vram_object* drm_gem_vram_of_bo(
>>>>>>>>> +  struct ttm_buffer_object *bo)
>>>>>>>>> +{
>>>>>>>>> +  return container_of(bo, struct drm_gem_vram_object, bo);
>>>>>>>>> +}
>>>>>>>> Indent funny. USe same indent as used in other parts of file for
>>>>>>>> function arguments.
>>>>>>> If I put the argument next to the function's name, it will exceed the
>>>>>>> 80-character limit. From the coding-style document, I could not see what
>>>>>>> to do in this case. One solution would move the return type to a
>>>>>>> separate line before the function name. I've not seen that anywhere in
>>>>>>> the source code, so moving the argument onto a separate line and
>>>>>>> indenting by one tab appears to be the next best solution. Please let me
>>>>>>> know if there's if there's a preferred style for cases like this one.
>>>>>> Readability has IMO higher priority than some limit of 80 chars.
>>>>>> And it hurts readability (at least my OCD) when style changes
>>>>>> as you do with indent here. So my personal preference is to fix
>>>>>> indent and accect longer lines.
>>>>> In this case the an often used convention (which is also kind of
>>>>> readable) is to add a newline after the return values, but before the
>>>>> function name. E.g. something like this:
>>>>>
>>>>> static inline struct drm_gem_vram_object*
>>>>> drm_gem_vram_of_bo(struct ttm_buffer_object *bo)
>>>>>
>>>>> Regards,
>>>>> Christian.
>>>>>
>>>>>> But you ask for a preferred style - which I do not think we have in this
>>>>>> case. So it boils down to what you prefer.
>>>>>>
>>>>>> Enough bikeshedding, thanks for the quick response.
>>>>>>
>>>>>>           Sam
>>>> --
>>>> Thomas Zimmermann
>>>> Graphics Driver Developer
>>>> SUSE Linux GmbH, Maxfeldstrasse 5, 90409 Nuernberg, Germany
>>>> GF: Felix Imendörffer, Mary Higgins, Sri Rasiah
>>>> HRB 21284 (AG Nürnberg)
>>>>
>>>> _______________________________________________
>>>> dri-devel mailing list
>>>> [e-mail]
>>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>>
>>>
>>> --
>>> Daniel Vetter
>>> Software Engineer, Intel Corporation
>>> [phone] - http://blog.ffwll.ch
>>
> 
> 
> _______________________________________________
> dri-devel mailing list
> [e-mail]
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>

Patch

diff --git a/Documentation/gpu/drm-mm.rst b/Documentation/gpu/drm-mm.rst
index 54a696d961a7..d5327ed608d7 100644
--- a/Documentation/gpu/drm-mm.rst
+++ b/Documentation/gpu/drm-mm.rst
@@ -380,6 +380,18 @@  GEM CMA Helper Functions Reference
 .. kernel-doc:: drivers/gpu/drm/drm_gem_cma_helper.c
    :export:
 
+GEM VRAM Helper Functions Reference
+----------------------------------
+
+.. kernel-doc:: drivers/gpu/drm/drm_gem_vram_helper.c
+   :doc: overview
+
+.. kernel-doc:: include/drm/drm_gem_vram_helper.h
+   :internal:
+
+.. kernel-doc:: drivers/gpu/drm/drm_gem_vram_helper.c
+   :export:
+
 VMA Offset Manager
 ==================
 
diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index 2267e84d5cb4..c0d49a6c09d2 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -160,6 +160,12 @@  config DRM_TTM
 	  GPU memory types. Will be enabled automatically if a device driver
 	  uses it.
 
+config DRM_VRAM_HELPER
+	tristate
+	depends on DRM && DRM_TTM
+	help
+	  Helpers for VRAM memory management
+
 config DRM_GEM_CMA_HELPER
 	bool
 	depends on DRM
@@ -179,6 +185,13 @@  config DRM_GEM_SHMEM_HELPER
 	help
 	  Choose this if you need the GEM shmem helper functions
 
+config DRM_GEM_VRAM_HELPER
+	bool
+	depends on DRM
+	select DRM_VRAM_HELPER
+	help
+	  Choose this if you need the GEM VRAM helper functions
+
 config DRM_VM
 	bool
 	depends on DRM && MMU
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index 72f5036d9bfa..dbe38fe1bcb3 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -32,6 +32,10 @@  drm-$(CONFIG_AGP) += drm_agpsupport.o
 drm-$(CONFIG_DEBUG_FS) += drm_debugfs.o drm_debugfs_crc.o
 drm-$(CONFIG_DRM_LOAD_EDID_FIRMWARE) += drm_edid_load.o
 
+drm_vram_helper-y := drm_vram_helper_common.o
+drm_vram_helper-$(CONFIG_DRM_GEM_VRAM_HELPER) += drm_gem_vram_helper.o
+obj-$(CONFIG_DRM_VRAM_HELPER) += drm_vram_helper.o
+
 drm_kms_helper-y := drm_crtc_helper.o drm_dp_helper.o drm_dsc.o drm_probe_helper.o \
 		drm_plane_helper.o drm_dp_mst_topology.o drm_atomic_helper.o \
 		drm_kms_helper_common.o drm_dp_dual_mode_helper.o \
diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c b/drivers/gpu/drm/drm_gem_vram_helper.c
new file mode 100644
index 000000000000..d39d8a5f36df
--- /dev/null
+++ b/drivers/gpu/drm/drm_gem_vram_helper.c
@@ -0,0 +1,410 @@ 
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+
+#include <drm/drm_gem_vram_helper.h>
+#include <drm/ttm/ttm_page_alloc.h>
+
+/**
+ * DOC: overview
+ *
+ * This library provides a GEM object that is backed by VRAM. It
+ * can be used for simple framebuffer devices with dedicated memory.
+ */
+
+/*
+ * Buffer-objects helpers
+ */
+
+static void drm_gem_vram_cleanup(struct drm_gem_vram_object *gbo)
+{
+	/* We got here via ttm_bo_put(), which means that the
+	 * TTM buffer object in 'bo' has already been cleaned
+	 * up; only release the GEM object. */
+	drm_gem_object_release(&gbo->gem);
+}
+
+static void drm_gem_vram_destroy(struct drm_gem_vram_object *gbo)
+{
+	drm_gem_vram_cleanup(gbo);
+	kfree(gbo);
+}
+
+static void ttm_buffer_object_destroy(struct ttm_buffer_object *bo)
+{
+	struct drm_gem_vram_object *gbo = drm_gem_vram_of_bo(bo);
+	drm_gem_vram_destroy(gbo);
+}
+
+static void drm_gem_vram_placement(struct drm_gem_vram_object *gbo, int pl_flag)
+{
+	unsigned int i;
+	unsigned int c = 0;
+
+	gbo->placement.placement = gbo->placements;
+	gbo->placement.busy_placement = gbo->placements;
+
+	if (pl_flag & TTM_PL_FLAG_VRAM)
+		gbo->placements[c++].flags = TTM_PL_FLAG_WC |
+					     TTM_PL_FLAG_UNCACHED |
+					     TTM_PL_FLAG_VRAM;
+
+	if (pl_flag & TTM_PL_FLAG_SYSTEM)
+		gbo->placements[c++].flags = TTM_PL_MASK_CACHING |
+					     TTM_PL_FLAG_SYSTEM;
+
+	if (!c)
+		gbo->placements[c++].flags = TTM_PL_MASK_CACHING |
+					     TTM_PL_FLAG_SYSTEM;
+
+	gbo->placement.num_placement = c;
+	gbo->placement.num_busy_placement = c;
+
+	for (i = 0; i < c; ++i) {
+		gbo->placements[i].fpfn = 0;
+		gbo->placements[i].lpfn = 0;
+	}
+}
+
+static int drm_gem_vram_init(struct drm_device *dev,
+			     struct ttm_bo_device *bdev,
+			     struct drm_gem_vram_object *gbo,
+			     unsigned long size, uint32_t pg_align,
+			     bool interruptible)
+{
+	int ret;
+	size_t acc_size;
+
+	ret = drm_gem_object_init(dev, &gbo->gem, size);
+	if (ret)
+		return ret;
+
+	acc_size = ttm_bo_dma_acc_size(bdev, size, sizeof(*gbo));
+
+	gbo->bo.bdev = bdev;
+	drm_gem_vram_placement(gbo, TTM_PL_FLAG_VRAM | TTM_PL_FLAG_SYSTEM);
+
+	ret = ttm_bo_init(bdev, &gbo->bo, size, ttm_bo_type_device,
+			  &gbo->placement, pg_align, interruptible, acc_size,
+			  NULL, NULL, ttm_buffer_object_destroy);
+	if (ret)
+		goto err_drm_gem_object_release;
+
+	return 0;
+
+err_drm_gem_object_release:
+	drm_gem_object_release(&gbo->gem);
+	return ret;
+}
+
+/**
+ * drm_gem_vram_create() - Creates a VRAM-backed GEM object
+ * @dev:		the DRM device
+ * @bdev:		the TTM BO device backing the object
+ * @size:		the buffer size in bytes
+ * @pg_align:		the buffer's alignment in multiples of the page size
+ * @interruptible:	sleep interruptible if waiting for memory
+ *
+ * Returns:
+ * A new instance of &struct drm_gem_vram_object on success, or
+ * an ERR_PTR()-encoded error code otherwise.
+ */
+struct drm_gem_vram_object* drm_gem_vram_create(struct drm_device *dev,
+						struct ttm_bo_device *bdev,
+						unsigned long size,
+						uint32_t pg_align,
+						bool interruptible)
+{
+	struct drm_gem_vram_object *gbo;
+	int ret;
+
+	gbo = kzalloc(sizeof(*gbo), GFP_KERNEL);
+	if (!gbo)
+		return ERR_PTR(-ENOMEM);
+
+	ret = drm_gem_vram_init(dev, bdev, gbo, size, pg_align, interruptible);
+	if (ret < 0)
+		goto err_kfree;
+
+	return gbo;
+
+err_kfree:
+	kfree(gbo);
+	return ERR_PTR(ret);
+}
+EXPORT_SYMBOL(drm_gem_vram_create);
+
+/**
+ * drm_gem_vram_put() - Releases a reference to a VRAM-backed GEM object
+ * @gbo:	the GEM VRAM object
+ *
+ * See ttm_bo_put() for more information.
+ */
+void drm_gem_vram_put(struct drm_gem_vram_object *gbo)
+{
+	ttm_bo_put(&gbo->bo);
+}
+EXPORT_SYMBOL(drm_gem_vram_put);
+
+/**
+ * drm_gem_vram_reserve() - Reserves a VRAM-backed GEM object
+ * @gbo:	the GEM VRAM object
+ * @no_wait:	don't wait for buffer object to become available
+ *
+ * See ttm_bo_reserve() for more information.
+ *
+ * Returns:
+ * 0 on success, or
+ * a negative error code otherwise
+ */
+int drm_gem_vram_reserve(struct drm_gem_vram_object *gbo, bool no_wait)
+{
+	return ttm_bo_reserve(&gbo->bo, true, no_wait, NULL);
+}
+EXPORT_SYMBOL(drm_gem_vram_reserve);
+
+/**
+ * drm_gem_vram_unreserve() - \
+	Release a reservation acquired by drm_gem_vram_reserve()
+ * @gbo:	the GEM VRAM object
+ *
+ * See ttm_bo_unreserve() for more information.
+ */
+void drm_gem_vram_unreserve(struct drm_gem_vram_object *gbo)
+{
+	ttm_bo_unreserve(&gbo->bo);
+}
+EXPORT_SYMBOL(drm_gem_vram_unreserve);
+
+/**
+ * drm_gem_vram_mmap_offset() - Returns a GEM VRAM object's mmap offset
+ * @gbo:	the GEM VRAM object
+ *
+ * See drm_vma_node_offset_addr() for more information.
+ *
+ * Returns:
+ * The buffer object's offset for userspace mappings on success, or
+ * 0 if no offset is allocated.
+ */
+u64 drm_gem_vram_mmap_offset(struct drm_gem_vram_object *gbo)
+{
+	return drm_vma_node_offset_addr(&gbo->bo.vma_node);
+}
+EXPORT_SYMBOL(drm_gem_vram_mmap_offset);
+
+/**
+ * drm_gem_vram_offset() - \
+	Returns a GEM VRAM object's offset in video memory
+ * @gbo:	the GEM VRAM object
+ *
+ * This function returns the buffer object's offset in the device's video
+ * memory. The buffer object has to be pinned to %TTM_PL_VRAM.
+ *
+ * Returns:
+ * The buffer object's offset in video memory on success, or
+ * a negative error code otherwise.
+ */
+s64 drm_gem_vram_offset(struct drm_gem_vram_object *gbo)
+{
+	if (!gbo->pin_count)
+		return (s64)-ENODEV;
+	return gbo->bo.offset;
+}
+EXPORT_SYMBOL(drm_gem_vram_offset);
+
+/**
+ * drm_gem_vram_pin() - Pins a GEM VRAM object in a region.
+ * @gbo:	the GEM VRAM object
+ * @pl_flag:	a bitmask of possible memory regions
+ *
+ * Pinning a buffer object ensures that it is not evicted from
+ * a memory region. A pinned buffer object has to be unpinned before
+ * it can be pinned to another region.
+ *
+ * Returns:
+ * 0 on success, or
+ * a negative error code otherwise.
+ */
+int drm_gem_vram_pin(struct drm_gem_vram_object *gbo, u32 pl_flag)
+{
+	int i, ret;
+	struct ttm_operation_ctx ctx = { false, false };
+
+	if (gbo->pin_count) {
+		++gbo->pin_count;
+		return 0;
+	}
+
+	drm_gem_vram_placement(gbo, pl_flag);
+	for (i = 0; i < gbo->placement.num_placement; ++i)
+		gbo->placements[i].flags |= TTM_PL_FLAG_NO_EVICT;
+
+	ret = ttm_bo_validate(&gbo->bo, &gbo->placement, &ctx);
+	if (ret < 0)
+		return ret;
+
+	gbo->pin_count = 1;
+
+	return 0;
+}
+EXPORT_SYMBOL(drm_gem_vram_pin);
+
+/**
+ * drm_gem_vram_unpin() - Unpins a GEM VRAM object
+ * @gbo:	the GEM VRAM object
+ *
+ * Returns:
+ * 0 on success, or
+ * a negative error code otherwise.
+ */
+int drm_gem_vram_unpin(struct drm_gem_vram_object *gbo)
+{
+	int i, ret;
+	struct ttm_operation_ctx ctx = { false, false };
+
+	if (!gbo->pin_count)
+		return 0;
+
+	--gbo->pin_count;
+	if (gbo->pin_count)
+		return 0;
+
+	for (i = 0; i < gbo->placement.num_placement ; ++i)
+		gbo->placements[i].flags &= ~TTM_PL_FLAG_NO_EVICT;
+
+	ret = ttm_bo_validate(&gbo->bo, &gbo->placement, &ctx);
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+EXPORT_SYMBOL(drm_gem_vram_unpin);
+
+/**
+ * drm_gem_vram_push_to_system() - \
+	Unpins a GEM VRAM object and moves it to system memory
+ * @gbo:	the GEM VRAM object
+ *
+ * This operation only works if the caller holds the final pin on the
+ * buffer object.
+ *
+ * Returns:
+ * 0 on success, or
+ * a negative error code otherwise.
+ */
+int drm_gem_vram_push_to_system(struct drm_gem_vram_object *gbo)
+{
+	int i, ret;
+	struct ttm_operation_ctx ctx = { false, false };
+
+	if (!gbo->pin_count)
+		return 0;
+
+	--gbo->pin_count;
+	if (gbo->pin_count)
+		return 0;
+
+	if (gbo->kmap.virtual)
+		ttm_bo_kunmap(&gbo->kmap);
+
+	drm_gem_vram_placement(gbo, TTM_PL_FLAG_SYSTEM);
+	for (i = 0; i < gbo->placement.num_placement ; ++i)
+		gbo->placements[i].flags |= TTM_PL_FLAG_NO_EVICT;
+
+	ret = ttm_bo_validate(&gbo->bo, &gbo->placement, &ctx);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+EXPORT_SYMBOL(drm_gem_vram_push_to_system);
+
+/**
+ * drm_gem_vram_kmap_at() - Maps a GEM VRAM object into kernel address space
+ * @gbo:	the GEM VRAM object
+ * @map:	establish a mapping if necessary
+ * @is_iomem:	returns true if the mapped memory is I/O memory, or false \
+	otherwise; can be NULL
+ * @kmap:	the mapping's kmap object
+ *
+ * This function maps the buffer object into the kernel's address space
+ * or returns the current mapping. If the parameter map is false, the
+ * function only queries the current mapping, but does not establish a
+ * new one.
+ *
+ * Returns:
+ * The buffers virtual address if mapped, or
+ * NULL if not mapped, or
+ * an ERR_PTR()-encoded error code otherwise.
+ */
+void* drm_gem_vram_kmap_at(struct drm_gem_vram_object *gbo, bool map,
+			   bool *is_iomem, struct ttm_bo_kmap_obj *kmap)
+{
+	int ret;
+
+	if (kmap->virtual || !map)
+		goto out;
+
+	ret = ttm_bo_kmap(&gbo->bo, 0, gbo->bo.num_pages, kmap);
+	if (ret)
+		return ERR_PTR(ret);
+
+out:
+	if (!is_iomem) {
+		return kmap->virtual;
+	}
+	if (!kmap->virtual) {
+		*is_iomem = false;
+		return NULL;
+	}
+	return ttm_kmap_obj_virtual(kmap, is_iomem);
+}
+EXPORT_SYMBOL(drm_gem_vram_kmap_at);
+
+/**
+ * drm_gem_vram_kmap() - Maps a GEM VRAM object into kernel address space
+ * @gbo:	the GEM VRAM object
+ * @map:	establish a mapping if necessary
+ * @is_iomem:	returns true if the mapped memory is I/O memory, or false \
+	otherwise; can be NULL
+ *
+ * This function maps the buffer object into the kernel's address space
+ * or returns the current mapping. If the parameter map is false, the
+ * function only queries the current mapping, but does not establish a
+ * new one.
+ *
+ * Returns:
+ * The buffers virtual address if mapped, or
+ * NULL if not mapped, or
+ * an ERR_PTR()-encoded error code otherwise.
+ */
+void* drm_gem_vram_kmap(struct drm_gem_vram_object *gbo, bool map,
+			bool *is_iomem)
+{
+	return drm_gem_vram_kmap_at(gbo, map, is_iomem, &gbo->kmap);
+}
+EXPORT_SYMBOL(drm_gem_vram_kmap);
+
+/**
+ * drm_gem_vram_kunmap_at() - Unmaps a GEM VRAM object
+ * @gbo:	the GEM VRAM object
+ * @kmap:	the mapping's kmap object
+ */
+void drm_gem_vram_kunmap_at(struct drm_gem_vram_object *gbo,
+			    struct ttm_bo_kmap_obj *kmap)
+{
+	if (!kmap->virtual)
+		return;
+
+	ttm_bo_kunmap(kmap);
+	kmap->virtual = NULL;
+}
+EXPORT_SYMBOL(drm_gem_vram_kunmap_at);
+
+/**
+ * drm_gem_vram_kunmap() - Unmaps a GEM VRAM object
+ * @gbo:	the GEM VRAM object
+ */
+void drm_gem_vram_kunmap(struct drm_gem_vram_object *gbo)
+{
+	drm_gem_vram_kunmap_at(gbo, &gbo->kmap);
+}
+EXPORT_SYMBOL(drm_gem_vram_kunmap);
diff --git a/drivers/gpu/drm/drm_vram_helper_common.c b/drivers/gpu/drm/drm_vram_helper_common.c
new file mode 100644
index 000000000000..76b6569c9aad
--- /dev/null
+++ b/drivers/gpu/drm/drm_vram_helper_common.c
@@ -0,0 +1,6 @@ 
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+
+#include <linux/module.h>
+
+MODULE_DESCRIPTION("DRM VRAM memory-management helpers");
+MODULE_LICENSE("GPL");
diff --git a/include/drm/drm_gem_vram_helper.h b/include/drm/drm_gem_vram_helper.h
new file mode 100644
index 000000000000..167616f552e5
--- /dev/null
+++ b/include/drm/drm_gem_vram_helper.h
@@ -0,0 +1,92 @@ 
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+
+#ifndef DRM_GEM_VRAM_HELPER_H
+#define DRM_GEM_VRAM_HELPER_H
+
+#include <drm/drm_gem.h>
+#include <drm/ttm/ttm_bo_api.h>
+#include <drm/ttm/ttm_placement.h>
+#include <linux/kernel.h> /* for container_of() */
+
+struct filp;
+
+#define DRM_GEM_VRAM_PL_FLAG_VRAM	TTM_PL_FLAG_VRAM
+#define DRM_GEM_VRAM_PL_FLAG_SYSTEM	TTM_PL_FLAG_SYSTEM
+
+/*
+ * Buffer-object helpers
+ */
+
+/**
+ * struct drm_gem_vram_object - GEM object backed by VRAM
+ * @gem:	GEM object
+ * @bo:		TTM buffer object
+ * @kmap:	Mapping information for @bo
+ * @placement:	TTM placement information. Supported placements are \
+	%TTM_PL_VRAM and %TTM_PL_SYSTEM
+ * @placements:	TTM placement information.
+ * @pin_count:	Pin counter
+ *
+ * The type struct drm_gem_vram_object represents a GEM object that is
+ * backed by VRAM. It can be used for simple frambuffer devices with
+ * dedicated memory. The buffer object can be evicted to system memory if
+ * video memory becomes scarce.
+ */
+struct drm_gem_vram_object {
+        struct drm_gem_object gem;
+        struct ttm_buffer_object bo;
+        struct ttm_bo_kmap_obj kmap;
+
+	/* Supported placements are %TTM_PL_VRAM and %TTM_PL_SYSTEM */
+        struct ttm_placement placement;
+        struct ttm_place placements[3];
+
+        int pin_count;
+};
+
+/**
+ * Returns the container of type &struct drm_gem_vram_object
+ * for field bo.
+ * @bo:		the VRAM buffer object
+ * Returns:	The containing GEM VRAM object
+ */
+static inline struct drm_gem_vram_object* drm_gem_vram_of_bo(
+	struct ttm_buffer_object *bo)
+{
+	return container_of(bo, struct drm_gem_vram_object, bo);
+}
+
+/**
+ * Returns the container of type &struct drm_gem_vram_object
+ * for field gem.
+ * @gem:	the GEM object
+ * Returns:	The containing GEM VRAM object
+ */
+static inline struct drm_gem_vram_object* drm_gem_vram_of_gem(
+	struct drm_gem_object *gem)
+{
+	return container_of(gem, struct drm_gem_vram_object, gem);
+}
+
+struct drm_gem_vram_object* drm_gem_vram_create(struct drm_device *dev,
+						struct ttm_bo_device* bdev,
+						unsigned long size,
+						uint32_t pg_align,
+						bool interruptible);
+void drm_gem_vram_put(struct drm_gem_vram_object *gbo);
+int drm_gem_vram_reserve(struct drm_gem_vram_object *gbo, bool no_wait);
+void drm_gem_vram_unreserve(struct drm_gem_vram_object *gbo);
+u64 drm_gem_vram_mmap_offset(struct drm_gem_vram_object *gbo);
+s64 drm_gem_vram_offset(struct drm_gem_vram_object *gbo);
+int drm_gem_vram_pin(struct drm_gem_vram_object *gbo, u32 pl_flag);
+int drm_gem_vram_unpin(struct drm_gem_vram_object *gbo);
+int drm_gem_vram_push_to_system(struct drm_gem_vram_object *gbo);
+void* drm_gem_vram_kmap_at(struct drm_gem_vram_object *gbo, bool map,
+			   bool *is_iomem, struct ttm_bo_kmap_obj *kmap);
+void* drm_gem_vram_kmap(struct drm_gem_vram_object *gbo, bool map,
+			bool *is_iomem);
+void drm_gem_vram_kunmap_at(struct drm_gem_vram_object *gbo,
+			    struct ttm_bo_kmap_obj *kmap);
+void drm_gem_vram_kunmap(struct drm_gem_vram_object *gbo);
+
+#endif