lkml.org 
[lkml]   [2023]   [Nov]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v18 04/26] drm/shmem-helper: Refactor locked/unlocked functions
    On Fri, 24 Nov 2023 11:40:06 +0100
    Maxime Ripard <mripard@kernel.org> 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@collabora.com>
    > > Suggested-by: Boris Brezillon <boris.brezillon@collabora.com>
    > > Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
    >
    > 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 :-).

    \
     
     \ /
      Last update: 2023-11-24 12:00    [W:3.823 / U:0.128 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site