Linux Virtualization - Jul 2019 - [PATCH 3/5] drm/ast: Replace struct ast_fbdev with generic framebuffer emulation

DRM client buffers are permanently mapped throughout their lifetime. This
prevents us from using generic framebuffer emulation for devices with
small dedicated video memory, such as ast or mgag200. With fb buffers
permanently mapped, such devices often won't have enougth space left to
display other content (e.g., X11).

This patch set introduces unmappable DRM client buffers for framebuffer
emulation with shadow buffers. While the shadow buffer remains in system
memory permanently, the respective buffer object will only be mapped briefly
during updates from the shadow buffer. Hence, the driver can relocate he
buffer object among memory regions as needed.

The default behoviour for DRM client buffers is still to be permanently
mapped.

The patch set converts ast and mgag200 to generic framebuffer emulation
and removes a large amount of framebuffer code from these drivers. For
bochs, a problem was reported where the driver could not display the console
because it was pinned in system memory. [1] The patch set fixes this bug
by converting bochs to use the shadow fb.

The patch set has been tested on ast and mga200 HW.

[1] https://lists.freedesktop.org/archives/dri-devel/2019-June/224423.html

Thomas Zimmermann (5):
  drm/client: Support unmapping of DRM client buffers
  drm/fb-helper: Unmap BO for shadow-buffered framebuffer console
  drm/ast: Replace struct ast_fbdev with generic framebuffer emulation
  drm/bochs: Use shadow buffer for bochs framebuffer console
  drm/mgag200: Replace struct mga_fbdev with generic framebuffer
    emulation

 drivers/gpu/drm/ast/Makefile           |   2 +-
 drivers/gpu/drm/ast/ast_drv.c          |  22 +-
 drivers/gpu/drm/ast/ast_drv.h          |  17 --
 drivers/gpu/drm/ast/ast_fb.c           | 341 -------------------------
 drivers/gpu/drm/ast/ast_main.c         |  30 ++-
 drivers/gpu/drm/ast/ast_mode.c         |  21 --
 drivers/gpu/drm/bochs/bochs_kms.c      |   2 +-
 drivers/gpu/drm/drm_client.c           |  71 ++++-
 drivers/gpu/drm/drm_fb_helper.c        |  14 +-
 drivers/gpu/drm/mgag200/Makefile       |   2 +-
 drivers/gpu/drm/mgag200/mgag200_drv.h  |  19 --
 drivers/gpu/drm/mgag200/mgag200_fb.c   | 309 ----------------------
 drivers/gpu/drm/mgag200/mgag200_main.c |  61 +++--
 drivers/gpu/drm/mgag200/mgag200_mode.c |  27 --
 include/drm/drm_client.h               |   3 +
 15 files changed, 154 insertions(+), 787 deletions(-)
 delete mode 100644 drivers/gpu/drm/ast/ast_fb.c
 delete mode 100644 drivers/gpu/drm/mgag200/mgag200_fb.c

--
2.21.0

DRM clients, such as the fbdev emulation, have their buffer objects
mapped by default. Mapping a buffer implicitly prevents its relocation.
Hence, the buffer may permanently consume video memory while it's
allocated. This is a problem for drivers of low-memory devices, such as
ast, mgag200 or older framebuffer hardware, which will then not have
enough memory to display other content (e.g., X11).

This patch introduces drm_client_buffer_vmap() and _vunmap(). Internal
DRM clients can use these functions to unmap and remap buffer objects
as needed.

There's no reference counting for vmap operations. Callers are expected
to either keep buffers mapped (as it is now), or call vmap and vunmap
in pairs around code that accesses the mapped memory.

Signed-off-by: Thomas Zimmermann <tzimmermann at suse.de>
---
 drivers/gpu/drm/drm_client.c | 71 +++++++++++++++++++++++++++++++-----
 include/drm/drm_client.h     |  3 ++
 2 files changed, 64 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/drm_client.c b/drivers/gpu/drm/drm_client.c
index 410572f14257..d04660c4470a 100644
--- a/drivers/gpu/drm/drm_client.c
+++ b/drivers/gpu/drm/drm_client.c
@@ -235,7 +235,8 @@ static void drm_client_buffer_delete(struct
drm_client_buffer *buffer)
 {
 	struct drm_device *dev = buffer->client->dev;
 
-	drm_gem_vunmap(buffer->gem, buffer->vaddr);
+	if (buffer->vaddr)
+		drm_gem_vunmap(buffer->gem, buffer->vaddr);
 
 	if (buffer->gem)
 		drm_gem_object_put_unlocked(buffer->gem);
@@ -281,6 +282,43 @@ drm_client_buffer_create(struct drm_client_dev *client, u32
width, u32 height, u
 
 	buffer->gem = obj;
 
+	vaddr = drm_client_buffer_vmap(buffer);
+	if (IS_ERR(vaddr)) {
+		ret = PTR_ERR(vaddr);
+		goto err_delete;
+	}
+
+	return buffer;
+
+err_delete:
+	drm_client_buffer_delete(buffer);
+
+	return ERR_PTR(ret);
+}
+
+/**
+ * drm_client_buffer_vmap - Map DRM client buffer into address space
+ * @buffer: DRM client buffer
+ *
+ * This function maps a client buffer into kernel address space. If the
+ * buffer is already mapped, it returns the mapping's address.
+ *
+ * Client buffer mappings are not ref'counted. Each call to
+ * drm_client_buffer_vmap() should be followed by a call to
+ * drm_client_buffer_vunmap(); or the client buffer should be mapped
+ * throughout its lifetime. The latter is the default.
+ *
+ * Returns:
+ *	The mapped memory's address
+ */
+void *
+drm_client_buffer_vmap(struct drm_client_buffer *buffer)
+{
+	void *vaddr;
+
+	if (buffer->vaddr)
+		return buffer->vaddr;
+
 	/*
 	 * FIXME: The dependency on GEM here isn't required, we could
 	 * convert the driver handle to a dma-buf instead and use the
@@ -289,21 +327,34 @@ drm_client_buffer_create(struct drm_client_dev *client,
u32 width, u32 height, u
 	 * fd_install step out of the driver backend hooks, to make that
 	 * final step optional for internal users.
 	 */
-	vaddr = drm_gem_vmap(obj);
-	if (IS_ERR(vaddr)) {
-		ret = PTR_ERR(vaddr);
-		goto err_delete;
-	}
+	vaddr = drm_gem_vmap(buffer->gem);
+	if (IS_ERR(vaddr))
+		return vaddr;
 
 	buffer->vaddr = vaddr;
 
-	return buffer;
+	return vaddr;
+}
+EXPORT_SYMBOL(drm_client_buffer_vmap);
 
-err_delete:
-	drm_client_buffer_delete(buffer);
+/**
+ * drm_client_buffer_vunmap - Unmap DRM client buffer
+ * @buffer: DRM client buffer
+ *
+ * This function removes a client buffer's memory mmapping. This
+ * function is only required by clients that manage their buffers
+ * by themselves. By default, DRM client buffers are mapped throughout
+ * their entire lifetime.
+ */
+void drm_client_buffer_vunmap(struct drm_client_buffer *buffer)
+{
+	if (!buffer->vaddr)
+		return;
 
-	return ERR_PTR(ret);
+	drm_gem_vunmap(buffer->gem, buffer->vaddr);
+	buffer->vaddr = NULL;
 }
+EXPORT_SYMBOL(drm_client_buffer_vunmap);
 
 static void drm_client_buffer_rmfb(struct drm_client_buffer *buffer)
 {
diff --git a/include/drm/drm_client.h b/include/drm/drm_client.h
index 72d51d1e9dd9..e1db1d9da0bf 100644
--- a/include/drm/drm_client.h
+++ b/include/drm/drm_client.h
@@ -149,6 +149,9 @@ struct drm_client_buffer {
 struct drm_client_buffer *
 drm_client_framebuffer_create(struct drm_client_dev *client, u32 width, u32
height, u32 format);
 void drm_client_framebuffer_delete(struct drm_client_buffer *buffer);
+void *
+drm_client_buffer_vmap(struct drm_client_buffer *buffer);
+void drm_client_buffer_vunmap(struct drm_client_buffer *buffer);
 
 int drm_client_modeset_create(struct drm_client_dev *client);
 void drm_client_modeset_free(struct drm_client_dev *client);
-- 
2.21.0

The shadow-buffered framebuffer console only needs the buffer object
to be mapped during updates. While not being updated from the shadow
buffer, the buffer object can remain unmapped.

An unmapped buffer object can be evicted to system memory and does
not consume video ram until displayed. This allows to use generic fbdev
emulation with drivers for low-memory devices, such as ast and mgag200.

Signed-off-by: Thomas Zimmermann <tzimmermann at suse.de>
---
 drivers/gpu/drm/drm_fb_helper.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 1984e5c54d58..2a6538f229e3 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -403,6 +403,7 @@ static void drm_fb_helper_dirty_work(struct work_struct
*work)
 	struct drm_clip_rect *clip = &helper->dirty_clip;
 	struct drm_clip_rect clip_copy;
 	unsigned long flags;
+	void *vaddr;
 
 	spin_lock_irqsave(&helper->dirty_lock, flags);
 	clip_copy = *clip;
@@ -412,10 +413,18 @@ static void drm_fb_helper_dirty_work(struct work_struct
*work)
 
 	/* call dirty callback only when it has been really touched */
 	if (clip_copy.x1 < clip_copy.x2 && clip_copy.y1 < clip_copy.y2)
{
+
 		/* Generic fbdev uses a shadow buffer */
-		if (helper->buffer)
+		if (helper->buffer) {
+			vaddr = drm_client_buffer_vmap(helper->buffer);
+			if (IS_ERR(vaddr))
+				return;
 			drm_fb_helper_dirty_blit_real(helper, &clip_copy);
+		}
 		helper->fb->funcs->dirty(helper->fb, NULL, 0, 0, &clip_copy,
1);
+
+		if (helper->buffer)
+			drm_client_buffer_vunmap(helper->buffer);
 	}
 }
 
@@ -2231,6 +2240,9 @@ int drm_fb_helper_generic_probe(struct drm_fb_helper
*fb_helper,
 		fbi->fbdefio = &drm_fbdev_defio;
 
 		fb_deferred_io_init(fbi);
+
+		/* unmapped by default */
+		drm_client_buffer_vunmap(fb_helper->buffer);
 	}
 
 	return 0;
-- 
2.21.0

This patch replaces ast's framebuffer console with DRM's generic
implememtation. All respective code is being removed from the driver.

The console is set up with a shadow buffer. The actual buffer object is
not permanently pinned in video ram, but just another buffer object that
the driver moves in and out of vram as necessary. The driver's function
ast_crtc_do_set_base() used to contain special handling for the framebuffer
console. With the new generic framebuffer, the driver does not need this
code an longer.

Signed-off-by: Thomas Zimmermann <tzimmermann at suse.de>
---
 drivers/gpu/drm/ast/Makefile   |   2 +-
 drivers/gpu/drm/ast/ast_drv.c  |  22 ++-
 drivers/gpu/drm/ast/ast_drv.h  |  17 --
 drivers/gpu/drm/ast/ast_fb.c   | 341 ---------------------------------
 drivers/gpu/drm/ast/ast_main.c |  30 ++-
 drivers/gpu/drm/ast/ast_mode.c |  21 --
 6 files changed, 41 insertions(+), 392 deletions(-)
 delete mode 100644 drivers/gpu/drm/ast/ast_fb.c

diff --git a/drivers/gpu/drm/ast/Makefile b/drivers/gpu/drm/ast/Makefile
index b086dae17013..561f7c4199e4 100644
--- a/drivers/gpu/drm/ast/Makefile
+++ b/drivers/gpu/drm/ast/Makefile
@@ -3,6 +3,6 @@
 # Makefile for the drm device driver.  This driver provides support for the
 # Direct Rendering Infrastructure (DRI) in XFree86 4.1.0 and higher.
 
-ast-y := ast_drv.o ast_main.o ast_mode.o ast_fb.o ast_ttm.o ast_post.o
ast_dp501.o
+ast-y := ast_drv.o ast_main.o ast_mode.o ast_ttm.o ast_post.o ast_dp501.o
 
 obj-$(CONFIG_DRM_AST) := ast.o
diff --git a/drivers/gpu/drm/ast/ast_drv.c b/drivers/gpu/drm/ast/ast_drv.c
index 3811997e78c4..75f55ac1a218 100644
--- a/drivers/gpu/drm/ast/ast_drv.c
+++ b/drivers/gpu/drm/ast/ast_drv.c
@@ -103,25 +103,29 @@ static int ast_drm_freeze(struct drm_device *dev)
 
 	pci_save_state(dev->pdev);
 
-	console_lock();
-	ast_fbdev_set_suspend(dev, 1);
-	console_unlock();
+	if (dev->fb_helper) {
+		console_lock();
+		drm_fb_helper_set_suspend(dev->fb_helper, 1);
+		console_unlock();
+	}
+
 	return 0;
 }
 
 static int ast_drm_thaw(struct drm_device *dev)
 {
-	int error = 0;
-
 	ast_post_gpu(dev);
 
 	drm_mode_config_reset(dev);
 	drm_helper_resume_force_mode(dev);
 
-	console_lock();
-	ast_fbdev_set_suspend(dev, 0);
-	console_unlock();
-	return error;
+	if (dev->fb_helper) {
+		console_lock();
+		drm_fb_helper_set_suspend(dev->fb_helper, 0);
+		console_unlock();
+	}
+
+	return 0;
 }
 
 static int ast_drm_resume(struct drm_device *dev)
diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h
index ca794a3fcf8f..907d7480b7ba 100644
--- a/drivers/gpu/drm/ast/ast_drv.h
+++ b/drivers/gpu/drm/ast/ast_drv.h
@@ -81,8 +81,6 @@ enum ast_tx_chip {
 #define AST_DRAM_4Gx16   7
 #define AST_DRAM_8Gx16   8
 
-struct ast_fbdev;
-
 struct ast_private {
 	struct drm_device *dev;
 
@@ -96,8 +94,6 @@ struct ast_private {
 	uint32_t mclk;
 	uint32_t vram_size;
 
-	struct ast_fbdev *fbdev;
-
 	int fb_mtrr;
 
 	struct drm_gem_object *cursor_cache;
@@ -239,14 +235,6 @@ struct ast_encoder {
 	struct drm_encoder base;
 };
 
-struct ast_fbdev {
-	struct drm_fb_helper helper; /* must be first */
-	void *sysram;
-	int size;
-	int x1, y1, x2, y2; /* dirty rect */
-	spinlock_t dirty_lock;
-};
-
 #define to_ast_crtc(x) container_of(x, struct ast_crtc, base)
 #define to_ast_connector(x) container_of(x, struct ast_connector, base)
 #define to_ast_encoder(x) container_of(x, struct ast_encoder, base)
@@ -289,11 +277,6 @@ struct ast_vbios_mode_info {
 extern int ast_mode_init(struct drm_device *dev);
 extern void ast_mode_fini(struct drm_device *dev);
 
-int ast_fbdev_init(struct drm_device *dev);
-void ast_fbdev_fini(struct drm_device *dev);
-void ast_fbdev_set_suspend(struct drm_device *dev, int state);
-void ast_fbdev_set_base(struct ast_private *ast, unsigned long gpu_addr);
-
 #define AST_MM_ALIGN_SHIFT 4
 #define AST_MM_ALIGN_MASK ((1 << AST_MM_ALIGN_SHIFT) - 1)
 
diff --git a/drivers/gpu/drm/ast/ast_fb.c b/drivers/gpu/drm/ast/ast_fb.c
deleted file mode 100644
index a849e58b40bc..000000000000
--- a/drivers/gpu/drm/ast/ast_fb.c
+++ /dev/null
@@ -1,341 +0,0 @@
-/*
- * Copyright 2012 Red Hat Inc.
- *
- * Permission is hereby granted, free of charge, to any person obtaining a
- * copy of this software and associated documentation files (the
- * "Software"), to deal in the Software without restriction,
including
- * without limitation the rights to use, copy, modify, merge, publish,
- * distribute, sub license, and/or sell copies of the Software, and to
- * permit persons to whom the Software is furnished to do so, subject to
- * the following conditions:
- *
- * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
EXPRESS OR
- * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
- * FITNESS FOR A PARTICULAR PURPOSE AND NON-INFRINGEMENT. IN NO EVENT SHALL
- * THE COPYRIGHT HOLDERS, AUTHORS AND/OR ITS SUPPLIERS BE LIABLE FOR ANY CLAIM,
- * DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
- * OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE
- * USE OR OTHER DEALINGS IN THE SOFTWARE.
- *
- * The above copyright notice and this permission notice (including the
- * next paragraph) shall be included in all copies or substantial portions
- * of the Software.
- *
- */
-/*
- * Authors: Dave Airlie <airlied at redhat.com>
- */
-#include <linux/module.h>
-#include <linux/kernel.h>
-#include <linux/errno.h>
-#include <linux/string.h>
-#include <linux/mm.h>
-#include <linux/tty.h>
-#include <linux/sysrq.h>
-#include <linux/delay.h>
-#include <linux/init.h>
-
-
-#include <drm/drmP.h>
-#include <drm/drm_crtc.h>
-#include <drm/drm_crtc_helper.h>
-#include <drm/drm_fb_helper.h>
-#include <drm/drm_gem_framebuffer_helper.h>
-#include <drm/drm_util.h>
-
-#include "ast_drv.h"
-
-static void ast_dirty_update(struct ast_fbdev *afbdev,
-			     int x, int y, int width, int height)
-{
-	int i;
-	struct drm_gem_vram_object *gbo;
-	int src_offset, dst_offset;
-	int ret;
-	u8 *dst;
-	bool unmap = false;
-	bool store_for_later = false;
-	int x2, y2;
-	unsigned long flags;
-	struct drm_framebuffer *fb = afbdev->helper.fb;
-	int bpp = fb->format->cpp[0];
-
-	gbo = drm_gem_vram_of_gem(fb->obj[0]);
-
-	if (drm_can_sleep()) {
-		/* We pin the BO so it won't be moved during the
-		 * update. The actual location, video RAM or system
-		 * memory, is not important.
-		 */
-		ret = drm_gem_vram_pin(gbo, 0);
-		if (ret) {
-			if (ret != -EBUSY)
-				return;
-			store_for_later = true;
-		}
-	} else {
-		store_for_later = true;
-	}
-
-	x2 = x + width - 1;
-	y2 = y + height - 1;
-	spin_lock_irqsave(&afbdev->dirty_lock, flags);
-
-	if (afbdev->y1 < y)
-		y = afbdev->y1;
-	if (afbdev->y2 > y2)
-		y2 = afbdev->y2;
-	if (afbdev->x1 < x)
-		x = afbdev->x1;
-	if (afbdev->x2 > x2)
-		x2 = afbdev->x2;
-
-	if (store_for_later) {
-		afbdev->x1 = x;
-		afbdev->x2 = x2;
-		afbdev->y1 = y;
-		afbdev->y2 = y2;
-		spin_unlock_irqrestore(&afbdev->dirty_lock, flags);
-		return;
-	}
-
-	afbdev->x1 = afbdev->y1 = INT_MAX;
-	afbdev->x2 = afbdev->y2 = 0;
-	spin_unlock_irqrestore(&afbdev->dirty_lock, flags);
-
-	dst = drm_gem_vram_kmap(gbo, false, NULL);
-	if (IS_ERR(dst)) {
-		DRM_ERROR("failed to kmap fb updates\n");
-		goto out;
-	} else if (!dst) {
-		dst = drm_gem_vram_kmap(gbo, true, NULL);
-		if (IS_ERR(dst)) {
-			DRM_ERROR("failed to kmap fb updates\n");
-			goto out;
-		}
-		unmap = true;
-	}
-
-	for (i = y; i <= y2; i++) {
-		/* assume equal stride for now */
-		src_offset = dst_offset = i * fb->pitches[0] + (x * bpp);
-		memcpy_toio(dst + dst_offset, afbdev->sysram + src_offset,
-			    (x2 - x + 1) * bpp);
-	}
-
-	if (unmap)
-		drm_gem_vram_kunmap(gbo);
-
-out:
-	drm_gem_vram_unpin(gbo);
-}
-
-static void ast_fillrect(struct fb_info *info,
-			 const struct fb_fillrect *rect)
-{
-	struct ast_fbdev *afbdev = info->par;
-	drm_fb_helper_sys_fillrect(info, rect);
-	ast_dirty_update(afbdev, rect->dx, rect->dy, rect->width,
-			 rect->height);
-}
-
-static void ast_copyarea(struct fb_info *info,
-			 const struct fb_copyarea *area)
-{
-	struct ast_fbdev *afbdev = info->par;
-	drm_fb_helper_sys_copyarea(info, area);
-	ast_dirty_update(afbdev, area->dx, area->dy, area->width,
-			 area->height);
-}
-
-static void ast_imageblit(struct fb_info *info,
-			  const struct fb_image *image)
-{
-	struct ast_fbdev *afbdev = info->par;
-	drm_fb_helper_sys_imageblit(info, image);
-	ast_dirty_update(afbdev, image->dx, image->dy, image->width,
-			 image->height);
-}
-
-static struct fb_ops astfb_ops = {
-	.owner = THIS_MODULE,
-	.fb_check_var = drm_fb_helper_check_var,
-	.fb_set_par = drm_fb_helper_set_par,
-	.fb_fillrect = ast_fillrect,
-	.fb_copyarea = ast_copyarea,
-	.fb_imageblit = ast_imageblit,
-	.fb_pan_display = drm_fb_helper_pan_display,
-	.fb_blank = drm_fb_helper_blank,
-	.fb_setcmap = drm_fb_helper_setcmap,
-};
-
-static int astfb_create_object(struct ast_fbdev *afbdev,
-			       const struct drm_mode_fb_cmd2 *mode_cmd,
-			       struct drm_gem_object **gobj_p)
-{
-	struct drm_device *dev = afbdev->helper.dev;
-	u32 size;
-	struct drm_gem_object *gobj;
-	int ret = 0;
-
-	size = mode_cmd->pitches[0] * mode_cmd->height;
-	ret = ast_gem_create(dev, size, true, &gobj);
-	if (ret)
-		return ret;
-
-	*gobj_p = gobj;
-	return ret;
-}
-
-static int astfb_create(struct drm_fb_helper *helper,
-			struct drm_fb_helper_surface_size *sizes)
-{
-	struct ast_fbdev *afbdev -		container_of(helper, struct ast_fbdev, helper);
-	struct drm_device *dev = afbdev->helper.dev;
-	struct drm_mode_fb_cmd2 mode_cmd;
-	struct drm_framebuffer *fb;
-	struct fb_info *info;
-	int size, ret;
-	void *sysram;
-	struct drm_gem_object *gobj = NULL;
-	mode_cmd.width = sizes->surface_width;
-	mode_cmd.height = sizes->surface_height;
-	mode_cmd.pitches[0] = mode_cmd.width * ((sizes->surface_bpp + 7)/8);
-
-	mode_cmd.pixel_format = drm_mode_legacy_fb_format(sizes->surface_bpp,
-							  sizes->surface_depth);
-
-	size = mode_cmd.pitches[0] * mode_cmd.height;
-
-	ret = astfb_create_object(afbdev, &mode_cmd, &gobj);
-	if (ret) {
-		DRM_ERROR("failed to create fbcon backing object %d\n", ret);
-		return ret;
-	}
-
-	sysram = vmalloc(size);
-	if (!sysram)
-		return -ENOMEM;
-
-	info = drm_fb_helper_alloc_fbi(helper);
-	if (IS_ERR(info)) {
-		ret = PTR_ERR(info);
-		goto out;
-	}
-	fb = drm_gem_fbdev_fb_create(dev, sizes, 0, gobj, NULL);
-	if (IS_ERR(fb)) {
-		ret = PTR_ERR(fb);
-		goto out;
-	}
-
-	afbdev->sysram = sysram;
-	afbdev->size = size;
-
-	afbdev->helper.fb = fb;
-
-	info->fbops = &astfb_ops;
-
-	info->apertures->ranges[0].base = pci_resource_start(dev->pdev, 0);
-	info->apertures->ranges[0].size = pci_resource_len(dev->pdev, 0);
-
-	drm_fb_helper_fill_info(info, &afbdev->helper, sizes);
-
-	info->screen_base = sysram;
-	info->screen_size = size;
-
-	info->pixmap.flags = FB_PIXMAP_SYSTEM;
-
-	DRM_DEBUG_KMS("allocated %dx%d\n",
-		      fb->width, fb->height);
-
-	return 0;
-
-out:
-	vfree(sysram);
-	return ret;
-}
-
-static const struct drm_fb_helper_funcs ast_fb_helper_funcs = {
-	.fb_probe = astfb_create,
-};
-
-static void ast_fbdev_destroy(struct drm_device *dev,
-			      struct ast_fbdev *afbdev)
-{
-	drm_helper_force_disable_all(dev);
-	drm_fb_helper_unregister_fbi(&afbdev->helper);
-
-	drm_fb_helper_fini(&afbdev->helper);
-	drm_framebuffer_put(afbdev->helper.fb);
-
-	vfree(afbdev->sysram);
-}
-
-int ast_fbdev_init(struct drm_device *dev)
-{
-	struct ast_private *ast = dev->dev_private;
-	struct ast_fbdev *afbdev;
-	int ret;
-
-	afbdev = kzalloc(sizeof(struct ast_fbdev), GFP_KERNEL);
-	if (!afbdev)
-		return -ENOMEM;
-
-	ast->fbdev = afbdev;
-	spin_lock_init(&afbdev->dirty_lock);
-
-	drm_fb_helper_prepare(dev, &afbdev->helper, &ast_fb_helper_funcs);
-
-	ret = drm_fb_helper_init(dev, &afbdev->helper, 1);
-	if (ret)
-		goto free;
-
-	ret = drm_fb_helper_single_add_all_connectors(&afbdev->helper);
-	if (ret)
-		goto fini;
-
-	/* disable all the possible outputs/crtcs before entering KMS mode */
-	drm_helper_disable_unused_functions(dev);
-
-	ret = drm_fb_helper_initial_config(&afbdev->helper, 32);
-	if (ret)
-		goto fini;
-
-	return 0;
-
-fini:
-	drm_fb_helper_fini(&afbdev->helper);
-free:
-	kfree(afbdev);
-	return ret;
-}
-
-void ast_fbdev_fini(struct drm_device *dev)
-{
-	struct ast_private *ast = dev->dev_private;
-
-	if (!ast->fbdev)
-		return;
-
-	ast_fbdev_destroy(dev, ast->fbdev);
-	kfree(ast->fbdev);
-	ast->fbdev = NULL;
-}
-
-void ast_fbdev_set_suspend(struct drm_device *dev, int state)
-{
-	struct ast_private *ast = dev->dev_private;
-
-	if (!ast->fbdev)
-		return;
-
-	drm_fb_helper_set_suspend(&ast->fbdev->helper, state);
-}
-
-void ast_fbdev_set_base(struct ast_private *ast, unsigned long gpu_addr)
-{
-	ast->fbdev->helper.fbdev->fix.smem_start -	
ast->fbdev->helper.fbdev->apertures->ranges[0].base + gpu_addr;
-	ast->fbdev->helper.fbdev->fix.smem_len = ast->vram_size -
gpu_addr;
-}
diff --git a/drivers/gpu/drm/ast/ast_main.c b/drivers/gpu/drm/ast/ast_main.c
index 1bd61696e509..78e8e20ff577 100644
--- a/drivers/gpu/drm/ast/ast_main.c
+++ b/drivers/gpu/drm/ast/ast_main.c
@@ -383,8 +383,33 @@ static int ast_get_dram_info(struct drm_device *dev)
 	return 0;
 }
 
+static int ast_framebuffer_dirtyfb(struct drm_framebuffer *fb,
+				   struct drm_file *file_priv,
+				   unsigned int flags,
+				   unsigned int color,
+				   struct drm_clip_rect *clips,
+				   unsigned int num_clips)
+{
+	/* empty placeholder function to enable fbcon shadow buffer */
+	return 0;
+}
+
+static const struct drm_framebuffer_funcs ast_framebuffer_funcs = {
+	.destroy	= drm_gem_fb_destroy,
+	.create_handle	= drm_gem_fb_create_handle,
+	.dirty		= ast_framebuffer_dirtyfb,
+};
+
+static struct drm_framebuffer *
+ast_mode_config_fb_create(struct drm_device *dev, struct drm_file *file,
+			  const struct drm_mode_fb_cmd2 *mode_cmd)
+{
+	return drm_gem_fb_create_with_funcs(dev, file, mode_cmd,
+					    &ast_framebuffer_funcs);
+}
+
 static const struct drm_mode_config_funcs ast_mode_funcs = {
-	.fb_create = drm_gem_fb_create
+	.fb_create = ast_mode_config_fb_create
 };
 
 static u32 ast_get_vram_info(struct drm_device *dev)
@@ -502,7 +527,7 @@ int ast_driver_load(struct drm_device *dev, unsigned long
flags)
 	if (ret)
 		goto out_free;
 
-	ret = ast_fbdev_init(dev);
+	ret = drm_fbdev_generic_setup(dev, 32);
 	if (ret)
 		goto out_free;
 
@@ -520,7 +545,6 @@ void ast_driver_unload(struct drm_device *dev)
 	ast_release_firmware(dev);
 	kfree(ast->dp501_fw_addr);
 	ast_mode_fini(dev);
-	ast_fbdev_fini(dev);
 	drm_mode_config_cleanup(dev);
 
 	ast_mm_fini(ast);
diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
index c48249df758e..3c0716891b29 100644
--- a/drivers/gpu/drm/ast/ast_mode.c
+++ b/drivers/gpu/drm/ast/ast_mode.c
@@ -525,18 +525,12 @@ static int ast_crtc_do_set_base(struct drm_crtc *crtc,
 				struct drm_framebuffer *fb,
 				int x, int y, int atomic)
 {
-	struct ast_private *ast = crtc->dev->dev_private;
 	struct drm_gem_vram_object *gbo;
 	int ret;
 	s64 gpu_addr;
-	void *base;
 
 	if (!atomic && fb) {
 		gbo = drm_gem_vram_of_gem(fb->obj[0]);
-
-		/* unmap if console */
-		if (ast->fbdev->helper.fb == fb)
-			drm_gem_vram_kunmap(gbo);
 		drm_gem_vram_unpin(gbo);
 	}
 
@@ -551,17 +545,6 @@ static int ast_crtc_do_set_base(struct drm_crtc *crtc,
 		goto err_drm_gem_vram_unpin;
 	}
 
-	if (ast->fbdev->helper.fb == crtc->primary->fb) {
-		/* if pushing console in kmap it */
-		base = drm_gem_vram_kmap(gbo, true, NULL);
-		if (IS_ERR(base)) {
-			ret = PTR_ERR(base);
-			DRM_ERROR("failed to kmap fbcon\n");
-		} else {
-			ast_fbdev_set_base(ast, gpu_addr);
-		}
-	}
-
 	ast_set_offset_reg(crtc);
 	ast_set_start_address_crt1(crtc, (u32)gpu_addr);
 
@@ -618,14 +601,10 @@ static void ast_crtc_disable(struct drm_crtc *crtc)
 	DRM_DEBUG_KMS("\n");
 	ast_crtc_dpms(crtc, DRM_MODE_DPMS_OFF);
 	if (crtc->primary->fb) {
-		struct ast_private *ast = crtc->dev->dev_private;
 		struct drm_framebuffer *fb = crtc->primary->fb;
 		struct drm_gem_vram_object *gbo  			drm_gem_vram_of_gem(fb->obj[0]);
 
-		/* unmap if console */
-		if (ast->fbdev->helper.fb == fb)
-			drm_gem_vram_kunmap(gbo);
 		drm_gem_vram_unpin(gbo);
 	}
 	crtc->primary->fb = NULL;
-- 
2.21.0

The bochs driver (and virtual hardware) requires buffer objects to
reside in video ram to display them to the screen. So it can not
display the framebuffer console because the respective buffer object
is permanently pinned in system memory.

Using a shadow buffer for the console solves this problem. The console
emulation will pin the buffer object only during updates from the shadow
buffer. Otherwise, the bochs driver can freely relocated the buffer
between system memory and video ram.

Signed-off-by: Thomas Zimmermann <tzimmermann at suse.de>
---
 drivers/gpu/drm/bochs/bochs_kms.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bochs/bochs_kms.c
b/drivers/gpu/drm/bochs/bochs_kms.c
index bc19dbd531ef..47fab4852483 100644
--- a/drivers/gpu/drm/bochs/bochs_kms.c
+++ b/drivers/gpu/drm/bochs/bochs_kms.c
@@ -172,7 +172,7 @@ bochs_gem_fb_create(struct drm_device *dev, struct drm_file
*file,
 	    mode_cmd->pixel_format != DRM_FORMAT_BGRX8888)
 		return ERR_PTR(-EINVAL);
 
-	return drm_gem_fb_create(dev, file, mode_cmd);
+	return drm_gem_fb_create_with_dirty(dev, file, mode_cmd);
 }
 
 const struct drm_mode_config_funcs bochs_mode_funcs = {
-- 
2.21.0

This patch replaces mgag200's framebuffer console with DRM's generic
implememtation. All respective code is being removed from the driver.

The console is set up with a shadow buffer. The actual buffer object is
not permanently pinned in video ram, but just another buffer object that
the driver moves in and out of vram as necessary. The driver's function
mga_crtc_do_set_base() used to contain special handling for the framebuffer
console. With the new generic framebuffer, the driver does not need this
code an longer.

Signed-off-by: Thomas Zimmermann <tzimmermann at suse.de>
---
 drivers/gpu/drm/mgag200/Makefile       |   2 +-
 drivers/gpu/drm/mgag200/mgag200_drv.h  |  19 --
 drivers/gpu/drm/mgag200/mgag200_fb.c   | 309 -------------------------
 drivers/gpu/drm/mgag200/mgag200_main.c |  61 ++---
 drivers/gpu/drm/mgag200/mgag200_mode.c |  27 ---
 5 files changed, 35 insertions(+), 383 deletions(-)
 delete mode 100644 drivers/gpu/drm/mgag200/mgag200_fb.c

diff --git a/drivers/gpu/drm/mgag200/Makefile b/drivers/gpu/drm/mgag200/Makefile
index 98d204408bd0..04b281bcf655 100644
--- a/drivers/gpu/drm/mgag200/Makefile
+++ b/drivers/gpu/drm/mgag200/Makefile
@@ -1,5 +1,5 @@
 # SPDX-License-Identifier: GPL-2.0-only
 mgag200-y   := mgag200_main.o mgag200_mode.o mgag200_cursor.o \
-	mgag200_drv.o mgag200_fb.o mgag200_i2c.o mgag200_ttm.o
+	mgag200_drv.o mgag200_i2c.o mgag200_ttm.o
 
 obj-$(CONFIG_DRM_MGAG200) += mgag200.o
diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h
b/drivers/gpu/drm/mgag200/mgag200_drv.h
index 3ab27f1053c1..1c93f8dc08c7 100644
--- a/drivers/gpu/drm/mgag200/mgag200_drv.h
+++ b/drivers/gpu/drm/mgag200/mgag200_drv.h
@@ -99,14 +99,6 @@
 #define to_mga_encoder(x) container_of(x, struct mga_encoder, base)
 #define to_mga_connector(x) container_of(x, struct mga_connector, base)
 
-struct mga_fbdev {
-	struct drm_fb_helper helper; /* must be first */
-	void *sysram;
-	int size;
-	int x1, y1, x2, y2; /* dirty rect */
-	spinlock_t dirty_lock;
-};
-
 struct mga_crtc {
 	struct drm_crtc base;
 	u8 lut_r[256], lut_g[256], lut_b[256];
@@ -180,7 +172,6 @@ struct mga_device {
 	struct mga_mc			mc;
 	struct mga_mode_info		mode_info;
 
-	struct mga_fbdev *mfbdev;
 	struct mga_cursor cursor;
 
 	bool				suspended;
@@ -201,19 +192,9 @@ struct mga_device {
 int mgag200_modeset_init(struct mga_device *mdev);
 void mgag200_modeset_fini(struct mga_device *mdev);
 
-				/* mgag200_fb.c */
-int mgag200_fbdev_init(struct mga_device *mdev);
-void mgag200_fbdev_fini(struct mga_device *mdev);
-
 				/* mgag200_main.c */
 int mgag200_driver_load(struct drm_device *dev, unsigned long flags);
 void mgag200_driver_unload(struct drm_device *dev);
-int mgag200_gem_create(struct drm_device *dev,
-		   u32 size, bool iskernel,
-		       struct drm_gem_object **obj);
-int mgag200_dumb_create(struct drm_file *file,
-			struct drm_device *dev,
-			struct drm_mode_create_dumb *args);
 
 				/* mgag200_i2c.c */
 struct mga_i2c_chan *mgag200_i2c_create(struct drm_device *dev);
diff --git a/drivers/gpu/drm/mgag200/mgag200_fb.c
b/drivers/gpu/drm/mgag200/mgag200_fb.c
deleted file mode 100644
index c77cf1b34c98..000000000000
--- a/drivers/gpu/drm/mgag200/mgag200_fb.c
+++ /dev/null
@@ -1,309 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0-only
-/*
- * Copyright 2010 Matt Turner.
- * Copyright 2012 Red Hat
- *
- * Authors: Matthew Garrett
- *          Matt Turner
- *          Dave Airlie
- */
-
-#include <linux/module.h>
-#include <linux/vmalloc.h>
-
-#include <drm/drm_crtc_helper.h>
-#include <drm/drm_fb_helper.h>
-#include <drm/drm_fourcc.h>
-#include <drm/drm_gem_framebuffer_helper.h>
-#include <drm/drm_util.h>
-
-#include "mgag200_drv.h"
-
-static void mga_dirty_update(struct mga_fbdev *mfbdev,
-			     int x, int y, int width, int height)
-{
-	int i;
-	struct drm_gem_vram_object *gbo;
-	int src_offset, dst_offset;
-	int ret;
-	u8 *dst;
-	bool unmap = false;
-	bool store_for_later = false;
-	int x2, y2;
-	unsigned long flags;
-	struct drm_framebuffer *fb = mfbdev->helper.fb;
-	int bpp = fb->format->cpp[0];
-
-	gbo = drm_gem_vram_of_gem(fb->obj[0]);
-
-	if (drm_can_sleep()) {
-		/* We pin the BO so it won't be moved during the
-		 * update. The actual location, video RAM or system
-		 * memory, is not important.
-		 */
-		ret = drm_gem_vram_pin(gbo, 0);
-		if (ret) {
-			if (ret != -EBUSY)
-				return;
-			store_for_later = true;
-		}
-	} else {
-		store_for_later = true;
-	}
-
-	x2 = x + width - 1;
-	y2 = y + height - 1;
-	spin_lock_irqsave(&mfbdev->dirty_lock, flags);
-
-	if (mfbdev->y1 < y)
-		y = mfbdev->y1;
-	if (mfbdev->y2 > y2)
-		y2 = mfbdev->y2;
-	if (mfbdev->x1 < x)
-		x = mfbdev->x1;
-	if (mfbdev->x2 > x2)
-		x2 = mfbdev->x2;
-
-	if (store_for_later) {
-		mfbdev->x1 = x;
-		mfbdev->x2 = x2;
-		mfbdev->y1 = y;
-		mfbdev->y2 = y2;
-		spin_unlock_irqrestore(&mfbdev->dirty_lock, flags);
-		return;
-	}
-
-	mfbdev->x1 = mfbdev->y1 = INT_MAX;
-	mfbdev->x2 = mfbdev->y2 = 0;
-	spin_unlock_irqrestore(&mfbdev->dirty_lock, flags);
-
-	dst = drm_gem_vram_kmap(gbo, false, NULL);
-	if (IS_ERR(dst)) {
-		DRM_ERROR("failed to kmap fb updates\n");
-		goto out;
-	} else if (!dst) {
-		dst = drm_gem_vram_kmap(gbo, true, NULL);
-		if (IS_ERR(dst)) {
-			DRM_ERROR("failed to kmap fb updates\n");
-			goto out;
-		}
-		unmap = true;
-	}
-
-	for (i = y; i <= y2; i++) {
-		/* assume equal stride for now */
-		src_offset = dst_offset = i * fb->pitches[0] + (x * bpp);
-		memcpy_toio(dst + dst_offset, mfbdev->sysram + src_offset,
-			    (x2 - x + 1) * bpp);
-	}
-
-	if (unmap)
-		drm_gem_vram_kunmap(gbo);
-
-out:
-	drm_gem_vram_unpin(gbo);
-}
-
-static void mga_fillrect(struct fb_info *info,
-			 const struct fb_fillrect *rect)
-{
-	struct mga_fbdev *mfbdev = info->par;
-	drm_fb_helper_sys_fillrect(info, rect);
-	mga_dirty_update(mfbdev, rect->dx, rect->dy, rect->width,
-			 rect->height);
-}
-
-static void mga_copyarea(struct fb_info *info,
-			 const struct fb_copyarea *area)
-{
-	struct mga_fbdev *mfbdev = info->par;
-	drm_fb_helper_sys_copyarea(info, area);
-	mga_dirty_update(mfbdev, area->dx, area->dy, area->width,
-			 area->height);
-}
-
-static void mga_imageblit(struct fb_info *info,
-			  const struct fb_image *image)
-{
-	struct mga_fbdev *mfbdev = info->par;
-	drm_fb_helper_sys_imageblit(info, image);
-	mga_dirty_update(mfbdev, image->dx, image->dy, image->width,
-			 image->height);
-}
-
-
-static struct fb_ops mgag200fb_ops = {
-	.owner = THIS_MODULE,
-	.fb_check_var = drm_fb_helper_check_var,
-	.fb_set_par = drm_fb_helper_set_par,
-	.fb_fillrect = mga_fillrect,
-	.fb_copyarea = mga_copyarea,
-	.fb_imageblit = mga_imageblit,
-	.fb_pan_display = drm_fb_helper_pan_display,
-	.fb_blank = drm_fb_helper_blank,
-	.fb_setcmap = drm_fb_helper_setcmap,
-};
-
-static int mgag200fb_create_object(struct mga_fbdev *afbdev,
-				   const struct drm_mode_fb_cmd2 *mode_cmd,
-				   struct drm_gem_object **gobj_p)
-{
-	struct drm_device *dev = afbdev->helper.dev;
-	u32 size;
-	struct drm_gem_object *gobj;
-	int ret = 0;
-
-	size = mode_cmd->pitches[0] * mode_cmd->height;
-	ret = mgag200_gem_create(dev, size, true, &gobj);
-	if (ret)
-		return ret;
-
-	*gobj_p = gobj;
-	return ret;
-}
-
-static int mgag200fb_create(struct drm_fb_helper *helper,
-			   struct drm_fb_helper_surface_size *sizes)
-{
-	struct mga_fbdev *mfbdev -		container_of(helper, struct mga_fbdev, helper);
-	struct drm_device *dev = mfbdev->helper.dev;
-	struct drm_mode_fb_cmd2 mode_cmd;
-	struct mga_device *mdev = dev->dev_private;
-	struct fb_info *info;
-	struct drm_framebuffer *fb;
-	struct drm_gem_object *gobj = NULL;
-	int ret;
-	void *sysram;
-	int size;
-
-	mode_cmd.width = sizes->surface_width;
-	mode_cmd.height = sizes->surface_height;
-	mode_cmd.pitches[0] = mode_cmd.width * ((sizes->surface_bpp + 7) / 8);
-
-	mode_cmd.pixel_format = drm_mode_legacy_fb_format(sizes->surface_bpp,
-							  sizes->surface_depth);
-	size = mode_cmd.pitches[0] * mode_cmd.height;
-
-	ret = mgag200fb_create_object(mfbdev, &mode_cmd, &gobj);
-	if (ret) {
-		DRM_ERROR("failed to create fbcon backing object %d\n", ret);
-		return ret;
-	}
-
-	sysram = vmalloc(size);
-	if (!sysram) {
-		ret = -ENOMEM;
-		goto err_drm_gem_object_put_unlocked;
-	}
-
-	info = drm_fb_helper_alloc_fbi(helper);
-	if (IS_ERR(info)) {
-		ret = PTR_ERR(info);
-		goto err_vfree;
-	}
-
-	fb = drm_gem_fbdev_fb_create(dev, sizes, 0, gobj, NULL);
-	if (IS_ERR(fb)) {
-		ret = PTR_ERR(fb);
-		goto err_vfree;
-	}
-
-	mfbdev->sysram = sysram;
-	mfbdev->size = size;
-
-	/* setup helper */
-	mfbdev->helper.fb = fb;
-
-	info->fbops = &mgag200fb_ops;
-
-	/* setup aperture base/size for vesafb takeover */
-	info->apertures->ranges[0].base = mdev->dev->mode_config.fb_base;
-	info->apertures->ranges[0].size = mdev->mc.vram_size;
-
-	drm_fb_helper_fill_info(info, &mfbdev->helper, sizes);
-
-	info->screen_base = sysram;
-	info->screen_size = size;
-	info->pixmap.flags = FB_PIXMAP_SYSTEM;
-
-	DRM_DEBUG_KMS("allocated %dx%d\n",
-		      fb->width, fb->height);
-
-	return 0;
-
-err_vfree:
-	vfree(sysram);
-err_drm_gem_object_put_unlocked:
-	drm_gem_object_put_unlocked(gobj);
-	return ret;
-}
-
-static int mga_fbdev_destroy(struct drm_device *dev,
-				struct mga_fbdev *mfbdev)
-{
-	drm_fb_helper_unregister_fbi(&mfbdev->helper);
-	drm_fb_helper_fini(&mfbdev->helper);
-	drm_framebuffer_put(mfbdev->helper.fb);
-
-	vfree(mfbdev->sysram);
-
-	return 0;
-}
-
-static const struct drm_fb_helper_funcs mga_fb_helper_funcs = {
-	.fb_probe = mgag200fb_create,
-};
-
-int mgag200_fbdev_init(struct mga_device *mdev)
-{
-	struct mga_fbdev *mfbdev;
-	int ret;
-	int bpp_sel = 32;
-
-	/* prefer 16bpp on low end gpus with limited VRAM */
-	if (IS_G200_SE(mdev) && mdev->mc.vram_size < (2048*1024))
-		bpp_sel = 16;
-
-	mfbdev = devm_kzalloc(mdev->dev->dev, sizeof(struct mga_fbdev),
GFP_KERNEL);
-	if (!mfbdev)
-		return -ENOMEM;
-
-	mdev->mfbdev = mfbdev;
-	spin_lock_init(&mfbdev->dirty_lock);
-
-	drm_fb_helper_prepare(mdev->dev, &mfbdev->helper,
&mga_fb_helper_funcs);
-
-	ret = drm_fb_helper_init(mdev->dev, &mfbdev->helper,
-				 MGAG200FB_CONN_LIMIT);
-	if (ret)
-		goto err_fb_helper;
-
-	ret = drm_fb_helper_single_add_all_connectors(&mfbdev->helper);
-	if (ret)
-		goto err_fb_setup;
-
-	/* disable all the possible outputs/crtcs before entering KMS mode */
-	drm_helper_disable_unused_functions(mdev->dev);
-
-	ret = drm_fb_helper_initial_config(&mfbdev->helper, bpp_sel);
-	if (ret)
-		goto err_fb_setup;
-
-	return 0;
-
-err_fb_setup:
-	drm_fb_helper_fini(&mfbdev->helper);
-err_fb_helper:
-	mdev->mfbdev = NULL;
-
-	return ret;
-}
-
-void mgag200_fbdev_fini(struct mga_device *mdev)
-{
-	if (!mdev->mfbdev)
-		return;
-
-	mga_fbdev_destroy(mdev->dev, mdev->mfbdev);
-}
diff --git a/drivers/gpu/drm/mgag200/mgag200_main.c
b/drivers/gpu/drm/mgag200/mgag200_main.c
index b10f7265b5c4..6d943a008752 100644
--- a/drivers/gpu/drm/mgag200/mgag200_main.c
+++ b/drivers/gpu/drm/mgag200/mgag200_main.c
@@ -14,8 +14,33 @@
 
 #include "mgag200_drv.h"
 
+static int mga_framebuffer_dirtyfb(struct drm_framebuffer *fb,
+				   struct drm_file *file_priv,
+				   unsigned int flags,
+				   unsigned int color,
+				   struct drm_clip_rect *clips,
+				   unsigned int num_clips)
+{
+	/* empty placeholder function to enable fbcon shadow buffer */
+	return 0;
+}
+
+static const struct drm_framebuffer_funcs mga_framebuffer_funcs = {
+	.destroy	= drm_gem_fb_destroy,
+	.create_handle	= drm_gem_fb_create_handle,
+	.dirty		= mga_framebuffer_dirtyfb,
+};
+
+static struct drm_framebuffer *
+mga_mode_config_funcs_fb_create(struct drm_device *dev, struct drm_file *file,
+				const struct drm_mode_fb_cmd2 *mode_cmd)
+{
+	return drm_gem_fb_create_with_funcs(dev, file, mode_cmd,
+					    &mga_framebuffer_funcs);
+}
+
 static const struct drm_mode_config_funcs mga_mode_funcs = {
-	.fb_create = drm_gem_fb_create
+	.fb_create = mga_mode_config_funcs_fb_create
 };
 
 static int mga_probe_vram(struct mga_device *mdev, void __iomem *mem)
@@ -162,7 +187,7 @@ int mgag200_driver_load(struct drm_device *dev, unsigned
long flags)
 	if (IS_G200_SE(mdev) && mdev->mc.vram_size < (2048*1024))
 		dev->mode_config.preferred_depth = 16;
 	else
-		dev->mode_config.preferred_depth = 24;
+		dev->mode_config.preferred_depth = 32;
 	dev->mode_config.prefer_shadow = 1;
 
 	r = mgag200_modeset_init(mdev);
@@ -186,6 +211,13 @@ int mgag200_driver_load(struct drm_device *dev, unsigned
long flags)
 	}
 	mdev->cursor.pixels_current = NULL;
 
+	r = drm_fbdev_generic_setup(mdev->dev, 0);
+	if (r) {
+		dev_err(&dev->pdev->dev,
+			"drm_fbdev_generic_setup failed: %d\n", r);
+		goto err_modeset;
+	}
+
 	return 0;
 
 err_modeset:
@@ -204,32 +236,7 @@ void mgag200_driver_unload(struct drm_device *dev)
 	if (mdev == NULL)
 		return;
 	mgag200_modeset_fini(mdev);
-	mgag200_fbdev_fini(mdev);
 	drm_mode_config_cleanup(dev);
 	mgag200_mm_fini(mdev);
 	dev->dev_private = NULL;
 }
-
-int mgag200_gem_create(struct drm_device *dev,
-		   u32 size, bool iskernel,
-		   struct drm_gem_object **obj)
-{
-	struct drm_gem_vram_object *gbo;
-	int ret;
-
-	*obj = NULL;
-
-	size = roundup(size, PAGE_SIZE);
-	if (size == 0)
-		return -EINVAL;
-
-	gbo = drm_gem_vram_create(dev, &dev->vram_mm->bdev, size, 0, false);
-	if (IS_ERR(gbo)) {
-		ret = PTR_ERR(gbo);
-		if (ret != -ERESTARTSYS)
-			DRM_ERROR("failed to allocate GEM object\n");
-		return ret;
-	}
-	*obj = &gbo->gem;
-	return 0;
-}
diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c
b/drivers/gpu/drm/mgag200/mgag200_mode.c
index a7cef78d426f..822f2a13748f 100644
--- a/drivers/gpu/drm/mgag200/mgag200_mode.c
+++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
@@ -860,18 +860,12 @@ static int mga_crtc_do_set_base(struct drm_crtc *crtc,
 				struct drm_framebuffer *fb,
 				int x, int y, int atomic)
 {
-	struct mga_device *mdev = crtc->dev->dev_private;
 	struct drm_gem_vram_object *gbo;
 	int ret;
 	s64 gpu_addr;
-	void *base;
 
 	if (!atomic && fb) {
 		gbo = drm_gem_vram_of_gem(fb->obj[0]);
-
-		/* unmap if console */
-		if (mdev->mfbdev->helper.fb == fb)
-			drm_gem_vram_kunmap(gbo);
 		drm_gem_vram_unpin(gbo);
 	}
 
@@ -886,15 +880,6 @@ static int mga_crtc_do_set_base(struct drm_crtc *crtc,
 		goto err_drm_gem_vram_unpin;
 	}
 
-	if (mdev->mfbdev->helper.fb == crtc->primary->fb) {
-		/* if pushing console in kmap it */
-		base = drm_gem_vram_kmap(gbo, true, NULL);
-		if (IS_ERR(base)) {
-			ret = PTR_ERR(base);
-			DRM_ERROR("failed to kmap fbcon\n");
-		}
-	}
-
 	mga_set_start_address(crtc, (u32)gpu_addr);
 
 	return 0;
@@ -1418,14 +1403,9 @@ static void mga_crtc_disable(struct drm_crtc *crtc)
 	DRM_DEBUG_KMS("\n");
 	mga_crtc_dpms(crtc, DRM_MODE_DPMS_OFF);
 	if (crtc->primary->fb) {
-		struct mga_device *mdev = crtc->dev->dev_private;
 		struct drm_framebuffer *fb = crtc->primary->fb;
 		struct drm_gem_vram_object *gbo  			drm_gem_vram_of_gem(fb->obj[0]);
-
-		/* unmap if console */
-		if (mdev->mfbdev->helper.fb == fb)
-			drm_gem_vram_kunmap(gbo);
 		drm_gem_vram_unpin(gbo);
 	}
 	crtc->primary->fb = NULL;
@@ -1718,7 +1698,6 @@ int mgag200_modeset_init(struct mga_device *mdev)
 {
 	struct drm_encoder *encoder;
 	struct drm_connector *connector;
-	int ret;
 
 	mdev->mode_info.mode_config_initialized = true;
 
@@ -1743,12 +1722,6 @@ int mgag200_modeset_init(struct mga_device *mdev)
 
 	drm_connector_attach_encoder(connector, encoder);
 
-	ret = mgag200_fbdev_init(mdev);
-	if (ret) {
-		DRM_ERROR("mga_fbdev_init failed\n");
-		return ret;
-	}
-
 	return 0;
 }
 
-- 
2.21.0

Den 03.07.2019 10.32, skrev Thomas Zimmermann:
> DRM clients, such as the fbdev emulation, have their buffer objects
> mapped by default. Mapping a buffer implicitly prevents its relocation.
> Hence, the buffer may permanently consume video memory while it's
> allocated. This is a problem for drivers of low-memory devices, such as
> ast, mgag200 or older framebuffer hardware, which will then not have
> enough memory to display other content (e.g., X11).
> 
> This patch introduces drm_client_buffer_vmap() and _vunmap(). Internal
> DRM clients can use these functions to unmap and remap buffer objects
> as needed.
> 
> There's no reference counting for vmap operations. Callers are expected
> to either keep buffers mapped (as it is now), or call vmap and vunmap
> in pairs around code that accesses the mapped memory.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann at suse.de>
> ---
>  drivers/gpu/drm/drm_client.c | 71 +++++++++++++++++++++++++++++++-----
>  include/drm/drm_client.h     |  3 ++
>  2 files changed, 64 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_client.c b/drivers/gpu/drm/drm_client.c
> index 410572f14257..d04660c4470a 100644
> --- a/drivers/gpu/drm/drm_client.c
> +++ b/drivers/gpu/drm/drm_client.c
> @@ -235,7 +235,8 @@ static void drm_client_buffer_delete(struct
drm_client_buffer *buffer)
>  {
>  	struct drm_device *dev = buffer->client->dev;
>  
> -	drm_gem_vunmap(buffer->gem, buffer->vaddr);
> +	if (buffer->vaddr)
No need for this, drm_gem_vunmap() has a NULL check.
> +		drm_gem_vunmap(buffer->gem, buffer->vaddr);
>  
>  	if (buffer->gem)
>  		drm_gem_object_put_unlocked(buffer->gem);
> @@ -281,6 +282,43 @@ drm_client_buffer_create(struct drm_client_dev
*client, u32 width, u32 height, u
>  
>  	buffer->gem = obj;
>  
> +	vaddr = drm_client_buffer_vmap(buffer);
I think we should change this and _not_ vmap on buffer creation.
Eventually we'll get bootsplash and console clients and they will also
have to deal with these low memory devices. AFAICS the only client that
will need a virtual address at all times is the fbdev client when it
doesn't use a shadow buffer.
> +	if (IS_ERR(vaddr)) {
> +		ret = PTR_ERR(vaddr);
> +		goto err_delete;
> +	}
> +
> +	return buffer;
> +
> +err_delete:
> +	drm_client_buffer_delete(buffer);
> +
> +	return ERR_PTR(ret);
> +}
> +
> +/**
> + * drm_client_buffer_vmap - Map DRM client buffer into address space
> + * @buffer: DRM client buffer
> + *
> + * This function maps a client buffer into kernel address space. If the
> + * buffer is already mapped, it returns the mapping's address.
> + *
> + * Client buffer mappings are not ref'counted. Each call to
> + * drm_client_buffer_vmap() should be followed by a call to
> + * drm_client_buffer_vunmap(); or the client buffer should be mapped
> + * throughout its lifetime. The latter is the default.
> + *
> + * Returns:
> + *	The mapped memory's address
> + */
> +void *
> +drm_client_buffer_vmap(struct drm_client_buffer *buffer)
> +{
> +	void *vaddr;
> +
> +	if (buffer->vaddr)
> +		return buffer->vaddr;
> +
>  	/*
>  	 * FIXME: The dependency on GEM here isn't required, we could
>  	 * convert the driver handle to a dma-buf instead and use the
> @@ -289,21 +327,34 @@ drm_client_buffer_create(struct drm_client_dev
*client, u32 width, u32 height, u
>  	 * fd_install step out of the driver backend hooks, to make that
>  	 * final step optional for internal users.
>  	 */
> -	vaddr = drm_gem_vmap(obj);
> -	if (IS_ERR(vaddr)) {
> -		ret = PTR_ERR(vaddr);
> -		goto err_delete;
> -	}
> +	vaddr = drm_gem_vmap(buffer->gem);
> +	if (IS_ERR(vaddr))
> +		return vaddr;
>  
>  	buffer->vaddr = vaddr;
>  
> -	return buffer;
> +	return vaddr;
> +}
> +EXPORT_SYMBOL(drm_client_buffer_vmap);
>  
> -err_delete:
> -	drm_client_buffer_delete(buffer);
> +/**
> + * drm_client_buffer_vunmap - Unmap DRM client buffer
> + * @buffer: DRM client buffer
> + *
> + * This function removes a client buffer's memory mmapping. This
> + * function is only required by clients that manage their buffers
> + * by themselves. By default, DRM client buffers are mapped throughout
> + * their entire lifetime.
> + */
> +void drm_client_buffer_vunmap(struct drm_client_buffer *buffer)
> +{
> +	if (!buffer->vaddr)
No need for a NULL check here either.

Noralf.
> +		return;
>  
> -	return ERR_PTR(ret);
> +	drm_gem_vunmap(buffer->gem, buffer->vaddr);
> +	buffer->vaddr = NULL;
>  }
> +EXPORT_SYMBOL(drm_client_buffer_vunmap);
>  
>  static void drm_client_buffer_rmfb(struct drm_client_buffer *buffer)
>  {
> diff --git a/include/drm/drm_client.h b/include/drm/drm_client.h
> index 72d51d1e9dd9..e1db1d9da0bf 100644
> --- a/include/drm/drm_client.h
> +++ b/include/drm/drm_client.h
> @@ -149,6 +149,9 @@ struct drm_client_buffer {
>  struct drm_client_buffer *
>  drm_client_framebuffer_create(struct drm_client_dev *client, u32 width,
u32 height, u32 format);
>  void drm_client_framebuffer_delete(struct drm_client_buffer *buffer);
> +void *
> +drm_client_buffer_vmap(struct drm_client_buffer *buffer);
> +void drm_client_buffer_vunmap(struct drm_client_buffer *buffer);
>  
>  int drm_client_modeset_create(struct drm_client_dev *client);
>  void drm_client_modeset_free(struct drm_client_dev *client);
>

Den 03.07.2019 10.33, skrev Thomas Zimmermann:
> This patch replaces ast's framebuffer console with DRM's generic
> implememtation. All respective code is being removed from the driver.
> 
> The console is set up with a shadow buffer. The actual buffer object is
> not permanently pinned in video ram, but just another buffer object that
> the driver moves in and out of vram as necessary. The driver's function
> ast_crtc_do_set_base() used to contain special handling for the framebuffer
> console. With the new generic framebuffer, the driver does not need this
> code an longer.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann at suse.de>
> ---
>  drivers/gpu/drm/ast/Makefile   |   2 +-
>  drivers/gpu/drm/ast/ast_drv.c  |  22 ++-
>  drivers/gpu/drm/ast/ast_drv.h  |  17 --
>  drivers/gpu/drm/ast/ast_fb.c   | 341 ---------------------------------
>  drivers/gpu/drm/ast/ast_main.c |  30 ++-
>  drivers/gpu/drm/ast/ast_mode.c |  21 --
>  6 files changed, 41 insertions(+), 392 deletions(-)
>  delete mode 100644 drivers/gpu/drm/ast/ast_fb.c
> 
> diff --git a/drivers/gpu/drm/ast/Makefile b/drivers/gpu/drm/ast/Makefile
> index b086dae17013..561f7c4199e4 100644
> --- a/drivers/gpu/drm/ast/Makefile
> +++ b/drivers/gpu/drm/ast/Makefile
> @@ -3,6 +3,6 @@
>  # Makefile for the drm device driver.  This driver provides support for
the
>  # Direct Rendering Infrastructure (DRI) in XFree86 4.1.0 and higher.
>  
> -ast-y := ast_drv.o ast_main.o ast_mode.o ast_fb.o ast_ttm.o ast_post.o
ast_dp501.o
> +ast-y := ast_drv.o ast_main.o ast_mode.o ast_ttm.o ast_post.o ast_dp501.o
>  
>  obj-$(CONFIG_DRM_AST) := ast.o
> diff --git a/drivers/gpu/drm/ast/ast_drv.c b/drivers/gpu/drm/ast/ast_drv.c
> index 3811997e78c4..75f55ac1a218 100644
> --- a/drivers/gpu/drm/ast/ast_drv.c
> +++ b/drivers/gpu/drm/ast/ast_drv.c
> @@ -103,25 +103,29 @@ static int ast_drm_freeze(struct drm_device *dev)
>  
>  	pci_save_state(dev->pdev);
>  
> -	console_lock();
> -	ast_fbdev_set_suspend(dev, 1);
> -	console_unlock();
> +	if (dev->fb_helper) {
> +		console_lock();
> +		drm_fb_helper_set_suspend(dev->fb_helper, 1);
> +		console_unlock();
> +	}
You can call drm_fb_helper_set_suspend_unlocked() unconditionally here
and it will do the NULL check and locking for you.
> +
>  	return 0;
>  }
>  
>  static int ast_drm_thaw(struct drm_device *dev)
>  {
> -	int error = 0;
> -
>  	ast_post_gpu(dev);
>  
>  	drm_mode_config_reset(dev);
>  	drm_helper_resume_force_mode(dev);
>  
> -	console_lock();
> -	ast_fbdev_set_suspend(dev, 0);
> -	console_unlock();
> -	return error;
> +	if (dev->fb_helper) {
> +		console_lock();
> +		drm_fb_helper_set_suspend(dev->fb_helper, 0);
> +		console_unlock();
> +	}
Same here.

With that:

Acked-by: Noralf Tr?nnes <noralf at tronnes.org>
> +
> +	return 0;
>  }
>  
>  static int ast_drm_resume(struct drm_device *dev)
> diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h
> index ca794a3fcf8f..907d7480b7ba 100644
> --- a/drivers/gpu/drm/ast/ast_drv.h
> +++ b/drivers/gpu/drm/ast/ast_drv.h
> @@ -81,8 +81,6 @@ enum ast_tx_chip {
>  #define AST_DRAM_4Gx16   7
>  #define AST_DRAM_8Gx16   8
>  
> -struct ast_fbdev;
> -
>  struct ast_private {
>  	struct drm_device *dev;
>  
> @@ -96,8 +94,6 @@ struct ast_private {
>  	uint32_t mclk;
>  	uint32_t vram_size;
>  
> -	struct ast_fbdev *fbdev;
> -
>  	int fb_mtrr;
>  
>  	struct drm_gem_object *cursor_cache;
> @@ -239,14 +235,6 @@ struct ast_encoder {
>  	struct drm_encoder base;
>  };
>  
> -struct ast_fbdev {
> -	struct drm_fb_helper helper; /* must be first */
> -	void *sysram;
> -	int size;
> -	int x1, y1, x2, y2; /* dirty rect */
> -	spinlock_t dirty_lock;
> -};
> -
>  #define to_ast_crtc(x) container_of(x, struct ast_crtc, base)
>  #define to_ast_connector(x) container_of(x, struct ast_connector, base)
>  #define to_ast_encoder(x) container_of(x, struct ast_encoder, base)
> @@ -289,11 +277,6 @@ struct ast_vbios_mode_info {
>  extern int ast_mode_init(struct drm_device *dev);
>  extern void ast_mode_fini(struct drm_device *dev);
>  
> -int ast_fbdev_init(struct drm_device *dev);
> -void ast_fbdev_fini(struct drm_device *dev);
> -void ast_fbdev_set_suspend(struct drm_device *dev, int state);
> -void ast_fbdev_set_base(struct ast_private *ast, unsigned long gpu_addr);
> -
>  #define AST_MM_ALIGN_SHIFT 4
>  #define AST_MM_ALIGN_MASK ((1 << AST_MM_ALIGN_SHIFT) - 1)
>  
> diff --git a/drivers/gpu/drm/ast/ast_fb.c b/drivers/gpu/drm/ast/ast_fb.c
> deleted file mode 100644
> index a849e58b40bc..000000000000
> --- a/drivers/gpu/drm/ast/ast_fb.c
> +++ /dev/null
> @@ -1,341 +0,0 @@
> -/*
> - * Copyright 2012 Red Hat Inc.
> - *
> - * Permission is hereby granted, free of charge, to any person obtaining a
> - * copy of this software and associated documentation files (the
> - * "Software"), to deal in the Software without restriction,
including
> - * without limitation the rights to use, copy, modify, merge, publish,
> - * distribute, sub license, and/or sell copies of the Software, and to
> - * permit persons to whom the Software is furnished to do so, subject to
> - * the following conditions:
> - *
> - * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY
KIND, EXPRESS OR
> - * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
MERCHANTABILITY,
> - * FITNESS FOR A PARTICULAR PURPOSE AND NON-INFRINGEMENT. IN NO EVENT
SHALL
> - * THE COPYRIGHT HOLDERS, AUTHORS AND/OR ITS SUPPLIERS BE LIABLE FOR ANY
CLAIM,
> - * DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
> - * OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR
THE
> - * USE OR OTHER DEALINGS IN THE SOFTWARE.
> - *
> - * The above copyright notice and this permission notice (including the
> - * next paragraph) shall be included in all copies or substantial portions
> - * of the Software.
> - *
> - */
> -/*
> - * Authors: Dave Airlie <airlied at redhat.com>
> - */
> -#include <linux/module.h>
> -#include <linux/kernel.h>
> -#include <linux/errno.h>
> -#include <linux/string.h>
> -#include <linux/mm.h>
> -#include <linux/tty.h>
> -#include <linux/sysrq.h>
> -#include <linux/delay.h>
> -#include <linux/init.h>
> -
> -
> -#include <drm/drmP.h>
> -#include <drm/drm_crtc.h>
> -#include <drm/drm_crtc_helper.h>
> -#include <drm/drm_fb_helper.h>
> -#include <drm/drm_gem_framebuffer_helper.h>
> -#include <drm/drm_util.h>
> -
> -#include "ast_drv.h"
> -
> -static void ast_dirty_update(struct ast_fbdev *afbdev,
> -			     int x, int y, int width, int height)
> -{
> -	int i;
> -	struct drm_gem_vram_object *gbo;
> -	int src_offset, dst_offset;
> -	int ret;
> -	u8 *dst;
> -	bool unmap = false;
> -	bool store_for_later = false;
> -	int x2, y2;
> -	unsigned long flags;
> -	struct drm_framebuffer *fb = afbdev->helper.fb;
> -	int bpp = fb->format->cpp[0];
> -
> -	gbo = drm_gem_vram_of_gem(fb->obj[0]);
> -
> -	if (drm_can_sleep()) {
> -		/* We pin the BO so it won't be moved during the
> -		 * update. The actual location, video RAM or system
> -		 * memory, is not important.
> -		 */
> -		ret = drm_gem_vram_pin(gbo, 0);
> -		if (ret) {
> -			if (ret != -EBUSY)
> -				return;
> -			store_for_later = true;
> -		}
> -	} else {
> -		store_for_later = true;
> -	}
> -
> -	x2 = x + width - 1;
> -	y2 = y + height - 1;
> -	spin_lock_irqsave(&afbdev->dirty_lock, flags);
> -
> -	if (afbdev->y1 < y)
> -		y = afbdev->y1;
> -	if (afbdev->y2 > y2)
> -		y2 = afbdev->y2;
> -	if (afbdev->x1 < x)
> -		x = afbdev->x1;
> -	if (afbdev->x2 > x2)
> -		x2 = afbdev->x2;
> -
> -	if (store_for_later) {
> -		afbdev->x1 = x;
> -		afbdev->x2 = x2;
> -		afbdev->y1 = y;
> -		afbdev->y2 = y2;
> -		spin_unlock_irqrestore(&afbdev->dirty_lock, flags);
> -		return;
> -	}
> -
> -	afbdev->x1 = afbdev->y1 = INT_MAX;
> -	afbdev->x2 = afbdev->y2 = 0;
> -	spin_unlock_irqrestore(&afbdev->dirty_lock, flags);
> -
> -	dst = drm_gem_vram_kmap(gbo, false, NULL);
> -	if (IS_ERR(dst)) {
> -		DRM_ERROR("failed to kmap fb updates\n");
> -		goto out;
> -	} else if (!dst) {
> -		dst = drm_gem_vram_kmap(gbo, true, NULL);
> -		if (IS_ERR(dst)) {
> -			DRM_ERROR("failed to kmap fb updates\n");
> -			goto out;
> -		}
> -		unmap = true;
> -	}
> -
> -	for (i = y; i <= y2; i++) {
> -		/* assume equal stride for now */
> -		src_offset = dst_offset = i * fb->pitches[0] + (x * bpp);
> -		memcpy_toio(dst + dst_offset, afbdev->sysram + src_offset,
> -			    (x2 - x + 1) * bpp);
> -	}
> -
> -	if (unmap)
> -		drm_gem_vram_kunmap(gbo);
> -
> -out:
> -	drm_gem_vram_unpin(gbo);
> -}
> -
> -static void ast_fillrect(struct fb_info *info,
> -			 const struct fb_fillrect *rect)
> -{
> -	struct ast_fbdev *afbdev = info->par;
> -	drm_fb_helper_sys_fillrect(info, rect);
> -	ast_dirty_update(afbdev, rect->dx, rect->dy, rect->width,
> -			 rect->height);
> -}
> -
> -static void ast_copyarea(struct fb_info *info,
> -			 const struct fb_copyarea *area)
> -{
> -	struct ast_fbdev *afbdev = info->par;
> -	drm_fb_helper_sys_copyarea(info, area);
> -	ast_dirty_update(afbdev, area->dx, area->dy, area->width,
> -			 area->height);
> -}
> -
> -static void ast_imageblit(struct fb_info *info,
> -			  const struct fb_image *image)
> -{
> -	struct ast_fbdev *afbdev = info->par;
> -	drm_fb_helper_sys_imageblit(info, image);
> -	ast_dirty_update(afbdev, image->dx, image->dy, image->width,
> -			 image->height);
> -}
> -
> -static struct fb_ops astfb_ops = {
> -	.owner = THIS_MODULE,
> -	.fb_check_var = drm_fb_helper_check_var,
> -	.fb_set_par = drm_fb_helper_set_par,
> -	.fb_fillrect = ast_fillrect,
> -	.fb_copyarea = ast_copyarea,
> -	.fb_imageblit = ast_imageblit,
> -	.fb_pan_display = drm_fb_helper_pan_display,
> -	.fb_blank = drm_fb_helper_blank,
> -	.fb_setcmap = drm_fb_helper_setcmap,
> -};
> -
> -static int astfb_create_object(struct ast_fbdev *afbdev,
> -			       const struct drm_mode_fb_cmd2 *mode_cmd,
> -			       struct drm_gem_object **gobj_p)
> -{
> -	struct drm_device *dev = afbdev->helper.dev;
> -	u32 size;
> -	struct drm_gem_object *gobj;
> -	int ret = 0;
> -
> -	size = mode_cmd->pitches[0] * mode_cmd->height;
> -	ret = ast_gem_create(dev, size, true, &gobj);
> -	if (ret)
> -		return ret;
> -
> -	*gobj_p = gobj;
> -	return ret;
> -}
> -
> -static int astfb_create(struct drm_fb_helper *helper,
> -			struct drm_fb_helper_surface_size *sizes)
> -{
> -	struct ast_fbdev *afbdev > -		container_of(helper, struct ast_fbdev,
helper);
> -	struct drm_device *dev = afbdev->helper.dev;
> -	struct drm_mode_fb_cmd2 mode_cmd;
> -	struct drm_framebuffer *fb;
> -	struct fb_info *info;
> -	int size, ret;
> -	void *sysram;
> -	struct drm_gem_object *gobj = NULL;
> -	mode_cmd.width = sizes->surface_width;
> -	mode_cmd.height = sizes->surface_height;
> -	mode_cmd.pitches[0] = mode_cmd.width * ((sizes->surface_bpp + 7)/8);
> -
> -	mode_cmd.pixel_format = drm_mode_legacy_fb_format(sizes->surface_bpp,
> -							  sizes->surface_depth);
> -
> -	size = mode_cmd.pitches[0] * mode_cmd.height;
> -
> -	ret = astfb_create_object(afbdev, &mode_cmd, &gobj);
> -	if (ret) {
> -		DRM_ERROR("failed to create fbcon backing object %d\n", ret);
> -		return ret;
> -	}
> -
> -	sysram = vmalloc(size);
> -	if (!sysram)
> -		return -ENOMEM;
> -
> -	info = drm_fb_helper_alloc_fbi(helper);
> -	if (IS_ERR(info)) {
> -		ret = PTR_ERR(info);
> -		goto out;
> -	}
> -	fb = drm_gem_fbdev_fb_create(dev, sizes, 0, gobj, NULL);
> -	if (IS_ERR(fb)) {
> -		ret = PTR_ERR(fb);
> -		goto out;
> -	}
> -
> -	afbdev->sysram = sysram;
> -	afbdev->size = size;
> -
> -	afbdev->helper.fb = fb;
> -
> -	info->fbops = &astfb_ops;
> -
> -	info->apertures->ranges[0].base = pci_resource_start(dev->pdev,
0);
> -	info->apertures->ranges[0].size = pci_resource_len(dev->pdev,
0);
> -
> -	drm_fb_helper_fill_info(info, &afbdev->helper, sizes);
> -
> -	info->screen_base = sysram;
> -	info->screen_size = size;
> -
> -	info->pixmap.flags = FB_PIXMAP_SYSTEM;
> -
> -	DRM_DEBUG_KMS("allocated %dx%d\n",
> -		      fb->width, fb->height);
> -
> -	return 0;
> -
> -out:
> -	vfree(sysram);
> -	return ret;
> -}
> -
> -static const struct drm_fb_helper_funcs ast_fb_helper_funcs = {
> -	.fb_probe = astfb_create,
> -};
> -
> -static void ast_fbdev_destroy(struct drm_device *dev,
> -			      struct ast_fbdev *afbdev)
> -{
> -	drm_helper_force_disable_all(dev);
> -	drm_fb_helper_unregister_fbi(&afbdev->helper);
> -
> -	drm_fb_helper_fini(&afbdev->helper);
> -	drm_framebuffer_put(afbdev->helper.fb);
> -
> -	vfree(afbdev->sysram);
> -}
> -
> -int ast_fbdev_init(struct drm_device *dev)
> -{
> -	struct ast_private *ast = dev->dev_private;
> -	struct ast_fbdev *afbdev;
> -	int ret;
> -
> -	afbdev = kzalloc(sizeof(struct ast_fbdev), GFP_KERNEL);
> -	if (!afbdev)
> -		return -ENOMEM;
> -
> -	ast->fbdev = afbdev;
> -	spin_lock_init(&afbdev->dirty_lock);
> -
> -	drm_fb_helper_prepare(dev, &afbdev->helper,
&ast_fb_helper_funcs);
> -
> -	ret = drm_fb_helper_init(dev, &afbdev->helper, 1);
> -	if (ret)
> -		goto free;
> -
> -	ret = drm_fb_helper_single_add_all_connectors(&afbdev->helper);
> -	if (ret)
> -		goto fini;
> -
> -	/* disable all the possible outputs/crtcs before entering KMS mode */
> -	drm_helper_disable_unused_functions(dev);
> -
> -	ret = drm_fb_helper_initial_config(&afbdev->helper, 32);
> -	if (ret)
> -		goto fini;
> -
> -	return 0;
> -
> -fini:
> -	drm_fb_helper_fini(&afbdev->helper);
> -free:
> -	kfree(afbdev);
> -	return ret;
> -}
> -
> -void ast_fbdev_fini(struct drm_device *dev)
> -{
> -	struct ast_private *ast = dev->dev_private;
> -
> -	if (!ast->fbdev)
> -		return;
> -
> -	ast_fbdev_destroy(dev, ast->fbdev);
> -	kfree(ast->fbdev);
> -	ast->fbdev = NULL;
> -}
> -
> -void ast_fbdev_set_suspend(struct drm_device *dev, int state)
> -{
> -	struct ast_private *ast = dev->dev_private;
> -
> -	if (!ast->fbdev)
> -		return;
> -
> -	drm_fb_helper_set_suspend(&ast->fbdev->helper, state);
> -}
> -
> -void ast_fbdev_set_base(struct ast_private *ast, unsigned long gpu_addr)
> -{
> -	ast->fbdev->helper.fbdev->fix.smem_start > -	
ast->fbdev->helper.fbdev->apertures->ranges[0].base + gpu_addr;
> -	ast->fbdev->helper.fbdev->fix.smem_len = ast->vram_size -
gpu_addr;
> -}
> diff --git a/drivers/gpu/drm/ast/ast_main.c
b/drivers/gpu/drm/ast/ast_main.c
> index 1bd61696e509..78e8e20ff577 100644
> --- a/drivers/gpu/drm/ast/ast_main.c
> +++ b/drivers/gpu/drm/ast/ast_main.c
> @@ -383,8 +383,33 @@ static int ast_get_dram_info(struct drm_device *dev)
>  	return 0;
>  }
>  
> +static int ast_framebuffer_dirtyfb(struct drm_framebuffer *fb,
> +				   struct drm_file *file_priv,
> +				   unsigned int flags,
> +				   unsigned int color,
> +				   struct drm_clip_rect *clips,
> +				   unsigned int num_clips)
> +{
> +	/* empty placeholder function to enable fbcon shadow buffer */
> +	return 0;
> +}
> +
> +static const struct drm_framebuffer_funcs ast_framebuffer_funcs = {
> +	.destroy	= drm_gem_fb_destroy,
> +	.create_handle	= drm_gem_fb_create_handle,
> +	.dirty		= ast_framebuffer_dirtyfb,
> +};
> +
> +static struct drm_framebuffer *
> +ast_mode_config_fb_create(struct drm_device *dev, struct drm_file *file,
> +			  const struct drm_mode_fb_cmd2 *mode_cmd)
> +{
> +	return drm_gem_fb_create_with_funcs(dev, file, mode_cmd,
> +					    &ast_framebuffer_funcs);
> +}
> +
>  static const struct drm_mode_config_funcs ast_mode_funcs = {
> -	.fb_create = drm_gem_fb_create
> +	.fb_create = ast_mode_config_fb_create
>  };
>  
>  static u32 ast_get_vram_info(struct drm_device *dev)
> @@ -502,7 +527,7 @@ int ast_driver_load(struct drm_device *dev, unsigned
long flags)
>  	if (ret)
>  		goto out_free;
>  
> -	ret = ast_fbdev_init(dev);
> +	ret = drm_fbdev_generic_setup(dev, 32);
>  	if (ret)
>  		goto out_free;
>  
> @@ -520,7 +545,6 @@ void ast_driver_unload(struct drm_device *dev)
>  	ast_release_firmware(dev);
>  	kfree(ast->dp501_fw_addr);
>  	ast_mode_fini(dev);
> -	ast_fbdev_fini(dev);
>  	drm_mode_config_cleanup(dev);
>  
>  	ast_mm_fini(ast);
> diff --git a/drivers/gpu/drm/ast/ast_mode.c
b/drivers/gpu/drm/ast/ast_mode.c
> index c48249df758e..3c0716891b29 100644
> --- a/drivers/gpu/drm/ast/ast_mode.c
> +++ b/drivers/gpu/drm/ast/ast_mode.c
> @@ -525,18 +525,12 @@ static int ast_crtc_do_set_base(struct drm_crtc
*crtc,
>  				struct drm_framebuffer *fb,
>  				int x, int y, int atomic)
>  {
> -	struct ast_private *ast = crtc->dev->dev_private;
>  	struct drm_gem_vram_object *gbo;
>  	int ret;
>  	s64 gpu_addr;
> -	void *base;
>  
>  	if (!atomic && fb) {
>  		gbo = drm_gem_vram_of_gem(fb->obj[0]);
> -
> -		/* unmap if console */
> -		if (ast->fbdev->helper.fb == fb)
> -			drm_gem_vram_kunmap(gbo);
>  		drm_gem_vram_unpin(gbo);
>  	}
>  
> @@ -551,17 +545,6 @@ static int ast_crtc_do_set_base(struct drm_crtc *crtc,
>  		goto err_drm_gem_vram_unpin;
>  	}
>  
> -	if (ast->fbdev->helper.fb == crtc->primary->fb) {
> -		/* if pushing console in kmap it */
> -		base = drm_gem_vram_kmap(gbo, true, NULL);
> -		if (IS_ERR(base)) {
> -			ret = PTR_ERR(base);
> -			DRM_ERROR("failed to kmap fbcon\n");
> -		} else {
> -			ast_fbdev_set_base(ast, gpu_addr);
> -		}
> -	}
> -
>  	ast_set_offset_reg(crtc);
>  	ast_set_start_address_crt1(crtc, (u32)gpu_addr);
>  
> @@ -618,14 +601,10 @@ static void ast_crtc_disable(struct drm_crtc *crtc)
>  	DRM_DEBUG_KMS("\n");
>  	ast_crtc_dpms(crtc, DRM_MODE_DPMS_OFF);
>  	if (crtc->primary->fb) {
> -		struct ast_private *ast = crtc->dev->dev_private;
>  		struct drm_framebuffer *fb = crtc->primary->fb;
>  		struct drm_gem_vram_object *gbo >  		
drm_gem_vram_of_gem(fb->obj[0]);
>  
> -		/* unmap if console */
> -		if (ast->fbdev->helper.fb == fb)
> -			drm_gem_vram_kunmap(gbo);
>  		drm_gem_vram_unpin(gbo);
>  	}
>  	crtc->primary->fb = NULL;
>

Den 03.07.2019 10.33, skrev Thomas Zimmermann:
> The bochs driver (and virtual hardware) requires buffer objects to
> reside in video ram to display them to the screen. So it can not
> display the framebuffer console because the respective buffer object
> is permanently pinned in system memory.
> 
> Using a shadow buffer for the console solves this problem. The console
> emulation will pin the buffer object only during updates from the shadow
> buffer. Otherwise, the bochs driver can freely relocated the buffer
> between system memory and video ram.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann at suse.de>
> ---
Acked-by: Noralf Tr?nnes <noralf at tronnes.org>

Den 03.07.2019 10.33, skrev Thomas Zimmermann:
> This patch replaces mgag200's framebuffer console with DRM's
generic
> implememtation. All respective code is being removed from the driver.
> 
> The console is set up with a shadow buffer. The actual buffer object is
> not permanently pinned in video ram, but just another buffer object that
> the driver moves in and out of vram as necessary. The driver's function
> mga_crtc_do_set_base() used to contain special handling for the framebuffer
> console. With the new generic framebuffer, the driver does not need this
> code an longer.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann at suse.de>
> ---
<snip>
> diff --git a/drivers/gpu/drm/mgag200/mgag200_main.c
b/drivers/gpu/drm/mgag200/mgag200_main.c
> index b10f7265b5c4..6d943a008752 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_main.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_main.c
> @@ -14,8 +14,33 @@
>  
>  #include "mgag200_drv.h"
>  
> +static int mga_framebuffer_dirtyfb(struct drm_framebuffer *fb,
> +				   struct drm_file *file_priv,
> +				   unsigned int flags,
> +				   unsigned int color,
> +				   struct drm_clip_rect *clips,
> +				   unsigned int num_clips)
> +{
> +	/* empty placeholder function to enable fbcon shadow buffer */
> +	return 0;
> +}
> +
> +static const struct drm_framebuffer_funcs mga_framebuffer_funcs = {
> +	.destroy	= drm_gem_fb_destroy,
> +	.create_handle	= drm_gem_fb_create_handle,
> +	.dirty		= mga_framebuffer_dirtyfb,
> +};
> +
> +static struct drm_framebuffer *
> +mga_mode_config_funcs_fb_create(struct drm_device *dev, struct drm_file
*file,
> +				const struct drm_mode_fb_cmd2 *mode_cmd)
> +{
> +	return drm_gem_fb_create_with_funcs(dev, file, mode_cmd,
> +					    &mga_framebuffer_funcs);
> +}
> +
>  static const struct drm_mode_config_funcs mga_mode_funcs = {
> -	.fb_create = drm_gem_fb_create
> +	.fb_create = mga_mode_config_funcs_fb_create
>  };
>  
>  static int mga_probe_vram(struct mga_device *mdev, void __iomem *mem)
> @@ -162,7 +187,7 @@ int mgag200_driver_load(struct drm_device *dev,
unsigned long flags)
>  	if (IS_G200_SE(mdev) && mdev->mc.vram_size < (2048*1024))
>  		dev->mode_config.preferred_depth = 16;
>  	else
> -		dev->mode_config.preferred_depth = 24;
> +		dev->mode_config.preferred_depth = 32;
Please mention this change and the reason for it in the commit message.
>  	dev->mode_config.prefer_shadow = 1;
>  
>  	r = mgag200_modeset_init(mdev);
> @@ -186,6 +211,13 @@ int mgag200_driver_load(struct drm_device *dev,
unsigned long flags)
>  	}
>  	mdev->cursor.pixels_current = NULL;
>  
> +	r = drm_fbdev_generic_setup(mdev->dev, 0);
> +	if (r) {
> +		dev_err(&dev->pdev->dev,
> +			"drm_fbdev_generic_setup failed: %d\n", r);
drm_fbdev_generic_setup() will print an error message on failure so you
don't need to do it.

Acked-by: Noralf Tr?nnes <noralf at tronnes.org>
> +		goto err_modeset;
> +	}
> +
>  	return 0;
>  
>  err_modeset:
> @@ -204,32 +236,7 @@ void mgag200_driver_unload(struct drm_device *dev)
>  	if (mdev == NULL)
>  		return;
>  	mgag200_modeset_fini(mdev);
> -	mgag200_fbdev_fini(mdev);
>  	drm_mode_config_cleanup(dev);
>  	mgag200_mm_fini(mdev);
>  	dev->dev_private = NULL;
>  }
> -
> -int mgag200_gem_create(struct drm_device *dev,
> -		   u32 size, bool iskernel,
> -		   struct drm_gem_object **obj)
> -{
> -	struct drm_gem_vram_object *gbo;
> -	int ret;
> -
> -	*obj = NULL;
> -
> -	size = roundup(size, PAGE_SIZE);
> -	if (size == 0)
> -		return -EINVAL;
> -
> -	gbo = drm_gem_vram_create(dev, &dev->vram_mm->bdev, size, 0,
false);
> -	if (IS_ERR(gbo)) {
> -		ret = PTR_ERR(gbo);
> -		if (ret != -ERESTARTSYS)
> -			DRM_ERROR("failed to allocate GEM object\n");
> -		return ret;
> -	}
> -	*obj = &gbo->gem;
> -	return 0;
> -}

Den 03.07.2019 10.32, skrev Thomas Zimmermann:
> DRM client buffers are permanently mapped throughout their lifetime. This
> prevents us from using generic framebuffer emulation for devices with
> small dedicated video memory, such as ast or mgag200. With fb buffers
> permanently mapped, such devices often won't have enougth space left to
> display other content (e.g., X11).
> 
> This patch set introduces unmappable DRM client buffers for framebuffer
> emulation with shadow buffers. While the shadow buffer remains in system
> memory permanently, the respective buffer object will only be mapped
briefly
> during updates from the shadow buffer. Hence, the driver can relocate he
> buffer object among memory regions as needed.
> 
> The default behoviour for DRM client buffers is still to be permanently
> mapped.
> 
> The patch set converts ast and mgag200 to generic framebuffer emulation
> and removes a large amount of framebuffer code from these drivers. For
> bochs, a problem was reported where the driver could not display the
console
> because it was pinned in system memory. [1] The patch set fixes this bug
> by converting bochs to use the shadow fb.
> 
> The patch set has been tested on ast and mga200 HW.
> 
I've been thinking, this enables dirty tracking in DRM userspace for
these drivers since the dirty callback is now set on all framebuffers.
Is this OK? Should we add a drm_fbdev_generic_shadow_setup() that sets a
flag enabling the shadow buffer instead?

Really nice diffstat by the way :-)

Noralf.
> [1] https://lists.freedesktop.org/archives/dri-devel/2019-June/224423.html
> 
> Thomas Zimmermann (5):
>   drm/client: Support unmapping of DRM client buffers
>   drm/fb-helper: Unmap BO for shadow-buffered framebuffer console
>   drm/ast: Replace struct ast_fbdev with generic framebuffer emulation
>   drm/bochs: Use shadow buffer for bochs framebuffer console
>   drm/mgag200: Replace struct mga_fbdev with generic framebuffer
>     emulation
> 
>  drivers/gpu/drm/ast/Makefile           |   2 +-
>  drivers/gpu/drm/ast/ast_drv.c          |  22 +-
>  drivers/gpu/drm/ast/ast_drv.h          |  17 --
>  drivers/gpu/drm/ast/ast_fb.c           | 341 -------------------------
>  drivers/gpu/drm/ast/ast_main.c         |  30 ++-
>  drivers/gpu/drm/ast/ast_mode.c         |  21 --
>  drivers/gpu/drm/bochs/bochs_kms.c      |   2 +-
>  drivers/gpu/drm/drm_client.c           |  71 ++++-
>  drivers/gpu/drm/drm_fb_helper.c        |  14 +-
>  drivers/gpu/drm/mgag200/Makefile       |   2 +-
>  drivers/gpu/drm/mgag200/mgag200_drv.h  |  19 --
>  drivers/gpu/drm/mgag200/mgag200_fb.c   | 309 ----------------------
>  drivers/gpu/drm/mgag200/mgag200_main.c |  61 +++--
>  drivers/gpu/drm/mgag200/mgag200_mode.c |  27 --
>  include/drm/drm_client.h               |   3 +
>  15 files changed, 154 insertions(+), 787 deletions(-)
>  delete mode 100644 drivers/gpu/drm/ast/ast_fb.c
>  delete mode 100644 drivers/gpu/drm/mgag200/mgag200_fb.c
> 
> --
> 2.21.0
>

Hi

Am 03.07.19 um 21:27 schrieb Noralf Tr?nnes:
> 
> 
> Den 03.07.2019 10.32, skrev Thomas Zimmermann:
>> DRM client buffers are permanently mapped throughout their lifetime.
This
>> prevents us from using generic framebuffer emulation for devices with
>> small dedicated video memory, such as ast or mgag200. With fb buffers
>> permanently mapped, such devices often won't have enougth space
left to
>> display other content (e.g., X11).
>>
>> This patch set introduces unmappable DRM client buffers for framebuffer
>> emulation with shadow buffers. While the shadow buffer remains in
system
>> memory permanently, the respective buffer object will only be mapped
briefly
>> during updates from the shadow buffer. Hence, the driver can relocate
he
>> buffer object among memory regions as needed.
>>
>> The default behoviour for DRM client buffers is still to be permanently
>> mapped.
>>
>> The patch set converts ast and mgag200 to generic framebuffer emulation
>> and removes a large amount of framebuffer code from these drivers. For
>> bochs, a problem was reported where the driver could not display the
console
>> because it was pinned in system memory. [1] The patch set fixes this
bug
>> by converting bochs to use the shadow fb.
>>
>> The patch set has been tested on ast and mga200 HW.
>>
> 
> I've been thinking, this enables dirty tracking in DRM userspace for
> these drivers since the dirty callback is now set on all framebuffers.
> Is this OK? Should we add a drm_fbdev_generic_shadow_setup() that sets a
> flag enabling the shadow buffer instead?
Fbdev emulation is special wrt framebuffer setup and there's no way to
distinguish a regular FB from the fbdev's FB. I've been trying
drm_fbdev_generic_shadow_setup(), but ended up duplicating
functionality. The problem was that we cannot get state-flag arguments
into drm_fb_helper_generic_probe().

There already is struct mode_config.prefer_shadow. It signals shadow FB
rendering to userspace. The easiest solution is to add
prefer_shadow_fbdev as well. If either flag is true, fbdev emulation
would enable shadow buffering.

I'm not sure if we should check for the dirty() callback at all.

Best regards
Thomas
> Really nice diffstat by the way :-)
> 
> Noralf.
> 
>> [1]
https://lists.freedesktop.org/archives/dri-devel/2019-June/224423.html
>>
>> Thomas Zimmermann (5):
>>   drm/client: Support unmapping of DRM client buffers
>>   drm/fb-helper: Unmap BO for shadow-buffered framebuffer console
>>   drm/ast: Replace struct ast_fbdev with generic framebuffer emulation
>>   drm/bochs: Use shadow buffer for bochs framebuffer console
>>   drm/mgag200: Replace struct mga_fbdev with generic framebuffer
>>     emulation
>>
>>  drivers/gpu/drm/ast/Makefile           |   2 +-
>>  drivers/gpu/drm/ast/ast_drv.c          |  22 +-
>>  drivers/gpu/drm/ast/ast_drv.h          |  17 --
>>  drivers/gpu/drm/ast/ast_fb.c           | 341 -------------------------
>>  drivers/gpu/drm/ast/ast_main.c         |  30 ++-
>>  drivers/gpu/drm/ast/ast_mode.c         |  21 --
>>  drivers/gpu/drm/bochs/bochs_kms.c      |   2 +-
>>  drivers/gpu/drm/drm_client.c           |  71 ++++-
>>  drivers/gpu/drm/drm_fb_helper.c        |  14 +-
>>  drivers/gpu/drm/mgag200/Makefile       |   2 +-
>>  drivers/gpu/drm/mgag200/mgag200_drv.h  |  19 --
>>  drivers/gpu/drm/mgag200/mgag200_fb.c   | 309 ----------------------
>>  drivers/gpu/drm/mgag200/mgag200_main.c |  61 +++--
>>  drivers/gpu/drm/mgag200/mgag200_mode.c |  27 --
>>  include/drm/drm_client.h               |   3 +
>>  15 files changed, 154 insertions(+), 787 deletions(-)
>>  delete mode 100644 drivers/gpu/drm/ast/ast_fb.c
>>  delete mode 100644 drivers/gpu/drm/mgag200/mgag200_fb.c
>>
>> --
>> 2.21.0
>>
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Linux GmbH, Maxfeldstrasse 5, 90409 Nuernberg, Germany
GF: Felix Imend?rffer, Mary Higgins, Sri Rasiah
HRB 21284 (AG N?rnberg)

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL:
<http://lists.linuxfoundation.org/pipermail/virtualization/attachments/20190704/3473f4ed/attachment.sig>