[Intel-gfx] [PATCH 01/18] drm: Introduce drm_mm_create_block()

[Ben Widawsky]

On Sun, 28 Oct 2012 09:57:21 +0000
Chris Wilson <chris at chris-wilson.co.uk> wrote:

> On Fri, 26 Oct 2012 14:47:43 -0700, Ben Widawsky <ben at bwidawsk.net>
> wrote:
> > On Fri, 19 Oct 2012 18:03:07 +0100
> > Chris Wilson <chris at chris-wilson.co.uk> wrote:
> > 
> > > To be used later by i915 to preallocate exact blocks of space
> > > from the range manager.
> > > 
> > > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> > > Cc: Dave Airlie <airlied at redhat.com>
> > > Cc: dri-devel at lists.freedestkop.org
> > 
> > With bikesheds below addressed or not:
> > Reviewed-by: Ben Widawsky <ben at bwidawsk.net>
> > > ---
> > >  drivers/gpu/drm/drm_mm.c |   49
> > > ++++++++++++++++++++++++++++++++++++++++++++++
> > > include/drm/drm_mm.h     |    4 ++++ 2 files changed, 53
> > > insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_mm.c b/drivers/gpu/drm/drm_mm.c
> > > index 9bb82f7..5db8c20 100644
> > > --- a/drivers/gpu/drm/drm_mm.c
> > > +++ b/drivers/gpu/drm/drm_mm.c
> > > @@ -161,6 +161,55 @@ static void drm_mm_insert_helper(struct
> > > drm_mm_node *hole_node, }
> > >  }
> > >  
> > > +struct drm_mm_node *drm_mm_create_block(struct drm_mm *mm,
> > > +					unsigned long start,
> > > +					unsigned long size,
> > > +					bool atomic)
> > > +{
> > 
> > <bikeshed>
> > You could add a best_fit field like some of the other interfaces
> > which will try to find a start == hole_start and end == hole_end.
> > I'd guess this interface won't be called enough to worry about
> > fragmentation too much though.
> > </bikeshed>
> 
> It's not a best fit though, it's an exact allocation request. The
> search is to find the location in the free list where we need to
> insert the node, and just as importantly reject the request if it
> would clobber an earlier allocation.

Yeah, my comment seems a bit silly now reading it again. I was
forgetting that start is a very specific place (as opposed to the
search_free case).

But, this does remind me since the hole_stack is ordered, instead of:
> +	if (hole_start > start || hole_end < end)
> +		continue;

Can't we do:
+	if (hole_start > start || hole_end < end)
+		break;

> 
> > 
> > > +	struct drm_mm_node *hole, *node;
> > > +	unsigned long end = start + size;
> > > +
> > > +	list_for_each_entry(hole, &mm->hole_stack, hole_stack) {
> > > +		unsigned long hole_start;
> > > +		unsigned long hole_end;
> > > +
> > > +		BUG_ON(!hole->hole_follows);
> > 
> > <bikeshed>
> > This isn't bad, but I don't think sticking the bug here is all that
> > helpful in finding where the bug occured, since it wasn't here.
> > WARN is perhaps more useful, but equally unhelpful IMO.
> > </bikeshed>
> 
> The BUG_ON() is to be consistent with the rest of the code, and so
> there isn't a conflict of interests when replacing all the common
> chunks with drm_mm_for_each_hole().
> -Chris
>