| Message ID | 20220331204651.2699107-5-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | etnaviv drm/sched stragglers | expand |
On Thu, 31 Mar 2022 at 22:46, 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 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. Hence go with the simple > fix for now. > > Another option is the fence import ioctl from Jason: > > https://lore.kernel.org/dri-devel/20210610210925.642582-7-jason@jlekstrand.net/ > > v2: Improve commit message per Lucas' suggestion. > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > Cc: Lucas Stach <l.stach@pengutronix.de> > Cc: Russell King <linux+etnaviv@armlinux.org.uk> > Cc: Christian Gmeiner <christian.gmeiner@gmail.com> > Cc: etnaviv@lists.freedesktop.org Hm thinking about this some more, with Christian's dma_resv_usage work this shouldn't be needed anymore. Christian, do you want me to drop this? -Daniel > --- > drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c > index 5f502c49aec2..14c5ff155336 100644 > --- a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c > +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c > @@ -178,19 +178,21 @@ static int submit_fence_sync(struct etnaviv_gem_submit *submit) > for (i = 0; i < submit->nr_bos; i++) { > struct etnaviv_gem_submit_bo *bo = &submit->bos[i]; > struct dma_resv *robj = bo->obj->base.resv; > + bool write = bo->flags & ETNA_SUBMIT_BO_WRITE; > > - if (!(bo->flags & ETNA_SUBMIT_BO_WRITE)) { > + if (!(write)) { > ret = dma_resv_reserve_shared(robj, 1); > if (ret) > return ret; > } > > - if (submit->flags & ETNA_SUBMIT_NO_IMPLICIT) > + /* exclusive fences must be ordered */ > + if (submit->flags & ETNA_SUBMIT_NO_IMPLICIT && !write) > continue; > > ret = drm_sched_job_add_implicit_dependencies(&submit->sched_job, > &bo->obj->base, > - bo->flags & ETNA_SUBMIT_BO_WRITE); > + write); > if (ret) > return ret; > } > -- > 2.34.1 >
Am 04.04.22 um 15:14 schrieb Daniel Vetter: > On Thu, 31 Mar 2022 at 22:46, 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 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. Hence go with the simple >> fix for now. >> >> Another option is the fence import ioctl from Jason: >> >> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Fdri-devel%2F20210610210925.642582-7-jason%40jlekstrand.net%2F&data=04%7C01%7Cchristian.koenig%40amd.com%7C7db3184856cd4b6fa2ce08da163d2543%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637846749141237874%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=CQqU37VcltOuwmDN3068yv1c%2FKJ9gaf1U3T7eBQohK4%3D&reserved=0 >> >> v2: Improve commit message per Lucas' suggestion. >> >> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> >> Cc: Lucas Stach <l.stach@pengutronix.de> >> Cc: Russell King <linux+etnaviv@armlinux.org.uk> >> Cc: Christian Gmeiner <christian.gmeiner@gmail.com> >> Cc: etnaviv@lists.freedesktop.org > Hm thinking about this some more, with Christian's dma_resv_usage work > this shouldn't be needed anymore. > > Christian, do you want me to drop this? If it isn't committed yet we could as well just drop it. I've already pushed the patch which makes this superfluous. Christian. > -Daniel > >> --- >> drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c | 8 +++++--- >> 1 file changed, 5 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c >> index 5f502c49aec2..14c5ff155336 100644 >> --- a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c >> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c >> @@ -178,19 +178,21 @@ static int submit_fence_sync(struct etnaviv_gem_submit *submit) >> for (i = 0; i < submit->nr_bos; i++) { >> struct etnaviv_gem_submit_bo *bo = &submit->bos[i]; >> struct dma_resv *robj = bo->obj->base.resv; >> + bool write = bo->flags & ETNA_SUBMIT_BO_WRITE; >> >> - if (!(bo->flags & ETNA_SUBMIT_BO_WRITE)) { >> + if (!(write)) { >> ret = dma_resv_reserve_shared(robj, 1); >> if (ret) >> return ret; >> } >> >> - if (submit->flags & ETNA_SUBMIT_NO_IMPLICIT) >> + /* exclusive fences must be ordered */ >> + if (submit->flags & ETNA_SUBMIT_NO_IMPLICIT && !write) >> continue; >> >> ret = drm_sched_job_add_implicit_dependencies(&submit->sched_job, >> &bo->obj->base, >> - bo->flags & ETNA_SUBMIT_BO_WRITE); >> + write); >> if (ret) >> return ret; >> } >> -- >> 2.34.1 >> >
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c index 5f502c49aec2..14c5ff155336 100644 --- a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c @@ -178,19 +178,21 @@ static int submit_fence_sync(struct etnaviv_gem_submit *submit) for (i = 0; i < submit->nr_bos; i++) { struct etnaviv_gem_submit_bo *bo = &submit->bos[i]; struct dma_resv *robj = bo->obj->base.resv; + bool write = bo->flags & ETNA_SUBMIT_BO_WRITE; - if (!(bo->flags & ETNA_SUBMIT_BO_WRITE)) { + if (!(write)) { ret = dma_resv_reserve_shared(robj, 1); if (ret) return ret; } - if (submit->flags & ETNA_SUBMIT_NO_IMPLICIT) + /* exclusive fences must be ordered */ + if (submit->flags & ETNA_SUBMIT_NO_IMPLICIT && !write) continue; ret = drm_sched_job_add_implicit_dependencies(&submit->sched_job, &bo->obj->base, - bo->flags & ETNA_SUBMIT_BO_WRITE); + write); if (ret) return ret; }
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 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. Hence go with the simple fix for now. Another option is the fence import ioctl from Jason: https://lore.kernel.org/dri-devel/20210610210925.642582-7-jason@jlekstrand.net/ v2: Improve commit message per Lucas' suggestion. Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> Cc: Lucas Stach <l.stach@pengutronix.de> Cc: Russell King <linux+etnaviv@armlinux.org.uk> Cc: Christian Gmeiner <christian.gmeiner@gmail.com> Cc: etnaviv@lists.freedesktop.org --- drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)