| Message ID | 20181207095601.2058-6-david1.zhou@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/11] dma-buf: make fence sequence numbers 64 bit v2 | expand |
On 07/12/2018 09:55, Chunming Zhou wrote: > user mode can query timeline payload. > v2: check return value of copy_to_user > v3: handle querying entry by entry > v4: rebase on new chain container, simplify interface > > Signed-off-by: Chunming Zhou <david1.zhou@amd.com> > Cc: Daniel Rakos <Daniel.Rakos@amd.com> > Cc: Jason Ekstrand <jason@jlekstrand.net> > Cc: Bas Nieuwenhuizen <bas@basnieuwenhuizen.nl> > Cc: Dave Airlie <airlied@redhat.com> > Cc: Christian König <christian.koenig@amd.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > --- > drivers/gpu/drm/drm_internal.h | 2 ++ > drivers/gpu/drm/drm_ioctl.c | 2 ++ > drivers/gpu/drm/drm_syncobj.c | 43 ++++++++++++++++++++++++++++++++++ > include/uapi/drm/drm.h | 10 ++++++++ > 4 files changed, 57 insertions(+) > > diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h > index 18b41e10195c..dab4d5936441 100644 > --- a/drivers/gpu/drm/drm_internal.h > +++ b/drivers/gpu/drm/drm_internal.h > @@ -184,6 +184,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_query_ioctl(struct drm_device *dev, void *data, > + struct drm_file *file_private); > > /* drm_framebuffer.c */ > void drm_framebuffer_print_info(struct drm_printer *p, unsigned int indent, > diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c > index a9a17ed35cc4..7578ef6dc1d1 100644 > --- a/drivers/gpu/drm/drm_ioctl.c > +++ b/drivers/gpu/drm/drm_ioctl.c > @@ -681,6 +681,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_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), > DRM_IOCTL_DEF(DRM_IOCTL_CRTC_QUEUE_SEQUENCE, drm_crtc_queue_sequence_ioctl, DRM_UNLOCKED), > DRM_IOCTL_DEF(DRM_IOCTL_MODE_CREATE_LEASE, drm_mode_create_lease_ioctl, DRM_MASTER|DRM_UNLOCKED), > diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c > index 348079bb0965..f97fa00ca1d0 100644 > --- a/drivers/gpu/drm/drm_syncobj.c > +++ b/drivers/gpu/drm/drm_syncobj.c > @@ -1061,3 +1061,46 @@ drm_syncobj_signal_ioctl(struct drm_device *dev, void *data, > > return ret; > } > + > +int drm_syncobj_query_ioctl(struct drm_device *dev, void *data, > + struct drm_file *file_private) > +{ > + struct drm_syncobj_timeline_array *args = data; > + struct drm_syncobj **syncobjs; > + uint64_t __user *points = u64_to_user_ptr(args->points); > + uint32_t i; > + int ret; > + > + if (!drm_core_check_feature(dev, DRIVER_SYNCOBJ)) > + return -ENODEV; > + > + 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; > + uint64_t point; > + > + fence = drm_syncobj_fence_get(syncobjs[i]); > + chain = to_dma_fence_chain(fence); > + point = chain ? fence->seqno : 0; Sorry, I don' t want to sound annoying, but this looks like this could report values going backward. Anything add a point X to a timeline that has reached value Y with X < Y would trigger that. Either through the submission or userspace signaling or importing another syncpoint's fence. -Lionel > + ret = copy_to_user(&points[i], &point, sizeof(uint64_t)); > + ret = ret ? -EFAULT : 0; > + if (ret) > + break; > + } > + drm_syncobj_array_free(syncobjs, args->count_handles); > + > + return ret; > +} > diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h > index 0092111d002c..b2c36f2b2599 100644 > --- a/include/uapi/drm/drm.h > +++ b/include/uapi/drm/drm.h > @@ -767,6 +767,14 @@ struct drm_syncobj_array { > __u32 pad; > }; > > +struct drm_syncobj_timeline_array { > + __u64 handles; > + __u64 points; > + __u32 count_handles; > + __u32 pad; > +}; > + > + > /* Query current scanout sequence number */ > struct drm_crtc_get_sequence { > __u32 crtc_id; /* requested crtc_id */ > @@ -924,6 +932,8 @@ extern "C" { > #define DRM_IOCTL_MODE_REVOKE_LEASE DRM_IOWR(0xC9, struct drm_mode_revoke_lease) > > #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) > + > /** > * Device specific ioctls should only be in their respective headers > * The device specific ioctl range is from 0x40 to 0x9f.
Am 15.02.19 um 20:31 schrieb Lionel Landwerlin via amd-gfx: > On 07/12/2018 09:55, Chunming Zhou wrote: >> user mode can query timeline payload. >> v2: check return value of copy_to_user >> v3: handle querying entry by entry >> v4: rebase on new chain container, simplify interface >> >> Signed-off-by: Chunming Zhou <david1.zhou@amd.com> >> Cc: Daniel Rakos <Daniel.Rakos@amd.com> >> Cc: Jason Ekstrand <jason@jlekstrand.net> >> Cc: Bas Nieuwenhuizen <bas@basnieuwenhuizen.nl> >> Cc: Dave Airlie <airlied@redhat.com> >> Cc: Christian König <christian.koenig@amd.com> >> Cc: Chris Wilson <chris@chris-wilson.co.uk> >> --- >> drivers/gpu/drm/drm_internal.h | 2 ++ >> drivers/gpu/drm/drm_ioctl.c | 2 ++ >> drivers/gpu/drm/drm_syncobj.c | 43 ++++++++++++++++++++++++++++++++++ >> include/uapi/drm/drm.h | 10 ++++++++ >> 4 files changed, 57 insertions(+) >> >> diff --git a/drivers/gpu/drm/drm_internal.h >> b/drivers/gpu/drm/drm_internal.h >> index 18b41e10195c..dab4d5936441 100644 >> --- a/drivers/gpu/drm/drm_internal.h >> +++ b/drivers/gpu/drm/drm_internal.h >> @@ -184,6 +184,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_query_ioctl(struct drm_device *dev, void *data, >> + struct drm_file *file_private); >> /* drm_framebuffer.c */ >> void drm_framebuffer_print_info(struct drm_printer *p, unsigned int >> indent, >> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c >> index a9a17ed35cc4..7578ef6dc1d1 100644 >> --- a/drivers/gpu/drm/drm_ioctl.c >> +++ b/drivers/gpu/drm/drm_ioctl.c >> @@ -681,6 +681,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_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), >> DRM_IOCTL_DEF(DRM_IOCTL_CRTC_QUEUE_SEQUENCE, >> drm_crtc_queue_sequence_ioctl, DRM_UNLOCKED), >> DRM_IOCTL_DEF(DRM_IOCTL_MODE_CREATE_LEASE, >> drm_mode_create_lease_ioctl, DRM_MASTER|DRM_UNLOCKED), >> diff --git a/drivers/gpu/drm/drm_syncobj.c >> b/drivers/gpu/drm/drm_syncobj.c >> index 348079bb0965..f97fa00ca1d0 100644 >> --- a/drivers/gpu/drm/drm_syncobj.c >> +++ b/drivers/gpu/drm/drm_syncobj.c >> @@ -1061,3 +1061,46 @@ drm_syncobj_signal_ioctl(struct drm_device >> *dev, void *data, >> return ret; >> } >> + >> +int drm_syncobj_query_ioctl(struct drm_device *dev, void *data, >> + struct drm_file *file_private) >> +{ >> + struct drm_syncobj_timeline_array *args = data; >> + struct drm_syncobj **syncobjs; >> + uint64_t __user *points = u64_to_user_ptr(args->points); >> + uint32_t i; >> + int ret; >> + >> + if (!drm_core_check_feature(dev, DRIVER_SYNCOBJ)) >> + return -ENODEV; >> + >> + 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; >> + uint64_t point; >> + >> + fence = drm_syncobj_fence_get(syncobjs[i]); >> + chain = to_dma_fence_chain(fence); >> + point = chain ? fence->seqno : 0; > > > Sorry, I don' t want to sound annoying, but this looks like this could > report values going backward. Well please be annoying as much as you can :) But yeah all that stuff has been discussed before as well. > > Anything add a point X to a timeline that has reached value Y with X < > Y would trigger that. Yes, that can indeed happen. But adding a timeline point X which is before the already added point Y is illegal in the first place :) So when the application does something stupid and breaks it can just keep the pieces. In the kernel we still do the most defensive thing and sync to everything in this case. I'm just not sure if we should print an error into syslog or just continue silently. Regards, Christian. > > Either through the submission or userspace signaling or importing > another syncpoint's fence. > > > -Lionel > > >> + ret = copy_to_user(&points[i], &point, sizeof(uint64_t)); >> + ret = ret ? -EFAULT : 0; >> + if (ret) >> + break; >> + } >> + drm_syncobj_array_free(syncobjs, args->count_handles); >> + >> + return ret; >> +} >> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h >> index 0092111d002c..b2c36f2b2599 100644 >> --- a/include/uapi/drm/drm.h >> +++ b/include/uapi/drm/drm.h >> @@ -767,6 +767,14 @@ struct drm_syncobj_array { >> __u32 pad; >> }; >> +struct drm_syncobj_timeline_array { >> + __u64 handles; >> + __u64 points; >> + __u32 count_handles; >> + __u32 pad; >> +}; >> + >> + >> /* Query current scanout sequence number */ >> struct drm_crtc_get_sequence { >> __u32 crtc_id; /* requested crtc_id */ >> @@ -924,6 +932,8 @@ extern "C" { >> #define DRM_IOCTL_MODE_REVOKE_LEASE DRM_IOWR(0xC9, struct >> drm_mode_revoke_lease) >> #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) >> + >> /** >> * Device specific ioctls should only be in their respective headers >> * The device specific ioctl range is from 0x40 to 0x9f. > > > _______________________________________________ > amd-gfx mailing list > amd-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/amd-gfx
On 2019年02月17日 03:22, Christian König wrote: > Am 15.02.19 um 20:31 schrieb Lionel Landwerlin via amd-gfx: >> On 07/12/2018 09:55, Chunming Zhou wrote: >>> user mode can query timeline payload. >>> v2: check return value of copy_to_user >>> v3: handle querying entry by entry >>> v4: rebase on new chain container, simplify interface >>> >>> Signed-off-by: Chunming Zhou <david1.zhou@amd.com> >>> Cc: Daniel Rakos <Daniel.Rakos@amd.com> >>> Cc: Jason Ekstrand <jason@jlekstrand.net> >>> Cc: Bas Nieuwenhuizen <bas@basnieuwenhuizen.nl> >>> Cc: Dave Airlie <airlied@redhat.com> >>> Cc: Christian König <christian.koenig@amd.com> >>> Cc: Chris Wilson <chris@chris-wilson.co.uk> >>> --- >>> drivers/gpu/drm/drm_internal.h | 2 ++ >>> drivers/gpu/drm/drm_ioctl.c | 2 ++ >>> drivers/gpu/drm/drm_syncobj.c | 43 >>> ++++++++++++++++++++++++++++++++++ >>> include/uapi/drm/drm.h | 10 ++++++++ >>> 4 files changed, 57 insertions(+) >>> >>> diff --git a/drivers/gpu/drm/drm_internal.h >>> b/drivers/gpu/drm/drm_internal.h >>> index 18b41e10195c..dab4d5936441 100644 >>> --- a/drivers/gpu/drm/drm_internal.h >>> +++ b/drivers/gpu/drm/drm_internal.h >>> @@ -184,6 +184,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_query_ioctl(struct drm_device *dev, void *data, >>> + struct drm_file *file_private); >>> /* drm_framebuffer.c */ >>> void drm_framebuffer_print_info(struct drm_printer *p, unsigned >>> int indent, >>> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c >>> index a9a17ed35cc4..7578ef6dc1d1 100644 >>> --- a/drivers/gpu/drm/drm_ioctl.c >>> +++ b/drivers/gpu/drm/drm_ioctl.c >>> @@ -681,6 +681,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_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), >>> DRM_IOCTL_DEF(DRM_IOCTL_CRTC_QUEUE_SEQUENCE, >>> drm_crtc_queue_sequence_ioctl, DRM_UNLOCKED), >>> DRM_IOCTL_DEF(DRM_IOCTL_MODE_CREATE_LEASE, >>> drm_mode_create_lease_ioctl, DRM_MASTER|DRM_UNLOCKED), >>> diff --git a/drivers/gpu/drm/drm_syncobj.c >>> b/drivers/gpu/drm/drm_syncobj.c >>> index 348079bb0965..f97fa00ca1d0 100644 >>> --- a/drivers/gpu/drm/drm_syncobj.c >>> +++ b/drivers/gpu/drm/drm_syncobj.c >>> @@ -1061,3 +1061,46 @@ drm_syncobj_signal_ioctl(struct drm_device >>> *dev, void *data, >>> return ret; >>> } >>> + >>> +int drm_syncobj_query_ioctl(struct drm_device *dev, void *data, >>> + struct drm_file *file_private) >>> +{ >>> + struct drm_syncobj_timeline_array *args = data; >>> + struct drm_syncobj **syncobjs; >>> + uint64_t __user *points = u64_to_user_ptr(args->points); >>> + uint32_t i; >>> + int ret; >>> + >>> + if (!drm_core_check_feature(dev, DRIVER_SYNCOBJ)) >>> + return -ENODEV; >>> + >>> + 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; >>> + uint64_t point; >>> + >>> + fence = drm_syncobj_fence_get(syncobjs[i]); >>> + chain = to_dma_fence_chain(fence); >>> + point = chain ? fence->seqno : 0; >> >> >> Sorry, I don' t want to sound annoying, but this looks like this >> could report values going backward. > > Well please be annoying as much as you can :) But yeah all that stuff > has been discussed before as well. > >> >> Anything add a point X to a timeline that has reached value Y with X >> < Y would trigger that. > > Yes, that can indeed happen. trigger what? when adding x (x < y), then return 0 when query? Why would this happen? No, syncobj->fence should always be there and the last chain node, if it is ever added. -David > But adding a timeline point X which is before the already added point > Y is illegal in the first place :) > > So when the application does something stupid and breaks it can just > keep the pieces. > > In the kernel we still do the most defensive thing and sync to > everything in this case. > > I'm just not sure if we should print an error into syslog or just > continue silently. > > Regards, > Christian. > >> >> Either through the submission or userspace signaling or importing >> another syncpoint's fence. >> >> >> -Lionel >> >> >>> + ret = copy_to_user(&points[i], &point, sizeof(uint64_t)); >>> + ret = ret ? -EFAULT : 0; >>> + if (ret) >>> + break; >>> + } >>> + drm_syncobj_array_free(syncobjs, args->count_handles); >>> + >>> + return ret; >>> +} >>> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h >>> index 0092111d002c..b2c36f2b2599 100644 >>> --- a/include/uapi/drm/drm.h >>> +++ b/include/uapi/drm/drm.h >>> @@ -767,6 +767,14 @@ struct drm_syncobj_array { >>> __u32 pad; >>> }; >>> +struct drm_syncobj_timeline_array { >>> + __u64 handles; >>> + __u64 points; >>> + __u32 count_handles; >>> + __u32 pad; >>> +}; >>> + >>> + >>> /* Query current scanout sequence number */ >>> struct drm_crtc_get_sequence { >>> __u32 crtc_id; /* requested crtc_id */ >>> @@ -924,6 +932,8 @@ extern "C" { >>> #define DRM_IOCTL_MODE_REVOKE_LEASE DRM_IOWR(0xC9, struct >>> drm_mode_revoke_lease) >>> #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) >>> + >>> /** >>> * Device specific ioctls should only be in their respective headers >>> * The device specific ioctl range is from 0x40 to 0x9f. >> >> >> _______________________________________________ >> amd-gfx mailing list >> amd-gfx@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/amd-gfx >
Am 18.02.19 um 04:10 schrieb zhoucm1: > > > On 2019年02月17日 03:22, Christian König wrote: >> Am 15.02.19 um 20:31 schrieb Lionel Landwerlin via amd-gfx: >>> On 07/12/2018 09:55, Chunming Zhou wrote: >>>> user mode can query timeline payload. >>>> v2: check return value of copy_to_user >>>> v3: handle querying entry by entry >>>> v4: rebase on new chain container, simplify interface >>>> >>>> Signed-off-by: Chunming Zhou <david1.zhou@amd.com> >>>> Cc: Daniel Rakos <Daniel.Rakos@amd.com> >>>> Cc: Jason Ekstrand <jason@jlekstrand.net> >>>> Cc: Bas Nieuwenhuizen <bas@basnieuwenhuizen.nl> >>>> Cc: Dave Airlie <airlied@redhat.com> >>>> Cc: Christian König <christian.koenig@amd.com> >>>> Cc: Chris Wilson <chris@chris-wilson.co.uk> >>>> --- >>>> drivers/gpu/drm/drm_internal.h | 2 ++ >>>> drivers/gpu/drm/drm_ioctl.c | 2 ++ >>>> drivers/gpu/drm/drm_syncobj.c | 43 >>>> ++++++++++++++++++++++++++++++++++ >>>> include/uapi/drm/drm.h | 10 ++++++++ >>>> 4 files changed, 57 insertions(+) >>>> >>>> diff --git a/drivers/gpu/drm/drm_internal.h >>>> b/drivers/gpu/drm/drm_internal.h >>>> index 18b41e10195c..dab4d5936441 100644 >>>> --- a/drivers/gpu/drm/drm_internal.h >>>> +++ b/drivers/gpu/drm/drm_internal.h >>>> @@ -184,6 +184,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_query_ioctl(struct drm_device *dev, void *data, >>>> + struct drm_file *file_private); >>>> /* drm_framebuffer.c */ >>>> void drm_framebuffer_print_info(struct drm_printer *p, unsigned >>>> int indent, >>>> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c >>>> index a9a17ed35cc4..7578ef6dc1d1 100644 >>>> --- a/drivers/gpu/drm/drm_ioctl.c >>>> +++ b/drivers/gpu/drm/drm_ioctl.c >>>> @@ -681,6 +681,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_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), >>>> DRM_IOCTL_DEF(DRM_IOCTL_CRTC_QUEUE_SEQUENCE, >>>> drm_crtc_queue_sequence_ioctl, DRM_UNLOCKED), >>>> DRM_IOCTL_DEF(DRM_IOCTL_MODE_CREATE_LEASE, >>>> drm_mode_create_lease_ioctl, DRM_MASTER|DRM_UNLOCKED), >>>> diff --git a/drivers/gpu/drm/drm_syncobj.c >>>> b/drivers/gpu/drm/drm_syncobj.c >>>> index 348079bb0965..f97fa00ca1d0 100644 >>>> --- a/drivers/gpu/drm/drm_syncobj.c >>>> +++ b/drivers/gpu/drm/drm_syncobj.c >>>> @@ -1061,3 +1061,46 @@ drm_syncobj_signal_ioctl(struct drm_device >>>> *dev, void *data, >>>> return ret; >>>> } >>>> + >>>> +int drm_syncobj_query_ioctl(struct drm_device *dev, void *data, >>>> + struct drm_file *file_private) >>>> +{ >>>> + struct drm_syncobj_timeline_array *args = data; >>>> + struct drm_syncobj **syncobjs; >>>> + uint64_t __user *points = u64_to_user_ptr(args->points); >>>> + uint32_t i; >>>> + int ret; >>>> + >>>> + if (!drm_core_check_feature(dev, DRIVER_SYNCOBJ)) >>>> + return -ENODEV; >>>> + >>>> + 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; >>>> + uint64_t point; >>>> + >>>> + fence = drm_syncobj_fence_get(syncobjs[i]); >>>> + chain = to_dma_fence_chain(fence); >>>> + point = chain ? fence->seqno : 0; >>> >>> >>> Sorry, I don' t want to sound annoying, but this looks like this >>> could report values going backward. >> >> Well please be annoying as much as you can :) But yeah all that stuff >> has been discussed before as well. >> >>> >>> Anything add a point X to a timeline that has reached value Y with X >>> < Y would trigger that. >> >> Yes, that can indeed happen. > trigger what? when adding x (x < y), then return 0 when query? > Why would this happen? > No, syncobj->fence should always be there and the last chain node, if > it is ever added. Well maybe Lionel should clarify a bit what he means? I thought he is concerned that the call could return values where X < Y, but that doesn't seem to be the case. Christian. > > -David >> But adding a timeline point X which is before the already added point >> Y is illegal in the first place :) >> >> So when the application does something stupid and breaks it can just >> keep the pieces. >> >> In the kernel we still do the most defensive thing and sync to >> everything in this case. >> >> I'm just not sure if we should print an error into syslog or just >> continue silently. >> >> Regards, >> Christian. >> >>> >>> Either through the submission or userspace signaling or importing >>> another syncpoint's fence. >>> >>> >>> -Lionel >>> >>> >>>> + ret = copy_to_user(&points[i], &point, sizeof(uint64_t)); >>>> + ret = ret ? -EFAULT : 0; >>>> + if (ret) >>>> + break; >>>> + } >>>> + drm_syncobj_array_free(syncobjs, args->count_handles); >>>> + >>>> + return ret; >>>> +} >>>> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h >>>> index 0092111d002c..b2c36f2b2599 100644 >>>> --- a/include/uapi/drm/drm.h >>>> +++ b/include/uapi/drm/drm.h >>>> @@ -767,6 +767,14 @@ struct drm_syncobj_array { >>>> __u32 pad; >>>> }; >>>> +struct drm_syncobj_timeline_array { >>>> + __u64 handles; >>>> + __u64 points; >>>> + __u32 count_handles; >>>> + __u32 pad; >>>> +}; >>>> + >>>> + >>>> /* Query current scanout sequence number */ >>>> struct drm_crtc_get_sequence { >>>> __u32 crtc_id; /* requested crtc_id */ >>>> @@ -924,6 +932,8 @@ extern "C" { >>>> #define DRM_IOCTL_MODE_REVOKE_LEASE DRM_IOWR(0xC9, struct >>>> drm_mode_revoke_lease) >>>> #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) >>>> + >>>> /** >>>> * Device specific ioctls should only be in their respective headers >>>> * The device specific ioctl range is from 0x40 to 0x9f. >>> >>> >>> _______________________________________________ >>> amd-gfx mailing list >>> amd-gfx@lists.freedesktop.org >>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx >> >
On 18/02/2019 07:28, Koenig, Christian wrote: > Am 18.02.19 um 04:10 schrieb zhoucm1: >> >> On 2019年02月17日 03:22, Christian König wrote: >>> Am 15.02.19 um 20:31 schrieb Lionel Landwerlin via amd-gfx: >>>> On 07/12/2018 09:55, Chunming Zhou wrote: >>>>> user mode can query timeline payload. >>>>> v2: check return value of copy_to_user >>>>> v3: handle querying entry by entry >>>>> v4: rebase on new chain container, simplify interface >>>>> >>>>> Signed-off-by: Chunming Zhou <david1.zhou@amd.com> >>>>> Cc: Daniel Rakos <Daniel.Rakos@amd.com> >>>>> Cc: Jason Ekstrand <jason@jlekstrand.net> >>>>> Cc: Bas Nieuwenhuizen <bas@basnieuwenhuizen.nl> >>>>> Cc: Dave Airlie <airlied@redhat.com> >>>>> Cc: Christian König <christian.koenig@amd.com> >>>>> Cc: Chris Wilson <chris@chris-wilson.co.uk> >>>>> --- >>>>> drivers/gpu/drm/drm_internal.h | 2 ++ >>>>> drivers/gpu/drm/drm_ioctl.c | 2 ++ >>>>> drivers/gpu/drm/drm_syncobj.c | 43 >>>>> ++++++++++++++++++++++++++++++++++ >>>>> include/uapi/drm/drm.h | 10 ++++++++ >>>>> 4 files changed, 57 insertions(+) >>>>> >>>>> diff --git a/drivers/gpu/drm/drm_internal.h >>>>> b/drivers/gpu/drm/drm_internal.h >>>>> index 18b41e10195c..dab4d5936441 100644 >>>>> --- a/drivers/gpu/drm/drm_internal.h >>>>> +++ b/drivers/gpu/drm/drm_internal.h >>>>> @@ -184,6 +184,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_query_ioctl(struct drm_device *dev, void *data, >>>>> + struct drm_file *file_private); >>>>> /* drm_framebuffer.c */ >>>>> void drm_framebuffer_print_info(struct drm_printer *p, unsigned >>>>> int indent, >>>>> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c >>>>> index a9a17ed35cc4..7578ef6dc1d1 100644 >>>>> --- a/drivers/gpu/drm/drm_ioctl.c >>>>> +++ b/drivers/gpu/drm/drm_ioctl.c >>>>> @@ -681,6 +681,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_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), >>>>> DRM_IOCTL_DEF(DRM_IOCTL_CRTC_QUEUE_SEQUENCE, >>>>> drm_crtc_queue_sequence_ioctl, DRM_UNLOCKED), >>>>> DRM_IOCTL_DEF(DRM_IOCTL_MODE_CREATE_LEASE, >>>>> drm_mode_create_lease_ioctl, DRM_MASTER|DRM_UNLOCKED), >>>>> diff --git a/drivers/gpu/drm/drm_syncobj.c >>>>> b/drivers/gpu/drm/drm_syncobj.c >>>>> index 348079bb0965..f97fa00ca1d0 100644 >>>>> --- a/drivers/gpu/drm/drm_syncobj.c >>>>> +++ b/drivers/gpu/drm/drm_syncobj.c >>>>> @@ -1061,3 +1061,46 @@ drm_syncobj_signal_ioctl(struct drm_device >>>>> *dev, void *data, >>>>> return ret; >>>>> } >>>>> + >>>>> +int drm_syncobj_query_ioctl(struct drm_device *dev, void *data, >>>>> + struct drm_file *file_private) >>>>> +{ >>>>> + struct drm_syncobj_timeline_array *args = data; >>>>> + struct drm_syncobj **syncobjs; >>>>> + uint64_t __user *points = u64_to_user_ptr(args->points); >>>>> + uint32_t i; >>>>> + int ret; >>>>> + >>>>> + if (!drm_core_check_feature(dev, DRIVER_SYNCOBJ)) >>>>> + return -ENODEV; >>>>> + >>>>> + 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; >>>>> + uint64_t point; >>>>> + >>>>> + fence = drm_syncobj_fence_get(syncobjs[i]); >>>>> + chain = to_dma_fence_chain(fence); >>>>> + point = chain ? fence->seqno : 0; >>>> >>>> Sorry, I don' t want to sound annoying, but this looks like this >>>> could report values going backward. >>> Well please be annoying as much as you can :) But yeah all that stuff >>> has been discussed before as well. >>> >>>> Anything add a point X to a timeline that has reached value Y with X >>>> < Y would trigger that. >>> Yes, that can indeed happen. >> trigger what? when adding x (x < y), then return 0 when query? >> Why would this happen? >> No, syncobj->fence should always be there and the last chain node, if >> it is ever added. > Well maybe Lionel should clarify a bit what he means? > > I thought he is concerned that the call could return values where X < Y, > but that doesn't seem to be the case. > > Christian. I meant something like this : t = create_timeline_syncobj() signal(t, 2) query(t) => 2 signal(t, 1) query(t) => 1 -Lionel > >> -David >>> But adding a timeline point X which is before the already added point >>> Y is illegal in the first place :) >>> >>> So when the application does something stupid and breaks it can just >>> keep the pieces. >>> >>> In the kernel we still do the most defensive thing and sync to >>> everything in this case. >>> >>> I'm just not sure if we should print an error into syslog or just >>> continue silently. >>> >>> Regards, >>> Christian. >>> >>>> Either through the submission or userspace signaling or importing >>>> another syncpoint's fence. >>>> >>>> >>>> -Lionel >>>> >>>> >>>>> + ret = copy_to_user(&points[i], &point, sizeof(uint64_t)); >>>>> + ret = ret ? -EFAULT : 0; >>>>> + if (ret) >>>>> + break; >>>>> + } >>>>> + drm_syncobj_array_free(syncobjs, args->count_handles); >>>>> + >>>>> + return ret; >>>>> +} >>>>> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h >>>>> index 0092111d002c..b2c36f2b2599 100644 >>>>> --- a/include/uapi/drm/drm.h >>>>> +++ b/include/uapi/drm/drm.h >>>>> @@ -767,6 +767,14 @@ struct drm_syncobj_array { >>>>> __u32 pad; >>>>> }; >>>>> +struct drm_syncobj_timeline_array { >>>>> + __u64 handles; >>>>> + __u64 points; >>>>> + __u32 count_handles; >>>>> + __u32 pad; >>>>> +}; >>>>> + >>>>> + >>>>> /* Query current scanout sequence number */ >>>>> struct drm_crtc_get_sequence { >>>>> __u32 crtc_id; /* requested crtc_id */ >>>>> @@ -924,6 +932,8 @@ extern "C" { >>>>> #define DRM_IOCTL_MODE_REVOKE_LEASE DRM_IOWR(0xC9, struct >>>>> drm_mode_revoke_lease) >>>>> #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) >>>>> + >>>>> /** >>>>> * Device specific ioctls should only be in their respective headers >>>>> * The device specific ioctl range is from 0x40 to 0x9f. >>>> >>>> _______________________________________________ >>>> amd-gfx mailing list >>>> amd-gfx@lists.freedesktop.org >>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h index 18b41e10195c..dab4d5936441 100644 --- a/drivers/gpu/drm/drm_internal.h +++ b/drivers/gpu/drm/drm_internal.h @@ -184,6 +184,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_query_ioctl(struct drm_device *dev, void *data, + struct drm_file *file_private); /* drm_framebuffer.c */ void drm_framebuffer_print_info(struct drm_printer *p, unsigned int indent, diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c index a9a17ed35cc4..7578ef6dc1d1 100644 --- a/drivers/gpu/drm/drm_ioctl.c +++ b/drivers/gpu/drm/drm_ioctl.c @@ -681,6 +681,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_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), DRM_IOCTL_DEF(DRM_IOCTL_CRTC_QUEUE_SEQUENCE, drm_crtc_queue_sequence_ioctl, DRM_UNLOCKED), DRM_IOCTL_DEF(DRM_IOCTL_MODE_CREATE_LEASE, drm_mode_create_lease_ioctl, DRM_MASTER|DRM_UNLOCKED), diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c index 348079bb0965..f97fa00ca1d0 100644 --- a/drivers/gpu/drm/drm_syncobj.c +++ b/drivers/gpu/drm/drm_syncobj.c @@ -1061,3 +1061,46 @@ drm_syncobj_signal_ioctl(struct drm_device *dev, void *data, return ret; } + +int drm_syncobj_query_ioctl(struct drm_device *dev, void *data, + struct drm_file *file_private) +{ + struct drm_syncobj_timeline_array *args = data; + struct drm_syncobj **syncobjs; + uint64_t __user *points = u64_to_user_ptr(args->points); + uint32_t i; + int ret; + + if (!drm_core_check_feature(dev, DRIVER_SYNCOBJ)) + return -ENODEV; + + 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; + uint64_t point; + + fence = drm_syncobj_fence_get(syncobjs[i]); + chain = to_dma_fence_chain(fence); + point = chain ? fence->seqno : 0; + ret = copy_to_user(&points[i], &point, sizeof(uint64_t)); + ret = ret ? -EFAULT : 0; + if (ret) + break; + } + drm_syncobj_array_free(syncobjs, args->count_handles); + + return ret; +} diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h index 0092111d002c..b2c36f2b2599 100644 --- a/include/uapi/drm/drm.h +++ b/include/uapi/drm/drm.h @@ -767,6 +767,14 @@ struct drm_syncobj_array { __u32 pad; }; +struct drm_syncobj_timeline_array { + __u64 handles; + __u64 points; + __u32 count_handles; + __u32 pad; +}; + + /* Query current scanout sequence number */ struct drm_crtc_get_sequence { __u32 crtc_id; /* requested crtc_id */ @@ -924,6 +932,8 @@ extern "C" { #define DRM_IOCTL_MODE_REVOKE_LEASE DRM_IOWR(0xC9, struct drm_mode_revoke_lease) #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) + /** * Device specific ioctls should only be in their respective headers * The device specific ioctl range is from 0x40 to 0x9f.
user mode can query timeline payload. v2: check return value of copy_to_user v3: handle querying entry by entry v4: rebase on new chain container, simplify interface Signed-off-by: Chunming Zhou <david1.zhou@amd.com> Cc: Daniel Rakos <Daniel.Rakos@amd.com> Cc: Jason Ekstrand <jason@jlekstrand.net> Cc: Bas Nieuwenhuizen <bas@basnieuwenhuizen.nl> Cc: Dave Airlie <airlied@redhat.com> Cc: Christian König <christian.koenig@amd.com> Cc: Chris Wilson <chris@chris-wilson.co.uk> --- drivers/gpu/drm/drm_internal.h | 2 ++ drivers/gpu/drm/drm_ioctl.c | 2 ++ drivers/gpu/drm/drm_syncobj.c | 43 ++++++++++++++++++++++++++++++++++ include/uapi/drm/drm.h | 10 ++++++++ 4 files changed, 57 insertions(+)