From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id A51FA1BCA07; Tue, 5 Aug 2025 14:48:05 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=10.30.226.201 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1754405285; cv=none; b=Ne+qHETnWCzRK7XVYiqXgaCfSsqZCtahaHbuf3kEewXTUjoGe1LudoXCNo4A87AW3vU8DLAMpvEMLTSffDcDJv6SX8USjxS5b8e7Jw6MRHmdoU/hUEgbjDTnRj4imPS+0zd04lA0L90tGJrMK57qUYeDgD29Rf+2iHIVo1R3o6k= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1754405285; c=relaxed/simple; bh=yDTCqMbktP5T7/s/6qRHaUnSmDaSqNv3r3EwtVnqXMQ=; h=Mime-Version:Content-Type:Date:Message-Id:Subject:Cc:To:From: References:In-Reply-To; b=HpmBNpWnAXVAgIchsFMHrpoALQZrbZPTeuZdk/pCzoC0fHBgbeV+USGkMgPpGalK0ll6trd2eiwM0tUNbrDmQmumM4BqQmGP6tXmgJGlpFsRwAM1xD4tO0vS/D7i2ubR8WFP78JlP5NzdZInKJHlSF3Pr6aqbJCmIpzYVXKaSp4= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=Zl23PYxd; arc=none smtp.client-ip=10.30.226.201 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="Zl23PYxd" Received: by smtp.kernel.org (Postfix) with ESMTPSA id 5B763C4CEF0; Tue, 5 Aug 2025 14:48:02 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1754405285; bh=yDTCqMbktP5T7/s/6qRHaUnSmDaSqNv3r3EwtVnqXMQ=; h=Date:Subject:Cc:To:From:References:In-Reply-To:From; b=Zl23PYxdyrbcMtf4QHnUXgEH+BAUYflh4vqk1AiWBRDYDwgmHTppBOdzdaSM9k9xx WTw9QBVWN2U5SIyMqY4zMZsuJGVquhLUemjqGgDSSw7amSiyGnqrJG9koeMFUUin3e 1MVL1tDp4/6oPQj2O9/u/d0JxxSOcmHYnT7V82nKx1xlmz904Fwo2snoBMi60OlVgP hHXk49QPIzuMv/Vf926DbbsKu/EBXKCuFONEqQVsih2cgu9VaqwLnYl/RM7hMf33oV yGv36wMeMhVs8J8zU10eLfOfMTFrGod+AFhum9ZlHlcFfh7IvaB9dxs4enM06NskHh g01FNu3ce8a6w== Precedence: bulk X-Mailing-List: linux-arm-msm@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=UTF-8 Date: Tue, 05 Aug 2025 16:48:00 +0200 Message-Id: Subject: Re: [PATCH RESEND 1/2] drm/gpuvm: Send in-place re-maps to the driver as remap Cc: , , , "Danilo Krummrich" , "Connor Abbott" , "Maarten Lankhorst" , "Maxime Ripard" , "Thomas Zimmermann" , "David Airlie" , "Simona Vetter" , "Lyude Paul" , "open list" , "open list:DRM DRIVER FOR NVIDIA GEFORCE/QUADRO GPUS" To: "Rob Clark" From: "Danilo Krummrich" References: <20250804214317.658704-1-robin.clark@oss.qualcomm.com> <20250804214317.658704-2-robin.clark@oss.qualcomm.com> In-Reply-To: On Tue Aug 5, 2025 at 4:32 PM CEST, Rob Clark wrote: > On Tue, Aug 5, 2025 at 2:33=E2=80=AFAM Danilo Krummrich = wrote: >> On Mon Aug 4, 2025 at 11:43 PM CEST, Rob Clark wrote: >> > diff --git a/drivers/gpu/drm/drm_gpuvm.c b/drivers/gpu/drm/drm_gpuvm.c >> > index bbc7fecb6f4a..e21782a97fbe 100644 >> > --- a/drivers/gpu/drm/drm_gpuvm.c >> > +++ b/drivers/gpu/drm/drm_gpuvm.c >> > @@ -2125,6 +2125,27 @@ __drm_gpuvm_sm_map(struct drm_gpuvm *gpuvm, >> > offset =3D=3D req_offset; >> > >> > if (end =3D=3D req_end) { >> > + if (merge) { >> > + /* >> > + * This is an exact remap of the= existing >> > + * VA (potentially flags change)= ? Pass >> > + * this to the driver as a remap= so it can >> > + * do an in-place update: >> > + */ >> > + struct drm_gpuva_op_map n =3D { >> > + .va.addr =3D va->va.addr= , >> > + .va.range =3D va->va.ran= ge, >> > + .gem.obj =3D va->gem.obj= , >> > + .gem.offset =3D va->gem.= offset, >> > + }; >> > + struct drm_gpuva_op_unmap u =3D = { >> > + .va =3D va, >> > + .keep =3D true, >> > + }; >> > + >> > + return op_remap_cb(ops, priv, NU= LL, &n, &u); >> > + } >> >> I don't see why this is necessary, a struct drm_gpuva_op_unmap carries t= he >> struct drm_gpuva to unmap. You can easily compare this to the original r= equest >> you gave to GPUVM, i.e. req_addr, req_range, req_obj, req_offset, etc. >> >> Which is what you have to do for any other unmap operation that has keep= =3D=3D true >> anyways, e.g. if D is the exact same as A, B and C. >> >> Cur >> --- >> 1 N >> |---A---|---B---|---C---| >> >> Req >> --- >> 1 N >> |-----------D-----------| > > Ugg, this means carrying around more state between the unmap and map > callbacks, vs. just handing all the data to the driver in a single > callback. For the keep=3D=3Dtrue case, nouveau just seems to skip the > unmap.. I guess in your case the map operation is tolerant of > overwriting existing mappings so this works out, which isn't the case > with io_pgtable. There is no "your case" as far as I'm concerned. Please don't think that I = don't care about solving a problem, just because it's not relevant for any of the drivers or subsystems I maintain. :) > I guess I could handle the specific case of an exact in-place remap in > the driver to handle this specific case. But the example you give > with multiple mappings would be harder to cope with. > > I still feel there is some room for improvement in gpuvm to make this > easier for drivers. Maybe what I proposed isn't the best general > solution, but somehow giving the drivers info about both the unmaps > and maps in the same callback would make things easier (and the remap > callback is _almost_ that). I generally agree with that, my concern is more about this specific patch. There are patches on the list that replace all the req_* arguments of __drm_gpuvm_sm_map() with a new struct drm_gpuvm_map_req. Maybe the unmap callbacks could simply provide a pointer to this object? > BR, > -R > >> >> In this case you get three unmap ops with keep =3D=3D true, which you ca= n compare to >> your request to figure out that you can keep the corresponding PTEs. >> >> Besides that it changes the semantics that the documentation mentions an= d that >> drivers are allowed to rely on, i.e. a struct drm_gpuva_op_remap represe= nts >> an actual change and any call to __drm_gpuvm_sm_map() results in an arbi= trary >> number of unmap ops, a maximum of two remap ops and exactly one map oper= ation. >> >> > ret =3D op_unmap_cb(ops, priv, va, merge= ); >> > if (ret) >> > return ret;