Hi all,
This patch series embeds drm_gem_object into struct radeon_bo and adjusts any references. I've decided against embedding the gem stuff into struct ttm_bo because - vmwgfx doesn't use gem and - ttm is used for implementing the memory management, whereas gem provides the userspace interface (I know, there's some overlap there, but that's not really a new problem).
Please review and consider merging for -next.
Yours, Daniel
Daniel Vetter (3): drm/radeon: embed struct drm_gem_object drm/radeon: introduce gem_to_radeon_bo helper drm/radeon: kill radeon_bo->gobj pointer
drivers/gpu/drm/radeon/atombios_crtc.c | 8 ++-- drivers/gpu/drm/radeon/evergreen_blit_kms.c | 2 +- drivers/gpu/drm/radeon/r600.c | 2 +- drivers/gpu/drm/radeon/r600_blit_kms.c | 2 +- drivers/gpu/drm/radeon/radeon.h | 3 +- drivers/gpu/drm/radeon/radeon_benchmark.c | 4 +- drivers/gpu/drm/radeon/radeon_cs.c | 2 +- drivers/gpu/drm/radeon/radeon_device.c | 4 +- drivers/gpu/drm/radeon/radeon_fb.c | 10 +++--- drivers/gpu/drm/radeon/radeon_gart.c | 2 +- drivers/gpu/drm/radeon/radeon_gem.c | 43 ++++++++++++--------------- drivers/gpu/drm/radeon/radeon_legacy_crtc.c | 4 +- drivers/gpu/drm/radeon/radeon_object.c | 24 +++++++-------- drivers/gpu/drm/radeon/radeon_object.h | 2 +- drivers/gpu/drm/radeon/radeon_ring.c | 4 +- drivers/gpu/drm/radeon/radeon_test.c | 4 +- drivers/gpu/drm/radeon/radeon_ttm.c | 2 +- drivers/gpu/drm/radeon/rv770.c | 2 +- 18 files changed, 59 insertions(+), 65 deletions(-)
Unconditionally initialize the drm gem object - it's not worth the trouble not to for the few kernel objects.
This patch only changes the place of the drm gem object, access is still done via pointers.
Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/radeon/evergreen_blit_kms.c | 2 +- drivers/gpu/drm/radeon/r600.c | 2 +- drivers/gpu/drm/radeon/r600_blit_kms.c | 2 +- drivers/gpu/drm/radeon/radeon.h | 1 + drivers/gpu/drm/radeon/radeon_benchmark.c | 4 ++-- drivers/gpu/drm/radeon/radeon_device.c | 2 +- drivers/gpu/drm/radeon/radeon_gart.c | 2 +- drivers/gpu/drm/radeon/radeon_gem.c | 22 +++++++++------------- drivers/gpu/drm/radeon/radeon_object.c | 16 +++++++++------- drivers/gpu/drm/radeon/radeon_object.h | 2 +- drivers/gpu/drm/radeon/radeon_ring.c | 4 ++-- drivers/gpu/drm/radeon/radeon_test.c | 4 ++-- drivers/gpu/drm/radeon/radeon_ttm.c | 2 +- drivers/gpu/drm/radeon/rv770.c | 2 +- 14 files changed, 33 insertions(+), 34 deletions(-)
diff --git a/drivers/gpu/drm/radeon/evergreen_blit_kms.c b/drivers/gpu/drm/radeon/evergreen_blit_kms.c index ac3b6dd..f8b4b1b 100644 --- a/drivers/gpu/drm/radeon/evergreen_blit_kms.c +++ b/drivers/gpu/drm/radeon/evergreen_blit_kms.c @@ -459,7 +459,7 @@ int evergreen_blit_init(struct radeon_device *rdev) obj_size += evergreen_ps_size * 4; obj_size = ALIGN(obj_size, 256);
- r = radeon_bo_create(rdev, NULL, obj_size, true, RADEON_GEM_DOMAIN_VRAM, + r = radeon_bo_create(rdev, obj_size, true, RADEON_GEM_DOMAIN_VRAM, &rdev->r600_blit.shader_obj); if (r) { DRM_ERROR("evergreen failed to allocate shader\n"); diff --git a/drivers/gpu/drm/radeon/r600.c b/drivers/gpu/drm/radeon/r600.c index 33952a1..82376b2 100644 --- a/drivers/gpu/drm/radeon/r600.c +++ b/drivers/gpu/drm/radeon/r600.c @@ -2723,7 +2723,7 @@ static int r600_ih_ring_alloc(struct radeon_device *rdev)
/* Allocate ring buffer */ if (rdev->ih.ring_obj == NULL) { - r = radeon_bo_create(rdev, NULL, rdev->ih.ring_size, + r = radeon_bo_create(rdev, rdev->ih.ring_size, true, RADEON_GEM_DOMAIN_GTT, &rdev->ih.ring_obj); diff --git a/drivers/gpu/drm/radeon/r600_blit_kms.c b/drivers/gpu/drm/radeon/r600_blit_kms.c index 8362974..c9cb553 100644 --- a/drivers/gpu/drm/radeon/r600_blit_kms.c +++ b/drivers/gpu/drm/radeon/r600_blit_kms.c @@ -501,7 +501,7 @@ int r600_blit_init(struct radeon_device *rdev) obj_size += r6xx_ps_size * 4; obj_size = ALIGN(obj_size, 256);
- r = radeon_bo_create(rdev, NULL, obj_size, true, RADEON_GEM_DOMAIN_VRAM, + r = radeon_bo_create(rdev, obj_size, true, RADEON_GEM_DOMAIN_VRAM, &rdev->r600_blit.shader_obj); if (r) { DRM_ERROR("r600 failed to allocate shader\n"); diff --git a/drivers/gpu/drm/radeon/radeon.h b/drivers/gpu/drm/radeon/radeon.h index 73f600d..e2409b9 100644 --- a/drivers/gpu/drm/radeon/radeon.h +++ b/drivers/gpu/drm/radeon/radeon.h @@ -256,6 +256,7 @@ struct radeon_bo { /* Constant after initialization */ struct radeon_device *rdev; struct drm_gem_object *gobj; + struct drm_gem_object gem_base; };
struct radeon_bo_list { diff --git a/drivers/gpu/drm/radeon/radeon_benchmark.c b/drivers/gpu/drm/radeon/radeon_benchmark.c index 7932dc4..1bdf3cc 100644 --- a/drivers/gpu/drm/radeon/radeon_benchmark.c +++ b/drivers/gpu/drm/radeon/radeon_benchmark.c @@ -41,7 +41,7 @@ void radeon_benchmark_move(struct radeon_device *rdev, unsigned bsize,
size = bsize; n = 1024; - r = radeon_bo_create(rdev, NULL, size, true, sdomain, &sobj); + r = radeon_bo_create(rdev, size, true, sdomain, &sobj); if (r) { goto out_cleanup; } @@ -53,7 +53,7 @@ void radeon_benchmark_move(struct radeon_device *rdev, unsigned bsize, if (r) { goto out_cleanup; } - r = radeon_bo_create(rdev, NULL, size, true, ddomain, &dobj); + r = radeon_bo_create(rdev, size, true, ddomain, &dobj); if (r) { goto out_cleanup; } diff --git a/drivers/gpu/drm/radeon/radeon_device.c b/drivers/gpu/drm/radeon/radeon_device.c index 8adfedf..6ac28ee 100644 --- a/drivers/gpu/drm/radeon/radeon_device.c +++ b/drivers/gpu/drm/radeon/radeon_device.c @@ -180,7 +180,7 @@ int radeon_wb_init(struct radeon_device *rdev) int r;
if (rdev->wb.wb_obj == NULL) { - r = radeon_bo_create(rdev, NULL, RADEON_GPU_PAGE_SIZE, true, + r = radeon_bo_create(rdev, RADEON_GPU_PAGE_SIZE, true, RADEON_GEM_DOMAIN_GTT, &rdev->wb.wb_obj); if (r) { dev_warn(rdev->dev, "(%d) create WB bo failed\n", r); diff --git a/drivers/gpu/drm/radeon/radeon_gart.c b/drivers/gpu/drm/radeon/radeon_gart.c index e65b903..344fe82 100644 --- a/drivers/gpu/drm/radeon/radeon_gart.c +++ b/drivers/gpu/drm/radeon/radeon_gart.c @@ -78,7 +78,7 @@ int radeon_gart_table_vram_alloc(struct radeon_device *rdev) int r;
if (rdev->gart.table.vram.robj == NULL) { - r = radeon_bo_create(rdev, NULL, rdev->gart.table_size, + r = radeon_bo_create(rdev, rdev->gart.table_size, true, RADEON_GEM_DOMAIN_VRAM, &rdev->gart.table.vram.robj); if (r) { diff --git a/drivers/gpu/drm/radeon/radeon_gem.c b/drivers/gpu/drm/radeon/radeon_gem.c index d1e595d..9873a96 100644 --- a/drivers/gpu/drm/radeon/radeon_gem.c +++ b/drivers/gpu/drm/radeon/radeon_gem.c @@ -32,7 +32,8 @@
int radeon_gem_object_init(struct drm_gem_object *obj) { - /* we do nothings here */ + BUG(); + return 0; }
@@ -44,9 +45,6 @@ void radeon_gem_object_free(struct drm_gem_object *gobj) if (robj) { radeon_bo_unref(&robj); } - - drm_gem_object_release(gobj); - kfree(gobj); }
int radeon_gem_object_create(struct radeon_device *rdev, int size, @@ -54,29 +52,27 @@ int radeon_gem_object_create(struct radeon_device *rdev, int size, bool discardable, bool kernel, struct drm_gem_object **obj) { - struct drm_gem_object *gobj; struct radeon_bo *robj; int r;
*obj = NULL; - gobj = drm_gem_object_alloc(rdev->ddev, size); - if (!gobj) { - return -ENOMEM; - } /* At least align on page size */ if (alignment < PAGE_SIZE) { alignment = PAGE_SIZE; } - r = radeon_bo_create(rdev, gobj, size, kernel, initial_domain, &robj); + r = radeon_bo_create(rdev, size, kernel, initial_domain, &robj); if (r) { if (r != -ERESTARTSYS) DRM_ERROR("Failed to allocate GEM object (%d, %d, %u, %d)\n", size, initial_domain, alignment, r); - drm_gem_object_unreference_unlocked(gobj); return r; } - gobj->driver_private = robj; - *obj = gobj; + *obj = &robj->gem_base; + + mutex_lock(&rdev->gem.mutex); + list_add_tail(&robj->list, &rdev->gem.objects); + mutex_unlock(&rdev->gem.mutex); + return 0; }
diff --git a/drivers/gpu/drm/radeon/radeon_object.c b/drivers/gpu/drm/radeon/radeon_object.c index d7ab914..30a2d54 100644 --- a/drivers/gpu/drm/radeon/radeon_object.c +++ b/drivers/gpu/drm/radeon/radeon_object.c @@ -54,6 +54,7 @@ static void radeon_ttm_bo_destroy(struct ttm_buffer_object *tbo) list_del_init(&bo->list); mutex_unlock(&bo->rdev->gem.mutex); radeon_bo_clear_surface_reg(bo); + drm_gem_object_release(&bo->gem_base); kfree(bo); }
@@ -85,7 +86,7 @@ void radeon_ttm_placement_from_domain(struct radeon_bo *rbo, u32 domain) rbo->placement.num_busy_placement = c; }
-int radeon_bo_create(struct radeon_device *rdev, struct drm_gem_object *gobj, +int radeon_bo_create(struct radeon_device *rdev, unsigned long size, bool kernel, u32 domain, struct radeon_bo **bo_ptr) { @@ -105,8 +106,14 @@ int radeon_bo_create(struct radeon_device *rdev, struct drm_gem_object *gobj, bo = kzalloc(sizeof(struct radeon_bo), GFP_KERNEL); if (bo == NULL) return -ENOMEM; + r = drm_gem_object_init(rdev->ddev, &bo->gem_base, size); + if (unlikely(r)) { + kfree(bo); + return r; + } bo->rdev = rdev; - bo->gobj = gobj; + bo->gobj = &bo->gem_base; + bo->gem_base.driver_private = bo; bo->surface_reg = -1; INIT_LIST_HEAD(&bo->list);
@@ -131,11 +138,6 @@ retry: return r; } *bo_ptr = bo; - if (gobj) { - mutex_lock(&bo->rdev->gem.mutex); - list_add_tail(&bo->list, &rdev->gem.objects); - mutex_unlock(&bo->rdev->gem.mutex); - } return 0; }
diff --git a/drivers/gpu/drm/radeon/radeon_object.h b/drivers/gpu/drm/radeon/radeon_object.h index 3481bc7..c3dd268 100644 --- a/drivers/gpu/drm/radeon/radeon_object.h +++ b/drivers/gpu/drm/radeon/radeon_object.h @@ -137,7 +137,7 @@ static inline int radeon_bo_wait(struct radeon_bo *bo, u32 *mem_type, }
extern int radeon_bo_create(struct radeon_device *rdev, - struct drm_gem_object *gobj, unsigned long size, + unsigned long size, bool kernel, u32 domain, struct radeon_bo **bo_ptr); extern int radeon_bo_kmap(struct radeon_bo *bo, void **ptr); diff --git a/drivers/gpu/drm/radeon/radeon_ring.c b/drivers/gpu/drm/radeon/radeon_ring.c index 6ea798c..750e965 100644 --- a/drivers/gpu/drm/radeon/radeon_ring.c +++ b/drivers/gpu/drm/radeon/radeon_ring.c @@ -175,7 +175,7 @@ int radeon_ib_pool_init(struct radeon_device *rdev) return 0; INIT_LIST_HEAD(&rdev->ib_pool.bogus_ib); /* Allocate 1M object buffer */ - r = radeon_bo_create(rdev, NULL, RADEON_IB_POOL_SIZE*64*1024, + r = radeon_bo_create(rdev, RADEON_IB_POOL_SIZE*64*1024, true, RADEON_GEM_DOMAIN_GTT, &rdev->ib_pool.robj); if (r) { @@ -332,7 +332,7 @@ int radeon_ring_init(struct radeon_device *rdev, unsigned ring_size) rdev->cp.ring_size = ring_size; /* Allocate ring buffer */ if (rdev->cp.ring_obj == NULL) { - r = radeon_bo_create(rdev, NULL, rdev->cp.ring_size, true, + r = radeon_bo_create(rdev, rdev->cp.ring_size, true, RADEON_GEM_DOMAIN_GTT, &rdev->cp.ring_obj); if (r) { diff --git a/drivers/gpu/drm/radeon/radeon_test.c b/drivers/gpu/drm/radeon/radeon_test.c index 313c96b..8d7bdd6 100644 --- a/drivers/gpu/drm/radeon/radeon_test.c +++ b/drivers/gpu/drm/radeon/radeon_test.c @@ -52,7 +52,7 @@ void radeon_test_moves(struct radeon_device *rdev) goto out_cleanup; }
- r = radeon_bo_create(rdev, NULL, size, true, RADEON_GEM_DOMAIN_VRAM, + r = radeon_bo_create(rdev, size, true, RADEON_GEM_DOMAIN_VRAM, &vram_obj); if (r) { DRM_ERROR("Failed to create VRAM object\n"); @@ -71,7 +71,7 @@ void radeon_test_moves(struct radeon_device *rdev) void **gtt_start, **gtt_end; void **vram_start, **vram_end;
- r = radeon_bo_create(rdev, NULL, size, true, + r = radeon_bo_create(rdev, size, true, RADEON_GEM_DOMAIN_GTT, gtt_obj + i); if (r) { DRM_ERROR("Failed to create GTT object %d\n", i); diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c b/drivers/gpu/drm/radeon/radeon_ttm.c index fe95bb3..0c19928 100644 --- a/drivers/gpu/drm/radeon/radeon_ttm.c +++ b/drivers/gpu/drm/radeon/radeon_ttm.c @@ -529,7 +529,7 @@ int radeon_ttm_init(struct radeon_device *rdev) DRM_ERROR("Failed initializing VRAM heap.\n"); return r; } - r = radeon_bo_create(rdev, NULL, 256 * 1024, true, + r = radeon_bo_create(rdev, 256 * 1024, true, RADEON_GEM_DOMAIN_VRAM, &rdev->stollen_vga_memory); if (r) { diff --git a/drivers/gpu/drm/radeon/rv770.c b/drivers/gpu/drm/radeon/rv770.c index 245374e..0eb74f6 100644 --- a/drivers/gpu/drm/radeon/rv770.c +++ b/drivers/gpu/drm/radeon/rv770.c @@ -914,7 +914,7 @@ static int rv770_vram_scratch_init(struct radeon_device *rdev) u64 gpu_addr;
if (rdev->vram_scratch.robj == NULL) { - r = radeon_bo_create(rdev, NULL, RADEON_GPU_PAGE_SIZE, + r = radeon_bo_create(rdev, RADEON_GPU_PAGE_SIZE, true, RADEON_GEM_DOMAIN_VRAM, &rdev->vram_scratch.robj); if (r) {
... and switch it to container_of upcasting.
Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/radeon/atombios_crtc.c | 8 ++++---- drivers/gpu/drm/radeon/radeon.h | 1 + drivers/gpu/drm/radeon/radeon_cs.c | 2 +- drivers/gpu/drm/radeon/radeon_device.c | 2 +- drivers/gpu/drm/radeon/radeon_fb.c | 10 +++++----- drivers/gpu/drm/radeon/radeon_gem.c | 21 ++++++++++----------- drivers/gpu/drm/radeon/radeon_legacy_crtc.c | 4 ++-- drivers/gpu/drm/radeon/radeon_object.c | 3 +-- 8 files changed, 25 insertions(+), 26 deletions(-)
diff --git a/drivers/gpu/drm/radeon/atombios_crtc.c b/drivers/gpu/drm/radeon/atombios_crtc.c index df2b6f2..7ecb154 100644 --- a/drivers/gpu/drm/radeon/atombios_crtc.c +++ b/drivers/gpu/drm/radeon/atombios_crtc.c @@ -978,7 +978,7 @@ static int evergreen_crtc_do_set_base(struct drm_crtc *crtc, * just update base pointers */ obj = radeon_fb->obj; - rbo = obj->driver_private; + rbo = gem_to_radeon_bo(obj); r = radeon_bo_reserve(rbo, false); if (unlikely(r != 0)) return r; @@ -1086,7 +1086,7 @@ static int evergreen_crtc_do_set_base(struct drm_crtc *crtc,
if (!atomic && fb && fb != crtc->fb) { radeon_fb = to_radeon_framebuffer(fb); - rbo = radeon_fb->obj->driver_private; + rbo = gem_to_radeon_bo(radeon_fb->obj); r = radeon_bo_reserve(rbo, false); if (unlikely(r != 0)) return r; @@ -1131,7 +1131,7 @@ static int avivo_crtc_do_set_base(struct drm_crtc *crtc, }
obj = radeon_fb->obj; - rbo = obj->driver_private; + rbo = gem_to_radeon_bo(obj); r = radeon_bo_reserve(rbo, false); if (unlikely(r != 0)) return r; @@ -1240,7 +1240,7 @@ static int avivo_crtc_do_set_base(struct drm_crtc *crtc,
if (!atomic && fb && fb != crtc->fb) { radeon_fb = to_radeon_framebuffer(fb); - rbo = radeon_fb->obj->driver_private; + rbo = gem_to_radeon_bo(radeon_fb->obj); r = radeon_bo_reserve(rbo, false); if (unlikely(r != 0)) return r; diff --git a/drivers/gpu/drm/radeon/radeon.h b/drivers/gpu/drm/radeon/radeon.h index e2409b9..239eba7 100644 --- a/drivers/gpu/drm/radeon/radeon.h +++ b/drivers/gpu/drm/radeon/radeon.h @@ -258,6 +258,7 @@ struct radeon_bo { struct drm_gem_object *gobj; struct drm_gem_object gem_base; }; +#define gem_to_radeon_bo(gobj) container_of((gobj), struct radeon_bo, gem_base)
struct radeon_bo_list { struct list_head list; diff --git a/drivers/gpu/drm/radeon/radeon_cs.c b/drivers/gpu/drm/radeon/radeon_cs.c index 6d64a27..64096f3 100644 --- a/drivers/gpu/drm/radeon/radeon_cs.c +++ b/drivers/gpu/drm/radeon/radeon_cs.c @@ -75,7 +75,7 @@ int radeon_cs_parser_relocs(struct radeon_cs_parser *p) return -ENOENT; } p->relocs_ptr[i] = &p->relocs[i]; - p->relocs[i].robj = p->relocs[i].gobj->driver_private; + p->relocs[i].robj = gem_to_radeon_bo(p->relocs[i].gobj); p->relocs[i].lobj.bo = p->relocs[i].robj; p->relocs[i].lobj.rdomain = r->read_domains; p->relocs[i].lobj.wdomain = r->write_domain; diff --git a/drivers/gpu/drm/radeon/radeon_device.c b/drivers/gpu/drm/radeon/radeon_device.c index 6ac28ee..3f42760 100644 --- a/drivers/gpu/drm/radeon/radeon_device.c +++ b/drivers/gpu/drm/radeon/radeon_device.c @@ -851,7 +851,7 @@ int radeon_suspend_kms(struct drm_device *dev, pm_message_t state) if (rfb == NULL || rfb->obj == NULL) { continue; } - robj = rfb->obj->driver_private; + robj = gem_to_radeon_bo(rfb->obj); /* don't unpin kernel fb objects */ if (!radeon_fbdev_robj_is_fb(rdev, robj)) { r = radeon_bo_reserve(robj, false); diff --git a/drivers/gpu/drm/radeon/radeon_fb.c b/drivers/gpu/drm/radeon/radeon_fb.c index efa2118..99712b3 100644 --- a/drivers/gpu/drm/radeon/radeon_fb.c +++ b/drivers/gpu/drm/radeon/radeon_fb.c @@ -90,7 +90,7 @@ static int radeon_align_pitch(struct radeon_device *rdev, int width, int bpp, bo
static void radeonfb_destroy_pinned_object(struct drm_gem_object *gobj) { - struct radeon_bo *rbo = gobj->driver_private; + struct radeon_bo *rbo = gem_to_radeon_bo(gobj); int ret;
ret = radeon_bo_reserve(rbo, false); @@ -128,7 +128,7 @@ static int radeonfb_create_pinned_object(struct radeon_fbdev *rfbdev, aligned_size); return -ENOMEM; } - rbo = gobj->driver_private; + rbo = gem_to_radeon_bo(gobj);
if (fb_tiled) tiling_flags = RADEON_TILING_MACRO; @@ -202,7 +202,7 @@ static int radeonfb_create(struct radeon_fbdev *rfbdev, mode_cmd.depth = sizes->surface_depth;
ret = radeonfb_create_pinned_object(rfbdev, &mode_cmd, &gobj); - rbo = gobj->driver_private; + rbo = gem_to_radeon_bo(gobj);
/* okay we have an object now allocate the framebuffer */ info = framebuffer_alloc(0, device); @@ -405,14 +405,14 @@ int radeon_fbdev_total_size(struct radeon_device *rdev) struct radeon_bo *robj; int size = 0;
- robj = rdev->mode_info.rfbdev->rfb.obj->driver_private; + robj = gem_to_radeon_bo(rdev->mode_info.rfbdev->rfb.obj); size += radeon_bo_size(robj); return size; }
bool radeon_fbdev_robj_is_fb(struct radeon_device *rdev, struct radeon_bo *robj) { - if (robj == rdev->mode_info.rfbdev->rfb.obj->driver_private) + if (robj == gem_to_radeon_bo(rdev->mode_info.rfbdev->rfb.obj)) return true; return false; } diff --git a/drivers/gpu/drm/radeon/radeon_gem.c b/drivers/gpu/drm/radeon/radeon_gem.c index 9873a96..072d21c 100644 --- a/drivers/gpu/drm/radeon/radeon_gem.c +++ b/drivers/gpu/drm/radeon/radeon_gem.c @@ -39,9 +39,8 @@ int radeon_gem_object_init(struct drm_gem_object *obj)
void radeon_gem_object_free(struct drm_gem_object *gobj) { - struct radeon_bo *robj = gobj->driver_private; + struct radeon_bo *robj = gem_to_radeon_bo(gobj);
- gobj->driver_private = NULL; if (robj) { radeon_bo_unref(&robj); } @@ -79,7 +78,7 @@ int radeon_gem_object_create(struct radeon_device *rdev, int size, int radeon_gem_object_pin(struct drm_gem_object *obj, uint32_t pin_domain, uint64_t *gpu_addr) { - struct radeon_bo *robj = obj->driver_private; + struct radeon_bo *robj = gem_to_radeon_bo(obj); int r;
r = radeon_bo_reserve(robj, false); @@ -92,7 +91,7 @@ int radeon_gem_object_pin(struct drm_gem_object *obj, uint32_t pin_domain,
void radeon_gem_object_unpin(struct drm_gem_object *obj) { - struct radeon_bo *robj = obj->driver_private; + struct radeon_bo *robj = gem_to_radeon_bo(obj); int r;
r = radeon_bo_reserve(robj, false); @@ -110,7 +109,7 @@ int radeon_gem_set_domain(struct drm_gem_object *gobj, int r;
/* FIXME: reeimplement */ - robj = gobj->driver_private; + robj = gem_to_radeon_bo(gobj); /* work out where to validate the buffer to */ domain = wdomain; if (!domain) { @@ -224,7 +223,7 @@ int radeon_gem_set_domain_ioctl(struct drm_device *dev, void *data, if (gobj == NULL) { return -ENOENT; } - robj = gobj->driver_private; + robj = gem_to_radeon_bo(gobj);
r = radeon_gem_set_domain(gobj, args->read_domains, args->write_domain);
@@ -243,7 +242,7 @@ int radeon_gem_mmap_ioctl(struct drm_device *dev, void *data, if (gobj == NULL) { return -ENOENT; } - robj = gobj->driver_private; + robj = gem_to_radeon_bo(gobj); args->addr_ptr = radeon_bo_mmap_offset(robj); drm_gem_object_unreference_unlocked(gobj); return 0; @@ -262,7 +261,7 @@ int radeon_gem_busy_ioctl(struct drm_device *dev, void *data, if (gobj == NULL) { return -ENOENT; } - robj = gobj->driver_private; + robj = gem_to_radeon_bo(gobj); r = radeon_bo_wait(robj, &cur_placement, true); switch (cur_placement) { case TTM_PL_VRAM: @@ -292,7 +291,7 @@ int radeon_gem_wait_idle_ioctl(struct drm_device *dev, void *data, if (gobj == NULL) { return -ENOENT; } - robj = gobj->driver_private; + robj = gem_to_radeon_bo(gobj); r = radeon_bo_wait(robj, NULL, false); /* callback hw specific functions if any */ if (robj->rdev->asic->ioctl_wait_idle) @@ -313,7 +312,7 @@ int radeon_gem_set_tiling_ioctl(struct drm_device *dev, void *data, gobj = drm_gem_object_lookup(dev, filp, args->handle); if (gobj == NULL) return -ENOENT; - robj = gobj->driver_private; + robj = gem_to_radeon_bo(gobj); r = radeon_bo_set_tiling_flags(robj, args->tiling_flags, args->pitch); drm_gem_object_unreference_unlocked(gobj); return r; @@ -331,7 +330,7 @@ int radeon_gem_get_tiling_ioctl(struct drm_device *dev, void *data, gobj = drm_gem_object_lookup(dev, filp, args->handle); if (gobj == NULL) return -ENOENT; - rbo = gobj->driver_private; + rbo = gem_to_radeon_bo(gobj); r = radeon_bo_reserve(rbo, false); if (unlikely(r != 0)) goto out; diff --git a/drivers/gpu/drm/radeon/radeon_legacy_crtc.c b/drivers/gpu/drm/radeon/radeon_legacy_crtc.c index ace2e63..ee76d29 100644 --- a/drivers/gpu/drm/radeon/radeon_legacy_crtc.c +++ b/drivers/gpu/drm/radeon/radeon_legacy_crtc.c @@ -415,7 +415,7 @@ int radeon_crtc_do_set_base(struct drm_crtc *crtc,
/* Pin framebuffer & get tilling informations */ obj = radeon_fb->obj; - rbo = obj->driver_private; + rbo = gem_to_radeon_bo(obj); r = radeon_bo_reserve(rbo, false); if (unlikely(r != 0)) return r; @@ -520,7 +520,7 @@ int radeon_crtc_do_set_base(struct drm_crtc *crtc,
if (!atomic && fb && fb != crtc->fb) { radeon_fb = to_radeon_framebuffer(fb); - rbo = radeon_fb->obj->driver_private; + rbo = gem_to_radeon_bo(radeon_fb->obj); r = radeon_bo_reserve(rbo, false); if (unlikely(r != 0)) return r; diff --git a/drivers/gpu/drm/radeon/radeon_object.c b/drivers/gpu/drm/radeon/radeon_object.c index 30a2d54..99efe4a 100644 --- a/drivers/gpu/drm/radeon/radeon_object.c +++ b/drivers/gpu/drm/radeon/radeon_object.c @@ -113,7 +113,7 @@ int radeon_bo_create(struct radeon_device *rdev, } bo->rdev = rdev; bo->gobj = &bo->gem_base; - bo->gem_base.driver_private = bo; + bo->gem_base.driver_private = NULL; bo->surface_reg = -1; INIT_LIST_HEAD(&bo->list);
@@ -266,7 +266,6 @@ void radeon_bo_force_delete(struct radeon_device *rdev) list_del_init(&bo->list); mutex_unlock(&bo->rdev->gem.mutex); radeon_bo_unref(&bo); - gobj->driver_private = NULL; drm_gem_object_unreference(gobj); mutex_unlock(&rdev->ddev->struct_mutex); }
Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/radeon/radeon.h | 1 - drivers/gpu/drm/radeon/radeon_object.c | 9 +++------ 2 files changed, 3 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/radeon/radeon.h b/drivers/gpu/drm/radeon/radeon.h index 239eba7..9464100 100644 --- a/drivers/gpu/drm/radeon/radeon.h +++ b/drivers/gpu/drm/radeon/radeon.h @@ -255,7 +255,6 @@ struct radeon_bo { int surface_reg; /* Constant after initialization */ struct radeon_device *rdev; - struct drm_gem_object *gobj; struct drm_gem_object gem_base; }; #define gem_to_radeon_bo(gobj) container_of((gobj), struct radeon_bo, gem_base) diff --git a/drivers/gpu/drm/radeon/radeon_object.c b/drivers/gpu/drm/radeon/radeon_object.c index 99efe4a..18db45f 100644 --- a/drivers/gpu/drm/radeon/radeon_object.c +++ b/drivers/gpu/drm/radeon/radeon_object.c @@ -112,7 +112,6 @@ int radeon_bo_create(struct radeon_device *rdev, return r; } bo->rdev = rdev; - bo->gobj = &bo->gem_base; bo->gem_base.driver_private = NULL; bo->surface_reg = -1; INIT_LIST_HEAD(&bo->list); @@ -250,7 +249,6 @@ int radeon_bo_evict_vram(struct radeon_device *rdev) void radeon_bo_force_delete(struct radeon_device *rdev) { struct radeon_bo *bo, *n; - struct drm_gem_object *gobj;
if (list_empty(&rdev->gem.objects)) { return; @@ -258,15 +256,14 @@ void radeon_bo_force_delete(struct radeon_device *rdev) dev_err(rdev->dev, "Userspace still has active objects !\n"); list_for_each_entry_safe(bo, n, &rdev->gem.objects, list) { mutex_lock(&rdev->ddev->struct_mutex); - gobj = bo->gobj; dev_err(rdev->dev, "%p %p %lu %lu force free\n", - gobj, bo, (unsigned long)gobj->size, - *((unsigned long *)&gobj->refcount)); + &bo->gem_base, bo, (unsigned long)bo->gem_base.size, + *((unsigned long *)&bo->gem_base.refcount)); mutex_lock(&bo->rdev->gem.mutex); list_del_init(&bo->list); mutex_unlock(&bo->rdev->gem.mutex); radeon_bo_unref(&bo); - drm_gem_object_unreference(gobj); + drm_gem_object_unreference(&bo->gem_base); mutex_unlock(&rdev->ddev->struct_mutex); } }
Hi, Daniel,
My main concerns previously for embedding GEM objects as user-space references for TTM has been twofold and implementation specific.
1) The locking has been using global mutexes where local spin- or RCU locks have been more appropriate. It looks like this has finally been / is finally going to be addressed.
2) The gem object is too specialized for general purpose use: a) The shmem member has no natural use with TTM except possibly as a persistent swap storage, but in an ideal world, TTM would talk to the swap cache directly so in the longer term there is no need for the shmem object at all. b) Implementations may want to use other user-space visible objects than buffer objects: For example fence objects or CPU synchronization objects. The common traits of / operations on these are user-space visibility, inter-process visibility, refcounting and destruction when the relevant file is closed.
Hence a class that provides only the user-space handles, naming (flink), refcounting and registering with a file handle would be the best choice of a "base" class. Traditional Gem objects could derive from those and provide any extra *generic* members needed for buffer objects.
This doesn't really affect your work though. Just some comments on why vmwgfx don't use GEM objects by default and how they could be made optimal for TTM-aware drivers.
Thanks, /Thomas
On 11/13/2010 09:57 PM, Daniel Vetter wrote:
Hi all,
This patch series embeds drm_gem_object into struct radeon_bo and adjusts any references. I've decided against embedding the gem stuff into struct ttm_bo because
- vmwgfx doesn't use gem and
- ttm is used for implementing the memory management, whereas gem provides the userspace interface (I know, there's some overlap there, but that's not really a new problem).
Please review and consider merging for -next.
Yours, Daniel
Daniel Vetter (3): drm/radeon: embed struct drm_gem_object drm/radeon: introduce gem_to_radeon_bo helper drm/radeon: kill radeon_bo->gobj pointer
drivers/gpu/drm/radeon/atombios_crtc.c | 8 ++-- drivers/gpu/drm/radeon/evergreen_blit_kms.c | 2 +- drivers/gpu/drm/radeon/r600.c | 2 +- drivers/gpu/drm/radeon/r600_blit_kms.c | 2 +- drivers/gpu/drm/radeon/radeon.h | 3 +- drivers/gpu/drm/radeon/radeon_benchmark.c | 4 +- drivers/gpu/drm/radeon/radeon_cs.c | 2 +- drivers/gpu/drm/radeon/radeon_device.c | 4 +- drivers/gpu/drm/radeon/radeon_fb.c | 10 +++--- drivers/gpu/drm/radeon/radeon_gart.c | 2 +- drivers/gpu/drm/radeon/radeon_gem.c | 43 ++++++++++++--------------- drivers/gpu/drm/radeon/radeon_legacy_crtc.c | 4 +- drivers/gpu/drm/radeon/radeon_object.c | 24 +++++++-------- drivers/gpu/drm/radeon/radeon_object.h | 2 +- drivers/gpu/drm/radeon/radeon_ring.c | 4 +- drivers/gpu/drm/radeon/radeon_test.c | 4 +- drivers/gpu/drm/radeon/radeon_ttm.c | 2 +- drivers/gpu/drm/radeon/rv770.c | 2 +- 18 files changed, 59 insertions(+), 65 deletions(-)
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Hi Thomas,
Thanks for your comments about ttm and vmwgfx. Some of my own ideas about where this might all be heading below.
On Mon, Nov 15, 2010 at 08:25:16AM +0100, Thomas Hellstrom wrote:
Hi, Daniel,
My main concerns previously for embedding GEM objects as user-space references for TTM has been twofold and implementation specific.
- The locking has been using global mutexes where local spin- or
RCU locks have been more appropriate. It looks like this has finally been / is finally going to be addressed.
gem object lookup / insert has always (iirc) been protected by a spinlock. drm/i915 is just a bit inefficient with lookups, hence this spinlock showing up in profiling.
- The gem object is too specialized for general purpose use:
a) The shmem member has no natural use with TTM except possibly as a persistent swap storage, but in an ideal world, TTM would talk to the swap cache directly so in the longer term there is no need for the shmem object at all.
Chris Wilson is working on a gem_vm for i915 that employs a address_space per bo and directly manages swap entries and has its own page allocator (actualy cflushed page cache). I haven't yet looked into this closely (especially the ttm part), but this might (at least in parts) be shareable between i915 and ttm. At least it gets rid of the shmfs inode in struct drm_gem_object!
b) Implementations may want to use other user-space visible objects than buffer objects: For example fence objects or CPU synchronization objects. The common traits of / operations on these are user-space visibility, inter-process visibility, refcounting and destruction when the relevant file is closed.
Hence a class that provides only the user-space handles, naming (flink), refcounting and registering with a file handle would be the best choice of a "base" class. Traditional Gem objects could derive from those and provide any extra *generic* members needed for buffer objects.
This doesn't really affect your work though. Just some comments on why vmwgfx don't use GEM objects by default and how they could be made optimal for TTM-aware drivers.
Yeah, I've noticed that vmwgfx is the (sometimes) sole user of many of the more fancy stuff in ttm. And I also don't like the duplication between gem and ttm. My plan (i.e. wishful thinking) is to have a common set of helper functions that can be shared between i915, radeon and nouveau and any other driver with a gem-like interface (i.e. buffer-objects + execbuffer, gpu<->cpu sync abstracted away by the kernel). Leaving the cracy stuff for drivers that need it (vmwgfx). Nothing concrete though (besides a new testing rig to start hacking on nouveau), I'll intend to simply plow along, hunting for common patterns.
Thanks, /Thomas
Yours, Daniel
On 11/15/2010 07:45 PM, Daniel Vetter wrote:
Hi Thomas,
Thanks for your comments about ttm and vmwgfx. Some of my own ideas about where this might all be heading below.
On Mon, Nov 15, 2010 at 08:25:16AM +0100, Thomas Hellstrom wrote:
Hi, Daniel,
My main concerns previously for embedding GEM objects as user-space references for TTM has been twofold and implementation specific.
- The locking has been using global mutexes where local spin- or
RCU locks have been more appropriate. It looks like this has finally been / is finally going to be addressed.
gem object lookup / insert has always (iirc) been protected by a spinlock. drm/i915 is just a bit inefficient with lookups, hence this spinlock showing up in profiling.
- The gem object is too specialized for general purpose use:
a) The shmem member has no natural use with TTM except possibly as a persistent swap storage, but in an ideal world, TTM would talk to the swap cache directly so in the longer term there is no need for the shmem object at all.
Chris Wilson is working on a gem_vm for i915 that employs a address_space per bo and directly manages swap entries and has its own page allocator (actualy cflushed page cache). I haven't yet looked into this closely (especially the ttm part), but this might (at least in parts) be shareable between i915 and ttm. At least it gets rid of the shmfs inode in struct drm_gem_object!
b) Implementations may want to use other user-space visible objects than buffer objects: For example fence objects or CPU synchronization objects. The common traits of / operations on these are user-space visibility, inter-process visibility, refcounting and destruction when the relevant file is closed.
Hence a class that provides only the user-space handles, naming (flink), refcounting and registering with a file handle would be the best choice of a "base" class. Traditional Gem objects could derive from those and provide any extra *generic* members needed for buffer objects.
This doesn't really affect your work though. Just some comments on why vmwgfx don't use GEM objects by default and how they could be made optimal for TTM-aware drivers.
Yeah, I've noticed that vmwgfx is the (sometimes) sole user of many of the more fancy stuff in ttm. And I also don't like the duplication between gem and ttm. My plan (i.e. wishful thinking) is to have a common set of helper functions that can be shared between i915, radeon and nouveau and any other driver with a gem-like interface (i.e. buffer-objects + execbuffer, gpu<->cpu sync abstracted away by the kernel). Leaving the cracy stuff for drivers that need it (vmwgfx). Nothing concrete though (besides a new testing rig to start hacking on nouveau), I'll intend to simply plow along, hunting for common patterns.
Sounds good. I actually think the only thing which is currently vmwgfx-specific are the ttm_objects and the ttm_locks. TTM objects could really be replaced by gem objects with the bo-specific part stripped, as discussed above. And then there is the ttm lock to allow concurrent execbufs running.
/Thomas
Thanks, /Thomas
Yours, Daniel
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Daniel Vetter -
Thomas Hellstrom