Re: [PATCH 26/33] drm/i915: Track pinned VMA

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux