Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:8 as permitted sender) client-ip=2620:137:e000::3:8;
Message-ID: <a0ba3f8c70ec939842f06127486e1e99795f7e6a.camel@linux.intel.com>
Subject: Re: [PATCH v4] Documentation/gpu: VM_BIND locking document
From:   Thomas =?ISO-8859-1?Q?Hellstr=F6m?= 
        <thomas.hellstrom@linux.intel.com>
To:     Danilo Krummrich <dakr@redhat.com>,
        Boris Brezillon <boris.brezillon@collabora.com>
Cc:     intel-xe@lists.freedesktop.org,
        Rodrigo Vivi <rodrigo.vivi@intel.com>,
        Matthew Brost <matthew.brost@intel.com>,
        Joonas Lahtinen <joonas.lahtinen@linux.intel.com>,
        Oak Zeng <oak.zeng@intel.com>, Daniel Vetter <daniel@ffwll.ch>,
        Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
        Francois Dugast <francois.dugast@intel.com>,
        dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org
Date:   Thu, 16 Nov 2023 15:09:11 +0100
In-Reply-To: <c129fa94-6456-4746-9477-737a0fb61f96@redhat.com>
References: <20231115124937.6740-1-thomas.hellstrom@linux.intel.com>
         <20231116104851.114bdb08@collabora.com>
         <0850281b667c4b88163dab60737dbc945ad742fd.camel@linux.intel.com>
         <c129fa94-6456-4746-9477-737a0fb61f96@redhat.com>
Organization: Intel Sweden AB, Registration Number: 556189-6027
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.46.4 (3.46.4-1.fc37) 
MIME-Version: 1.0
Precedence: bulk

On Thu, 2023-11-16 at 15:02 +0100, Danilo Krummrich wrote:
> On 11/16/23 12:48, Thomas Hellstr=C3=B6m wrote:
>=20
> <snip>
>=20
> > > > +Locks used and locking orders
> > > > +=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D
> > > > +
> > > > +One of the benefits of VM_BIND is that local GEM objects share
> > > > the
> > > > gpu_vm's
> > > > +dma_resv object and hence the dma_resv lock. So even with a
> > > > huge
> > > > +number of local GEM objects, only one lock is needed to make
> > > > the
> > > > exec
> > > > +sequence atomic.
> > > > +
> > > > +The following locks and locking orders are used:
> > > > +
> > > > +* The ``gpu_vm->lock`` (optionally an rwsem). Protects how the
> > > > gpu_vm is
> > > > +=C2=A0 partitioned into gpu_vmas. It can also protect the gpu_vm's
> > > > list
> > > > of
> > > > +=C2=A0 userptr gpu_vmas. With a CPU mm analogy this would
> > > > correspond to
> > > > the
> > > > +=C2=A0 mmap_lock.
> > >=20
> > > I don't see any drm_gpuvm::lock field in Danilo's latest
> > > patchset,
> > > so,
> > > unless I missed one version, and this lock is actually provided
> > > by
> > > drm_gpuvm, I would mention this is a driver-specific lock. This
> > > comment
> > > applies to all the locks you describe here actually (mention
> > > which
> > > ones
> > > are provided by drm_gpuvm, and which ones are driver-specific).
> >=20
> > These will be needed also by gpuvm when implementing userptr vmas,
> > so I
> > can mention that drm_gpuvm is currently lacking a userptr
> > implementation, so "the locks described below are to be considered
> > driver-specific for now"
>=20
> Since Xe already implements userptr support, are you guys maybe
> interested
> in extending drm_gpuvm accordingly? :-)
>=20

I've been thinking of that but in that case that needs to happen after
the xe merge. Also we ofc need to clear it with the people who do
resource allocation on our side :)

Thanks,
Thomas