On 11/28/23 15:37, Boris Brezillon wrote:
> On Tue, 28 Nov 2023 12:14:42 +0100
> Maxime Ripard <mripard@xxxxxxxxxx> 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 <mripard@xxxxxxxxxx> 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 <boris.brezillon@xxxxxxxxxxxxx>
>>>>> Suggested-by: Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx>
>>>>> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@xxxxxxxxxxxxx>
>>>>
>>>> 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 :)
--
Best regards,
Dmitry
