On 2023-06-23 05:08, Christian König wrote:
> Some Android CTS is testing for that.
>
It's not entirely clear what "that" is, other than by the subject title
of the patch. Something like "Record and return the signalling time of
merged fences, as well as regular fences, since some Android CTS(?)
is testing for this time." would be very helpful as a commit description.
> v2: use the current time if the fence is still in the signaling path and
> the timestamp not yet available.
>
> Signed-off-by: Christian König <christian.koenig@xxxxxxx>
> ---
> drivers/dma-buf/dma-fence-unwrap.c | 20 +++++++++++++++++---
> drivers/dma-buf/dma-fence.c | 5 +++--
> drivers/gpu/drm/drm_syncobj.c | 2 +-
> include/linux/dma-fence.h | 2 +-
> 4 files changed, 22 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/dma-buf/dma-fence-unwrap.c b/drivers/dma-buf/dma-fence-unwrap.c
> index 7002bca792ff..46c2d3a474cd 100644
> --- a/drivers/dma-buf/dma-fence-unwrap.c
> +++ b/drivers/dma-buf/dma-fence-unwrap.c
> @@ -66,18 +66,32 @@ struct dma_fence *__dma_fence_unwrap_merge(unsigned int num_fences,
> {
> struct dma_fence_array *result;
> struct dma_fence *tmp, **array;
> + ktime_t timestamp;
> unsigned int i;
> size_t count;
>
> count = 0;
> + timestamp = ns_to_ktime(0);
> for (i = 0; i < num_fences; ++i) {
> - dma_fence_unwrap_for_each(tmp, &iter[i], fences[i])
> - if (!dma_fence_is_signaled(tmp))
> + dma_fence_unwrap_for_each(tmp, &iter[i], fences[i]) {
> + if (!dma_fence_is_signaled(tmp)) {
> ++count;
> + } else if (test_bit(DMA_FENCE_FLAG_TIMESTAMP_BIT,
> + &tmp->flags)) {
> + if (ktime_after(tmp->timestamp, timestamp))
> + timestamp = tmp->timestamp;
> + } else {
> + /*
> + * Use the current time if the fence is
> + * currently signaling.
> + */
> + timestamp = ktime_get();
> + }
> + }
> }
>
> if (count == 0)
Just before this "if" I would've added a comment to describe
what exactly is being checked and happening, since working out
the negated if (!dma_fence_is_signaled(tmp)) ++count; is a bit
cryptic. Perhaps something like,
/* If all fences have signalled, then return a private
* signalled fence.
*/
if (count == 0)
And I can see that "count" is being reused down below in the logic
of this function, but perhaps using another variable named "signalled",
and then counting positive conditional, and then comparing,
/* If all fences have signalled, then return a private
* signalled fence.
*/
if (signalled == num_fences)
...
Would've made the code clearer.
The compiler would optimize the use of a second variable to
the same register anyway.
A moot point now perhaps, but we should keep note for future submissions.
--
Regards,
Luben
