[PATCH 0/4] drm/udl: Replace fbdev by generic emulation

[PATCH 0/4] drm/udl: Replace fbdev by generic emulation

[Thomas Zimmermann]

The udl driver can use the generic fbdev emulation. After conversion,
a number of cleanups can be applied.

The fbdev conversion is in patch 1. The original fbdev code in udl
mapped the framebuffer's GEM object memory unconditionally, and
unmapped the memory in the object's free() function. The respective
code in free() can now be removed (Patch 2). And as udl was the
only remaining external user of unlink_framebuffer(), that function
now becomes an internal interface of the fbdev code.

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

Thomas Zimmermann (4):
  drm/udl: Replace fbdev code with generic emulation
  drm/udl: Remove udl implementation of GEM's free_object()
  drm/fb-helper: Remove drm_fb_helper_unlink_fbi()
  fbdev: Unexport unlink_framebuffer()

 drivers/gpu/drm/drm_fb_helper.c   |  16 +-
 drivers/gpu/drm/udl/udl_drv.c     |   4 +-
 drivers/gpu/drm/udl/udl_drv.h     |   5 -
 drivers/gpu/drm/udl/udl_fb.c      | 270 +-----------------------------
 drivers/gpu/drm/udl/udl_gem.c     |  18 +-
 drivers/gpu/drm/udl/udl_main.c    |   2 -
 drivers/gpu/drm/udl/udl_modeset.c |   3 +-
 drivers/video/fbdev/core/fbmem.c  |   3 +-
 include/drm/drm_fb_helper.h       |   6 -
 include/linux/fb.h                |   1 -
 10 files changed, 10 insertions(+), 318 deletions(-)

--
2.23.0

[PATCH 1/4] drm/udl: Replace fbdev code with generic emulation

[Thomas Zimmermann]

The udl driver can use the generic fbdev implementation. Convert it.

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

diff --git a/drivers/gpu/drm/udl/udl_drv.c b/drivers/gpu/drm/udl/udl_drv.c
index 563cc5809e56..55c0f9dfee29 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_gem_shmem_helper.h>
 #include <drm/drm_ioctl.h>
@@ -47,6 +48,8 @@ static struct drm_driver driver = {
 	.driver_features = DRIVER_MODESET | DRIVER_GEM,
 	.release = udl_driver_release,
 
+	.lastclose = drm_fb_helper_lastclose,
+
 	/* gem hooks */
 	.gem_create_object = udl_driver_gem_create_object,
 
@@ -119,7 +122,6 @@ static void udl_usb_disconnect(struct usb_interface *interface)
 	struct drm_device *dev = usb_get_intfdata(interface);
 
 	drm_kms_helper_poll_disable(dev);
-	udl_fbdev_unplug(dev);
 	udl_drop_usb(dev);
 	drm_dev_unplug(dev);
 	drm_dev_put(dev);
diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h
index 987d99ae2dfa..5bdc57779f95 100644
--- a/drivers/gpu/drm/udl/udl_drv.h
+++ b/drivers/gpu/drm/udl/udl_drv.h
@@ -47,8 +47,6 @@ struct urb_list {
 	size_t size;
 };
 
-struct udl_fbdev;
-
 struct udl_device {
 	struct drm_device drm;
 	struct device *dev;
@@ -62,7 +60,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 */
@@ -98,8 +95,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,
 		      struct drm_file *file,
diff --git a/drivers/gpu/drm/udl/udl_fb.c b/drivers/gpu/drm/udl/udl_fb.c
index f8153b726343..afe74f892a2b 100644
--- a/drivers/gpu/drm/udl/udl_fb.c
+++ b/drivers/gpu/drm/udl/udl_fb.c
@@ -20,19 +20,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)
@@ -156,123 +146,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,
@@ -347,154 +220,15 @@ 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 drm_gem_shmem_object *shmem;
-	void *vaddr;
-	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);
-
-	shmem = drm_gem_shmem_create(dev, size);
-	if (IS_ERR(shmem)) {
-		ret = PTR_ERR(shmem);
-		goto out;
-	}
-
-	vaddr = drm_gem_shmem_vmap(&shmem->base);
-	if (IS_ERR(vaddr)) {
-		ret = PTR_ERR(vaddr);
-		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, shmem);
-	if (ret)
-		goto out_gfree;
-
-	fb = &ufbdev->ufb.base;
-
-	ufbdev->helper.fb = fb;
-
-	info->screen_base = vaddr;
-	info->fix.smem_len = size;
-	info->fix.smem_start = (unsigned long)vaddr;
-
-	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.shmem->vaddr);
-
-	return ret;
-out_gfree:
-	drm_gem_object_put_unlocked(&ufbdev->ufb.shmem->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.shmem) {
-		drm_framebuffer_unregister_private(&ufbdev->ufb.base);
-		drm_framebuffer_cleanup(&ufbdev->ufb.base);
-		drm_gem_object_put_unlocked(&ufbdev->ufb.shmem->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);
-	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);
+	ret = drm_fbdev_generic_setup(dev, 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);
 }
 
 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

Re: [PATCH 1/4] drm/udl: Replace fbdev code with generic emulation

[Noralf Trønnes]

Den 08.11.2019 13.33, skrev Thomas Zimmermann:
> The udl driver can use the generic fbdev implementation. Convert it.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---

> diff --git a/drivers/gpu/drm/udl/udl_drv.c b/drivers/gpu/drm/udl/udl_drv.c
> index 563cc5809e56..55c0f9dfee29 100644
> --- a/drivers/gpu/drm/udl/udl_drv.c
> +++ b/drivers/gpu/drm/udl/udl_drv.c

> @@ -47,6 +48,8 @@ static struct drm_driver driver = {
>  	.driver_features = DRIVER_MODESET | DRIVER_GEM,
>  	.release = udl_driver_release,
>  
> +	.lastclose = drm_fb_helper_lastclose,
> +

No need to set this, it's already wired up:

drm_lastclose -> drm_client_dev_restore -> drm_fbdev_client_restore ->
drm_fb_helper_lastclose

>  	/* gem hooks */
>  	.gem_create_object = udl_driver_gem_create_object,
>  

> diff --git a/drivers/gpu/drm/udl/udl_fb.c b/drivers/gpu/drm/udl/udl_fb.c
> index f8153b726343..afe74f892a2b 100644
> --- a/drivers/gpu/drm/udl/udl_fb.c
> +++ b/drivers/gpu/drm/udl/udl_fb.c
> @@ -20,19 +20,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);

Maybe fb_bpp can be dropped too?

It's possible to set it on the command line:

video=<xres>x<yres>-<bpp>

I haven't tried it so I can't say for certain that it actually works.

Ref: Documentation/fb/modedb.rst and drm_fb_helper_single_fb_probe()

> -module_param(fb_defio, int, S_IWUSR | S_IRUSR | S_IWGRP | S_IRGRP);
> -

> 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

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

No need to set this, it's already wired up:

drm_kms_helper_hotplug_event -> drm_client_dev_hotplug ->
drm_fbdev_client_hotplug -> drm_fb_helper_hotplug_event

Noralf.

>  };
>  
>  int udl_modeset_init(struct drm_device *dev)
> 

Re: [PATCH 1/4] drm/udl: Replace fbdev code with generic emulation

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 2991 bytes --]

Hi

Am 08.11.19 um 16:37 schrieb Noralf Trønnes:
> 
> 
> Den 08.11.2019 13.33, skrev Thomas Zimmermann:
>> The udl driver can use the generic fbdev implementation. Convert it.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
> 
>> diff --git a/drivers/gpu/drm/udl/udl_drv.c b/drivers/gpu/drm/udl/udl_drv.c
>> index 563cc5809e56..55c0f9dfee29 100644
>> --- a/drivers/gpu/drm/udl/udl_drv.c
>> +++ b/drivers/gpu/drm/udl/udl_drv.c
> 
>> @@ -47,6 +48,8 @@ static struct drm_driver driver = {
>>  	.driver_features = DRIVER_MODESET | DRIVER_GEM,
>>  	.release = udl_driver_release,
>>  
>> +	.lastclose = drm_fb_helper_lastclose,
>> +
> 
> No need to set this, it's already wired up:
> 
> drm_lastclose -> drm_client_dev_restore -> drm_fbdev_client_restore ->
> drm_fb_helper_lastclose
> 
>>  	/* gem hooks */
>>  	.gem_create_object = udl_driver_gem_create_object,
>>  
> 
>> diff --git a/drivers/gpu/drm/udl/udl_fb.c b/drivers/gpu/drm/udl/udl_fb.c
>> index f8153b726343..afe74f892a2b 100644
>> --- a/drivers/gpu/drm/udl/udl_fb.c
>> +++ b/drivers/gpu/drm/udl/udl_fb.c
>> @@ -20,19 +20,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);
> 
> Maybe fb_bpp can be dropped too?

Sure, makes sense.

The driver exposes a preferred color depth of 24 bpp, which we may want
to change to 16 then. The internal framebuffer is only 16 bpp anyway.

Best regards
Thomas

> 
> It's possible to set it on the command line:
> 
> video=<xres>x<yres>-<bpp>
> 
> I haven't tried it so I can't say for certain that it actually works>
> Ref: Documentation/fb/modedb.rst and drm_fb_helper_single_fb_probe()
> 
>> -module_param(fb_defio, int, S_IWUSR | S_IRUSR | S_IWGRP | S_IRGRP);
>> -
> 
>> 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
> 
>> @@ -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,
> 
> No need to set this, it's already wired up:
> 
> drm_kms_helper_hotplug_event -> drm_client_dev_hotplug ->
> drm_fbdev_client_hotplug -> drm_fb_helper_hotplug_event
> 
> Noralf.
> 
>>  };
>>  
>>  int udl_modeset_init(struct drm_device *dev)
>>

-- 
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 #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/4] drm/udl: Replace fbdev code with generic emulation

[Daniel Vetter]

On Tue, Nov 12, 2019 at 12:55 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>
> Hi
>
> Am 08.11.19 um 16:37 schrieb Noralf Trønnes:
> >
> >
> > Den 08.11.2019 13.33, skrev Thomas Zimmermann:
> >> The udl driver can use the generic fbdev implementation. Convert it.
> >>
> >> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> >> ---
> >
> >> diff --git a/drivers/gpu/drm/udl/udl_drv.c b/drivers/gpu/drm/udl/udl_drv.c
> >> index 563cc5809e56..55c0f9dfee29 100644
> >> --- a/drivers/gpu/drm/udl/udl_drv.c
> >> +++ b/drivers/gpu/drm/udl/udl_drv.c
> >
> >> @@ -47,6 +48,8 @@ static struct drm_driver driver = {
> >>      .driver_features = DRIVER_MODESET | DRIVER_GEM,
> >>      .release = udl_driver_release,
> >>
> >> +    .lastclose = drm_fb_helper_lastclose,
> >> +
> >
> > No need to set this, it's already wired up:
> >
> > drm_lastclose -> drm_client_dev_restore -> drm_fbdev_client_restore ->
> > drm_fb_helper_lastclose
> >
> >>      /* gem hooks */
> >>      .gem_create_object = udl_driver_gem_create_object,
> >>
> >
> >> diff --git a/drivers/gpu/drm/udl/udl_fb.c b/drivers/gpu/drm/udl/udl_fb.c
> >> index f8153b726343..afe74f892a2b 100644
> >> --- a/drivers/gpu/drm/udl/udl_fb.c
> >> +++ b/drivers/gpu/drm/udl/udl_fb.c
> >> @@ -20,19 +20,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);
> >
> > Maybe fb_bpp can be dropped too?
>
> Sure, makes sense.
>
> The driver exposes a preferred color depth of 24 bpp, which we may want
> to change to 16 then. The internal framebuffer is only 16 bpp anyway.

Just something that crossed my mind: Should we ensure that the
preferred format of the primary plane (should be the first in the
format array) matches up with the preferred bpp setting? Maybe even
enforce that for drivers with an explicit primary plane (i.e. atomic
drivers). I think tiny drivers get this right already.
-Daniel

>
> Best regards
> Thomas
>
> >
> > It's possible to set it on the command line:
> >
> > video=<xres>x<yres>-<bpp>
> >
> > I haven't tried it so I can't say for certain that it actually works>
> > Ref: Documentation/fb/modedb.rst and drm_fb_helper_single_fb_probe()
> >
> >> -module_param(fb_defio, int, S_IWUSR | S_IRUSR | S_IWGRP | S_IRGRP);
> >> -
> >
> >> 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
> >
> >> @@ -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,
> >
> > No need to set this, it's already wired up:
> >
> > drm_kms_helper_hotplug_event -> drm_client_dev_hotplug ->
> > drm_fbdev_client_hotplug -> drm_fb_helper_hotplug_event
> >
> > Noralf.
> >
> >>  };
> >>
> >>  int udl_modeset_init(struct drm_device *dev)
> >>
>
> --
> 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

Re: [PATCH 1/4] drm/udl: Replace fbdev code with generic emulation

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 4536 bytes --]

Hi

Am 12.11.19 um 14:40 schrieb Daniel Vetter:
> On Tue, Nov 12, 2019 at 12:55 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>>
>> Hi
>>
>> Am 08.11.19 um 16:37 schrieb Noralf Trønnes:
>>>
>>>
>>> Den 08.11.2019 13.33, skrev Thomas Zimmermann:
>>>> The udl driver can use the generic fbdev implementation. Convert it.
>>>>
>>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>>>> ---
>>>
>>>> diff --git a/drivers/gpu/drm/udl/udl_drv.c b/drivers/gpu/drm/udl/udl_drv.c
>>>> index 563cc5809e56..55c0f9dfee29 100644
>>>> --- a/drivers/gpu/drm/udl/udl_drv.c
>>>> +++ b/drivers/gpu/drm/udl/udl_drv.c
>>>
>>>> @@ -47,6 +48,8 @@ static struct drm_driver driver = {
>>>>      .driver_features = DRIVER_MODESET | DRIVER_GEM,
>>>>      .release = udl_driver_release,
>>>>
>>>> +    .lastclose = drm_fb_helper_lastclose,
>>>> +
>>>
>>> No need to set this, it's already wired up:
>>>
>>> drm_lastclose -> drm_client_dev_restore -> drm_fbdev_client_restore ->
>>> drm_fb_helper_lastclose
>>>
>>>>      /* gem hooks */
>>>>      .gem_create_object = udl_driver_gem_create_object,
>>>>
>>>
>>>> diff --git a/drivers/gpu/drm/udl/udl_fb.c b/drivers/gpu/drm/udl/udl_fb.c
>>>> index f8153b726343..afe74f892a2b 100644
>>>> --- a/drivers/gpu/drm/udl/udl_fb.c
>>>> +++ b/drivers/gpu/drm/udl/udl_fb.c
>>>> @@ -20,19 +20,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);
>>>
>>> Maybe fb_bpp can be dropped too?
>>
>> Sure, makes sense.
>>
>> The driver exposes a preferred color depth of 24 bpp, which we may want
>> to change to 16 then. The internal framebuffer is only 16 bpp anyway.
> 
> Just something that crossed my mind: Should we ensure that the
> preferred format of the primary plane (should be the first in the
> format array) matches up with the preferred bpp setting? Maybe even
> enforce that for drivers with an explicit primary plane (i.e. atomic
> drivers). I think tiny drivers get this right already.

IMHO that makes if the userspace can handle it. The preferred bpp could
also be retrieved from the formats array automatically. What about HW
with multiple CRTCs with different format defaults (sounds weird, I know)?

WRT udl: For v3 of this patchset I've set the preferred color depth to
32 bpp; although the internal FB is always at 16 bpp. Because when I
tested with a dual-screen setup (radeon + udl) X11 didn't support the 16
bpp output on the second screen (the one driven by udl). Only setting
both screen to 32 bpp worked out of the box. And the preferred 24 bpp
are not even supported by udl.

Best regards
Thomas

> -Daniel
> 
>>
>> Best regards
>> Thomas
>>
>>>
>>> It's possible to set it on the command line:
>>>
>>> video=<xres>x<yres>-<bpp>
>>>
>>> I haven't tried it so I can't say for certain that it actually works>
>>> Ref: Documentation/fb/modedb.rst and drm_fb_helper_single_fb_probe()
>>>
>>>> -module_param(fb_defio, int, S_IWUSR | S_IRUSR | S_IWGRP | S_IRGRP);
>>>> -
>>>
>>>> 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
>>>
>>>> @@ -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,
>>>
>>> No need to set this, it's already wired up:
>>>
>>> drm_kms_helper_hotplug_event -> drm_client_dev_hotplug ->
>>> drm_fbdev_client_hotplug -> drm_fb_helper_hotplug_event
>>>
>>> Noralf.
>>>
>>>>  };
>>>>
>>>>  int udl_modeset_init(struct drm_device *dev)
>>>>
>>
>> --
>> Thomas Zimmermann
>> Graphics Driver Developer
>> SUSE Software Solutions Germany GmbH
>> Maxfeldstr. 5, 90409 Nürnberg, Germany
>> (HRB 36809, AG Nürnberg)
>> Geschäftsführer: Felix Imendörffer
>>
> 
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer


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

Re: [PATCH 1/4] drm/udl: Replace fbdev code with generic emulation

[Daniel Vetter]

On Tue, Nov 12, 2019 at 3:03 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>
> Hi
>
> Am 12.11.19 um 14:40 schrieb Daniel Vetter:
> > On Tue, Nov 12, 2019 at 12:55 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
> >>
> >> Hi
> >>
> >> Am 08.11.19 um 16:37 schrieb Noralf Trønnes:
> >>>
> >>>
> >>> Den 08.11.2019 13.33, skrev Thomas Zimmermann:
> >>>> The udl driver can use the generic fbdev implementation. Convert it.
> >>>>
> >>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> >>>> ---
> >>>
> >>>> diff --git a/drivers/gpu/drm/udl/udl_drv.c b/drivers/gpu/drm/udl/udl_drv.c
> >>>> index 563cc5809e56..55c0f9dfee29 100644
> >>>> --- a/drivers/gpu/drm/udl/udl_drv.c
> >>>> +++ b/drivers/gpu/drm/udl/udl_drv.c
> >>>
> >>>> @@ -47,6 +48,8 @@ static struct drm_driver driver = {
> >>>>      .driver_features = DRIVER_MODESET | DRIVER_GEM,
> >>>>      .release = udl_driver_release,
> >>>>
> >>>> +    .lastclose = drm_fb_helper_lastclose,
> >>>> +
> >>>
> >>> No need to set this, it's already wired up:
> >>>
> >>> drm_lastclose -> drm_client_dev_restore -> drm_fbdev_client_restore ->
> >>> drm_fb_helper_lastclose
> >>>
> >>>>      /* gem hooks */
> >>>>      .gem_create_object = udl_driver_gem_create_object,
> >>>>
> >>>
> >>>> diff --git a/drivers/gpu/drm/udl/udl_fb.c b/drivers/gpu/drm/udl/udl_fb.c
> >>>> index f8153b726343..afe74f892a2b 100644
> >>>> --- a/drivers/gpu/drm/udl/udl_fb.c
> >>>> +++ b/drivers/gpu/drm/udl/udl_fb.c
> >>>> @@ -20,19 +20,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);
> >>>
> >>> Maybe fb_bpp can be dropped too?
> >>
> >> Sure, makes sense.
> >>
> >> The driver exposes a preferred color depth of 24 bpp, which we may want
> >> to change to 16 then. The internal framebuffer is only 16 bpp anyway.
> >
> > Just something that crossed my mind: Should we ensure that the
> > preferred format of the primary plane (should be the first in the
> > format array) matches up with the preferred bpp setting? Maybe even
> > enforce that for drivers with an explicit primary plane (i.e. atomic
> > drivers). I think tiny drivers get this right already.
>
> IMHO that makes if the userspace can handle it. The preferred bpp could
> also be retrieved from the formats array automatically. What about HW
> with multiple CRTCs with different format defaults (sounds weird, I know)?

Ime I haven't seen such a case yet. What I have seen is that the most
preferred format might be some fancy compressed format, which not all
formats support. But which you can't render into without mesa anyway,
so really doesn't matter for preferred bpp.

> WRT udl: For v3 of this patchset I've set the preferred color depth to
> 32 bpp; although the internal FB is always at 16 bpp. Because when I
> tested with a dual-screen setup (radeon + udl) X11 didn't support the 16
> bpp output on the second screen (the one driven by udl). Only setting
> both screen to 32 bpp worked out of the box. And the preferred 24 bpp
> are not even supported by udl.

Uh, if we can only set preferred bpp to make X happy, and X can only
support one preferred bpp, then everyone needs to set 32bit. Which
defeats the point (and we'd need to hardcode it to 32bpp). Is this
really the case?
-Daniel

>
> Best regards
> Thomas
>
> > -Daniel
> >
> >>
> >> Best regards
> >> Thomas
> >>
> >>>
> >>> It's possible to set it on the command line:
> >>>
> >>> video=<xres>x<yres>-<bpp>
> >>>
> >>> I haven't tried it so I can't say for certain that it actually works>
> >>> Ref: Documentation/fb/modedb.rst and drm_fb_helper_single_fb_probe()
> >>>
> >>>> -module_param(fb_defio, int, S_IWUSR | S_IRUSR | S_IWGRP | S_IRGRP);
> >>>> -
> >>>
> >>>> 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
> >>>
> >>>> @@ -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,
> >>>
> >>> No need to set this, it's already wired up:
> >>>
> >>> drm_kms_helper_hotplug_event -> drm_client_dev_hotplug ->
> >>> drm_fbdev_client_hotplug -> drm_fb_helper_hotplug_event
> >>>
> >>> Noralf.
> >>>
> >>>>  };
> >>>>
> >>>>  int udl_modeset_init(struct drm_device *dev)
> >>>>
> >>
> >> --
> >> Thomas Zimmermann
> >> Graphics Driver Developer
> >> SUSE Software Solutions Germany GmbH
> >> Maxfeldstr. 5, 90409 Nürnberg, Germany
> >> (HRB 36809, AG Nürnberg)
> >> Geschäftsführer: Felix Imendörffer
> >>
> >
> >
>
> --
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Software Solutions Germany GmbH
> Maxfeldstr. 5, 90409 Nürnberg, Germany
> (HRB 36809, AG Nürnberg)
> Geschäftsführer: Felix Imendörffer
>


-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 1/4] drm/udl: Replace fbdev code with generic emulation

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 5787 bytes --]

Hi

Am 12.11.19 um 15:31 schrieb Daniel Vetter:
> On Tue, Nov 12, 2019 at 3:03 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>>
>> Hi
>>
>> Am 12.11.19 um 14:40 schrieb Daniel Vetter:
>>> On Tue, Nov 12, 2019 at 12:55 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>>>>
>>>> Hi
>>>>
>>>> Am 08.11.19 um 16:37 schrieb Noralf Trønnes:
>>>>>
>>>>>
>>>>> Den 08.11.2019 13.33, skrev Thomas Zimmermann:
>>>>>> The udl driver can use the generic fbdev implementation. Convert it.
>>>>>>
>>>>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>>>>>> ---
>>>>>
>>>>>> diff --git a/drivers/gpu/drm/udl/udl_drv.c b/drivers/gpu/drm/udl/udl_drv.c
>>>>>> index 563cc5809e56..55c0f9dfee29 100644
>>>>>> --- a/drivers/gpu/drm/udl/udl_drv.c
>>>>>> +++ b/drivers/gpu/drm/udl/udl_drv.c
>>>>>
>>>>>> @@ -47,6 +48,8 @@ static struct drm_driver driver = {
>>>>>>      .driver_features = DRIVER_MODESET | DRIVER_GEM,
>>>>>>      .release = udl_driver_release,
>>>>>>
>>>>>> +    .lastclose = drm_fb_helper_lastclose,
>>>>>> +
>>>>>
>>>>> No need to set this, it's already wired up:
>>>>>
>>>>> drm_lastclose -> drm_client_dev_restore -> drm_fbdev_client_restore ->
>>>>> drm_fb_helper_lastclose
>>>>>
>>>>>>      /* gem hooks */
>>>>>>      .gem_create_object = udl_driver_gem_create_object,
>>>>>>
>>>>>
>>>>>> diff --git a/drivers/gpu/drm/udl/udl_fb.c b/drivers/gpu/drm/udl/udl_fb.c
>>>>>> index f8153b726343..afe74f892a2b 100644
>>>>>> --- a/drivers/gpu/drm/udl/udl_fb.c
>>>>>> +++ b/drivers/gpu/drm/udl/udl_fb.c
>>>>>> @@ -20,19 +20,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);
>>>>>
>>>>> Maybe fb_bpp can be dropped too?
>>>>
>>>> Sure, makes sense.
>>>>
>>>> The driver exposes a preferred color depth of 24 bpp, which we may want
>>>> to change to 16 then. The internal framebuffer is only 16 bpp anyway.
>>>
>>> Just something that crossed my mind: Should we ensure that the
>>> preferred format of the primary plane (should be the first in the
>>> format array) matches up with the preferred bpp setting? Maybe even
>>> enforce that for drivers with an explicit primary plane (i.e. atomic
>>> drivers). I think tiny drivers get this right already.
>>
>> IMHO that makes if the userspace can handle it. The preferred bpp could
>> also be retrieved from the formats array automatically. What about HW
>> with multiple CRTCs with different format defaults (sounds weird, I know)?
> 
> Ime I haven't seen such a case yet. What I have seen is that the most
> preferred format might be some fancy compressed format, which not all
> formats support. But which you can't render into without mesa anyway,
> so really doesn't matter for preferred bpp.
> 
>> WRT udl: For v3 of this patchset I've set the preferred color depth to
>> 32 bpp; although the internal FB is always at 16 bpp. Because when I
>> tested with a dual-screen setup (radeon + udl) X11 didn't support the 16
>> bpp output on the second screen (the one driven by udl). Only setting
>> both screen to 32 bpp worked out of the box. And the preferred 24 bpp
>> are not even supported by udl.
> 
> Uh, if we can only set preferred bpp to make X happy, and X can only
> support one preferred bpp, then everyone needs to set 32bit. Which
> defeats the point (and we'd need to hardcode it to 32bpp). Is this
> really the case?

I guess it would have worked if both screens preferred 16 bpp.

Best regards
Thomas

> -Daniel
> 
>>
>> Best regards
>> Thomas
>>
>>> -Daniel
>>>
>>>>
>>>> Best regards
>>>> Thomas
>>>>
>>>>>
>>>>> It's possible to set it on the command line:
>>>>>
>>>>> video=<xres>x<yres>-<bpp>
>>>>>
>>>>> I haven't tried it so I can't say for certain that it actually works>
>>>>> Ref: Documentation/fb/modedb.rst and drm_fb_helper_single_fb_probe()
>>>>>
>>>>>> -module_param(fb_defio, int, S_IWUSR | S_IRUSR | S_IWGRP | S_IRGRP);
>>>>>> -
>>>>>
>>>>>> 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
>>>>>
>>>>>> @@ -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,
>>>>>
>>>>> No need to set this, it's already wired up:
>>>>>
>>>>> drm_kms_helper_hotplug_event -> drm_client_dev_hotplug ->
>>>>> drm_fbdev_client_hotplug -> drm_fb_helper_hotplug_event
>>>>>
>>>>> Noralf.
>>>>>
>>>>>>  };
>>>>>>
>>>>>>  int udl_modeset_init(struct drm_device *dev)
>>>>>>
>>>>
>>>> --
>>>> Thomas Zimmermann
>>>> Graphics Driver Developer
>>>> SUSE Software Solutions Germany GmbH
>>>> Maxfeldstr. 5, 90409 Nürnberg, Germany
>>>> (HRB 36809, AG Nürnberg)
>>>> Geschäftsführer: Felix Imendörffer
>>>>
>>>
>>>
>>
>> --
>> Thomas Zimmermann
>> Graphics Driver Developer
>> SUSE Software Solutions Germany GmbH
>> Maxfeldstr. 5, 90409 Nürnberg, Germany
>> (HRB 36809, AG Nürnberg)
>> Geschäftsführer: Felix Imendörffer
>>
> 
> 

-- 
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 #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/4] drm/udl: Replace fbdev code with generic emulation

[Daniel Vetter]

On Tue, Nov 12, 2019 at 3:51 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>
> Hi
>
> Am 12.11.19 um 15:31 schrieb Daniel Vetter:
> > On Tue, Nov 12, 2019 at 3:03 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
> >>
> >> Hi
> >>
> >> Am 12.11.19 um 14:40 schrieb Daniel Vetter:
> >>> On Tue, Nov 12, 2019 at 12:55 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
> >>>>
> >>>> Hi
> >>>>
> >>>> Am 08.11.19 um 16:37 schrieb Noralf Trønnes:
> >>>>>
> >>>>>
> >>>>> Den 08.11.2019 13.33, skrev Thomas Zimmermann:
> >>>>>> The udl driver can use the generic fbdev implementation. Convert it.
> >>>>>>
> >>>>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> >>>>>> ---
> >>>>>
> >>>>>> diff --git a/drivers/gpu/drm/udl/udl_drv.c b/drivers/gpu/drm/udl/udl_drv.c
> >>>>>> index 563cc5809e56..55c0f9dfee29 100644
> >>>>>> --- a/drivers/gpu/drm/udl/udl_drv.c
> >>>>>> +++ b/drivers/gpu/drm/udl/udl_drv.c
> >>>>>
> >>>>>> @@ -47,6 +48,8 @@ static struct drm_driver driver = {
> >>>>>>      .driver_features = DRIVER_MODESET | DRIVER_GEM,
> >>>>>>      .release = udl_driver_release,
> >>>>>>
> >>>>>> +    .lastclose = drm_fb_helper_lastclose,
> >>>>>> +
> >>>>>
> >>>>> No need to set this, it's already wired up:
> >>>>>
> >>>>> drm_lastclose -> drm_client_dev_restore -> drm_fbdev_client_restore ->
> >>>>> drm_fb_helper_lastclose
> >>>>>
> >>>>>>      /* gem hooks */
> >>>>>>      .gem_create_object = udl_driver_gem_create_object,
> >>>>>>
> >>>>>
> >>>>>> diff --git a/drivers/gpu/drm/udl/udl_fb.c b/drivers/gpu/drm/udl/udl_fb.c
> >>>>>> index f8153b726343..afe74f892a2b 100644
> >>>>>> --- a/drivers/gpu/drm/udl/udl_fb.c
> >>>>>> +++ b/drivers/gpu/drm/udl/udl_fb.c
> >>>>>> @@ -20,19 +20,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);
> >>>>>
> >>>>> Maybe fb_bpp can be dropped too?
> >>>>
> >>>> Sure, makes sense.
> >>>>
> >>>> The driver exposes a preferred color depth of 24 bpp, which we may want
> >>>> to change to 16 then. The internal framebuffer is only 16 bpp anyway.
> >>>
> >>> Just something that crossed my mind: Should we ensure that the
> >>> preferred format of the primary plane (should be the first in the
> >>> format array) matches up with the preferred bpp setting? Maybe even
> >>> enforce that for drivers with an explicit primary plane (i.e. atomic
> >>> drivers). I think tiny drivers get this right already.
> >>
> >> IMHO that makes if the userspace can handle it. The preferred bpp could
> >> also be retrieved from the formats array automatically. What about HW
> >> with multiple CRTCs with different format defaults (sounds weird, I know)?
> >
> > Ime I haven't seen such a case yet. What I have seen is that the most
> > preferred format might be some fancy compressed format, which not all
> > formats support. But which you can't render into without mesa anyway,
> > so really doesn't matter for preferred bpp.
> >
> >> WRT udl: For v3 of this patchset I've set the preferred color depth to
> >> 32 bpp; although the internal FB is always at 16 bpp. Because when I
> >> tested with a dual-screen setup (radeon + udl) X11 didn't support the 16
> >> bpp output on the second screen (the one driven by udl). Only setting
> >> both screen to 32 bpp worked out of the box. And the preferred 24 bpp
> >> are not even supported by udl.
> >
> > Uh, if we can only set preferred bpp to make X happy, and X can only
> > support one preferred bpp, then everyone needs to set 32bit. Which
> > defeats the point (and we'd need to hardcode it to 32bpp). Is this
> > really the case?
>
> I guess it would have worked if both screens preferred 16 bpp.

Just noticed that current depth is 24 bpp ... does that work with
current X? If not I think we should actually set it to 16 bpp, and fix
up X. Not as in "make it handle multi-bpp", that's too hard, but make
it pick a common format that works for all drivers (which usually
means go with 32bpp). Since if we just go with 32bpp to paper over X,
then the preferred bpp uapi becomes meaningless.
-Daniel

>
> Best regards
> Thomas
>
> > -Daniel
> >
> >>
> >> Best regards
> >> Thomas
> >>
> >>> -Daniel
> >>>
> >>>>
> >>>> Best regards
> >>>> Thomas
> >>>>
> >>>>>
> >>>>> It's possible to set it on the command line:
> >>>>>
> >>>>> video=<xres>x<yres>-<bpp>
> >>>>>
> >>>>> I haven't tried it so I can't say for certain that it actually works>
> >>>>> Ref: Documentation/fb/modedb.rst and drm_fb_helper_single_fb_probe()
> >>>>>
> >>>>>> -module_param(fb_defio, int, S_IWUSR | S_IRUSR | S_IWGRP | S_IRGRP);
> >>>>>> -
> >>>>>
> >>>>>> 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
> >>>>>
> >>>>>> @@ -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,
> >>>>>
> >>>>> No need to set this, it's already wired up:
> >>>>>
> >>>>> drm_kms_helper_hotplug_event -> drm_client_dev_hotplug ->
> >>>>> drm_fbdev_client_hotplug -> drm_fb_helper_hotplug_event
> >>>>>
> >>>>> Noralf.
> >>>>>
> >>>>>>  };
> >>>>>>
> >>>>>>  int udl_modeset_init(struct drm_device *dev)
> >>>>>>
> >>>>
> >>>> --
> >>>> Thomas Zimmermann
> >>>> Graphics Driver Developer
> >>>> SUSE Software Solutions Germany GmbH
> >>>> Maxfeldstr. 5, 90409 Nürnberg, Germany
> >>>> (HRB 36809, AG Nürnberg)
> >>>> Geschäftsführer: Felix Imendörffer
> >>>>
> >>>
> >>>
> >>
> >> --
> >> Thomas Zimmermann
> >> Graphics Driver Developer
> >> SUSE Software Solutions Germany GmbH
> >> Maxfeldstr. 5, 90409 Nürnberg, Germany
> >> (HRB 36809, AG Nürnberg)
> >> Geschäftsführer: Felix Imendörffer
> >>
> >
> >
>
> --
> 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

Re: [PATCH 1/4] drm/udl: Replace fbdev code with generic emulation

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 7387 bytes --]

Hi

Am 12.11.19 um 16:13 schrieb Daniel Vetter:
> On Tue, Nov 12, 2019 at 3:51 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>>
>> Hi
>>
>> Am 12.11.19 um 15:31 schrieb Daniel Vetter:
>>> On Tue, Nov 12, 2019 at 3:03 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>>>>
>>>> Hi
>>>>
>>>> Am 12.11.19 um 14:40 schrieb Daniel Vetter:
>>>>> On Tue, Nov 12, 2019 at 12:55 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>>>>>>
>>>>>> Hi
>>>>>>
>>>>>> Am 08.11.19 um 16:37 schrieb Noralf Trønnes:
>>>>>>>
>>>>>>>
>>>>>>> Den 08.11.2019 13.33, skrev Thomas Zimmermann:
>>>>>>>> The udl driver can use the generic fbdev implementation. Convert it.
>>>>>>>>
>>>>>>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>>>>>>>> ---
>>>>>>>
>>>>>>>> diff --git a/drivers/gpu/drm/udl/udl_drv.c b/drivers/gpu/drm/udl/udl_drv.c
>>>>>>>> index 563cc5809e56..55c0f9dfee29 100644
>>>>>>>> --- a/drivers/gpu/drm/udl/udl_drv.c
>>>>>>>> +++ b/drivers/gpu/drm/udl/udl_drv.c
>>>>>>>
>>>>>>>> @@ -47,6 +48,8 @@ static struct drm_driver driver = {
>>>>>>>>      .driver_features = DRIVER_MODESET | DRIVER_GEM,
>>>>>>>>      .release = udl_driver_release,
>>>>>>>>
>>>>>>>> +    .lastclose = drm_fb_helper_lastclose,
>>>>>>>> +
>>>>>>>
>>>>>>> No need to set this, it's already wired up:
>>>>>>>
>>>>>>> drm_lastclose -> drm_client_dev_restore -> drm_fbdev_client_restore ->
>>>>>>> drm_fb_helper_lastclose
>>>>>>>
>>>>>>>>      /* gem hooks */
>>>>>>>>      .gem_create_object = udl_driver_gem_create_object,
>>>>>>>>
>>>>>>>
>>>>>>>> diff --git a/drivers/gpu/drm/udl/udl_fb.c b/drivers/gpu/drm/udl/udl_fb.c
>>>>>>>> index f8153b726343..afe74f892a2b 100644
>>>>>>>> --- a/drivers/gpu/drm/udl/udl_fb.c
>>>>>>>> +++ b/drivers/gpu/drm/udl/udl_fb.c
>>>>>>>> @@ -20,19 +20,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);
>>>>>>>
>>>>>>> Maybe fb_bpp can be dropped too?
>>>>>>
>>>>>> Sure, makes sense.
>>>>>>
>>>>>> The driver exposes a preferred color depth of 24 bpp, which we may want
>>>>>> to change to 16 then. The internal framebuffer is only 16 bpp anyway.
>>>>>
>>>>> Just something that crossed my mind: Should we ensure that the
>>>>> preferred format of the primary plane (should be the first in the
>>>>> format array) matches up with the preferred bpp setting? Maybe even
>>>>> enforce that for drivers with an explicit primary plane (i.e. atomic
>>>>> drivers). I think tiny drivers get this right already.
>>>>
>>>> IMHO that makes if the userspace can handle it. The preferred bpp could
>>>> also be retrieved from the formats array automatically. What about HW
>>>> with multiple CRTCs with different format defaults (sounds weird, I know)?
>>>
>>> Ime I haven't seen such a case yet. What I have seen is that the most
>>> preferred format might be some fancy compressed format, which not all
>>> formats support. But which you can't render into without mesa anyway,
>>> so really doesn't matter for preferred bpp.
>>>
>>>> WRT udl: For v3 of this patchset I've set the preferred color depth to
>>>> 32 bpp; although the internal FB is always at 16 bpp. Because when I
>>>> tested with a dual-screen setup (radeon + udl) X11 didn't support the 16
>>>> bpp output on the second screen (the one driven by udl). Only setting
>>>> both screen to 32 bpp worked out of the box. And the preferred 24 bpp
>>>> are not even supported by udl.
>>>
>>> Uh, if we can only set preferred bpp to make X happy, and X can only
>>> support one preferred bpp, then everyone needs to set 32bit. Which
>>> defeats the point (and we'd need to hardcode it to 32bpp). Is this
>>> really the case?
>>
>> I guess it would have worked if both screens preferred 16 bpp.
> 
> Just noticed that current depth is 24 bpp ... does that work with
> current X? If not I think we should actually set it to 16 bpp, and fix
> up X. Not as in "make it handle multi-bpp", that's too hard, but make
> it pick a common format that works for all drivers (which usually
> means go with 32bpp). Since if we just go with 32bpp to paper over X,
> then the preferred bpp uapi becomes meaningless.

The good news is that it apparently works with planes correctly configured.

I tested with udl being converted to simple pipe and universal planes.
It exposes RGB565, XRGB8888 and preferred_depth=16. X did the correct
thing and choose a format the works with radeon and udl.

For the next iteration of the fbdev conversion, I'll just keep the
current values. And once converted to universal planes, we can set the
optimal values instead.

Best regards
Thomas

> -Daniel
> 
>>
>> Best regards
>> Thomas
>>
>>> -Daniel
>>>
>>>>
>>>> Best regards
>>>> Thomas
>>>>
>>>>> -Daniel
>>>>>
>>>>>>
>>>>>> Best regards
>>>>>> Thomas
>>>>>>
>>>>>>>
>>>>>>> It's possible to set it on the command line:
>>>>>>>
>>>>>>> video=<xres>x<yres>-<bpp>
>>>>>>>
>>>>>>> I haven't tried it so I can't say for certain that it actually works>
>>>>>>> Ref: Documentation/fb/modedb.rst and drm_fb_helper_single_fb_probe()
>>>>>>>
>>>>>>>> -module_param(fb_defio, int, S_IWUSR | S_IRUSR | S_IWGRP | S_IRGRP);
>>>>>>>> -
>>>>>>>
>>>>>>>> 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
>>>>>>>
>>>>>>>> @@ -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,
>>>>>>>
>>>>>>> No need to set this, it's already wired up:
>>>>>>>
>>>>>>> drm_kms_helper_hotplug_event -> drm_client_dev_hotplug ->
>>>>>>> drm_fbdev_client_hotplug -> drm_fb_helper_hotplug_event
>>>>>>>
>>>>>>> Noralf.
>>>>>>>
>>>>>>>>  };
>>>>>>>>
>>>>>>>>  int udl_modeset_init(struct drm_device *dev)
>>>>>>>>
>>>>>>
>>>>>> --
>>>>>> Thomas Zimmermann
>>>>>> Graphics Driver Developer
>>>>>> SUSE Software Solutions Germany GmbH
>>>>>> Maxfeldstr. 5, 90409 Nürnberg, Germany
>>>>>> (HRB 36809, AG Nürnberg)
>>>>>> Geschäftsführer: Felix Imendörffer
>>>>>>
>>>>>
>>>>>
>>>>
>>>> --
>>>> Thomas Zimmermann
>>>> Graphics Driver Developer
>>>> SUSE Software Solutions Germany GmbH
>>>> Maxfeldstr. 5, 90409 Nürnberg, Germany
>>>> (HRB 36809, AG Nürnberg)
>>>> Geschäftsführer: Felix Imendörffer
>>>>
>>>
>>>
>>
>> --
>> Thomas Zimmermann
>> Graphics Driver Developer
>> SUSE Software Solutions Germany GmbH
>> Maxfeldstr. 5, 90409 Nürnberg, Germany
>> (HRB 36809, AG Nürnberg)
>> Geschäftsführer: Felix Imendörffer
>>
> 
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer


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

Re: [PATCH 1/4] drm/udl: Replace fbdev code with generic emulation

[Daniel Vetter]

On Wed, Nov 13, 2019 at 10:51:51AM +0100, Thomas Zimmermann wrote:
> Hi
> 
> Am 12.11.19 um 16:13 schrieb Daniel Vetter:
> > On Tue, Nov 12, 2019 at 3:51 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
> >>
> >> Hi
> >>
> >> Am 12.11.19 um 15:31 schrieb Daniel Vetter:
> >>> On Tue, Nov 12, 2019 at 3:03 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
> >>>>
> >>>> Hi
> >>>>
> >>>> Am 12.11.19 um 14:40 schrieb Daniel Vetter:
> >>>>> On Tue, Nov 12, 2019 at 12:55 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
> >>>>>>
> >>>>>> Hi
> >>>>>>
> >>>>>> Am 08.11.19 um 16:37 schrieb Noralf Trønnes:
> >>>>>>>
> >>>>>>>
> >>>>>>> Den 08.11.2019 13.33, skrev Thomas Zimmermann:
> >>>>>>>> The udl driver can use the generic fbdev implementation. Convert it.
> >>>>>>>>
> >>>>>>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> >>>>>>>> ---
> >>>>>>>
> >>>>>>>> diff --git a/drivers/gpu/drm/udl/udl_drv.c b/drivers/gpu/drm/udl/udl_drv.c
> >>>>>>>> index 563cc5809e56..55c0f9dfee29 100644
> >>>>>>>> --- a/drivers/gpu/drm/udl/udl_drv.c
> >>>>>>>> +++ b/drivers/gpu/drm/udl/udl_drv.c
> >>>>>>>
> >>>>>>>> @@ -47,6 +48,8 @@ static struct drm_driver driver = {
> >>>>>>>>      .driver_features = DRIVER_MODESET | DRIVER_GEM,
> >>>>>>>>      .release = udl_driver_release,
> >>>>>>>>
> >>>>>>>> +    .lastclose = drm_fb_helper_lastclose,
> >>>>>>>> +
> >>>>>>>
> >>>>>>> No need to set this, it's already wired up:
> >>>>>>>
> >>>>>>> drm_lastclose -> drm_client_dev_restore -> drm_fbdev_client_restore ->
> >>>>>>> drm_fb_helper_lastclose
> >>>>>>>
> >>>>>>>>      /* gem hooks */
> >>>>>>>>      .gem_create_object = udl_driver_gem_create_object,
> >>>>>>>>
> >>>>>>>
> >>>>>>>> diff --git a/drivers/gpu/drm/udl/udl_fb.c b/drivers/gpu/drm/udl/udl_fb.c
> >>>>>>>> index f8153b726343..afe74f892a2b 100644
> >>>>>>>> --- a/drivers/gpu/drm/udl/udl_fb.c
> >>>>>>>> +++ b/drivers/gpu/drm/udl/udl_fb.c
> >>>>>>>> @@ -20,19 +20,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);
> >>>>>>>
> >>>>>>> Maybe fb_bpp can be dropped too?
> >>>>>>
> >>>>>> Sure, makes sense.
> >>>>>>
> >>>>>> The driver exposes a preferred color depth of 24 bpp, which we may want
> >>>>>> to change to 16 then. The internal framebuffer is only 16 bpp anyway.
> >>>>>
> >>>>> Just something that crossed my mind: Should we ensure that the
> >>>>> preferred format of the primary plane (should be the first in the
> >>>>> format array) matches up with the preferred bpp setting? Maybe even
> >>>>> enforce that for drivers with an explicit primary plane (i.e. atomic
> >>>>> drivers). I think tiny drivers get this right already.
> >>>>
> >>>> IMHO that makes if the userspace can handle it. The preferred bpp could
> >>>> also be retrieved from the formats array automatically. What about HW
> >>>> with multiple CRTCs with different format defaults (sounds weird, I know)?
> >>>
> >>> Ime I haven't seen such a case yet. What I have seen is that the most
> >>> preferred format might be some fancy compressed format, which not all
> >>> formats support. But which you can't render into without mesa anyway,
> >>> so really doesn't matter for preferred bpp.
> >>>
> >>>> WRT udl: For v3 of this patchset I've set the preferred color depth to
> >>>> 32 bpp; although the internal FB is always at 16 bpp. Because when I
> >>>> tested with a dual-screen setup (radeon + udl) X11 didn't support the 16
> >>>> bpp output on the second screen (the one driven by udl). Only setting
> >>>> both screen to 32 bpp worked out of the box. And the preferred 24 bpp
> >>>> are not even supported by udl.
> >>>
> >>> Uh, if we can only set preferred bpp to make X happy, and X can only
> >>> support one preferred bpp, then everyone needs to set 32bit. Which
> >>> defeats the point (and we'd need to hardcode it to 32bpp). Is this
> >>> really the case?
> >>
> >> I guess it would have worked if both screens preferred 16 bpp.
> > 
> > Just noticed that current depth is 24 bpp ... does that work with
> > current X? If not I think we should actually set it to 16 bpp, and fix
> > up X. Not as in "make it handle multi-bpp", that's too hard, but make
> > it pick a common format that works for all drivers (which usually
> > means go with 32bpp). Since if we just go with 32bpp to paper over X,
> > then the preferred bpp uapi becomes meaningless.
> 
> The good news is that it apparently works with planes correctly configured.
> 
> I tested with udl being converted to simple pipe and universal planes.
> It exposes RGB565, XRGB8888 and preferred_depth\x16. X did the correct
> thing and choose a format the works with radeon and udl.

Awesome!

> For the next iteration of the fbdev conversion, I'll just keep the
> current values. And once converted to universal planes, we can set the
> optimal values instead.

Yeah sounds good. Worst case it should at least make it easier to undo the
bpp change.
-Daniel

> 
> Best regards
> Thomas
> 
> > -Daniel
> > 
> >>
> >> Best regards
> >> Thomas
> >>
> >>> -Daniel
> >>>
> >>>>
> >>>> Best regards
> >>>> Thomas
> >>>>
> >>>>> -Daniel
> >>>>>
> >>>>>>
> >>>>>> Best regards
> >>>>>> Thomas
> >>>>>>
> >>>>>>>
> >>>>>>> It's possible to set it on the command line:
> >>>>>>>
> >>>>>>> video=<xres>x<yres>-<bpp>
> >>>>>>>
> >>>>>>> I haven't tried it so I can't say for certain that it actually works>
> >>>>>>> Ref: Documentation/fb/modedb.rst and drm_fb_helper_single_fb_probe()
> >>>>>>>
> >>>>>>>> -module_param(fb_defio, int, S_IWUSR | S_IRUSR | S_IWGRP | S_IRGRP);
> >>>>>>>> -
> >>>>>>>
> >>>>>>>> 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
> >>>>>>>
> >>>>>>>> @@ -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,
> >>>>>>>
> >>>>>>> No need to set this, it's already wired up:
> >>>>>>>
> >>>>>>> drm_kms_helper_hotplug_event -> drm_client_dev_hotplug ->
> >>>>>>> drm_fbdev_client_hotplug -> drm_fb_helper_hotplug_event
> >>>>>>>
> >>>>>>> Noralf.
> >>>>>>>
> >>>>>>>>  };
> >>>>>>>>
> >>>>>>>>  int udl_modeset_init(struct drm_device *dev)
> >>>>>>>>
> >>>>>>
> >>>>>> --
> >>>>>> Thomas Zimmermann
> >>>>>> Graphics Driver Developer
> >>>>>> SUSE Software Solutions Germany GmbH
> >>>>>> Maxfeldstr. 5, 90409 Nürnberg, Germany
> >>>>>> (HRB 36809, AG Nürnberg)
> >>>>>> Geschäftsführer: Felix Imendörffer
> >>>>>>
> >>>>>
> >>>>>
> >>>>
> >>>> --
> >>>> Thomas Zimmermann
> >>>> Graphics Driver Developer
> >>>> SUSE Software Solutions Germany GmbH
> >>>> Maxfeldstr. 5, 90409 Nürnberg, Germany
> >>>> (HRB 36809, AG Nürnberg)
> >>>> Geschäftsführer: Felix Imendörffer
> >>>>
> >>>
> >>>
> >>
> >> --
> >> Thomas Zimmermann
> >> Graphics Driver Developer
> >> SUSE Software Solutions Germany GmbH
> >> Maxfeldstr. 5, 90409 Nürnberg, Germany
> >> (HRB 36809, AG Nürnberg)
> >> Geschäftsführer: Felix Imendörffer
> >>
> > 
> > 
> 
> -- 
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Software Solutions Germany GmbH
> Maxfeldstr. 5, 90409 Nürnberg, Germany
> (HRB 36809, AG Nürnberg)
> Geschäftsführer: Felix Imendörffer
> 




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

[PATCH 2/4] drm/udl: Remove udl implementation of GEM's free_object()

[Thomas Zimmermann]

Udl used to have a custom implementation for free_object() of
struct drm_gem_object_funcs. It unmapped the memory buffer of
the fbdev emulation. With the switch to generic fbdev emulation,
this is now handled by the fbdev code internally.

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

diff --git a/drivers/gpu/drm/udl/udl_gem.c b/drivers/gpu/drm/udl/udl_gem.c
index 6eade6b4b0dc..b6e26f98aa0a 100644
--- a/drivers/gpu/drm/udl/udl_gem.c
+++ b/drivers/gpu/drm/udl/udl_gem.c
@@ -17,22 +17,6 @@
  * GEM object funcs
  */
 
-static void udl_gem_object_free_object(struct drm_gem_object *obj)
-{
-	struct drm_gem_shmem_object *shmem = to_drm_gem_shmem_obj(obj);
-
-	/* Fbdev emulation vmaps the buffer. Unmap it here for consistency
-	 * with the original udl GEM code.
-	 *
-	 * TODO: Switch to generic fbdev emulation and release the
-	 *       GEM object with drm_gem_shmem_free_object().
-	 */
-	if (shmem->vaddr)
-		drm_gem_shmem_vunmap(obj, shmem->vaddr);
-
-	drm_gem_shmem_free_object(obj);
-}
-
 static int udl_gem_object_mmap(struct drm_gem_object *obj,
 			       struct vm_area_struct *vma)
 {
@@ -91,7 +75,7 @@ static void *udl_gem_object_vmap(struct drm_gem_object *obj)
 }
 
 static const struct drm_gem_object_funcs udl_gem_object_funcs = {
-	.free = udl_gem_object_free_object,
+	.free = drm_gem_shmem_free_object,
 	.print_info = drm_gem_shmem_print_info,
 	.pin = drm_gem_shmem_pin,
 	.unpin = drm_gem_shmem_unpin,
-- 
2.23.0

[PATCH 3/4] drm/fb-helper: Remove drm_fb_helper_unlink_fbi()

[Thomas Zimmermann]

There are no callers of drm_fb_helper_unlink_fbi() left. Remove the
function.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/drm_fb_helper.c | 16 +---------------
 include/drm/drm_fb_helper.h     |  6 ------
 2 files changed, 1 insertion(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 1038a2f0639e..eb97f34a15ea 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -563,8 +563,7 @@ EXPORT_SYMBOL(drm_fb_helper_unregister_fbi);
  * drm_fb_helper_fini - finialize a &struct drm_fb_helper
  * @fb_helper: driver-allocated fbdev helper, can be NULL
  *
- * This cleans up all remaining resources associated with @fb_helper. Must be
- * called after drm_fb_helper_unlink_fbi() was called.
+ * This cleans up all remaining resources associated with @fb_helper.
  */
 void drm_fb_helper_fini(struct drm_fb_helper *fb_helper)
 {
@@ -604,19 +603,6 @@ void drm_fb_helper_fini(struct drm_fb_helper *fb_helper)
 }
 EXPORT_SYMBOL(drm_fb_helper_fini);
 
-/**
- * drm_fb_helper_unlink_fbi - wrapper around unlink_framebuffer
- * @fb_helper: driver-allocated fbdev helper, can be NULL
- *
- * A wrapper around unlink_framebuffer implemented by fbdev core
- */
-void drm_fb_helper_unlink_fbi(struct drm_fb_helper *fb_helper)
-{
-	if (fb_helper && fb_helper->fbdev)
-		unlink_framebuffer(fb_helper->fbdev);
-}
-EXPORT_SYMBOL(drm_fb_helper_unlink_fbi);
-
 static bool drm_fbdev_use_shadow_fb(struct drm_fb_helper *fb_helper)
 {
 	struct drm_device *dev = fb_helper->dev;
diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
index e3a75ff07390..1c2e0c3cf857 100644
--- a/include/drm/drm_fb_helper.h
+++ b/include/drm/drm_fb_helper.h
@@ -231,8 +231,6 @@ void drm_fb_helper_fill_info(struct fb_info *info,
 			     struct drm_fb_helper *fb_helper,
 			     struct drm_fb_helper_surface_size *sizes);
 
-void drm_fb_helper_unlink_fbi(struct drm_fb_helper *fb_helper);
-
 void drm_fb_helper_deferred_io(struct fb_info *info,
 			       struct list_head *pagelist);
 
@@ -356,10 +354,6 @@ static inline int drm_fb_helper_ioctl(struct fb_info *info, unsigned int cmd,
 	return 0;
 }
 
-static inline void drm_fb_helper_unlink_fbi(struct drm_fb_helper *fb_helper)
-{
-}
-
 static inline void drm_fb_helper_deferred_io(struct fb_info *info,
 					     struct list_head *pagelist)
 {
-- 
2.23.0

[PATCH 4/4] fbdev: Unexport unlink_framebuffer()

[Thomas Zimmermann]

There are no external callers of unlink_framebuffer() left. Make the
function an internal interface.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/video/fbdev/core/fbmem.c | 3 +--
 include/linux/fb.h               | 1 -
 2 files changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
index 95c32952fa8a..86b06a599f96 100644
--- a/drivers/video/fbdev/core/fbmem.c
+++ b/drivers/video/fbdev/core/fbmem.c
@@ -1673,7 +1673,7 @@ static void unbind_console(struct fb_info *fb_info)
 	console_unlock();
 }
 
-void unlink_framebuffer(struct fb_info *fb_info)
+static void unlink_framebuffer(struct fb_info *fb_info)
 {
 	int i;
 
@@ -1692,7 +1692,6 @@ void unlink_framebuffer(struct fb_info *fb_info)
 
 	fb_info->dev = NULL;
 }
-EXPORT_SYMBOL(unlink_framebuffer);
 
 static void do_unregister_framebuffer(struct fb_info *fb_info)
 {
diff --git a/include/linux/fb.h b/include/linux/fb.h
index 41e0069eca0a..a6ad528990de 100644
--- a/include/linux/fb.h
+++ b/include/linux/fb.h
@@ -606,7 +606,6 @@ extern ssize_t fb_sys_write(struct fb_info *info, const char __user *buf,
 /* drivers/video/fbmem.c */
 extern int register_framebuffer(struct fb_info *fb_info);
 extern void unregister_framebuffer(struct fb_info *fb_info);
-extern void unlink_framebuffer(struct fb_info *fb_info);
 extern int remove_conflicting_pci_framebuffers(struct pci_dev *pdev,
 					       const char *name);
 extern int remove_conflicting_framebuffers(struct apertures_struct *a,
-- 
2.23.0