[PATCH 09/14] drm/exynos: remove call to drm_gem_free_mmap_offset()

[Joonyoung Shim]

On 09/25/2015 06:15 PM, Inki Dae wrote:
> On 2015년 09월 24일 10:01, Joonyoung Shim wrote:
>> Hi Inki,
>>
>> On 08/17/2015 06:03 PM, Inki Dae wrote:
>>> On 2015년 08월 17일 17:17, Joonyoung Shim wrote:
>>>> On 08/17/2015 04:52 PM, Inki Dae wrote:
>>>>> On 2015년 08월 17일 14:29, Joonyoung Shim wrote:
>>>>>> On 08/16/2015 02:07 PM, Inki Dae wrote:
>>>>>>> On 2015년 07월 28일 17:53, Joonyoung Shim wrote:
>>>>>>>> The drm_gem_object_release() function already performs this cleanup,
>>>>>>>> so there is no reason to do it explicitly.
>>>>>>>>
>>>>>>>> Signed-off-by: Joonyoung Shim <jy0922.shim at samsung.com>
>>>>>>>> ---
>>>>>>>>  drivers/gpu/drm/exynos/exynos_drm_gem.c | 3 ---
>>>>>>>>  1 file changed, 3 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_gem.c b/drivers/gpu/drm/exynos/exynos_drm_gem.c
>>>>>>>> index c76aa8a..ab7d029 100644
>>>>>>>> --- a/drivers/gpu/drm/exynos/exynos_drm_gem.c
>>>>>>>> +++ b/drivers/gpu/drm/exynos/exynos_drm_gem.c
>>>>>>>> @@ -100,8 +100,6 @@ out:
>>>>>>>>  	exynos_drm_fini_buf(obj->dev, buf);
>>>>>>>>  	exynos_gem_obj->buffer = NULL;
>>>>>>>>  
>>>>>>>> -	drm_gem_free_mmap_offset(obj);
>>>>>>>> -
>>>>>>>>  	/* release file pointer to gem object. */
>>>>>>>>  	drm_gem_object_release(obj);
>>>>>>>>  
>>>>>>>> @@ -600,7 +598,6 @@ int exynos_drm_gem_mmap(struct file *filp, struct vm_area_struct *vma)
>>>>>>>>  
>>>>>>>>  err_close_vm:
>>>>>>>>  	drm_gem_vm_close(vma);
>>>>>>>> -	drm_gem_free_mmap_offset(obj);
>>>>>>>
>>>>>>> Without previous patch, drm_gem_free_mmap_offset is required. I guess
>>>>>>> the reason you removed above line is that you thought
>>>>>>> drm_gem_object_release function would be called by drm_gem_vm_close
>>>>>>> function which drops a reference of the gem object.
>>>>>>>
>>>>>>> However, drm_gem_vm_close should be a pair with drm_gem_vm_open
>>>>>>> function. These will be called whenever a process opens or closes the
>>>>>>> VMA. So the reference count of the gem object had already been taken by
>>>>>>> open operation when a new reference to the VMA had been created.
>>>>>>>
>>>>>>
>>>>>> This changes is not related with drm_gem_vm_close and prior patch. Why
>>>>>> should free mmap offset when mmap operation is failed? The mmap offset
>>>>>> can be used repeatedly.
>>>>>
>>>>> Isn't vm space of vm manager still used even if any user-space process
>>>>> doesn't use the region? And if mmap is failed, then the user-space
>>>>> process will be terminated. Do you think it can be re-tried? However,
>>>>> mmap system call never return -EAGAIN. Is it reasonable to you? I cannot
>>>>> understand how the mmap offset can be re-used. So can you show me some
>>>>> example?
>>>>>
>>>>
>>>> Currently, mmap offset of exynos-drm gem is made by                 
>>>> DRM_IOCTL_MODE_MAP_DUMB ioctl and mmap() ioctl just uses the mmap     
>>>> offset. User will use same mmap offset about same gem. It's why mmap
>>>> offset made by DRM_IOCTL_MODE_MAP_DUMB ioctl is unnecessary, it's just
>>>> enough to make mmap offset from when gem is create. You can get a
>>>> reference from drm_gem_cma_helper.c file.
>>>
>>> Hmm... It's not that the mmap offset can be re-used or not. It's that
>>> the mmap offset should be released or not when mmap failed. As your
>>> original comment, the call of drm_gem_free_mmap_offset() is unnecessary
>>> if mmap offset is created when gem creation because the mmap offset is
>>> removed by drm_gem_object_release() when gem is destroyed - gem should
>>> also be released when mmap failed.
>>>
>>> Ok, let's create mmap offset at gem creation and remove it gem
>>> releasing. Will merge these two patches.
>>>
>>
>> I can't find them from your git. Could you merge them?
> 
> Oops, sorry. Merged.
> 

Thanks for merge, but you are missing the first patch still,
http://www.spinics.net/lists/dri-devel/msg86891.html

Thanks.