Re: [Intel-gfx] [PATCH 1/5] drm/i915: Revert "drm/i915/gem: Asynchronous cmdparser"

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

 



On Wed, Jun 02, 2021 at 11:41:45AM -0500, Jason Ekstrand wrote:
> This reverts 686c7c35abc2 ("drm/i915/gem: Asynchronous cmdparser").  The
> justification for this commit in the git history was a vague comment
> about getting it out from under the struct_mutex.  While this may
> improve perf for some workloads on Gen7 platforms where we rely on the
> command parser for features such as indirect rendering, no numbers were
> provided to prove such an improvement.  It claims to closed two
> gitlab/bugzilla issues but with no explanation whatsoever as to why or
> what bug it's fixing.
> 
> Meanwhile, by moving command parsing off to an async callback, it leaves
> us with a problem of what to do on error.  When things were synchronous,
> EXECBUFFER2 would fail with an error code if parsing failed.  When
> moving it to async, we needed another way to handle that error and the
> solution employed was to set an error on the dma_fence and then trust
> that said error gets propagated to the client eventually.  Moving back
> to synchronous will help us untangle the fence error propagation mess.
> 
> This also reverts most of 0edbb9ba1bfe ("drm/i915: Move cmd parser
> pinning to execbuffer") which is a refactor of some of our allocation
> paths for asynchronous parsing.  Now that everything is synchronous, we
> don't need it.
> 
> Signed-off-by: Jason Ekstrand <jason@xxxxxxxxxxxxxx>
> Cc: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
> Reviewed-by: Jon Bloomfield <jon.bloomfield@xxxxxxxxx>

This needs the same Cc: stable and Fixes: lines as the dma_fence error
propagation revert. Otherwise the cmd parser breaks, which isn't great.

> ---
>  .../gpu/drm/i915/gem/i915_gem_execbuffer.c    | 227 +-----------------
>  .../i915/gem/selftests/i915_gem_execbuffer.c  |   4 +
>  drivers/gpu/drm/i915/i915_cmd_parser.c        | 132 +++++-----
>  drivers/gpu/drm/i915/i915_drv.h               |   7 +-
>  4 files changed, 91 insertions(+), 279 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> index 297143511f99b..a49da4b24d4d4 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> @@ -25,10 +25,8 @@
>  #include "i915_gem_clflush.h"
>  #include "i915_gem_context.h"
>  #include "i915_gem_ioctls.h"
> -#include "i915_sw_fence_work.h"
>  #include "i915_trace.h"
>  #include "i915_user_extensions.h"
> -#include "i915_memcpy.h"
>  
>  struct eb_vma {
>  	struct i915_vma *vma;
> @@ -1456,6 +1454,10 @@ static u32 *reloc_gpu(struct i915_execbuffer *eb,
>  		int err;
>  		struct intel_engine_cs *engine = eb->engine;
>  
> +		/* If we need to copy for the cmdparser, we will stall anyway */
> +		if (eb_use_cmdparser(eb))
> +			return ERR_PTR(-EWOULDBLOCK);
> +
>  		if (!reloc_can_use_engine(engine)) {
>  			engine = engine->gt->engine_class[COPY_ENGINE_CLASS][0];
>  			if (!engine)
> @@ -2372,217 +2374,6 @@ shadow_batch_pin(struct i915_execbuffer *eb,
>  	return vma;
>  }
>  
> -struct eb_parse_work {
> -	struct dma_fence_work base;
> -	struct intel_engine_cs *engine;
> -	struct i915_vma *batch;
> -	struct i915_vma *shadow;
> -	struct i915_vma *trampoline;
> -	unsigned long batch_offset;
> -	unsigned long batch_length;
> -	unsigned long *jump_whitelist;
> -	const void *batch_map;
> -	void *shadow_map;
> -};
> -
> -static int __eb_parse(struct dma_fence_work *work)
> -{
> -	struct eb_parse_work *pw = container_of(work, typeof(*pw), base);
> -	int ret;
> -	bool cookie;
> -
> -	cookie = dma_fence_begin_signalling();
> -	ret = intel_engine_cmd_parser(pw->engine,
> -				      pw->batch,
> -				      pw->batch_offset,
> -				      pw->batch_length,
> -				      pw->shadow,
> -				      pw->jump_whitelist,
> -				      pw->shadow_map,
> -				      pw->batch_map);
> -	dma_fence_end_signalling(cookie);
> -
> -	return ret;
> -}
> -
> -static void __eb_parse_release(struct dma_fence_work *work)
> -{
> -	struct eb_parse_work *pw = container_of(work, typeof(*pw), base);
> -
> -	if (!IS_ERR_OR_NULL(pw->jump_whitelist))
> -		kfree(pw->jump_whitelist);
> -
> -	if (pw->batch_map)
> -		i915_gem_object_unpin_map(pw->batch->obj);
> -	else
> -		i915_gem_object_unpin_pages(pw->batch->obj);
> -
> -	i915_gem_object_unpin_map(pw->shadow->obj);
> -
> -	if (pw->trampoline)
> -		i915_active_release(&pw->trampoline->active);
> -	i915_active_release(&pw->shadow->active);
> -	i915_active_release(&pw->batch->active);
> -}
> -
> -static const struct dma_fence_work_ops eb_parse_ops = {
> -	.name = "eb_parse",
> -	.work = __eb_parse,
> -	.release = __eb_parse_release,
> -};
> -
> -static inline int
> -__parser_mark_active(struct i915_vma *vma,
> -		     struct intel_timeline *tl,
> -		     struct dma_fence *fence)
> -{
> -	struct intel_gt_buffer_pool_node *node = vma->private;
> -
> -	return i915_active_ref(&node->active, tl->fence_context, fence);
> -}
> -
> -static int
> -parser_mark_active(struct eb_parse_work *pw, struct intel_timeline *tl)
> -{
> -	int err;
> -
> -	mutex_lock(&tl->mutex);
> -
> -	err = __parser_mark_active(pw->shadow, tl, &pw->base.dma);
> -	if (err)
> -		goto unlock;
> -
> -	if (pw->trampoline) {
> -		err = __parser_mark_active(pw->trampoline, tl, &pw->base.dma);
> -		if (err)
> -			goto unlock;
> -	}
> -
> -unlock:
> -	mutex_unlock(&tl->mutex);
> -	return err;
> -}
> -
> -static int eb_parse_pipeline(struct i915_execbuffer *eb,
> -			     struct i915_vma *shadow,
> -			     struct i915_vma *trampoline)
> -{
> -	struct eb_parse_work *pw;
> -	struct drm_i915_gem_object *batch = eb->batch->vma->obj;
> -	bool needs_clflush;
> -	int err;
> -
> -	GEM_BUG_ON(overflows_type(eb->batch_start_offset, pw->batch_offset));
> -	GEM_BUG_ON(overflows_type(eb->batch_len, pw->batch_length));
> -
> -	pw = kzalloc(sizeof(*pw), GFP_KERNEL);
> -	if (!pw)
> -		return -ENOMEM;
> -
> -	err = i915_active_acquire(&eb->batch->vma->active);
> -	if (err)
> -		goto err_free;
> -
> -	err = i915_active_acquire(&shadow->active);
> -	if (err)
> -		goto err_batch;
> -
> -	if (trampoline) {
> -		err = i915_active_acquire(&trampoline->active);
> -		if (err)
> -			goto err_shadow;
> -	}
> -
> -	pw->shadow_map = i915_gem_object_pin_map(shadow->obj, I915_MAP_WB);
> -	if (IS_ERR(pw->shadow_map)) {
> -		err = PTR_ERR(pw->shadow_map);
> -		goto err_trampoline;
> -	}
> -
> -	needs_clflush =
> -		!(batch->cache_coherent & I915_BO_CACHE_COHERENT_FOR_READ);
> -
> -	pw->batch_map = ERR_PTR(-ENODEV);
> -	if (needs_clflush && i915_has_memcpy_from_wc())
> -		pw->batch_map = i915_gem_object_pin_map(batch, I915_MAP_WC);
> -
> -	if (IS_ERR(pw->batch_map)) {
> -		err = i915_gem_object_pin_pages(batch);
> -		if (err)
> -			goto err_unmap_shadow;
> -		pw->batch_map = NULL;
> -	}
> -
> -	pw->jump_whitelist =
> -		intel_engine_cmd_parser_alloc_jump_whitelist(eb->batch_len,
> -							     trampoline);
> -	if (IS_ERR(pw->jump_whitelist)) {
> -		err = PTR_ERR(pw->jump_whitelist);
> -		goto err_unmap_batch;
> -	}
> -
> -	dma_fence_work_init(&pw->base, &eb_parse_ops);
> -
> -	pw->engine = eb->engine;
> -	pw->batch = eb->batch->vma;
> -	pw->batch_offset = eb->batch_start_offset;
> -	pw->batch_length = eb->batch_len;
> -	pw->shadow = shadow;
> -	pw->trampoline = trampoline;
> -
> -	/* Mark active refs early for this worker, in case we get interrupted */
> -	err = parser_mark_active(pw, eb->context->timeline);
> -	if (err)
> -		goto err_commit;
> -
> -	err = dma_resv_reserve_shared(pw->batch->resv, 1);
> -	if (err)
> -		goto err_commit;
> -
> -	err = dma_resv_reserve_shared(shadow->resv, 1);
> -	if (err)
> -		goto err_commit;
> -
> -	/* Wait for all writes (and relocs) into the batch to complete */
> -	err = i915_sw_fence_await_reservation(&pw->base.chain,
> -					      pw->batch->resv, NULL, false,
> -					      0, I915_FENCE_GFP);
> -	if (err < 0)
> -		goto err_commit;
> -
> -	/* Keep the batch alive and unwritten as we parse */
> -	dma_resv_add_shared_fence(pw->batch->resv, &pw->base.dma);
> -
> -	/* Force execution to wait for completion of the parser */
> -	dma_resv_add_excl_fence(shadow->resv, &pw->base.dma);
> -
> -	dma_fence_work_commit_imm(&pw->base);
> -	return 0;
> -
> -err_commit:
> -	i915_sw_fence_set_error_once(&pw->base.chain, err);
> -	dma_fence_work_commit_imm(&pw->base);
> -	return err;
> -
> -err_unmap_batch:
> -	if (pw->batch_map)
> -		i915_gem_object_unpin_map(batch);
> -	else
> -		i915_gem_object_unpin_pages(batch);
> -err_unmap_shadow:
> -	i915_gem_object_unpin_map(shadow->obj);
> -err_trampoline:
> -	if (trampoline)
> -		i915_active_release(&trampoline->active);
> -err_shadow:
> -	i915_active_release(&shadow->active);
> -err_batch:
> -	i915_active_release(&eb->batch->vma->active);
> -err_free:
> -	kfree(pw);
> -	return err;
> -}
> -
>  static struct i915_vma *eb_dispatch_secure(struct i915_execbuffer *eb, struct i915_vma *vma)
>  {
>  	/*
> @@ -2672,7 +2463,15 @@ static int eb_parse(struct i915_execbuffer *eb)
>  		goto err_trampoline;
>  	}
>  
> -	err = eb_parse_pipeline(eb, shadow, trampoline);
> +	err = dma_resv_reserve_shared(shadow->resv, 1);
> +	if (err)
> +		goto err_trampoline;
> +
> +	err = intel_engine_cmd_parser(eb->engine,
> +				      eb->batch->vma,
> +				      eb->batch_start_offset,
> +				      eb->batch_len,
> +				      shadow, trampoline);
>  	if (err)
>  		goto err_unpin_batch;
>  
> diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/selftests/i915_gem_execbuffer.c
> index 4df505e4c53ae..16162fc2782dc 100644
> --- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_execbuffer.c
> @@ -125,6 +125,10 @@ static int igt_gpu_reloc(void *arg)
>  	intel_gt_pm_get(&eb.i915->gt);
>  
>  	for_each_uabi_engine(eb.engine, eb.i915) {
> +		if (intel_engine_requires_cmd_parser(eb.engine) ||
> +		    intel_engine_using_cmd_parser(eb.engine))
> +			continue;
> +
>  		reloc_cache_init(&eb.reloc_cache, eb.i915);
>  		memset(map, POISON_INUSE, 4096);
>  
> diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c b/drivers/gpu/drm/i915/i915_cmd_parser.c
> index 5b4b2bd46e7c8..056a233f443b4 100644
> --- a/drivers/gpu/drm/i915/i915_cmd_parser.c
> +++ b/drivers/gpu/drm/i915/i915_cmd_parser.c
> @@ -1145,19 +1145,41 @@ find_reg(const struct intel_engine_cs *engine, u32 addr)
>  static u32 *copy_batch(struct drm_i915_gem_object *dst_obj,
>  		       struct drm_i915_gem_object *src_obj,
>  		       unsigned long offset, unsigned long length,
> -		       void *dst, const void *src)
> +		       bool *needs_clflush_after)
>  {
> -	bool needs_clflush =
> -		!(src_obj->cache_coherent & I915_BO_CACHE_COHERENT_FOR_READ);
> -
> -	if (src) {
> -		GEM_BUG_ON(!needs_clflush);
> -		i915_unaligned_memcpy_from_wc(dst, src + offset, length);
> -	} else {
> -		struct scatterlist *sg;
> +	unsigned int src_needs_clflush;
> +	unsigned int dst_needs_clflush;
> +	void *dst, *src;
> +	int ret;
> +
> +	ret = i915_gem_object_prepare_write(dst_obj, &dst_needs_clflush);
> +	if (ret)
> +		return ERR_PTR(ret);
> +
> +	dst = i915_gem_object_pin_map(dst_obj, I915_MAP_WB);
> +	i915_gem_object_finish_access(dst_obj);
> +	if (IS_ERR(dst))
> +		return dst;
> +
> +	ret = i915_gem_object_prepare_read(src_obj, &src_needs_clflush);
> +	if (ret) {
> +		i915_gem_object_unpin_map(dst_obj);
> +		return ERR_PTR(ret);
> +	}
> +
> +	src = ERR_PTR(-ENODEV);
> +	if (src_needs_clflush && i915_has_memcpy_from_wc()) {
> +		src = i915_gem_object_pin_map(src_obj, I915_MAP_WC);
> +		if (!IS_ERR(src)) {
> +			i915_unaligned_memcpy_from_wc(dst,
> +						      src + offset,
> +						      length);
> +			i915_gem_object_unpin_map(src_obj);
> +		}
> +	}
> +	if (IS_ERR(src)) {
> +		unsigned long x, n, remain;
>  		void *ptr;
> -		unsigned int x, sg_ofs;
> -		unsigned long remain;
>  
>  		/*
>  		 * We can avoid clflushing partial cachelines before the write
> @@ -1168,40 +1190,34 @@ static u32 *copy_batch(struct drm_i915_gem_object *dst_obj,
>  		 * validate up to the end of the batch.
>  		 */
>  		remain = length;
> -		if (!(dst_obj->cache_coherent & I915_BO_CACHE_COHERENT_FOR_READ))
> +		if (dst_needs_clflush & CLFLUSH_BEFORE)
>  			remain = round_up(remain,
>  					  boot_cpu_data.x86_clflush_size);
>  
>  		ptr = dst;
>  		x = offset_in_page(offset);
> -		sg = i915_gem_object_get_sg(src_obj, offset >> PAGE_SHIFT, &sg_ofs, false);
> -
> -		while (remain) {
> -			unsigned long sg_max = sg->length >> PAGE_SHIFT;
> -
> -			for (; remain && sg_ofs < sg_max; sg_ofs++) {
> -				unsigned long len = min(remain, PAGE_SIZE - x);
> -				void *map;
> -
> -				map = kmap_atomic(nth_page(sg_page(sg), sg_ofs));
> -				if (needs_clflush)
> -					drm_clflush_virt_range(map + x, len);
> -				memcpy(ptr, map + x, len);
> -				kunmap_atomic(map);
> -
> -				ptr += len;
> -				remain -= len;
> -				x = 0;
> -			}
> -
> -			sg_ofs = 0;
> -			sg = sg_next(sg);
> +		for (n = offset >> PAGE_SHIFT; remain; n++) {
> +			int len = min(remain, PAGE_SIZE - x);
> +
> +			src = kmap_atomic(i915_gem_object_get_page(src_obj, n));
> +			if (src_needs_clflush)
> +				drm_clflush_virt_range(src + x, len);
> +			memcpy(ptr, src + x, len);
> +			kunmap_atomic(src);
> +
> +			ptr += len;
> +			remain -= len;
> +			x = 0;
>  		}
>  	}
>  
> +	i915_gem_object_finish_access(src_obj);
> +
>  	memset32(dst + length, 0, (dst_obj->base.size - length) / sizeof(u32));
>  
>  	/* dst_obj is returned with vmap pinned */
> +	*needs_clflush_after = dst_needs_clflush & CLFLUSH_AFTER;
> +
>  	return dst;
>  }
>  
> @@ -1360,6 +1376,9 @@ static int check_bbstart(u32 *cmd, u32 offset, u32 length,
>  	if (target_cmd_index == offset)
>  		return 0;
>  
> +	if (IS_ERR(jump_whitelist))
> +		return PTR_ERR(jump_whitelist);
> +
>  	if (!test_bit(target_cmd_index, jump_whitelist)) {
>  		DRM_DEBUG("CMD: BB_START to 0x%llx not a previously executed cmd\n",
>  			  jump_target);
> @@ -1369,28 +1388,10 @@ static int check_bbstart(u32 *cmd, u32 offset, u32 length,
>  	return 0;
>  }
>  
> -/**
> - * intel_engine_cmd_parser_alloc_jump_whitelist() - preallocate jump whitelist for intel_engine_cmd_parser()
> - * @batch_length: length of the commands in batch_obj
> - * @trampoline: Whether jump trampolines are used.
> - *
> - * Preallocates a jump whitelist for parsing the cmd buffer in intel_engine_cmd_parser().
> - * This has to be preallocated, because the command parser runs in signaling context,
> - * and may not allocate any memory.
> - *
> - * Return: NULL or pointer to a jump whitelist, or ERR_PTR() on failure. Use
> - * IS_ERR() to check for errors. Must bre freed() with kfree().
> - *
> - * NULL is a valid value, meaning no allocation was required.
> - */
> -unsigned long *intel_engine_cmd_parser_alloc_jump_whitelist(u32 batch_length,
> -							    bool trampoline)
> +static unsigned long *alloc_whitelist(u32 batch_length)
>  {
>  	unsigned long *jmp;
>  
> -	if (trampoline)
> -		return NULL;
> -
>  	/*
>  	 * We expect batch_length to be less than 256KiB for known users,
>  	 * i.e. we need at most an 8KiB bitmap allocation which should be
> @@ -1425,21 +1426,21 @@ unsigned long *intel_engine_cmd_parser_alloc_jump_whitelist(u32 batch_length,
>   * Return: non-zero if the parser finds violations or otherwise fails; -EACCES
>   * if the batch appears legal but should use hardware parsing
>   */
> +
>  int intel_engine_cmd_parser(struct intel_engine_cs *engine,
>  			    struct i915_vma *batch,
>  			    unsigned long batch_offset,
>  			    unsigned long batch_length,
>  			    struct i915_vma *shadow,
> -			    unsigned long *jump_whitelist,
> -			    void *shadow_map,
> -			    const void *batch_map)
> +			    bool trampoline)
>  {
>  	u32 *cmd, *batch_end, offset = 0;
>  	struct drm_i915_cmd_descriptor default_desc = noop_desc;
>  	const struct drm_i915_cmd_descriptor *desc = &default_desc;
> +	bool needs_clflush_after = false;
> +	unsigned long *jump_whitelist;
>  	u64 batch_addr, shadow_addr;
>  	int ret = 0;
> -	bool trampoline = !jump_whitelist;
>  
>  	GEM_BUG_ON(!IS_ALIGNED(batch_offset, sizeof(*cmd)));
>  	GEM_BUG_ON(!IS_ALIGNED(batch_length, sizeof(*cmd)));
> @@ -1447,8 +1448,18 @@ int intel_engine_cmd_parser(struct intel_engine_cs *engine,
>  				     batch->size));
>  	GEM_BUG_ON(!batch_length);
>  
> -	cmd = copy_batch(shadow->obj, batch->obj, batch_offset, batch_length,
> -			 shadow_map, batch_map);
> +	cmd = copy_batch(shadow->obj, batch->obj,
> +			 batch_offset, batch_length,
> +			 &needs_clflush_after);
> +	if (IS_ERR(cmd)) {
> +		DRM_DEBUG("CMD: Failed to copy batch\n");
> +		return PTR_ERR(cmd);
> +	}
> +
> +	jump_whitelist = NULL;
> +	if (!trampoline)
> +		/* Defer failure until attempted use */
> +		jump_whitelist = alloc_whitelist(batch_length);
>  
>  	shadow_addr = gen8_canonical_addr(shadow->node.start);
>  	batch_addr = gen8_canonical_addr(batch->node.start + batch_offset);
> @@ -1549,6 +1560,9 @@ int intel_engine_cmd_parser(struct intel_engine_cs *engine,
>  
>  	i915_gem_object_flush_map(shadow->obj);
>  
> +	if (!IS_ERR_OR_NULL(jump_whitelist))
> +		kfree(jump_whitelist);
> +	i915_gem_object_unpin_map(shadow->obj);
>  	return ret;
>  }
>  
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 39b5e019c1a5b..92003970253e5 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1913,17 +1913,12 @@ const char *i915_cache_level_str(struct drm_i915_private *i915, int type);
>  int i915_cmd_parser_get_version(struct drm_i915_private *dev_priv);
>  int intel_engine_init_cmd_parser(struct intel_engine_cs *engine);
>  void intel_engine_cleanup_cmd_parser(struct intel_engine_cs *engine);
> -unsigned long *intel_engine_cmd_parser_alloc_jump_whitelist(u32 batch_length,
> -							    bool trampoline);
> -
>  int intel_engine_cmd_parser(struct intel_engine_cs *engine,
>  			    struct i915_vma *batch,
>  			    unsigned long batch_offset,
>  			    unsigned long batch_length,
>  			    struct i915_vma *shadow,
> -			    unsigned long *jump_whitelist,
> -			    void *shadow_map,
> -			    const void *batch_map);
> +			    bool trampoline);
>  #define I915_CMD_PARSER_TRAMPOLINE_SIZE 8
>  
>  /* intel_device_info.c */
> -- 
> 2.31.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch



[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux