From mboxrd@z Thu Jan 1 00:00:00 1970 From: Chris Wilson Subject: Re: [PATCH 4/4] drm/i915: Use eb for more stuff Date: Thu, 13 Oct 2011 13:00:02 +0100 Message-ID: References: <1318394633-11143-1-git-send-email-ben@bwidawsk.net> <1318460182-21302-1-git-send-email-ben@bwidawsk.net> <1318460182-21302-5-git-send-email-ben@bwidawsk.net> <20111013085825.GF2938@phenom.ffwll.local> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by gabe.freedesktop.org (Postfix) with ESMTP id 966A89E7C2 for ; Thu, 13 Oct 2011 05:00:05 -0700 (PDT) In-Reply-To: <20111013085825.GF2938@phenom.ffwll.local> 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: Daniel Vetter , Ben Widawsky Cc: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org On Thu, 13 Oct 2011 10:58:25 +0200, Daniel Vetter wrote: > On Wed, Oct 12, 2011 at 03:56:22PM -0700, Ben Widawsky wrote: > > This refactor is useful for some future work I'll be doing on the > > execbuffer path. In addition to being a pretty easy prerequisite, it > > also helped me track down the bug uncovered in the first patch. > > > > Signed-off-by: Ben Widawsky > > --- > > drivers/gpu/drm/i915/i915_gem_execbuffer.c | 102 ++++++++++++++-------------- > > 1 files changed, 51 insertions(+), 51 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c > > index 182a2b9..b3beaae 100644 > > --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c > > +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c > > @@ -40,6 +40,16 @@ struct change_domains { > > uint32_t flips; > > }; > > > > +struct eb_objects { > > + struct drm_i915_gem_object *batch_obj; > > + struct list_head objects; > > + int buffer_count; > > + int mode; > > + int and; > > While you whack this code, can you do an s/and/end, I think that's just a > typo ... It was meant to be 'and' since it was the value of the mask I was anding with. I'm sorry I appear to have spent too much time inside the Xserver, end = and + 1. > > > + struct hlist_head buckets[0]; > > + /* DO NOT PUT ANYTHING HERE */ > > When you drop the array size, the compiler will enforce that for you (for > $compiler = gcc at least). I wanted to say that surely the comment was obvious from the '[0]' ;-) > > > +}; > > + > > /* > > * Set the next domain for the specified object. This > > * may not actually perform the necessary flushing/invaliding though, > > @@ -207,11 +217,6 @@ i915_gem_object_set_to_gpu_domain(struct drm_i915_gem_object *obj, > > cd->flush_rings |= ring->id; > > } > > > > -struct eb_objects { > > - int and; > > - struct hlist_head buckets[0]; > > -}; > > - > > static struct eb_objects * > > eb_create(int size) > > { > > @@ -225,6 +230,8 @@ eb_create(int size) > > if (eb == NULL) > > return eb; > > > > + INIT_LIST_HEAD(&eb->objects); > > + > > eb->and = count - 1; > > return eb; > > } > > @@ -232,12 +239,22 @@ eb_create(int size) > > static void > > eb_reset(struct eb_objects *eb) > > { > > + while (!list_empty(&eb->objects)) { > > list_for_each_entry_safe No, the while (!empty) is the more common and much simpler idiom to use here when clearing a list. > > - mode = args->flags & I915_EXEC_CONSTANTS_MASK; > > - ret = i915_gem_set_constant_offset(ring, mode); > > + eb->mode = args->flags & I915_EXEC_CONSTANTS_MASK; > > + ret = i915_gem_set_constant_offset(ring, eb->mode); > > The eb->mode refactor here otoh looks a bit superflous. Is this needed for > a future patch of yours? I'm waiting to see how this pans out as well... ;-) -Chris -- Chris Wilson, Intel Open Source Technology Centre