| Message ID | 1458148026-3057-1-git-send-email-matthew.auld@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, On 16/03/16 17:07, Matthew Auld wrote: > No functional change, just makes the code easier to follow. > > Signed-off-by: Matthew Auld <matthew.auld@intel.com> > --- > drivers/gpu/drm/i915/i915_gem.c | 41 +++++++++++------------------------------ > 1 file changed, 11 insertions(+), 30 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index b854af2..5a8d69d 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -3470,46 +3470,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, obj_size, fence_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; > + obj_size = i915_ggtt_view_size(obj, ggtt_view); > } 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; > + obj_size = obj->base.size; > } PIN_MAPPABLE mandates PIN_GLOBAL, and PIN_GLOBAL mandates GGTT (see i915_gem_object_do_pin), so I think the cleanup should be just not to do any of the fence business on if !ggtt branch. if (i915_is_ggtt(vm)) { ... existing code ... } else { size = obj->base.size; } Check for NULL ggtt_view is also done on the higher level so perhaps that can go as well to consolidate the checks at one place. > + fence_size = i915_gem_get_gtt_size(dev, obj_size, obj->tiling_mode); > + fence_alignment = i915_gem_get_gtt_alignment(dev, obj_size, > + obj->tiling_mode, true); > + unfenced_alignment = i915_gem_get_gtt_alignment(dev, obj_size, > + obj->tiling_mode, > + false); > + size = flags & PIN_MAPPABLE ? fence_size : obj_size; > + > start = flags & PIN_OFFSET_BIAS ? flags & PIN_OFFSET_MASK : 0; > end = vm->total; > if (flags & PIN_MAPPABLE) > Regards, Tvrtko
Hi,
If we don't do any of the fence business for !i915_is_gtt, then will
this not change the following code:
if (alignment == 0)
alignment = flags & PIN_MAPPABLE ? fence_alignment : unfenced_alignment;
Or am I missing something?
Regards,
Matt
On Thu, Mar 17, 2016 at 01:41:32PM +0000, Matthew Auld wrote: > Hi, > > If we don't do any of the fence business for !i915_is_gtt, then will > this not change the following code: > > if (alignment == 0) > alignment = flags & PIN_MAPPABLE ? fence_alignment : unfenced_alignment; > > Or am I missing something? Fwiw, the patches I have sent previously change it to: size = max(size, vma->size); if (flags & PIN_MAPPABLE) size = i915_gem_get_gtt_size(dev, size, obj->tiling_mode); alignment = max_t(u64, max(alignment, vma->display_alignment), i915_gem_get_gtt_alignment(dev, size, obj->tiling_mode, flags & PIN_MAPPABLE)); if (alignment == 4096) alignment = 0; start = flags & PIN_OFFSET_BIAS ? flags & PIN_OFFSET_MASK : 0; end = vma->vm->total; if (flags & PIN_MAPPABLE) end = min_t(u64, end, dev_priv->gtt.mappable_end); if (flags & PIN_ZONE_4G) end = min_t(u64, end, (1ULL << 32) - PAGE_SIZE); -Chris
On 17/03/16 13:41, Matthew Auld wrote: > Hi, > > If we don't do any of the fence business for !i915_is_gtt, then will > this not change the following code: > > if (alignment == 0) > alignment = flags & PIN_MAPPABLE ? fence_alignment : unfenced_alignment; > > Or am I missing something? No I missed that bit. In which case your cleanup looks OK to me in principle. I would just not use obj_size since it is not that. Perhaps just reuse the existing size variable to hold the intermediate result? Regards, Tvrtko
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index b854af2..5a8d69d 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -3470,46 +3470,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, obj_size, fence_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; + obj_size = i915_ggtt_view_size(obj, ggtt_view); } 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; + obj_size = obj->base.size; } + fence_size = i915_gem_get_gtt_size(dev, obj_size, obj->tiling_mode); + fence_alignment = i915_gem_get_gtt_alignment(dev, obj_size, + obj->tiling_mode, true); + unfenced_alignment = i915_gem_get_gtt_alignment(dev, obj_size, + obj->tiling_mode, + false); + size = flags & PIN_MAPPABLE ? fence_size : obj_size; + start = flags & PIN_OFFSET_BIAS ? flags & PIN_OFFSET_MASK : 0; end = vm->total; if (flags & PIN_MAPPABLE)
No functional change, just makes the code easier to follow. Signed-off-by: Matthew Auld <matthew.auld@intel.com> --- drivers/gpu/drm/i915/i915_gem.c | 41 +++++++++++------------------------------ 1 file changed, 11 insertions(+), 30 deletions(-)