Received: by 2002:a25:8b91:0:0:0:0:0 with SMTP id j17csp5432286ybl; Tue, 14 Jan 2020 08:50:53 -0800 (PST) X-Google-Smtp-Source: APXvYqww54Rl40Yn5+4dhy+1LXx7Gu8NVZu0tK/538OrO9irhyTely7Wxr4di3QfqzeP3qGjF+bp X-Received: by 2002:a05:6830:1515:: with SMTP id k21mr16351041otp.177.1579020653475; Tue, 14 Jan 2020 08:50:53 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1579020653; cv=none; d=google.com; s=arc-20160816; b=CGGmlN5vsFMxVEpbsvZceroeA3YI5L0feXWM0b7nwLhyfo8r2GPV+Sbr6GBz3eP/fO Ex6bgd44H6rUMtxWDRSdgafezewA7TArHNCPvBb8+x1dUAdgE6KRAYDwVjC1aOTsq3b3 dYzQ6NG6FhOp66tK4cpHxS8mKcpeaQnmP0rBI8+uuNkpQL+0rZI5Pt/+BldL0W2kyPYo Sg+sNAbEJbdzmJoCjogtOvVlV2+HewE6EgAH/rgFYcqhlNSaNGRZ/jK0EuTpCGIxp550 siuPHEO54qRF/4fRn7uwlZxSnuXtSfg3Hy94+3wXSzdkwpkaiiEG0vfcY6xFzAeUMJyy aYrg== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:dkim-signature; bh=2I42vpGfXb9/ZHBDOiHtgHbR+MXS22hQ7Kw201plqiQ=; b=PwkpWYd+fvd26U7WZQKPRYWevIgeHaasprgT46FCl73R/yk0NIq9WFoXmPlBwGb+Zv JxmAz8EHx+JfQki9V6m6v0uI2Ex4ifIwhaCU7/xIC/6aocGGuRPSZyg7wSa/G3yIO5pI OCD/qwoD6s/vgKIzGU90u9eKg5Kc3uo35Y2qdx2HiR1BqgUu+92XvTdecsC5/xuSEAHj poaUXc2Vo3lmhBIC+9EkYPl7fG+sFYOxauXdW15NjkbtDpPgaeVdrOw6me0kevUMyNY5 e3qxaSVj3CxI6r4ibErC65evM8u3+hS7Ta0nfFtq1z4FdqDeHWBniQTQGNtOkUMuHw+z oirg== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@chromium.org header.s=google header.b=Cz6sxrqA; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=chromium.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id 17si9842114otv.149.2020.01.14.08.50.42; Tue, 14 Jan 2020 08:50:53 -0800 (PST) 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; Authentication-Results: mx.google.com; dkim=pass header.i=@chromium.org header.s=google header.b=Cz6sxrqA; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=chromium.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728895AbgANQtA (ORCPT + 99 others); Tue, 14 Jan 2020 11:49:00 -0500 Received: from mail-io1-f66.google.com ([209.85.166.66]:35908 "EHLO mail-io1-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1728848AbgANQtA (ORCPT ); Tue, 14 Jan 2020 11:49:00 -0500 Received: by mail-io1-f66.google.com with SMTP id d15so14589349iog.3 for ; Tue, 14 Jan 2020 08:48:59 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=2I42vpGfXb9/ZHBDOiHtgHbR+MXS22hQ7Kw201plqiQ=; b=Cz6sxrqAb7LCUQiu1Gc9r2whIiuWlFwRAExhb1Xci/cHNNOYBUrN1GHNe57QvtnyXk DhogmGlU6CVzi8R3rYenmNJ2BT0HpoQO08aWl5PaftQuDkh+F8By1hzOJLuq7TI50qX3 QyYudstWPwyEVnaZ9mKZWETrLifTcmaeE+WEM= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=2I42vpGfXb9/ZHBDOiHtgHbR+MXS22hQ7Kw201plqiQ=; b=L1d2dwNzWCoGsAgqtazdhRR2pElMXEOnNR/DB8alTrh09E5977rEh1O230aPkbDibJ TXjD3zjIARivdtF1s6ArulnTsxPqaaY0gyemco9gm1Q/nByd1vCFCFdmQ5lLdQGMKdCl qgd9gWQP/Mp9YRRvw0K64SEUlcLWaeCMU95vkZ0k1k5ZoiopNULhNhe0EF/Lo9gv7Nfc h59G7FqzA8DjhbcvOPTCRFI6bDuSeBkfFq7vCXzNsgxnU6SU778EagYhMI9oZj5hJOTR Qae9MrjID4U/2CdzJqCxbhC+efkITyJZa3h1HXnewK7MfEZjbU9rOQ5NfNlaIsPyxfbZ gpew== X-Gm-Message-State: APjAAAVzbuyrPcrAWTT776tnvEBsbTGkhQnOCtMK8+GbZ7D68fpexnL8 UDWrDdG2/PhRDDVZKfKWpaDtRxrczuZdQzXk5uMkiw== X-Received: by 2002:a02:3312:: with SMTP id c18mr19408348jae.24.1579020539044; Tue, 14 Jan 2020 08:48:59 -0800 (PST) MIME-Version: 1.0 References: <20200113153605.52350-1-brian@brkho.com> <20200113153605.52350-3-brian@brkho.com> <20200113225516.GA157345@google.com> <20200114164034.GA213224@google.com> In-Reply-To: <20200114164034.GA213224@google.com> From: Rob Clark Date: Tue, 14 Jan 2020 08:48:48 -0800 Message-ID: Subject: Re: [PATCH 2/2] drm/msm: Add MSM_WAIT_IOVA ioctl To: Brian Ho Cc: Kristian Kristensen , freedreno , hoegsberg , Rob Clark , Sean Paul , David Airlie , Daniel Vetter , "open list:DRM DRIVER FOR MSM ADRENO GPU" , "open list:DRM DRIVER FOR MSM ADRENO GPU" , open list Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Jan 14, 2020 at 8:40 AM Brian Ho wrote: > > On Mon, Jan 13, 2020 at 03:17:38PM -0800, Rob Clark wrote: > > On Mon, Jan 13, 2020 at 2:55 PM Brian Ho wrote: > > > > > > On Mon, Jan 13, 2020 at 09:57:43AM -0800, Kristian Kristensen wrote: > > > > On Mon, Jan 13, 2020 at 8:25 AM Rob Clark wrote: > > > > > > > > > On Mon, Jan 13, 2020 at 7:37 AM Brian Ho wrote: > > > > > > > > > > > > Implements an ioctl to wait until a value at a given iova is greater > > > > > > than or equal to a supplied value. > > > > > > > > > > > > This will initially be used by turnip (open-source Vulkan driver for > > > > > > QC in mesa) for occlusion queries where the userspace driver can > > > > > > block on a query becoming available before continuing via > > > > > > vkGetQueryPoolResults. > > > > > > > > > > > > Signed-off-by: Brian Ho > > > > > > --- > > > > > > drivers/gpu/drm/msm/msm_drv.c | 63 +++++++++++++++++++++++++++++++++-- > > > > > > include/uapi/drm/msm_drm.h | 13 ++++++++ > > > > > > 2 files changed, 74 insertions(+), 2 deletions(-) > > > > > > > > > > > > diff --git a/drivers/gpu/drm/msm/msm_drv.c > > > > > b/drivers/gpu/drm/msm/msm_drv.c > > > > > > index c84f0a8b3f2c..dcc46874a5a2 100644 > > > > > > --- a/drivers/gpu/drm/msm/msm_drv.c > > > > > > +++ b/drivers/gpu/drm/msm/msm_drv.c > > > > > > @@ -36,10 +36,11 @@ > > > > > > * MSM_GEM_INFO ioctl. > > > > > > * - 1.4.0 - softpin, MSM_RELOC_BO_DUMP, and GEM_INFO support to set/get > > > > > > * GEM object's debug name > > > > > > - * - 1.5.0 - Add SUBMITQUERY_QUERY ioctl > > > > > > + * - 1.5.0 - Add SUBMITQUEUE_QUERY ioctl > > > > > > + * - 1.6.0 - Add WAIT_IOVA ioctl > > > > > > */ > > > > > > #define MSM_VERSION_MAJOR 1 > > > > > > -#define MSM_VERSION_MINOR 5 > > > > > > +#define MSM_VERSION_MINOR 6 > > > > > > #define MSM_VERSION_PATCHLEVEL 0 > > > > > > > > > > > > static const struct drm_mode_config_funcs mode_config_funcs = { > > > > > > @@ -952,6 +953,63 @@ static int msm_ioctl_submitqueue_close(struct > > > > > drm_device *dev, void *data, > > > > > > return msm_submitqueue_remove(file->driver_priv, id); > > > > > > } > > > > > > > > > > > > +static int msm_ioctl_wait_iova(struct drm_device *dev, void *data, > > > > > > + struct drm_file *file) > > > > > > +{ > > > > > > + struct msm_drm_private *priv = dev->dev_private; > > > > > > + struct drm_gem_object *obj; > > > > > > + struct drm_msm_wait_iova *args = data; > > > > > > + ktime_t timeout = to_ktime(args->timeout); > > > > > > + unsigned long remaining_jiffies = timeout_to_jiffies(&timeout); > > > > > > + struct msm_gpu *gpu = priv->gpu; > > > > > > + void *base_vaddr; > > > > > > + uint64_t *vaddr; > > > > > > + int ret; > > > > > > + > > > > > > + if (args->pad) > > > > > > + return -EINVAL; > > > > > > + > > > > > > + if (!gpu) > > > > > > + return 0; > > > > > > > > > > hmm, I'm not sure we should return zero in this case.. maybe -ENODEV? > > > > > > > > > > > + > > > > > > + obj = drm_gem_object_lookup(file, args->handle); > > > > > > + if (!obj) > > > > > > + return -ENOENT; > > > > > > + > > > > > > + base_vaddr = msm_gem_get_vaddr(obj); > > > > > > + if (IS_ERR(base_vaddr)) { > > > > > > + ret = PTR_ERR(base_vaddr); > > > > > > + goto err_put_gem_object; > > > > > > + } > > > > > > + if (args->offset + sizeof(*vaddr) > obj->size) { > > > > > > + ret = -EINVAL; > > > > > > + goto err_put_vaddr; > > > > > > + } > > > > > > + > > > > > > + vaddr = base_vaddr + args->offset; > > > > > > + > > > > > > + /* Assumes WC mapping */ > > > > > > + ret = wait_event_interruptible_timeout( > > > > > > + gpu->event, *vaddr >= args->value, > > > > > remaining_jiffies); > > > > > > > > > > > > > This needs to do the awkward looking > > > > > > > > (int64_t)(*data - value) >= 0 > > > > > > > > to properly handle the wraparound case. > > > > > > > > > > I think this comparison will run into issues if we allow for 64-bit > > > reference values. For example, if value is ULLONG_MAX, and *data > > > starts at 0 on the first comparison, we'll immediately return. > > > > > > It's not too much of an issue in fence_completed (msm_fence.c), but > > > in this ioctl, *data can grow at an arbitrary rate. Are we concerned > > > about this? > > > > > > > > + > > > > > > + if (ret == 0) { > > > > > > + ret = -ETIMEDOUT; > > > > > > + goto err_put_vaddr; > > > > > > + } else if (ret == -ERESTARTSYS) { > > > > > > + goto err_put_vaddr; > > > > > > + } > > > > > > > > > > maybe: > > > > > > > > > > } else { > > > > > ret = 0; > > > > > } > > > > > > > > > > and then drop the next three lines? > > > > > > > > > > > + > > > > > > + msm_gem_put_vaddr(obj); > > > > > > + drm_gem_object_put_unlocked(obj); > > > > > > + return 0; > > > > > > + > > > > > > +err_put_vaddr: > > > > > > + msm_gem_put_vaddr(obj); > > > > > > +err_put_gem_object: > > > > > > + drm_gem_object_put_unlocked(obj); > > > > > > + return ret; > > > > > > +} > > > > > > + > > > > > > static const struct drm_ioctl_desc msm_ioctls[] = { > > > > > > DRM_IOCTL_DEF_DRV(MSM_GET_PARAM, msm_ioctl_get_param, > > > > > DRM_RENDER_ALLOW), > > > > > > DRM_IOCTL_DEF_DRV(MSM_GEM_NEW, msm_ioctl_gem_new, > > > > > DRM_RENDER_ALLOW), > > > > > > @@ -964,6 +1022,7 @@ static const struct drm_ioctl_desc msm_ioctls[] = { > > > > > > DRM_IOCTL_DEF_DRV(MSM_SUBMITQUEUE_NEW, > > > > > msm_ioctl_submitqueue_new, DRM_RENDER_ALLOW), > > > > > > DRM_IOCTL_DEF_DRV(MSM_SUBMITQUEUE_CLOSE, > > > > > msm_ioctl_submitqueue_close, DRM_RENDER_ALLOW), > > > > > > DRM_IOCTL_DEF_DRV(MSM_SUBMITQUEUE_QUERY, > > > > > msm_ioctl_submitqueue_query, DRM_RENDER_ALLOW), > > > > > > + DRM_IOCTL_DEF_DRV(MSM_WAIT_IOVA, msm_ioctl_wait_iova, > > > > > DRM_RENDER_ALLOW), > > > > > > }; > > > > > > > > > > > > static const struct vm_operations_struct vm_ops = { > > > > > > diff --git a/include/uapi/drm/msm_drm.h b/include/uapi/drm/msm_drm.h > > > > > > index 0b85ed6a3710..8477f28a4ee1 100644 > > > > > > --- a/include/uapi/drm/msm_drm.h > > > > > > +++ b/include/uapi/drm/msm_drm.h > > > > > > @@ -298,6 +298,17 @@ struct drm_msm_submitqueue_query { > > > > > > __u32 pad; > > > > > > }; > > > > > > > > > > > > +/* This ioctl blocks until the u64 value at bo + offset is greater than > > > > > or > > > > > > + * equal to the reference value. > > > > > > + */ > > > > > > +struct drm_msm_wait_iova { > > > > > > + __u32 handle; /* in, GEM handle */ > > > > > > + __u32 pad; > > > > > > + struct drm_msm_timespec timeout; /* in */ > > > > > > + __u64 offset; /* offset into bo */ > > > > > > + __u64 value; /* reference value */ > > > > > > > > > > Maybe we should go ahead and add a __u64 mask; > > > > > > > > > > that would let us wait for 32b values as well, and wait for bits in a > > > > > bitmask > > > > > > > > > > > > > I think we'd be OK to just default to 32 bit values instead, since most of > > > > the CP commands that this is intended to work with (CP_EVENT_WRITE, > > > > CP_WAIT_MEM_GTE etc) operate on 32 bit values. We could move 'value' to the > > > > slot right after 'handle' but then we'd not have any pad/reserved fields. > > > > Maybe we keep 'value' 64 bit but restrict it to 32 bits, with an option to > > > > add a 64 bit flag in 'pad' later on? > > > > > > > > > > FWIW, the current usage of this in my mesa MR uses a 64 bit value. > > > There's no super great reason that the available bit is 64 bits and > > > not 32 bits (I think it made the addressing math a bit simpler), but > > > I'm fine with whatever you all decide on here. > > > > > > > I assume you are waiting for a fence value written w/ CP_EVENT_WRITE? > > Or at least that is what I'd recommend. That would be 32b > > > > BR, > > -R > > > > So it's actually a little bit different than that. I allocate a bo > for the occlusion query which has an "availability" bit (0 for > unavailable, 1 for available). When the occlusion query ends, we > write the fragments passed value to the bo via CP_EVENT_WRITE and > then wait for that write to complete before setting the available > bit to 1 via a simple CP_MEM_WRITE [1]. > > It's that CP_MEM_WRITE that I plan on waiting on with this new iova > ioctl. > > [1] https://gitlab.freedesktop.org/mesa/mesa/blob/768106c50a5569796bb6d5e04b5e4d65c1d00ea0/src/freedreno/vulkan/tu_query.c#L529 > hmm, interesting.. I had in mind something more like: https://gitlab.freedesktop.org/drm/msm/blob/msm-next/drivers/gpu/drm/msm/adreno/a6xx_gpu.c#L137 The high bit in the first dword of the packet (which we probably shouldn't open-code) is the "give me an irq after the value in last dword is written to memory".. (I haven't checked yet whether we can use the "gimme an irq" bit from userspace) BR, -R