On Wed, Jul 7, 2021 at 2:31 PM Lucas Stach <l.stach@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx/ > > > > > > 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@xxxxxxxxx> > > > > Cc: Lucas Stach <l.stach@xxxxxxxxxxxxxx> > > > > Cc: Russell King <linux+etnaviv@xxxxxxxxxxxxxxx> > > > > Cc: Christian Gmeiner <christian.gmeiner@xxxxxxxxx> > > > > Cc: etnaviv@xxxxxxxxxxxxxxxxxxxxx > > > > --- > > > > 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; > > > > } > > > > > > > > > > > > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
