From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ben Widawsky Subject: Re: [PATCH 4/4] drm/i915: Use eb for more stuff Date: Thu, 13 Oct 2011 10:31:11 -0700 Message-ID: <20111013173110.GC32465@bolo_yeung.jf.intel.com> 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 cloud01.chad-versace.us (184-106-247-128.static.cloud-ips.com [184.106.247.128]) by gabe.freedesktop.org (Postfix) with ESMTP id 2FD9B9E8B0 for ; Thu, 13 Oct 2011 10:35:08 -0700 (PDT) Content-Disposition: inline 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: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org On Thu, Oct 13, 2011 at 01:00:02PM +0100, Chris Wilson wrote: > 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]' ;-) > Well not obvious to me. Though it hindsight it should have been obvious. GCC 4.6.1 does not produce an error or warning if you do this. > > > > - 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 It's needed for a future patch, I'm cool with call this superfluous for now, and Keith can take it, or wait until it's needed. I can make it a bit more uniform as Chris recommended in some other comment. Ben