On 2019年04月01日 16:19, Lionel Landwerlin wrote:
On 01/04/2019 06:54, Zhou, David(ChunMing) wrote:
-----Original Message-----
From: Lionel Landwerlin <lionel.g.landwer...@intel.com>
Sent: Saturday, March 30, 2019 10:09 PM
To: Koenig, Christian <christian.koe...@amd.com>; Zhou, David(ChunMing)
<david1.z...@amd.com>; dri-de...@lists.freedesktop.org; amd-
g...@lists.freedesktop.org; ja...@jlekstrand.net; Hector, Tobias
<tobias.hec...@amd.com>
Subject: Re: [PATCH 2/9] drm/syncobj: add new drm_syncobj_add_point
interface v4
On 28/03/2019 15:18, Christian König wrote:
Am 28.03.19 um 14:50 schrieb Lionel Landwerlin:
On 25/03/2019 08:32, Chunming Zhou wrote:
From: Christian König <ckoenig.leichtzumer...@gmail.com>
Use the dma_fence_chain object to create a timeline of fence objects
instead of just replacing the existing fence.
v2: rebase and cleanup
v3: fix garbage collection parameters
v4: add unorder point check, print a warn calltrace
Signed-off-by: Christian König <christian.koe...@amd.com>
Cc: Lionel Landwerlin <lionel.g.landwer...@intel.com>
---
drivers/gpu/drm/drm_syncobj.c | 39
+++++++++++++++++++++++++++++++++++
include/drm/drm_syncobj.h | 5 +++++
2 files changed, 44 insertions(+)
diff --git a/drivers/gpu/drm/drm_syncobj.c
b/drivers/gpu/drm/drm_syncobj.c index 5329e66598c6..19a9ce638119
100644
--- a/drivers/gpu/drm/drm_syncobj.c
+++ b/drivers/gpu/drm/drm_syncobj.c
@@ -122,6 +122,45 @@ static void drm_syncobj_remove_wait(struct
drm_syncobj *syncobj,
spin_unlock(&syncobj->lock);
}
+/**
+ * drm_syncobj_add_point - add new timeline point to the syncobj
+ * @syncobj: sync object to add timeline point do
+ * @chain: chain node to use to add the point
+ * @fence: fence to encapsulate in the chain node
+ * @point: sequence number to use for the point
+ *
+ * Add the chain node as new timeline point to the syncobj.
+ */
+void drm_syncobj_add_point(struct drm_syncobj *syncobj,
+ struct dma_fence_chain *chain,
+ struct dma_fence *fence,
+ uint64_t point)
+{
+ struct syncobj_wait_entry *cur, *tmp;
+ struct dma_fence *prev;
+
+ dma_fence_get(fence);
+
+ spin_lock(&syncobj->lock);
+
+ prev = drm_syncobj_fence_get(syncobj);
+ /* You are adding an unorder point to timeline, which could
cause payload returned from query_ioctl is 0! */
+ WARN_ON_ONCE(prev && prev->seqno >= point);
I think the WARN/BUG macros should only fire when there is an issue
with programming from within the kernel.
But this particular warning can be triggered by an application.
Probably best to just remove it?
Yeah, that was also my argument against it.
Key point here is that we still want to note somehow that userspace
did something wrong and returning an error is not an option.
Maybe just use DRM_ERROR with a static variable to print the message
only once.
Christian.
I don't really see any point in printing an error once. If you run your
application twice you end up thinking there was an issue just on the
first run
but it's actually always wrong.
Except this nitpick, is there any other concern to push whole patch
set? Is that time to push whole patch set?
-David
Looks good to me.
Does that mean we can add your RB on patch set so that we can submit the
patch set to drm-misc-next branch?
I have an additional change to make drm_syncobj_find_fence() also
return the drm_syncobj :
https://github.com/djdeath/linux/commit/0b7732b267b931339d71fe6f493ea6fa4eab453e
This is needed in i915 to avoid looking up the drm_syncobj handle twice.
Our driver allows to wait on the syncobj's dma_fence that we're then
going to replace so we need to get bot the fence & syncobj at the same
time.
I guess it can go in a follow up series.
Yes, agree.
Thanks for your effort as well,
-David
-Lionel
Unless we're willing to take the syncobj lock for longer periods of
time when
adding points, I guess we'll have to defer validation to validation
layers.
-Lionel
-Lionel
+ dma_fence_chain_init(chain, prev, fence, point);
+ rcu_assign_pointer(syncobj->fence, &chain->base);
+
+ list_for_each_entry_safe(cur, tmp, &syncobj->cb_list, node) {
+ list_del_init(&cur->node);
+ syncobj_wait_syncobj_func(syncobj, cur);
+ }
+ spin_unlock(&syncobj->lock);
+
+ /* Walk the chain once to trigger garbage collection */
+ dma_fence_chain_for_each(fence, prev);
+ dma_fence_put(prev);
+}
+EXPORT_SYMBOL(drm_syncobj_add_point);
+
/**
* drm_syncobj_replace_fence - replace fence in a sync object.
* @syncobj: Sync object to replace fence in diff --git
a/include/drm/drm_syncobj.h b/include/drm/drm_syncobj.h index
0311c9fdbd2f..6cf7243a1dc5 100644
--- a/include/drm/drm_syncobj.h
+++ b/include/drm/drm_syncobj.h
@@ -27,6 +27,7 @@
#define __DRM_SYNCOBJ_H__
#include <linux/dma-fence.h>
+#include <linux/dma-fence-chain.h>
struct drm_file;
@@ -112,6 +113,10 @@ drm_syncobj_fence_get(struct drm_syncobj
*syncobj)
struct drm_syncobj *drm_syncobj_find(struct drm_file
*file_private,
u32 handle);
+void drm_syncobj_add_point(struct drm_syncobj *syncobj,
+ struct dma_fence_chain *chain,
+ struct dma_fence *fence,
+ uint64_t point);
void drm_syncobj_replace_fence(struct drm_syncobj *syncobj,
struct dma_fence *fence);
int drm_syncobj_find_fence(struct drm_file *file_private,
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
