On Wed, Nov 29, 2023 at 04:47:05PM +0100, Boris Brezillon wrote: > On Wed, 29 Nov 2023 16:15:27 +0100 > Maxime Ripard <mripard@xxxxxxxxxx> wrote: > > > > Now, let's assume we drop the _locked() suffix on > > > drm_gem_shmem_v[un]map(), but keep it on other helpers that need both > > > variants. This results in an inconsistent naming scheme inside the > > > same source file, which I find utterly confusing. > > > > > > Note that the initial reason I asked Dmitry if he could add the > > > _locked suffix to drm_gem_shmem_vmap() is because I started using > > > drm_gem_shmem_vmap() in powervr, before realizing this version wasn't > > > taking the lock, and I should have used drm_gem_vmap_unlocked() > > > instead, so this is not something I'm making up. > > > > Sorry if I gave you the impression I thought that you're making that up, > > I'm not. > > > > Thanks for the explanation btw, I think I get what you're saying now: > > > > - drm_gem_shmem_vmap() is never taking the locks because the core > > expects to take them before calling them. > > > > - drm_gem_shmem_vunmap() is never taking the locks because the core > > expects to take them before calling them. > > Correct. > > > > > - Some other code path can still call those helpers in drivers, and the > > locking isn't handled by the core anymore. > > They can, if they want to v[un]map a BO and they already acquired the > GEM resv lock. But I'm not sure anyone needs to do that yet. The main > reason for exposing these helpers is if one driver needs to overload the > default gem_shmem_funcs. > > > > > - We now have _vmap/vunmap_unlocked functions to take those locks for > > those code paths > > We don't have drm_gem_shmem_vmap/vunmap_unlocked(), we have > drm_gem_shmem_vmap/vunmap_locked(), which can be called directly, but > are mainly used to populate the drm_gem_object_funcs vtable. If drivers > want to v[un]map in a path where the resv lock is not held, they should > call drm_gem_vmap/vunmap_unlocked() (which are renamed > drm_gem_vmap/vunmap() in patch 1 of this series). Mind the **drm_gem_** > vs **drm_gem_shmem_** difference in the helper names. drm_gem_ helpers > are provided by drm_gem.c and call drm_gem_object_funcs callback, which > are supposed to be populated with drm_gem_shmem helpers. > > > > > - And the variant names are now confusing, making people use the > > lockless version in situations where they should have use the locked > > one. > > That's what happened to me, at least. > > > > > Is that a correct summary? > > Almost ;-). > > > > > If so, then I agree that we need to change the name. > > Cool. > > > > > We discussed it some more on IRC, and we agree that the "default" > > function should handle the locking properly and that's what the most > > common case should use. > > Agree if by 'default' you mean the lock is always acquired by the > helper, not 'let's decide based on what users do most of the time with > this specific helper', because otherwise we'd be back to a situation > where the name doesn't clearly encode the function behavior. > > > > > So that means than drm_gem_shmem_vmap/vunmap() should take the lock > > itself, and drm_gem_shmem_vmap/vunmap_nolock/unlocked never does. > > Not sure we have a need for drm_gem_shmem_vmap/vunmap(), but if we ever > add such helpers, they would acquire the resv lock, indeed. > > Just to be clear, _nolock == _locked in the current semantics :-). > _nolock means 'don't take the lock', and _locked means 'lock is already > held'. > > > > > I think I'd prefer the nolock variant over unlocked still. > > Okay, that means s/_locked/_nolock/ in drm_gem_shmem_helpers.{c,h}, I > guess. > > > > > And I also think we can improve the documentation and add lockdep calls > > Lockdep asserts are already there, I think. > > > to make sure that the difference between variants is clear in the doc, > > and if someone still get confused we can catch it. > > > > Does that sound like a plan? > > Assuming I understood it correctly, yes. Can you just confirm my > understanding is correct though? We are. Sorry for delaying this :) Maxime
Attachment:
signature.asc
Description: PGP signature