(was: ast, mgag200: Map console BO while it's being displayed)
Generic fbdev emulation maps and unmaps the console BO for updating it's content from the shadow buffer. If this involves an actual mapping operation (instead of reusing an existing mapping), lots of debug messages may be printed, such as
x86/PAT: Overlap at 0xd0000000-0xd1000000 x86/PAT: reserve_memtype added [mem 0xd0000000-0xd02fffff], track write-combining, req write-combining, ret write-combining x86/PAT: free_memtype request [mem 0xd0000000-0xd02fffff]
as reported at [1]. Drivers using VRAM helpers may also see reduced performance as the mapping operations can create overhead.
In v3 of the patch set, this problem is being solved by lazyly unmapping the buffer as suggested by Gerd. Unmapping with drm_gem_vram_kunmap() only changes a reference counter. VRAM helpers only perform the unmapping operation when TTM evicts the buffer object from its current location. If the buffer is never evicted, the existing mapping is reused by later calls to drm_gem_vram_kmap().
v3: * implement lazy unmapping v2: * fixed comment typos
[1] https://lists.freedesktop.org/archives/dri-devel/2019-September/234308.html
Thomas Zimmermann (3): drm/vram: Add kmap ref-counting to GEM VRAM objects drm/vram: Add infrastructure for move_notify() drm/vram: Implement lazy unmapping for GEM VRAM buffers
drivers/gpu/drm/drm_gem_vram_helper.c | 112 +++++++++++++++++++++----- drivers/gpu/drm/drm_vram_mm_helper.c | 12 +++ include/drm/drm_gem_vram_helper.h | 23 ++++++ include/drm/drm_vram_mm_helper.h | 4 + 4 files changed, 130 insertions(+), 21 deletions(-)
-- 2.23.0
The kmap and kunmap operations of GEM VRAM buffers can now be called in interleaving pairs. The first call to drm_gem_vram_kmap() maps the buffer's memory to kernel address space and the final call to drm_gem_vram_kunmap() unmaps the memory. Intermediate calls to these functions increment or decrement a reference counter.
This change allows for keeping buffer memory mapped for longer and minimizes the amount of changes to TLB, page tables, etc.
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de Cc: Davidlohr Bueso dave@stgolabs.net Reviewed-by: Gerd Hoffmann kraxel@redhat.com --- drivers/gpu/drm/drm_gem_vram_helper.c | 74 ++++++++++++++++++++------- include/drm/drm_gem_vram_helper.h | 19 +++++++ 2 files changed, 75 insertions(+), 18 deletions(-)
diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c b/drivers/gpu/drm/drm_gem_vram_helper.c index fd751078bae1..6c7912092876 100644 --- a/drivers/gpu/drm/drm_gem_vram_helper.c +++ b/drivers/gpu/drm/drm_gem_vram_helper.c @@ -26,7 +26,11 @@ static void drm_gem_vram_cleanup(struct drm_gem_vram_object *gbo) * TTM buffer object in 'bo' has already been cleaned * up; only release the GEM object. */ + + WARN_ON(gbo->kmap_use_count); + drm_gem_object_release(&gbo->bo.base); + mutex_destroy(&gbo->kmap_lock); }
static void drm_gem_vram_destroy(struct drm_gem_vram_object *gbo) @@ -100,6 +104,8 @@ static int drm_gem_vram_init(struct drm_device *dev, if (ret) goto err_drm_gem_object_release;
+ mutex_init(&gbo->kmap_lock); + return 0;
err_drm_gem_object_release: @@ -283,6 +289,34 @@ int drm_gem_vram_unpin(struct drm_gem_vram_object *gbo) } EXPORT_SYMBOL(drm_gem_vram_unpin);
+static void *drm_gem_vram_kmap_locked(struct drm_gem_vram_object *gbo, + bool map, bool *is_iomem) +{ + int ret; + struct ttm_bo_kmap_obj *kmap = &gbo->kmap; + + if (gbo->kmap_use_count > 0) + goto out; + + 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 (!kmap->virtual) { + if (is_iomem) + *is_iomem = false; + return NULL; /* not mapped; don't increment ref */ + } + ++gbo->kmap_use_count; + if (is_iomem) + return ttm_kmap_obj_virtual(kmap, is_iomem); + return kmap->virtual; +} + /** * drm_gem_vram_kmap() - Maps a GEM VRAM object into kernel address space * @gbo: the GEM VRAM object @@ -304,40 +338,44 @@ void *drm_gem_vram_kmap(struct drm_gem_vram_object *gbo, bool map, bool *is_iomem) { int ret; - struct ttm_bo_kmap_obj *kmap = &gbo->kmap; - - if (kmap->virtual || !map) - goto out; + void *virtual;
- ret = ttm_bo_kmap(&gbo->bo, 0, gbo->bo.num_pages, kmap); + ret = mutex_lock_interruptible(&gbo->kmap_lock); if (ret) return ERR_PTR(ret); + virtual = drm_gem_vram_kmap_locked(gbo, map, is_iomem); + mutex_unlock(&gbo->kmap_lock);
-out: - if (!is_iomem) - return kmap->virtual; - if (!kmap->virtual) { - *is_iomem = false; - return NULL; - } - return ttm_kmap_obj_virtual(kmap, is_iomem); + return virtual; } EXPORT_SYMBOL(drm_gem_vram_kmap);
-/** - * 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) +static void drm_gem_vram_kunmap_locked(struct drm_gem_vram_object *gbo) { struct ttm_bo_kmap_obj *kmap = &gbo->kmap;
+ if (WARN_ON_ONCE(!gbo->kmap_use_count)) + return; + if (--gbo->kmap_use_count > 0) + return; + if (!kmap->virtual) return;
ttm_bo_kunmap(kmap); kmap->virtual = NULL; } + +/** + * 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) +{ + mutex_lock(&gbo->kmap_lock); + drm_gem_vram_kunmap_locked(gbo); + mutex_unlock(&gbo->kmap_lock); +} EXPORT_SYMBOL(drm_gem_vram_kunmap);
/** diff --git a/include/drm/drm_gem_vram_helper.h b/include/drm/drm_gem_vram_helper.h index ac217d768456..8c08bc87b788 100644 --- a/include/drm/drm_gem_vram_helper.h +++ b/include/drm/drm_gem_vram_helper.h @@ -34,11 +34,30 @@ struct vm_area_struct; * backed by VRAM. It can be used for simple framebuffer devices with * dedicated memory. The buffer object can be evicted to system memory if * video memory becomes scarce. + * + * GEM VRAM objects perform reference counting for pin and mapping + * operations. So a buffer object that has been pinned N times with + * drm_gem_vram_pin() must be unpinned N times with + * drm_gem_vram_unpin(). The same applies to pairs of + * drm_gem_vram_kmap() and drm_gem_vram_kunmap(). */ struct drm_gem_vram_object { struct ttm_buffer_object bo; struct ttm_bo_kmap_obj kmap;
+ /** + * @kmap_lock: Protects the kmap address and use count + */ + struct mutex kmap_lock; + + /** + * @kmap_use_count: + * + * Reference count on the virtual address. + * The address are un-mapped when the count reaches zero. + */ + unsigned int kmap_use_count; + /* Supported placements are %TTM_PL_VRAM and %TTM_PL_SYSTEM */ struct ttm_placement placement; struct ttm_place placements[2];
On Fri, Sep 06, 2019 at 10:52:12AM +0200, Thomas Zimmermann wrote:
The kmap and kunmap operations of GEM VRAM buffers can now be called in interleaving pairs. The first call to drm_gem_vram_kmap() maps the buffer's memory to kernel address space and the final call to drm_gem_vram_kunmap() unmaps the memory. Intermediate calls to these functions increment or decrement a reference counter.
This change allows for keeping buffer memory mapped for longer and minimizes the amount of changes to TLB, page tables, etc.
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de Cc: Davidlohr Bueso dave@stgolabs.net Reviewed-by: Gerd Hoffmann kraxel@redhat.com
drivers/gpu/drm/drm_gem_vram_helper.c | 74 ++++++++++++++++++++------- include/drm/drm_gem_vram_helper.h | 19 +++++++ 2 files changed, 75 insertions(+), 18 deletions(-)
diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c b/drivers/gpu/drm/drm_gem_vram_helper.c index fd751078bae1..6c7912092876 100644 --- a/drivers/gpu/drm/drm_gem_vram_helper.c +++ b/drivers/gpu/drm/drm_gem_vram_helper.c @@ -26,7 +26,11 @@ static void drm_gem_vram_cleanup(struct drm_gem_vram_object *gbo) * TTM buffer object in 'bo' has already been cleaned * up; only release the GEM object. */
- WARN_ON(gbo->kmap_use_count);
- drm_gem_object_release(&gbo->bo.base);
- mutex_destroy(&gbo->kmap_lock);
}
static void drm_gem_vram_destroy(struct drm_gem_vram_object *gbo) @@ -100,6 +104,8 @@ static int drm_gem_vram_init(struct drm_device *dev, if (ret) goto err_drm_gem_object_release;
- mutex_init(&gbo->kmap_lock);
- return 0;
err_drm_gem_object_release: @@ -283,6 +289,34 @@ int drm_gem_vram_unpin(struct drm_gem_vram_object *gbo) } EXPORT_SYMBOL(drm_gem_vram_unpin);
+static void *drm_gem_vram_kmap_locked(struct drm_gem_vram_object *gbo,
bool map, bool *is_iomem)+{
- int ret;
- struct ttm_bo_kmap_obj *kmap = &gbo->kmap;
- if (gbo->kmap_use_count > 0)
goto out;- 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 (!kmap->virtual) {
if (is_iomem)*is_iomem = false;return NULL; /* not mapped; don't increment ref */- }
- ++gbo->kmap_use_count;
- if (is_iomem)
return ttm_kmap_obj_virtual(kmap, is_iomem);- return kmap->virtual;
+}
/**
- drm_gem_vram_kmap() - Maps a GEM VRAM object into kernel address space
- @gbo: the GEM VRAM object
@@ -304,40 +338,44 @@ void *drm_gem_vram_kmap(struct drm_gem_vram_object *gbo, bool map, bool *is_iomem) { int ret;
- struct ttm_bo_kmap_obj *kmap = &gbo->kmap;
- if (kmap->virtual || !map)
goto out;
- void *virtual;
- ret = ttm_bo_kmap(&gbo->bo, 0, gbo->bo.num_pages, kmap);
- ret = mutex_lock_interruptible(&gbo->kmap_lock); if (ret) return ERR_PTR(ret);
- virtual = drm_gem_vram_kmap_locked(gbo, map, is_iomem);
- mutex_unlock(&gbo->kmap_lock);
-out:
- if (!is_iomem)
return kmap->virtual;- if (!kmap->virtual) {
*is_iomem = false;return NULL;- }
- return ttm_kmap_obj_virtual(kmap, is_iomem);
- return virtual;
} EXPORT_SYMBOL(drm_gem_vram_kmap);
-/**
- 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) +static void drm_gem_vram_kunmap_locked(struct drm_gem_vram_object *gbo) { struct ttm_bo_kmap_obj *kmap = &gbo->kmap;
if (WARN_ON_ONCE(!gbo->kmap_use_count))
return;if (--gbo->kmap_use_count > 0)
return;if (!kmap->virtual) return;
ttm_bo_kunmap(kmap); kmap->virtual = NULL;
}
+/**
- 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) +{
- mutex_lock(&gbo->kmap_lock);
- drm_gem_vram_kunmap_locked(gbo);
- mutex_unlock(&gbo->kmap_lock);
+} EXPORT_SYMBOL(drm_gem_vram_kunmap);
/** diff --git a/include/drm/drm_gem_vram_helper.h b/include/drm/drm_gem_vram_helper.h index ac217d768456..8c08bc87b788 100644 --- a/include/drm/drm_gem_vram_helper.h +++ b/include/drm/drm_gem_vram_helper.h @@ -34,11 +34,30 @@ struct vm_area_struct;
- backed by VRAM. It can be used for simple framebuffer devices with
- dedicated memory. The buffer object can be evicted to system memory if
- video memory becomes scarce.
- GEM VRAM objects perform reference counting for pin and mapping
- operations. So a buffer object that has been pinned N times with
- drm_gem_vram_pin() must be unpinned N times with
- drm_gem_vram_unpin(). The same applies to pairs of
*/
- drm_gem_vram_kmap() and drm_gem_vram_kunmap().
struct drm_gem_vram_object { struct ttm_buffer_object bo; struct ttm_bo_kmap_obj kmap;
- /**
* @kmap_lock: Protects the kmap address and use count*/- struct mutex kmap_lock;
Isn't the locking here a bit racy: The ttm side is protected by the dma_resv ww_mutex, but you have your own lock here. So if you race kmap on one side (from fbdev) with a modeset that evicts stuff (from ttm) things go boom.
I think simpler to just reuse the ttm bo lock (reserve/unreserve). It's atm still a bit special, but Christian König has plans to make reserve/unreserve really nothing more than dma_resv_lock/unlock. -Daniel
- /**
* @kmap_use_count:** Reference count on the virtual address.* The address are un-mapped when the count reaches zero.*/- unsigned int kmap_use_count;
- /* Supported placements are %TTM_PL_VRAM and %TTM_PL_SYSTEM */ struct ttm_placement placement; struct ttm_place placements[2];
-- 2.23.0
This patch prepares VRAM helpers for lazy unmapping of buffer objects.
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de --- drivers/gpu/drm/drm_vram_mm_helper.c | 12 ++++++++++++ include/drm/drm_vram_mm_helper.h | 4 ++++ 2 files changed, 16 insertions(+)
diff --git a/drivers/gpu/drm/drm_vram_mm_helper.c b/drivers/gpu/drm/drm_vram_mm_helper.c index c911781d6728..31984690d5f3 100644 --- a/drivers/gpu/drm/drm_vram_mm_helper.c +++ b/drivers/gpu/drm/drm_vram_mm_helper.c @@ -98,6 +98,17 @@ static int bo_driver_verify_access(struct ttm_buffer_object *bo, return vmm->funcs->verify_access(bo, filp); }
+static void bo_driver_move_notify(struct ttm_buffer_object *bo, + bool evict, + struct ttm_mem_reg *new_mem) +{ + struct drm_vram_mm *vmm = drm_vram_mm_of_bdev(bo->bdev); + + if (!vmm->funcs || !vmm->funcs->move_notify) + return; + vmm->funcs->move_notify(bo, evict, new_mem); +} + static int bo_driver_io_mem_reserve(struct ttm_bo_device *bdev, struct ttm_mem_reg *mem) { @@ -140,6 +151,7 @@ static struct ttm_bo_driver bo_driver = { .eviction_valuable = ttm_bo_eviction_valuable, .evict_flags = bo_driver_evict_flags, .verify_access = bo_driver_verify_access, + .move_notify = bo_driver_move_notify, .io_mem_reserve = bo_driver_io_mem_reserve, .io_mem_free = bo_driver_io_mem_free, }; diff --git a/include/drm/drm_vram_mm_helper.h b/include/drm/drm_vram_mm_helper.h index 2aacfb1ccfae..7fb8700f45fe 100644 --- a/include/drm/drm_vram_mm_helper.h +++ b/include/drm/drm_vram_mm_helper.h @@ -15,6 +15,8 @@ struct drm_device; &ttm_bo_driver.evict_flags * @verify_access: Provides an implementation for \ struct &ttm_bo_driver.verify_access + * @move_notify: Provides an implementation for + * struct &ttm_bo_driver.move_notify * * These callback function integrate VRAM MM with TTM buffer objects. New * functions can be added if necessary. @@ -23,6 +25,8 @@ struct drm_vram_mm_funcs { void (*evict_flags)(struct ttm_buffer_object *bo, struct ttm_placement *placement); int (*verify_access)(struct ttm_buffer_object *bo, struct file *filp); + void (*move_notify)(struct ttm_buffer_object *bo, bool evict, + struct ttm_mem_reg *new_mem); };
/**
On Fri, Sep 06, 2019 at 10:52:13AM +0200, Thomas Zimmermann wrote:
This patch prepares VRAM helpers for lazy unmapping of buffer objects.
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de
drivers/gpu/drm/drm_vram_mm_helper.c | 12 ++++++++++++ include/drm/drm_vram_mm_helper.h | 4 ++++ 2 files changed, 16 insertions(+)
diff --git a/drivers/gpu/drm/drm_vram_mm_helper.c b/drivers/gpu/drm/drm_vram_mm_helper.c index c911781d6728..31984690d5f3 100644 --- a/drivers/gpu/drm/drm_vram_mm_helper.c +++ b/drivers/gpu/drm/drm_vram_mm_helper.c @@ -98,6 +98,17 @@ static int bo_driver_verify_access(struct ttm_buffer_object *bo, return vmm->funcs->verify_access(bo, filp); }
+static void bo_driver_move_notify(struct ttm_buffer_object *bo,
bool evict,struct ttm_mem_reg *new_mem)+{
- struct drm_vram_mm *vmm = drm_vram_mm_of_bdev(bo->bdev);
- if (!vmm->funcs || !vmm->funcs->move_notify)
return;- vmm->funcs->move_notify(bo, evict, new_mem);
+}
static int bo_driver_io_mem_reserve(struct ttm_bo_device *bdev, struct ttm_mem_reg *mem) { @@ -140,6 +151,7 @@ static struct ttm_bo_driver bo_driver = { .eviction_valuable = ttm_bo_eviction_valuable, .evict_flags = bo_driver_evict_flags, .verify_access = bo_driver_verify_access,
- .move_notify = bo_driver_move_notify, .io_mem_reserve = bo_driver_io_mem_reserve, .io_mem_free = bo_driver_io_mem_free,
}; diff --git a/include/drm/drm_vram_mm_helper.h b/include/drm/drm_vram_mm_helper.h index 2aacfb1ccfae..7fb8700f45fe 100644 --- a/include/drm/drm_vram_mm_helper.h +++ b/include/drm/drm_vram_mm_helper.h @@ -15,6 +15,8 @@ struct drm_device; &ttm_bo_driver.evict_flags
- @verify_access: Provides an implementation for \
struct &ttm_bo_driver.verify_access
- @move_notify: Provides an implementation for
struct &ttm_bo_driver.move_notify- These callback function integrate VRAM MM with TTM buffer objects. New
- functions can be added if necessary.
@@ -23,6 +25,8 @@ struct drm_vram_mm_funcs { void (*evict_flags)(struct ttm_buffer_object *bo, struct ttm_placement *placement); int (*verify_access)(struct ttm_buffer_object *bo, struct file *filp);
- void (*move_notify)(struct ttm_buffer_object *bo, bool evict,
struct ttm_mem_reg *new_mem);
Is this indirection really worth it? We have a grand total of 1 driver which isn't using gem (vmwgfx), and I don't think that one will ever switch over to vram helpers.
I'd fold that all in. Helpers don't need to be flexible enough to support every possible use-case (that's just the job of the core), they can be opinionated to get cleaner code for most cases.
For this case here if you fold this in you can unexport 3 EXPORT_SYMBOLs (4 with this patch here), which I think is a neat simplification of the complexity exposed to driver writers. Just put the necessary declarations into a drm_vram_helper_internal.h so that drivers don't get at it by accident (like the other drm*internal.h helpers we have). -Daniel
};
/**
2.23.0
Hi
Am 06.09.19 um 11:28 schrieb Daniel Vetter:
On Fri, Sep 06, 2019 at 10:52:13AM +0200, Thomas Zimmermann wrote:
This patch prepares VRAM helpers for lazy unmapping of buffer objects.
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de
drivers/gpu/drm/drm_vram_mm_helper.c | 12 ++++++++++++ include/drm/drm_vram_mm_helper.h | 4 ++++ 2 files changed, 16 insertions(+)
diff --git a/drivers/gpu/drm/drm_vram_mm_helper.c b/drivers/gpu/drm/drm_vram_mm_helper.c index c911781d6728..31984690d5f3 100644 --- a/drivers/gpu/drm/drm_vram_mm_helper.c +++ b/drivers/gpu/drm/drm_vram_mm_helper.c @@ -98,6 +98,17 @@ static int bo_driver_verify_access(struct ttm_buffer_object *bo, return vmm->funcs->verify_access(bo, filp); }
+static void bo_driver_move_notify(struct ttm_buffer_object *bo,
bool evict,struct ttm_mem_reg *new_mem)+{
- struct drm_vram_mm *vmm = drm_vram_mm_of_bdev(bo->bdev);
- if (!vmm->funcs || !vmm->funcs->move_notify)
return;- vmm->funcs->move_notify(bo, evict, new_mem);
+}
static int bo_driver_io_mem_reserve(struct ttm_bo_device *bdev, struct ttm_mem_reg *mem) { @@ -140,6 +151,7 @@ static struct ttm_bo_driver bo_driver = { .eviction_valuable = ttm_bo_eviction_valuable, .evict_flags = bo_driver_evict_flags, .verify_access = bo_driver_verify_access,
- .move_notify = bo_driver_move_notify, .io_mem_reserve = bo_driver_io_mem_reserve, .io_mem_free = bo_driver_io_mem_free,
}; diff --git a/include/drm/drm_vram_mm_helper.h b/include/drm/drm_vram_mm_helper.h index 2aacfb1ccfae..7fb8700f45fe 100644 --- a/include/drm/drm_vram_mm_helper.h +++ b/include/drm/drm_vram_mm_helper.h @@ -15,6 +15,8 @@ struct drm_device; &ttm_bo_driver.evict_flags
- @verify_access: Provides an implementation for \
struct &ttm_bo_driver.verify_access
- @move_notify: Provides an implementation for
struct &ttm_bo_driver.move_notify- These callback function integrate VRAM MM with TTM buffer objects. New
- functions can be added if necessary.
@@ -23,6 +25,8 @@ struct drm_vram_mm_funcs { void (*evict_flags)(struct ttm_buffer_object *bo, struct ttm_placement *placement); int (*verify_access)(struct ttm_buffer_object *bo, struct file *filp);
- void (*move_notify)(struct ttm_buffer_object *bo, bool evict,
struct ttm_mem_reg *new_mem);Is this indirection really worth it? We have a grand total of 1 driver which isn't using gem (vmwgfx), and I don't think that one will ever switch over to vram helpers.
I'd fold that all in. Helpers don't need to be flexible enough to support every possible use-case (that's just the job of the core), they can be opinionated to get cleaner code for most cases.
The original idea was to make this as flexible as possible; probably more flexible than necessary. I wouldn't mind merging everything into one file, but please not in this patch set, can we keep it for now and I send you a cleanup next?
Best regards Thomas
For this case here if you fold this in you can unexport 3 EXPORT_SYMBOLs (4 with this patch here), which I think is a neat simplification of the complexity exposed to driver writers. Just put the necessary declarations into a drm_vram_helper_internal.h so that drivers don't get at it by accident (like the other drm*internal.h helpers we have). -Daniel
};
/**
2.23.0
On Fri, Sep 6, 2019 at 12:24 PM Thomas Zimmermann tzimmermann@suse.de wrote:
Hi
Am 06.09.19 um 11:28 schrieb Daniel Vetter:
On Fri, Sep 06, 2019 at 10:52:13AM +0200, Thomas Zimmermann wrote:
This patch prepares VRAM helpers for lazy unmapping of buffer objects.
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de
drivers/gpu/drm/drm_vram_mm_helper.c | 12 ++++++++++++ include/drm/drm_vram_mm_helper.h | 4 ++++ 2 files changed, 16 insertions(+)
diff --git a/drivers/gpu/drm/drm_vram_mm_helper.c b/drivers/gpu/drm/drm_vram_mm_helper.c index c911781d6728..31984690d5f3 100644 --- a/drivers/gpu/drm/drm_vram_mm_helper.c +++ b/drivers/gpu/drm/drm_vram_mm_helper.c @@ -98,6 +98,17 @@ static int bo_driver_verify_access(struct ttm_buffer_object *bo, return vmm->funcs->verify_access(bo, filp); }
+static void bo_driver_move_notify(struct ttm_buffer_object *bo,
bool evict,struct ttm_mem_reg *new_mem)+{
- struct drm_vram_mm *vmm = drm_vram_mm_of_bdev(bo->bdev);
- if (!vmm->funcs || !vmm->funcs->move_notify)
return;- vmm->funcs->move_notify(bo, evict, new_mem);
+}
static int bo_driver_io_mem_reserve(struct ttm_bo_device *bdev, struct ttm_mem_reg *mem) { @@ -140,6 +151,7 @@ static struct ttm_bo_driver bo_driver = { .eviction_valuable = ttm_bo_eviction_valuable, .evict_flags = bo_driver_evict_flags, .verify_access = bo_driver_verify_access,
- .move_notify = bo_driver_move_notify, .io_mem_reserve = bo_driver_io_mem_reserve, .io_mem_free = bo_driver_io_mem_free,
}; diff --git a/include/drm/drm_vram_mm_helper.h b/include/drm/drm_vram_mm_helper.h index 2aacfb1ccfae..7fb8700f45fe 100644 --- a/include/drm/drm_vram_mm_helper.h +++ b/include/drm/drm_vram_mm_helper.h @@ -15,6 +15,8 @@ struct drm_device; &ttm_bo_driver.evict_flags
- @verify_access: Provides an implementation for \ struct &ttm_bo_driver.verify_access
- @move_notify: Provides an implementation for
struct &ttm_bo_driver.move_notify- These callback function integrate VRAM MM with TTM buffer objects. New
- functions can be added if necessary.
@@ -23,6 +25,8 @@ struct drm_vram_mm_funcs { void (*evict_flags)(struct ttm_buffer_object *bo, struct ttm_placement *placement); int (*verify_access)(struct ttm_buffer_object *bo, struct file *filp);
- void (*move_notify)(struct ttm_buffer_object *bo, bool evict,
struct ttm_mem_reg *new_mem);Is this indirection really worth it? We have a grand total of 1 driver which isn't using gem (vmwgfx), and I don't think that one will ever switch over to vram helpers.
I'd fold that all in. Helpers don't need to be flexible enough to support every possible use-case (that's just the job of the core), they can be opinionated to get cleaner code for most cases.
The original idea was to make this as flexible as possible; probably more flexible than necessary. I wouldn't mind merging everything into one file, but please not in this patch set, can we keep it for now and I send you a cleanup next?
Oh sure, I just wondered about why this flexibility exists and realized there's not really a user for it. And pondering this more I didn't come up with a use-case where it might reasonably be needed, and you don't want to roll your own complete, and couldn't come up with anything. Aside from the locking question on patch 1 I think this looks like a really tidy solution for the fbdev mapping issue, happy to ack once we've figured out the locking thing. -Daniel
Best regards Thomas
For this case here if you fold this in you can unexport 3 EXPORT_SYMBOLs (4 with this patch here), which I think is a neat simplification of the complexity exposed to driver writers. Just put the necessary declarations into a drm_vram_helper_internal.h so that drivers don't get at it by accident (like the other drm*internal.h helpers we have). -Daniel
};
/**
2.23.0
-- 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)
Hi
Am 06.09.19 um 15:05 schrieb Daniel Vetter:
On Fri, Sep 6, 2019 at 12:24 PM Thomas Zimmermann tzimmermann@suse.de wrote:
Hi
Am 06.09.19 um 11:28 schrieb Daniel Vetter:
On Fri, Sep 06, 2019 at 10:52:13AM +0200, Thomas Zimmermann wrote:
This patch prepares VRAM helpers for lazy unmapping of buffer objects.
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de
drivers/gpu/drm/drm_vram_mm_helper.c | 12 ++++++++++++ include/drm/drm_vram_mm_helper.h | 4 ++++ 2 files changed, 16 insertions(+)
diff --git a/drivers/gpu/drm/drm_vram_mm_helper.c b/drivers/gpu/drm/drm_vram_mm_helper.c index c911781d6728..31984690d5f3 100644 --- a/drivers/gpu/drm/drm_vram_mm_helper.c +++ b/drivers/gpu/drm/drm_vram_mm_helper.c @@ -98,6 +98,17 @@ static int bo_driver_verify_access(struct ttm_buffer_object *bo, return vmm->funcs->verify_access(bo, filp); }
+static void bo_driver_move_notify(struct ttm_buffer_object *bo,
bool evict,struct ttm_mem_reg *new_mem)+{
- struct drm_vram_mm *vmm = drm_vram_mm_of_bdev(bo->bdev);
- if (!vmm->funcs || !vmm->funcs->move_notify)
return;- vmm->funcs->move_notify(bo, evict, new_mem);
+}
static int bo_driver_io_mem_reserve(struct ttm_bo_device *bdev, struct ttm_mem_reg *mem) { @@ -140,6 +151,7 @@ static struct ttm_bo_driver bo_driver = { .eviction_valuable = ttm_bo_eviction_valuable, .evict_flags = bo_driver_evict_flags, .verify_access = bo_driver_verify_access,
- .move_notify = bo_driver_move_notify, .io_mem_reserve = bo_driver_io_mem_reserve, .io_mem_free = bo_driver_io_mem_free,
}; diff --git a/include/drm/drm_vram_mm_helper.h b/include/drm/drm_vram_mm_helper.h index 2aacfb1ccfae..7fb8700f45fe 100644 --- a/include/drm/drm_vram_mm_helper.h +++ b/include/drm/drm_vram_mm_helper.h @@ -15,6 +15,8 @@ struct drm_device; &ttm_bo_driver.evict_flags
- @verify_access: Provides an implementation for \ struct &ttm_bo_driver.verify_access
- @move_notify: Provides an implementation for
struct &ttm_bo_driver.move_notify- These callback function integrate VRAM MM with TTM buffer objects. New
- functions can be added if necessary.
@@ -23,6 +25,8 @@ struct drm_vram_mm_funcs { void (*evict_flags)(struct ttm_buffer_object *bo, struct ttm_placement *placement); int (*verify_access)(struct ttm_buffer_object *bo, struct file *filp);
- void (*move_notify)(struct ttm_buffer_object *bo, bool evict,
struct ttm_mem_reg *new_mem);Is this indirection really worth it? We have a grand total of 1 driver which isn't using gem (vmwgfx), and I don't think that one will ever switch over to vram helpers.
I'd fold that all in. Helpers don't need to be flexible enough to support every possible use-case (that's just the job of the core), they can be opinionated to get cleaner code for most cases.
The original idea was to make this as flexible as possible; probably more flexible than necessary. I wouldn't mind merging everything into one file, but please not in this patch set, can we keep it for now and I send you a cleanup next?
Oh sure, I just wondered about why this flexibility exists and realized there's not really a user for it. And pondering this more I didn't come up with a use-case where it might reasonably be needed, and you don't want to roll your own complete, and couldn't come up with anything. Aside from the locking question on patch 1 I think this looks like a really tidy solution for the fbdev mapping issue, happy to ack once we've figured out the locking thing.
Great. There's a v4 of the patch set that should resolve the locking problem.
Best regards Thomas
-Daniel
Best regards Thomas
For this case here if you fold this in you can unexport 3 EXPORT_SYMBOLs (4 with this patch here), which I think is a neat simplification of the complexity exposed to driver writers. Just put the necessary declarations into a drm_vram_helper_internal.h so that drivers don't get at it by accident (like the other drm*internal.h helpers we have). -Daniel
};
/**
2.23.0
-- 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)
Frequent mapping and unmapping a buffer object adds overhead for modifying the page table and creates debug output. Unmapping a buffer is only required when the memory manager evicts the buffer from its current location.
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de --- drivers/gpu/drm/drm_gem_vram_helper.c | 48 ++++++++++++++++++++++----- include/drm/drm_gem_vram_helper.h | 4 +++ 2 files changed, 44 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c b/drivers/gpu/drm/drm_gem_vram_helper.c index 6c7912092876..973c703534d4 100644 --- a/drivers/gpu/drm/drm_gem_vram_helper.c +++ b/drivers/gpu/drm/drm_gem_vram_helper.c @@ -352,18 +352,17 @@ EXPORT_SYMBOL(drm_gem_vram_kmap);
static void drm_gem_vram_kunmap_locked(struct drm_gem_vram_object *gbo) { - struct ttm_bo_kmap_obj *kmap = &gbo->kmap; - if (WARN_ON_ONCE(!gbo->kmap_use_count)) return; if (--gbo->kmap_use_count > 0) return;
- if (!kmap->virtual) - return; - - ttm_bo_kunmap(kmap); - kmap->virtual = NULL; + /* + * Permanently mapping and unmapping buffers adds overhead from + * updating the page tables and creates debugging output. Therefore, + * we delay the actual unmap operation until the BO gets evicted + * from memory. See drm_gem_vram_bo_driver_move_notify(). + */ }
/** @@ -489,6 +488,38 @@ int drm_gem_vram_bo_driver_verify_access(struct ttm_buffer_object *bo, } EXPORT_SYMBOL(drm_gem_vram_bo_driver_verify_access);
+/** + * drm_gem_vram_bo_driver_move_notify() - + * Implements &struct ttm_bo_driver.move_notify + * @bo: TTM buffer object. Refers to &struct drm_gem_vram_object.bo + * @evict: True, if the BO is being evicted from graphics memory; + * false otherwise. + * @new_mem: New memory region, or NULL on destruction + */ +void drm_gem_vram_bo_driver_move_notify(struct ttm_buffer_object *bo, + bool evict, + struct ttm_mem_reg *new_mem) +{ + struct drm_gem_vram_object *gbo; + struct ttm_bo_kmap_obj *kmap; + + /* TTM may pass BOs that are not GEM VRAM BOs. */ + if (!drm_is_gem_vram(bo)) + return; + + gbo = drm_gem_vram_of_bo(bo); + kmap = &gbo->kmap; + + if (WARN_ON_ONCE(gbo->kmap_use_count)) + return; + + if (!kmap->virtual) + return; + ttm_bo_kunmap(kmap); + kmap->virtual = NULL; +} +EXPORT_SYMBOL(drm_gem_vram_bo_driver_move_notify); + /* * drm_gem_vram_mm_funcs - Functions for &struct drm_vram_mm * @@ -498,7 +529,8 @@ EXPORT_SYMBOL(drm_gem_vram_bo_driver_verify_access); */ const struct drm_vram_mm_funcs drm_gem_vram_mm_funcs = { .evict_flags = drm_gem_vram_bo_driver_evict_flags, - .verify_access = drm_gem_vram_bo_driver_verify_access + .verify_access = drm_gem_vram_bo_driver_verify_access, + .move_notify = drm_gem_vram_bo_driver_move_notify, }; EXPORT_SYMBOL(drm_gem_vram_mm_funcs);
diff --git a/include/drm/drm_gem_vram_helper.h b/include/drm/drm_gem_vram_helper.h index 8c08bc87b788..e5ef0e4ab2e4 100644 --- a/include/drm/drm_gem_vram_helper.h +++ b/include/drm/drm_gem_vram_helper.h @@ -117,6 +117,10 @@ int drm_gem_vram_fill_create_dumb(struct drm_file *file, void drm_gem_vram_bo_driver_evict_flags(struct ttm_buffer_object *bo, struct ttm_placement *pl);
+void drm_gem_vram_bo_driver_move_notify(struct ttm_buffer_object *bo, + bool evict, + struct ttm_mem_reg *new_mem); + int drm_gem_vram_bo_driver_verify_access(struct ttm_buffer_object *bo, struct file *filp);
+void drm_gem_vram_bo_driver_move_notify(struct ttm_buffer_object *bo,
bool evict,struct ttm_mem_reg *new_mem)+{
[ ... ]
- if (!kmap->virtual)
return;- ttm_bo_kunmap(kmap);
- kmap->virtual = NULL;
+}
I think ttm_buffer_object_destroy() needs "if (kmap->virtual) ttm_bo_kunmap()" too, due to the lazy unmap you can land there with an active mapping.
cheers, Gerd
Hi
Am 06.09.19 um 11:39 schrieb Gerd Hoffmann:
+void drm_gem_vram_bo_driver_move_notify(struct ttm_buffer_object *bo,
bool evict,struct ttm_mem_reg *new_mem)+{
[ ... ]
- if (!kmap->virtual)
return;- ttm_bo_kunmap(kmap);
- kmap->virtual = NULL;
+}
I think ttm_buffer_object_destroy() needs "if (kmap->virtual) ttm_bo_kunmap()" too, due to the lazy unmap you can land there with an active mapping.
Hmm, from my understanding, that final call to move_notify() is done by ttm_bo_cleanup_memtype_use(), which is called from within ttm_bo_put().
If we want to add a final kunmap, There's also the release_notify() callback, which is probably the right place to put it.
Best regards Thomas
cheers, Gerd
On Fri, Sep 06, 2019 at 12:37:47PM +0200, Thomas Zimmermann wrote:
Hi
Am 06.09.19 um 11:39 schrieb Gerd Hoffmann:
+void drm_gem_vram_bo_driver_move_notify(struct ttm_buffer_object *bo,
bool evict,struct ttm_mem_reg *new_mem)+{
[ ... ]
- if (!kmap->virtual)
return;- ttm_bo_kunmap(kmap);
- kmap->virtual = NULL;
+}
I think ttm_buffer_object_destroy() needs "if (kmap->virtual) ttm_bo_kunmap()" too, due to the lazy unmap you can land there with an active mapping.
Hmm, from my understanding, that final call to move_notify() is done by ttm_bo_cleanup_memtype_use(), which is called from within ttm_bo_put().
Ah, good, sounds like this should work indeed. Maybe add WARN_ON(kmap->virtual), just to be sure we don't overlooked something.
cheers, Gerd
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Gerd Hoffmann -
Thomas Zimmermann