From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-13.6 required=3.0 tests=BAYES_00,DKIM_INVALID, DKIM_SIGNED,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 2F0BAC07E99 for ; Mon, 12 Jul 2021 15:07:23 +0000 (UTC) Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id DB8A3611CC for ; Mon, 12 Jul 2021 15:07:22 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org DB8A3611CC Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=jlekstrand.net Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=dri-devel-bounces@lists.freedesktop.org Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 5F3698972B; Mon, 12 Jul 2021 15:07:22 +0000 (UTC) Received: from mail-yb1-xb34.google.com (mail-yb1-xb34.google.com [IPv6:2607:f8b0:4864:20::b34]) by gabe.freedesktop.org (Postfix) with ESMTPS id B6D3989733 for ; Mon, 12 Jul 2021 15:07:21 +0000 (UTC) Received: by mail-yb1-xb34.google.com with SMTP id g5so29450379ybu.10 for ; Mon, 12 Jul 2021 08:07:21 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=jlekstrand-net.20150623.gappssmtp.com; s=20150623; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=lhobAOl+mjClAm3Zzwd0eWj9BCTZt7LJmhiyF2rBZlQ=; b=oeSgsS3eKvCrpjPwDtkUVZB7O9Wh8HabklA8RUu+ohICARuQs7WEqovnFiRd5+LIpL a6uTlFnw8OiXgU9H5F15KRHe5v0EnkuJ3A4Ps/hJ5f5SuRqiOyPMzh/0dnj8Udq4G+3/ r2I5FK/pMYmD6lk9pgE19VOiglEKaUuMiAaArSZ5k2EdbKvCg4jM/r7T66eYDf5FfBqo ZEsNjyXs96vtXAo1wF5ykb7ASp9rMnAwzzTUtyzAbjwexjmK5x5TmltJIOTrGOh7alNC kVsFD2oojIccEffIKjJKDkwLLu06MijXI4fe71EqR5mraZG1xYou1Vmr9O1ZiXC1Kck5 htxg== 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=lhobAOl+mjClAm3Zzwd0eWj9BCTZt7LJmhiyF2rBZlQ=; b=repSBcL4gkqYV6BKFnkncMr6GSZzR8hE0x/48cOH9ZFirafHBASgrnV0GNtTiSirpU /969DjC4VRtrGmR0nyMgG2xq24ha13SXD0TiJdPL8fENWQ5RJVdnbvLXpuY4mHRwqK/8 QKoLQaX+ZovIzVcIYbEEzll2NlkjnEwiN21RsxZo2qMxrt/Gnj+sInLfky0yY191a+Rb BV612/5w6SRBZsU7KYThLkWsfNk2TJLtflw/fTn1RmgQ4hCTW6VGvfULpEPBXQ1DG0AQ 2lYHOUQ6a+NOPT2EWgRpDu745gHnzp75tXc243TBUzqz09H8Mi2oegSJB/yYxuEJ5VpN tiyg== X-Gm-Message-State: AOAM533QNaeHmapmQoojarGQ3ha0y+HfK/UsOeSfTe0v2lPic/JYXl3z E9xLE01sQnAnSk3e21VZD7afnq0Q5QRMcaH1v6JHoQ== X-Google-Smtp-Source: ABdhPJzFMBfhxqlYwEtHSPqwwI3jitun2HjJMdyFRglEQ7D+Op+IcfYPZ5Xqr4zFjfaXGYX2ohRpqsOEn6iYlrUjVv8= X-Received: by 2002:a25:2f89:: with SMTP id v131mr67843894ybv.469.1626102440702; Mon, 12 Jul 2021 08:07:20 -0700 (PDT) MIME-Version: 1.0 References: <20210709114120.651309-1-maarten.lankhorst@linux.intel.com> In-Reply-To: From: Jason Ekstrand Date: Mon, 12 Jul 2021 10:07:09 -0500 Message-ID: Subject: Re: [Intel-gfx] [PATCH v2] drm/i915: Add TTM offset argument to mmap. To: Daniel Vetter Content-Type: text/plain; charset="UTF-8" X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Intel GFX , Maling list - DRI developers Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" On Mon, Jul 12, 2021 at 9:38 AM Daniel Vetter wrote: > > On Mon, Jul 12, 2021 at 09:31:13AM -0500, Jason Ekstrand wrote: > > On Mon, Jul 12, 2021 at 9:12 AM Daniel Vetter wrote: > > > > > > On Mon, Jul 12, 2021 at 08:47:24AM -0500, Jason Ekstrand wrote: > > > > On Fri, Jul 9, 2021 at 6:41 AM Maarten Lankhorst > > > > wrote: > > > > > > > > > > This is only used for ttm, and tells userspace that the mapping type is > > > > > ignored. This disables the other type of mmap offsets when discrete > > > > > memory is used, so fix the selftests as well. > > > > > > > > > > Document the struct as well, so it shows up in docbook correctly. > > > > > > > > > > Changes since v1: > > > > > - Add docbook entries. > > > > > > > > > > Signed-off-by: Maarten Lankhorst > > > > > --- > > > > > drivers/gpu/drm/i915/gem/i915_gem_mman.c | 17 ++++++- > > > > > .../gpu/drm/i915/gem/i915_gem_object_types.h | 1 + > > > > > .../drm/i915/gem/selftests/i915_gem_mman.c | 27 +++++++++- > > > > > include/uapi/drm/i915_drm.h | 51 +++++++++++++++---- > > > > > 4 files changed, 82 insertions(+), 14 deletions(-) > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.c b/drivers/gpu/drm/i915/gem/i915_gem_mman.c > > > > > index a90f796e85c0..b34be9e5d094 100644 > > > > > --- a/drivers/gpu/drm/i915/gem/i915_gem_mman.c > > > > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.c > > > > > @@ -679,10 +679,16 @@ __assign_mmap_offset(struct drm_i915_gem_object *obj, > > > > > return -ENODEV; > > > > > > > > > > if (obj->ops->mmap_offset) { > > > > > + if (mmap_type != I915_MMAP_TYPE_TTM) > > > > > + return -ENODEV; > > > > > + > > > > > *offset = obj->ops->mmap_offset(obj); > > > > > return 0; > > > > > } > > > > > > > > > > + if (mmap_type == I915_MMAP_TYPE_TTM) > > > > > + return -ENODEV; > > > > > + > > > > > if (mmap_type != I915_MMAP_TYPE_GTT && > > > > > !i915_gem_object_has_struct_page(obj) && > > > > > !i915_gem_object_has_iomem(obj)) > > > > > @@ -727,7 +733,9 @@ i915_gem_dumb_mmap_offset(struct drm_file *file, > > > > > { > > > > > enum i915_mmap_type mmap_type; > > > > > > > > > > - if (boot_cpu_has(X86_FEATURE_PAT)) > > > > > + if (HAS_LMEM(to_i915(dev))) > > > > > + mmap_type = I915_MMAP_TYPE_TTM; > > > > > + else if (boot_cpu_has(X86_FEATURE_PAT)) > > > > > mmap_type = I915_MMAP_TYPE_WC; > > > > > else if (!i915_ggtt_has_aperture(&to_i915(dev)->ggtt)) > > > > > return -ENODEV; > > > > > @@ -798,6 +806,10 @@ i915_gem_mmap_offset_ioctl(struct drm_device *dev, void *data, > > > > > type = I915_MMAP_TYPE_UC; > > > > > break; > > > > > > > > > > + case I915_MMAP_OFFSET_TTM: > > > > > + type = I915_MMAP_TYPE_TTM; > > > > > + break; > > > > > + > > > > > default: > > > > > return -EINVAL; > > > > > } > > > > > @@ -968,6 +980,9 @@ int i915_gem_mmap(struct file *filp, struct vm_area_struct *vma) > > > > > vma->vm_ops = &vm_ops_cpu; > > > > > break; > > > > > > > > > > + case I915_MMAP_TYPE_TTM: > > > > > + GEM_WARN_ON(mmo->mmap_type == I915_MMAP_TYPE_TTM); > > > > > + /* fall-through */ > > > > > case I915_MMAP_TYPE_WB: > > > > > vma->vm_page_prot = vm_get_page_prot(vma->vm_flags); > > > > > vma->vm_ops = &vm_ops_cpu; > > > > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h > > > > > index ef3de2ae9723..d4c42bcdfeb6 100644 > > > > > --- a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h > > > > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h > > > > > @@ -105,6 +105,7 @@ enum i915_mmap_type { > > > > > I915_MMAP_TYPE_WC, > > > > > I915_MMAP_TYPE_WB, > > > > > I915_MMAP_TYPE_UC, > > > > > + I915_MMAP_TYPE_TTM, > > > > > }; > > > > > > > > > > struct i915_mmap_offset { > > > > > diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c b/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c > > > > > index 1da8bd675e54..27a35d88e5f5 100644 > > > > > --- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c > > > > > +++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c > > > > > @@ -573,6 +573,14 @@ static int make_obj_busy(struct drm_i915_gem_object *obj) > > > > > return 0; > > > > > } > > > > > > > > > > +static enum i915_mmap_type default_mapping(struct drm_i915_private *i915) > > > > > +{ > > > > > + if (HAS_LMEM(i915)) > > > > > + return I915_MMAP_TYPE_TTM; > > > > > + > > > > > + return I915_MMAP_TYPE_GTT; > > > > > +} > > > > > + > > > > > static bool assert_mmap_offset(struct drm_i915_private *i915, > > > > > unsigned long size, > > > > > int expected) > > > > > @@ -585,7 +593,7 @@ static bool assert_mmap_offset(struct drm_i915_private *i915, > > > > > if (IS_ERR(obj)) > > > > > return expected && expected == PTR_ERR(obj); > > > > > > > > > > - ret = __assign_mmap_offset(obj, I915_MMAP_TYPE_GTT, &offset, NULL); > > > > > + ret = __assign_mmap_offset(obj, default_mapping(i915), &offset, NULL); > > > > > i915_gem_object_put(obj); > > > > > > > > > > return ret == expected; > > > > > @@ -689,7 +697,7 @@ static int igt_mmap_offset_exhaustion(void *arg) > > > > > goto out; > > > > > } > > > > > > > > > > - err = __assign_mmap_offset(obj, I915_MMAP_TYPE_GTT, &offset, NULL); > > > > > + err = __assign_mmap_offset(obj, default_mapping(i915), &offset, NULL); > > > > > if (err) { > > > > > pr_err("Unable to insert object into reclaimed hole\n"); > > > > > goto err_obj; > > > > > @@ -831,8 +839,14 @@ static int wc_check(struct drm_i915_gem_object *obj) > > > > > > > > > > static bool can_mmap(struct drm_i915_gem_object *obj, enum i915_mmap_type type) > > > > > { > > > > > + struct drm_i915_private *i915 = to_i915(obj->base.dev); > > > > > bool no_map; > > > > > > > > > > + if (HAS_LMEM(i915)) > > > > > + return type == I915_MMAP_TYPE_TTM; > > > > > + else if (type == I915_MMAP_TYPE_TTM) > > > > > + return false; > > > > > + > > > > > if (type == I915_MMAP_TYPE_GTT && > > > > > !i915_ggtt_has_aperture(&to_i915(obj->base.dev)->ggtt)) > > > > > return false; > > > > > @@ -970,6 +984,8 @@ static int igt_mmap(void *arg) > > > > > err = __igt_mmap(i915, obj, I915_MMAP_TYPE_GTT); > > > > > if (err == 0) > > > > > err = __igt_mmap(i915, obj, I915_MMAP_TYPE_WC); > > > > > + if (err == 0) > > > > > + err = __igt_mmap(i915, obj, I915_MMAP_TYPE_TTM); > > > > > > > > > > i915_gem_object_put(obj); > > > > > if (err) > > > > > @@ -987,6 +1003,7 @@ static const char *repr_mmap_type(enum i915_mmap_type type) > > > > > case I915_MMAP_TYPE_WB: return "wb"; > > > > > case I915_MMAP_TYPE_WC: return "wc"; > > > > > case I915_MMAP_TYPE_UC: return "uc"; > > > > > + case I915_MMAP_TYPE_TTM: return "ttm"; > > > > > default: return "unknown"; > > > > > } > > > > > } > > > > > @@ -1100,6 +1117,8 @@ static int igt_mmap_access(void *arg) > > > > > err = __igt_mmap_access(i915, obj, I915_MMAP_TYPE_WC); > > > > > if (err == 0) > > > > > err = __igt_mmap_access(i915, obj, I915_MMAP_TYPE_UC); > > > > > + if (err == 0) > > > > > + err = __igt_mmap_access(i915, obj, I915_MMAP_TYPE_TTM); > > > > > > > > > > i915_gem_object_put(obj); > > > > > if (err) > > > > > @@ -1241,6 +1260,8 @@ static int igt_mmap_gpu(void *arg) > > > > > err = __igt_mmap_gpu(i915, obj, I915_MMAP_TYPE_GTT); > > > > > if (err == 0) > > > > > err = __igt_mmap_gpu(i915, obj, I915_MMAP_TYPE_WC); > > > > > + if (err == 0) > > > > > + err = __igt_mmap_gpu(i915, obj, I915_MMAP_TYPE_TTM); > > > > > > > > > > i915_gem_object_put(obj); > > > > > if (err) > > > > > @@ -1396,6 +1417,8 @@ static int igt_mmap_revoke(void *arg) > > > > > err = __igt_mmap_revoke(i915, obj, I915_MMAP_TYPE_GTT); > > > > > if (err == 0) > > > > > err = __igt_mmap_revoke(i915, obj, I915_MMAP_TYPE_WC); > > > > > + if (err == 0) > > > > > + err = __igt_mmap_revoke(i915, obj, I915_MMAP_TYPE_TTM); > > > > > > > > > > i915_gem_object_put(obj); > > > > > if (err) > > > > > diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h > > > > > index e334a8b14ef2..1610ed40b4b5 100644 > > > > > --- a/include/uapi/drm/i915_drm.h > > > > > +++ b/include/uapi/drm/i915_drm.h > > > > > @@ -849,31 +849,60 @@ struct drm_i915_gem_mmap_gtt { > > > > > __u64 offset; > > > > > }; > > > > > > > > > > +/** > > > > > + * struct drm_i915_gem_mmap_offset - Retrieve an offset so we can mmap this buffer object. > > > > > + * > > > > > + * This struct is passed as argument to the `DRM_IOCTL_I915_GEM_MMAP_OFFSET` ioctl, > > > > > + * and is used to retrieve the fake offset to mmap an object specified by &handle. > > > > > + * > > > > > + * The legacy way of using `DRM_IOCTL_I915_GEM_MMAP` is removed on gen12+. > > > > > + * `DRM_IOCTL_I915_GEM_MMAP_GTT` is an older supported alias to this struct, but will behave > > > > > + * as setting the &extensions to 0, and &flags to `I915_MMAP_OFFSET_GTT`. > > > > > + */ > > > > > struct drm_i915_gem_mmap_offset { > > > > > - /** Handle for the object being mapped. */ > > > > > + /** @handle: Handle for the object being mapped. */ > > > > > __u32 handle; > > > > > + /** @pad: Must be zero */ > > > > > __u32 pad; > > > > > /** > > > > > - * Fake offset to use for subsequent mmap call > > > > > + * @offset: The fake offset to use for subsequent mmap call > > > > > * > > > > > * This is a fixed-size type for 32/64 compatibility. > > > > > */ > > > > > __u64 offset; > > > > > > > > > > /** > > > > > - * Flags for extended behaviour. > > > > > + * @flags: Flags for extended behaviour. > > > > > + * > > > > > + * It is mandatory that one of the `MMAP_OFFSET` types > > > > > + * should be included: > > > > > + * - `I915_MMAP_OFFSET_GTT`: Use mmap with the object bound to GTT. > > > > > + * - `I915_MMAP_OFFSET_WC`: Use Write-Combined caching. > > > > > + * - `I915_MMAP_OFFSET_WB`: Use Write-Back caching. > > > > > + * - `I915_MMAP_OFFSET_TTM`: Use TTM to determine caching based on object placement. > > > > > + * > > > > > + * Only on devices with local memory is `I915_MMAP_OFFSET_TTM` valid. On > > > > > + * devices without local memory, this caching mode is invalid. > > > > > * > > > > > - * It is mandatory that one of the MMAP_OFFSET types > > > > > - * (GTT, WC, WB, UC, etc) should be included. > > > > > + * As caching mode when specifying `I915_MMAP_OFFSET_TTM`, WC or WB will > > > > > + * be used, depending on the object placement. WC will be used > > > > > + * when the object resides in local memory, WB otherwise. > > > > > */ > > > > > __u64 flags; > > > > > -#define I915_MMAP_OFFSET_GTT 0 > > > > > -#define I915_MMAP_OFFSET_WC 1 > > > > > -#define I915_MMAP_OFFSET_WB 2 > > > > > -#define I915_MMAP_OFFSET_UC 3 > > > > > > > > > > - /* > > > > > - * Zero-terminated chain of extensions. > > > > > +/** Use an mmap for the object by binding to GTT. */ > > > > > +#define I915_MMAP_OFFSET_GTT 0 > > > > > +/** Use Write-Combined caching. */ > > > > > +#define I915_MMAP_OFFSET_WC 1 > > > > > +/** Use Write-Back caching. */ > > > > > +#define I915_MMAP_OFFSET_WB 2 > > > > > +/** Do not use caching when binding this mmap. */ > > > > > +#define I915_MMAP_OFFSET_UC 3 > > > > > +/** Use the TTM binding, which determines the appropriate caching mode. */ > > > > > +#define I915_MMAP_OFFSET_TTM 4 > > > > > > > > I'm not sure I like the name here. TTM is an implementation detail, > > > > not a kind of map. I think we want something more like > > > > I915_MMAP_OFFSET_IMPLICIT or something like that. The semantics here, > > > > as far as I can tell, are "the mmap type is based on the object; you > > > > can't change it." On DG1, the mmap type is fixed for all objects. On > > > > integrated, we could have a BO create option for the mmap type but we > > > > don't yet. > > > > > > Yeah it's not a great name, but also we didn't come up with anything > > > substantially better on irc. More ideas: > > > - PREDEFINED > > > - PRESELECTED > > > > > > Note that we want to pick a value here which also makes sense for when we > > > extend GEM_CREATE_EXT to allow you to select the single mmap mode you get > > > at creation time (for igfx cleanup of the uapi). So IMPLICIT isn't the > > > greatest name either, when we'll allow userspace to explicit chose it - > > > just not here anymore. > > > > Yeah, none of them are great. Maybe FIXED? Or PER_BO? Or BO_CREATE? > > Or just BO? I think the semantics we want are "whatever was on the > > BO at create time". > > _FIXED sounds like the least horrible one to me. Fine with me. > -Daniel > > > > > --Jason > > > > > > > > Anyway, pick a name and we'll paint this bikeshed, I don't care much > > > really. As long as there's kerneldoc :-) > > > -Daniel > > > > > > > > > > > --Jason > > > > > > > > > + > > > > > + /** > > > > > + * @extensions: Zero-terminated chain of extensions. > > > > > * > > > > > * No current extensions defined; mbz. > > > > > */ > > > > > -- > > > > > 2.31.0 > > > > > > > > > > _______________________________________________ > > > > > Intel-gfx mailing list > > > > > Intel-gfx@lists.freedesktop.org > > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > _______________________________________________ > > > > Intel-gfx mailing list > > > > Intel-gfx@lists.freedesktop.org > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > > > -- > > > Daniel Vetter > > > Software Engineer, Intel Corporation > > > http://blog.ffwll.ch > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-13.6 required=3.0 tests=BAYES_00,DKIM_INVALID, DKIM_SIGNED,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id EE43DC07E99 for ; Mon, 12 Jul 2021 15:07:27 +0000 (UTC) Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id A6CA761206 for ; Mon, 12 Jul 2021 15:07:27 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org A6CA761206 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=jlekstrand.net Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=intel-gfx-bounces@lists.freedesktop.org Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 85D7F89733; Mon, 12 Jul 2021 15:07:22 +0000 (UTC) Received: from mail-yb1-xb34.google.com (mail-yb1-xb34.google.com [IPv6:2607:f8b0:4864:20::b34]) by gabe.freedesktop.org (Postfix) with ESMTPS id AFE418972B for ; Mon, 12 Jul 2021 15:07:21 +0000 (UTC) Received: by mail-yb1-xb34.google.com with SMTP id k184so29470140ybf.12 for ; Mon, 12 Jul 2021 08:07:21 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=jlekstrand-net.20150623.gappssmtp.com; s=20150623; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=lhobAOl+mjClAm3Zzwd0eWj9BCTZt7LJmhiyF2rBZlQ=; b=oeSgsS3eKvCrpjPwDtkUVZB7O9Wh8HabklA8RUu+ohICARuQs7WEqovnFiRd5+LIpL a6uTlFnw8OiXgU9H5F15KRHe5v0EnkuJ3A4Ps/hJ5f5SuRqiOyPMzh/0dnj8Udq4G+3/ r2I5FK/pMYmD6lk9pgE19VOiglEKaUuMiAaArSZ5k2EdbKvCg4jM/r7T66eYDf5FfBqo ZEsNjyXs96vtXAo1wF5ykb7ASp9rMnAwzzTUtyzAbjwexjmK5x5TmltJIOTrGOh7alNC kVsFD2oojIccEffIKjJKDkwLLu06MijXI4fe71EqR5mraZG1xYou1Vmr9O1ZiXC1Kck5 htxg== 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=lhobAOl+mjClAm3Zzwd0eWj9BCTZt7LJmhiyF2rBZlQ=; b=V99uesueFnW7V7UpbMWkvqCOXQ6Yxl9cNgR7Q+CfezYYjrG1AztTpMPAWJ7dcVq83p J01RlP4iw2bYMccjcFgOGKr20YNP32WFNxD25s82mykF0Q6Pq0jCQhKux/PuRqiz0pQr uzQOIjsrFNRISHMSdbrp6XF0jIQhiVNpQzChWXXIgc6xzsNJwjm3EmHhjOon/DPg2woY /0uWMaN2w9mRe50ivVGqTFiBrqAwfhNhYDxSYeP2t0KM5PWX9Grd+MkOHdIYgXAuhNdq Nlz2t5nmoH0lVibSTaSsw7ukFNKqwK9hdp23BWI3TafzX9dQJ1qfcuw8sM+WOLSs4gzE vGFw== X-Gm-Message-State: AOAM533gu9P81sWpQzGZ2kflDvP9zPPmOZd5c7pHswZJRbGUcSq/diLW saCL4qerdwg5qaYvtf7nH/lcZtjkUMCZh47F4FXR7A== X-Google-Smtp-Source: ABdhPJzFMBfhxqlYwEtHSPqwwI3jitun2HjJMdyFRglEQ7D+Op+IcfYPZ5Xqr4zFjfaXGYX2ohRpqsOEn6iYlrUjVv8= X-Received: by 2002:a25:2f89:: with SMTP id v131mr67843894ybv.469.1626102440702; Mon, 12 Jul 2021 08:07:20 -0700 (PDT) MIME-Version: 1.0 References: <20210709114120.651309-1-maarten.lankhorst@linux.intel.com> In-Reply-To: From: Jason Ekstrand Date: Mon, 12 Jul 2021 10:07:09 -0500 Message-ID: To: Daniel Vetter Subject: Re: [Intel-gfx] [PATCH v2] drm/i915: Add TTM offset argument to mmap. X-BeenThere: intel-gfx@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Intel graphics driver community testing & development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Intel GFX , Maling list - DRI developers Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" On Mon, Jul 12, 2021 at 9:38 AM Daniel Vetter wrote: > > On Mon, Jul 12, 2021 at 09:31:13AM -0500, Jason Ekstrand wrote: > > On Mon, Jul 12, 2021 at 9:12 AM Daniel Vetter wrote: > > > > > > On Mon, Jul 12, 2021 at 08:47:24AM -0500, Jason Ekstrand wrote: > > > > On Fri, Jul 9, 2021 at 6:41 AM Maarten Lankhorst > > > > wrote: > > > > > > > > > > This is only used for ttm, and tells userspace that the mapping type is > > > > > ignored. This disables the other type of mmap offsets when discrete > > > > > memory is used, so fix the selftests as well. > > > > > > > > > > Document the struct as well, so it shows up in docbook correctly. > > > > > > > > > > Changes since v1: > > > > > - Add docbook entries. > > > > > > > > > > Signed-off-by: Maarten Lankhorst > > > > > --- > > > > > drivers/gpu/drm/i915/gem/i915_gem_mman.c | 17 ++++++- > > > > > .../gpu/drm/i915/gem/i915_gem_object_types.h | 1 + > > > > > .../drm/i915/gem/selftests/i915_gem_mman.c | 27 +++++++++- > > > > > include/uapi/drm/i915_drm.h | 51 +++++++++++++++---- > > > > > 4 files changed, 82 insertions(+), 14 deletions(-) > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.c b/drivers/gpu/drm/i915/gem/i915_gem_mman.c > > > > > index a90f796e85c0..b34be9e5d094 100644 > > > > > --- a/drivers/gpu/drm/i915/gem/i915_gem_mman.c > > > > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.c > > > > > @@ -679,10 +679,16 @@ __assign_mmap_offset(struct drm_i915_gem_object *obj, > > > > > return -ENODEV; > > > > > > > > > > if (obj->ops->mmap_offset) { > > > > > + if (mmap_type != I915_MMAP_TYPE_TTM) > > > > > + return -ENODEV; > > > > > + > > > > > *offset = obj->ops->mmap_offset(obj); > > > > > return 0; > > > > > } > > > > > > > > > > + if (mmap_type == I915_MMAP_TYPE_TTM) > > > > > + return -ENODEV; > > > > > + > > > > > if (mmap_type != I915_MMAP_TYPE_GTT && > > > > > !i915_gem_object_has_struct_page(obj) && > > > > > !i915_gem_object_has_iomem(obj)) > > > > > @@ -727,7 +733,9 @@ i915_gem_dumb_mmap_offset(struct drm_file *file, > > > > > { > > > > > enum i915_mmap_type mmap_type; > > > > > > > > > > - if (boot_cpu_has(X86_FEATURE_PAT)) > > > > > + if (HAS_LMEM(to_i915(dev))) > > > > > + mmap_type = I915_MMAP_TYPE_TTM; > > > > > + else if (boot_cpu_has(X86_FEATURE_PAT)) > > > > > mmap_type = I915_MMAP_TYPE_WC; > > > > > else if (!i915_ggtt_has_aperture(&to_i915(dev)->ggtt)) > > > > > return -ENODEV; > > > > > @@ -798,6 +806,10 @@ i915_gem_mmap_offset_ioctl(struct drm_device *dev, void *data, > > > > > type = I915_MMAP_TYPE_UC; > > > > > break; > > > > > > > > > > + case I915_MMAP_OFFSET_TTM: > > > > > + type = I915_MMAP_TYPE_TTM; > > > > > + break; > > > > > + > > > > > default: > > > > > return -EINVAL; > > > > > } > > > > > @@ -968,6 +980,9 @@ int i915_gem_mmap(struct file *filp, struct vm_area_struct *vma) > > > > > vma->vm_ops = &vm_ops_cpu; > > > > > break; > > > > > > > > > > + case I915_MMAP_TYPE_TTM: > > > > > + GEM_WARN_ON(mmo->mmap_type == I915_MMAP_TYPE_TTM); > > > > > + /* fall-through */ > > > > > case I915_MMAP_TYPE_WB: > > > > > vma->vm_page_prot = vm_get_page_prot(vma->vm_flags); > > > > > vma->vm_ops = &vm_ops_cpu; > > > > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h > > > > > index ef3de2ae9723..d4c42bcdfeb6 100644 > > > > > --- a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h > > > > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h > > > > > @@ -105,6 +105,7 @@ enum i915_mmap_type { > > > > > I915_MMAP_TYPE_WC, > > > > > I915_MMAP_TYPE_WB, > > > > > I915_MMAP_TYPE_UC, > > > > > + I915_MMAP_TYPE_TTM, > > > > > }; > > > > > > > > > > struct i915_mmap_offset { > > > > > diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c b/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c > > > > > index 1da8bd675e54..27a35d88e5f5 100644 > > > > > --- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c > > > > > +++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_mman.c > > > > > @@ -573,6 +573,14 @@ static int make_obj_busy(struct drm_i915_gem_object *obj) > > > > > return 0; > > > > > } > > > > > > > > > > +static enum i915_mmap_type default_mapping(struct drm_i915_private *i915) > > > > > +{ > > > > > + if (HAS_LMEM(i915)) > > > > > + return I915_MMAP_TYPE_TTM; > > > > > + > > > > > + return I915_MMAP_TYPE_GTT; > > > > > +} > > > > > + > > > > > static bool assert_mmap_offset(struct drm_i915_private *i915, > > > > > unsigned long size, > > > > > int expected) > > > > > @@ -585,7 +593,7 @@ static bool assert_mmap_offset(struct drm_i915_private *i915, > > > > > if (IS_ERR(obj)) > > > > > return expected && expected == PTR_ERR(obj); > > > > > > > > > > - ret = __assign_mmap_offset(obj, I915_MMAP_TYPE_GTT, &offset, NULL); > > > > > + ret = __assign_mmap_offset(obj, default_mapping(i915), &offset, NULL); > > > > > i915_gem_object_put(obj); > > > > > > > > > > return ret == expected; > > > > > @@ -689,7 +697,7 @@ static int igt_mmap_offset_exhaustion(void *arg) > > > > > goto out; > > > > > } > > > > > > > > > > - err = __assign_mmap_offset(obj, I915_MMAP_TYPE_GTT, &offset, NULL); > > > > > + err = __assign_mmap_offset(obj, default_mapping(i915), &offset, NULL); > > > > > if (err) { > > > > > pr_err("Unable to insert object into reclaimed hole\n"); > > > > > goto err_obj; > > > > > @@ -831,8 +839,14 @@ static int wc_check(struct drm_i915_gem_object *obj) > > > > > > > > > > static bool can_mmap(struct drm_i915_gem_object *obj, enum i915_mmap_type type) > > > > > { > > > > > + struct drm_i915_private *i915 = to_i915(obj->base.dev); > > > > > bool no_map; > > > > > > > > > > + if (HAS_LMEM(i915)) > > > > > + return type == I915_MMAP_TYPE_TTM; > > > > > + else if (type == I915_MMAP_TYPE_TTM) > > > > > + return false; > > > > > + > > > > > if (type == I915_MMAP_TYPE_GTT && > > > > > !i915_ggtt_has_aperture(&to_i915(obj->base.dev)->ggtt)) > > > > > return false; > > > > > @@ -970,6 +984,8 @@ static int igt_mmap(void *arg) > > > > > err = __igt_mmap(i915, obj, I915_MMAP_TYPE_GTT); > > > > > if (err == 0) > > > > > err = __igt_mmap(i915, obj, I915_MMAP_TYPE_WC); > > > > > + if (err == 0) > > > > > + err = __igt_mmap(i915, obj, I915_MMAP_TYPE_TTM); > > > > > > > > > > i915_gem_object_put(obj); > > > > > if (err) > > > > > @@ -987,6 +1003,7 @@ static const char *repr_mmap_type(enum i915_mmap_type type) > > > > > case I915_MMAP_TYPE_WB: return "wb"; > > > > > case I915_MMAP_TYPE_WC: return "wc"; > > > > > case I915_MMAP_TYPE_UC: return "uc"; > > > > > + case I915_MMAP_TYPE_TTM: return "ttm"; > > > > > default: return "unknown"; > > > > > } > > > > > } > > > > > @@ -1100,6 +1117,8 @@ static int igt_mmap_access(void *arg) > > > > > err = __igt_mmap_access(i915, obj, I915_MMAP_TYPE_WC); > > > > > if (err == 0) > > > > > err = __igt_mmap_access(i915, obj, I915_MMAP_TYPE_UC); > > > > > + if (err == 0) > > > > > + err = __igt_mmap_access(i915, obj, I915_MMAP_TYPE_TTM); > > > > > > > > > > i915_gem_object_put(obj); > > > > > if (err) > > > > > @@ -1241,6 +1260,8 @@ static int igt_mmap_gpu(void *arg) > > > > > err = __igt_mmap_gpu(i915, obj, I915_MMAP_TYPE_GTT); > > > > > if (err == 0) > > > > > err = __igt_mmap_gpu(i915, obj, I915_MMAP_TYPE_WC); > > > > > + if (err == 0) > > > > > + err = __igt_mmap_gpu(i915, obj, I915_MMAP_TYPE_TTM); > > > > > > > > > > i915_gem_object_put(obj); > > > > > if (err) > > > > > @@ -1396,6 +1417,8 @@ static int igt_mmap_revoke(void *arg) > > > > > err = __igt_mmap_revoke(i915, obj, I915_MMAP_TYPE_GTT); > > > > > if (err == 0) > > > > > err = __igt_mmap_revoke(i915, obj, I915_MMAP_TYPE_WC); > > > > > + if (err == 0) > > > > > + err = __igt_mmap_revoke(i915, obj, I915_MMAP_TYPE_TTM); > > > > > > > > > > i915_gem_object_put(obj); > > > > > if (err) > > > > > diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h > > > > > index e334a8b14ef2..1610ed40b4b5 100644 > > > > > --- a/include/uapi/drm/i915_drm.h > > > > > +++ b/include/uapi/drm/i915_drm.h > > > > > @@ -849,31 +849,60 @@ struct drm_i915_gem_mmap_gtt { > > > > > __u64 offset; > > > > > }; > > > > > > > > > > +/** > > > > > + * struct drm_i915_gem_mmap_offset - Retrieve an offset so we can mmap this buffer object. > > > > > + * > > > > > + * This struct is passed as argument to the `DRM_IOCTL_I915_GEM_MMAP_OFFSET` ioctl, > > > > > + * and is used to retrieve the fake offset to mmap an object specified by &handle. > > > > > + * > > > > > + * The legacy way of using `DRM_IOCTL_I915_GEM_MMAP` is removed on gen12+. > > > > > + * `DRM_IOCTL_I915_GEM_MMAP_GTT` is an older supported alias to this struct, but will behave > > > > > + * as setting the &extensions to 0, and &flags to `I915_MMAP_OFFSET_GTT`. > > > > > + */ > > > > > struct drm_i915_gem_mmap_offset { > > > > > - /** Handle for the object being mapped. */ > > > > > + /** @handle: Handle for the object being mapped. */ > > > > > __u32 handle; > > > > > + /** @pad: Must be zero */ > > > > > __u32 pad; > > > > > /** > > > > > - * Fake offset to use for subsequent mmap call > > > > > + * @offset: The fake offset to use for subsequent mmap call > > > > > * > > > > > * This is a fixed-size type for 32/64 compatibility. > > > > > */ > > > > > __u64 offset; > > > > > > > > > > /** > > > > > - * Flags for extended behaviour. > > > > > + * @flags: Flags for extended behaviour. > > > > > + * > > > > > + * It is mandatory that one of the `MMAP_OFFSET` types > > > > > + * should be included: > > > > > + * - `I915_MMAP_OFFSET_GTT`: Use mmap with the object bound to GTT. > > > > > + * - `I915_MMAP_OFFSET_WC`: Use Write-Combined caching. > > > > > + * - `I915_MMAP_OFFSET_WB`: Use Write-Back caching. > > > > > + * - `I915_MMAP_OFFSET_TTM`: Use TTM to determine caching based on object placement. > > > > > + * > > > > > + * Only on devices with local memory is `I915_MMAP_OFFSET_TTM` valid. On > > > > > + * devices without local memory, this caching mode is invalid. > > > > > * > > > > > - * It is mandatory that one of the MMAP_OFFSET types > > > > > - * (GTT, WC, WB, UC, etc) should be included. > > > > > + * As caching mode when specifying `I915_MMAP_OFFSET_TTM`, WC or WB will > > > > > + * be used, depending on the object placement. WC will be used > > > > > + * when the object resides in local memory, WB otherwise. > > > > > */ > > > > > __u64 flags; > > > > > -#define I915_MMAP_OFFSET_GTT 0 > > > > > -#define I915_MMAP_OFFSET_WC 1 > > > > > -#define I915_MMAP_OFFSET_WB 2 > > > > > -#define I915_MMAP_OFFSET_UC 3 > > > > > > > > > > - /* > > > > > - * Zero-terminated chain of extensions. > > > > > +/** Use an mmap for the object by binding to GTT. */ > > > > > +#define I915_MMAP_OFFSET_GTT 0 > > > > > +/** Use Write-Combined caching. */ > > > > > +#define I915_MMAP_OFFSET_WC 1 > > > > > +/** Use Write-Back caching. */ > > > > > +#define I915_MMAP_OFFSET_WB 2 > > > > > +/** Do not use caching when binding this mmap. */ > > > > > +#define I915_MMAP_OFFSET_UC 3 > > > > > +/** Use the TTM binding, which determines the appropriate caching mode. */ > > > > > +#define I915_MMAP_OFFSET_TTM 4 > > > > > > > > I'm not sure I like the name here. TTM is an implementation detail, > > > > not a kind of map. I think we want something more like > > > > I915_MMAP_OFFSET_IMPLICIT or something like that. The semantics here, > > > > as far as I can tell, are "the mmap type is based on the object; you > > > > can't change it." On DG1, the mmap type is fixed for all objects. On > > > > integrated, we could have a BO create option for the mmap type but we > > > > don't yet. > > > > > > Yeah it's not a great name, but also we didn't come up with anything > > > substantially better on irc. More ideas: > > > - PREDEFINED > > > - PRESELECTED > > > > > > Note that we want to pick a value here which also makes sense for when we > > > extend GEM_CREATE_EXT to allow you to select the single mmap mode you get > > > at creation time (for igfx cleanup of the uapi). So IMPLICIT isn't the > > > greatest name either, when we'll allow userspace to explicit chose it - > > > just not here anymore. > > > > Yeah, none of them are great. Maybe FIXED? Or PER_BO? Or BO_CREATE? > > Or just BO? I think the semantics we want are "whatever was on the > > BO at create time". > > _FIXED sounds like the least horrible one to me. Fine with me. > -Daniel > > > > > --Jason > > > > > > > > Anyway, pick a name and we'll paint this bikeshed, I don't care much > > > really. As long as there's kerneldoc :-) > > > -Daniel > > > > > > > > > > > --Jason > > > > > > > > > + > > > > > + /** > > > > > + * @extensions: Zero-terminated chain of extensions. > > > > > * > > > > > * No current extensions defined; mbz. > > > > > */ > > > > > -- > > > > > 2.31.0 > > > > > > > > > > _______________________________________________ > > > > > Intel-gfx mailing list > > > > > Intel-gfx@lists.freedesktop.org > > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > _______________________________________________ > > > > Intel-gfx mailing list > > > > Intel-gfx@lists.freedesktop.org > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > > > -- > > > Daniel Vetter > > > Software Engineer, Intel Corporation > > > http://blog.ffwll.ch > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx