Received: by 2002:a05:7412:31a9:b0:e2:908c:2ebd with SMTP id et41csp4076884rdb; Thu, 14 Sep 2023 11:01:57 -0700 (PDT) X-Google-Smtp-Source: AGHT+IGKenmBzVCCgTy/c9+afXZlUXW3RdKPymzWf8VQ9W+W0U67BME01MCBQiD4LD3rJrWWEVxC X-Received: by 2002:a17:903:41c2:b0:1bb:b74c:88f6 with SMTP id u2-20020a17090341c200b001bbb74c88f6mr7268481ple.16.1694714517517; Thu, 14 Sep 2023 11:01:57 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1694714517; cv=none; d=google.com; s=arc-20160816; b=xggHH1FMeQPJtusO8j8TeSMrSB7wwBFEp9qrBxfzW0T3j+iioOKzDFd+Afo26Tedwf xSeT6rx7GuzK7qnPjQjD5RXwBzs42ogawdD984O2pFRovbsVYQ2RhjKR7FN0cdkiKIxt bFWXmRySZWRjpLcg8TOMUQVlz1KDdPRPg0PuHsDWBC1WW0h3RE9d3TTifC0jJnyZZCAj s/mBv5OoRW1AcsBwLim3CE/QHfOe3wJcRuF1QI8QQRfeb1XYnYA90GqA8a/YZoAG+o7E 7UvjnZmLfvF36H01GcB4MYG5SEpxdvvN3SYwwhZaOAQhAWZxglwVFMZq9P9lRfklZSVd xi7Q== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :organization:references:in-reply-to:message-id:subject:cc:to:from :date:dkim-signature; bh=yB/86UDPaVI8ZThacc2MW02aRvODRCK4vvtd/1uGkFU=; fh=iUS413dVenIF5zzUEj/JAYSspxKEUmfTSWp8twPe5Io=; b=Z9LHkU/CWRV7yk7h00p7SIrwi6lnVq6P1yiGODHRMfw6gB2wV6Nm9r5hF5FSowxmV0 /ryjlJNPKZvdeRqRuQjG3LgetimQ3LmOhNWeYkk4qxjM3YiRhz0xNvf0PB3NpMugqFoM fgQLFHAVb7uolR01DnpwJeWgRcagTQ+xDMxvT5uDRJKXzM3ofdZP5mi1g+Za10USVQ85 O1YkuY66xqNGv2TVH1TNxaMGIi8HVG6semapmO2uiqoiqBd8J4+dn+Q9uDmI3oFdPtmr fKhlf7oQDlv/mPko/kOaow+JLzwuOyjMxigqjg518H9Qpsk7jcILMw6uFjn78peUGYDV 77yw== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@collabora.com header.s=mail header.b=iPLvICBC; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:7 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=QUARANTINE sp=QUARANTINE dis=NONE) header.from=collabora.com Return-Path: Received: from snail.vger.email (snail.vger.email. [2620:137:e000::3:7]) by mx.google.com with ESMTPS id m18-20020a170902f65200b001c3411c9b83si2044993plg.454.2023.09.14.11.01.50 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 14 Sep 2023 11:01:57 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:7 as permitted sender) client-ip=2620:137:e000::3:7; Authentication-Results: mx.google.com; dkim=pass header.i=@collabora.com header.s=mail header.b=iPLvICBC; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:7 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=QUARANTINE sp=QUARANTINE dis=NONE) header.from=collabora.com Received: from out1.vger.email (depot.vger.email [IPv6:2620:137:e000::3:0]) by snail.vger.email (Postfix) with ESMTP id 862FD8213F1F; Wed, 13 Sep 2023 04:33:57 -0700 (PDT) X-Virus-Status: Clean X-Virus-Scanned: clamav-milter 0.103.10 at snail.vger.email Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S240597AbjIMLd5 (ORCPT + 99 others); Wed, 13 Sep 2023 07:33:57 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:33386 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S240498AbjIMLdp (ORCPT ); Wed, 13 Sep 2023 07:33:45 -0400 Received: from madras.collabora.co.uk (madras.collabora.co.uk [46.235.227.172]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 2C4531FFD for ; Wed, 13 Sep 2023 04:33:23 -0700 (PDT) Received: from localhost (unknown [IPv6:2a01:e0a:2c:6930:5cf4:84a1:2763:fe0d]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) (Authenticated sender: bbrezillon) by madras.collabora.co.uk (Postfix) with ESMTPSA id F26A9660733B; Wed, 13 Sep 2023 12:33:21 +0100 (BST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=collabora.com; s=mail; t=1694604802; bh=Tt93rndTGtAd7o+182CWzlEvCDnXimjI5xCIO44d5qU=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=iPLvICBCqTKl3+YY9MwjzvGIOIcg5Q6p+uYOXL3L/5A2iLFOv1yCNiktEhNYkMsZj 1umH5QEUtrnGUCdomLTFLB3NgKUmzOGqHBfHQq/LzSLPEnE60KWY6IdY4h5Z86qRvg 9pE7Xze3IuZH3pshmjM2ZJlcQpoN6joG3+S//KX/K2Tg0PLTGD1JTrHdNmVakZN4yI GmZH2VW5evsn6vTzL5afbiXT4G/FVabj0vj1NNHcTxZfc7dJ/D57V2UIL5EQKF5ZF8 DnEW/d5E4wGjcTGWKHXWTkGol6NmwyutmVSnREzZQSdW0qZeJ/py7+iGMrmaSSCwol ruRp4c6VITstA== Date: Wed, 13 Sep 2023 13:33:18 +0200 From: Boris Brezillon To: Thomas =?UTF-8?B?SGVsbHN0csO2bQ==?= Cc: Dave Airlie , Danilo Krummrich , daniel@ffwll.ch, matthew.brost@intel.com, sarah.walker@imgtec.com, donald.robson@imgtec.com, christian.koenig@amd.com, faith.ekstrand@collabora.com, dri-devel@lists.freedesktop.org, nouveau@lists.freedesktop.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH drm-misc-next v3 6/7] drm/gpuvm: generalize dma_resv/extobj handling and GEM validation Message-ID: <20230913133318.15edec7c@collabora.com> In-Reply-To: References: <20230909153125.30032-1-dakr@redhat.com> <20230909153125.30032-7-dakr@redhat.com> <20230913090311.5eeb026a@collabora.com> <20230913091918.62c06a30@collabora.com> Organization: Collabora X-Mailer: Claws Mail 4.1.1 (GTK 3.24.38; x86_64-redhat-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (snail.vger.email [0.0.0.0]); Wed, 13 Sep 2023 04:33:57 -0700 (PDT) On Wed, 13 Sep 2023 12:39:01 +0200 Thomas Hellstr=C3=B6m wrote: > Hi, >=20 > On 9/13/23 09:19, Boris Brezillon wrote: > > On Wed, 13 Sep 2023 17:05:42 +1000 > > Dave Airlie wrote: > > =20 > >> On Wed, 13 Sept 2023 at 17:03, Boris Brezillon > >> wrote: =20 > >>> On Tue, 12 Sep 2023 18:20:32 +0200 > >>> Thomas Hellstr=C3=B6m wrote: > >>> =20 > >>>>> +/** > >>>>> + * get_next_vm_bo_from_list() - get the next vm_bo element > >>>>> + * @__gpuvm: The GPU VM > >>>>> + * @__list_name: The name of the list we're iterating on > >>>>> + * @__local_list: A pointer to the local list used to store alread= y iterated items > >>>>> + * @__prev_vm_bo: The previous element we got from drm_gpuvm_get_n= ext_cached_vm_bo() > >>>>> + * > >>>>> + * This helper is here to provide lockless list iteration. Lockles= s as in, the > >>>>> + * iterator releases the lock immediately after picking the first = element from > >>>>> + * the list, so list insertion deletion can happen concurrently. = =20 > >>>> Are the list spinlocks needed for that async state update from within > >>>> the dma-fence critical section we've discussed previously? =20 > >>> Any driver calling _[un]link() from its drm_gpu_scheduler::run_job() > >>> hook will be in this situation (Panthor at the moment, PowerVR soon).= I > >>> get that Xe and Nouveau don't need that because they update the VM > >>> state early (in the ioctl path), but I keep thinking this will hurt us > >>> if we don't think it through from the beginning, because once you've > >>> set this logic to depend only on resv locks, it will be pretty hard to > >>> get back to a solution which lets synchronous VM_BINDs take precedence > >>> on asynchronous request, and, with vkQueueBindSparse() passing extern= al > >>> deps (plus the fact the VM_BIND queue might be pretty deep), it can > >>> take a long time to get your synchronous VM_BIND executed... =20 >=20 > So this would boil down to either (possibly opt-in) keeping the spinlock= =20 > approach or pushing the unlink out to a wq then? Deferred _unlink() would not be an issue, since I already defer the drm_gpuva destruction to a wq, it would just a be a matter of moving the _unlink() call there as well. But _link() also takes the GEM gpuva list lock, and that one is bit tricky, in that sm_map() can trigger 2 more _link() calls for the prev/next mappings, which we can't guess until we get to execute the VM update. If we mandate the use of the GEM resv lock, that simply means async VM updates (AKA calling drm_gpuvm_sm_[un]map()) are not an option. And if this is what everyone agrees on, then I'd like the APIs that make this sort of async VM update possible (drm_gpuvm_sm_[un]map(), the drm_gpuvm_ops::sm_step* methods, and probably other things) to be dropped, so we don't make it look like it's something we support. > BTW, as also asked in a reply to Danilo, how do you call unlink from=20 > run_job() when it was requiring the obj->dma_resv lock, or was that a WIP? _unlink() makes sure the GEM gpuva list lock is taken, but this can be a custom lock (see drm_gem_gpuva_set_lock()). In panthor we have panthor_gem_object::gpuva_list_lock that's dedicated the gpuva list protection. We make sure we never take this lock while allocating memory to guarantee the dma-signalling path can't deadlock. >=20 > >>> =20 > >> btw what is the use case for this? do we have actual vulkan > >> applications we know will have problems here? =20 > > I don't, but I think that's a concern Faith raised at some point (dates > > back from when I was reading threads describing how VM_BIND on i915 > > should work, and I was clearly discovering this whole VM_BIND thing at > > that time, so maybe I misunderstood). > > =20 > >> it feels like a bit of premature optimisation, but maybe we have use c= ases. =20 > > Might be, but that's the sort of thing that would put us in a corner if > > we don't have a plan for when the needs arise. Besides, if we don't > > want to support that case because it's too complicated, I'd recommend > > dropping all the drm_gpuvm APIs that let people think this mode is > > valid/supported (map/remap/unmap hooks in drm_gpuvm_ops, > > drm_gpuvm_sm_[un]map helpers, etc). Keeping them around just adds to the > > confusion. =20 >=20 > Xe allows bypassing the bind-queue with another bind-queue, but to=20 > completely avoid dependencies between queues the Operations may not=20 > overlap. So, you check the VM state with some VM lock held (would be the VM resv in my case), and if the mapping is new (no overlaps with pre-existing mappings), you queue it to the fast-track/sync-VM_BIND queue. What would be missing I guess is a way to know if the mapping is active (MMU has been updated) or pending (MMU update queued to the bind-queue), so I can fast-track mapping/unmapping of active mappings. This would leave overlapping sync/async VM updates, which can't happen in practice unless userspace is doing something wrong (sparse bindings always go through vkQueueBindSparse). I'll give it a try. > (And the definition of overlap is currently page-table=20 > structure updates may not overlap) but no guarantees are made about=20 > priority. >=20 > /Thomas >=20 >=20 >=20