[PATCH 09/14] drm/exynos: remove call to drm_gem_free_mmap_offset()
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 --- 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.
[PATCH 09/14] drm/exynos: remove call to drm_gem_free_mmap_offset()
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 >>> --- >>> 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. >
[PATCH 09/14] drm/exynos: remove call to drm_gem_free_mmap_offset()
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 >> --- >> 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.
[PATCH 09/14] drm/exynos: remove call to drm_gem_free_mmap_offset()
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 > --- > 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 >
[PATCH 09/14] drm/exynos: remove call to drm_gem_free_mmap_offset()
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 --- 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.
[PATCH 09/14] drm/exynos: remove call to drm_gem_free_mmap_offset()
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 >>> --- >>> 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? >
[PATCH 09/14] drm/exynos: remove call to drm_gem_free_mmap_offset()
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 >> --- >> 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.
[PATCH 09/14] drm/exynos: remove call to drm_gem_free_mmap_offset()
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 > --- > 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; > } >
[PATCH 09/14] drm/exynos: remove call to drm_gem_free_mmap_offset()
The drm_gem_object_release() function already performs this cleanup, so there is no reason to do it explicitly. Signed-off-by: Joonyoung Shim --- 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); return ret; } -- 1.9.1
