From mboxrd@z Thu Jan 1 00:00:00 1970 From: Thomas Hellstrom Subject: Re: [PATCH 0/9] make struct drm_mm_node embeddable Date: Mon, 15 Nov 2010 08:58:13 +0100 Message-ID: <4CE0E815.3000908@shipmail.org> References: <1289583401-16605-1-git-send-email-daniel.vetter@ffwll.ch> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: Received: from smtp-outbound-1.vmware.com (smtp-outbound-1.vmware.com [65.115.85.69]) by gabe.freedesktop.org (Postfix) with ESMTP id E77769E9AD for ; Sun, 14 Nov 2010 23:58:26 -0800 (PST) In-Reply-To: <1289583401-16605-1-git-send-email-daniel.vetter@ffwll.ch> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: dri-devel-bounces+sf-dri-devel=m.gmane.org@lists.freedesktop.org Errors-To: dri-devel-bounces+sf-dri-devel=m.gmane.org@lists.freedesktop.org To: Daniel Vetter Cc: dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org On 11/12/2010 06:36 PM, Daniel Vetter wrote: > Hi all, > > This patch-set changes the algorithm in drm_mm.c to not need additional > allocations to track free space and adds an api to make embedding struct > drm_mm_node possible. Benefits: > > - If struct drm_mm_node is provided, no allocations need to be done anymore > in drm_mm. It looks like some decent surgery, but ttm should be able to > drop its preallocation dance. > - void *priv is back, but done right ;) > - Avoids a pointer chase when lru-scanning in i915 and saves a few bytes. > > As a proof of concept I've converted i915. Beware though, the drm/i915 > patches depend on my direct-gtt patches (which are actually the reason for > this series here). > > Tested on my i855gm, i945gme, ironlake and agp rv570. > > Comments, flames, reviews highly welcome. > Hi, Daniel! Nice work, although I have some comments about general applicability that we perhaps need to think about. 1) The space representation and space allocation algorithm is something that is private to the aperture management system. For a specialized implementation like i915 that is all fine, but Ben has recently abstracted that part out of the core TTM bo implementation. As an example, vmwgfx is now using kernel idas to manage aperture space, and drm_mm objects for traditional VRAM space. Hence, embedding drm_mm objects into ttm bos will not really be worthwile. At least not for aperture space management, and TTM will need to continue to "dance", both in the ida case and in the drm_mm case. For device address space, the situation is different, though, and it should be possible to embed the drm_mm objects, but that brings up the next thing: 2) The algorithm used by drm_mm has been around for a while and has seen a fair amount of changes, but nobody has yet attacked the algorithm used to search for free space, which was just quickly put together as an improvement on what was the old mesa range manager. In moderate fragmentation situations, the performance will degrade, particularly with "best match" searches. In the near future we'd probably want to add something like a "hole rb tree" rather than a "hole list", and a choice of algorithm for the user. With embeddable objects, unless you want to waste space for unused members, you'd need a separate drm_mm node subclass for each algorithm, whereas if you don't embed, you only need to allocate what you need. /Thomas > Please consider merging the core drm parts (and the nouveau prep patch) for > -next (the i915 patches need coordination with Chris Wilson, they're rather > invasive). > > Thanks, Daniel > > Daniel Vetter (9): > drm/nouveau: don't munge in drm_mm internals > drm: mm: track free areas implicitly > drm: mm: extract node insert helper functions > drm: mm: add api for embedding struct drm_mm_node > drm/i915: embed struct drm_mm_node into struct drm_i915_gem_object > drm/i915: kill obj->gtt_offset > drm/i915: kill gtt_list > drm: mm: add helper to unwind scan state > drm/i915: use drm_mm_for_each_scanned_node_reverse helper > > drivers/gpu/drm/drm_mm.c | 570 ++++++++++++++++------------- > drivers/gpu/drm/i915/i915_debugfs.c | 22 +- > drivers/gpu/drm/i915/i915_drv.h | 13 +- > drivers/gpu/drm/i915/i915_gem.c | 173 ++++----- > drivers/gpu/drm/i915/i915_gem_debug.c | 10 +- > drivers/gpu/drm/i915/i915_gem_evict.c | 37 +- > drivers/gpu/drm/i915/i915_gem_gtt.c | 14 +- > drivers/gpu/drm/i915/i915_gem_tiling.c | 6 +- > drivers/gpu/drm/i915/i915_irq.c | 34 +- > drivers/gpu/drm/i915/intel_display.c | 26 +- > drivers/gpu/drm/i915/intel_fb.c | 6 +- > drivers/gpu/drm/i915/intel_overlay.c | 14 +- > drivers/gpu/drm/i915/intel_ringbuffer.c | 10 +- > drivers/gpu/drm/nouveau/nouveau_object.c | 2 +- > drivers/gpu/drm/nouveau/nv50_instmem.c | 2 +- > include/drm/drm_mm.h | 49 ++- > 16 files changed, 525 insertions(+), 463 deletions(-) > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel >