Re: [PATCH 17/23] dma-buf: specify usage while adding fences to dma_resv obj v5

[Daniel Vetter <daniel@ffwll.ch>]

On Fri, Apr 01, 2022 at 05:01:13PM +0200, Christian König wrote:
> 
> 
> Am 29.03.22 um 17:43 schrieb Daniel Vetter:
> > On Mon, Mar 21, 2022 at 02:58:50PM +0100, Christian König wrote:
> > [SNIP]
> > >   /**
> > > - * dma_resv_add_shared_fence - Add a fence to a shared slot
> > > + * dma_resv_add_fence - Add a fence to the dma_resv obj
> > >    * @obj: the reservation object
> > > - * @fence: the shared fence to add
> > > + * @fence: the fence to add
> > > + * @usage: how the fence is used, see enum dma_resv_usage
> > >    *
> > > - * Add a fence to a shared slot, @obj must be locked with dma_resv_lock(), and
> > > + * Add a fence to a slot, @obj must be locked with dma_resv_lock(), and
> > >    * dma_resv_reserve_fences() has been called.
> > >    *
> > >    * See also &dma_resv.fence for a discussion of the semantics.
> > >    */
> > > -void dma_resv_add_shared_fence(struct dma_resv *obj, struct dma_fence *fence)
> > > +void dma_resv_add_fence(struct dma_resv *obj, struct dma_fence *fence,
> > > +			enum dma_resv_usage usage)
> > >   {
> > >   	struct dma_resv_list *fobj;
> > >   	struct dma_fence *old;
> > > @@ -274,44 +308,45 @@ void dma_resv_add_shared_fence(struct dma_resv *obj, struct dma_fence *fence)
> > >   	dma_resv_assert_held(obj);
> > > -	/* Drivers should not add containers here, instead add each fence
> > > -	 * individually.
> > > +	/* TODO: Drivers should not add containers here, instead add each fence
> > > +	 * individually. Disabled for now until we cleaned up amdgpu/ttm.
> > >   	 */
> > > -	WARN_ON(dma_fence_is_container(fence));
> > > +	/* WARN_ON(dma_fence_is_container(fence)); */
> > Uh this looks like it's a misplaced hack?
> 
> Unfortunately not.
> 
> > If you do need it and cant get rid of it with patch reordering, then I
> > think it needs to be split out for extra attention.
> 
> The problem is that I would need to squash removing the amdgpu workaround
> into this patch as well.
> 
> And I don't really want to make this patch more complicated that it already
> is.

Yeah I got it later on. Please explain the story in the commit message,
and how it'll be resolved. Otherwise this is a bit much wtf to merge :-)

> 
> > > diff --git a/drivers/gpu/drm/lima/lima_gem.c b/drivers/gpu/drm/lima/lima_gem.c
> > > index 9435a3ca71c8..38caa7f78871 100644
> > > --- a/drivers/gpu/drm/lima/lima_gem.c
> > > +++ b/drivers/gpu/drm/lima/lima_gem.c
> > > @@ -366,7 +366,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);
> > Not very compile-tested it seems.
> 
> At least it used to compile fine once, but obviously need to give it another
> go.
> 
> > I think it'd be good to split this further:
> > 
> > - Add dma_resv_add_fence, which just adds either an exclusive or shared
> >    fences.
> > - Convert drivers, cc driver authors (this patch doesn't seem to have
> >    them).
> > 
> > I think the above two could also be a single patch, but should work even
> > more split.
> 
> That is easier said than done. I will see what I can do.
> 
> The other documentation comments you had should be fixed in the next round,
> but you might want to take another full look at this.

Yeah I get that it's utter pain. I think if you add a list to the commit
message with a few comments on how each driver is touched and all that
(i.e. at least type up the separate commmit messages for the separate
patches that should be split, but are a real pain to split), then I think
that's fine.

I've also done audit patches in the past which had that per-driver blurb
to cover all the cases, sometimes that's the least crappy way to do
things.

Holds also for the other patches which then add USAGE_KERNEL and
USAGE_BOOKKEEPING - splitting is a bit much but at least having a
per-driver blurb of what/why you change would be really good to include I
think. Just so we remember a bit easier why things changed. I think then
we should be good here with these (well aside from the one ttm change that
I didn't follow yet in another patch).
-Daniel

> 
> Thanks,
> Christian.
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch