From mboxrd@z Thu Jan  1 00:00:00 1970
From: Ben Widawsky <ben@bwidawsk.net>
Subject: Re: [PATCH 01/18] drm: Introduce drm_mm_create_block()
Date: Sun, 28 Oct 2012 11:14:25 -0700
Message-ID: <20121028111425.00000355@unknown>
References: <1350666204-8101-1-git-send-email-chris@chris-wilson.co.uk>
	<20121026144743.6d168d86@bwidawsk.net>
	<b94cdc$74b5g6@fmsmga001.fm.intel.com>
	<20121028111205.00005f88@unknown>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org>
Received: from shiva.chad-versace.us (209-20-75-48.static.cloud-ips.com
	[209.20.75.48])
	by gabe.freedesktop.org (Postfix) with ESMTP id D3F029E710
	for <intel-gfx@lists.freedesktop.org>;
	Sun, 28 Oct 2012 11:14:48 -0700 (PDT)
In-Reply-To: <20121028111205.00005f88@unknown>
List-Unsubscribe: <http://lists.freedesktop.org/mailman/options/intel-gfx>,
	<mailto:intel-gfx-request@lists.freedesktop.org?subject=unsubscribe>
List-Archive: <http://lists.freedesktop.org/archives/intel-gfx>
List-Post: <mailto:intel-gfx@lists.freedesktop.org>
List-Help: <mailto:intel-gfx-request@lists.freedesktop.org?subject=help>
List-Subscribe: <http://lists.freedesktop.org/mailman/listinfo/intel-gfx>,
	<mailto:intel-gfx-request@lists.freedesktop.org?subject=subscribe>
Sender: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org
Errors-To: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org
To: Chris Wilson <chris@chris-wilson.co.uk>
Cc: intel-gfx@lists.freedesktop.org
List-Id: intel-gfx@lists.freedesktop.org

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