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)@@ -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.