From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ben Widawsky Subject: Re: [PATCH 30/66] drm/i915: Getter/setter for object attributes Date: Mon, 1 Jul 2013 11:32:19 -0700 Message-ID: <20130701183218.GC4242@bwidawsk.net> References: <1372375867-1003-1-git-send-email-ben@bwidawsk.net> <1372375867-1003-31-git-send-email-ben@bwidawsk.net> <20130630130005.GK18285@phenom.ffwll.local> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from shiva.localdomain (unknown [209.20.75.48]) by gabe.freedesktop.org (Postfix) with ESMTP id C92C4E5CB7 for ; Mon, 1 Jul 2013 11:32:26 -0700 (PDT) Content-Disposition: inline In-Reply-To: <20130630130005.GK18285@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 Cc: Intel GFX List-Id: intel-gfx@lists.freedesktop.org On Sun, Jun 30, 2013 at 03:00:05PM +0200, Daniel Vetter wrote: > On Thu, Jun 27, 2013 at 04:30:31PM -0700, Ben Widawsky wrote: > > This will be handy when we add VMs. It's not strictly, necessary, but it > > will make the code much cleaner. > > > > Signed-off-by: Ben Widawsky > > You're going to hate, but this is patch ordering fail. Imo this should be > one of the very first patches, at least before you kill obj->gtt_offset. > > To increase your hatred some more, I have bikesheds on the names, too. > > I think the best would be to respin this patch and merge it right away. > It'll cause tons of conflicts. But keeping it as no. 30 in this series > will be even worse, since merging the first 30 patches won't happen > instantly. So much more potential for rebase hell imo. > > The MO for when you stumble over such a giant renaming operation should be > imo to submit the "add inline abstraction functions" patch(es) right away. > That way everyone else who potentially works in the same area also gets a > heads up. > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > > index bc80ce0..56d47bc 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.h > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > @@ -1349,6 +1349,27 @@ struct drm_i915_gem_object { > > > > #define to_intel_bo(x) container_of(x, struct drm_i915_gem_object, base) > > > > +static inline unsigned long i915_gem_obj_offset(struct drm_i915_gem_object *o) > > +{ > > + return o->gtt_space->start; > > +} > > To differentiate from the ppgtt offset I'd call this > i915_gem_obj_ggtt_offset. > > > + > > +static inline bool i915_gem_obj_bound(struct drm_i915_gem_object *o) > > +{ > > + return o->gtt_space != NULL; > > +} > > Same here, I think we want ggtt inserted. > > > + > > +static inline unsigned long i915_gem_obj_size(struct drm_i915_gem_object *o) > > +{ > > + return o->gtt_space->size; > > +} > > This is even more misleading and the real reasons I vote for all the ggtt > bikesheds: ggtt_size != obj->size is very much possible (on gen2/3 only > though). We use that to satisfy alignment/size constraints on tiled > objects. So the i915_gem_obj_ggtt_size rename is mandatory here. > > > + > > +static inline void i915_gem_obj_set_color(struct drm_i915_gem_object *o, > > + enum i915_cache_level color) > > +{ > > + o->gtt_space->color = color; > > +} > > Dito for consistency. > > Cheers, Daniel > > All of this is addressed in future patches. As we've discussed, I think I'll have to respin it anyway, so I'll name it as such upfront. To me it felt a little weird to start calling things "ggtt" before I made the separation. [snip] -- Ben Widawsky, Intel Open Source Technology Center