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=-10.5 required=3.0 tests=BAYES_00, DKIM_ADSP_CUSTOM_MED,DKIM_INVALID,DKIM_SIGNED,FREEMAIL_FORGED_FROMDOMAIN, FREEMAIL_FROM,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 63174C4320A for ; Wed, 28 Jul 2021 14:22:47 +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 2D89F60F45 for ; Wed, 28 Jul 2021 14:22:47 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org 2D89F60F45 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=gmail.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=lists.freedesktop.org Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 595956EACD; Wed, 28 Jul 2021 14:22:45 +0000 (UTC) Received: from mail-qv1-xf29.google.com (mail-qv1-xf29.google.com [IPv6:2607:f8b0:4864:20::f29]) by gabe.freedesktop.org (Postfix) with ESMTPS id CE52E6E9B0; Wed, 28 Jul 2021 14:22:30 +0000 (UTC) Received: by mail-qv1-xf29.google.com with SMTP id w6so1601936qvh.3; Wed, 28 Jul 2021 07:22:30 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc:content-transfer-encoding; bh=REguxMrHBQDa0JkRmuDefkm6jZHt6N1UdAh0jBrpPII=; b=SIbIOj0GB0xhbNCzcJJeUIMaXu7ElmlVCh/If2w6qhlDnkg0M6KvxidPwaInlm3oH/ sNFV4e599JFzcvN/d0KPTB1Pv+ZHFMCHrIJbVvmJvdSJ6kgAVn8hiQiXbhstdLxovpEb 4YryCADoEdl+pIfoTnelc/9APyCtlduhhSro491A+WZTl3QFGjuqO+RKfolJDAQfyEJ+ AAJ2xutI5plPMBGi+5BNs6Rm4PCqN6rNSRwTzcQbanMhYHXfHHD/3L6b8J3ZsjlXuOFz 8yb0olgktrTFaOhZ6WgzdQwTR7Gdut6U4RDhHazdtA9fKOLIageEiN7UuYk6llba6MVm PxIQ== 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:content-transfer-encoding; bh=REguxMrHBQDa0JkRmuDefkm6jZHt6N1UdAh0jBrpPII=; b=odUYRkCW3dezlZuRHvuufQpRZTuJf6J4E9bR0oMtAkRF1w2EVmmzDZ0Rz0CgxE6GYJ ir+8dUCQKlzXXl3MmRB2Hm95dRHvMSzPtWxHhxguwylPOcJqaimhlnmBua7szlF3GP/a wdMQ+eY3hXPNYzgBO7u3IXv//faOpxjUELlDHXsW8YrpdiqtTN77Gr2TNKNTdLuNfKyD +Sw8Zn/L0RkntHX7NXq5wlmwEv5NxpJKOwajwQXaxufjmj4EGa/ronOH0EBdpcrhYBME WMNq4sVmYJtkwL4cw0CuT7m+icsTs+d0qW7ZttDB/1S61slJXbu8qfOZ1BKOyBcyEOSS M+ww== X-Gm-Message-State: AOAM5338PTQanfgcQBY5lwSr4N3fycGFrZtnRc8GA7NSC+rsmeHn3Dny xWy95T4aKkJosMK3m8OUxupogq6MD8i49l1aETo= X-Google-Smtp-Source: ABdhPJzsBJNU17E0qoGgia79Fu1bXaO3ZGF9JUTW52Sr1nLJhk/3x63jZkY6WIX/DG8Wq8TsS9JKVxi2+fiX7a1NToU= X-Received: by 2002:a0c:f6c6:: with SMTP id d6mr196596qvo.30.1627482149827; Wed, 28 Jul 2021 07:22:29 -0700 (PDT) MIME-Version: 1.0 References: <20210723113405.427004-1-matthew.auld@intel.com> <0988bf67-c42a-1e7e-af77-ae2da65b036f@linux.intel.com> In-Reply-To: From: Matthew Auld Date: Wed, 28 Jul 2021 15:22:03 +0100 Message-ID: Subject: Re: [Intel-gfx] [PATCH] drm/i915/userptr: Probe existence of backing struct pages upon creation To: Tvrtko Ursulin Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable 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: =?UTF-8?Q?Thomas_Hellstr=C3=B6m?= , Daniel Vetter , Intel GFX , Maling list - DRI developers , Chris Wilson , Kenneth Graunke , Matthew Auld , Jason Ekstrand Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" On Mon, 26 Jul 2021 at 17:10, Tvrtko Ursulin wrote: > > > On 26/07/2021 16:14, Jason Ekstrand wrote: > > On Mon, Jul 26, 2021 at 3:31 AM Maarten Lankhorst > > wrote: > >> > >> Op 23-07-2021 om 13:34 schreef Matthew Auld: > >>> From: Chris Wilson > >>> > >>> Jason Ekstrand requested a more efficient method than userptr+set-dom= ain > >>> to determine if the userptr object was backed by a complete set of pa= ges > >>> upon creation. To be more efficient than simply populating the userpt= r > >>> using get_user_pages() (as done by the call to set-domain or execbuf)= , > >>> we can walk the tree of vm_area_struct and check for gaps or vma not > >>> backed by struct page (VM_PFNMAP). The question is how to handle > >>> VM_MIXEDMAP which may be either struct page or pfn backed... > >>> > >>> With discrete we are going to drop support for set_domain(), so offer= ing > >>> a way to probe the pages, without having to resort to dummy batches h= as > >>> been requested. > >>> > >>> v2: > >>> - add new query param for the PROBE flag, so userspace can easily > >>> check if the kernel supports it(Jason). > >>> - use mmap_read_{lock, unlock}. > >>> - add some kernel-doc. > >>> v3: > >>> - In the docs also mention that PROBE doesn't guarantee that the page= s > >>> will remain valid by the time they are actually used(Tvrtko). > >>> - Add a small comment for the hole finding logic(Jason). > >>> - Move the param next to all the other params which just return true. > >>> > >>> Testcase: igt/gem_userptr_blits/probe > >>> Signed-off-by: Chris Wilson > >>> Signed-off-by: Matthew Auld > >>> Cc: Thomas Hellstr=C3=B6m > >>> Cc: Maarten Lankhorst > >>> Cc: Tvrtko Ursulin > >>> Cc: Jordan Justen > >>> Cc: Kenneth Graunke > >>> Cc: Jason Ekstrand > >>> Cc: Daniel Vetter > >>> Cc: Ramalingam C > >>> Reviewed-by: Tvrtko Ursulin > >>> Acked-by: Kenneth Graunke > >>> Reviewed-by: Jason Ekstrand > >>> --- > >>> drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 41 ++++++++++++++++++= ++- > >>> drivers/gpu/drm/i915/i915_getparam.c | 1 + > >>> include/uapi/drm/i915_drm.h | 20 ++++++++++ > >>> 3 files changed, 61 insertions(+), 1 deletion(-) > >>> > >>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c b/drivers/gp= u/drm/i915/gem/i915_gem_userptr.c > >>> index 56edfeff8c02..468a7a617fbf 100644 > >>> --- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c > >>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c > >>> @@ -422,6 +422,34 @@ static const struct drm_i915_gem_object_ops i915= _gem_userptr_ops =3D { > >>> > >>> #endif > >>> > >>> +static int > >>> +probe_range(struct mm_struct *mm, unsigned long addr, unsigned long = len) > >>> +{ > >>> + const unsigned long end =3D addr + len; > >>> + struct vm_area_struct *vma; > >>> + int ret =3D -EFAULT; > >>> + > >>> + mmap_read_lock(mm); > >>> + for (vma =3D find_vma(mm, addr); vma; vma =3D vma->vm_next) { > >>> + /* Check for holes, note that we also update the addr b= elow */ > >>> + if (vma->vm_start > addr) > >>> + break; > >>> + > >>> + if (vma->vm_flags & (VM_PFNMAP | VM_MIXEDMAP)) > >>> + break; > >>> + > >>> + if (vma->vm_end >=3D end) { > >>> + ret =3D 0; > >>> + break; > >>> + } > >>> + > >>> + addr =3D vma->vm_end; > >>> + } > >>> + mmap_read_unlock(mm); > >>> + > >>> + return ret; > >>> +} > >>> + > >>> /* > >>> * Creates a new mm object that wraps some normal memory from the p= rocess > >>> * context - user memory. > >>> @@ -477,7 +505,8 @@ i915_gem_userptr_ioctl(struct drm_device *dev, > >>> } > >>> > >>> if (args->flags & ~(I915_USERPTR_READ_ONLY | > >>> - I915_USERPTR_UNSYNCHRONIZED)) > >>> + I915_USERPTR_UNSYNCHRONIZED | > >>> + I915_USERPTR_PROBE)) > >>> return -EINVAL; > >>> > >>> if (i915_gem_object_size_2big(args->user_size)) > >>> @@ -504,6 +533,16 @@ i915_gem_userptr_ioctl(struct drm_device *dev, > >>> return -ENODEV; > >>> } > >>> > >>> + if (args->flags & I915_USERPTR_PROBE) { > >>> + /* > >>> + * Check that the range pointed to represents real stru= ct > >>> + * pages and not iomappings (at this moment in time!) > >>> + */ > >>> + ret =3D probe_range(current->mm, args->user_ptr, args->= user_size); > >>> + if (ret) > >>> + return ret; > >>> + } > >>> + > >>> #ifdef CONFIG_MMU_NOTIFIER > >>> obj =3D i915_gem_object_alloc(); > >>> if (obj =3D=3D NULL) > >>> diff --git a/drivers/gpu/drm/i915/i915_getparam.c b/drivers/gpu/drm/i= 915/i915_getparam.c > >>> index 24e18219eb50..bbb7cac43eb4 100644 > >>> --- a/drivers/gpu/drm/i915/i915_getparam.c > >>> +++ b/drivers/gpu/drm/i915/i915_getparam.c > >>> @@ -134,6 +134,7 @@ int i915_getparam_ioctl(struct drm_device *dev, v= oid *data, > >>> case I915_PARAM_HAS_EXEC_FENCE_ARRAY: > >>> case I915_PARAM_HAS_EXEC_SUBMIT_FENCE: > >>> case I915_PARAM_HAS_EXEC_TIMELINE_FENCES: > >>> + case I915_PARAM_HAS_USERPTR_PROBE: > >>> /* For the time being all of these are always true; > >>> * if some supported hardware does not have one of the= se > >>> * features this value needs to be provided from > >>> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.= h > >>> index 975087553ea0..0d290535a6e5 100644 > >>> --- a/include/uapi/drm/i915_drm.h > >>> +++ b/include/uapi/drm/i915_drm.h > >>> @@ -674,6 +674,9 @@ typedef struct drm_i915_irq_wait { > >>> */ > >>> #define I915_PARAM_HAS_EXEC_TIMELINE_FENCES 55 > >>> > >>> +/* Query if the kernel supports the I915_USERPTR_PROBE flag. */ > >>> +#define I915_PARAM_HAS_USERPTR_PROBE 56 > >>> + > >>> /* Must be kept compact -- no holes and well documented */ > >>> > >>> typedef struct drm_i915_getparam { > >>> @@ -2222,12 +2225,29 @@ struct drm_i915_gem_userptr { > >>> * through the GTT. If the HW can't support readonly access, a= n error is > >>> * returned. > >>> * > >>> + * I915_USERPTR_PROBE: > >>> + * > >>> + * Probe the provided @user_ptr range and validate that the @us= er_ptr is > >>> + * indeed pointing to normal memory and that the range is also = valid. > >>> + * For example if some garbage address is given to the kernel, = then this > >>> + * should complain. > >>> + * > >>> + * Returns -EFAULT if the probe failed. > >>> + * > >>> + * Note that this doesn't populate the backing pages, and also = doesn't > >>> + * guarantee that the object will remain valid when the object = is > >>> + * eventually used. > >>> + * > >>> + * The kernel supports this feature if I915_PARAM_HAS_USERPTR_P= ROBE > >>> + * returns a non-zero value. > >>> + * > >>> * I915_USERPTR_UNSYNCHRONIZED: > >>> * > >>> * NOT USED. Setting this flag will result in an error. > >>> */ > >>> __u32 flags; > >>> #define I915_USERPTR_READ_ONLY 0x1 > >>> +#define I915_USERPTR_PROBE 0x2 > >>> #define I915_USERPTR_UNSYNCHRONIZED 0x80000000 > >>> /** > >>> * @handle: Returned handle for the object. > >> > >> Could we use _VALIDATE instead of probe? Or at least pin the pages as = well, so we don't have to do it later? > > > > I only care that the name matches what it does. _VALIDATE sounds like > > it does a full validation of everything such that, if the import > > succeeds, execbuf will as well. If we pin the pages at the same time, > > maybe that's true? _PROBE, on the other hand, sounds a lot more like > > No it is not possible to guarantee backing store remains valid until > execbuf. > > > a one-time best-effort check which may race with other stuff and > > doesn't guarantee future success. That's in line with what the > > current patch does. > > > >> We already have i915_gem_object_userptr_validate, no need to dupe it. > > > > I have no opinion on this. > > I was actually suggesting the same as Maarten here - that we should add > a "populate" flag. But opinion was that was not desired - please look > for the older threads to see the reasoning there. So how should we proceed here? Maarten? > > Regards, > > Tvrtko > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx