Re: [PATCH 2/9] drm/syncobj: add new drm_syncobj_add_point interface v4
On Mon, Apr 15, 2019 at 8:56 PM Koenig, Christian wrote: > > Am 15.04.19 um 20:54 schrieb Dave Airlie: > >> Well, I've got commit rights as well. > >> > >> Going to remove the warning, add your rb and push everything if nobody > >> objects in the next hour or so. > > So this got committed and is in my -next tree, but where is the > > userspace and igt tests? > > I was waiting for this to land in -next before pushing the libdrm > changes with amdgpu test cases. > > As soon as those are committed Lionel and David wanted to commit the igt > tests. Testcases are ofc great, but that's not the userspace we're looking for :-) All still khr embargo? Even if that's all available at under khr nda, and reviewed by relevant folks I'm not sure we're setting a great precedence here - if khr still wants to be able to change the interface spec, casting the kernel uapi into stone is a bit premature. Aside from that this excludes everyone without khr nda, which isn't a nice move really. Note: Picky because this is winsys stuff, so relevant beyond just a single driver - for driver/hw specific stuff it makes sense to allow plenty of leeway to work around various embargos. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 2/9] drm/syncobj: add new drm_syncobj_add_point interface v4
Am 15.04.19 um 20:54 schrieb Dave Airlie: >> Well, I've got commit rights as well. >> >> Going to remove the warning, add your rb and push everything if nobody >> objects in the next hour or so. > So this got committed and is in my -next tree, but where is the > userspace and igt tests? I was waiting for this to land in -next before pushing the libdrm changes with amdgpu test cases. As soon as those are committed Lionel and David wanted to commit the igt tests. Christian. > > There needs to be a functional mesa userspace and a set of IGTs for > this, maybe I've overlooked them if I haven't we can't ship this. > > Dave. ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 2/9] drm/syncobj: add new drm_syncobj_add_point interface v4
> Well, I've got commit rights as well. > > Going to remove the warning, add your rb and push everything if nobody > objects in the next hour or so. So this got committed and is in my -next tree, but where is the userspace and igt tests? There needs to be a functional mesa userspace and a set of IGTs for this, maybe I've overlooked them if I haven't we can't ship this. Dave. ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH 2/9] drm/syncobj: add new drm_syncobj_add_point interface v4
From: Christian König 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 Cc: Lionel Landwerlin Reviewed-by: Lionel Landwerlin --- drivers/gpu/drm/drm_syncobj.c | 40 +++ include/drm/drm_syncobj.h | 5 + 2 files changed, 45 insertions(+) diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c index 5329e66598c6..dbe4a1c75fbc 100644 --- a/drivers/gpu/drm/drm_syncobj.c +++ b/drivers/gpu/drm/drm_syncobj.c @@ -122,6 +122,46 @@ static void drm_syncobj_remove_wait(struct drm_syncobj *syncobj, spin_unlock(>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(>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! */ + if (prev && prev->seqno >= point) + DRM_ERROR("You are adding an unorder point to timeline!\n"); + dma_fence_chain_init(chain, prev, fence, point); + rcu_assign_pointer(syncobj->fence, >base); + + list_for_each_entry_safe(cur, tmp, >cb_list, node) { + list_del_init(>node); + syncobj_wait_syncobj_func(syncobj, cur); + } + spin_unlock(>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 +#include 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, -- 2.17.1 ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 2/9] drm/syncobj: add new drm_syncobj_add_point interface v4
Am 01.04.19 um 11:05 schrieb Lionel Landwerlin: > On 01/04/2019 11:50, zhoucm1 wrote: >> >> >> On 2019年04月01日 16:19, Lionel Landwerlin wrote: >>> On 01/04/2019 06:54, Zhou, David(ChunMing) wrote: >>>> >>>>> -Original Message- >>>>> From: Lionel Landwerlin >>>>> Sent: Saturday, March 30, 2019 10:09 PM >>>>> To: Koenig, Christian ; Zhou, >>>>> David(ChunMing) >>>>> ; dri-de...@lists.freedesktop.org; amd- >>>>> g...@lists.freedesktop.org; ja...@jlekstrand.net; Hector, Tobias >>>>> >>>>> 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 >>>>>>>> >>>>>>>> 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 >>>>>>>> Cc: Lionel Landwerlin >>>>>>>> --- >>>>>>>> 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(>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(>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 tha
Re: [PATCH 2/9] drm/syncobj: add new drm_syncobj_add_point interface v4
On 01/04/2019 11:50, zhoucm1 wrote: On 2019年04月01日 16:19, Lionel Landwerlin wrote: On 01/04/2019 06:54, Zhou, David(ChunMing) wrote: -Original Message- From: Lionel Landwerlin Sent: Saturday, March 30, 2019 10:09 PM To: Koenig, Christian ; Zhou, David(ChunMing) ; dri-de...@lists.freedesktop.org; amd- g...@lists.freedesktop.org; ja...@jlekstrand.net; Hector, Tobias 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 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 Cc: Lionel Landwerlin --- 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(>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(>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? Yes : Reviewed-by: Lionel Landwerlin Not too sure about the process for drm-misc-next. Sounds like Sumit Semwal is the go to person according to MAINTAINERS. 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 Thanks to you! -Lionel -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, >base); + + list_for_each_entry_safe(cur, tmp, >cb_list, node) { + list_del_init(>node); + syncobj_wait_syncobj_func(syncobj, cur); + } + spin_unlock(>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 +#include struct drm_file; @@ -112,6 +113,10 @@ drm_syncobj_fence_get(struct drm_sy
Re: [PATCH 2/9] drm/syncobj: add new drm_syncobj_add_point interface v4
On 2019年04月01日 16:19, Lionel Landwerlin wrote: On 01/04/2019 06:54, Zhou, David(ChunMing) wrote: -Original Message- From: Lionel Landwerlin Sent: Saturday, March 30, 2019 10:09 PM To: Koenig, Christian ; Zhou, David(ChunMing) ; dri-de...@lists.freedesktop.org; amd- g...@lists.freedesktop.org; ja...@jlekstrand.net; Hector, Tobias 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 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 Cc: Lionel Landwerlin --- 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(>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(>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, >base); + + list_for_each_entry_safe(cur, tmp, >cb_list, node) { + list_del_init(>node); + syncobj_wait_syncobj_func(syncobj, cur); + } + spin_unlock(>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 +#include 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,
Re: [PATCH 2/9] drm/syncobj: add new drm_syncobj_add_point interface v4
On 01/04/2019 06:54, Zhou, David(ChunMing) wrote: -Original Message- From: Lionel Landwerlin Sent: Saturday, March 30, 2019 10:09 PM To: Koenig, Christian ; Zhou, David(ChunMing) ; dri-de...@lists.freedesktop.org; amd- g...@lists.freedesktop.org; ja...@jlekstrand.net; Hector, Tobias 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 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 Cc: Lionel Landwerlin --- 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(>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(>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. 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. -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, >base); + + list_for_each_entry_safe(cur, tmp, >cb_list, node) { + list_del_init(>node); + syncobj_wait_syncobj_func(syncobj, cur); + } + spin_unlock(>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 +#include 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_f
RE: [PATCH 2/9] drm/syncobj: add new drm_syncobj_add_point interface v4
> -Original Message- > From: Lionel Landwerlin > Sent: Saturday, March 30, 2019 10:09 PM > To: Koenig, Christian ; Zhou, David(ChunMing) > ; dri-de...@lists.freedesktop.org; amd- > g...@lists.freedesktop.org; ja...@jlekstrand.net; Hector, Tobias > > 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 > >>> > >>> 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 > >>> Cc: Lionel Landwerlin > >>> --- > >>> 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(>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(>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 > > 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, >base); > >>> + > >>> + list_for_each_entry_safe(cur, tmp, >cb_list, node) { > >>> + list_del_init(>node); > >>> + syncobj_wait_syncobj_func(syncobj, cur); > >>> + } > >>> + spin_unlock(>lock); > >>> + > >>> + /* Walk the chain once to trigger garbage collection */ > >>> + dma_fence_c
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 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 Cc: Lionel Landwerlin --- 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(>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(>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. 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, >base); + + list_for_each_entry_safe(cur, tmp, >cb_list, node) { + list_del_init(>node); + syncobj_wait_syncobj_func(syncobj, cur); + } + spin_unlock(>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 +#include 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
Re: [PATCH 2/9] drm/syncobj: add new drm_syncobj_add_point interface v4
On 2019-03-28 4:18 p.m., 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 >>> >>> 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 >>> Cc: Lionel Landwerlin >>> --- >>> 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(>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(>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. How about DRM_WARN_ONCE ? -- Earthling Michel Dänzer | https://www.amd.com Libre software enthusiast | Mesa and X developer ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH 2/9] drm/syncobj: add new drm_syncobj_add_point interface v4
Am 28.03.19 um 14:50 schrieb Lionel Landwerlin: On 25/03/2019 08:32, Chunming Zhou wrote: From: Christian König 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 Cc: Lionel Landwerlin --- 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(>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(>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. -Lionel + dma_fence_chain_init(chain, prev, fence, point); + rcu_assign_pointer(syncobj->fence, >base); + + list_for_each_entry_safe(cur, tmp, >cb_list, node) { + list_del_init(>node); + syncobj_wait_syncobj_func(syncobj, cur); + } + spin_unlock(>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 +#include 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
Re: [PATCH 2/9] drm/syncobj: add new drm_syncobj_add_point interface v4
On 25/03/2019 08:32, Chunming Zhou wrote: From: Christian König 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 Cc: Lionel Landwerlin --- 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(>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(>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? -Lionel + dma_fence_chain_init(chain, prev, fence, point); + rcu_assign_pointer(syncobj->fence, >base); + + list_for_each_entry_safe(cur, tmp, >cb_list, node) { + list_del_init(>node); + syncobj_wait_syncobj_func(syncobj, cur); + } + spin_unlock(>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 +#include 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
[PATCH 2/9] drm/syncobj: add new drm_syncobj_add_point interface v4
From: Christian König 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 Cc: Lionel Landwerlin --- 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(>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(>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); + dma_fence_chain_init(chain, prev, fence, point); + rcu_assign_pointer(syncobj->fence, >base); + + list_for_each_entry_safe(cur, tmp, >cb_list, node) { + list_del_init(>node); + syncobj_wait_syncobj_func(syncobj, cur); + } + spin_unlock(>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 +#include 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, -- 2.17.1 ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH 2/9] drm/syncobj: add new drm_syncobj_add_point interface v4
From: Christian König 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 Cc: Lionel Landwerlin --- 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(>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(>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); + dma_fence_chain_init(chain, prev, fence, point); + rcu_assign_pointer(syncobj->fence, >base); + + list_for_each_entry_safe(cur, tmp, >cb_list, node) { + list_del_init(>node); + syncobj_wait_syncobj_func(syncobj, cur); + } + spin_unlock(>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 +#include 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, -- 2.17.1 ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH 2/9] drm/syncobj: add new drm_syncobj_add_point interface v4
From: Christian König 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 --- 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(>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(>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); + dma_fence_chain_init(chain, prev, fence, point); + rcu_assign_pointer(syncobj->fence, >base); + + list_for_each_entry_safe(cur, tmp, >cb_list, node) { + list_del_init(>node); + syncobj_wait_syncobj_func(syncobj, cur); + } + spin_unlock(>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 +#include 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, -- 2.17.1 ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx