[PATCH v2] drm/panfrost: Add the panfrost_gem_mapping concept

[Rob Herring]

On Wed, Dec 11, 2019 at 6:38 AM Steven Price <steven.price at arm.com> wrote:
>
> On 10/12/2019 23:08, Rob Herring wrote:
> > From: Boris Brezillon <boris.brezillon at collabora.com>
> >
> > With the introduction of per-FD address space, the same BO can be mapped
> > in different address space if the BO is globally visible (GEM_FLINK)
> > and opened in different context or if the dmabuf is self-imported. The
> > current implementation does not take that case into account, and
> > attaches the mapping directly to the panfrost_gem_object.
> >
> > Let's create a panfrost_gem_mapping struct and allow multiple mappings
> > per BO.
> >
> > The mappings are refcounted which helps solve another problem where
> > mappings were torn down (GEM handle closed by userspace) while GPU
> > jobs accessing those BOs were still in-flight. Jobs now keep a
> > reference on the mappings they use.
> >
> > v2 (robh):
> > - Minor review comment clean-ups from Steven
> > - Use list_is_singular helper
> > - Just WARN if we add a mapping when madvise state is not WILLNEED.
> >   With that, drop the use of object_name_lock.
> >
> > Fixes: a5efb4c9a562 ("drm/panfrost: Restructure the GEM object creation")
> > Fixes: 7282f7645d06 ("drm/panfrost: Implement per FD address spaces")
> > Cc: <stable at vger.kernel.org>
> > Signed-off-by: Boris Brezillon <boris.brezillon at collabora.com>
> > Signed-off-by: Rob Herring <robh at kernel.org>
> > ---
> > I've hacked up IGT prime_self_import test to run on panfrost other than
> > the 2 test which depend on i915 debugfs files (export-vs-gem_close-race,
> > reimport-vs-gem_close-race). With this patch, they now pass.
> >
> > I'm not adding the test to IGT which is just a copy-n-paste of the
> > original except for different wrappers for BO alloc and mmap. That
> > should be fixed first IMO.
> >
> > Rob
> >
> >  drivers/gpu/drm/panfrost/panfrost_drv.c       |  91 +++++++++++--
> >  drivers/gpu/drm/panfrost/panfrost_gem.c       | 123 +++++++++++++++---
> >  drivers/gpu/drm/panfrost/panfrost_gem.h       |  41 +++++-
> >  .../gpu/drm/panfrost/panfrost_gem_shrinker.c  |   3 +-
> >  drivers/gpu/drm/panfrost/panfrost_job.c       |  13 +-
> >  drivers/gpu/drm/panfrost/panfrost_job.h       |   1 +
> >  drivers/gpu/drm/panfrost/panfrost_mmu.c       |  61 +++++----
> >  drivers/gpu/drm/panfrost/panfrost_mmu.h       |   6 +-
> >  drivers/gpu/drm/panfrost/panfrost_perfcnt.c   |  34 +++--
> >  9 files changed, 299 insertions(+), 74 deletions(-)
> >
>
> <snip>
>
> > diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.c b/drivers/gpu/drm/panfrost/panfrost_gem.c
> > index fd766b1395fb..3a7862e3e775 100644
> > --- a/drivers/gpu/drm/panfrost/panfrost_gem.c
> > +++ b/drivers/gpu/drm/panfrost/panfrost_gem.c
> > @@ -29,6 +29,12 @@ static void panfrost_gem_free_object(struct drm_gem_object *obj)
> >       list_del_init(&bo->base.madv_list);
> >       mutex_unlock(&pfdev->shrinker_lock);
> >
> > +     /*
> > +      * If we still have mappings attached to the BO, there's a problem in
> > +      * our refcounting.
> > +      */
> > +     WARN_ON_ONCE(!list_empty(&bo->mappings.list));
> > +
> >       if (bo->sgts) {
> >               int i;
> >               int n_sgt = bo->base.base.size / SZ_2M;
> > @@ -46,6 +52,68 @@ static void panfrost_gem_free_object(struct drm_gem_object *obj)
> >       drm_gem_shmem_free_object(obj);
> >  }
> >
> > +struct panfrost_gem_mapping *
> > +panfrost_gem_mapping_get(struct panfrost_gem_object *bo,
> > +                      struct panfrost_file_priv *priv)
> > +{
> > +     struct panfrost_gem_mapping *iter;
> > +
> > +     mutex_lock(&bo->mappings.lock);
> > +     list_for_each_entry(iter, &bo->mappings.list, node) {
> > +             if (iter->mmu == &priv->mmu) {
> > +                     kref_get(&iter->refcount);
> > +                     break;
> > +             }
> > +     }
> > +     mutex_unlock(&bo->mappings.lock);
> > +
> > +     return iter;
>
> If the entry isn't found then iter will equal
> container_of(&bo->mappings.list, struct panfrost_gem_mapping, node) -
> but you actually want a NULL return in this case.

Ugg, yes. I knew that...

> I also think the previous version with a "member" variable being
> returned was clearer.

I over interpreted what you were suggesting. Will change it back and
*just* add the break.

Rob