On Fri, Apr 06, 2018 at 04:56:48PM -0700, Keith Packard wrote: > (This is an RFC on whether this pair of ioctls seems reasonable. The > code compiles, but I haven't tested it as I'm away from home this > weekend.) > > I'm rewriting my implementation of the Vulkan EXT_display_control > extension, which provides a way to signal a Vulkan fence at vblank > time. I had implemented it using events, but that isn't great as the > Vulkan API includes the ability to wait for any of a set of fences to > be signaled. As the other Vulkan fences are implemented using > dma_fences in the kernel, and (eventually) using syncobj up in user > space, it seems reasonable to use syncobjs for everything and hook > vblank to those. > > In any case, I'm proposing two new syncobj/vblank ioctls (the names > aren't great; suggestions welcome, as usual): > > DRM_IOCTL_CRTC_QUEUE_SYNCOBJ > > Create a new syncobj that will be signaled at (or after) the > specified vblank sequence value. This uses the same parameters > to specify the target sequence as > DRM_IOCTL_CRTC_QUEUE_SEQUENCE. My understanding of drm_syncobj is that you: - Create a syncobj with the drm_syncobj_create ioctl. - Pass it around to various driver callbacks who update the attached dma_fence pointer using drm_syncobj_replace_fence(). Yes amdgpu does this a bit differently, but that seems to be the exception. >From that pov I'd massage the uapi to just extend drm_crtc_queue_sequence_ioctl with a new syncobj flag. Syncobj we can just add at the bottom of struct drm_crtc_queue_sequence (drm structs can be extended like that, it's part of the uapi). Or we reuse user_data, but that's a bit a hack. We also don't need a new event type, vblank code simply singals event->fence, which we'll arrange to be the fence behind the syncobj. > DRM_IOCTL_CRTC_GET_SYNCOBJ > > Once the above syncobj has been signaled, this ioctl allows > the application to find out when that happened, returning both > the vblank sequence count and time (in ns). The android syncpt stuff already had the concept of a fence timestamp, and we carried it over as part of struct dma_fence.timestamp. It's just not exposed yet as proper uapi. I think we should aim a bit more into that direction with something like the below sketch: - Add a dma_fence_signal_ts, which allows us to set the timestamp from a hw clock. - Use that in the vblank code. - Add new drm_syncobj ioctl to query the timestamp of the attached fence (if it's signalled). That would entirely avoid the special-case ioctl just for vblank syncobj here. Also, this might be useful in your implementation of VK_GOOGLE_display_timing, since the current query timestamp you're using won't take into account irq wakeup latency. Using fence->timestamp will still miss the irq->atomic worker wakupe latency, but should be a lot better already. > I'd like to hear comments on whether this seems reasonable, or whether > I should go in some other direction. Besides a few bikesheds to align better with other stuff going around I think this looks good. -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
