Re: [PATCH] drm/etnaviv: Implement mmap as GEM object function

From: Thomas Zimmermann <tzimmermann@suse.de>
To: Lucas Stach <l.stach@pengutronix.de>,
	linux+etnaviv@armlinux.org.uk, christian.gmeiner@gmail.com,
	airlied@linux.ie, daniel@ffwll.ch
Cc: etnaviv@lists.freedesktop.org, dri-devel@lists.freedesktop.org
Subject: Re: [PATCH] drm/etnaviv: Implement mmap as GEM object function
Date: Thu, 24 Jun 2021 14:22:25 +0200	[thread overview]
Message-ID: <42715b01-9944-ef8e-b41a-895d60a7ba09@suse.de> (raw)
In-Reply-To: <d31b8b5237695d9bea3ad52b9be410249d12d652.camel@pengutronix.de>

[-- Attachment #1.1: Type: text/plain, Size: 5762 bytes --]

Am 24.06.21 um 12:50 schrieb Lucas Stach:
> Am Donnerstag, dem 24.06.2021 um 12:47 +0200 schrieb Thomas Zimmermann:
>> Hi
>>
>> Am 24.06.21 um 11:11 schrieb Lucas Stach:
>>> Am Donnerstag, dem 24.06.2021 um 10:58 +0200 schrieb Thomas Zimmermann:
>>>> Moving the driver-specific mmap code into a GEM object function allows
>>>> for using DRM helpers for various mmap callbacks.
>>>>
>>>> The respective etnaviv functions are being removed. The file_operations
>>>> structure fops is now being created by the helper macro
>>>> DEFINE_DRM_GEM_FOPS().
>>>>
>>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>>>> ---
>>>>    drivers/gpu/drm/etnaviv/etnaviv_drv.c       | 14 ++------------
>>>>    drivers/gpu/drm/etnaviv/etnaviv_drv.h       |  3 ---
>>>>    drivers/gpu/drm/etnaviv/etnaviv_gem.c       | 18 +++++-------------
>>>>    drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c | 13 -------------
>>>>    4 files changed, 7 insertions(+), 41 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.c b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
>>>> index f0a07278ad04..7dcc6392792d 100644
>>>> --- a/drivers/gpu/drm/etnaviv/etnaviv_drv.c
>>>> +++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
>>>> @@ -468,17 +468,7 @@ static const struct drm_ioctl_desc etnaviv_ioctls[] = {
>>>>    	ETNA_IOCTL(PM_QUERY_SIG, pm_query_sig, DRM_RENDER_ALLOW),
>>>>    };
>>>>    
>>>> -static const struct file_operations fops = {
>>>> -	.owner              = THIS_MODULE,
>>>> -	.open               = drm_open,
>>>> -	.release            = drm_release,
>>>> -	.unlocked_ioctl     = drm_ioctl,
>>>> -	.compat_ioctl       = drm_compat_ioctl,
>>>> -	.poll               = drm_poll,
>>>> -	.read               = drm_read,
>>>> -	.llseek             = no_llseek,
>>>> -	.mmap               = etnaviv_gem_mmap,
>>>> -};
>>>> +DEFINE_DRM_GEM_FOPS(fops);
>>>>    
>>>>    static const struct drm_driver etnaviv_drm_driver = {
>>>>    	.driver_features    = DRIVER_GEM | DRIVER_RENDER,
>>>> @@ -487,7 +477,7 @@ static const struct drm_driver etnaviv_drm_driver = {
>>>>    	.prime_handle_to_fd = drm_gem_prime_handle_to_fd,
>>>>    	.prime_fd_to_handle = drm_gem_prime_fd_to_handle,
>>>>    	.gem_prime_import_sg_table = etnaviv_gem_prime_import_sg_table,
>>>> -	.gem_prime_mmap     = etnaviv_gem_prime_mmap,
>>>> +	.gem_prime_mmap     = drm_gem_prime_mmap,
>>>>    #ifdef CONFIG_DEBUG_FS
>>>>    	.debugfs_init       = etnaviv_debugfs_init,
>>>>    #endif
>>>> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.h b/drivers/gpu/drm/etnaviv/etnaviv_drv.h
>>>> index 003288ebd896..049ae87de9be 100644
>>>> --- a/drivers/gpu/drm/etnaviv/etnaviv_drv.h
>>>> +++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.h
>>>> @@ -47,12 +47,9 @@ struct etnaviv_drm_private {
>>>>    int etnaviv_ioctl_gem_submit(struct drm_device *dev, void *data,
>>>>    		struct drm_file *file);
>>>>    
>>>> -int etnaviv_gem_mmap(struct file *filp, struct vm_area_struct *vma);
>>>>    int etnaviv_gem_mmap_offset(struct drm_gem_object *obj, u64 *offset);
>>>>    struct sg_table *etnaviv_gem_prime_get_sg_table(struct drm_gem_object *obj);
>>>>    int etnaviv_gem_prime_vmap(struct drm_gem_object *obj, struct dma_buf_map *map);
>>>> -int etnaviv_gem_prime_mmap(struct drm_gem_object *obj,
>>>> -			   struct vm_area_struct *vma);
>>>>    struct drm_gem_object *etnaviv_gem_prime_import_sg_table(struct drm_device *dev,
>>>>    	struct dma_buf_attachment *attach, struct sg_table *sg);
>>>>    int etnaviv_gem_prime_pin(struct drm_gem_object *obj);
>>>> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.c b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
>>>> index b8fa6ed3dd73..8f1b5af47dd6 100644
>>>> --- a/drivers/gpu/drm/etnaviv/etnaviv_gem.c
>>>> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
>>>> @@ -130,8 +130,7 @@ static int etnaviv_gem_mmap_obj(struct etnaviv_gem_object *etnaviv_obj,
>>>>    {
>>>>    	pgprot_t vm_page_prot;
>>>>    
>>>> -	vma->vm_flags &= ~VM_PFNMAP;
>>>> -	vma->vm_flags |= VM_MIXEDMAP;
>>>> +	vma->vm_flags |= VM_IO | VM_MIXEDMAP | VM_DONTEXPAND | VM_DONTDUMP;
>>>
>>> I don't fully understand why this change is needed and the commit log
>>> is silent about it. Excuse my ignorance, but can you please explain the
>>> reasoning behind this change?
>>
>> Sure, sorry for being brief.
>>
>> I worked on cleaning up the deprecated gem_prime_* callbacks in struct
>> drm_driver. These are supposed to be GEM object functions. The only
>> obsolete gem prime callback in drm_driver is gem_prime_mmap.
> 
> Sorry, that's a misunderstanding. I see the justification for the patch
> as a whole. I was asking specifically about the hunk above my comment.
> Why are the vm_flags changed and how did you come up with this exact
> combination of flags?

I took the existing implementation and looked through it for the current 
combination of flags.

Etnaviv calls etnaviv_gem_prime_mmap(), which in turn calls 
drm_gem_mmap_obj(). [1] Because etnaviv doesn't have a gem object mmap 
callback, drm_gem_mmap_obj() sets some default flags. [2] VM_PFNMAP 
later gets cleared by the current code, so I left it out. And that's 
where these flags come from.

I should add that I don't have the HW to test this change. If you can, 
maybe give it a try.

Best regards
Thomas

[1] 
https://elixir.bootlin.com/linux/v5.12/source/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c#L43

[2] 
https://elixir.bootlin.com/linux/v5.12/source/drivers/gpu/drm/drm_gem.c#L1084

> 
> Regards,
> Lucas
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]