qxl: refactor to use drm_fb_helper_fbdev_setup

Lekensteyn · kraxel · commit bf8744e40cd6 · 2018-09-12T09:01:18.000+02:00
Lots of code can be removed by relying on fb-helper: - "struct drm_framebuffer" moves to fb_helper.fb. - "struct drm_gem_object" moves to fb_helper.obj[0]. - "struct qxl_device" can be inferred as drm_fb_helper is embedded. - qxl_user_framebuffer_create -> drm_gem_fb_create. - qxl_user_framebuffer_destroy -> drm_gem_fb_destroy. - qxl_fbdev_destroy -> drm_fb_helper_fbdev_teardown + vfree(shadow). Remove unused code: - qxl_fbdev_qobj_is_fb, qxl_fbdev_set_suspend. - Unused fields of qxl_fbdev: delayed_ops, delayed_ops_lock, size. Misc notes: - The dirty callback is preserved as it is necessary to trigger update commands in the hw (the screen stays black otherwise). - No idea when .create_handle in drm_framebuffer_funcs is used, but use the same drm_gem_fb_create_handle to match drm_gem_fb_funcs. - I don't know why qxl_fb_find_or_create_single used to check for an existing framebuffer and removed that check to match other drivers. - Use of drm_fb_helper_fbdev_teardown also requires "info->fbdefio" to be dynamically allocated. Replace the existing defio config by drm_fb_helper_defio_init to accomodate this. Testing results: startx with fbdev, modesetting and qxl all seems to work. Tested also with CONFIG_DRM_FBDEV_EMULATION=n, fbdev obviously fails but others are fine. QEMU -spice and QEMU -spice with vdagent and multiple (resized) displays (via remote-viewer) also works. unbind vtconsole and rmmod has *not* regressed (i.e. it still trips on a use-after-free in qxl_check_idle via qxl_ttm_fini). Ideally setup/teardown is replaced by drm_fbdev_generic_setup as that would result in further code reduction, improve error handling (like not leaking shadow memory), but unfortunately QXL has no implementation for qxl_gem_prime_vmap. Signed-off-by: Peter Wu <peter@lekensteyn.nl> Link: http://patchwork.freedesktop.org/patch/msgid/20180910132156.23201-1-peter@lekensteyn.nl Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
diff --git a/drivers/gpu/drm/qxl/qxl_display.c b/drivers/gpu/drm/qxl/qxl_display.c
@@ -28,6 +28,7 @@
 #include <drm/drm_plane_helper.h>
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_atomic.h>
+#include <drm/drm_gem_framebuffer_helper.h>
 
 #include "qxl_drv.h"
 #include "qxl_object.h"
@@ -388,33 +389,21 @@ static const struct drm_crtc_funcs qxl_crtc_funcs = {
 	.atomic_destroy_state = drm_atomic_helper_crtc_destroy_state,
 };
 
-void qxl_user_framebuffer_destroy(struct drm_framebuffer *fb)
-{
-	struct qxl_framebuffer *qxl_fb = to_qxl_framebuffer(fb);
-	struct qxl_bo *bo = gem_to_qxl_bo(qxl_fb->obj);
-
-	WARN_ON(bo->shadow);
-	drm_gem_object_put_unlocked(qxl_fb->obj);
-	drm_framebuffer_cleanup(fb);
-	kfree(qxl_fb);
-}
-
 static int qxl_framebuffer_surface_dirty(struct drm_framebuffer *fb,
 					 struct drm_file *file_priv,
 					 unsigned flags, unsigned color,
 					 struct drm_clip_rect *clips,
 					 unsigned num_clips)
 {
 	/* TODO: vmwgfx where this was cribbed from had locking. Why? */
-	struct qxl_framebuffer *qxl_fb = to_qxl_framebuffer(fb);
-	struct qxl_device *qdev = qxl_fb->base.dev->dev_private;
+	struct qxl_device *qdev = fb->dev->dev_private;
 	struct drm_clip_rect norect;
 	struct qxl_bo *qobj;
 	int inc = 1;
 
 	drm_modeset_lock_all(fb->dev);
 
-	qobj = gem_to_qxl_bo(qxl_fb->obj);
+	qobj = gem_to_qxl_bo(fb->obj[0]);
 	/* if we aren't primary surface ignore this */
 	if (!qobj->is_primary) {
 		drm_modeset_unlock_all(fb->dev);
@@ -432,7 +421,7 @@ static int qxl_framebuffer_surface_dirty(struct drm_framebuffer *fb,
 		inc = 2; /* skip source rects */
 	}
 
-	qxl_draw_dirty_fb(qdev, qxl_fb, qobj, flags, color,
+	qxl_draw_dirty_fb(qdev, fb, qobj, flags, color,
 			  clips, num_clips, inc);
 
 	drm_modeset_unlock_all(fb->dev);
@@ -441,31 +430,11 @@ static int qxl_framebuffer_surface_dirty(struct drm_framebuffer *fb,
 }
 
 static const struct drm_framebuffer_funcs qxl_fb_funcs = {
-	.destroy = qxl_user_framebuffer_destroy,
+	.destroy = drm_gem_fb_destroy,
 	.dirty = qxl_framebuffer_surface_dirty,
-/*	TODO?
- *	.create_handle = qxl_user_framebuffer_create_handle, */
+	.create_handle = drm_gem_fb_create_handle,
 };
 
-int
-qxl_framebuffer_init(struct drm_device *dev,
-		     struct qxl_framebuffer *qfb,
-		     const struct drm_mode_fb_cmd2 *mode_cmd,
-		     struct drm_gem_object *obj,
-		     const struct drm_framebuffer_funcs *funcs)
-{
-	int ret;
-
-	qfb->obj = obj;
-	drm_helper_mode_fill_fb_struct(dev, &qfb->base, mode_cmd);
-	ret = drm_framebuffer_init(dev, &qfb->base, funcs);
-	if (ret) {
-		qfb->obj = NULL;
-		return ret;
-	}
-	return 0;
-}
-
 static void qxl_crtc_atomic_enable(struct drm_crtc *crtc,
 				   struct drm_crtc_state *old_state)
 {
@@ -488,14 +457,12 @@ static int qxl_primary_atomic_check(struct drm_plane *plane,
 				    struct drm_plane_state *state)
 {
 	struct qxl_device *qdev = plane->dev->dev_private;
-	struct qxl_framebuffer *qfb;
 	struct qxl_bo *bo;
 
 	if (!state->crtc || !state->fb)
 		return 0;
 
-	qfb = to_qxl_framebuffer(state->fb);
-	bo = gem_to_qxl_bo(qfb->obj);
+	bo = gem_to_qxl_bo(state->fb->obj[0]);
 
 	if (bo->surf.stride * bo->surf.height > qdev->vram_size) {
 		DRM_ERROR("Mode doesn't fit in vram size (vgamem)");
@@ -556,23 +523,19 @@ static void qxl_primary_atomic_update(struct drm_plane *plane,
 				      struct drm_plane_state *old_state)
 {
 	struct qxl_device *qdev = plane->dev->dev_private;
-	struct qxl_framebuffer *qfb =
-		to_qxl_framebuffer(plane->state->fb);
-	struct qxl_framebuffer *qfb_old;
-	struct qxl_bo *bo = gem_to_qxl_bo(qfb->obj);
+	struct qxl_bo *bo = gem_to_qxl_bo(plane->state->fb->obj[0]);
 	struct qxl_bo *bo_old;
 	struct drm_clip_rect norect = {
 	    .x1 = 0,
 	    .y1 = 0,
-	    .x2 = qfb->base.width,
-	    .y2 = qfb->base.height
+	    .x2 = plane->state->fb->width,
+	    .y2 = plane->state->fb->height
 	};
 	int ret;
 	bool same_shadow = false;
 
 	if (old_state->fb) {
-		qfb_old = to_qxl_framebuffer(old_state->fb);
-		bo_old = gem_to_qxl_bo(qfb_old->obj);
+		bo_old = gem_to_qxl_bo(old_state->fb->obj[0]);
 	} else {
 		bo_old = NULL;
 	}
@@ -602,7 +565,7 @@ static void qxl_primary_atomic_update(struct drm_plane *plane,
 		bo->is_primary = true;
 	}
 
-	qxl_draw_dirty_fb(qdev, qfb, bo, 0, 0, &norect, 1, 1);
+	qxl_draw_dirty_fb(qdev, plane->state->fb, bo, 0, 0, &norect, 1, 1);
 }
 
 static void qxl_primary_atomic_disable(struct drm_plane *plane,
@@ -611,9 +574,7 @@ static void qxl_primary_atomic_disable(struct drm_plane *plane,
 	struct qxl_device *qdev = plane->dev->dev_private;
 
 	if (old_state->fb) {
-		struct qxl_framebuffer *qfb =
-			to_qxl_framebuffer(old_state->fb);
-		struct qxl_bo *bo = gem_to_qxl_bo(qfb->obj);
+		struct qxl_bo *bo = gem_to_qxl_bo(old_state->fb->obj[0]);
 
 		if (bo->is_primary) {
 			qxl_io_destroy_primary(qdev);
@@ -645,7 +606,7 @@ static void qxl_cursor_atomic_update(struct drm_plane *plane,
 		return;
 
 	if (fb != old_state->fb) {
-		obj = to_qxl_framebuffer(fb)->obj;
+		obj = fb->obj[0];
 		user_bo = gem_to_qxl_bo(obj);
 
 		/* pinning is done in the prepare/cleanup framevbuffer */
@@ -765,13 +726,13 @@ static int qxl_plane_prepare_fb(struct drm_plane *plane,
 	if (!new_state->fb)
 		return 0;
 
-	obj = to_qxl_framebuffer(new_state->fb)->obj;
+	obj = new_state->fb->obj[0];
 	user_bo = gem_to_qxl_bo(obj);
 
 	if (plane->type == DRM_PLANE_TYPE_PRIMARY &&
 	    user_bo->is_dumb && !user_bo->shadow) {
 		if (plane->state->fb) {
-			obj = to_qxl_framebuffer(plane->state->fb)->obj;
+			obj = plane->state->fb->obj[0];
 			old_bo = gem_to_qxl_bo(obj);
 		}
 		if (old_bo && old_bo->shadow &&
@@ -815,7 +776,7 @@ static void qxl_plane_cleanup_fb(struct drm_plane *plane,
 		return;
 	}
 
-	obj = to_qxl_framebuffer(old_state->fb)->obj;
+	obj = old_state->fb->obj[0];
 	user_bo = gem_to_qxl_bo(obj);
 	qxl_bo_unpin(user_bo);
 
@@ -1115,26 +1076,8 @@ qxl_user_framebuffer_create(struct drm_device *dev,
 			    struct drm_file *file_priv,
 			    const struct drm_mode_fb_cmd2 *mode_cmd)
 {
-	struct drm_gem_object *obj;
-	struct qxl_framebuffer *qxl_fb;
-	int ret;
-
-	obj = drm_gem_object_lookup(file_priv, mode_cmd->handles[0]);
-	if (!obj)
-		return NULL;
-
-	qxl_fb = kzalloc(sizeof(*qxl_fb), GFP_KERNEL);
-	if (qxl_fb == NULL)
-		return NULL;
-
-	ret = qxl_framebuffer_init(dev, qxl_fb, mode_cmd, obj, &qxl_fb_funcs);
-	if (ret) {
-		kfree(qxl_fb);
-		drm_gem_object_put_unlocked(obj);
-		return NULL;
-	}
-
-	return &qxl_fb->base;
+	return drm_gem_fb_create_with_funcs(dev, file_priv, mode_cmd,
+					    &qxl_fb_funcs);
 }
 
 static const struct drm_mode_config_funcs qxl_mode_funcs = {
@@ -1221,7 +1164,6 @@ int qxl_modeset_init(struct qxl_device *qdev)
 	}
 
 	qxl_display_read_client_monitors_config(qdev);
-	qdev->mode_info.mode_config_initialized = true;
 
 	drm_mode_config_reset(&qdev->ddev);
 
@@ -1237,8 +1179,5 @@ void qxl_modeset_fini(struct qxl_device *qdev)
 	qxl_fbdev_fini(qdev);
 
 	qxl_destroy_monitors_object(qdev);
-	if (qdev->mode_info.mode_config_initialized) {
-		drm_mode_config_cleanup(&qdev->ddev);
-		qdev->mode_info.mode_config_initialized = false;
-	}
+	drm_mode_config_cleanup(&qdev->ddev);
 }
diff --git a/drivers/gpu/drm/qxl/qxl_draw.c b/drivers/gpu/drm/qxl/qxl_draw.c
@@ -262,7 +262,7 @@ void qxl_draw_opaque_fb(const struct qxl_fb_image *qxl_fb_image,
  * by treating them differently in the server.
  */
 void qxl_draw_dirty_fb(struct qxl_device *qdev,
-		       struct qxl_framebuffer *qxl_fb,
+		       struct drm_framebuffer *fb,
 		       struct qxl_bo *bo,
 		       unsigned flags, unsigned color,
 		       struct drm_clip_rect *clips,
@@ -281,9 +281,9 @@ void qxl_draw_dirty_fb(struct qxl_device *qdev,
 	struct qxl_drawable *drawable;
 	struct qxl_rect drawable_rect;
 	struct qxl_rect *rects;
-	int stride = qxl_fb->base.pitches[0];
+	int stride = fb->pitches[0];
 	/* depth is not actually interesting, we don't mask with it */
-	int depth = qxl_fb->base.format->cpp[0] * 8;
+	int depth = fb->format->cpp[0] * 8;
 	uint8_t *surface_base;
 	struct qxl_release *release;
 	struct qxl_bo *clips_bo;
diff --git a/drivers/gpu/drm/qxl/qxl_drv.h b/drivers/gpu/drm/qxl/qxl_drv.h
@@ -38,6 +38,7 @@
 
 #include <drm/drm_crtc.h>
 #include <drm/drm_encoder.h>
+#include <drm/drm_fb_helper.h>
 #include <drm/drm_gem.h>
 #include <drm/drmP.h>
 #include <drm/ttm/ttm_bo_api.h>
@@ -121,15 +122,9 @@ struct qxl_output {
 	struct drm_encoder enc;
 };
 
-struct qxl_framebuffer {
-	struct drm_framebuffer base;
-	struct drm_gem_object *obj;
-};
-
 #define to_qxl_crtc(x) container_of(x, struct qxl_crtc, base)
 #define drm_connector_to_qxl_output(x) container_of(x, struct qxl_output, base)
 #define drm_encoder_to_qxl_output(x) container_of(x, struct qxl_output, enc)
-#define to_qxl_framebuffer(x) container_of(x, struct qxl_framebuffer, base)
 
 struct qxl_mman {
 	struct ttm_bo_global_ref        bo_global_ref;
@@ -138,13 +133,6 @@ struct qxl_mman {
 	struct ttm_bo_device		bdev;
 };
 
-struct qxl_mode_info {
-	bool mode_config_initialized;
-
-	/* pointer to fbdev info structure */
-	struct qxl_fbdev *qfbdev;
-};
-
 
 struct qxl_memslot {
 	uint8_t		generation;
@@ -232,10 +220,9 @@ struct qxl_device {
 	void *ram;
 	struct qxl_mman		mman;
 	struct qxl_gem		gem;
-	struct qxl_mode_info mode_info;
 
-	struct fb_info			*fbdev_info;
-	struct qxl_framebuffer	*fbdev_qfb;
+	struct drm_fb_helper	fb_helper;
+
 	void *ram_physical;
 
 	struct qxl_ring *release_ring;
@@ -349,19 +336,8 @@ qxl_bo_physical_address(struct qxl_device *qdev, struct qxl_bo *bo,
 
 int qxl_fbdev_init(struct qxl_device *qdev);
 void qxl_fbdev_fini(struct qxl_device *qdev);
-int qxl_get_handle_for_primary_fb(struct qxl_device *qdev,
-				  struct drm_file *file_priv,
-				  uint32_t *handle);
-void qxl_fbdev_set_suspend(struct qxl_device *qdev, int state);
 
 /* qxl_display.c */
-void qxl_user_framebuffer_destroy(struct drm_framebuffer *fb);
-int
-qxl_framebuffer_init(struct drm_device *dev,
-		     struct qxl_framebuffer *rfb,
-		     const struct drm_mode_fb_cmd2 *mode_cmd,
-		     struct drm_gem_object *obj,
-		     const struct drm_framebuffer_funcs *funcs);
 void qxl_display_read_client_monitors_config(struct qxl_device *qdev);
 int qxl_create_monitors_object(struct qxl_device *qdev);
 int qxl_destroy_monitors_object(struct qxl_device *qdev);
@@ -471,7 +447,7 @@ void qxl_draw_opaque_fb(const struct qxl_fb_image *qxl_fb_image,
 			int stride /* filled in if 0 */);
 
 void qxl_draw_dirty_fb(struct qxl_device *qdev,
-		       struct qxl_framebuffer *qxl_fb,
+		       struct drm_framebuffer *fb,
 		       struct qxl_bo *bo,
 		       unsigned flags, unsigned color,
 		       struct drm_clip_rect *clips,
diff --git a/drivers/gpu/drm/qxl/qxl_fb.c b/drivers/gpu/drm/qxl/qxl_fb.c
