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

[Thomas Zimmermann <tzimmermann@suse.de>]

[-- Attachment #1.1.1: Type: text/plain, Size: 6568 bytes --]

Hi Daniel

Am 13.05.20 um 17:49 schrieb 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@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.

With these patches, dma-buf imports get the writecombine mapping
unconditionally. I don't know if that is enough, but at least it's the
old behavior.

> 
> 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.

Great. I'll take another look at your patch series tomorrow.

Best regards
Thomas

> -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
>>
> 

-- 
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


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel