| Message ID | 1458298019-3755-1-git-send-email-matthew.auld@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 18/03/16 10:46, Matthew Auld wrote: > No functional change, just makes the code easier to follow. > > v2: > - Remove local fence_size variable > (Tvrtko Ursulin) > - Remove redundant NULL ggtt_view check > - Reuse size variable > > Signed-off-by: Matthew Auld <matthew.auld@intel.com> > --- > drivers/gpu/drm/i915/i915_gem.c | 51 +++++++++++------------------------------ > 1 file changed, 14 insertions(+), 37 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index f45856d..e5d9d0b 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -3468,50 +3468,27 @@ i915_gem_object_bind_to_vm(struct drm_i915_gem_object *obj, > u32 fence_alignment, unfenced_alignment; > u32 search_flag, alloc_flag; > u64 start, end; > - u64 size, fence_size; > + u64 size; > struct i915_vma *vma; > int ret; > > - if (i915_is_ggtt(vm)) { > - u32 view_size; > - > - if (WARN_ON(!ggtt_view)) > - return ERR_PTR(-EINVAL); > - > - view_size = i915_ggtt_view_size(obj, ggtt_view); > - > - fence_size = i915_gem_get_gtt_size(dev, > - view_size, > - obj->tiling_mode); > - fence_alignment = i915_gem_get_gtt_alignment(dev, > - view_size, > - obj->tiling_mode, > - true); > - unfenced_alignment = i915_gem_get_gtt_alignment(dev, > - view_size, > - obj->tiling_mode, > - false); > - size = flags & PIN_MAPPABLE ? fence_size : view_size; > - } else { > - fence_size = i915_gem_get_gtt_size(dev, > - obj->base.size, > - obj->tiling_mode); > - fence_alignment = i915_gem_get_gtt_alignment(dev, > - obj->base.size, > - obj->tiling_mode, > - true); > - unfenced_alignment = > - i915_gem_get_gtt_alignment(dev, > - obj->base.size, > - obj->tiling_mode, > - false); > - size = flags & PIN_MAPPABLE ? fence_size : obj->base.size; > - } > + if (i915_is_ggtt(vm)) > + size = i915_ggtt_view_size(obj, ggtt_view); > + else > + size = obj->base.size; > + > + fence_alignment = i915_gem_get_gtt_alignment(dev, size, > + obj->tiling_mode, true); > + unfenced_alignment = i915_gem_get_gtt_alignment(dev, size, > + obj->tiling_mode, > + false); > > start = flags & PIN_OFFSET_BIAS ? flags & PIN_OFFSET_MASK : 0; > end = vm->total; > - if (flags & PIN_MAPPABLE) > + if (flags & PIN_MAPPABLE) { > end = min_t(u64, end, dev_priv->gtt.mappable_end); > + size = i915_gem_get_gtt_size(dev, size, obj->tiling_mode); > + } > if (flags & PIN_ZONE_4G) > end = min_t(u64, end, (1ULL << 32) - PAGE_SIZE); > > Looks good to me. Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Regards, Tvrtko
On Fri, Mar 18, 2016 at 11:10:50AM +0000, Tvrtko Ursulin wrote: > > > On 18/03/16 10:46, Matthew Auld wrote: > >No functional change, just makes the code easier to follow. > > > >v2: > > - Remove local fence_size variable > >(Tvrtko Ursulin) > > - Remove redundant NULL ggtt_view check > > - Reuse size variable > > > >Signed-off-by: Matthew Auld <matthew.auld@intel.com> > >--- > > drivers/gpu/drm/i915/i915_gem.c | 51 +++++++++++------------------------------ > > 1 file changed, 14 insertions(+), 37 deletions(-) > > > >diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > >index f45856d..e5d9d0b 100644 > >--- a/drivers/gpu/drm/i915/i915_gem.c > >+++ b/drivers/gpu/drm/i915/i915_gem.c > >@@ -3468,50 +3468,27 @@ i915_gem_object_bind_to_vm(struct drm_i915_gem_object *obj, > > u32 fence_alignment, unfenced_alignment; > > u32 search_flag, alloc_flag; > > u64 start, end; > >- u64 size, fence_size; > >+ u64 size; > > struct i915_vma *vma; > > int ret; > > > >- if (i915_is_ggtt(vm)) { > >- u32 view_size; > >- > >- if (WARN_ON(!ggtt_view)) > >- return ERR_PTR(-EINVAL); > >- > >- view_size = i915_ggtt_view_size(obj, ggtt_view); > >- > >- fence_size = i915_gem_get_gtt_size(dev, > >- view_size, > >- obj->tiling_mode); > >- fence_alignment = i915_gem_get_gtt_alignment(dev, > >- view_size, > >- obj->tiling_mode, > >- true); > >- unfenced_alignment = i915_gem_get_gtt_alignment(dev, > >- view_size, > >- obj->tiling_mode, > >- false); > >- size = flags & PIN_MAPPABLE ? fence_size : view_size; > >- } else { > >- fence_size = i915_gem_get_gtt_size(dev, > >- obj->base.size, > >- obj->tiling_mode); > >- fence_alignment = i915_gem_get_gtt_alignment(dev, > >- obj->base.size, > >- obj->tiling_mode, > >- true); > >- unfenced_alignment = > >- i915_gem_get_gtt_alignment(dev, > >- obj->base.size, > >- obj->tiling_mode, > >- false); > >- size = flags & PIN_MAPPABLE ? fence_size : obj->base.size; > >- } > >+ if (i915_is_ggtt(vm)) > >+ size = i915_ggtt_view_size(obj, ggtt_view); > >+ else > >+ size = obj->base.size; > >+ > >+ fence_alignment = i915_gem_get_gtt_alignment(dev, size, > >+ obj->tiling_mode, true); > >+ unfenced_alignment = i915_gem_get_gtt_alignment(dev, size, > >+ obj->tiling_mode, > >+ false); > > > > start = flags & PIN_OFFSET_BIAS ? flags & PIN_OFFSET_MASK : 0; > > end = vm->total; > >- if (flags & PIN_MAPPABLE) > >+ if (flags & PIN_MAPPABLE) { > > end = min_t(u64, end, dev_priv->gtt.mappable_end); > >+ size = i915_gem_get_gtt_size(dev, size, obj->tiling_mode); No. Keep the start, end computation separate. For example the above size needs only be done when inspecting the i915_is_ggtt(). If you simplified the alignement as well, it becomes clearer. If you reviewed the patches to handle vma, it would help. The key question is what igt did you run to verify the changes? -Chris
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index f45856d..e5d9d0b 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -3468,50 +3468,27 @@ i915_gem_object_bind_to_vm(struct drm_i915_gem_object *obj, u32 fence_alignment, unfenced_alignment; u32 search_flag, alloc_flag; u64 start, end; - u64 size, fence_size; + u64 size; struct i915_vma *vma; int ret; - if (i915_is_ggtt(vm)) { - u32 view_size; - - if (WARN_ON(!ggtt_view)) - return ERR_PTR(-EINVAL); - - view_size = i915_ggtt_view_size(obj, ggtt_view); - - fence_size = i915_gem_get_gtt_size(dev, - view_size, - obj->tiling_mode); - fence_alignment = i915_gem_get_gtt_alignment(dev, - view_size, - obj->tiling_mode, - true); - unfenced_alignment = i915_gem_get_gtt_alignment(dev, - view_size, - obj->tiling_mode, - false); - size = flags & PIN_MAPPABLE ? fence_size : view_size; - } else { - fence_size = i915_gem_get_gtt_size(dev, - obj->base.size, - obj->tiling_mode); - fence_alignment = i915_gem_get_gtt_alignment(dev, - obj->base.size, - obj->tiling_mode, - true); - unfenced_alignment = - i915_gem_get_gtt_alignment(dev, - obj->base.size, - obj->tiling_mode, - false); - size = flags & PIN_MAPPABLE ? fence_size : obj->base.size; - } + if (i915_is_ggtt(vm)) + size = i915_ggtt_view_size(obj, ggtt_view); + else + size = obj->base.size; + + fence_alignment = i915_gem_get_gtt_alignment(dev, size, + obj->tiling_mode, true); + unfenced_alignment = i915_gem_get_gtt_alignment(dev, size, + obj->tiling_mode, + false); start = flags & PIN_OFFSET_BIAS ? flags & PIN_OFFSET_MASK : 0; end = vm->total; - if (flags & PIN_MAPPABLE) + if (flags & PIN_MAPPABLE) { end = min_t(u64, end, dev_priv->gtt.mappable_end); + size = i915_gem_get_gtt_size(dev, size, obj->tiling_mode); + } if (flags & PIN_ZONE_4G) end = min_t(u64, end, (1ULL << 32) - PAGE_SIZE);
No functional change, just makes the code easier to follow. v2: - Remove local fence_size variable (Tvrtko Ursulin) - Remove redundant NULL ggtt_view check - Reuse size variable Signed-off-by: Matthew Auld <matthew.auld@intel.com> --- drivers/gpu/drm/i915/i915_gem.c | 51 +++++++++++------------------------------ 1 file changed, 14 insertions(+), 37 deletions(-)