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

[Boris Brezillon <boris.brezillon@collabora.com>]

On Wed, 29 Nov 2023 16:15:27 +0100
Maxime Ripard <mripard@kernel.org> 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?

Regards,

Boris