Re: [EXTERNAL] Re: [PATCH] sync_file: Return reasonable timestamp when merging signaled fences
On Thu, May 09, 2019 at 12:46:05PM +0100, Chris Wilson wrote: > Quoting Michael Yang (2019-05-09 05:34:11) > > If all the sync points were signaled in both fences a and b, > > there was only one sync point in merged fence which is a_fence[0]. > > The Fence structure in android framework might be confused about > > timestamp if there were any sync points which were signaled after > > a_fence[0]. It might be more reasonable to use timestamp of last signaled > > sync point to represent the merged fence. > > The issue can be found from EGL extension ANDROID_get_frame_timestamps. > > Sometimes the return value of EGL_READS_DONE_TIME_ANDROID is head of > > the return value of EGL_RENDERING_COMPLETE_TIME_ANDROID. > > That means display/composition had been completed before rendering > > was completed that is incorrect. > > > > Some discussion can be found at: > > https://urldefense.proofpoint.com/v2/url?u=https-3A__android-2Dreview.googlesource.com_c_kernel_common_-2B_907009=DwIFaQ=bq9ppmgvSw3oQFfR871D_w=Ngg6vhouPkgwSIbDMU7rDN0ZfT2Qax50xuWkXXqQ3zw=N9R9dXGJ3zk0e0gXNM4tsiro7xCOLlWx6c3HAEseSSw=6sY2t9i2wvylWH-rPUlvY1MIuWKjCPzT8SeCgpZOIGk= > > > > > > Signed-off-by: Michael Yang > > --- > > Hi, > > I didn't get response since I previously sent this a month ago. > > Could someone have a chance to look at it please? > > Thanks. > > drivers/dma-buf/sync_file.c | 25 +++-- > > 1 file changed, 23 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c > > index 4f6305c..d46bfe1 100644 > > --- a/drivers/dma-buf/sync_file.c > > +++ b/drivers/dma-buf/sync_file.c > > @@ -274,8 +274,29 @@ static struct sync_file *sync_file_merge(const char > > *name, struct sync_file *a, > > for (; i_b < b_num_fences; i_b++) > > add_fence(fences, , b_fences[i_b]); > > > > - if (i == 0) > > - fences[i++] = dma_fence_get(a_fences[0]); > > + /* If all the sync pts were signaled, then adding the sync_pt who > > +* was the last signaled to the fence. > > +*/ > > + if (i == 0) { > > + struct dma_fence *last_signaled_sync_pt = a_fences[0]; > > + int iter; > > + > > + for (iter = 1; iter < a_num_fences; iter++) { > > If there is more than one fence, sync_file->fence is a fence_array and > its timestamp is what you want. If there is one fence, sync_file->fence > is a pointer to that fence, and naturally has the right timestamp. > > In short, this should be handled by dma_fence_array_create() when given > a complete set of signaled fences, it too should inherit the signaled > status with the timestamp being taken from the last fence. It should > also be careful to inherit the error status. > -Chris Thanks Chris for the inputs. For this case, there will be only one fence in sync_file->fence after doing sync_file_merge(). Regarding to the current implementation, dma_fence_array_create() is not called as num_fences is equal to 1. I was wondering do you suggest that we pass a complete set of signaled fences to sync_file_set_fence() and handle it in dma_fence_array_create(). Thanks. - Michael
[PATCH] sync_file: Return reasonable timestamp when merging signaled fences
If all the sync points were signaled in both fences a and b, there was only one sync point in merged fence which is a_fence[0]. The Fence structure in android framework might be confused about timestamp if there were any sync points which were signaled after a_fence[0]. It might be more reasonable to use timestamp of last signaled sync point to represent the merged fence. The issue can be found from EGL extension ANDROID_get_frame_timestamps. Sometimes the return value of EGL_READS_DONE_TIME_ANDROID is head of the return value of EGL_RENDERING_COMPLETE_TIME_ANDROID. That means display/composition had been completed before rendering was completed that is incorrect. Some discussion can be found at: https://android-review.googlesource.com/c/kernel/common/+/907009 Signed-off-by: Michael Yang --- Hi, I didn't get response since I previously sent this a month ago. Could someone have a chance to look at it please? Thanks. drivers/dma-buf/sync_file.c | 25 +++-- 1 file changed, 23 insertions(+), 2 deletions(-) diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c index 4f6305c..d46bfe1 100644 --- a/drivers/dma-buf/sync_file.c +++ b/drivers/dma-buf/sync_file.c @@ -274,8 +274,29 @@ static struct sync_file *sync_file_merge(const char *name, struct sync_file *a, for (; i_b < b_num_fences; i_b++) add_fence(fences, , b_fences[i_b]); - if (i == 0) - fences[i++] = dma_fence_get(a_fences[0]); + /* If all the sync pts were signaled, then adding the sync_pt who +* was the last signaled to the fence. +*/ + if (i == 0) { + struct dma_fence *last_signaled_sync_pt = a_fences[0]; + int iter; + + for (iter = 1; iter < a_num_fences; iter++) { + if (ktime_compare(last_signaled_sync_pt->timestamp, + a_fences[iter]->timestamp) < 0) { + last_signaled_sync_pt = a_fences[iter]; + } + } + + for (iter = 0; iter < b_num_fences; iter++) { + if (ktime_compare(last_signaled_sync_pt->timestamp, + b_fences[iter]->timestamp) < 0) { + last_signaled_sync_pt = b_fences[iter]; + } + } + + fences[i++] = dma_fence_get(last_signaled_sync_pt); + } if (num_fences > i) { nfences = krealloc(fences, i * sizeof(*fences), -- 2.7.4 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] sync_file: Return reasonable timestamp when merging signaled fences
If all the sync points were signaled in both fences a and b, there was only one sync point in merged fence which is a_fence[0]. The Fence structure in android framework might be confused about timestamp if there were any sync points which were signaled after a_fence[0]. It might be more reasonable to use timestamp of last signaled sync point to represent the merged fence. The issue can be found from EGL extension ANDROID_get_frame_timestamps. Sometimes the return value of EGL_READS_DONE_TIME_ANDROID is head of the return value of EGL_RENDERING_COMPLETE_TIME_ANDROID. That means display/composition had been completed before rendering was completed that is incorrect. Some discussion can be found at: https://android-review.googlesource.com/c/kernel/common/+/907009 Signed-off-by: Michael Yang --- drivers/dma-buf/sync_file.c | 25 +++-- 1 file changed, 23 insertions(+), 2 deletions(-) diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c index 4f6305c..d46bfe1 100644 --- a/drivers/dma-buf/sync_file.c +++ b/drivers/dma-buf/sync_file.c @@ -274,8 +274,29 @@ static struct sync_file *sync_file_merge(const char *name, struct sync_file *a, for (; i_b < b_num_fences; i_b++) add_fence(fences, , b_fences[i_b]); - if (i == 0) - fences[i++] = dma_fence_get(a_fences[0]); + /* If all the sync pts were signaled, then adding the sync_pt who +* was the last signaled to the fence. +*/ + if (i == 0) { + struct dma_fence *last_signaled_sync_pt = a_fences[0]; + int iter; + + for (iter = 1; iter < a_num_fences; iter++) { + if (ktime_compare(last_signaled_sync_pt->timestamp, + a_fences[iter]->timestamp) < 0) { + last_signaled_sync_pt = a_fences[iter]; + } + } + + for (iter = 0; iter < b_num_fences; iter++) { + if (ktime_compare(last_signaled_sync_pt->timestamp, + b_fences[iter]->timestamp) < 0) { + last_signaled_sync_pt = b_fences[iter]; + } + } + + fences[i++] = dma_fence_get(last_signaled_sync_pt); + } if (num_fences > i) { nfences = krealloc(fences, i * sizeof(*fences), -- 2.7.4 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel