Re: [PATCH 2/4] [v3] drm/i915: cleanup map&fence in bind

[Daniel Vetter <daniel@ffwll.ch>]

On Wed, Aug 14, 2013 at 10:27:42AM -0700, Ben Widawsky wrote:
> On Wed, Aug 14, 2013 at 10:18:55AM +0200, Daniel Vetter wrote:
> > On Tue, Aug 13, 2013 at 06:09:07PM -0700, Ben Widawsky wrote:
> > > Cleanup the map and fenceable setting during bind to make more sense,
> > > and not check i915_is_ggtt() 2 unnecessary times
> > > 
> > > v2: Move the bools into the if block (Chris) - There are ways to tidy
> > > this function (fence calculations for instance) even further, but they
> > > are quite invasive, so I am punting on those unless specifically asked.
> > > 
> > > v3: Add newline between variable declaration and logic (Chris)
> > > 
> > > Recommended-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> > > ---
> > >  drivers/gpu/drm/i915/i915_gem.c | 19 +++++++++----------
> > >  1 file changed, 9 insertions(+), 10 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > > index 4a58ead..01cc016 100644
> > > --- a/drivers/gpu/drm/i915/i915_gem.c
> > > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > > @@ -3106,7 +3106,6 @@ i915_gem_object_bind_to_vm(struct drm_i915_gem_object *obj,
> > >  	struct drm_device *dev = obj->base.dev;
> > >  	drm_i915_private_t *dev_priv = dev->dev_private;
> > >  	u32 size, fence_size, fence_alignment, unfenced_alignment;
> > > -	bool mappable, fenceable;
> > >  	size_t gtt_max =
> > >  		map_and_fenceable ? dev_priv->gtt.mappable_end : vm->total;
> > >  	struct i915_vma *vma;
> > > @@ -3191,18 +3190,18 @@ search_free:
> > >  	list_move_tail(&obj->global_list, &dev_priv->mm.bound_list);
> > >  	list_add_tail(&vma->mm_list, &vm->inactive_list);
> > >  
> > > -	fenceable =
> > > -		i915_is_ggtt(vm) &&
> > > -		i915_gem_obj_ggtt_size(obj) == fence_size &&
> > > -		(i915_gem_obj_ggtt_offset(obj) & (fence_alignment - 1)) == 0;
> > > +	if (i915_is_ggtt(vm)) {
> > > +		bool mappable, fenceable;
> > >  
> > > -	mappable =
> > > -		i915_is_ggtt(vm) &&
> > > -		vma->node.start + obj->base.size <= dev_priv->gtt.mappable_end;
> > > +		fenceable =
> > > +			i915_gem_obj_ggtt_size(obj) == fence_size &&
> > > +			(i915_gem_obj_ggtt_offset(obj) & (fence_alignment - 1)) == 0;
> > 
> > Why not go the full mounty and also use vma->node here? Would also make
> > checkpatch a bit more happy. I'll do a follow-up commit.
> > -Daniel
> > 
> 
> You've already done it, so it's moot. The idea I had was to use "ggtt"
> as much as possible when it can only every be ggtt. I think this would
> be helpful for both clarity, and debug.

I disagree here and I think the extra indirection hampers code readability
- I always end up checking your little functions to make sure they
actually fish out the right value from the right vma. So my plan is that
once this all lands I'll fully rip them out.

This wasn't ever about performance, although I admit that unnecessary
looping in our gem code does irk me a bit ;-) But again mostly from a
clarify pov.

For marking ggtt-only paths I think Chris' suggestion to enclose such code
in if (is_ggtt(vm)) {} blocks which you've implemented here is much better
for clarity.

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch