Message ID | 20210624085800.7941-1-tzimmermann@suse.de (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | drm/etnaviv: Implement mmap as GEM object function | expand |
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? Regards, Lucas > > vm_page_prot = vm_get_page_prot(vma->vm_flags); > > @@ -154,19 +153,11 @@ static int etnaviv_gem_mmap_obj(struct etnaviv_gem_object *etnaviv_obj, > return 0; > } > > -int etnaviv_gem_mmap(struct file *filp, struct vm_area_struct *vma) > +static int etnaviv_gem_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma) > { > - struct etnaviv_gem_object *obj; > - int ret; > - > - ret = drm_gem_mmap(filp, vma); > - if (ret) { > - DBG("mmap failed: %d", ret); > - return ret; > - } > + struct etnaviv_gem_object *etnaviv_obj = to_etnaviv_bo(obj); > > - obj = to_etnaviv_bo(vma->vm_private_data); > - return obj->ops->mmap(obj, vma); > + return etnaviv_obj->ops->mmap(etnaviv_obj, vma); > } > > static vm_fault_t etnaviv_gem_fault(struct vm_fault *vmf) > @@ -567,6 +558,7 @@ static const struct drm_gem_object_funcs etnaviv_gem_object_funcs = { > .unpin = etnaviv_gem_prime_unpin, > .get_sg_table = etnaviv_gem_prime_get_sg_table, > .vmap = etnaviv_gem_prime_vmap, > + .mmap = etnaviv_gem_mmap, > .vm_ops = &vm_ops, > }; > > diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c b/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c > index d741b1d735f7..6d8bed9c739d 100644 > --- a/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c > +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c > @@ -34,19 +34,6 @@ int etnaviv_gem_prime_vmap(struct drm_gem_object *obj, struct dma_buf_map *map) > return 0; > } > > -int etnaviv_gem_prime_mmap(struct drm_gem_object *obj, > - struct vm_area_struct *vma) > -{ > - struct etnaviv_gem_object *etnaviv_obj = to_etnaviv_bo(obj); > - int ret; > - > - ret = drm_gem_mmap_obj(obj, obj->size, vma); > - if (ret < 0) > - return ret; > - > - return etnaviv_obj->ops->mmap(etnaviv_obj, vma); > -} > - > int etnaviv_gem_prime_pin(struct drm_gem_object *obj) > { > if (!obj->import_attach) {
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. Currently drivers implement mmap in via callbacks in struct file_operations.mmap, struct drm_driver.gem_prime_mmap and struct drm_gem_object_funcs.mmap (and sometimes an additional mmap for fbdev emulation). That's way too much duplication. The correct place to implement mmap is in struct drm_gem_object_funcs. I'm consolidating DRM mmap code in struct drm_gem_object_funcs.mmap. There's even a fixme comment about this. [1] With the cleaned up mmap, DRM drivers can switch to DRM helpers and macros for all other instances of mmap. And only a handful of drivers if left to convert. For these final drivers (e.g., etnaviv) I replace the driver code with the generic helpers and move the specific flags and setup into the GEM object's mmap function. Once finished, all DRM drivers will implement gem_prime_mmap with drm_gem_prime_mmap(). The core code can be further simplified from there and this will allow to remove gem_prime_mmap and the occasional fbdev mmap. Best regards Thomas [1] https://elixir.bootlin.com/linux/latest/source/include/drm/drm_drv.h#L388 > > Regards, > Lucas > >> >> vm_page_prot = vm_get_page_prot(vma->vm_flags); >> >> @@ -154,19 +153,11 @@ static int etnaviv_gem_mmap_obj(struct etnaviv_gem_object *etnaviv_obj, >> return 0; >> } >> >> -int etnaviv_gem_mmap(struct file *filp, struct vm_area_struct *vma) >> +static int etnaviv_gem_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma) >> { >> - struct etnaviv_gem_object *obj; >> - int ret; >> - >> - ret = drm_gem_mmap(filp, vma); >> - if (ret) { >> - DBG("mmap failed: %d", ret); >> - return ret; >> - } >> + struct etnaviv_gem_object *etnaviv_obj = to_etnaviv_bo(obj); >> >> - obj = to_etnaviv_bo(vma->vm_private_data); >> - return obj->ops->mmap(obj, vma); >> + return etnaviv_obj->ops->mmap(etnaviv_obj, vma); >> } >> >> static vm_fault_t etnaviv_gem_fault(struct vm_fault *vmf) >> @@ -567,6 +558,7 @@ static const struct drm_gem_object_funcs etnaviv_gem_object_funcs = { >> .unpin = etnaviv_gem_prime_unpin, >> .get_sg_table = etnaviv_gem_prime_get_sg_table, >> .vmap = etnaviv_gem_prime_vmap, >> + .mmap = etnaviv_gem_mmap, >> .vm_ops = &vm_ops, >> }; >> >> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c b/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c >> index d741b1d735f7..6d8bed9c739d 100644 >> --- a/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c >> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c >> @@ -34,19 +34,6 @@ int etnaviv_gem_prime_vmap(struct drm_gem_object *obj, struct dma_buf_map *map) >> return 0; >> } >> >> -int etnaviv_gem_prime_mmap(struct drm_gem_object *obj, >> - struct vm_area_struct *vma) >> -{ >> - struct etnaviv_gem_object *etnaviv_obj = to_etnaviv_bo(obj); >> - int ret; >> - >> - ret = drm_gem_mmap_obj(obj, obj->size, vma); >> - if (ret < 0) >> - return ret; >> - >> - return etnaviv_obj->ops->mmap(etnaviv_obj, vma); >> -} >> - >> int etnaviv_gem_prime_pin(struct drm_gem_object *obj) >> { >> if (!obj->import_attach) { > >
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
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 >
Hi, will you review my patch? Best regards Thomas 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? > > Regards, > Lucas >
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> Thanks, I've tested this patch and applied to my etnaviv/next branch. Regards, Lucas > --- > 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; > > vm_page_prot = vm_get_page_prot(vma->vm_flags); > > @@ -154,19 +153,11 @@ static int etnaviv_gem_mmap_obj(struct etnaviv_gem_object *etnaviv_obj, > return 0; > } > > -int etnaviv_gem_mmap(struct file *filp, struct vm_area_struct *vma) > +static int etnaviv_gem_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma) > { > - struct etnaviv_gem_object *obj; > - int ret; > - > - ret = drm_gem_mmap(filp, vma); > - if (ret) { > - DBG("mmap failed: %d", ret); > - return ret; > - } > + struct etnaviv_gem_object *etnaviv_obj = to_etnaviv_bo(obj); > > - obj = to_etnaviv_bo(vma->vm_private_data); > - return obj->ops->mmap(obj, vma); > + return etnaviv_obj->ops->mmap(etnaviv_obj, vma); > } > > static vm_fault_t etnaviv_gem_fault(struct vm_fault *vmf) > @@ -567,6 +558,7 @@ static const struct drm_gem_object_funcs etnaviv_gem_object_funcs = { > .unpin = etnaviv_gem_prime_unpin, > .get_sg_table = etnaviv_gem_prime_get_sg_table, > .vmap = etnaviv_gem_prime_vmap, > + .mmap = etnaviv_gem_mmap, > .vm_ops = &vm_ops, > }; > > diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c b/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c > index d741b1d735f7..6d8bed9c739d 100644 > --- a/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c > +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c > @@ -34,19 +34,6 @@ int etnaviv_gem_prime_vmap(struct drm_gem_object *obj, struct dma_buf_map *map) > return 0; > } > > -int etnaviv_gem_prime_mmap(struct drm_gem_object *obj, > - struct vm_area_struct *vma) > -{ > - struct etnaviv_gem_object *etnaviv_obj = to_etnaviv_bo(obj); > - int ret; > - > - ret = drm_gem_mmap_obj(obj, obj->size, vma); > - if (ret < 0) > - return ret; > - > - return etnaviv_obj->ops->mmap(etnaviv_obj, vma); > -} > - > int etnaviv_gem_prime_pin(struct drm_gem_object *obj) > { > if (!obj->import_attach) {
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; vm_page_prot = vm_get_page_prot(vma->vm_flags); @@ -154,19 +153,11 @@ static int etnaviv_gem_mmap_obj(struct etnaviv_gem_object *etnaviv_obj, return 0; } -int etnaviv_gem_mmap(struct file *filp, struct vm_area_struct *vma) +static int etnaviv_gem_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma) { - struct etnaviv_gem_object *obj; - int ret; - - ret = drm_gem_mmap(filp, vma); - if (ret) { - DBG("mmap failed: %d", ret); - return ret; - } + struct etnaviv_gem_object *etnaviv_obj = to_etnaviv_bo(obj); - obj = to_etnaviv_bo(vma->vm_private_data); - return obj->ops->mmap(obj, vma); + return etnaviv_obj->ops->mmap(etnaviv_obj, vma); } static vm_fault_t etnaviv_gem_fault(struct vm_fault *vmf) @@ -567,6 +558,7 @@ static const struct drm_gem_object_funcs etnaviv_gem_object_funcs = { .unpin = etnaviv_gem_prime_unpin, .get_sg_table = etnaviv_gem_prime_get_sg_table, .vmap = etnaviv_gem_prime_vmap, + .mmap = etnaviv_gem_mmap, .vm_ops = &vm_ops, }; diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c b/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c index d741b1d735f7..6d8bed9c739d 100644 --- a/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c @@ -34,19 +34,6 @@ int etnaviv_gem_prime_vmap(struct drm_gem_object *obj, struct dma_buf_map *map) return 0; } -int etnaviv_gem_prime_mmap(struct drm_gem_object *obj, - struct vm_area_struct *vma) -{ - struct etnaviv_gem_object *etnaviv_obj = to_etnaviv_bo(obj); - int ret; - - ret = drm_gem_mmap_obj(obj, obj->size, vma); - if (ret < 0) - return ret; - - return etnaviv_obj->ops->mmap(etnaviv_obj, vma); -} - int etnaviv_gem_prime_pin(struct drm_gem_object *obj) { if (!obj->import_attach) {
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(-)