[PATCH 1/5] drm/panfrost: Restructure the GEM object creation

[Rob Herring]

On Thu, Jul 18, 2019 at 9:03 AM Steven Price <steven.price at arm.com> wrote:
>
> On 17/07/2019 19:33, Rob Herring wrote:
> > Setting the GPU VA when creating the GEM object doesn't allow for any
> > conditional adjustments. In preparation to support adjusting the
> > mapping, restructure the GEM object creation to map the GEM object after
> > we've created the base shmem object.
> >
> > Cc: Tomeu Vizoso <tomeu.vizoso at collabora.com>
> > Cc: Boris Brezillon <boris.brezillon at collabora.com>
> > Cc: Robin Murphy <robin.murphy at arm.com>
> > Cc: Steven Price <steven.price at arm.com>
> > Cc: Alyssa Rosenzweig <alyssa at rosenzweig.io>
> > Signed-off-by: Rob Herring <robh at kernel.org>
>
> Hi Rob,
>
> I couldn't work out what base this series should be applied to, but I've
> tried manually applying it against Linus' tree and run a few tests.

It's drm-misc-next plus some fixes in drm-misc-fixes that haven't been
merged into drm-misc-next yet. I'll post a git branch on the next
version.

> PANFROST_BO_NOEXEC works as expected, but PANFROST_BO_HEAP seems to
> cause a memory leak.
>
> > ---
> >  drivers/gpu/drm/panfrost/panfrost_drv.c | 21 +++------
> >  drivers/gpu/drm/panfrost/panfrost_gem.c | 58 ++++++++++++++++++++-----
> >  drivers/gpu/drm/panfrost/panfrost_gem.h |  5 +++
> >  3 files changed, 59 insertions(+), 25 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
> > index cb43ff4ebf4a..d354b92964d5 100644
> > --- a/drivers/gpu/drm/panfrost/panfrost_drv.c
> > +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
> > @@ -46,29 +46,20 @@ static int panfrost_ioctl_get_param(struct drm_device *ddev, void *data, struct
> >  static int panfrost_ioctl_create_bo(struct drm_device *dev, void *data,
> >               struct drm_file *file)
> >  {
> > -     int ret;
> > -     struct drm_gem_shmem_object *shmem;
> > +     struct panfrost_gem_object *bo;
> >       struct drm_panfrost_create_bo *args = data;
> >
> >       if (!args->size || args->flags || args->pad)
> >               return -EINVAL;
> >
> > -     shmem = drm_gem_shmem_create_with_handle(file, dev, args->size,
> > -                                              &args->handle);
> > -     if (IS_ERR(shmem))
> > -             return PTR_ERR(shmem);
> > -
> > -     ret = panfrost_mmu_map(to_panfrost_bo(&shmem->base));
> > -     if (ret)
> > -             goto err_free;
> > +     bo = panfrost_gem_create_with_handle(file, dev, args->size, args->flags,
> > +                                          &args->handle);
> > +     if (IS_ERR(bo))
> > +             return PTR_ERR(bo);
> >
> > -     args->offset = to_panfrost_bo(&shmem->base)->node.start << PAGE_SHIFT;
> > +     args->offset = bo->node.start << PAGE_SHIFT;
> >
> >       return 0;
> > -
> > -err_free:
> > -     drm_gem_handle_delete(file, args->handle);
> > -     return ret;
> >  }
> >
> >  /**
> > diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.c b/drivers/gpu/drm/panfrost/panfrost_gem.c
> > index 543ab1b81bd5..df70dcf3cb2f 100644
> > --- a/drivers/gpu/drm/panfrost/panfrost_gem.c
> > +++ b/drivers/gpu/drm/panfrost/panfrost_gem.c
> > @@ -23,7 +23,8 @@ static void panfrost_gem_free_object(struct drm_gem_object *obj)
> >               panfrost_mmu_unmap(bo);
> >
> >       spin_lock(&pfdev->mm_lock);
> > -     drm_mm_remove_node(&bo->node);
> > +     if (drm_mm_node_allocated(&bo->node))
> > +             drm_mm_remove_node(&bo->node);
>
> I'm not sure this change should be here - it looks more like it was
> meant as part of patch 4.

It's needed here because because we do the mapping later instead of
the .gem_create_object() hook. So when we free the object in case of
errors, the mapping may or may not have been created.

Rob