Re: [PATCH 1/3] drm: Add colouring to the range allocator

[Daniel Vetter <daniel@ffwll.ch>]

On Tue, Jul 10, 2012 at 10:29:09AM +0100, Chris Wilson wrote:
> On Tue, 10 Jul 2012 11:21:57 +0200, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Mon, Jul 09, 2012 at 12:34:37PM +0100, Chris Wilson wrote:
> > > In order to support snoopable memory on non-LLC architectures (so that
> > > we can bind vgem objects into the i915 GATT for example), we have to
> > > avoid the prefetcher on the GPU from crossing memory domains and so
> > > prevent allocation of a snoopable PTE immediately following an uncached
> > > PTE. To do that, we need to extend the range allocator with support for
> > > tracking and segregating different node colours.
> > > 
> > > This will be used by i915 to segregate memory domains within the GTT.
> > > 
> > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > Cc: Dave Airlie <airlied@redhat.com
> > > Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> > > Cc: Ben Skeggs <bskeggs@redhat.com>
> > > Cc: Jerome Glisse <jglisse@redhat.com>
> > > Cc: Alex Deucher <alexander.deucher@amd.com>
> > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > 
> > Two little bikesheds:
> > - Do we really need 64bits of colour? Especially since we have quite a few
> >   bits of space left ...
> 
> It was following the convention that we passed around an argument large
> enough to stuff a pointer into if we ever needed to make a far more
> complex decision.

I think the right thing to do in that case would be to embed the gtt_space
and do an upcast ;-)

> > - I think we could add a new insert_color helper that always takes a range
> >   (we can select the right rang in the driver). That way this patch
> >   wouldn't need to touch the drivers, and we could take the opportunity to
> >   embed the gtt_space mm_node into our gem object ...
> 
> I was just a bit more wary of adding yet another helper since they
> quickly get just as confusing as the extra arguments they replace. :)

Oh, I guess you mean a different helper than I do. I think we should add a
new drm_mm_insert_node_colour function that takes a pre-allocated mm_node,
color and range and goes hole-hunting. That way we'd avoid changing any of
the existing drivers (who rather likely will never care about). And I
wouldn't have to convert over the drm_mm functions that deal with
pre-allocated drm_mm_node structs when I get around to resurrect the
embedd gtt_space patch.

I agree that shoveling all the alignment constrains into a new helper for
would be a bit too much overkill
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48