All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/5] drm/udl: Convert to generic fbdev emulation
@ 2019-10-24 14:42 Thomas Zimmermann
  2019-10-24 14:42 ` [PATCH 1/5] drm/udl: Clear BO vmapping pointer after unmapping BO memory Thomas Zimmermann
                   ` (4 more replies)
  0 siblings, 5 replies; 31+ messages in thread
From: Thomas Zimmermann @ 2019-10-24 14:42 UTC (permalink / raw)
  To: airlied, sean, daniel, sam, noralf; +Cc: Thomas Zimmermann, dri-devel

This patchset replaces udl's fbdev code with the generic implementation.

The first patch fixes a bug that didn't trigger yet because the current
fbdev never unmaps the BO. Patches 2 to 3 add missing interfaces to the
udl driver. Patch 4 sets mapping flags. In the final patch, we remove a
lot of code and set a few helpers instead.

The patchset was tested by running the fbdev console, X11, and Weston on
a DisplayLink adapter.

Thomas Zimmermann (5):
  drm/udl: Clear BO vmapping pointer after unmapping BO memory
  drm/udl: Set drm_driver.gem_prime_mmap
  drm/udl: Add GEM object functions for free(), vmap(), and vunmap()
  drm/udl: Map BO memory pages in unencrypted mode
  drm/udl: Replace fbdev code with generic emulation

 drivers/gpu/drm/udl/udl_drv.c     |   4 +
 drivers/gpu/drm/udl/udl_drv.h     |   4 -
 drivers/gpu/drm/udl/udl_fb.c      | 263 +-----------------------------
 drivers/gpu/drm/udl/udl_gem.c     |  40 ++++-
 drivers/gpu/drm/udl/udl_main.c    |   2 -
 drivers/gpu/drm/udl/udl_modeset.c |   3 +-
 6 files changed, 48 insertions(+), 268 deletions(-)

--
2.23.0

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

^ permalink raw reply	[flat|nested] 31+ messages in thread
* [PATCH 1/5] drm/udl: Clear BO vmapping pointer after unmapping BO memory
  2019-10-24 14:42 [PATCH 0/5] drm/udl: Convert to generic fbdev emulation Thomas Zimmermann
@ 2019-10-24 14:42 ` Thomas Zimmermann
  2019-10-25  7:39   ` Daniel Vetter
  2019-10-24 14:42 ` [PATCH 2/5] drm/udl: Set drm_driver.gem_prime_mmap Thomas Zimmermann
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 31+ messages in thread
From: Thomas Zimmermann @ 2019-10-24 14:42 UTC (permalink / raw)
  To: airlied, sean, daniel, sam, noralf; +Cc: Thomas Zimmermann, dri-devel

Unmapping the BO memory with udl_gem_vunmap() creates a dangling pointer
in struct udl_gem_object.vmapping. This can crash udl_handle_damage(),
which check the pointer's value for NULL. Clear the pointer to NULL and
let udl_handle_damage() re-establish the mapping if necessary.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/udl/udl_gem.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/udl/udl_gem.c b/drivers/gpu/drm/udl/udl_gem.c
index b23a5c2fcd80..3ea0cd9ae2d6 100644
--- a/drivers/gpu/drm/udl/udl_gem.c
+++ b/drivers/gpu/drm/udl/udl_gem.c
@@ -174,6 +174,7 @@ void udl_gem_vunmap(struct udl_gem_object *obj)
 	}
 
 	vunmap(obj->vmapping);
+	obj->vmapping = NULL;
 
 	udl_gem_put_pages(obj);
 }
-- 
2.23.0

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

^ permalink raw reply related	[flat|nested] 31+ messages in thread
* [PATCH 2/5] drm/udl: Set drm_driver.gem_prime_mmap
  2019-10-24 14:42 [PATCH 0/5] drm/udl: Convert to generic fbdev emulation Thomas Zimmermann
  2019-10-24 14:42 ` [PATCH 1/5] drm/udl: Clear BO vmapping pointer after unmapping BO memory Thomas Zimmermann
@ 2019-10-24 14:42 ` Thomas Zimmermann
  2019-10-24 14:42 ` [PATCH 3/5] drm/udl: Add GEM object functions for free(), vmap(), and vunmap() Thomas Zimmermann
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 31+ messages in thread
From: Thomas Zimmermann @ 2019-10-24 14:42 UTC (permalink / raw)
  To: airlied, sean, daniel, sam, noralf; +Cc: Thomas Zimmermann, dri-devel

This interface is required by the mmap support of generic
fbdev emulation.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/udl/udl_drv.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/udl/udl_drv.c b/drivers/gpu/drm/udl/udl_drv.c
index 8426669433e4..15ad7a338f9d 100644
--- a/drivers/gpu/drm/udl/udl_drv.c
+++ b/drivers/gpu/drm/udl/udl_drv.c
@@ -74,6 +74,7 @@ static struct drm_driver driver = {
 	.prime_fd_to_handle = drm_gem_prime_fd_to_handle,
 	.gem_prime_export = udl_gem_prime_export,
 	.gem_prime_import = udl_gem_prime_import,
+	.gem_prime_mmap = drm_gem_prime_mmap,
 
 	.name = DRIVER_NAME,
 	.desc = DRIVER_DESC,
-- 
2.23.0

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

^ permalink raw reply related	[flat|nested] 31+ messages in thread
* [PATCH 3/5] drm/udl: Add GEM object functions for free(), vmap(), and vunmap()
  2019-10-24 14:42 [PATCH 0/5] drm/udl: Convert to generic fbdev emulation Thomas Zimmermann
  2019-10-24 14:42 ` [PATCH 1/5] drm/udl: Clear BO vmapping pointer after unmapping BO memory Thomas Zimmermann
  2019-10-24 14:42 ` [PATCH 2/5] drm/udl: Set drm_driver.gem_prime_mmap Thomas Zimmermann
@ 2019-10-24 14:42 ` Thomas Zimmermann
  2019-10-25  7:40   ` Daniel Vetter
  2019-10-24 14:42 ` [PATCH 4/5] drm/udl: Map BO memory pages in unencrypted mode Thomas Zimmermann
  2019-10-24 14:42 ` [PATCH 5/5] drm/udl: Replace fbdev code with generic emulation Thomas Zimmermann
  4 siblings, 1 reply; 31+ messages in thread
From: Thomas Zimmermann @ 2019-10-24 14:42 UTC (permalink / raw)
  To: airlied, sean, daniel, sam, noralf; +Cc: Thomas Zimmermann, dri-devel

Implementing vmap() and vunmap() of struct drm_gem_object_funcs is
required by generic fbdev emulation. Supporting free() is easy as
well. More udl-specific interfaces can probably be replaced by GEM
object functions in later patches.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/udl/udl_gem.c | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/drivers/gpu/drm/udl/udl_gem.c b/drivers/gpu/drm/udl/udl_gem.c
index 3ea0cd9ae2d6..6ca097c270d6 100644
--- a/drivers/gpu/drm/udl/udl_gem.c
+++ b/drivers/gpu/drm/udl/udl_gem.c
@@ -11,6 +11,8 @@
 
 #include "udl_drv.h"
 
+static const struct drm_gem_object_funcs udl_gem_object_funcs;
+
 struct udl_gem_object *udl_gem_alloc_object(struct drm_device *dev,
 					    size_t size)
 {
@@ -25,6 +27,7 @@ struct udl_gem_object *udl_gem_alloc_object(struct drm_device *dev,
 		return NULL;
 	}
 
+	obj->base.funcs = &udl_gem_object_funcs;
 	obj->flags = UDL_BO_CACHEABLE;
 	return obj;
 }
@@ -230,3 +233,34 @@ int udl_gem_mmap(struct drm_file *file, struct drm_device *dev,
 	mutex_unlock(&udl->gem_lock);
 	return ret;
 }
+
+/*
+ * Helpers for struct drm_gem_object_funcs
+ */
+
+static void udl_gem_object_free(struct drm_gem_object *obj)
+{
+	udl_gem_free_object(obj);
+}
+
+static void *udl_gem_object_vmap(struct drm_gem_object *obj)
+{
+	struct udl_gem_object *uobj = to_udl_bo(obj);
+	int ret;
+
+	ret = udl_gem_vmap(uobj);
+	if (ret)
+		return ERR_PTR(ret);
+	return uobj->vmapping;
+}
+
+static void udl_gem_object_vunmap(struct drm_gem_object *obj, void *vaddr)
+{
+	return udl_gem_vunmap(to_udl_bo(obj));
+}
+
+static const struct drm_gem_object_funcs udl_gem_object_funcs = {
+	.free	= udl_gem_object_free,
+	.vmap	= udl_gem_object_vmap,
+	.vunmap	= udl_gem_object_vunmap,
+};
-- 
2.23.0

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

^ permalink raw reply related	[flat|nested] 31+ messages in thread
* [PATCH 4/5] drm/udl: Map BO memory pages in unencrypted mode
  2019-10-24 14:42 [PATCH 0/5] drm/udl: Convert to generic fbdev emulation Thomas Zimmermann
                   ` (2 preceding siblings ...)
  2019-10-24 14:42 ` [PATCH 3/5] drm/udl: Add GEM object functions for free(), vmap(), and vunmap() Thomas Zimmermann
@ 2019-10-24 14:42 ` Thomas Zimmermann
  2019-10-24 14:42 ` [PATCH 5/5] drm/udl: Replace fbdev code with generic emulation Thomas Zimmermann
  4 siblings, 0 replies; 31+ messages in thread
From: Thomas Zimmermann @ 2019-10-24 14:42 UTC (permalink / raw)
  To: airlied, sean, daniel, sam, noralf; +Cc: Thomas Zimmermann, dri-devel

The udl driver's fbdev code maps pages in unencrypted mode. To switch
over to generic fbdev emulation, we set this flag in the BO mapping code.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/udl/udl_gem.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/udl/udl_gem.c b/drivers/gpu/drm/udl/udl_gem.c
index 6ca097c270d6..68e4757e83f5 100644
--- a/drivers/gpu/drm/udl/udl_gem.c
+++ b/drivers/gpu/drm/udl/udl_gem.c
@@ -100,6 +100,9 @@ int udl_drm_gem_mmap(struct file *filp, struct vm_area_struct *vma)
 
 	update_vm_cache_attr(to_udl_bo(vma->vm_private_data), vma);
 
+	/* We don't want the framebuffer to be mapped encrypted */
+	vma->vm_page_prot = pgprot_decrypted(vma->vm_page_prot);
+
 	return ret;
 }
 
@@ -158,7 +161,7 @@ int udl_gem_vmap(struct udl_gem_object *obj)
 			return -ENOMEM;
 		return 0;
 	}
-		
+
 	ret = udl_gem_get_pages(obj);
 	if (ret)
 		return ret;
-- 
2.23.0

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

^ permalink raw reply related	[flat|nested] 31+ messages in thread
* [PATCH 5/5] drm/udl: Replace fbdev code with generic emulation
  2019-10-24 14:42 [PATCH 0/5] drm/udl: Convert to generic fbdev emulation Thomas Zimmermann
                   ` (3 preceding siblings ...)
  2019-10-24 14:42 ` [PATCH 4/5] drm/udl: Map BO memory pages in unencrypted mode Thomas Zimmermann
@ 2019-10-24 14:42 ` Thomas Zimmermann
  2019-10-25  7:47   ` Daniel Vetter
  4 siblings, 1 reply; 31+ messages in thread
From: Thomas Zimmermann @ 2019-10-24 14:42 UTC (permalink / raw)
  To: airlied, sean, daniel, sam, noralf; +Cc: Thomas Zimmermann, dri-devel

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/udl/udl_drv.c     |   3 +
 drivers/gpu/drm/udl/udl_drv.h     |   4 -
 drivers/gpu/drm/udl/udl_fb.c      | 263 +-----------------------------
 drivers/gpu/drm/udl/udl_main.c    |   2 -
 drivers/gpu/drm/udl/udl_modeset.c |   3 +-
 5 files changed, 8 insertions(+), 267 deletions(-)

diff --git a/drivers/gpu/drm/udl/udl_drv.c b/drivers/gpu/drm/udl/udl_drv.c
index 15ad7a338f9d..6beaa1109c2c 100644
--- a/drivers/gpu/drm/udl/udl_drv.c
+++ b/drivers/gpu/drm/udl/udl_drv.c
@@ -7,6 +7,7 @@
 
 #include <drm/drm_crtc_helper.h>
 #include <drm/drm_drv.h>
+#include <drm/drm_fb_helper.h>
 #include <drm/drm_file.h>
 #include <drm/drm_ioctl.h>
 #include <drm/drm_probe_helper.h>
@@ -62,6 +63,8 @@ static struct drm_driver driver = {
 	.driver_features = DRIVER_MODESET | DRIVER_GEM,
 	.release = udl_driver_release,
 
+	.lastclose = drm_fb_helper_lastclose,
+
 	/* gem hooks */
 	.gem_free_object_unlocked = udl_gem_free_object,
 	.gem_vm_ops = &udl_gem_vm_ops,
diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h
index 12a970fd9a87..5f8a7ac084f6 100644
--- a/drivers/gpu/drm/udl/udl_drv.h
+++ b/drivers/gpu/drm/udl/udl_drv.h
@@ -50,8 +50,6 @@ struct urb_list {
 	size_t size;
 };
 
-struct udl_fbdev;
-
 struct udl_device {
 	struct drm_device drm;
 	struct device *dev;
@@ -65,7 +63,6 @@ struct udl_device {
 	struct urb_list urbs;
 	atomic_t lost_pixels; /* 1 = a render op failed. Need screen refresh */
 
-	struct udl_fbdev *fbdev;
 	char mode_buf[1024];
 	uint32_t mode_buf_len;
 	atomic_t bytes_rendered; /* raw pixel-bytes driver asked to render */
@@ -111,7 +108,6 @@ int udl_init(struct udl_device *udl);
 void udl_fini(struct drm_device *dev);
 
 int udl_fbdev_init(struct drm_device *dev);
-void udl_fbdev_cleanup(struct drm_device *dev);
 void udl_fbdev_unplug(struct drm_device *dev);
 struct drm_framebuffer *
 udl_fb_user_fb_create(struct drm_device *dev,
diff --git a/drivers/gpu/drm/udl/udl_fb.c b/drivers/gpu/drm/udl/udl_fb.c
index ef3504d06343..43a1da3a56c3 100644
--- a/drivers/gpu/drm/udl/udl_fb.c
+++ b/drivers/gpu/drm/udl/udl_fb.c
@@ -19,19 +19,9 @@
 
 #include "udl_drv.h"
 
-#define DL_DEFIO_WRITE_DELAY    (HZ/20) /* fb_deferred_io.delay in jiffies */
-
-static int fb_defio = 0;  /* Optionally enable experimental fb_defio mmap support */
 static int fb_bpp = 16;
 
 module_param(fb_bpp, int, S_IWUSR | S_IRUSR | S_IWGRP | S_IRGRP);
-module_param(fb_defio, int, S_IWUSR | S_IRUSR | S_IWGRP | S_IRGRP);
-
-struct udl_fbdev {
-	struct drm_fb_helper helper; /* must be first */
-	struct udl_framebuffer ufb;
-	int fb_count;
-};
 
 #define DL_ALIGN_UP(x, a) ALIGN(x, a)
 #define DL_ALIGN_DOWN(x, a) ALIGN_DOWN(x, a)
@@ -157,123 +147,6 @@ int udl_handle_damage(struct udl_framebuffer *fb, int x, int y,
 	return 0;
 }
 
-static int udl_fb_mmap(struct fb_info *info, struct vm_area_struct *vma)
-{
-	unsigned long start = vma->vm_start;
-	unsigned long size = vma->vm_end - vma->vm_start;
-	unsigned long offset;
-	unsigned long page, pos;
-
-	if (vma->vm_pgoff > (~0UL >> PAGE_SHIFT))
-		return -EINVAL;
-
-	offset = vma->vm_pgoff << PAGE_SHIFT;
-
-	if (offset > info->fix.smem_len || size > info->fix.smem_len - offset)
-		return -EINVAL;
-
-	pos = (unsigned long)info->fix.smem_start + offset;
-
-	pr_debug("mmap() framebuffer addr:%lu size:%lu\n",
-		  pos, size);
-
-	/* We don't want the framebuffer to be mapped encrypted */
-	vma->vm_page_prot = pgprot_decrypted(vma->vm_page_prot);
-
-	while (size > 0) {
-		page = vmalloc_to_pfn((void *)pos);
-		if (remap_pfn_range(vma, start, page, PAGE_SIZE, PAGE_SHARED))
-			return -EAGAIN;
-
-		start += PAGE_SIZE;
-		pos += PAGE_SIZE;
-		if (size > PAGE_SIZE)
-			size -= PAGE_SIZE;
-		else
-			size = 0;
-	}
-
-	/* VM_IO | VM_DONTEXPAND | VM_DONTDUMP are set by remap_pfn_range() */
-	return 0;
-}
-
-/*
- * It's common for several clients to have framebuffer open simultaneously.
- * e.g. both fbcon and X. Makes things interesting.
- * Assumes caller is holding info->lock (for open and release at least)
- */
-static int udl_fb_open(struct fb_info *info, int user)
-{
-	struct udl_fbdev *ufbdev = info->par;
-	struct drm_device *dev = ufbdev->ufb.base.dev;
-	struct udl_device *udl = to_udl(dev);
-
-	/* If the USB device is gone, we don't accept new opens */
-	if (drm_dev_is_unplugged(&udl->drm))
-		return -ENODEV;
-
-	ufbdev->fb_count++;
-
-#ifdef CONFIG_DRM_FBDEV_EMULATION
-	if (fb_defio && (info->fbdefio == NULL)) {
-		/* enable defio at last moment if not disabled by client */
-
-		struct fb_deferred_io *fbdefio;
-
-		fbdefio = kzalloc(sizeof(struct fb_deferred_io), GFP_KERNEL);
-
-		if (fbdefio) {
-			fbdefio->delay = DL_DEFIO_WRITE_DELAY;
-			fbdefio->deferred_io = drm_fb_helper_deferred_io;
-		}
-
-		info->fbdefio = fbdefio;
-		fb_deferred_io_init(info);
-	}
-#endif
-
-	pr_debug("open /dev/fb%d user=%d fb_info=%p count=%d\n",
-		  info->node, user, info, ufbdev->fb_count);
-
-	return 0;
-}
-
-
-/*
- * Assumes caller is holding info->lock mutex (for open and release at least)
- */
-static int udl_fb_release(struct fb_info *info, int user)
-{
-	struct udl_fbdev *ufbdev = info->par;
-
-	ufbdev->fb_count--;
-
-#ifdef CONFIG_DRM_FBDEV_EMULATION
-	if ((ufbdev->fb_count == 0) && (info->fbdefio)) {
-		fb_deferred_io_cleanup(info);
-		kfree(info->fbdefio);
-		info->fbdefio = NULL;
-		info->fbops->fb_mmap = udl_fb_mmap;
-	}
-#endif
-
-	pr_debug("released /dev/fb%d user=%d count=%d\n",
-		info->node, user, ufbdev->fb_count);
-
-	return 0;
-}
-
-static struct fb_ops udlfb_ops = {
-	.owner = THIS_MODULE,
-	DRM_FB_HELPER_DEFAULT_OPS,
-	.fb_fillrect = drm_fb_helper_sys_fillrect,
-	.fb_copyarea = drm_fb_helper_sys_copyarea,
-	.fb_imageblit = drm_fb_helper_sys_imageblit,
-	.fb_mmap = udl_fb_mmap,
-	.fb_open = udl_fb_open,
-	.fb_release = udl_fb_release,
-};
-
 static int udl_user_framebuffer_dirty(struct drm_framebuffer *fb,
 				      struct drm_file *file,
 				      unsigned flags, unsigned color,
@@ -346,150 +219,20 @@ udl_framebuffer_init(struct drm_device *dev,
 	return ret;
 }
 
-
-static int udlfb_create(struct drm_fb_helper *helper,
-			struct drm_fb_helper_surface_size *sizes)
-{
-	struct udl_fbdev *ufbdev =
-		container_of(helper, struct udl_fbdev, helper);
-	struct drm_device *dev = ufbdev->helper.dev;
-	struct fb_info *info;
-	struct drm_framebuffer *fb;
-	struct drm_mode_fb_cmd2 mode_cmd;
-	struct udl_gem_object *obj;
-	uint32_t size;
-	int ret = 0;
-
-	if (sizes->surface_bpp == 24)
-		sizes->surface_bpp = 32;
-
-	mode_cmd.width = sizes->surface_width;
-	mode_cmd.height = sizes->surface_height;
-	mode_cmd.pitches[0] = mode_cmd.width * ((sizes->surface_bpp + 7) / 8);
-
-	mode_cmd.pixel_format = drm_mode_legacy_fb_format(sizes->surface_bpp,
-							  sizes->surface_depth);
-
-	size = mode_cmd.pitches[0] * mode_cmd.height;
-	size = ALIGN(size, PAGE_SIZE);
-
-	obj = udl_gem_alloc_object(dev, size);
-	if (!obj)
-		goto out;
-
-	ret = udl_gem_vmap(obj);
-	if (ret) {
-		DRM_ERROR("failed to vmap fb\n");
-		goto out_gfree;
-	}
-
-	info = drm_fb_helper_alloc_fbi(helper);
-	if (IS_ERR(info)) {
-		ret = PTR_ERR(info);
-		goto out_gfree;
-	}
-
-	ret = udl_framebuffer_init(dev, &ufbdev->ufb, &mode_cmd, obj);
-	if (ret)
-		goto out_gfree;
-
-	fb = &ufbdev->ufb.base;
-
-	ufbdev->helper.fb = fb;
-
-	info->screen_base = ufbdev->ufb.obj->vmapping;
-	info->fix.smem_len = size;
-	info->fix.smem_start = (unsigned long)ufbdev->ufb.obj->vmapping;
-
-	info->fbops = &udlfb_ops;
-	drm_fb_helper_fill_info(info, &ufbdev->helper, sizes);
-
-	DRM_DEBUG_KMS("allocated %dx%d vmal %p\n",
-		      fb->width, fb->height,
-		      ufbdev->ufb.obj->vmapping);
-
-	return ret;
-out_gfree:
-	drm_gem_object_put_unlocked(&ufbdev->ufb.obj->base);
-out:
-	return ret;
-}
-
-static const struct drm_fb_helper_funcs udl_fb_helper_funcs = {
-	.fb_probe = udlfb_create,
-};
-
-static void udl_fbdev_destroy(struct drm_device *dev,
-			      struct udl_fbdev *ufbdev)
-{
-	drm_fb_helper_unregister_fbi(&ufbdev->helper);
-	drm_fb_helper_fini(&ufbdev->helper);
-	if (ufbdev->ufb.obj) {
-		drm_framebuffer_unregister_private(&ufbdev->ufb.base);
-		drm_framebuffer_cleanup(&ufbdev->ufb.base);
-		drm_gem_object_put_unlocked(&ufbdev->ufb.obj->base);
-	}
-}
-
 int udl_fbdev_init(struct drm_device *dev)
 {
-	struct udl_device *udl = to_udl(dev);
 	int bpp_sel = fb_bpp;
-	struct udl_fbdev *ufbdev;
 	int ret;
 
-	ufbdev = kzalloc(sizeof(struct udl_fbdev), GFP_KERNEL);
-	if (!ufbdev)
-		return -ENOMEM;
-
-	udl->fbdev = ufbdev;
-
-	drm_fb_helper_prepare(dev, &ufbdev->helper, &udl_fb_helper_funcs);
-
-	ret = drm_fb_helper_init(dev, &ufbdev->helper, 1);
-	if (ret)
-		goto free;
-
-	ret = drm_fb_helper_single_add_all_connectors(&ufbdev->helper);
+	ret = drm_fbdev_generic_setup(dev, bpp_sel);
 	if (ret)
-		goto fini;
-
-	/* disable all the possible outputs/crtcs before entering KMS mode */
-	drm_helper_disable_unused_functions(dev);
-
-	ret = drm_fb_helper_initial_config(&ufbdev->helper, bpp_sel);
-	if (ret)
-		goto fini;
-
+		return ret;
 	return 0;
-
-fini:
-	drm_fb_helper_fini(&ufbdev->helper);
-free:
-	kfree(ufbdev);
-	return ret;
-}
-
-void udl_fbdev_cleanup(struct drm_device *dev)
-{
-	struct udl_device *udl = to_udl(dev);
-	if (!udl->fbdev)
-		return;
-
-	udl_fbdev_destroy(dev, udl->fbdev);
-	kfree(udl->fbdev);
-	udl->fbdev = NULL;
 }
 
 void udl_fbdev_unplug(struct drm_device *dev)
 {
-	struct udl_device *udl = to_udl(dev);
-	struct udl_fbdev *ufbdev;
-	if (!udl->fbdev)
-		return;
-
-	ufbdev = udl->fbdev;
-	drm_fb_helper_unlink_fbi(&ufbdev->helper);
+	drm_fb_helper_unlink_fbi(dev->fb_helper);
 }
 
 struct drm_framebuffer *
diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c
index 4e854e017390..2a6399290f09 100644
--- a/drivers/gpu/drm/udl/udl_main.c
+++ b/drivers/gpu/drm/udl/udl_main.c
@@ -367,6 +367,4 @@ void udl_fini(struct drm_device *dev)
 
 	if (udl->urbs.count)
 		udl_free_urb_list(dev);
-
-	udl_fbdev_cleanup(dev);
 }
diff --git a/drivers/gpu/drm/udl/udl_modeset.c b/drivers/gpu/drm/udl/udl_modeset.c
index bc1ab6060dc6..1517d5e881b8 100644
--- a/drivers/gpu/drm/udl/udl_modeset.c
+++ b/drivers/gpu/drm/udl/udl_modeset.c
@@ -10,6 +10,7 @@
  */
 
 #include <drm/drm_crtc_helper.h>
+#include <drm/drm_fb_helper.h>
 #include <drm/drm_modeset_helper_vtables.h>
 #include <drm/drm_vblank.h>
 
@@ -422,7 +423,7 @@ static int udl_crtc_init(struct drm_device *dev)
 
 static const struct drm_mode_config_funcs udl_mode_funcs = {
 	.fb_create = udl_fb_user_fb_create,
-	.output_poll_changed = NULL,
+	.output_poll_changed = drm_fb_helper_output_poll_changed,
 };
 
 int udl_modeset_init(struct drm_device *dev)
-- 
2.23.0

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

^ permalink raw reply related	[flat|nested] 31+ messages in thread
* Re: [PATCH 1/5] drm/udl: Clear BO vmapping pointer after unmapping BO memory
  2019-10-24 14:42 ` [PATCH 1/5] drm/udl: Clear BO vmapping pointer after unmapping BO memory Thomas Zimmermann
@ 2019-10-25  7:39   ` Daniel Vetter
  0 siblings, 0 replies; 31+ messages in thread
From: Daniel Vetter @ 2019-10-25  7:39 UTC (permalink / raw)
  To: Thomas Zimmermann; +Cc: sean, dri-devel, airlied, sam

On Thu, Oct 24, 2019 at 04:42:33PM +0200, Thomas Zimmermann wrote:
> Unmapping the BO memory with udl_gem_vunmap() creates a dangling pointer
> in struct udl_gem_object.vmapping. This can crash udl_handle_damage(),
> which check the pointer's value for NULL. Clear the pointer to NULL and
> let udl_handle_damage() re-establish the mapping if necessary.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>

Hm right now this is not a problem, becase we remove the vmapping only
when we free the underlying object. If that changes somehow, then what we
actually need is to start refcount the vmapping (and drop the trick in
udl_handle_damage and unconditionally vmap/vunmap). Might be easier to
just cut over to shmem helpers.
-Daniel

> ---
>  drivers/gpu/drm/udl/udl_gem.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/gpu/drm/udl/udl_gem.c b/drivers/gpu/drm/udl/udl_gem.c
> index b23a5c2fcd80..3ea0cd9ae2d6 100644
> --- a/drivers/gpu/drm/udl/udl_gem.c
> +++ b/drivers/gpu/drm/udl/udl_gem.c
> @@ -174,6 +174,7 @@ void udl_gem_vunmap(struct udl_gem_object *obj)
>  	}
>  
>  	vunmap(obj->vmapping);
> +	obj->vmapping = NULL;
>  
>  	udl_gem_put_pages(obj);
>  }
> -- 
> 2.23.0
> 

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

^ permalink raw reply	[flat|nested] 31+ messages in thread
* Re: [PATCH 3/5] drm/udl: Add GEM object functions for free(), vmap(), and vunmap()
  2019-10-24 14:42 ` [PATCH 3/5] drm/udl: Add GEM object functions for free(), vmap(), and vunmap() Thomas Zimmermann
@ 2019-10-25  7:40   ` Daniel Vetter
  2019-10-25  7:59       ` Thomas Zimmermann
  0 siblings, 1 reply; 31+ messages in thread
From: Daniel Vetter @ 2019-10-25  7:40 UTC (permalink / raw)
  To: Thomas Zimmermann; +Cc: sean, dri-devel, airlied, sam

On Thu, Oct 24, 2019 at 04:42:35PM +0200, Thomas Zimmermann wrote:
> Implementing vmap() and vunmap() of struct drm_gem_object_funcs is
> required by generic fbdev emulation. Supporting free() is easy as
> well. More udl-specific interfaces can probably be replaced by GEM
> object functions in later patches.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>  drivers/gpu/drm/udl/udl_gem.c | 34 ++++++++++++++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
> 
> diff --git a/drivers/gpu/drm/udl/udl_gem.c b/drivers/gpu/drm/udl/udl_gem.c
> index 3ea0cd9ae2d6..6ca097c270d6 100644
> --- a/drivers/gpu/drm/udl/udl_gem.c
> +++ b/drivers/gpu/drm/udl/udl_gem.c
> @@ -11,6 +11,8 @@
>  
>  #include "udl_drv.h"
>  
> +static const struct drm_gem_object_funcs udl_gem_object_funcs;
> +
>  struct udl_gem_object *udl_gem_alloc_object(struct drm_device *dev,
>  					    size_t size)
>  {
> @@ -25,6 +27,7 @@ struct udl_gem_object *udl_gem_alloc_object(struct drm_device *dev,
>  		return NULL;
>  	}
>  
> +	obj->base.funcs = &udl_gem_object_funcs;
>  	obj->flags = UDL_BO_CACHEABLE;
>  	return obj;
>  }
> @@ -230,3 +233,34 @@ int udl_gem_mmap(struct drm_file *file, struct drm_device *dev,
>  	mutex_unlock(&udl->gem_lock);
>  	return ret;
>  }
> +
> +/*
> + * Helpers for struct drm_gem_object_funcs
> + */
> +
> +static void udl_gem_object_free(struct drm_gem_object *obj)
> +{
> +	udl_gem_free_object(obj);
> +}
> +
> +static void *udl_gem_object_vmap(struct drm_gem_object *obj)
> +{
> +	struct udl_gem_object *uobj = to_udl_bo(obj);
> +	int ret;
> +
> +	ret = udl_gem_vmap(uobj);
> +	if (ret)
> +		return ERR_PTR(ret);
> +	return uobj->vmapping;
> +}
> +
> +static void udl_gem_object_vunmap(struct drm_gem_object *obj, void *vaddr)
> +{
> +	return udl_gem_vunmap(to_udl_bo(obj));
> +}
> +
> +static const struct drm_gem_object_funcs udl_gem_object_funcs = {
> +	.free	= udl_gem_object_free,
> +	.vmap	= udl_gem_object_vmap,
> +	.vunmap	= udl_gem_object_vunmap,

Yeah this doesn't work, you need to refcount the vmap here I think. I'd
say simpler to first cut over to shmem helpers.
-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

^ permalink raw reply	[flat|nested] 31+ messages in thread
* Re: [PATCH 5/5] drm/udl: Replace fbdev code with generic emulation
  2019-10-24 14:42 ` [PATCH 5/5] drm/udl: Replace fbdev code with generic emulation Thomas Zimmermann
@ 2019-10-25  7:47   ` Daniel Vetter
  2019-10-25  8:00     ` Daniel Vetter
  2019-10-25  8:08     ` Thomas Zimmermann
  0 siblings, 2 replies; 31+ messages in thread
From: Daniel Vetter @ 2019-10-25  7:47 UTC (permalink / raw)
  To: Thomas Zimmermann; +Cc: sean, dri-devel, airlied, sam

On Thu, Oct 24, 2019 at 04:42:37PM +0200, Thomas Zimmermann wrote:
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>  drivers/gpu/drm/udl/udl_drv.c     |   3 +
>  drivers/gpu/drm/udl/udl_drv.h     |   4 -
>  drivers/gpu/drm/udl/udl_fb.c      | 263 +-----------------------------
>  drivers/gpu/drm/udl/udl_main.c    |   2 -
>  drivers/gpu/drm/udl/udl_modeset.c |   3 +-
>  5 files changed, 8 insertions(+), 267 deletions(-)
> 
> diff --git a/drivers/gpu/drm/udl/udl_drv.c b/drivers/gpu/drm/udl/udl_drv.c
> index 15ad7a338f9d..6beaa1109c2c 100644
> --- a/drivers/gpu/drm/udl/udl_drv.c
> +++ b/drivers/gpu/drm/udl/udl_drv.c
> @@ -7,6 +7,7 @@
>  
>  #include <drm/drm_crtc_helper.h>
>  #include <drm/drm_drv.h>
> +#include <drm/drm_fb_helper.h>
>  #include <drm/drm_file.h>
>  #include <drm/drm_ioctl.h>
>  #include <drm/drm_probe_helper.h>
> @@ -62,6 +63,8 @@ static struct drm_driver driver = {
>  	.driver_features = DRIVER_MODESET | DRIVER_GEM,
>  	.release = udl_driver_release,
>  
> +	.lastclose = drm_fb_helper_lastclose,
> +
>  	/* gem hooks */
>  	.gem_free_object_unlocked = udl_gem_free_object,
>  	.gem_vm_ops = &udl_gem_vm_ops,
> diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h
> index 12a970fd9a87..5f8a7ac084f6 100644
> --- a/drivers/gpu/drm/udl/udl_drv.h
> +++ b/drivers/gpu/drm/udl/udl_drv.h
> @@ -50,8 +50,6 @@ struct urb_list {
>  	size_t size;
>  };
>  
> -struct udl_fbdev;
> -
>  struct udl_device {
>  	struct drm_device drm;
>  	struct device *dev;
> @@ -65,7 +63,6 @@ struct udl_device {
>  	struct urb_list urbs;
>  	atomic_t lost_pixels; /* 1 = a render op failed. Need screen refresh */
>  
> -	struct udl_fbdev *fbdev;
>  	char mode_buf[1024];
>  	uint32_t mode_buf_len;
>  	atomic_t bytes_rendered; /* raw pixel-bytes driver asked to render */
> @@ -111,7 +108,6 @@ int udl_init(struct udl_device *udl);
>  void udl_fini(struct drm_device *dev);
>  
>  int udl_fbdev_init(struct drm_device *dev);
> -void udl_fbdev_cleanup(struct drm_device *dev);
>  void udl_fbdev_unplug(struct drm_device *dev);
>  struct drm_framebuffer *
>  udl_fb_user_fb_create(struct drm_device *dev,
> diff --git a/drivers/gpu/drm/udl/udl_fb.c b/drivers/gpu/drm/udl/udl_fb.c
> index ef3504d06343..43a1da3a56c3 100644
> --- a/drivers/gpu/drm/udl/udl_fb.c
> +++ b/drivers/gpu/drm/udl/udl_fb.c
> @@ -19,19 +19,9 @@
>  
>  #include "udl_drv.h"
>  
> -#define DL_DEFIO_WRITE_DELAY    (HZ/20) /* fb_deferred_io.delay in jiffies */
> -
> -static int fb_defio = 0;  /* Optionally enable experimental fb_defio mmap support */
>  static int fb_bpp = 16;
>  
>  module_param(fb_bpp, int, S_IWUSR | S_IRUSR | S_IWGRP | S_IRGRP);
> -module_param(fb_defio, int, S_IWUSR | S_IRUSR | S_IWGRP | S_IRGRP);
> -
> -struct udl_fbdev {
> -	struct drm_fb_helper helper; /* must be first */
> -	struct udl_framebuffer ufb;
> -	int fb_count;
> -};
>  
>  #define DL_ALIGN_UP(x, a) ALIGN(x, a)
>  #define DL_ALIGN_DOWN(x, a) ALIGN_DOWN(x, a)
> @@ -157,123 +147,6 @@ int udl_handle_damage(struct udl_framebuffer *fb, int x, int y,
>  	return 0;
>  }
>  
> -static int udl_fb_mmap(struct fb_info *info, struct vm_area_struct *vma)
> -{
> -	unsigned long start = vma->vm_start;
> -	unsigned long size = vma->vm_end - vma->vm_start;
> -	unsigned long offset;
> -	unsigned long page, pos;
> -
> -	if (vma->vm_pgoff > (~0UL >> PAGE_SHIFT))
> -		return -EINVAL;
> -
> -	offset = vma->vm_pgoff << PAGE_SHIFT;
> -
> -	if (offset > info->fix.smem_len || size > info->fix.smem_len - offset)
> -		return -EINVAL;
> -
> -	pos = (unsigned long)info->fix.smem_start + offset;
> -
> -	pr_debug("mmap() framebuffer addr:%lu size:%lu\n",
> -		  pos, size);
> -
> -	/* We don't want the framebuffer to be mapped encrypted */
> -	vma->vm_page_prot = pgprot_decrypted(vma->vm_page_prot);
> -
> -	while (size > 0) {
> -		page = vmalloc_to_pfn((void *)pos);
> -		if (remap_pfn_range(vma, start, page, PAGE_SIZE, PAGE_SHARED))
> -			return -EAGAIN;
> -
> -		start += PAGE_SIZE;
> -		pos += PAGE_SIZE;
> -		if (size > PAGE_SIZE)
> -			size -= PAGE_SIZE;
> -		else
> -			size = 0;
> -	}
> -
> -	/* VM_IO | VM_DONTEXPAND | VM_DONTDUMP are set by remap_pfn_range() */
> -	return 0;
> -}
> -
> -/*
> - * It's common for several clients to have framebuffer open simultaneously.
> - * e.g. both fbcon and X. Makes things interesting.
> - * Assumes caller is holding info->lock (for open and release at least)
> - */
> -static int udl_fb_open(struct fb_info *info, int user)
> -{
> -	struct udl_fbdev *ufbdev = info->par;
> -	struct drm_device *dev = ufbdev->ufb.base.dev;
> -	struct udl_device *udl = to_udl(dev);
> -
> -	/* If the USB device is gone, we don't accept new opens */
> -	if (drm_dev_is_unplugged(&udl->drm))
> -		return -ENODEV;
> -
> -	ufbdev->fb_count++;
> -
> -#ifdef CONFIG_DRM_FBDEV_EMULATION
> -	if (fb_defio && (info->fbdefio == NULL)) {
> -		/* enable defio at last moment if not disabled by client */
> -
> -		struct fb_deferred_io *fbdefio;
> -
> -		fbdefio = kzalloc(sizeof(struct fb_deferred_io), GFP_KERNEL);
> -
> -		if (fbdefio) {
> -			fbdefio->delay = DL_DEFIO_WRITE_DELAY;
> -			fbdefio->deferred_io = drm_fb_helper_deferred_io;
> -		}
> -
> -		info->fbdefio = fbdefio;
> -		fb_deferred_io_init(info);
> -	}
> -#endif
> -
> -	pr_debug("open /dev/fb%d user=%d fb_info=%p count=%d\n",
> -		  info->node, user, info, ufbdev->fb_count);
> -
> -	return 0;
> -}
> -
> -
> -/*
> - * Assumes caller is holding info->lock mutex (for open and release at least)
> - */
> -static int udl_fb_release(struct fb_info *info, int user)
> -{
> -	struct udl_fbdev *ufbdev = info->par;
> -
> -	ufbdev->fb_count--;
> -
> -#ifdef CONFIG_DRM_FBDEV_EMULATION
> -	if ((ufbdev->fb_count == 0) && (info->fbdefio)) {
> -		fb_deferred_io_cleanup(info);
> -		kfree(info->fbdefio);
> -		info->fbdefio = NULL;
> -		info->fbops->fb_mmap = udl_fb_mmap;
> -	}
> -#endif
> -
> -	pr_debug("released /dev/fb%d user=%d count=%d\n",
> -		info->node, user, ufbdev->fb_count);
> -
> -	return 0;
> -}
> -
> -static struct fb_ops udlfb_ops = {
> -	.owner = THIS_MODULE,
> -	DRM_FB_HELPER_DEFAULT_OPS,
> -	.fb_fillrect = drm_fb_helper_sys_fillrect,
> -	.fb_copyarea = drm_fb_helper_sys_copyarea,
> -	.fb_imageblit = drm_fb_helper_sys_imageblit,
> -	.fb_mmap = udl_fb_mmap,
> -	.fb_open = udl_fb_open,
> -	.fb_release = udl_fb_release,
> -};
> -
>  static int udl_user_framebuffer_dirty(struct drm_framebuffer *fb,
>  				      struct drm_file *file,
>  				      unsigned flags, unsigned color,
> @@ -346,150 +219,20 @@ udl_framebuffer_init(struct drm_device *dev,
>  	return ret;
>  }
>  
> -
> -static int udlfb_create(struct drm_fb_helper *helper,
> -			struct drm_fb_helper_surface_size *sizes)
> -{
> -	struct udl_fbdev *ufbdev =
> -		container_of(helper, struct udl_fbdev, helper);
> -	struct drm_device *dev = ufbdev->helper.dev;
> -	struct fb_info *info;
> -	struct drm_framebuffer *fb;
> -	struct drm_mode_fb_cmd2 mode_cmd;
> -	struct udl_gem_object *obj;
> -	uint32_t size;
> -	int ret = 0;
> -
> -	if (sizes->surface_bpp == 24)
> -		sizes->surface_bpp = 32;
> -
> -	mode_cmd.width = sizes->surface_width;
> -	mode_cmd.height = sizes->surface_height;
> -	mode_cmd.pitches[0] = mode_cmd.width * ((sizes->surface_bpp + 7) / 8);
> -
> -	mode_cmd.pixel_format = drm_mode_legacy_fb_format(sizes->surface_bpp,
> -							  sizes->surface_depth);
> -
> -	size = mode_cmd.pitches[0] * mode_cmd.height;
> -	size = ALIGN(size, PAGE_SIZE);
> -
> -	obj = udl_gem_alloc_object(dev, size);
> -	if (!obj)
> -		goto out;
> -
> -	ret = udl_gem_vmap(obj);
> -	if (ret) {
> -		DRM_ERROR("failed to vmap fb\n");
> -		goto out_gfree;
> -	}
> -
> -	info = drm_fb_helper_alloc_fbi(helper);
> -	if (IS_ERR(info)) {
> -		ret = PTR_ERR(info);
> -		goto out_gfree;
> -	}
> -
> -	ret = udl_framebuffer_init(dev, &ufbdev->ufb, &mode_cmd, obj);
> -	if (ret)
> -		goto out_gfree;
> -
> -	fb = &ufbdev->ufb.base;
> -
> -	ufbdev->helper.fb = fb;
> -
> -	info->screen_base = ufbdev->ufb.obj->vmapping;
> -	info->fix.smem_len = size;
> -	info->fix.smem_start = (unsigned long)ufbdev->ufb.obj->vmapping;
> -
> -	info->fbops = &udlfb_ops;
> -	drm_fb_helper_fill_info(info, &ufbdev->helper, sizes);
> -
> -	DRM_DEBUG_KMS("allocated %dx%d vmal %p\n",
> -		      fb->width, fb->height,
> -		      ufbdev->ufb.obj->vmapping);
> -
> -	return ret;
> -out_gfree:
> -	drm_gem_object_put_unlocked(&ufbdev->ufb.obj->base);
> -out:
> -	return ret;
> -}
> -
> -static const struct drm_fb_helper_funcs udl_fb_helper_funcs = {
> -	.fb_probe = udlfb_create,
> -};
> -
> -static void udl_fbdev_destroy(struct drm_device *dev,
> -			      struct udl_fbdev *ufbdev)
> -{
> -	drm_fb_helper_unregister_fbi(&ufbdev->helper);
> -	drm_fb_helper_fini(&ufbdev->helper);
> -	if (ufbdev->ufb.obj) {
> -		drm_framebuffer_unregister_private(&ufbdev->ufb.base);
> -		drm_framebuffer_cleanup(&ufbdev->ufb.base);
> -		drm_gem_object_put_unlocked(&ufbdev->ufb.obj->base);
> -	}
> -}
> -
>  int udl_fbdev_init(struct drm_device *dev)
>  {
> -	struct udl_device *udl = to_udl(dev);
>  	int bpp_sel = fb_bpp;
> -	struct udl_fbdev *ufbdev;
>  	int ret;
>  
> -	ufbdev = kzalloc(sizeof(struct udl_fbdev), GFP_KERNEL);
> -	if (!ufbdev)
> -		return -ENOMEM;
> -
> -	udl->fbdev = ufbdev;
> -
> -	drm_fb_helper_prepare(dev, &ufbdev->helper, &udl_fb_helper_funcs);
> -
> -	ret = drm_fb_helper_init(dev, &ufbdev->helper, 1);
> -	if (ret)
> -		goto free;
> -
> -	ret = drm_fb_helper_single_add_all_connectors(&ufbdev->helper);
> +	ret = drm_fbdev_generic_setup(dev, bpp_sel);
>  	if (ret)
> -		goto fini;
> -
> -	/* disable all the possible outputs/crtcs before entering KMS mode */
> -	drm_helper_disable_unused_functions(dev);
> -
> -	ret = drm_fb_helper_initial_config(&ufbdev->helper, bpp_sel);
> -	if (ret)
> -		goto fini;
> -
> +		return ret;
>  	return 0;
> -
> -fini:
> -	drm_fb_helper_fini(&ufbdev->helper);
> -free:
> -	kfree(ufbdev);
> -	return ret;
> -}
> -
> -void udl_fbdev_cleanup(struct drm_device *dev)
> -{
> -	struct udl_device *udl = to_udl(dev);
> -	if (!udl->fbdev)
> -		return;
> -
> -	udl_fbdev_destroy(dev, udl->fbdev);
> -	kfree(udl->fbdev);
> -	udl->fbdev = NULL;
>  }
>  
>  void udl_fbdev_unplug(struct drm_device *dev)
>  {
> -	struct udl_device *udl = to_udl(dev);
> -	struct udl_fbdev *ufbdev;
> -	if (!udl->fbdev)
> -		return;
> -
> -	ufbdev = udl->fbdev;
> -	drm_fb_helper_unlink_fbi(&ufbdev->helper);
> +	drm_fb_helper_unlink_fbi(dev->fb_helper);

Uh I think this here can be all garbage-collected. The generic fbdev
already calls drm_fb_helper_unregister_fbi, which calls
unregister_framebuffer, which is a strict superset of unlink_framebuffer.
The later isn't really enough for hotunplug.

So for generic fbdev you really shouldn't need any cleanup nor unplug
functions anymore.

Also udl is the only caller of drm_fb_helper_unlink_fbi, so we could drop
that. Which removes the only external caller of unlink_framebuffer, so we
can drop that too. Care to follow up with those 2 patches?

Aside from this this patch here looks great!
-Daniel

>  }
>  
>  struct drm_framebuffer *
> diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c
> index 4e854e017390..2a6399290f09 100644
> --- a/drivers/gpu/drm/udl/udl_main.c
> +++ b/drivers/gpu/drm/udl/udl_main.c
> @@ -367,6 +367,4 @@ void udl_fini(struct drm_device *dev)
>  
>  	if (udl->urbs.count)
>  		udl_free_urb_list(dev);
> -
> -	udl_fbdev_cleanup(dev);
>  }
> diff --git a/drivers/gpu/drm/udl/udl_modeset.c b/drivers/gpu/drm/udl/udl_modeset.c
> index bc1ab6060dc6..1517d5e881b8 100644
> --- a/drivers/gpu/drm/udl/udl_modeset.c
> +++ b/drivers/gpu/drm/udl/udl_modeset.c
> @@ -10,6 +10,7 @@
>   */
>  
>  #include <drm/drm_crtc_helper.h>
> +#include <drm/drm_fb_helper.h>
>  #include <drm/drm_modeset_helper_vtables.h>
>  #include <drm/drm_vblank.h>
>  
> @@ -422,7 +423,7 @@ static int udl_crtc_init(struct drm_device *dev)
>  
>  static const struct drm_mode_config_funcs udl_mode_funcs = {
>  	.fb_create = udl_fb_user_fb_create,
> -	.output_poll_changed = NULL,
> +	.output_poll_changed = drm_fb_helper_output_poll_changed,
>  };
>  
>  int udl_modeset_init(struct drm_device *dev)
> -- 
> 2.23.0
> 

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

^ permalink raw reply	[flat|nested] 31+ messages in thread
* Re: [PATCH 3/5] drm/udl: Add GEM object functions for free(), vmap(), and vunmap()
@ 2019-10-25  7:59       ` Thomas Zimmermann
  0 siblings, 0 replies; 31+ messages in thread
From: Thomas Zimmermann @ 2019-10-25  7:59 UTC (permalink / raw)
  To: Daniel Vetter; +Cc: sam, airlied, sean, dri-devel


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

Hi

Am 25.10.19 um 09:40 schrieb Daniel Vetter:
> On Thu, Oct 24, 2019 at 04:42:35PM +0200, Thomas Zimmermann wrote:
>> Implementing vmap() and vunmap() of struct drm_gem_object_funcs is
>> required by generic fbdev emulation. Supporting free() is easy as
>> well. More udl-specific interfaces can probably be replaced by GEM
>> object functions in later patches.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
>>  drivers/gpu/drm/udl/udl_gem.c | 34 ++++++++++++++++++++++++++++++++++
>>  1 file changed, 34 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/udl/udl_gem.c b/drivers/gpu/drm/udl/udl_gem.c
>> index 3ea0cd9ae2d6..6ca097c270d6 100644
>> --- a/drivers/gpu/drm/udl/udl_gem.c
>> +++ b/drivers/gpu/drm/udl/udl_gem.c
>> @@ -11,6 +11,8 @@
>>  
>>  #include "udl_drv.h"
>>  
>> +static const struct drm_gem_object_funcs udl_gem_object_funcs;
>> +
>>  struct udl_gem_object *udl_gem_alloc_object(struct drm_device *dev,
>>  					    size_t size)
>>  {
>> @@ -25,6 +27,7 @@ struct udl_gem_object *udl_gem_alloc_object(struct drm_device *dev,
>>  		return NULL;
>>  	}
>>  
>> +	obj->base.funcs = &udl_gem_object_funcs;
>>  	obj->flags = UDL_BO_CACHEABLE;
>>  	return obj;
>>  }
>> @@ -230,3 +233,34 @@ int udl_gem_mmap(struct drm_file *file, struct drm_device *dev,
>>  	mutex_unlock(&udl->gem_lock);
>>  	return ret;
>>  }
>> +
>> +/*
>> + * Helpers for struct drm_gem_object_funcs
>> + */
>> +
>> +static void udl_gem_object_free(struct drm_gem_object *obj)
>> +{
>> +	udl_gem_free_object(obj);
>> +}
>> +
>> +static void *udl_gem_object_vmap(struct drm_gem_object *obj)
>> +{
>> +	struct udl_gem_object *uobj = to_udl_bo(obj);
>> +	int ret;
>> +
>> +	ret = udl_gem_vmap(uobj);
>> +	if (ret)
>> +		return ERR_PTR(ret);
>> +	return uobj->vmapping;
>> +}
>> +
>> +static void udl_gem_object_vunmap(struct drm_gem_object *obj, void *vaddr)
>> +{
>> +	return udl_gem_vunmap(to_udl_bo(obj));
>> +}
>> +
>> +static const struct drm_gem_object_funcs udl_gem_object_funcs = {
>> +	.free	= udl_gem_object_free,
>> +	.vmap	= udl_gem_object_vmap,
>> +	.vunmap	= udl_gem_object_vunmap,
> 
> Yeah this doesn't work, you need to refcount the vmap here I think. I'd

I see. Should have thought of that...

> say simpler to first cut over to shmem helpers.

Right. I was already attempting the conversion and the udl gem is more
or less the same as shmem, except for the flags field. [1] The flag
signals page attributes for mmap-ing [2]. For prime-imported BOs its is
set to writecombining, and for local BOs it is set to cached. Shmem
always maps with writecombining.

I don't see why udl BOs are special wrt to mmap-ing. It seems to be some
kind of optimization. Do you have an idea?

Best regards
Thomas

[1]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/udl/udl_drv.h?h=v5.3#n78
[2]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/udl/udl_gem.c?h=v5.3#n57

> -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: 159 bytes --]

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

^ permalink raw reply	[flat|nested] 31+ messages in thread
* Re: [PATCH 3/5] drm/udl: Add GEM object functions for free(), vmap(),  and vunmap()
@ 2019-10-25  7:59       ` Thomas Zimmermann
  0 siblings, 0 replies; 31+ messages in thread
From: Thomas Zimmermann @ 2019-10-25  7:59 UTC (permalink / raw)
  To: Daniel Vetter; +Cc: sam, airlied, sean, dri-devel


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

Hi

Am 25.10.19 um 09:40 schrieb Daniel Vetter:
> On Thu, Oct 24, 2019 at 04:42:35PM +0200, Thomas Zimmermann wrote:
>> Implementing vmap() and vunmap() of struct drm_gem_object_funcs is
>> required by generic fbdev emulation. Supporting free() is easy as
>> well. More udl-specific interfaces can probably be replaced by GEM
>> object functions in later patches.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
>>  drivers/gpu/drm/udl/udl_gem.c | 34 ++++++++++++++++++++++++++++++++++
>>  1 file changed, 34 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/udl/udl_gem.c b/drivers/gpu/drm/udl/udl_gem.c
>> index 3ea0cd9ae2d6..6ca097c270d6 100644
>> --- a/drivers/gpu/drm/udl/udl_gem.c
>> +++ b/drivers/gpu/drm/udl/udl_gem.c
>> @@ -11,6 +11,8 @@
>>  
>>  #include "udl_drv.h"
>>  
>> +static const struct drm_gem_object_funcs udl_gem_object_funcs;
>> +
>>  struct udl_gem_object *udl_gem_alloc_object(struct drm_device *dev,
>>  					    size_t size)
>>  {
>> @@ -25,6 +27,7 @@ struct udl_gem_object *udl_gem_alloc_object(struct drm_device *dev,
>>  		return NULL;
>>  	}
>>  
>> +	obj->base.funcs = &udl_gem_object_funcs;
>>  	obj->flags = UDL_BO_CACHEABLE;
>>  	return obj;
>>  }
>> @@ -230,3 +233,34 @@ int udl_gem_mmap(struct drm_file *file, struct drm_device *dev,
>>  	mutex_unlock(&udl->gem_lock);
>>  	return ret;
>>  }
>> +
>> +/*
>> + * Helpers for struct drm_gem_object_funcs
>> + */
>> +
>> +static void udl_gem_object_free(struct drm_gem_object *obj)
>> +{
>> +	udl_gem_free_object(obj);
>> +}
>> +
>> +static void *udl_gem_object_vmap(struct drm_gem_object *obj)
>> +{
>> +	struct udl_gem_object *uobj = to_udl_bo(obj);
>> +	int ret;
>> +
>> +	ret = udl_gem_vmap(uobj);
>> +	if (ret)
>> +		return ERR_PTR(ret);
>> +	return uobj->vmapping;
>> +}
>> +
>> +static void udl_gem_object_vunmap(struct drm_gem_object *obj, void *vaddr)
>> +{
>> +	return udl_gem_vunmap(to_udl_bo(obj));
>> +}
>> +
>> +static const struct drm_gem_object_funcs udl_gem_object_funcs = {
>> +	.free	= udl_gem_object_free,
>> +	.vmap	= udl_gem_object_vmap,
>> +	.vunmap	= udl_gem_object_vunmap,
> 
> Yeah this doesn't work, you need to refcount the vmap here I think. I'd

I see. Should have thought of that...

> say simpler to first cut over to shmem helpers.

Right. I was already attempting the conversion and the udl gem is more
or less the same as shmem, except for the flags field. [1] The flag
signals page attributes for mmap-ing [2]. For prime-imported BOs its is
set to writecombining, and for local BOs it is set to cached. Shmem
always maps with writecombining.

I don't see why udl BOs are special wrt to mmap-ing. It seems to be some
kind of optimization. Do you have an idea?

Best regards
Thomas

[1]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/udl/udl_drv.h?h=v5.3#n78
[2]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/udl/udl_gem.c?h=v5.3#n57

> -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: 159 bytes --]

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

^ permalink raw reply	[flat|nested] 31+ messages in thread
* Re: [PATCH 5/5] drm/udl: Replace fbdev code with generic emulation
  2019-10-25  7:47   ` Daniel Vetter
@ 2019-10-25  8:00     ` Daniel Vetter
  2019-10-25 11:22       ` Noralf Trønnes
  2019-10-25  8:08     ` Thomas Zimmermann
  1 sibling, 1 reply; 31+ messages in thread
From: Daniel Vetter @ 2019-10-25  8:00 UTC (permalink / raw)
  To: Thomas Zimmermann; +Cc: sean, dri-devel, airlied, sam

On Fri, Oct 25, 2019 at 09:47:46AM +0200, Daniel Vetter wrote:
> On Thu, Oct 24, 2019 at 04:42:37PM +0200, Thomas Zimmermann wrote:
> > Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> > ---
> >  drivers/gpu/drm/udl/udl_drv.c     |   3 +
> >  drivers/gpu/drm/udl/udl_drv.h     |   4 -
> >  drivers/gpu/drm/udl/udl_fb.c      | 263 +-----------------------------
> >  drivers/gpu/drm/udl/udl_main.c    |   2 -
> >  drivers/gpu/drm/udl/udl_modeset.c |   3 +-
> >  5 files changed, 8 insertions(+), 267 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/udl/udl_drv.c b/drivers/gpu/drm/udl/udl_drv.c
> > index 15ad7a338f9d..6beaa1109c2c 100644
> > --- a/drivers/gpu/drm/udl/udl_drv.c
> > +++ b/drivers/gpu/drm/udl/udl_drv.c
> > @@ -7,6 +7,7 @@
> >  
> >  #include <drm/drm_crtc_helper.h>
> >  #include <drm/drm_drv.h>
> > +#include <drm/drm_fb_helper.h>
> >  #include <drm/drm_file.h>
> >  #include <drm/drm_ioctl.h>
> >  #include <drm/drm_probe_helper.h>
> > @@ -62,6 +63,8 @@ static struct drm_driver driver = {
> >  	.driver_features = DRIVER_MODESET | DRIVER_GEM,
> >  	.release = udl_driver_release,
> >  
> > +	.lastclose = drm_fb_helper_lastclose,
> > +
> >  	/* gem hooks */
> >  	.gem_free_object_unlocked = udl_gem_free_object,
> >  	.gem_vm_ops = &udl_gem_vm_ops,
> > diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h
> > index 12a970fd9a87..5f8a7ac084f6 100644
> > --- a/drivers/gpu/drm/udl/udl_drv.h
> > +++ b/drivers/gpu/drm/udl/udl_drv.h
> > @@ -50,8 +50,6 @@ struct urb_list {
> >  	size_t size;
> >  };
> >  
> > -struct udl_fbdev;
> > -
> >  struct udl_device {
> >  	struct drm_device drm;
> >  	struct device *dev;
> > @@ -65,7 +63,6 @@ struct udl_device {
> >  	struct urb_list urbs;
> >  	atomic_t lost_pixels; /* 1 = a render op failed. Need screen refresh */
> >  
> > -	struct udl_fbdev *fbdev;
> >  	char mode_buf[1024];
> >  	uint32_t mode_buf_len;
> >  	atomic_t bytes_rendered; /* raw pixel-bytes driver asked to render */
> > @@ -111,7 +108,6 @@ int udl_init(struct udl_device *udl);
> >  void udl_fini(struct drm_device *dev);
> >  
> >  int udl_fbdev_init(struct drm_device *dev);
> > -void udl_fbdev_cleanup(struct drm_device *dev);
> >  void udl_fbdev_unplug(struct drm_device *dev);
> >  struct drm_framebuffer *
> >  udl_fb_user_fb_create(struct drm_device *dev,
> > diff --git a/drivers/gpu/drm/udl/udl_fb.c b/drivers/gpu/drm/udl/udl_fb.c
> > index ef3504d06343..43a1da3a56c3 100644
> > --- a/drivers/gpu/drm/udl/udl_fb.c
> > +++ b/drivers/gpu/drm/udl/udl_fb.c
> > @@ -19,19 +19,9 @@
> >  
> >  #include "udl_drv.h"
> >  
> > -#define DL_DEFIO_WRITE_DELAY    (HZ/20) /* fb_deferred_io.delay in jiffies */
> > -
> > -static int fb_defio = 0;  /* Optionally enable experimental fb_defio mmap support */

Correction on my enthusiasm, this here is a problem:

The udl defio support as-is is broken, fbdev defio and shmem are fight
over the page flags. Not a problem with the old code, since disabled by
default. But will be a problem with the new code. I guess you didn't test
fbdev mmap? We unfortunately also lack an easy igt testcase for this ...

The problem is fairly tricky to solve, here's an untested idea that might
work:

https://dri.freedesktop.org/docs/drm/gpu/todo.html#generic-fbdev-defio-support

Cheers, Daniel



> >  static int fb_bpp = 16;
> >  
> >  module_param(fb_bpp, int, S_IWUSR | S_IRUSR | S_IWGRP | S_IRGRP);
> > -module_param(fb_defio, int, S_IWUSR | S_IRUSR | S_IWGRP | S_IRGRP);
> > -
> > -struct udl_fbdev {
> > -	struct drm_fb_helper helper; /* must be first */
> > -	struct udl_framebuffer ufb;
> > -	int fb_count;
> > -};
> >  
> >  #define DL_ALIGN_UP(x, a) ALIGN(x, a)
> >  #define DL_ALIGN_DOWN(x, a) ALIGN_DOWN(x, a)
> > @@ -157,123 +147,6 @@ int udl_handle_damage(struct udl_framebuffer *fb, int x, int y,
> >  	return 0;
> >  }
> >  
> > -static int udl_fb_mmap(struct fb_info *info, struct vm_area_struct *vma)
> > -{
> > -	unsigned long start = vma->vm_start;
> > -	unsigned long size = vma->vm_end - vma->vm_start;
> > -	unsigned long offset;
> > -	unsigned long page, pos;
> > -
> > -	if (vma->vm_pgoff > (~0UL >> PAGE_SHIFT))
> > -		return -EINVAL;
> > -
> > -	offset = vma->vm_pgoff << PAGE_SHIFT;
> > -
> > -	if (offset > info->fix.smem_len || size > info->fix.smem_len - offset)
> > -		return -EINVAL;
> > -
> > -	pos = (unsigned long)info->fix.smem_start + offset;
> > -
> > -	pr_debug("mmap() framebuffer addr:%lu size:%lu\n",
> > -		  pos, size);
> > -
> > -	/* We don't want the framebuffer to be mapped encrypted */
> > -	vma->vm_page_prot = pgprot_decrypted(vma->vm_page_prot);
> > -
> > -	while (size > 0) {
> > -		page = vmalloc_to_pfn((void *)pos);
> > -		if (remap_pfn_range(vma, start, page, PAGE_SIZE, PAGE_SHARED))
> > -			return -EAGAIN;
> > -
> > -		start += PAGE_SIZE;
> > -		pos += PAGE_SIZE;
> > -		if (size > PAGE_SIZE)
> > -			size -= PAGE_SIZE;
> > -		else
> > -			size = 0;
> > -	}
> > -
> > -	/* VM_IO | VM_DONTEXPAND | VM_DONTDUMP are set by remap_pfn_range() */
> > -	return 0;
> > -}
> > -
> > -/*
> > - * It's common for several clients to have framebuffer open simultaneously.
> > - * e.g. both fbcon and X. Makes things interesting.
> > - * Assumes caller is holding info->lock (for open and release at least)
> > - */
> > -static int udl_fb_open(struct fb_info *info, int user)
> > -{
> > -	struct udl_fbdev *ufbdev = info->par;
> > -	struct drm_device *dev = ufbdev->ufb.base.dev;
> > -	struct udl_device *udl = to_udl(dev);
> > -
> > -	/* If the USB device is gone, we don't accept new opens */
> > -	if (drm_dev_is_unplugged(&udl->drm))
> > -		return -ENODEV;
> > -
> > -	ufbdev->fb_count++;
> > -
> > -#ifdef CONFIG_DRM_FBDEV_EMULATION
> > -	if (fb_defio && (info->fbdefio == NULL)) {
> > -		/* enable defio at last moment if not disabled by client */
> > -
> > -		struct fb_deferred_io *fbdefio;
> > -
> > -		fbdefio = kzalloc(sizeof(struct fb_deferred_io), GFP_KERNEL);
> > -
> > -		if (fbdefio) {
> > -			fbdefio->delay = DL_DEFIO_WRITE_DELAY;
> > -			fbdefio->deferred_io = drm_fb_helper_deferred_io;
> > -		}
> > -
> > -		info->fbdefio = fbdefio;
> > -		fb_deferred_io_init(info);
> > -	}
> > -#endif
> > -
> > -	pr_debug("open /dev/fb%d user=%d fb_info=%p count=%d\n",
> > -		  info->node, user, info, ufbdev->fb_count);
> > -
> > -	return 0;
> > -}
> > -
> > -
> > -/*
> > - * Assumes caller is holding info->lock mutex (for open and release at least)
> > - */
> > -static int udl_fb_release(struct fb_info *info, int user)
> > -{
> > -	struct udl_fbdev *ufbdev = info->par;
> > -
> > -	ufbdev->fb_count--;
> > -
> > -#ifdef CONFIG_DRM_FBDEV_EMULATION
> > -	if ((ufbdev->fb_count == 0) && (info->fbdefio)) {
> > -		fb_deferred_io_cleanup(info);
> > -		kfree(info->fbdefio);
> > -		info->fbdefio = NULL;
> > -		info->fbops->fb_mmap = udl_fb_mmap;
> > -	}
> > -#endif
> > -
> > -	pr_debug("released /dev/fb%d user=%d count=%d\n",
> > -		info->node, user, ufbdev->fb_count);
> > -
> > -	return 0;
> > -}
> > -
> > -static struct fb_ops udlfb_ops = {
> > -	.owner = THIS_MODULE,
> > -	DRM_FB_HELPER_DEFAULT_OPS,
> > -	.fb_fillrect = drm_fb_helper_sys_fillrect,
> > -	.fb_copyarea = drm_fb_helper_sys_copyarea,
> > -	.fb_imageblit = drm_fb_helper_sys_imageblit,
> > -	.fb_mmap = udl_fb_mmap,
> > -	.fb_open = udl_fb_open,
> > -	.fb_release = udl_fb_release,
> > -};
> > -
> >  static int udl_user_framebuffer_dirty(struct drm_framebuffer *fb,
> >  				      struct drm_file *file,
> >  				      unsigned flags, unsigned color,
> > @@ -346,150 +219,20 @@ udl_framebuffer_init(struct drm_device *dev,
> >  	return ret;
> >  }
> >  
> > -
> > -static int udlfb_create(struct drm_fb_helper *helper,
> > -			struct drm_fb_helper_surface_size *sizes)
> > -{
> > -	struct udl_fbdev *ufbdev =
> > -		container_of(helper, struct udl_fbdev, helper);
> > -	struct drm_device *dev = ufbdev->helper.dev;
> > -	struct fb_info *info;
> > -	struct drm_framebuffer *fb;
> > -	struct drm_mode_fb_cmd2 mode_cmd;
> > -	struct udl_gem_object *obj;
> > -	uint32_t size;
> > -	int ret = 0;
> > -
> > -	if (sizes->surface_bpp == 24)
> > -		sizes->surface_bpp = 32;
> > -
> > -	mode_cmd.width = sizes->surface_width;
> > -	mode_cmd.height = sizes->surface_height;
> > -	mode_cmd.pitches[0] = mode_cmd.width * ((sizes->surface_bpp + 7) / 8);
> > -
> > -	mode_cmd.pixel_format = drm_mode_legacy_fb_format(sizes->surface_bpp,
> > -							  sizes->surface_depth);
> > -
> > -	size = mode_cmd.pitches[0] * mode_cmd.height;
> > -	size = ALIGN(size, PAGE_SIZE);
> > -
> > -	obj = udl_gem_alloc_object(dev, size);
> > -	if (!obj)
> > -		goto out;
> > -
> > -	ret = udl_gem_vmap(obj);
> > -	if (ret) {
> > -		DRM_ERROR("failed to vmap fb\n");
> > -		goto out_gfree;
> > -	}
> > -
> > -	info = drm_fb_helper_alloc_fbi(helper);
> > -	if (IS_ERR(info)) {
> > -		ret = PTR_ERR(info);
> > -		goto out_gfree;
> > -	}
> > -
> > -	ret = udl_framebuffer_init(dev, &ufbdev->ufb, &mode_cmd, obj);
> > -	if (ret)
> > -		goto out_gfree;
> > -
> > -	fb = &ufbdev->ufb.base;
> > -
> > -	ufbdev->helper.fb = fb;
> > -
> > -	info->screen_base = ufbdev->ufb.obj->vmapping;
> > -	info->fix.smem_len = size;
> > -	info->fix.smem_start = (unsigned long)ufbdev->ufb.obj->vmapping;
> > -
> > -	info->fbops = &udlfb_ops;
> > -	drm_fb_helper_fill_info(info, &ufbdev->helper, sizes);
> > -
> > -	DRM_DEBUG_KMS("allocated %dx%d vmal %p\n",
> > -		      fb->width, fb->height,
> > -		      ufbdev->ufb.obj->vmapping);
> > -
> > -	return ret;
> > -out_gfree:
> > -	drm_gem_object_put_unlocked(&ufbdev->ufb.obj->base);
> > -out:
> > -	return ret;
> > -}
> > -
> > -static const struct drm_fb_helper_funcs udl_fb_helper_funcs = {
> > -	.fb_probe = udlfb_create,
> > -};
> > -
> > -static void udl_fbdev_destroy(struct drm_device *dev,
> > -			      struct udl_fbdev *ufbdev)
> > -{
> > -	drm_fb_helper_unregister_fbi(&ufbdev->helper);
> > -	drm_fb_helper_fini(&ufbdev->helper);
> > -	if (ufbdev->ufb.obj) {
> > -		drm_framebuffer_unregister_private(&ufbdev->ufb.base);
> > -		drm_framebuffer_cleanup(&ufbdev->ufb.base);
> > -		drm_gem_object_put_unlocked(&ufbdev->ufb.obj->base);
> > -	}
> > -}
> > -
> >  int udl_fbdev_init(struct drm_device *dev)
> >  {
> > -	struct udl_device *udl = to_udl(dev);
> >  	int bpp_sel = fb_bpp;
> > -	struct udl_fbdev *ufbdev;
> >  	int ret;
> >  
> > -	ufbdev = kzalloc(sizeof(struct udl_fbdev), GFP_KERNEL);
> > -	if (!ufbdev)
> > -		return -ENOMEM;
> > -
> > -	udl->fbdev = ufbdev;
> > -
> > -	drm_fb_helper_prepare(dev, &ufbdev->helper, &udl_fb_helper_funcs);
> > -
> > -	ret = drm_fb_helper_init(dev, &ufbdev->helper, 1);
> > -	if (ret)
> > -		goto free;
> > -
> > -	ret = drm_fb_helper_single_add_all_connectors(&ufbdev->helper);
> > +	ret = drm_fbdev_generic_setup(dev, bpp_sel);
> >  	if (ret)
> > -		goto fini;
> > -
> > -	/* disable all the possible outputs/crtcs before entering KMS mode */
> > -	drm_helper_disable_unused_functions(dev);
> > -
> > -	ret = drm_fb_helper_initial_config(&ufbdev->helper, bpp_sel);
> > -	if (ret)
> > -		goto fini;
> > -
> > +		return ret;
> >  	return 0;
> > -
> > -fini:
> > -	drm_fb_helper_fini(&ufbdev->helper);
> > -free:
> > -	kfree(ufbdev);
> > -	return ret;
> > -}
> > -
> > -void udl_fbdev_cleanup(struct drm_device *dev)
> > -{
> > -	struct udl_device *udl = to_udl(dev);
> > -	if (!udl->fbdev)
> > -		return;
> > -
> > -	udl_fbdev_destroy(dev, udl->fbdev);
> > -	kfree(udl->fbdev);
> > -	udl->fbdev = NULL;
> >  }
> >  
> >  void udl_fbdev_unplug(struct drm_device *dev)
> >  {
> > -	struct udl_device *udl = to_udl(dev);
> > -	struct udl_fbdev *ufbdev;
> > -	if (!udl->fbdev)
> > -		return;
> > -
> > -	ufbdev = udl->fbdev;
> > -	drm_fb_helper_unlink_fbi(&ufbdev->helper);
> > +	drm_fb_helper_unlink_fbi(dev->fb_helper);
> 
> Uh I think this here can be all garbage-collected. The generic fbdev
> already calls drm_fb_helper_unregister_fbi, which calls
> unregister_framebuffer, which is a strict superset of unlink_framebuffer.
> The later isn't really enough for hotunplug.
> 
> So for generic fbdev you really shouldn't need any cleanup nor unplug
> functions anymore.
> 
> Also udl is the only caller of drm_fb_helper_unlink_fbi, so we could drop
> that. Which removes the only external caller of unlink_framebuffer, so we
> can drop that too. Care to follow up with those 2 patches?
> 
> Aside from this this patch here looks great!
> -Daniel
> 
> >  }
> >  
> >  struct drm_framebuffer *
> > diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c
> > index 4e854e017390..2a6399290f09 100644
> > --- a/drivers/gpu/drm/udl/udl_main.c
> > +++ b/drivers/gpu/drm/udl/udl_main.c
> > @@ -367,6 +367,4 @@ void udl_fini(struct drm_device *dev)
> >  
> >  	if (udl->urbs.count)
> >  		udl_free_urb_list(dev);
> > -
> > -	udl_fbdev_cleanup(dev);
> >  }
> > diff --git a/drivers/gpu/drm/udl/udl_modeset.c b/drivers/gpu/drm/udl/udl_modeset.c
> > index bc1ab6060dc6..1517d5e881b8 100644
> > --- a/drivers/gpu/drm/udl/udl_modeset.c
> > +++ b/drivers/gpu/drm/udl/udl_modeset.c
> > @@ -10,6 +10,7 @@
> >   */
> >  
> >  #include <drm/drm_crtc_helper.h>
> > +#include <drm/drm_fb_helper.h>
> >  #include <drm/drm_modeset_helper_vtables.h>
> >  #include <drm/drm_vblank.h>
> >  
> > @@ -422,7 +423,7 @@ static int udl_crtc_init(struct drm_device *dev)
> >  
> >  static const struct drm_mode_config_funcs udl_mode_funcs = {
> >  	.fb_create = udl_fb_user_fb_create,
> > -	.output_poll_changed = NULL,
> > +	.output_poll_changed = drm_fb_helper_output_poll_changed,
> >  };
> >  
> >  int udl_modeset_init(struct drm_device *dev)
> > -- 
> > 2.23.0
> > 
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

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

^ permalink raw reply	[flat|nested] 31+ messages in thread
* Re: [PATCH 5/5] drm/udl: Replace fbdev code with generic emulation
  2019-10-25  7:47   ` Daniel Vetter
  2019-10-25  8:00     ` Daniel Vetter
@ 2019-10-25  8:08     ` Thomas Zimmermann
  2019-10-25 19:10       ` Daniel Vetter
  1 sibling, 1 reply; 31+ messages in thread
From: Thomas Zimmermann @ 2019-10-25  8:08 UTC (permalink / raw)
  To: Daniel Vetter; +Cc: sam, airlied, sean, dri-devel


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

Hi

Am 25.10.19 um 09:47 schrieb Daniel Vetter:
> On Thu, Oct 24, 2019 at 04:42:37PM +0200, Thomas Zimmermann wrote:
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
>>  drivers/gpu/drm/udl/udl_drv.c     |   3 +
>>  drivers/gpu/drm/udl/udl_drv.h     |   4 -
>>  drivers/gpu/drm/udl/udl_fb.c      | 263 +-----------------------------
>>  drivers/gpu/drm/udl/udl_main.c    |   2 -
>>  drivers/gpu/drm/udl/udl_modeset.c |   3 +-
>>  5 files changed, 8 insertions(+), 267 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/udl/udl_drv.c b/drivers/gpu/drm/udl/udl_drv.c
>> index 15ad7a338f9d..6beaa1109c2c 100644
>> --- a/drivers/gpu/drm/udl/udl_drv.c
>> +++ b/drivers/gpu/drm/udl/udl_drv.c
>> @@ -7,6 +7,7 @@
>>  
>>  #include <drm/drm_crtc_helper.h>
>>  #include <drm/drm_drv.h>
>> +#include <drm/drm_fb_helper.h>
>>  #include <drm/drm_file.h>
>>  #include <drm/drm_ioctl.h>
>>  #include <drm/drm_probe_helper.h>
>> @@ -62,6 +63,8 @@ static struct drm_driver driver = {
>>  	.driver_features = DRIVER_MODESET | DRIVER_GEM,
>>  	.release = udl_driver_release,
>>  
>> +	.lastclose = drm_fb_helper_lastclose,
>> +
>>  	/* gem hooks */
>>  	.gem_free_object_unlocked = udl_gem_free_object,
>>  	.gem_vm_ops = &udl_gem_vm_ops,
>> diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h
>> index 12a970fd9a87..5f8a7ac084f6 100644
>> --- a/drivers/gpu/drm/udl/udl_drv.h
>> +++ b/drivers/gpu/drm/udl/udl_drv.h
>> @@ -50,8 +50,6 @@ struct urb_list {
>>  	size_t size;
>>  };
>>  
>> -struct udl_fbdev;
>> -
>>  struct udl_device {
>>  	struct drm_device drm;
>>  	struct device *dev;
>> @@ -65,7 +63,6 @@ struct udl_device {
>>  	struct urb_list urbs;
>>  	atomic_t lost_pixels; /* 1 = a render op failed. Need screen refresh */
>>  
>> -	struct udl_fbdev *fbdev;
>>  	char mode_buf[1024];
>>  	uint32_t mode_buf_len;
>>  	atomic_t bytes_rendered; /* raw pixel-bytes driver asked to render */
>> @@ -111,7 +108,6 @@ int udl_init(struct udl_device *udl);
>>  void udl_fini(struct drm_device *dev);
>>  
>>  int udl_fbdev_init(struct drm_device *dev);
>> -void udl_fbdev_cleanup(struct drm_device *dev);
>>  void udl_fbdev_unplug(struct drm_device *dev);
>>  struct drm_framebuffer *
>>  udl_fb_user_fb_create(struct drm_device *dev,
>> diff --git a/drivers/gpu/drm/udl/udl_fb.c b/drivers/gpu/drm/udl/udl_fb.c
>> index ef3504d06343..43a1da3a56c3 100644
>> --- a/drivers/gpu/drm/udl/udl_fb.c
>> +++ b/drivers/gpu/drm/udl/udl_fb.c
>> @@ -19,19 +19,9 @@
>>  
>>  #include "udl_drv.h"
>>  
>> -#define DL_DEFIO_WRITE_DELAY    (HZ/20) /* fb_deferred_io.delay in jiffies */
>> -
>> -static int fb_defio = 0;  /* Optionally enable experimental fb_defio mmap support */
>>  static int fb_bpp = 16;
>>  
>>  module_param(fb_bpp, int, S_IWUSR | S_IRUSR | S_IWGRP | S_IRGRP);
>> -module_param(fb_defio, int, S_IWUSR | S_IRUSR | S_IWGRP | S_IRGRP);
>> -
>> -struct udl_fbdev {
>> -	struct drm_fb_helper helper; /* must be first */
>> -	struct udl_framebuffer ufb;
>> -	int fb_count;
>> -};
>>  
>>  #define DL_ALIGN_UP(x, a) ALIGN(x, a)
>>  #define DL_ALIGN_DOWN(x, a) ALIGN_DOWN(x, a)
>> @@ -157,123 +147,6 @@ int udl_handle_damage(struct udl_framebuffer *fb, int x, int y,
>>  	return 0;
>>  }
>>  
>> -static int udl_fb_mmap(struct fb_info *info, struct vm_area_struct *vma)
>> -{
>> -	unsigned long start = vma->vm_start;
>> -	unsigned long size = vma->vm_end - vma->vm_start;
>> -	unsigned long offset;
>> -	unsigned long page, pos;
>> -
>> -	if (vma->vm_pgoff > (~0UL >> PAGE_SHIFT))
>> -		return -EINVAL;
>> -
>> -	offset = vma->vm_pgoff << PAGE_SHIFT;
>> -
>> -	if (offset > info->fix.smem_len || size > info->fix.smem_len - offset)
>> -		return -EINVAL;
>> -
>> -	pos = (unsigned long)info->fix.smem_start + offset;
>> -
>> -	pr_debug("mmap() framebuffer addr:%lu size:%lu\n",
>> -		  pos, size);
>> -
>> -	/* We don't want the framebuffer to be mapped encrypted */
>> -	vma->vm_page_prot = pgprot_decrypted(vma->vm_page_prot);
>> -
>> -	while (size > 0) {
>> -		page = vmalloc_to_pfn((void *)pos);
>> -		if (remap_pfn_range(vma, start, page, PAGE_SIZE, PAGE_SHARED))
>> -			return -EAGAIN;
>> -
>> -		start += PAGE_SIZE;
>> -		pos += PAGE_SIZE;
>> -		if (size > PAGE_SIZE)
>> -			size -= PAGE_SIZE;
>> -		else
>> -			size = 0;
>> -	}
>> -
>> -	/* VM_IO | VM_DONTEXPAND | VM_DONTDUMP are set by remap_pfn_range() */
>> -	return 0;
>> -}
>> -
>> -/*
>> - * It's common for several clients to have framebuffer open simultaneously.
>> - * e.g. both fbcon and X. Makes things interesting.
>> - * Assumes caller is holding info->lock (for open and release at least)
>> - */
>> -static int udl_fb_open(struct fb_info *info, int user)
>> -{
>> -	struct udl_fbdev *ufbdev = info->par;
>> -	struct drm_device *dev = ufbdev->ufb.base.dev;
>> -	struct udl_device *udl = to_udl(dev);
>> -
>> -	/* If the USB device is gone, we don't accept new opens */
>> -	if (drm_dev_is_unplugged(&udl->drm))
>> -		return -ENODEV;
>> -
>> -	ufbdev->fb_count++;
>> -
>> -#ifdef CONFIG_DRM_FBDEV_EMULATION
>> -	if (fb_defio && (info->fbdefio == NULL)) {
>> -		/* enable defio at last moment if not disabled by client */
>> -
>> -		struct fb_deferred_io *fbdefio;
>> -
>> -		fbdefio = kzalloc(sizeof(struct fb_deferred_io), GFP_KERNEL);
>> -
>> -		if (fbdefio) {
>> -			fbdefio->delay = DL_DEFIO_WRITE_DELAY;
>> -			fbdefio->deferred_io = drm_fb_helper_deferred_io;
>> -		}
>> -
>> -		info->fbdefio = fbdefio;
>> -		fb_deferred_io_init(info);
>> -	}
>> -#endif
>> -
>> -	pr_debug("open /dev/fb%d user=%d fb_info=%p count=%d\n",
>> -		  info->node, user, info, ufbdev->fb_count);
>> -
>> -	return 0;
>> -}
>> -
>> -
>> -/*
>> - * Assumes caller is holding info->lock mutex (for open and release at least)
>> - */
>> -static int udl_fb_release(struct fb_info *info, int user)
>> -{
>> -	struct udl_fbdev *ufbdev = info->par;
>> -
>> -	ufbdev->fb_count--;
>> -
>> -#ifdef CONFIG_DRM_FBDEV_EMULATION
>> -	if ((ufbdev->fb_count == 0) && (info->fbdefio)) {
>> -		fb_deferred_io_cleanup(info);
>> -		kfree(info->fbdefio);
>> -		info->fbdefio = NULL;
>> -		info->fbops->fb_mmap = udl_fb_mmap;
>> -	}
>> -#endif
>> -
>> -	pr_debug("released /dev/fb%d user=%d count=%d\n",
>> -		info->node, user, ufbdev->fb_count);
>> -
>> -	return 0;
>> -}
>> -
>> -static struct fb_ops udlfb_ops = {
>> -	.owner = THIS_MODULE,
>> -	DRM_FB_HELPER_DEFAULT_OPS,
>> -	.fb_fillrect = drm_fb_helper_sys_fillrect,
>> -	.fb_copyarea = drm_fb_helper_sys_copyarea,
>> -	.fb_imageblit = drm_fb_helper_sys_imageblit,
>> -	.fb_mmap = udl_fb_mmap,
>> -	.fb_open = udl_fb_open,
>> -	.fb_release = udl_fb_release,
>> -};
>> -
>>  static int udl_user_framebuffer_dirty(struct drm_framebuffer *fb,
>>  				      struct drm_file *file,
>>  				      unsigned flags, unsigned color,
>> @@ -346,150 +219,20 @@ udl_framebuffer_init(struct drm_device *dev,
>>  	return ret;
>>  }
>>  
>> -
>> -static int udlfb_create(struct drm_fb_helper *helper,
>> -			struct drm_fb_helper_surface_size *sizes)
>> -{
>> -	struct udl_fbdev *ufbdev =
>> -		container_of(helper, struct udl_fbdev, helper);
>> -	struct drm_device *dev = ufbdev->helper.dev;
>> -	struct fb_info *info;
>> -	struct drm_framebuffer *fb;
>> -	struct drm_mode_fb_cmd2 mode_cmd;
>> -	struct udl_gem_object *obj;
>> -	uint32_t size;
>> -	int ret = 0;
>> -
>> -	if (sizes->surface_bpp == 24)
>> -		sizes->surface_bpp = 32;
>> -
>> -	mode_cmd.width = sizes->surface_width;
>> -	mode_cmd.height = sizes->surface_height;
>> -	mode_cmd.pitches[0] = mode_cmd.width * ((sizes->surface_bpp + 7) / 8);
>> -
>> -	mode_cmd.pixel_format = drm_mode_legacy_fb_format(sizes->surface_bpp,
>> -							  sizes->surface_depth);
>> -
>> -	size = mode_cmd.pitches[0] * mode_cmd.height;
>> -	size = ALIGN(size, PAGE_SIZE);
>> -
>> -	obj = udl_gem_alloc_object(dev, size);
>> -	if (!obj)
>> -		goto out;
>> -
>> -	ret = udl_gem_vmap(obj);
>> -	if (ret) {
>> -		DRM_ERROR("failed to vmap fb\n");
>> -		goto out_gfree;
>> -	}
>> -
>> -	info = drm_fb_helper_alloc_fbi(helper);
>> -	if (IS_ERR(info)) {
>> -		ret = PTR_ERR(info);
>> -		goto out_gfree;
>> -	}
>> -
>> -	ret = udl_framebuffer_init(dev, &ufbdev->ufb, &mode_cmd, obj);
>> -	if (ret)
>> -		goto out_gfree;
>> -
>> -	fb = &ufbdev->ufb.base;
>> -
>> -	ufbdev->helper.fb = fb;
>> -
>> -	info->screen_base = ufbdev->ufb.obj->vmapping;
>> -	info->fix.smem_len = size;
>> -	info->fix.smem_start = (unsigned long)ufbdev->ufb.obj->vmapping;
>> -
>> -	info->fbops = &udlfb_ops;
>> -	drm_fb_helper_fill_info(info, &ufbdev->helper, sizes);
>> -
>> -	DRM_DEBUG_KMS("allocated %dx%d vmal %p\n",
>> -		      fb->width, fb->height,
>> -		      ufbdev->ufb.obj->vmapping);
>> -
>> -	return ret;
>> -out_gfree:
>> -	drm_gem_object_put_unlocked(&ufbdev->ufb.obj->base);
>> -out:
>> -	return ret;
>> -}
>> -
>> -static const struct drm_fb_helper_funcs udl_fb_helper_funcs = {
>> -	.fb_probe = udlfb_create,
>> -};
>> -
>> -static void udl_fbdev_destroy(struct drm_device *dev,
>> -			      struct udl_fbdev *ufbdev)
>> -{
>> -	drm_fb_helper_unregister_fbi(&ufbdev->helper);
>> -	drm_fb_helper_fini(&ufbdev->helper);
>> -	if (ufbdev->ufb.obj) {
>> -		drm_framebuffer_unregister_private(&ufbdev->ufb.base);
>> -		drm_framebuffer_cleanup(&ufbdev->ufb.base);
>> -		drm_gem_object_put_unlocked(&ufbdev->ufb.obj->base);
>> -	}
>> -}
>> -
>>  int udl_fbdev_init(struct drm_device *dev)
>>  {
>> -	struct udl_device *udl = to_udl(dev);
>>  	int bpp_sel = fb_bpp;
>> -	struct udl_fbdev *ufbdev;
>>  	int ret;
>>  
>> -	ufbdev = kzalloc(sizeof(struct udl_fbdev), GFP_KERNEL);
>> -	if (!ufbdev)
>> -		return -ENOMEM;
>> -
>> -	udl->fbdev = ufbdev;
>> -
>> -	drm_fb_helper_prepare(dev, &ufbdev->helper, &udl_fb_helper_funcs);
>> -
>> -	ret = drm_fb_helper_init(dev, &ufbdev->helper, 1);
>> -	if (ret)
>> -		goto free;
>> -
>> -	ret = drm_fb_helper_single_add_all_connectors(&ufbdev->helper);
>> +	ret = drm_fbdev_generic_setup(dev, bpp_sel);
>>  	if (ret)
>> -		goto fini;
>> -
>> -	/* disable all the possible outputs/crtcs before entering KMS mode */
>> -	drm_helper_disable_unused_functions(dev);
>> -
>> -	ret = drm_fb_helper_initial_config(&ufbdev->helper, bpp_sel);
>> -	if (ret)
>> -		goto fini;
>> -
>> +		return ret;
>>  	return 0;
>> -
>> -fini:
>> -	drm_fb_helper_fini(&ufbdev->helper);
>> -free:
>> -	kfree(ufbdev);
>> -	return ret;
>> -}
>> -
>> -void udl_fbdev_cleanup(struct drm_device *dev)
>> -{
>> -	struct udl_device *udl = to_udl(dev);
>> -	if (!udl->fbdev)
>> -		return;
>> -
>> -	udl_fbdev_destroy(dev, udl->fbdev);
>> -	kfree(udl->fbdev);
>> -	udl->fbdev = NULL;
>>  }
>>  
>>  void udl_fbdev_unplug(struct drm_device *dev)
>>  {
>> -	struct udl_device *udl = to_udl(dev);
>> -	struct udl_fbdev *ufbdev;
>> -	if (!udl->fbdev)
>> -		return;
>> -
>> -	ufbdev = udl->fbdev;
>> -	drm_fb_helper_unlink_fbi(&ufbdev->helper);
>> +	drm_fb_helper_unlink_fbi(dev->fb_helper);
> 
> Uh I think this here can be all garbage-collected. The generic fbdev
> already calls drm_fb_helper_unregister_fbi, which calls
> unregister_framebuffer, which is a strict superset of unlink_framebuffer.
> The later isn't really enough for hotunplug.
> 
> So for generic fbdev you really shouldn't need any cleanup nor unplug
> functions anymore.
> 
> Also udl is the only caller of drm_fb_helper_unlink_fbi, so we could drop
> that. Which removes the only external caller of unlink_framebuffer, so we
> can drop that too. Care to follow up with those 2 patches?

Sure. BTW is there a policy for keeping potentially useful functions? I
recently converted vboxvideo to generic fbdev and notices that there are
no more users of drm_fb_helper_fbdev_{setup,teardown}(). But there's
still a TODO item for converting drivers over to them. Shall they remain
in the code base?

I have quite a few patches for udl in the making. After converting to
shmem and generic fbdev, udl_framebuffer can be replaced with the GEM
framebuffer code. The udl modesetting code is an epicenter of simple
display pipelines. Converting this over should give atomic mode setting.

Best regards
Thomas

> Aside from this this patch here looks great!
> -Daniel
> 
>>  }
>>  
>>  struct drm_framebuffer *
>> diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c
>> index 4e854e017390..2a6399290f09 100644
>> --- a/drivers/gpu/drm/udl/udl_main.c
>> +++ b/drivers/gpu/drm/udl/udl_main.c
>> @@ -367,6 +367,4 @@ void udl_fini(struct drm_device *dev)
>>  
>>  	if (udl->urbs.count)
>>  		udl_free_urb_list(dev);
>> -
>> -	udl_fbdev_cleanup(dev);
>>  }
>> diff --git a/drivers/gpu/drm/udl/udl_modeset.c b/drivers/gpu/drm/udl/udl_modeset.c
>> index bc1ab6060dc6..1517d5e881b8 100644
>> --- a/drivers/gpu/drm/udl/udl_modeset.c
>> +++ b/drivers/gpu/drm/udl/udl_modeset.c
>> @@ -10,6 +10,7 @@
>>   */
>>  
>>  #include <drm/drm_crtc_helper.h>
>> +#include <drm/drm_fb_helper.h>
>>  #include <drm/drm_modeset_helper_vtables.h>
>>  #include <drm/drm_vblank.h>
>>  
>> @@ -422,7 +423,7 @@ static int udl_crtc_init(struct drm_device *dev)
>>  
>>  static const struct drm_mode_config_funcs udl_mode_funcs = {
>>  	.fb_create = udl_fb_user_fb_create,
>> -	.output_poll_changed = NULL,
>> +	.output_poll_changed = drm_fb_helper_output_poll_changed,
>>  };
>>  
>>  int udl_modeset_init(struct drm_device *dev)
>> -- 
>> 2.23.0
>>
> 

-- 
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: 159 bytes --]

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

^ permalink raw reply	[flat|nested] 31+ messages in thread
* Re: [PATCH 3/5] drm/udl: Add GEM object functions for free(), vmap(), and vunmap()
@ 2019-10-25  9:28         ` Daniel Vetter
  0 siblings, 0 replies; 31+ messages in thread
From: Daniel Vetter @ 2019-10-25  9:28 UTC (permalink / raw)
  To: Thomas Zimmermann; +Cc: Sam Ravnborg, Dave Airlie, Sean Paul, dri-devel

On Fri, Oct 25, 2019 at 9:59 AM Thomas Zimmermann <tzimmermann@suse.de> wrote
>
> Hi
>
> Am 25.10.19 um 09:40 schrieb Daniel Vetter:
> > On Thu, Oct 24, 2019 at 04:42:35PM +0200, Thomas Zimmermann wrote:
> >> Implementing vmap() and vunmap() of struct drm_gem_object_funcs is
> >> required by generic fbdev emulation. Supporting free() is easy as
> >> well. More udl-specific interfaces can probably be replaced by GEM
> >> object functions in later patches.
> >>
> >> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> >> ---
> >>  drivers/gpu/drm/udl/udl_gem.c | 34 ++++++++++++++++++++++++++++++++++
> >>  1 file changed, 34 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/udl/udl_gem.c b/drivers/gpu/drm/udl/udl_gem.c
> >> index 3ea0cd9ae2d6..6ca097c270d6 100644
> >> --- a/drivers/gpu/drm/udl/udl_gem.c
> >> +++ b/drivers/gpu/drm/udl/udl_gem.c
> >> @@ -11,6 +11,8 @@
> >>
> >>  #include "udl_drv.h"
> >>
> >> +static const struct drm_gem_object_funcs udl_gem_object_funcs;
> >> +
> >>  struct udl_gem_object *udl_gem_alloc_object(struct drm_device *dev,
> >>                                          size_t size)
> >>  {
> >> @@ -25,6 +27,7 @@ struct udl_gem_object *udl_gem_alloc_object(struct drm_device *dev,
> >>              return NULL;
> >>      }
> >>
> >> +    obj->base.funcs = &udl_gem_object_funcs;
> >>      obj->flags = UDL_BO_CACHEABLE;
> >>      return obj;
> >>  }
> >> @@ -230,3 +233,34 @@ int udl_gem_mmap(struct drm_file *file, struct drm_device *dev,
> >>      mutex_unlock(&udl->gem_lock);
> >>      return ret;
> >>  }
> >> +
> >> +/*
> >> + * Helpers for struct drm_gem_object_funcs
> >> + */
> >> +
> >> +static void udl_gem_object_free(struct drm_gem_object *obj)
> >> +{
> >> +    udl_gem_free_object(obj);
> >> +}
> >> +
> >> +static void *udl_gem_object_vmap(struct drm_gem_object *obj)
> >> +{
> >> +    struct udl_gem_object *uobj = to_udl_bo(obj);
> >> +    int ret;
> >> +
> >> +    ret = udl_gem_vmap(uobj);
> >> +    if (ret)
> >> +            return ERR_PTR(ret);
> >> +    return uobj->vmapping;
> >> +}
> >> +
> >> +static void udl_gem_object_vunmap(struct drm_gem_object *obj, void *vaddr)
> >> +{
> >> +    return udl_gem_vunmap(to_udl_bo(obj));
> >> +}
> >> +
> >> +static const struct drm_gem_object_funcs udl_gem_object_funcs = {
> >> +    .free   = udl_gem_object_free,
> >> +    .vmap   = udl_gem_object_vmap,
> >> +    .vunmap = udl_gem_object_vunmap,
> >
> > Yeah this doesn't work, you need to refcount the vmap here I think. I'd
>
> I see. Should have thought of that...
>
> > say simpler to first cut over to shmem helpers.
>
> Right. I was already attempting the conversion and the udl gem is more
> or less the same as shmem, except for the flags field. [1] The flag
> signals page attributes for mmap-ing [2]. For prime-imported BOs its is
> set to writecombining, and for local BOs it is set to cached. Shmem
> always maps with writecombining.
>
> I don't see why udl BOs are special wrt to mmap-ing. It seems to be some
> kind of optimization. Do you have an idea?
>
> Best regards
> Thomas
>
> [1]
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/udl/udl_drv.h?h=v5.3#n78
> [2]
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/udl/udl_gem.c?h=v5.3#n57

udl doesn't set prefer_shadow = 1, which means compositors will render
directly into the buffer. And for that you want caching. For imported
dma-buf otoh you want wc to make sure it's coherent. Otherwise I guess
udl would need to get the dma_buf_begin/end_cpu_access calls added
(and that would probably be faster than going with wc for everything).
So we do want cachable, it's going to suck otherwise.

But that should be fairly easy to do by overwriting the obj->mmap
callback and wrapping it around the shmem one.

What surprises me more is that udl supports mmap on imported dma-buf,
that's some real quirky stuff. Not sure there's really other drivers
doing that. Might be easier to rip that out :-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 31+ messages in thread
* Re: [PATCH 3/5] drm/udl: Add GEM object functions for free(), vmap(),  and vunmap()
@ 2019-10-25  9:28         ` Daniel Vetter
  0 siblings, 0 replies; 31+ messages in thread
From: Daniel Vetter @ 2019-10-25  9:28 UTC (permalink / raw)
  To: Thomas Zimmermann; +Cc: Sam Ravnborg, Dave Airlie, Sean Paul, dri-devel

On Fri, Oct 25, 2019 at 9:59 AM Thomas Zimmermann <tzimmermann@suse.de> wrote
>
> Hi
>
> Am 25.10.19 um 09:40 schrieb Daniel Vetter:
> > On Thu, Oct 24, 2019 at 04:42:35PM +0200, Thomas Zimmermann wrote:
> >> Implementing vmap() and vunmap() of struct drm_gem_object_funcs is
> >> required by generic fbdev emulation. Supporting free() is easy as
> >> well. More udl-specific interfaces can probably be replaced by GEM
> >> object functions in later patches.
> >>
> >> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> >> ---
> >>  drivers/gpu/drm/udl/udl_gem.c | 34 ++++++++++++++++++++++++++++++++++
> >>  1 file changed, 34 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/udl/udl_gem.c b/drivers/gpu/drm/udl/udl_gem.c
> >> index 3ea0cd9ae2d6..6ca097c270d6 100644
> >> --- a/drivers/gpu/drm/udl/udl_gem.c
> >> +++ b/drivers/gpu/drm/udl/udl_gem.c
> >> @@ -11,6 +11,8 @@
> >>
> >>  #include "udl_drv.h"
> >>
> >> +static const struct drm_gem_object_funcs udl_gem_object_funcs;
> >> +
> >>  struct udl_gem_object *udl_gem_alloc_object(struct drm_device *dev,
> >>                                          size_t size)
> >>  {
> >> @@ -25,6 +27,7 @@ struct udl_gem_object *udl_gem_alloc_object(struct drm_device *dev,
> >>              return NULL;
> >>      }
> >>
> >> +    obj->base.funcs = &udl_gem_object_funcs;
> >>      obj->flags = UDL_BO_CACHEABLE;
> >>      return obj;
> >>  }
> >> @@ -230,3 +233,34 @@ int udl_gem_mmap(struct drm_file *file, struct drm_device *dev,
> >>      mutex_unlock(&udl->gem_lock);
> >>      return ret;
> >>  }
> >> +
> >> +/*
> >> + * Helpers for struct drm_gem_object_funcs
> >> + */
> >> +
> >> +static void udl_gem_object_free(struct drm_gem_object *obj)
> >> +{
> >> +    udl_gem_free_object(obj);
> >> +}
> >> +
> >> +static void *udl_gem_object_vmap(struct drm_gem_object *obj)
> >> +{
> >> +    struct udl_gem_object *uobj = to_udl_bo(obj);
> >> +    int ret;
> >> +
> >> +    ret = udl_gem_vmap(uobj);
> >> +    if (ret)
> >> +            return ERR_PTR(ret);
> >> +    return uobj->vmapping;
> >> +}
> >> +
> >> +static void udl_gem_object_vunmap(struct drm_gem_object *obj, void *vaddr)
> >> +{
> >> +    return udl_gem_vunmap(to_udl_bo(obj));
> >> +}
> >> +
> >> +static const struct drm_gem_object_funcs udl_gem_object_funcs = {
> >> +    .free   = udl_gem_object_free,
> >> +    .vmap   = udl_gem_object_vmap,
> >> +    .vunmap = udl_gem_object_vunmap,
> >
> > Yeah this doesn't work, you need to refcount the vmap here I think. I'd
>
> I see. Should have thought of that...
>
> > say simpler to first cut over to shmem helpers.
>
> Right. I was already attempting the conversion and the udl gem is more
> or less the same as shmem, except for the flags field. [1] The flag
> signals page attributes for mmap-ing [2]. For prime-imported BOs its is
> set to writecombining, and for local BOs it is set to cached. Shmem
> always maps with writecombining.
>
> I don't see why udl BOs are special wrt to mmap-ing. It seems to be some
> kind of optimization. Do you have an idea?
>
> Best regards
> Thomas
>
> [1]
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/udl/udl_drv.h?h=v5.3#n78
> [2]
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/udl/udl_gem.c?h=v5.3#n57

udl doesn't set prefer_shadow = 1, which means compositors will render
directly into the buffer. And for that you want caching. For imported
dma-buf otoh you want wc to make sure it's coherent. Otherwise I guess
udl would need to get the dma_buf_begin/end_cpu_access calls added
(and that would probably be faster than going with wc for everything).
So we do want cachable, it's going to suck otherwise.

But that should be fairly easy to do by overwriting the obj->mmap
callback and wrapping it around the shmem one.

What surprises me more is that udl supports mmap on imported dma-buf,
that's some real quirky stuff. Not sure there's really other drivers
doing that. Might be easier to rip that out :-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 31+ messages in thread
* Re: [PATCH 3/5] drm/udl: Add GEM object functions for free(), vmap(), and vunmap()
@ 2019-10-25 10:12           ` Thomas Zimmermann
  0 siblings, 0 replies; 31+ messages in thread
From: Thomas Zimmermann @ 2019-10-25 10:12 UTC (permalink / raw)
  To: Daniel Vetter; +Cc: Sam Ravnborg, Dave Airlie, Sean Paul, dri-devel


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

Hi

Am 25.10.19 um 11:28 schrieb Daniel Vetter:
> On Fri, Oct 25, 2019 at 9:59 AM Thomas Zimmermann <tzimmermann@suse.de> wrote
>>
>> Hi
>>
>> Am 25.10.19 um 09:40 schrieb Daniel Vetter:
>>> On Thu, Oct 24, 2019 at 04:42:35PM +0200, Thomas Zimmermann wrote:
>>>> Implementing vmap() and vunmap() of struct drm_gem_object_funcs is
>>>> required by generic fbdev emulation. Supporting free() is easy as
>>>> well. More udl-specific interfaces can probably be replaced by GEM
>>>> object functions in later patches.
>>>>
>>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>>>> ---
>>>>  drivers/gpu/drm/udl/udl_gem.c | 34 ++++++++++++++++++++++++++++++++++
>>>>  1 file changed, 34 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/udl/udl_gem.c b/drivers/gpu/drm/udl/udl_gem.c
>>>> index 3ea0cd9ae2d6..6ca097c270d6 100644
>>>> --- a/drivers/gpu/drm/udl/udl_gem.c
>>>> +++ b/drivers/gpu/drm/udl/udl_gem.c
>>>> @@ -11,6 +11,8 @@
>>>>
>>>>  #include "udl_drv.h"
>>>>
>>>> +static const struct drm_gem_object_funcs udl_gem_object_funcs;
>>>> +
>>>>  struct udl_gem_object *udl_gem_alloc_object(struct drm_device *dev,
>>>>                                          size_t size)
>>>>  {
>>>> @@ -25,6 +27,7 @@ struct udl_gem_object *udl_gem_alloc_object(struct drm_device *dev,
>>>>              return NULL;
>>>>      }
>>>>
>>>> +    obj->base.funcs = &udl_gem_object_funcs;
>>>>      obj->flags = UDL_BO_CACHEABLE;
>>>>      return obj;
>>>>  }
>>>> @@ -230,3 +233,34 @@ int udl_gem_mmap(struct drm_file *file, struct drm_device *dev,
>>>>      mutex_unlock(&udl->gem_lock);
>>>>      return ret;
>>>>  }
>>>> +
>>>> +/*
>>>> + * Helpers for struct drm_gem_object_funcs
>>>> + */
>>>> +
>>>> +static void udl_gem_object_free(struct drm_gem_object *obj)
>>>> +{
>>>> +    udl_gem_free_object(obj);
>>>> +}
>>>> +
>>>> +static void *udl_gem_object_vmap(struct drm_gem_object *obj)
>>>> +{
>>>> +    struct udl_gem_object *uobj = to_udl_bo(obj);
>>>> +    int ret;
>>>> +
>>>> +    ret = udl_gem_vmap(uobj);
>>>> +    if (ret)
>>>> +            return ERR_PTR(ret);
>>>> +    return uobj->vmapping;
>>>> +}
>>>> +
>>>> +static void udl_gem_object_vunmap(struct drm_gem_object *obj, void *vaddr)
>>>> +{
>>>> +    return udl_gem_vunmap(to_udl_bo(obj));
>>>> +}
>>>> +
>>>> +static const struct drm_gem_object_funcs udl_gem_object_funcs = {
>>>> +    .free   = udl_gem_object_free,
>>>> +    .vmap   = udl_gem_object_vmap,
>>>> +    .vunmap = udl_gem_object_vunmap,
>>>
>>> Yeah this doesn't work, you need to refcount the vmap here I think. I'd
>>
>> I see. Should have thought of that...
>>
>>> say simpler to first cut over to shmem helpers.
>>
>> Right. I was already attempting the conversion and the udl gem is more
>> or less the same as shmem, except for the flags field. [1] The flag
>> signals page attributes for mmap-ing [2]. For prime-imported BOs its is
>> set to writecombining, and for local BOs it is set to cached. Shmem
>> always maps with writecombining.
>>
>> I don't see why udl BOs are special wrt to mmap-ing. It seems to be some
>> kind of optimization. Do you have an idea?
>>
>> Best regards
>> Thomas
>>
>> [1]
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/udl/udl_drv.h?h=v5.3#n78
>> [2]
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/udl/udl_gem.c?h=v5.3#n57
> 
> udl doesn't set prefer_shadow = 1, which means compositors will render
> directly into the buffer. And for that you want caching. For imported
> dma-buf otoh you want wc to make sure it's coherent. Otherwise I guess
> udl would need to get the dma_buf_begin/end_cpu_access calls added
> (and that would probably be faster than going with wc for everything).
> So we do want cachable, it's going to suck otherwise.

Thanks for clarifying. Actually, it does have these calls in its dirty
handler. [1]

> 
> But that should be fairly easy to do by overwriting the obj->mmap
> callback and wrapping it around the shmem one.

Is there a reason why shmem doesn't implement the wc-vs.cached logic?
Could this be added?

(I'm just trying to understand the available options here before
attempting to do a conversion.)

Best regards
Thomas

> 
> What surprises me more is that udl supports mmap on imported dma-buf,
> that's some real quirky stuff. Not sure there's really other drivers
> doing that. Might be easier to rip that out :-)
> -Daniel
> 

[1]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/udl/udl_fb.c?h=v5.3#n293


-- 
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: 159 bytes --]

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

^ permalink raw reply	[flat|nested] 31+ messages in thread
* Re: [PATCH 3/5] drm/udl: Add GEM object functions for free(), vmap(),  and vunmap()
@ 2019-10-25 10:12           ` Thomas Zimmermann
  0 siblings, 0 replies; 31+ messages in thread
From: Thomas Zimmermann @ 2019-10-25 10:12 UTC (permalink / raw)
  To: Daniel Vetter; +Cc: Sam Ravnborg, Dave Airlie, Sean Paul, dri-devel


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

Hi

Am 25.10.19 um 11:28 schrieb Daniel Vetter:
> On Fri, Oct 25, 2019 at 9:59 AM Thomas Zimmermann <tzimmermann@suse.de> wrote
>>
>> Hi
>>
>> Am 25.10.19 um 09:40 schrieb Daniel Vetter:
>>> On Thu, Oct 24, 2019 at 04:42:35PM +0200, Thomas Zimmermann wrote:
>>>> Implementing vmap() and vunmap() of struct drm_gem_object_funcs is
>>>> required by generic fbdev emulation. Supporting free() is easy as
>>>> well. More udl-specific interfaces can probably be replaced by GEM
>>>> object functions in later patches.
>>>>
>>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>>>> ---
>>>>  drivers/gpu/drm/udl/udl_gem.c | 34 ++++++++++++++++++++++++++++++++++
>>>>  1 file changed, 34 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/udl/udl_gem.c b/drivers/gpu/drm/udl/udl_gem.c
>>>> index 3ea0cd9ae2d6..6ca097c270d6 100644
>>>> --- a/drivers/gpu/drm/udl/udl_gem.c
>>>> +++ b/drivers/gpu/drm/udl/udl_gem.c
>>>> @@ -11,6 +11,8 @@
>>>>
>>>>  #include "udl_drv.h"
>>>>
>>>> +static const struct drm_gem_object_funcs udl_gem_object_funcs;
>>>> +
>>>>  struct udl_gem_object *udl_gem_alloc_object(struct drm_device *dev,
>>>>                                          size_t size)
>>>>  {
>>>> @@ -25,6 +27,7 @@ struct udl_gem_object *udl_gem_alloc_object(struct drm_device *dev,
>>>>              return NULL;
>>>>      }
>>>>
>>>> +    obj->base.funcs = &udl_gem_object_funcs;
>>>>      obj->flags = UDL_BO_CACHEABLE;
>>>>      return obj;
>>>>  }
>>>> @@ -230,3 +233,34 @@ int udl_gem_mmap(struct drm_file *file, struct drm_device *dev,
>>>>      mutex_unlock(&udl->gem_lock);
>>>>      return ret;
>>>>  }
>>>> +
>>>> +/*
>>>> + * Helpers for struct drm_gem_object_funcs
>>>> + */
>>>> +
>>>> +static void udl_gem_object_free(struct drm_gem_object *obj)
>>>> +{
>>>> +    udl_gem_free_object(obj);
>>>> +}
>>>> +
>>>> +static void *udl_gem_object_vmap(struct drm_gem_object *obj)
>>>> +{
>>>> +    struct udl_gem_object *uobj = to_udl_bo(obj);
>>>> +    int ret;
>>>> +
>>>> +    ret = udl_gem_vmap(uobj);
>>>> +    if (ret)
>>>> +            return ERR_PTR(ret);
>>>> +    return uobj->vmapping;
>>>> +}
>>>> +
>>>> +static void udl_gem_object_vunmap(struct drm_gem_object *obj, void *vaddr)
>>>> +{
>>>> +    return udl_gem_vunmap(to_udl_bo(obj));
>>>> +}
>>>> +
>>>> +static const struct drm_gem_object_funcs udl_gem_object_funcs = {
>>>> +    .free   = udl_gem_object_free,
>>>> +    .vmap   = udl_gem_object_vmap,
>>>> +    .vunmap = udl_gem_object_vunmap,
>>>
>>> Yeah this doesn't work, you need to refcount the vmap here I think. I'd
>>
>> I see. Should have thought of that...
>>
>>> say simpler to first cut over to shmem helpers.
>>
>> Right. I was already attempting the conversion and the udl gem is more
>> or less the same as shmem, except for the flags field. [1] The flag
>> signals page attributes for mmap-ing [2]. For prime-imported BOs its is
>> set to writecombining, and for local BOs it is set to cached. Shmem
>> always maps with writecombining.
>>
>> I don't see why udl BOs are special wrt to mmap-ing. It seems to be some
>> kind of optimization. Do you have an idea?
>>
>> Best regards
>> Thomas
>>
>> [1]
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/udl/udl_drv.h?h=v5.3#n78
>> [2]
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/udl/udl_gem.c?h=v5.3#n57
> 
> udl doesn't set prefer_shadow = 1, which means compositors will render
> directly into the buffer. And for that you want caching. For imported
> dma-buf otoh you want wc to make sure it's coherent. Otherwise I guess
> udl would need to get the dma_buf_begin/end_cpu_access calls added
> (and that would probably be faster than going with wc for everything).
> So we do want cachable, it's going to suck otherwise.

Thanks for clarifying. Actually, it does have these calls in its dirty
handler. [1]

> 
> But that should be fairly easy to do by overwriting the obj->mmap
> callback and wrapping it around the shmem one.

Is there a reason why shmem doesn't implement the wc-vs.cached logic?
Could this be added?

(I'm just trying to understand the available options here before
attempting to do a conversion.)

Best regards
Thomas

> 
> What surprises me more is that udl supports mmap on imported dma-buf,
> that's some real quirky stuff. Not sure there's really other drivers
> doing that. Might be easier to rip that out :-)
> -Daniel
> 

[1]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/udl/udl_fb.c?h=v5.3#n293


-- 
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: 159 bytes --]

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

^ permalink raw reply	[flat|nested] 31+ messages in thread
* Re: [PATCH 5/5] drm/udl: Replace fbdev code with generic emulation
  2019-10-25  8:00     ` Daniel Vetter
@ 2019-10-25 11:22       ` Noralf Trønnes
  2019-10-25 11:44         ` Thomas Zimmermann
  0 siblings, 1 reply; 31+ messages in thread
From: Noralf Trønnes @ 2019-10-25 11:22 UTC (permalink / raw)
  To: Daniel Vetter, Thomas Zimmermann; +Cc: airlied, sean, dri-devel, sam



Den 25.10.2019 10.00, skrev Daniel Vetter:
> On Fri, Oct 25, 2019 at 09:47:46AM +0200, Daniel Vetter wrote:
>> On Thu, Oct 24, 2019 at 04:42:37PM +0200, Thomas Zimmermann wrote:
>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>>> ---
>>>  drivers/gpu/drm/udl/udl_drv.c     |   3 +
>>>  drivers/gpu/drm/udl/udl_drv.h     |   4 -
>>>  drivers/gpu/drm/udl/udl_fb.c      | 263 +-----------------------------
>>>  drivers/gpu/drm/udl/udl_main.c    |   2 -
>>>  drivers/gpu/drm/udl/udl_modeset.c |   3 +-
>>>  5 files changed, 8 insertions(+), 267 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/udl/udl_drv.c b/drivers/gpu/drm/udl/udl_drv.c
>>> index 15ad7a338f9d..6beaa1109c2c 100644
>>> --- a/drivers/gpu/drm/udl/udl_drv.c
>>> +++ b/drivers/gpu/drm/udl/udl_drv.c
>>> @@ -7,6 +7,7 @@
>>>  
>>>  #include <drm/drm_crtc_helper.h>
>>>  #include <drm/drm_drv.h>
>>> +#include <drm/drm_fb_helper.h>
>>>  #include <drm/drm_file.h>
>>>  #include <drm/drm_ioctl.h>
>>>  #include <drm/drm_probe_helper.h>
>>> @@ -62,6 +63,8 @@ static struct drm_driver driver = {
>>>  	.driver_features = DRIVER_MODESET | DRIVER_GEM,
>>>  	.release = udl_driver_release,
>>>  
>>> +	.lastclose = drm_fb_helper_lastclose,
>>> +
>>>  	/* gem hooks */
>>>  	.gem_free_object_unlocked = udl_gem_free_object,
>>>  	.gem_vm_ops = &udl_gem_vm_ops,
>>> diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h
>>> index 12a970fd9a87..5f8a7ac084f6 100644
>>> --- a/drivers/gpu/drm/udl/udl_drv.h
>>> +++ b/drivers/gpu/drm/udl/udl_drv.h
>>> @@ -50,8 +50,6 @@ struct urb_list {
>>>  	size_t size;
>>>  };
>>>  
>>> -struct udl_fbdev;
>>> -
>>>  struct udl_device {
>>>  	struct drm_device drm;
>>>  	struct device *dev;
>>> @@ -65,7 +63,6 @@ struct udl_device {
>>>  	struct urb_list urbs;
>>>  	atomic_t lost_pixels; /* 1 = a render op failed. Need screen refresh */
>>>  
>>> -	struct udl_fbdev *fbdev;
>>>  	char mode_buf[1024];
>>>  	uint32_t mode_buf_len;
>>>  	atomic_t bytes_rendered; /* raw pixel-bytes driver asked to render */
>>> @@ -111,7 +108,6 @@ int udl_init(struct udl_device *udl);
>>>  void udl_fini(struct drm_device *dev);
>>>  
>>>  int udl_fbdev_init(struct drm_device *dev);
>>> -void udl_fbdev_cleanup(struct drm_device *dev);
>>>  void udl_fbdev_unplug(struct drm_device *dev);
>>>  struct drm_framebuffer *
>>>  udl_fb_user_fb_create(struct drm_device *dev,
>>> diff --git a/drivers/gpu/drm/udl/udl_fb.c b/drivers/gpu/drm/udl/udl_fb.c
>>> index ef3504d06343..43a1da3a56c3 100644
>>> --- a/drivers/gpu/drm/udl/udl_fb.c
>>> +++ b/drivers/gpu/drm/udl/udl_fb.c
>>> @@ -19,19 +19,9 @@
>>>  
>>>  #include "udl_drv.h"
>>>  
>>> -#define DL_DEFIO_WRITE_DELAY    (HZ/20) /* fb_deferred_io.delay in jiffies */
>>> -
>>> -static int fb_defio = 0;  /* Optionally enable experimental fb_defio mmap support */
> 
> Correction on my enthusiasm, this here is a problem:
> 
> The udl defio support as-is is broken, fbdev defio and shmem are fight
> over the page flags. Not a problem with the old code, since disabled by
> default. But will be a problem with the new code. I guess you didn't test
> fbdev mmap? We unfortunately also lack an easy igt testcase for this ...

This is where the shadow buffer comes to the rescue. fbdev gets a
vmalloc buffer and this is blitted on the shmem buffer in the defio
callback before calling the framebuffer .dirty callback. So the defio
internals never sees the shmem buffer. At least this worked when I was
writing the shmem helper.

Noralf.

> 
> The problem is fairly tricky to solve, here's an untested idea that might
> work:
> 
> https://dri.freedesktop.org/docs/drm/gpu/todo.html#generic-fbdev-defio-support
> 
> Cheers, Daniel
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 31+ messages in thread
* Re: [PATCH 5/5] drm/udl: Replace fbdev code with generic emulation
  2019-10-25 11:22       ` Noralf Trønnes
@ 2019-10-25 11:44         ` Thomas Zimmermann
  0 siblings, 0 replies; 31+ messages in thread
From: Thomas Zimmermann @ 2019-10-25 11:44 UTC (permalink / raw)
  To: Noralf Trønnes, Daniel Vetter; +Cc: airlied, sean, dri-devel, sam


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

Hi Noralf

Am 25.10.19 um 13:22 schrieb Noralf Trønnes:
> 
> 
> Den 25.10.2019 10.00, skrev Daniel Vetter:
>> On Fri, Oct 25, 2019 at 09:47:46AM +0200, Daniel Vetter wrote:
>>> On Thu, Oct 24, 2019 at 04:42:37PM +0200, Thomas Zimmermann wrote:
>>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>>>> ---
>>>>  drivers/gpu/drm/udl/udl_drv.c     |   3 +
>>>>  drivers/gpu/drm/udl/udl_drv.h     |   4 -
>>>>  drivers/gpu/drm/udl/udl_fb.c      | 263 +-----------------------------
>>>>  drivers/gpu/drm/udl/udl_main.c    |   2 -
>>>>  drivers/gpu/drm/udl/udl_modeset.c |   3 +-
>>>>  5 files changed, 8 insertions(+), 267 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/udl/udl_drv.c b/drivers/gpu/drm/udl/udl_drv.c
>>>> index 15ad7a338f9d..6beaa1109c2c 100644
>>>> --- a/drivers/gpu/drm/udl/udl_drv.c
>>>> +++ b/drivers/gpu/drm/udl/udl_drv.c
>>>> @@ -7,6 +7,7 @@
>>>>  
>>>>  #include <drm/drm_crtc_helper.h>
>>>>  #include <drm/drm_drv.h>
>>>> +#include <drm/drm_fb_helper.h>
>>>>  #include <drm/drm_file.h>
>>>>  #include <drm/drm_ioctl.h>
>>>>  #include <drm/drm_probe_helper.h>
>>>> @@ -62,6 +63,8 @@ static struct drm_driver driver = {
>>>>  	.driver_features = DRIVER_MODESET | DRIVER_GEM,
>>>>  	.release = udl_driver_release,
>>>>  
>>>> +	.lastclose = drm_fb_helper_lastclose,
>>>> +
>>>>  	/* gem hooks */
>>>>  	.gem_free_object_unlocked = udl_gem_free_object,
>>>>  	.gem_vm_ops = &udl_gem_vm_ops,
>>>> diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h
>>>> index 12a970fd9a87..5f8a7ac084f6 100644
>>>> --- a/drivers/gpu/drm/udl/udl_drv.h
>>>> +++ b/drivers/gpu/drm/udl/udl_drv.h
>>>> @@ -50,8 +50,6 @@ struct urb_list {
>>>>  	size_t size;
>>>>  };
>>>>  
>>>> -struct udl_fbdev;
>>>> -
>>>>  struct udl_device {
>>>>  	struct drm_device drm;
>>>>  	struct device *dev;
>>>> @@ -65,7 +63,6 @@ struct udl_device {
>>>>  	struct urb_list urbs;
>>>>  	atomic_t lost_pixels; /* 1 = a render op failed. Need screen refresh */
>>>>  
>>>> -	struct udl_fbdev *fbdev;
>>>>  	char mode_buf[1024];
>>>>  	uint32_t mode_buf_len;
>>>>  	atomic_t bytes_rendered; /* raw pixel-bytes driver asked to render */
>>>> @@ -111,7 +108,6 @@ int udl_init(struct udl_device *udl);
>>>>  void udl_fini(struct drm_device *dev);
>>>>  
>>>>  int udl_fbdev_init(struct drm_device *dev);
>>>> -void udl_fbdev_cleanup(struct drm_device *dev);
>>>>  void udl_fbdev_unplug(struct drm_device *dev);
>>>>  struct drm_framebuffer *
>>>>  udl_fb_user_fb_create(struct drm_device *dev,
>>>> diff --git a/drivers/gpu/drm/udl/udl_fb.c b/drivers/gpu/drm/udl/udl_fb.c
>>>> index ef3504d06343..43a1da3a56c3 100644
>>>> --- a/drivers/gpu/drm/udl/udl_fb.c
>>>> +++ b/drivers/gpu/drm/udl/udl_fb.c
>>>> @@ -19,19 +19,9 @@
>>>>  
>>>>  #include "udl_drv.h"
>>>>  
>>>> -#define DL_DEFIO_WRITE_DELAY    (HZ/20) /* fb_deferred_io.delay in jiffies */
>>>> -
>>>> -static int fb_defio = 0;  /* Optionally enable experimental fb_defio mmap support */
>>
>> Correction on my enthusiasm, this here is a problem:
>>
>> The udl defio support as-is is broken, fbdev defio and shmem are fight
>> over the page flags. Not a problem with the old code, since disabled by
>> default. But will be a problem with the new code. I guess you didn't test
>> fbdev mmap? We unfortunately also lack an easy igt testcase for this ...
> 
> This is where the shadow buffer comes to the rescue. fbdev gets a
> vmalloc buffer and this is blitted on the shmem buffer in the defio
> callback before calling the framebuffer .dirty callback. So the defio
> internals never sees the shmem buffer. At least this worked when I was
> writing the shmem helper.

There's already a patchset on the ML, cleaning up the docs around fb
defio. Please take a look.

Best regards
Thomas

> 
> Noralf.
> 
>>
>> The problem is fairly tricky to solve, here's an untested idea that might
>> work:
>>
>> https://dri.freedesktop.org/docs/drm/gpu/todo.html#generic-fbdev-defio-support
>>
>> 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: 159 bytes --]

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

^ permalink raw reply	[flat|nested] 31+ messages in thread
* Re: [PATCH 3/5] drm/udl: Add GEM object functions for free(), vmap(), and vunmap()
@ 2019-10-25 11:44             ` Noralf Trønnes
  0 siblings, 0 replies; 31+ messages in thread
From: Noralf Trønnes @ 2019-10-25 11:44 UTC (permalink / raw)
  To: Thomas Zimmermann, Daniel Vetter
  Cc: Sean Paul, Dave Airlie, Sam Ravnborg, dri-devel



Den 25.10.2019 12.12, skrev Thomas Zimmermann:
> Hi
> 
> Am 25.10.19 um 11:28 schrieb Daniel Vetter:
>> On Fri, Oct 25, 2019 at 9:59 AM Thomas Zimmermann <tzimmermann@suse.de> wrote
>>>
>>> Hi
>>>
>>> Am 25.10.19 um 09:40 schrieb Daniel Vetter:
>>>> On Thu, Oct 24, 2019 at 04:42:35PM +0200, Thomas Zimmermann wrote:
>>>>> Implementing vmap() and vunmap() of struct drm_gem_object_funcs is
>>>>> required by generic fbdev emulation. Supporting free() is easy as
>>>>> well. More udl-specific interfaces can probably be replaced by GEM
>>>>> object functions in later patches.
>>>>>
>>>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>>>>> ---
>>>>>  drivers/gpu/drm/udl/udl_gem.c | 34 ++++++++++++++++++++++++++++++++++
>>>>>  1 file changed, 34 insertions(+)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/udl/udl_gem.c b/drivers/gpu/drm/udl/udl_gem.c
>>>>> index 3ea0cd9ae2d6..6ca097c270d6 100644
>>>>> --- a/drivers/gpu/drm/udl/udl_gem.c
>>>>> +++ b/drivers/gpu/drm/udl/udl_gem.c
>>>>> @@ -11,6 +11,8 @@
>>>>>
>>>>>  #include "udl_drv.h"
>>>>>
>>>>> +static const struct drm_gem_object_funcs udl_gem_object_funcs;
>>>>> +
>>>>>  struct udl_gem_object *udl_gem_alloc_object(struct drm_device *dev,
>>>>>                                          size_t size)
>>>>>  {
>>>>> @@ -25,6 +27,7 @@ struct udl_gem_object *udl_gem_alloc_object(struct drm_device *dev,
>>>>>              return NULL;
>>>>>      }
>>>>>
>>>>> +    obj->base.funcs = &udl_gem_object_funcs;
>>>>>      obj->flags = UDL_BO_CACHEABLE;
>>>>>      return obj;
>>>>>  }
>>>>> @@ -230,3 +233,34 @@ int udl_gem_mmap(struct drm_file *file, struct drm_device *dev,
>>>>>      mutex_unlock(&udl->gem_lock);
>>>>>      return ret;
>>>>>  }
>>>>> +
>>>>> +/*
>>>>> + * Helpers for struct drm_gem_object_funcs
>>>>> + */
>>>>> +
>>>>> +static void udl_gem_object_free(struct drm_gem_object *obj)
>>>>> +{
>>>>> +    udl_gem_free_object(obj);
>>>>> +}
>>>>> +
>>>>> +static void *udl_gem_object_vmap(struct drm_gem_object *obj)
>>>>> +{
>>>>> +    struct udl_gem_object *uobj = to_udl_bo(obj);
>>>>> +    int ret;
>>>>> +
>>>>> +    ret = udl_gem_vmap(uobj);
>>>>> +    if (ret)
>>>>> +            return ERR_PTR(ret);
>>>>> +    return uobj->vmapping;
>>>>> +}
>>>>> +
>>>>> +static void udl_gem_object_vunmap(struct drm_gem_object *obj, void *vaddr)
>>>>> +{
>>>>> +    return udl_gem_vunmap(to_udl_bo(obj));
>>>>> +}
>>>>> +
>>>>> +static const struct drm_gem_object_funcs udl_gem_object_funcs = {
>>>>> +    .free   = udl_gem_object_free,
>>>>> +    .vmap   = udl_gem_object_vmap,
>>>>> +    .vunmap = udl_gem_object_vunmap,
>>>>
>>>> Yeah this doesn't work, you need to refcount the vmap here I think. I'd
>>>
>>> I see. Should have thought of that...
>>>
>>>> say simpler to first cut over to shmem helpers.
>>>
>>> Right. I was already attempting the conversion and the udl gem is more
>>> or less the same as shmem, except for the flags field. [1] The flag
>>> signals page attributes for mmap-ing [2]. For prime-imported BOs its is
>>> set to writecombining, and for local BOs it is set to cached. Shmem
>>> always maps with writecombining.
>>>
>>> I don't see why udl BOs are special wrt to mmap-ing. It seems to be some
>>> kind of optimization. Do you have an idea?
>>>
>>> Best regards
>>> Thomas
>>>
>>> [1]
>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/udl/udl_drv.h?h=v5.3#n78
>>> [2]
>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/udl/udl_gem.c?h=v5.3#n57
>>
>> udl doesn't set prefer_shadow = 1, which means compositors will render
>> directly into the buffer. And for that you want caching. For imported
>> dma-buf otoh you want wc to make sure it's coherent. Otherwise I guess
>> udl would need to get the dma_buf_begin/end_cpu_access calls added
>> (and that would probably be faster than going with wc for everything).
>> So we do want cachable, it's going to suck otherwise.
> 
> Thanks for clarifying. Actually, it does have these calls in its dirty
> handler. [1]
> 
>>
>> But that should be fairly easy to do by overwriting the obj->mmap
>> callback and wrapping it around the shmem one.
> 
> Is there a reason why shmem doesn't implement the wc-vs.cached logic?
> Could this be added?
> 

I had a flag to set this in the initial version of the shmem helper
modeled after udl, but Thomas Hellstrom brought up a question and it was
dropped. The issue was beyond my understanding:

[PATCH v3 0/2] drm: Add shmem GEM library
https://lists.freedesktop.org/archives/dri-devel/2018-September/190001.html

In tinydrm I have sped up uncached writes on arm by copying one pixel
line at a time into a temporary buffer before accessing the individual
bytes. See drm_fb_swab16().

Noralf.

> (I'm just trying to understand the available options here before
> attempting to do a conversion.)
> 
> Best regards
> Thomas
> 
>>
>> What surprises me more is that udl supports mmap on imported dma-buf,
>> that's some real quirky stuff. Not sure there's really other drivers
>> doing that. Might be easier to rip that out :-)
>> -Daniel
>>
> 
> [1]
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/udl/udl_fb.c?h=v5.3#n293
> 
> 
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 31+ messages in thread
* Re: [PATCH 3/5] drm/udl: Add GEM object functions for free(), vmap(),  and vunmap()
@ 2019-10-25 11:44             ` Noralf Trønnes
  0 siblings, 0 replies; 31+ messages in thread
From: Noralf Trønnes @ 2019-10-25 11:44 UTC (permalink / raw)
  To: Thomas Zimmermann, Daniel Vetter
  Cc: Sean Paul, Dave Airlie, Sam Ravnborg, dri-devel



Den 25.10.2019 12.12, skrev Thomas Zimmermann:
> Hi
> 
> Am 25.10.19 um 11:28 schrieb Daniel Vetter:
>> On Fri, Oct 25, 2019 at 9:59 AM Thomas Zimmermann <tzimmermann@suse.de> wrote
>>>
>>> Hi
>>>
>>> Am 25.10.19 um 09:40 schrieb Daniel Vetter:
>>>> On Thu, Oct 24, 2019 at 04:42:35PM +0200, Thomas Zimmermann wrote:
>>>>> Implementing vmap() and vunmap() of struct drm_gem_object_funcs is
>>>>> required by generic fbdev emulation. Supporting free() is easy as
>>>>> well. More udl-specific interfaces can probably be replaced by GEM
>>>>> object functions in later patches.
>>>>>
>>>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>>>>> ---
>>>>>  drivers/gpu/drm/udl/udl_gem.c | 34 ++++++++++++++++++++++++++++++++++
>>>>>  1 file changed, 34 insertions(+)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/udl/udl_gem.c b/drivers/gpu/drm/udl/udl_gem.c
>>>>> index 3ea0cd9ae2d6..6ca097c270d6 100644
>>>>> --- a/drivers/gpu/drm/udl/udl_gem.c
>>>>> +++ b/drivers/gpu/drm/udl/udl_gem.c
>>>>> @@ -11,6 +11,8 @@
>>>>>
>>>>>  #include "udl_drv.h"
>>>>>
>>>>> +static const struct drm_gem_object_funcs udl_gem_object_funcs;
>>>>> +
>>>>>  struct udl_gem_object *udl_gem_alloc_object(struct drm_device *dev,
>>>>>                                          size_t size)
>>>>>  {
>>>>> @@ -25,6 +27,7 @@ struct udl_gem_object *udl_gem_alloc_object(struct drm_device *dev,
>>>>>              return NULL;
>>>>>      }
>>>>>
>>>>> +    obj->base.funcs = &udl_gem_object_funcs;
>>>>>      obj->flags = UDL_BO_CACHEABLE;
>>>>>      return obj;
>>>>>  }
>>>>> @@ -230,3 +233,34 @@ int udl_gem_mmap(struct drm_file *file, struct drm_device *dev,
>>>>>      mutex_unlock(&udl->gem_lock);
>>>>>      return ret;
>>>>>  }
>>>>> +
>>>>> +/*
>>>>> + * Helpers for struct drm_gem_object_funcs
>>>>> + */
>>>>> +
>>>>> +static void udl_gem_object_free(struct drm_gem_object *obj)
>>>>> +{
>>>>> +    udl_gem_free_object(obj);
>>>>> +}
>>>>> +
>>>>> +static void *udl_gem_object_vmap(struct drm_gem_object *obj)
>>>>> +{
>>>>> +    struct udl_gem_object *uobj = to_udl_bo(obj);
>>>>> +    int ret;
>>>>> +
>>>>> +    ret = udl_gem_vmap(uobj);
>>>>> +    if (ret)
>>>>> +            return ERR_PTR(ret);
>>>>> +    return uobj->vmapping;
>>>>> +}
>>>>> +
>>>>> +static void udl_gem_object_vunmap(struct drm_gem_object *obj, void *vaddr)
>>>>> +{
>>>>> +    return udl_gem_vunmap(to_udl_bo(obj));
>>>>> +}
>>>>> +
>>>>> +static const struct drm_gem_object_funcs udl_gem_object_funcs = {
>>>>> +    .free   = udl_gem_object_free,
>>>>> +    .vmap   = udl_gem_object_vmap,
>>>>> +    .vunmap = udl_gem_object_vunmap,
>>>>
>>>> Yeah this doesn't work, you need to refcount the vmap here I think. I'd
>>>
>>> I see. Should have thought of that...
>>>
>>>> say simpler to first cut over to shmem helpers.
>>>
>>> Right. I was already attempting the conversion and the udl gem is more
>>> or less the same as shmem, except for the flags field. [1] The flag
>>> signals page attributes for mmap-ing [2]. For prime-imported BOs its is
>>> set to writecombining, and for local BOs it is set to cached. Shmem
>>> always maps with writecombining.
>>>
>>> I don't see why udl BOs are special wrt to mmap-ing. It seems to be some
>>> kind of optimization. Do you have an idea?
>>>
>>> Best regards
>>> Thomas
>>>
>>> [1]
>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/udl/udl_drv.h?h=v5.3#n78
>>> [2]
>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/udl/udl_gem.c?h=v5.3#n57
>>
>> udl doesn't set prefer_shadow = 1, which means compositors will render
>> directly into the buffer. And for that you want caching. For imported
>> dma-buf otoh you want wc to make sure it's coherent. Otherwise I guess
>> udl would need to get the dma_buf_begin/end_cpu_access calls added
>> (and that would probably be faster than going with wc for everything).
>> So we do want cachable, it's going to suck otherwise.
> 
> Thanks for clarifying. Actually, it does have these calls in its dirty
> handler. [1]
> 
>>
>> But that should be fairly easy to do by overwriting the obj->mmap
>> callback and wrapping it around the shmem one.
> 
> Is there a reason why shmem doesn't implement the wc-vs.cached logic?
> Could this be added?
> 

I had a flag to set this in the initial version of the shmem helper
modeled after udl, but Thomas Hellstrom brought up a question and it was
dropped. The issue was beyond my understanding:

[PATCH v3 0/2] drm: Add shmem GEM library
https://lists.freedesktop.org/archives/dri-devel/2018-September/190001.html

In tinydrm I have sped up uncached writes on arm by copying one pixel
line at a time into a temporary buffer before accessing the individual
bytes. See drm_fb_swab16().

Noralf.

> (I'm just trying to understand the available options here before
> attempting to do a conversion.)
> 
> Best regards
> Thomas
> 
>>
>> What surprises me more is that udl supports mmap on imported dma-buf,
>> that's some real quirky stuff. Not sure there's really other drivers
>> doing that. Might be easier to rip that out :-)
>> -Daniel
>>
> 
> [1]
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/udl/udl_fb.c?h=v5.3#n293
> 
> 
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 31+ messages in thread
* Re: [PATCH 3/5] drm/udl: Add GEM object functions for free(), vmap(), and vunmap()
@ 2019-10-25 11:47               ` Noralf Trønnes
  0 siblings, 0 replies; 31+ messages in thread
From: Noralf Trønnes @ 2019-10-25 11:47 UTC (permalink / raw)
  To: Thomas Zimmermann, Daniel Vetter
  Cc: Dave Airlie, Sean Paul, dri-devel, Sam Ravnborg



Den 25.10.2019 13.44, skrev Noralf Trønnes:
> 
> 
> Den 25.10.2019 12.12, skrev Thomas Zimmermann:
>> Hi
>>
>> Am 25.10.19 um 11:28 schrieb Daniel Vetter:
>>> On Fri, Oct 25, 2019 at 9:59 AM Thomas Zimmermann <tzimmermann@suse.de> wrote
>>>>
>>>> Hi
>>>>
>>>> Am 25.10.19 um 09:40 schrieb Daniel Vetter:
>>>>> On Thu, Oct 24, 2019 at 04:42:35PM +0200, Thomas Zimmermann wrote:
>>>>>> Implementing vmap() and vunmap() of struct drm_gem_object_funcs is
>>>>>> required by generic fbdev emulation. Supporting free() is easy as
>>>>>> well. More udl-specific interfaces can probably be replaced by GEM
>>>>>> object functions in later patches.
>>>>>>
>>>>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>>>>>> ---
>>>>>>  drivers/gpu/drm/udl/udl_gem.c | 34 ++++++++++++++++++++++++++++++++++
>>>>>>  1 file changed, 34 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/udl/udl_gem.c b/drivers/gpu/drm/udl/udl_gem.c
>>>>>> index 3ea0cd9ae2d6..6ca097c270d6 100644
>>>>>> --- a/drivers/gpu/drm/udl/udl_gem.c
>>>>>> +++ b/drivers/gpu/drm/udl/udl_gem.c
>>>>>> @@ -11,6 +11,8 @@
>>>>>>
>>>>>>  #include "udl_drv.h"
>>>>>>
>>>>>> +static const struct drm_gem_object_funcs udl_gem_object_funcs;
>>>>>> +
>>>>>>  struct udl_gem_object *udl_gem_alloc_object(struct drm_device *dev,
>>>>>>                                          size_t size)
>>>>>>  {
>>>>>> @@ -25,6 +27,7 @@ struct udl_gem_object *udl_gem_alloc_object(struct drm_device *dev,
>>>>>>              return NULL;
>>>>>>      }
>>>>>>
>>>>>> +    obj->base.funcs = &udl_gem_object_funcs;
>>>>>>      obj->flags = UDL_BO_CACHEABLE;
>>>>>>      return obj;
>>>>>>  }
>>>>>> @@ -230,3 +233,34 @@ int udl_gem_mmap(struct drm_file *file, struct drm_device *dev,
>>>>>>      mutex_unlock(&udl->gem_lock);
>>>>>>      return ret;
>>>>>>  }
>>>>>> +
>>>>>> +/*
>>>>>> + * Helpers for struct drm_gem_object_funcs
>>>>>> + */
>>>>>> +
>>>>>> +static void udl_gem_object_free(struct drm_gem_object *obj)
>>>>>> +{
>>>>>> +    udl_gem_free_object(obj);
>>>>>> +}
>>>>>> +
>>>>>> +static void *udl_gem_object_vmap(struct drm_gem_object *obj)
>>>>>> +{
>>>>>> +    struct udl_gem_object *uobj = to_udl_bo(obj);
>>>>>> +    int ret;
>>>>>> +
>>>>>> +    ret = udl_gem_vmap(uobj);
>>>>>> +    if (ret)
>>>>>> +            return ERR_PTR(ret);
>>>>>> +    return uobj->vmapping;
>>>>>> +}
>>>>>> +
>>>>>> +static void udl_gem_object_vunmap(struct drm_gem_object *obj, void *vaddr)
>>>>>> +{
>>>>>> +    return udl_gem_vunmap(to_udl_bo(obj));
>>>>>> +}
>>>>>> +
>>>>>> +static const struct drm_gem_object_funcs udl_gem_object_funcs = {
>>>>>> +    .free   = udl_gem_object_free,
>>>>>> +    .vmap   = udl_gem_object_vmap,
>>>>>> +    .vunmap = udl_gem_object_vunmap,
>>>>>
>>>>> Yeah this doesn't work, you need to refcount the vmap here I think. I'd
>>>>
>>>> I see. Should have thought of that...
>>>>
>>>>> say simpler to first cut over to shmem helpers.
>>>>
>>>> Right. I was already attempting the conversion and the udl gem is more
>>>> or less the same as shmem, except for the flags field. [1] The flag
>>>> signals page attributes for mmap-ing [2]. For prime-imported BOs its is
>>>> set to writecombining, and for local BOs it is set to cached. Shmem
>>>> always maps with writecombining.
>>>>
>>>> I don't see why udl BOs are special wrt to mmap-ing. It seems to be some
>>>> kind of optimization. Do you have an idea?
>>>>
>>>> Best regards
>>>> Thomas
>>>>
>>>> [1]
>>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/udl/udl_drv.h?h=v5.3#n78
>>>> [2]
>>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/udl/udl_gem.c?h=v5.3#n57
>>>
>>> udl doesn't set prefer_shadow = 1, which means compositors will render
>>> directly into the buffer. And for that you want caching. For imported
>>> dma-buf otoh you want wc to make sure it's coherent. Otherwise I guess
>>> udl would need to get the dma_buf_begin/end_cpu_access calls added
>>> (and that would probably be faster than going with wc for everything).
>>> So we do want cachable, it's going to suck otherwise.
>>
>> Thanks for clarifying. Actually, it does have these calls in its dirty
>> handler. [1]
>>
>>>
>>> But that should be fairly easy to do by overwriting the obj->mmap
>>> callback and wrapping it around the shmem one.
>>
>> Is there a reason why shmem doesn't implement the wc-vs.cached logic?
>> Could this be added?
>>
> 
> I had a flag to set this in the initial version of the shmem helper
> modeled after udl, but Thomas Hellstrom brought up a question and it was
> dropped. The issue was beyond my understanding:
> 
> [PATCH v3 0/2] drm: Add shmem GEM library
> https://lists.freedesktop.org/archives/dri-devel/2018-September/190001.html
> 
> In tinydrm I have sped up uncached writes on arm by copying one pixel

s/writes/reads/

> line at a time into a temporary buffer before accessing the individual
> bytes. See drm_fb_swab16().
> 
> Noralf.
> 
>> (I'm just trying to understand the available options here before
>> attempting to do a conversion.)
>>
>> Best regards
>> Thomas
>>
>>>
>>> What surprises me more is that udl supports mmap on imported dma-buf,
>>> that's some real quirky stuff. Not sure there's really other drivers
>>> doing that. Might be easier to rip that out :-)
>>> -Daniel
>>>
>>
>> [1]
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/udl/udl_fb.c?h=v5.3#n293
>>
>>
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 31+ messages in thread
* Re: [PATCH 3/5] drm/udl: Add GEM object functions for free(), vmap(),  and vunmap()
@ 2019-10-25 11:47               ` Noralf Trønnes
  0 siblings, 0 replies; 31+ messages in thread
From: Noralf Trønnes @ 2019-10-25 11:47 UTC (permalink / raw)
  To: Thomas Zimmermann, Daniel Vetter
  Cc: Dave Airlie, Sean Paul, dri-devel, Sam Ravnborg



Den 25.10.2019 13.44, skrev Noralf Trønnes:
> 
> 
> Den 25.10.2019 12.12, skrev Thomas Zimmermann:
>> Hi
>>
>> Am 25.10.19 um 11:28 schrieb Daniel Vetter:
>>> On Fri, Oct 25, 2019 at 9:59 AM Thomas Zimmermann <tzimmermann@suse.de> wrote
>>>>
>>>> Hi
>>>>
>>>> Am 25.10.19 um 09:40 schrieb Daniel Vetter:
>>>>> On Thu, Oct 24, 2019 at 04:42:35PM +0200, Thomas Zimmermann wrote:
>>>>>> Implementing vmap() and vunmap() of struct drm_gem_object_funcs is
>>>>>> required by generic fbdev emulation. Supporting free() is easy as
>>>>>> well. More udl-specific interfaces can probably be replaced by GEM
>>>>>> object functions in later patches.
>>>>>>
>>>>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>>>>>> ---
>>>>>>  drivers/gpu/drm/udl/udl_gem.c | 34 ++++++++++++++++++++++++++++++++++
>>>>>>  1 file changed, 34 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/udl/udl_gem.c b/drivers/gpu/drm/udl/udl_gem.c
>>>>>> index 3ea0cd9ae2d6..6ca097c270d6 100644
>>>>>> --- a/drivers/gpu/drm/udl/udl_gem.c
>>>>>> +++ b/drivers/gpu/drm/udl/udl_gem.c
>>>>>> @@ -11,6 +11,8 @@
>>>>>>
>>>>>>  #include "udl_drv.h"
>>>>>>
>>>>>> +static const struct drm_gem_object_funcs udl_gem_object_funcs;
>>>>>> +
>>>>>>  struct udl_gem_object *udl_gem_alloc_object(struct drm_device *dev,
>>>>>>                                          size_t size)
>>>>>>  {
>>>>>> @@ -25,6 +27,7 @@ struct udl_gem_object *udl_gem_alloc_object(struct drm_device *dev,
>>>>>>              return NULL;
>>>>>>      }
>>>>>>
>>>>>> +    obj->base.funcs = &udl_gem_object_funcs;
>>>>>>      obj->flags = UDL_BO_CACHEABLE;
>>>>>>      return obj;
>>>>>>  }
>>>>>> @@ -230,3 +233,34 @@ int udl_gem_mmap(struct drm_file *file, struct drm_device *dev,
>>>>>>      mutex_unlock(&udl->gem_lock);
>>>>>>      return ret;
>>>>>>  }
>>>>>> +
>>>>>> +/*
>>>>>> + * Helpers for struct drm_gem_object_funcs
>>>>>> + */
>>>>>> +
>>>>>> +static void udl_gem_object_free(struct drm_gem_object *obj)
>>>>>> +{
>>>>>> +    udl_gem_free_object(obj);
>>>>>> +}
>>>>>> +
>>>>>> +static void *udl_gem_object_vmap(struct drm_gem_object *obj)
>>>>>> +{
>>>>>> +    struct udl_gem_object *uobj = to_udl_bo(obj);
>>>>>> +    int ret;
>>>>>> +
>>>>>> +    ret = udl_gem_vmap(uobj);
>>>>>> +    if (ret)
>>>>>> +            return ERR_PTR(ret);
>>>>>> +    return uobj->vmapping;
>>>>>> +}
>>>>>> +
>>>>>> +static void udl_gem_object_vunmap(struct drm_gem_object *obj, void *vaddr)
>>>>>> +{
>>>>>> +    return udl_gem_vunmap(to_udl_bo(obj));
>>>>>> +}
>>>>>> +
>>>>>> +static const struct drm_gem_object_funcs udl_gem_object_funcs = {
>>>>>> +    .free   = udl_gem_object_free,
>>>>>> +    .vmap   = udl_gem_object_vmap,
>>>>>> +    .vunmap = udl_gem_object_vunmap,
>>>>>
>>>>> Yeah this doesn't work, you need to refcount the vmap here I think. I'd
>>>>
>>>> I see. Should have thought of that...
>>>>
>>>>> say simpler to first cut over to shmem helpers.
>>>>
>>>> Right. I was already attempting the conversion and the udl gem is more
>>>> or less the same as shmem, except for the flags field. [1] The flag
>>>> signals page attributes for mmap-ing [2]. For prime-imported BOs its is
>>>> set to writecombining, and for local BOs it is set to cached. Shmem
>>>> always maps with writecombining.
>>>>
>>>> I don't see why udl BOs are special wrt to mmap-ing. It seems to be some
>>>> kind of optimization. Do you have an idea?
>>>>
>>>> Best regards
>>>> Thomas
>>>>
>>>> [1]
>>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/udl/udl_drv.h?h=v5.3#n78
>>>> [2]
>>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/udl/udl_gem.c?h=v5.3#n57
>>>
>>> udl doesn't set prefer_shadow = 1, which means compositors will render
>>> directly into the buffer. And for that you want caching. For imported
>>> dma-buf otoh you want wc to make sure it's coherent. Otherwise I guess
>>> udl would need to get the dma_buf_begin/end_cpu_access calls added
>>> (and that would probably be faster than going with wc for everything).
>>> So we do want cachable, it's going to suck otherwise.
>>
>> Thanks for clarifying. Actually, it does have these calls in its dirty
>> handler. [1]
>>
>>>
>>> But that should be fairly easy to do by overwriting the obj->mmap
>>> callback and wrapping it around the shmem one.
>>
>> Is there a reason why shmem doesn't implement the wc-vs.cached logic?
>> Could this be added?
>>
> 
> I had a flag to set this in the initial version of the shmem helper
> modeled after udl, but Thomas Hellstrom brought up a question and it was
> dropped. The issue was beyond my understanding:
> 
> [PATCH v3 0/2] drm: Add shmem GEM library
> https://lists.freedesktop.org/archives/dri-devel/2018-September/190001.html
> 
> In tinydrm I have sped up uncached writes on arm by copying one pixel

s/writes/reads/

> line at a time into a temporary buffer before accessing the individual
> bytes. See drm_fb_swab16().
> 
> Noralf.
> 
>> (I'm just trying to understand the available options here before
>> attempting to do a conversion.)
>>
>> Best regards
>> Thomas
>>
>>>
>>> What surprises me more is that udl supports mmap on imported dma-buf,
>>> that's some real quirky stuff. Not sure there's really other drivers
>>> doing that. Might be easier to rip that out :-)
>>> -Daniel
>>>
>>
>> [1]
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/udl/udl_fb.c?h=v5.3#n293
>>
>>
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 31+ messages in thread
* Re: [PATCH 3/5] drm/udl: Add GEM object functions for free(), vmap(), and vunmap()
@ 2019-10-25 12:20               ` Thomas Zimmermann
  0 siblings, 0 replies; 31+ messages in thread
From: Thomas Zimmermann @ 2019-10-25 12:20 UTC (permalink / raw)
  To: Noralf Trønnes, Daniel Vetter
  Cc: Dave Airlie, Sean Paul, dri-devel, Sam Ravnborg, Gerd Hoffmann


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

(cc: Gerd)

Hi

Am 25.10.19 um 13:44 schrieb Noralf Trønnes:
> 
> 
> Den 25.10.2019 12.12, skrev Thomas Zimmermann:
>> Hi
>>
>> Am 25.10.19 um 11:28 schrieb Daniel Vetter:
>>> On Fri, Oct 25, 2019 at 9:59 AM Thomas Zimmermann <tzimmermann@suse.de> wrote
>>>>
>>>> Hi
>>>>
>>>> Am 25.10.19 um 09:40 schrieb Daniel Vetter:
>>>>> On Thu, Oct 24, 2019 at 04:42:35PM +0200, Thomas Zimmermann wrote:
>>>>>> Implementing vmap() and vunmap() of struct drm_gem_object_funcs is
>>>>>> required by generic fbdev emulation. Supporting free() is easy as
>>>>>> well. More udl-specific interfaces can probably be replaced by GEM
>>>>>> object functions in later patches.
>>>>>>
>>>>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>>>>>> ---
>>>>>>  drivers/gpu/drm/udl/udl_gem.c | 34 ++++++++++++++++++++++++++++++++++
>>>>>>  1 file changed, 34 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/udl/udl_gem.c b/drivers/gpu/drm/udl/udl_gem.c
>>>>>> index 3ea0cd9ae2d6..6ca097c270d6 100644
>>>>>> --- a/drivers/gpu/drm/udl/udl_gem.c
>>>>>> +++ b/drivers/gpu/drm/udl/udl_gem.c
>>>>>> @@ -11,6 +11,8 @@
>>>>>>
>>>>>>  #include "udl_drv.h"
>>>>>>
>>>>>> +static const struct drm_gem_object_funcs udl_gem_object_funcs;
>>>>>> +
>>>>>>  struct udl_gem_object *udl_gem_alloc_object(struct drm_device *dev,
>>>>>>                                          size_t size)
>>>>>>  {
>>>>>> @@ -25,6 +27,7 @@ struct udl_gem_object *udl_gem_alloc_object(struct drm_device *dev,
>>>>>>              return NULL;
>>>>>>      }
>>>>>>
>>>>>> +    obj->base.funcs = &udl_gem_object_funcs;
>>>>>>      obj->flags = UDL_BO_CACHEABLE;
>>>>>>      return obj;
>>>>>>  }
>>>>>> @@ -230,3 +233,34 @@ int udl_gem_mmap(struct drm_file *file, struct drm_device *dev,
>>>>>>      mutex_unlock(&udl->gem_lock);
>>>>>>      return ret;
>>>>>>  }
>>>>>> +
>>>>>> +/*
>>>>>> + * Helpers for struct drm_gem_object_funcs
>>>>>> + */
>>>>>> +
>>>>>> +static void udl_gem_object_free(struct drm_gem_object *obj)
>>>>>> +{
>>>>>> +    udl_gem_free_object(obj);
>>>>>> +}
>>>>>> +
>>>>>> +static void *udl_gem_object_vmap(struct drm_gem_object *obj)
>>>>>> +{
>>>>>> +    struct udl_gem_object *uobj = to_udl_bo(obj);
>>>>>> +    int ret;
>>>>>> +
>>>>>> +    ret = udl_gem_vmap(uobj);
>>>>>> +    if (ret)
>>>>>> +            return ERR_PTR(ret);
>>>>>> +    return uobj->vmapping;
>>>>>> +}
>>>>>> +
>>>>>> +static void udl_gem_object_vunmap(struct drm_gem_object *obj, void *vaddr)
>>>>>> +{
>>>>>> +    return udl_gem_vunmap(to_udl_bo(obj));
>>>>>> +}
>>>>>> +
>>>>>> +static const struct drm_gem_object_funcs udl_gem_object_funcs = {
>>>>>> +    .free   = udl_gem_object_free,
>>>>>> +    .vmap   = udl_gem_object_vmap,
>>>>>> +    .vunmap = udl_gem_object_vunmap,
>>>>>
>>>>> Yeah this doesn't work, you need to refcount the vmap here I think. I'd
>>>>
>>>> I see. Should have thought of that...
>>>>
>>>>> say simpler to first cut over to shmem helpers.
>>>>
>>>> Right. I was already attempting the conversion and the udl gem is more
>>>> or less the same as shmem, except for the flags field. [1] The flag
>>>> signals page attributes for mmap-ing [2]. For prime-imported BOs its is
>>>> set to writecombining, and for local BOs it is set to cached. Shmem
>>>> always maps with writecombining.
>>>>
>>>> I don't see why udl BOs are special wrt to mmap-ing. It seems to be some
>>>> kind of optimization. Do you have an idea?
>>>>
>>>> Best regards
>>>> Thomas
>>>>
>>>> [1]
>>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/udl/udl_drv.h?h=v5.3#n78
>>>> [2]
>>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/udl/udl_gem.c?h=v5.3#n57
>>>
>>> udl doesn't set prefer_shadow = 1, which means compositors will render
>>> directly into the buffer. And for that you want caching. For imported
>>> dma-buf otoh you want wc to make sure it's coherent. Otherwise I guess
>>> udl would need to get the dma_buf_begin/end_cpu_access calls added
>>> (and that would probably be faster than going with wc for everything).
>>> So we do want cachable, it's going to suck otherwise.
>>
>> Thanks for clarifying. Actually, it does have these calls in its dirty
>> handler. [1]
>>
>>>
>>> But that should be fairly easy to do by overwriting the obj->mmap
>>> callback and wrapping it around the shmem one.
>>
>> Is there a reason why shmem doesn't implement the wc-vs.cached logic?
>> Could this be added?
>>
> 
> I had a flag to set this in the initial version of the shmem helper
> modeled after udl, but Thomas Hellstrom brought up a question and it was
> dropped. The issue was beyond my understanding:
> 
> [PATCH v3 0/2] drm: Add shmem GEM library
> https://lists.freedesktop.org/archives/dri-devel/2018-September/190001.html

If I understand that discussion correctly, the concern was that write
combining and shared memory would not work well together. So you went
with always-cached?

Just recently, Gerd added unconditional write combining in rev 0be8958936.

Best regards
Thomas

> 
> In tinydrm I have sped up uncached writes on arm by copying one pixel
> line at a time into a temporary buffer before accessing the individual
> bytes. See drm_fb_swab16().
> 
> Noralf.
> 
>> (I'm just trying to understand the available options here before
>> attempting to do a conversion.)
>>
>> Best regards
>> Thomas
>>
>>>
>>> What surprises me more is that udl supports mmap on imported dma-buf,
>>> that's some real quirky stuff. Not sure there's really other drivers
>>> doing that. Might be easier to rip that out :-)
>>> -Daniel
>>>
>>
>> [1]
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/udl/udl_fb.c?h=v5.3#n293
>>
>>
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>
> _______________________________________________
> 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: 159 bytes --]

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

^ permalink raw reply	[flat|nested] 31+ messages in thread
* Re: [PATCH 3/5] drm/udl: Add GEM object functions for free(), vmap(),  and vunmap()
@ 2019-10-25 12:20               ` Thomas Zimmermann
  0 siblings, 0 replies; 31+ messages in thread
From: Thomas Zimmermann @ 2019-10-25 12:20 UTC (permalink / raw)
  To: Noralf Trønnes, Daniel Vetter
  Cc: Dave Airlie, Sean Paul, dri-devel, Sam Ravnborg, Gerd Hoffmann


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

(cc: Gerd)

Hi

Am 25.10.19 um 13:44 schrieb Noralf Trønnes:
> 
> 
> Den 25.10.2019 12.12, skrev Thomas Zimmermann:
>> Hi
>>
>> Am 25.10.19 um 11:28 schrieb Daniel Vetter:
>>> On Fri, Oct 25, 2019 at 9:59 AM Thomas Zimmermann <tzimmermann@suse.de> wrote
>>>>
>>>> Hi
>>>>
>>>> Am 25.10.19 um 09:40 schrieb Daniel Vetter:
>>>>> On Thu, Oct 24, 2019 at 04:42:35PM +0200, Thomas Zimmermann wrote:
>>>>>> Implementing vmap() and vunmap() of struct drm_gem_object_funcs is
>>>>>> required by generic fbdev emulation. Supporting free() is easy as
>>>>>> well. More udl-specific interfaces can probably be replaced by GEM
>>>>>> object functions in later patches.
>>>>>>
>>>>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>>>>>> ---
>>>>>>  drivers/gpu/drm/udl/udl_gem.c | 34 ++++++++++++++++++++++++++++++++++
>>>>>>  1 file changed, 34 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/udl/udl_gem.c b/drivers/gpu/drm/udl/udl_gem.c
>>>>>> index 3ea0cd9ae2d6..6ca097c270d6 100644
>>>>>> --- a/drivers/gpu/drm/udl/udl_gem.c
>>>>>> +++ b/drivers/gpu/drm/udl/udl_gem.c
>>>>>> @@ -11,6 +11,8 @@
>>>>>>
>>>>>>  #include "udl_drv.h"
>>>>>>
>>>>>> +static const struct drm_gem_object_funcs udl_gem_object_funcs;
>>>>>> +
>>>>>>  struct udl_gem_object *udl_gem_alloc_object(struct drm_device *dev,
>>>>>>                                          size_t size)
>>>>>>  {
>>>>>> @@ -25,6 +27,7 @@ struct udl_gem_object *udl_gem_alloc_object(struct drm_device *dev,
>>>>>>              return NULL;
>>>>>>      }
>>>>>>
>>>>>> +    obj->base.funcs = &udl_gem_object_funcs;
>>>>>>      obj->flags = UDL_BO_CACHEABLE;
>>>>>>      return obj;
>>>>>>  }
>>>>>> @@ -230,3 +233,34 @@ int udl_gem_mmap(struct drm_file *file, struct drm_device *dev,
>>>>>>      mutex_unlock(&udl->gem_lock);
>>>>>>      return ret;
>>>>>>  }
>>>>>> +
>>>>>> +/*
>>>>>> + * Helpers for struct drm_gem_object_funcs
>>>>>> + */
>>>>>> +
>>>>>> +static void udl_gem_object_free(struct drm_gem_object *obj)
>>>>>> +{
>>>>>> +    udl_gem_free_object(obj);
>>>>>> +}
>>>>>> +
>>>>>> +static void *udl_gem_object_vmap(struct drm_gem_object *obj)
>>>>>> +{
>>>>>> +    struct udl_gem_object *uobj = to_udl_bo(obj);
>>>>>> +    int ret;
>>>>>> +
>>>>>> +    ret = udl_gem_vmap(uobj);
>>>>>> +    if (ret)
>>>>>> +            return ERR_PTR(ret);
>>>>>> +    return uobj->vmapping;
>>>>>> +}
>>>>>> +
>>>>>> +static void udl_gem_object_vunmap(struct drm_gem_object *obj, void *vaddr)
>>>>>> +{
>>>>>> +    return udl_gem_vunmap(to_udl_bo(obj));
>>>>>> +}
>>>>>> +
>>>>>> +static const struct drm_gem_object_funcs udl_gem_object_funcs = {
>>>>>> +    .free   = udl_gem_object_free,
>>>>>> +    .vmap   = udl_gem_object_vmap,
>>>>>> +    .vunmap = udl_gem_object_vunmap,
>>>>>
>>>>> Yeah this doesn't work, you need to refcount the vmap here I think. I'd
>>>>
>>>> I see. Should have thought of that...
>>>>
>>>>> say simpler to first cut over to shmem helpers.
>>>>
>>>> Right. I was already attempting the conversion and the udl gem is more
>>>> or less the same as shmem, except for the flags field. [1] The flag
>>>> signals page attributes for mmap-ing [2]. For prime-imported BOs its is
>>>> set to writecombining, and for local BOs it is set to cached. Shmem
>>>> always maps with writecombining.
>>>>
>>>> I don't see why udl BOs are special wrt to mmap-ing. It seems to be some
>>>> kind of optimization. Do you have an idea?
>>>>
>>>> Best regards
>>>> Thomas
>>>>
>>>> [1]
>>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/udl/udl_drv.h?h=v5.3#n78
>>>> [2]
>>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/udl/udl_gem.c?h=v5.3#n57
>>>
>>> udl doesn't set prefer_shadow = 1, which means compositors will render
>>> directly into the buffer. And for that you want caching. For imported
>>> dma-buf otoh you want wc to make sure it's coherent. Otherwise I guess
>>> udl would need to get the dma_buf_begin/end_cpu_access calls added
>>> (and that would probably be faster than going with wc for everything).
>>> So we do want cachable, it's going to suck otherwise.
>>
>> Thanks for clarifying. Actually, it does have these calls in its dirty
>> handler. [1]
>>
>>>
>>> But that should be fairly easy to do by overwriting the obj->mmap
>>> callback and wrapping it around the shmem one.
>>
>> Is there a reason why shmem doesn't implement the wc-vs.cached logic?
>> Could this be added?
>>
> 
> I had a flag to set this in the initial version of the shmem helper
> modeled after udl, but Thomas Hellstrom brought up a question and it was
> dropped. The issue was beyond my understanding:
> 
> [PATCH v3 0/2] drm: Add shmem GEM library
> https://lists.freedesktop.org/archives/dri-devel/2018-September/190001.html

If I understand that discussion correctly, the concern was that write
combining and shared memory would not work well together. So you went
with always-cached?

Just recently, Gerd added unconditional write combining in rev 0be8958936.

Best regards
Thomas

> 
> In tinydrm I have sped up uncached writes on arm by copying one pixel
> line at a time into a temporary buffer before accessing the individual
> bytes. See drm_fb_swab16().
> 
> Noralf.
> 
>> (I'm just trying to understand the available options here before
>> attempting to do a conversion.)
>>
>> Best regards
>> Thomas
>>
>>>
>>> What surprises me more is that udl supports mmap on imported dma-buf,
>>> that's some real quirky stuff. Not sure there's really other drivers
>>> doing that. Might be easier to rip that out :-)
>>> -Daniel
>>>
>>
>> [1]
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/udl/udl_fb.c?h=v5.3#n293
>>
>>
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>
> _______________________________________________
> 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: 159 bytes --]

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

^ permalink raw reply	[flat|nested] 31+ messages in thread
* Re: [PATCH 3/5] drm/udl: Add GEM object functions for free(), vmap(), and vunmap()
  2019-10-25 12:20               ` Thomas Zimmermann
  (?)
@ 2019-10-25 13:32               ` Gerd Hoffmann
  2019-10-25 14:53                   ` Thomas Zimmermann
  -1 siblings, 1 reply; 31+ messages in thread
From: Gerd Hoffmann @ 2019-10-25 13:32 UTC (permalink / raw)
  To: Thomas Zimmermann; +Cc: Sean Paul, dri-devel, Dave Airlie, Sam Ravnborg

  Hi,

> > I had a flag to set this in the initial version of the shmem helper
> > modeled after udl, but Thomas Hellstrom brought up a question and it was
> > dropped. The issue was beyond my understanding:
> > 
> > [PATCH v3 0/2] drm: Add shmem GEM library
> > https://lists.freedesktop.org/archives/dri-devel/2018-September/190001.html
> 
> If I understand that discussion correctly, the concern was that write
> combining and shared memory would not work well together. So you went
> with always-cached?
> 
> Just recently, Gerd added unconditional write combining in rev 0be8958936.

Well, it's not really added.  It's the same thing drm_gem_mmap_obj()
does for you when you don't have a drm_gem_object_funcs.mmap callback.

But, yes, the reason this is done in the driver's mmap() callback with
the new mmap code path is to give drivers the option to override this
by supplying their own mmap() handler.  So going with shmem helpers +
custom mmap callback is a reasonable approach.

HTH,
  Gerd

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

^ permalink raw reply	[flat|nested] 31+ messages in thread
* Re: [PATCH 3/5] drm/udl: Add GEM object functions for free(), vmap(), and vunmap()
@ 2019-10-25 13:44                 ` Noralf Trønnes
  0 siblings, 0 replies; 31+ messages in thread
From: Noralf Trønnes @ 2019-10-25 13:44 UTC (permalink / raw)
  To: Thomas Zimmermann, Daniel Vetter
  Cc: Dave Airlie, Sean Paul, dri-devel, Sam Ravnborg, Gerd Hoffmann



Den 25.10.2019 14.20, skrev Thomas Zimmermann:
> (cc: Gerd)
> 
> Hi
> 
> Am 25.10.19 um 13:44 schrieb Noralf Trønnes:
>>
>>
>> Den 25.10.2019 12.12, skrev Thomas Zimmermann:
>>> Hi
>>>
>>> Am 25.10.19 um 11:28 schrieb Daniel Vetter:
>>>> On Fri, Oct 25, 2019 at 9:59 AM Thomas Zimmermann <tzimmermann@suse.de> wrote
>>>>>
>>>>> Hi
>>>>>
>>>>> Am 25.10.19 um 09:40 schrieb Daniel Vetter:
>>>>>> On Thu, Oct 24, 2019 at 04:42:35PM +0200, Thomas Zimmermann wrote:
>>>>>>> Implementing vmap() and vunmap() of struct drm_gem_object_funcs is
>>>>>>> required by generic fbdev emulation. Supporting free() is easy as
>>>>>>> well. More udl-specific interfaces can probably be replaced by GEM
>>>>>>> object functions in later patches.
>>>>>>>
>>>>>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>>>>>>> ---
>>>>>>>  drivers/gpu/drm/udl/udl_gem.c | 34 ++++++++++++++++++++++++++++++++++
>>>>>>>  1 file changed, 34 insertions(+)
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/udl/udl_gem.c b/drivers/gpu/drm/udl/udl_gem.c
>>>>>>> index 3ea0cd9ae2d6..6ca097c270d6 100644
>>>>>>> --- a/drivers/gpu/drm/udl/udl_gem.c
>>>>>>> +++ b/drivers/gpu/drm/udl/udl_gem.c
>>>>>>> @@ -11,6 +11,8 @@
>>>>>>>
>>>>>>>  #include "udl_drv.h"
>>>>>>>
>>>>>>> +static const struct drm_gem_object_funcs udl_gem_object_funcs;
>>>>>>> +
>>>>>>>  struct udl_gem_object *udl_gem_alloc_object(struct drm_device *dev,
>>>>>>>                                          size_t size)
>>>>>>>  {
>>>>>>> @@ -25,6 +27,7 @@ struct udl_gem_object *udl_gem_alloc_object(struct drm_device *dev,
>>>>>>>              return NULL;
>>>>>>>      }
>>>>>>>
>>>>>>> +    obj->base.funcs = &udl_gem_object_funcs;
>>>>>>>      obj->flags = UDL_BO_CACHEABLE;
>>>>>>>      return obj;
>>>>>>>  }
>>>>>>> @@ -230,3 +233,34 @@ int udl_gem_mmap(struct drm_file *file, struct drm_device *dev,
>>>>>>>      mutex_unlock(&udl->gem_lock);
>>>>>>>      return ret;
>>>>>>>  }
>>>>>>> +
>>>>>>> +/*
>>>>>>> + * Helpers for struct drm_gem_object_funcs
>>>>>>> + */
>>>>>>> +
>>>>>>> +static void udl_gem_object_free(struct drm_gem_object *obj)
>>>>>>> +{
>>>>>>> +    udl_gem_free_object(obj);
>>>>>>> +}
>>>>>>> +
>>>>>>> +static void *udl_gem_object_vmap(struct drm_gem_object *obj)
>>>>>>> +{
>>>>>>> +    struct udl_gem_object *uobj = to_udl_bo(obj);
>>>>>>> +    int ret;
>>>>>>> +
>>>>>>> +    ret = udl_gem_vmap(uobj);
>>>>>>> +    if (ret)
>>>>>>> +            return ERR_PTR(ret);
>>>>>>> +    return uobj->vmapping;
>>>>>>> +}
>>>>>>> +
>>>>>>> +static void udl_gem_object_vunmap(struct drm_gem_object *obj, void *vaddr)
>>>>>>> +{
>>>>>>> +    return udl_gem_vunmap(to_udl_bo(obj));
>>>>>>> +}
>>>>>>> +
>>>>>>> +static const struct drm_gem_object_funcs udl_gem_object_funcs = {
>>>>>>> +    .free   = udl_gem_object_free,
>>>>>>> +    .vmap   = udl_gem_object_vmap,
>>>>>>> +    .vunmap = udl_gem_object_vunmap,
>>>>>>
>>>>>> Yeah this doesn't work, you need to refcount the vmap here I think. I'd
>>>>>
>>>>> I see. Should have thought of that...
>>>>>
>>>>>> say simpler to first cut over to shmem helpers.
>>>>>
>>>>> Right. I was already attempting the conversion and the udl gem is more
>>>>> or less the same as shmem, except for the flags field. [1] The flag
>>>>> signals page attributes for mmap-ing [2]. For prime-imported BOs its is
>>>>> set to writecombining, and for local BOs it is set to cached. Shmem
>>>>> always maps with writecombining.
>>>>>
>>>>> I don't see why udl BOs are special wrt to mmap-ing. It seems to be some
>>>>> kind of optimization. Do you have an idea?
>>>>>
>>>>> Best regards
>>>>> Thomas
>>>>>
>>>>> [1]
>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/udl/udl_drv.h?h=v5.3#n78
>>>>> [2]
>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/udl/udl_gem.c?h=v5.3#n57
>>>>
>>>> udl doesn't set prefer_shadow = 1, which means compositors will render
>>>> directly into the buffer. And for that you want caching. For imported
>>>> dma-buf otoh you want wc to make sure it's coherent. Otherwise I guess
>>>> udl would need to get the dma_buf_begin/end_cpu_access calls added
>>>> (and that would probably be faster than going with wc for everything).
>>>> So we do want cachable, it's going to suck otherwise.
>>>
>>> Thanks for clarifying. Actually, it does have these calls in its dirty
>>> handler. [1]
>>>
>>>>
>>>> But that should be fairly easy to do by overwriting the obj->mmap
>>>> callback and wrapping it around the shmem one.
>>>
>>> Is there a reason why shmem doesn't implement the wc-vs.cached logic?
>>> Could this be added?
>>>
>>
>> I had a flag to set this in the initial version of the shmem helper
>> modeled after udl, but Thomas Hellstrom brought up a question and it was
>> dropped. The issue was beyond my understanding:
>>
>> [PATCH v3 0/2] drm: Add shmem GEM library
>> https://lists.freedesktop.org/archives/dri-devel/2018-September/190001.html
> 
> If I understand that discussion correctly, the concern was that write
> combining and shared memory would not work well together. So you went
> with always-cached?
> 
> Just recently, Gerd added unconditional write combining in rev 0be8958936.
> 

I don't remember, Rob picked up the patch since I couldn't use shmem
after all for tinydrm.

I see in the commit message that he made this change:

v7:
- Use write-combine for mmap instead. This is the more common
  case. (robher)

Then we have this:
drm/gem_shmem: Use a writecombine mapping for ->vaddr (be7d9f05c53e)

So it looks like my initial work has been fixed up piece by piece to use
writecombine.

Noralf.

> Best regards
> Thomas
> 
>>
>> In tinydrm I have sped up uncached writes on arm by copying one pixel
>> line at a time into a temporary buffer before accessing the individual
>> bytes. See drm_fb_swab16().
>>
>> Noralf.
>>
>>> (I'm just trying to understand the available options here before
>>> attempting to do a conversion.)
>>>
>>> Best regards
>>> Thomas
>>>
>>>>
>>>> What surprises me more is that udl supports mmap on imported dma-buf,
>>>> that's some real quirky stuff. Not sure there's really other drivers
>>>> doing that. Might be easier to rip that out :-)
>>>> -Daniel
>>>>
>>>
>>> [1]
>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/udl/udl_fb.c?h=v5.3#n293
>>>
>>>
>>>
>>> _______________________________________________
>>> dri-devel mailing list
>>> dri-devel@lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 31+ messages in thread
* Re: [PATCH 3/5] drm/udl: Add GEM object functions for free(), vmap(),  and vunmap()
@ 2019-10-25 13:44                 ` Noralf Trønnes
  0 siblings, 0 replies; 31+ messages in thread
From: Noralf Trønnes @ 2019-10-25 13:44 UTC (permalink / raw)
  To: Thomas Zimmermann, Daniel Vetter
  Cc: Dave Airlie, Sean Paul, dri-devel, Sam Ravnborg, Gerd Hoffmann



Den 25.10.2019 14.20, skrev Thomas Zimmermann:
> (cc: Gerd)
> 
> Hi
> 
> Am 25.10.19 um 13:44 schrieb Noralf Trønnes:
>>
>>
>> Den 25.10.2019 12.12, skrev Thomas Zimmermann:
>>> Hi
>>>
>>> Am 25.10.19 um 11:28 schrieb Daniel Vetter:
>>>> On Fri, Oct 25, 2019 at 9:59 AM Thomas Zimmermann <tzimmermann@suse.de> wrote
>>>>>
>>>>> Hi
>>>>>
>>>>> Am 25.10.19 um 09:40 schrieb Daniel Vetter:
>>>>>> On Thu, Oct 24, 2019 at 04:42:35PM +0200, Thomas Zimmermann wrote:
>>>>>>> Implementing vmap() and vunmap() of struct drm_gem_object_funcs is
>>>>>>> required by generic fbdev emulation. Supporting free() is easy as
>>>>>>> well. More udl-specific interfaces can probably be replaced by GEM
>>>>>>> object functions in later patches.
>>>>>>>
>>>>>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>>>>>>> ---
>>>>>>>  drivers/gpu/drm/udl/udl_gem.c | 34 ++++++++++++++++++++++++++++++++++
>>>>>>>  1 file changed, 34 insertions(+)
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/udl/udl_gem.c b/drivers/gpu/drm/udl/udl_gem.c
>>>>>>> index 3ea0cd9ae2d6..6ca097c270d6 100644
>>>>>>> --- a/drivers/gpu/drm/udl/udl_gem.c
>>>>>>> +++ b/drivers/gpu/drm/udl/udl_gem.c
>>>>>>> @@ -11,6 +11,8 @@
>>>>>>>
>>>>>>>  #include "udl_drv.h"
>>>>>>>
>>>>>>> +static const struct drm_gem_object_funcs udl_gem_object_funcs;
>>>>>>> +
>>>>>>>  struct udl_gem_object *udl_gem_alloc_object(struct drm_device *dev,
>>>>>>>                                          size_t size)
>>>>>>>  {
>>>>>>> @@ -25,6 +27,7 @@ struct udl_gem_object *udl_gem_alloc_object(struct drm_device *dev,
>>>>>>>              return NULL;
>>>>>>>      }
>>>>>>>
>>>>>>> +    obj->base.funcs = &udl_gem_object_funcs;
>>>>>>>      obj->flags = UDL_BO_CACHEABLE;
>>>>>>>      return obj;
>>>>>>>  }
>>>>>>> @@ -230,3 +233,34 @@ int udl_gem_mmap(struct drm_file *file, struct drm_device *dev,
>>>>>>>      mutex_unlock(&udl->gem_lock);
>>>>>>>      return ret;
>>>>>>>  }
>>>>>>> +
>>>>>>> +/*
>>>>>>> + * Helpers for struct drm_gem_object_funcs
>>>>>>> + */
>>>>>>> +
>>>>>>> +static void udl_gem_object_free(struct drm_gem_object *obj)
>>>>>>> +{
>>>>>>> +    udl_gem_free_object(obj);
>>>>>>> +}
>>>>>>> +
>>>>>>> +static void *udl_gem_object_vmap(struct drm_gem_object *obj)
>>>>>>> +{
>>>>>>> +    struct udl_gem_object *uobj = to_udl_bo(obj);
>>>>>>> +    int ret;
>>>>>>> +
>>>>>>> +    ret = udl_gem_vmap(uobj);
>>>>>>> +    if (ret)
>>>>>>> +            return ERR_PTR(ret);
>>>>>>> +    return uobj->vmapping;
>>>>>>> +}
>>>>>>> +
>>>>>>> +static void udl_gem_object_vunmap(struct drm_gem_object *obj, void *vaddr)
>>>>>>> +{
>>>>>>> +    return udl_gem_vunmap(to_udl_bo(obj));
>>>>>>> +}
>>>>>>> +
>>>>>>> +static const struct drm_gem_object_funcs udl_gem_object_funcs = {
>>>>>>> +    .free   = udl_gem_object_free,
>>>>>>> +    .vmap   = udl_gem_object_vmap,
>>>>>>> +    .vunmap = udl_gem_object_vunmap,
>>>>>>
>>>>>> Yeah this doesn't work, you need to refcount the vmap here I think. I'd
>>>>>
>>>>> I see. Should have thought of that...
>>>>>
>>>>>> say simpler to first cut over to shmem helpers.
>>>>>
>>>>> Right. I was already attempting the conversion and the udl gem is more
>>>>> or less the same as shmem, except for the flags field. [1] The flag
>>>>> signals page attributes for mmap-ing [2]. For prime-imported BOs its is
>>>>> set to writecombining, and for local BOs it is set to cached. Shmem
>>>>> always maps with writecombining.
>>>>>
>>>>> I don't see why udl BOs are special wrt to mmap-ing. It seems to be some
>>>>> kind of optimization. Do you have an idea?
>>>>>
>>>>> Best regards
>>>>> Thomas
>>>>>
>>>>> [1]
>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/udl/udl_drv.h?h=v5.3#n78
>>>>> [2]
>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/udl/udl_gem.c?h=v5.3#n57
>>>>
>>>> udl doesn't set prefer_shadow = 1, which means compositors will render
>>>> directly into the buffer. And for that you want caching. For imported
>>>> dma-buf otoh you want wc to make sure it's coherent. Otherwise I guess
>>>> udl would need to get the dma_buf_begin/end_cpu_access calls added
>>>> (and that would probably be faster than going with wc for everything).
>>>> So we do want cachable, it's going to suck otherwise.
>>>
>>> Thanks for clarifying. Actually, it does have these calls in its dirty
>>> handler. [1]
>>>
>>>>
>>>> But that should be fairly easy to do by overwriting the obj->mmap
>>>> callback and wrapping it around the shmem one.
>>>
>>> Is there a reason why shmem doesn't implement the wc-vs.cached logic?
>>> Could this be added?
>>>
>>
>> I had a flag to set this in the initial version of the shmem helper
>> modeled after udl, but Thomas Hellstrom brought up a question and it was
>> dropped. The issue was beyond my understanding:
>>
>> [PATCH v3 0/2] drm: Add shmem GEM library
>> https://lists.freedesktop.org/archives/dri-devel/2018-September/190001.html
> 
> If I understand that discussion correctly, the concern was that write
> combining and shared memory would not work well together. So you went
> with always-cached?
> 
> Just recently, Gerd added unconditional write combining in rev 0be8958936.
> 

I don't remember, Rob picked up the patch since I couldn't use shmem
after all for tinydrm.

I see in the commit message that he made this change:

v7:
- Use write-combine for mmap instead. This is the more common
  case. (robher)

Then we have this:
drm/gem_shmem: Use a writecombine mapping for ->vaddr (be7d9f05c53e)

So it looks like my initial work has been fixed up piece by piece to use
writecombine.

Noralf.

> Best regards
> Thomas
> 
>>
>> In tinydrm I have sped up uncached writes on arm by copying one pixel
>> line at a time into a temporary buffer before accessing the individual
>> bytes. See drm_fb_swab16().
>>
>> Noralf.
>>
>>> (I'm just trying to understand the available options here before
>>> attempting to do a conversion.)
>>>
>>> Best regards
>>> Thomas
>>>
>>>>
>>>> What surprises me more is that udl supports mmap on imported dma-buf,
>>>> that's some real quirky stuff. Not sure there's really other drivers
>>>> doing that. Might be easier to rip that out :-)
>>>> -Daniel
>>>>
>>>
>>> [1]
>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/udl/udl_fb.c?h=v5.3#n293
>>>
>>>
>>>
>>> _______________________________________________
>>> dri-devel mailing list
>>> dri-devel@lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 31+ messages in thread
* Re: [PATCH 3/5] drm/udl: Add GEM object functions for free(), vmap(), and vunmap()
@ 2019-10-25 14:53                   ` Thomas Zimmermann
  0 siblings, 0 replies; 31+ messages in thread
From: Thomas Zimmermann @ 2019-10-25 14:53 UTC (permalink / raw)
  To: Gerd Hoffmann; +Cc: Sam Ravnborg, Dave Airlie, Sean Paul, dri-devel


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

Hi

Am 25.10.19 um 15:32 schrieb Gerd Hoffmann:
>   Hi,
> 
>>> I had a flag to set this in the initial version of the shmem helper
>>> modeled after udl, but Thomas Hellstrom brought up a question and it was
>>> dropped. The issue was beyond my understanding:
>>>
>>> [PATCH v3 0/2] drm: Add shmem GEM library
>>> https://lists.freedesktop.org/archives/dri-devel/2018-September/190001.html
>>
>> If I understand that discussion correctly, the concern was that write
>> combining and shared memory would not work well together. So you went
>> with always-cached?
>>
>> Just recently, Gerd added unconditional write combining in rev 0be8958936.
> 
> Well, it's not really added.  It's the same thing drm_gem_mmap_obj()
> does for you when you don't have a drm_gem_object_funcs.mmap callback.
> 
> But, yes, the reason this is done in the driver's mmap() callback with
> the new mmap code path is to give drivers the option to override this
> by supplying their own mmap() handler.  So going with shmem helpers +
> custom mmap callback is a reasonable approach.
> 
> HTH,

Absolutely.

Thanks everyone for the feedback.

Best regards
Thomas

>   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: 159 bytes --]

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

^ permalink raw reply	[flat|nested] 31+ messages in thread
* Re: [PATCH 3/5] drm/udl: Add GEM object functions for free(), vmap(),  and vunmap()
@ 2019-10-25 14:53                   ` Thomas Zimmermann
  0 siblings, 0 replies; 31+ messages in thread
From: Thomas Zimmermann @ 2019-10-25 14:53 UTC (permalink / raw)
  To: Gerd Hoffmann; +Cc: Sam Ravnborg, Dave Airlie, Sean Paul, dri-devel


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

Hi

Am 25.10.19 um 15:32 schrieb Gerd Hoffmann:
>   Hi,
> 
>>> I had a flag to set this in the initial version of the shmem helper
>>> modeled after udl, but Thomas Hellstrom brought up a question and it was
>>> dropped. The issue was beyond my understanding:
>>>
>>> [PATCH v3 0/2] drm: Add shmem GEM library
>>> https://lists.freedesktop.org/archives/dri-devel/2018-September/190001.html
>>
>> If I understand that discussion correctly, the concern was that write
>> combining and shared memory would not work well together. So you went
>> with always-cached?
>>
>> Just recently, Gerd added unconditional write combining in rev 0be8958936.
> 
> Well, it's not really added.  It's the same thing drm_gem_mmap_obj()
> does for you when you don't have a drm_gem_object_funcs.mmap callback.
> 
> But, yes, the reason this is done in the driver's mmap() callback with
> the new mmap code path is to give drivers the option to override this
> by supplying their own mmap() handler.  So going with shmem helpers +
> custom mmap callback is a reasonable approach.
> 
> HTH,

Absolutely.

Thanks everyone for the feedback.

Best regards
Thomas

>   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: 159 bytes --]

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

^ permalink raw reply	[flat|nested] 31+ messages in thread
* Re: [PATCH 5/5] drm/udl: Replace fbdev code with generic emulation
  2019-10-25  8:08     ` Thomas Zimmermann
@ 2019-10-25 19:10       ` Daniel Vetter
  0 siblings, 0 replies; 31+ messages in thread
From: Daniel Vetter @ 2019-10-25 19:10 UTC (permalink / raw)
  To: Thomas Zimmermann; +Cc: Sam Ravnborg, Dave Airlie, Sean Paul, dri-devel

On Fri, Oct 25, 2019 at 10:08 AM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>
> Hi
>
> Am 25.10.19 um 09:47 schrieb Daniel Vetter:
> > On Thu, Oct 24, 2019 at 04:42:37PM +0200, Thomas Zimmermann wrote:
> >> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> >> ---
> >>  drivers/gpu/drm/udl/udl_drv.c     |   3 +
> >>  drivers/gpu/drm/udl/udl_drv.h     |   4 -
> >>  drivers/gpu/drm/udl/udl_fb.c      | 263 +-----------------------------
> >>  drivers/gpu/drm/udl/udl_main.c    |   2 -
> >>  drivers/gpu/drm/udl/udl_modeset.c |   3 +-
> >>  5 files changed, 8 insertions(+), 267 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/udl/udl_drv.c b/drivers/gpu/drm/udl/udl_drv.c
> >> index 15ad7a338f9d..6beaa1109c2c 100644
> >> --- a/drivers/gpu/drm/udl/udl_drv.c
> >> +++ b/drivers/gpu/drm/udl/udl_drv.c
> >> @@ -7,6 +7,7 @@
> >>
> >>  #include <drm/drm_crtc_helper.h>
> >>  #include <drm/drm_drv.h>
> >> +#include <drm/drm_fb_helper.h>
> >>  #include <drm/drm_file.h>
> >>  #include <drm/drm_ioctl.h>
> >>  #include <drm/drm_probe_helper.h>
> >> @@ -62,6 +63,8 @@ static struct drm_driver driver = {
> >>      .driver_features = DRIVER_MODESET | DRIVER_GEM,
> >>      .release = udl_driver_release,
> >>
> >> +    .lastclose = drm_fb_helper_lastclose,
> >> +
> >>      /* gem hooks */
> >>      .gem_free_object_unlocked = udl_gem_free_object,
> >>      .gem_vm_ops = &udl_gem_vm_ops,
> >> diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h
> >> index 12a970fd9a87..5f8a7ac084f6 100644
> >> --- a/drivers/gpu/drm/udl/udl_drv.h
> >> +++ b/drivers/gpu/drm/udl/udl_drv.h
> >> @@ -50,8 +50,6 @@ struct urb_list {
> >>      size_t size;
> >>  };
> >>
> >> -struct udl_fbdev;
> >> -
> >>  struct udl_device {
> >>      struct drm_device drm;
> >>      struct device *dev;
> >> @@ -65,7 +63,6 @@ struct udl_device {
> >>      struct urb_list urbs;
> >>      atomic_t lost_pixels; /* 1 = a render op failed. Need screen refresh */
> >>
> >> -    struct udl_fbdev *fbdev;
> >>      char mode_buf[1024];
> >>      uint32_t mode_buf_len;
> >>      atomic_t bytes_rendered; /* raw pixel-bytes driver asked to render */
> >> @@ -111,7 +108,6 @@ int udl_init(struct udl_device *udl);
> >>  void udl_fini(struct drm_device *dev);
> >>
> >>  int udl_fbdev_init(struct drm_device *dev);
> >> -void udl_fbdev_cleanup(struct drm_device *dev);
> >>  void udl_fbdev_unplug(struct drm_device *dev);
> >>  struct drm_framebuffer *
> >>  udl_fb_user_fb_create(struct drm_device *dev,
> >> diff --git a/drivers/gpu/drm/udl/udl_fb.c b/drivers/gpu/drm/udl/udl_fb.c
> >> index ef3504d06343..43a1da3a56c3 100644
> >> --- a/drivers/gpu/drm/udl/udl_fb.c
> >> +++ b/drivers/gpu/drm/udl/udl_fb.c
> >> @@ -19,19 +19,9 @@
> >>
> >>  #include "udl_drv.h"
> >>
> >> -#define DL_DEFIO_WRITE_DELAY    (HZ/20) /* fb_deferred_io.delay in jiffies */
> >> -
> >> -static int fb_defio = 0;  /* Optionally enable experimental fb_defio mmap support */
> >>  static int fb_bpp = 16;
> >>
> >>  module_param(fb_bpp, int, S_IWUSR | S_IRUSR | S_IWGRP | S_IRGRP);
> >> -module_param(fb_defio, int, S_IWUSR | S_IRUSR | S_IWGRP | S_IRGRP);
> >> -
> >> -struct udl_fbdev {
> >> -    struct drm_fb_helper helper; /* must be first */
> >> -    struct udl_framebuffer ufb;
> >> -    int fb_count;
> >> -};
> >>
> >>  #define DL_ALIGN_UP(x, a) ALIGN(x, a)
> >>  #define DL_ALIGN_DOWN(x, a) ALIGN_DOWN(x, a)
> >> @@ -157,123 +147,6 @@ int udl_handle_damage(struct udl_framebuffer *fb, int x, int y,
> >>      return 0;
> >>  }
> >>
> >> -static int udl_fb_mmap(struct fb_info *info, struct vm_area_struct *vma)
> >> -{
> >> -    unsigned long start = vma->vm_start;
> >> -    unsigned long size = vma->vm_end - vma->vm_start;
> >> -    unsigned long offset;
> >> -    unsigned long page, pos;
> >> -
> >> -    if (vma->vm_pgoff > (~0UL >> PAGE_SHIFT))
> >> -            return -EINVAL;
> >> -
> >> -    offset = vma->vm_pgoff << PAGE_SHIFT;
> >> -
> >> -    if (offset > info->fix.smem_len || size > info->fix.smem_len - offset)
> >> -            return -EINVAL;
> >> -
> >> -    pos = (unsigned long)info->fix.smem_start + offset;
> >> -
> >> -    pr_debug("mmap() framebuffer addr:%lu size:%lu\n",
> >> -              pos, size);
> >> -
> >> -    /* We don't want the framebuffer to be mapped encrypted */
> >> -    vma->vm_page_prot = pgprot_decrypted(vma->vm_page_prot);
> >> -
> >> -    while (size > 0) {
> >> -            page = vmalloc_to_pfn((void *)pos);
> >> -            if (remap_pfn_range(vma, start, page, PAGE_SIZE, PAGE_SHARED))
> >> -                    return -EAGAIN;
> >> -
> >> -            start += PAGE_SIZE;
> >> -            pos += PAGE_SIZE;
> >> -            if (size > PAGE_SIZE)
> >> -                    size -= PAGE_SIZE;
> >> -            else
> >> -                    size = 0;
> >> -    }
> >> -
> >> -    /* VM_IO | VM_DONTEXPAND | VM_DONTDUMP are set by remap_pfn_range() */
> >> -    return 0;
> >> -}
> >> -
> >> -/*
> >> - * It's common for several clients to have framebuffer open simultaneously.
> >> - * e.g. both fbcon and X. Makes things interesting.
> >> - * Assumes caller is holding info->lock (for open and release at least)
> >> - */
> >> -static int udl_fb_open(struct fb_info *info, int user)
> >> -{
> >> -    struct udl_fbdev *ufbdev = info->par;
> >> -    struct drm_device *dev = ufbdev->ufb.base.dev;
> >> -    struct udl_device *udl = to_udl(dev);
> >> -
> >> -    /* If the USB device is gone, we don't accept new opens */
> >> -    if (drm_dev_is_unplugged(&udl->drm))
> >> -            return -ENODEV;
> >> -
> >> -    ufbdev->fb_count++;
> >> -
> >> -#ifdef CONFIG_DRM_FBDEV_EMULATION
> >> -    if (fb_defio && (info->fbdefio == NULL)) {
> >> -            /* enable defio at last moment if not disabled by client */
> >> -
> >> -            struct fb_deferred_io *fbdefio;
> >> -
> >> -            fbdefio = kzalloc(sizeof(struct fb_deferred_io), GFP_KERNEL);
> >> -
> >> -            if (fbdefio) {
> >> -                    fbdefio->delay = DL_DEFIO_WRITE_DELAY;
> >> -                    fbdefio->deferred_io = drm_fb_helper_deferred_io;
> >> -            }
> >> -
> >> -            info->fbdefio = fbdefio;
> >> -            fb_deferred_io_init(info);
> >> -    }
> >> -#endif
> >> -
> >> -    pr_debug("open /dev/fb%d user=%d fb_info=%p count=%d\n",
> >> -              info->node, user, info, ufbdev->fb_count);
> >> -
> >> -    return 0;
> >> -}
> >> -
> >> -
> >> -/*
> >> - * Assumes caller is holding info->lock mutex (for open and release at least)
> >> - */
> >> -static int udl_fb_release(struct fb_info *info, int user)
> >> -{
> >> -    struct udl_fbdev *ufbdev = info->par;
> >> -
> >> -    ufbdev->fb_count--;
> >> -
> >> -#ifdef CONFIG_DRM_FBDEV_EMULATION
> >> -    if ((ufbdev->fb_count == 0) && (info->fbdefio)) {
> >> -            fb_deferred_io_cleanup(info);
> >> -            kfree(info->fbdefio);
> >> -            info->fbdefio = NULL;
> >> -            info->fbops->fb_mmap = udl_fb_mmap;
> >> -    }
> >> -#endif
> >> -
> >> -    pr_debug("released /dev/fb%d user=%d count=%d\n",
> >> -            info->node, user, ufbdev->fb_count);
> >> -
> >> -    return 0;
> >> -}
> >> -
> >> -static struct fb_ops udlfb_ops = {
> >> -    .owner = THIS_MODULE,
> >> -    DRM_FB_HELPER_DEFAULT_OPS,
> >> -    .fb_fillrect = drm_fb_helper_sys_fillrect,
> >> -    .fb_copyarea = drm_fb_helper_sys_copyarea,
> >> -    .fb_imageblit = drm_fb_helper_sys_imageblit,
> >> -    .fb_mmap = udl_fb_mmap,
> >> -    .fb_open = udl_fb_open,
> >> -    .fb_release = udl_fb_release,
> >> -};
> >> -
> >>  static int udl_user_framebuffer_dirty(struct drm_framebuffer *fb,
> >>                                    struct drm_file *file,
> >>                                    unsigned flags, unsigned color,
> >> @@ -346,150 +219,20 @@ udl_framebuffer_init(struct drm_device *dev,
> >>      return ret;
> >>  }
> >>
> >> -
> >> -static int udlfb_create(struct drm_fb_helper *helper,
> >> -                    struct drm_fb_helper_surface_size *sizes)
> >> -{
> >> -    struct udl_fbdev *ufbdev =
> >> -            container_of(helper, struct udl_fbdev, helper);
> >> -    struct drm_device *dev = ufbdev->helper.dev;
> >> -    struct fb_info *info;
> >> -    struct drm_framebuffer *fb;
> >> -    struct drm_mode_fb_cmd2 mode_cmd;
> >> -    struct udl_gem_object *obj;
> >> -    uint32_t size;
> >> -    int ret = 0;
> >> -
> >> -    if (sizes->surface_bpp == 24)
> >> -            sizes->surface_bpp = 32;
> >> -
> >> -    mode_cmd.width = sizes->surface_width;
> >> -    mode_cmd.height = sizes->surface_height;
> >> -    mode_cmd.pitches[0] = mode_cmd.width * ((sizes->surface_bpp + 7) / 8);
> >> -
> >> -    mode_cmd.pixel_format = drm_mode_legacy_fb_format(sizes->surface_bpp,
> >> -                                                      sizes->surface_depth);
> >> -
> >> -    size = mode_cmd.pitches[0] * mode_cmd.height;
> >> -    size = ALIGN(size, PAGE_SIZE);
> >> -
> >> -    obj = udl_gem_alloc_object(dev, size);
> >> -    if (!obj)
> >> -            goto out;
> >> -
> >> -    ret = udl_gem_vmap(obj);
> >> -    if (ret) {
> >> -            DRM_ERROR("failed to vmap fb\n");
> >> -            goto out_gfree;
> >> -    }
> >> -
> >> -    info = drm_fb_helper_alloc_fbi(helper);
> >> -    if (IS_ERR(info)) {
> >> -            ret = PTR_ERR(info);
> >> -            goto out_gfree;
> >> -    }
> >> -
> >> -    ret = udl_framebuffer_init(dev, &ufbdev->ufb, &mode_cmd, obj);
> >> -    if (ret)
> >> -            goto out_gfree;
> >> -
> >> -    fb = &ufbdev->ufb.base;
> >> -
> >> -    ufbdev->helper.fb = fb;
> >> -
> >> -    info->screen_base = ufbdev->ufb.obj->vmapping;
> >> -    info->fix.smem_len = size;
> >> -    info->fix.smem_start = (unsigned long)ufbdev->ufb.obj->vmapping;
> >> -
> >> -    info->fbops = &udlfb_ops;
> >> -    drm_fb_helper_fill_info(info, &ufbdev->helper, sizes);
> >> -
> >> -    DRM_DEBUG_KMS("allocated %dx%d vmal %p\n",
> >> -                  fb->width, fb->height,
> >> -                  ufbdev->ufb.obj->vmapping);
> >> -
> >> -    return ret;
> >> -out_gfree:
> >> -    drm_gem_object_put_unlocked(&ufbdev->ufb.obj->base);
> >> -out:
> >> -    return ret;
> >> -}
> >> -
> >> -static const struct drm_fb_helper_funcs udl_fb_helper_funcs = {
> >> -    .fb_probe = udlfb_create,
> >> -};
> >> -
> >> -static void udl_fbdev_destroy(struct drm_device *dev,
> >> -                          struct udl_fbdev *ufbdev)
> >> -{
> >> -    drm_fb_helper_unregister_fbi(&ufbdev->helper);
> >> -    drm_fb_helper_fini(&ufbdev->helper);
> >> -    if (ufbdev->ufb.obj) {
> >> -            drm_framebuffer_unregister_private(&ufbdev->ufb.base);
> >> -            drm_framebuffer_cleanup(&ufbdev->ufb.base);
> >> -            drm_gem_object_put_unlocked(&ufbdev->ufb.obj->base);
> >> -    }
> >> -}
> >> -
> >>  int udl_fbdev_init(struct drm_device *dev)
> >>  {
> >> -    struct udl_device *udl = to_udl(dev);
> >>      int bpp_sel = fb_bpp;
> >> -    struct udl_fbdev *ufbdev;
> >>      int ret;
> >>
> >> -    ufbdev = kzalloc(sizeof(struct udl_fbdev), GFP_KERNEL);
> >> -    if (!ufbdev)
> >> -            return -ENOMEM;
> >> -
> >> -    udl->fbdev = ufbdev;
> >> -
> >> -    drm_fb_helper_prepare(dev, &ufbdev->helper, &udl_fb_helper_funcs);
> >> -
> >> -    ret = drm_fb_helper_init(dev, &ufbdev->helper, 1);
> >> -    if (ret)
> >> -            goto free;
> >> -
> >> -    ret = drm_fb_helper_single_add_all_connectors(&ufbdev->helper);
> >> +    ret = drm_fbdev_generic_setup(dev, bpp_sel);
> >>      if (ret)
> >> -            goto fini;
> >> -
> >> -    /* disable all the possible outputs/crtcs before entering KMS mode */
> >> -    drm_helper_disable_unused_functions(dev);
> >> -
> >> -    ret = drm_fb_helper_initial_config(&ufbdev->helper, bpp_sel);
> >> -    if (ret)
> >> -            goto fini;
> >> -
> >> +            return ret;
> >>      return 0;
> >> -
> >> -fini:
> >> -    drm_fb_helper_fini(&ufbdev->helper);
> >> -free:
> >> -    kfree(ufbdev);
> >> -    return ret;
> >> -}
> >> -
> >> -void udl_fbdev_cleanup(struct drm_device *dev)
> >> -{
> >> -    struct udl_device *udl = to_udl(dev);
> >> -    if (!udl->fbdev)
> >> -            return;
> >> -
> >> -    udl_fbdev_destroy(dev, udl->fbdev);
> >> -    kfree(udl->fbdev);
> >> -    udl->fbdev = NULL;
> >>  }
> >>
> >>  void udl_fbdev_unplug(struct drm_device *dev)
> >>  {
> >> -    struct udl_device *udl = to_udl(dev);
> >> -    struct udl_fbdev *ufbdev;
> >> -    if (!udl->fbdev)
> >> -            return;
> >> -
> >> -    ufbdev = udl->fbdev;
> >> -    drm_fb_helper_unlink_fbi(&ufbdev->helper);
> >> +    drm_fb_helper_unlink_fbi(dev->fb_helper);
> >
> > Uh I think this here can be all garbage-collected. The generic fbdev
> > already calls drm_fb_helper_unregister_fbi, which calls
> > unregister_framebuffer, which is a strict superset of unlink_framebuffer.
> > The later isn't really enough for hotunplug.
> >
> > So for generic fbdev you really shouldn't need any cleanup nor unplug
> > functions anymore.
> >
> > Also udl is the only caller of drm_fb_helper_unlink_fbi, so we could drop
> > that. Which removes the only external caller of unlink_framebuffer, so we
> > can drop that too. Care to follow up with those 2 patches?
>
> Sure. BTW is there a policy for keeping potentially useful functions? I
> recently converted vboxvideo to generic fbdev and notices that there are
> no more users of drm_fb_helper_fbdev_{setup,teardown}(). But there's
> still a TODO item for converting drivers over to them. Shall they remain
> in the code base?

As Noralf mentioned, maybe better to ditch them, and update the TODO
entry to instead encourage the generic fbdev code. That one is even
less code.

> I have quite a few patches for udl in the making. After converting to
> shmem and generic fbdev, udl_framebuffer can be replaced with the GEM
> framebuffer code. The udl modesetting code is an epicenter of simple
> display pipelines. Converting this over should give atomic mode setting.

Awesome!
-Daniel

>
> Best regards
> Thomas
>
> > Aside from this this patch here looks great!
> > -Daniel
> >
> >>  }
> >>
> >>  struct drm_framebuffer *
> >> diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c
> >> index 4e854e017390..2a6399290f09 100644
> >> --- a/drivers/gpu/drm/udl/udl_main.c
> >> +++ b/drivers/gpu/drm/udl/udl_main.c
> >> @@ -367,6 +367,4 @@ void udl_fini(struct drm_device *dev)
> >>
> >>      if (udl->urbs.count)
> >>              udl_free_urb_list(dev);
> >> -
> >> -    udl_fbdev_cleanup(dev);
> >>  }
> >> diff --git a/drivers/gpu/drm/udl/udl_modeset.c b/drivers/gpu/drm/udl/udl_modeset.c
> >> index bc1ab6060dc6..1517d5e881b8 100644
> >> --- a/drivers/gpu/drm/udl/udl_modeset.c
> >> +++ b/drivers/gpu/drm/udl/udl_modeset.c
> >> @@ -10,6 +10,7 @@
> >>   */
> >>
> >>  #include <drm/drm_crtc_helper.h>
> >> +#include <drm/drm_fb_helper.h>
> >>  #include <drm/drm_modeset_helper_vtables.h>
> >>  #include <drm/drm_vblank.h>
> >>
> >> @@ -422,7 +423,7 @@ static int udl_crtc_init(struct drm_device *dev)
> >>
> >>  static const struct drm_mode_config_funcs udl_mode_funcs = {
> >>      .fb_create = udl_fb_user_fb_create,
> >> -    .output_poll_changed = NULL,
> >> +    .output_poll_changed = drm_fb_helper_output_poll_changed,
> >>  };
> >>
> >>  int udl_modeset_init(struct drm_device *dev)
> >> --
> >> 2.23.0
> >>
> >
>
> --
> 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
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 31+ messages in thread
end of thread, other threads:[~2019-10-25 19:10 UTC | newest]

Thread overview: 31+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-24 14:42 [PATCH 0/5] drm/udl: Convert to generic fbdev emulation Thomas Zimmermann
2019-10-24 14:42 ` [PATCH 1/5] drm/udl: Clear BO vmapping pointer after unmapping BO memory Thomas Zimmermann
2019-10-25  7:39   ` Daniel Vetter
2019-10-24 14:42 ` [PATCH 2/5] drm/udl: Set drm_driver.gem_prime_mmap Thomas Zimmermann
2019-10-24 14:42 ` [PATCH 3/5] drm/udl: Add GEM object functions for free(), vmap(), and vunmap() Thomas Zimmermann
2019-10-25  7:40   ` Daniel Vetter
2019-10-25  7:59     ` Thomas Zimmermann
2019-10-25  7:59       ` Thomas Zimmermann
2019-10-25  9:28       ` Daniel Vetter
2019-10-25  9:28         ` Daniel Vetter
2019-10-25 10:12         ` Thomas Zimmermann
2019-10-25 10:12           ` Thomas Zimmermann
2019-10-25 11:44           ` Noralf Trønnes
2019-10-25 11:44             ` Noralf Trønnes
2019-10-25 11:47             ` Noralf Trønnes
2019-10-25 11:47               ` Noralf Trønnes
2019-10-25 12:20             ` Thomas Zimmermann
2019-10-25 12:20               ` Thomas Zimmermann
2019-10-25 13:32               ` Gerd Hoffmann
2019-10-25 14:53                 ` Thomas Zimmermann
2019-10-25 14:53                   ` Thomas Zimmermann
2019-10-25 13:44               ` Noralf Trønnes
2019-10-25 13:44                 ` Noralf Trønnes
2019-10-24 14:42 ` [PATCH 4/5] drm/udl: Map BO memory pages in unencrypted mode Thomas Zimmermann
2019-10-24 14:42 ` [PATCH 5/5] drm/udl: Replace fbdev code with generic emulation Thomas Zimmermann
2019-10-25  7:47   ` Daniel Vetter
2019-10-25  8:00     ` Daniel Vetter
2019-10-25 11:22       ` Noralf Trønnes
2019-10-25 11:44         ` Thomas Zimmermann
2019-10-25  8:08     ` Thomas Zimmermann
2019-10-25 19:10       ` Daniel Vetter

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.