[PATCH v2 0/8] drm/fbdev-generic: Mandatory shadow buffering

[PATCH v2 0/8] drm/fbdev-generic: Mandatory shadow buffering

[Thomas Zimmermann]

After adding fbdev-dma and converting drivers, all users of
fbdev-generic require shadow buffering. Make it mandatory and
remove all other codepaths. This change greatly simplifies the
code for generic fbdev emulation. It will work with any driver
that supports GEM's vmap and vunmap.

The change further allows for a number of cleanups and fixes. The
flag prefer_shadow_fbdev is unused and to be removed. Probing in
fbdev-generic is now simple enough to roll back if it fails. Further
simplify the code for exporting the framebuffer's physical address.
Finally rename the symbols to follow other fbdev emulation.

v2:
	* handle screen_size in separate patches (Javier)

Thomas Zimmermann (8):
  drm/fbdev-generic: Always use shadow buffering
  drm/fbdev-generic: Remove unused prefer_shadow_fbdev flag
  drm/fb-helper: Export drm_fb_helper_release_info()
  drm/fb-helper: Support smem_len in deferred I/O
  drm/fbdev-generic: Set screen size to size of GEM buffer
  drm/fbdev-generic: Clean up after failed probing
  drm/fb-helper: Consolidate CONFIG_DRM_FBDEV_LEAK_PHYS_SMEM
  drm/fbdev-generic: Rename symbols

 drivers/gpu/drm/drm_fb_helper.c     |  63 ++++---
 drivers/gpu/drm/drm_fbdev_dma.c     |   9 +-
 drivers/gpu/drm/drm_fbdev_generic.c | 279 +++++++++-------------------
 drivers/gpu/drm/tiny/bochs.c        |   1 -
 drivers/gpu/drm/vmwgfx/vmwgfx_kms.c |   1 -
 include/drm/drm_fb_helper.h         |  14 +-
 include/drm/drm_mode_config.h       |   7 -
 7 files changed, 130 insertions(+), 244 deletions(-)


base-commit: 3333280906872bb8c2dff02666c7a8e46f085b24
-- 
2.40.0

[PATCH v2 1/8] drm/fbdev-generic: Always use shadow buffering

[Thomas Zimmermann]

Remove all codepaths that implement fbdev output directly on GEM
buffers. Always allocate a shadow buffer in system memory and set
up deferred I/O for mmap.

The fbdev code that operated directly on GEM buffers was used by
drivers based on GEM DMA helpers. Those drivers have been migrated
to use fbdev-dma, a dedicated fbdev emulation for DMA memory. All
remaining users of fbdev-generic require shadow buffering.

Memory management of the remaining callers uses TTM, GEM SHMEM
helpers or a variant of GEM DMA helpers that is incompatible with
fbdev-dma. Therefore remove the unused codepaths from fbdev-generic
and simplify the code.

Using a shadow buffer with deferred I/O is probably the best case
for most remaining callers. Some of the TTM-based drivers might
benefit from a dedicated fbdev emulation that operates directly on
the driver's video memory.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
Acked-by: Zack Rusin <zackr@vmware.com>
---
 drivers/gpu/drm/drm_fbdev_generic.c | 184 +++++-----------------------
 1 file changed, 30 insertions(+), 154 deletions(-)

diff --git a/drivers/gpu/drm/drm_fbdev_generic.c b/drivers/gpu/drm/drm_fbdev_generic.c
index 4d6325e91565..e48a8e82378d 100644
--- a/drivers/gpu/drm/drm_fbdev_generic.c
+++ b/drivers/gpu/drm/drm_fbdev_generic.c
@@ -11,16 +11,6 @@
 
 #include <drm/drm_fbdev_generic.h>
 
-static bool drm_fbdev_use_shadow_fb(struct drm_fb_helper *fb_helper)
-{
-	struct drm_device *dev = fb_helper->dev;
-	struct drm_framebuffer *fb = fb_helper->fb;
-
-	return dev->mode_config.prefer_shadow_fbdev ||
-	       dev->mode_config.prefer_shadow ||
-	       fb->funcs->dirty;
-}
-
 /* @user: 1=userspace, 0=fbcon */
 static int drm_fbdev_fb_open(struct fb_info *info, int user)
 {
@@ -46,115 +36,33 @@ static int drm_fbdev_fb_release(struct fb_info *info, int user)
 static void drm_fbdev_fb_destroy(struct fb_info *info)
 {
 	struct drm_fb_helper *fb_helper = info->par;
-	void *shadow = NULL;
+	void *shadow = info->screen_buffer;
 
 	if (!fb_helper->dev)
 		return;
 
-	if (info->fbdefio)
-		fb_deferred_io_cleanup(info);
-	if (drm_fbdev_use_shadow_fb(fb_helper))
-		shadow = info->screen_buffer;
-
+	fb_deferred_io_cleanup(info);
 	drm_fb_helper_fini(fb_helper);
-
-	if (shadow)
-		vfree(shadow);
-	else if (fb_helper->buffer)
-		drm_client_buffer_vunmap(fb_helper->buffer);
-
+	vfree(shadow);
 	drm_client_framebuffer_delete(fb_helper->buffer);
-	drm_client_release(&fb_helper->client);
 
+	drm_client_release(&fb_helper->client);
 	drm_fb_helper_unprepare(fb_helper);
 	kfree(fb_helper);
 }
 
-static int drm_fbdev_fb_mmap(struct fb_info *info, struct vm_area_struct *vma)
-{
-	struct drm_fb_helper *fb_helper = info->par;
-
-	if (drm_fbdev_use_shadow_fb(fb_helper))
-		return fb_deferred_io_mmap(info, vma);
-	else if (fb_helper->dev->driver->gem_prime_mmap)
-		return fb_helper->dev->driver->gem_prime_mmap(fb_helper->buffer->gem, vma);
-	else
-		return -ENODEV;
-}
-
-static bool drm_fbdev_use_iomem(struct fb_info *info)
-{
-	struct drm_fb_helper *fb_helper = info->par;
-	struct drm_client_buffer *buffer = fb_helper->buffer;
-
-	return !drm_fbdev_use_shadow_fb(fb_helper) && buffer->map.is_iomem;
-}
-
-static ssize_t drm_fbdev_fb_read(struct fb_info *info, char __user *buf,
-				 size_t count, loff_t *ppos)
-{
-	ssize_t ret;
-
-	if (drm_fbdev_use_iomem(info))
-		ret = drm_fb_helper_cfb_read(info, buf, count, ppos);
-	else
-		ret = drm_fb_helper_sys_read(info, buf, count, ppos);
-
-	return ret;
-}
-
-static ssize_t drm_fbdev_fb_write(struct fb_info *info, const char __user *buf,
-				  size_t count, loff_t *ppos)
-{
-	ssize_t ret;
-
-	if (drm_fbdev_use_iomem(info))
-		ret = drm_fb_helper_cfb_write(info, buf, count, ppos);
-	else
-		ret = drm_fb_helper_sys_write(info, buf, count, ppos);
-
-	return ret;
-}
-
-static void drm_fbdev_fb_fillrect(struct fb_info *info,
-				  const struct fb_fillrect *rect)
-{
-	if (drm_fbdev_use_iomem(info))
-		drm_fb_helper_cfb_fillrect(info, rect);
-	else
-		drm_fb_helper_sys_fillrect(info, rect);
-}
-
-static void drm_fbdev_fb_copyarea(struct fb_info *info,
-				  const struct fb_copyarea *area)
-{
-	if (drm_fbdev_use_iomem(info))
-		drm_fb_helper_cfb_copyarea(info, area);
-	else
-		drm_fb_helper_sys_copyarea(info, area);
-}
-
-static void drm_fbdev_fb_imageblit(struct fb_info *info,
-				   const struct fb_image *image)
-{
-	if (drm_fbdev_use_iomem(info))
-		drm_fb_helper_cfb_imageblit(info, image);
-	else
-		drm_fb_helper_sys_imageblit(info, image);
-}
-
 static const struct fb_ops drm_fbdev_fb_ops = {
 	.owner		= THIS_MODULE,
-	DRM_FB_HELPER_DEFAULT_OPS,
 	.fb_open	= drm_fbdev_fb_open,
 	.fb_release	= drm_fbdev_fb_release,
+	.fb_read	= drm_fb_helper_sys_read,
+	.fb_write	= drm_fb_helper_sys_write,
+	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	= fb_deferred_io_mmap,
 	.fb_destroy	= drm_fbdev_fb_destroy,
-	.fb_mmap	= drm_fbdev_fb_mmap,
-	.fb_read	= drm_fbdev_fb_read,
-	.fb_write	= drm_fbdev_fb_write,
-	.fb_fillrect	= drm_fbdev_fb_fillrect,
-	.fb_copyarea	= drm_fbdev_fb_copyarea,
-	.fb_imageblit	= drm_fbdev_fb_imageblit,
 };
 
 /*
@@ -169,7 +77,6 @@ static int drm_fbdev_fb_probe(struct drm_fb_helper *fb_helper,
 	struct drm_framebuffer *fb;
 	struct fb_info *info;
 	u32 format;
-	struct iosys_map map;
 	int ret;
 
 	drm_dbg_kms(dev, "surface width(%d), height(%d) and bpp(%d)\n",
@@ -197,44 +104,21 @@ static int drm_fbdev_fb_probe(struct drm_fb_helper *fb_helper,
 
 	drm_fb_helper_fill_info(info, fb_helper, sizes);
 
-	if (drm_fbdev_use_shadow_fb(fb_helper)) {
-		info->screen_buffer = vzalloc(info->screen_size);
-		if (!info->screen_buffer)
-			return -ENOMEM;
-		info->flags |= FBINFO_VIRTFB | FBINFO_READS_FAST;
+	info->screen_buffer = vzalloc(info->screen_size);
+	if (!info->screen_buffer)
+		return -ENOMEM;
+	info->flags |= FBINFO_VIRTFB | FBINFO_READS_FAST;
 
-		/* Set a default deferred I/O handler */
-		fb_helper->fbdefio.delay = HZ / 20;
-		fb_helper->fbdefio.deferred_io = drm_fb_helper_deferred_io;
+	info->fix.smem_start = page_to_phys(vmalloc_to_page(info->screen_buffer));
 
-		info->fbdefio = &fb_helper->fbdefio;
-		ret = fb_deferred_io_init(info);
-		if (ret)
-			return ret;
-	} else {
-		/* buffer is mapped for HW framebuffer */
-		ret = drm_client_buffer_vmap(fb_helper->buffer, &map);
-		if (ret)
-			return ret;
-		if (map.is_iomem) {
-			info->screen_base = map.vaddr_iomem;
-		} else {
-			info->screen_buffer = map.vaddr;
-			info->flags |= FBINFO_VIRTFB;
-		}
-
-		/*
-		 * Shamelessly leak the physical address to user-space. As
-		 * page_to_phys() is undefined for I/O memory, warn in this
-		 * case.
-		 */
-#if IS_ENABLED(CONFIG_DRM_FBDEV_LEAK_PHYS_SMEM)
-		if (fb_helper->hint_leak_smem_start && info->fix.smem_start == 0 &&
-		    !drm_WARN_ON_ONCE(dev, map.is_iomem))
-			info->fix.smem_start =
-				page_to_phys(virt_to_page(info->screen_buffer));
-#endif
-	}
+	/* Set a default deferred I/O handler */
+	fb_helper->fbdefio.delay = HZ / 20;
+	fb_helper->fbdefio.deferred_io = drm_fb_helper_deferred_io;
+
+	info->fbdefio = &fb_helper->fbdefio;
+	ret = fb_deferred_io_init(info);
+	if (ret)
+		return ret;
 
 	return 0;
 }
@@ -318,18 +202,13 @@ static int drm_fbdev_fb_dirty(struct drm_fb_helper *helper, struct drm_clip_rect
 	struct drm_device *dev = helper->dev;
 	int ret;
 
-	if (!drm_fbdev_use_shadow_fb(helper))
-		return 0;
-
 	/* Call damage handlers only if necessary */
 	if (!(clip->x1 < clip->x2 && clip->y1 < clip->y2))
 		return 0;
 
-	if (helper->buffer) {
-		ret = drm_fbdev_damage_blit(helper, clip);
-		if (drm_WARN_ONCE(dev, ret, "Damage blitter failed: ret=%d\n", ret))
-			return ret;
-	}
+	ret = drm_fbdev_damage_blit(helper, clip);
+	if (drm_WARN_ONCE(dev, ret, "Damage blitter failed: ret=%d\n", ret))
+		return ret;
 
 	if (helper->fb->funcs->dirty) {
 		ret = helper->fb->funcs->dirty(helper->fb, NULL, 0, 0, clip, 1);
@@ -415,12 +294,9 @@ static const struct drm_client_funcs drm_fbdev_client_funcs = {
  * suspend/resume need to call drm_fb_helper_set_suspend_unlocked() themselves.
  * Simple drivers might use drm_mode_config_helper_suspend().
  *
- * Drivers that set the dirty callback on their framebuffer will get a shadow
- * fbdev buffer that is blitted onto the real buffer. This is done in order to
- * make deferred I/O work with all kinds of buffers. A shadow buffer can be
- * requested explicitly by setting struct drm_mode_config.prefer_shadow or
- * struct drm_mode_config.prefer_shadow_fbdev to true beforehand. This is
- * required to use generic fbdev emulation with SHMEM helpers.
+ * In order to provide fixed mmap-able memory ranges, generic fbdev emulation
+ * uses a shadow buffer in system memory. The implementation blits the shadow
+ * fbdev buffer onto the real buffer in regular intervals.
  *
  * This function is safe to call even when there are no connectors present.
  * Setup will be retried on the next hotplug event.
-- 
2.40.0

Re: [v2,1/8] drm/fbdev-generic: Always use shadow buffering

[Sui jingfeng]

On 2023/3/20 23:07, Thomas Zimmermann wrote:
> Remove all codepaths that implement fbdev output directly on GEM
> buffers. Always allocate a shadow buffer in system memory and set
> up deferred I/O for mmap.
>
> The fbdev code that operated directly on GEM buffers was used by
> drivers based on GEM DMA helpers. Those drivers have been migrated
> to use fbdev-dma, a dedicated fbdev emulation for DMA memory. All
> remaining users of fbdev-generic require shadow buffering.
>
> Memory management of the remaining callers uses TTM, GEM SHMEM
> helpers or a variant of GEM DMA helpers that is incompatible with
> fbdev-dma. Therefore remove the unused codepaths from fbdev-generic
> and simplify the code.
>
> Using a shadow buffer with deferred I/O is probably the best case
> for most remaining callers. Some of the TTM-based drivers might
> benefit from a dedicated fbdev emulation that operates directly on
> the driver's video memory.

I don't understand here,  the TTM-based drivers should have equivalent 
performance

with you implement. Because device memory typically very slow for cpu 
read, at least

this is true for Mips and loongarch architecture.  TTM-based drivers for 
those platform

is also prefer to render to system ram first(for fast reading and 
compositing) and then

blit to the real framebuffer pinned to VRAM.


In turn, I think shmem helper based drivers might benefit from a 
dedicated fbdev emulation.

Because you are blit to the shadow of the video memory for shmem helper 
based driver. The

driver may need another blit to the ultimate framebuffer.  Using a 
shadow buffer is still acceptable

though, but why  do you say "the TTM-based drivers might benefit from a 
dedicated fbdev emulation" ?

>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
> Acked-by: Zack Rusin <zackr@vmware.com>
> ---
>   drivers/gpu/drm/drm_fbdev_generic.c | 184 +++++-----------------------
>   1 file changed, 30 insertions(+), 154 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_fbdev_generic.c b/drivers/gpu/drm/drm_fbdev_generic.c
> index 4d6325e91565..e48a8e82378d 100644
> --- a/drivers/gpu/drm/drm_fbdev_generic.c
> +++ b/drivers/gpu/drm/drm_fbdev_generic.c
> @@ -11,16 +11,6 @@
>   
>   #include <drm/drm_fbdev_generic.h>
>   
> -static bool drm_fbdev_use_shadow_fb(struct drm_fb_helper *fb_helper)
> -{
> -	struct drm_device *dev = fb_helper->dev;
> -	struct drm_framebuffer *fb = fb_helper->fb;
> -
> -	return dev->mode_config.prefer_shadow_fbdev ||
> -	       dev->mode_config.prefer_shadow ||
> -	       fb->funcs->dirty;
> -}
> -
>   /* @user: 1=userspace, 0=fbcon */
>   static int drm_fbdev_fb_open(struct fb_info *info, int user)
>   {
> @@ -46,115 +36,33 @@ static int drm_fbdev_fb_release(struct fb_info *info, int user)
>   static void drm_fbdev_fb_destroy(struct fb_info *info)
>   {
>   	struct drm_fb_helper *fb_helper = info->par;
> -	void *shadow = NULL;
> +	void *shadow = info->screen_buffer;
>   
>   	if (!fb_helper->dev)
>   		return;
>   
> -	if (info->fbdefio)
> -		fb_deferred_io_cleanup(info);
> -	if (drm_fbdev_use_shadow_fb(fb_helper))
> -		shadow = info->screen_buffer;
> -
> +	fb_deferred_io_cleanup(info);
>   	drm_fb_helper_fini(fb_helper);
> -
> -	if (shadow)
> -		vfree(shadow);
> -	else if (fb_helper->buffer)
> -		drm_client_buffer_vunmap(fb_helper->buffer);
> -
> +	vfree(shadow);
>   	drm_client_framebuffer_delete(fb_helper->buffer);
> -	drm_client_release(&fb_helper->client);
>   
> +	drm_client_release(&fb_helper->client);
>   	drm_fb_helper_unprepare(fb_helper);
>   	kfree(fb_helper);
>   }
>   
> -static int drm_fbdev_fb_mmap(struct fb_info *info, struct vm_area_struct *vma)
> -{
> -	struct drm_fb_helper *fb_helper = info->par;
> -
> -	if (drm_fbdev_use_shadow_fb(fb_helper))
> -		return fb_deferred_io_mmap(info, vma);
> -	else if (fb_helper->dev->driver->gem_prime_mmap)
> -		return fb_helper->dev->driver->gem_prime_mmap(fb_helper->buffer->gem, vma);
> -	else
> -		return -ENODEV;
> -}
> -
> -static bool drm_fbdev_use_iomem(struct fb_info *info)
> -{
> -	struct drm_fb_helper *fb_helper = info->par;
> -	struct drm_client_buffer *buffer = fb_helper->buffer;
> -
> -	return !drm_fbdev_use_shadow_fb(fb_helper) && buffer->map.is_iomem;
> -}
> -
> -static ssize_t drm_fbdev_fb_read(struct fb_info *info, char __user *buf,
> -				 size_t count, loff_t *ppos)
> -{
> -	ssize_t ret;
> -
> -	if (drm_fbdev_use_iomem(info))
> -		ret = drm_fb_helper_cfb_read(info, buf, count, ppos);
> -	else
> -		ret = drm_fb_helper_sys_read(info, buf, count, ppos);
> -
> -	return ret;
> -}
> -
> -static ssize_t drm_fbdev_fb_write(struct fb_info *info, const char __user *buf,
> -				  size_t count, loff_t *ppos)
> -{
> -	ssize_t ret;
> -
> -	if (drm_fbdev_use_iomem(info))
> -		ret = drm_fb_helper_cfb_write(info, buf, count, ppos);
> -	else
> -		ret = drm_fb_helper_sys_write(info, buf, count, ppos);
> -
> -	return ret;
> -}
> -
> -static void drm_fbdev_fb_fillrect(struct fb_info *info,
> -				  const struct fb_fillrect *rect)
> -{
> -	if (drm_fbdev_use_iomem(info))
> -		drm_fb_helper_cfb_fillrect(info, rect);
> -	else
> -		drm_fb_helper_sys_fillrect(info, rect);
> -}
> -
> -static void drm_fbdev_fb_copyarea(struct fb_info *info,
> -				  const struct fb_copyarea *area)
> -{
> -	if (drm_fbdev_use_iomem(info))
> -		drm_fb_helper_cfb_copyarea(info, area);
> -	else
> -		drm_fb_helper_sys_copyarea(info, area);
> -}
> -
> -static void drm_fbdev_fb_imageblit(struct fb_info *info,
> -				   const struct fb_image *image)
> -{
> -	if (drm_fbdev_use_iomem(info))
> -		drm_fb_helper_cfb_imageblit(info, image);
> -	else
> -		drm_fb_helper_sys_imageblit(info, image);
> -}
> -
>   static const struct fb_ops drm_fbdev_fb_ops = {
>   	.owner		= THIS_MODULE,
> -	DRM_FB_HELPER_DEFAULT_OPS,
>   	.fb_open	= drm_fbdev_fb_open,
>   	.fb_release	= drm_fbdev_fb_release,
> +	.fb_read	= drm_fb_helper_sys_read,
> +	.fb_write	= drm_fb_helper_sys_write,
> +	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	= fb_deferred_io_mmap,
>   	.fb_destroy	= drm_fbdev_fb_destroy,
> -	.fb_mmap	= drm_fbdev_fb_mmap,
> -	.fb_read	= drm_fbdev_fb_read,
> -	.fb_write	= drm_fbdev_fb_write,
> -	.fb_fillrect	= drm_fbdev_fb_fillrect,
> -	.fb_copyarea	= drm_fbdev_fb_copyarea,
> -	.fb_imageblit	= drm_fbdev_fb_imageblit,
>   };
>   
>   /*
> @@ -169,7 +77,6 @@ static int drm_fbdev_fb_probe(struct drm_fb_helper *fb_helper,
>   	struct drm_framebuffer *fb;
>   	struct fb_info *info;
>   	u32 format;
> -	struct iosys_map map;
>   	int ret;
>   
>   	drm_dbg_kms(dev, "surface width(%d), height(%d) and bpp(%d)\n",
> @@ -197,44 +104,21 @@ static int drm_fbdev_fb_probe(struct drm_fb_helper *fb_helper,
>   
>   	drm_fb_helper_fill_info(info, fb_helper, sizes);
>   
> -	if (drm_fbdev_use_shadow_fb(fb_helper)) {
> -		info->screen_buffer = vzalloc(info->screen_size);
> -		if (!info->screen_buffer)
> -			return -ENOMEM;
> -		info->flags |= FBINFO_VIRTFB | FBINFO_READS_FAST;
> +	info->screen_buffer = vzalloc(info->screen_size);
> +	if (!info->screen_buffer)
> +		return -ENOMEM;
> +	info->flags |= FBINFO_VIRTFB | FBINFO_READS_FAST;
>   
> -		/* Set a default deferred I/O handler */
> -		fb_helper->fbdefio.delay = HZ / 20;
> -		fb_helper->fbdefio.deferred_io = drm_fb_helper_deferred_io;
> +	info->fix.smem_start = page_to_phys(vmalloc_to_page(info->screen_buffer));

Why  simply use  screen_buffer instead of info->screen_buffer here ?

info->fix.smem_start = page_to_phys(vmalloc_to_page(screen_buffer));

I'm asking because I see you use vfree(screen_buffer) below the err_vfree label in this function.


I also want to ask another question here:
I heard,//the/ /memory/ /allocated/ /by//*//vzalloc//*//is/ /not/ /physically/ /contiguous/. /Why such a virtual address can convert to physical address by//page_to_phys(vmalloc_to_page())?//Does it legal for a GPU without MMU accessing such a physical address leaked to user-space?

> -		info->fbdefio = &fb_helper->fbdefio;
> -		ret = fb_deferred_io_init(info);
> -		if (ret)
> -			return ret;
> -	} else {
> -		/* buffer is mapped for HW framebuffer */
> -		ret = drm_client_buffer_vmap(fb_helper->buffer, &map);
> -		if (ret)
> -			return ret;
> -		if (map.is_iomem) {
> -			info->screen_base = map.vaddr_iomem;
> -		} else {
> -			info->screen_buffer = map.vaddr;
> -			info->flags |= FBINFO_VIRTFB;
> -		}
> -
> -		/*
> -		 * Shamelessly leak the physical address to user-space. As
> -		 * page_to_phys() is undefined for I/O memory, warn in this
> -		 * case.
> -		 */
> -#if IS_ENABLED(CONFIG_DRM_FBDEV_LEAK_PHYS_SMEM)
> -		if (fb_helper->hint_leak_smem_start && info->fix.smem_start == 0 &&
> -		    !drm_WARN_ON_ONCE(dev, map.is_iomem))
> -			info->fix.smem_start =
> -				page_to_phys(virt_to_page(info->screen_buffer));
> -#endif
> -	}
> +	/* Set a default deferred I/O handler */
> +	fb_helper->fbdefio.delay = HZ / 20;
> +	fb_helper->fbdefio.deferred_io = drm_fb_helper_deferred_io;
> +
> +	info->fbdefio = &fb_helper->fbdefio;
> +	ret = fb_deferred_io_init(info);
> +	if (ret)
> +		return ret;
>   
>   	return 0;
>   }
> @@ -318,18 +202,13 @@ static int drm_fbdev_fb_dirty(struct drm_fb_helper *helper, struct drm_clip_rect
>   	struct drm_device *dev = helper->dev;
>   	int ret;
>   
> -	if (!drm_fbdev_use_shadow_fb(helper))
> -		return 0;
> -
>   	/* Call damage handlers only if necessary */
>   	if (!(clip->x1 < clip->x2 && clip->y1 < clip->y2))
>   		return 0;
>   
> -	if (helper->buffer) {
> -		ret = drm_fbdev_damage_blit(helper, clip);
> -		if (drm_WARN_ONCE(dev, ret, "Damage blitter failed: ret=%d\n", ret))
> -			return ret;
> -	}
> +	ret = drm_fbdev_damage_blit(helper, clip);
> +	if (drm_WARN_ONCE(dev, ret, "Damage blitter failed: ret=%d\n", ret))
> +		return ret;
>   
>   	if (helper->fb->funcs->dirty) {
>   		ret = helper->fb->funcs->dirty(helper->fb, NULL, 0, 0, clip, 1);
> @@ -415,12 +294,9 @@ static const struct drm_client_funcs drm_fbdev_client_funcs = {
>    * suspend/resume need to call drm_fb_helper_set_suspend_unlocked() themselves.
>    * Simple drivers might use drm_mode_config_helper_suspend().
>    *
> - * Drivers that set the dirty callback on their framebuffer will get a shadow
> - * fbdev buffer that is blitted onto the real buffer. This is done in order to
> - * make deferred I/O work with all kinds of buffers. A shadow buffer can be
> - * requested explicitly by setting struct drm_mode_config.prefer_shadow or
> - * struct drm_mode_config.prefer_shadow_fbdev to true beforehand. This is
> - * required to use generic fbdev emulation with SHMEM helpers.
> + * In order to provide fixed mmap-able memory ranges,

I don't understand here, what do you mean about *fixed*?

fixed relative to what? Can you say more?

Re: [v2,1/8] drm/fbdev-generic: Always use shadow buffering

[Thomas Zimmermann]

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

Hi,

thanks for testing the patchset.

Am 21.03.23 um 16:23 schrieb Sui jingfeng:
> 
> On 2023/3/20 23:07, Thomas Zimmermann wrote:
>> Remove all codepaths that implement fbdev output directly on GEM
>> buffers. Always allocate a shadow buffer in system memory and set
>> up deferred I/O for mmap.
>>
>> The fbdev code that operated directly on GEM buffers was used by
>> drivers based on GEM DMA helpers. Those drivers have been migrated
>> to use fbdev-dma, a dedicated fbdev emulation for DMA memory. All
>> remaining users of fbdev-generic require shadow buffering.
>>
>> Memory management of the remaining callers uses TTM, GEM SHMEM
>> helpers or a variant of GEM DMA helpers that is incompatible with
>> fbdev-dma. Therefore remove the unused codepaths from fbdev-generic
>> and simplify the code.
>>
>> Using a shadow buffer with deferred I/O is probably the best case
>> for most remaining callers. Some of the TTM-based drivers might
>> benefit from a dedicated fbdev emulation that operates directly on
>> the driver's video memory.
> 
> I don't understand here,  the TTM-based drivers should have equivalent 
> performance
> 
> with you implement. Because device memory typically very slow for cpu 
> read, at least
> 
> this is true for Mips and loongarch architecture.  TTM-based drivers for 
> those platform
> 
> is also prefer to render to system ram first(for fast reading and 
> compositing) and then
> 
> blit to the real framebuffer pinned to VRAM.

Right. But in practice, writing the console directly to the video ram is 
perceived as faster. The shadow buffer needs an additional memcpy to the 
video ram. That adds latency to the output. It's more a slowness in 
perception than in actual I/O access.

The drawing helpers for system memory have also been slow compared to 
the helper for I/O memory. I've fixed some of it, but I'm not sure if I 
covered all cases. See [1] for the patches.

[1] https://patchwork.freedesktop.org/series/100317/

And finally, with the fbdev buffer in video memory, drivers could 
utilize hardware blitting to further speed up drawing. Although not many 
drivers ever did this.

> 
> 
> In turn, I think shmem helper based drivers might benefit from a 
> dedicated fbdev emulation.
> 
> Because you are blit to the shadow of the video memory for shmem helper 
> based driver. The
> 
> driver may need another blit to the ultimate framebuffer.  Using a 

Indeed. SHMEM has two memcpy operations. If we could remove the fbdev 
shadow buffer and operate on the SHMEM buffer directly, we'd reduce 
overhead.

It would also mean that we'd mmap the SHMEM pages to fbdev's userspace. 
I've not yet managed to implement this successfully, as it would require 
new mmap code in the GEM SHMEM object for this purpose. And so far, I've 
always ran into a bug where the fbdev emulation would end up in a 
corrupt state.

I've spend time on 'fbdev-shmem' emulation, but given the problems, it 
hasn't paid off so far.

Best regards
Thomas

> shadow buffer is still acceptable
> 
> though, but why  do you say "the TTM-based drivers might benefit from a 
> dedicated fbdev emulation" ?
> 
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
>> Acked-by: Zack Rusin <zackr@vmware.com>
>> ---
>>   drivers/gpu/drm/drm_fbdev_generic.c | 184 +++++-----------------------
>>   1 file changed, 30 insertions(+), 154 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_fbdev_generic.c 
>> b/drivers/gpu/drm/drm_fbdev_generic.c
>> index 4d6325e91565..e48a8e82378d 100644
>> --- a/drivers/gpu/drm/drm_fbdev_generic.c
>> +++ b/drivers/gpu/drm/drm_fbdev_generic.c
>> @@ -11,16 +11,6 @@
>>   #include <drm/drm_fbdev_generic.h>
>> -static bool drm_fbdev_use_shadow_fb(struct drm_fb_helper *fb_helper)
>> -{
>> -    struct drm_device *dev = fb_helper->dev;
>> -    struct drm_framebuffer *fb = fb_helper->fb;
>> -
>> -    return dev->mode_config.prefer_shadow_fbdev ||
>> -           dev->mode_config.prefer_shadow ||
>> -           fb->funcs->dirty;
>> -}
>> -
>>   /* @user: 1=userspace, 0=fbcon */
>>   static int drm_fbdev_fb_open(struct fb_info *info, int user)
>>   {
>> @@ -46,115 +36,33 @@ static int drm_fbdev_fb_release(struct fb_info 
>> *info, int user)
>>   static void drm_fbdev_fb_destroy(struct fb_info *info)
>>   {
>>       struct drm_fb_helper *fb_helper = info->par;
>> -    void *shadow = NULL;
>> +    void *shadow = info->screen_buffer;
>>       if (!fb_helper->dev)
>>           return;
>> -    if (info->fbdefio)
>> -        fb_deferred_io_cleanup(info);
>> -    if (drm_fbdev_use_shadow_fb(fb_helper))
>> -        shadow = info->screen_buffer;
>> -
>> +    fb_deferred_io_cleanup(info);
>>       drm_fb_helper_fini(fb_helper);
>> -
>> -    if (shadow)
>> -        vfree(shadow);
>> -    else if (fb_helper->buffer)
>> -        drm_client_buffer_vunmap(fb_helper->buffer);
>> -
>> +    vfree(shadow);
>>       drm_client_framebuffer_delete(fb_helper->buffer);
>> -    drm_client_release(&fb_helper->client);
>> +    drm_client_release(&fb_helper->client);
>>       drm_fb_helper_unprepare(fb_helper);
>>       kfree(fb_helper);
>>   }
>> -static int drm_fbdev_fb_mmap(struct fb_info *info, struct 
>> vm_area_struct *vma)
>> -{
>> -    struct drm_fb_helper *fb_helper = info->par;
>> -
>> -    if (drm_fbdev_use_shadow_fb(fb_helper))
>> -        return fb_deferred_io_mmap(info, vma);
>> -    else if (fb_helper->dev->driver->gem_prime_mmap)
>> -        return 
>> fb_helper->dev->driver->gem_prime_mmap(fb_helper->buffer->gem, vma);
>> -    else
>> -        return -ENODEV;
>> -}
>> -
>> -static bool drm_fbdev_use_iomem(struct fb_info *info)
>> -{
>> -    struct drm_fb_helper *fb_helper = info->par;
>> -    struct drm_client_buffer *buffer = fb_helper->buffer;
>> -
>> -    return !drm_fbdev_use_shadow_fb(fb_helper) && buffer->map.is_iomem;
>> -}
>> -
>> -static ssize_t drm_fbdev_fb_read(struct fb_info *info, char __user *buf,
>> -                 size_t count, loff_t *ppos)
>> -{
>> -    ssize_t ret;
>> -
>> -    if (drm_fbdev_use_iomem(info))
>> -        ret = drm_fb_helper_cfb_read(info, buf, count, ppos);
>> -    else
>> -        ret = drm_fb_helper_sys_read(info, buf, count, ppos);
>> -
>> -    return ret;
>> -}
>> -
>> -static ssize_t drm_fbdev_fb_write(struct fb_info *info, const char 
>> __user *buf,
>> -                  size_t count, loff_t *ppos)
>> -{
>> -    ssize_t ret;
>> -
>> -    if (drm_fbdev_use_iomem(info))
>> -        ret = drm_fb_helper_cfb_write(info, buf, count, ppos);
>> -    else
>> -        ret = drm_fb_helper_sys_write(info, buf, count, ppos);
>> -
>> -    return ret;
>> -}
>> -
>> -static void drm_fbdev_fb_fillrect(struct fb_info *info,
>> -                  const struct fb_fillrect *rect)
>> -{
>> -    if (drm_fbdev_use_iomem(info))
>> -        drm_fb_helper_cfb_fillrect(info, rect);
>> -    else
>> -        drm_fb_helper_sys_fillrect(info, rect);
>> -}
>> -
>> -static void drm_fbdev_fb_copyarea(struct fb_info *info,
>> -                  const struct fb_copyarea *area)
>> -{
>> -    if (drm_fbdev_use_iomem(info))
>> -        drm_fb_helper_cfb_copyarea(info, area);
>> -    else
>> -        drm_fb_helper_sys_copyarea(info, area);
>> -}
>> -
>> -static void drm_fbdev_fb_imageblit(struct fb_info *info,
>> -                   const struct fb_image *image)
>> -{
>> -    if (drm_fbdev_use_iomem(info))
>> -        drm_fb_helper_cfb_imageblit(info, image);
>> -    else
>> -        drm_fb_helper_sys_imageblit(info, image);
>> -}
>> -
>>   static const struct fb_ops drm_fbdev_fb_ops = {
>>       .owner        = THIS_MODULE,
>> -    DRM_FB_HELPER_DEFAULT_OPS,
>>       .fb_open    = drm_fbdev_fb_open,
>>       .fb_release    = drm_fbdev_fb_release,
>> +    .fb_read    = drm_fb_helper_sys_read,
>> +    .fb_write    = drm_fb_helper_sys_write,
>> +    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    = fb_deferred_io_mmap,
>>       .fb_destroy    = drm_fbdev_fb_destroy,
>> -    .fb_mmap    = drm_fbdev_fb_mmap,
>> -    .fb_read    = drm_fbdev_fb_read,
>> -    .fb_write    = drm_fbdev_fb_write,
>> -    .fb_fillrect    = drm_fbdev_fb_fillrect,
>> -    .fb_copyarea    = drm_fbdev_fb_copyarea,
>> -    .fb_imageblit    = drm_fbdev_fb_imageblit,
>>   };
>>   /*
>> @@ -169,7 +77,6 @@ static int drm_fbdev_fb_probe(struct drm_fb_helper 
>> *fb_helper,
>>       struct drm_framebuffer *fb;
>>       struct fb_info *info;
>>       u32 format;
>> -    struct iosys_map map;
>>       int ret;
>>       drm_dbg_kms(dev, "surface width(%d), height(%d) and bpp(%d)\n",
>> @@ -197,44 +104,21 @@ static int drm_fbdev_fb_probe(struct 
>> drm_fb_helper *fb_helper,
>>       drm_fb_helper_fill_info(info, fb_helper, sizes);
>> -    if (drm_fbdev_use_shadow_fb(fb_helper)) {
>> -        info->screen_buffer = vzalloc(info->screen_size);
>> -        if (!info->screen_buffer)
>> -            return -ENOMEM;
>> -        info->flags |= FBINFO_VIRTFB | FBINFO_READS_FAST;
>> +    info->screen_buffer = vzalloc(info->screen_size);
>> +    if (!info->screen_buffer)
>> +        return -ENOMEM;
>> +    info->flags |= FBINFO_VIRTFB | FBINFO_READS_FAST;
>> -        /* Set a default deferred I/O handler */
>> -        fb_helper->fbdefio.delay = HZ / 20;
>> -        fb_helper->fbdefio.deferred_io = drm_fb_helper_deferred_io;
>> +    info->fix.smem_start = 
>> page_to_phys(vmalloc_to_page(info->screen_buffer));
> 
> Why  simply use  screen_buffer instead of info->screen_buffer here ?
> 
> info->fix.smem_start = page_to_phys(vmalloc_to_page(screen_buffer));
> 
> I'm asking because I see you use vfree(screen_buffer) below the 
> err_vfree label in this function.
> 
> 
> I also want to ask another question here:
> I heard,//the/ /memory/ /allocated/ /by//*//vzalloc//*//is/ /not/ 
> /physically/ /contiguous/. /Why such a virtual address can convert to 
> physical address by//page_to_phys(vmalloc_to_page())?//Does it legal for 
> a GPU without MMU accessing such a physical address leaked to user-space?
> 
>> -        info->fbdefio = &fb_helper->fbdefio;
>> -        ret = fb_deferred_io_init(info);
>> -        if (ret)
>> -            return ret;
>> -    } else {
>> -        /* buffer is mapped for HW framebuffer */
>> -        ret = drm_client_buffer_vmap(fb_helper->buffer, &map);
>> -        if (ret)
>> -            return ret;
>> -        if (map.is_iomem) {
>> -            info->screen_base = map.vaddr_iomem;
>> -        } else {
>> -            info->screen_buffer = map.vaddr;
>> -            info->flags |= FBINFO_VIRTFB;
>> -        }
>> -
>> -        /*
>> -         * Shamelessly leak the physical address to user-space. As
>> -         * page_to_phys() is undefined for I/O memory, warn in this
>> -         * case.
>> -         */
>> -#if IS_ENABLED(CONFIG_DRM_FBDEV_LEAK_PHYS_SMEM)
>> -        if (fb_helper->hint_leak_smem_start && info->fix.smem_start 
>> == 0 &&
>> -            !drm_WARN_ON_ONCE(dev, map.is_iomem))
>> -            info->fix.smem_start =
>> -                page_to_phys(virt_to_page(info->screen_buffer));
>> -#endif
>> -    }
>> +    /* Set a default deferred I/O handler */
>> +    fb_helper->fbdefio.delay = HZ / 20;
>> +    fb_helper->fbdefio.deferred_io = drm_fb_helper_deferred_io;
>> +
>> +    info->fbdefio = &fb_helper->fbdefio;
>> +    ret = fb_deferred_io_init(info);
>> +    if (ret)
>> +        return ret;
>>       return 0;
>>   }
>> @@ -318,18 +202,13 @@ static int drm_fbdev_fb_dirty(struct 
>> drm_fb_helper *helper, struct drm_clip_rect
>>       struct drm_device *dev = helper->dev;
>>       int ret;
>> -    if (!drm_fbdev_use_shadow_fb(helper))
>> -        return 0;
>> -
>>       /* Call damage handlers only if necessary */
>>       if (!(clip->x1 < clip->x2 && clip->y1 < clip->y2))
>>           return 0;
>> -    if (helper->buffer) {
>> -        ret = drm_fbdev_damage_blit(helper, clip);
>> -        if (drm_WARN_ONCE(dev, ret, "Damage blitter failed: 
>> ret=%d\n", ret))
>> -            return ret;
>> -    }
>> +    ret = drm_fbdev_damage_blit(helper, clip);
>> +    if (drm_WARN_ONCE(dev, ret, "Damage blitter failed: ret=%d\n", ret))
>> +        return ret;
>>       if (helper->fb->funcs->dirty) {
>>           ret = helper->fb->funcs->dirty(helper->fb, NULL, 0, 0, clip, 
>> 1);
>> @@ -415,12 +294,9 @@ static const struct drm_client_funcs 
>> drm_fbdev_client_funcs = {
>>    * suspend/resume need to call drm_fb_helper_set_suspend_unlocked() 
>> themselves.
>>    * Simple drivers might use drm_mode_config_helper_suspend().
>>    *
>> - * Drivers that set the dirty callback on their framebuffer will get 
>> a shadow
>> - * fbdev buffer that is blitted onto the real buffer. This is done in 
>> order to
>> - * make deferred I/O work with all kinds of buffers. A shadow buffer 
>> can be
>> - * requested explicitly by setting struct 
>> drm_mode_config.prefer_shadow or
>> - * struct drm_mode_config.prefer_shadow_fbdev to true beforehand. 
>> This is
>> - * required to use generic fbdev emulation with SHMEM helpers.
>> + * In order to provide fixed mmap-able memory ranges,
> 
> I don't understand here, what do you mean about *fixed*?
> 
> fixed relative to what? Can you say more?
> 

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

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

Re: [v2,1/8] drm/fbdev-generic: Always use shadow buffering

[Sui Jingfeng]

Tested-by: Sui Jingfeng <suijingfeng@loongson.cn>

On 2023/3/20 23:07, Thomas Zimmermann wrote:
> Remove all codepaths that implement fbdev output directly on GEM
> buffers. Always allocate a shadow buffer in system memory and set
> up deferred I/O for mmap.
>
> The fbdev code that operated directly on GEM buffers was used by
> drivers based on GEM DMA helpers. Those drivers have been migrated
> to use fbdev-dma, a dedicated fbdev emulation for DMA memory. All
> remaining users of fbdev-generic require shadow buffering.
>
> Memory management of the remaining callers uses TTM, GEM SHMEM
> helpers or a variant of GEM DMA helpers that is incompatible with
> fbdev-dma. Therefore remove the unused codepaths from fbdev-generic
> and simplify the code.
>
> Using a shadow buffer with deferred I/O is probably the best case
> for most remaining callers. Some of the TTM-based drivers might
> benefit from a dedicated fbdev emulation that operates directly on
> the driver's video memory.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
> Acked-by: Zack Rusin <zackr@vmware.com>
> ---
>   drivers/gpu/drm/drm_fbdev_generic.c | 184 +++++-----------------------
>   1 file changed, 30 insertions(+), 154 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_fbdev_generic.c b/drivers/gpu/drm/drm_fbdev_generic.c
> index 4d6325e91565..e48a8e82378d 100644
> --- a/drivers/gpu/drm/drm_fbdev_generic.c
> +++ b/drivers/gpu/drm/drm_fbdev_generic.c
> @@ -11,16 +11,6 @@
>   
>   #include <drm/drm_fbdev_generic.h>
>   
> -static bool drm_fbdev_use_shadow_fb(struct drm_fb_helper *fb_helper)
> -{
> -	struct drm_device *dev = fb_helper->dev;
> -	struct drm_framebuffer *fb = fb_helper->fb;
> -
> -	return dev->mode_config.prefer_shadow_fbdev ||
> -	       dev->mode_config.prefer_shadow ||
> -	       fb->funcs->dirty;
> -}
> -
>   /* @user: 1=userspace, 0=fbcon */
>   static int drm_fbdev_fb_open(struct fb_info *info, int user)
>   {
> @@ -46,115 +36,33 @@ static int drm_fbdev_fb_release(struct fb_info *info, int user)
>   static void drm_fbdev_fb_destroy(struct fb_info *info)
>   {
>   	struct drm_fb_helper *fb_helper = info->par;
> -	void *shadow = NULL;
> +	void *shadow = info->screen_buffer;
>   
>   	if (!fb_helper->dev)
>   		return;
>   
> -	if (info->fbdefio)
> -		fb_deferred_io_cleanup(info);
> -	if (drm_fbdev_use_shadow_fb(fb_helper))
> -		shadow = info->screen_buffer;
> -
> +	fb_deferred_io_cleanup(info);
>   	drm_fb_helper_fini(fb_helper);
> -
> -	if (shadow)
> -		vfree(shadow);
> -	else if (fb_helper->buffer)
> -		drm_client_buffer_vunmap(fb_helper->buffer);
> -
> +	vfree(shadow);
>   	drm_client_framebuffer_delete(fb_helper->buffer);
> -	drm_client_release(&fb_helper->client);
>   
> +	drm_client_release(&fb_helper->client);
>   	drm_fb_helper_unprepare(fb_helper);
>   	kfree(fb_helper);
>   }
>   
> -static int drm_fbdev_fb_mmap(struct fb_info *info, struct vm_area_struct *vma)
> -{
> -	struct drm_fb_helper *fb_helper = info->par;
> -
> -	if (drm_fbdev_use_shadow_fb(fb_helper))
> -		return fb_deferred_io_mmap(info, vma);
> -	else if (fb_helper->dev->driver->gem_prime_mmap)
> -		return fb_helper->dev->driver->gem_prime_mmap(fb_helper->buffer->gem, vma);
> -	else
> -		return -ENODEV;
> -}
> -
> -static bool drm_fbdev_use_iomem(struct fb_info *info)
> -{
> -	struct drm_fb_helper *fb_helper = info->par;
> -	struct drm_client_buffer *buffer = fb_helper->buffer;
> -
> -	return !drm_fbdev_use_shadow_fb(fb_helper) && buffer->map.is_iomem;
> -}
> -
> -static ssize_t drm_fbdev_fb_read(struct fb_info *info, char __user *buf,
> -				 size_t count, loff_t *ppos)
> -{
> -	ssize_t ret;
> -
> -	if (drm_fbdev_use_iomem(info))
> -		ret = drm_fb_helper_cfb_read(info, buf, count, ppos);
> -	else
> -		ret = drm_fb_helper_sys_read(info, buf, count, ppos);
> -
> -	return ret;
> -}
> -
> -static ssize_t drm_fbdev_fb_write(struct fb_info *info, const char __user *buf,
> -				  size_t count, loff_t *ppos)
> -{
> -	ssize_t ret;
> -
> -	if (drm_fbdev_use_iomem(info))
> -		ret = drm_fb_helper_cfb_write(info, buf, count, ppos);
> -	else
> -		ret = drm_fb_helper_sys_write(info, buf, count, ppos);
> -
> -	return ret;
> -}
> -
> -static void drm_fbdev_fb_fillrect(struct fb_info *info,
> -				  const struct fb_fillrect *rect)
> -{
> -	if (drm_fbdev_use_iomem(info))
> -		drm_fb_helper_cfb_fillrect(info, rect);
> -	else
> -		drm_fb_helper_sys_fillrect(info, rect);
> -}
> -
> -static void drm_fbdev_fb_copyarea(struct fb_info *info,
> -				  const struct fb_copyarea *area)
> -{
> -	if (drm_fbdev_use_iomem(info))
> -		drm_fb_helper_cfb_copyarea(info, area);
> -	else
> -		drm_fb_helper_sys_copyarea(info, area);
> -}
> -
> -static void drm_fbdev_fb_imageblit(struct fb_info *info,
> -				   const struct fb_image *image)
> -{
> -	if (drm_fbdev_use_iomem(info))
> -		drm_fb_helper_cfb_imageblit(info, image);
> -	else
> -		drm_fb_helper_sys_imageblit(info, image);
> -}
> -
>   static const struct fb_ops drm_fbdev_fb_ops = {
>   	.owner		= THIS_MODULE,
> -	DRM_FB_HELPER_DEFAULT_OPS,
>   	.fb_open	= drm_fbdev_fb_open,
>   	.fb_release	= drm_fbdev_fb_release,
> +	.fb_read	= drm_fb_helper_sys_read,
> +	.fb_write	= drm_fb_helper_sys_write,
> +	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	= fb_deferred_io_mmap,
>   	.fb_destroy	= drm_fbdev_fb_destroy,
> -	.fb_mmap	= drm_fbdev_fb_mmap,
> -	.fb_read	= drm_fbdev_fb_read,
> -	.fb_write	= drm_fbdev_fb_write,
> -	.fb_fillrect	= drm_fbdev_fb_fillrect,
> -	.fb_copyarea	= drm_fbdev_fb_copyarea,
> -	.fb_imageblit	= drm_fbdev_fb_imageblit,
>   };
>   
>   /*
> @@ -169,7 +77,6 @@ static int drm_fbdev_fb_probe(struct drm_fb_helper *fb_helper,
>   	struct drm_framebuffer *fb;
>   	struct fb_info *info;
>   	u32 format;
> -	struct iosys_map map;
>   	int ret;
>   
>   	drm_dbg_kms(dev, "surface width(%d), height(%d) and bpp(%d)\n",
> @@ -197,44 +104,21 @@ static int drm_fbdev_fb_probe(struct drm_fb_helper *fb_helper,
>   
>   	drm_fb_helper_fill_info(info, fb_helper, sizes);
>   
> -	if (drm_fbdev_use_shadow_fb(fb_helper)) {
> -		info->screen_buffer = vzalloc(info->screen_size);
> -		if (!info->screen_buffer)
> -			return -ENOMEM;
> -		info->flags |= FBINFO_VIRTFB | FBINFO_READS_FAST;
> +	info->screen_buffer = vzalloc(info->screen_size);
> +	if (!info->screen_buffer)
> +		return -ENOMEM;
> +	info->flags |= FBINFO_VIRTFB | FBINFO_READS_FAST;
>   
> -		/* Set a default deferred I/O handler */
> -		fb_helper->fbdefio.delay = HZ / 20;
> -		fb_helper->fbdefio.deferred_io = drm_fb_helper_deferred_io;
> +	info->fix.smem_start = page_to_phys(vmalloc_to_page(info->screen_buffer));
>   
> -		info->fbdefio = &fb_helper->fbdefio;
> -		ret = fb_deferred_io_init(info);
> -		if (ret)
> -			return ret;
> -	} else {
> -		/* buffer is mapped for HW framebuffer */
> -		ret = drm_client_buffer_vmap(fb_helper->buffer, &map);
> -		if (ret)
> -			return ret;
> -		if (map.is_iomem) {
> -			info->screen_base = map.vaddr_iomem;
> -		} else {
> -			info->screen_buffer = map.vaddr;
> -			info->flags |= FBINFO_VIRTFB;
> -		}
> -
> -		/*
> -		 * Shamelessly leak the physical address to user-space. As
> -		 * page_to_phys() is undefined for I/O memory, warn in this
> -		 * case.
> -		 */
> -#if IS_ENABLED(CONFIG_DRM_FBDEV_LEAK_PHYS_SMEM)
> -		if (fb_helper->hint_leak_smem_start && info->fix.smem_start == 0 &&
> -		    !drm_WARN_ON_ONCE(dev, map.is_iomem))
> -			info->fix.smem_start =
> -				page_to_phys(virt_to_page(info->screen_buffer));
> -#endif
> -	}
> +	/* Set a default deferred I/O handler */
> +	fb_helper->fbdefio.delay = HZ / 20;
> +	fb_helper->fbdefio.deferred_io = drm_fb_helper_deferred_io;
> +
> +	info->fbdefio = &fb_helper->fbdefio;
> +	ret = fb_deferred_io_init(info);
> +	if (ret)
> +		return ret;
>   
>   	return 0;
>   }
> @@ -318,18 +202,13 @@ static int drm_fbdev_fb_dirty(struct drm_fb_helper *helper, struct drm_clip_rect
>   	struct drm_device *dev = helper->dev;
>   	int ret;
>   
> -	if (!drm_fbdev_use_shadow_fb(helper))
> -		return 0;
> -
>   	/* Call damage handlers only if necessary */
>   	if (!(clip->x1 < clip->x2 && clip->y1 < clip->y2))
>   		return 0;
>   
> -	if (helper->buffer) {
> -		ret = drm_fbdev_damage_blit(helper, clip);
> -		if (drm_WARN_ONCE(dev, ret, "Damage blitter failed: ret=%d\n", ret))
> -			return ret;
> -	}
> +	ret = drm_fbdev_damage_blit(helper, clip);
> +	if (drm_WARN_ONCE(dev, ret, "Damage blitter failed: ret=%d\n", ret))
> +		return ret;
>   
>   	if (helper->fb->funcs->dirty) {
>   		ret = helper->fb->funcs->dirty(helper->fb, NULL, 0, 0, clip, 1);
> @@ -415,12 +294,9 @@ static const struct drm_client_funcs drm_fbdev_client_funcs = {
>    * suspend/resume need to call drm_fb_helper_set_suspend_unlocked() themselves.
>    * Simple drivers might use drm_mode_config_helper_suspend().
>    *
> - * Drivers that set the dirty callback on their framebuffer will get a shadow
> - * fbdev buffer that is blitted onto the real buffer. This is done in order to
> - * make deferred I/O work with all kinds of buffers. A shadow buffer can be
> - * requested explicitly by setting struct drm_mode_config.prefer_shadow or
> - * struct drm_mode_config.prefer_shadow_fbdev to true beforehand. This is
> - * required to use generic fbdev emulation with SHMEM helpers.
> + * In order to provide fixed mmap-able memory ranges, generic fbdev emulation
> + * uses a shadow buffer in system memory. The implementation blits the shadow
> + * fbdev buffer onto the real buffer in regular intervals.
>    *
>    * This function is safe to call even when there are no connectors present.
>    * Setup will be retried on the next hotplug event.

[PATCH v2 2/8] drm/fbdev-generic: Remove unused prefer_shadow_fbdev flag

[Thomas Zimmermann]

Remove the flag prefer_shadow_fbdev from struct drm_mode_config.
Drivers set this flag to enable shadow buffering in the generic
fbdev emulation. Such shadow buffering is now mandatory, so the
flag is unused.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
Reviewed-by: Zack Rusin <zackr@vmware.com>
---
 drivers/gpu/drm/tiny/bochs.c        | 1 -
 drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 1 -
 include/drm/drm_mode_config.h       | 7 -------
 3 files changed, 9 deletions(-)

diff --git a/drivers/gpu/drm/tiny/bochs.c b/drivers/gpu/drm/tiny/bochs.c
index 024346054c70..d254679a136e 100644
--- a/drivers/gpu/drm/tiny/bochs.c
+++ b/drivers/gpu/drm/tiny/bochs.c
@@ -545,7 +545,6 @@ static int bochs_kms_init(struct bochs_device *bochs)
 
 	bochs->dev->mode_config.preferred_depth = 24;
 	bochs->dev->mode_config.prefer_shadow = 0;
-	bochs->dev->mode_config.prefer_shadow_fbdev = 1;
 	bochs->dev->mode_config.quirk_addfb_prefer_host_byte_order = true;
 
 	bochs->dev->mode_config.funcs = &bochs_mode_funcs;
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
index 84d6380b9895..5162a7a12792 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
@@ -2112,7 +2112,6 @@ int vmw_kms_init(struct vmw_private *dev_priv)
 	dev->mode_config.max_width = dev_priv->texture_max_width;
 	dev->mode_config.max_height = dev_priv->texture_max_height;
 	dev->mode_config.preferred_depth = dev_priv->assume_16bpp ? 16 : 32;
-	dev->mode_config.prefer_shadow_fbdev = !dev_priv->has_mob;
 
 	drm_mode_create_suggested_offset_properties(dev);
 	vmw_kms_create_hotplug_mode_update_property(dev_priv);
diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
index e5b053001d22..973119a9176b 100644
--- a/include/drm/drm_mode_config.h
+++ b/include/drm/drm_mode_config.h
@@ -890,13 +890,6 @@ struct drm_mode_config {
 	/* dumb ioctl parameters */
 	uint32_t preferred_depth, prefer_shadow;
 
-	/**
-	 * @prefer_shadow_fbdev:
-	 *
-	 * Hint to framebuffer emulation to prefer shadow-fb rendering.
-	 */
-	bool prefer_shadow_fbdev;
-
 	/**
 	 * @quirk_addfb_prefer_xbgr_30bpp:
 	 *
-- 
2.40.0

Re: [v2,2/8] drm/fbdev-generic: Remove unused prefer_shadow_fbdev flag

[Sui jingfeng]

Well, this patch looks good to me.

Tested-by: Sui Jingfeng <suijingfeng@loongson.cn>

On 2023/3/20 23:07, Thomas Zimmermann wrote:
> Remove the flag prefer_shadow_fbdev from struct drm_mode_config.
> Drivers set this flag to enable shadow buffering in the generic
> fbdev emulation. Such shadow buffering is now mandatory, so the
> flag is unused.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
> Reviewed-by: Zack Rusin <zackr@vmware.com>
> ---
>   drivers/gpu/drm/tiny/bochs.c        | 1 -
>   drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 1 -
>   include/drm/drm_mode_config.h       | 7 -------
>   3 files changed, 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/tiny/bochs.c b/drivers/gpu/drm/tiny/bochs.c
> index 024346054c70..d254679a136e 100644
> --- a/drivers/gpu/drm/tiny/bochs.c
> +++ b/drivers/gpu/drm/tiny/bochs.c
> @@ -545,7 +545,6 @@ static int bochs_kms_init(struct bochs_device *bochs)
>   
>   	bochs->dev->mode_config.preferred_depth = 24;
>   	bochs->dev->mode_config.prefer_shadow = 0;
> -	bochs->dev->mode_config.prefer_shadow_fbdev = 1;
>   	bochs->dev->mode_config.quirk_addfb_prefer_host_byte_order = true;
>   
>   	bochs->dev->mode_config.funcs = &bochs_mode_funcs;
> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> index 84d6380b9895..5162a7a12792 100644
> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
> @@ -2112,7 +2112,6 @@ int vmw_kms_init(struct vmw_private *dev_priv)
>   	dev->mode_config.max_width = dev_priv->texture_max_width;
>   	dev->mode_config.max_height = dev_priv->texture_max_height;
>   	dev->mode_config.preferred_depth = dev_priv->assume_16bpp ? 16 : 32;
> -	dev->mode_config.prefer_shadow_fbdev = !dev_priv->has_mob;
>   
>   	drm_mode_create_suggested_offset_properties(dev);
>   	vmw_kms_create_hotplug_mode_update_property(dev_priv);
> diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
> index e5b053001d22..973119a9176b 100644
> --- a/include/drm/drm_mode_config.h
> +++ b/include/drm/drm_mode_config.h
> @@ -890,13 +890,6 @@ struct drm_mode_config {
>   	/* dumb ioctl parameters */
>   	uint32_t preferred_depth, prefer_shadow;
>   
> -	/**
> -	 * @prefer_shadow_fbdev:
> -	 *
> -	 * Hint to framebuffer emulation to prefer shadow-fb rendering.
> -	 */
> -	bool prefer_shadow_fbdev;
> -
>   	/**
>   	 * @quirk_addfb_prefer_xbgr_30bpp:
>   	 *

[PATCH v2 3/8] drm/fb-helper: Export drm_fb_helper_release_info()

[Thomas Zimmermann]

Export the fb_info release code as drm_fb_helper_release_info(). Will
help with cleaning up failed fbdev probing.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
Acked-by: Zack Rusin <zackr@vmware.com>
---
 drivers/gpu/drm/drm_fb_helper.c | 33 ++++++++++++++++++++++++---------
 include/drm/drm_fb_helper.h     |  5 +++++
 2 files changed, 29 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index a39998047f8a..7e96ed9efdb5 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -538,6 +538,29 @@ struct fb_info *drm_fb_helper_alloc_info(struct drm_fb_helper *fb_helper)
 }
 EXPORT_SYMBOL(drm_fb_helper_alloc_info);
 
+/**
+ * drm_fb_helper_release_info - release fb_info and its members
+ * @fb_helper: driver-allocated fbdev helper
+ *
+ * A helper to release fb_info and the member cmap.  Drivers do not
+ * need to release the allocated fb_info structure themselves, this is
+ * automatically done when calling drm_fb_helper_fini().
+ */
+void drm_fb_helper_release_info(struct drm_fb_helper *fb_helper)
+{
+	struct fb_info *info = fb_helper->info;
+
+	if (!info)
+		return;
+
+	fb_helper->info = NULL;
+
+	if (info->cmap.len)
+		fb_dealloc_cmap(&info->cmap);
+	framebuffer_release(info);
+}
+EXPORT_SYMBOL(drm_fb_helper_release_info);
+
 /**
  * drm_fb_helper_unregister_info - unregister fb_info framebuffer device
  * @fb_helper: driver-allocated fbdev helper, can be NULL
@@ -561,8 +584,6 @@ EXPORT_SYMBOL(drm_fb_helper_unregister_info);
  */
 void drm_fb_helper_fini(struct drm_fb_helper *fb_helper)
 {
-	struct fb_info *info;
-
 	if (!fb_helper)
 		return;
 
@@ -574,13 +595,7 @@ void drm_fb_helper_fini(struct drm_fb_helper *fb_helper)
 	cancel_work_sync(&fb_helper->resume_work);
 	cancel_work_sync(&fb_helper->damage_work);
 
-	info = fb_helper->info;
-	if (info) {
-		if (info->cmap.len)
-			fb_dealloc_cmap(&info->cmap);
-		framebuffer_release(info);
-	}
-	fb_helper->info = NULL;
+	drm_fb_helper_release_info(fb_helper);
 
 	mutex_lock(&kernel_fb_helper_lock);
 	if (!list_empty(&fb_helper->kernel_fb_list)) {
diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
index 013654de3fc5..c5822ec2fdd1 100644
--- a/include/drm/drm_fb_helper.h
+++ b/include/drm/drm_fb_helper.h
@@ -256,6 +256,7 @@ int drm_fb_helper_check_var(struct fb_var_screeninfo *var,
 int drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper);
 
 struct fb_info *drm_fb_helper_alloc_info(struct drm_fb_helper *fb_helper);
+void drm_fb_helper_release_info(struct drm_fb_helper *fb_helper);
 void drm_fb_helper_unregister_info(struct drm_fb_helper *fb_helper);
 void drm_fb_helper_fill_info(struct fb_info *info,
 			     struct drm_fb_helper *fb_helper,
@@ -365,6 +366,10 @@ drm_fb_helper_alloc_info(struct drm_fb_helper *fb_helper)
 	return NULL;
 }
 
+static inline void drm_fb_helper_release_info(struct drm_fb_helper *fb_helper)
+{
+}
+
 static inline void drm_fb_helper_unregister_info(struct drm_fb_helper *fb_helper)
 {
 }
-- 
2.40.0

Re: [v2,3/8] drm/fb-helper: Export drm_fb_helper_release_info()

[Sui jingfeng]

Tested-by: Sui Jingfeng<suijingfeng@loongson.cn>

On 2023/3/20 23:07, Thomas Zimmermann wrote:
> Export the fb_info release code as drm_fb_helper_release_info(). Will
> help with cleaning up failed fbdev probing.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
> Acked-by: Zack Rusin <zackr@vmware.com>
> ---
>   drivers/gpu/drm/drm_fb_helper.c | 33 ++++++++++++++++++++++++---------
>   include/drm/drm_fb_helper.h     |  5 +++++
>   2 files changed, 29 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index a39998047f8a..7e96ed9efdb5 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -538,6 +538,29 @@ struct fb_info *drm_fb_helper_alloc_info(struct drm_fb_helper *fb_helper)
>   }
>   EXPORT_SYMBOL(drm_fb_helper_alloc_info);
>   
> +/**
> + * drm_fb_helper_release_info - release fb_info and its members
> + * @fb_helper: driver-allocated fbdev helper
> + *
> + * A helper to release fb_info and the member cmap.  Drivers do not
> + * need to release the allocated fb_info structure themselves, this is
> + * automatically done when calling drm_fb_helper_fini().
> + */
> +void drm_fb_helper_release_info(struct drm_fb_helper *fb_helper)
> +{
> +	struct fb_info *info = fb_helper->info;
> +
> +	if (!info)
> +		return;
> +
> +	fb_helper->info = NULL;
> +
> +	if (info->cmap.len)
> +		fb_dealloc_cmap(&info->cmap);
> +	framebuffer_release(info);
> +}
> +EXPORT_SYMBOL(drm_fb_helper_release_info);
> +
>   /**
>    * drm_fb_helper_unregister_info - unregister fb_info framebuffer device
>    * @fb_helper: driver-allocated fbdev helper, can be NULL
> @@ -561,8 +584,6 @@ EXPORT_SYMBOL(drm_fb_helper_unregister_info);
>    */
>   void drm_fb_helper_fini(struct drm_fb_helper *fb_helper)
>   {
> -	struct fb_info *info;
> -
>   	if (!fb_helper)
>   		return;
>   
> @@ -574,13 +595,7 @@ void drm_fb_helper_fini(struct drm_fb_helper *fb_helper)
>   	cancel_work_sync(&fb_helper->resume_work);
>   	cancel_work_sync(&fb_helper->damage_work);
>   
> -	info = fb_helper->info;
> -	if (info) {
> -		if (info->cmap.len)
> -			fb_dealloc_cmap(&info->cmap);
> -		framebuffer_release(info);
> -	}
> -	fb_helper->info = NULL;
> +	drm_fb_helper_release_info(fb_helper);
>   
>   	mutex_lock(&kernel_fb_helper_lock);
>   	if (!list_empty(&fb_helper->kernel_fb_list)) {
> diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
> index 013654de3fc5..c5822ec2fdd1 100644
> --- a/include/drm/drm_fb_helper.h
> +++ b/include/drm/drm_fb_helper.h
> @@ -256,6 +256,7 @@ int drm_fb_helper_check_var(struct fb_var_screeninfo *var,
>   int drm_fb_helper_restore_fbdev_mode_unlocked(struct drm_fb_helper *fb_helper);
>   
>   struct fb_info *drm_fb_helper_alloc_info(struct drm_fb_helper *fb_helper);
> +void drm_fb_helper_release_info(struct drm_fb_helper *fb_helper);
>   void drm_fb_helper_unregister_info(struct drm_fb_helper *fb_helper);
>   void drm_fb_helper_fill_info(struct fb_info *info,
>   			     struct drm_fb_helper *fb_helper,
> @@ -365,6 +366,10 @@ drm_fb_helper_alloc_info(struct drm_fb_helper *fb_helper)
>   	return NULL;
>   }
>   
> +static inline void drm_fb_helper_release_info(struct drm_fb_helper *fb_helper)
> +{
> +}
> +
>   static inline void drm_fb_helper_unregister_info(struct drm_fb_helper *fb_helper)
>   {
>   }

[PATCH v2 4/8] drm/fb-helper: Support smem_len in deferred I/O

[Thomas Zimmermann]

The size of the framebuffer can either be stored in screen_info or
smem_len. Take both into account in the deferred I/O code.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/drm_fb_helper.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 7e96ed9efdb5..bb0b25209b3e 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -672,7 +672,7 @@ static void drm_fb_helper_memory_range_to_clip(struct fb_info *info, off_t off,
 void drm_fb_helper_deferred_io(struct fb_info *info, struct list_head *pagereflist)
 {
 	struct drm_fb_helper *helper = info->par;
-	unsigned long start, end, min_off, max_off;
+	unsigned long start, end, min_off, max_off, total_size;
 	struct fb_deferred_io_pageref *pageref;
 	struct drm_rect damage_area;
 
@@ -690,7 +690,11 @@ void drm_fb_helper_deferred_io(struct fb_info *info, struct list_head *pagerefli
 	 * of the screen and account for non-existing scanlines. Hence,
 	 * keep the covered memory area within the screen buffer.
 	 */
-	max_off = min(max_off, info->screen_size);
+	if (info->screen_size)
+		total_size = info->screen_size;
+	else
+		total_size = info->fix.smem_len;
+	max_off = min(max_off, total_size);
 
 	if (min_off < max_off) {
 		drm_fb_helper_memory_range_to_clip(info, min_off, max_off - min_off, &damage_area);
-- 
2.40.0

Re: [PATCH v2 4/8] drm/fb-helper: Support smem_len in deferred I/O

[Javier Martinez Canillas]

Thomas Zimmermann <tzimmermann@suse.de> writes:

> The size of the framebuffer can either be stored in screen_info or
> smem_len. Take both into account in the deferred I/O code.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [v2,4/8] drm/fb-helper: Support smem_len in deferred I/O

[Sui Jingfeng]

Yet, better to clarify which one should be used, not two.

Tested-by: Sui Jingfeng <suijingfeng@loongson.cn>


On 2023/3/20 23:07, Thomas Zimmermann wrote:
> The size of the framebuffer can either be stored in screen_info or
> smem_len. Take both into account in the deferred I/O code.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
> ---
>   drivers/gpu/drm/drm_fb_helper.c | 8 ++++++--
>   1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 7e96ed9efdb5..bb0b25209b3e 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -672,7 +672,7 @@ static void drm_fb_helper_memory_range_to_clip(struct fb_info *info, off_t off,
>   void drm_fb_helper_deferred_io(struct fb_info *info, struct list_head *pagereflist)
>   {
>   	struct drm_fb_helper *helper = info->par;
> -	unsigned long start, end, min_off, max_off;
> +	unsigned long start, end, min_off, max_off, total_size;
>   	struct fb_deferred_io_pageref *pageref;
>   	struct drm_rect damage_area;
>   
> @@ -690,7 +690,11 @@ void drm_fb_helper_deferred_io(struct fb_info *info, struct list_head *pagerefli
>   	 * of the screen and account for non-existing scanlines. Hence,
>   	 * keep the covered memory area within the screen buffer.
>   	 */
> -	max_off = min(max_off, info->screen_size);
> +	if (info->screen_size)
> +		total_size = info->screen_size;
> +	else
> +		total_size = info->fix.smem_len;
> +	max_off = min(max_off, total_size);
>   
>   	if (min_off < max_off) {
>   		drm_fb_helper_memory_range_to_clip(info, min_off, max_off - min_off, &damage_area);

[PATCH v2 5/8] drm/fbdev-generic: Set screen size to size of GEM buffer

[Thomas Zimmermann]

The size of the screen memory should be equivalent to the size of
the screen's GEM buffer. Don't recalculate the value.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/drm_fbdev_generic.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/drm_fbdev_generic.c b/drivers/gpu/drm/drm_fbdev_generic.c
index e48a8e82378d..73834a3cc6b0 100644
--- a/drivers/gpu/drm/drm_fbdev_generic.c
+++ b/drivers/gpu/drm/drm_fbdev_generic.c
@@ -7,6 +7,7 @@
 #include <drm/drm_drv.h>
 #include <drm/drm_fb_helper.h>
 #include <drm/drm_framebuffer.h>
+#include <drm/drm_gem.h>
 #include <drm/drm_print.h>
 
 #include <drm/drm_fbdev_generic.h>
@@ -74,8 +75,8 @@ static int drm_fbdev_fb_probe(struct drm_fb_helper *fb_helper,
 	struct drm_client_dev *client = &fb_helper->client;
 	struct drm_device *dev = fb_helper->dev;
 	struct drm_client_buffer *buffer;
-	struct drm_framebuffer *fb;
 	struct fb_info *info;
+	size_t screen_size;
 	u32 format;
 	int ret;
 
@@ -91,20 +92,20 @@ static int drm_fbdev_fb_probe(struct drm_fb_helper *fb_helper,
 
 	fb_helper->buffer = buffer;
 	fb_helper->fb = buffer->fb;
-	fb = buffer->fb;
+	screen_size = buffer->gem->size;
 
 	info = drm_fb_helper_alloc_info(fb_helper);
 	if (IS_ERR(info))
 		return PTR_ERR(info);
 
 	info->fbops = &drm_fbdev_fb_ops;
-	info->screen_size = sizes->surface_height * fb->pitches[0];
-	info->fix.smem_len = info->screen_size;
+	info->screen_size = screen_size;
+	info->fix.smem_len = screen_size;
 	info->flags = FBINFO_DEFAULT;
 
 	drm_fb_helper_fill_info(info, fb_helper, sizes);
 
-	info->screen_buffer = vzalloc(info->screen_size);
+	info->screen_buffer = vzalloc(screen_size);
 	if (!info->screen_buffer)
 		return -ENOMEM;
 	info->flags |= FBINFO_VIRTFB | FBINFO_READS_FAST;
-- 
2.40.0

Re: [PATCH v2 5/8] drm/fbdev-generic: Set screen size to size of GEM buffer

[Javier Martinez Canillas]

Thomas Zimmermann <tzimmermann@suse.de> writes:

> The size of the screen memory should be equivalent to the size of
> the screen's GEM buffer. Don't recalculate the value.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [v2, 5/8] drm/fbdev-generic: Set screen size to size of GEM buffer

[Sui Jingfeng]

Tested-by: Sui Jingfeng <suijingfeng@loongson.cn>

On 2023/3/20 23:07, Thomas Zimmermann wrote:
> The size of the screen memory should be equivalent to the size of
> the screen's GEM buffer. Don't recalculate the value.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
> ---
>   drivers/gpu/drm/drm_fbdev_generic.c | 11 ++++++-----
>   1 file changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_fbdev_generic.c b/drivers/gpu/drm/drm_fbdev_generic.c
> index e48a8e82378d..73834a3cc6b0 100644
> --- a/drivers/gpu/drm/drm_fbdev_generic.c
> +++ b/drivers/gpu/drm/drm_fbdev_generic.c
> @@ -7,6 +7,7 @@
>   #include <drm/drm_drv.h>
>   #include <drm/drm_fb_helper.h>
>   #include <drm/drm_framebuffer.h>
> +#include <drm/drm_gem.h>
>   #include <drm/drm_print.h>
>   
>   #include <drm/drm_fbdev_generic.h>
> @@ -74,8 +75,8 @@ static int drm_fbdev_fb_probe(struct drm_fb_helper *fb_helper,
>   	struct drm_client_dev *client = &fb_helper->client;
>   	struct drm_device *dev = fb_helper->dev;
>   	struct drm_client_buffer *buffer;
> -	struct drm_framebuffer *fb;
>   	struct fb_info *info;
> +	size_t screen_size;
>   	u32 format;
>   	int ret;
>   
> @@ -91,20 +92,20 @@ static int drm_fbdev_fb_probe(struct drm_fb_helper *fb_helper,
>   
>   	fb_helper->buffer = buffer;
>   	fb_helper->fb = buffer->fb;
> -	fb = buffer->fb;
> +	screen_size = buffer->gem->size;
>   
>   	info = drm_fb_helper_alloc_info(fb_helper);
>   	if (IS_ERR(info))
>   		return PTR_ERR(info);
>   
>   	info->fbops = &drm_fbdev_fb_ops;
> -	info->screen_size = sizes->surface_height * fb->pitches[0];
> -	info->fix.smem_len = info->screen_size;
> +	info->screen_size = screen_size;
> +	info->fix.smem_len = screen_size;
>   	info->flags = FBINFO_DEFAULT;
>   
>   	drm_fb_helper_fill_info(info, fb_helper, sizes);
>   
> -	info->screen_buffer = vzalloc(info->screen_size);
> +	info->screen_buffer = vzalloc(screen_size);
>   	if (!info->screen_buffer)
>   		return -ENOMEM;
>   	info->flags |= FBINFO_VIRTFB | FBINFO_READS_FAST;

[PATCH v2 6/8] drm/fbdev-generic: Clean up after failed probing

[Thomas Zimmermann]

Clean up fbdev and client state if the probe function fails. It
used to leak allocated resources. Also reorder the individual steps
to simplify cleanup.

v2:
	* move screen_size update into separate patches

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
Acked-by: Zack Rusin <zackr@vmware.com>
---
 drivers/gpu/drm/drm_fbdev_generic.c | 40 ++++++++++++++++++++---------
 1 file changed, 28 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/drm_fbdev_generic.c b/drivers/gpu/drm/drm_fbdev_generic.c
index 73834a3cc6b0..e7eeba0c44b4 100644
--- a/drivers/gpu/drm/drm_fbdev_generic.c
+++ b/drivers/gpu/drm/drm_fbdev_generic.c
@@ -77,6 +77,7 @@ static int drm_fbdev_fb_probe(struct drm_fb_helper *fb_helper,
 	struct drm_client_buffer *buffer;
 	struct fb_info *info;
 	size_t screen_size;
+	void *screen_buffer;
 	u32 format;
 	int ret;
 
@@ -92,36 +93,51 @@ static int drm_fbdev_fb_probe(struct drm_fb_helper *fb_helper,
 
 	fb_helper->buffer = buffer;
 	fb_helper->fb = buffer->fb;
+
 	screen_size = buffer->gem->size;
+	screen_buffer = vzalloc(screen_size);
+	if (!screen_buffer) {
+		ret = -ENOMEM;
+		goto err_drm_client_framebuffer_delete;
+	}
 
 	info = drm_fb_helper_alloc_info(fb_helper);
-	if (IS_ERR(info))
-		return PTR_ERR(info);
+	if (IS_ERR(info)) {
+		ret = PTR_ERR(info);
+		goto err_vfree;
+	}
+
+	drm_fb_helper_fill_info(info, fb_helper, sizes);
 
 	info->fbops = &drm_fbdev_fb_ops;
-	info->screen_size = screen_size;
-	info->fix.smem_len = screen_size;
 	info->flags = FBINFO_DEFAULT;
 
-	drm_fb_helper_fill_info(info, fb_helper, sizes);
-
-	info->screen_buffer = vzalloc(screen_size);
-	if (!info->screen_buffer)
-		return -ENOMEM;
+	/* screen */
 	info->flags |= FBINFO_VIRTFB | FBINFO_READS_FAST;
-
+	info->screen_buffer = screen_buffer;
 	info->fix.smem_start = page_to_phys(vmalloc_to_page(info->screen_buffer));
+	info->fix.smem_len = screen_size;
 
-	/* Set a default deferred I/O handler */
+	/* deferred I/O */
 	fb_helper->fbdefio.delay = HZ / 20;
 	fb_helper->fbdefio.deferred_io = drm_fb_helper_deferred_io;
 
 	info->fbdefio = &fb_helper->fbdefio;
 	ret = fb_deferred_io_init(info);
 	if (ret)
-		return ret;
+		goto err_drm_fb_helper_release_info;
 
 	return 0;
+
+err_drm_fb_helper_release_info:
+	drm_fb_helper_release_info(fb_helper);
+err_vfree:
+	vfree(screen_buffer);
+err_drm_client_framebuffer_delete:
+	fb_helper->fb = NULL;
+	fb_helper->buffer = NULL;
+	drm_client_framebuffer_delete(buffer);
+	return ret;
 }
 
 static void drm_fbdev_damage_blit_real(struct drm_fb_helper *fb_helper,
-- 
2.40.0

Re: [v2,6/8] drm/fbdev-generic: Clean up after failed probing

[Sui Jingfeng]

Tested-by: Sui Jingfeng <suijingfeng@loongson.cn>

On 2023/3/20 23:07, Thomas Zimmermann wrote:
> Clean up fbdev and client state if the probe function fails. It
> used to leak allocated resources. Also reorder the individual steps
> to simplify cleanup.
>
> v2:
> 	* move screen_size update into separate patches
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
> Acked-by: Zack Rusin <zackr@vmware.com>
> ---
>   drivers/gpu/drm/drm_fbdev_generic.c | 40 ++++++++++++++++++++---------
>   1 file changed, 28 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_fbdev_generic.c b/drivers/gpu/drm/drm_fbdev_generic.c
> index 73834a3cc6b0..e7eeba0c44b4 100644
> --- a/drivers/gpu/drm/drm_fbdev_generic.c
> +++ b/drivers/gpu/drm/drm_fbdev_generic.c
> @@ -77,6 +77,7 @@ static int drm_fbdev_fb_probe(struct drm_fb_helper *fb_helper,
>   	struct drm_client_buffer *buffer;
>   	struct fb_info *info;
>   	size_t screen_size;
> +	void *screen_buffer;
>   	u32 format;
>   	int ret;
>   
> @@ -92,36 +93,51 @@ static int drm_fbdev_fb_probe(struct drm_fb_helper *fb_helper,
>   
>   	fb_helper->buffer = buffer;
>   	fb_helper->fb = buffer->fb;
> +
>   	screen_size = buffer->gem->size;
> +	screen_buffer = vzalloc(screen_size);
> +	if (!screen_buffer) {
> +		ret = -ENOMEM;
> +		goto err_drm_client_framebuffer_delete;
> +	}
>   
>   	info = drm_fb_helper_alloc_info(fb_helper);
> -	if (IS_ERR(info))
> -		return PTR_ERR(info);
> +	if (IS_ERR(info)) {
> +		ret = PTR_ERR(info);
> +		goto err_vfree;
> +	}
> +
> +	drm_fb_helper_fill_info(info, fb_helper, sizes);
>   
>   	info->fbops = &drm_fbdev_fb_ops;
> -	info->screen_size = screen_size;
> -	info->fix.smem_len = screen_size;
>   	info->flags = FBINFO_DEFAULT;
>   
> -	drm_fb_helper_fill_info(info, fb_helper, sizes);
> -
> -	info->screen_buffer = vzalloc(screen_size);
> -	if (!info->screen_buffer)
> -		return -ENOMEM;
> +	/* screen */
>   	info->flags |= FBINFO_VIRTFB | FBINFO_READS_FAST;
> -
> +	info->screen_buffer = screen_buffer;
>   	info->fix.smem_start = page_to_phys(vmalloc_to_page(info->screen_buffer));
> +	info->fix.smem_len = screen_size;
>   
> -	/* Set a default deferred I/O handler */
> +	/* deferred I/O */
>   	fb_helper->fbdefio.delay = HZ / 20;
>   	fb_helper->fbdefio.deferred_io = drm_fb_helper_deferred_io;
>   
>   	info->fbdefio = &fb_helper->fbdefio;
>   	ret = fb_deferred_io_init(info);
>   	if (ret)
> -		return ret;
> +		goto err_drm_fb_helper_release_info;
>   
>   	return 0;
> +
> +err_drm_fb_helper_release_info:
> +	drm_fb_helper_release_info(fb_helper);
> +err_vfree:
> +	vfree(screen_buffer);
> +err_drm_client_framebuffer_delete:
> +	fb_helper->fb = NULL;
> +	fb_helper->buffer = NULL;
> +	drm_client_framebuffer_delete(buffer);
> +	return ret;
>   }
>   
>   static void drm_fbdev_damage_blit_real(struct drm_fb_helper *fb_helper,

[PATCH v2 7/8] drm/fb-helper: Consolidate CONFIG_DRM_FBDEV_LEAK_PHYS_SMEM

[Thomas Zimmermann]

Consolidate all handling of CONFIG_DRM_FBDEV_LEAK_PHYS_SMEM by
making the module parameter optional in drm_fb_helper.c.

Without the config option, modules can set smem_start in struct
fb_info for internal usage, but not export if to userspace. The
address can only be exported by enabling the option and setting
the module parameter. Also update the comment.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
Acked-by: Zack Rusin <zackr@vmware.com>
---
 drivers/gpu/drm/drm_fb_helper.c | 22 ++++++++--------------
 drivers/gpu/drm/drm_fbdev_dma.c |  9 +--------
 include/drm/drm_fb_helper.h     |  9 ---------
 3 files changed, 9 insertions(+), 31 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index bb0b25209b3e..63ec95e86d0e 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -60,16 +60,17 @@ MODULE_PARM_DESC(drm_fbdev_overalloc,
  * In order to keep user-space compatibility, we want in certain use-cases
  * to keep leaking the fbdev physical address to the user-space program
  * handling the fbdev buffer.
- * This is a bad habit essentially kept into closed source opengl driver
- * that should really be moved into open-source upstream projects instead
- * of using legacy physical addresses in user space to communicate with
- * other out-of-tree kernel modules.
+ *
+ * This is a bad habit, essentially kept to support closed-source OpenGL
+ * drivers that should really be moved into open-source upstream projects
+ * instead of using legacy physical addresses in user space to communicate
+ * with other out-of-tree kernel modules.
  *
  * This module_param *should* be removed as soon as possible and be
  * considered as a broken and legacy behaviour from a modern fbdev device.
  */
-#if IS_ENABLED(CONFIG_DRM_FBDEV_LEAK_PHYS_SMEM)
 static bool drm_leak_fbdev_smem;
+#if IS_ENABLED(CONFIG_DRM_FBDEV_LEAK_PHYS_SMEM)
 module_param_unsafe(drm_leak_fbdev_smem, bool, 0600);
 MODULE_PARM_DESC(drm_leak_fbdev_smem,
 		 "Allow unsafe leaking fbdev physical smem address [default=false]");
@@ -1983,10 +1984,6 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper)
 		return ret;
 	}
 
-#if IS_ENABLED(CONFIG_DRM_FBDEV_LEAK_PHYS_SMEM)
-	fb_helper->hint_leak_smem_start = drm_leak_fbdev_smem;
-#endif
-
 	/* push down into drivers */
 	ret = (*fb_helper->funcs->fb_probe)(fb_helper, &sizes);
 	if (ret < 0)
@@ -2185,11 +2182,8 @@ __drm_fb_helper_initial_config_and_unlock(struct drm_fb_helper *fb_helper)
 
 	info = fb_helper->info;
 	info->var.pixclock = 0;
-	/* Shamelessly allow physical address leaking to userspace */
-#if IS_ENABLED(CONFIG_DRM_FBDEV_LEAK_PHYS_SMEM)
-	if (!fb_helper->hint_leak_smem_start)
-#endif
-		/* don't leak any physical addresses to userspace */
+
+	if (!drm_leak_fbdev_smem)
 		info->flags |= FBINFO_HIDE_SMEM_START;
 
 	/* Need to drop locks to avoid recursive deadlock in
diff --git a/drivers/gpu/drm/drm_fbdev_dma.c b/drivers/gpu/drm/drm_fbdev_dma.c
index cf553ac12a0f..728deffcc0d9 100644
--- a/drivers/gpu/drm/drm_fbdev_dma.c
+++ b/drivers/gpu/drm/drm_fbdev_dma.c
@@ -136,16 +136,9 @@ static int drm_fbdev_dma_helper_fb_probe(struct drm_fb_helper *fb_helper,
 		info->flags |= FBINFO_READS_FAST; /* signal caching */
 	info->screen_size = sizes->surface_height * fb->pitches[0];
 	info->screen_buffer = map.vaddr;
+	info->fix.smem_start = page_to_phys(virt_to_page(info->screen_buffer));
 	info->fix.smem_len = info->screen_size;
 
-#if IS_ENABLED(CONFIG_DRM_FBDEV_LEAK_PHYS_SMEM)
-	/*
-	 * Shamelessly leak the physical address to user-space.
-	 */
-	if (fb_helper->hint_leak_smem_start && !info->fix.smem_start)
-		info->fix.smem_start = page_to_phys(virt_to_page(info->screen_buffer));
-#endif
-
 	return 0;
 
 err_drm_client_buffer_vunmap:
diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
index c5822ec2fdd1..72032c354a30 100644
--- a/include/drm/drm_fb_helper.h
+++ b/include/drm/drm_fb_helper.h
@@ -195,15 +195,6 @@ struct drm_fb_helper {
 	 */
 	int preferred_bpp;
 
-	/**
-	 * @hint_leak_smem_start:
-	 *
-	 * Hint to the fbdev emulation to store the framebuffer's physical
-	 * address in struct &fb_info.fix.smem_start. If the hint is unset,
-	 * the smem_start field should always be cleared to zero.
-	 */
-	bool hint_leak_smem_start;
-
 #ifdef CONFIG_FB_DEFERRED_IO
 	/**
 	 * @fbdefio:
-- 
2.40.0

Re: [v2,7/8] drm/fb-helper: Consolidate CONFIG_DRM_FBDEV_LEAK_PHYS_SMEM

[Sui jingfeng]

Tested-by: Sui Jingfeng<suijingfeng@loongson.cn>

On 2023/3/20 23:07, Thomas Zimmermann wrote:
> Consolidate all handling of CONFIG_DRM_FBDEV_LEAK_PHYS_SMEM by
> making the module parameter optional in drm_fb_helper.c.
>
> Without the config option, modules can set smem_start in struct
> fb_info for internal usage, but not export if to userspace. The
> address can only be exported by enabling the option and setting
> the module parameter. Also update the comment.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
> Acked-by: Zack Rusin <zackr@vmware.com>
> ---
>   drivers/gpu/drm/drm_fb_helper.c | 22 ++++++++--------------
>   drivers/gpu/drm/drm_fbdev_dma.c |  9 +--------
>   include/drm/drm_fb_helper.h     |  9 ---------
>   3 files changed, 9 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index bb0b25209b3e..63ec95e86d0e 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -60,16 +60,17 @@ MODULE_PARM_DESC(drm_fbdev_overalloc,
>    * In order to keep user-space compatibility, we want in certain use-cases
>    * to keep leaking the fbdev physical address to the user-space program
>    * handling the fbdev buffer.
> - * This is a bad habit essentially kept into closed source opengl driver
> - * that should really be moved into open-source upstream projects instead
> - * of using legacy physical addresses in user space to communicate with
> - * other out-of-tree kernel modules.
> + *
> + * This is a bad habit, essentially kept to support closed-source OpenGL
> + * drivers that should really be moved into open-source upstream projects
> + * instead of using legacy physical addresses in user space to communicate
> + * with other out-of-tree kernel modules.
>    *
>    * This module_param *should* be removed as soon as possible and be
>    * considered as a broken and legacy behaviour from a modern fbdev device.
>    */
> -#if IS_ENABLED(CONFIG_DRM_FBDEV_LEAK_PHYS_SMEM)
>   static bool drm_leak_fbdev_smem;
> +#if IS_ENABLED(CONFIG_DRM_FBDEV_LEAK_PHYS_SMEM)
>   module_param_unsafe(drm_leak_fbdev_smem, bool, 0600);
>   MODULE_PARM_DESC(drm_leak_fbdev_smem,
>   		 "Allow unsafe leaking fbdev physical smem address [default=false]");
> @@ -1983,10 +1984,6 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper)
>   		return ret;
>   	}
>   
> -#if IS_ENABLED(CONFIG_DRM_FBDEV_LEAK_PHYS_SMEM)
> -	fb_helper->hint_leak_smem_start = drm_leak_fbdev_smem;
> -#endif
> -
>   	/* push down into drivers */
>   	ret = (*fb_helper->funcs->fb_probe)(fb_helper, &sizes);
>   	if (ret < 0)
> @@ -2185,11 +2182,8 @@ __drm_fb_helper_initial_config_and_unlock(struct drm_fb_helper *fb_helper)
>   
>   	info = fb_helper->info;
>   	info->var.pixclock = 0;
> -	/* Shamelessly allow physical address leaking to userspace */
> -#if IS_ENABLED(CONFIG_DRM_FBDEV_LEAK_PHYS_SMEM)
> -	if (!fb_helper->hint_leak_smem_start)
> -#endif
> -		/* don't leak any physical addresses to userspace */
> +
> +	if (!drm_leak_fbdev_smem)
>   		info->flags |= FBINFO_HIDE_SMEM_START;
>   
>   	/* Need to drop locks to avoid recursive deadlock in
> diff --git a/drivers/gpu/drm/drm_fbdev_dma.c b/drivers/gpu/drm/drm_fbdev_dma.c
> index cf553ac12a0f..728deffcc0d9 100644
> --- a/drivers/gpu/drm/drm_fbdev_dma.c
> +++ b/drivers/gpu/drm/drm_fbdev_dma.c
> @@ -136,16 +136,9 @@ static int drm_fbdev_dma_helper_fb_probe(struct drm_fb_helper *fb_helper,
>   		info->flags |= FBINFO_READS_FAST; /* signal caching */
>   	info->screen_size = sizes->surface_height * fb->pitches[0];
>   	info->screen_buffer = map.vaddr;
> +	info->fix.smem_start = page_to_phys(virt_to_page(info->screen_buffer));
>   	info->fix.smem_len = info->screen_size;
>   
> -#if IS_ENABLED(CONFIG_DRM_FBDEV_LEAK_PHYS_SMEM)
> -	/*
> -	 * Shamelessly leak the physical address to user-space.
> -	 */
> -	if (fb_helper->hint_leak_smem_start && !info->fix.smem_start)
> -		info->fix.smem_start = page_to_phys(virt_to_page(info->screen_buffer));
> -#endif
> -
>   	return 0;
>   
>   err_drm_client_buffer_vunmap:
> diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
> index c5822ec2fdd1..72032c354a30 100644
> --- a/include/drm/drm_fb_helper.h
> +++ b/include/drm/drm_fb_helper.h
> @@ -195,15 +195,6 @@ struct drm_fb_helper {
>   	 */
>   	int preferred_bpp;
>   
> -	/**
> -	 * @hint_leak_smem_start:
> -	 *
> -	 * Hint to the fbdev emulation to store the framebuffer's physical
> -	 * address in struct &fb_info.fix.smem_start. If the hint is unset,
> -	 * the smem_start field should always be cleared to zero.
> -	 */
> -	bool hint_leak_smem_start;
> -
>   #ifdef CONFIG_FB_DEFERRED_IO
>   	/**
>   	 * @fbdefio:

[PATCH v2 8/8] drm/fbdev-generic: Rename symbols

[Thomas Zimmermann]

Rename symbols to match the style of other fbdev-emulation source
code. No functional changes.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
Acked-by: Zack Rusin <zackr@vmware.com>
---
 drivers/gpu/drm/drm_fbdev_generic.c | 66 ++++++++++++++---------------
 1 file changed, 33 insertions(+), 33 deletions(-)

diff --git a/drivers/gpu/drm/drm_fbdev_generic.c b/drivers/gpu/drm/drm_fbdev_generic.c
index e7eeba0c44b4..8e5148bf40bb 100644
--- a/drivers/gpu/drm/drm_fbdev_generic.c
+++ b/drivers/gpu/drm/drm_fbdev_generic.c
@@ -13,7 +13,7 @@
 #include <drm/drm_fbdev_generic.h>
 
 /* @user: 1=userspace, 0=fbcon */
-static int drm_fbdev_fb_open(struct fb_info *info, int user)
+static int drm_fbdev_generic_fb_open(struct fb_info *info, int user)
 {
 	struct drm_fb_helper *fb_helper = info->par;
 
@@ -24,7 +24,7 @@ static int drm_fbdev_fb_open(struct fb_info *info, int user)
 	return 0;
 }
 
-static int drm_fbdev_fb_release(struct fb_info *info, int user)
+static int drm_fbdev_generic_fb_release(struct fb_info *info, int user)
 {
 	struct drm_fb_helper *fb_helper = info->par;
 
@@ -34,7 +34,7 @@ static int drm_fbdev_fb_release(struct fb_info *info, int user)
 	return 0;
 }
 
-static void drm_fbdev_fb_destroy(struct fb_info *info)
+static void drm_fbdev_generic_fb_destroy(struct fb_info *info)
 {
 	struct drm_fb_helper *fb_helper = info->par;
 	void *shadow = info->screen_buffer;
@@ -52,10 +52,10 @@ static void drm_fbdev_fb_destroy(struct fb_info *info)
 	kfree(fb_helper);
 }
 
-static const struct fb_ops drm_fbdev_fb_ops = {
+static const struct fb_ops drm_fbdev_generic_fb_ops = {
 	.owner		= THIS_MODULE,
-	.fb_open	= drm_fbdev_fb_open,
-	.fb_release	= drm_fbdev_fb_release,
+	.fb_open	= drm_fbdev_generic_fb_open,
+	.fb_release	= drm_fbdev_generic_fb_release,
 	.fb_read	= drm_fb_helper_sys_read,
 	.fb_write	= drm_fb_helper_sys_write,
 	DRM_FB_HELPER_DEFAULT_OPS,
@@ -63,14 +63,14 @@ static const struct fb_ops drm_fbdev_fb_ops = {
 	.fb_copyarea	= drm_fb_helper_sys_copyarea,
 	.fb_imageblit	= drm_fb_helper_sys_imageblit,
 	.fb_mmap	= fb_deferred_io_mmap,
-	.fb_destroy	= drm_fbdev_fb_destroy,
+	.fb_destroy	= drm_fbdev_generic_fb_destroy,
 };
 
 /*
  * This function uses the client API to create a framebuffer backed by a dumb buffer.
  */
-static int drm_fbdev_fb_probe(struct drm_fb_helper *fb_helper,
-			      struct drm_fb_helper_surface_size *sizes)
+static int drm_fbdev_generic_helper_fb_probe(struct drm_fb_helper *fb_helper,
+					     struct drm_fb_helper_surface_size *sizes)
 {
 	struct drm_client_dev *client = &fb_helper->client;
 	struct drm_device *dev = fb_helper->dev;
@@ -109,7 +109,7 @@ static int drm_fbdev_fb_probe(struct drm_fb_helper *fb_helper,
 
 	drm_fb_helper_fill_info(info, fb_helper, sizes);
 
-	info->fbops = &drm_fbdev_fb_ops;
+	info->fbops = &drm_fbdev_generic_fb_ops;
 	info->flags = FBINFO_DEFAULT;
 
 	/* screen */
@@ -140,9 +140,9 @@ static int drm_fbdev_fb_probe(struct drm_fb_helper *fb_helper,
 	return ret;
 }
 
-static void drm_fbdev_damage_blit_real(struct drm_fb_helper *fb_helper,
-				       struct drm_clip_rect *clip,
-				       struct iosys_map *dst)
+static void drm_fbdev_generic_damage_blit_real(struct drm_fb_helper *fb_helper,
+					       struct drm_clip_rect *clip,
+					       struct iosys_map *dst)
 {
 	struct drm_framebuffer *fb = fb_helper->fb;
 	size_t offset = clip->y1 * fb->pitches[0];
@@ -179,8 +179,8 @@ static void drm_fbdev_damage_blit_real(struct drm_fb_helper *fb_helper,
 	}
 }
 
-static int drm_fbdev_damage_blit(struct drm_fb_helper *fb_helper,
-				 struct drm_clip_rect *clip)
+static int drm_fbdev_generic_damage_blit(struct drm_fb_helper *fb_helper,
+					 struct drm_clip_rect *clip)
 {
 	struct drm_client_buffer *buffer = fb_helper->buffer;
 	struct iosys_map map, dst;
@@ -204,7 +204,7 @@ static int drm_fbdev_damage_blit(struct drm_fb_helper *fb_helper,
 		goto out;
 
 	dst = map;
-	drm_fbdev_damage_blit_real(fb_helper, clip, &dst);
+	drm_fbdev_generic_damage_blit_real(fb_helper, clip, &dst);
 
 	drm_client_buffer_vunmap(buffer);
 
@@ -214,7 +214,8 @@ static int drm_fbdev_damage_blit(struct drm_fb_helper *fb_helper,
 	return ret;
 }
 
-static int drm_fbdev_fb_dirty(struct drm_fb_helper *helper, struct drm_clip_rect *clip)
+static int drm_fbdev_generic_helper_fb_dirty(struct drm_fb_helper *helper,
+					     struct drm_clip_rect *clip)
 {
 	struct drm_device *dev = helper->dev;
 	int ret;
@@ -223,7 +224,7 @@ static int drm_fbdev_fb_dirty(struct drm_fb_helper *helper, struct drm_clip_rect
 	if (!(clip->x1 < clip->x2 && clip->y1 < clip->y2))
 		return 0;
 
-	ret = drm_fbdev_damage_blit(helper, clip);
+	ret = drm_fbdev_generic_damage_blit(helper, clip);
 	if (drm_WARN_ONCE(dev, ret, "Damage blitter failed: ret=%d\n", ret))
 		return ret;
 
@@ -236,12 +237,12 @@ static int drm_fbdev_fb_dirty(struct drm_fb_helper *helper, struct drm_clip_rect
 	return 0;
 }
 
-static const struct drm_fb_helper_funcs drm_fb_helper_generic_funcs = {
-	.fb_probe = drm_fbdev_fb_probe,
-	.fb_dirty = drm_fbdev_fb_dirty,
+static const struct drm_fb_helper_funcs drm_fbdev_generic_helper_funcs = {
+	.fb_probe = drm_fbdev_generic_helper_fb_probe,
+	.fb_dirty = drm_fbdev_generic_helper_fb_dirty,
 };
 
-static void drm_fbdev_client_unregister(struct drm_client_dev *client)
+static void drm_fbdev_generic_client_unregister(struct drm_client_dev *client)
 {
 	struct drm_fb_helper *fb_helper = drm_fb_helper_from_client(client);
 
@@ -254,14 +255,14 @@ static void drm_fbdev_client_unregister(struct drm_client_dev *client)
 	}
 }
 
-static int drm_fbdev_client_restore(struct drm_client_dev *client)
+static int drm_fbdev_generic_client_restore(struct drm_client_dev *client)
 {
 	drm_fb_helper_lastclose(client->dev);
 
 	return 0;
 }
 
-static int drm_fbdev_client_hotplug(struct drm_client_dev *client)
+static int drm_fbdev_generic_client_hotplug(struct drm_client_dev *client)
 {
 	struct drm_fb_helper *fb_helper = drm_fb_helper_from_client(client);
 	struct drm_device *dev = client->dev;
@@ -290,11 +291,11 @@ static int drm_fbdev_client_hotplug(struct drm_client_dev *client)
 	return ret;
 }
 
-static const struct drm_client_funcs drm_fbdev_client_funcs = {
+static const struct drm_client_funcs drm_fbdev_generic_client_funcs = {
 	.owner		= THIS_MODULE,
-	.unregister	= drm_fbdev_client_unregister,
-	.restore	= drm_fbdev_client_restore,
-	.hotplug	= drm_fbdev_client_hotplug,
+	.unregister	= drm_fbdev_generic_client_unregister,
+	.restore	= drm_fbdev_generic_client_restore,
+	.hotplug	= drm_fbdev_generic_client_hotplug,
 };
 
 /**
@@ -320,8 +321,7 @@ static const struct drm_client_funcs drm_fbdev_client_funcs = {
  *
  * The fbdev is destroyed by drm_dev_unregister().
  */
-void drm_fbdev_generic_setup(struct drm_device *dev,
-			     unsigned int preferred_bpp)
+void drm_fbdev_generic_setup(struct drm_device *dev, unsigned int preferred_bpp)
 {
 	struct drm_fb_helper *fb_helper;
 	int ret;
@@ -332,15 +332,15 @@ void drm_fbdev_generic_setup(struct drm_device *dev,
 	fb_helper = kzalloc(sizeof(*fb_helper), GFP_KERNEL);
 	if (!fb_helper)
 		return;
-	drm_fb_helper_prepare(dev, fb_helper, preferred_bpp, &drm_fb_helper_generic_funcs);
+	drm_fb_helper_prepare(dev, fb_helper, preferred_bpp, &drm_fbdev_generic_helper_funcs);
 
-	ret = drm_client_init(dev, &fb_helper->client, "fbdev", &drm_fbdev_client_funcs);
+	ret = drm_client_init(dev, &fb_helper->client, "fbdev", &drm_fbdev_generic_client_funcs);
 	if (ret) {
 		drm_err(dev, "Failed to register client: %d\n", ret);
 		goto err_drm_client_init;
 	}
 
-	ret = drm_fbdev_client_hotplug(&fb_helper->client);
+	ret = drm_fbdev_generic_client_hotplug(&fb_helper->client);
 	if (ret)
 		drm_dbg_kms(dev, "client hotplug ret=%d\n", ret);
 
-- 
2.40.0

Re: [v2,8/8] drm/fbdev-generic: Rename symbols

[Sui jingfeng]

I have tested this patch on my loongson mips64el machine,
not seeing any weird happens.
Tested-by: Sui Jingfeng <suijingfeng@loongson.cn>

On 2023/3/20 23:07, Thomas Zimmermann wrote:
> Rename symbols to match the style of other fbdev-emulation source
> code. No functional changes.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
> Acked-by: Zack Rusin <zackr@vmware.com>
> ---
>   drivers/gpu/drm/drm_fbdev_generic.c | 66 ++++++++++++++---------------
>   1 file changed, 33 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_fbdev_generic.c b/drivers/gpu/drm/drm_fbdev_generic.c
> index e7eeba0c44b4..8e5148bf40bb 100644
> --- a/drivers/gpu/drm/drm_fbdev_generic.c
> +++ b/drivers/gpu/drm/drm_fbdev_generic.c
> @@ -13,7 +13,7 @@
>   #include <drm/drm_fbdev_generic.h>
>   
>   /* @user: 1=userspace, 0=fbcon */
> -static int drm_fbdev_fb_open(struct fb_info *info, int user)
> +static int drm_fbdev_generic_fb_open(struct fb_info *info, int user)
>   {
>   	struct drm_fb_helper *fb_helper = info->par;
>   
> @@ -24,7 +24,7 @@ static int drm_fbdev_fb_open(struct fb_info *info, int user)
>   	return 0;
>   }
>   
> -static int drm_fbdev_fb_release(struct fb_info *info, int user)
> +static int drm_fbdev_generic_fb_release(struct fb_info *info, int user)
>   {
>   	struct drm_fb_helper *fb_helper = info->par;
>   
> @@ -34,7 +34,7 @@ static int drm_fbdev_fb_release(struct fb_info *info, int user)
>   	return 0;
>   }
>   
> -static void drm_fbdev_fb_destroy(struct fb_info *info)
> +static void drm_fbdev_generic_fb_destroy(struct fb_info *info)
>   {
>   	struct drm_fb_helper *fb_helper = info->par;
>   	void *shadow = info->screen_buffer;
> @@ -52,10 +52,10 @@ static void drm_fbdev_fb_destroy(struct fb_info *info)
>   	kfree(fb_helper);
>   }
>   
> -static const struct fb_ops drm_fbdev_fb_ops = {
> +static const struct fb_ops drm_fbdev_generic_fb_ops = {
>   	.owner		= THIS_MODULE,
> -	.fb_open	= drm_fbdev_fb_open,
> -	.fb_release	= drm_fbdev_fb_release,
> +	.fb_open	= drm_fbdev_generic_fb_open,
> +	.fb_release	= drm_fbdev_generic_fb_release,
>   	.fb_read	= drm_fb_helper_sys_read,
>   	.fb_write	= drm_fb_helper_sys_write,
>   	DRM_FB_HELPER_DEFAULT_OPS,
> @@ -63,14 +63,14 @@ static const struct fb_ops drm_fbdev_fb_ops = {
>   	.fb_copyarea	= drm_fb_helper_sys_copyarea,
>   	.fb_imageblit	= drm_fb_helper_sys_imageblit,
>   	.fb_mmap	= fb_deferred_io_mmap,
> -	.fb_destroy	= drm_fbdev_fb_destroy,
> +	.fb_destroy	= drm_fbdev_generic_fb_destroy,
>   };
>   
>   /*
>    * This function uses the client API to create a framebuffer backed by a dumb buffer.
>    */
> -static int drm_fbdev_fb_probe(struct drm_fb_helper *fb_helper,
> -			      struct drm_fb_helper_surface_size *sizes)
> +static int drm_fbdev_generic_helper_fb_probe(struct drm_fb_helper *fb_helper,
> +					     struct drm_fb_helper_surface_size *sizes)
>   {
>   	struct drm_client_dev *client = &fb_helper->client;
>   	struct drm_device *dev = fb_helper->dev;
> @@ -109,7 +109,7 @@ static int drm_fbdev_fb_probe(struct drm_fb_helper *fb_helper,
>   
>   	drm_fb_helper_fill_info(info, fb_helper, sizes);
>   
> -	info->fbops = &drm_fbdev_fb_ops;
> +	info->fbops = &drm_fbdev_generic_fb_ops;
>   	info->flags = FBINFO_DEFAULT;
>   
>   	/* screen */
> @@ -140,9 +140,9 @@ static int drm_fbdev_fb_probe(struct drm_fb_helper *fb_helper,
>   	return ret;
>   }
>   
> -static void drm_fbdev_damage_blit_real(struct drm_fb_helper *fb_helper,
> -				       struct drm_clip_rect *clip,
> -				       struct iosys_map *dst)
> +static void drm_fbdev_generic_damage_blit_real(struct drm_fb_helper *fb_helper,
> +					       struct drm_clip_rect *clip,
> +					       struct iosys_map *dst)
>   {
>   	struct drm_framebuffer *fb = fb_helper->fb;
>   	size_t offset = clip->y1 * fb->pitches[0];
> @@ -179,8 +179,8 @@ static void drm_fbdev_damage_blit_real(struct drm_fb_helper *fb_helper,
>   	}
>   }
>   
> -static int drm_fbdev_damage_blit(struct drm_fb_helper *fb_helper,
> -				 struct drm_clip_rect *clip)
> +static int drm_fbdev_generic_damage_blit(struct drm_fb_helper *fb_helper,
> +					 struct drm_clip_rect *clip)
>   {
>   	struct drm_client_buffer *buffer = fb_helper->buffer;
>   	struct iosys_map map, dst;
> @@ -204,7 +204,7 @@ static int drm_fbdev_damage_blit(struct drm_fb_helper *fb_helper,
>   		goto out;
>   
>   	dst = map;
> -	drm_fbdev_damage_blit_real(fb_helper, clip, &dst);
> +	drm_fbdev_generic_damage_blit_real(fb_helper, clip, &dst);
>   
>   	drm_client_buffer_vunmap(buffer);
>   
> @@ -214,7 +214,8 @@ static int drm_fbdev_damage_blit(struct drm_fb_helper *fb_helper,
>   	return ret;
>   }
>   
> -static int drm_fbdev_fb_dirty(struct drm_fb_helper *helper, struct drm_clip_rect *clip)
> +static int drm_fbdev_generic_helper_fb_dirty(struct drm_fb_helper *helper,
> +					     struct drm_clip_rect *clip)
>   {
>   	struct drm_device *dev = helper->dev;
>   	int ret;
> @@ -223,7 +224,7 @@ static int drm_fbdev_fb_dirty(struct drm_fb_helper *helper, struct drm_clip_rect
>   	if (!(clip->x1 < clip->x2 && clip->y1 < clip->y2))
>   		return 0;
>   
> -	ret = drm_fbdev_damage_blit(helper, clip);
> +	ret = drm_fbdev_generic_damage_blit(helper, clip);
>   	if (drm_WARN_ONCE(dev, ret, "Damage blitter failed: ret=%d\n", ret))
>   		return ret;
>   
> @@ -236,12 +237,12 @@ static int drm_fbdev_fb_dirty(struct drm_fb_helper *helper, struct drm_clip_rect
>   	return 0;
>   }
>   
> -static const struct drm_fb_helper_funcs drm_fb_helper_generic_funcs = {
> -	.fb_probe = drm_fbdev_fb_probe,
> -	.fb_dirty = drm_fbdev_fb_dirty,
> +static const struct drm_fb_helper_funcs drm_fbdev_generic_helper_funcs = {
> +	.fb_probe = drm_fbdev_generic_helper_fb_probe,
> +	.fb_dirty = drm_fbdev_generic_helper_fb_dirty,
>   };
>   
> -static void drm_fbdev_client_unregister(struct drm_client_dev *client)
> +static void drm_fbdev_generic_client_unregister(struct drm_client_dev *client)
>   {
>   	struct drm_fb_helper *fb_helper = drm_fb_helper_from_client(client);
>   
> @@ -254,14 +255,14 @@ static void drm_fbdev_client_unregister(struct drm_client_dev *client)
>   	}
>   }
>   
> -static int drm_fbdev_client_restore(struct drm_client_dev *client)
> +static int drm_fbdev_generic_client_restore(struct drm_client_dev *client)
>   {
>   	drm_fb_helper_lastclose(client->dev);
>   
>   	return 0;
>   }
>   
> -static int drm_fbdev_client_hotplug(struct drm_client_dev *client)
> +static int drm_fbdev_generic_client_hotplug(struct drm_client_dev *client)
>   {
>   	struct drm_fb_helper *fb_helper = drm_fb_helper_from_client(client);
>   	struct drm_device *dev = client->dev;
> @@ -290,11 +291,11 @@ static int drm_fbdev_client_hotplug(struct drm_client_dev *client)
>   	return ret;
>   }
>   
> -static const struct drm_client_funcs drm_fbdev_client_funcs = {
> +static const struct drm_client_funcs drm_fbdev_generic_client_funcs = {
>   	.owner		= THIS_MODULE,
> -	.unregister	= drm_fbdev_client_unregister,
> -	.restore	= drm_fbdev_client_restore,
> -	.hotplug	= drm_fbdev_client_hotplug,
> +	.unregister	= drm_fbdev_generic_client_unregister,
> +	.restore	= drm_fbdev_generic_client_restore,
> +	.hotplug	= drm_fbdev_generic_client_hotplug,
>   };
>   
>   /**
> @@ -320,8 +321,7 @@ static const struct drm_client_funcs drm_fbdev_client_funcs = {
>    *
>    * The fbdev is destroyed by drm_dev_unregister().
>    */
> -void drm_fbdev_generic_setup(struct drm_device *dev,
> -			     unsigned int preferred_bpp)
> +void drm_fbdev_generic_setup(struct drm_device *dev, unsigned int preferred_bpp)
>   {
>   	struct drm_fb_helper *fb_helper;
>   	int ret;
> @@ -332,15 +332,15 @@ void drm_fbdev_generic_setup(struct drm_device *dev,
>   	fb_helper = kzalloc(sizeof(*fb_helper), GFP_KERNEL);
>   	if (!fb_helper)
>   		return;
> -	drm_fb_helper_prepare(dev, fb_helper, preferred_bpp, &drm_fb_helper_generic_funcs);
> +	drm_fb_helper_prepare(dev, fb_helper, preferred_bpp, &drm_fbdev_generic_helper_funcs);
>   
> -	ret = drm_client_init(dev, &fb_helper->client, "fbdev", &drm_fbdev_client_funcs);
> +	ret = drm_client_init(dev, &fb_helper->client, "fbdev", &drm_fbdev_generic_client_funcs);
>   	if (ret) {
>   		drm_err(dev, "Failed to register client: %d\n", ret);
>   		goto err_drm_client_init;
>   	}
>   
> -	ret = drm_fbdev_client_hotplug(&fb_helper->client);
> +	ret = drm_fbdev_generic_client_hotplug(&fb_helper->client);
>   	if (ret)
>   		drm_dbg_kms(dev, "client hotplug ret=%d\n", ret);
>