From patchwork Thu Jul 6 16:27:11 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Keith Packard X-Patchwork-Id: 9828607 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id CEF9E60317 for ; Thu, 6 Jul 2017 16:27:16 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id BD9372861B for ; Thu, 6 Jul 2017 16:27:16 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id B1CE3286E6; Thu, 6 Jul 2017 16:27:16 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-4.2 required=2.0 tests=BAYES_00, RCVD_IN_DNSWL_MED autolearn=ham version=3.3.1 Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher DHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id EB3112861B for ; Thu, 6 Jul 2017 16:27:15 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 0D51B6E625; Thu, 6 Jul 2017 16:27:15 +0000 (UTC) X-Original-To: dri-devel@lists.freedesktop.org Delivered-To: dri-devel@lists.freedesktop.org Received: from elaine.keithp.com (home.keithp.com [63.227.221.253]) by gabe.freedesktop.org (Postfix) with ESMTP id 7042E6E625 for ; Thu, 6 Jul 2017 16:27:13 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by elaine.keithp.com (Postfix) with ESMTP id 4D4B13F205DF; Thu, 6 Jul 2017 09:27:12 -0700 (PDT) X-Virus-Scanned: Debian amavisd-new at keithp.com Received: from elaine.keithp.com ([127.0.0.1]) by localhost (elaine.keithp.com [127.0.0.1]) (amavisd-new, port 10024) with LMTP id vIuN6cSdQUe8; Thu, 6 Jul 2017 09:27:11 -0700 (PDT) Received: from hiro.keithp.com (hiro.keithp.com [10.0.0.36]) by elaine.keithp.com (Postfix) with ESMTPSA id 56B233F203B9; Thu, 6 Jul 2017 09:27:11 -0700 (PDT) Received: by hiro.keithp.com (Postfix, from userid 1001) id 3D3EC19011CC; Thu, 6 Jul 2017 09:27:12 -0700 (PDT) From: Keith Packard To: Daniel Vetter Subject: Re: [PATCH 3/3] drm: Add CRTC_GET_SEQUENCE and CRTC_QUEUE_SEQUENCE ioctls In-Reply-To: <20170706075313.bn2exiklfabgc25t@phenom.ffwll.local> References: <20170705221013.27940-1-keithp@keithp.com> <20170705221013.27940-4-keithp@keithp.com> <20170706075313.bn2exiklfabgc25t@phenom.ffwll.local> Date: Thu, 06 Jul 2017 09:27:11 -0700 Message-ID: <86y3s1bkf4.fsf@keithp.com> MIME-Version: 1.0 Cc: Dave Airlie , dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" X-Virus-Scanned: ClamAV using ClamSMTP Daniel Vetter writes: > I very much like this since the old ioctl really is a rather bad horror > show. And since it's tied in with ums drivers everything is > complicated. Thanks for your kind words. > I started a discussion a while back whether these should be restricted to > DRM_MASTER (i.e. the modeset resource owner) or available to everyone. > Since it's read-only I guess we can keep it accessible to everyone, but it > has a bit the problem that client app developers see this, think it does > what it does and then use it to schedule frames without asking the > compositor. Which sometimes even works, but isn't really proper design. > The reasons seems to be that on X11 there's no EGL extension for accurate > timing frame updates (DRI2/3 can do it ofc, and glx exposes it, but glx is > uncool or something like that). In the absence of a suitable EGL api, I'm not sure what to suggest, other than fixing EGL instead of blaming the kernel... However, for the leasing stuff, this doesn't really matter as I've got a master FD to play with, so if you wanted to restrict it to master, that'd be fine by me. >> + >> +/* >> + * Get crtc VBLANK count. >> + * >> + * \param dev DRM device >> + * \param data user arguement, pointing to a drm_crtc_get_sequence structure. >> + * \param file_priv drm file private for the user's open file descriptor >> + */ > > Since this stuff isn't parsed by kerneldoc I tend to just free-form ioctl > comments completely. Someday maybe someone even gets around to doing > proper uabi documentation :-) Just an aside. I'm just trying to follow along with the local "conventions" in the file. Let me know if you have a future plan to make this better and I'll just reformat to suit. >> + >> +int drm_crtc_get_sequence_ioctl(struct drm_device *dev, void *data, >> + struct drm_file *file_priv) >> +{ >> + struct drm_crtc *crtc; >> + int pipe; >> + struct drm_crtc_get_sequence *get_seq = data; >> + struct timespec now; >> + > > You need a DRIVER_MODESET check here or the drm_crtc_find will oops. Same > below. Like this? >> + /* Check for valid signal edge */ >> + if (!(flags & DRM_CRTC_SEQUENCE_FIRST_PIXEL_OUT)) >> + return -EINVAL; > > This seems new, maybe drop it until we need it? It's part of Vulkan, so I want to expose it in the kernel API. And, making sure user-space isn't setting unexpected bits seems like a good idea. > drm_crtc_vblank_get pls, would be sweet if we can avoid the (dev, pipe) > pairs as much as possible. Same for all the others. Sure. I'll note that this is just a wrapper around drm_vblank_get/put at this point :-) > I think here there's no need for the accurate version, since we > force-enable the vblanks already. Agreed. >> + e->pipe = pipe; >> + e->event.base.type = DRM_EVENT_CRTC_SEQUENCE; >> + e->event.base.length = sizeof(e->event.seq); >> + e->event.seq.user_data = queue_seq->user_data; > > No need for crtc_id in this event? Or do we expect userspace to encode > that in the user_data somehow? I feared changing the size of the event and so I left that out. And, yes, userspace will almost certainly encode a pointer in the user_data, which can include whatever information it needs. > But might be useful just for consistency. This would require making the event larger, which seems like a bad idea... >> +#define DRM_CRTC_SEQUENCE_FIRST_PIXEL_OUT 0x00000004 /* Signal when first pixel is displayed */ > > I thought this is already the semantics our current vblank events have (in > the timestamp, when exactly the event comes out isn't defined further than > "somewhere around vblank"). Since it's unsed I'd just remove this > #define. Vulkan has this single define, but may have more in the future so I wanted to make sure the application was able to tell if the kernel supported new modes if/when they appear. Reliably returning -EINVAL today when the application asks for something which isn't supported seems like good practice. > In both ioctl handlers pls make sure everything you don't look at is 0, > including unused stuff like pad. Otherwise userspace might fail to clear > them, and we can never use them in the future. Maybe just rename pad to > flags right away. Good idea. Above, you asked me to return whether the crtc was active in the get_sequence ioctl; I suggested putting that in the existing pad field, which would leave the whole structure defined. I've got tiny patches for each piece which I've stuck on my drm-sequence-64 branch at git://people.freedesktop.org/~keithp/linux.git drm-sequence-64 Most of those are included above, with the exception of the drm_crtc_vblank_get/put changes. Thanks much for your careful review. diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c index e39b2bd074e4..3738ff484f36 100644 --- a/drivers/gpu/drm/drm_vblank.c +++ b/drivers/gpu/drm/drm_vblank.c @@ -1712,6 +1712,9 @@ int drm_crtc_get_sequence_ioctl(struct drm_device *dev, void *data, struct drm_crtc_get_sequence *get_seq = data; struct timespec now; + if (!drm_core_check_feature(dev, DRIVER_MODESET)) + return -EINVAL; + if (!dev->irq_enabled) return -EINVAL; @@ -1749,6 +1752,9 @@ int drm_crtc_queue_sequence_ioctl(struct drm_device *dev, void *data, int ret; unsigned long spin_flags; + if (!drm_core_check_feature(dev, DRIVER_MODESET)) + return -EINVAL; + if (!dev->irq_enabled) return -EINVAL; >> + get_seq->sequence = drm_vblank_count_and_time(dev, pipe, &now); > > This can give you and old vblank if the vblank is off (i.e. sw state > hasn't be regularly updated). I think we want a new > drm_crtc_accurate_vblank_count_and_time variant. Right, I saw that code in the wait_vblank case and forgot to carry it over. Here's a duplicate of what that function does; we'll need this code in any case for drivers which don't provide the necessary support for accurate vblank counts: --- a/drivers/gpu/drm/drm_vblank.c +++ b/drivers/gpu/drm/drm_vblank.c @@ -1711,6 +1711,8 @@ int drm_crtc_get_sequence_ioctl(struct drm_device *dev, void *data, int pipe; struct drm_crtc_get_sequence *get_seq = data; struct timespec now; + bool vblank_enabled; + int ret; if (!drm_core_check_feature(dev, DRIVER_MODESET)) return -EINVAL; @@ -1724,8 +1726,19 @@ int drm_crtc_get_sequence_ioctl(struct drm_device *dev, void *data, pipe = drm_crtc_index(crtc); + vblank_enabled = dev->vblank_disable_immediately && READ_ONCE(vblank->enabled); + + if (!vblank_enabled) { + ret = drm_vblank_get(dev, pipe); + if (ret) { + DRM_DEBUG("crtc %d failed to acquire vblank counter, %d\n", pipe, ret); + return ret; + } + } get_seq->sequence = drm_vblank_count_and_time(dev, pipe, &now); get_seq->sequence_ns = timespec_to_ns(&now); + if (!vblank_enabled) + drm_vblank_put(dev, pipe); return 0; } > Another thing that is very ill-defined in the old vblank ioctl and that we > could fix here: Asking for vblanks when the CRTC is off is a bit silly. > Asking for the sequence when it's off makes some sense, but might still be > good to give userspace some indication in the new struct? This also from > the pov of the unpriviledge vblank waiter use-case that I wondered about > earlier. Hrm. It's certainly easy to do, however an application using this without knowing the enabled state of the crtc is probably not doing the right thing... Here's what that looks like, I think, in case we want to do this: --- a/drivers/gpu/drm/drm_vblank.c +++ b/drivers/gpu/drm/drm_vblank.c @@ -1735,6 +1735,12 @@ int drm_crtc_get_sequence_ioctl(struct drm_device *dev, void *data, return ret; } } + drm_modeset_lock(&crtc->mutex, NULL); + if (crtc->state) + get_seq->active = crtc->state->enable; + else + get_seq->active = crtc->enabled; + drm_modeset_unlock(&crtc->mutex); get_seq->sequence = drm_vblank_count_and_time(dev, pipe, &now); get_seq->sequence_ns = timespec_to_ns(&now); if (!vblank_enabled)