On Thu, Aug 11, 2016 at 03:18:44PM +0300, Joonas Lahtinen wrote:
> On su, 2016-08-07 at 15:45 +0100, Chris Wilson wrote:
> > @@ -1616,7 +1618,7 @@ i915_gem_mmap_ioctl(struct drm_device *dev, void
> > *data,
> >
> > /**
> > * i915_gem_fault - fault a page into the GTT
> > - * @vma: VMA in question
> > + * @mm: VMA in question
>
> should be @vm or whatever the correct name.
>
> > * @vmf: fault info
> > *
> > * The fault handler is set up by drm_gem_mmap() when a object is GTT
> > mapped
> > @@ -1630,20 +1632,21 @@ i915_gem_mmap_ioctl(struct drm_device *dev, void
> > *data,
> > * suffer if the GTT working set is large or there are few fence registers
> > * left.
> > */
> > -int i915_gem_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
> > +int i915_gem_fault(struct vm_area_struct *vm, struct vm_fault *vmf)
>
> 'vm' is used elsewhere for the address space, maybe 'kvma'? Or 'area'
> (used in linux/mm.h too occasionally)
I also was tempted by kvma. But area is better.
>
> > @@ -1722,13 +1726,13 @@ int i915_gem_fault(struct vm_area_struct *vma,
> > struct vm_fault *vmf)
> > } else {
> > if (!obj->fault_mappable) {
> > unsigned long size = min_t(unsigned long,
> > - vma->vm_end - vma->vm_start,
> > + vm->vm_end - vm->vm_start,
> > obj->base.size);
> > int i;
> >
> > for (i = 0; i < size >> PAGE_SHIFT; i++) {
> > - ret = vm_insert_pfn(vma,
> > - (unsigned
> > long)vma->vm_start + i * PAGE_SIZE,
> > + ret = vm_insert_pfn(vm,
> > + (unsigned long)vm->vm_start
> > + i * PAGE_SIZE,
>
> Hmm, vm->vm_start is already unsigned long, so cast could be
> eliminated.
>
> > pfn + i);
> > if (ret)
> > break;
> > @@ -1736,12 +1740,12 @@ int i915_gem_fault(struct vm_area_struct *vma,
> > struct vm_fault *vmf)
> >
> > obj->fault_mappable = true;
> > } else
> > - ret = vm_insert_pfn(vma,
> > + ret = vm_insert_pfn(vm,
> > (unsigned long)vmf->virtual_address,
> > pfn + page_offset);
> > }
> > err_unpin:
> > - i915_gem_object_ggtt_unpin_view(obj, &view);
> > + __i915_vma_unpin(vma);
> > err_unlock:
> > mutex_unlock(&dev->struct_mutex);
> > err_rpm:
> > @@ -3190,7 +3194,7 @@ i915_gem_object_set_to_gtt_domain(struct
> > drm_i915_gem_object *obj, bool write)
> > old_write_domain);
> >
> > /* And bump the LRU for this access */
> > - vma = i915_gem_obj_to_ggtt(obj);
> > + vma = i915_gem_object_to_ggtt(obj, NULL);
> > if (vma &&
> > drm_mm_node_allocated(&vma->node) &&
> > !i915_vma_is_active(vma))
> > @@ -3414,11 +3418,12 @@ rpm_put:
> > * Can be called from an uninterruptible phase (modesetting) and allows
> > * any flushes to be pipelined (for pageflips).
> > */
> > -int
> > +struct i915_vma *
> > i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
> > u32 alignment,
> > const struct i915_ggtt_view *view)
> > {
> > + struct i915_vma *vma;
> > u32 old_read_domains, old_write_domain;
> > int ret;
> >
> > @@ -3438,19 +3443,23 @@ i915_gem_object_pin_to_display_plane(struct
> > drm_i915_gem_object *obj,
> > */
> > ret = i915_gem_object_set_cache_level(obj,
> > HAS_WT(obj->base.dev) ?
> > I915_CACHE_WT : I915_CACHE_NONE);
> > - if (ret)
> > + if (ret) {
> > + vma = ERR_PTR(ret);
> > goto err_unpin_display;
> > + }
> >
> > /* As the user may map the buffer once pinned in the display plane
> > * (e.g. libkms for the bootup splash), we have to ensure that we
> > * always use map_and_fenceable for all scanout buffers.
> > */
> > - ret = i915_gem_object_ggtt_pin(obj, view, 0, alignment,
> > + vma = i915_gem_object_ggtt_pin(obj, view, 0, alignment,
> > view->type == I915_GGTT_VIEW_NORMAL ?
> > PIN_MAPPABLE : 0);
> > - if (ret)
> > + if (IS_ERR(vma))
> > goto err_unpin_display;
> >
> > + WARN_ON(obj->pin_display > i915_vma_pin_count(vma));
> > +
> > i915_gem_object_flush_cpu_write_domain(obj);
> >
> > old_write_domain = obj->base.write_domain;
> > @@ -3466,23 +3475,23 @@ i915_gem_object_pin_to_display_plane(struct
> > drm_i915_gem_object *obj,
> > old_read_domains,
> > old_write_domain);
> >
> > - return 0;
> > + return vma;
> >
> > err_unpin_display:
> > obj->pin_display--;
> > - return ret;
> > + return vma;
> > }
> >
> > void
> > -i915_gem_object_unpin_from_display_plane(struct drm_i915_gem_object *obj,
> > - const struct i915_ggtt_view *view)
> > +i915_gem_object_unpin_from_display_plane(struct i915_vma *vma)
> > {
> > - if (WARN_ON(obj->pin_display == 0))
> > + if (WARN_ON(vma->obj->pin_display == 0))
> > return;
> >
> > - i915_gem_object_ggtt_unpin_view(obj, view);
> > + vma->obj->pin_display--;
> >
> > - obj->pin_display--;
> > + i915_vma_unpin(vma);
> > + WARN_ON(vma->obj->pin_display > i915_vma_pin_count(vma));
> > }
> >
> > /**
> > @@ -3679,27 +3688,25 @@ err:
> > return ret;
> > }
> >
> > -int
> > +struct i915_vma *
> > i915_gem_object_ggtt_pin(struct drm_i915_gem_object *obj,
> > - const struct i915_ggtt_view *view,
> > + const struct i915_ggtt_view *ggtt_view,
>
> Hmm, why distinctive name compared to other places? This function has
> _ggtt_ in the name, so should be implicit.
>
> > u64 size,
> > u64 alignment,
> > u64 flags)
> > {
>
> <SNIP>
>
> > -/* All the new VM stuff */
>
> Oh noes, we destroy all the new stuff :P
>
> > @@ -349,30 +349,34 @@ relocate_entry_gtt(struct drm_i915_gem_object *obj,
> > struct drm_i915_gem_relocation_entry *reloc,
> > uint64_t target_offset)
> > {
> > - struct drm_device *dev = obj->base.dev;
> > - struct drm_i915_private *dev_priv = to_i915(dev);
> > + struct drm_i915_private *dev_priv = to_i915(obj->base.dev);
> > struct i915_ggtt *ggtt = &dev_priv->ggtt;
> > + struct i915_vma *vma;
> > uint64_t delta = relocation_target(reloc, target_offset);
> > uint64_t offset;
> > void __iomem *reloc_page;
> > int ret;
> >
> > + vma = i915_gem_object_ggtt_pin(obj, NULL, 0, 0, PIN_MAPPABLE);
> > + if (IS_ERR(vma))
> > + return PTR_ERR(vma);
> > +
> > ret = i915_gem_object_set_to_gtt_domain(obj, true);
> > if (ret)
> > - return ret;
> > + goto unpin;
> >
> > ret = i915_gem_object_put_fence(obj);
> > if (ret)
> > - return ret;
> > + goto unpin;
> >
> > /* Map the page containing the relocation we're going to perform. */
> > - offset = i915_gem_obj_ggtt_offset(obj);
> > + offset = vma->node.start;
> > offset += reloc->offset;
>
> Could concatenate to previous line now;
>
> offset = vma->node.start + reloc->offset;
>
> > -static struct i915_vma*
> > +static struct i915_vma *
> > i915_gem_execbuffer_parse(struct intel_engine_cs *engine,
> > struct drm_i915_gem_exec_object2 *shadow_exec_entry,
> > struct drm_i915_gem_object *batch_obj,
> > @@ -1305,31 +1311,30 @@ i915_gem_execbuffer_parse(struct intel_engine_cs
> > *engine,
> > batch_start_offset,
> > batch_len,
> > is_master);
> > - if (ret)
> > + if (ret) {
> > + if (ret == -EACCES) /* unhandled chained batch */
> > + vma = NULL;
> > + else
> > + vma = ERR_PTR(ret);
> > goto err;
> > + }
> >
> > - ret = i915_gem_object_ggtt_pin(shadow_batch_obj, NULL, 0, 0, 0);
> > - if (ret)
> > + vma = i915_gem_object_ggtt_pin(shadow_batch_obj, NULL, 0, 0, 0);
> > + if (IS_ERR(vma)) {
> > + ret = PTR_ERR(vma);
>
> Hmm, 'err' label no longer cares about ret, so this is redundant. Or
> should the ret-to-vma translation been kept at the end?
>
> > goto err;
> > -
> > - i915_gem_object_unpin_pages(shadow_batch_obj);
> > + }
> >
> > memset(shadow_exec_entry, 0, sizeof(*shadow_exec_entry));
> >
> > - vma = i915_gem_obj_to_ggtt(shadow_batch_obj);
> > vma->exec_entry = shadow_exec_entry;
> > vma->exec_entry->flags = __EXEC_OBJECT_HAS_PIN;
> > i915_gem_object_get(shadow_batch_obj);
> > list_add_tail(&vma->exec_list, &eb->vmas);
> >
> > - return vma;
> > -
> > err:
> > i915_gem_object_unpin_pages(shadow_batch_obj);
> > - if (ret == -EACCES) /* unhandled chained batch */
> > - return NULL;
> > - else
> > - return ERR_PTR(ret);
> > + return vma;
> > }
> >
>
> <SNIP>
>
> > @@ -432,13 +432,7 @@ bool
> > i915_gem_object_pin_fence(struct drm_i915_gem_object *obj)
> > {
> > if (obj->fence_reg != I915_FENCE_REG_NONE) {
> > - struct drm_i915_private *dev_priv = to_i915(obj->base.dev);
> > - struct i915_vma *ggtt_vma = i915_gem_obj_to_ggtt(obj);
> > -
> > - WARN_ON(!ggtt_vma ||
> > - dev_priv->fence_regs[obj->fence_reg].pin_count >
> > - i915_vma_pin_count(ggtt_vma));
>
> Is this WARN_ON deliberately removed, not worthy GEM_BUG_ON?
The pin_count check is not strictly true for all futures, and the
ggtt_vma can just explode if NULL until it too is replaced in a few
patches time.
> > - dev_priv->fence_regs[obj->fence_reg].pin_count++;
> > + to_i915(obj->base.dev)->fence_regs[obj->fence_reg].pin_count++;
>
> This is not the most readable line of code one can imagine. dev_priv
> alias does make readability better occasionally.
This just makes another patch smaller. I've not qualms about this line
;)
> > @@ -3351,14 +3351,10 @@ __i915_gem_vma_create(struct drm_i915_gem_object
> > *obj,
> >
> > GEM_BUG_ON(vm->closed);
> >
> > - if (WARN_ON(i915_is_ggtt(vm) != !!view))
> > - return ERR_PTR(-EINVAL);
> > -
> > vma = kmem_cache_zalloc(to_i915(obj->base.dev)->vmas, GFP_KERNEL);
> > if (vma == NULL)
> > return ERR_PTR(-ENOMEM);
> >
> > - INIT_LIST_HEAD(&vma->obj_link);
>
> This disappears completely?
It was never required.
> > @@ -3378,56 +3373,76 @@ __i915_gem_vma_create(struct drm_i915_gem_object
> > *obj,
> > +static inline bool vma_matches(struct i915_vma *vma,
> > + struct i915_address_space *vm,
> > + const struct i915_ggtt_view *view)
>
> Function name is not clearest. But I can not suggest a better one off
> the top of my head.
>
> > static struct drm_i915_error_object *
> > i915_error_object_create(struct drm_i915_private *dev_priv,
> > - struct drm_i915_gem_object *src,
> > - struct i915_address_space *vm)
> > + struct i915_vma *vma)
> > {
> > struct i915_ggtt *ggtt = &dev_priv->ggtt;
> > + struct drm_i915_gem_object *src;
> > struct drm_i915_error_object *dst;
> > - struct i915_vma *vma = NULL;
> > int num_pages;
> > bool use_ggtt;
> > int i = 0;
> > u64 reloc_offset;
> >
> > - if (src == NULL || src->pages == NULL)
> > + if (!vma)
> > + return NULL;
> > +
> > + src = vma->obj;
> > + if (!src->pages)
> > return NULL;
> >
> > num_pages = src->base.size >> PAGE_SHIFT;
> >
> > dst = kmalloc(sizeof(*dst) + num_pages * sizeof(u32 *), GFP_ATOMIC);
> > - if (dst == NULL)
> > + if (!dst)
> > return NULL;
> >
> > - if (i915_gem_obj_bound(src, vm))
> > - dst->gtt_offset = i915_gem_obj_offset(src, vm);
> > - else
> > - dst->gtt_offset = -1;
>
> What was the purpose of this line previously, the calculations would
> get majestically messed up?
No purpose. It would have taken quite a bit of abuse to be able to
trigger it, and certainly nothing relevant to the hang.
> > @@ -1035,11 +1029,8 @@ static void i915_gem_record_rings(struct
> > drm_i915_private *dev_priv,
> > struct drm_i915_gem_request *request;
> > int i, count;
> >
> > - if (dev_priv->semaphore) {
> > - error->semaphore =
> > - i915_error_ggtt_object_create(dev_priv,
> > - dev_priv->semaphore->obj);
> > - }
> > + error->semaphore =
> > + i915_error_object_create(dev_priv, dev_priv->semaphore);
>
> Not sure if I like hiding the NULL tolerancy inside the function.
Otoh, it makes it a lot cleaner.
> > @@ -2291,7 +2293,8 @@ void intel_unpin_fb_obj(struct drm_framebuffer *fb,
> > unsigned int rotation)
> > if (view.type == I915_GGTT_VIEW_NORMAL)
> > i915_gem_object_unpin_fence(obj);
> >
> > - i915_gem_object_unpin_from_display_plane(obj, &view);
> > + vma = i915_gem_object_to_ggtt(obj, &view);
>
> GEM_BUG_ON(!vma)?
The goal and original patch passed in the vma to free. I gave up
tracking that patch through the ongoing atomic transition.
> This might be the only acceptable use of if () in teardown path.
>
> I hope there is an excellent revision listing in the next iteration.
> This was a pain to go through.
vN: Address some of Joonas's stylistic nitpicks.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
