On Fri, Jul 02, 2021 at 01:16:39PM +0200, Christian König wrote:

Explicitly document that code can't assume that shared fences signal after the exclusive fence.

Signed-off-by: Christian König christian.koenig@amd.com

drivers/dma-buf/dma-resv.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c index f26c71747d43..4ab02b6c387a 100644 --- a/drivers/dma-buf/dma-resv.c +++ b/drivers/dma-buf/dma-resv.c @@ -235,7 +235,10 @@ EXPORT_SYMBOL(dma_resv_reset_shared_max);

• @fence: the shared fence to add
• Add a fence to a shared slot, obj->lock must be held, and

• • dma_resv_reserve_shared() has been called.

• • dma_resv_reserve_shared() has been called. The shared fences can signal in
• • any order and there is especially no guarantee that shared fences signal
• • after the exclusive one. Code relying on any signaling order is broken and
• • needs to be fixed.

This feels like the last place I'd go look for how I should handle dependencies. It's the function for adding shared fences after all, has absolutely nothing to do with whether we should wait for them.

I'll type up something else. -Daniel

*/ void dma_resv_add_shared_fence(struct dma_resv *obj, struct dma_fence *fence) { -- 2.25.1

On Fri, Jul 02, 2021 at 01:16:40PM +0200, Christian König wrote:

As the name implies if testing all fences is requested we should indeed test all fences and not skip the exclusive one because we see shared ones.

v2: fix logic once more

Signed-off-by: Christian König christian.koenig@amd.com

Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch

---

drivers/dma-buf/dma-resv.c | 33 ++++++++++++--------------------- 1 file changed, 12 insertions(+), 21 deletions(-)

diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c index 4ab02b6c387a..18dd5a6ca06c 100644 --- a/drivers/dma-buf/dma-resv.c +++ b/drivers/dma-buf/dma-resv.c @@ -618,25 +618,21 @@ static inline int dma_resv_test_signaled_single(struct dma_fence *passed_fence) */ bool dma_resv_test_signaled(struct dma_resv *obj, bool test_all) {

• unsigned int seq, shared_count;

• struct dma_fence *fence;

• unsigned int seq; int ret;

  rcu_read_lock();

retry: ret = true;

• shared_count = 0; seq = read_seqcount_begin(&obj->seq);

  if (test_all) { struct dma_resv_list *fobj = dma_resv_shared_list(obj);

• unsigned int i;
  

• if (fobj)
  

• 	shared_count = fobj->shared_count;
  

• unsigned int i, shared_count;
  
  

• shared_count = fobj ? fobj->shared_count : 0;
  

  for (i = 0; i < shared_count; ++i) {

• 	struct dma_fence *fence;
  

• fence = rcu_dereference(fobj->shared[i]);
  ret = dma_resv_test_signaled_single(fence);
  if (ret < 0)
  

@@ -644,24 +640,19 @@ bool dma_resv_test_signaled(struct dma_resv *obj, bool test_all) else if (!ret) break; }

• if (read_seqcount_retry(&obj->seq, seq))
  

• 	goto retry;
  

  }

• if (!shared_count) {

• struct dma_fence *fence_excl = dma_resv_excl_fence(obj);
  

• if (fence_excl) {
  

• 	ret = dma_resv_test_signaled_single(fence_excl);
  

• 	if (ret < 0)
  

• 		goto retry;
  

• fence = dma_resv_excl_fence(obj);
• if (ret && fence) {

• ret = dma_resv_test_signaled_single(fence);
  

• if (ret < 0)
  

• 	goto retry;
  
  

• 	if (read_seqcount_retry(&obj->seq, seq))
  

• 		goto retry;
  

• }
  

  }

• if (read_seqcount_retry(&obj->seq, seq))

• goto retry;
  

• rcu_read_unlock(); return ret;

}

2.25.1

On Fri, Jul 02, 2021 at 01:16:41PM +0200, Christian König wrote:

Drivers also need to to sync to the exclusive fence when a shared one is present.

Signed-off-by: Christian König christian.koenig@amd.com

Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch

---

drivers/gpu/drm/nouveau/nouveau_fence.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/nouveau/nouveau_fence.c b/drivers/gpu/drm/nouveau/nouveau_fence.c index 6b43918035df..05d0b3eb3690 100644 --- a/drivers/gpu/drm/nouveau/nouveau_fence.c +++ b/drivers/gpu/drm/nouveau/nouveau_fence.c @@ -358,7 +358,7 @@ nouveau_fence_sync(struct nouveau_bo *nvbo, struct nouveau_channel *chan, bool e fobj = dma_resv_shared_list(resv); fence = dma_resv_excl_fence(resv);

• if (fence && (!exclusive || !fobj || !fobj->shared_count)) {

• if (fence) { struct nouveau_channel *prev = NULL; bool must_wait = true;

-- 2.25.1

On Fri, Jul 02, 2021 at 01:16:42PM +0200, Christian König wrote:

Drivers also need to to sync to the exclusive fence when a shared one is present.

Completely untested since the driver won't even compile on !ARM.

It's really not that hard to set up a cross-compiler, reasonable distros have that now all packages. Does explain though why you tend to break the arm build with drm-misc patches.

Please fix this.

Signed-off-by: Christian König christian.koenig@amd.com

Reviewed-by: Daniel Vetter daniel.vetter@ffwll.ch

---

drivers/gpu/drm/msm/msm_gem.c | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c index a94a43de95ef..72a07e311de3 100644 --- a/drivers/gpu/drm/msm/msm_gem.c +++ b/drivers/gpu/drm/msm/msm_gem.c @@ -817,17 +817,15 @@ int msm_gem_sync_object(struct drm_gem_object *obj, struct dma_fence *fence; int i, ret;

• fobj = dma_resv_shared_list(obj->resv);
• if (!fobj || (fobj->shared_count == 0)) {

• fence = dma_resv_excl_fence(obj->resv);
  

• /* don't need to wait on our own fences, since ring is fifo */
  

• if (fence && (fence->context != fctx->context)) {
  

• 	ret = dma_fence_wait(fence, true);
  

• 	if (ret)
  

• 		return ret;
  

• }
  

• fence = dma_resv_excl_fence(obj->resv);

• /* don't need to wait on our own fences, since ring is fifo */

• if (fence && (fence->context != fctx->context)) {

• ret = dma_fence_wait(fence, true);
  

• if (ret)
  

• 	return ret;
  

  }

• fobj = dma_resv_shared_list(obj->resv); if (!exclusive || !fobj) return 0;

-- 2.25.1