From mboxrd@z Thu Jan 1 00:00:00 1970 From: Eric Anholt Subject: Re: [PATCH v3 2/2] drm/lima: driver for ARM Mali4xx GPUs Date: Tue, 05 Mar 2019 12:18:47 -0800 Message-ID: <87imwx5b2w.fsf@anholt.net> References: <20190227134124.28602-1-yuq825@gmail.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============1394747924==" Return-path: In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Rob Herring , Qiang Yu Cc: Marek Vasut , Simon Shields , lima@lists.freedesktop.org, Andreas Baierl , Maxime Ripard , Christian =?utf-8?Q?K=C3=B6nig?= , Neil Armstrong , dri-devel , Vasily Khoruzhick , David Airlie , Sean Paul , Erico Nunes List-Id: dri-devel@lists.freedesktop.org --===============1394747924== Content-Type: multipart/signed; boundary="=-=-="; micalg=pgp-sha512; protocol="application/pgp-signature" --=-=-= Content-Type: text/plain Rob Herring writes: > On Fri, Mar 1, 2019 at 11:23 PM Qiang Yu wrote: >> >> > > +static struct lima_fence *lima_fence_create(struct lima_sched_pipe *pipe) >> > > +{ >> > > + struct lima_fence *fence; >> > > + >> > > + fence = kmem_cache_zalloc(lima_fence_slab, GFP_KERNEL); >> > >> > Out of curiosity, what is the benefit of using a separate slab here? >> > If this is beneficial, then other drivers should do this too and it >> > should be common. Otherwise, it adds some complexity. >> >> fence is pretty frequently alloc free struct, so make it a slab. And it's used >> in get/put pattern, so may live longer than embedded struct. This is referenced >> from amdgpu driver. >> >> > >> > And maybe the slab should be initialzed in probe rather than module_init. >> > >> Either way is OK. But live in module init is easier not to init twice >> for two devices. > > True, but I was thinking more about initializing it for 0 devices > which can be common if built-in on a multi-platform kernel. > >> > > +int lima_vm_bo_add(struct lima_vm *vm, struct lima_bo *bo, bool create) >> > > +{ >> > > + struct lima_bo_va *bo_va; >> > > + int err; >> > > + >> > > + mutex_lock(&bo->lock); >> > > + >> > > + bo_va = lima_vm_bo_find(vm, bo); >> > > + if (bo_va) { >> > > + bo_va->ref_count++; >> > > + mutex_unlock(&bo->lock); >> > > + return 0; >> > > + } >> > > + >> > > + /* should not create new bo_va if not asked by caller */ >> > > + if (!create) { >> > > + mutex_unlock(&bo->lock); >> > > + return -ENOENT; >> > > + } >> > > + >> > > + bo_va = kzalloc(sizeof(*bo_va), GFP_KERNEL); >> > > + if (!bo_va) { >> > > + err = -ENOMEM; >> > > + goto err_out0; >> > > + } >> > > + >> > > + bo_va->vm = vm; >> > > + bo_va->ref_count = 1; >> > > + >> > > + mutex_lock(&vm->lock); >> > > + >> > > + err = drm_mm_insert_node(&vm->mm, &bo_va->node, bo->gem.size); >> > > + if (err) >> > > + goto err_out1; >> > > + >> > > + err = lima_vm_map_page_table(vm, bo->pages_dma_addr, bo_va->node.start, >> > > + bo_va->node.start + bo_va->node.size - 1); >> > > + if (err) >> > > + goto err_out2; >> > > + >> > > + mutex_unlock(&vm->lock); >> > > + >> > > + list_add_tail(&bo_va->list, &bo->va); >> > >> > So you can have 1 BO at multiple VAs? Is that really needed? >> > >> Actually 1 BO can't have multi VA in single VM, but one VA in each VM. >> When a BO is exported/imported between two process, i.e. xserver and client, >> two processes have different VM, so can't make sure it can be mapped at the same >> place. > > Right, but when you import a BO, a new BO struct is created and > therefore a new list. If there's only 1 VA, then you don't need a > list. Just move 'node' into lima_bo. (It is possible I missed some > detail though.) You only make a new GEM BO struct on importing a new dmabuf into the driver -- export/imports between process share the same GEM BO struct (unless I've misread what you're saying). --=-=-= Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCgAdFiEE/JuuFDWp9/ZkuCBXtdYpNtH8nugFAlx+2acACgkQtdYpNtH8 nugfmhAAnOnj9hLZS0V0AnBRvdC6QCmfwdsaSyDyGWStw4O4bnhxqPmKaMMEnTby uyYkTWMOYNtG1zYBlD3U1VDbuWAnCDjsN9EO+oakQyuslnYf9gY3DJ+boGJFhPDp opdCpWxpq0g46sVAuAJJr6jb9/rv4TuQovl+qhH/vC29x6dn1n09U9+lGiYXuNTf 5iNjnjlH2/Dd4J5vx3JvGlnrnnk5hUAPkP8CyzaoQKNIsaho7mxmEogIPhC3ISO2 nK7gMdFqizXkSbK5lDrms4JVwGs21BLP3iu20rxK8tjUUbzPyPT7wGeZ1lk8peP+ sckvU86MDOV2Md+/g0NKkgiajq93e0ByAH12cKFTRDX/K0P4NuFKCQWhij/nYFtQ AZ6rbkpzUv/STDblIdsihcHlTOFS4iJyfffnu4ea7+Sy4FrXiKfCvN9lI0L9rldO K20oAeMnf2SbmNtdMQLhJWnT5r12HRJVc3yZyDVKyVPko0ow6F8MIVa/Ui/MBA67 LQuz8tBdaxYsCIcmQx6NzWRR43laElaY7d072FpySeeaDU/QmbS/o5iZPgtiUFRJ Ohnmql849zTyYaqBkbhJbjMBOqb//lT/emTq94m7MjXAEKmASRK4ulG6sBFCjzAY LuHNv1R2Nu8FMKbKGZd41ICHP4g6QW77nyI2I67bpgF2S6NT1M0= =r2Zf -----END PGP SIGNATURE----- --=-=-=-- --===============1394747924== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KZHJpLWRldmVs IG1haWxpbmcgbGlzdApkcmktZGV2ZWxAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHBzOi8vbGlz dHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8vZHJpLWRldmVs --===============1394747924==--