On Fri, Jul 29, 2016 at 09:53:11AM +0300, Joonas Lahtinen wrote:
> On ke, 2016-07-27 at 12:14 +0100, Chris Wilson wrote:
> > -uint32_t
> > -i915_gem_get_gtt_size(struct drm_device *dev, uint32_t size, int tiling_mode);
> > -uint32_t
> > -i915_gem_get_gtt_alignment(struct drm_device *dev, uint32_t size,
> > - int tiling_mode, bool fenced);
> > +uint64_t
>
> u64 for consistency with code elsewhere. Applies to all the type
> changes.
>
> > start = flags & PIN_OFFSET_BIAS ? flags & PIN_OFFSET_MASK : 0;
> > - end = vm->total;
> > +
> > + end = vma->vm->total;
>
> While touching, I might change the end to vm_end or so...
I wouldn't. It's not derived from the address space but our request.
> > if (flags & PIN_MAPPABLE)
> > end = min_t(u64, end, dev_priv->ggtt.mappable_end);
> > if (flags & PIN_ZONE_4G)
> > @@ -3030,8 +3018,7 @@ i915_gem_object_insert_into_vm(struct drm_i915_gem_object *obj,
> > * attempt to find space.
> > */
> > if (size > end) {
> > - DRM_DEBUG("Attempting to bind an object (view type=%u) larger than the aperture: request=%llu [object=%zd] > %s aperture=%llu\n",
> > - ggtt_view ? ggtt_view->type : 0,
> > + DRM_DEBUG("Attempting to bind an object larger than the aperture: request=%llu [object=%zd] > %s aperture=%llu\n",
>
> No view type no more?
There will be no view type here anymore. It is less important than the
request flags, but this is a user debug message ideally the information
presented here would closely relate to the user entry point.
> > vma->node.start = offset;
> > vma->node.size = size;
> > vma->node.color = obj->cache_level;
> > - ret = drm_mm_reserve_node(&vm->mm, &vma->node);
> > + ret = drm_mm_reserve_node(&vma->vm->mm, &vma->node);
>
> Not sure if dropping the vm alias makes things look any better, unless
> you intend to create i915_vma_reserve_mem() or so?
We do. I'm not fond of having unnecessary offscreen locals, and here we
can see clearly how the mm relates to the vma which makes it easier to
compare this callsite to similar code.
> > @@ -3077,37 +3060,39 @@ i915_gem_object_insert_into_vm(struct drm_i915_gem_object *obj,
> > alloc_flag = DRM_MM_CREATE_DEFAULT;
> > }
> >
> > + if (alignment <= 4096)
> > + alignment = 0; /* for efficient drm_mm searching */
> > +
>
> This is obviously not related and should be mentioned in the commit message or split.
I had wondered where that had buried itself. It is self-evident, right?
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
