From: Daniel Vetter <daniel@ffwll.ch>
To: Keith Packard <keithp@keithp.com>,
linux-kernel@vger.kernel.org, Dave Airlie <airlied@redhat.com>,
dri-devel@lists.freedesktop.org
Subject: Re: [PATCH 0/1] drm: Add crtc_queue_syncobj and crtc_get_syncobj ioctls
Date: Tue, 24 Apr 2018 19:45:14 +0200 [thread overview]
Message-ID: <20180424174514.GB25142@phenom.ffwll.local> (raw)
In-Reply-To: <20180409091416.GT31310@phenom.ffwll.local>
On Mon, Apr 09, 2018 at 11:14:17AM +0200, Daniel Vetter wrote:
> 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.
Ok, I'm going to retract my entire suggestion above for the GET ioctl. It
would neatly work for vk_google_display_timing, but the KHR version of
that extensions very much sounds like we want the kernel to report 2 (or
maybe even 3) different timestamps back. An ioctl is much easier to extend
than shoe-horning all that into the dma_fence/drm_syncobj abstraction.
One bikeshed maybe on top: Maybe call the ioctl CRTC_GET_TS_DATA, since
that's what you're getting, not the syncobj.
Sorry for flopping around on this, just learned this all in discussions
past week.
-Daniel
>
> > 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
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
prev parent reply other threads:[~2018-04-24 17:45 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-04-06 23:56 [PATCH 0/1] drm: Add crtc_queue_syncobj and crtc_get_syncobj ioctls Keith Packard
2018-04-06 23:56 ` [PATCH] " Keith Packard
[not found] ` <CAOFGe943O_premhAP0JLV4QMTqxxr3vgm2o_-7m1_S=dpj7dEw@mail.gmail.com>
2018-04-07 2:51 ` Keith Packard
[not found] ` <CAOFGe94tJG-GELF-RnokWOQHjrbAMOZp2EGM2=Vgh=hP0Ne1jg@mail.gmail.com>
2018-04-08 2:31 ` Keith Packard
2018-04-09 9:14 ` [PATCH 0/1] " Daniel Vetter
2018-04-24 17:45 ` Daniel Vetter [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20180424174514.GB25142@phenom.ffwll.local \
--to=daniel@ffwll.ch \
--cc=airlied@redhat.com \
--cc=dri-devel@lists.freedesktop.org \
--cc=keithp@keithp.com \
--cc=linux-kernel@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).