Re: [PATCH 01/18] drm: Introduce drm_mm_create_block()

[Ben Widawsky <ben@bwidawsk.net>]

By the way, you noticed you got the dri-devel address wrong?

On Sun, 28 Oct 2012 11:12:05 -0700
Ben Widawsky <ben@bwidawsk.net> wrote:

> On Sun, 28 Oct 2012 09:57:21 +0000
> Chris Wilson <chris@chris-wilson.co.uk> wrote:
> 
> > On Fri, 26 Oct 2012 14:47:43 -0700, Ben Widawsky <ben@bwidawsk.net>
> > wrote:
> > > On Fri, 19 Oct 2012 18:03:07 +0100
> > > Chris Wilson <chris@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@chris-wilson.co.uk>
> > > > Cc: Dave Airlie <airlied@redhat.com>
> > > > Cc: dri-devel@lists.freedestkop.org
> > > 
> > > With bikesheds below addressed or not:
> > > Reviewed-by: Ben Widawsky <ben@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
> > 
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx