Re: [PATCH 1/4] drm/vkms: Add functions to map GEM backing storage

On Mon, Jul 09, 2018 at 06:44:26PM +0300, Haneen Mohammed wrote:

This patch add the necessary functions to map GEM backing memory into the kernel's virtual address space.

Signed-off-by: Haneen Mohammed hamohammed.sa@gmail.com

drivers/gpu/drm/vkms/vkms_drv.c | 2 ++ drivers/gpu/drm/vkms/vkms_drv.h | 5 ++++ drivers/gpu/drm/vkms/vkms_gem.c | 50 +++++++++++++++++++++++++++++++++ 3 files changed, 57 insertions(+)

diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c index fe93f8c17997..e48c8eeb495a 100644 --- a/drivers/gpu/drm/vkms/vkms_drv.c +++ b/drivers/gpu/drm/vkms/vkms_drv.c @@ -52,9 +52,11 @@ static struct drm_driver vkms_driver = { .driver_features = DRIVER_MODESET | DRIVER_ATOMIC | DRIVER_GEM, .release = vkms_release, .fops = &vkms_driver_fops,

• .dumb_create = vkms_dumb_create, .dumb_map_offset = vkms_dumb_map, .gem_vm_ops = &vkms_gem_vm_ops,
• .gem_free_object_unlocked = vkms_gem_free_object,

This is a separate bugfix, fixing a fairly huge memory leak. I think it'd be good to catch this in the future, by adding a

else WARN(1, "no gem free callback, leaking memory\n");

to the end of drm_gem_object_free. Can you pls do that?

Also since this line here is a bugfix separate from the vmap support, can you pls split it out?

.name = DRIVER_NAME, .desc = DRIVER_DESC, diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h index 76f1720f81a5..d339a8108d85 100644 --- a/drivers/gpu/drm/vkms/vkms_drv.h +++ b/drivers/gpu/drm/vkms/vkms_drv.h @@ -35,6 +35,7 @@ struct vkms_gem_object { struct drm_gem_object gem; struct mutex pages_lock; /* Page lock used in page fault handler */ struct page **pages;

• void *vaddr;

};

int vkms_crtc_init(struct drm_device *dev, struct drm_crtc *crtc, @@ -58,4 +59,8 @@ int vkms_dumb_create(struct drm_file *file, struct drm_device *dev, int vkms_dumb_map(struct drm_file *file, struct drm_device *dev, u32 handle, u64 *offset);

+void vkms_gem_free_object(struct drm_gem_object *obj);

+void *vkms_gem_vmap(struct drm_gem_object *obj);

#endif /* _VKMS_DRV_H_ */ diff --git a/drivers/gpu/drm/vkms/vkms_gem.c b/drivers/gpu/drm/vkms/vkms_gem.c index 9f820f56b9e0..249855dded63 100644 --- a/drivers/gpu/drm/vkms/vkms_gem.c +++ b/drivers/gpu/drm/vkms/vkms_gem.c @@ -166,3 +166,53 @@ int vkms_dumb_map(struct drm_file *file, struct drm_device *dev,

return ret; }

+void vkms_gem_free_object(struct drm_gem_object *obj) +{

• struct vkms_gem_object *vkms_obj = container_of(obj,

• 					struct vkms_gem_object,
  

• 					gem);
  

• kvfree(vkms_obj->pages);

We need to put the pages here too if ->pages exists, there's a put_pages helper for that.

Also probably a good idea to vunmap here too.

But I think both cases (i.e. vmap not yet released and pages still around) would indicate a bug in the vkms driver of releasing the object while it's still in use somewhere. So maybe also add a

WARN_ON(obj->pages); WARN_ON(obj->vmap);

here to make sure this doesn't happen? -Daniel

• mutex_destroy(&vkms_obj->pages_lock);
• drm_gem_object_release(obj);
• kfree(vkms_obj);

+}

+struct page **get_pages(struct vkms_gem_object *vkms_obj) +{

• struct drm_gem_object *gem_obj = &vkms_obj->gem;
• struct page **pages = vkms_obj->pages;
• if (!pages) {

• mutex_lock(&vkms_obj->pages_lock);
  

• pages = drm_gem_get_pages(gem_obj);
  

• if (IS_ERR(pages)) {
  

• 	mutex_unlock(&vkms_obj->pages_lock);
  

• 	return pages;
  

• }
  

• vkms_obj->pages = pages;
  

• mutex_unlock(&vkms_obj->pages_lock);
  

• }
• return pages;

+}

+void *vkms_gem_vmap(struct drm_gem_object *obj) +{

• void *vaddr = NULL;
• struct vkms_gem_object *vkms_obj = container_of(obj,

• 					struct vkms_gem_object,
  

• 					gem);
  

• unsigned int n_pages = obj->size >> PAGE_SHIFT;
• struct page **pages = get_pages(vkms_obj);
• if (IS_ERR(pages)) {

• DRM_INFO("pages allocation failed %ld\n", PTR_ERR(pages));
  

• return vaddr;
  

• }
• vaddr = vmap(pages, n_pages, VM_MAP, PAGE_KERNEL);
• return vaddr;

+}

2.17.1