Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1383505 > unrolled thread

[PATCH 0/8] drm: Add fbdev deferred io support to helpers

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/8] drm: Add fbdev deferred io support to helpers Noralf Trønnes <noralf@tronnes.org> - 2016-04-20 17:40 +0200
    [PATCH 1/8] drm/rect: Add some drm_clip_rect utility functions Noralf Trønnes <noralf@tronnes.org> - 2016-04-20 17:40 +0200
    [PATCH 7/8] drm/qxl: Use drm_fb_helper deferred_io support Noralf Trønnes <noralf@tronnes.org> - 2016-04-20 17:40 +0200
      Re: [PATCH 7/8] drm/qxl: Use drm_fb_helper deferred_io support Daniel Vetter <daniel@ffwll.ch> - 2016-04-20 19:50 +0200
        Re: [PATCH 7/8] drm/qxl: Use drm_fb_helper deferred_io support Noralf Trønnes <noralf@tronnes.org> - 2016-04-20 21:10 +0200
          Re: [PATCH 7/8] drm/qxl: Use drm_fb_helper deferred_io support Daniel Vetter <daniel@ffwll.ch> - 2016-04-21 09:50 +0200
            Re: [PATCH 7/8] drm/qxl: Use drm_fb_helper deferred_io support Daniel Vetter <daniel@ffwll.ch> - 2016-04-21 09:50 +0200
              Re: [PATCH 7/8] drm/qxl: Use drm_fb_helper deferred_io support Daniel Vetter <daniel@ffwll.ch> - 2016-04-21 10:00 +0200

#1383505 — [PATCH 0/8] drm: Add fbdev deferred io support to helpers

This patchset adds fbdev deferred io support to drm_fb_helper and
drm_fb_cma_helper.

It defers fbdev mmap and fb_{write,fillrect,copyarea,imageblit} damage and
channels it through the (struct drm_framebuffer_funcs)->dirty callback on
the fb_helper framebuffer which will always run in process context.

I have also added patches that converts qxl and udl to use this
deferred io support. I have only compile tested it, no functional testing.
I know that qxl is purely a software thing so I could actually test it, but
I have never used qemu so I'm not keen on spending a lot of time on that.

This was originally part of the tinydrm patchset.

Changes since RFC:
- Fix drm_clip_rect use to be exclusive on x2/y2
- Put drm_clip_rect functions in drm_rect.{h,c}
- Take into account that (struct fb_ops *)->fb_{write,...}() can be called
  from atomic context (spin_lock_irqsave)
- Export fb_deferred_io_mmap()
- Add some more documentation
- Add qxl and udl patches

Noralf Trønnes (8):
  drm/rect: Add some drm_clip_rect utility functions
  drm/udl: Change drm_fb_helper_sys_*() calls to sys_*()
  drm/qxl: Change drm_fb_helper_sys_*() calls to sys_*()
  drm/fb-helper: Add fb_deferred_io support
  fbdev: fb_defio: Export fb_deferred_io_mmap
  drm/fb-cma-helper: Add fb_deferred_io support
  drm/qxl: Use drm_fb_helper deferred_io support
  drm/udl: Use drm_fb_helper deferred_io support

 drivers/gpu/drm/drm_fb_cma_helper.c | 190 +++++++++++++++++++++++++++++--
 drivers/gpu/drm/drm_fb_helper.c     | 119 ++++++++++++++++++-
 drivers/gpu/drm/drm_rect.c          |  67 +++++++++++
 drivers/gpu/drm/qxl/qxl_display.c   |   9 +-
 drivers/gpu/drm/qxl/qxl_drv.h       |   7 +-
 drivers/gpu/drm/qxl/qxl_fb.c        | 220 +++++++++---------------------------
 drivers/gpu/drm/qxl/qxl_kms.c       |   4 -
 drivers/gpu/drm/udl/udl_drv.h       |   2 -
 drivers/gpu/drm/udl/udl_fb.c        | 152 ++-----------------------
 drivers/video/fbdev/core/fb_defio.c |   3 +-
 include/drm/drm_fb_cma_helper.h     |  14 +++
 include/drm/drm_fb_helper.h         |  15 +++
 include/drm/drm_rect.h              |  69 +++++++++++
 include/linux/fb.h                  |   1 +
 14 files changed, 538 insertions(+), 334 deletions(-)

--
2.2.2

[toc] | [next] | [standalone]

#1383506 — [PATCH 1/8] drm/rect: Add some drm_clip_rect utility functions

Add some utility functions for struct drm_clip_rect.

Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---
 drivers/gpu/drm/drm_rect.c | 67 ++++++++++++++++++++++++++++++++++++++++++++
 include/drm/drm_rect.h     | 69 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 136 insertions(+)

diff --git a/drivers/gpu/drm/drm_rect.c b/drivers/gpu/drm/drm_rect.c
index a8e2c86..a9fb1a8 100644
--- a/drivers/gpu/drm/drm_rect.c
+++ b/drivers/gpu/drm/drm_rect.c
@@ -434,3 +434,70 @@ void drm_rect_rotate_inv(struct drm_rect *r,
 	}
 }
 EXPORT_SYMBOL(drm_rect_rotate_inv);
+
+/**
+ * drm_clip_rect_intersect - intersect two clip rectangles
+ * @r1: first clip rectangle
+ * @r2: second clip rectangle
+ *
+ * Calculate the intersection of clip rectangles @r1 and @r2.
+ * @r1 will be overwritten with the intersection.
+ *
+ * RETURNS:
+ * %true if rectangle @r1 is still visible after the operation,
+ * %false otherwise.
+ */
+bool drm_clip_rect_intersect(struct drm_clip_rect *r1,
+			     const struct drm_clip_rect *r2)
+{
+	r1->x1 = max(r1->x1, r2->x1);
+	r1->y1 = max(r1->y1, r2->y1);
+	r1->x2 = min(r1->x2, r2->x2);
+	r1->y2 = min(r1->y2, r2->y2);
+
+	return drm_clip_rect_visible(r1);
+}
+EXPORT_SYMBOL(drm_clip_rect_intersect);
+
+/**
+ * drm_clip_rect_merge - Merge clip rectangles
+ * @dst: destination clip rectangle
+ * @src: source clip rectangle(s), can be NULL
+ * @num_clips: number of source clip rectangles
+ * @flags: drm_mode_fb_dirty_cmd flags (DRM_MODE_FB_DIRTY_ANNOTATE_COPY)
+ * @width: width of clip rectangle if @src is NULL
+ * @height: height of clip rectangle if @src is NULL
+ *
+ * The dirtyfb ioctl allows for a NULL clip rectangle to be passed in,
+ * so if @src is NULL, width and height is used to set a full clip rectangle.
+ * @dst takes part in the merge unless it is empty {0,0,0,0}.
+ */
+void drm_clip_rect_merge(struct drm_clip_rect *dst,
+			 struct drm_clip_rect *src, unsigned num_clips,
+			 unsigned flags, u32 width, u32 height)
+{
+	int i;
+
+	if (!src || !num_clips) {
+		dst->x1 = 0;
+		dst->x2 = width;
+		dst->y1 = 0;
+		dst->y2 = height;
+		return;
+	}
+
+	if (drm_clip_rect_is_empty(dst)) {
+		dst->x1 = ~0;
+		dst->y1 = ~0;
+	}
+
+	for (i = 0; i < num_clips; i++) {
+		if (flags & DRM_MODE_FB_DIRTY_ANNOTATE_COPY)
+			i++;
+		dst->x1 = min(dst->x1, src[i].x1);
+		dst->x2 = max(dst->x2, src[i].x2);
+		dst->y1 = min(dst->y1, src[i].y1);
+		dst->y2 = max(dst->y2, src[i].y2);
+	}
+}
+EXPORT_SYMBOL(drm_clip_rect_merge);
diff --git a/include/drm/drm_rect.h b/include/drm/drm_rect.h
index 83bb156..936ad8d 100644
--- a/include/drm/drm_rect.h
+++ b/include/drm/drm_rect.h
@@ -24,6 +24,8 @@
 #ifndef DRM_RECT_H
 #define DRM_RECT_H
 
+#include <uapi/drm/drm.h>
+
 /**
  * DOC: rect utils
  *
@@ -171,4 +173,71 @@ void drm_rect_rotate_inv(struct drm_rect *r,
 			 int width, int height,
 			 unsigned int rotation);
 
+/**
+ * drm_clip_rect_width - determine the clip rectangle width
+ * @r: clip rectangle whose width is returned
+ *
+ * RETURNS:
+ * The width of the clip rectangle.
+ */
+static inline int drm_clip_rect_width(const struct drm_clip_rect *r)
+{
+	return r->x2 - r->x1;
+}
+
+/**
+ * drm_clip_rect_height - determine the clip rectangle height
+ * @r: clip rectangle whose height is returned
+ *
+ * RETURNS:
+ * The height of the clip rectangle.
+ */
+static inline int drm_clip_rect_height(const struct drm_clip_rect *r)
+{
+	return r->y2 - r->y1;
+}
+
+/**
+ * drm_clip_rect_visible - determine if the the clip rectangle is visible
+ * @r: clip rectangle whose visibility is returned
+ *
+ * RETURNS:
+ * %true if the clip rectangle is visible, %false otherwise.
+ */
+static inline bool drm_clip_rect_visible(const struct drm_clip_rect *r)
+{
+	return drm_clip_rect_width(r) > 0 && drm_clip_rect_height(r) > 0;
+}
+
+/**
+ * drm_clip_rect_reset - Reset clip rectangle
+ * @clip: clip rectangle
+ *
+ * Sets clip rectangle to {0,0,0,0}.
+ */
+static inline void drm_clip_rect_reset(struct drm_clip_rect *clip)
+{
+	clip->x1 = 0;
+	clip->x2 = 0;
+	clip->y1 = 0;
+	clip->y2 = 0;
+}
+
+/**
+ * drm_clip_rect_is_empty - Is clip rectangle empty?
+ * @clip: clip rectangle
+ *
+ * Returns true if clip rectangle is {0,0,0,0}.
+ */
+static inline bool drm_clip_rect_is_empty(struct drm_clip_rect *clip)
+{
+	return (!clip->x1 && !clip->x2 && !clip->y1 && !clip->y2);
+}
+
+bool drm_clip_rect_intersect(struct drm_clip_rect *r1,
+			     const struct drm_clip_rect *r2);
+void drm_clip_rect_merge(struct drm_clip_rect *dst,
+			 struct drm_clip_rect *src, unsigned num_clips,
+			 unsigned flags, u32 width, u32 height);
+
 #endif
-- 
2.2.2

[toc] | [prev] | [next] | [standalone]

#1383508 — [PATCH 7/8] drm/qxl: Use drm_fb_helper deferred_io support

Use the fbdev deferred io support in drm_fb_helper.
The (struct fb_ops *)->fb_{fillrect,copyarea,imageblit} functions will
now be deferred in the same way that mmap damage is, instead of being
flushed directly.
This patch has only been compile tested.

Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---
 drivers/gpu/drm/qxl/qxl_display.c |   9 +-
 drivers/gpu/drm/qxl/qxl_drv.h     |   7 +-
 drivers/gpu/drm/qxl/qxl_fb.c      | 220 ++++++++++----------------------------
 drivers/gpu/drm/qxl/qxl_kms.c     |   4 -
 4 files changed, 62 insertions(+), 178 deletions(-)

diff --git a/drivers/gpu/drm/qxl/qxl_display.c b/drivers/gpu/drm/qxl/qxl_display.c
index 030409a..9a03524 100644
--- a/drivers/gpu/drm/qxl/qxl_display.c
+++ b/drivers/gpu/drm/qxl/qxl_display.c
@@ -465,7 +465,7 @@ static const struct drm_crtc_funcs qxl_crtc_funcs = {
 	.page_flip = qxl_crtc_page_flip,
 };
 
-static void qxl_user_framebuffer_destroy(struct drm_framebuffer *fb)
+void qxl_user_framebuffer_destroy(struct drm_framebuffer *fb)
 {
 	struct qxl_framebuffer *qxl_fb = to_qxl_framebuffer(fb);
 
@@ -527,12 +527,13 @@ 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)
+		     struct drm_gem_object *obj,
+		     const struct drm_framebuffer_funcs *funcs)
 {
 	int ret;
 
 	qfb->obj = obj;
-	ret = drm_framebuffer_init(dev, &qfb->base, &qxl_fb_funcs);
+	ret = drm_framebuffer_init(dev, &qfb->base, funcs);
 	if (ret) {
 		qfb->obj = NULL;
 		return ret;
@@ -999,7 +1000,7 @@ qxl_user_framebuffer_create(struct drm_device *dev,
 	if (qxl_fb == NULL)
 		return NULL;
 
-	ret = qxl_framebuffer_init(dev, qxl_fb, mode_cmd, obj);
+	ret = qxl_framebuffer_init(dev, qxl_fb, mode_cmd, obj, &qxl_fb_funcs);
 	if (ret) {
 		kfree(qxl_fb);
 		drm_gem_object_unreference_unlocked(obj);
diff --git a/drivers/gpu/drm/qxl/qxl_drv.h b/drivers/gpu/drm/qxl/qxl_drv.h
index 3f3897e..3ad6604 100644
--- a/drivers/gpu/drm/qxl/qxl_drv.h
+++ b/drivers/gpu/drm/qxl/qxl_drv.h
@@ -324,8 +324,6 @@ struct qxl_device {
 	struct workqueue_struct *gc_queue;
 	struct work_struct gc_work;
 
-	struct work_struct fb_work;
-
 	struct drm_property *hotplug_mode_update_property;
 	int monitors_config_width;
 	int monitors_config_height;
@@ -389,11 +387,13 @@ int qxl_get_handle_for_primary_fb(struct qxl_device *qdev,
 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);
+		     struct drm_gem_object *obj,
+		     const struct drm_framebuffer_funcs *funcs);
 void qxl_display_read_client_monitors_config(struct qxl_device *qdev);
 void qxl_send_monitors_config(struct qxl_device *qdev);
 int qxl_create_monitors_object(struct qxl_device *qdev);
@@ -553,7 +553,6 @@ int qxl_irq_init(struct qxl_device *qdev);
 irqreturn_t qxl_irq_handler(int irq, void *arg);
 
 /* qxl_fb.c */
-int qxl_fb_init(struct qxl_device *qdev);
 bool qxl_fbdev_qobj_is_fb(struct qxl_device *qdev, struct qxl_bo *qobj);
 
 int qxl_debugfs_add_files(struct qxl_device *qdev,
diff --git a/drivers/gpu/drm/qxl/qxl_fb.c b/drivers/gpu/drm/qxl/qxl_fb.c
index 06f032d..090dcee 100644
--- a/drivers/gpu/drm/qxl/qxl_fb.c
+++ b/drivers/gpu/drm/qxl/qxl_fb.c
@@ -30,6 +30,7 @@
 #include "drm/drm.h"
 #include "drm/drm_crtc.h"
 #include "drm/drm_crtc_helper.h"
+#include "drm/drm_rect.h"
 #include "qxl_drv.h"
 
 #include "qxl_object.h"
@@ -46,15 +47,6 @@ struct qxl_fbdev {
 	struct list_head delayed_ops;
 	void *shadow;
 	int size;
-
-	/* dirty memory logging */
-	struct {
-		spinlock_t lock;
-		unsigned x1;
-		unsigned y1;
-		unsigned x2;
-		unsigned y2;
-	} dirty;
 };
 
 static void qxl_fb_image_init(struct qxl_fb_image *qxl_fb_image,
@@ -82,169 +74,18 @@ static void qxl_fb_image_init(struct qxl_fb_image *qxl_fb_image,
 	}
 }
 
-static void qxl_fb_dirty_flush(struct fb_info *info)
-{
-	struct qxl_fbdev *qfbdev = info->par;
-	struct qxl_device *qdev = qfbdev->qdev;
-	struct qxl_fb_image qxl_fb_image;
-	struct fb_image *image = &qxl_fb_image.fb_image;
-	unsigned long flags;
-	u32 x1, x2, y1, y2;
-
-	/* TODO: hard coding 32 bpp */
-	int stride = qfbdev->qfb.base.pitches[0];
-
-	spin_lock_irqsave(&qfbdev->dirty.lock, flags);
-
-	x1 = qfbdev->dirty.x1;
-	x2 = qfbdev->dirty.x2;
-	y1 = qfbdev->dirty.y1;
-	y2 = qfbdev->dirty.y2;
-	qfbdev->dirty.x1 = 0;
-	qfbdev->dirty.x2 = 0;
-	qfbdev->dirty.y1 = 0;
-	qfbdev->dirty.y2 = 0;
-
-	spin_unlock_irqrestore(&qfbdev->dirty.lock, flags);
-
-	/*
-	 * we are using a shadow draw buffer, at qdev->surface0_shadow
-	 */
-	qxl_io_log(qdev, "dirty x[%d, %d], y[%d, %d]", x1, x2, y1, y2);
-	image->dx = x1;
-	image->dy = y1;
-	image->width = x2 - x1 + 1;
-	image->height = y2 - y1 + 1;
-	image->fg_color = 0xffffffff; /* unused, just to avoid uninitialized
-					 warnings */
-	image->bg_color = 0;
-	image->depth = 32;	     /* TODO: take from somewhere? */
-	image->cmap.start = 0;
-	image->cmap.len = 0;
-	image->cmap.red = NULL;
-	image->cmap.green = NULL;
-	image->cmap.blue = NULL;
-	image->cmap.transp = NULL;
-	image->data = qfbdev->shadow + (x1 * 4) + (stride * y1);
-
-	qxl_fb_image_init(&qxl_fb_image, qdev, info, NULL);
-	qxl_draw_opaque_fb(&qxl_fb_image, stride);
-}
-
-static void qxl_dirty_update(struct qxl_fbdev *qfbdev,
-			     int x, int y, int width, int height)
-{
-	struct qxl_device *qdev = qfbdev->qdev;
-	unsigned long flags;
-	int x2, y2;
-
-	x2 = x + width - 1;
-	y2 = y + height - 1;
-
-	spin_lock_irqsave(&qfbdev->dirty.lock, flags);
-
-	if ((qfbdev->dirty.y2 - qfbdev->dirty.y1) &&
-	    (qfbdev->dirty.x2 - qfbdev->dirty.x1)) {
-		if (qfbdev->dirty.y1 < y)
-			y = qfbdev->dirty.y1;
-		if (qfbdev->dirty.y2 > y2)
-			y2 = qfbdev->dirty.y2;
-		if (qfbdev->dirty.x1 < x)
-			x = qfbdev->dirty.x1;
-		if (qfbdev->dirty.x2 > x2)
-			x2 = qfbdev->dirty.x2;
-	}
-
-	qfbdev->dirty.x1 = x;
-	qfbdev->dirty.x2 = x2;
-	qfbdev->dirty.y1 = y;
-	qfbdev->dirty.y2 = y2;
-
-	spin_unlock_irqrestore(&qfbdev->dirty.lock, flags);
-
-	schedule_work(&qdev->fb_work);
-}
-
-static void qxl_deferred_io(struct fb_info *info,
-			    struct list_head *pagelist)
-{
-	struct qxl_fbdev *qfbdev = info->par;
-	unsigned long start, end, min, max;
-	struct page *page;
-	int y1, y2;
-
-	min = ULONG_MAX;
-	max = 0;
-	list_for_each_entry(page, pagelist, lru) {
-		start = page->index << PAGE_SHIFT;
-		end = start + PAGE_SIZE - 1;
-		min = min(min, start);
-		max = max(max, end);
-	}
-
-	if (min < max) {
-		y1 = min / info->fix.line_length;
-		y2 = (max / info->fix.line_length) + 1;
-		qxl_dirty_update(qfbdev, 0, y1, info->var.xres, y2 - y1);
-	}
-};
-
 static struct fb_deferred_io qxl_defio = {
 	.delay		= QXL_DIRTY_DELAY,
-	.deferred_io	= qxl_deferred_io,
+	.deferred_io	= drm_fb_helper_deferred_io,
 };
 
-static void qxl_fb_fillrect(struct fb_info *info,
-			    const struct fb_fillrect *rect)
-{
-	struct qxl_fbdev *qfbdev = info->par;
-
-	sys_fillrect(info, rect);
-	qxl_dirty_update(qfbdev, rect->dx, rect->dy, rect->width,
-			 rect->height);
-}
-
-static void qxl_fb_copyarea(struct fb_info *info,
-			    const struct fb_copyarea *area)
-{
-	struct qxl_fbdev *qfbdev = info->par;
-
-	sys_copyarea(info, area);
-	qxl_dirty_update(qfbdev, area->dx, area->dy, area->width,
-			 area->height);
-}
-
-static void qxl_fb_imageblit(struct fb_info *info,
-			     const struct fb_image *image)
-{
-	struct qxl_fbdev *qfbdev = info->par;
-
-	sys_imageblit(info, image);
-	qxl_dirty_update(qfbdev, image->dx, image->dy, image->width,
-			 image->height);
-}
-
-static void qxl_fb_work(struct work_struct *work)
-{
-	struct qxl_device *qdev = container_of(work, struct qxl_device, fb_work);
-	struct qxl_fbdev *qfbdev = qdev->mode_info.qfbdev;
-
-	qxl_fb_dirty_flush(qfbdev->helper.fbdev);
-}
-
-int qxl_fb_init(struct qxl_device *qdev)
-{
-	INIT_WORK(&qdev->fb_work, qxl_fb_work);
-	return 0;
-}
-
 static struct fb_ops qxlfb_ops = {
 	.owner = THIS_MODULE,
 	.fb_check_var = drm_fb_helper_check_var,
 	.fb_set_par = drm_fb_helper_set_par, /* TODO: copy vmwgfx */
-	.fb_fillrect = qxl_fb_fillrect,
-	.fb_copyarea = qxl_fb_copyarea,
-	.fb_imageblit = qxl_fb_imageblit,
+	.fb_fillrect = drm_fb_helper_sys_fillrect,
+	.fb_copyarea = drm_fb_helper_sys_copyarea,
+	.fb_imageblit = drm_fb_helper_sys_imageblit,
 	.fb_pan_display = drm_fb_helper_pan_display,
 	.fb_blank = drm_fb_helper_blank,
 	.fb_setcmap = drm_fb_helper_setcmap,
@@ -338,6 +179,53 @@ out_unref:
 	return ret;
 }
 
+static int qxlfb_framebuffer_dirty(struct drm_framebuffer *fb,
+				   struct drm_file *file_priv,
+				   unsigned flags, unsigned color,
+				   struct drm_clip_rect *clips,
+				   unsigned num_clips)
+{
+	struct qxl_device *qdev = fb->dev->dev_private;
+	struct fb_info *info = qdev->fbdev_info;
+	struct qxl_fbdev *qfbdev = info->par;
+	struct qxl_fb_image qxl_fb_image;
+	struct fb_image *image = &qxl_fb_image.fb_image;
+
+	/* TODO: hard coding 32 bpp */
+	int stride = qfbdev->qfb.base.pitches[0];
+
+	/*
+	 * we are using a shadow draw buffer, at qdev->surface0_shadow
+	 */
+	qxl_io_log(qdev, "dirty x[%d, %d], y[%d, %d]", clips->x1, clips->x2,
+		   clips->y1, clips->y2);
+	image->dx = clips->x1;
+	image->dy = clips->y1;
+	image->width = drm_clip_rect_width(clips);
+	image->height = drm_clip_rect_height(clips);
+	image->fg_color = 0xffffffff; /* unused, just to avoid uninitialized
+					 warnings */
+	image->bg_color = 0;
+	image->depth = 32;	     /* TODO: take from somewhere? */
+	image->cmap.start = 0;
+	image->cmap.len = 0;
+	image->cmap.red = NULL;
+	image->cmap.green = NULL;
+	image->cmap.blue = NULL;
+	image->cmap.transp = NULL;
+	image->data = qfbdev->shadow + (clips->x1 * 4) + (stride * clips->y1);
+
+	qxl_fb_image_init(&qxl_fb_image, qdev, info, NULL);
+	qxl_draw_opaque_fb(&qxl_fb_image, stride);
+
+	return 0;
+}
+
+static const struct drm_framebuffer_funcs qxlfb_fb_funcs = {
+	.destroy = qxl_user_framebuffer_destroy,
+	.dirty = qxlfb_framebuffer_dirty,
+};
+
 static int qxlfb_create(struct qxl_fbdev *qfbdev,
 			struct drm_fb_helper_surface_size *sizes)
 {
@@ -383,7 +271,8 @@ static int qxlfb_create(struct qxl_fbdev *qfbdev,
 
 	info->par = qfbdev;
 
-	qxl_framebuffer_init(qdev->ddev, &qfbdev->qfb, &mode_cmd, gobj);
+	qxl_framebuffer_init(qdev->ddev, &qfbdev->qfb, &mode_cmd, gobj,
+			     &qxlfb_fb_funcs);
 
 	fb = &qfbdev->qfb.base;
 
@@ -504,7 +393,6 @@ int qxl_fbdev_init(struct qxl_device *qdev)
 	qfbdev->qdev = qdev;
 	qdev->mode_info.qfbdev = qfbdev;
 	spin_lock_init(&qfbdev->delayed_ops_lock);
-	spin_lock_init(&qfbdev->dirty.lock);
 	INIT_LIST_HEAD(&qfbdev->delayed_ops);
 
 	drm_fb_helper_prepare(qdev->ddev, &qfbdev->helper,
diff --git a/drivers/gpu/drm/qxl/qxl_kms.c b/drivers/gpu/drm/qxl/qxl_kms.c
index b2977a1..2319800 100644
--- a/drivers/gpu/drm/qxl/qxl_kms.c
+++ b/drivers/gpu/drm/qxl/qxl_kms.c
@@ -261,10 +261,6 @@ static int qxl_device_init(struct qxl_device *qdev,
 	qdev->gc_queue = create_singlethread_workqueue("qxl_gc");
 	INIT_WORK(&qdev->gc_work, qxl_gc_work);
 
-	r = qxl_fb_init(qdev);
-	if (r)
-		return r;
-
 	return 0;
 }
 
-- 
2.2.2

[toc] | [prev] | [next] | [standalone]

#1383587 — Re: [PATCH 7/8] drm/qxl: Use drm_fb_helper deferred_io support

On Wed, Apr 20, 2016 at 05:25:28PM +0200, Noralf Trønnes wrote:
> Use the fbdev deferred io support in drm_fb_helper.
> The (struct fb_ops *)->fb_{fillrect,copyarea,imageblit} functions will
> now be deferred in the same way that mmap damage is, instead of being
> flushed directly.
> This patch has only been compile tested.
> 
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> ---
>  drivers/gpu/drm/qxl/qxl_display.c |   9 +-
>  drivers/gpu/drm/qxl/qxl_drv.h     |   7 +-
>  drivers/gpu/drm/qxl/qxl_fb.c      | 220 ++++++++++----------------------------
>  drivers/gpu/drm/qxl/qxl_kms.c     |   4 -
>  4 files changed, 62 insertions(+), 178 deletions(-)
> 
> diff --git a/drivers/gpu/drm/qxl/qxl_display.c b/drivers/gpu/drm/qxl/qxl_display.c
> index 030409a..9a03524 100644
> --- a/drivers/gpu/drm/qxl/qxl_display.c
> +++ b/drivers/gpu/drm/qxl/qxl_display.c
> @@ -465,7 +465,7 @@ static const struct drm_crtc_funcs qxl_crtc_funcs = {
>  	.page_flip = qxl_crtc_page_flip,
>  };
>  
> -static void qxl_user_framebuffer_destroy(struct drm_framebuffer *fb)
> +void qxl_user_framebuffer_destroy(struct drm_framebuffer *fb)
>  {
>  	struct qxl_framebuffer *qxl_fb = to_qxl_framebuffer(fb);
>  
> @@ -527,12 +527,13 @@ 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)
> +		     struct drm_gem_object *obj,
> +		     const struct drm_framebuffer_funcs *funcs)

There should be no need at all to have a separate fb funcs table for the
fbdev fb. Both /should/ be able to use the exact same (already existing)
->dirty() callback. We need this only in CMA because CMA is a midlayer
used by multiple drivers.

With that change you should be able to condense this patch down to pretty
much just removing lines. Which is Good (tm).

Cheers, Daniel

>  {
>  	int ret;
>  
>  	qfb->obj = obj;
> -	ret = drm_framebuffer_init(dev, &qfb->base, &qxl_fb_funcs);
> +	ret = drm_framebuffer_init(dev, &qfb->base, funcs);
>  	if (ret) {
>  		qfb->obj = NULL;
>  		return ret;
> @@ -999,7 +1000,7 @@ qxl_user_framebuffer_create(struct drm_device *dev,
>  	if (qxl_fb == NULL)
>  		return NULL;
>  
> -	ret = qxl_framebuffer_init(dev, qxl_fb, mode_cmd, obj);
> +	ret = qxl_framebuffer_init(dev, qxl_fb, mode_cmd, obj, &qxl_fb_funcs);
>  	if (ret) {
>  		kfree(qxl_fb);
>  		drm_gem_object_unreference_unlocked(obj);
> diff --git a/drivers/gpu/drm/qxl/qxl_drv.h b/drivers/gpu/drm/qxl/qxl_drv.h
> index 3f3897e..3ad6604 100644
> --- a/drivers/gpu/drm/qxl/qxl_drv.h
> +++ b/drivers/gpu/drm/qxl/qxl_drv.h
> @@ -324,8 +324,6 @@ struct qxl_device {
>  	struct workqueue_struct *gc_queue;
>  	struct work_struct gc_work;
>  
> -	struct work_struct fb_work;
> -
>  	struct drm_property *hotplug_mode_update_property;
>  	int monitors_config_width;
>  	int monitors_config_height;
> @@ -389,11 +387,13 @@ int qxl_get_handle_for_primary_fb(struct qxl_device *qdev,
>  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);
> +		     struct drm_gem_object *obj,
> +		     const struct drm_framebuffer_funcs *funcs);
>  void qxl_display_read_client_monitors_config(struct qxl_device *qdev);
>  void qxl_send_monitors_config(struct qxl_device *qdev);
>  int qxl_create_monitors_object(struct qxl_device *qdev);
> @@ -553,7 +553,6 @@ int qxl_irq_init(struct qxl_device *qdev);
>  irqreturn_t qxl_irq_handler(int irq, void *arg);
>  
>  /* qxl_fb.c */
> -int qxl_fb_init(struct qxl_device *qdev);
>  bool qxl_fbdev_qobj_is_fb(struct qxl_device *qdev, struct qxl_bo *qobj);
>  
>  int qxl_debugfs_add_files(struct qxl_device *qdev,
> diff --git a/drivers/gpu/drm/qxl/qxl_fb.c b/drivers/gpu/drm/qxl/qxl_fb.c
> index 06f032d..090dcee 100644
> --- a/drivers/gpu/drm/qxl/qxl_fb.c
> +++ b/drivers/gpu/drm/qxl/qxl_fb.c
> @@ -30,6 +30,7 @@
>  #include "drm/drm.h"
>  #include "drm/drm_crtc.h"
>  #include "drm/drm_crtc_helper.h"
> +#include "drm/drm_rect.h"
>  #include "qxl_drv.h"
>  
>  #include "qxl_object.h"
> @@ -46,15 +47,6 @@ struct qxl_fbdev {
>  	struct list_head delayed_ops;
>  	void *shadow;
>  	int size;
> -
> -	/* dirty memory logging */
> -	struct {
> -		spinlock_t lock;
> -		unsigned x1;
> -		unsigned y1;
> -		unsigned x2;
> -		unsigned y2;
> -	} dirty;
>  };
>  
>  static void qxl_fb_image_init(struct qxl_fb_image *qxl_fb_image,
> @@ -82,169 +74,18 @@ static void qxl_fb_image_init(struct qxl_fb_image *qxl_fb_image,
>  	}
>  }
>  
> -static void qxl_fb_dirty_flush(struct fb_info *info)
> -{
> -	struct qxl_fbdev *qfbdev = info->par;
> -	struct qxl_device *qdev = qfbdev->qdev;
> -	struct qxl_fb_image qxl_fb_image;
> -	struct fb_image *image = &qxl_fb_image.fb_image;
> -	unsigned long flags;
> -	u32 x1, x2, y1, y2;
> -
> -	/* TODO: hard coding 32 bpp */
> -	int stride = qfbdev->qfb.base.pitches[0];
> -
> -	spin_lock_irqsave(&qfbdev->dirty.lock, flags);
> -
> -	x1 = qfbdev->dirty.x1;
> -	x2 = qfbdev->dirty.x2;
> -	y1 = qfbdev->dirty.y1;
> -	y2 = qfbdev->dirty.y2;
> -	qfbdev->dirty.x1 = 0;
> -	qfbdev->dirty.x2 = 0;
> -	qfbdev->dirty.y1 = 0;
> -	qfbdev->dirty.y2 = 0;
> -
> -	spin_unlock_irqrestore(&qfbdev->dirty.lock, flags);
> -
> -	/*
> -	 * we are using a shadow draw buffer, at qdev->surface0_shadow
> -	 */
> -	qxl_io_log(qdev, "dirty x[%d, %d], y[%d, %d]", x1, x2, y1, y2);
> -	image->dx = x1;
> -	image->dy = y1;
> -	image->width = x2 - x1 + 1;
> -	image->height = y2 - y1 + 1;
> -	image->fg_color = 0xffffffff; /* unused, just to avoid uninitialized
> -					 warnings */
> -	image->bg_color = 0;
> -	image->depth = 32;	     /* TODO: take from somewhere? */
> -	image->cmap.start = 0;
> -	image->cmap.len = 0;
> -	image->cmap.red = NULL;
> -	image->cmap.green = NULL;
> -	image->cmap.blue = NULL;
> -	image->cmap.transp = NULL;
> -	image->data = qfbdev->shadow + (x1 * 4) + (stride * y1);
> -
> -	qxl_fb_image_init(&qxl_fb_image, qdev, info, NULL);
> -	qxl_draw_opaque_fb(&qxl_fb_image, stride);
> -}
> -
> -static void qxl_dirty_update(struct qxl_fbdev *qfbdev,
> -			     int x, int y, int width, int height)
> -{
> -	struct qxl_device *qdev = qfbdev->qdev;
> -	unsigned long flags;
> -	int x2, y2;
> -
> -	x2 = x + width - 1;
> -	y2 = y + height - 1;
> -
> -	spin_lock_irqsave(&qfbdev->dirty.lock, flags);
> -
> -	if ((qfbdev->dirty.y2 - qfbdev->dirty.y1) &&
> -	    (qfbdev->dirty.x2 - qfbdev->dirty.x1)) {
> -		if (qfbdev->dirty.y1 < y)
> -			y = qfbdev->dirty.y1;
> -		if (qfbdev->dirty.y2 > y2)
> -			y2 = qfbdev->dirty.y2;
> -		if (qfbdev->dirty.x1 < x)
> -			x = qfbdev->dirty.x1;
> -		if (qfbdev->dirty.x2 > x2)
> -			x2 = qfbdev->dirty.x2;
> -	}
> -
> -	qfbdev->dirty.x1 = x;
> -	qfbdev->dirty.x2 = x2;
> -	qfbdev->dirty.y1 = y;
> -	qfbdev->dirty.y2 = y2;
> -
> -	spin_unlock_irqrestore(&qfbdev->dirty.lock, flags);
> -
> -	schedule_work(&qdev->fb_work);
> -}
> -
> -static void qxl_deferred_io(struct fb_info *info,
> -			    struct list_head *pagelist)
> -{
> -	struct qxl_fbdev *qfbdev = info->par;
> -	unsigned long start, end, min, max;
> -	struct page *page;
> -	int y1, y2;
> -
> -	min = ULONG_MAX;
> -	max = 0;
> -	list_for_each_entry(page, pagelist, lru) {
> -		start = page->index << PAGE_SHIFT;
> -		end = start + PAGE_SIZE - 1;
> -		min = min(min, start);
> -		max = max(max, end);
> -	}
> -
> -	if (min < max) {
> -		y1 = min / info->fix.line_length;
> -		y2 = (max / info->fix.line_length) + 1;
> -		qxl_dirty_update(qfbdev, 0, y1, info->var.xres, y2 - y1);
> -	}
> -};
> -
>  static struct fb_deferred_io qxl_defio = {
>  	.delay		= QXL_DIRTY_DELAY,
> -	.deferred_io	= qxl_deferred_io,
> +	.deferred_io	= drm_fb_helper_deferred_io,
>  };
>  
> -static void qxl_fb_fillrect(struct fb_info *info,
> -			    const struct fb_fillrect *rect)
> -{
> -	struct qxl_fbdev *qfbdev = info->par;
> -
> -	sys_fillrect(info, rect);
> -	qxl_dirty_update(qfbdev, rect->dx, rect->dy, rect->width,
> -			 rect->height);
> -}
> -
> -static void qxl_fb_copyarea(struct fb_info *info,
> -			    const struct fb_copyarea *area)
> -{
> -	struct qxl_fbdev *qfbdev = info->par;
> -
> -	sys_copyarea(info, area);
> -	qxl_dirty_update(qfbdev, area->dx, area->dy, area->width,
> -			 area->height);
> -}
> -
> -static void qxl_fb_imageblit(struct fb_info *info,
> -			     const struct fb_image *image)
> -{
> -	struct qxl_fbdev *qfbdev = info->par;
> -
> -	sys_imageblit(info, image);
> -	qxl_dirty_update(qfbdev, image->dx, image->dy, image->width,
> -			 image->height);
> -}
> -
> -static void qxl_fb_work(struct work_struct *work)
> -{
> -	struct qxl_device *qdev = container_of(work, struct qxl_device, fb_work);
> -	struct qxl_fbdev *qfbdev = qdev->mode_info.qfbdev;
> -
> -	qxl_fb_dirty_flush(qfbdev->helper.fbdev);
> -}
> -
> -int qxl_fb_init(struct qxl_device *qdev)
> -{
> -	INIT_WORK(&qdev->fb_work, qxl_fb_work);
> -	return 0;
> -}
> -
>  static struct fb_ops qxlfb_ops = {
>  	.owner = THIS_MODULE,
>  	.fb_check_var = drm_fb_helper_check_var,
>  	.fb_set_par = drm_fb_helper_set_par, /* TODO: copy vmwgfx */
> -	.fb_fillrect = qxl_fb_fillrect,
> -	.fb_copyarea = qxl_fb_copyarea,
> -	.fb_imageblit = qxl_fb_imageblit,
> +	.fb_fillrect = drm_fb_helper_sys_fillrect,
> +	.fb_copyarea = drm_fb_helper_sys_copyarea,
> +	.fb_imageblit = drm_fb_helper_sys_imageblit,
>  	.fb_pan_display = drm_fb_helper_pan_display,
>  	.fb_blank = drm_fb_helper_blank,
>  	.fb_setcmap = drm_fb_helper_setcmap,
> @@ -338,6 +179,53 @@ out_unref:
>  	return ret;
>  }
>  
> +static int qxlfb_framebuffer_dirty(struct drm_framebuffer *fb,
> +				   struct drm_file *file_priv,
> +				   unsigned flags, unsigned color,
> +				   struct drm_clip_rect *clips,
> +				   unsigned num_clips)
> +{
> +	struct qxl_device *qdev = fb->dev->dev_private;
> +	struct fb_info *info = qdev->fbdev_info;
> +	struct qxl_fbdev *qfbdev = info->par;
> +	struct qxl_fb_image qxl_fb_image;
> +	struct fb_image *image = &qxl_fb_image.fb_image;
> +
> +	/* TODO: hard coding 32 bpp */
> +	int stride = qfbdev->qfb.base.pitches[0];
> +
> +	/*
> +	 * we are using a shadow draw buffer, at qdev->surface0_shadow
> +	 */
> +	qxl_io_log(qdev, "dirty x[%d, %d], y[%d, %d]", clips->x1, clips->x2,
> +		   clips->y1, clips->y2);
> +	image->dx = clips->x1;
> +	image->dy = clips->y1;
> +	image->width = drm_clip_rect_width(clips);
> +	image->height = drm_clip_rect_height(clips);
> +	image->fg_color = 0xffffffff; /* unused, just to avoid uninitialized
> +					 warnings */
> +	image->bg_color = 0;
> +	image->depth = 32;	     /* TODO: take from somewhere? */
> +	image->cmap.start = 0;
> +	image->cmap.len = 0;
> +	image->cmap.red = NULL;
> +	image->cmap.green = NULL;
> +	image->cmap.blue = NULL;
> +	image->cmap.transp = NULL;
> +	image->data = qfbdev->shadow + (clips->x1 * 4) + (stride * clips->y1);
> +
> +	qxl_fb_image_init(&qxl_fb_image, qdev, info, NULL);
> +	qxl_draw_opaque_fb(&qxl_fb_image, stride);
> +
> +	return 0;
> +}
> +
> +static const struct drm_framebuffer_funcs qxlfb_fb_funcs = {
> +	.destroy = qxl_user_framebuffer_destroy,
> +	.dirty = qxlfb_framebuffer_dirty,
> +};
> +
>  static int qxlfb_create(struct qxl_fbdev *qfbdev,
>  			struct drm_fb_helper_surface_size *sizes)
>  {
> @@ -383,7 +271,8 @@ static int qxlfb_create(struct qxl_fbdev *qfbdev,
>  
>  	info->par = qfbdev;
>  
> -	qxl_framebuffer_init(qdev->ddev, &qfbdev->qfb, &mode_cmd, gobj);
> +	qxl_framebuffer_init(qdev->ddev, &qfbdev->qfb, &mode_cmd, gobj,
> +			     &qxlfb_fb_funcs);
>  
>  	fb = &qfbdev->qfb.base;
>  
> @@ -504,7 +393,6 @@ int qxl_fbdev_init(struct qxl_device *qdev)
>  	qfbdev->qdev = qdev;
>  	qdev->mode_info.qfbdev = qfbdev;
>  	spin_lock_init(&qfbdev->delayed_ops_lock);
> -	spin_lock_init(&qfbdev->dirty.lock);
>  	INIT_LIST_HEAD(&qfbdev->delayed_ops);
>  
>  	drm_fb_helper_prepare(qdev->ddev, &qfbdev->helper,
> diff --git a/drivers/gpu/drm/qxl/qxl_kms.c b/drivers/gpu/drm/qxl/qxl_kms.c
> index b2977a1..2319800 100644
> --- a/drivers/gpu/drm/qxl/qxl_kms.c
> +++ b/drivers/gpu/drm/qxl/qxl_kms.c
> @@ -261,10 +261,6 @@ static int qxl_device_init(struct qxl_device *qdev,
>  	qdev->gc_queue = create_singlethread_workqueue("qxl_gc");
>  	INIT_WORK(&qdev->gc_work, qxl_gc_work);
>  
> -	r = qxl_fb_init(qdev);
> -	if (r)
> -		return r;
> -
>  	return 0;
>  }
>  
> -- 
> 2.2.2
> 

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

[toc] | [prev] | [next] | [standalone]

#1383651 — Re: [PATCH 7/8] drm/qxl: Use drm_fb_helper deferred_io support

Den 20.04.2016 19:47, skrev Daniel Vetter:
> On Wed, Apr 20, 2016 at 05:25:28PM +0200, Noralf Trønnes wrote:
>> Use the fbdev deferred io support in drm_fb_helper.
>> The (struct fb_ops *)->fb_{fillrect,copyarea,imageblit} functions will
>> now be deferred in the same way that mmap damage is, instead of being
>> flushed directly.
>> This patch has only been compile tested.
>>
>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
>> ---
>>   drivers/gpu/drm/qxl/qxl_display.c |   9 +-
>>   drivers/gpu/drm/qxl/qxl_drv.h     |   7 +-
>>   drivers/gpu/drm/qxl/qxl_fb.c      | 220 ++++++++++----------------------------
>>   drivers/gpu/drm/qxl/qxl_kms.c     |   4 -
>>   4 files changed, 62 insertions(+), 178 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/qxl/qxl_display.c b/drivers/gpu/drm/qxl/qxl_display.c
>> index 030409a..9a03524 100644
>> --- a/drivers/gpu/drm/qxl/qxl_display.c
>> +++ b/drivers/gpu/drm/qxl/qxl_display.c
>> @@ -465,7 +465,7 @@ static const struct drm_crtc_funcs qxl_crtc_funcs = {
>>   	.page_flip = qxl_crtc_page_flip,
>>   };
>>   
>> -static void qxl_user_framebuffer_destroy(struct drm_framebuffer *fb)
>> +void qxl_user_framebuffer_destroy(struct drm_framebuffer *fb)
>>   {
>>   	struct qxl_framebuffer *qxl_fb = to_qxl_framebuffer(fb);
>>   
>> @@ -527,12 +527,13 @@ 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)
>> +		     struct drm_gem_object *obj,
>> +		     const struct drm_framebuffer_funcs *funcs)
> There should be no need at all to have a separate fb funcs table for the
> fbdev fb. Both /should/ be able to use the exact same (already existing)
> ->dirty() callback. We need this only in CMA because CMA is a midlayer
> used by multiple drivers.

I don't see how I can avoid it.

fbdev framebuffer flushing:

static void qxl_fb_dirty_flush(struct fb_info *info)
{
         qxl_fb_image_init(&qxl_fb_image, qdev, info, NULL);
         qxl_draw_opaque_fb(&qxl_fb_image, stride);
}

drm framebuffer flushing:

static int qxl_framebuffer_surface_dirty(...)
{
         qxl_draw_dirty_fb(...);
}

qxl_draw_opaque_fb() and qxl_draw_dirty_fb() differ so much that it's way
over my head to see if they can be combined.
Here's an online diff of the two functions:
https://www.diffchecker.com/jqbbalux


>
> With that change you should be able to condense this patch down to pretty
> much just removing lines. Which is Good (tm).
>
> Cheers, Daniel
>
>>   {
>>   	int ret;
>>   
>>   	qfb->obj = obj;
>> -	ret = drm_framebuffer_init(dev, &qfb->base, &qxl_fb_funcs);
>> +	ret = drm_framebuffer_init(dev, &qfb->base, funcs);
>>   	if (ret) {
>>   		qfb->obj = NULL;
>>   		return ret;
>> @@ -999,7 +1000,7 @@ qxl_user_framebuffer_create(struct drm_device *dev,
>>   	if (qxl_fb == NULL)
>>   		return NULL;
>>   
>> -	ret = qxl_framebuffer_init(dev, qxl_fb, mode_cmd, obj);
>> +	ret = qxl_framebuffer_init(dev, qxl_fb, mode_cmd, obj, &qxl_fb_funcs);
>>   	if (ret) {
>>   		kfree(qxl_fb);
>>   		drm_gem_object_unreference_unlocked(obj);
>> diff --git a/drivers/gpu/drm/qxl/qxl_drv.h b/drivers/gpu/drm/qxl/qxl_drv.h
>> index 3f3897e..3ad6604 100644
>> --- a/drivers/gpu/drm/qxl/qxl_drv.h
>> +++ b/drivers/gpu/drm/qxl/qxl_drv.h
>> @@ -324,8 +324,6 @@ struct qxl_device {
>>   	struct workqueue_struct *gc_queue;
>>   	struct work_struct gc_work;
>>   
>> -	struct work_struct fb_work;
>> -
>>   	struct drm_property *hotplug_mode_update_property;
>>   	int monitors_config_width;
>>   	int monitors_config_height;
>> @@ -389,11 +387,13 @@ int qxl_get_handle_for_primary_fb(struct qxl_device *qdev,
>>   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);
>> +		     struct drm_gem_object *obj,
>> +		     const struct drm_framebuffer_funcs *funcs);
>>   void qxl_display_read_client_monitors_config(struct qxl_device *qdev);
>>   void qxl_send_monitors_config(struct qxl_device *qdev);
>>   int qxl_create_monitors_object(struct qxl_device *qdev);
>> @@ -553,7 +553,6 @@ int qxl_irq_init(struct qxl_device *qdev);
>>   irqreturn_t qxl_irq_handler(int irq, void *arg);
>>   
>>   /* qxl_fb.c */
>> -int qxl_fb_init(struct qxl_device *qdev);
>>   bool qxl_fbdev_qobj_is_fb(struct qxl_device *qdev, struct qxl_bo *qobj);
>>   
>>   int qxl_debugfs_add_files(struct qxl_device *qdev,
>> diff --git a/drivers/gpu/drm/qxl/qxl_fb.c b/drivers/gpu/drm/qxl/qxl_fb.c
>> index 06f032d..090dcee 100644
>> --- a/drivers/gpu/drm/qxl/qxl_fb.c
>> +++ b/drivers/gpu/drm/qxl/qxl_fb.c
>> @@ -30,6 +30,7 @@
>>   #include "drm/drm.h"
>>   #include "drm/drm_crtc.h"
>>   #include "drm/drm_crtc_helper.h"
>> +#include "drm/drm_rect.h"
>>   #include "qxl_drv.h"
>>   
>>   #include "qxl_object.h"
>> @@ -46,15 +47,6 @@ struct qxl_fbdev {
>>   	struct list_head delayed_ops;
>>   	void *shadow;
>>   	int size;
>> -
>> -	/* dirty memory logging */
>> -	struct {
>> -		spinlock_t lock;
>> -		unsigned x1;
>> -		unsigned y1;
>> -		unsigned x2;
>> -		unsigned y2;
>> -	} dirty;
>>   };
>>   
>>   static void qxl_fb_image_init(struct qxl_fb_image *qxl_fb_image,
>> @@ -82,169 +74,18 @@ static void qxl_fb_image_init(struct qxl_fb_image *qxl_fb_image,
>>   	}
>>   }
>>   
>> -static void qxl_fb_dirty_flush(struct fb_info *info)
>> -{
>> -	struct qxl_fbdev *qfbdev = info->par;
>> -	struct qxl_device *qdev = qfbdev->qdev;
>> -	struct qxl_fb_image qxl_fb_image;
>> -	struct fb_image *image = &qxl_fb_image.fb_image;
>> -	unsigned long flags;
>> -	u32 x1, x2, y1, y2;
>> -
>> -	/* TODO: hard coding 32 bpp */
>> -	int stride = qfbdev->qfb.base.pitches[0];
>> -
>> -	spin_lock_irqsave(&qfbdev->dirty.lock, flags);
>> -
>> -	x1 = qfbdev->dirty.x1;
>> -	x2 = qfbdev->dirty.x2;
>> -	y1 = qfbdev->dirty.y1;
>> -	y2 = qfbdev->dirty.y2;
>> -	qfbdev->dirty.x1 = 0;
>> -	qfbdev->dirty.x2 = 0;
>> -	qfbdev->dirty.y1 = 0;
>> -	qfbdev->dirty.y2 = 0;
>> -
>> -	spin_unlock_irqrestore(&qfbdev->dirty.lock, flags);
>> -
>> -	/*
>> -	 * we are using a shadow draw buffer, at qdev->surface0_shadow
>> -	 */
>> -	qxl_io_log(qdev, "dirty x[%d, %d], y[%d, %d]", x1, x2, y1, y2);
>> -	image->dx = x1;
>> -	image->dy = y1;
>> -	image->width = x2 - x1 + 1;
>> -	image->height = y2 - y1 + 1;
>> -	image->fg_color = 0xffffffff; /* unused, just to avoid uninitialized
>> -					 warnings */
>> -	image->bg_color = 0;
>> -	image->depth = 32;	     /* TODO: take from somewhere? */
>> -	image->cmap.start = 0;
>> -	image->cmap.len = 0;
>> -	image->cmap.red = NULL;
>> -	image->cmap.green = NULL;
>> -	image->cmap.blue = NULL;
>> -	image->cmap.transp = NULL;
>> -	image->data = qfbdev->shadow + (x1 * 4) + (stride * y1);
>> -
>> -	qxl_fb_image_init(&qxl_fb_image, qdev, info, NULL);
>> -	qxl_draw_opaque_fb(&qxl_fb_image, stride);
>> -}
>> -
>> -static void qxl_dirty_update(struct qxl_fbdev *qfbdev,
>> -			     int x, int y, int width, int height)
>> -{
>> -	struct qxl_device *qdev = qfbdev->qdev;
>> -	unsigned long flags;
>> -	int x2, y2;
>> -
>> -	x2 = x + width - 1;
>> -	y2 = y + height - 1;
>> -
>> -	spin_lock_irqsave(&qfbdev->dirty.lock, flags);
>> -
>> -	if ((qfbdev->dirty.y2 - qfbdev->dirty.y1) &&
>> -	    (qfbdev->dirty.x2 - qfbdev->dirty.x1)) {
>> -		if (qfbdev->dirty.y1 < y)
>> -			y = qfbdev->dirty.y1;
>> -		if (qfbdev->dirty.y2 > y2)
>> -			y2 = qfbdev->dirty.y2;
>> -		if (qfbdev->dirty.x1 < x)
>> -			x = qfbdev->dirty.x1;
>> -		if (qfbdev->dirty.x2 > x2)
>> -			x2 = qfbdev->dirty.x2;
>> -	}
>> -
>> -	qfbdev->dirty.x1 = x;
>> -	qfbdev->dirty.x2 = x2;
>> -	qfbdev->dirty.y1 = y;
>> -	qfbdev->dirty.y2 = y2;
>> -
>> -	spin_unlock_irqrestore(&qfbdev->dirty.lock, flags);
>> -
>> -	schedule_work(&qdev->fb_work);
>> -}
>> -
>> -static void qxl_deferred_io(struct fb_info *info,
>> -			    struct list_head *pagelist)
>> -{
>> -	struct qxl_fbdev *qfbdev = info->par;
>> -	unsigned long start, end, min, max;
>> -	struct page *page;
>> -	int y1, y2;
>> -
>> -	min = ULONG_MAX;
>> -	max = 0;
>> -	list_for_each_entry(page, pagelist, lru) {
>> -		start = page->index << PAGE_SHIFT;
>> -		end = start + PAGE_SIZE - 1;
>> -		min = min(min, start);
>> -		max = max(max, end);
>> -	}
>> -
>> -	if (min < max) {
>> -		y1 = min / info->fix.line_length;
>> -		y2 = (max / info->fix.line_length) + 1;
>> -		qxl_dirty_update(qfbdev, 0, y1, info->var.xres, y2 - y1);
>> -	}
>> -};
>> -
>>   static struct fb_deferred_io qxl_defio = {
>>   	.delay		= QXL_DIRTY_DELAY,
>> -	.deferred_io	= qxl_deferred_io,
>> +	.deferred_io	= drm_fb_helper_deferred_io,
>>   };
>>   
>> -static void qxl_fb_fillrect(struct fb_info *info,
>> -			    const struct fb_fillrect *rect)
>> -{
>> -	struct qxl_fbdev *qfbdev = info->par;
>> -
>> -	sys_fillrect(info, rect);
>> -	qxl_dirty_update(qfbdev, rect->dx, rect->dy, rect->width,
>> -			 rect->height);
>> -}
>> -
>> -static void qxl_fb_copyarea(struct fb_info *info,
>> -			    const struct fb_copyarea *area)
>> -{
>> -	struct qxl_fbdev *qfbdev = info->par;
>> -
>> -	sys_copyarea(info, area);
>> -	qxl_dirty_update(qfbdev, area->dx, area->dy, area->width,
>> -			 area->height);
>> -}
>> -
>> -static void qxl_fb_imageblit(struct fb_info *info,
>> -			     const struct fb_image *image)
>> -{
>> -	struct qxl_fbdev *qfbdev = info->par;
>> -
>> -	sys_imageblit(info, image);
>> -	qxl_dirty_update(qfbdev, image->dx, image->dy, image->width,
>> -			 image->height);
>> -}
>> -
>> -static void qxl_fb_work(struct work_struct *work)
>> -{
>> -	struct qxl_device *qdev = container_of(work, struct qxl_device, fb_work);
>> -	struct qxl_fbdev *qfbdev = qdev->mode_info.qfbdev;
>> -
>> -	qxl_fb_dirty_flush(qfbdev->helper.fbdev);
>> -}
>> -
>> -int qxl_fb_init(struct qxl_device *qdev)
>> -{
>> -	INIT_WORK(&qdev->fb_work, qxl_fb_work);
>> -	return 0;
>> -}
>> -
>>   static struct fb_ops qxlfb_ops = {
>>   	.owner = THIS_MODULE,
>>   	.fb_check_var = drm_fb_helper_check_var,
>>   	.fb_set_par = drm_fb_helper_set_par, /* TODO: copy vmwgfx */
>> -	.fb_fillrect = qxl_fb_fillrect,
>> -	.fb_copyarea = qxl_fb_copyarea,
>> -	.fb_imageblit = qxl_fb_imageblit,
>> +	.fb_fillrect = drm_fb_helper_sys_fillrect,
>> +	.fb_copyarea = drm_fb_helper_sys_copyarea,
>> +	.fb_imageblit = drm_fb_helper_sys_imageblit,
>>   	.fb_pan_display = drm_fb_helper_pan_display,
>>   	.fb_blank = drm_fb_helper_blank,
>>   	.fb_setcmap = drm_fb_helper_setcmap,
>> @@ -338,6 +179,53 @@ out_unref:
>>   	return ret;
>>   }
>>   
>> +static int qxlfb_framebuffer_dirty(struct drm_framebuffer *fb,
>> +				   struct drm_file *file_priv,
>> +				   unsigned flags, unsigned color,
>> +				   struct drm_clip_rect *clips,
>> +				   unsigned num_clips)
>> +{
>> +	struct qxl_device *qdev = fb->dev->dev_private;
>> +	struct fb_info *info = qdev->fbdev_info;
>> +	struct qxl_fbdev *qfbdev = info->par;
>> +	struct qxl_fb_image qxl_fb_image;
>> +	struct fb_image *image = &qxl_fb_image.fb_image;
>> +
>> +	/* TODO: hard coding 32 bpp */
>> +	int stride = qfbdev->qfb.base.pitches[0];
>> +
>> +	/*
>> +	 * we are using a shadow draw buffer, at qdev->surface0_shadow
>> +	 */
>> +	qxl_io_log(qdev, "dirty x[%d, %d], y[%d, %d]", clips->x1, clips->x2,
>> +		   clips->y1, clips->y2);
>> +	image->dx = clips->x1;
>> +	image->dy = clips->y1;
>> +	image->width = drm_clip_rect_width(clips);
>> +	image->height = drm_clip_rect_height(clips);
>> +	image->fg_color = 0xffffffff; /* unused, just to avoid uninitialized
>> +					 warnings */
>> +	image->bg_color = 0;
>> +	image->depth = 32;	     /* TODO: take from somewhere? */
>> +	image->cmap.start = 0;
>> +	image->cmap.len = 0;
>> +	image->cmap.red = NULL;
>> +	image->cmap.green = NULL;
>> +	image->cmap.blue = NULL;
>> +	image->cmap.transp = NULL;
>> +	image->data = qfbdev->shadow + (clips->x1 * 4) + (stride * clips->y1);
>> +
>> +	qxl_fb_image_init(&qxl_fb_image, qdev, info, NULL);
>> +	qxl_draw_opaque_fb(&qxl_fb_image, stride);
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct drm_framebuffer_funcs qxlfb_fb_funcs = {
>> +	.destroy = qxl_user_framebuffer_destroy,
>> +	.dirty = qxlfb_framebuffer_dirty,
>> +};
>> +
>>   static int qxlfb_create(struct qxl_fbdev *qfbdev,
>>   			struct drm_fb_helper_surface_size *sizes)
>>   {
>> @@ -383,7 +271,8 @@ static int qxlfb_create(struct qxl_fbdev *qfbdev,
>>   
>>   	info->par = qfbdev;
>>   
>> -	qxl_framebuffer_init(qdev->ddev, &qfbdev->qfb, &mode_cmd, gobj);
>> +	qxl_framebuffer_init(qdev->ddev, &qfbdev->qfb, &mode_cmd, gobj,
>> +			     &qxlfb_fb_funcs);
>>   
>>   	fb = &qfbdev->qfb.base;
>>   
>> @@ -504,7 +393,6 @@ int qxl_fbdev_init(struct qxl_device *qdev)
>>   	qfbdev->qdev = qdev;
>>   	qdev->mode_info.qfbdev = qfbdev;
>>   	spin_lock_init(&qfbdev->delayed_ops_lock);
>> -	spin_lock_init(&qfbdev->dirty.lock);
>>   	INIT_LIST_HEAD(&qfbdev->delayed_ops);
>>   
>>   	drm_fb_helper_prepare(qdev->ddev, &qfbdev->helper,
>> diff --git a/drivers/gpu/drm/qxl/qxl_kms.c b/drivers/gpu/drm/qxl/qxl_kms.c
>> index b2977a1..2319800 100644
>> --- a/drivers/gpu/drm/qxl/qxl_kms.c
>> +++ b/drivers/gpu/drm/qxl/qxl_kms.c
>> @@ -261,10 +261,6 @@ static int qxl_device_init(struct qxl_device *qdev,
>>   	qdev->gc_queue = create_singlethread_workqueue("qxl_gc");
>>   	INIT_WORK(&qdev->gc_work, qxl_gc_work);
>>   
>> -	r = qxl_fb_init(qdev);
>> -	if (r)
>> -		return r;
>> -
>>   	return 0;
>>   }
>>   
>> -- 
>> 2.2.2
>>

[toc] | [prev] | [next] | [standalone]

#1383914 — Re: [PATCH 7/8] drm/qxl: Use drm_fb_helper deferred_io support

On Wed, Apr 20, 2016 at 09:04:38PM +0200, Noralf Trønnes wrote:
> 
> Den 20.04.2016 19:47, skrev Daniel Vetter:
> >On Wed, Apr 20, 2016 at 05:25:28PM +0200, Noralf Trønnes wrote:
> >>Use the fbdev deferred io support in drm_fb_helper.
> >>The (struct fb_ops *)->fb_{fillrect,copyarea,imageblit} functions will
> >>now be deferred in the same way that mmap damage is, instead of being
> >>flushed directly.
> >>This patch has only been compile tested.
> >>
> >>Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> >>---
> >>  drivers/gpu/drm/qxl/qxl_display.c |   9 +-
> >>  drivers/gpu/drm/qxl/qxl_drv.h     |   7 +-
> >>  drivers/gpu/drm/qxl/qxl_fb.c      | 220 ++++++++++----------------------------
> >>  drivers/gpu/drm/qxl/qxl_kms.c     |   4 -
> >>  4 files changed, 62 insertions(+), 178 deletions(-)
> >>
> >>diff --git a/drivers/gpu/drm/qxl/qxl_display.c b/drivers/gpu/drm/qxl/qxl_display.c
> >>index 030409a..9a03524 100644
> >>--- a/drivers/gpu/drm/qxl/qxl_display.c
> >>+++ b/drivers/gpu/drm/qxl/qxl_display.c
> >>@@ -465,7 +465,7 @@ static const struct drm_crtc_funcs qxl_crtc_funcs = {
> >>  	.page_flip = qxl_crtc_page_flip,
> >>  };
> >>-static void qxl_user_framebuffer_destroy(struct drm_framebuffer *fb)
> >>+void qxl_user_framebuffer_destroy(struct drm_framebuffer *fb)
> >>  {
> >>  	struct qxl_framebuffer *qxl_fb = to_qxl_framebuffer(fb);
> >>@@ -527,12 +527,13 @@ 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)
> >>+		     struct drm_gem_object *obj,
> >>+		     const struct drm_framebuffer_funcs *funcs)
> >There should be no need at all to have a separate fb funcs table for the
> >fbdev fb. Both /should/ be able to use the exact same (already existing)
> >->dirty() callback. We need this only in CMA because CMA is a midlayer
> >used by multiple drivers.
> 
> I don't see how I can avoid it.
> 
> fbdev framebuffer flushing:
> 
> static void qxl_fb_dirty_flush(struct fb_info *info)
> {
>         qxl_fb_image_init(&qxl_fb_image, qdev, info, NULL);
>         qxl_draw_opaque_fb(&qxl_fb_image, stride);
> }
> 
> drm framebuffer flushing:
> 
> static int qxl_framebuffer_surface_dirty(...)
> {
>         qxl_draw_dirty_fb(...);
> }
> 
> qxl_draw_opaque_fb() and qxl_draw_dirty_fb() differ so much that it's way
> over my head to see if they can be combined.
> Here's an online diff of the two functions:
> https://www.diffchecker.com/jqbbalux

Imo nuke the fbdev one entirely. If it breaks then it's either a bug in
your generic fbdefio code, or the qxl ->dirty implementation has a bug. It
should work ;-)

Ok, slightly more seriously the difference seems to be that the fbdev one
support paletted mode too. But since qxl has 0 pixel format checking
anywhere I have no idea whether that's dead code (i.e. broken) or actually
working. I guess keeping the split is ok, if we add a big FIXME comment to
it that this is very fishy.
-Daniel


> 
> 
> >
> >With that change you should be able to condense this patch down to pretty
> >much just removing lines. Which is Good (tm).
> >
> >Cheers, Daniel
> >
> >>  {
> >>  	int ret;
> >>  	qfb->obj = obj;
> >>-	ret = drm_framebuffer_init(dev, &qfb->base, &qxl_fb_funcs);
> >>+	ret = drm_framebuffer_init(dev, &qfb->base, funcs);
> >>  	if (ret) {
> >>  		qfb->obj = NULL;
> >>  		return ret;
> >>@@ -999,7 +1000,7 @@ qxl_user_framebuffer_create(struct drm_device *dev,
> >>  	if (qxl_fb == NULL)
> >>  		return NULL;
> >>-	ret = qxl_framebuffer_init(dev, qxl_fb, mode_cmd, obj);
> >>+	ret = qxl_framebuffer_init(dev, qxl_fb, mode_cmd, obj, &qxl_fb_funcs);
> >>  	if (ret) {
> >>  		kfree(qxl_fb);
> >>  		drm_gem_object_unreference_unlocked(obj);
> >>diff --git a/drivers/gpu/drm/qxl/qxl_drv.h b/drivers/gpu/drm/qxl/qxl_drv.h
> >>index 3f3897e..3ad6604 100644
> >>--- a/drivers/gpu/drm/qxl/qxl_drv.h
> >>+++ b/drivers/gpu/drm/qxl/qxl_drv.h
> >>@@ -324,8 +324,6 @@ struct qxl_device {
> >>  	struct workqueue_struct *gc_queue;
> >>  	struct work_struct gc_work;
> >>-	struct work_struct fb_work;
> >>-
> >>  	struct drm_property *hotplug_mode_update_property;
> >>  	int monitors_config_width;
> >>  	int monitors_config_height;
> >>@@ -389,11 +387,13 @@ int qxl_get_handle_for_primary_fb(struct qxl_device *qdev,
> >>  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);
> >>+		     struct drm_gem_object *obj,
> >>+		     const struct drm_framebuffer_funcs *funcs);
> >>  void qxl_display_read_client_monitors_config(struct qxl_device *qdev);
> >>  void qxl_send_monitors_config(struct qxl_device *qdev);
> >>  int qxl_create_monitors_object(struct qxl_device *qdev);
> >>@@ -553,7 +553,6 @@ int qxl_irq_init(struct qxl_device *qdev);
> >>  irqreturn_t qxl_irq_handler(int irq, void *arg);
> >>  /* qxl_fb.c */
> >>-int qxl_fb_init(struct qxl_device *qdev);
> >>  bool qxl_fbdev_qobj_is_fb(struct qxl_device *qdev, struct qxl_bo *qobj);
> >>  int qxl_debugfs_add_files(struct qxl_device *qdev,
> >>diff --git a/drivers/gpu/drm/qxl/qxl_fb.c b/drivers/gpu/drm/qxl/qxl_fb.c
> >>index 06f032d..090dcee 100644
> >>--- a/drivers/gpu/drm/qxl/qxl_fb.c
> >>+++ b/drivers/gpu/drm/qxl/qxl_fb.c
> >>@@ -30,6 +30,7 @@
> >>  #include "drm/drm.h"
> >>  #include "drm/drm_crtc.h"
> >>  #include "drm/drm_crtc_helper.h"
> >>+#include "drm/drm_rect.h"
> >>  #include "qxl_drv.h"
> >>  #include "qxl_object.h"
> >>@@ -46,15 +47,6 @@ struct qxl_fbdev {
> >>  	struct list_head delayed_ops;
> >>  	void *shadow;
> >>  	int size;
> >>-
> >>-	/* dirty memory logging */
> >>-	struct {
> >>-		spinlock_t lock;
> >>-		unsigned x1;
> >>-		unsigned y1;
> >>-		unsigned x2;
> >>-		unsigned y2;
> >>-	} dirty;
> >>  };
> >>  static void qxl_fb_image_init(struct qxl_fb_image *qxl_fb_image,
> >>@@ -82,169 +74,18 @@ static void qxl_fb_image_init(struct qxl_fb_image *qxl_fb_image,
> >>  	}
> >>  }
> >>-static void qxl_fb_dirty_flush(struct fb_info *info)
> >>-{
> >>-	struct qxl_fbdev *qfbdev = info->par;
> >>-	struct qxl_device *qdev = qfbdev->qdev;
> >>-	struct qxl_fb_image qxl_fb_image;
> >>-	struct fb_image *image = &qxl_fb_image.fb_image;
> >>-	unsigned long flags;
> >>-	u32 x1, x2, y1, y2;
> >>-
> >>-	/* TODO: hard coding 32 bpp */
> >>-	int stride = qfbdev->qfb.base.pitches[0];
> >>-
> >>-	spin_lock_irqsave(&qfbdev->dirty.lock, flags);
> >>-
> >>-	x1 = qfbdev->dirty.x1;
> >>-	x2 = qfbdev->dirty.x2;
> >>-	y1 = qfbdev->dirty.y1;
> >>-	y2 = qfbdev->dirty.y2;
> >>-	qfbdev->dirty.x1 = 0;
> >>-	qfbdev->dirty.x2 = 0;
> >>-	qfbdev->dirty.y1 = 0;
> >>-	qfbdev->dirty.y2 = 0;
> >>-
> >>-	spin_unlock_irqrestore(&qfbdev->dirty.lock, flags);
> >>-
> >>-	/*
> >>-	 * we are using a shadow draw buffer, at qdev->surface0_shadow
> >>-	 */
> >>-	qxl_io_log(qdev, "dirty x[%d, %d], y[%d, %d]", x1, x2, y1, y2);
> >>-	image->dx = x1;
> >>-	image->dy = y1;
> >>-	image->width = x2 - x1 + 1;
> >>-	image->height = y2 - y1 + 1;
> >>-	image->fg_color = 0xffffffff; /* unused, just to avoid uninitialized
> >>-					 warnings */
> >>-	image->bg_color = 0;
> >>-	image->depth = 32;	     /* TODO: take from somewhere? */
> >>-	image->cmap.start = 0;
> >>-	image->cmap.len = 0;
> >>-	image->cmap.red = NULL;
> >>-	image->cmap.green = NULL;
> >>-	image->cmap.blue = NULL;
> >>-	image->cmap.transp = NULL;
> >>-	image->data = qfbdev->shadow + (x1 * 4) + (stride * y1);
> >>-
> >>-	qxl_fb_image_init(&qxl_fb_image, qdev, info, NULL);
> >>-	qxl_draw_opaque_fb(&qxl_fb_image, stride);
> >>-}
> >>-
> >>-static void qxl_dirty_update(struct qxl_fbdev *qfbdev,
> >>-			     int x, int y, int width, int height)
> >>-{
> >>-	struct qxl_device *qdev = qfbdev->qdev;
> >>-	unsigned long flags;
> >>-	int x2, y2;
> >>-
> >>-	x2 = x + width - 1;
> >>-	y2 = y + height - 1;
> >>-
> >>-	spin_lock_irqsave(&qfbdev->dirty.lock, flags);
> >>-
> >>-	if ((qfbdev->dirty.y2 - qfbdev->dirty.y1) &&
> >>-	    (qfbdev->dirty.x2 - qfbdev->dirty.x1)) {
> >>-		if (qfbdev->dirty.y1 < y)
> >>-			y = qfbdev->dirty.y1;
> >>-		if (qfbdev->dirty.y2 > y2)
> >>-			y2 = qfbdev->dirty.y2;
> >>-		if (qfbdev->dirty.x1 < x)
> >>-			x = qfbdev->dirty.x1;
> >>-		if (qfbdev->dirty.x2 > x2)
> >>-			x2 = qfbdev->dirty.x2;
> >>-	}
> >>-
> >>-	qfbdev->dirty.x1 = x;
> >>-	qfbdev->dirty.x2 = x2;
> >>-	qfbdev->dirty.y1 = y;
> >>-	qfbdev->dirty.y2 = y2;
> >>-
> >>-	spin_unlock_irqrestore(&qfbdev->dirty.lock, flags);
> >>-
> >>-	schedule_work(&qdev->fb_work);
> >>-}
> >>-
> >>-static void qxl_deferred_io(struct fb_info *info,
> >>-			    struct list_head *pagelist)
> >>-{
> >>-	struct qxl_fbdev *qfbdev = info->par;
> >>-	unsigned long start, end, min, max;
> >>-	struct page *page;
> >>-	int y1, y2;
> >>-
> >>-	min = ULONG_MAX;
> >>-	max = 0;
> >>-	list_for_each_entry(page, pagelist, lru) {
> >>-		start = page->index << PAGE_SHIFT;
> >>-		end = start + PAGE_SIZE - 1;
> >>-		min = min(min, start);
> >>-		max = max(max, end);
> >>-	}
> >>-
> >>-	if (min < max) {
> >>-		y1 = min / info->fix.line_length;
> >>-		y2 = (max / info->fix.line_length) + 1;
> >>-		qxl_dirty_update(qfbdev, 0, y1, info->var.xres, y2 - y1);
> >>-	}
> >>-};
> >>-
> >>  static struct fb_deferred_io qxl_defio = {
> >>  	.delay		= QXL_DIRTY_DELAY,
> >>-	.deferred_io	= qxl_deferred_io,
> >>+	.deferred_io	= drm_fb_helper_deferred_io,
> >>  };
> >>-static void qxl_fb_fillrect(struct fb_info *info,
> >>-			    const struct fb_fillrect *rect)
> >>-{
> >>-	struct qxl_fbdev *qfbdev = info->par;
> >>-
> >>-	sys_fillrect(info, rect);
> >>-	qxl_dirty_update(qfbdev, rect->dx, rect->dy, rect->width,
> >>-			 rect->height);
> >>-}
> >>-
> >>-static void qxl_fb_copyarea(struct fb_info *info,
> >>-			    const struct fb_copyarea *area)
> >>-{
> >>-	struct qxl_fbdev *qfbdev = info->par;
> >>-
> >>-	sys_copyarea(info, area);
> >>-	qxl_dirty_update(qfbdev, area->dx, area->dy, area->width,
> >>-			 area->height);
> >>-}
> >>-
> >>-static void qxl_fb_imageblit(struct fb_info *info,
> >>-			     const struct fb_image *image)
> >>-{
> >>-	struct qxl_fbdev *qfbdev = info->par;
> >>-
> >>-	sys_imageblit(info, image);
> >>-	qxl_dirty_update(qfbdev, image->dx, image->dy, image->width,
> >>-			 image->height);
> >>-}
> >>-
> >>-static void qxl_fb_work(struct work_struct *work)
> >>-{
> >>-	struct qxl_device *qdev = container_of(work, struct qxl_device, fb_work);
> >>-	struct qxl_fbdev *qfbdev = qdev->mode_info.qfbdev;
> >>-
> >>-	qxl_fb_dirty_flush(qfbdev->helper.fbdev);
> >>-}
> >>-
> >>-int qxl_fb_init(struct qxl_device *qdev)
> >>-{
> >>-	INIT_WORK(&qdev->fb_work, qxl_fb_work);
> >>-	return 0;
> >>-}
> >>-
> >>  static struct fb_ops qxlfb_ops = {
> >>  	.owner = THIS_MODULE,
> >>  	.fb_check_var = drm_fb_helper_check_var,
> >>  	.fb_set_par = drm_fb_helper_set_par, /* TODO: copy vmwgfx */
> >>-	.fb_fillrect = qxl_fb_fillrect,
> >>-	.fb_copyarea = qxl_fb_copyarea,
> >>-	.fb_imageblit = qxl_fb_imageblit,
> >>+	.fb_fillrect = drm_fb_helper_sys_fillrect,
> >>+	.fb_copyarea = drm_fb_helper_sys_copyarea,
> >>+	.fb_imageblit = drm_fb_helper_sys_imageblit,
> >>  	.fb_pan_display = drm_fb_helper_pan_display,
> >>  	.fb_blank = drm_fb_helper_blank,
> >>  	.fb_setcmap = drm_fb_helper_setcmap,
> >>@@ -338,6 +179,53 @@ out_unref:
> >>  	return ret;
> >>  }
> >>+static int qxlfb_framebuffer_dirty(struct drm_framebuffer *fb,
> >>+				   struct drm_file *file_priv,
> >>+				   unsigned flags, unsigned color,
> >>+				   struct drm_clip_rect *clips,
> >>+				   unsigned num_clips)
> >>+{
> >>+	struct qxl_device *qdev = fb->dev->dev_private;
> >>+	struct fb_info *info = qdev->fbdev_info;
> >>+	struct qxl_fbdev *qfbdev = info->par;
> >>+	struct qxl_fb_image qxl_fb_image;
> >>+	struct fb_image *image = &qxl_fb_image.fb_image;
> >>+
> >>+	/* TODO: hard coding 32 bpp */
> >>+	int stride = qfbdev->qfb.base.pitches[0];
> >>+
> >>+	/*
> >>+	 * we are using a shadow draw buffer, at qdev->surface0_shadow
> >>+	 */
> >>+	qxl_io_log(qdev, "dirty x[%d, %d], y[%d, %d]", clips->x1, clips->x2,
> >>+		   clips->y1, clips->y2);
> >>+	image->dx = clips->x1;
> >>+	image->dy = clips->y1;
> >>+	image->width = drm_clip_rect_width(clips);
> >>+	image->height = drm_clip_rect_height(clips);
> >>+	image->fg_color = 0xffffffff; /* unused, just to avoid uninitialized
> >>+					 warnings */
> >>+	image->bg_color = 0;
> >>+	image->depth = 32;	     /* TODO: take from somewhere? */
> >>+	image->cmap.start = 0;
> >>+	image->cmap.len = 0;
> >>+	image->cmap.red = NULL;
> >>+	image->cmap.green = NULL;
> >>+	image->cmap.blue = NULL;
> >>+	image->cmap.transp = NULL;
> >>+	image->data = qfbdev->shadow + (clips->x1 * 4) + (stride * clips->y1);
> >>+
> >>+	qxl_fb_image_init(&qxl_fb_image, qdev, info, NULL);
> >>+	qxl_draw_opaque_fb(&qxl_fb_image, stride);
> >>+
> >>+	return 0;
> >>+}
> >>+
> >>+static const struct drm_framebuffer_funcs qxlfb_fb_funcs = {
> >>+	.destroy = qxl_user_framebuffer_destroy,
> >>+	.dirty = qxlfb_framebuffer_dirty,
> >>+};
> >>+
> >>  static int qxlfb_create(struct qxl_fbdev *qfbdev,
> >>  			struct drm_fb_helper_surface_size *sizes)
> >>  {
> >>@@ -383,7 +271,8 @@ static int qxlfb_create(struct qxl_fbdev *qfbdev,
> >>  	info->par = qfbdev;
> >>-	qxl_framebuffer_init(qdev->ddev, &qfbdev->qfb, &mode_cmd, gobj);
> >>+	qxl_framebuffer_init(qdev->ddev, &qfbdev->qfb, &mode_cmd, gobj,
> >>+			     &qxlfb_fb_funcs);
> >>  	fb = &qfbdev->qfb.base;
> >>@@ -504,7 +393,6 @@ int qxl_fbdev_init(struct qxl_device *qdev)
> >>  	qfbdev->qdev = qdev;
> >>  	qdev->mode_info.qfbdev = qfbdev;
> >>  	spin_lock_init(&qfbdev->delayed_ops_lock);
> >>-	spin_lock_init(&qfbdev->dirty.lock);
> >>  	INIT_LIST_HEAD(&qfbdev->delayed_ops);
> >>  	drm_fb_helper_prepare(qdev->ddev, &qfbdev->helper,
> >>diff --git a/drivers/gpu/drm/qxl/qxl_kms.c b/drivers/gpu/drm/qxl/qxl_kms.c
> >>index b2977a1..2319800 100644
> >>--- a/drivers/gpu/drm/qxl/qxl_kms.c
> >>+++ b/drivers/gpu/drm/qxl/qxl_kms.c
> >>@@ -261,10 +261,6 @@ static int qxl_device_init(struct qxl_device *qdev,
> >>  	qdev->gc_queue = create_singlethread_workqueue("qxl_gc");
> >>  	INIT_WORK(&qdev->gc_work, qxl_gc_work);
> >>-	r = qxl_fb_init(qdev);
> >>-	if (r)
> >>-		return r;
> >>-
> >>  	return 0;
> >>  }
> >>-- 
> >>2.2.2
> >>
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[toc] | [prev] | [next] | [standalone]

#1383915 — Re: [PATCH 7/8] drm/qxl: Use drm_fb_helper deferred_io support

On Thu, Apr 21, 2016 at 09:41:34AM +0200, Daniel Vetter wrote:
> On Wed, Apr 20, 2016 at 09:04:38PM +0200, Noralf Trønnes wrote:
> > 
> > Den 20.04.2016 19:47, skrev Daniel Vetter:
> > >On Wed, Apr 20, 2016 at 05:25:28PM +0200, Noralf Trønnes wrote:
> > >>Use the fbdev deferred io support in drm_fb_helper.
> > >>The (struct fb_ops *)->fb_{fillrect,copyarea,imageblit} functions will
> > >>now be deferred in the same way that mmap damage is, instead of being
> > >>flushed directly.
> > >>This patch has only been compile tested.
> > >>
> > >>Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> > >>---
> > >>  drivers/gpu/drm/qxl/qxl_display.c |   9 +-
> > >>  drivers/gpu/drm/qxl/qxl_drv.h     |   7 +-
> > >>  drivers/gpu/drm/qxl/qxl_fb.c      | 220 ++++++++++----------------------------
> > >>  drivers/gpu/drm/qxl/qxl_kms.c     |   4 -
> > >>  4 files changed, 62 insertions(+), 178 deletions(-)
> > >>
> > >>diff --git a/drivers/gpu/drm/qxl/qxl_display.c b/drivers/gpu/drm/qxl/qxl_display.c
> > >>index 030409a..9a03524 100644
> > >>--- a/drivers/gpu/drm/qxl/qxl_display.c
> > >>+++ b/drivers/gpu/drm/qxl/qxl_display.c
> > >>@@ -465,7 +465,7 @@ static const struct drm_crtc_funcs qxl_crtc_funcs = {
> > >>  	.page_flip = qxl_crtc_page_flip,
> > >>  };
> > >>-static void qxl_user_framebuffer_destroy(struct drm_framebuffer *fb)
> > >>+void qxl_user_framebuffer_destroy(struct drm_framebuffer *fb)
> > >>  {
> > >>  	struct qxl_framebuffer *qxl_fb = to_qxl_framebuffer(fb);
> > >>@@ -527,12 +527,13 @@ 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)
> > >>+		     struct drm_gem_object *obj,
> > >>+		     const struct drm_framebuffer_funcs *funcs)
> > >There should be no need at all to have a separate fb funcs table for the
> > >fbdev fb. Both /should/ be able to use the exact same (already existing)
> > >->dirty() callback. We need this only in CMA because CMA is a midlayer
> > >used by multiple drivers.
> > 
> > I don't see how I can avoid it.
> > 
> > fbdev framebuffer flushing:
> > 
> > static void qxl_fb_dirty_flush(struct fb_info *info)
> > {
> >         qxl_fb_image_init(&qxl_fb_image, qdev, info, NULL);
> >         qxl_draw_opaque_fb(&qxl_fb_image, stride);
> > }
> > 
> > drm framebuffer flushing:
> > 
> > static int qxl_framebuffer_surface_dirty(...)
> > {
> >         qxl_draw_dirty_fb(...);
> > }
> > 
> > qxl_draw_opaque_fb() and qxl_draw_dirty_fb() differ so much that it's way
> > over my head to see if they can be combined.
> > Here's an online diff of the two functions:
> > https://www.diffchecker.com/jqbbalux
> 
> Imo nuke the fbdev one entirely. If it breaks then it's either a bug in
> your generic fbdefio code, or the qxl ->dirty implementation has a bug. It
> should work ;-)
> 
> Ok, slightly more seriously the difference seems to be that the fbdev one
> support paletted mode too. But since qxl has 0 pixel format checking
> anywhere I have no idea whether that's dead code (i.e. broken) or actually
> working. I guess keeping the split is ok, if we add a big FIXME comment to
> it that this is very fishy.

Ok, I read around a bit more. The only things qxl seems to support are
bits_per_pixel of 1, 24 and 32 (see qxl_image_init_helper). And drm has no
way to pass in 1 bpp images. And it doesn't support 8 bit paletted, which
is the only paletted thing drm supports.

So if you totally feel like I think we could add format checking for
DRM_FORMAT_XRGB8888 and DRM_FORMAT_RGB888 in qxl_framebuffer_init and then
rip out all that code. But that's a few more patches and probably should
be tested actually ;-)

FIXME plus explaing it all in the commit message is fine with me too.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[toc] | [prev] | [next] | [standalone]

#1383917 — Re: [PATCH 7/8] drm/qxl: Use drm_fb_helper deferred_io support

On Thu, Apr 21, 2016 at 09:49:39AM +0200, Daniel Vetter wrote:
> On Thu, Apr 21, 2016 at 09:41:34AM +0200, Daniel Vetter wrote:
> > On Wed, Apr 20, 2016 at 09:04:38PM +0200, Noralf Trønnes wrote:
> > > 
> > > Den 20.04.2016 19:47, skrev Daniel Vetter:
> > > >On Wed, Apr 20, 2016 at 05:25:28PM +0200, Noralf Trønnes wrote:
> > > >>Use the fbdev deferred io support in drm_fb_helper.
> > > >>The (struct fb_ops *)->fb_{fillrect,copyarea,imageblit} functions will
> > > >>now be deferred in the same way that mmap damage is, instead of being
> > > >>flushed directly.
> > > >>This patch has only been compile tested.
> > > >>
> > > >>Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> > > >>---
> > > >>  drivers/gpu/drm/qxl/qxl_display.c |   9 +-
> > > >>  drivers/gpu/drm/qxl/qxl_drv.h     |   7 +-
> > > >>  drivers/gpu/drm/qxl/qxl_fb.c      | 220 ++++++++++----------------------------
> > > >>  drivers/gpu/drm/qxl/qxl_kms.c     |   4 -
> > > >>  4 files changed, 62 insertions(+), 178 deletions(-)
> > > >>
> > > >>diff --git a/drivers/gpu/drm/qxl/qxl_display.c b/drivers/gpu/drm/qxl/qxl_display.c
> > > >>index 030409a..9a03524 100644
> > > >>--- a/drivers/gpu/drm/qxl/qxl_display.c
> > > >>+++ b/drivers/gpu/drm/qxl/qxl_display.c
> > > >>@@ -465,7 +465,7 @@ static const struct drm_crtc_funcs qxl_crtc_funcs = {
> > > >>  	.page_flip = qxl_crtc_page_flip,
> > > >>  };
> > > >>-static void qxl_user_framebuffer_destroy(struct drm_framebuffer *fb)
> > > >>+void qxl_user_framebuffer_destroy(struct drm_framebuffer *fb)
> > > >>  {
> > > >>  	struct qxl_framebuffer *qxl_fb = to_qxl_framebuffer(fb);
> > > >>@@ -527,12 +527,13 @@ 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)
> > > >>+		     struct drm_gem_object *obj,
> > > >>+		     const struct drm_framebuffer_funcs *funcs)
> > > >There should be no need at all to have a separate fb funcs table for the
> > > >fbdev fb. Both /should/ be able to use the exact same (already existing)
> > > >->dirty() callback. We need this only in CMA because CMA is a midlayer
> > > >used by multiple drivers.
> > > 
> > > I don't see how I can avoid it.
> > > 
> > > fbdev framebuffer flushing:
> > > 
> > > static void qxl_fb_dirty_flush(struct fb_info *info)
> > > {
> > >         qxl_fb_image_init(&qxl_fb_image, qdev, info, NULL);
> > >         qxl_draw_opaque_fb(&qxl_fb_image, stride);
> > > }
> > > 
> > > drm framebuffer flushing:
> > > 
> > > static int qxl_framebuffer_surface_dirty(...)
> > > {
> > >         qxl_draw_dirty_fb(...);
> > > }
> > > 
> > > qxl_draw_opaque_fb() and qxl_draw_dirty_fb() differ so much that it's way
> > > over my head to see if they can be combined.
> > > Here's an online diff of the two functions:
> > > https://www.diffchecker.com/jqbbalux
> > 
> > Imo nuke the fbdev one entirely. If it breaks then it's either a bug in
> > your generic fbdefio code, or the qxl ->dirty implementation has a bug. It
> > should work ;-)
> > 
> > Ok, slightly more seriously the difference seems to be that the fbdev one
> > support paletted mode too. But since qxl has 0 pixel format checking
> > anywhere I have no idea whether that's dead code (i.e. broken) or actually
> > working. I guess keeping the split is ok, if we add a big FIXME comment to
> > it that this is very fishy.
> 
> Ok, I read around a bit more. The only things qxl seems to support are
> bits_per_pixel of 1, 24 and 32 (see qxl_image_init_helper). And drm has no
> way to pass in 1 bpp images. And it doesn't support 8 bit paletted, which
> is the only paletted thing drm supports.
> 
> So if you totally feel like I think we could add format checking for
> DRM_FORMAT_XRGB8888 and DRM_FORMAT_RGB888 in qxl_framebuffer_init and then
> rip out all that code. But that's a few more patches and probably should
> be tested actually ;-)

Even simpler: Check for bits_per_pixel == 24 || 32, since that matches the
only other check in qxl. Extremely unlikely qxl supports all these
formats, but meh ...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web