Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Mon, 9 Apr 2018 11:14:17 +0200
From:   Daniel Vetter <daniel@ffwll.ch>
To:     Keith Packard <keithp@keithp.com>
Cc:     linux-kernel@vger.kernel.org, Dave Airlie <airlied@redhat.com>,
        Daniel Vetter <daniel@ffwll.ch>,
        dri-devel@lists.freedesktop.org
Subject: Re: [PATCH 0/1] drm: Add crtc_queue_syncobj and crtc_get_syncobj
 ioctls
Message-ID: <20180409091416.GT31310@phenom.ffwll.local>
Mail-Followup-To: Keith Packard <keithp@keithp.com>,
        linux-kernel@vger.kernel.org, Dave Airlie <airlied@redhat.com>,
        dri-devel@lists.freedesktop.org
References: <20180406235649.9494-1-keithp@keithp.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20180406235649.9494-1-keithp@keithp.com>
User-Agent: Mutt/1.9.4 (2018-02-28)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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