From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ben Widawsky Subject: Re: [PATCH 01/18] drm: Introduce drm_mm_create_block() Date: Sun, 28 Oct 2012 11:12:05 -0700 Message-ID: <20121028111205.00005f88@unknown> References: <1350666204-8101-1-git-send-email-chris@chris-wilson.co.uk> <20121026144743.6d168d86@bwidawsk.net> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: 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 B4F399E710 for ; Sun, 28 Oct 2012 11:12:29 -0700 (PDT) In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-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 Cc: dri-devel@lists.freedestkop.org, Dave Airlie , intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org On Sun, 28 Oct 2012 09:57:21 +0000 Chris Wilson wrote: > On Fri, 26 Oct 2012 14:47:43 -0700, Ben Widawsky > wrote: > > On Fri, 19 Oct 2012 18:03:07 +0100 > > Chris Wilson wrote: > > > > > To be used later by i915 to preallocate exact blocks of space > > > from the range manager. > > > > > > Signed-off-by: Chris Wilson > > > Cc: Dave Airlie > > > Cc: dri-devel@lists.freedestkop.org > > > > With bikesheds below addressed or not: > > Reviewed-by: Ben Widawsky > > > --- > > > 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) > > > +{ > > > > > > 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. > > > > 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); > > > > > > 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. > > > > 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 >