[PATCH 2/2] drm/udl: Use GEM vmap/mmap function from SHMEM helpers

[Daniel Vetter]

On Wed, May 13, 2020 at 05:03:12PM +0200, Thomas Zimmermann wrote:
> The udl driver contains an implementation of GEM vmap and mmap
> operations that is identical to the common SHMEM helper; except
> that udl's code does not support writecombine mappings.
> 
> Convert udl to regular SHMEM helper functions. There's no reason
> to have udl behave differently from all other SHMEM drivers.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann at suse.de>

So I remember the problem again I think ... on some x86 gpu drivers
(*cough* i915 on specific chipsets) you get an uncached dma-buf.

Which means if you set up a cached mapping, you get corruption.

But if the shmem helpers to correctly forward _all_ calls to the dma-buf
functions of the exporter, this works. I've tried to clean up that a bit
with my patch series that I just posted this week, to make sure there's no
bugs like that.

I think once we have that fully sorted, we could land this and be happy.
-Daniel

> ---
>  drivers/gpu/drm/udl/Makefile  |   2 +-
>  drivers/gpu/drm/udl/udl_drv.c |   3 -
>  drivers/gpu/drm/udl/udl_drv.h |   3 -
>  drivers/gpu/drm/udl/udl_gem.c | 106 ----------------------------------
>  4 files changed, 1 insertion(+), 113 deletions(-)
>  delete mode 100644 drivers/gpu/drm/udl/udl_gem.c
> 
> diff --git a/drivers/gpu/drm/udl/Makefile b/drivers/gpu/drm/udl/Makefile
> index b50179bb4de06..24d61f61d7db2 100644
> --- a/drivers/gpu/drm/udl/Makefile
> +++ b/drivers/gpu/drm/udl/Makefile
> @@ -1,4 +1,4 @@
>  # SPDX-License-Identifier: GPL-2.0-only
> -udl-y := udl_drv.o udl_modeset.o udl_connector.o udl_main.o udl_transfer.o udl_gem.o
> +udl-y := udl_drv.o udl_modeset.o udl_connector.o udl_main.o udl_transfer.o
>  
>  obj-$(CONFIG_DRM_UDL) := udl.o
> diff --git a/drivers/gpu/drm/udl/udl_drv.c b/drivers/gpu/drm/udl/udl_drv.c
> index d1aa50fd6d65a..cf5b679bf58bb 100644
> --- a/drivers/gpu/drm/udl/udl_drv.c
> +++ b/drivers/gpu/drm/udl/udl_drv.c
> @@ -37,9 +37,6 @@ DEFINE_DRM_GEM_FOPS(udl_driver_fops);
>  static struct drm_driver driver = {
>  	.driver_features = DRIVER_ATOMIC | DRIVER_GEM | DRIVER_MODESET,
>  
> -	/* gem hooks */
> -	.gem_create_object = udl_driver_gem_create_object,
> -
>  	.fops = &udl_driver_fops,
>  	DRM_GEM_SHMEM_DRIVER_OPS,
>  
> diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h
> index 2642f94a63fc8..b1461f30780bc 100644
> --- a/drivers/gpu/drm/udl/udl_drv.h
> +++ b/drivers/gpu/drm/udl/udl_drv.h
> @@ -81,9 +81,6 @@ int udl_render_hline(struct drm_device *dev, int log_bpp, struct urb **urb_ptr,
>  		     const char *front, char **urb_buf_ptr,
>  		     u32 byte_offset, u32 device_byte_offset, u32 byte_width);
>  
> -struct drm_gem_object *udl_driver_gem_create_object(struct drm_device *dev,
> -						    size_t size);
> -
>  int udl_drop_usb(struct drm_device *dev);
>  
>  #define CMD_WRITE_RAW8   "\xAF\x60" /**< 8 bit raw write command. */
> diff --git a/drivers/gpu/drm/udl/udl_gem.c b/drivers/gpu/drm/udl/udl_gem.c
> deleted file mode 100644
> index b6e26f98aa0af..0000000000000
> --- a/drivers/gpu/drm/udl/udl_gem.c
> +++ /dev/null
> @@ -1,106 +0,0 @@
> -// SPDX-License-Identifier: GPL-2.0-only
> -/*
> - * Copyright (C) 2012 Red Hat
> - */
> -
> -#include <linux/dma-buf.h>
> -#include <linux/vmalloc.h>
> -
> -#include <drm/drm_drv.h>
> -#include <drm/drm_gem_shmem_helper.h>
> -#include <drm/drm_mode.h>
> -#include <drm/drm_prime.h>
> -
> -#include "udl_drv.h"
> -
> -/*
> - * GEM object funcs
> - */
> -
> -static int udl_gem_object_mmap(struct drm_gem_object *obj,
> -			       struct vm_area_struct *vma)
> -{
> -	int ret;
> -
> -	ret = drm_gem_shmem_mmap(obj, vma);
> -	if (ret)
> -		return ret;
> -
> -	vma->vm_page_prot = vm_get_page_prot(vma->vm_flags);
> -	if (obj->import_attach)
> -		vma->vm_page_prot = pgprot_writecombine(vma->vm_page_prot);
> -	vma->vm_page_prot = pgprot_decrypted(vma->vm_page_prot);
> -
> -	return 0;
> -}
> -
> -static void *udl_gem_object_vmap(struct drm_gem_object *obj)
> -{
> -	struct drm_gem_shmem_object *shmem = to_drm_gem_shmem_obj(obj);
> -	int ret;
> -
> -	ret = mutex_lock_interruptible(&shmem->vmap_lock);
> -	if (ret)
> -		return ERR_PTR(ret);
> -
> -	if (shmem->vmap_use_count++ > 0)
> -		goto out;
> -
> -	ret = drm_gem_shmem_get_pages(shmem);
> -	if (ret)
> -		goto err_zero_use;
> -
> -	if (obj->import_attach)
> -		shmem->vaddr = dma_buf_vmap(obj->import_attach->dmabuf);
> -	else
> -		shmem->vaddr = vmap(shmem->pages, obj->size >> PAGE_SHIFT,
> -				    VM_MAP, PAGE_KERNEL);
> -
> -	if (!shmem->vaddr) {
> -		DRM_DEBUG_KMS("Failed to vmap pages\n");
> -		ret = -ENOMEM;
> -		goto err_put_pages;
> -	}
> -
> -out:
> -	mutex_unlock(&shmem->vmap_lock);
> -	return shmem->vaddr;
> -
> -err_put_pages:
> -	drm_gem_shmem_put_pages(shmem);
> -err_zero_use:
> -	shmem->vmap_use_count = 0;
> -	mutex_unlock(&shmem->vmap_lock);
> -	return ERR_PTR(ret);
> -}
> -
> -static const struct drm_gem_object_funcs udl_gem_object_funcs = {
> -	.free = drm_gem_shmem_free_object,
> -	.print_info = drm_gem_shmem_print_info,
> -	.pin = drm_gem_shmem_pin,
> -	.unpin = drm_gem_shmem_unpin,
> -	.get_sg_table = drm_gem_shmem_get_sg_table,
> -	.vmap = udl_gem_object_vmap,
> -	.vunmap = drm_gem_shmem_vunmap,
> -	.mmap = udl_gem_object_mmap,
> -};
> -
> -/*
> - * Helpers for struct drm_driver
> - */
> -
> -struct drm_gem_object *udl_driver_gem_create_object(struct drm_device *dev,
> -						    size_t size)
> -{
> -	struct drm_gem_shmem_object *shmem;
> -	struct drm_gem_object *obj;
> -
> -	shmem = kzalloc(sizeof(*shmem), GFP_KERNEL);
> -	if (!shmem)
> -		return NULL;
> -
> -	obj = &shmem->base;
> -	obj->funcs = &udl_gem_object_funcs;
> -
> -	return obj;
> -}
> -- 
> 2.26.2
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch