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 <ben at bwidawsk.net>
>
> 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
