diff mbox

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

Message ID 1438073609-32664-9-git-send-email-jy0922.shim@samsung.com
State New, archived
Headers show

Commit Message

Joonyoung Shim July 28, 2015, 8:53 a.m. UTC
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@samsung.com>
---
 drivers/gpu/drm/exynos/exynos_drm_gem.c | 3 ---
 1 file changed, 3 deletions(-)

Comments

Inki Dae Aug. 16, 2015, 5:07 a.m. UTC | #1
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@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.

Thanks,
Inki Dae

>  
>  	return ret;
>  }
>
Joonyoung Shim Aug. 17, 2015, 5:29 a.m. UTC | #2
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@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.
Inki Dae Aug. 17, 2015, 7:52 a.m. UTC | #3
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@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?

>
Joonyoung Shim Aug. 17, 2015, 8:17 a.m. UTC | #4
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@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.
Inki Dae Aug. 17, 2015, 9:03 a.m. UTC | #5
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@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.

Thanks,
Inki Dae

>
Joonyoung Shim Sept. 24, 2015, 1:01 a.m. UTC | #6
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@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?

Thanks.
Inki Dae Sept. 25, 2015, 9:15 a.m. UTC | #7
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@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,
Inki Dae

> 
> Thanks.
>
Joonyoung Shim Sept. 30, 2015, 5:45 a.m. UTC | #8
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@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.
diff mbox

Patch

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);
 
 	return ret;
 }