| Message ID | 20210712175352.802687-15-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | None | expand |
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 >
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 > >
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
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
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,
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(-)