From: zhoucm1 <zhoucm1-5C7GfCeVMHo@public.gmane.org>
To: Lionel Landwerlin
<lionel.g.landwerlin-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>,
Chunming Zhou <david1.zhou-5C7GfCeVMHo@public.gmane.org>,
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org,
amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org,
jason-fQELhIk9awXprZlt/sZkLg@public.gmane.org,
Christian.Koenig-5C7GfCeVMHo@public.gmane.org
Subject: Re: [PATCH 8/9] drm/syncobj: add timeline signal ioctl for syncobj v3
Date: Wed, 20 Mar 2019 11:53:19 +0800 [thread overview]
Message-ID: <e4f8f010-2649-f8fc-5949-9b652a3e977e@amd.com> (raw)
In-Reply-To: <78d9e191-7cdd-c9f1-c33e-2707b44e4932-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
On 2019年03月19日 19:54, Lionel Landwerlin wrote:
> On 15/03/2019 12:09, Chunming Zhou wrote:
>> v2: individually allocate chain array, since chain node is free
>> independently.
>> v3: all existing points must be already signaled before cpu perform
>> signal operation,
>> so add check condition for that.
>>
>> Signed-off-by: Chunming Zhou <david1.zhou@amd.com>
>> ---
>> drivers/gpu/drm/drm_internal.h | 2 +
>> drivers/gpu/drm/drm_ioctl.c | 2 +
>> drivers/gpu/drm/drm_syncobj.c | 103 +++++++++++++++++++++++++++++++++
>> include/uapi/drm/drm.h | 1 +
>> 4 files changed, 108 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_internal.h
>> b/drivers/gpu/drm/drm_internal.h
>> index dd11ae5f1eef..d9a483a5fce0 100644
>> --- a/drivers/gpu/drm/drm_internal.h
>> +++ b/drivers/gpu/drm/drm_internal.h
>> @@ -190,6 +190,8 @@ int drm_syncobj_reset_ioctl(struct drm_device
>> *dev, void *data,
>> struct drm_file *file_private);
>> int drm_syncobj_signal_ioctl(struct drm_device *dev, void *data,
>> struct drm_file *file_private);
>> +int drm_syncobj_timeline_signal_ioctl(struct drm_device *dev, void
>> *data,
>> + struct drm_file *file_private);
>> int drm_syncobj_query_ioctl(struct drm_device *dev, void *data,
>> struct drm_file *file_private);
>> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
>> index 92b3b7b2fd81..d337f161909c 100644
>> --- a/drivers/gpu/drm/drm_ioctl.c
>> +++ b/drivers/gpu/drm/drm_ioctl.c
>> @@ -696,6 +696,8 @@ static const struct drm_ioctl_desc drm_ioctls[] = {
>> DRM_UNLOCKED|DRM_RENDER_ALLOW),
>> DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_SIGNAL, drm_syncobj_signal_ioctl,
>> DRM_UNLOCKED|DRM_RENDER_ALLOW),
>> + DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_TIMELINE_SIGNAL,
>> drm_syncobj_timeline_signal_ioctl,
>> + DRM_UNLOCKED|DRM_RENDER_ALLOW),
>> DRM_IOCTL_DEF(DRM_IOCTL_SYNCOBJ_QUERY, drm_syncobj_query_ioctl,
>> DRM_UNLOCKED|DRM_RENDER_ALLOW),
>> DRM_IOCTL_DEF(DRM_IOCTL_CRTC_GET_SEQUENCE,
>> drm_crtc_get_sequence_ioctl, DRM_UNLOCKED),
>> diff --git a/drivers/gpu/drm/drm_syncobj.c
>> b/drivers/gpu/drm/drm_syncobj.c
>> index 306c7b7e2770..eaeb038f97d7 100644
>> --- a/drivers/gpu/drm/drm_syncobj.c
>> +++ b/drivers/gpu/drm/drm_syncobj.c
>> @@ -1183,6 +1183,109 @@ drm_syncobj_signal_ioctl(struct drm_device
>> *dev, void *data,
>> return ret;
>> }
>> +int
>> +drm_syncobj_timeline_signal_ioctl(struct drm_device *dev, void *data,
>> + struct drm_file *file_private)
>> +{
>> + struct drm_syncobj_timeline_array *args = data;
>> + struct drm_syncobj **syncobjs;
>> + struct dma_fence_chain **chains;
>> + uint64_t *points;
>> + uint32_t i, j, timeline_count = 0;
>> + int ret;
>> +
>> + if (!drm_core_check_feature(dev, DRIVER_SYNCOBJ))
>> + return -EOPNOTSUPP;
>> +
>> + if (args->pad != 0)
>> + return -EINVAL;
>> +
>> + if (args->count_handles == 0)
>> + return -EINVAL;
>> +
>> + ret = drm_syncobj_array_find(file_private,
>> + u64_to_user_ptr(args->handles),
>> + args->count_handles,
>> + &syncobjs);
>> + if (ret < 0)
>> + return ret;
>> +
>> + for (i = 0; i < args->count_handles; i++) {
>> + struct dma_fence_chain *chain;
>> + struct dma_fence *fence;
>> +
>> + fence = drm_syncobj_fence_get(syncobjs[i]);
>> + chain = to_dma_fence_chain(fence);
>> + if (chain) {
>> + struct dma_fence *iter;
>> +
>> + dma_fence_chain_for_each(iter, fence) {
>> + if (!iter)
>> + break;
>> + if (!dma_fence_is_signaled(iter)) {
>> + dma_fence_put(iter);
>> + DRM_ERROR("Client must guarantee all existing
>> timeline points signaled before performing host signal operation!");
>> + ret = -EPERM;
>> + goto out;
>
>
> Sorry if I'm failing to remember whether we discussed this before.
>
>
> Signaling a point from the host should be fine even if the previous
> points in the timeline are not signaled.
ok, will remove that checking.
>
> After all this can happen on the device side as well (out of order
> signaling).
>
>
> I thought the thing we didn't want is out of order submission.
>
> Just checking the last chain node seqno against the host signal point
> should be enough.
>
>
> What about simply returning -EPERM, we can warn the application from
> userspace?
OK, will add that.
>
>
>> + }
>> + }
>> + }
>> + }
>> +
>> + points = kmalloc_array(args->count_handles, sizeof(*points),
>> + GFP_KERNEL);
>> + if (!points) {
>> + ret = -ENOMEM;
>> + goto out;
>> + }
>> + if (!u64_to_user_ptr(args->points)) {
>> + memset(points, 0, args->count_handles * sizeof(uint64_t));
>> + } else if (copy_from_user(points, u64_to_user_ptr(args->points),
>> + sizeof(uint64_t) * args->count_handles)) {
>> + ret = -EFAULT;
>> + goto err_points;
>> + }
>> +
>> +
>> + for (i = 0; i < args->count_handles; i++) {
>> + if (points[i])
>> + timeline_count++;
>> + }
>> + chains = kmalloc_array(timeline_count, sizeof(void *), GFP_KERNEL);
>> + if (!chains) {
>> + ret = -ENOMEM;
>> + goto err_points;
>> + }
>> + for (i = 0; i < timeline_count; i++) {
>> + chains[i] = kzalloc(sizeof(struct dma_fence_chain),
>> GFP_KERNEL);
>> + if (!chains[i]) {
>> + for (j = 0; j < i; j++)
>> + kfree(chains[j]);
>> + ret = -ENOMEM;
>> + goto err_chains;
>> + }
>> + }
>> +
>> + for (i = 0, j = 0; i < args->count_handles; i++) {
>> + if (points[i]) {
>> + struct dma_fence *fence = dma_fence_get_stub();
>> +
>> + drm_syncobj_add_point(syncobjs[i], chains[j++],
>
>
> Isn't this breaking with j++ ?, it should be part of the for() loop
> expression : for (i = 0, j = 0; i < arrags->count_handles; i++, j++)
>
>
> Otherwise j will not increment at the same pace as i.
I think that is intentional, because syncobj array could contain both
binary and timeline. j is for timelien_count in this context.
-David
>
>
>> + fence, points[i]);
>> + dma_fence_put(fence);
>> + } else
>> + drm_syncobj_assign_null_handle(syncobjs[i]);
>> + }
>> +err_chains:
>> + kfree(chains);
>> +err_points:
>> + kfree(points);
>> +out:
>> + drm_syncobj_array_free(syncobjs, args->count_handles);
>> +
>> + return ret;
>> +}
>> +
>> int drm_syncobj_query_ioctl(struct drm_device *dev, void *data,
>> struct drm_file *file_private)
>> {
>> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
>> index 4c1e2e6579fa..fe00b74268eb 100644
>> --- a/include/uapi/drm/drm.h
>> +++ b/include/uapi/drm/drm.h
>> @@ -943,6 +943,7 @@ extern "C" {
>> #define DRM_IOCTL_SYNCOBJ_TIMELINE_WAIT DRM_IOWR(0xCA, struct
>> drm_syncobj_timeline_wait)
>> #define DRM_IOCTL_SYNCOBJ_QUERY DRM_IOWR(0xCB, struct
>> drm_syncobj_timeline_array)
>> #define DRM_IOCTL_SYNCOBJ_TRANSFER DRM_IOWR(0xCC, struct
>> drm_syncobj_transfer)
>> +#define DRM_IOCTL_SYNCOBJ_TIMELINE_SIGNAL DRM_IOWR(0xCD, struct
>> drm_syncobj_timeline_array)
>> /**
>> * Device specific ioctls should only be in their respective headers
>
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
next prev parent reply other threads:[~2019-03-20 3:53 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-03-15 12:09 [PATCH 1/9] dma-buf: add new dma_fence_chain container v5 Chunming Zhou
2019-03-15 12:09 ` [PATCH 2/9] drm/syncobj: add new drm_syncobj_add_point interface v4 Chunming Zhou
2019-03-15 12:09 ` [PATCH 3/9] drm/syncobj: add support for timeline point wait v8 Chunming Zhou
2019-03-18 16:59 ` Lionel Landwerlin
[not found] ` <5b68b54a-5d8d-9666-bd45-aa53b6b295e7-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
2019-03-18 17:20 ` Koenig, Christian
[not found] ` <990db9e7-6cae-1165-637c-84aed3a9af49-5C7GfCeVMHo@public.gmane.org>
2019-03-18 18:19 ` Lionel Landwerlin
2019-03-15 12:09 ` [PATCH 4/9] drm/syncobj: add timeline payload query ioctl v6 Chunming Zhou
2019-03-15 12:09 ` [PATCH 6/9] drm/amdgpu: add timeline support in amdgpu CS v3 Chunming Zhou
[not found] ` <20190315120959.25489-1-david1.zhou-5C7GfCeVMHo@public.gmane.org>
2019-03-15 12:09 ` [PATCH 5/9] drm/syncobj: use the timeline point in drm_syncobj_find_fence v3 Chunming Zhou
2019-03-16 1:10 ` kbuild test robot
2019-03-15 12:09 ` [PATCH 7/9] drm/syncobj: add transition iotcls between binary and timeline v2 Chunming Zhou
2019-03-15 12:09 ` [PATCH 8/9] drm/syncobj: add timeline signal ioctl for syncobj v3 Chunming Zhou
[not found] ` <20190315120959.25489-8-david1.zhou-5C7GfCeVMHo@public.gmane.org>
2019-03-19 11:54 ` Lionel Landwerlin
[not found] ` <78d9e191-7cdd-c9f1-c33e-2707b44e4932-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
2019-03-20 3:53 ` zhoucm1 [this message]
[not found] ` <e4f8f010-2649-f8fc-5949-9b652a3e977e-5C7GfCeVMHo@public.gmane.org>
2019-03-20 11:58 ` Lionel Landwerlin
2019-03-20 13:23 ` Lionel Landwerlin
2019-03-15 12:09 ` [PATCH 9/9] drm/amdgpu: update version for timeline syncobj support in amdgpu Chunming Zhou
2019-03-19 12:27 ` [PATCH 1/9] dma-buf: add new dma_fence_chain container v5 Lionel Landwerlin
[not found] ` <946e2cc2-eda7-7e7f-80ad-fa7161942930-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
2019-03-19 12:29 ` Christian König
-- strict thread matches above, loose matches on Subject: below --
2019-03-12 3:10 Chunming Zhou
2019-03-12 3:10 ` [PATCH 8/9] drm/syncobj: add timeline signal ioctl for syncobj v3 Chunming Zhou
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=e4f8f010-2649-f8fc-5949-9b652a3e977e@amd.com \
--to=zhoucm1-5c7gfcevmho@public.gmane.org \
--cc=Christian.Koenig-5C7GfCeVMHo@public.gmane.org \
--cc=amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org \
--cc=david1.zhou-5C7GfCeVMHo@public.gmane.org \
--cc=dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org \
--cc=jason-fQELhIk9awXprZlt/sZkLg@public.gmane.org \
--cc=lionel.g.landwerlin-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).