On Wed, Dec 11, 2019 at 6:38 AM Steven Price <steven.price@xxxxxxx> wrote:
>
> On 10/12/2019 23:08, Rob Herring wrote:
> > From: Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxxxxx>
> > Signed-off-by: Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx>
> > Signed-off-by: Rob Herring <robh@xxxxxxxxxx>
> > ---
> > 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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
