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

Steven Price steven.price at arm.com
Mon Jul 22 09:49:17 UTC 2019


On 19/07/2019 23:22, Rob Herring wrote:
> 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.

No problem, I just wasn't entirely sure I'd applied it correctly.

>> 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.

Ah, that makes sense - I'd got a bit thrown off by looking for potential
memory leaks everywhere :)

Steve


More information about the dri-devel mailing list