[RFC 0/5] rework fences on struct sync_file

[Christian König]

Am 24.06.2016 um 16:59 schrieb Gustavo Padovan:
> 2016-06-24 Christian König :
>
>> Am 24.06.2016 um 15:17 schrieb Gustavo Padovan:
>>> Hi Christian,
>>>
>>> 2016-06-24 Christian König :
>>>
 Am 23.06.2016 um 17:29 schrieb Gustavo Padovan:
> From: Gustavo Padovan 
>
> Hi all,
>
> This is an attempt to improve fence support on Sync File. The basic idea
> is to have only sync_file->fence and store all fences there, either as
> normal fences or fence_arrays. That way we can remove some potential
> duplication when using fence_array with sync_file: the duplication of the 
> array
> of fences and the duplication of fence_add_callback() for all fences.
>
> Now when creating a new sync_file during the merge process 
> sync_file_set_fence()
> will set sync_file->fence based on the number of fences for that 
> sync_file. If
> there is more than one fence a fence_array is created. One important 
> advantage
> approach is that we only add one fence callback now, no matter how many 
> fences
> there are in a sync_file - the individual callbacks are added by 
> fence_array.
>
> Two fence ops had to be created to help abstract the difference between 
> handling
> fences and fences_arrays: .teardown() and .get_fences(). The former run 
> needed
> on fence_array, and the latter just return a copy of all fences in the 
> fence.
> I'm not so sure about adding those two, speacially .get_fences(). What do 
> you
> think?
 Clearly not a good idea to add this a fence ops, cause those are 
 specialized
 functions for only a certain fence implementation (the fence_array).
>>> Are you refering only to .get_fences()?
>> That comment was only for the get_fences() operation, but the teardown()
>> callback looks very suspicious to me as well.
>>
>> Can you explain once more why that should be necessary?
> When the sync_file owner exits we need to clean up it and that means releasing
> the fence too, however with fence_array we can't just call fence_put()
> as a extra reference to array->base for each fence is held when enabling
> signalling. Thus we need a prior step, that I called teardown(), to
> remove the callback for not signaled fences and put the extra
> references.
>
> Another way to do this would be:
>
>   if (fence_is_array(sync_file->fence))
>   fence_array_destroy(to_fence_array(sync_file->fence));
>   else
>   fence_put(sync_file_fence);
>
> This would avoid the extra ops, maybe we should go this way.

NAK on both approaches. The fence array grabs another reference on 
itself for each callback it registers, so this isn't necessary:

> for (i = 0; i < array->num_fences; ++i) {
> cb[i].array = array;
> /*
>  * As we may report that the fence is signaled before all
>  * callbacks are complete, we need to take an additional
>  * reference count on the array so that we do not free 
> it too
>  * early. The core fence handling will only hold the 
> reference
>  * until we signal the array as complete (but that is now
>  * insufficient).
>  */
> fence_get(>base);
> if (fence_add_callback(array->fences[i], [i].cb,
>fence_array_cb_func)) {
> fence_put(>base);
> if (atomic_dec_and_test(>num_pending))
> return false;
> }
> }

So you can just use fence_remove_callback() and then fence_put() without 
worrying about the reference.

Regards,
Christian.

>
>   Gustavo

[RFC 0/5] rework fences on struct sync_file

[Christian König]

Am 24.06.2016 um 15:17 schrieb Gustavo Padovan:
> Hi Christian,
>
> 2016-06-24 Christian König :
>
>> Am 23.06.2016 um 17:29 schrieb Gustavo Padovan:
>>> From: Gustavo Padovan 
>>>
>>> Hi all,
>>>
>>> This is an attempt to improve fence support on Sync File. The basic idea
>>> is to have only sync_file->fence and store all fences there, either as
>>> normal fences or fence_arrays. That way we can remove some potential
>>> duplication when using fence_array with sync_file: the duplication of the 
>>> array
>>> of fences and the duplication of fence_add_callback() for all fences.
>>>
>>> Now when creating a new sync_file during the merge process 
>>> sync_file_set_fence()
>>> will set sync_file->fence based on the number of fences for that sync_file. 
>>> If
>>> there is more than one fence a fence_array is created. One important 
>>> advantage
>>> approach is that we only add one fence callback now, no matter how many 
>>> fences
>>> there are in a sync_file - the individual callbacks are added by 
>>> fence_array.
>>>
>>> Two fence ops had to be created to help abstract the difference between 
>>> handling
>>> fences and fences_arrays: .teardown() and .get_fences(). The former run 
>>> needed
>>> on fence_array, and the latter just return a copy of all fences in the 
>>> fence.
>>> I'm not so sure about adding those two, speacially .get_fences(). What do 
>>> you
>>> think?
>> Clearly not a good idea to add this a fence ops, cause those are specialized
>> functions for only a certain fence implementation (the fence_array).
> Are you refering only to .get_fences()?

That comment was only for the get_fences() operation, but the teardown() 
callback looks very suspicious to me as well.

Can you explain once more why that should be necessary?

Regards,
Christian.

>
>> What you should do is try to cast the fence in your sync file using
>> to_fence_array() and then you can access the fences in the array.
> Yes, that seems a better idea I think. The initial idea was to abstract
> the difference as much as possible, but it doesn't seem really worth
> for .get_fences().
>
>   Gustavo
> ___
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

[RFC 0/5] rework fences on struct sync_file

[Gustavo Padovan]

2016-06-24 Christian König :

> Am 24.06.2016 um 16:59 schrieb Gustavo Padovan:
> > 2016-06-24 Christian König :
> > 
> > > Am 24.06.2016 um 15:17 schrieb Gustavo Padovan:
> > > > Hi Christian,
> > > > 
> > > > 2016-06-24 Christian König :
> > > > 
> > > > > Am 23.06.2016 um 17:29 schrieb Gustavo Padovan:
> > > > > > From: Gustavo Padovan 
> > > > > > 
> > > > > > Hi all,
> > > > > > 
> > > > > > This is an attempt to improve fence support on Sync File. The basic 
> > > > > > idea
> > > > > > is to have only sync_file->fence and store all fences there, either 
> > > > > > as
> > > > > > normal fences or fence_arrays. That way we can remove some potential
> > > > > > duplication when using fence_array with sync_file: the duplication 
> > > > > > of the array
> > > > > > of fences and the duplication of fence_add_callback() for all 
> > > > > > fences.
> > > > > > 
> > > > > > Now when creating a new sync_file during the merge process 
> > > > > > sync_file_set_fence()
> > > > > > will set sync_file->fence based on the number of fences for that 
> > > > > > sync_file. If
> > > > > > there is more than one fence a fence_array is created. One 
> > > > > > important advantage
> > > > > > approach is that we only add one fence callback now, no matter how 
> > > > > > many fences
> > > > > > there are in a sync_file - the individual callbacks are added by 
> > > > > > fence_array.
> > > > > > 
> > > > > > Two fence ops had to be created to help abstract the difference 
> > > > > > between handling
> > > > > > fences and fences_arrays: .teardown() and .get_fences(). The former 
> > > > > > run needed
> > > > > > on fence_array, and the latter just return a copy of all fences in 
> > > > > > the fence.
> > > > > > I'm not so sure about adding those two, speacially .get_fences(). 
> > > > > > What do you
> > > > > > think?
> > > > > Clearly not a good idea to add this a fence ops, cause those are 
> > > > > specialized
> > > > > functions for only a certain fence implementation (the fence_array).
> > > > Are you refering only to .get_fences()?
> > > That comment was only for the get_fences() operation, but the teardown()
> > > callback looks very suspicious to me as well.
> > > 
> > > Can you explain once more why that should be necessary?
> > When the sync_file owner exits we need to clean up it and that means 
> > releasing
> > the fence too, however with fence_array we can't just call fence_put()
> > as a extra reference to array->base for each fence is held when enabling
> > signalling. Thus we need a prior step, that I called teardown(), to
> > remove the callback for not signaled fences and put the extra
> > references.
> > 
> > Another way to do this would be:
> > 
> > if (fence_is_array(sync_file->fence))
> > fence_array_destroy(to_fence_array(sync_file->fence));
> > else
> > fence_put(sync_file_fence);
> > 
> > This would avoid the extra ops, maybe we should go this way.
> 
> NAK on both approaches. The fence array grabs another reference on itself
> for each callback it registers, so this isn't necessary:
> 
> > for (i = 0; i < array->num_fences; ++i) {
> > cb[i].array = array;
> > /*
> >  * As we may report that the fence is signaled before all
> >  * callbacks are complete, we need to take an additional
> >  * reference count on the array so that we do not free
> > it too
> >  * early. The core fence handling will only hold the
> > reference
> >  * until we signal the array as complete (but that is now
> >  * insufficient).
> >  */
> > fence_get(>base);
> > if (fence_add_callback(array->fences[i], [i].cb,
> >fence_array_cb_func)) {
> > fence_put(>base);
> > if (atomic_dec_and_test(>num_pending))
> > return false;
> > }
> > }
> 
> So you can just use fence_remove_callback() and then fence_put() without
> worrying about the reference.

Yes. That is what I have in mind for fence_array_destroy() in the
snippet of code in the last e-mail. That plus the last fence_put() to
release the fence_array().

Gustavo

[RFC 0/5] rework fences on struct sync_file

[Gustavo Padovan]

2016-06-24 Christian König :

> Am 24.06.2016 um 15:17 schrieb Gustavo Padovan:
> > Hi Christian,
> > 
> > 2016-06-24 Christian König :
> > 
> > > Am 23.06.2016 um 17:29 schrieb Gustavo Padovan:
> > > > From: Gustavo Padovan 
> > > > 
> > > > Hi all,
> > > > 
> > > > This is an attempt to improve fence support on Sync File. The basic idea
> > > > is to have only sync_file->fence and store all fences there, either as
> > > > normal fences or fence_arrays. That way we can remove some potential
> > > > duplication when using fence_array with sync_file: the duplication of 
> > > > the array
> > > > of fences and the duplication of fence_add_callback() for all fences.
> > > > 
> > > > Now when creating a new sync_file during the merge process 
> > > > sync_file_set_fence()
> > > > will set sync_file->fence based on the number of fences for that 
> > > > sync_file. If
> > > > there is more than one fence a fence_array is created. One important 
> > > > advantage
> > > > approach is that we only add one fence callback now, no matter how many 
> > > > fences
> > > > there are in a sync_file - the individual callbacks are added by 
> > > > fence_array.
> > > > 
> > > > Two fence ops had to be created to help abstract the difference between 
> > > > handling
> > > > fences and fences_arrays: .teardown() and .get_fences(). The former run 
> > > > needed
> > > > on fence_array, and the latter just return a copy of all fences in the 
> > > > fence.
> > > > I'm not so sure about adding those two, speacially .get_fences(). What 
> > > > do you
> > > > think?
> > > Clearly not a good idea to add this a fence ops, cause those are 
> > > specialized
> > > functions for only a certain fence implementation (the fence_array).
> > Are you refering only to .get_fences()?
> 
> That comment was only for the get_fences() operation, but the teardown()
> callback looks very suspicious to me as well.
> 
> Can you explain once more why that should be necessary?

When the sync_file owner exits we need to clean up it and that means releasing
the fence too, however with fence_array we can't just call fence_put()
as a extra reference to array->base for each fence is held when enabling
signalling. Thus we need a prior step, that I called teardown(), to
remove the callback for not signaled fences and put the extra
references.

Another way to do this would be:

if (fence_is_array(sync_file->fence))
fence_array_destroy(to_fence_array(sync_file->fence));
else
fence_put(sync_file_fence);

This would avoid the extra ops, maybe we should go this way.

Gustavo

[RFC 0/5] rework fences on struct sync_file

[Christian König]

Am 23.06.2016 um 17:29 schrieb Gustavo Padovan:
> From: Gustavo Padovan 
>
> Hi all,
>
> This is an attempt to improve fence support on Sync File. The basic idea
> is to have only sync_file->fence and store all fences there, either as
> normal fences or fence_arrays. That way we can remove some potential
> duplication when using fence_array with sync_file: the duplication of the 
> array
> of fences and the duplication of fence_add_callback() for all fences.
>
> Now when creating a new sync_file during the merge process 
> sync_file_set_fence()
> will set sync_file->fence based on the number of fences for that sync_file. If
> there is more than one fence a fence_array is created. One important advantage
> approach is that we only add one fence callback now, no matter how many fences
> there are in a sync_file - the individual callbacks are added by fence_array.
>
> Two fence ops had to be created to help abstract the difference between 
> handling
> fences and fences_arrays: .teardown() and .get_fences(). The former run needed
> on fence_array, and the latter just return a copy of all fences in the fence.
> I'm not so sure about adding those two, speacially .get_fences(). What do you
> think?

Clearly not a good idea to add this a fence ops, cause those are 
specialized functions for only a certain fence implementation (the 
fence_array).

What you should do is try to cast the fence in your sync file using 
to_fence_array() and then you can access the fences in the array.

Regards,
Christian.

>
> Please comment! Thanks.
>
>   Gustavo
> ---
>
> Gustavo Padovan (5):
>dma-buf/fence: add .teardown() ops
>dma-buf/fence-array: add fence_array_teardown()
>dma-buf/fence: add .get_fences() ops
>dma-buf/fence-array: add fence_array_get_fences()
>dma-buf/sync_file: rework fence storage in struct file
>
>   drivers/dma-buf/fence-array.c|  30 
>   drivers/dma-buf/fence.c  |  21 ++
>   drivers/dma-buf/sync_file.c  | 129 
> +--
>   drivers/staging/android/sync_debug.c |   5 +-
>   include/linux/fence.h|  10 +++
>   include/linux/sync_file.h|  12 ++--
>   6 files changed, 161 insertions(+), 46 deletions(-)
>

[RFC 0/5] rework fences on struct sync_file

[Gustavo Padovan]

Hi Christian,

2016-06-24 Christian König :

> Am 23.06.2016 um 17:29 schrieb Gustavo Padovan:
> > From: Gustavo Padovan 
> > 
> > Hi all,
> > 
> > This is an attempt to improve fence support on Sync File. The basic idea
> > is to have only sync_file->fence and store all fences there, either as
> > normal fences or fence_arrays. That way we can remove some potential
> > duplication when using fence_array with sync_file: the duplication of the 
> > array
> > of fences and the duplication of fence_add_callback() for all fences.
> > 
> > Now when creating a new sync_file during the merge process 
> > sync_file_set_fence()
> > will set sync_file->fence based on the number of fences for that sync_file. 
> > If
> > there is more than one fence a fence_array is created. One important 
> > advantage
> > approach is that we only add one fence callback now, no matter how many 
> > fences
> > there are in a sync_file - the individual callbacks are added by 
> > fence_array.
> > 
> > Two fence ops had to be created to help abstract the difference between 
> > handling
> > fences and fences_arrays: .teardown() and .get_fences(). The former run 
> > needed
> > on fence_array, and the latter just return a copy of all fences in the 
> > fence.
> > I'm not so sure about adding those two, speacially .get_fences(). What do 
> > you
> > think?
> 
> Clearly not a good idea to add this a fence ops, cause those are specialized
> functions for only a certain fence implementation (the fence_array).

Are you refering only to .get_fences()?

> 
> What you should do is try to cast the fence in your sync file using
> to_fence_array() and then you can access the fences in the array.

Yes, that seems a better idea I think. The initial idea was to abstract 
the difference as much as possible, but it doesn't seem really worth
for .get_fences().

Gustavo

[RFC 0/5] rework fences on struct sync_file

[Gustavo Padovan]

From: Gustavo Padovan 

Hi all,

This is an attempt to improve fence support on Sync File. The basic idea
is to have only sync_file->fence and store all fences there, either as
normal fences or fence_arrays. That way we can remove some potential
duplication when using fence_array with sync_file: the duplication of the array
of fences and the duplication of fence_add_callback() for all fences. 

Now when creating a new sync_file during the merge process sync_file_set_fence()
will set sync_file->fence based on the number of fences for that sync_file. If
there is more than one fence a fence_array is created. One important advantage
approach is that we only add one fence callback now, no matter how many fences
there are in a sync_file - the individual callbacks are added by fence_array.

Two fence ops had to be created to help abstract the difference between handling
fences and fences_arrays: .teardown() and .get_fences(). The former run needed
on fence_array, and the latter just return a copy of all fences in the fence.
I'm not so sure about adding those two, speacially .get_fences(). What do you
think?

Please comment! Thanks.

Gustavo
---

Gustavo Padovan (5):
  dma-buf/fence: add .teardown() ops
  dma-buf/fence-array: add fence_array_teardown()
  dma-buf/fence: add .get_fences() ops
  dma-buf/fence-array: add fence_array_get_fences()
  dma-buf/sync_file: rework fence storage in struct file

 drivers/dma-buf/fence-array.c|  30 
 drivers/dma-buf/fence.c  |  21 ++
 drivers/dma-buf/sync_file.c  | 129 +--
 drivers/staging/android/sync_debug.c |   5 +-
 include/linux/fence.h|  10 +++
 include/linux/sync_file.h|  12 ++--
 6 files changed, 161 insertions(+), 46 deletions(-)

-- 
2.5.5