| Message ID | 20210706101209.3034092-4-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | dma-resv fence DAG fixes | expand |
Hi Daniel, I'm feeling like I miss a ton of context here, so some maybe dumb questions/remarks below. Am Dienstag, dem 06.07.2021 um 12:12 +0200 schrieb Daniel Vetter: > There's only one exclusive slot, and we must not break the ordering. > > 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/ Sorry, but why is the fence import ioctl a alternative to the fix proposed in this patch? > > 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(-) > > diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c > index 92478a50a580..5c4fed2b7c6a 100644 > --- a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c > +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c > @@ -178,18 +178,20 @@ 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)) { No parenthesis around the write needed. > ret = dma_resv_reserve_shared(robj, 1); > if (ret) > return ret; > } > > - if (submit->flags & ETNA_SUBMIT_NO_IMPLICIT) > + /* exclusive fences must be ordered */ I feel like this comment isn't really helpful. It might tell you all you need to know if you just paged in all the context around dma_resv and the dependency graph, but it's not more than noise to me right now. I guess the comment should answer the question against what the exclusive fence we are going to add needs to be ordered and why it isn't safe to skip implicit sync in that case. Regards, Lucas > + if (submit->flags & ETNA_SUBMIT_NO_IMPLICIT && !write) > continue; > > ret = drm_sched_job_await_implicit(&submit->sched_job, &bo->obj->base, > - bo->flags & ETNA_SUBMIT_BO_WRITE); > + write); > if (ret) > return ret; > }
On Wed, Jul 7, 2021 at 10:54 AM Lucas Stach <l.stach@pengutronix.de> wrote: > > Hi Daniel, > > I'm feeling like I miss a ton of context here, so some maybe dumb > questions/remarks below. > > Am Dienstag, dem 06.07.2021 um 12:12 +0200 schrieb Daniel Vetter: > > There's only one exclusive slot, and we must not break the ordering. > > > > 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/ > > Sorry, but why is the fence import ioctl a alternative to the fix > proposed in this patch? It's not an alternative to fixing the issue here, it's an alternative to trying to fix this here without adding more dependencies. Depends a bit what exactly you want to do, I just linked this for the bigger picture. > > > > > 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(-) > > > > diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c > > index 92478a50a580..5c4fed2b7c6a 100644 > > --- a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c > > +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c > > @@ -178,18 +178,20 @@ 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)) { > > No parenthesis around the write needed. > > > ret = dma_resv_reserve_shared(robj, 1); > > if (ret) > > return ret; > > } > > > > - if (submit->flags & ETNA_SUBMIT_NO_IMPLICIT) > > + /* exclusive fences must be ordered */ > > I feel like this comment isn't really helpful. It might tell you all > you need to know if you just paged in all the context around dma_resv > and the dependency graph, but it's not more than noise to me right now. > > I guess the comment should answer the question against what the > exclusive fence we are going to add needs to be ordered and why it > isn't safe to skip implicit sync in that case. The full-length comment is the doc patch, which is last in the series. Essentially the rule is that your not allowed to drop fences on the floor (which you do if you just set a new write fence and not take any of the existing fences as dependencies), at least for shared buffers. But since it's easier to be consistent I think it's best if we do this just across the board. Like the commit message explains, there's a few ways to fix this: - just treat it as implicit synced - chain the fences together - that way you don't sync, but also there's no fence that's being lost. This is what amdgpu does, and also what the new import ioctl does. 2nd option is a lot more involved, and since all the dma-resv stuff is a bit under discussion, I went with the most minimal thing possible. -Daniel > > Regards, > Lucas > > + if (submit->flags & ETNA_SUBMIT_NO_IMPLICIT && !write) > > continue; > > > > ret = drm_sched_job_await_implicit(&submit->sched_job, &bo->obj->base, > > - bo->flags & ETNA_SUBMIT_BO_WRITE); > > + write); > > if (ret) > > return ret; > > } > >
Am Mittwoch, dem 07.07.2021 um 13:37 +0200 schrieb Daniel Vetter: > On Wed, Jul 7, 2021 at 10:54 AM Lucas Stach <l.stach@pengutronix.de> wrote: > > > > Hi Daniel, > > > > I'm feeling like I miss a ton of context here, so some maybe dumb > > questions/remarks below. > > > > Am Dienstag, dem 06.07.2021 um 12:12 +0200 schrieb Daniel Vetter: > > > There's only one exclusive slot, and we must not break the ordering. > > > > > > 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/ > > > > Sorry, but why is the fence import ioctl a alternative to the fix > > proposed in this patch? > > It's not an alternative to fixing the issue here, it's an alternative > to trying to fix this here without adding more dependencies. Depends a > bit what exactly you want to do, I just linked this for the bigger > picture. > I appreciate the bigger picture, it just makes it harder to follow what is going on in this exact commit when trying to match up the code change with the commit message. I would have expected this link to only be part of the cover letter explaining the series, instead of being part of the commit message. > > > > > > > > > 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(-) > > > > > > diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c > > > index 92478a50a580..5c4fed2b7c6a 100644 > > > --- a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c > > > +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c > > > @@ -178,18 +178,20 @@ 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)) { > > > > No parenthesis around the write needed. > > > > > ret = dma_resv_reserve_shared(robj, 1); > > > if (ret) > > > return ret; > > > } > > > > > > - if (submit->flags & ETNA_SUBMIT_NO_IMPLICIT) > > > + /* exclusive fences must be ordered */ > > > > I feel like this comment isn't really helpful. It might tell you all > > you need to know if you just paged in all the context around dma_resv > > and the dependency graph, but it's not more than noise to me right now. > > > > I guess the comment should answer the question against what the > > exclusive fence we are going to add needs to be ordered and why it > > isn't safe to skip implicit sync in that case. > > The full-length comment is the doc patch, which is last in the series. > Essentially the rule is that your not allowed to drop fences on the > floor (which you do if you just set a new write fence and not take any > of the existing fences as dependencies), at least for shared buffers. > But since it's easier to be consistent I think it's best if we do this > just across the board. > > Like the commit message explains, there's a few ways to fix this: > - just treat it as implicit synced > - chain the fences together - that way you don't sync, but also > there's no fence that's being lost. This is what amdgpu does, and also > what the new import ioctl does. > > 2nd option is a lot more involved, and since all the dma-resv stuff is > a bit under discussion, I went with the most minimal thing possible. Thanks for the confirmation, that was as much as I figured from the doc patch as well. So with that in mind I would expect this comment to read something like this: "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." Regards, Lucas > -Daniel > > > > > Regards, > > Lucas > > > + if (submit->flags & ETNA_SUBMIT_NO_IMPLICIT && !write) > > > continue; > > > > > > ret = drm_sched_job_await_implicit(&submit->sched_job, &bo->obj->base, > > > - bo->flags & ETNA_SUBMIT_BO_WRITE); > > > + write); > > > if (ret) > > > return ret; > > > } > > > > > >
On Wed, Jul 7, 2021 at 2:31 PM Lucas Stach <l.stach@pengutronix.de> wrote: > > Am Mittwoch, dem 07.07.2021 um 13:37 +0200 schrieb Daniel Vetter: > > On Wed, Jul 7, 2021 at 10:54 AM Lucas Stach <l.stach@pengutronix.de> wrote: > > > > > > Hi Daniel, > > > > > > I'm feeling like I miss a ton of context here, so some maybe dumb > > > questions/remarks below. > > > > > > Am Dienstag, dem 06.07.2021 um 12:12 +0200 schrieb Daniel Vetter: > > > > There's only one exclusive slot, and we must not break the ordering. > > > > > > > > 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/ > > > > > > Sorry, but why is the fence import ioctl a alternative to the fix > > > proposed in this patch? > > > > It's not an alternative to fixing the issue here, it's an alternative > > to trying to fix this here without adding more dependencies. Depends a > > bit what exactly you want to do, I just linked this for the bigger > > picture. > > > I appreciate the bigger picture, it just makes it harder to follow what > is going on in this exact commit when trying to match up the code > change with the commit message. I would have expected this link to only > be part of the cover letter explaining the series, instead of being > part of the commit message. I wanted to list all the better fix options in the commit message so that you have the full context. > > > > > > > > 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(-) > > > > > > > > diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c > > > > index 92478a50a580..5c4fed2b7c6a 100644 > > > > --- a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c > > > > +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c > > > > @@ -178,18 +178,20 @@ 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)) { > > > > > > No parenthesis around the write needed. > > > > > > > ret = dma_resv_reserve_shared(robj, 1); > > > > if (ret) > > > > return ret; > > > > } > > > > > > > > - if (submit->flags & ETNA_SUBMIT_NO_IMPLICIT) > > > > + /* exclusive fences must be ordered */ > > > > > > I feel like this comment isn't really helpful. It might tell you all > > > you need to know if you just paged in all the context around dma_resv > > > and the dependency graph, but it's not more than noise to me right now. > > > > > > I guess the comment should answer the question against what the > > > exclusive fence we are going to add needs to be ordered and why it > > > isn't safe to skip implicit sync in that case. > > > > The full-length comment is the doc patch, which is last in the series. > > Essentially the rule is that your not allowed to drop fences on the > > floor (which you do if you just set a new write fence and not take any > > of the existing fences as dependencies), at least for shared buffers. > > But since it's easier to be consistent I think it's best if we do this > > just across the board. > > > > Like the commit message explains, there's a few ways to fix this: > > - just treat it as implicit synced > > - chain the fences together - that way you don't sync, but also > > there's no fence that's being lost. This is what amdgpu does, and also > > what the new import ioctl does. > > > > 2nd option is a lot more involved, and since all the dma-resv stuff is > > a bit under discussion, I went with the most minimal thing possible. > > Thanks for the confirmation, that was as much as I figured from the doc > patch as well. So with that in mind I would expect this comment to read > something like this: > "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." Thanks for the suggestion, I've applied that to all the 3 patches for msm/etnaviv and i915. I hope with that added there's enough context in the commit message that at least the gist is understandable without full context down the road. -Daniel > > Regards, > Lucas > > > -Daniel > > > > > > > > Regards, > > > Lucas > > > > + if (submit->flags & ETNA_SUBMIT_NO_IMPLICIT && !write) > > > > continue; > > > > > > > > ret = drm_sched_job_await_implicit(&submit->sched_job, &bo->obj->base, > > > > - bo->flags & ETNA_SUBMIT_BO_WRITE); > > > > + write); > > > > if (ret) > > > > return ret; > > > > } > > > > > > > > > > > >
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c index 92478a50a580..5c4fed2b7c6a 100644 --- a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c @@ -178,18 +178,20 @@ 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_await_implicit(&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. 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/ 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(-)