[PATCH 0/2] Default to cachable mappings for GEM SHMEM

[PATCH 0/2] Default to cachable mappings for GEM SHMEM

[Thomas Zimmermann]

This patchset complements Daniel's recent patches for SHMEM. [1]

By default SHMEM maps pages using writecombine. Only virtio sets the
SHMEM implmentation to use cached mappings. Udl implements its own
vmap/mmap functions, which always maps pages with caching.

Unify the settings by switching the SHMEM code to use cached mappings
(i.e., PAGE_KERNEL actually). The exception is dma-buf imported pages,
which are still mapped using writecombine.

Tested with udl-driven hardware.

[1] https://lists.freedesktop.org/archives/dri-devel/2020-May/265468.html

Thomas Zimmermann (2):
  drm/shmem: Use cached mappings by default
  drm/udl: Use GEM vmap/mmap function from SHMEM helpers

 drivers/gpu/drm/drm_gem_shmem_helper.c  |   6 +-
 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 ------------------------
 drivers/gpu/drm/virtio/virtgpu_object.c |   1 -
 include/drm/drm_gem_shmem_helper.h      |   4 +-
 7 files changed, 7 insertions(+), 118 deletions(-)
 delete mode 100644 drivers/gpu/drm/udl/udl_gem.c

--
2.26.2

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

[PATCH 1/2] drm/shmem: Use cached mappings by default

[Thomas Zimmermann]

SHMEM-buffer backing storage is allocated from system memory; which is
typically cachable. Currently, only virtio uses cachable mappings; udl
uses its own vmap/mmap implementation for cachable mappings. Other
drivers default to writecombine mappings.

Use cached mappings by default. The exception is pages imported via
dma-buf. DMA memory is usually not cached.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/drm_gem_shmem_helper.c  | 6 ++++--
 drivers/gpu/drm/virtio/virtgpu_object.c | 1 -
 include/drm/drm_gem_shmem_helper.h      | 4 ++--
 3 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
index df31e5782eed1..1ce90325dfa31 100644
--- a/drivers/gpu/drm/drm_gem_shmem_helper.c
+++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
@@ -259,7 +259,7 @@ static void *drm_gem_shmem_vmap_locked(struct drm_gem_shmem_object *shmem)
 	} else {
 		pgprot_t prot = PAGE_KERNEL;
 
-		if (!shmem->map_cached)
+		if (shmem->map_wc)
 			prot = pgprot_writecombine(prot);
 		shmem->vaddr = vmap(shmem->pages, obj->size >> PAGE_SHIFT,
 				    VM_MAP, prot);
@@ -546,7 +546,7 @@ int drm_gem_shmem_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma)
 
 	vma->vm_flags |= VM_MIXEDMAP | VM_DONTEXPAND;
 	vma->vm_page_prot = vm_get_page_prot(vma->vm_flags);
-	if (!shmem->map_cached)
+	if (shmem->map_wc)
 		vma->vm_page_prot = pgprot_writecombine(vma->vm_page_prot);
 	vma->vm_ops = &drm_gem_shmem_vm_ops;
 
@@ -664,6 +664,8 @@ drm_gem_shmem_prime_import_sg_table(struct drm_device *dev,
 	if (IS_ERR(shmem))
 		return ERR_CAST(shmem);
 
+	shmem->map_wc = false; /* dma-buf mappings use writecombine */
+
 	shmem->pages = kvmalloc_array(npages, sizeof(struct page *), GFP_KERNEL);
 	if (!shmem->pages) {
 		ret = -ENOMEM;
diff --git a/drivers/gpu/drm/virtio/virtgpu_object.c b/drivers/gpu/drm/virtio/virtgpu_object.c
index 6ccbd01cd888c..80ba6b2b61668 100644
--- a/drivers/gpu/drm/virtio/virtgpu_object.c
+++ b/drivers/gpu/drm/virtio/virtgpu_object.c
@@ -132,7 +132,6 @@ struct drm_gem_object *virtio_gpu_create_object(struct drm_device *dev,
 
 	dshmem = &shmem->base.base;
 	dshmem->base.funcs = &virtio_gpu_shmem_funcs;
-	dshmem->map_cached = true;
 	return &dshmem->base;
 }
 
diff --git a/include/drm/drm_gem_shmem_helper.h b/include/drm/drm_gem_shmem_helper.h
index 294b2931c4cc0..a5bc082a77c48 100644
--- a/include/drm/drm_gem_shmem_helper.h
+++ b/include/drm/drm_gem_shmem_helper.h
@@ -98,9 +98,9 @@ struct drm_gem_shmem_object {
 	unsigned int vmap_use_count;
 
 	/**
-	 * @map_cached: map object cached (instead of using writecombine).
+	 * @map_wc: map object using writecombine (instead of cached).
 	 */
-	bool map_cached;
+	bool map_wc;
 };
 
 #define to_drm_gem_shmem_obj(obj) \
-- 
2.26.2

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

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

[Thomas Zimmermann]

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

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

Re: [PATCH 1/2] drm/shmem: Use cached mappings by default

[Thomas Zimmermann]

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



Am 13.05.20 um 17:03 schrieb Thomas Zimmermann:
> SHMEM-buffer backing storage is allocated from system memory; which is
> typically cachable. Currently, only virtio uses cachable mappings; udl
> uses its own vmap/mmap implementation for cachable mappings. Other
> drivers default to writecombine mappings.
> 
> Use cached mappings by default. The exception is pages imported via
> dma-buf. DMA memory is usually not cached.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>  drivers/gpu/drm/drm_gem_shmem_helper.c  | 6 ++++--
>  drivers/gpu/drm/virtio/virtgpu_object.c | 1 -
>  include/drm/drm_gem_shmem_helper.h      | 4 ++--
>  3 files changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
> index df31e5782eed1..1ce90325dfa31 100644
> --- a/drivers/gpu/drm/drm_gem_shmem_helper.c
> +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
> @@ -259,7 +259,7 @@ static void *drm_gem_shmem_vmap_locked(struct drm_gem_shmem_object *shmem)
>  	} else {
>  		pgprot_t prot = PAGE_KERNEL;
>  
> -		if (!shmem->map_cached)
> +		if (shmem->map_wc)
>  			prot = pgprot_writecombine(prot);
>  		shmem->vaddr = vmap(shmem->pages, obj->size >> PAGE_SHIFT,
>  				    VM_MAP, prot);
> @@ -546,7 +546,7 @@ int drm_gem_shmem_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma)
>  
>  	vma->vm_flags |= VM_MIXEDMAP | VM_DONTEXPAND;
>  	vma->vm_page_prot = vm_get_page_prot(vma->vm_flags);
> -	if (!shmem->map_cached)
> +	if (shmem->map_wc)
>  		vma->vm_page_prot = pgprot_writecombine(vma->vm_page_prot);
>  	vma->vm_ops = &drm_gem_shmem_vm_ops;
>  
> @@ -664,6 +664,8 @@ drm_gem_shmem_prime_import_sg_table(struct drm_device *dev,
>  	if (IS_ERR(shmem))
>  		return ERR_CAST(shmem);
>  
> +	shmem->map_wc = false; /* dma-buf mappings use writecombine */

Just when I posted the patch, I saw this bug. map_wc should be set to
true to enable writecombine mappings for dma-buf pages. Will be fixed in
the next iteration.

> +
>  	shmem->pages = kvmalloc_array(npages, sizeof(struct page *), GFP_KERNEL);
>  	if (!shmem->pages) {
>  		ret = -ENOMEM;
> diff --git a/drivers/gpu/drm/virtio/virtgpu_object.c b/drivers/gpu/drm/virtio/virtgpu_object.c
> index 6ccbd01cd888c..80ba6b2b61668 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_object.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_object.c
> @@ -132,7 +132,6 @@ struct drm_gem_object *virtio_gpu_create_object(struct drm_device *dev,
>  
>  	dshmem = &shmem->base.base;
>  	dshmem->base.funcs = &virtio_gpu_shmem_funcs;
> -	dshmem->map_cached = true;
>  	return &dshmem->base;
>  }
>  
> diff --git a/include/drm/drm_gem_shmem_helper.h b/include/drm/drm_gem_shmem_helper.h
> index 294b2931c4cc0..a5bc082a77c48 100644
> --- a/include/drm/drm_gem_shmem_helper.h
> +++ b/include/drm/drm_gem_shmem_helper.h
> @@ -98,9 +98,9 @@ struct drm_gem_shmem_object {
>  	unsigned int vmap_use_count;
>  
>  	/**
> -	 * @map_cached: map object cached (instead of using writecombine).
> +	 * @map_wc: map object using writecombine (instead of cached).
>  	 */
> -	bool map_cached;
> +	bool map_wc;
>  };
>  
>  #define to_drm_gem_shmem_obj(obj) \
> 

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

Re: [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@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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Thomas Zimmermann]

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

Re: [PATCH 1/2] drm/shmem: Use cached mappings by default

[Daniel Vetter]

On Wed, May 13, 2020 at 05:03:11PM +0200, Thomas Zimmermann wrote:
> SHMEM-buffer backing storage is allocated from system memory; which is
> typically cachable. Currently, only virtio uses cachable mappings; udl
> uses its own vmap/mmap implementation for cachable mappings. Other
> drivers default to writecombine mappings.

I'm pretty sure this breaks all these drivers. quick grep on a few
functions says this is used by lima, panfrost, v3d. And they definitely
need uncached/wc stuff afaiui. Or I'm completely missing something?
-Daniel

> 
> Use cached mappings by default. The exception is pages imported via
> dma-buf. DMA memory is usually not cached.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>  drivers/gpu/drm/drm_gem_shmem_helper.c  | 6 ++++--
>  drivers/gpu/drm/virtio/virtgpu_object.c | 1 -
>  include/drm/drm_gem_shmem_helper.h      | 4 ++--
>  3 files changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
> index df31e5782eed1..1ce90325dfa31 100644
> --- a/drivers/gpu/drm/drm_gem_shmem_helper.c
> +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
> @@ -259,7 +259,7 @@ static void *drm_gem_shmem_vmap_locked(struct drm_gem_shmem_object *shmem)
>  	} else {
>  		pgprot_t prot = PAGE_KERNEL;
>  
> -		if (!shmem->map_cached)
> +		if (shmem->map_wc)
>  			prot = pgprot_writecombine(prot);
>  		shmem->vaddr = vmap(shmem->pages, obj->size >> PAGE_SHIFT,
>  				    VM_MAP, prot);
> @@ -546,7 +546,7 @@ int drm_gem_shmem_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma)
>  
>  	vma->vm_flags |= VM_MIXEDMAP | VM_DONTEXPAND;
>  	vma->vm_page_prot = vm_get_page_prot(vma->vm_flags);
> -	if (!shmem->map_cached)
> +	if (shmem->map_wc)
>  		vma->vm_page_prot = pgprot_writecombine(vma->vm_page_prot);
>  	vma->vm_ops = &drm_gem_shmem_vm_ops;
>  
> @@ -664,6 +664,8 @@ drm_gem_shmem_prime_import_sg_table(struct drm_device *dev,
>  	if (IS_ERR(shmem))
>  		return ERR_CAST(shmem);
>  
> +	shmem->map_wc = false; /* dma-buf mappings use writecombine */
> +
>  	shmem->pages = kvmalloc_array(npages, sizeof(struct page *), GFP_KERNEL);
>  	if (!shmem->pages) {
>  		ret = -ENOMEM;
> diff --git a/drivers/gpu/drm/virtio/virtgpu_object.c b/drivers/gpu/drm/virtio/virtgpu_object.c
> index 6ccbd01cd888c..80ba6b2b61668 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_object.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_object.c
> @@ -132,7 +132,6 @@ struct drm_gem_object *virtio_gpu_create_object(struct drm_device *dev,
>  
>  	dshmem = &shmem->base.base;
>  	dshmem->base.funcs = &virtio_gpu_shmem_funcs;
> -	dshmem->map_cached = true;
>  	return &dshmem->base;
>  }
>  
> diff --git a/include/drm/drm_gem_shmem_helper.h b/include/drm/drm_gem_shmem_helper.h
> index 294b2931c4cc0..a5bc082a77c48 100644
> --- a/include/drm/drm_gem_shmem_helper.h
> +++ b/include/drm/drm_gem_shmem_helper.h
> @@ -98,9 +98,9 @@ struct drm_gem_shmem_object {
>  	unsigned int vmap_use_count;
>  
>  	/**
> -	 * @map_cached: map object cached (instead of using writecombine).
> +	 * @map_wc: map object using writecombine (instead of cached).
>  	 */
> -	bool map_cached;
> +	bool map_wc;
>  };
>  
>  #define to_drm_gem_shmem_obj(obj) \
> -- 
> 2.26.2
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [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@suse.de>

Since I'm not sure how patch 1 works in this series I think keeping the
udl_driver_gem_create_object but just let it set the map_cached = true; is
all we need?
-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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] drm/shmem: Use cached mappings by default

[Thomas Zimmermann]

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

Hi

Am 14.05.20 um 14:40 schrieb Daniel Vetter:
> On Wed, May 13, 2020 at 05:03:11PM +0200, Thomas Zimmermann wrote:
>> SHMEM-buffer backing storage is allocated from system memory; which is
>> typically cachable. Currently, only virtio uses cachable mappings; udl
>> uses its own vmap/mmap implementation for cachable mappings. Other
>> drivers default to writecombine mappings.
> 
> I'm pretty sure this breaks all these drivers. quick grep on a few
> functions says this is used by lima, panfrost, v3d. And they definitely
> need uncached/wc stuff afaiui. Or I'm completely missing something?

OK, I better get some testing for this code. :D

> -Daniel
> 
>>
>> Use cached mappings by default. The exception is pages imported via
>> dma-buf. DMA memory is usually not cached.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
>>  drivers/gpu/drm/drm_gem_shmem_helper.c  | 6 ++++--
>>  drivers/gpu/drm/virtio/virtgpu_object.c | 1 -
>>  include/drm/drm_gem_shmem_helper.h      | 4 ++--
>>  3 files changed, 6 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
>> index df31e5782eed1..1ce90325dfa31 100644
>> --- a/drivers/gpu/drm/drm_gem_shmem_helper.c
>> +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
>> @@ -259,7 +259,7 @@ static void *drm_gem_shmem_vmap_locked(struct drm_gem_shmem_object *shmem)
>>  	} else {
>>  		pgprot_t prot = PAGE_KERNEL;
>>  
>> -		if (!shmem->map_cached)
>> +		if (shmem->map_wc)
>>  			prot = pgprot_writecombine(prot);
>>  		shmem->vaddr = vmap(shmem->pages, obj->size >> PAGE_SHIFT,
>>  				    VM_MAP, prot);
>> @@ -546,7 +546,7 @@ int drm_gem_shmem_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma)
>>  
>>  	vma->vm_flags |= VM_MIXEDMAP | VM_DONTEXPAND;
>>  	vma->vm_page_prot = vm_get_page_prot(vma->vm_flags);
>> -	if (!shmem->map_cached)
>> +	if (shmem->map_wc)
>>  		vma->vm_page_prot = pgprot_writecombine(vma->vm_page_prot);
>>  	vma->vm_ops = &drm_gem_shmem_vm_ops;
>>  
>> @@ -664,6 +664,8 @@ drm_gem_shmem_prime_import_sg_table(struct drm_device *dev,
>>  	if (IS_ERR(shmem))
>>  		return ERR_CAST(shmem);
>>  
>> +	shmem->map_wc = false; /* dma-buf mappings use writecombine */
>> +
>>  	shmem->pages = kvmalloc_array(npages, sizeof(struct page *), GFP_KERNEL);
>>  	if (!shmem->pages) {
>>  		ret = -ENOMEM;
>> diff --git a/drivers/gpu/drm/virtio/virtgpu_object.c b/drivers/gpu/drm/virtio/virtgpu_object.c
>> index 6ccbd01cd888c..80ba6b2b61668 100644
>> --- a/drivers/gpu/drm/virtio/virtgpu_object.c
>> +++ b/drivers/gpu/drm/virtio/virtgpu_object.c
>> @@ -132,7 +132,6 @@ struct drm_gem_object *virtio_gpu_create_object(struct drm_device *dev,
>>  
>>  	dshmem = &shmem->base.base;
>>  	dshmem->base.funcs = &virtio_gpu_shmem_funcs;
>> -	dshmem->map_cached = true;
>>  	return &dshmem->base;
>>  }
>>  
>> diff --git a/include/drm/drm_gem_shmem_helper.h b/include/drm/drm_gem_shmem_helper.h
>> index 294b2931c4cc0..a5bc082a77c48 100644
>> --- a/include/drm/drm_gem_shmem_helper.h
>> +++ b/include/drm/drm_gem_shmem_helper.h
>> @@ -98,9 +98,9 @@ struct drm_gem_shmem_object {
>>  	unsigned int vmap_use_count;
>>  
>>  	/**
>> -	 * @map_cached: map object cached (instead of using writecombine).
>> +	 * @map_wc: map object using writecombine (instead of cached).
>>  	 */
>> -	bool map_cached;
>> +	bool map_wc;
>>  };
>>  
>>  #define to_drm_gem_shmem_obj(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

Re: [PATCH 1/2] drm/shmem: Use cached mappings by default

[Rob Herring]

On Thu, May 14, 2020 at 7:40 AM Daniel Vetter <daniel@ffwll.ch> wrote:
>
> On Wed, May 13, 2020 at 05:03:11PM +0200, Thomas Zimmermann wrote:
> > SHMEM-buffer backing storage is allocated from system memory; which is
> > typically cachable. Currently, only virtio uses cachable mappings; udl
> > uses its own vmap/mmap implementation for cachable mappings. Other
> > drivers default to writecombine mappings.
>
> I'm pretty sure this breaks all these drivers. quick grep on a few
> functions says this is used by lima, panfrost, v3d. And they definitely
> need uncached/wc stuff afaiui. Or I'm completely missing something?

Yes, that would be my guess. DMA is usually non-coherent on Arm.

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

Re: [PATCH 1/2] drm/shmem: Use cached mappings by default

[Thomas Zimmermann]

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

Hi

Am 14.05.20 um 22:36 schrieb Rob Herring:
> On Thu, May 14, 2020 at 7:40 AM Daniel Vetter <daniel@ffwll.ch> wrote:
>>
>> On Wed, May 13, 2020 at 05:03:11PM +0200, Thomas Zimmermann wrote:
>>> SHMEM-buffer backing storage is allocated from system memory; which is
>>> typically cachable. Currently, only virtio uses cachable mappings; udl
>>> uses its own vmap/mmap implementation for cachable mappings. Other
>>> drivers default to writecombine mappings.
>>
>> I'm pretty sure this breaks all these drivers. quick grep on a few
>> functions says this is used by lima, panfrost, v3d. And they definitely
>> need uncached/wc stuff afaiui. Or I'm completely missing something?
> 

OK. I think I'll just make a patch that adds a .gem_create_object helper
that sets the map_cached flag. So drivers can opt-in.

> Yes, that would be my guess. DMA is usually non-coherent on Arm.

Can one of you give me some pointer to what you're looking at? For
example, I grepped for use of vmap within lima and only found [1].
That's a for memcpy with no DMA involved. There's got to be something
I'm missing.

Best regards
Thomas

[1]
https://cgit.freedesktop.org/drm/drm-tip/tree/drivers/gpu/drm/lima/lima_sched.c#n391

> 
> Rob
> 

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

Re: [PATCH 1/2] drm/shmem: Use cached mappings by default

[Daniel Vetter]

On Fri, May 15, 2020 at 08:58:02AM +0200, Thomas Zimmermann wrote:
> Hi
> 
> Am 14.05.20 um 22:36 schrieb Rob Herring:
> > On Thu, May 14, 2020 at 7:40 AM Daniel Vetter <daniel@ffwll.ch> wrote:
> >>
> >> On Wed, May 13, 2020 at 05:03:11PM +0200, Thomas Zimmermann wrote:
> >>> SHMEM-buffer backing storage is allocated from system memory; which is
> >>> typically cachable. Currently, only virtio uses cachable mappings; udl
> >>> uses its own vmap/mmap implementation for cachable mappings. Other
> >>> drivers default to writecombine mappings.
> >>
> >> I'm pretty sure this breaks all these drivers. quick grep on a few
> >> functions says this is used by lima, panfrost, v3d. And they definitely
> >> need uncached/wc stuff afaiui. Or I'm completely missing something?
> > 
> 
> OK. I think I'll just make a patch that adds a .gem_create_object helper
> that sets the map_cached flag. So drivers can opt-in.
> 
> > Yes, that would be my guess. DMA is usually non-coherent on Arm.
> 
> Can one of you give me some pointer to what you're looking at? For
> example, I grepped for use of vmap within lima and only found [1].
> That's a for memcpy with no DMA involved. There's got to be something
> I'm missing.
> 
> Best regards
> Thomas
> 
> [1]
> https://cgit.freedesktop.org/drm/drm-tip/tree/drivers/gpu/drm/lima/lima_sched.c#n391

$ git grep drm_gem_shmem_mmap

We also need correct access from userspace, otherwise the gpu is going to
be sad.
-Daniel
> 
> > 
> > Rob
> > 
> 
> -- 
> 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
> 




-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] drm/shmem: Use cached mappings by default

[Thomas Zimmermann]

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

Hi

Am 15.05.20 um 16:10 schrieb Daniel Vetter:
> On Fri, May 15, 2020 at 08:58:02AM +0200, Thomas Zimmermann wrote:
>> Hi
>>
>> Am 14.05.20 um 22:36 schrieb Rob Herring:
>>> On Thu, May 14, 2020 at 7:40 AM Daniel Vetter <daniel@ffwll.ch> wrote:
>>>>
>>>> On Wed, May 13, 2020 at 05:03:11PM +0200, Thomas Zimmermann wrote:
>>>>> SHMEM-buffer backing storage is allocated from system memory; which is
>>>>> typically cachable. Currently, only virtio uses cachable mappings; udl
>>>>> uses its own vmap/mmap implementation for cachable mappings. Other
>>>>> drivers default to writecombine mappings.
>>>>
>>>> I'm pretty sure this breaks all these drivers. quick grep on a few
>>>> functions says this is used by lima, panfrost, v3d. And they definitely
>>>> need uncached/wc stuff afaiui. Or I'm completely missing something?
>>>
>>
>> OK. I think I'll just make a patch that adds a .gem_create_object helper
>> that sets the map_cached flag. So drivers can opt-in.
>>
>>> Yes, that would be my guess. DMA is usually non-coherent on Arm.
>>
>> Can one of you give me some pointer to what you're looking at? For
>> example, I grepped for use of vmap within lima and only found [1].
>> That's a for memcpy with no DMA involved. There's got to be something
>> I'm missing.
>>
>> Best regards
>> Thomas
>>
>> [1]
>> https://cgit.freedesktop.org/drm/drm-tip/tree/drivers/gpu/drm/lima/lima_sched.c#n391
> 
> $ git grep drm_gem_shmem_mmap
> 
> We also need correct access from userspace, otherwise the gpu is going to
> be sad.

I've been thinking about this, and I think it means that we can never
have cached mappings anywhere. Even if shmem supports it internally for
most drivers, as soon as the page are exported, the importer could
expect uncached memory.

Given that, I think the correct thing to do is to udl's gem code and use
the default implementation.

Best regards
Thomas


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

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

Re: [PATCH 1/2] drm/shmem: Use cached mappings by default

[Gerd Hoffmann]

> > $ git grep drm_gem_shmem_mmap
> > 
> > We also need correct access from userspace, otherwise the gpu is going to
> > be sad.
> 
> I've been thinking about this, and I think it means that we can never
> have cached mappings anywhere. Even if shmem supports it internally for
> most drivers, as soon as the page are exported, the importer could
> expect uncached memory.

The importer should not expect anything but call dma-buf ops so the
exporter has a chance to handle this correctly.

(Yes, we don't get this right everywhere, some drivers take the dma-bufs
list of pages and do their own thing ...).

take care,
  Gerd

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

Re: [PATCH 1/2] drm/shmem: Use cached mappings by default

[Thomas Zimmermann]

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

Hi Gerd

Am 18.05.20 um 10:23 schrieb Gerd Hoffmann:
>>> $ git grep drm_gem_shmem_mmap
>>>
>>> We also need correct access from userspace, otherwise the gpu is going to
>>> be sad.
>>
>> I've been thinking about this, and I think it means that we can never
>> have cached mappings anywhere. Even if shmem supports it internally for
>> most drivers, as soon as the page are exported, the importer could
>> expect uncached memory.
> 
> The importer should not expect anything but call dma-buf ops so the
> exporter has a chance to handle this correctly.

I have the following case in mind: Suppose the exporter maps cached
pages and the importer expects uncached pages for DMA. There is
map_dma_buf/unmap_dma_buf, which can implement a cache flush for the
cached pages. Is it guaranteed that the importer calls this around each
DMA operation? If it doesn't, it could miss later buffer updates from
the exporter. And there's apparently no explicit flush op in dma_buf_ops.

Best regards
Thomas

> 
> (Yes, we don't get this right everywhere, some drivers take the dma-bufs
> list of pages and do their own thing ...).
> 
> take care,
>   Gerd
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 

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

Re: [PATCH 1/2] drm/shmem: Use cached mappings by default

[Gerd Hoffmann]

On Mon, May 18, 2020 at 10:50:15AM +0200, Thomas Zimmermann wrote:
> Hi Gerd
> 
> Am 18.05.20 um 10:23 schrieb Gerd Hoffmann:
> >>> $ git grep drm_gem_shmem_mmap
> >>>
> >>> We also need correct access from userspace, otherwise the gpu is going to
> >>> be sad.
> >>
> >> I've been thinking about this, and I think it means that we can never
> >> have cached mappings anywhere. Even if shmem supports it internally for
> >> most drivers, as soon as the page are exported, the importer could
> >> expect uncached memory.
> > 
> > The importer should not expect anything but call dma-buf ops so the
> > exporter has a chance to handle this correctly.
> 
> I have the following case in mind: Suppose the exporter maps cached
> pages and the importer expects uncached pages for DMA. There is
> map_dma_buf/unmap_dma_buf, which can implement a cache flush for the
> cached pages. Is it guaranteed that the importer calls this around each
> DMA operation?

I think the importer is supposed to do that, but I wouldn't surprised if
there are cases in tree where this isn't implemented correctly ...

take care,
  Gerd

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

Re: [PATCH 1/2] drm/shmem: Use cached mappings by default

[Daniel Vetter]

On Mon, May 18, 2020 at 12:11:32PM +0200, Gerd Hoffmann wrote:
> On Mon, May 18, 2020 at 10:50:15AM +0200, Thomas Zimmermann wrote:
> > Hi Gerd
> > 
> > Am 18.05.20 um 10:23 schrieb Gerd Hoffmann:
> > >>> $ git grep drm_gem_shmem_mmap
> > >>>
> > >>> We also need correct access from userspace, otherwise the gpu is going to
> > >>> be sad.
> > >>
> > >> I've been thinking about this, and I think it means that we can never
> > >> have cached mappings anywhere. Even if shmem supports it internally for
> > >> most drivers, as soon as the page are exported, the importer could
> > >> expect uncached memory.
> > > 
> > > The importer should not expect anything but call dma-buf ops so the
> > > exporter has a chance to handle this correctly.
> > 
> > I have the following case in mind: Suppose the exporter maps cached
> > pages and the importer expects uncached pages for DMA. There is
> > map_dma_buf/unmap_dma_buf, which can implement a cache flush for the
> > cached pages. Is it guaranteed that the importer calls this around each
> > DMA operation?
> 
> I think the importer is supposed to do that, but I wouldn't surprised if
> there are cases in tree where this isn't implemented correctly ...

Yup, this is very much a case of "supposed to" but "in practice, many
actually dont". The reason is that setting up mappings is expensive, so
best avoid.

We filled that gap a few years after dma-buf landed with the
begin/end_cpu_access hooks, which allow the exporter to do cache flushing
(using something like dma_sync_sg_for_device/cpu) and for this to all work
properly. We even added ioctl so that the mmap on the dma-buf works
correctly.

But most importers still ignore this, so it's all fail :-/ But in theory
the pieces to make cached mappings work over dma-buf, even for importers
that need uncached, are all there.

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] drm/shmem: Use cached mappings by default

[Thomas Zimmermann]

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

Hi

Am 18.05.20 um 16:40 schrieb Daniel Vetter:
> On Mon, May 18, 2020 at 12:11:32PM +0200, Gerd Hoffmann wrote:
>> On Mon, May 18, 2020 at 10:50:15AM +0200, Thomas Zimmermann wrote:
>>> Hi Gerd
>>>
>>> Am 18.05.20 um 10:23 schrieb Gerd Hoffmann:
>>>>>> $ git grep drm_gem_shmem_mmap
>>>>>>
>>>>>> We also need correct access from userspace, otherwise the gpu is going to
>>>>>> be sad.
>>>>>
>>>>> I've been thinking about this, and I think it means that we can never
>>>>> have cached mappings anywhere. Even if shmem supports it internally for
>>>>> most drivers, as soon as the page are exported, the importer could
>>>>> expect uncached memory.
>>>>
>>>> The importer should not expect anything but call dma-buf ops so the
>>>> exporter has a chance to handle this correctly.
>>>
>>> I have the following case in mind: Suppose the exporter maps cached
>>> pages and the importer expects uncached pages for DMA. There is
>>> map_dma_buf/unmap_dma_buf, which can implement a cache flush for the
>>> cached pages. Is it guaranteed that the importer calls this around each
>>> DMA operation?
>>
>> I think the importer is supposed to do that, but I wouldn't surprised if
>> there are cases in tree where this isn't implemented correctly ...
> 
> Yup, this is very much a case of "supposed to" but "in practice, many
> actually dont". The reason is that setting up mappings is expensive, so
> best avoid.

Thanks to both of you for answering the question.

> 
> We filled that gap a few years after dma-buf landed with the
> begin/end_cpu_access hooks, which allow the exporter to do cache flushing
> (using something like dma_sync_sg_for_device/cpu) and for this to all work
> properly. We even added ioctl so that the mmap on the dma-buf works
> correctly.
> 
> But most importers still ignore this, so it's all fail :-/ But in theory
> the pieces to make cached mappings work over dma-buf, even for importers
> that need uncached, are all there.
> 
> Cheers, Daniel
> 

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

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

[Thomas Zimmermann]

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

Hi

Am 14.05.20 um 14:44 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>
> 
> Since I'm not sure how patch 1 works in this series I think keeping the
> udl_driver_gem_create_object but just let it set the map_cached = true; is
> all we need?

Yes. It tested this localy and it worked. The create function can simply
go into udl_drv.c. I'll send out a patch later.

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