Re: [PATCH 0/1] drm: Add crtc_queue_syncobj and crtc_get_syncobj ioctls

[Daniel Vetter <daniel@ffwll.ch>]

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