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@xxxxxxxxx>
> ---
> 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
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
