Re: [PATCH 0/9] make struct drm_mm_node embeddable

[Thomas Hellstrom <thomas@shipmail.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
>