On Mon, Jul 1, 2013 at 8:32 PM, Ben Widawsky <ben at bwidawsk.net> wrote:
> 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.
I think now that we know what the end result should (more or less at
least) look like we can aim to make it right the first time we touch a
piece of code. That will reduce the churn in the patch series and so
make the beast easier to review.
Imo foreshadowing (to keep consistent with the "a patch series should
tell a story" analogy) is perfectly fine, and in many cases helps in
understanding the big picture of a large pile of patches.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
