Re: [PATCH v18 04/26] drm/shmem-helper: Refactor locked/unlocked functions

[Maxime Ripard]

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