| Message ID | 20190723142114.24688-1-david1.zhou@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/syncobj: extend syncobj query ability v2 | expand |
On 23/07/2019 17:21, Chunming Zhou wrote: > user space needs a flexiable query ability. > So that umd can get last signaled or submitted point. > v2: > add sanitizer checking. > > Change-Id: I6512b430524ebabe715e602a2bf5abb0a7e780ea > Signed-off-by: Chunming Zhou <david1.zhou@amd.com> > Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com> > Cc: Christian König <Christian.Koenig@amd.com> Reviewed-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com> > --- > drivers/gpu/drm/drm_syncobj.c | 34 ++++++++++++++++++++-------------- > include/uapi/drm/drm.h | 3 ++- > 2 files changed, 22 insertions(+), 15 deletions(-) > > diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c > index 3d400905100b..3fc8f66ada68 100644 > --- a/drivers/gpu/drm/drm_syncobj.c > +++ b/drivers/gpu/drm/drm_syncobj.c > @@ -1197,7 +1197,7 @@ drm_syncobj_timeline_signal_ioctl(struct drm_device *dev, void *data, > if (!drm_core_check_feature(dev, DRIVER_SYNCOBJ_TIMELINE)) > return -EOPNOTSUPP; > > - if (args->pad != 0) > + if (args->flags != 0) > return -EINVAL; > > if (args->count_handles == 0) > @@ -1268,7 +1268,7 @@ int drm_syncobj_query_ioctl(struct drm_device *dev, void *data, > if (!drm_core_check_feature(dev, DRIVER_SYNCOBJ_TIMELINE)) > return -EOPNOTSUPP; > > - if (args->pad != 0) > + if (args->flags & ~DRM_SYNCOBJ_QUERY_FLAGS_LAST_SUBMITTED) > return -EINVAL; > > if (args->count_handles == 0) > @@ -1291,23 +1291,29 @@ int drm_syncobj_query_ioctl(struct drm_device *dev, void *data, > if (chain) { > struct dma_fence *iter, *last_signaled = NULL; > > - dma_fence_chain_for_each(iter, fence) { > - if (!iter) > - break; > - dma_fence_put(last_signaled); > - last_signaled = dma_fence_get(iter); > - if (!to_dma_fence_chain(last_signaled)->prev_seqno) > - /* It is most likely that timeline has > - * unorder points. */ > - break; > + if (args->flags & > + DRM_SYNCOBJ_QUERY_FLAGS_LAST_SUBMITTED) { > + point = fence->seqno; > + } else { > + dma_fence_chain_for_each(iter, fence) { > + if (!iter) > + break; > + dma_fence_put(last_signaled); > + last_signaled = dma_fence_get(iter); > + if (!to_dma_fence_chain(last_signaled)->prev_seqno) > + /* It is most likely that timeline has > + * unorder points. */ > + break; > + } > + point = dma_fence_is_signaled(last_signaled) ? > + last_signaled->seqno : > + to_dma_fence_chain(last_signaled)->prev_seqno; > } > - point = dma_fence_is_signaled(last_signaled) ? > - last_signaled->seqno : > - to_dma_fence_chain(last_signaled)->prev_seqno; > dma_fence_put(last_signaled); > } else { > point = 0; > } > + dma_fence_put(fence); > ret = copy_to_user(&points[i], &point, sizeof(uint64_t)); > ret = ret ? -EFAULT : 0; > if (ret) > diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h > index 661d73f9a919..fd987ce24d9f 100644 > --- a/include/uapi/drm/drm.h > +++ b/include/uapi/drm/drm.h > @@ -777,11 +777,12 @@ struct drm_syncobj_array { > __u32 pad; > }; > > +#define DRM_SYNCOBJ_QUERY_FLAGS_LAST_SUBMITTED (1 << 0) /* last available point on timeline syncobj */ > struct drm_syncobj_timeline_array { > __u64 handles; > __u64 points; > __u32 count_handles; > - __u32 pad; > + __u32 flags; > }; > >
On 2019年07月24日 03:20, Lionel Landwerlin wrote: > On 23/07/2019 17:21, Chunming Zhou wrote: >> user space needs a flexiable query ability. >> So that umd can get last signaled or submitted point. >> v2: >> add sanitizer checking. >> >> Change-Id: I6512b430524ebabe715e602a2bf5abb0a7e780ea >> Signed-off-by: Chunming Zhou <david1.zhou@amd.com> >> Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com> >> Cc: Christian König <Christian.Koenig@amd.com> > > Reviewed-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com> Thanks. Which branch should this patch go to? Is it OK to push to amd-staging-drm-next? Or should it go to drm-misc? If drm-misc, I need your help to push it since I have no permission to write. -David > >> --- >> drivers/gpu/drm/drm_syncobj.c | 34 ++++++++++++++++++++-------------- >> include/uapi/drm/drm.h | 3 ++- >> 2 files changed, 22 insertions(+), 15 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_syncobj.c >> b/drivers/gpu/drm/drm_syncobj.c >> index 3d400905100b..3fc8f66ada68 100644 >> --- a/drivers/gpu/drm/drm_syncobj.c >> +++ b/drivers/gpu/drm/drm_syncobj.c >> @@ -1197,7 +1197,7 @@ drm_syncobj_timeline_signal_ioctl(struct >> drm_device *dev, void *data, >> if (!drm_core_check_feature(dev, DRIVER_SYNCOBJ_TIMELINE)) >> return -EOPNOTSUPP; >> - if (args->pad != 0) >> + if (args->flags != 0) >> return -EINVAL; >> if (args->count_handles == 0) >> @@ -1268,7 +1268,7 @@ int drm_syncobj_query_ioctl(struct drm_device >> *dev, void *data, >> if (!drm_core_check_feature(dev, DRIVER_SYNCOBJ_TIMELINE)) >> return -EOPNOTSUPP; >> - if (args->pad != 0) >> + if (args->flags & ~DRM_SYNCOBJ_QUERY_FLAGS_LAST_SUBMITTED) >> return -EINVAL; >> if (args->count_handles == 0) >> @@ -1291,23 +1291,29 @@ int drm_syncobj_query_ioctl(struct drm_device >> *dev, void *data, >> if (chain) { >> struct dma_fence *iter, *last_signaled = NULL; >> - dma_fence_chain_for_each(iter, fence) { >> - if (!iter) >> - break; >> - dma_fence_put(last_signaled); >> - last_signaled = dma_fence_get(iter); >> - if (!to_dma_fence_chain(last_signaled)->prev_seqno) >> - /* It is most likely that timeline has >> - * unorder points. */ >> - break; >> + if (args->flags & >> + DRM_SYNCOBJ_QUERY_FLAGS_LAST_SUBMITTED) { >> + point = fence->seqno; >> + } else { >> + dma_fence_chain_for_each(iter, fence) { >> + if (!iter) >> + break; >> + dma_fence_put(last_signaled); >> + last_signaled = dma_fence_get(iter); >> + if (!to_dma_fence_chain(last_signaled)->prev_seqno) >> + /* It is most likely that timeline has >> + * unorder points. */ >> + break; >> + } >> + point = dma_fence_is_signaled(last_signaled) ? >> + last_signaled->seqno : >> + to_dma_fence_chain(last_signaled)->prev_seqno; >> } >> - point = dma_fence_is_signaled(last_signaled) ? >> - last_signaled->seqno : >> - to_dma_fence_chain(last_signaled)->prev_seqno; >> dma_fence_put(last_signaled); >> } else { >> point = 0; >> } >> + dma_fence_put(fence); >> ret = copy_to_user(&points[i], &point, sizeof(uint64_t)); >> ret = ret ? -EFAULT : 0; >> if (ret) >> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h >> index 661d73f9a919..fd987ce24d9f 100644 >> --- a/include/uapi/drm/drm.h >> +++ b/include/uapi/drm/drm.h >> @@ -777,11 +777,12 @@ struct drm_syncobj_array { >> __u32 pad; >> }; >> +#define DRM_SYNCOBJ_QUERY_FLAGS_LAST_SUBMITTED (1 << 0) /* last >> available point on timeline syncobj */ >> struct drm_syncobj_timeline_array { >> __u64 handles; >> __u64 points; >> __u32 count_handles; >> - __u32 pad; >> + __u32 flags; >> }; > >
Am 24.07.19 um 08:32 schrieb zhoucm1: > > > On 2019年07月24日 03:20, Lionel Landwerlin wrote: >> On 23/07/2019 17:21, Chunming Zhou wrote: >>> user space needs a flexiable query ability. >>> So that umd can get last signaled or submitted point. >>> v2: >>> add sanitizer checking. >>> >>> Change-Id: I6512b430524ebabe715e602a2bf5abb0a7e780ea >>> Signed-off-by: Chunming Zhou <david1.zhou@amd.com> >>> Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com> >>> Cc: Christian König <Christian.Koenig@amd.com> >> >> Reviewed-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com> > > Thanks. > Which branch should this patch go to? > Is it OK to push to amd-staging-drm-next? > Or should it go to drm-misc? > If drm-misc, I need your help to push it since I have no permission to > write. drm-misc-next is probably more appropriated. I'm going to take of that, but first let me push my fix now. Christian. > > -David >> >>> --- >>> drivers/gpu/drm/drm_syncobj.c | 34 ++++++++++++++++++++-------------- >>> include/uapi/drm/drm.h | 3 ++- >>> 2 files changed, 22 insertions(+), 15 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/drm_syncobj.c >>> b/drivers/gpu/drm/drm_syncobj.c >>> index 3d400905100b..3fc8f66ada68 100644 >>> --- a/drivers/gpu/drm/drm_syncobj.c >>> +++ b/drivers/gpu/drm/drm_syncobj.c >>> @@ -1197,7 +1197,7 @@ drm_syncobj_timeline_signal_ioctl(struct >>> drm_device *dev, void *data, >>> if (!drm_core_check_feature(dev, DRIVER_SYNCOBJ_TIMELINE)) >>> return -EOPNOTSUPP; >>> - if (args->pad != 0) >>> + if (args->flags != 0) >>> return -EINVAL; >>> if (args->count_handles == 0) >>> @@ -1268,7 +1268,7 @@ int drm_syncobj_query_ioctl(struct drm_device >>> *dev, void *data, >>> if (!drm_core_check_feature(dev, DRIVER_SYNCOBJ_TIMELINE)) >>> return -EOPNOTSUPP; >>> - if (args->pad != 0) >>> + if (args->flags & ~DRM_SYNCOBJ_QUERY_FLAGS_LAST_SUBMITTED) >>> return -EINVAL; >>> if (args->count_handles == 0) >>> @@ -1291,23 +1291,29 @@ int drm_syncobj_query_ioctl(struct >>> drm_device *dev, void *data, >>> if (chain) { >>> struct dma_fence *iter, *last_signaled = NULL; >>> - dma_fence_chain_for_each(iter, fence) { >>> - if (!iter) >>> - break; >>> - dma_fence_put(last_signaled); >>> - last_signaled = dma_fence_get(iter); >>> - if (!to_dma_fence_chain(last_signaled)->prev_seqno) >>> - /* It is most likely that timeline has >>> - * unorder points. */ >>> - break; >>> + if (args->flags & >>> + DRM_SYNCOBJ_QUERY_FLAGS_LAST_SUBMITTED) { >>> + point = fence->seqno; >>> + } else { >>> + dma_fence_chain_for_each(iter, fence) { >>> + if (!iter) >>> + break; >>> + dma_fence_put(last_signaled); >>> + last_signaled = dma_fence_get(iter); >>> + if >>> (!to_dma_fence_chain(last_signaled)->prev_seqno) >>> + /* It is most likely that timeline has >>> + * unorder points. */ >>> + break; >>> + } >>> + point = dma_fence_is_signaled(last_signaled) ? >>> + last_signaled->seqno : >>> + to_dma_fence_chain(last_signaled)->prev_seqno; >>> } >>> - point = dma_fence_is_signaled(last_signaled) ? >>> - last_signaled->seqno : >>> - to_dma_fence_chain(last_signaled)->prev_seqno; >>> dma_fence_put(last_signaled); >>> } else { >>> point = 0; >>> } >>> + dma_fence_put(fence); >>> ret = copy_to_user(&points[i], &point, sizeof(uint64_t)); >>> ret = ret ? -EFAULT : 0; >>> if (ret) >>> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h >>> index 661d73f9a919..fd987ce24d9f 100644 >>> --- a/include/uapi/drm/drm.h >>> +++ b/include/uapi/drm/drm.h >>> @@ -777,11 +777,12 @@ struct drm_syncobj_array { >>> __u32 pad; >>> }; >>> +#define DRM_SYNCOBJ_QUERY_FLAGS_LAST_SUBMITTED (1 << 0) /* last >>> available point on timeline syncobj */ >>> struct drm_syncobj_timeline_array { >>> __u64 handles; >>> __u64 points; >>> __u32 count_handles; >>> - __u32 pad; >>> + __u32 flags; >>> }; >> >> >
On Wed, Jul 24, 2019 at 07:48:23AM +0000, Koenig, Christian wrote: > Am 24.07.19 um 08:32 schrieb zhoucm1: > > > > > > On 2019年07月24日 03:20, Lionel Landwerlin wrote: > >> On 23/07/2019 17:21, Chunming Zhou wrote: > >>> user space needs a flexiable query ability. > >>> So that umd can get last signaled or submitted point. > >>> v2: > >>> add sanitizer checking. > >>> > >>> Change-Id: I6512b430524ebabe715e602a2bf5abb0a7e780ea > >>> Signed-off-by: Chunming Zhou <david1.zhou@amd.com> > >>> Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com> > >>> Cc: Christian König <Christian.Koenig@amd.com> > >> > >> Reviewed-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com> > > > > Thanks. > > Which branch should this patch go to? > > Is it OK to push to amd-staging-drm-next? > > Or should it go to drm-misc? > > If drm-misc, I need your help to push it since I have no permission to > > write. > > drm-misc-next is probably more appropriated. > > I'm going to take of that, but first let me push my fix now. btw we once discussed to hide the syncobj stuff until the userspace side is public too. Not seeing that merged anywhere. Can you pls take care of this? I'm not against developing new uapi in upstream, that's occasionally just the most reasonable thing to do here. But we are quite badly flaunting the entire "needs open source consumer in userspace" rule here ... -Daniel > > Christian. > > > > > -David > >> > >>> --- > >>> drivers/gpu/drm/drm_syncobj.c | 34 ++++++++++++++++++++-------------- > >>> include/uapi/drm/drm.h | 3 ++- > >>> 2 files changed, 22 insertions(+), 15 deletions(-) > >>> > >>> diff --git a/drivers/gpu/drm/drm_syncobj.c > >>> b/drivers/gpu/drm/drm_syncobj.c > >>> index 3d400905100b..3fc8f66ada68 100644 > >>> --- a/drivers/gpu/drm/drm_syncobj.c > >>> +++ b/drivers/gpu/drm/drm_syncobj.c > >>> @@ -1197,7 +1197,7 @@ drm_syncobj_timeline_signal_ioctl(struct > >>> drm_device *dev, void *data, > >>> if (!drm_core_check_feature(dev, DRIVER_SYNCOBJ_TIMELINE)) > >>> return -EOPNOTSUPP; > >>> - if (args->pad != 0) > >>> + if (args->flags != 0) > >>> return -EINVAL; > >>> if (args->count_handles == 0) > >>> @@ -1268,7 +1268,7 @@ int drm_syncobj_query_ioctl(struct drm_device > >>> *dev, void *data, > >>> if (!drm_core_check_feature(dev, DRIVER_SYNCOBJ_TIMELINE)) > >>> return -EOPNOTSUPP; > >>> - if (args->pad != 0) > >>> + if (args->flags & ~DRM_SYNCOBJ_QUERY_FLAGS_LAST_SUBMITTED) > >>> return -EINVAL; > >>> if (args->count_handles == 0) > >>> @@ -1291,23 +1291,29 @@ int drm_syncobj_query_ioctl(struct > >>> drm_device *dev, void *data, > >>> if (chain) { > >>> struct dma_fence *iter, *last_signaled = NULL; > >>> - dma_fence_chain_for_each(iter, fence) { > >>> - if (!iter) > >>> - break; > >>> - dma_fence_put(last_signaled); > >>> - last_signaled = dma_fence_get(iter); > >>> - if (!to_dma_fence_chain(last_signaled)->prev_seqno) > >>> - /* It is most likely that timeline has > >>> - * unorder points. */ > >>> - break; > >>> + if (args->flags & > >>> + DRM_SYNCOBJ_QUERY_FLAGS_LAST_SUBMITTED) { > >>> + point = fence->seqno; > >>> + } else { > >>> + dma_fence_chain_for_each(iter, fence) { > >>> + if (!iter) > >>> + break; > >>> + dma_fence_put(last_signaled); > >>> + last_signaled = dma_fence_get(iter); > >>> + if > >>> (!to_dma_fence_chain(last_signaled)->prev_seqno) > >>> + /* It is most likely that timeline has > >>> + * unorder points. */ > >>> + break; > >>> + } > >>> + point = dma_fence_is_signaled(last_signaled) ? > >>> + last_signaled->seqno : > >>> + to_dma_fence_chain(last_signaled)->prev_seqno; > >>> } > >>> - point = dma_fence_is_signaled(last_signaled) ? > >>> - last_signaled->seqno : > >>> - to_dma_fence_chain(last_signaled)->prev_seqno; > >>> dma_fence_put(last_signaled); > >>> } else { > >>> point = 0; > >>> } > >>> + dma_fence_put(fence); > >>> ret = copy_to_user(&points[i], &point, sizeof(uint64_t)); > >>> ret = ret ? -EFAULT : 0; > >>> if (ret) > >>> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h > >>> index 661d73f9a919..fd987ce24d9f 100644 > >>> --- a/include/uapi/drm/drm.h > >>> +++ b/include/uapi/drm/drm.h > >>> @@ -777,11 +777,12 @@ struct drm_syncobj_array { > >>> __u32 pad; > >>> }; > >>> +#define DRM_SYNCOBJ_QUERY_FLAGS_LAST_SUBMITTED (1 << 0) /* last > >>> available point on timeline syncobj */ > >>> struct drm_syncobj_timeline_array { > >>> __u64 handles; > >>> __u64 points; > >>> __u32 count_handles; > >>> - __u32 pad; > >>> + __u32 flags; > >>> }; > >> > >> > > > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
在 2019/7/25 20:21, Daniel Vetter 写道: > On Wed, Jul 24, 2019 at 07:48:23AM +0000, Koenig, Christian wrote: >> Am 24.07.19 um 08:32 schrieb zhoucm1: >>> >>> On 2019年07月24日 03:20, Lionel Landwerlin wrote: >>>> On 23/07/2019 17:21, Chunming Zhou wrote: >>>>> user space needs a flexiable query ability. >>>>> So that umd can get last signaled or submitted point. >>>>> v2: >>>>> add sanitizer checking. >>>>> >>>>> Change-Id: I6512b430524ebabe715e602a2bf5abb0a7e780ea >>>>> Signed-off-by: Chunming Zhou <david1.zhou@amd.com> >>>>> Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com> >>>>> Cc: Christian König <Christian.Koenig@amd.com> >>>> Reviewed-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com> >>> Thanks. >>> Which branch should this patch go to? >>> Is it OK to push to amd-staging-drm-next? >>> Or should it go to drm-misc? >>> If drm-misc, I need your help to push it since I have no permission to >>> write. >> drm-misc-next is probably more appropriated. >> >> I'm going to take of that, but first let me push my fix now. > btw we once discussed to hide the syncobj stuff until the userspace side > is public too. Not seeing that merged anywhere. Can you pls take care of > this? > > I'm not against developing new uapi in upstream, that's occasionally just > the most reasonable thing to do here. But we are quite badly flaunting the > entire "needs open source consumer in userspace" rule here ... Hi Daniel, I got your means. The enablement feature flag is already removed from drm-misc, so it shouldn't have other effect. If only amd driver stuff and only amd uses that, we can flexiblely deal with that. Since syncobj is moved to drm level, we are working here. I think you should already see that timeline syncobj is also used by Intel ANV, and Lionel is co-working with us. If we hide these stuff, how do we know who is working on that? How do we share one same implementation? Where should we discuss? everyone implements own's desgin behind? -David > -Daniel > >> Christian. >> >>> -David >>>>> --- >>>>> drivers/gpu/drm/drm_syncobj.c | 34 ++++++++++++++++++++-------------- >>>>> include/uapi/drm/drm.h | 3 ++- >>>>> 2 files changed, 22 insertions(+), 15 deletions(-) >>>>> >>>>> diff --git a/drivers/gpu/drm/drm_syncobj.c >>>>> b/drivers/gpu/drm/drm_syncobj.c >>>>> index 3d400905100b..3fc8f66ada68 100644 >>>>> --- a/drivers/gpu/drm/drm_syncobj.c >>>>> +++ b/drivers/gpu/drm/drm_syncobj.c >>>>> @@ -1197,7 +1197,7 @@ drm_syncobj_timeline_signal_ioctl(struct >>>>> drm_device *dev, void *data, >>>>> if (!drm_core_check_feature(dev, DRIVER_SYNCOBJ_TIMELINE)) >>>>> return -EOPNOTSUPP; >>>>> - if (args->pad != 0) >>>>> + if (args->flags != 0) >>>>> return -EINVAL; >>>>> if (args->count_handles == 0) >>>>> @@ -1268,7 +1268,7 @@ int drm_syncobj_query_ioctl(struct drm_device >>>>> *dev, void *data, >>>>> if (!drm_core_check_feature(dev, DRIVER_SYNCOBJ_TIMELINE)) >>>>> return -EOPNOTSUPP; >>>>> - if (args->pad != 0) >>>>> + if (args->flags & ~DRM_SYNCOBJ_QUERY_FLAGS_LAST_SUBMITTED) >>>>> return -EINVAL; >>>>> if (args->count_handles == 0) >>>>> @@ -1291,23 +1291,29 @@ int drm_syncobj_query_ioctl(struct >>>>> drm_device *dev, void *data, >>>>> if (chain) { >>>>> struct dma_fence *iter, *last_signaled = NULL; >>>>> - dma_fence_chain_for_each(iter, fence) { >>>>> - if (!iter) >>>>> - break; >>>>> - dma_fence_put(last_signaled); >>>>> - last_signaled = dma_fence_get(iter); >>>>> - if (!to_dma_fence_chain(last_signaled)->prev_seqno) >>>>> - /* It is most likely that timeline has >>>>> - * unorder points. */ >>>>> - break; >>>>> + if (args->flags & >>>>> + DRM_SYNCOBJ_QUERY_FLAGS_LAST_SUBMITTED) { >>>>> + point = fence->seqno; >>>>> + } else { >>>>> + dma_fence_chain_for_each(iter, fence) { >>>>> + if (!iter) >>>>> + break; >>>>> + dma_fence_put(last_signaled); >>>>> + last_signaled = dma_fence_get(iter); >>>>> + if >>>>> (!to_dma_fence_chain(last_signaled)->prev_seqno) >>>>> + /* It is most likely that timeline has >>>>> + * unorder points. */ >>>>> + break; >>>>> + } >>>>> + point = dma_fence_is_signaled(last_signaled) ? >>>>> + last_signaled->seqno : >>>>> + to_dma_fence_chain(last_signaled)->prev_seqno; >>>>> } >>>>> - point = dma_fence_is_signaled(last_signaled) ? >>>>> - last_signaled->seqno : >>>>> - to_dma_fence_chain(last_signaled)->prev_seqno; >>>>> dma_fence_put(last_signaled); >>>>> } else { >>>>> point = 0; >>>>> } >>>>> + dma_fence_put(fence); >>>>> ret = copy_to_user(&points[i], &point, sizeof(uint64_t)); >>>>> ret = ret ? -EFAULT : 0; >>>>> if (ret) >>>>> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h >>>>> index 661d73f9a919..fd987ce24d9f 100644 >>>>> --- a/include/uapi/drm/drm.h >>>>> +++ b/include/uapi/drm/drm.h >>>>> @@ -777,11 +777,12 @@ struct drm_syncobj_array { >>>>> __u32 pad; >>>>> }; >>>>> +#define DRM_SYNCOBJ_QUERY_FLAGS_LAST_SUBMITTED (1 << 0) /* last >>>>> available point on timeline syncobj */ >>>>> struct drm_syncobj_timeline_array { >>>>> __u64 handles; >>>>> __u64 points; >>>>> __u32 count_handles; >>>>> - __u32 pad; >>>>> + __u32 flags; >>>>> }; >>>> >> _______________________________________________ >> dri-devel mailing list >> dri-devel@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Thu, Jul 25, 2019 at 5:01 PM Chunming Zhou <zhoucm1@amd.com> wrote: > > > 在 2019/7/25 20:21, Daniel Vetter 写道: > > On Wed, Jul 24, 2019 at 07:48:23AM +0000, Koenig, Christian wrote: > >> Am 24.07.19 um 08:32 schrieb zhoucm1: > >>> > >>> On 2019年07月24日 03:20, Lionel Landwerlin wrote: > >>>> On 23/07/2019 17:21, Chunming Zhou wrote: > >>>>> user space needs a flexiable query ability. > >>>>> So that umd can get last signaled or submitted point. > >>>>> v2: > >>>>> add sanitizer checking. > >>>>> > >>>>> Change-Id: I6512b430524ebabe715e602a2bf5abb0a7e780ea > >>>>> Signed-off-by: Chunming Zhou <david1.zhou@amd.com> > >>>>> Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com> > >>>>> Cc: Christian König <Christian.Koenig@amd.com> > >>>> Reviewed-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com> > >>> Thanks. > >>> Which branch should this patch go to? > >>> Is it OK to push to amd-staging-drm-next? > >>> Or should it go to drm-misc? > >>> If drm-misc, I need your help to push it since I have no permission to > >>> write. > >> drm-misc-next is probably more appropriated. > >> > >> I'm going to take of that, but first let me push my fix now. > > btw we once discussed to hide the syncobj stuff until the userspace side > > is public too. Not seeing that merged anywhere. Can you pls take care of > > this? > > > > I'm not against developing new uapi in upstream, that's occasionally just > > the most reasonable thing to do here. But we are quite badly flaunting the > > entire "needs open source consumer in userspace" rule here ... > > Hi Daniel, > > I got your means. The enablement feature flag is already removed from > drm-misc, so it shouldn't have other effect. Ah I totally missed the patch from Alex which just reverts it. I thought the plan was to have a module options that taints the kernel when you enable this code. That's what we've done for atomic (or any of the other hidden uapi). With the current approach we're essentially merging dead code, which doesn't sound like a good idea really. Iirc there was a patch from Lionel to add that module option, but I didn't see it land. That's why I asked. > If only amd driver stuff and only amd uses that, we can flexiblely deal > with that. > > Since syncobj is moved to drm level, we are working here. I think you > should already see that timeline syncobj is also used by Intel ANV, and > Lionel is co-working with us. If we hide these stuff, how do we know who > is working on that? How do we share one same implementation? Where > should we discuss? everyone implements own's desgin behind? Uh ... what are you talking about? Nowhere did I propose to hide stuff and not work together ... how did you come up with that idea? I'm confused. -Daniel > > -David > > > -Daniel > > > >> Christian. > >> > >>> -David > >>>>> --- > >>>>> drivers/gpu/drm/drm_syncobj.c | 34 ++++++++++++++++++++-------------- > >>>>> include/uapi/drm/drm.h | 3 ++- > >>>>> 2 files changed, 22 insertions(+), 15 deletions(-) > >>>>> > >>>>> diff --git a/drivers/gpu/drm/drm_syncobj.c > >>>>> b/drivers/gpu/drm/drm_syncobj.c > >>>>> index 3d400905100b..3fc8f66ada68 100644 > >>>>> --- a/drivers/gpu/drm/drm_syncobj.c > >>>>> +++ b/drivers/gpu/drm/drm_syncobj.c > >>>>> @@ -1197,7 +1197,7 @@ drm_syncobj_timeline_signal_ioctl(struct > >>>>> drm_device *dev, void *data, > >>>>> if (!drm_core_check_feature(dev, DRIVER_SYNCOBJ_TIMELINE)) > >>>>> return -EOPNOTSUPP; > >>>>> - if (args->pad != 0) > >>>>> + if (args->flags != 0) > >>>>> return -EINVAL; > >>>>> if (args->count_handles == 0) > >>>>> @@ -1268,7 +1268,7 @@ int drm_syncobj_query_ioctl(struct drm_device > >>>>> *dev, void *data, > >>>>> if (!drm_core_check_feature(dev, DRIVER_SYNCOBJ_TIMELINE)) > >>>>> return -EOPNOTSUPP; > >>>>> - if (args->pad != 0) > >>>>> + if (args->flags & ~DRM_SYNCOBJ_QUERY_FLAGS_LAST_SUBMITTED) > >>>>> return -EINVAL; > >>>>> if (args->count_handles == 0) > >>>>> @@ -1291,23 +1291,29 @@ int drm_syncobj_query_ioctl(struct > >>>>> drm_device *dev, void *data, > >>>>> if (chain) { > >>>>> struct dma_fence *iter, *last_signaled = NULL; > >>>>> - dma_fence_chain_for_each(iter, fence) { > >>>>> - if (!iter) > >>>>> - break; > >>>>> - dma_fence_put(last_signaled); > >>>>> - last_signaled = dma_fence_get(iter); > >>>>> - if (!to_dma_fence_chain(last_signaled)->prev_seqno) > >>>>> - /* It is most likely that timeline has > >>>>> - * unorder points. */ > >>>>> - break; > >>>>> + if (args->flags & > >>>>> + DRM_SYNCOBJ_QUERY_FLAGS_LAST_SUBMITTED) { > >>>>> + point = fence->seqno; > >>>>> + } else { > >>>>> + dma_fence_chain_for_each(iter, fence) { > >>>>> + if (!iter) > >>>>> + break; > >>>>> + dma_fence_put(last_signaled); > >>>>> + last_signaled = dma_fence_get(iter); > >>>>> + if > >>>>> (!to_dma_fence_chain(last_signaled)->prev_seqno) > >>>>> + /* It is most likely that timeline has > >>>>> + * unorder points. */ > >>>>> + break; > >>>>> + } > >>>>> + point = dma_fence_is_signaled(last_signaled) ? > >>>>> + last_signaled->seqno : > >>>>> + to_dma_fence_chain(last_signaled)->prev_seqno; > >>>>> } > >>>>> - point = dma_fence_is_signaled(last_signaled) ? > >>>>> - last_signaled->seqno : > >>>>> - to_dma_fence_chain(last_signaled)->prev_seqno; > >>>>> dma_fence_put(last_signaled); > >>>>> } else { > >>>>> point = 0; > >>>>> } > >>>>> + dma_fence_put(fence); > >>>>> ret = copy_to_user(&points[i], &point, sizeof(uint64_t)); > >>>>> ret = ret ? -EFAULT : 0; > >>>>> if (ret) > >>>>> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h > >>>>> index 661d73f9a919..fd987ce24d9f 100644 > >>>>> --- a/include/uapi/drm/drm.h > >>>>> +++ b/include/uapi/drm/drm.h > >>>>> @@ -777,11 +777,12 @@ struct drm_syncobj_array { > >>>>> __u32 pad; > >>>>> }; > >>>>> +#define DRM_SYNCOBJ_QUERY_FLAGS_LAST_SUBMITTED (1 << 0) /* last > >>>>> available point on timeline syncobj */ > >>>>> struct drm_syncobj_timeline_array { > >>>>> __u64 handles; > >>>>> __u64 points; > >>>>> __u32 count_handles; > >>>>> - __u32 pad; > >>>>> + __u32 flags; > >>>>> }; > >>>> > >> _______________________________________________ > >> dri-devel mailing list > >> dri-devel@lists.freedesktop.org > >> https://lists.freedesktop.org/mailman/listinfo/dri-devel
diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c index 3d400905100b..3fc8f66ada68 100644 --- a/drivers/gpu/drm/drm_syncobj.c +++ b/drivers/gpu/drm/drm_syncobj.c @@ -1197,7 +1197,7 @@ drm_syncobj_timeline_signal_ioctl(struct drm_device *dev, void *data, if (!drm_core_check_feature(dev, DRIVER_SYNCOBJ_TIMELINE)) return -EOPNOTSUPP; - if (args->pad != 0) + if (args->flags != 0) return -EINVAL; if (args->count_handles == 0) @@ -1268,7 +1268,7 @@ int drm_syncobj_query_ioctl(struct drm_device *dev, void *data, if (!drm_core_check_feature(dev, DRIVER_SYNCOBJ_TIMELINE)) return -EOPNOTSUPP; - if (args->pad != 0) + if (args->flags & ~DRM_SYNCOBJ_QUERY_FLAGS_LAST_SUBMITTED) return -EINVAL; if (args->count_handles == 0) @@ -1291,23 +1291,29 @@ int drm_syncobj_query_ioctl(struct drm_device *dev, void *data, if (chain) { struct dma_fence *iter, *last_signaled = NULL; - dma_fence_chain_for_each(iter, fence) { - if (!iter) - break; - dma_fence_put(last_signaled); - last_signaled = dma_fence_get(iter); - if (!to_dma_fence_chain(last_signaled)->prev_seqno) - /* It is most likely that timeline has - * unorder points. */ - break; + if (args->flags & + DRM_SYNCOBJ_QUERY_FLAGS_LAST_SUBMITTED) { + point = fence->seqno; + } else { + dma_fence_chain_for_each(iter, fence) { + if (!iter) + break; + dma_fence_put(last_signaled); + last_signaled = dma_fence_get(iter); + if (!to_dma_fence_chain(last_signaled)->prev_seqno) + /* It is most likely that timeline has + * unorder points. */ + break; + } + point = dma_fence_is_signaled(last_signaled) ? + last_signaled->seqno : + to_dma_fence_chain(last_signaled)->prev_seqno; } - point = dma_fence_is_signaled(last_signaled) ? - last_signaled->seqno : - to_dma_fence_chain(last_signaled)->prev_seqno; dma_fence_put(last_signaled); } else { point = 0; } + dma_fence_put(fence); ret = copy_to_user(&points[i], &point, sizeof(uint64_t)); ret = ret ? -EFAULT : 0; if (ret) diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h index 661d73f9a919..fd987ce24d9f 100644 --- a/include/uapi/drm/drm.h +++ b/include/uapi/drm/drm.h @@ -777,11 +777,12 @@ struct drm_syncobj_array { __u32 pad; }; +#define DRM_SYNCOBJ_QUERY_FLAGS_LAST_SUBMITTED (1 << 0) /* last available point on timeline syncobj */ struct drm_syncobj_timeline_array { __u64 handles; __u64 points; __u32 count_handles; - __u32 pad; + __u32 flags; };
user space needs a flexiable query ability. So that umd can get last signaled or submitted point. v2: add sanitizer checking. Change-Id: I6512b430524ebabe715e602a2bf5abb0a7e780ea Signed-off-by: Chunming Zhou <david1.zhou@amd.com> Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com> Cc: Christian König <Christian.Koenig@amd.com> --- drivers/gpu/drm/drm_syncobj.c | 34 ++++++++++++++++++++-------------- include/uapi/drm/drm.h | 3 ++- 2 files changed, 22 insertions(+), 15 deletions(-)