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)