On Thu, Apr 07, 2022 at 10:01:52AM +0200, Christian König wrote:
> Am 06.04.22 um 14:35 schrieb Daniel Vetter:
> > On Wed, Apr 06, 2022 at 02:32:22PM +0200, Daniel Vetter wrote:
> > > On Wed, Apr 06, 2022 at 09:51:19AM +0200, Christian König wrote:
> > > > [SNIP]
> > > > diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
> > > > index 53f7c78628a4..98bb5c9239de 100644
> > > > --- a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
> > > > +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
> > > > @@ -202,14 +202,10 @@ static void submit_attach_object_fences(struct etnaviv_gem_submit *submit)
> > > > for (i = 0; i < submit->nr_bos; i++) {
> > > > struct drm_gem_object *obj = &submit->bos[i].obj->base;
> > > > + bool write = submit->bos[i].flags & ETNA_SUBMIT_BO_WRITE;
> > > > - if (submit->bos[i].flags & ETNA_SUBMIT_BO_WRITE)
> > > > - dma_resv_add_excl_fence(obj->resv,
> > > > - submit->out_fence);
> > > > - else
> > > > - dma_resv_add_shared_fence(obj->resv,
> > > > - submit->out_fence);
> > > > -
> > > > + dma_resv_add_fence(obj->resv, submit->out_fence, write ?
> > > > + DMA_RESV_USAGE_WRITE : DMA_RESV_USAGE_READ);
> > > Iirc I had some suggestions to use dma_resv_usage_rw here and above. Do
> > > these happen in later patches? There's also a few more of these later on.
>
> That won't work. dma_resv_usage_rw() returns the usage as necessary for
> dependencies. In other words write return DMA_RESV_USAGE_READ and read
> return DMA_RESV_USAGE_WRITE.
Hm right, that's a bit annoying due to the asymetry in dependencies and
adding fences.
>
> What we could do is to add a dma_resv_add_fence_rw() wrapper which does the
> necessary ?: in a central place.
I'm not sure it's overkill, but what about something like this:
enum drm_sync_mode {
DRM_SYNC_NO_IMPLICIT,
DRM_SYNC_WRITE,
DRM_SYNC_READ,
}
And then two functions, on in the drm/sched which replaces the current
add_implicit_dependencies, and the other which would be in the glorious
future eu utils shared between ttm and gem drivers, which adds the fence
with the right usage. And they would take care of the right mapping in
each case.
And then all we'd still have in driver code is mapping from random
bonghits driver flags to drm_sync_mode, and all the confusion would be in
shared code. And see above, at least for me it's confusing as heck :-)
>
> > > > diff --git a/drivers/gpu/drm/lima/lima_gem.c b/drivers/gpu/drm/lima/lima_gem.c
> > > > index e0a11ee0e86d..cb3bfccc930f 100644
> > > > --- a/drivers/gpu/drm/lima/lima_gem.c
> > > > +++ b/drivers/gpu/drm/lima/lima_gem.c
> > > > @@ -367,7 +367,7 @@ int lima_gem_submit(struct drm_file *file, struct lima_submit *submit)
> > > > if (submit->bos[i].flags & LIMA_SUBMIT_BO_WRITE)
> > > > dma_resv_add_excl_fence(lima_bo_resv(bos[i]), fence);
> > > > else
> > > > - dma_resv_add_shared_fence(lima_bo_resv(bos[i]), fence);
> > > > + dma_resv_add_fence(lima_bo_resv(bos[i]), fence);
> > Correction on the r-b, I'm still pretty sure that this won't compile at
> > all.
>
> Grrr, I've forgot to add CONFIG_OF to my compile build config.
>
> BTW: Do we have a tool for compile test coverage of patches? E.g. something
> which figures out if a build created an o file for each c file a patch
> touched?
Just regrets when I screw up :-/
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
