Christian König
2021-Apr-16 13:51 UTC
[Nouveau] [PATCH v3 5/7] drm/vmwgfx: Inline ttm_bo_mmap() into vmwgfx driver
Am 16.04.21 um 15:46 schrieb Christian K?nig:> Am 16.04.21 um 15:31 schrieb Thomas Zimmermann: >> The vmwgfx driver is the only remaining user of ttm_bo_mmap(). Inline >> the code. The internal helper ttm_bo_vm_lookup() is now also part of >> vmwgfx as vmw_bo_vm_lookup(). >> >> v2: >> ????* replace pr_err() with drm_err() (Zack) >> >> Signed-off-by: Thomas Zimmermann <tzimmermann at suse.de> >> Reviewed-by: Zack Rusin <zackr at vmware.com> >> --- >> ? drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c | 56 ++++++++++++++++++++++-- >> ? 1 file changed, 53 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c >> b/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c >> index cb9975889e2f..c8b6543b4e39 100644 >> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c >> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c >> @@ -27,6 +27,32 @@ >> ? ? #include "vmwgfx_drv.h" >> ? +static struct ttm_buffer_object *vmw_bo_vm_lookup(struct >> ttm_device *bdev, >> +????????????????????????? unsigned long offset, >> +????????????????????????? unsigned long pages) >> +{ >> +??? struct vmw_private *dev_priv = container_of(bdev, struct >> vmw_private, bdev); >> +??? struct drm_device *drm = &dev_priv->drm; >> +??? struct drm_vma_offset_node *node; >> +??? struct ttm_buffer_object *bo = NULL; >> + >> +??? drm_vma_offset_lock_lookup(bdev->vma_manager); >> + >> +??? node = drm_vma_offset_lookup_locked(bdev->vma_manager, offset, >> pages); >> +??? if (likely(node)) { >> +??????? bo = container_of(node, struct ttm_buffer_object, >> +????????????????? base.vma_node); >> +??????? bo = ttm_bo_get_unless_zero(bo); >> +??? } >> + >> +??? drm_vma_offset_unlock_lookup(bdev->vma_manager); >> + >> +??? if (!bo) >> +??????? drm_err(drm, "Could not find buffer object to map\n"); >> + >> +??? return bo; >> +} >> + >> ? int vmw_mmap(struct file *filp, struct vm_area_struct *vma) >> ? { >> ????? static const struct vm_operations_struct vmw_vm_ops = { >> @@ -41,10 +67,28 @@ int vmw_mmap(struct file *filp, struct >> vm_area_struct *vma) >> ????? }; >> ????? struct drm_file *file_priv = filp->private_data; >> ????? struct vmw_private *dev_priv = vmw_priv(file_priv->minor->dev); >> -??? int ret = ttm_bo_mmap(filp, vma, &dev_priv->bdev); >> +??? struct ttm_device *bdev = &dev_priv->bdev; >> +??? struct ttm_buffer_object *bo; >> +??? int ret; >> + >> +??? if (unlikely(vma->vm_pgoff < DRM_FILE_PAGE_OFFSET_START)) >> +??????? return -EINVAL; >> + >> +??? bo = vmw_bo_vm_lookup(bdev, vma->vm_pgoff, vma_pages(vma)); >> +??? if (unlikely(!bo)) >> +??????? return -EINVAL; >> ? -??? if (ret) >> -??????? return ret; >> +??? if (unlikely(!bo->bdev->funcs->verify_access)) { >> +??????? ret = -EPERM; >> +??????? goto out_unref; >> +??? } >> +??? ret = bo->bdev->funcs->verify_access(bo, filp); > > Is there any reason we can't call vmw_verify_access() directly here? > > Would allow us to completely nuke the verify_access callback as well > as far as I can see.Forget what I said, couldn't see the next patch in my mailbox at time of writing. Whole series is Reviewed-by: Christian K?nig <christian.koenig at amd.com> Thanks for the nice cleanup, Christian.> > Regards, > Christian. > >> +??? if (unlikely(ret != 0)) >> +??????? goto out_unref; >> + >> +??? ret = ttm_bo_mmap_obj(vma, bo); >> +??? if (unlikely(ret != 0)) >> +??????? goto out_unref; >> ? ????? vma->vm_ops = &vmw_vm_ops; >> ? @@ -52,7 +96,13 @@ int vmw_mmap(struct file *filp, struct >> vm_area_struct *vma) >> ????? if (!is_cow_mapping(vma->vm_flags)) >> ????????? vma->vm_flags = (vma->vm_flags & ~VM_MIXEDMAP) | VM_PFNMAP; >> ? +??? ttm_bo_put(bo); /* release extra ref taken by >> ttm_bo_mmap_obj() */ >> + >> ????? return 0; >> + >> +out_unref: >> +??? ttm_bo_put(bo); >> +??? return ret; >> ? } >> ? ? /* struct vmw_validation_mem callback */ >
Thomas Zimmermann
2021-Apr-20 07:51 UTC
[Nouveau] [PATCH v3 5/7] drm/vmwgfx: Inline ttm_bo_mmap() into vmwgfx driver
Hi Am 16.04.21 um 15:51 schrieb Christian K?nig:> Am 16.04.21 um 15:46 schrieb Christian K?nig: >> Am 16.04.21 um 15:31 schrieb Thomas Zimmermann: >>> The vmwgfx driver is the only remaining user of ttm_bo_mmap(). Inline >>> the code. The internal helper ttm_bo_vm_lookup() is now also part of >>> vmwgfx as vmw_bo_vm_lookup(). >>> >>> v2: >>> ????* replace pr_err() with drm_err() (Zack) >>> >>> Signed-off-by: Thomas Zimmermann <tzimmermann at suse.de> >>> Reviewed-by: Zack Rusin <zackr at vmware.com> >>> --- >>> ? drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c | 56 ++++++++++++++++++++++-- >>> ? 1 file changed, 53 insertions(+), 3 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c >>> b/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c >>> index cb9975889e2f..c8b6543b4e39 100644 >>> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c >>> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c >>> @@ -27,6 +27,32 @@ >>> ? ? #include "vmwgfx_drv.h" >>> ? +static struct ttm_buffer_object *vmw_bo_vm_lookup(struct >>> ttm_device *bdev, >>> +????????????????????????? unsigned long offset, >>> +????????????????????????? unsigned long pages) >>> +{ >>> +??? struct vmw_private *dev_priv = container_of(bdev, struct >>> vmw_private, bdev); >>> +??? struct drm_device *drm = &dev_priv->drm; >>> +??? struct drm_vma_offset_node *node; >>> +??? struct ttm_buffer_object *bo = NULL; >>> + >>> +??? drm_vma_offset_lock_lookup(bdev->vma_manager); >>> + >>> +??? node = drm_vma_offset_lookup_locked(bdev->vma_manager, offset, >>> pages); >>> +??? if (likely(node)) { >>> +??????? bo = container_of(node,struct ttm_buffer_object,>>> +????????????????? base.vma_node); >>> +??????? bo = ttm_bo_get_unless_zero(bo); >>> +??? } >>> + >>> +??? drm_vma_offset_unlock_lookup(bdev->vma_manager); >>> + >>> +??? if (!bo) >>> +??????? drm_err(drm, "Could not find buffer object to map\n"); >>> + >>> +??? return bo; >>> +} >>> + >>> ? int vmw_mmap(struct file *filp, struct vm_area_struct *vma) >>> ? { >>> ????? static const struct vm_operations_struct vmw_vm_ops = { >>> @@ -41,10 +67,28 @@ int vmw_mmap(struct file *filp, struct >>> vm_area_struct *vma) >>> ????? }; >>> ????? struct drm_file *file_priv = filp->private_data; >>> ????? struct vmw_private *dev_priv = vmw_priv(file_priv->minor->dev); >>> -??? int ret = ttm_bo_mmap(filp, vma, &dev_priv->bdev); >>> +??? struct ttm_device *bdev = &dev_priv->bdev; >>> +??? struct ttm_buffer_object *bo; >>> +??? int ret; >>> + >>> +??? if (unlikely(vma->vm_pgoff < DRM_FILE_PAGE_OFFSET_START)) >>> +??????? return -EINVAL; >>> + >>> +??? bo = vmw_bo_vm_lookup(bdev, vma->vm_pgoff, vma_pages(vma)); >>> +??? if (unlikely(!bo)) >>> +??????? return -EINVAL; >>> ? -??? if (ret) >>> -??????? return ret; >>> +??? if (unlikely(!bo->bdev->funcs->verify_access)) { >>> +??????? ret = -EPERM; >>> +??????? goto out_unref; >>> +??? } >>> +??? ret = bo->bdev->funcs->verify_access(bo, filp); >> >> Is there any reason we can't call vmw_verify_access() directly here? >> >> Would allow us to completely nuke the verify_access callback as well >> as far as I can see. > > Forget what I said, couldn't see the next patch in my mailbox at time of > writing. > > Whole series is Reviewed-by: Christian K?nig <christian.koenig at amd.com>Thanks a lot. If I'm not mistaken, the patches at [1] need to go in first. So it could take a a bit until this lands. Otherwise, this series could go through the same tree as [1] if nouveau and vmwgfx devs don't mind. Best regards Thomas [1] https://patchwork.freedesktop.org/series/88822/> > Thanks for the nice cleanup, > Christian. > >> >> Regards, >> Christian. >> >>> +??? if (unlikely(ret != 0)) >>> +??????? goto out_unref; >>> + >>> +??? ret = ttm_bo_mmap_obj(vma, bo); >>> +??? if (unlikely(ret != 0)) >>> +??????? goto out_unref; >>> ? ????? vma->vm_ops = &vmw_vm_ops; >>> ? @@ -52,7 +96,13 @@ int vmw_mmap(struct file *filp, struct >>> vm_area_struct *vma) >>> ????? if (!is_cow_mapping(vma->vm_flags)) >>> ????????? vma->vm_flags = (vma->vm_flags & ~VM_MIXEDMAP) | VM_PFNMAP; >>> ? +??? ttm_bo_put(bo); /* release extra ref takenby>>> ttm_bo_mmap_obj() */ >>> + >>> ????? return 0; >>> + >>> +out_unref: >>> +??? ttm_bo_put(bo); >>> +??? return ret; >>> ? } >>> ? ? /* struct vmw_validation_mem callback */ >> > > _______________________________________________ > dri-devel mailing list > dri-devel at lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel-- 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 -------------- next part -------------- A non-text attachment was scrubbed... Name: OpenPGP_signature Type: application/pgp-signature Size: 840 bytes Desc: OpenPGP digital signature URL: <https://lists.freedesktop.org/archives/nouveau/attachments/20210420/e96853dd/attachment-0001.sig>