Re: [PATCH v2] Remove custom dumb_map_offset implementation in msm driver

[Rob Clark]

On Tue, Nov 21, 2023 at 5:14 AM Dmitry Baryshkov
<dmitry.baryshkov@xxxxxxxxxx> wrote:
>
> On Tue, 21 Nov 2023 at 04:26, Rob Clark <robdclark@xxxxxxxxx> wrote:
> >
> > On Wed, Nov 15, 2023 at 11:33 AM Dmitry Baryshkov
> > <dmitry.baryshkov@xxxxxxxxxx> wrote:
> > >
> > > On Wed, 15 Nov 2023 at 20:46, Dipam Turkar <dipamt1729@xxxxxxxxx> wrote:
> > > >
> > > > They are not outdated, my bad. I went through the locks' code and saw that they have been updated. But they are probably not necessary here as most of the drivers do not use any form of locking in their implementations. The generic implementations drm_gem_dumb_map_offset() and drm_gem_ttm_dumb_map_offset() do not have any locking mechanisms either.
> > >
> > > Excuse me, but this doesn't sound right to me. There are different
> > > drivers with different implementations. So either we'd need a good
> > > explanation of why it is not necessary, or this patch is NAKed.
> >
> > Digging a bit thru history, it looks like commit 0de23977cfeb
> > ("drm/gem: convert to new unified vma manager") made external locking
> > unnecessary, since the vma mgr already had it's own internal locking.
>
> So, should we drop our own locking system?

specifically for _just_ vma_offset_manager/vma_node, we could.  But I
think that only amounts to mmap_offset().

BR,
-R

> >
> > BR,
> > -R
> >
> > > >
> > > > Thanks and regards
> > > > Dipam Turkar
> > > >
> > > > On Wed, Nov 15, 2023 at 8:37 PM Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx> wrote:
> > > >>
> > > >> On Wed, 15 Nov 2023 at 16:30, Dipam Turkar <dipamt1729@xxxxxxxxx> wrote:
> > > >> >
> > > >> > Make msm use drm_gem_create_map_offset() instead of its custom
> > > >> > implementation for associating GEM object with a fake offset. Since,
> > > >> > we already have this generic implementation, we don't need the custom
> > > >> > implementation and it is better to standardize the code for GEM based
> > > >> > drivers. This also removes the outdated locking leftovers.
> > > >>
> > > >> Why are they outdated?
> > > >>
> > > >> >
> > > >> > Signed-off-by: Dipam Turkar <dipamt1729@xxxxxxxxx>
> > > >> > ---
> > > >> >  drivers/gpu/drm/msm/msm_drv.c |  2 +-
> > > >> >  drivers/gpu/drm/msm/msm_gem.c | 21 ---------------------
> > > >> >  drivers/gpu/drm/msm/msm_gem.h |  2 --
> > > >> >  3 files changed, 1 insertion(+), 24 deletions(-)
> > > >> >
> > > >> > Changes in v2:
> > > >> > Modify commit message to include the absence of internal locking leftovers
> > > >> > around allocating a fake offset in msm_gem_mmap_offset() in the generic
> > > >> > implementation drm_gem_create_map_offset().
> > > >> >
> > > >> > diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> > > >> > index a428951ee539..86a15992c717 100644
> > > >> > --- a/drivers/gpu/drm/msm/msm_drv.c
> > > >> > +++ b/drivers/gpu/drm/msm/msm_drv.c
> > > >> > @@ -1085,7 +1085,7 @@ static const struct drm_driver msm_driver = {
> > > >> >         .open               = msm_open,
> > > >> >         .postclose          = msm_postclose,
> > > >> >         .dumb_create        = msm_gem_dumb_create,
> > > >> > -       .dumb_map_offset    = msm_gem_dumb_map_offset,
> > > >> > +       .dumb_map_offset    = drm_gem_dumb_map_offset,
> > > >> >         .gem_prime_import_sg_table = msm_gem_prime_import_sg_table,
> > > >> >  #ifdef CONFIG_DEBUG_FS
> > > >> >         .debugfs_init       = msm_debugfs_init,
> > > >> > diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
> > > >> > index db1e748daa75..489694ef79cb 100644
> > > >> > --- a/drivers/gpu/drm/msm/msm_gem.c
> > > >> > +++ b/drivers/gpu/drm/msm/msm_gem.c
> > > >> > @@ -671,27 +671,6 @@ int msm_gem_dumb_create(struct drm_file *file, struct drm_device *dev,
> > > >> >                         MSM_BO_SCANOUT | MSM_BO_WC, &args->handle, "dumb");
> > > >> >  }
> > > >> >
> > > >> > -int msm_gem_dumb_map_offset(struct drm_file *file, struct drm_device *dev,
> > > >> > -               uint32_t handle, uint64_t *offset)
> > > >> > -{
> > > >> > -       struct drm_gem_object *obj;
> > > >> > -       int ret = 0;
> > > >> > -
> > > >> > -       /* GEM does all our handle to object mapping */
> > > >> > -       obj = drm_gem_object_lookup(file, handle);
> > > >> > -       if (obj == NULL) {
> > > >> > -               ret = -ENOENT;
> > > >> > -               goto fail;
> > > >> > -       }
> > > >> > -
> > > >> > -       *offset = msm_gem_mmap_offset(obj);
> > > >> > -
> > > >> > -       drm_gem_object_put(obj);
> > > >> > -
> > > >> > -fail:
> > > >> > -       return ret;
> > > >> > -}
> > > >> > -
> > > >> >  static void *get_vaddr(struct drm_gem_object *obj, unsigned madv)
> > > >> >  {
> > > >> >         struct msm_gem_object *msm_obj = to_msm_bo(obj);
> > > >> > diff --git a/drivers/gpu/drm/msm/msm_gem.h b/drivers/gpu/drm/msm/msm_gem.h
> > > >> > index 8ddef5443140..dc74a0ef865d 100644
> > > >> > --- a/drivers/gpu/drm/msm/msm_gem.h
> > > >> > +++ b/drivers/gpu/drm/msm/msm_gem.h
> > > >> > @@ -139,8 +139,6 @@ struct page **msm_gem_pin_pages(struct drm_gem_object *obj);
> > > >> >  void msm_gem_unpin_pages(struct drm_gem_object *obj);
> > > >> >  int msm_gem_dumb_create(struct drm_file *file, struct drm_device *dev,
> > > >> >                 struct drm_mode_create_dumb *args);
> > > >> > -int msm_gem_dumb_map_offset(struct drm_file *file, struct drm_device *dev,
> > > >> > -               uint32_t handle, uint64_t *offset);
> > > >> >  void *msm_gem_get_vaddr_locked(struct drm_gem_object *obj);
> > > >> >  void *msm_gem_get_vaddr(struct drm_gem_object *obj);
> > > >> >  void *msm_gem_get_vaddr_active(struct drm_gem_object *obj);
> > > >> > --
> > > >> > 2.34.1
> > > >> >
> > > >>
> > > >>
> > > >> --
> > > >> With best wishes
> > > >> Dmitry
> > >
> > >
> > >
> > > --
> > > With best wishes
> > > Dmitry
>
>
>
> --
> With best wishes
> Dmitry