diff mbox

[08/14] drm/exynos: create a fake mmap offset with gem creation

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

Commit Message

Joonyoung Shim July 28, 2015, 8:53 a.m. UTC
Don't create a fake mmap offset in exynos_drm_gem_dumb_map_offset. If
not, it will call drm_gem_create_mmap_offset whenever user requests
DRM_IOCTL_MODE_MAP_DUMB ioctl.

Signed-off-by: Joonyoung Shim <jy0922.shim@samsung.com>
---
 drivers/gpu/drm/exynos/exynos_drm_gem.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

Comments

Inki Dae Aug. 16, 2015, 4:50 a.m. UTC | #1
On 2015? 07? 28? 17:53, Joonyoung Shim wrote:
> Don't create a fake mmap offset in exynos_drm_gem_dumb_map_offset. If
> not, it will call drm_gem_create_mmap_offset whenever user requests
> DRM_IOCTL_MODE_MAP_DUMB ioctl.

This patch makes drm_gem_create_mmap_offset to be called even in case of
not using dumb* interfaces. I.e.,
exynos_drm_gem_create_ioctl -> exynos_drm_gem_mmap

And drm_gem_create_mmap_offset checks if vma_node was already allocated
or not so this patch doesn't make sense.

Thanks,
Inki Dae

> 
> Signed-off-by: Joonyoung Shim <jy0922.shim@samsung.com>
> ---
>  drivers/gpu/drm/exynos/exynos_drm_gem.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_gem.c b/drivers/gpu/drm/exynos/exynos_drm_gem.c
> index 550d267..c76aa8a 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_gem.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_gem.c
> @@ -151,6 +151,13 @@ struct exynos_drm_gem_obj *exynos_drm_gem_init(struct drm_device *dev,
>  		return ERR_PTR(ret);
>  	}
>  
> +	ret = drm_gem_create_mmap_offset(obj);
> +	if (ret < 0) {
> +		drm_gem_object_release(obj);
> +		kfree(exynos_gem_obj);
> +		return ERR_PTR(ret);
> +	}
> +
>  	DRM_DEBUG_KMS("created file object = 0x%x\n", (unsigned int)obj->filp);
>  
>  	return exynos_gem_obj;
> @@ -521,14 +528,9 @@ int exynos_drm_gem_dumb_map_offset(struct drm_file *file_priv,
>  		goto unlock;
>  	}
>  
> -	ret = drm_gem_create_mmap_offset(obj);
> -	if (ret)
> -		goto out;
> -
>  	*offset = drm_vma_node_offset_addr(&obj->vma_node);
>  	DRM_DEBUG_KMS("offset = 0x%lx\n", (unsigned long)*offset);
>  
> -out:
>  	drm_gem_object_unreference(obj);
>  unlock:
>  	mutex_unlock(&dev->struct_mutex);
>
Joonyoung Shim Aug. 17, 2015, 5:29 a.m. UTC | #2
On 08/16/2015 01:50 PM, Inki Dae wrote:
> On 2015? 07? 28? 17:53, Joonyoung Shim wrote:
>> Don't create a fake mmap offset in exynos_drm_gem_dumb_map_offset. If
>> not, it will call drm_gem_create_mmap_offset whenever user requests
>> DRM_IOCTL_MODE_MAP_DUMB ioctl.
> 
> This patch makes drm_gem_create_mmap_offset to be called even in case of
> not using dumb* interfaces. I.e.,
> exynos_drm_gem_create_ioctl -> exynos_drm_gem_mmap
> 

I know mmap() of exynos-drm also needs mmap offset in drm_gem_mmap().

> And drm_gem_create_mmap_offset checks if vma_node was already allocated
> or not so this patch doesn't make sense.
> 

OK, but it calls drm_gem_create_mmap_offset still and will be returned
after checking node->allocated. It's not unnecessary to me.

> Thanks,
> Inki Dae
> 
>>
>> Signed-off-by: Joonyoung Shim <jy0922.shim@samsung.com>
>> ---
>>  drivers/gpu/drm/exynos/exynos_drm_gem.c | 12 +++++++-----
>>  1 file changed, 7 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_gem.c b/drivers/gpu/drm/exynos/exynos_drm_gem.c
>> index 550d267..c76aa8a 100644
>> --- a/drivers/gpu/drm/exynos/exynos_drm_gem.c
>> +++ b/drivers/gpu/drm/exynos/exynos_drm_gem.c
>> @@ -151,6 +151,13 @@ struct exynos_drm_gem_obj *exynos_drm_gem_init(struct drm_device *dev,
>>  		return ERR_PTR(ret);
>>  	}
>>  
>> +	ret = drm_gem_create_mmap_offset(obj);
>> +	if (ret < 0) {
>> +		drm_gem_object_release(obj);
>> +		kfree(exynos_gem_obj);
>> +		return ERR_PTR(ret);
>> +	}
>> +
>>  	DRM_DEBUG_KMS("created file object = 0x%x\n", (unsigned int)obj->filp);
>>  
>>  	return exynos_gem_obj;
>> @@ -521,14 +528,9 @@ int exynos_drm_gem_dumb_map_offset(struct drm_file *file_priv,
>>  		goto unlock;
>>  	}
>>  
>> -	ret = drm_gem_create_mmap_offset(obj);
>> -	if (ret)
>> -		goto out;
>> -
>>  	*offset = drm_vma_node_offset_addr(&obj->vma_node);
>>  	DRM_DEBUG_KMS("offset = 0x%lx\n", (unsigned long)*offset);
>>  
>> -out:
>>  	drm_gem_object_unreference(obj);
>>  unlock:
>>  	mutex_unlock(&dev->struct_mutex);
>>
> 
>
Inki Dae Aug. 17, 2015, 7:39 a.m. UTC | #3
On 2015? 08? 17? 14:29, Joonyoung Shim wrote:
> On 08/16/2015 01:50 PM, Inki Dae wrote:
>> On 2015? 07? 28? 17:53, Joonyoung Shim wrote:
>>> Don't create a fake mmap offset in exynos_drm_gem_dumb_map_offset. If
>>> not, it will call drm_gem_create_mmap_offset whenever user requests
>>> DRM_IOCTL_MODE_MAP_DUMB ioctl.
>>
>> This patch makes drm_gem_create_mmap_offset to be called even in case of
>> not using dumb* interfaces. I.e.,
>> exynos_drm_gem_create_ioctl -> exynos_drm_gem_mmap
>>
> 
> I know mmap() of exynos-drm also needs mmap offset in drm_gem_mmap().

Ah, sorry. We already removed Exynos specific mmap ioctl. So we could
never use direct mapping ioctl anymore. I confused that.

> 
>> And drm_gem_create_mmap_offset checks if vma_node was already allocated
>> or not so this patch doesn't make sense.
>>
> 
> OK, but it calls drm_gem_create_mmap_offset still and will be returned
> after checking node->allocated. It's not unnecessary to me.
> 
>> Thanks,
>> Inki Dae
>>
>>>
>>> Signed-off-by: Joonyoung Shim <jy0922.shim@samsung.com>
>>> ---
>>>  drivers/gpu/drm/exynos/exynos_drm_gem.c | 12 +++++++-----
>>>  1 file changed, 7 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_gem.c b/drivers/gpu/drm/exynos/exynos_drm_gem.c
>>> index 550d267..c76aa8a 100644
>>> --- a/drivers/gpu/drm/exynos/exynos_drm_gem.c
>>> +++ b/drivers/gpu/drm/exynos/exynos_drm_gem.c
>>> @@ -151,6 +151,13 @@ struct exynos_drm_gem_obj *exynos_drm_gem_init(struct drm_device *dev,
>>>  		return ERR_PTR(ret);
>>>  	}
>>>  
>>> +	ret = drm_gem_create_mmap_offset(obj);
>>> +	if (ret < 0) {
>>> +		drm_gem_object_release(obj);
>>> +		kfree(exynos_gem_obj);
>>> +		return ERR_PTR(ret);
>>> +	}
>>> +
>>>  	DRM_DEBUG_KMS("created file object = 0x%x\n", (unsigned int)obj->filp);
>>>  
>>>  	return exynos_gem_obj;
>>> @@ -521,14 +528,9 @@ int exynos_drm_gem_dumb_map_offset(struct drm_file *file_priv,
>>>  		goto unlock;
>>>  	}
>>>  
>>> -	ret = drm_gem_create_mmap_offset(obj);
>>> -	if (ret)
>>> -		goto out;
>>> -
>>>  	*offset = drm_vma_node_offset_addr(&obj->vma_node);
>>>  	DRM_DEBUG_KMS("offset = 0x%lx\n", (unsigned long)*offset);
>>>  
>>> -out:
>>>  	drm_gem_object_unreference(obj);
>>>  unlock:
>>>  	mutex_unlock(&dev->struct_mutex);
>>>
>>
>>
> 
>
Daniel Vetter Nov. 16, 2015, 4:22 p.m. UTC | #4
On Tue, Jul 28, 2015 at 05:53:23PM +0900, Joonyoung Shim wrote:
> Don't create a fake mmap offset in exynos_drm_gem_dumb_map_offset. If
> not, it will call drm_gem_create_mmap_offset whenever user requests
> DRM_IOCTL_MODE_MAP_DUMB ioctl.
> 
> Signed-off-by: Joonyoung Shim <jy0922.shim@samsung.com>
> ---
>  drivers/gpu/drm/exynos/exynos_drm_gem.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_gem.c b/drivers/gpu/drm/exynos/exynos_drm_gem.c
> index 550d267..c76aa8a 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_gem.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_gem.c
> @@ -151,6 +151,13 @@ struct exynos_drm_gem_obj *exynos_drm_gem_init(struct drm_device *dev,
>  		return ERR_PTR(ret);
>  	}
>  
> +	ret = drm_gem_create_mmap_offset(obj);
> +	if (ret < 0) {
> +		drm_gem_object_release(obj);
> +		kfree(exynos_gem_obj);
> +		return ERR_PTR(ret);
> +	}
> +
>  	DRM_DEBUG_KMS("created file object = 0x%x\n", (unsigned int)obj->filp);
>  
>  	return exynos_gem_obj;
> @@ -521,14 +528,9 @@ int exynos_drm_gem_dumb_map_offset(struct drm_file *file_priv,
>  		goto unlock;
>  	}
>  
> -	ret = drm_gem_create_mmap_offset(obj);

drm_gem_create_mmap_offset internally checks whether it's been already
(protected by locks), so this code is perfectly fine. I don't see any
justification for this change (but only noticed it because rockchip
cargo-culted this change).
-Daniel

> -	if (ret)
> -		goto out;
> -
>  	*offset = drm_vma_node_offset_addr(&obj->vma_node);
>  	DRM_DEBUG_KMS("offset = 0x%lx\n", (unsigned long)*offset);
>  
> -out:
>  	drm_gem_object_unreference(obj);
>  unlock:
>  	mutex_unlock(&dev->struct_mutex);
> -- 
> 1.9.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
Daniel Vetter Nov. 16, 2015, 4:23 p.m. UTC | #5
On Mon, Nov 16, 2015 at 05:22:42PM +0100, Daniel Vetter wrote:
> On Tue, Jul 28, 2015 at 05:53:23PM +0900, Joonyoung Shim wrote:
> > Don't create a fake mmap offset in exynos_drm_gem_dumb_map_offset. If
> > not, it will call drm_gem_create_mmap_offset whenever user requests
> > DRM_IOCTL_MODE_MAP_DUMB ioctl.
> > 
> > Signed-off-by: Joonyoung Shim <jy0922.shim@samsung.com>
> > ---
> >  drivers/gpu/drm/exynos/exynos_drm_gem.c | 12 +++++++-----
> >  1 file changed, 7 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/exynos/exynos_drm_gem.c b/drivers/gpu/drm/exynos/exynos_drm_gem.c
> > index 550d267..c76aa8a 100644
> > --- a/drivers/gpu/drm/exynos/exynos_drm_gem.c
> > +++ b/drivers/gpu/drm/exynos/exynos_drm_gem.c
> > @@ -151,6 +151,13 @@ struct exynos_drm_gem_obj *exynos_drm_gem_init(struct drm_device *dev,
> >  		return ERR_PTR(ret);
> >  	}
> >  
> > +	ret = drm_gem_create_mmap_offset(obj);
> > +	if (ret < 0) {
> > +		drm_gem_object_release(obj);
> > +		kfree(exynos_gem_obj);
> > +		return ERR_PTR(ret);
> > +	}
> > +
> >  	DRM_DEBUG_KMS("created file object = 0x%x\n", (unsigned int)obj->filp);
> >  
> >  	return exynos_gem_obj;
> > @@ -521,14 +528,9 @@ int exynos_drm_gem_dumb_map_offset(struct drm_file *file_priv,
> >  		goto unlock;
> >  	}
> >  
> > -	ret = drm_gem_create_mmap_offset(obj);
> 
> drm_gem_create_mmap_offset internally checks whether it's been already
> (protected by locks), so this code is perfectly fine. I don't see any
> justification for this change (but only noticed it because rockchip
> cargo-culted this change).

I think it'd be good to revert this to stay consistent with cma helpers
and other drivers.
-Daniel

> -Daniel
> 
> > -	if (ret)
> > -		goto out;
> > -
> >  	*offset = drm_vma_node_offset_addr(&obj->vma_node);
> >  	DRM_DEBUG_KMS("offset = 0x%lx\n", (unsigned long)*offset);
> >  
> > -out:
> >  	drm_gem_object_unreference(obj);
> >  unlock:
> >  	mutex_unlock(&dev->struct_mutex);
> > -- 
> > 1.9.1
> > 
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
Inki Dae Nov. 17, 2015, 2:53 a.m. UTC | #6
2015? 11? 17? 01:23? Daniel Vetter ?(?) ? ?:
> On Mon, Nov 16, 2015 at 05:22:42PM +0100, Daniel Vetter wrote:
>> On Tue, Jul 28, 2015 at 05:53:23PM +0900, Joonyoung Shim wrote:
>>> Don't create a fake mmap offset in exynos_drm_gem_dumb_map_offset. If
>>> not, it will call drm_gem_create_mmap_offset whenever user requests
>>> DRM_IOCTL_MODE_MAP_DUMB ioctl.
>>>
>>> Signed-off-by: Joonyoung Shim <jy0922.shim@samsung.com>
>>> ---
>>>  drivers/gpu/drm/exynos/exynos_drm_gem.c | 12 +++++++-----
>>>  1 file changed, 7 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_gem.c b/drivers/gpu/drm/exynos/exynos_drm_gem.c
>>> index 550d267..c76aa8a 100644
>>> --- a/drivers/gpu/drm/exynos/exynos_drm_gem.c
>>> +++ b/drivers/gpu/drm/exynos/exynos_drm_gem.c
>>> @@ -151,6 +151,13 @@ struct exynos_drm_gem_obj *exynos_drm_gem_init(struct drm_device *dev,
>>>  		return ERR_PTR(ret);
>>>  	}
>>>  
>>> +	ret = drm_gem_create_mmap_offset(obj);
>>> +	if (ret < 0) {
>>> +		drm_gem_object_release(obj);
>>> +		kfree(exynos_gem_obj);
>>> +		return ERR_PTR(ret);
>>> +	}
>>> +
>>>  	DRM_DEBUG_KMS("created file object = 0x%x\n", (unsigned int)obj->filp);
>>>  
>>>  	return exynos_gem_obj;
>>> @@ -521,14 +528,9 @@ int exynos_drm_gem_dumb_map_offset(struct drm_file *file_priv,
>>>  		goto unlock;
>>>  	}
>>>  
>>> -	ret = drm_gem_create_mmap_offset(obj);
>>
>> drm_gem_create_mmap_offset internally checks whether it's been already
>> (protected by locks), so this code is perfectly fine. I don't see any
>> justification for this change (but only noticed it because rockchip
>> cargo-culted this change).
> 
> I think it'd be good to revert this to stay consistent with cma helpers
> and other drivers.

At least, seems cma halper doesn't also call drm_gem_create_mmap_offset function
at drm_gem_cma_dumb_map_offset function and calls it at cma creation instead.
So I think now Exynos drm keeps consistent with cma helper.

Thanks,
Inki Dae

> -Daniel
> 
>> -Daniel
>>
>>> -	if (ret)
>>> -		goto out;
>>> -
>>>  	*offset = drm_vma_node_offset_addr(&obj->vma_node);
>>>  	DRM_DEBUG_KMS("offset = 0x%lx\n", (unsigned long)*offset);
>>>  
>>> -out:
>>>  	drm_gem_object_unreference(obj);
>>>  unlock:
>>>  	mutex_unlock(&dev->struct_mutex);
>>> -- 
>>> 1.9.1
>>>
>>> _______________________________________________
>>> dri-devel mailing list
>>> dri-devel@lists.freedesktop.org
>>> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>>
>> -- 
>> Daniel Vetter
>> Software Engineer, Intel Corporation
>> http://blog.ffwll.ch
>
Daniel Vetter Nov. 17, 2015, 9:42 a.m. UTC | #7
On Tue, Nov 17, 2015 at 11:53:28AM +0900, Inki Dae wrote:
> 
> 
> 2015? 11? 17? 01:23? Daniel Vetter ?(?) ? ?:
> > On Mon, Nov 16, 2015 at 05:22:42PM +0100, Daniel Vetter wrote:
> >> On Tue, Jul 28, 2015 at 05:53:23PM +0900, Joonyoung Shim wrote:
> >>> Don't create a fake mmap offset in exynos_drm_gem_dumb_map_offset. If
> >>> not, it will call drm_gem_create_mmap_offset whenever user requests
> >>> DRM_IOCTL_MODE_MAP_DUMB ioctl.
> >>>
> >>> Signed-off-by: Joonyoung Shim <jy0922.shim@samsung.com>
> >>> ---
> >>>  drivers/gpu/drm/exynos/exynos_drm_gem.c | 12 +++++++-----
> >>>  1 file changed, 7 insertions(+), 5 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_gem.c b/drivers/gpu/drm/exynos/exynos_drm_gem.c
> >>> index 550d267..c76aa8a 100644
> >>> --- a/drivers/gpu/drm/exynos/exynos_drm_gem.c
> >>> +++ b/drivers/gpu/drm/exynos/exynos_drm_gem.c
> >>> @@ -151,6 +151,13 @@ struct exynos_drm_gem_obj *exynos_drm_gem_init(struct drm_device *dev,
> >>>  		return ERR_PTR(ret);
> >>>  	}
> >>>  
> >>> +	ret = drm_gem_create_mmap_offset(obj);
> >>> +	if (ret < 0) {
> >>> +		drm_gem_object_release(obj);
> >>> +		kfree(exynos_gem_obj);
> >>> +		return ERR_PTR(ret);
> >>> +	}
> >>> +
> >>>  	DRM_DEBUG_KMS("created file object = 0x%x\n", (unsigned int)obj->filp);
> >>>  
> >>>  	return exynos_gem_obj;
> >>> @@ -521,14 +528,9 @@ int exynos_drm_gem_dumb_map_offset(struct drm_file *file_priv,
> >>>  		goto unlock;
> >>>  	}
> >>>  
> >>> -	ret = drm_gem_create_mmap_offset(obj);
> >>
> >> drm_gem_create_mmap_offset internally checks whether it's been already
> >> (protected by locks), so this code is perfectly fine. I don't see any
> >> justification for this change (but only noticed it because rockchip
> >> cargo-culted this change).
> > 
> > I think it'd be good to revert this to stay consistent with cma helpers
> > and other drivers.
> 
> At least, seems cma halper doesn't also call drm_gem_create_mmap_offset function
> at drm_gem_cma_dumb_map_offset function and calls it at cma creation instead.
> So I think now Exynos drm keeps consistent with cma helper.

Indeed, common style is to create the offset at alloc time - I checked a
few other drivers too. Never mind my comment, sorry for the noise.
-Daniel
diff mbox

Patch

diff --git a/drivers/gpu/drm/exynos/exynos_drm_gem.c b/drivers/gpu/drm/exynos/exynos_drm_gem.c
index 550d267..c76aa8a 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_gem.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_gem.c
@@ -151,6 +151,13 @@  struct exynos_drm_gem_obj *exynos_drm_gem_init(struct drm_device *dev,
 		return ERR_PTR(ret);
 	}
 
+	ret = drm_gem_create_mmap_offset(obj);
+	if (ret < 0) {
+		drm_gem_object_release(obj);
+		kfree(exynos_gem_obj);
+		return ERR_PTR(ret);
+	}
+
 	DRM_DEBUG_KMS("created file object = 0x%x\n", (unsigned int)obj->filp);
 
 	return exynos_gem_obj;
@@ -521,14 +528,9 @@  int exynos_drm_gem_dumb_map_offset(struct drm_file *file_priv,
 		goto unlock;
 	}
 
-	ret = drm_gem_create_mmap_offset(obj);
-	if (ret)
-		goto out;
-
 	*offset = drm_vma_node_offset_addr(&obj->vma_node);
 	DRM_DEBUG_KMS("offset = 0x%lx\n", (unsigned long)*offset);
 
-out:
 	drm_gem_object_unreference(obj);
 unlock:
 	mutex_unlock(&dev->struct_mutex);