| Message ID | 1376442549-5087-2-git-send-email-benjamin.widawsky@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 > + > + mappable = > + vma->node.start + obj->base.size <= dev_priv->gtt.mappable_end; > > - /* Map and fenceable only changes if the VM is the global GGTT */ > - if (i915_is_ggtt(vm)) > obj->map_and_fenceable = mappable && fenceable; > + } > > WARN_ON(map_and_fenceable && !obj->map_and_fenceable); > > -- > 1.8.3.4 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
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. The extra indirection would be immeasurable. Again, you've already changed it, so meh.
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
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; + + mappable = + vma->node.start + obj->base.size <= dev_priv->gtt.mappable_end; - /* Map and fenceable only changes if the VM is the global GGTT */ - if (i915_is_ggtt(vm)) obj->map_and_fenceable = mappable && fenceable; + } WARN_ON(map_and_fenceable && !obj->map_and_fenceable);