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
