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

Commit Message

Dmitry Osipenko Oct. 29, 2023, 11:01 p.m. UTC

Add locked and remove unlocked postfixes from drm-shmem function names,
making names consistent with the drm/gem core code.

Reviewed-by: Boris Brezillon <[e-mail]>
Suggested-by: Boris Brezillon <[e-mail]>
Signed-off-by: Dmitry Osipenko <[e-mail]>
---
 drivers/gpu/drm/drm_gem_shmem_helper.c        | 64 +++++++++----------
 drivers/gpu/drm/lima/lima_gem.c               |  8 +--
 drivers/gpu/drm/panfrost/panfrost_drv.c       |  2 +-
 drivers/gpu/drm/panfrost/panfrost_gem.c       |  6 +-
 .../gpu/drm/panfrost/panfrost_gem_shrinker.c  |  2 +-
 drivers/gpu/drm/panfrost/panfrost_mmu.c       |  2 +-
 drivers/gpu/drm/v3d/v3d_bo.c                  |  4 +-
 drivers/gpu/drm/virtio/virtgpu_object.c       |  4 +-
 include/drm/drm_gem_shmem_helper.h            | 36 +++++------
 9 files changed, 64 insertions(+), 64 deletions(-)

Comments

Maxime Ripard Nov. 24, 2023, 10:40 a.m. UTC | #1

On Mon, Oct 30, 2023 at 02:01:43AM +0300, Dmitry Osipenko wrote:
> Add locked and remove unlocked postfixes from drm-shmem function names,
> making names consistent with the drm/gem core code.
> 
> Reviewed-by: Boris Brezillon <[e-mail]>
> Suggested-by: Boris Brezillon <[e-mail]>
> Signed-off-by: Dmitry Osipenko <[e-mail]>

This contradicts my earlier ack on a patch but...

> ---
>  drivers/gpu/drm/drm_gem_shmem_helper.c        | 64 +++++++++----------
>  drivers/gpu/drm/lima/lima_gem.c               |  8 +--
>  drivers/gpu/drm/panfrost/panfrost_drv.c       |  2 +-
>  drivers/gpu/drm/panfrost/panfrost_gem.c       |  6 +-
>  .../gpu/drm/panfrost/panfrost_gem_shrinker.c  |  2 +-
>  drivers/gpu/drm/panfrost/panfrost_mmu.c       |  2 +-
>  drivers/gpu/drm/v3d/v3d_bo.c                  |  4 +-
>  drivers/gpu/drm/virtio/virtgpu_object.c       |  4 +-
>  include/drm/drm_gem_shmem_helper.h            | 36 +++++------
>  9 files changed, 64 insertions(+), 64 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
> index 0d61f2b3e213..154585ddae08 100644
> --- a/drivers/gpu/drm/drm_gem_shmem_helper.c
> +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
> @@ -43,8 +43,8 @@ static const struct drm_gem_object_funcs drm_gem_shmem_funcs = {
>  	.pin = drm_gem_shmem_object_pin,
>  	.unpin = drm_gem_shmem_object_unpin,
>  	.get_sg_table = drm_gem_shmem_object_get_sg_table,
> -	.vmap = drm_gem_shmem_object_vmap,
> -	.vunmap = drm_gem_shmem_object_vunmap,
> +	.vmap = drm_gem_shmem_object_vmap_locked,
> +	.vunmap = drm_gem_shmem_object_vunmap_locked,

While I think we should indeed be consistent with the names, I would
also expect helpers to get the locking right by default.

I'm not sure how reasonable it is, but I think I'd prefer to turn this
around and keep the drm_gem_shmem_object_vmap/unmap helpers name, and
convert whatever function needs to be converted to the unlock suffix so
we get a consistent naming.

Does that make sense?
Maxime

Boris Brezillon Nov. 24, 2023, 10:44 a.m. UTC | #2

On Fri, 24 Nov 2023 11:40:06 +0100
Maxime Ripard <[e-mail]> wrote:

> On Mon, Oct 30, 2023 at 02:01:43AM +0300, Dmitry Osipenko wrote:
> > Add locked and remove unlocked postfixes from drm-shmem function names,
> > making names consistent with the drm/gem core code.
> > 
> > Reviewed-by: Boris Brezillon <[e-mail]>
> > Suggested-by: Boris Brezillon <[e-mail]>
> > Signed-off-by: Dmitry Osipenko <[e-mail]>  
> 
> This contradicts my earlier ack on a patch but...
> 
> > ---
> >  drivers/gpu/drm/drm_gem_shmem_helper.c        | 64 +++++++++----------
> >  drivers/gpu/drm/lima/lima_gem.c               |  8 +--
> >  drivers/gpu/drm/panfrost/panfrost_drv.c       |  2 +-
> >  drivers/gpu/drm/panfrost/panfrost_gem.c       |  6 +-
> >  .../gpu/drm/panfrost/panfrost_gem_shrinker.c  |  2 +-
> >  drivers/gpu/drm/panfrost/panfrost_mmu.c       |  2 +-
> >  drivers/gpu/drm/v3d/v3d_bo.c                  |  4 +-
> >  drivers/gpu/drm/virtio/virtgpu_object.c       |  4 +-
> >  include/drm/drm_gem_shmem_helper.h            | 36 +++++------
> >  9 files changed, 64 insertions(+), 64 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
> > index 0d61f2b3e213..154585ddae08 100644
> > --- a/drivers/gpu/drm/drm_gem_shmem_helper.c
> > +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
> > @@ -43,8 +43,8 @@ static const struct drm_gem_object_funcs drm_gem_shmem_funcs = {
> >  	.pin = drm_gem_shmem_object_pin,
> >  	.unpin = drm_gem_shmem_object_unpin,
> >  	.get_sg_table = drm_gem_shmem_object_get_sg_table,
> > -	.vmap = drm_gem_shmem_object_vmap,
> > -	.vunmap = drm_gem_shmem_object_vunmap,
> > +	.vmap = drm_gem_shmem_object_vmap_locked,
> > +	.vunmap = drm_gem_shmem_object_vunmap_locked,  
> 
> While I think we should indeed be consistent with the names, I would
> also expect helpers to get the locking right by default.
> 
> I'm not sure how reasonable it is, but I think I'd prefer to turn this
> around and keep the drm_gem_shmem_object_vmap/unmap helpers name, and
> convert whatever function needs to be converted to the unlock suffix so
> we get a consistent naming.
> 
> Does that make sense?

I don't mind, as long as it's consistent, it's just that that there's
probably more to patch if we do it the other way around.

Boris Brezillon Nov. 24, 2023, 10:59 a.m. UTC | #3

On Fri, 24 Nov 2023 11:40:06 +0100
Maxime Ripard <[e-mail]> wrote:

> On Mon, Oct 30, 2023 at 02:01:43AM +0300, Dmitry Osipenko wrote:
> > Add locked and remove unlocked postfixes from drm-shmem function names,
> > making names consistent with the drm/gem core code.
> > 
> > Reviewed-by: Boris Brezillon <[e-mail]>
> > Suggested-by: Boris Brezillon <[e-mail]>
> > Signed-off-by: Dmitry Osipenko <[e-mail]>  
> 
> This contradicts my earlier ack on a patch but...
> 
> > ---
> >  drivers/gpu/drm/drm_gem_shmem_helper.c        | 64 +++++++++----------
> >  drivers/gpu/drm/lima/lima_gem.c               |  8 +--
> >  drivers/gpu/drm/panfrost/panfrost_drv.c       |  2 +-
> >  drivers/gpu/drm/panfrost/panfrost_gem.c       |  6 +-
> >  .../gpu/drm/panfrost/panfrost_gem_shrinker.c  |  2 +-
> >  drivers/gpu/drm/panfrost/panfrost_mmu.c       |  2 +-
> >  drivers/gpu/drm/v3d/v3d_bo.c                  |  4 +-
> >  drivers/gpu/drm/virtio/virtgpu_object.c       |  4 +-
> >  include/drm/drm_gem_shmem_helper.h            | 36 +++++------
> >  9 files changed, 64 insertions(+), 64 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
> > index 0d61f2b3e213..154585ddae08 100644
> > --- a/drivers/gpu/drm/drm_gem_shmem_helper.c
> > +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
> > @@ -43,8 +43,8 @@ static const struct drm_gem_object_funcs drm_gem_shmem_funcs = {
> >  	.pin = drm_gem_shmem_object_pin,
> >  	.unpin = drm_gem_shmem_object_unpin,
> >  	.get_sg_table = drm_gem_shmem_object_get_sg_table,
> > -	.vmap = drm_gem_shmem_object_vmap,
> > -	.vunmap = drm_gem_shmem_object_vunmap,
> > +	.vmap = drm_gem_shmem_object_vmap_locked,
> > +	.vunmap = drm_gem_shmem_object_vunmap_locked,  
> 
> While I think we should indeed be consistent with the names, I would
> also expect helpers to get the locking right by default.

Wait, actually I think this patch does what you suggest already. The
_locked() prefix tells the caller: "you should take care of the locking,
I expect the lock to be held when this is hook/function is called". So
helpers without the _locked() prefix take care of the locking (which I
guess matches your 'helpers get the locking right' expectation), and
those with the _locked() prefix don't.

> 
> I'm not sure how reasonable it is, but I think I'd prefer to turn this
> around and keep the drm_gem_shmem_object_vmap/unmap helpers name, and
> convert whatever function needs to be converted to the unlock suffix so
> we get a consistent naming.

That would be an _unlocked() prefix if we do it the other way around. I
think the main confusion comes from the names of the hooks in
drm_gem_shmem_funcs. Some of them, like drm_gem_shmem_funcs::v[un]map()
are called with the GEM resv lock held, and locking is handled by the
core, others, like drm_gem_shmem_funcs::[un]pin() are called
without the GEM resv lock held, and locking is deferred to the
implementation. As I said, I don't mind prefixing hooks/helpers with
_unlocked() for those that take care of the locking, and no prefix for
those that expects locks to be held, as long as it's consistent, but I
just wanted to make sure we're on the same page :-).

Maxime Ripard Nov. 28, 2023, 11:14 a.m. UTC | #4

Hi,

On Fri, Nov 24, 2023 at 11:59:11AM +0100, Boris Brezillon wrote:
> On Fri, 24 Nov 2023 11:40:06 +0100
> Maxime Ripard <[e-mail]> wrote:
> 
> > On Mon, Oct 30, 2023 at 02:01:43AM +0300, Dmitry Osipenko wrote:
> > > Add locked and remove unlocked postfixes from drm-shmem function names,
> > > making names consistent with the drm/gem core code.
> > > 
> > > Reviewed-by: Boris Brezillon <[e-mail]>
> > > Suggested-by: Boris Brezillon <[e-mail]>
> > > Signed-off-by: Dmitry Osipenko <[e-mail]>  
> > 
> > This contradicts my earlier ack on a patch but...
> > 
> > > ---
> > >  drivers/gpu/drm/drm_gem_shmem_helper.c        | 64 +++++++++----------
> > >  drivers/gpu/drm/lima/lima_gem.c               |  8 +--
> > >  drivers/gpu/drm/panfrost/panfrost_drv.c       |  2 +-
> > >  drivers/gpu/drm/panfrost/panfrost_gem.c       |  6 +-
> > >  .../gpu/drm/panfrost/panfrost_gem_shrinker.c  |  2 +-
> > >  drivers/gpu/drm/panfrost/panfrost_mmu.c       |  2 +-
> > >  drivers/gpu/drm/v3d/v3d_bo.c                  |  4 +-
> > >  drivers/gpu/drm/virtio/virtgpu_object.c       |  4 +-
> > >  include/drm/drm_gem_shmem_helper.h            | 36 +++++------
> > >  9 files changed, 64 insertions(+), 64 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
> > > index 0d61f2b3e213..154585ddae08 100644
> > > --- a/drivers/gpu/drm/drm_gem_shmem_helper.c
> > > +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
> > > @@ -43,8 +43,8 @@ static const struct drm_gem_object_funcs drm_gem_shmem_funcs = {
> > >  	.pin = drm_gem_shmem_object_pin,
> > >  	.unpin = drm_gem_shmem_object_unpin,
> > >  	.get_sg_table = drm_gem_shmem_object_get_sg_table,
> > > -	.vmap = drm_gem_shmem_object_vmap,
> > > -	.vunmap = drm_gem_shmem_object_vunmap,
> > > +	.vmap = drm_gem_shmem_object_vmap_locked,
> > > +	.vunmap = drm_gem_shmem_object_vunmap_locked,  
> > 
> > While I think we should indeed be consistent with the names, I would
> > also expect helpers to get the locking right by default.
> 
> Wait, actually I think this patch does what you suggest already. The
> _locked() prefix tells the caller: "you should take care of the locking,
> I expect the lock to be held when this is hook/function is called". So
> helpers without the _locked() prefix take care of the locking (which I
> guess matches your 'helpers get the locking right' expectation), and
> those with the _locked() prefix don't.

What I meant by "getting the locking right" is indeed a bit ambiguous,
sorry. What I'm trying to say I guess is that, in this particular case,
I don't think you can expect the vmap implementation to be called with
or without the locks held. The doc for that function will say that it's
either one or the other, but not both.

So helpers should follow what is needed to provide a default vmap/vunmap
implementation, including what locking is expected from a vmap/vunmap
implementation.

If that means that vmap is always called with the locks taken, then
drm_gem_shmem_object_vmap can just assume that it will be called with
the locks taken and there's no need to mention it in the name (and you
can probably sprinkle a couple of lockdep assertion to make sure the
locking is indeed consistent).

> > I'm not sure how reasonable it is, but I think I'd prefer to turn this
> > around and keep the drm_gem_shmem_object_vmap/unmap helpers name, and
> > convert whatever function needs to be converted to the unlock suffix so
> > we get a consistent naming.
> 
> That would be an _unlocked() prefix if we do it the other way around. I
> think the main confusion comes from the names of the hooks in
> drm_gem_shmem_funcs. Some of them, like drm_gem_shmem_funcs::v[un]map()
> are called with the GEM resv lock held, and locking is handled by the
> core, others, like drm_gem_shmem_funcs::[un]pin() are called
> without the GEM resv lock held, and locking is deferred to the
> implementation. As I said, I don't mind prefixing hooks/helpers with
> _unlocked() for those that take care of the locking, and no prefix for
> those that expects locks to be held, as long as it's consistent, but I
> just wanted to make sure we're on the same page :-).

What about _nolock then? It's the same number of characters than
_locked, plus it expresses what the function is (not) doing, not what
context it's supposed to be called in?

Maxime

Boris Brezillon Nov. 28, 2023, 12:37 p.m. UTC | #5

On Tue, 28 Nov 2023 12:14:42 +0100
Maxime Ripard <[e-mail]> wrote:

> Hi,
> 
> On Fri, Nov 24, 2023 at 11:59:11AM +0100, Boris Brezillon wrote:
> > On Fri, 24 Nov 2023 11:40:06 +0100
> > Maxime Ripard <[e-mail]> wrote:
> >   
> > > On Mon, Oct 30, 2023 at 02:01:43AM +0300, Dmitry Osipenko wrote:  
> > > > Add locked and remove unlocked postfixes from drm-shmem function names,
> > > > making names consistent with the drm/gem core code.
> > > > 
> > > > Reviewed-by: Boris Brezillon <[e-mail]>
> > > > Suggested-by: Boris Brezillon <[e-mail]>
> > > > Signed-off-by: Dmitry Osipenko <[e-mail]>    
> > > 
> > > This contradicts my earlier ack on a patch but...
> > >   
> > > > ---
> > > >  drivers/gpu/drm/drm_gem_shmem_helper.c        | 64 +++++++++----------
> > > >  drivers/gpu/drm/lima/lima_gem.c               |  8 +--
> > > >  drivers/gpu/drm/panfrost/panfrost_drv.c       |  2 +-
> > > >  drivers/gpu/drm/panfrost/panfrost_gem.c       |  6 +-
> > > >  .../gpu/drm/panfrost/panfrost_gem_shrinker.c  |  2 +-
> > > >  drivers/gpu/drm/panfrost/panfrost_mmu.c       |  2 +-
> > > >  drivers/gpu/drm/v3d/v3d_bo.c                  |  4 +-
> > > >  drivers/gpu/drm/virtio/virtgpu_object.c       |  4 +-
> > > >  include/drm/drm_gem_shmem_helper.h            | 36 +++++------
> > > >  9 files changed, 64 insertions(+), 64 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
> > > > index 0d61f2b3e213..154585ddae08 100644
> > > > --- a/drivers/gpu/drm/drm_gem_shmem_helper.c
> > > > +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
> > > > @@ -43,8 +43,8 @@ static const struct drm_gem_object_funcs drm_gem_shmem_funcs = {
> > > >  	.pin = drm_gem_shmem_object_pin,
> > > >  	.unpin = drm_gem_shmem_object_unpin,
> > > >  	.get_sg_table = drm_gem_shmem_object_get_sg_table,
> > > > -	.vmap = drm_gem_shmem_object_vmap,
> > > > -	.vunmap = drm_gem_shmem_object_vunmap,
> > > > +	.vmap = drm_gem_shmem_object_vmap_locked,
> > > > +	.vunmap = drm_gem_shmem_object_vunmap_locked,    
> > > 
> > > While I think we should indeed be consistent with the names, I would
> > > also expect helpers to get the locking right by default.  
> > 
> > Wait, actually I think this patch does what you suggest already. The
> > _locked() prefix tells the caller: "you should take care of the locking,
> > I expect the lock to be held when this is hook/function is called". So
> > helpers without the _locked() prefix take care of the locking (which I
> > guess matches your 'helpers get the locking right' expectation), and
> > those with the _locked() prefix don't.  
> 
> What I meant by "getting the locking right" is indeed a bit ambiguous,
> sorry. What I'm trying to say I guess is that, in this particular case,
> I don't think you can expect the vmap implementation to be called with
> or without the locks held. The doc for that function will say that it's
> either one or the other, but not both.
> 
> So helpers should follow what is needed to provide a default vmap/vunmap
> implementation, including what locking is expected from a vmap/vunmap
> implementation.

Hm, yeah, I think that's a matter of taste. When locking is often
deferrable, like it is in DRM, I find it beneficial for funcions and
function pointers to reflect the locking scheme, rather than relying on
people properly reading the doc, especially when this is the only
outlier in the group of drm_gem_object_funcs we already have, and it's
not event documented at the drm_gem_object_funcs level [1] :P.

> 
> If that means that vmap is always called with the locks taken, then
> drm_gem_shmem_object_vmap can just assume that it will be called with
> the locks taken and there's no need to mention it in the name (and you
> can probably sprinkle a couple of lockdep assertion to make sure the
> locking is indeed consistent).

Things get very confusing when you end up having drm_gem_shmem helpers
that are suffixed with _locked() to encode the fact locking is the
caller's responsibility and no suffix for the
callee-takes-care-of-the-locking semantics, while other helpers that are
not suffixed at all actually implement the
caller-should-take-care-of-the-locking semantics.

> 
> > > I'm not sure how reasonable it is, but I think I'd prefer to turn this
> > > around and keep the drm_gem_shmem_object_vmap/unmap helpers name, and
> > > convert whatever function needs to be converted to the unlock suffix so
> > > we get a consistent naming.  
> > 
> > That would be an _unlocked() prefix if we do it the other way around. I
> > think the main confusion comes from the names of the hooks in
> > drm_gem_shmem_funcs. Some of them, like drm_gem_shmem_funcs::v[un]map()
> > are called with the GEM resv lock held, and locking is handled by the
> > core, others, like drm_gem_shmem_funcs::[un]pin() are called
> > without the GEM resv lock held, and locking is deferred to the
> > implementation. As I said, I don't mind prefixing hooks/helpers with
> > _unlocked() for those that take care of the locking, and no prefix for
> > those that expects locks to be held, as long as it's consistent, but I
> > just wanted to make sure we're on the same page :-).  
> 
> What about _nolock then? It's the same number of characters than
> _locked, plus it expresses what the function is (not) doing, not what
> context it's supposed to be called in?

Just did a quick

  git grep _nolock drivers/gpu/drm

and it returns zero result, where the _locked/_unlocked pattern seems
to already be widely used. Not saying we shouldn't change that, but it
doesn't feel like a change we should do as part of this series.

Regards,

Boris

[1]https://elixir.bootlin.com/linux/v6.7-rc3/source/include/drm/drm_gem.h#L155

Dmitry Osipenko Nov. 28, 2023, 10:05 p.m. UTC | #6

On 11/28/23 15:37, Boris Brezillon wrote:
> On Tue, 28 Nov 2023 12:14:42 +0100
> Maxime Ripard <[e-mail]> wrote:
> 
>> Hi,
>>
>> On Fri, Nov 24, 2023 at 11:59:11AM +0100, Boris Brezillon wrote:
>>> On Fri, 24 Nov 2023 11:40:06 +0100
>>> Maxime Ripard <[e-mail]> wrote:
>>>   
>>>> On Mon, Oct 30, 2023 at 02:01:43AM +0300, Dmitry Osipenko wrote:  
>>>>> Add locked and remove unlocked postfixes from drm-shmem function names,
>>>>> making names consistent with the drm/gem core code.
>>>>>
>>>>> Reviewed-by: Boris Brezillon <[e-mail]>
>>>>> Suggested-by: Boris Brezillon <[e-mail]>
>>>>> Signed-off-by: Dmitry Osipenko <[e-mail]>    
>>>>
>>>> This contradicts my earlier ack on a patch but...
>>>>   
>>>>> ---
>>>>>  drivers/gpu/drm/drm_gem_shmem_helper.c        | 64 +++++++++----------
>>>>>  drivers/gpu/drm/lima/lima_gem.c               |  8 +--
>>>>>  drivers/gpu/drm/panfrost/panfrost_drv.c       |  2 +-
>>>>>  drivers/gpu/drm/panfrost/panfrost_gem.c       |  6 +-
>>>>>  .../gpu/drm/panfrost/panfrost_gem_shrinker.c  |  2 +-
>>>>>  drivers/gpu/drm/panfrost/panfrost_mmu.c       |  2 +-
>>>>>  drivers/gpu/drm/v3d/v3d_bo.c                  |  4 +-
>>>>>  drivers/gpu/drm/virtio/virtgpu_object.c       |  4 +-
>>>>>  include/drm/drm_gem_shmem_helper.h            | 36 +++++------
>>>>>  9 files changed, 64 insertions(+), 64 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
>>>>> index 0d61f2b3e213..154585ddae08 100644
>>>>> --- a/drivers/gpu/drm/drm_gem_shmem_helper.c
>>>>> +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
>>>>> @@ -43,8 +43,8 @@ static const struct drm_gem_object_funcs drm_gem_shmem_funcs = {
>>>>>  	.pin = drm_gem_shmem_object_pin,
>>>>>  	.unpin = drm_gem_shmem_object_unpin,
>>>>>  	.get_sg_table = drm_gem_shmem_object_get_sg_table,
>>>>> -	.vmap = drm_gem_shmem_object_vmap,
>>>>> -	.vunmap = drm_gem_shmem_object_vunmap,
>>>>> +	.vmap = drm_gem_shmem_object_vmap_locked,
>>>>> +	.vunmap = drm_gem_shmem_object_vunmap_locked,    
>>>>
>>>> While I think we should indeed be consistent with the names, I would
>>>> also expect helpers to get the locking right by default.  
>>>
>>> Wait, actually I think this patch does what you suggest already. The
>>> _locked() prefix tells the caller: "you should take care of the locking,
>>> I expect the lock to be held when this is hook/function is called". So
>>> helpers without the _locked() prefix take care of the locking (which I
>>> guess matches your 'helpers get the locking right' expectation), and
>>> those with the _locked() prefix don't.  
>>
>> What I meant by "getting the locking right" is indeed a bit ambiguous,
>> sorry. What I'm trying to say I guess is that, in this particular case,
>> I don't think you can expect the vmap implementation to be called with
>> or without the locks held. The doc for that function will say that it's
>> either one or the other, but not both.
>>
>> So helpers should follow what is needed to provide a default vmap/vunmap
>> implementation, including what locking is expected from a vmap/vunmap
>> implementation.
> 
> Hm, yeah, I think that's a matter of taste. When locking is often
> deferrable, like it is in DRM, I find it beneficial for funcions and
> function pointers to reflect the locking scheme, rather than relying on
> people properly reading the doc, especially when this is the only
> outlier in the group of drm_gem_object_funcs we already have, and it's
> not event documented at the drm_gem_object_funcs level [1] :P.
> 
>>
>> If that means that vmap is always called with the locks taken, then
>> drm_gem_shmem_object_vmap can just assume that it will be called with
>> the locks taken and there's no need to mention it in the name (and you
>> can probably sprinkle a couple of lockdep assertion to make sure the
>> locking is indeed consistent).
> 
> Things get very confusing when you end up having drm_gem_shmem helpers
> that are suffixed with _locked() to encode the fact locking is the
> caller's responsibility and no suffix for the
> callee-takes-care-of-the-locking semantics, while other helpers that are
> not suffixed at all actually implement the
> caller-should-take-care-of-the-locking semantics.
> 
>>
>>>> I'm not sure how reasonable it is, but I think I'd prefer to turn this
>>>> around and keep the drm_gem_shmem_object_vmap/unmap helpers name, and
>>>> convert whatever function needs to be converted to the unlock suffix so
>>>> we get a consistent naming.  
>>>
>>> That would be an _unlocked() prefix if we do it the other way around. I
>>> think the main confusion comes from the names of the hooks in
>>> drm_gem_shmem_funcs. Some of them, like drm_gem_shmem_funcs::v[un]map()
>>> are called with the GEM resv lock held, and locking is handled by the
>>> core, others, like drm_gem_shmem_funcs::[un]pin() are called
>>> without the GEM resv lock held, and locking is deferred to the
>>> implementation. As I said, I don't mind prefixing hooks/helpers with
>>> _unlocked() for those that take care of the locking, and no prefix for
>>> those that expects locks to be held, as long as it's consistent, but I
>>> just wanted to make sure we're on the same page :-).  
>>
>> What about _nolock then? It's the same number of characters than
>> _locked, plus it expresses what the function is (not) doing, not what
>> context it's supposed to be called in?
> 
> Just did a quick
> 
>   git grep _nolock drivers/gpu/drm
> 
> and it returns zero result, where the _locked/_unlocked pattern seems
> to already be widely used. Not saying we shouldn't change that, but it
> doesn't feel like a change we should do as part of this series.
> 
> Regards,
> 
> Boris
> 
> [1]https://elixir.bootlin.com/linux/v6.7-rc3/source/include/drm/drm_gem.h#L155

I'm fine with dropping the _locked() postfix from the common GEM helpers
and documenting the locking rule in drm_gem. Thank you all for the
suggestions :)

Boris Brezillon Nov. 29, 2023, 7:53 a.m. UTC | #7

On Wed, 29 Nov 2023 01:05:14 +0300
Dmitry Osipenko <[e-mail]> wrote:

> On 11/28/23 15:37, Boris Brezillon wrote:
> > On Tue, 28 Nov 2023 12:14:42 +0100
> > Maxime Ripard <[e-mail]> wrote:
> >   
> >> Hi,
> >>
> >> On Fri, Nov 24, 2023 at 11:59:11AM +0100, Boris Brezillon wrote:  
> >>> On Fri, 24 Nov 2023 11:40:06 +0100
> >>> Maxime Ripard <[e-mail]> wrote:
> >>>     
> >>>> On Mon, Oct 30, 2023 at 02:01:43AM +0300, Dmitry Osipenko wrote:    
> >>>>> Add locked and remove unlocked postfixes from drm-shmem function names,
> >>>>> making names consistent with the drm/gem core code.
> >>>>>
> >>>>> Reviewed-by: Boris Brezillon <[e-mail]>
> >>>>> Suggested-by: Boris Brezillon <[e-mail]>
> >>>>> Signed-off-by: Dmitry Osipenko <[e-mail]>      
> >>>>
> >>>> This contradicts my earlier ack on a patch but...
> >>>>     
> >>>>> ---
> >>>>>  drivers/gpu/drm/drm_gem_shmem_helper.c        | 64 +++++++++----------
> >>>>>  drivers/gpu/drm/lima/lima_gem.c               |  8 +--
> >>>>>  drivers/gpu/drm/panfrost/panfrost_drv.c       |  2 +-
> >>>>>  drivers/gpu/drm/panfrost/panfrost_gem.c       |  6 +-
> >>>>>  .../gpu/drm/panfrost/panfrost_gem_shrinker.c  |  2 +-
> >>>>>  drivers/gpu/drm/panfrost/panfrost_mmu.c       |  2 +-
> >>>>>  drivers/gpu/drm/v3d/v3d_bo.c                  |  4 +-
> >>>>>  drivers/gpu/drm/virtio/virtgpu_object.c       |  4 +-
> >>>>>  include/drm/drm_gem_shmem_helper.h            | 36 +++++------
> >>>>>  9 files changed, 64 insertions(+), 64 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
> >>>>> index 0d61f2b3e213..154585ddae08 100644
> >>>>> --- a/drivers/gpu/drm/drm_gem_shmem_helper.c
> >>>>> +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
> >>>>> @@ -43,8 +43,8 @@ static const struct drm_gem_object_funcs drm_gem_shmem_funcs = {
> >>>>>  	.pin = drm_gem_shmem_object_pin,
> >>>>>  	.unpin = drm_gem_shmem_object_unpin,
> >>>>>  	.get_sg_table = drm_gem_shmem_object_get_sg_table,
> >>>>> -	.vmap = drm_gem_shmem_object_vmap,
> >>>>> -	.vunmap = drm_gem_shmem_object_vunmap,
> >>>>> +	.vmap = drm_gem_shmem_object_vmap_locked,
> >>>>> +	.vunmap = drm_gem_shmem_object_vunmap_locked,      
> >>>>
> >>>> While I think we should indeed be consistent with the names, I would
> >>>> also expect helpers to get the locking right by default.    
> >>>
> >>> Wait, actually I think this patch does what you suggest already. The
> >>> _locked() prefix tells the caller: "you should take care of the locking,
> >>> I expect the lock to be held when this is hook/function is called". So
> >>> helpers without the _locked() prefix take care of the locking (which I
> >>> guess matches your 'helpers get the locking right' expectation), and
> >>> those with the _locked() prefix don't.    
> >>
> >> What I meant by "getting the locking right" is indeed a bit ambiguous,
> >> sorry. What I'm trying to say I guess is that, in this particular case,
> >> I don't think you can expect the vmap implementation to be called with
> >> or without the locks held. The doc for that function will say that it's
> >> either one or the other, but not both.
> >>
> >> So helpers should follow what is needed to provide a default vmap/vunmap
> >> implementation, including what locking is expected from a vmap/vunmap
> >> implementation.  
> > 
> > Hm, yeah, I think that's a matter of taste. When locking is often
> > deferrable, like it is in DRM, I find it beneficial for funcions and
> > function pointers to reflect the locking scheme, rather than relying on
> > people properly reading the doc, especially when this is the only
> > outlier in the group of drm_gem_object_funcs we already have, and it's
> > not event documented at the drm_gem_object_funcs level [1] :P.
> >   
> >>
> >> If that means that vmap is always called with the locks taken, then
> >> drm_gem_shmem_object_vmap can just assume that it will be called with
> >> the locks taken and there's no need to mention it in the name (and you
> >> can probably sprinkle a couple of lockdep assertion to make sure the
> >> locking is indeed consistent).  
> > 
> > Things get very confusing when you end up having drm_gem_shmem helpers
> > that are suffixed with _locked() to encode the fact locking is the
> > caller's responsibility and no suffix for the
> > callee-takes-care-of-the-locking semantics, while other helpers that are
> > not suffixed at all actually implement the
> > caller-should-take-care-of-the-locking semantics.
> >   
> >>  
> >>>> I'm not sure how reasonable it is, but I think I'd prefer to turn this
> >>>> around and keep the drm_gem_shmem_object_vmap/unmap helpers name, and
> >>>> convert whatever function needs to be converted to the unlock suffix so
> >>>> we get a consistent naming.    
> >>>
> >>> That would be an _unlocked() prefix if we do it the other way around. I
> >>> think the main confusion comes from the names of the hooks in
> >>> drm_gem_shmem_funcs. Some of them, like drm_gem_shmem_funcs::v[un]map()
> >>> are called with the GEM resv lock held, and locking is handled by the
> >>> core, others, like drm_gem_shmem_funcs::[un]pin() are called
> >>> without the GEM resv lock held, and locking is deferred to the
> >>> implementation. As I said, I don't mind prefixing hooks/helpers with
> >>> _unlocked() for those that take care of the locking, and no prefix for
> >>> those that expects locks to be held, as long as it's consistent, but I
> >>> just wanted to make sure we're on the same page :-).    
> >>
> >> What about _nolock then? It's the same number of characters than
> >> _locked, plus it expresses what the function is (not) doing, not what
> >> context it's supposed to be called in?  
> > 
> > Just did a quick
> > 
> >   git grep _nolock drivers/gpu/drm
> > 
> > and it returns zero result, where the _locked/_unlocked pattern seems
> > to already be widely used. Not saying we shouldn't change that, but it
> > doesn't feel like a change we should do as part of this series.
> > 
> > Regards,
> > 
> > Boris
> > 
> > [1]https://elixir.bootlin.com/linux/v6.7-rc3/source/include/drm/drm_gem.h#L155  
> 
> I'm fine with dropping the _locked() postfix from the common GEM helpers
> and documenting the locking rule in drm_gem. Thank you all for the
> suggestions :)

Sorry to disagree, but I think a proper function name/suffix is
sometimes worth a few lines of doc. Not saying we should do one or the
other, I think we should do both. But when I see a function suffixed
_locked, _unlocked or _nolock, I can immediately tell if this function
defers the locking to the caller or not, and then go check which lock
in the function doc.

And the second thing I'm not happy with, is the fact we go back to an
inconsistent naming in drm_gem_shmem_helper.c, where some functions
deferring the locking to the caller are suffixed _locked and others are
not, because ultimately, you need a different name when you expose the
two variants...

Dmitry Osipenko Nov. 29, 2023, 10:47 a.m. UTC | #8

On 11/29/23 10:53, Boris Brezillon wrote:
> On Wed, 29 Nov 2023 01:05:14 +0300
> Dmitry Osipenko <[e-mail]> wrote:
> 
>> On 11/28/23 15:37, Boris Brezillon wrote:
>>> On Tue, 28 Nov 2023 12:14:42 +0100
>>> Maxime Ripard <[e-mail]> wrote:
>>>   
>>>> Hi,
>>>>
>>>> On Fri, Nov 24, 2023 at 11:59:11AM +0100, Boris Brezillon wrote:  
>>>>> On Fri, 24 Nov 2023 11:40:06 +0100
>>>>> Maxime Ripard <[e-mail]> wrote:
>>>>>     
>>>>>> On Mon, Oct 30, 2023 at 02:01:43AM +0300, Dmitry Osipenko wrote:    
>>>>>>> Add locked and remove unlocked postfixes from drm-shmem function names,
>>>>>>> making names consistent with the drm/gem core code.
>>>>>>>
>>>>>>> Reviewed-by: Boris Brezillon <[e-mail]>
>>>>>>> Suggested-by: Boris Brezillon <[e-mail]>
>>>>>>> Signed-off-by: Dmitry Osipenko <[e-mail]>      
>>>>>>
>>>>>> This contradicts my earlier ack on a patch but...
>>>>>>     
>>>>>>> ---
>>>>>>>  drivers/gpu/drm/drm_gem_shmem_helper.c        | 64 +++++++++----------
>>>>>>>  drivers/gpu/drm/lima/lima_gem.c               |  8 +--
>>>>>>>  drivers/gpu/drm/panfrost/panfrost_drv.c       |  2 +-
>>>>>>>  drivers/gpu/drm/panfrost/panfrost_gem.c       |  6 +-
>>>>>>>  .../gpu/drm/panfrost/panfrost_gem_shrinker.c  |  2 +-
>>>>>>>  drivers/gpu/drm/panfrost/panfrost_mmu.c       |  2 +-
>>>>>>>  drivers/gpu/drm/v3d/v3d_bo.c                  |  4 +-
>>>>>>>  drivers/gpu/drm/virtio/virtgpu_object.c       |  4 +-
>>>>>>>  include/drm/drm_gem_shmem_helper.h            | 36 +++++------
>>>>>>>  9 files changed, 64 insertions(+), 64 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
>>>>>>> index 0d61f2b3e213..154585ddae08 100644
>>>>>>> --- a/drivers/gpu/drm/drm_gem_shmem_helper.c
>>>>>>> +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
>>>>>>> @@ -43,8 +43,8 @@ static const struct drm_gem_object_funcs drm_gem_shmem_funcs = {
>>>>>>>  	.pin = drm_gem_shmem_object_pin,
>>>>>>>  	.unpin = drm_gem_shmem_object_unpin,
>>>>>>>  	.get_sg_table = drm_gem_shmem_object_get_sg_table,
>>>>>>> -	.vmap = drm_gem_shmem_object_vmap,
>>>>>>> -	.vunmap = drm_gem_shmem_object_vunmap,
>>>>>>> +	.vmap = drm_gem_shmem_object_vmap_locked,
>>>>>>> +	.vunmap = drm_gem_shmem_object_vunmap_locked,      
>>>>>>
>>>>>> While I think we should indeed be consistent with the names, I would
>>>>>> also expect helpers to get the locking right by default.    
>>>>>
>>>>> Wait, actually I think this patch does what you suggest already. The
>>>>> _locked() prefix tells the caller: "you should take care of the locking,
>>>>> I expect the lock to be held when this is hook/function is called". So
>>>>> helpers without the _locked() prefix take care of the locking (which I
>>>>> guess matches your 'helpers get the locking right' expectation), and
>>>>> those with the _locked() prefix don't.    
>>>>
>>>> What I meant by "getting the locking right" is indeed a bit ambiguous,
>>>> sorry. What I'm trying to say I guess is that, in this particular case,
>>>> I don't think you can expect the vmap implementation to be called with
>>>> or without the locks held. The doc for that function will say that it's
>>>> either one or the other, but not both.
>>>>
>>>> So helpers should follow what is needed to provide a default vmap/vunmap
>>>> implementation, including what locking is expected from a vmap/vunmap
>>>> implementation.  
>>>
>>> Hm, yeah, I think that's a matter of taste. When locking is often
>>> deferrable, like it is in DRM, I find it beneficial for funcions and
>>> function pointers to reflect the locking scheme, rather than relying on
>>> people properly reading the doc, especially when this is the only
>>> outlier in the group of drm_gem_object_funcs we already have, and it's
>>> not event documented at the drm_gem_object_funcs level [1] :P.
>>>   
>>>>
>>>> If that means that vmap is always called with the locks taken, then
>>>> drm_gem_shmem_object_vmap can just assume that it will be called with
>>>> the locks taken and there's no need to mention it in the name (and you
>>>> can probably sprinkle a couple of lockdep assertion to make sure the
>>>> locking is indeed consistent).  
>>>
>>> Things get very confusing when you end up having drm_gem_shmem helpers
>>> that are suffixed with _locked() to encode the fact locking is the
>>> caller's responsibility and no suffix for the
>>> callee-takes-care-of-the-locking semantics, while other helpers that are
>>> not suffixed at all actually implement the
>>> caller-should-take-care-of-the-locking semantics.
>>>   
>>>>  
>>>>>> I'm not sure how reasonable it is, but I think I'd prefer to turn this
>>>>>> around and keep the drm_gem_shmem_object_vmap/unmap helpers name, and
>>>>>> convert whatever function needs to be converted to the unlock suffix so
>>>>>> we get a consistent naming.    
>>>>>
>>>>> That would be an _unlocked() prefix if we do it the other way around. I
>>>>> think the main confusion comes from the names of the hooks in
>>>>> drm_gem_shmem_funcs. Some of them, like drm_gem_shmem_funcs::v[un]map()
>>>>> are called with the GEM resv lock held, and locking is handled by the
>>>>> core, others, like drm_gem_shmem_funcs::[un]pin() are called
>>>>> without the GEM resv lock held, and locking is deferred to the
>>>>> implementation. As I said, I don't mind prefixing hooks/helpers with
>>>>> _unlocked() for those that take care of the locking, and no prefix for
>>>>> those that expects locks to be held, as long as it's consistent, but I
>>>>> just wanted to make sure we're on the same page :-).    
>>>>
>>>> What about _nolock then? It's the same number of characters than
>>>> _locked, plus it expresses what the function is (not) doing, not what
>>>> context it's supposed to be called in?  
>>>
>>> Just did a quick
>>>
>>>   git grep _nolock drivers/gpu/drm
>>>
>>> and it returns zero result, where the _locked/_unlocked pattern seems
>>> to already be widely used. Not saying we shouldn't change that, but it
>>> doesn't feel like a change we should do as part of this series.
>>>
>>> Regards,
>>>
>>> Boris
>>>
>>> [1]https://elixir.bootlin.com/linux/v6.7-rc3/source/include/drm/drm_gem.h#L155  
>>
>> I'm fine with dropping the _locked() postfix from the common GEM helpers
>> and documenting the locking rule in drm_gem. Thank you all for the
>> suggestions :)
> 
> Sorry to disagree, but I think a proper function name/suffix is
> sometimes worth a few lines of doc. Not saying we should do one or the
> other, I think we should do both. But when I see a function suffixed
> _locked, _unlocked or _nolock, I can immediately tell if this function
> defers the locking to the caller or not, and then go check which lock
> in the function doc.
> 
> And the second thing I'm not happy with, is the fact we go back to an
> inconsistent naming in drm_gem_shmem_helper.c, where some functions
> deferring the locking to the caller are suffixed _locked and others are
> not, because ultimately, you need a different name when you expose the
> two variants...

By the `common GEM helpers` I meant the .vmap drm-shmem common helpers
used for drm_gem_object_funcs, like was suggested by Maxime. The rest of
functions will retain the _locked part. Sorry for the confusion :)

Boris Brezillon Nov. 29, 2023, 10:57 a.m. UTC | #9

On Wed, 29 Nov 2023 13:47:21 +0300
Dmitry Osipenko <[e-mail]> wrote:

> On 11/29/23 10:53, Boris Brezillon wrote:
> > On Wed, 29 Nov 2023 01:05:14 +0300
> > Dmitry Osipenko <[e-mail]> wrote:
> >   
> >> On 11/28/23 15:37, Boris Brezillon wrote:  
> >>> On Tue, 28 Nov 2023 12:14:42 +0100
> >>> Maxime Ripard <[e-mail]> wrote:
> >>>     
> >>>> Hi,
> >>>>
> >>>> On Fri, Nov 24, 2023 at 11:59:11AM +0100, Boris Brezillon wrote:    
> >>>>> On Fri, 24 Nov 2023 11:40:06 +0100
> >>>>> Maxime Ripard <[e-mail]> wrote:
> >>>>>       
> >>>>>> On Mon, Oct 30, 2023 at 02:01:43AM +0300, Dmitry Osipenko wrote:      
> >>>>>>> Add locked and remove unlocked postfixes from drm-shmem function names,
> >>>>>>> making names consistent with the drm/gem core code.
> >>>>>>>
> >>>>>>> Reviewed-by: Boris Brezillon <[e-mail]>
> >>>>>>> Suggested-by: Boris Brezillon <[e-mail]>
> >>>>>>> Signed-off-by: Dmitry Osipenko <[e-mail]>        
> >>>>>>
> >>>>>> This contradicts my earlier ack on a patch but...
> >>>>>>       
> >>>>>>> ---
> >>>>>>>  drivers/gpu/drm/drm_gem_shmem_helper.c        | 64 +++++++++----------
> >>>>>>>  drivers/gpu/drm/lima/lima_gem.c               |  8 +--
> >>>>>>>  drivers/gpu/drm/panfrost/panfrost_drv.c       |  2 +-
> >>>>>>>  drivers/gpu/drm/panfrost/panfrost_gem.c       |  6 +-
> >>>>>>>  .../gpu/drm/panfrost/panfrost_gem_shrinker.c  |  2 +-
> >>>>>>>  drivers/gpu/drm/panfrost/panfrost_mmu.c       |  2 +-
> >>>>>>>  drivers/gpu/drm/v3d/v3d_bo.c                  |  4 +-
> >>>>>>>  drivers/gpu/drm/virtio/virtgpu_object.c       |  4 +-
> >>>>>>>  include/drm/drm_gem_shmem_helper.h            | 36 +++++------
> >>>>>>>  9 files changed, 64 insertions(+), 64 deletions(-)
> >>>>>>>
> >>>>>>> diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
> >>>>>>> index 0d61f2b3e213..154585ddae08 100644
> >>>>>>> --- a/drivers/gpu/drm/drm_gem_shmem_helper.c
> >>>>>>> +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
> >>>>>>> @@ -43,8 +43,8 @@ static const struct drm_gem_object_funcs drm_gem_shmem_funcs = {
> >>>>>>>  	.pin = drm_gem_shmem_object_pin,
> >>>>>>>  	.unpin = drm_gem_shmem_object_unpin,
> >>>>>>>  	.get_sg_table = drm_gem_shmem_object_get_sg_table,
> >>>>>>> -	.vmap = drm_gem_shmem_object_vmap,
> >>>>>>> -	.vunmap = drm_gem_shmem_object_vunmap,
> >>>>>>> +	.vmap = drm_gem_shmem_object_vmap_locked,
> >>>>>>> +	.vunmap = drm_gem_shmem_object_vunmap_locked,        
> >>>>>>
> >>>>>> While I think we should indeed be consistent with the names, I would
> >>>>>> also expect helpers to get the locking right by default.      
> >>>>>
> >>>>> Wait, actually I think this patch does what you suggest already. The
> >>>>> _locked() prefix tells the caller: "you should take care of the locking,
> >>>>> I expect the lock to be held when this is hook/function is called". So
> >>>>> helpers without the _locked() prefix take care of the locking (which I
> >>>>> guess matches your 'helpers get the locking right' expectation), and
> >>>>> those with the _locked() prefix don't.      
> >>>>
> >>>> What I meant by "getting the locking right" is indeed a bit ambiguous,
> >>>> sorry. What I'm trying to say I guess is that, in this particular case,
> >>>> I don't think you can expect the vmap implementation to be called with
> >>>> or without the locks held. The doc for that function will say that it's
> >>>> either one or the other, but not both.
> >>>>
> >>>> So helpers should follow what is needed to provide a default vmap/vunmap
> >>>> implementation, including what locking is expected from a vmap/vunmap
> >>>> implementation.    
> >>>
> >>> Hm, yeah, I think that's a matter of taste. When locking is often
> >>> deferrable, like it is in DRM, I find it beneficial for funcions and
> >>> function pointers to reflect the locking scheme, rather than relying on
> >>> people properly reading the doc, especially when this is the only
> >>> outlier in the group of drm_gem_object_funcs we already have, and it's
> >>> not event documented at the drm_gem_object_funcs level [1] :P.
> >>>     
> >>>>
> >>>> If that means that vmap is always called with the locks taken, then
> >>>> drm_gem_shmem_object_vmap can just assume that it will be called with
> >>>> the locks taken and there's no need to mention it in the name (and you
> >>>> can probably sprinkle a couple of lockdep assertion to make sure the
> >>>> locking is indeed consistent).    
> >>>
> >>> Things get very confusing when you end up having drm_gem_shmem helpers
> >>> that are suffixed with _locked() to encode the fact locking is the
> >>> caller's responsibility and no suffix for the
> >>> callee-takes-care-of-the-locking semantics, while other helpers that are
> >>> not suffixed at all actually implement the
> >>> caller-should-take-care-of-the-locking semantics.
> >>>     
> >>>>    
> >>>>>> I'm not sure how reasonable it is, but I think I'd prefer to turn this
> >>>>>> around and keep the drm_gem_shmem_object_vmap/unmap helpers name, and
> >>>>>> convert whatever function needs to be converted to the unlock suffix so
> >>>>>> we get a consistent naming.      
> >>>>>
> >>>>> That would be an _unlocked() prefix if we do it the other way around. I
> >>>>> think the main confusion comes from the names of the hooks in
> >>>>> drm_gem_shmem_funcs. Some of them, like drm_gem_shmem_funcs::v[un]map()
> >>>>> are called with the GEM resv lock held, and locking is handled by the
> >>>>> core, others, like drm_gem_shmem_funcs::[un]pin() are called
> >>>>> without the GEM resv lock held, and locking is deferred to the
> >>>>> implementation. As I said, I don't mind prefixing hooks/helpers with
> >>>>> _unlocked() for those that take care of the locking, and no prefix for
> >>>>> those that expects locks to be held, as long as it's consistent, but I
> >>>>> just wanted to make sure we're on the same page :-).      
> >>>>
> >>>> What about _nolock then? It's the same number of characters than
> >>>> _locked, plus it expresses what the function is (not) doing, not what
> >>>> context it's supposed to be called in?    
> >>>
> >>> Just did a quick
> >>>
> >>>   git grep _nolock drivers/gpu/drm
> >>>
> >>> and it returns zero result, where the _locked/_unlocked pattern seems
> >>> to already be widely used. Not saying we shouldn't change that, but it
> >>> doesn't feel like a change we should do as part of this series.
> >>>
> >>> Regards,
> >>>
> >>> Boris
> >>>
> >>> [1]https://elixir.bootlin.com/linux/v6.7-rc3/source/include/drm/drm_gem.h#L155    
> >>
> >> I'm fine with dropping the _locked() postfix from the common GEM helpers
> >> and documenting the locking rule in drm_gem. Thank you all for the
> >> suggestions :)  
> > 
> > Sorry to disagree, but I think a proper function name/suffix is
> > sometimes worth a few lines of doc. Not saying we should do one or the
> > other, I think we should do both. But when I see a function suffixed
> > _locked, _unlocked or _nolock, I can immediately tell if this function
> > defers the locking to the caller or not, and then go check which lock
> > in the function doc.
> > 
> > And the second thing I'm not happy with, is the fact we go back to an
> > inconsistent naming in drm_gem_shmem_helper.c, where some functions
> > deferring the locking to the caller are suffixed _locked and others are
> > not, because ultimately, you need a different name when you expose the
> > two variants...  
> 
> By the `common GEM helpers` I meant the .vmap drm-shmem common helpers
> used for drm_gem_object_funcs, like was suggested by Maxime. The rest of
> functions will retain the _locked part. Sorry for the confusion :)

Well, even if it's just
s/drm_gem_shmem_v[un]map_locked/drm_gem_shmem_v[un]map/, it's still
inconsistent with the rest of the helpers we have there (_locked suffix
for those deferring the locking to the caller, and no suffix when the
lock is taken by the helper). To be clear, I won't block the patch
because of that, but I still think this is the wrong move...

Maxime Ripard Nov. 29, 2023, 1:09 p.m. UTC | #10

On Wed, Nov 29, 2023 at 08:53:30AM +0100, Boris Brezillon wrote:
> On Wed, 29 Nov 2023 01:05:14 +0300
> Dmitry Osipenko <[e-mail]> wrote:
> 
> > On 11/28/23 15:37, Boris Brezillon wrote:
> > > On Tue, 28 Nov 2023 12:14:42 +0100
> > > Maxime Ripard <[e-mail]> wrote:
> > >   
> > >> Hi,
> > >>
> > >> On Fri, Nov 24, 2023 at 11:59:11AM +0100, Boris Brezillon wrote:  
> > >>> On Fri, 24 Nov 2023 11:40:06 +0100
> > >>> Maxime Ripard <[e-mail]> wrote:
> > >>>     
> > >>>> On Mon, Oct 30, 2023 at 02:01:43AM +0300, Dmitry Osipenko wrote:    
> > >>>>> Add locked and remove unlocked postfixes from drm-shmem function names,
> > >>>>> making names consistent with the drm/gem core code.
> > >>>>>
> > >>>>> Reviewed-by: Boris Brezillon <[e-mail]>
> > >>>>> Suggested-by: Boris Brezillon <[e-mail]>
> > >>>>> Signed-off-by: Dmitry Osipenko <[e-mail]>      
> > >>>>
> > >>>> This contradicts my earlier ack on a patch but...
> > >>>>     
> > >>>>> ---
> > >>>>>  drivers/gpu/drm/drm_gem_shmem_helper.c        | 64 +++++++++----------
> > >>>>>  drivers/gpu/drm/lima/lima_gem.c               |  8 +--
> > >>>>>  drivers/gpu/drm/panfrost/panfrost_drv.c       |  2 +-
> > >>>>>  drivers/gpu/drm/panfrost/panfrost_gem.c       |  6 +-
> > >>>>>  .../gpu/drm/panfrost/panfrost_gem_shrinker.c  |  2 +-
> > >>>>>  drivers/gpu/drm/panfrost/panfrost_mmu.c       |  2 +-
> > >>>>>  drivers/gpu/drm/v3d/v3d_bo.c                  |  4 +-
> > >>>>>  drivers/gpu/drm/virtio/virtgpu_object.c       |  4 +-
> > >>>>>  include/drm/drm_gem_shmem_helper.h            | 36 +++++------
> > >>>>>  9 files changed, 64 insertions(+), 64 deletions(-)
> > >>>>>
> > >>>>> diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
> > >>>>> index 0d61f2b3e213..154585ddae08 100644
> > >>>>> --- a/drivers/gpu/drm/drm_gem_shmem_helper.c
> > >>>>> +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
> > >>>>> @@ -43,8 +43,8 @@ static const struct drm_gem_object_funcs drm_gem_shmem_funcs = {
> > >>>>>  	.pin = drm_gem_shmem_object_pin,
> > >>>>>  	.unpin = drm_gem_shmem_object_unpin,
> > >>>>>  	.get_sg_table = drm_gem_shmem_object_get_sg_table,
> > >>>>> -	.vmap = drm_gem_shmem_object_vmap,
> > >>>>> -	.vunmap = drm_gem_shmem_object_vunmap,
> > >>>>> +	.vmap = drm_gem_shmem_object_vmap_locked,
> > >>>>> +	.vunmap = drm_gem_shmem_object_vunmap_locked,      
> > >>>>
> > >>>> While I think we should indeed be consistent with the names, I would
> > >>>> also expect helpers to get the locking right by default.    
> > >>>
> > >>> Wait, actually I think this patch does what you suggest already. The
> > >>> _locked() prefix tells the caller: "you should take care of the locking,
> > >>> I expect the lock to be held when this is hook/function is called". So
> > >>> helpers without the _locked() prefix take care of the locking (which I
> > >>> guess matches your 'helpers get the locking right' expectation), and
> > >>> those with the _locked() prefix don't.    
> > >>
> > >> What I meant by "getting the locking right" is indeed a bit ambiguous,
> > >> sorry. What I'm trying to say I guess is that, in this particular case,
> > >> I don't think you can expect the vmap implementation to be called with
> > >> or without the locks held. The doc for that function will say that it's
> > >> either one or the other, but not both.
> > >>
> > >> So helpers should follow what is needed to provide a default vmap/vunmap
> > >> implementation, including what locking is expected from a vmap/vunmap
> > >> implementation.  
> > > 
> > > Hm, yeah, I think that's a matter of taste. When locking is often
> > > deferrable, like it is in DRM, I find it beneficial for funcions and
> > > function pointers to reflect the locking scheme, rather than relying on
> > > people properly reading the doc, especially when this is the only
> > > outlier in the group of drm_gem_object_funcs we already have, and it's
> > > not event documented at the drm_gem_object_funcs level [1] :P.
> > >   
> > >>
> > >> If that means that vmap is always called with the locks taken, then
> > >> drm_gem_shmem_object_vmap can just assume that it will be called with
> > >> the locks taken and there's no need to mention it in the name (and you
> > >> can probably sprinkle a couple of lockdep assertion to make sure the
> > >> locking is indeed consistent).  
> > > 
> > > Things get very confusing when you end up having drm_gem_shmem helpers
> > > that are suffixed with _locked() to encode the fact locking is the
> > > caller's responsibility and no suffix for the
> > > callee-takes-care-of-the-locking semantics, while other helpers that are
> > > not suffixed at all actually implement the
> > > caller-should-take-care-of-the-locking semantics.
> > >   
> > >>  
> > >>>> I'm not sure how reasonable it is, but I think I'd prefer to turn this
> > >>>> around and keep the drm_gem_shmem_object_vmap/unmap helpers name, and
> > >>>> convert whatever function needs to be converted to the unlock suffix so
> > >>>> we get a consistent naming.    
> > >>>
> > >>> That would be an _unlocked() prefix if we do it the other way around. I
> > >>> think the main confusion comes from the names of the hooks in
> > >>> drm_gem_shmem_funcs. Some of them, like drm_gem_shmem_funcs::v[un]map()
> > >>> are called with the GEM resv lock held, and locking is handled by the
> > >>> core, others, like drm_gem_shmem_funcs::[un]pin() are called
> > >>> without the GEM resv lock held, and locking is deferred to the
> > >>> implementation. As I said, I don't mind prefixing hooks/helpers with
> > >>> _unlocked() for those that take care of the locking, and no prefix for
> > >>> those that expects locks to be held, as long as it's consistent, but I
> > >>> just wanted to make sure we're on the same page :-).    
> > >>
> > >> What about _nolock then? It's the same number of characters than
> > >> _locked, plus it expresses what the function is (not) doing, not what
> > >> context it's supposed to be called in?  
> > > 
> > > Just did a quick
> > > 
> > >   git grep _nolock drivers/gpu/drm
> > > 
> > > and it returns zero result, where the _locked/_unlocked pattern seems
> > > to already be widely used. Not saying we shouldn't change that, but it
> > > doesn't feel like a change we should do as part of this series.
> > > 
> > > Regards,
> > > 
> > > Boris
> > > 
> > > [1]https://elixir.bootlin.com/linux/v6.7-rc3/source/include/drm/drm_gem.h#L155  
> > 
> > I'm fine with dropping the _locked() postfix from the common GEM helpers
> > and documenting the locking rule in drm_gem. Thank you all for the
> > suggestions :)
> 
> Sorry to disagree, but I think a proper function name/suffix is
> sometimes worth a few lines of doc. Not saying we should do one or the
> other, I think we should do both. But when I see a function suffixed
> _locked, _unlocked or _nolock, I can immediately tell if this function
> defers the locking to the caller or not, and then go check which lock
> in the function doc.
> 
> And the second thing I'm not happy with, is the fact we go back to an
> inconsistent naming in drm_gem_shmem_helper.c, where some functions
> deferring the locking to the caller are suffixed _locked and others are
> not, because ultimately, you need a different name when you expose the
> two variants...

I guess one of the point I was trying to make was also: why do you need
both?

If one is better than the other (whatever better means here), then all
drivers should use it.

The counterpart being that if provided a choice, you can be sure that a
lot of people will get it wrong. The one example I have in mind for
example was the drm_atomic_helper_commit_tail vs
drm_atomic_helper_commit_tail_rpm. The latter is now widely used, and
most of it is cargo-cult.

I think you were referring to the locks being deferred vs taken right
now before, why do we need to have the choice between the two?

Maxime

Boris Brezillon Nov. 29, 2023, 1:46 p.m. UTC | #11

On Wed, 29 Nov 2023 14:09:47 +0100
Maxime Ripard <[e-mail]> wrote:

> On Wed, Nov 29, 2023 at 08:53:30AM +0100, Boris Brezillon wrote:
> > On Wed, 29 Nov 2023 01:05:14 +0300
> > Dmitry Osipenko <[e-mail]> wrote:
> >   
> > > On 11/28/23 15:37, Boris Brezillon wrote:  
> > > > On Tue, 28 Nov 2023 12:14:42 +0100
> > > > Maxime Ripard <[e-mail]> wrote:
> > > >     
> > > >> Hi,
> > > >>
> > > >> On Fri, Nov 24, 2023 at 11:59:11AM +0100, Boris Brezillon wrote:    
> > > >>> On Fri, 24 Nov 2023 11:40:06 +0100
> > > >>> Maxime Ripard <[e-mail]> wrote:
> > > >>>       
> > > >>>> On Mon, Oct 30, 2023 at 02:01:43AM +0300, Dmitry Osipenko wrote:      
> > > >>>>> Add locked and remove unlocked postfixes from drm-shmem function names,
> > > >>>>> making names consistent with the drm/gem core code.
> > > >>>>>
> > > >>>>> Reviewed-by: Boris Brezillon <[e-mail]>
> > > >>>>> Suggested-by: Boris Brezillon <[e-mail]>
> > > >>>>> Signed-off-by: Dmitry Osipenko <[e-mail]>        
> > > >>>>
> > > >>>> This contradicts my earlier ack on a patch but...
> > > >>>>       
> > > >>>>> ---
> > > >>>>>  drivers/gpu/drm/drm_gem_shmem_helper.c        | 64 +++++++++----------
> > > >>>>>  drivers/gpu/drm/lima/lima_gem.c               |  8 +--
> > > >>>>>  drivers/gpu/drm/panfrost/panfrost_drv.c       |  2 +-
> > > >>>>>  drivers/gpu/drm/panfrost/panfrost_gem.c       |  6 +-
> > > >>>>>  .../gpu/drm/panfrost/panfrost_gem_shrinker.c  |  2 +-
> > > >>>>>  drivers/gpu/drm/panfrost/panfrost_mmu.c       |  2 +-
> > > >>>>>  drivers/gpu/drm/v3d/v3d_bo.c                  |  4 +-
> > > >>>>>  drivers/gpu/drm/virtio/virtgpu_object.c       |  4 +-
> > > >>>>>  include/drm/drm_gem_shmem_helper.h            | 36 +++++------
> > > >>>>>  9 files changed, 64 insertions(+), 64 deletions(-)
> > > >>>>>
> > > >>>>> diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
> > > >>>>> index 0d61f2b3e213..154585ddae08 100644
> > > >>>>> --- a/drivers/gpu/drm/drm_gem_shmem_helper.c
> > > >>>>> +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
> > > >>>>> @@ -43,8 +43,8 @@ static const struct drm_gem_object_funcs drm_gem_shmem_funcs = {
> > > >>>>>  	.pin = drm_gem_shmem_object_pin,
> > > >>>>>  	.unpin = drm_gem_shmem_object_unpin,
> > > >>>>>  	.get_sg_table = drm_gem_shmem_object_get_sg_table,
> > > >>>>> -	.vmap = drm_gem_shmem_object_vmap,
> > > >>>>> -	.vunmap = drm_gem_shmem_object_vunmap,
> > > >>>>> +	.vmap = drm_gem_shmem_object_vmap_locked,
> > > >>>>> +	.vunmap = drm_gem_shmem_object_vunmap_locked,        
> > > >>>>
> > > >>>> While I think we should indeed be consistent with the names, I would
> > > >>>> also expect helpers to get the locking right by default.      
> > > >>>
> > > >>> Wait, actually I think this patch does what you suggest already. The
> > > >>> _locked() prefix tells the caller: "you should take care of the locking,
> > > >>> I expect the lock to be held when this is hook/function is called". So
> > > >>> helpers without the _locked() prefix take care of the locking (which I
> > > >>> guess matches your 'helpers get the locking right' expectation), and
> > > >>> those with the _locked() prefix don't.      
> > > >>
> > > >> What I meant by "getting the locking right" is indeed a bit ambiguous,
> > > >> sorry. What I'm trying to say I guess is that, in this particular case,
> > > >> I don't think you can expect the vmap implementation to be called with
> > > >> or without the locks held. The doc for that function will say that it's
> > > >> either one or the other, but not both.
> > > >>
> > > >> So helpers should follow what is needed to provide a default vmap/vunmap
> > > >> implementation, including what locking is expected from a vmap/vunmap
> > > >> implementation.    
> > > > 
> > > > Hm, yeah, I think that's a matter of taste. When locking is often
> > > > deferrable, like it is in DRM, I find it beneficial for funcions and
> > > > function pointers to reflect the locking scheme, rather than relying on
> > > > people properly reading the doc, especially when this is the only
> > > > outlier in the group of drm_gem_object_funcs we already have, and it's
> > > > not event documented at the drm_gem_object_funcs level [1] :P.
> > > >     
> > > >>
> > > >> If that means that vmap is always called with the locks taken, then
> > > >> drm_gem_shmem_object_vmap can just assume that it will be called with
> > > >> the locks taken and there's no need to mention it in the name (and you
> > > >> can probably sprinkle a couple of lockdep assertion to make sure the
> > > >> locking is indeed consistent).    
> > > > 
> > > > Things get very confusing when you end up having drm_gem_shmem helpers
> > > > that are suffixed with _locked() to encode the fact locking is the
> > > > caller's responsibility and no suffix for the
> > > > callee-takes-care-of-the-locking semantics, while other helpers that are
> > > > not suffixed at all actually implement the
> > > > caller-should-take-care-of-the-locking semantics.
> > > >     
> > > >>    
> > > >>>> I'm not sure how reasonable it is, but I think I'd prefer to turn this
> > > >>>> around and keep the drm_gem_shmem_object_vmap/unmap helpers name, and
> > > >>>> convert whatever function needs to be converted to the unlock suffix so
> > > >>>> we get a consistent naming.      
> > > >>>
> > > >>> That would be an _unlocked() prefix if we do it the other way around. I
> > > >>> think the main confusion comes from the names of the hooks in
> > > >>> drm_gem_shmem_funcs. Some of them, like drm_gem_shmem_funcs::v[un]map()
> > > >>> are called with the GEM resv lock held, and locking is handled by the
> > > >>> core, others, like drm_gem_shmem_funcs::[un]pin() are called
> > > >>> without the GEM resv lock held, and locking is deferred to the
> > > >>> implementation. As I said, I don't mind prefixing hooks/helpers with
> > > >>> _unlocked() for those that take care of the locking, and no prefix for
> > > >>> those that expects locks to be held, as long as it's consistent, but I
> > > >>> just wanted to make sure we're on the same page :-).      
> > > >>
> > > >> What about _nolock then? It's the same number of characters than
> > > >> _locked, plus it expresses what the function is (not) doing, not what
> > > >> context it's supposed to be called in?    
> > > > 
> > > > Just did a quick
> > > > 
> > > >   git grep _nolock drivers/gpu/drm
> > > > 
> > > > and it returns zero result, where the _locked/_unlocked pattern seems
> > > > to already be widely used. Not saying we shouldn't change that, but it
> > > > doesn't feel like a change we should do as part of this series.
> > > > 
> > > > Regards,
> > > > 
> > > > Boris
> > > > 
> > > > [1]https://elixir.bootlin.com/linux/v6.7-rc3/source/include/drm/drm_gem.h#L155    
> > > 
> > > I'm fine with dropping the _locked() postfix from the common GEM helpers
> > > and documenting the locking rule in drm_gem. Thank you all for the
> > > suggestions :)  
> > 
> > Sorry to disagree, but I think a proper function name/suffix is
> > sometimes worth a few lines of doc. Not saying we should do one or the
> > other, I think we should do both. But when I see a function suffixed
> > _locked, _unlocked or _nolock, I can immediately tell if this function
> > defers the locking to the caller or not, and then go check which lock
> > in the function doc.
> > 
> > And the second thing I'm not happy with, is the fact we go back to an
> > inconsistent naming in drm_gem_shmem_helper.c, where some functions
> > deferring the locking to the caller are suffixed _locked and others are
> > not, because ultimately, you need a different name when you expose the
> > two variants...  
> 
> I guess one of the point I was trying to make was also: why do you need
> both?
> 
> If one is better than the other (whatever better means here), then all
> drivers should use it.
> 
> The counterpart being that if provided a choice, you can be sure that a
> lot of people will get it wrong. The one example I have in mind for
> example was the drm_atomic_helper_commit_tail vs
> drm_atomic_helper_commit_tail_rpm. The latter is now widely used, and
> most of it is cargo-cult.
> 
> I think you were referring to the locks being deferred vs taken right
> now before, why do we need to have the choice between the two?

Because DRM locking is complex, and you sometimes have to call some
helpers in a context where you already hold the GEM dma_resv lock.
That's not the case for _v[un]map(), because the core always takes the
lock for us if we call drm_gem_vmap_unlocked(). 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. Not saying the
confusion only comes from the naming, because the various layers of
indirection we have clearly don't help, but having a name reflecting
the fact the locking is deferred to the caller would have helped, I
think.

Maxime Ripard Nov. 29, 2023, 3:15 p.m. UTC | #12

On Wed, Nov 29, 2023 at 02:46:09PM +0100, Boris Brezillon wrote:
> On Wed, 29 Nov 2023 14:09:47 +0100
> Maxime Ripard <[e-mail]> wrote:
> 
> > On Wed, Nov 29, 2023 at 08:53:30AM +0100, Boris Brezillon wrote:
> > > On Wed, 29 Nov 2023 01:05:14 +0300
> > > Dmitry Osipenko <[e-mail]> wrote:
> > >   
> > > > On 11/28/23 15:37, Boris Brezillon wrote:  
> > > > > On Tue, 28 Nov 2023 12:14:42 +0100
> > > > > Maxime Ripard <[e-mail]> wrote:
> > > > >     
> > > > >> Hi,
> > > > >>
> > > > >> On Fri, Nov 24, 2023 at 11:59:11AM +0100, Boris Brezillon wrote:    
> > > > >>> On Fri, 24 Nov 2023 11:40:06 +0100
> > > > >>> Maxime Ripard <[e-mail]> wrote:
> > > > >>>       
> > > > >>>> On Mon, Oct 30, 2023 at 02:01:43AM +0300, Dmitry Osipenko wrote:      
> > > > >>>>> Add locked and remove unlocked postfixes from drm-shmem function names,
> > > > >>>>> making names consistent with the drm/gem core code.
> > > > >>>>>
> > > > >>>>> Reviewed-by: Boris Brezillon <[e-mail]>
> > > > >>>>> Suggested-by: Boris Brezillon <[e-mail]>
> > > > >>>>> Signed-off-by: Dmitry Osipenko <[e-mail]>        
> > > > >>>>
> > > > >>>> This contradicts my earlier ack on a patch but...
> > > > >>>>       
> > > > >>>>> ---
> > > > >>>>>  drivers/gpu/drm/drm_gem_shmem_helper.c        | 64 +++++++++----------
> > > > >>>>>  drivers/gpu/drm/lima/lima_gem.c               |  8 +--
> > > > >>>>>  drivers/gpu/drm/panfrost/panfrost_drv.c       |  2 +-
> > > > >>>>>  drivers/gpu/drm/panfrost/panfrost_gem.c       |  6 +-
> > > > >>>>>  .../gpu/drm/panfrost/panfrost_gem_shrinker.c  |  2 +-
> > > > >>>>>  drivers/gpu/drm/panfrost/panfrost_mmu.c       |  2 +-
> > > > >>>>>  drivers/gpu/drm/v3d/v3d_bo.c                  |  4 +-
> > > > >>>>>  drivers/gpu/drm/virtio/virtgpu_object.c       |  4 +-
> > > > >>>>>  include/drm/drm_gem_shmem_helper.h            | 36 +++++------
> > > > >>>>>  9 files changed, 64 insertions(+), 64 deletions(-)
> > > > >>>>>
> > > > >>>>> diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
> > > > >>>>> index 0d61f2b3e213..154585ddae08 100644
> > > > >>>>> --- a/drivers/gpu/drm/drm_gem_shmem_helper.c
> > > > >>>>> +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
> > > > >>>>> @@ -43,8 +43,8 @@ static const struct drm_gem_object_funcs drm_gem_shmem_funcs = {
> > > > >>>>>  	.pin = drm_gem_shmem_object_pin,
> > > > >>>>>  	.unpin = drm_gem_shmem_object_unpin,
> > > > >>>>>  	.get_sg_table = drm_gem_shmem_object_get_sg_table,
> > > > >>>>> -	.vmap = drm_gem_shmem_object_vmap,
> > > > >>>>> -	.vunmap = drm_gem_shmem_object_vunmap,
> > > > >>>>> +	.vmap = drm_gem_shmem_object_vmap_locked,
> > > > >>>>> +	.vunmap = drm_gem_shmem_object_vunmap_locked,        
> > > > >>>>
> > > > >>>> While I think we should indeed be consistent with the names, I would
> > > > >>>> also expect helpers to get the locking right by default.      
> > > > >>>
> > > > >>> Wait, actually I think this patch does what you suggest already. The
> > > > >>> _locked() prefix tells the caller: "you should take care of the locking,
> > > > >>> I expect the lock to be held when this is hook/function is called". So
> > > > >>> helpers without the _locked() prefix take care of the locking (which I
> > > > >>> guess matches your 'helpers get the locking right' expectation), and
> > > > >>> those with the _locked() prefix don't.      
> > > > >>
> > > > >> What I meant by "getting the locking right" is indeed a bit ambiguous,
> > > > >> sorry. What I'm trying to say I guess is that, in this particular case,
> > > > >> I don't think you can expect the vmap implementation to be called with
> > > > >> or without the locks held. The doc for that function will say that it's
> > > > >> either one or the other, but not both.
> > > > >>
> > > > >> So helpers should follow what is needed to provide a default vmap/vunmap
> > > > >> implementation, including what locking is expected from a vmap/vunmap
> > > > >> implementation.    
> > > > > 
> > > > > Hm, yeah, I think that's a matter of taste. When locking is often
> > > > > deferrable, like it is in DRM, I find it beneficial for funcions and
> > > > > function pointers to reflect the locking scheme, rather than relying on
> > > > > people properly reading the doc, especially when this is the only
> > > > > outlier in the group of drm_gem_object_funcs we already have, and it's
> > > > > not event documented at the drm_gem_object_funcs level [1] :P.
> > > > >     
> > > > >>
> > > > >> If that means that vmap is always called with the locks taken, then
> > > > >> drm_gem_shmem_object_vmap can just assume that it will be called with
> > > > >> the locks taken and there's no need to mention it in the name (and you
> > > > >> can probably sprinkle a couple of lockdep assertion to make sure the
> > > > >> locking is indeed consistent).    
> > > > > 
> > > > > Things get very confusing when you end up having drm_gem_shmem helpers
> > > > > that are suffixed with _locked() to encode the fact locking is the
> > > > > caller's responsibility and no suffix for the
> > > > > callee-takes-care-of-the-locking semantics, while other helpers that are
> > > > > not suffixed at all actually implement the
> > > > > caller-should-take-care-of-the-locking semantics.
> > > > >     
> > > > >>    
> > > > >>>> I'm not sure how reasonable it is, but I think I'd prefer to turn this
> > > > >>>> around and keep the drm_gem_shmem_object_vmap/unmap helpers name, and
> > > > >>>> convert whatever function needs to be converted to the unlock suffix so
> > > > >>>> we get a consistent naming.      
> > > > >>>
> > > > >>> That would be an _unlocked() prefix if we do it the other way around. I
> > > > >>> think the main confusion comes from the names of the hooks in
> > > > >>> drm_gem_shmem_funcs. Some of them, like drm_gem_shmem_funcs::v[un]map()
> > > > >>> are called with the GEM resv lock held, and locking is handled by the
> > > > >>> core, others, like drm_gem_shmem_funcs::[un]pin() are called
> > > > >>> without the GEM resv lock held, and locking is deferred to the
> > > > >>> implementation. As I said, I don't mind prefixing hooks/helpers with
> > > > >>> _unlocked() for those that take care of the locking, and no prefix for
> > > > >>> those that expects locks to be held, as long as it's consistent, but I
> > > > >>> just wanted to make sure we're on the same page :-).      
> > > > >>
> > > > >> What about _nolock then? It's the same number of characters than
> > > > >> _locked, plus it expresses what the function is (not) doing, not what
> > > > >> context it's supposed to be called in?    
> > > > > 
> > > > > Just did a quick
> > > > > 
> > > > >   git grep _nolock drivers/gpu/drm
> > > > > 
> > > > > and it returns zero result, where the _locked/_unlocked pattern seems
> > > > > to already be widely used. Not saying we shouldn't change that, but it
> > > > > doesn't feel like a change we should do as part of this series.
> > > > > 
> > > > > Regards,
> > > > > 
> > > > > Boris
> > > > > 
> > > > > [1]https://elixir.bootlin.com/linux/v6.7-rc3/source/include/drm/drm_gem.h#L155    
> > > > 
> > > > I'm fine with dropping the _locked() postfix from the common GEM helpers
> > > > and documenting the locking rule in drm_gem. Thank you all for the
> > > > suggestions :)  
> > > 
> > > Sorry to disagree, but I think a proper function name/suffix is
> > > sometimes worth a few lines of doc. Not saying we should do one or the
> > > other, I think we should do both. But when I see a function suffixed
> > > _locked, _unlocked or _nolock, I can immediately tell if this function
> > > defers the locking to the caller or not, and then go check which lock
> > > in the function doc.
> > > 
> > > And the second thing I'm not happy with, is the fact we go back to an
> > > inconsistent naming in drm_gem_shmem_helper.c, where some functions
> > > deferring the locking to the caller are suffixed _locked and others are
> > > not, because ultimately, you need a different name when you expose the
> > > two variants...  
> > 
> > I guess one of the point I was trying to make was also: why do you need
> > both?
> > 
> > If one is better than the other (whatever better means here), then all
> > drivers should use it.
> > 
> > The counterpart being that if provided a choice, you can be sure that a
> > lot of people will get it wrong. The one example I have in mind for
> > example was the drm_atomic_helper_commit_tail vs
> > drm_atomic_helper_commit_tail_rpm. The latter is now widely used, and
> > most of it is cargo-cult.
> > 
> > I think you were referring to the locks being deferred vs taken right
> > now before, why do we need to have the choice between the two?
>
> Because DRM locking is complex, and you sometimes have to call some
> helpers in a context where you already hold the GEM dma_resv lock.
> That's not the case for _v[un]map(), because the core always takes the
> lock for us if we call drm_gem_vmap_unlocked().

Ok

> 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.

 - Some other code path can still call those helpers in drivers, and the
   locking isn't handled by the core anymore.

 - We now have _vmap/vunmap_unlocked functions to take those locks for
   those code paths

 - And the variant names are now confusing, making people use the
   lockless version in situations where they should have use the locked
   one.

Is that a correct summary?

If so, then I agree that we need to change the name.

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.

So that means than drm_gem_shmem_vmap/vunmap() should take the lock
itself, and drm_gem_shmem_vmap/vunmap_nolock/unlocked never does.

I think I'd prefer the nolock variant over unlocked still.

And I also think we can improve the documentation and add lockdep calls
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?

Maxime

Boris Brezillon Nov. 29, 2023, 3:47 p.m. UTC | #13

On Wed, 29 Nov 2023 16:15:27 +0100
Maxime Ripard <[e-mail]> 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

Maxime Ripard Dec. 4, 2023, 12:55 p.m. UTC | #14

On Wed, Nov 29, 2023 at 04:47:05PM +0100, Boris Brezillon wrote:
> On Wed, 29 Nov 2023 16:15:27 +0100
> Maxime Ripard <[e-mail]> 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

Dmitry Osipenko Dec. 5, 2023, 11:43 a.m. UTC | #15

On 12/4/23 15:55, Maxime Ripard wrote:
>> Okay, that means s/_locked/_nolock/ in drm_gem_shmem_helpers.{c,h}, I
>> guess.

DRM subsys and majority of kernel uses common _locked postfix. We should
retain the old naming scheme by using _locked() in DRM. It's not
worthwhile changing the name to a much less popular variant for a no
good reason.

Maxime, are you okay with keeping the _locked name?

Maxime Ripard Dec. 14, 2023, 6:16 p.m. UTC | #16

On Tue, Dec 05, 2023 at 02:43:16PM +0300, Dmitry Osipenko wrote:
> On 12/4/23 15:55, Maxime Ripard wrote:
> >> Okay, that means s/_locked/_nolock/ in drm_gem_shmem_helpers.{c,h}, I
> >> guess.
> 
> DRM subsys and majority of kernel uses common _locked postfix. We should
> retain the old naming scheme by using _locked() in DRM. It's not
> worthwhile changing the name to a much less popular variant for a no
> good reason.
> 
> Maxime, are you okay with keeping the _locked name?

Yeah... I still don't really like it, but you're right that it's best to
remain consistent over my opinion :)

Maxime

Dmitry Osipenko Dec. 15, 2023, 12:42 a.m. UTC | #17

On 12/14/23 21:16, Maxime Ripard wrote:
> On Tue, Dec 05, 2023 at 02:43:16PM +0300, Dmitry Osipenko wrote:
>> On 12/4/23 15:55, Maxime Ripard wrote:
>>>> Okay, that means s/_locked/_nolock/ in drm_gem_shmem_helpers.{c,h}, I
>>>> guess.
>>
>> DRM subsys and majority of kernel uses common _locked postfix. We should
>> retain the old naming scheme by using _locked() in DRM. It's not
>> worthwhile changing the name to a much less popular variant for a no
>> good reason.
>>
>> Maxime, are you okay with keeping the _locked name?
> 
> Yeah... I still don't really like it, but you're right that it's best to
> remain consistent over my opinion :)
Thanks for the review!

Best regards,
Dmitry

Patch

diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
index 0d61f2b3e213..154585ddae08 100644
--- a/drivers/gpu/drm/drm_gem_shmem_helper.c
+++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
@@ -43,8 +43,8 @@  static const struct drm_gem_object_funcs drm_gem_shmem_funcs = {
 	.pin = drm_gem_shmem_object_pin,
 	.unpin = drm_gem_shmem_object_unpin,
 	.get_sg_table = drm_gem_shmem_object_get_sg_table,
-	.vmap = drm_gem_shmem_object_vmap,
-	.vunmap = drm_gem_shmem_object_vunmap,
+	.vmap = drm_gem_shmem_object_vmap_locked,
+	.vunmap = drm_gem_shmem_object_vunmap_locked,
 	.mmap = drm_gem_shmem_object_mmap,
 	.vm_ops = &drm_gem_shmem_vm_ops,
 };
@@ -153,7 +153,7 @@  void drm_gem_shmem_free(struct drm_gem_shmem_object *shmem)
 			kfree(shmem->sgt);
 		}
 		if (shmem->pages)
-			drm_gem_shmem_put_pages(shmem);
+			drm_gem_shmem_put_pages_locked(shmem);
 
 		drm_WARN_ON(obj->dev, shmem->pages_use_count);
 
@@ -165,7 +165,7 @@  void drm_gem_shmem_free(struct drm_gem_shmem_object *shmem)
 }
 EXPORT_SYMBOL_GPL(drm_gem_shmem_free);
 
-static int drm_gem_shmem_get_pages(struct drm_gem_shmem_object *shmem)
+static int drm_gem_shmem_get_pages_locked(struct drm_gem_shmem_object *shmem)
 {
 	struct drm_gem_object *obj = &shmem->base;
 	struct page **pages;
@@ -199,12 +199,12 @@  static int drm_gem_shmem_get_pages(struct drm_gem_shmem_object *shmem)
 }
 
 /*
- * drm_gem_shmem_put_pages - Decrease use count on the backing pages for a shmem GEM object
+ * drm_gem_shmem_put_pages_locked - Decrease use count on the backing pages for a shmem GEM object
  * @shmem: shmem GEM object
  *
  * This function decreases the use count and puts the backing pages when use drops to zero.
  */
-void drm_gem_shmem_put_pages(struct drm_gem_shmem_object *shmem)
+void drm_gem_shmem_put_pages_locked(struct drm_gem_shmem_object *shmem)
 {
 	struct drm_gem_object *obj = &shmem->base;
 
@@ -226,7 +226,7 @@  void drm_gem_shmem_put_pages(struct drm_gem_shmem_object *shmem)
 			  shmem->pages_mark_accessed_on_put);
 	shmem->pages = NULL;
 }
-EXPORT_SYMBOL_GPL(drm_gem_shmem_put_pages);
+EXPORT_SYMBOL_GPL(drm_gem_shmem_put_pages_locked);
 
 static int drm_gem_shmem_pin_locked(struct drm_gem_shmem_object *shmem)
 {
@@ -234,7 +234,7 @@  static int drm_gem_shmem_pin_locked(struct drm_gem_shmem_object *shmem)
 
 	dma_resv_assert_held(shmem->base.resv);
 
-	ret = drm_gem_shmem_get_pages(shmem);
+	ret = drm_gem_shmem_get_pages_locked(shmem);
 
 	return ret;
 }
@@ -243,7 +243,7 @@  static void drm_gem_shmem_unpin_locked(struct drm_gem_shmem_object *shmem)
 {
 	dma_resv_assert_held(shmem->base.resv);
 
-	drm_gem_shmem_put_pages(shmem);
+	drm_gem_shmem_put_pages_locked(shmem);
 }
 
 /**
@@ -293,7 +293,7 @@  void drm_gem_shmem_unpin(struct drm_gem_shmem_object *shmem)
 EXPORT_SYMBOL_GPL(drm_gem_shmem_unpin);
 
 /*
- * drm_gem_shmem_vmap - Create a virtual mapping for a shmem GEM object
+ * drm_gem_shmem_vmap_locked - Create a virtual mapping for a shmem GEM object
  * @shmem: shmem GEM object
  * @map: Returns the kernel virtual address of the SHMEM GEM object's backing
  *       store.
@@ -302,13 +302,13 @@  EXPORT_SYMBOL_GPL(drm_gem_shmem_unpin);
  * exists for the buffer backing the shmem GEM object. It hides the differences
  * between dma-buf imported and natively allocated objects.
  *
- * Acquired mappings should be cleaned up by calling drm_gem_shmem_vunmap().
+ * Acquired mappings should be cleaned up by calling drm_gem_shmem_vunmap_locked().
  *
  * Returns:
  * 0 on success or a negative error code on failure.
  */
-int drm_gem_shmem_vmap(struct drm_gem_shmem_object *shmem,
-		       struct iosys_map *map)
+int drm_gem_shmem_vmap_locked(struct drm_gem_shmem_object *shmem,
+			      struct iosys_map *map)
 {
 	struct drm_gem_object *obj = &shmem->base;
 	int ret = 0;
@@ -331,7 +331,7 @@  int drm_gem_shmem_vmap(struct drm_gem_shmem_object *shmem,
 			return 0;
 		}
 
-		ret = drm_gem_shmem_get_pages(shmem);
+		ret = drm_gem_shmem_get_pages_locked(shmem);
 		if (ret)
 			goto err_zero_use;
 
@@ -354,28 +354,28 @@  int drm_gem_shmem_vmap(struct drm_gem_shmem_object *shmem,
 
 err_put_pages:
 	if (!obj->import_attach)
-		drm_gem_shmem_put_pages(shmem);
+		drm_gem_shmem_put_pages_locked(shmem);
 err_zero_use:
 	shmem->vmap_use_count = 0;
 
 	return ret;
 }
-EXPORT_SYMBOL_GPL(drm_gem_shmem_vmap);
+EXPORT_SYMBOL_GPL(drm_gem_shmem_vmap_locked);
 
 /*
- * drm_gem_shmem_vunmap - Unmap a virtual mapping for a shmem GEM object
+ * drm_gem_shmem_vunmap_locked - Unmap a virtual mapping for a shmem GEM object
  * @shmem: shmem GEM object
  * @map: Kernel virtual address where the SHMEM GEM object was mapped
  *
  * This function cleans up a kernel virtual address mapping acquired by
- * drm_gem_shmem_vmap(). The mapping is only removed when the use count drops to
- * zero.
+ * drm_gem_shmem_vmap_locked(). The mapping is only removed when the use count
+ * drops to zero.
  *
  * This function hides the differences between dma-buf imported and natively
  * allocated objects.
  */
-void drm_gem_shmem_vunmap(struct drm_gem_shmem_object *shmem,
-			  struct iosys_map *map)
+void drm_gem_shmem_vunmap_locked(struct drm_gem_shmem_object *shmem,
+				 struct iosys_map *map)
 {
 	struct drm_gem_object *obj = &shmem->base;
 
@@ -391,12 +391,12 @@  void drm_gem_shmem_vunmap(struct drm_gem_shmem_object *shmem,
 			return;
 
 		vunmap(shmem->vaddr);
-		drm_gem_shmem_put_pages(shmem);
+		drm_gem_shmem_put_pages_locked(shmem);
 	}
 
 	shmem->vaddr = NULL;
 }
-EXPORT_SYMBOL_GPL(drm_gem_shmem_vunmap);
+EXPORT_SYMBOL_GPL(drm_gem_shmem_vunmap_locked);
 
 static int
 drm_gem_shmem_create_with_handle(struct drm_file *file_priv,
@@ -424,7 +424,7 @@  drm_gem_shmem_create_with_handle(struct drm_file *file_priv,
 /* Update madvise status, returns true if not purged, else
  * false or -errno.
  */
-int drm_gem_shmem_madvise(struct drm_gem_shmem_object *shmem, int madv)
+int drm_gem_shmem_madvise_locked(struct drm_gem_shmem_object *shmem, int madv)
 {
 	dma_resv_assert_held(shmem->base.resv);
 
@@ -435,9 +435,9 @@  int drm_gem_shmem_madvise(struct drm_gem_shmem_object *shmem, int madv)
 
 	return (madv >= 0);
 }
-EXPORT_SYMBOL_GPL(drm_gem_shmem_madvise);
+EXPORT_SYMBOL_GPL(drm_gem_shmem_madvise_locked);
 
-void drm_gem_shmem_purge(struct drm_gem_shmem_object *shmem)
+void drm_gem_shmem_purge_locked(struct drm_gem_shmem_object *shmem)
 {
 	struct drm_gem_object *obj = &shmem->base;
 	struct drm_device *dev = obj->dev;
@@ -451,7 +451,7 @@  void drm_gem_shmem_purge(struct drm_gem_shmem_object *shmem)
 	kfree(shmem->sgt);
 	shmem->sgt = NULL;
 
-	drm_gem_shmem_put_pages(shmem);
+	drm_gem_shmem_put_pages_locked(shmem);
 
 	shmem->madv = -1;
 
@@ -467,7 +467,7 @@  void drm_gem_shmem_purge(struct drm_gem_shmem_object *shmem)
 
 	invalidate_mapping_pages(file_inode(obj->filp)->i_mapping, 0, (loff_t)-1);
 }
-EXPORT_SYMBOL_GPL(drm_gem_shmem_purge);
+EXPORT_SYMBOL_GPL(drm_gem_shmem_purge_locked);
 
 /**
  * drm_gem_shmem_dumb_create - Create a dumb shmem buffer object
@@ -564,7 +564,7 @@  static void drm_gem_shmem_vm_close(struct vm_area_struct *vma)
 	struct drm_gem_shmem_object *shmem = to_drm_gem_shmem_obj(obj);
 
 	dma_resv_lock(shmem->base.resv, NULL);
-	drm_gem_shmem_put_pages(shmem);
+	drm_gem_shmem_put_pages_locked(shmem);
 	dma_resv_unlock(shmem->base.resv);
 
 	drm_gem_vm_close(vma);
@@ -611,7 +611,7 @@  int drm_gem_shmem_mmap(struct drm_gem_shmem_object *shmem, struct vm_area_struct
 	}
 
 	dma_resv_lock(shmem->base.resv, NULL);
-	ret = drm_gem_shmem_get_pages(shmem);
+	ret = drm_gem_shmem_get_pages_locked(shmem);
 	dma_resv_unlock(shmem->base.resv);
 
 	if (ret)
@@ -679,7 +679,7 @@  static struct sg_table *drm_gem_shmem_get_pages_sgt_locked(struct drm_gem_shmem_
 
 	drm_WARN_ON(obj->dev, obj->import_attach);
 
-	ret = drm_gem_shmem_get_pages(shmem);
+	ret = drm_gem_shmem_get_pages_locked(shmem);
 	if (ret)
 		return ERR_PTR(ret);
 
@@ -701,7 +701,7 @@  static struct sg_table *drm_gem_shmem_get_pages_sgt_locked(struct drm_gem_shmem_
 	sg_free_table(sgt);
 	kfree(sgt);
 err_put_pages:
-	drm_gem_shmem_put_pages(shmem);
+	drm_gem_shmem_put_pages_locked(shmem);
 	return ERR_PTR(ret);
 }
 
diff --git a/drivers/gpu/drm/lima/lima_gem.c b/drivers/gpu/drm/lima/lima_gem.c
index 4f9736e5f929..62d4a409faa8 100644
--- a/drivers/gpu/drm/lima/lima_gem.c
+++ b/drivers/gpu/drm/lima/lima_gem.c
@@ -180,7 +180,7 @@  static int lima_gem_pin(struct drm_gem_object *obj)
 	if (bo->heap_size)
 		return -EINVAL;
 
-	return drm_gem_shmem_pin(&bo->base);
+	return drm_gem_shmem_object_pin(obj);
 }
 
 static int lima_gem_vmap(struct drm_gem_object *obj, struct iosys_map *map)
@@ -190,7 +190,7 @@  static int lima_gem_vmap(struct drm_gem_object *obj, struct iosys_map *map)
 	if (bo->heap_size)
 		return -EINVAL;
 
-	return drm_gem_shmem_vmap(&bo->base, map);
+	return drm_gem_shmem_object_vmap_locked(obj, map);
 }
 
 static int lima_gem_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma)
@@ -200,7 +200,7 @@  static int lima_gem_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma)
 	if (bo->heap_size)
 		return -EINVAL;
 
-	return drm_gem_shmem_mmap(&bo->base, vma);
+	return drm_gem_shmem_object_mmap(obj, vma);
 }
 
 static const struct drm_gem_object_funcs lima_gem_funcs = {
@@ -212,7 +212,7 @@  static const struct drm_gem_object_funcs lima_gem_funcs = {
 	.unpin = drm_gem_shmem_object_unpin,
 	.get_sg_table = drm_gem_shmem_object_get_sg_table,
 	.vmap = lima_gem_vmap,
-	.vunmap = drm_gem_shmem_object_vunmap,
+	.vunmap = drm_gem_shmem_object_vunmap_locked,
 	.mmap = lima_gem_mmap,
 	.vm_ops = &drm_gem_shmem_vm_ops,
 };
diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
index b834777b409b..7f2aba96d5b9 100644
--- a/drivers/gpu/drm/panfrost/panfrost_drv.c
+++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
@@ -438,7 +438,7 @@  static int panfrost_ioctl_madvise(struct drm_device *dev, void *data,
 		}
 	}
 
-	args->retained = drm_gem_shmem_madvise(&bo->base, args->madv);
+	args->retained = drm_gem_shmem_madvise_locked(&bo->base, args->madv);
 
 	if (args->retained) {
 		if (args->madv == PANFROST_MADV_DONTNEED)
diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.c b/drivers/gpu/drm/panfrost/panfrost_gem.c
index 0cf64456e29a..6b77d8cebcb2 100644
--- a/drivers/gpu/drm/panfrost/panfrost_gem.c
+++ b/drivers/gpu/drm/panfrost/panfrost_gem.c
@@ -192,7 +192,7 @@  static int panfrost_gem_pin(struct drm_gem_object *obj)
 	if (bo->is_heap)
 		return -EINVAL;
 
-	return drm_gem_shmem_pin(&bo->base);
+	return drm_gem_shmem_object_pin(obj);
 }
 
 static enum drm_gem_object_status panfrost_gem_status(struct drm_gem_object *obj)
@@ -231,8 +231,8 @@  static const struct drm_gem_object_funcs panfrost_gem_funcs = {
 	.pin = panfrost_gem_pin,
 	.unpin = drm_gem_shmem_object_unpin,
 	.get_sg_table = drm_gem_shmem_object_get_sg_table,
-	.vmap = drm_gem_shmem_object_vmap,
-	.vunmap = drm_gem_shmem_object_vunmap,
+	.vmap = drm_gem_shmem_object_vmap_locked,
+	.vunmap = drm_gem_shmem_object_vunmap_locked,
 	.mmap = drm_gem_shmem_object_mmap,
 	.status = panfrost_gem_status,
 	.rss = panfrost_gem_rss,
diff --git a/drivers/gpu/drm/panfrost/panfrost_gem_shrinker.c b/drivers/gpu/drm/panfrost/panfrost_gem_shrinker.c
index 6a71a2555f85..72193bd734e1 100644
--- a/drivers/gpu/drm/panfrost/panfrost_gem_shrinker.c
+++ b/drivers/gpu/drm/panfrost/panfrost_gem_shrinker.c
@@ -52,7 +52,7 @@  static bool panfrost_gem_purge(struct drm_gem_object *obj)
 		goto unlock_mappings;
 
 	panfrost_gem_teardown_mappings_locked(bo);
-	drm_gem_shmem_purge(&bo->base);
+	drm_gem_shmem_purge_locked(&bo->base);
 	ret = true;
 
 	dma_resv_unlock(shmem->base.resv);
diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c
index 846dd697c410..9fd4a89c52dd 100644
--- a/drivers/gpu/drm/panfrost/panfrost_mmu.c
+++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c
@@ -536,7 +536,7 @@  static int panfrost_mmu_map_fault_addr(struct panfrost_device *pfdev, int as,
 err_map:
 	sg_free_table(sgt);
 err_pages:
-	drm_gem_shmem_put_pages(&bo->base);
+	drm_gem_shmem_put_pages_locked(&bo->base);
 err_unlock:
 	dma_resv_unlock(obj->resv);
 err_bo:
diff --git a/drivers/gpu/drm/v3d/v3d_bo.c b/drivers/gpu/drm/v3d/v3d_bo.c
index 8b3229a37c6d..42cd874f6810 100644
--- a/drivers/gpu/drm/v3d/v3d_bo.c
+++ b/drivers/gpu/drm/v3d/v3d_bo.c
@@ -56,8 +56,8 @@  static const struct drm_gem_object_funcs v3d_gem_funcs = {
 	.pin = drm_gem_shmem_object_pin,
 	.unpin = drm_gem_shmem_object_unpin,
 	.get_sg_table = drm_gem_shmem_object_get_sg_table,
-	.vmap = drm_gem_shmem_object_vmap,
-	.vunmap = drm_gem_shmem_object_vunmap,
+	.vmap = drm_gem_shmem_object_vmap_locked,
+	.vunmap = drm_gem_shmem_object_vunmap_locked,
 	.mmap = drm_gem_shmem_object_mmap,
 	.vm_ops = &drm_gem_shmem_vm_ops,
 };
diff --git a/drivers/gpu/drm/virtio/virtgpu_object.c b/drivers/gpu/drm/virtio/virtgpu_object.c
index c7e74cf13022..ee5d2a70656b 100644
--- a/drivers/gpu/drm/virtio/virtgpu_object.c
+++ b/drivers/gpu/drm/virtio/virtgpu_object.c
@@ -106,8 +106,8 @@  static const struct drm_gem_object_funcs virtio_gpu_shmem_funcs = {
 	.pin = drm_gem_shmem_object_pin,
 	.unpin = drm_gem_shmem_object_unpin,
 	.get_sg_table = drm_gem_shmem_object_get_sg_table,
-	.vmap = drm_gem_shmem_object_vmap,
-	.vunmap = drm_gem_shmem_object_vunmap,
+	.vmap = drm_gem_shmem_object_vmap_locked,
+	.vunmap = drm_gem_shmem_object_vunmap_locked,
 	.mmap = drm_gem_shmem_object_mmap,
 	.vm_ops = &drm_gem_shmem_vm_ops,
 };
diff --git a/include/drm/drm_gem_shmem_helper.h b/include/drm/drm_gem_shmem_helper.h
index bf0c31aa8fbe..6ee4a4046980 100644
--- a/include/drm/drm_gem_shmem_helper.h
+++ b/include/drm/drm_gem_shmem_helper.h
@@ -99,16 +99,16 @@  struct drm_gem_shmem_object {
 struct drm_gem_shmem_object *drm_gem_shmem_create(struct drm_device *dev, size_t size);
 void drm_gem_shmem_free(struct drm_gem_shmem_object *shmem);
 
-void drm_gem_shmem_put_pages(struct drm_gem_shmem_object *shmem);
+void drm_gem_shmem_put_pages_locked(struct drm_gem_shmem_object *shmem);
 int drm_gem_shmem_pin(struct drm_gem_shmem_object *shmem);
 void drm_gem_shmem_unpin(struct drm_gem_shmem_object *shmem);
-int drm_gem_shmem_vmap(struct drm_gem_shmem_object *shmem,
-		       struct iosys_map *map);
-void drm_gem_shmem_vunmap(struct drm_gem_shmem_object *shmem,
-			  struct iosys_map *map);
+int drm_gem_shmem_vmap_locked(struct drm_gem_shmem_object *shmem,
+			      struct iosys_map *map);
+void drm_gem_shmem_vunmap_locked(struct drm_gem_shmem_object *shmem,
+				 struct iosys_map *map);
 int drm_gem_shmem_mmap(struct drm_gem_shmem_object *shmem, struct vm_area_struct *vma);
 
-int drm_gem_shmem_madvise(struct drm_gem_shmem_object *shmem, int madv);
+int drm_gem_shmem_madvise_locked(struct drm_gem_shmem_object *shmem, int madv);
 
 static inline bool drm_gem_shmem_is_purgeable(struct drm_gem_shmem_object *shmem)
 {
@@ -117,7 +117,7 @@  static inline bool drm_gem_shmem_is_purgeable(struct drm_gem_shmem_object *shmem
 		!shmem->base.dma_buf && !shmem->base.import_attach;
 }
 
-void drm_gem_shmem_purge(struct drm_gem_shmem_object *shmem);
+void drm_gem_shmem_purge_locked(struct drm_gem_shmem_object *shmem);
 
 struct sg_table *drm_gem_shmem_get_sg_table(struct drm_gem_shmem_object *shmem);
 struct sg_table *drm_gem_shmem_get_pages_sgt(struct drm_gem_shmem_object *shmem);
@@ -208,22 +208,22 @@  static inline struct sg_table *drm_gem_shmem_object_get_sg_table(struct drm_gem_
 }
 
 /*
- * drm_gem_shmem_object_vmap - GEM object function for drm_gem_shmem_vmap()
+ * drm_gem_shmem_object_vmap_locked - GEM object function for drm_gem_shmem_vmap_locked()
  * @obj: GEM object
  * @map: Returns the kernel virtual address of the SHMEM GEM object's backing store.
  *
- * This function wraps drm_gem_shmem_vmap(). Drivers that employ the shmem helpers should
- * use it as their &drm_gem_object_funcs.vmap handler.
+ * This function wraps drm_gem_shmem_vmap_locked(). Drivers that employ the shmem
+ * helpers should use it as their &drm_gem_object_funcs.vmap handler.
  *
  * Returns:
  * 0 on success or a negative error code on failure.
  */
-static inline int drm_gem_shmem_object_vmap(struct drm_gem_object *obj,
-					    struct iosys_map *map)
+static inline int drm_gem_shmem_object_vmap_locked(struct drm_gem_object *obj,
+						   struct iosys_map *map)
 {
 	struct drm_gem_shmem_object *shmem = to_drm_gem_shmem_obj(obj);
 
-	return drm_gem_shmem_vmap(shmem, map);
+	return drm_gem_shmem_vmap_locked(shmem, map);
 }
 
 /*
@@ -231,15 +231,15 @@  static inline int drm_gem_shmem_object_vmap(struct drm_gem_object *obj,
  * @obj: GEM object
  * @map: Kernel virtual address where the SHMEM GEM object was mapped
  *
- * This function wraps drm_gem_shmem_vunmap(). Drivers that employ the shmem helpers should
- * use it as their &drm_gem_object_funcs.vunmap handler.
+ * This function wraps drm_gem_shmem_vunmap_locked(). Drivers that employ the shmem
+ * helpers should use it as their &drm_gem_object_funcs.vunmap handler.
  */
-static inline void drm_gem_shmem_object_vunmap(struct drm_gem_object *obj,
-					       struct iosys_map *map)
+static inline void drm_gem_shmem_object_vunmap_locked(struct drm_gem_object *obj,
+						      struct iosys_map *map)
 {
 	struct drm_gem_shmem_object *shmem = to_drm_gem_shmem_obj(obj);
 
-	drm_gem_shmem_vunmap(shmem, map);
+	drm_gem_shmem_vunmap_locked(shmem, map);
 }
 
 /**