On Thu, Jul 19, 2018 at 03:18:03AM +0300, Haneen Mohammed wrote:
> This patch add the necessary functions to map/unmap GEM
> backing memory to the kernel's virtual address space.
>
> Signed-off-by: Haneen Mohammed <hamohammed.sa@xxxxxxxxx>
> ---
> Changes in v2:
> - add vkms_gem_vunmap
> - use vmap_count to guard against multiple prepare_fb calls on the same
> fb
> Changes in v3:
> - change vkms_gem_vmap to retrun int
> - cleanup if vmap failed
> - increment vmap_count after successful vmap
>
> drivers/gpu/drm/vkms/vkms_drv.h | 9 ++++
> drivers/gpu/drm/vkms/vkms_gem.c | 80 ++++++++++++++++++++++++++++++++-
> 2 files changed, 88 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
> index 07be29f2dc44..47048f707566 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.h
> +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> @@ -39,6 +39,8 @@ struct vkms_gem_object {
> struct drm_gem_object gem;
> struct mutex pages_lock; /* Page lock used in page fault handler */
> struct page **pages;
> + unsigned int vmap_count;
> + void *vaddr;
> };
>
> #define drm_crtc_to_vkms_output(target) \
> @@ -47,6 +49,9 @@ struct vkms_gem_object {
> #define drm_device_to_vkms_device(target) \
> container_of(target, struct vkms_device, drm)
>
> +#define drm_gem_to_vkms_gem(target)\
> + container_of(target, struct vkms_gem_object, gem)
> +
> /* CRTC */
> int vkms_crtc_init(struct drm_device *dev, struct drm_crtc *crtc,
> struct drm_plane *primary, struct drm_plane *cursor);
> @@ -75,4 +80,8 @@ int vkms_dumb_map(struct drm_file *file, struct drm_device *dev,
>
> void vkms_gem_free_object(struct drm_gem_object *obj);
>
> +int vkms_gem_vmap(struct drm_gem_object *obj);
> +
> +void vkms_gem_vunmap(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 c7e38368602b..2e55c906d9b2 100644
> --- a/drivers/gpu/drm/vkms/vkms_gem.c
> +++ b/drivers/gpu/drm/vkms/vkms_gem.c
> @@ -37,7 +37,9 @@ void vkms_gem_free_object(struct drm_gem_object *obj)
> struct vkms_gem_object *gem = container_of(obj, struct vkms_gem_object,
> gem);
>
> - kvfree(gem->pages);
> + WARN_ON(gem->pages);
> + WARN_ON(gem->vaddr);
> +
> mutex_destroy(&gem->pages_lock);
> drm_gem_object_release(obj);
> kfree(gem);
> @@ -177,3 +179,79 @@ int vkms_dumb_map(struct drm_file *file, struct drm_device *dev,
>
> return ret;
> }
> +
> +static struct page **_get_pages(struct vkms_gem_object *vkms_obj)
> +{
> + struct drm_gem_object *gem_obj = &vkms_obj->gem;
> +
> + if (!vkms_obj->pages) {
> + struct page **pages = drm_gem_get_pages(gem_obj);
> +
> + if (IS_ERR(pages))
> + return pages;
> +
> + if (cmpxchg(&vkms_obj->pages, NULL, pages))
> + drm_gem_put_pages(gem_obj, pages, false, true);
> + }
> +
> + return vkms_obj->pages;
> +}
> +
> +void vkms_gem_vunmap(struct drm_gem_object *obj)
> +{
> + struct vkms_gem_object *vkms_obj = drm_gem_to_vkms_gem(obj);
> +
> + mutex_lock(&vkms_obj->pages_lock);
> + if (vkms_obj->vmap_count < 1) {
> + WARN_ON(vkms_obj->vaddr);
> + WARN_ON(vkms_obj->pages);
> + mutex_unlock(&vkms_obj->pages_lock);
> + return;
> + }
> +
> + vkms_obj->vmap_count--;
> +
> + if (vkms_obj->vmap_count == 0) {
> + vunmap(vkms_obj->vaddr);
> + vkms_obj->vaddr = NULL;
> + drm_gem_put_pages(obj, vkms_obj->pages, false, true);
> + vkms_obj->pages = NULL;
> + }
> +
> + mutex_unlock(&vkms_obj->pages_lock);
> +}
> +
> +int vkms_gem_vmap(struct drm_gem_object *obj)
> +{
> + struct vkms_gem_object *vkms_obj = drm_gem_to_vkms_gem(obj);
> + int ret = 0;
> +
> + mutex_lock(&vkms_obj->pages_lock);
> +
> + if (!vkms_obj->vaddr) {
> + unsigned int n_pages = obj->size >> PAGE_SHIFT;
> + struct page **pages = _get_pages(vkms_obj);
> +
> + if (IS_ERR(pages)) {
> + ret = PTR_ERR(pages);
> + goto fail;
> + }
> +
> + vkms_obj->vaddr = vmap(pages, n_pages, VM_MAP, PAGE_KERNEL);
> + if (!vkms_obj->vaddr) {
> + ret = -ENOMEM;
> + drm_gem_put_pages(obj, vkms_obj->pages, false, true);
> + vkms_obj->pages = NULL;
> + goto fail;
> + }
> +
> + vkms_obj->vmap_count++;
> + }
> +
> + mutex_unlock(&vkms_obj->pages_lock);
> + return 0;
> +
> +fail:
Since fail does the same as the success path, there are 2 ways of handling this
that would be (IMO) better:
1- Rename fail to out and remove the redundant 2 lines above
2- Add a new label above fail called err_vmap and do the ret/put_pages/pages=NULL
cleanup there, dropping into the unlock and return after.
With either of these changes,
Reviewed-by: Sean Paul <seanpaul@xxxxxxxxxxxx>
Sean
> + mutex_unlock(&vkms_obj->pages_lock);
> + return ret;
> +}
> --
> 2.17.1
>
--
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
