LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] sync_file: Return reasonable timestamp when merging signaled fences
@ 2019-04-08  8:01 Michael Yang
  2019-05-09  4:34 ` Michael Yang
  0 siblings, 1 reply; 5+ messages in thread
From: Michael Yang @ 2019-04-08  8:01 UTC (permalink / raw)
  To: sumit.semwal
  Cc: dri-devel, linux-kernel, gustavo, linux-media, linaro-mm-sig,
	michael.yang

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 <michael.yang@imgtec.com>
---
 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, &i, 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


^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH] sync_file: Return reasonable timestamp when merging signaled fences
  2019-04-08  8:01 [PATCH] sync_file: Return reasonable timestamp when merging signaled fences Michael Yang
@ 2019-05-09  4:34 ` Michael Yang
  2019-05-09 11:46   ` Chris Wilson
  0 siblings, 1 reply; 5+ messages in thread
From: Michael Yang @ 2019-05-09  4:34 UTC (permalink / raw)
  To: sumit.semwal
  Cc: dri-devel, linux-kernel, gustavo, linux-media, linaro-mm-sig,
	michael.yang, gregkh

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 <michael.yang@imgtec.com>
---
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, &i, 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


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] sync_file: Return reasonable timestamp when merging signaled fences
  2019-05-09  4:34 ` Michael Yang
@ 2019-05-09 11:46   ` Chris Wilson
  2019-05-14  7:55     ` [EXTERNAL] " Michael Yang
  0 siblings, 1 reply; 5+ messages in thread
From: Chris Wilson @ 2019-05-09 11:46 UTC (permalink / raw)
  To: Michael Yang, sumit.semwal
  Cc: dri-devel, linux-kernel, gustavo, linux-media, linaro-mm-sig,
	michael.yang, gregkh

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://android-review.googlesource.com/c/kernel/common/+/907009
> 
> Signed-off-by: Michael Yang <michael.yang@imgtec.com>
> ---
> 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, &i, 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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [EXTERNAL] Re: [PATCH] sync_file: Return reasonable timestamp when merging signaled fences
  2019-05-09 11:46   ` Chris Wilson
@ 2019-05-14  7:55     ` Michael Yang
  2019-05-14  8:06       ` Chris Wilson
  0 siblings, 1 reply; 5+ messages in thread
From: Michael Yang @ 2019-05-14  7:55 UTC (permalink / raw)
  To: Chris Wilson
  Cc: sumit.semwal, dri-devel, linux-kernel, gustavo, linux-media,
	linaro-mm-sig, gregkh

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&d=DwIFaQ&c=bq9ppmgvSw3oQFfR871D_w&r=Ngg6vhouPkgwSIbDMU7rDN0ZfT2Qax50xuWkXXqQ3zw&m=N9R9dXGJ3zk0e0gXNM4tsiro7xCOLlWx6c3HAEseSSw&s=6sY2t9i2wvylWH-rPUlvY1MIuWKjCPzT8SeCgpZOIGk&e= 
> > 
> > Signed-off-by: Michael Yang <michael.yang@imgtec.com>
> > ---
> > 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, &i, 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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [EXTERNAL] Re: [PATCH] sync_file: Return reasonable timestamp when merging signaled fences
  2019-05-14  7:55     ` [EXTERNAL] " Michael Yang
@ 2019-05-14  8:06       ` Chris Wilson
  0 siblings, 0 replies; 5+ messages in thread
From: Chris Wilson @ 2019-05-14  8:06 UTC (permalink / raw)
  To: Michael Yang
  Cc: sumit.semwal, dri-devel, linux-kernel, gustavo, linux-media,
	linaro-mm-sig, gregkh

Quoting Michael Yang (2019-05-14 08:55:37)
> 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&d=DwIFaQ&c=bq9ppmgvSw3oQFfR871D_w&r=Ngg6vhouPkgwSIbDMU7rDN0ZfT2Qax50xuWkXXqQ3zw&m=N9R9dXGJ3zk0e0gXNM4tsiro7xCOLlWx6c3HAEseSSw&s=6sY2t9i2wvylWH-rPUlvY1MIuWKjCPzT8SeCgpZOIGk&e= 
> > > 
> > > Signed-off-by: Michael Yang <michael.yang@imgtec.com>
> > > ---
> > > 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, &i, 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.

No, in the case there is only one fence, we just inherit its timestamp
along with its fence status. (A single fence is the degenerate case of
a fence array.)
-Chris

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2019-05-14  8:23 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-04-08  8:01 [PATCH] sync_file: Return reasonable timestamp when merging signaled fences Michael Yang
2019-05-09  4:34 ` Michael Yang
2019-05-09 11:46   ` Chris Wilson
2019-05-14  7:55     ` [EXTERNAL] " Michael Yang
2019-05-14  8:06       ` Chris Wilson

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).