* [PATCH v4 14/18] drm/msm: Don't break exclusive fence ordering
[not found] <20210712175352.802687-1-daniel.vetter@ffwll.ch>
@ 2021-07-12 17:53 ` Daniel Vetter
2021-07-13 16:55 ` Rob Clark
0 siblings, 1 reply; 5+ messages in thread
From: Daniel Vetter @ 2021-07-12 17:53 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] 5+ messages in thread
* Re: [PATCH v4 14/18] drm/msm: Don't break exclusive fence ordering
2021-07-12 17:53 ` [PATCH v4 14/18] drm/msm: Don't break exclusive fence ordering Daniel Vetter
@ 2021-07-13 16:55 ` Rob Clark
2021-07-13 16:58 ` Daniel Vetter
0 siblings, 1 reply; 5+ messages in thread
From: Rob Clark @ 2021-07-13 16:55 UTC (permalink / raw)
To: Daniel Vetter
Cc: DRI Development, Intel Graphics Development, Lucas Stach,
Daniel Vetter, Sean Paul, linux-arm-msm, freedreno
On Mon, Jul 12, 2021 at 1:02 PM Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
>
> 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;
In practice, modern userspace (the kind that is more likely to set the
no-implicit flag on every submit) also sets MSM_SUBMIT_BO_WRITE on
every bo, to shave some cpu overhead so I suppose this would not
really hurt anything
Do you know if this is covered in any piglit/etc test?
BR,
-R
>
> ret = msm_gem_sync_object(&msm_obj->base, submit->ring->fctx,
> --
> 2.32.0
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v4 14/18] drm/msm: Don't break exclusive fence ordering
2021-07-13 16:55 ` Rob Clark
@ 2021-07-13 16:58 ` Daniel Vetter
2021-07-13 17:46 ` Rob Clark
0 siblings, 1 reply; 5+ messages in thread
From: Daniel Vetter @ 2021-07-13 16:58 UTC (permalink / raw)
To: Rob Clark
Cc: DRI Development, Intel Graphics Development, Lucas Stach,
Daniel Vetter, Sean Paul, linux-arm-msm, freedreno
On Tue, Jul 13, 2021 at 6:51 PM Rob Clark <robdclark@gmail.com> wrote:
>
> On Mon, Jul 12, 2021 at 1:02 PM Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> >
> > 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;
>
> In practice, modern userspace (the kind that is more likely to set the
> no-implicit flag on every submit) also sets MSM_SUBMIT_BO_WRITE on
> every bo, to shave some cpu overhead so I suppose this would not
> really hurt anything
>
> Do you know if this is covered in any piglit/etc test?
You need some command submission, plus buffer sharing with vgem
setting it's own exclusive fences, plus checking with dma_buf poll()
whether it signals all in the right order. That's pretty low-level, so
maybe something in igt, but I haven't typed that. Maybe I need to do
that for i915 at least.
-Daniel
> BR,
> -R
>
> >
> > ret = msm_gem_sync_object(&msm_obj->base, submit->ring->fctx,
> > --
> > 2.32.0
> >
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v4 14/18] drm/msm: Don't break exclusive fence ordering
2021-07-13 17:46 ` Rob Clark
@ 2021-07-13 17:45 ` Daniel Vetter
0 siblings, 0 replies; 5+ messages in thread
From: Daniel Vetter @ 2021-07-13 17:45 UTC (permalink / raw)
To: Rob Clark
Cc: DRI Development, Intel Graphics Development, Lucas Stach,
Daniel Vetter, Sean Paul, linux-arm-msm, freedreno
On Tue, Jul 13, 2021 at 7:42 PM Rob Clark <robdclark@gmail.com> wrote:
> On Tue, Jul 13, 2021 at 9:58 AM Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> >
> > On Tue, Jul 13, 2021 at 6:51 PM Rob Clark <robdclark@gmail.com> wrote:
> > >
> > > On Mon, Jul 12, 2021 at 1:02 PM Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> > > >
> > > > 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;
> > >
> > > In practice, modern userspace (the kind that is more likely to set the
> > > no-implicit flag on every submit) also sets MSM_SUBMIT_BO_WRITE on
> > > every bo, to shave some cpu overhead so I suppose this would not
> > > really hurt anything
> > >
> > > Do you know if this is covered in any piglit/etc test?
> >
> > You need some command submission, plus buffer sharing with vgem
> > setting it's own exclusive fences, plus checking with dma_buf poll()
> > whether it signals all in the right order. That's pretty low-level, so
> > maybe something in igt, but I haven't typed that. Maybe I need to do
> > that for i915 at least.
>
> ok, you lost me at vgem ;-)
>
> (the vgem vs cache situation on arm is kinda hopeless)
Oh that explains a few things ... I just found out why vgem is failing
for wc buffers on x86 (on some of our less-coherent igpu at least),
and wondered how the heck this works on arm. Sounds like it just
doesn't :-/
On the testcase: You'd never actually check buffer contents, only
fences, so the test would still work.
-Daniel
>
> BR,
> -R
>
> > -Daniel
> >
> > > BR,
> > > -R
> > >
> > > >
> > > > ret = msm_gem_sync_object(&msm_obj->base, submit->ring->fctx,
> > > > --
> > > > 2.32.0
> > > >
> >
> >
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v4 14/18] drm/msm: Don't break exclusive fence ordering
2021-07-13 16:58 ` Daniel Vetter
@ 2021-07-13 17:46 ` Rob Clark
2021-07-13 17:45 ` Daniel Vetter
0 siblings, 1 reply; 5+ messages in thread
From: Rob Clark @ 2021-07-13 17:46 UTC (permalink / raw)
To: Daniel Vetter
Cc: DRI Development, Intel Graphics Development, Lucas Stach,
Daniel Vetter, Sean Paul, linux-arm-msm, freedreno
On Tue, Jul 13, 2021 at 9:58 AM Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
>
> On Tue, Jul 13, 2021 at 6:51 PM Rob Clark <robdclark@gmail.com> wrote:
> >
> > On Mon, Jul 12, 2021 at 1:02 PM Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> > >
> > > 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;
> >
> > In practice, modern userspace (the kind that is more likely to set the
> > no-implicit flag on every submit) also sets MSM_SUBMIT_BO_WRITE on
> > every bo, to shave some cpu overhead so I suppose this would not
> > really hurt anything
> >
> > Do you know if this is covered in any piglit/etc test?
>
> You need some command submission, plus buffer sharing with vgem
> setting it's own exclusive fences, plus checking with dma_buf poll()
> whether it signals all in the right order. That's pretty low-level, so
> maybe something in igt, but I haven't typed that. Maybe I need to do
> that for i915 at least.
ok, you lost me at vgem ;-)
(the vgem vs cache situation on arm is kinda hopeless)
BR,
-R
> -Daniel
>
> > BR,
> > -R
> >
> > >
> > > ret = msm_gem_sync_object(&msm_obj->base, submit->ring->fctx,
> > > --
> > > 2.32.0
> > >
>
>
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-07-13 17:45 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <20210712175352.802687-1-daniel.vetter@ffwll.ch>
2021-07-12 17:53 ` [PATCH v4 14/18] drm/msm: Don't break exclusive fence ordering Daniel Vetter
2021-07-13 16:55 ` Rob Clark
2021-07-13 16:58 ` Daniel Vetter
2021-07-13 17:46 ` Rob Clark
2021-07-13 17:45 ` 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).