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

["Christian König" <christian.koenig@amd.com>]

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.

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

Thanks,
Christian.