On 19/07/2019 23:22, Rob Herring wrote:
> On Thu, Jul 18, 2019 at 9:03 AM Steven Price <steven.price@xxxxxxx> 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@xxxxxxxxxxxxx>
>>> Cc: Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx>
>>> Cc: Robin Murphy <robin.murphy@xxxxxxx>
>>> Cc: Steven Price <steven.price@xxxxxxx>
>>> Cc: Alyssa Rosenzweig <alyssa@xxxxxxxxxxxxx>
>>> Signed-off-by: Rob Herring <robh@xxxxxxxxxx>
>>
>> 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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
