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

From: Lucas Stach <l.stach@pengutronix.de>
To: Thomas Zimmermann <tzimmermann@suse.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 12:50:56 +0200	[thread overview]
Message-ID: <d31b8b5237695d9bea3ad52b9be410249d12d652.camel@pengutronix.de> (raw)
In-Reply-To: <ddae9158-0aa7-f861-2c61-5d8f8d28fb62@suse.de>

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?

Regards,
Lucas