linux-arm-msm.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3 15/20] drm/msm: Don't break exclusive fence ordering
       [not found] <20210708173754.3877540-1-daniel.vetter@ffwll.ch>
@ 2021-07-08 17:37 ` Daniel Vetter
  2021-07-08 17:37 ` [PATCH v3 16/20] drm/msm: always wait for the exclusive fence Daniel Vetter
  1 sibling, 0 replies; 4+ messages in thread
From: Daniel Vetter @ 2021-07-08 17:37 UTC (permalink / raw)
  To: DRI Development
  Cc: Intel Graphics Development, Daniel Vetter, Lucas Stach,
	Daniel Vetter, Rob Clark, Sean Paul, linux-arm-msm, freedreno

There's only one exclusive slot, and we must not break the ordering.

Adding a new exclusive fence drops all previous fences from the
dma_resv. To avoid violating the signalling order we err on the side of
over-synchronizing by waiting for the existing fences, even if
userspace asked us to ignore them.

A better fix would be to us a dma_fence_chain or _array like e.g.
amdgpu now uses, but
- msm has a synchronous dma_fence_wait for anything from another
  context, so doesn't seem to care much,
- and it probably makes sense to lift this into dma-resv.c code as a
  proper concept, so that drivers don't have to hack up their own
  solution each on their own.

v2: Improve commit message per Lucas' suggestion.

Cc: Lucas Stach <l.stach@pengutronix.de>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Cc: Rob Clark <robdclark@gmail.com>
Cc: Sean Paul <sean@poorly.run>
Cc: linux-arm-msm@vger.kernel.org
Cc: freedreno@lists.freedesktop.org
---
 drivers/gpu/drm/msm/msm_gem_submit.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c b/drivers/gpu/drm/msm/msm_gem_submit.c
index b71da71a3dd8..edd0051d849f 100644
--- a/drivers/gpu/drm/msm/msm_gem_submit.c
+++ b/drivers/gpu/drm/msm/msm_gem_submit.c
@@ -306,7 +306,8 @@ static int submit_fence_sync(struct msm_gem_submit *submit, bool no_implicit)
 				return ret;
 		}
 
-		if (no_implicit)
+		/* exclusive fences must be ordered */
+		if (no_implicit && !write)
 			continue;
 
 		ret = msm_gem_sync_object(&msm_obj->base, submit->ring->fctx,
-- 
2.32.0


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* [PATCH v3 16/20] drm/msm: always wait for the exclusive fence
       [not found] <20210708173754.3877540-1-daniel.vetter@ffwll.ch>
  2021-07-08 17:37 ` [PATCH v3 15/20] drm/msm: Don't break exclusive fence ordering Daniel Vetter
@ 2021-07-08 17:37 ` Daniel Vetter
  2021-07-09  8:48   ` Christian König
  1 sibling, 1 reply; 4+ messages in thread
From: Daniel Vetter @ 2021-07-08 17:37 UTC (permalink / raw)
  To: DRI Development
  Cc: Intel Graphics Development, Christian König,
	Christian König, Daniel Vetter, Rob Clark, Sean Paul,
	linux-arm-msm, freedreno

From: Christian König <ckoenig.leichtzumerken@gmail.com>

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>
[danvet: Not that hard to compile-test on arm ...]
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Rob Clark <robdclark@gmail.com>
Cc: Sean Paul <sean@poorly.run>
Cc: linux-arm-msm@vger.kernel.org
Cc: freedreno@lists.freedesktop.org
---
 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 141178754231..d9c4f1deeafb 100644
--- a/drivers/gpu/drm/msm/msm_gem.c
+++ b/drivers/gpu/drm/msm/msm_gem.c
@@ -812,17 +812,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.32.0


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH v3 16/20] drm/msm: always wait for the exclusive fence
  2021-07-08 17:37 ` [PATCH v3 16/20] drm/msm: always wait for the exclusive fence Daniel Vetter
@ 2021-07-09  8:48   ` Christian König
  2021-07-09  9:15     ` Daniel Vetter
  0 siblings, 1 reply; 4+ messages in thread
From: Christian König @ 2021-07-09  8:48 UTC (permalink / raw)
  To: Daniel Vetter, DRI Development
  Cc: Intel Graphics Development, Christian König, Rob Clark,
	Sean Paul, linux-arm-msm, freedreno

Am 08.07.21 um 19:37 schrieb Daniel Vetter:
> From: Christian König <ckoenig.leichtzumerken@gmail.com>
>
> 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>
> [danvet: Not that hard to compile-test on arm ...]
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Rob Clark <robdclark@gmail.com>
> Cc: Sean Paul <sean@poorly.run>
> Cc: linux-arm-msm@vger.kernel.org
> Cc: freedreno@lists.freedesktop.org

Wondering a bit why you have that in this patch set now.

But any objections that we push this now?

Thanks,
Christian.

> ---
>   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 141178754231..d9c4f1deeafb 100644
> --- a/drivers/gpu/drm/msm/msm_gem.c
> +++ b/drivers/gpu/drm/msm/msm_gem.c
> @@ -812,17 +812,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;
>   


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v3 16/20] drm/msm: always wait for the exclusive fence
  2021-07-09  8:48   ` Christian König
@ 2021-07-09  9:15     ` Daniel Vetter
  0 siblings, 0 replies; 4+ messages in thread
From: Daniel Vetter @ 2021-07-09  9:15 UTC (permalink / raw)
  To: Christian König
  Cc: DRI Development, Intel Graphics Development,
	Christian König, Rob Clark, Sean Paul, linux-arm-msm,
	freedreno

On Fri, Jul 9, 2021 at 10:48 AM Christian König
<ckoenig.leichtzumerken@gmail.com> wrote:
> Am 08.07.21 um 19:37 schrieb Daniel Vetter:
> > From: Christian König <ckoenig.leichtzumerken@gmail.com>
> >
> > 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>
> > [danvet: Not that hard to compile-test on arm ...]
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Cc: Rob Clark <robdclark@gmail.com>
> > Cc: Sean Paul <sean@poorly.run>
> > Cc: linux-arm-msm@vger.kernel.org
> > Cc: freedreno@lists.freedesktop.org
>
> Wondering a bit why you have that in this patch set now.
>
> But any objections that we push this now?

Cover letter of the first one version explained that I just wanted to
have all the msm fixes in this series, for completeness. But yeah I
thought I put an r-b on your series for this? There was one patch
where I was unhappy with the docs, but there's a new patch for that
now here.
-Daniel

> Thanks,
> Christian.
>
> > ---
> >   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 141178754231..d9c4f1deeafb 100644
> > --- a/drivers/gpu/drm/msm/msm_gem.c
> > +++ b/drivers/gpu/drm/msm/msm_gem.c
> > @@ -812,17 +812,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;
> >
>


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

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2021-07-09  9:15 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <20210708173754.3877540-1-daniel.vetter@ffwll.ch>
2021-07-08 17:37 ` [PATCH v3 15/20] drm/msm: Don't break exclusive fence ordering Daniel Vetter
2021-07-08 17:37 ` [PATCH v3 16/20] drm/msm: always wait for the exclusive fence Daniel Vetter
2021-07-09  8:48   ` Christian König
2021-07-09  9:15     ` Daniel Vetter

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).