Re: [Intel-gfx] [PATCH 03/10] drm/syncobj: add new drm_syncobj_add_point interface v2

[Daniel Vetter <daniel-/w4YWyX8dFk@public.gmane.org>]

On Fri, Dec 07, 2018 at 11:54:15PM +0800, Chunming Zhou wrote:
> From: Christian König <ckoenig.leichtzumerken@gmail.com>
> 
> Use the dma_fence_chain object to create a timeline of fence objects
> instead of just replacing the existing fence.
> 
> v2: rebase and cleanup
> 
> Signed-off-by: Christian König <christian.koenig@amd.com>

Somewhat jumping back into this. Not sure we discussed this already or
not. I'm a bit unclear on why we have to chain the fences in the timeline:

- The timeline stuff is modelled after the WDDM2 monitored fences. Which
  really are just u64 counters in memory somewhere (I think could be
  system ram or vram). Because WDDM2 has the memory management entirely
  separated from rendering synchronization it totally allows userspace to
  create loops and deadlocks and everything else nasty using this - the
  memory manager won't deadlock because these monitored fences never leak
  into the buffer manager. And if CS deadlock, gpu reset takes care of the
  mess.

- This has a few consequences, as in they seem to indeed work like a
  memory location: Userspace incrementing out-of-order (because they run
  batches updating the same fence on different engines) is totally fine,
  as is doing anything else "stupid".

- Now on linux we can't allow anything, because we need to make sure that
  deadlocks don't leak into the memory manager. But as long as we block
  until the underlying dma_fence has materialized, nothing userspace can
  do will lead to such a deadlock. Even if userspace ends up submitting
  jobs without enough built-in synchronization, leading to out-of-order
  signalling of fences on that "timeline". And I don't think that would
  pose a problem for us.

Essentially I think we can look at timeline syncobj as a dma_fence
container indexed through an integer, and there's no need to enforce that
the timline works like a real dma_fence timeline, with all it's
guarantees. It's just a pile of (possibly, if userspace is stupid)
unrelated dma_fences. You could implement the entire thing in userspace
after all, except for the "we want to share these timeline objects between
processes" problem.

tldr; I think we can drop the dma_fence_chain complexity completely. Or at
least I'm not really understanding why it's needed.

Of course that means drivers cannot treat a drm_syncobj timeline as a
dma_fence timeline. But given the future fences stuff and all that, that's
already out of the window anyway.

What am I missing?
-Daniel

> ---
>  drivers/gpu/drm/drm_syncobj.c | 37 +++++++++++++++++++++++++++++++++++
>  include/drm/drm_syncobj.h     |  5 +++++
>  2 files changed, 42 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
> index e19525af0cce..51f798e2194f 100644
> --- a/drivers/gpu/drm/drm_syncobj.c
> +++ b/drivers/gpu/drm/drm_syncobj.c
> @@ -122,6 +122,43 @@ static void drm_syncobj_remove_wait(struct drm_syncobj *syncobj,
>  	spin_unlock(&syncobj->lock);
>  }
>  
> +/**
> + * drm_syncobj_add_point - add new timeline point to the syncobj
> + * @syncobj: sync object to add timeline point do
> + * @chain: chain node to use to add the point
> + * @fence: fence to encapsulate in the chain node
> + * @point: sequence number to use for the point
> + *
> + * Add the chain node as new timeline point to the syncobj.
> + */
> +void drm_syncobj_add_point(struct drm_syncobj *syncobj,
> +			   struct dma_fence_chain *chain,
> +			   struct dma_fence *fence,
> +			   uint64_t point)
> +{
> +	struct syncobj_wait_entry *cur, *tmp;
> +	struct dma_fence *prev;
> +
> +	dma_fence_get(fence);
> +
> +	spin_lock(&syncobj->lock);
> +
> +	prev = rcu_dereference_protected(syncobj->fence,
> +					 lockdep_is_held(&syncobj->lock));
> +	dma_fence_chain_init(chain, prev, fence, point);
> +	rcu_assign_pointer(syncobj->fence, &chain->base);
> +
> +	list_for_each_entry_safe(cur, tmp, &syncobj->cb_list, node) {
> +		list_del_init(&cur->node);
> +		syncobj_wait_syncobj_func(syncobj, cur);
> +	}
> +	spin_unlock(&syncobj->lock);
> +
> +	/* Walk the chain once to trigger garbage collection */
> +	dma_fence_chain_for_each(prev, fence);
> +}
> +EXPORT_SYMBOL(drm_syncobj_add_point);
> +
>  /**
>   * drm_syncobj_replace_fence - replace fence in a sync object.
>   * @syncobj: Sync object to replace fence in
> diff --git a/include/drm/drm_syncobj.h b/include/drm/drm_syncobj.h
> index 7c6ed845c70d..8acb4ae4f311 100644
> --- a/include/drm/drm_syncobj.h
> +++ b/include/drm/drm_syncobj.h
> @@ -27,6 +27,7 @@
>  #define __DRM_SYNCOBJ_H__
>  
>  #include "linux/dma-fence.h"
> +#include "linux/dma-fence-chain.h"
>  
>  /**
>   * struct drm_syncobj - sync object.
> @@ -110,6 +111,10 @@ drm_syncobj_fence_get(struct drm_syncobj *syncobj)
>  
>  struct drm_syncobj *drm_syncobj_find(struct drm_file *file_private,
>  				     u32 handle);
> +void drm_syncobj_add_point(struct drm_syncobj *syncobj,
> +			   struct dma_fence_chain *chain,
> +			   struct dma_fence *fence,
> +			   uint64_t point);
>  void drm_syncobj_replace_fence(struct drm_syncobj *syncobj,
>  			       struct dma_fence *fence);
>  int drm_syncobj_find_fence(struct drm_file *file_private,
> -- 
> 2.17.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx